cros: Fix cursor for chrome --mash

Grouping the ash service and ui service into the same process broke
image cursors. Fix it by providing the necessary task runner for cursor
resource loading. Make MashServiceRegistry into a class so it can hold
the cursor set.

Also refactor ui::Service InitParams to make it more clear when it is
running in-process vs. out-of-process.

Bug: 722527
Test: Run chrome --mash, hovering NTP tiles has hand cursor
Change-Id: Ie2b3fc9d523af9f06e16a1b6137f8481182169de
Reviewed-on: https://chromium-review.googlesource.com/832996Reviewed-by: Scott Violet <sky@chromium.org>
Reviewed-by: Ken Rockot <rockot@chromium.org>
Reviewed-by: Sadrul Chowdhury <sadrul@chromium.org>
Commit-Queue: James Cook <jamescook@chromium.org>
Cr-Commit-Position: refs/heads/master@{#526998}

chrome/utility/DEPS

@@ -50,6 +50,7 @@ specific_include_rules = {
     "+components/font_service/public/interfaces",
     "+mash/quick_launch/public",
     "+mash/quick_launch/quick_launch.h",
+    "+services/ui/common/image_cursors_set.h",
     "+services/ui/public",
     "+services/ui/service.h",
   ],

chrome/utility/chrome_content_utility_client.cc

@@ -62,7 +62,7 @@
 #endif
 #endif
 
-#if BUILDFLAG(ENABLE_MUS)
+#if defined(OS_CHROMEOS)
 #include "chrome/utility/mash_service_factory.h"
 #endif
 
@@ -144,6 +144,10 @@ ChromeContentUtilityClient::ChromeContentUtilityClient()
     (BUILDFLAG(ENABLE_BASIC_PRINTING) && defined(OS_WIN))
   handlers_.push_back(base::MakeUnique<printing::PrintingHandler>());
 #endif
+
+#if defined(OS_CHROMEOS)
+  mash_service_factory_ = std::make_unique<MashServiceFactory>();
+#endif
 }
 
 ChromeContentUtilityClient::~ChromeContentUtilityClient() = default;
@@ -293,7 +297,8 @@ void ChromeContentUtilityClient::RegisterServices(
 #endif  // BUILDFLAG(ENABLE_EXTENSIONS)
 
 #if defined(OS_CHROMEOS)
-  RegisterOutOfProcessMashServices(services);
+  // TODO(jamescook): Figure out why we have to do this when not using --mash.
+  mash_service_factory_->RegisterOutOfProcessServices(services);
 #endif
 }
 

chrome/utility/chrome_content_utility_client.h

@@ -11,6 +11,7 @@
 #include "base/macros.h"
 #include "content/public/utility/content_utility_client.h"
 
+class MashServiceFactory;
 class UtilityMessageHandler;
 
 class ChromeContentUtilityClient : public content::ContentUtilityClient {
@@ -42,6 +43,11 @@ class ChromeContentUtilityClient : public content::ContentUtilityClient {
   // True if the utility process runs with elevated privileges.
   bool utility_process_running_elevated_;
 
+#if defined(OS_CHROMEOS)
+  // Must be owned by utility main thread.
+  std::unique_ptr<MashServiceFactory> mash_service_factory_;
+#endif
+
   DISALLOW_COPY_AND_ASSIGN(ChromeContentUtilityClient);
 };
 

chrome/utility/mash_service_factory.cc

@@ -16,6 +16,7 @@
 #include "components/font_service/public/interfaces/constants.mojom.h"
 #include "mash/quick_launch/public/interfaces/constants.mojom.h"
 #include "mash/quick_launch/quick_launch.h"
+#include "services/ui/common/image_cursors_set.h"
 #include "services/ui/public/interfaces/constants.mojom.h"
 #include "services/ui/service.h"
 
@@ -32,25 +33,31 @@ void RegisterMashService(
   services->emplace(name, service_info);
 }
 
+// Runs on the UI service main thread.
 // NOTE: For --mus the UI service is created at the //chrome/browser layer,
 // not in //content. See ServiceManagerContext.
 std::unique_ptr<service_manager::Service> CreateUiService(
-    const scoped_refptr<base::SingleThreadTaskRunner>& task_runner) {
-  ui::Service::InProcessConfig config;
-  config.resource_runner = task_runner;
-  // TODO(jamescook): Fix cursor loading.
-  config.should_host_viz = true;
-  return std::make_unique<ui::Service>(&config);
+    const scoped_refptr<base::SingleThreadTaskRunner>& resource_runner,
+    base::WeakPtr<ui::ImageCursorsSet> image_cursors_set_weak_ptr) {
+  ui::Service::InitParams params;
+  params.running_standalone = false;
+  params.resource_runner = resource_runner;
+  params.image_cursors_set_weak_ptr = image_cursors_set_weak_ptr;
+  params.should_host_viz = true;
+  return std::make_unique<ui::Service>(params);
 }
 
+// Runs on the utility process main thread.
 void RegisterUiService(
-    content::ContentUtilityClient::StaticServiceMap* services) {
+    content::ContentUtilityClient::StaticServiceMap* services,
+    ui::ImageCursorsSet* cursors) {
   service_manager::EmbeddedServiceInfo service_info;
   service_info.use_own_thread = true;
   service_info.message_loop_type = base::MessageLoop::TYPE_UI;
   service_info.thread_priority = base::ThreadPriority::DISPLAY;
-  service_info.factory = base::BindRepeating(
-      &CreateUiService, base::ThreadTaskRunnerHandle::Get());
+  service_info.factory =
+      base::BindRepeating(&CreateUiService, base::ThreadTaskRunnerHandle::Get(),
+                          cursors->GetWeakPtr());
   services->emplace(ui::mojom::kServiceName, service_info);
 }
 
@@ -78,9 +85,14 @@ std::unique_ptr<service_manager::Service> CreateFontService() {
 
 }  // namespace
 
-void RegisterOutOfProcessMashServices(
+MashServiceFactory::MashServiceFactory()
+    : cursors_(std::make_unique<ui::ImageCursorsSet>()) {}
+
+MashServiceFactory::~MashServiceFactory() = default;
+
+void MashServiceFactory::RegisterOutOfProcessServices(
     content::ContentUtilityClient::StaticServiceMap* services) {
-  RegisterUiService(services);
+  RegisterUiService(services, cursors_.get());
   RegisterMashService(services, mash::quick_launch::mojom::kServiceName,
                       &CreateQuickLaunch);
   RegisterMashService(services, ash::mojom::kServiceName, &CreateAshService);

chrome/utility/mash_service_factory.h

@@ -5,10 +5,29 @@
 #ifndef CHROME_UTILITY_MASH_SERVICE_FACTORY_H_
 #define CHROME_UTILITY_MASH_SERVICE_FACTORY_H_
 
+#include <memory>
+
 #include "content/public/utility/content_utility_client.h"
 
-// Registers the out-of-process services for --mash.
-void RegisterOutOfProcessMashServices(
-    content::ContentUtilityClient::StaticServiceMap* services);
+namespace ui {
+class ImageCursorsSet;
+}
+
+// Lives on the utility process main thread.
+class MashServiceFactory {
+ public:
+  MashServiceFactory();
+  ~MashServiceFactory();
+
+  // Registers out-of-process services for --mash.
+  void RegisterOutOfProcessServices(
+      content::ContentUtilityClient::StaticServiceMap* services);
+
+ private:
+  // Must live on the utility main thread.
+  std::unique_ptr<ui::ImageCursorsSet> cursors_;
+
+  DISALLOW_COPY_AND_ASSIGN(MashServiceFactory);
+};
 
 #endif  // CHROME_UTILITY_MASH_SERVICE_FACTORY_H_

content/browser/service_manager/service_manager_context.cc

@@ -272,12 +272,13 @@ std::unique_ptr<service_manager::Service> CreateEmbeddedUIService(
     const scoped_refptr<base::SingleThreadTaskRunner>& task_runner,
     base::WeakPtr<ui::ImageCursorsSet> image_cursors_set_weak_ptr,
     discardable_memory::DiscardableSharedMemoryManager* memory_manager) {
-  ui::Service::InProcessConfig config;
-  config.resource_runner = task_runner;
-  config.image_cursors_set_weak_ptr = image_cursors_set_weak_ptr;
-  config.memory_manager = memory_manager;
-  config.should_host_viz = switches::IsMusHostingViz();
-  return std::make_unique<ui::Service>(&config);
+  ui::Service::InitParams params;
+  params.running_standalone = false;
+  params.resource_runner = task_runner;
+  params.image_cursors_set_weak_ptr = image_cursors_set_weak_ptr;
+  params.memory_manager = memory_manager;
+  params.should_host_viz = switches::IsMusHostingViz();
+  return std::make_unique<ui::Service>(params);
 }
 
 void RegisterUIServiceInProcessIfNecessary(

services/ui/main.cc

@@ -8,8 +8,9 @@
 #include "services/ui/service.h"
 
 MojoResult ServiceMain(MojoHandle service_request_handle) {
-  ui::Service* ui_service = new ui::Service;
-  ui_service->set_running_standalone(true);
+  ui::Service::InitParams params;
+  params.running_standalone = true;
+  ui::Service* ui_service = new ui::Service(params);
   service_manager::ServiceRunner runner(ui_service);
   runner.set_message_loop_type(base::MessageLoop::TYPE_UI);
   return runner.Run(service_request_handle);

services/ui/service.cc

@@ -80,27 +80,22 @@ const char kResourceFile200[] = "mus_app_resources_200.pak";
 
 class ThreadedImageCursorsFactoryImpl : public ws::ThreadedImageCursorsFactory {
  public:
-  // Uses the same InProcessConfig as the UI Service. |config| will be null when
-  // the UI Service runs in it's own separate process as opposed to the WM's
-  // process.
-  explicit ThreadedImageCursorsFactoryImpl(
-      const Service::InProcessConfig* config) {
-    if (config) {
-      resource_runner_ = config->resource_runner;
-      image_cursors_set_weak_ptr_ = config->image_cursors_set_weak_ptr;
-      DCHECK(resource_runner_);
+  explicit ThreadedImageCursorsFactoryImpl(const Service::InitParams& params) {
+    // When running in-process use |resource_runner_| to load cursors.
+    if (params.resource_runner) {
+      resource_runner_ = params.resource_runner;
+      // |params.image_cursors_set_weak_ptr| must be set, but don't DCHECK
+      // because it can only be dereferenced on |resource_runner_|.
+      image_cursors_set_weak_ptr_ = params.image_cursors_set_weak_ptr;
     }
   }
 
-  ~ThreadedImageCursorsFactoryImpl() override {}
+  ~ThreadedImageCursorsFactoryImpl() override = default;
 
   // ws::ThreadedImageCursorsFactory:
   std::unique_ptr<ws::ThreadedImageCursors> CreateCursors() override {
-    // |resource_runner_| will not be initialized if and only if UI Service runs
-    // in it's own separate process. In this case we can (lazily) initialize it
-    // to the current thread (i.e. the UI Services's thread). We also initialize
-    // the local |image_cursors_set_| and make |image_cursors_set_weak_ptr_|
-    // point to it.
+    // When running out-of-process lazily initialize the resource runner to the
+    // UI service's thread.
     if (!resource_runner_) {
       resource_runner_ = base::ThreadTaskRunnerHandle::Get();
       image_cursors_set_ = std::make_unique<ui::ImageCursorsSet>();
@@ -135,19 +130,21 @@ struct Service::UserState {
   std::unique_ptr<ws::WindowTreeHostFactory> window_tree_host_factory;
 };
 
-Service::InProcessConfig::InProcessConfig() = default;
+Service::InitParams::InitParams() = default;
 
-Service::InProcessConfig::~InProcessConfig() = default;
+Service::InitParams::~InitParams() = default;
 
-Service::Service(const InProcessConfig* config)
-    : is_in_process_(config != nullptr),
+Service::Service(const InitParams& params)
+    : running_standalone_(params.running_standalone),
       threaded_image_cursors_factory_(
-          std::make_unique<ThreadedImageCursorsFactoryImpl>(config)),
+          std::make_unique<ThreadedImageCursorsFactoryImpl>(params)),
       test_config_(false),
       ime_registrar_(&ime_driver_),
-      discardable_shared_memory_manager_(config ? config->memory_manager
-                                                : nullptr),
-      should_host_viz_(!config || config->should_host_viz) {}
+      discardable_shared_memory_manager_(params.memory_manager),
+      should_host_viz_(params.should_host_viz) {
+  // UI service must host viz when running in its own process.
+  DCHECK(!running_standalone_ || should_host_viz_);
+}
 
 Service::~Service() {
   in_destructor_ = true;
@@ -171,7 +168,7 @@ Service::~Service() {
 }
 
 bool Service::InitializeResources(service_manager::Connector* connector) {
-  if (is_in_process() || ui::ResourceBundle::HasSharedInstance())
+  if (!running_standalone_ || ui::ResourceBundle::HasSharedInstance())
     return true;
 
   std::set<std::string> resource_paths;
@@ -187,8 +184,7 @@ bool Service::InitializeResources(service_manager::Connector* connector) {
     return false;
   }
 
-  if (running_standalone_)
-    ui::RegisterPathProvider();
+  ui::RegisterPathProvider();
 
   // Initialize resource bundle with 1x and 2x cursor bitmaps.
   ui::ResourceBundle::InitSharedInstanceWithPakFileRegion(
@@ -256,7 +252,7 @@ void Service::OnStart() {
   ui::KeyboardLayoutEngineManager::GetKeyboardLayoutEngine()
       ->SetCurrentLayoutByName("us");
 
-  if (!is_in_process()) {
+  if (running_standalone_) {
     client_native_pixmap_factory_ = ui::CreateClientNativePixmapFactoryOzone();
     gfx::ClientNativePixmapFactory::SetInstance(
         client_native_pixmap_factory_.get());
@@ -411,8 +407,9 @@ void Service::OnWillCreateTreeForWindowManager(
       ws::DisplayCreationConfig::MANUAL) {
 #if defined(OS_CHROMEOS)
     display::ScreenManagerForwarding::Mode mode =
-        is_in_process() ? display::ScreenManagerForwarding::Mode::IN_WM_PROCESS
-                        : display::ScreenManagerForwarding::Mode::OWN_PROCESS;
+        running_standalone_
+            ? display::ScreenManagerForwarding::Mode::OWN_PROCESS
+            : display::ScreenManagerForwarding::Mode::IN_WM_PROCESS;
     screen_manager_ = std::make_unique<display::ScreenManagerForwarding>(mode);
 #else
     CHECK(false);

services/ui/service.h

@@ -74,11 +74,12 @@ class WindowServer;
 class Service : public service_manager::Service,
                 public ws::WindowServerDelegate {
  public:
-  // Contains the configuration necessary to run the UI Service inside the
-  // Window Manager's process.
-  struct InProcessConfig {
-    InProcessConfig();
-    ~InProcessConfig();
+  struct InitParams {
+    InitParams();
+    ~InitParams();
+
+    // UI service runs in its own process (i.e. not embedded in browser or ash).
+    bool running_standalone = false;
 
     // Can be used to load resources.
     scoped_refptr<base::SingleThreadTaskRunner> resource_runner = nullptr;
@@ -95,17 +96,12 @@ class Service : public service_manager::Service,
     bool should_host_viz = true;
 
    private:
-    DISALLOW_COPY_AND_ASSIGN(InProcessConfig);
+    DISALLOW_COPY_AND_ASSIGN(InitParams);
   };
 
-  // |config| should be null when UI Service runs in it's own separate process,
-  // as opposed to inside the Window Manager's process.
-  explicit Service(const InProcessConfig* config = nullptr);
+  explicit Service(const InitParams& params);
   ~Service() override;
 
-  // Call if the ui::Service is being run as a standalone process.
-  void set_running_standalone(bool value) { running_standalone_ = value; }
-
  private:
   // Holds InterfaceRequests received before the first WindowTreeHost Display
   // has been established.
@@ -114,8 +110,6 @@ class Service : public service_manager::Service,
 
   using UserIdToUserState = std::map<ws::UserId, std::unique_ptr<UserState>>;
 
-  bool is_in_process() const { return is_in_process_; }
-
   // Attempts to initialize the resource bundle. Returns true if successful,
   // otherwise false if resources cannot be loaded.
   bool InitializeResources(service_manager::Connector* connector);
@@ -204,9 +198,9 @@ class Service : public service_manager::Service,
   // and must outlive |registry_|.
   InputDeviceServer input_device_server_;
 
-  // True if the UI Service runs inside WM's process, false if it runs inside
-  // its own process.
-  const bool is_in_process_;
+  // True if the UI Service runs runs inside its own process, false if it is
+  // embedded in another process.
+  const bool running_standalone_;
 
   std::unique_ptr<ws::ThreadedImageCursorsFactory>
       threaded_image_cursors_factory_;
@@ -248,8 +242,6 @@ class Service : public service_manager::Service,
 
   bool in_destructor_ = false;
 
-  bool running_standalone_ = false;
-
   DISALLOW_COPY_AND_ASSIGN(Service);
 };