mus: Fix ash add/remove display accelerator.

This CL fixes the ash add/remove display accelerators when running with
--mus. When the accelerator is pressed it needs to call
TestDisplayController::AddRemoveDisplay(). The TestDisplayControllerPtr
is moved into DisplayManager to simplify things. This allows removing
the special case for the DEV_ADD_REMOVE_DISPLAY accelerator.

Both --mus and --mash will set TestDisplayControllerPtr on startup if
they are running on Linux desktop. It would be possible to do something
similar with classic ash in the future.

Bug: 779845
Change-Id: I70b058777944217e55ef105b9d1ac65f05e28b3a
Reviewed-on: https://chromium-review.googlesource.com/746375Reviewed-by: Scott Violet <sky@chromium.org>
Reviewed-by: Tom Sepez <tsepez@chromium.org>
Commit-Queue: kylechar <kylechar@chromium.org>
Cr-Commit-Position: refs/heads/master@{#512996}

ash/accelerators/accelerator_controller.cc

@@ -1049,6 +1049,7 @@ bool AcceleratorController::CanPerformAction(
     case DEBUG_TOGGLE_WALLPAPER_MODE:
     case DEBUG_TRIGGER_CRASH:
       return debug::DebugAcceleratorsEnabled();
+    case DEV_ADD_REMOVE_DISPLAY:
     case DEV_TOGGLE_UNIFIED_DESKTOP:
       return debug::DeveloperAcceleratorsEnabled();
     case DISABLE_CAPS_LOCK:
@@ -1191,6 +1192,9 @@ void AcceleratorController::PerformAction(AcceleratorAction action,
     case DEBUG_TRIGGER_CRASH:
       debug::PerformDebugActionIfEnabled(action);
       break;
+    case DEV_ADD_REMOVE_DISPLAY:
+      Shell::Get()->display_manager()->AddRemoveDisplay();
+      break;
     case DEV_TOGGLE_UNIFIED_DESKTOP:
       HandleToggleUnifiedDesktop();
       break;

ash/accelerators/accelerator_controller_delegate_classic.cc

@@ -123,7 +123,6 @@ bool AcceleratorControllerDelegateClassic::HandlesAction(
     case DEBUG_TOGGLE_SHOW_DEBUG_BORDERS:
     case DEBUG_TOGGLE_SHOW_FPS_COUNTER:
     case DEBUG_TOGGLE_SHOW_PAINT_RECTS:
-    case DEV_ADD_REMOVE_DISPLAY:
     case LOCK_PRESSED:
     case LOCK_RELEASED:
     case MAGNIFY_SCREEN_ZOOM_IN:
@@ -155,8 +154,6 @@ bool AcceleratorControllerDelegateClassic::CanPerformAction(
     case DEBUG_TOGGLE_SHOW_FPS_COUNTER:
     case DEBUG_TOGGLE_SHOW_PAINT_RECTS:
       return debug::DebugAcceleratorsEnabled();
-    case DEV_ADD_REMOVE_DISPLAY:
-      return debug::DeveloperAcceleratorsEnabled();
     case MAGNIFY_SCREEN_ZOOM_IN:
     case MAGNIFY_SCREEN_ZOOM_OUT:
       return CanHandleMagnifyScreen();
@@ -198,9 +195,6 @@ void AcceleratorControllerDelegateClassic::PerformAction(
     case DEBUG_TOGGLE_SHOW_PAINT_RECTS:
       debug::ToggleShowPaintRects();
       break;
-    case DEV_ADD_REMOVE_DISPLAY:
-      Shell::Get()->display_manager()->AddRemoveDisplay();
-      break;
     case LOCK_PRESSED:
     case LOCK_RELEASED:
       Shell::Get()->power_button_controller()->OnLockButtonEvent(

ash/mus/accelerators/accelerator_controller_delegate_mus.cc

@@ -27,7 +27,6 @@ bool AcceleratorControllerDelegateMus::HandlesAction(AcceleratorAction action) {
   // should be moved to accelerator_controller.cc/h. See
   // http://crbug.com/612331.
   switch (action) {
-    case DEV_ADD_REMOVE_DISPLAY:
     case TOUCH_HUD_PROJECTION_TOGGLE:
       return true;
     case DEBUG_TOGGLE_DEVICE_SCALE_FACTOR:
@@ -59,7 +58,6 @@ bool AcceleratorControllerDelegateMus::CanPerformAction(
     const ui::Accelerator& accelerator,
     const ui::Accelerator& previous_accelerator) {
   switch (action) {
-    case DEV_ADD_REMOVE_DISPLAY:
     case TOUCH_HUD_PROJECTION_TOGGLE:
       return true;
     default:
@@ -72,14 +70,6 @@ void AcceleratorControllerDelegateMus::PerformAction(
     AcceleratorAction action,
     const ui::Accelerator& accelerator) {
   switch (action) {
-    case DEV_ADD_REMOVE_DISPLAY: {
-      if (!test_display_controller_) {
-        window_manager_->connector()->BindInterface(ui::mojom::kServiceName,
-                                                    &test_display_controller_);
-      }
-      test_display_controller_->ToggleAddRemoveDisplay();
-      break;
-    }
     case TOUCH_HUD_PROJECTION_TOGGLE: {
       mash::mojom::LaunchablePtr launchable;
       window_manager_->connector()->BindInterface("touch_hud", &launchable);

ash/mus/accelerators/accelerator_controller_delegate_mus.h

@@ -7,7 +7,6 @@
 
 #include "ash/accelerators/accelerator_controller_delegate.h"
 #include "base/macros.h"
-#include "services/ui/public/interfaces/display/test_display_controller.mojom.h"
 
 namespace ash {
 namespace mus {
@@ -31,8 +30,6 @@ class AcceleratorControllerDelegateMus : public AcceleratorControllerDelegate {
  private:
   WindowManager* window_manager_;
 
-  display::mojom::TestDisplayControllerPtr test_display_controller_;
-
   DISALLOW_COPY_AND_ASSIGN(AcceleratorControllerDelegateMus);
 };
 

ash/mus/manifest.json

@@ -43,7 +43,7 @@
         "*": [ "accessibility", "app" ],
         "ash_pref_connector": [ "pref_connector" ],
         "local_state": [ "pref_client" ],
-        "ui": [ "display_test", "window_manager" ],
+        "ui": [ "display_dev", "window_manager" ],
         "touch_hud": [ "mash:launchable" ]
       }
     }

ash/mus/standalone/manifest.json

@@ -27,7 +27,7 @@
       "requires": {
         "*": [ "accessibility", "app" ],
         "preferences_forwarder": [ "pref_client" ],
-        "ui": [ "display_test", "window_manager" ],
+        "ui": [ "display_dev", "window_manager" ],
         "touch_hud": [ "mash:launchable" ]
       }
     }

ash/shell.cc

@@ -167,6 +167,7 @@
 #include "ui/display/manager/chromeos/display_configurator.h"
 #include "ui/display/manager/chromeos/touch_transform_setter.h"
 #include "ui/display/manager/display_manager.h"
+#include "ui/display/mojo/dev_display_controller.mojom.h"
 #include "ui/display/screen.h"
 #include "ui/display/types/native_display_delegate.h"
 #include "ui/events/event_target_iterator.h"
@@ -928,24 +929,35 @@ void Shell::Init(const ShellInitParams& init_params) {
   display_configurator_->AddObserver(projecting_observer_.get());
   AddShellObserver(projecting_observer_.get());
 
-  if (!display_initialized &&
-      (config != Config::CLASSIC || chromeos::IsRunningAsSystemCompositor())) {
-    display_change_observer_ = std::make_unique<display::DisplayChangeObserver>(
-        display_configurator_.get(), display_manager_.get());
-
-    shutdown_observer_ =
-        std::make_unique<ShutdownObserver>(display_configurator_.get());
-
-    // Register |display_change_observer_| first so that the rest of
-    // observer gets invoked after the root windows are configured.
-    display_configurator_->AddObserver(display_change_observer_.get());
-    display_error_observer_.reset(new DisplayErrorObserver());
-    display_configurator_->AddObserver(display_error_observer_.get());
-    display_configurator_->set_state_controller(display_change_observer_.get());
-    display_configurator_->set_mirroring_controller(display_manager_.get());
-    display_configurator_->ForceInitialConfigure();
-    display_initialized = true;
+  if (!display_initialized) {
+    if (config != Config::CLASSIC && !chromeos::IsRunningAsSystemCompositor()) {
+      display::mojom::DevDisplayControllerPtr controller;
+      shell_delegate_->GetShellConnector()->BindInterface(
+          ui::mojom::kServiceName, &controller);
+      display_manager_->SetDevDisplayController(std::move(controller));
+    }
+
+    if (config != Config::CLASSIC || chromeos::IsRunningAsSystemCompositor()) {
+      display_change_observer_ =
+          std::make_unique<display::DisplayChangeObserver>(
+              display_configurator_.get(), display_manager_.get());
+
+      shutdown_observer_ =
+          std::make_unique<ShutdownObserver>(display_configurator_.get());
+
+      // Register |display_change_observer_| first so that the rest of
+      // observer gets invoked after the root windows are configured.
+      display_configurator_->AddObserver(display_change_observer_.get());
+      display_error_observer_.reset(new DisplayErrorObserver());
+      display_configurator_->AddObserver(display_error_observer_.get());
+      display_configurator_->set_state_controller(
+          display_change_observer_.get());
+      display_configurator_->set_mirroring_controller(display_manager_.get());
+      display_configurator_->ForceInitialConfigure();
+      display_initialized = true;
+    }
   }
+
   display_color_manager_ =
       std::make_unique<DisplayColorManager>(display_configurator_.get());
 

chrome/browser/chrome_content_browser_manifest_overlay.json

@@ -48,7 +48,7 @@
         "pdf_to_pwg_raster_converter": [ "converter" ],
         "preferences": [ "pref_client", "pref_control" ],
         "ui": [
-          "display_controller",
+          "display_dev",
           "ime_registrar",
           "input_device_controller",
           "window_manager"

services/ui/display/screen_manager_forwarding.cc

@@ -42,7 +42,7 @@ ScreenManagerForwarding::ScreenManagerForwarding(Mode mode)
     : is_in_process_(mode == Mode::IN_WM_PROCESS),
       screen_(base::MakeUnique<display::ScreenBase>()),
       binding_(this),
-      test_controller_binding_(this) {
+      dev_controller_binding_(this) {
   if (!is_in_process_)
     Screen::SetScreenInstance(screen_.get());
 }
@@ -60,8 +60,8 @@ void ScreenManagerForwarding::AddInterfaces(
   registry->AddInterface<mojom::NativeDisplayDelegate>(
       base::Bind(&ScreenManagerForwarding::BindNativeDisplayDelegateRequest,
                  base::Unretained(this)));
-  registry->AddInterface<mojom::TestDisplayController>(
-      base::Bind(&ScreenManagerForwarding::BindTestDisplayControllerRequest,
+  registry->AddInterface<mojom::DevDisplayController>(
+      base::Bind(&ScreenManagerForwarding::BindDevDisplayControllerRequest,
                  base::Unretained(this)));
 }
 
@@ -235,11 +235,11 @@ void ScreenManagerForwarding::BindNativeDisplayDelegateRequest(
   binding_.Bind(std::move(request));
 }
 
-void ScreenManagerForwarding::BindTestDisplayControllerRequest(
-    mojom::TestDisplayControllerRequest request,
+void ScreenManagerForwarding::BindDevDisplayControllerRequest(
+    mojom::DevDisplayControllerRequest request,
     const service_manager::BindSourceInfo& source_info) {
-  DCHECK(!test_controller_binding_.is_bound());
-  test_controller_binding_.Bind(std::move(request));
+  DCHECK(!dev_controller_binding_.is_bound());
+  dev_controller_binding_.Bind(std::move(request));
 }
 
 void ScreenManagerForwarding::ForwardGetDisplays(

services/ui/display/screen_manager_forwarding.h

@@ -12,7 +12,7 @@
 #include "base/macros.h"
 #include "mojo/public/cpp/bindings/binding.h"
 #include "services/ui/display/screen_manager.h"
-#include "services/ui/public/interfaces/display/test_display_controller.mojom.h"
+#include "ui/display/mojo/dev_display_controller.mojom.h"
 #include "ui/display/mojo/native_display_delegate.mojom.h"
 #include "ui/display/types/native_display_observer.h"
 
@@ -25,7 +25,7 @@ class NativeDisplayDelegate;
 // This will own a real NativeDisplayDelegate and forwards calls to and
 // responses from it over Mojo.
 class ScreenManagerForwarding : public ScreenManager,
-                                public mojom::TestDisplayController,
+                                public mojom::DevDisplayController,
                                 public NativeDisplayObserver,
                                 public mojom::NativeDisplayDelegate {
  public:
@@ -73,15 +73,15 @@ class ScreenManagerForwarding : public ScreenManager,
       const std::vector<display::GammaRampRGBEntry>& gamma_lut,
       const std::vector<float>& correction_matrix) override;
 
-  // mojom::TestDisplayController:
+  // mojom::DevDisplayController:
   void ToggleAddRemoveDisplay() override;
 
  private:
   void BindNativeDisplayDelegateRequest(
       mojom::NativeDisplayDelegateRequest request,
       const service_manager::BindSourceInfo& source_info);
-  void BindTestDisplayControllerRequest(
-      mojom::TestDisplayControllerRequest request,
+  void BindDevDisplayControllerRequest(
+      mojom::DevDisplayControllerRequest request,
       const service_manager::BindSourceInfo& source_info);
 
   // Forwards results from GetDisplays() back with |callback|.
@@ -103,7 +103,7 @@ class ScreenManagerForwarding : public ScreenManager,
   mojo::Binding<mojom::NativeDisplayDelegate> binding_;
   mojom::NativeDisplayObserverPtr observer_;
 
-  mojo::Binding<mojom::TestDisplayController> test_controller_binding_;
+  mojo::Binding<mojom::DevDisplayController> dev_controller_binding_;
 
   std::unique_ptr<display::NativeDisplayDelegate> native_display_delegate_;
 

services/ui/display/screen_manager_ozone_internal.cc

@@ -112,8 +112,8 @@ void ScreenManagerOzoneInternal::AddInterfaces(
   registry->AddInterface<mojom::OutputProtection>(
       base::Bind(&ScreenManagerOzoneInternal::BindOutputProtectionRequest,
                  base::Unretained(this)));
-  registry->AddInterface<mojom::TestDisplayController>(
-      base::Bind(&ScreenManagerOzoneInternal::BindTestDisplayControllerRequest,
+  registry->AddInterface<mojom::DevDisplayController>(
+      base::Bind(&ScreenManagerOzoneInternal::BindDevDisplayControllerRequest,
                  base::Unretained(this)));
 }
 
@@ -344,8 +344,8 @@ void ScreenManagerOzoneInternal::BindOutputProtectionRequest(
       std::move(request));
 }
 
-void ScreenManagerOzoneInternal::BindTestDisplayControllerRequest(
-    mojom::TestDisplayControllerRequest request,
+void ScreenManagerOzoneInternal::BindDevDisplayControllerRequest(
+    mojom::DevDisplayControllerRequest request,
     const service_manager::BindSourceInfo& source_info) {
   test_bindings_.AddBinding(this, std::move(request));
 }

services/ui/display/screen_manager_ozone_internal.h

@@ -15,11 +15,11 @@
 #include "services/ui/display/viewport_metrics.h"
 #include "services/ui/public/interfaces/display/display_controller.mojom.h"
 #include "services/ui/public/interfaces/display/output_protection.mojom.h"
-#include "services/ui/public/interfaces/display/test_display_controller.mojom.h"
 #include "ui/display/display.h"
 #include "ui/display/display_observer.h"
 #include "ui/display/manager/chromeos/display_configurator.h"
 #include "ui/display/manager/display_manager.h"
+#include "ui/display/mojo/dev_display_controller.mojom.h"
 #include "ui/display/types/display_constants.h"
 
 namespace display {
@@ -33,7 +33,7 @@ class TouchTransformController;
 // all attached physical displays on the the ozone platform when operating in
 // internal window mode.
 class ScreenManagerOzoneInternal : public ScreenManager,
-                                   public mojom::TestDisplayController,
+                                   public mojom::DevDisplayController,
                                    public mojom::DisplayController,
                                    public DisplayObserver,
                                    public DisplayManager::Delegate {
@@ -51,7 +51,7 @@ class ScreenManagerOzoneInternal : public ScreenManager,
   void RequestCloseDisplay(int64_t display_id) override;
   display::ScreenBase* GetScreen() override;
 
-  // mojom::TestDisplayController:
+  // mojom::DevDisplayController:
   void ToggleAddRemoveDisplay() override;
 
   // mojom::DisplayController:
@@ -95,8 +95,8 @@ class ScreenManagerOzoneInternal : public ScreenManager,
       mojom::OutputProtectionRequest request,
       const service_manager::BindSourceInfo& source_info);
 
-  void BindTestDisplayControllerRequest(
-      mojom::TestDisplayControllerRequest request,
+  void BindDevDisplayControllerRequest(
+      mojom::DevDisplayControllerRequest request,
       const service_manager::BindSourceInfo& source_info);
 
   DisplayConfigurator display_configurator_;
@@ -123,7 +123,7 @@ class ScreenManagerOzoneInternal : public ScreenManager,
   int64_t primary_display_id_ = kInvalidDisplayId;
 
   mojo::BindingSet<mojom::DisplayController> controller_bindings_;
-  mojo::BindingSet<mojom::TestDisplayController> test_bindings_;
+  mojo::BindingSet<mojom::DevDisplayController> test_bindings_;
 
   DISALLOW_COPY_AND_ASSIGN(ScreenManagerOzoneInternal);
 };

services/ui/manifest.json

@@ -36,8 +36,8 @@
         "display_output_protection": [
           "display::mojom::OutputProtection"
         ],
-        "display_test": [
-          "display::mojom::TestDisplayController"
+        "display_dev": [
+          "display::mojom::DevDisplayController"
         ],
         "gpu_client": [
           "ui::mojom::Gpu"

services/ui/public/interfaces/display/BUILD.gn

@@ -8,7 +8,6 @@ mojom("display") {
   sources = [
     "display_controller.mojom",
     "output_protection.mojom",
-    "test_display_controller.mojom",
   ]
 
   public_deps = [

ui/display/manager/display_manager.cc

@@ -283,6 +283,11 @@ DisplayManager::~DisplayManager() {
 #endif
 }
 
+void DisplayManager::SetDevDisplayController(
+    mojom::DevDisplayControllerPtr controller) {
+  dev_display_controller_ = std::move(controller);
+}
+
 bool DisplayManager::InitFromCommandLine() {
   DisplayInfoList info_list;
   base::CommandLine* command_line = base::CommandLine::ForCurrentProcess();
@@ -1203,6 +1208,14 @@ void DisplayManager::SetMirrorMode(bool mirror) {
 
 void DisplayManager::AddRemoveDisplay() {
   DCHECK(!active_display_list_.empty());
+
+  // DevDisplayController will have NativeDisplayDelegate add/remove a display
+  // so that the full display configuration code runs.
+  if (dev_display_controller_.is_bound()) {
+    dev_display_controller_->ToggleAddRemoveDisplay();
+    return;
+  }
+
   DisplayInfoList new_display_info_list;
   const ManagedDisplayInfo& first_display =
       IsInUnifiedMode()

ui/display/manager/display_manager.h

@@ -25,6 +25,7 @@
 #include "ui/display/display_observer.h"
 #include "ui/display/manager/display_manager_export.h"
 #include "ui/display/manager/managed_display_info.h"
+#include "ui/display/mojo/dev_display_controller.mojom.h"
 #include "ui/display/types/display_constants.h"
 #include "ui/display/unified_desktop_utils.h"
 
@@ -128,6 +129,11 @@ class DISPLAY_MANAGER_EXPORT DisplayManager
   // Returns the display id of the first display in the outupt list.
   int64_t first_display_id() const { return first_display_id_; }
 
+  // Sets controller used to add/remove fake displays. If this is set then
+  // AddRemoveDisplay() will delegate out to |dev_display_controller_| instead
+  // of adding/removing a ManagedDisplayInfo.
+  void SetDevDisplayController(mojom::DevDisplayControllerPtr controller);
+
   // Initializes displays using command line flag. Returns false if no command
   // line flag was provided.
   bool InitFromCommandLine();
@@ -587,6 +593,8 @@ class DISPLAY_MANAGER_EXPORT DisplayManager
 
   base::ObserverList<DisplayObserver> observers_;
 
+  display::mojom::DevDisplayControllerPtr dev_display_controller_;
+
   // This is incremented whenever a BeginEndNotifier is created and decremented
   // when destroyed. BeginEndNotifier uses this to track when it should call
   // OnWillProcessDisplayChanges() and OnDidProcessDisplayChanges().

ui/display/mojo/BUILD.gn

@@ -6,6 +6,7 @@ import("//mojo/public/tools/bindings/mojom.gni")
 
 mojom("interfaces") {
   sources = [
+    "dev_display_controller.mojom",
     "display.mojom",
     "display_constants.mojom",
     "display_layout.mojom",

services/ui/public/interfaces/display/test_display_controller.mojom → ui/display/mojo/dev_display_controller.mojom

-// Copyright 2016 The Chromium Authors. All rights reserved.
+// Copyright 2017 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 module display.mojom;
 
 // Provides a way to modify the display state at runtime. Will only work when
-// running off device with fake displays.
-interface TestDisplayController {
+// running off device with fake displays for development.
+interface DevDisplayController {
 
   // Toggles adding or removing a fake display. If there is only one display
   // a second display is added. If there is more than one display then the last