Use BrowserContextKeyedService for ArcAccessibilityHelperBridge.

Along the change, fixed a bug where primary user is in background.

BUG=672829
TEST=Ran on DUT. Ran trybot.

Change-Id: I83c080fd005bb55cb1c1570f6ab465ad6524a2aa
Reviewed-on: https://chromium-review.googlesource.com/563151
Commit-Queue: Hidehiko Abe <hidehiko@chromium.org>
Reviewed-by: Yusuke Sato <yusukes@chromium.org>
Reviewed-by: Yuki Awano <yawano@chromium.org>
Cr-Commit-Position: refs/heads/master@{#485247}

chrome/browser/chromeos/BUILD.gn

@@ -282,6 +282,8 @@ source_set("chromeos") {
     "app_mode/startup_app_launcher.h",
     "arc/accessibility/arc_accessibility_helper_bridge.cc",
     "arc/accessibility/arc_accessibility_helper_bridge.h",
+    "arc/accessibility/arc_accessibility_helper_bridge_factory.cc",
+    "arc/accessibility/arc_accessibility_helper_bridge_factory.h",
     "arc/accessibility/ax_tree_source_arc.cc",
     "arc/accessibility/ax_tree_source_arc.h",
     "arc/arc_auth_notification.cc",

chrome/browser/chromeos/arc/accessibility/arc_accessibility_helper_bridge.cc

@@ -11,6 +11,7 @@
 #include "chrome/browser/profiles/profile.h"
 #include "chromeos/chromeos_switches.h"
 #include "components/arc/arc_bridge_service.h"
+#include "components/arc/arc_service_manager.h"
 #include "components/exo/shell_surface.h"
 #include "components/exo/surface.h"
 #include "ui/aura/client/aura_constants.h"
@@ -43,10 +44,11 @@ int32_t GetTaskId(aura::Window* window) {
   return task_id;
 }
 
-void DispatchFocusChange(arc::mojom::AccessibilityNodeInfoData* node_data) {
+void DispatchFocusChange(arc::mojom::AccessibilityNodeInfoData* node_data,
+                         Profile* profile) {
   chromeos::AccessibilityManager* accessibility_manager =
       chromeos::AccessibilityManager::Get();
-  if (!accessibility_manager)
+  if (!accessibility_manager || accessibility_manager->profile() != profile)
     return;
 
   exo::WMHelper* wm_helper = exo::WMHelper::GetInstance();
@@ -66,7 +68,7 @@ void DispatchFocusChange(arc::mojom::AccessibilityNodeInfoData* node_data) {
   accessibility_manager->OnViewFocusedInArc(bounds_in_screen);
 }
 
-arc::mojom::AccessibilityFilterType GetFilterType() {
+arc::mojom::AccessibilityFilterType GetFilterTypeForProfile(Profile* profile) {
   if (base::CommandLine::ForCurrentProcess()->HasSwitch(
           chromeos::switches::kEnableChromeVoxArcSupport)) {
     return arc::mojom::AccessibilityFilterType::ALL;
@@ -77,6 +79,10 @@ arc::mojom::AccessibilityFilterType GetFilterType() {
   if (!accessibility_manager)
     return arc::mojom::AccessibilityFilterType::OFF;
 
+  // TODO(yawano): Support the case where primary user is in background.
+  if (accessibility_manager->profile() != profile)
+    return arc::mojom::AccessibilityFilterType::OFF;
+
   if (accessibility_manager->IsSpokenFeedbackEnabled())
     return arc::mojom::AccessibilityFilterType::WHITELISTED_PACKAGE_NAME;
 
@@ -91,57 +97,57 @@ arc::mojom::AccessibilityFilterType GetFilterType() {
 namespace arc {
 
 ArcAccessibilityHelperBridge::ArcAccessibilityHelperBridge(
-    ArcBridgeService* bridge_service)
-    : ArcService(bridge_service), binding_(this), current_task_id_(kNoTaskId) {
-  arc_bridge_service()->accessibility_helper()->AddObserver(this);
+    Profile* profile,
+    ArcBridgeService* arc_bridge_service)
+    : profile_(profile),
+      arc_bridge_service_(arc_bridge_service),
+      binding_(this),
+      current_task_id_(kNoTaskId) {
+  arc_bridge_service_->accessibility_helper()->AddObserver(this);
+
+  // Null on testing.
+  auto* app_list_prefs = ArcAppListPrefs::Get(profile_);
+  if (app_list_prefs)
+    app_list_prefs->AddObserver(this);
 }
 
-ArcAccessibilityHelperBridge::~ArcAccessibilityHelperBridge() {
+ArcAccessibilityHelperBridge::~ArcAccessibilityHelperBridge() = default;
+
+void ArcAccessibilityHelperBridge::Shutdown() {
   // We do not unregister ourselves from WMHelper as an ActivationObserver
   // because it is always null at this point during teardown.
 
-  chromeos::AccessibilityManager* accessibility_manager =
-      chromeos::AccessibilityManager::Get();
-  if (accessibility_manager) {
-    Profile* profile = accessibility_manager->profile();
-    if (profile && ArcAppListPrefs::Get(profile))
-      ArcAppListPrefs::Get(profile)->RemoveObserver(this);
-  }
-
-  ArcBridgeService* arc_bridge_service_ptr = arc_bridge_service();
-  if (arc_bridge_service_ptr)
-    arc_bridge_service_ptr->accessibility_helper()->RemoveObserver(this);
+  // Null on testing.
+  auto* app_list_prefs = ArcAppListPrefs::Get(profile_);
+  if (app_list_prefs)
+    app_list_prefs->RemoveObserver(this);
+
+  // TODO(hidehiko): Currently, the lifetime of ArcBridgeService and
+  // BrowserContextKeyedService is not nested.
+  // If ArcServiceManager::Get() returns nullptr, it is already destructed,
+  // so do not touch it.
+  if (ArcServiceManager::Get())
+    arc_bridge_service_->accessibility_helper()->RemoveObserver(this);
 }
 
 void ArcAccessibilityHelperBridge::OnInstanceReady() {
-  chromeos::AccessibilityManager* accessibility_manager =
-      chromeos::AccessibilityManager::Get();
-  if (!accessibility_manager)
-    return;
-
-  Profile* profile = accessibility_manager->profile();
-  if (!profile)
-    return;
-
-  ArcAppListPrefs* arc_app_list_prefs = ArcAppListPrefs::Get(profile);
-  if (!arc_app_list_prefs)
-    return;
-
-  arc_app_list_prefs->AddObserver(this);
   auto* instance = ARC_GET_INSTANCE_FOR_METHOD(
-      arc_bridge_service()->accessibility_helper(), Init);
+      arc_bridge_service_->accessibility_helper(), Init);
   DCHECK(instance);
 
   mojom::AccessibilityHelperHostPtr host_proxy;
   binding_.Bind(mojo::MakeRequest(&host_proxy));
   instance->Init(std::move(host_proxy));
 
-  arc::mojom::AccessibilityFilterType filter_type = GetFilterType();
+  arc::mojom::AccessibilityFilterType filter_type =
+      GetFilterTypeForProfile(profile_);
   instance->SetFilter(filter_type);
 
   if (filter_type == arc::mojom::AccessibilityFilterType::ALL ||
       filter_type ==
           arc::mojom::AccessibilityFilterType::WHITELISTED_PACKAGE_NAME) {
+    // TODO(yawano): Handle the case where filter_type has changed between
+    // OFF/FOCUS and ALL/WHITELISTED_PACKAGE_NAME after this initialization.
     exo::WMHelper::GetInstance()->AddActivationObserver(this);
   }
 }
@@ -150,12 +156,14 @@ void ArcAccessibilityHelperBridge::OnAccessibilityEventDeprecated(
     mojom::AccessibilityEventType event_type,
     mojom::AccessibilityNodeInfoDataPtr event_source) {
   if (event_type == arc::mojom::AccessibilityEventType::VIEW_FOCUSED)
-    DispatchFocusChange(event_source.get());
+    DispatchFocusChange(event_source.get(), profile_);
 }
 
 void ArcAccessibilityHelperBridge::OnAccessibilityEvent(
     mojom::AccessibilityEventDataPtr event_data) {
-  arc::mojom::AccessibilityFilterType filter_type = GetFilterType();
+  // TODO(yawano): Handle AccessibilityFilterType::OFF.
+  arc::mojom::AccessibilityFilterType filter_type =
+      GetFilterTypeForProfile(profile_);
 
   if (filter_type == arc::mojom::AccessibilityFilterType::ALL ||
       filter_type ==
@@ -203,7 +211,7 @@ void ArcAccessibilityHelperBridge::OnAccessibilityEvent(
     return;
 
   CHECK_EQ(1U, event_data.get()->node_data.size());
-  DispatchFocusChange(event_data.get()->node_data[0].get());
+  DispatchFocusChange(event_data.get()->node_data[0].get(), profile_);
 }
 
 void ArcAccessibilityHelperBridge::OnAction(
@@ -227,7 +235,7 @@ void ArcAccessibilityHelperBridge::OnAction(
   }
 
   auto* instance = ARC_GET_INSTANCE_FOR_METHOD(
-      arc_bridge_service()->accessibility_helper(), PerformAction);
+      arc_bridge_service_->accessibility_helper(), PerformAction);
   instance->PerformAction(std::move(action_data));
 }
 

chrome/browser/chromeos/arc/accessibility/arc_accessibility_helper_bridge.h

@@ -12,32 +12,38 @@
 
 #include "chrome/browser/chromeos/arc/accessibility/ax_tree_source_arc.h"
 #include "chrome/browser/ui/app_list/arc/arc_app_list_prefs.h"
-#include "components/arc/arc_service.h"
 #include "components/arc/common/accessibility_helper.mojom.h"
 #include "components/arc/instance_holder.h"
 #include "components/exo/wm_helper.h"
+#include "components/keyed_service/core/keyed_service.h"
 #include "mojo/public/cpp/bindings/binding.h"
 #include "ui/accessibility/ax_host_delegate.h"
 
+class Profile;
+
 namespace arc {
 
-class ArcBridgeService;
 class AXTreeSourceArc;
+class ArcBridgeService;
 
 // ArcAccessibilityHelperBridge is an instance to receive converted Android
 // accessibility events and info via mojo interface and dispatch them to chrome
 // os components.
 class ArcAccessibilityHelperBridge
-    : public ArcService,
+    : public KeyedService,
       public mojom::AccessibilityHelperHost,
       public InstanceHolder<mojom::AccessibilityHelperInstance>::Observer,
       public exo::WMHelper::ActivationObserver,
       public AXTreeSourceArc::Delegate,
       public ArcAppListPrefs::Observer {
  public:
-  explicit ArcAccessibilityHelperBridge(ArcBridgeService* bridge_service);
+  ArcAccessibilityHelperBridge(Profile* profile,
+                               ArcBridgeService* arc_bridge_service);
   ~ArcAccessibilityHelperBridge() override;
 
+  // KeyedService overrides.
+  void Shutdown() override;
+
   // InstanceHolder<mojom::AccessibilityHelperInstance>::Observer overrides.
   void OnInstanceReady() override;
 
@@ -73,8 +79,9 @@ class ArcAccessibilityHelperBridge
   void OnWindowActivated(aura::Window* gained_active,
                          aura::Window* lost_active) override;
 
+  Profile* const profile_;
+  ArcBridgeService* const arc_bridge_service_;
   mojo::Binding<mojom::AccessibilityHelperHost> binding_;
-
   std::map<std::string, std::unique_ptr<AXTreeSourceArc>> package_name_to_tree_;
   std::map<std::string, std::set<int32_t>> package_name_to_task_ids_;
   int32_t current_task_id_;

chrome/browser/chromeos/arc/accessibility/arc_accessibility_helper_bridge_factory.cc

+// 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.
+
+#include "chrome/browser/chromeos/arc/accessibility/arc_accessibility_helper_bridge_factory.h"
+
+#include "base/memory/singleton.h"
+#include "chrome/browser/chromeos/arc/accessibility/arc_accessibility_helper_bridge.h"
+#include "chrome/browser/profiles/profile.h"
+#include "chrome/browser/ui/app_list/arc/arc_app_list_prefs_factory.h"
+#include "components/arc/arc_service_manager.h"
+#include "components/keyed_service/content/browser_context_dependency_manager.h"
+
+namespace arc {
+
+// static
+ArcAccessibilityHelperBridge*
+ArcAccessibilityHelperBridgeFactory::GetForBrowserContext(
+    content::BrowserContext* context) {
+  return static_cast<ArcAccessibilityHelperBridge*>(
+      GetInstance()->GetServiceForBrowserContext(context, true /* create */));
+}
+
+// static
+ArcAccessibilityHelperBridgeFactory*
+ArcAccessibilityHelperBridgeFactory::GetInstance() {
+  return base::Singleton<ArcAccessibilityHelperBridgeFactory>::get();
+}
+
+ArcAccessibilityHelperBridgeFactory::ArcAccessibilityHelperBridgeFactory()
+    : BrowserContextKeyedServiceFactory(
+          "ArcAccessibilityHelperBridgeFactory",
+          BrowserContextDependencyManager::GetInstance()) {
+  // ArcAccessibilityHelperBridge needs to track task creation and destruction
+  // in the container, which are notified to ArcAppListPrefs via Mojo.
+  DependsOn(ArcAppListPrefsFactory::GetInstance());
+}
+
+ArcAccessibilityHelperBridgeFactory::~ArcAccessibilityHelperBridgeFactory() =
+    default;
+
+KeyedService* ArcAccessibilityHelperBridgeFactory::BuildServiceInstanceFor(
+    content::BrowserContext* context) const {
+  auto* arc_service_manager = arc::ArcServiceManager::Get();
+
+  // Practically, this is in testing case.
+  if (!arc_service_manager) {
+    VLOG(2) << "ArcServiceManager is not available.";
+    return nullptr;
+  }
+
+  if (arc_service_manager->browser_context() != context) {
+    VLOG(2) << "Non ARC allowed browser context.";
+    return nullptr;
+  }
+
+  return new ArcAccessibilityHelperBridge(
+      Profile::FromBrowserContext(context),
+      arc_service_manager->arc_bridge_service());
+}
+
+}  // namespace arc

chrome/browser/chromeos/arc/accessibility/arc_accessibility_helper_bridge_factory.h

+// 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.
+
+#ifndef CHROME_BROWSER_CHROMEOS_ARC_ACCESSIBILITY_ARC_ACCESSIBILITY_HELPER_BRIDGE_FACTORY_H_
+#define CHROME_BROWSER_CHROMEOS_ARC_ACCESSIBILITY_ARC_ACCESSIBILITY_HELPER_BRIDGE_FACTORY_H_
+
+#include "base/macros.h"
+#include "components/keyed_service/content/browser_context_keyed_service_factory.h"
+
+namespace base {
+template <typename T>
+struct DefaultSingletonTraits;
+}  // namespace base
+
+namespace arc {
+
+class ArcAccessibilityHelperBridge;
+
+// Factory for ArcAccessibilityHelperBridge.
+class ArcAccessibilityHelperBridgeFactory
+    : public BrowserContextKeyedServiceFactory {
+ public:
+  // Returns the ArcAccessibilityHelperBridge for the given |context|,
+  // or nullptr if |context| is not allowed to use ARC.
+  // If the instance is not yet created, this creates a new service instance.
+  static ArcAccessibilityHelperBridge* GetForBrowserContext(
+      content::BrowserContext* context);
+
+  // Return the factory instance.
+  static ArcAccessibilityHelperBridgeFactory* GetInstance();
+
+ private:
+  friend struct base::DefaultSingletonTraits<
+      ArcAccessibilityHelperBridgeFactory>;
+
+  ArcAccessibilityHelperBridgeFactory();
+  ~ArcAccessibilityHelperBridgeFactory() override;
+
+  // BrowserContextKeyedServiceFactory overrides:
+  KeyedService* BuildServiceInstanceFor(
+      content::BrowserContext* context) const override;
+
+  DISALLOW_COPY_AND_ASSIGN(ArcAccessibilityHelperBridgeFactory);
+};
+
+}  // namespace arc
+
+#endif  // CHROME_BROWSER_CHROMEOS_ARC_ACCESSIBILITY_ARC_ACCESSIBILITY_HELPER_BRIDGE_FACTORY_H_

chrome/browser/chromeos/arc/accessibility/arc_accessibility_helper_bridge_unittest.cc

@@ -8,10 +8,12 @@
 #include <utility>
 
 #include "base/command_line.h"
+#include "chrome/test/base/testing_profile.h"
 #include "chromeos/chromeos_switches.h"
 #include "components/arc/arc_bridge_service.h"
 #include "components/arc/common/accessibility_helper.mojom.h"
 #include "components/exo/wm_helper.h"
+#include "content/public/test/test_browser_thread_bundle.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "ui/display/display.h"
 #include "ui/display/manager/managed_display_info.h"
@@ -57,9 +59,20 @@ class ArcAccessibilityHelperBridgeTest : public testing::Test {
   void SetUp() override {
     wm_helper_ = base::MakeUnique<FakeWMHelper>();
     exo::WMHelper::SetInstance(wm_helper_.get());
+    testing_profile_ = base::MakeUnique<TestingProfile>();
     bridge_service_ = base::MakeUnique<ArcBridgeService>();
     accessibility_helper_bridge_ =
-        base::MakeUnique<ArcAccessibilityHelperBridge>(bridge_service_.get());
+        base::MakeUnique<ArcAccessibilityHelperBridge>(testing_profile_.get(),
+                                                       bridge_service_.get());
+  }
+
+  void TearDown() override {
+    accessibility_helper_bridge_->Shutdown();
+    accessibility_helper_bridge_.reset();
+    bridge_service_.reset();
+    testing_profile_.reset();
+    exo::WMHelper::SetInstance(nullptr);
+    wm_helper_.reset();
   }
 
   ArcAccessibilityHelperBridge* accessibility_helper_bridge() {
@@ -67,7 +80,9 @@ class ArcAccessibilityHelperBridgeTest : public testing::Test {
   }
 
  private:
+  content::TestBrowserThreadBundle thread_bundle_;
   std::unique_ptr<FakeWMHelper> wm_helper_;
+  std::unique_ptr<TestingProfile> testing_profile_;
   std::unique_ptr<ArcBridgeService> bridge_service_;
   std::unique_ptr<ArcAccessibilityHelperBridge> accessibility_helper_bridge_;
 

chrome/browser/chromeos/arc/arc_service_launcher.cc

@@ -10,7 +10,7 @@
 #include "base/logging.h"
 #include "base/memory/ptr_util.h"
 #include "chrome/browser/chromeos/app_mode/arc/arc_kiosk_app_service.h"
-#include "chrome/browser/chromeos/arc/accessibility/arc_accessibility_helper_bridge.h"
+#include "chrome/browser/chromeos/arc/accessibility/arc_accessibility_helper_bridge_factory.h"
 #include "chrome/browser/chromeos/arc/arc_play_store_enabled_preference_handler.h"
 #include "chrome/browser/chromeos/arc/arc_session_manager.h"
 #include "chrome/browser/chromeos/arc/arc_util.h"
@@ -94,8 +94,6 @@ void ArcServiceLauncher::Initialize() {
           base::Bind(ArcSession::Create, arc_bridge_service)));
 
   // List in lexicographical order.
-  arc_service_manager_->AddService(
-      base::MakeUnique<ArcAccessibilityHelperBridge>(arc_bridge_service));
   arc_service_manager_->AddService(
       base::MakeUnique<ArcAudioBridge>(arc_bridge_service));
   arc_service_manager_->AddService(
@@ -185,6 +183,7 @@ void ArcServiceLauncher::MaybeSetProfile(Profile* profile) {
     arc_session_manager_->Shutdown();
 
   arc_session_manager_->SetProfile(profile);
+  arc_service_manager_->set_browser_context(profile);
 }
 
 void ArcServiceLauncher::OnPrimaryUserProfilePrepared(Profile* profile) {
@@ -197,7 +196,11 @@ void ArcServiceLauncher::OnPrimaryUserProfilePrepared(Profile* profile) {
     return;
   }
 
-  // List in lexicographical order
+  // Instantiate ARC related BrowserContextKeyedService classes which need
+  // to be running at the beginning of the container run.
+  // List in lexicographical order.
+  ArcAccessibilityHelperBridgeFactory::GetForBrowserContext(profile);
+
   arc_service_manager_->AddService(base::MakeUnique<ArcBootPhaseMonitorBridge>(
       arc_service_manager_->arc_bridge_service(),
       multi_user_util::GetAccountIdFromProfile(profile)));

components/arc/arc_service_manager.h

@@ -18,6 +18,10 @@
 #include "components/arc/arc_service.h"
 #include "components/arc/intent_helper/local_activity_resolver.h"
 
+namespace content {
+class BrowserContext;
+}  // namespace content
+
 namespace arc {
 
 class ArcBridgeService;
@@ -63,6 +67,18 @@ class ArcServiceManager {
   ArcServiceManager();
   ~ArcServiceManager();
 
+  // Returns the current BrowserContext which ARC is allowed.
+  // This is workaround to split the dependency from chrome/.
+  // TODO(hidehiko): Remove this when we move IsArcAllowedForProfile() to
+  // components/arc.
+  content::BrowserContext* browser_context() { return browser_context_; }
+
+  // TODO(hidehiko): Remove this when we move IsArcAllowedForProfile() to
+  // components/arc. See browser_context() for details.
+  void set_browser_context(content::BrowserContext* browser_context) {
+    browser_context_ = browser_context;
+  }
+
   // |arc_bridge_service| can only be accessed on the thread that this
   // class was created on.
   ArcBridgeService* arc_bridge_service();
@@ -119,6 +135,17 @@ class ArcServiceManager {
   std::unordered_multimap<std::string, std::unique_ptr<ArcService>> services_;
   scoped_refptr<LocalActivityResolver> activity_resolver_;
 
+  // This holds the pointer to the BrowserContext (practically Profile)
+  // which is allowed to use ARC.
+  // This is set just before BrowserContextKeyedService classes are
+  // instantiated.
+  // So, in BrowserContextKeyedServiceFactory::BuildServiceInstanceFor(),
+  // given BrowserContext pointer can be compared to this to check if it is
+  // allowed to use ARC.
+  // TODO(hidehiko): Remove this when we move IsArcAllowedForProfile() to
+  // components/arc. See browser_context() for details.
+  content::BrowserContext* browser_context_ = nullptr;
+
   DISALLOW_COPY_AND_ASSIGN(ArcServiceManager);
 };