Use wm::ActivationChangeObserver for ArcMetricsService

Originally aura::client::FocusChangeObserver was used, but the
window supplied in the OnWindowFocused() callback is not always the
top-level window, which makes it tricky to determine if it's an
ARC++ window.
By using ActivationChangeObserver the top-level window is supplied
in the OnWindowActivated() callback.

Bug: 887798
Change-Id: I8f90184c1992254f96793739799707b0527d1fd6
Reviewed-on: https://chromium-review.googlesource.com/1235488
Reviewed-by: Yusuke Sato <yusukes@chromium.org>
Reviewed-by: Maajid <maajid@chromium.org>
Reviewed-by: Nico Weber <thakis@chromium.org>
Commit-Queue: Shao-Chuan Lee <shaochuan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#593100}

components/arc/metrics/DEPS

@@ -1,3 +1,4 @@
 include_rules = [
   "+ui/aura",
+  "+ui/wm/public",
 ]

components/arc/metrics/arc_metrics_service.cc

@@ -56,22 +56,21 @@ class ArcWindowDelegateImpl : public ArcMetricsService::ArcWindowDelegate {
 
   ~ArcWindowDelegateImpl() override = default;
 
-  bool IsActiveArcAppWindow(const aura::Window* window) const override {
+  bool IsArcAppWindow(const aura::Window* window) const override {
-    aura::Window* active = exo::WMHelper::GetInstance()->GetActiveWindow();
+    return arc::IsArcAppWindow(window);
-    return window == active && IsArcAppWindow(window);
   }
 
-  void RegisterFocusChangeObserver() override {
+  void RegisterActivationChangeObserver() override {
     // If WMHelper doesn't exist, do nothing. This occurs in tests.
     if (exo::WMHelper::HasInstance())
-      exo::WMHelper::GetInstance()->AddFocusObserver(service_);
+      exo::WMHelper::GetInstance()->AddActivationObserver(service_);
   }
 
-  void UnregisterFocusChangeObserver() override {
+  void UnregisterActivationChangeObserver() override {
     // If WMHelper is already destroyed, do nothing.
     // TODO(crbug.com/748380): Fix shutdown order.
     if (exo::WMHelper::HasInstance())
-      exo::WMHelper::GetInstance()->RemoveFocusObserver(service_);
+      exo::WMHelper::GetInstance()->RemoveActivationObserver(service_);
   }
 
  private:
@@ -122,14 +121,14 @@ ArcMetricsService::ArcMetricsService(content::BrowserContext* context,
       weak_ptr_factory_(this) {
   arc_bridge_service_->metrics()->SetHost(this);
   arc_bridge_service_->process()->AddObserver(&process_observer_);
-  arc_window_delegate_->RegisterFocusChangeObserver();
+  arc_window_delegate_->RegisterActivationChangeObserver();
 }
 
 ArcMetricsService::~ArcMetricsService() {
   DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
   arc_bridge_service_->process()->RemoveObserver(&process_observer_);
   arc_bridge_service_->metrics()->SetHost(nullptr);
-  arc_window_delegate_->UnregisterFocusChangeObserver();
+  arc_window_delegate_->UnregisterActivationChangeObserver();
 }
 
 void ArcMetricsService::SetArcWindowDelegateForTesting(
@@ -258,9 +257,11 @@ void ArcMetricsService::RecordNativeBridgeUMA() {
   UMA_HISTOGRAM_ENUMERATION("Arc.NativeBridge", native_bridge_type_);
 }
 
-void ArcMetricsService::OnWindowFocused(aura::Window* gained_focus,
+void ArcMetricsService::OnWindowActivated(
-                                        aura::Window* lost_focus) {
+    wm::ActivationChangeObserver::ActivationReason reason,
-  if (!arc_window_delegate_->IsActiveArcAppWindow(gained_focus))
+    aura::Window* gained_active,
+    aura::Window* lost_active) {
+  if (!arc_window_delegate_->IsArcAppWindow(gained_active))
     return;
   UMA_HISTOGRAM_ENUMERATION(
       "Arc.UserInteraction",

components/arc/metrics/arc_metrics_service.h

@@ -17,7 +17,7 @@
 #include "components/arc/common/process.mojom.h"
 #include "components/arc/connection_observer.h"
 #include "components/keyed_service/core/keyed_service.h"
-#include "ui/aura/client/focus_change_observer.h"
+#include "ui/wm/public/activation_change_observer.h"
 
 namespace aura {
 class Window;
@@ -33,7 +33,7 @@ class ArcBridgeService;
 
 // Collects information from other ArcServices and send UMA metrics.
 class ArcMetricsService : public KeyedService,
-                          public aura::client::FocusChangeObserver,
+                          public wm::ActivationChangeObserver,
                           public mojom::MetricsHost {
  public:
   // These values are persisted to logs, and should therefore never be
@@ -55,10 +55,10 @@ class ArcMetricsService : public KeyedService,
   class ArcWindowDelegate {
    public:
     virtual ~ArcWindowDelegate() = default;
-    // Returns whether |window| is an active ARC window.
+    // Returns whether |window| is an ARC window.
-    virtual bool IsActiveArcAppWindow(const aura::Window* window) const = 0;
+    virtual bool IsArcAppWindow(const aura::Window* window) const = 0;
-    virtual void RegisterFocusChangeObserver() = 0;
+    virtual void RegisterActivationChangeObserver() = 0;
-    virtual void UnregisterFocusChangeObserver() = 0;
+    virtual void UnregisterActivationChangeObserver() = 0;
   };
 
   // Sets the fake ArcWindowDelegate for testing.
@@ -89,10 +89,11 @@ class ArcMetricsService : public KeyedService,
   // container or as UNKNOWN if the value has not been recieved yet.
   void RecordNativeBridgeUMA();
 
-  // aura::client::FocusChangeObserver overrides.
+  // wm::ActivationChangeObserver overrides.
   // Records to UMA when a user has interacted with an ARC app window.
-  void OnWindowFocused(aura::Window* gained_focus,
+  void OnWindowActivated(wm::ActivationChangeObserver::ActivationReason reason,
-                       aura::Window* lost_focus) override;
+                         aura::Window* gained_active,
+                         aura::Window* lost_active) override;
 
   NativeBridgeType native_bridge_type_for_testing() const {
     return native_bridge_type_;

components/arc/metrics/arc_metrics_service_unittest.cc

@@ -36,12 +36,12 @@ class FakeArcWindowDelegate : public ArcMetricsService::ArcWindowDelegate {
   FakeArcWindowDelegate() = default;
   ~FakeArcWindowDelegate() override = default;
 
-  bool IsActiveArcAppWindow(const aura::Window* window) const override {
+  bool IsArcAppWindow(const aura::Window* window) const override {
     return focused_window_id_ == arc_window_id_;
   }
 
-  void RegisterFocusChangeObserver() override {}
+  void RegisterActivationChangeObserver() override {}
-  void UnregisterFocusChangeObserver() override {}
+  void UnregisterActivationChangeObserver() override {}
 
   std::unique_ptr<aura::Window> CreateFakeArcWindow() {
     const int id = next_id_++;
@@ -322,7 +322,9 @@ TEST_F(ArcMetricsServiceTest, RecordArcWindowFocusAction) {
   base::HistogramTester tester;
   fake_arc_window_delegate()->FocusWindow(fake_arc_window());
 
-  service()->OnWindowFocused(fake_arc_window(), nullptr);
+  service()->OnWindowActivated(
+      wm::ActivationChangeObserver::ActivationReason::INPUT_EVENT,
+      fake_arc_window(), nullptr);
 
   tester.ExpectBucketCount(
       "Arc.UserInteraction",
@@ -334,14 +336,18 @@ TEST_F(ArcMetricsServiceTest, RecordNothingNonArcWindowFocusAction) {
 
   // Focus an ARC window once so that the histogram is created.
   fake_arc_window_delegate()->FocusWindow(fake_arc_window());
-  service()->OnWindowFocused(fake_arc_window(), nullptr);
+  service()->OnWindowActivated(
+      wm::ActivationChangeObserver::ActivationReason::INPUT_EVENT,
+      fake_arc_window(), nullptr);
   tester.ExpectBucketCount(
       "Arc.UserInteraction",
       static_cast<int>(UserInteractionType::APP_CONTENT_WINDOW_INTERACTION), 1);
 
   // Focusing a non-ARC window should not increase the bucket count.
   fake_arc_window_delegate()->FocusWindow(fake_non_arc_window());
-  service()->OnWindowFocused(fake_non_arc_window(), nullptr);
+  service()->OnWindowActivated(
+      wm::ActivationChangeObserver::ActivationReason::INPUT_EVENT,
+      fake_non_arc_window(), nullptr);
 
   tester.ExpectBucketCount(
       "Arc.UserInteraction",