Record DemoMode.ActiveApp when ARC package name is delayed.

Bug: 929570
Change-Id: I027130d4301bd9d3a6c2e19f5ac56d9600c16af2
Reviewed-on: https://chromium-review.googlesource.com/c/1457920Reviewed-by: Dan Erat <derat@chromium.org>
Commit-Queue: Danan S <danan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#630547}

ash/metrics/demo_session_metrics_recorder.cc

@@ -98,27 +98,43 @@ DemoModeApp GetAppFromPackageName(const std::string& package_name) {
   return DemoModeApp::kOtherArcApp;
 }
 
+AppType GetAppType(const aura::Window* window) {
+  return static_cast<AppType>(window->GetProperty(aura::client::kAppType));
+}
+
+bool IsArcWindow(const aura::Window* window) {
+  return (GetAppType(window) == AppType::ARC_APP);
+}
+
 const std::string* GetArcPackageName(const aura::Window* window) {
+  DCHECK(IsArcWindow(window));
   return window->GetProperty(kArcPackageNameKey);
 }
 
-const ShelfID GetShelfID(const aura::Window* window) {
-  return ShelfID::Deserialize(window->GetProperty(kShelfIDKey));
+bool CanGetAppFromWindow(const aura::Window* window) {
+  // For ARC apps we can only get the App if the package
+  // name is not null.
+  if (IsArcWindow(window)) {
+    return GetArcPackageName(window) != nullptr;
+  }
+  // We can always get the App for non-ARC windows.
+  return true;
 }
 
-AppType GetAppType(const aura::Window* window) {
-  return static_cast<AppType>(window->GetProperty(aura::client::kAppType));
+const ShelfID GetShelfID(const aura::Window* window) {
+  return ShelfID::Deserialize(window->GetProperty(kShelfIDKey));
 }
 
 // Maps the app-like thing in |window| to a DemoModeApp value for metrics.
 DemoModeApp GetAppFromWindow(const aura::Window* window) {
+  DCHECK(CanGetAppFromWindow(window));
+
   AppType app_type = GetAppType(window);
   if (app_type == AppType::ARC_APP) {
     // The ShelfID app id isn't used to identify ARC++ apps since it's a hash of
     // both the package name and the activity.
     const std::string* package_name = GetArcPackageName(window);
-    return package_name ? GetAppFromPackageName(*package_name)
-                        : DemoModeApp::kOtherArcApp;
+    return GetAppFromPackageName(*package_name);
   }
 
   std::string app_id = GetShelfID(window).app_id;
@@ -150,6 +166,49 @@ DemoModeApp GetAppFromWindow(const aura::Window* window) {
 }  // namespace
 
 // Observes for changes in a window's ArcPackageName property for the purpose of
+// logging  of active app samples.
+class DemoSessionMetricsRecorder::ActiveAppArcPackageNameObserver
+    : public aura::WindowObserver {
+ public:
+  explicit ActiveAppArcPackageNameObserver(
+      DemoSessionMetricsRecorder* metrics_recorder)
+      : metrics_recorder_(metrics_recorder), scoped_observer_(this) {}
+
+  // aura::WindowObserver
+  void OnWindowPropertyChanged(aura::Window* window,
+                               const void* key,
+                               intptr_t old) override {
+    if (key != kArcPackageNameKey)
+      return;
+
+    const std::string* package_name = GetArcPackageName(window);
+
+    if (package_name) {
+      metrics_recorder_->RecordActiveAppSample(
+          GetAppFromPackageName(*package_name));
+    } else {
+      VLOG(1) << "Got null ARC package name";
+    }
+
+    scoped_observer_.Remove(window);
+  }
+
+  void OnWindowDestroyed(aura::Window* window) override {
+    if (scoped_observer_.IsObserving(window))
+      scoped_observer_.Remove(window);
+  }
+
+  void ObserveWindow(aura::Window* window) { scoped_observer_.Add(window); }
+
+ private:
+  DemoSessionMetricsRecorder* metrics_recorder_;
+  ScopedObserver<aura::Window, ActiveAppArcPackageNameObserver>
+      scoped_observer_;
+
+  DISALLOW_COPY_AND_ASSIGN(ActiveAppArcPackageNameObserver);
+};
+
+// Observes changes in a window's ArcPackageName property for the purpose of
 // logging of unique launches of ARC apps.
 class DemoSessionMetricsRecorder::UniqueAppsLaunchedArcPackageNameObserver
     : public aura::WindowObserver {
@@ -167,21 +226,22 @@ class DemoSessionMetricsRecorder::UniqueAppsLaunchedArcPackageNameObserver
 
     const std::string* package_name = GetArcPackageName(window);
 
-    if (package_name != nullptr) {
+    if (package_name) {
       metrics_recorder_->RecordAppLaunch(*package_name, AppType::ARC_APP);
     } else {
       VLOG(1) << "Got null ARC package name";
     }
 
-    window->RemoveObserver(this);
+    scoped_observer_.Remove(window);
   }
 
-  void ObserveWindow(aura::Window* window) { scoped_observer_.Add(window); }
-
   void OnWindowDestroyed(aura::Window* window) override {
-    window->RemoveObserver(this);
+    if (scoped_observer_.IsObserving(window))
+      scoped_observer_.Remove(window);
   }
 
+  void ObserveWindow(aura::Window* window) { scoped_observer_.Add(window); }
+
  private:
   DemoSessionMetricsRecorder* metrics_recorder_;
   ScopedObserver<aura::Window, UniqueAppsLaunchedArcPackageNameObserver>
@@ -195,7 +255,9 @@ DemoSessionMetricsRecorder::DemoSessionMetricsRecorder(
     : timer_(std::move(timer)),
       observer_(this),
       unique_apps_arc_package_name_observer_(
-          std::make_unique<UniqueAppsLaunchedArcPackageNameObserver>(this)) {
+          std::make_unique<UniqueAppsLaunchedArcPackageNameObserver>(this)),
+      active_app_arc_package_name_observer_(
+          std::make_unique<ActiveAppArcPackageNameObserver>(this)) {
   // Outside of tests, use a normal repeating timer.
   if (!timer_.get())
     timer_ = std::make_unique<base::RepeatingTimer>();
@@ -311,6 +373,10 @@ void DemoSessionMetricsRecorder::StartRecording() {
                 &DemoSessionMetricsRecorder::TakeSampleOrPause);
 }
 
+void DemoSessionMetricsRecorder::RecordActiveAppSample(DemoModeApp app) {
+  unreported_samples_.push_back(app);
+}
+
 void DemoSessionMetricsRecorder::TakeSampleOrPause() {
   // After enough inactive time, assume the user left.
   if (++periods_since_activity_ > kMaxPeriodsWithoutActivity) {
@@ -320,15 +386,21 @@ void DemoSessionMetricsRecorder::TakeSampleOrPause() {
     return;
   }
 
-  const aura::Window* window =
-      Shell::Get()->activation_client()->GetActiveWindow();
+  aura::Window* window = Shell::Get()->activation_client()->GetActiveWindow();
   if (!window)
     return;
 
+  // If there is no ARC package name available, set up a listener
+  // to be informed when it is available.
+  if (IsArcWindow(window) && !CanGetAppFromWindow(window)) {
+    active_app_arc_package_name_observer_->ObserveWindow(window);
+    return;
+  }
+
   DemoModeApp app = window->type() == aura::client::WINDOW_TYPE_NORMAL
                         ? GetAppFromWindow(window)
                         : DemoModeApp::kOtherWindow;
-  unreported_samples_.push_back(app);
+  RecordActiveAppSample(app);
 }
 
 void DemoSessionMetricsRecorder::ReportSamples() {

ash/metrics/demo_session_metrics_recorder.h

@@ -91,6 +91,9 @@ class ASH_EXPORT DemoSessionMetricsRecorder
   // Emits histograms for recorded samples.
   void ReportSamples();
 
+  // Records |app| as being seen while sampling all active apps.
+  void RecordActiveAppSample(DemoModeApp app);
+
   // Indicates whether the specified app_id should be recorded for
   // the unique-apps-launched stat.
   bool ShouldRecordAppLaunch(const std::string& app_id);
@@ -123,11 +126,15 @@ class ASH_EXPORT DemoSessionMetricsRecorder
   ScopedObserver<ui::UserActivityDetector, DemoSessionMetricsRecorder>
       observer_;
 
+  class ActiveAppArcPackageNameObserver;
   class UniqueAppsLaunchedArcPackageNameObserver;
 
   std::unique_ptr<UniqueAppsLaunchedArcPackageNameObserver>
       unique_apps_arc_package_name_observer_;
 
+  std::unique_ptr<ActiveAppArcPackageNameObserver>
+      active_app_arc_package_name_observer_;
+
   DISALLOW_COPY_AND_ASSIGN(DemoSessionMetricsRecorder);
 };
 

ash/metrics/demo_session_metrics_recorder_unittest.cc

@@ -239,6 +239,54 @@ TEST_F(DemoSessionMetricsRecorderTest, AppTypes) {
   histogram_tester_->ExpectTotalCount("DemoMode.ActiveApp", 10);
 }
 
+// Verify samples are correct when multiple windows types become active.
+TEST_F(DemoSessionMetricsRecorderTest, ActiveAppAfterDelayedArcPackageName) {
+  // Create an ARC window with an empty package name.
+  std::unique_ptr<aura::Window> arc_window = CreateArcWindow("");
+
+  wm::ActivateWindow(arc_window.get());
+  FireTimer();
+  SendUserActivity();
+
+  // There should be no app activity recorded yet, because there was
+  // no package name in the ARC window.
+  histogram_tester_->ExpectTotalCount("DemoMode.ActiveApp", 0);
+
+  // Set the package name after window creation/activation.
+  arc_window->SetProperty(kArcPackageNameKey,
+                          new std::string("com.google.Photos"));
+
+  // Trigger sample reporting by sending user activity.
+  SendUserActivity();
+
+  histogram_tester_->ExpectBucketCount(
+      "DemoMode.ActiveApp",
+      DemoSessionMetricsRecorder::DemoModeApp::kGooglePhotos, 1);
+
+  // Set the package name again.  The count shouldn't change because
+  // after getting the package name once, we stop observing the
+  // window.
+  arc_window->SetProperty(kArcPackageNameKey,
+                          new std::string("com.google.Photos"));
+  // Trigger sample reporting by sending user activity.
+  SendUserActivity();
+
+  histogram_tester_->ExpectBucketCount(
+      "DemoMode.ActiveApp",
+      DemoSessionMetricsRecorder::DemoModeApp::kGooglePhotos, 1);
+
+  // Delete the window.
+  arc_window.reset();
+
+  // Trigger sample reporting by sending user activity.
+  SendUserActivity();
+
+  // The count should not be affected.
+  histogram_tester_->ExpectBucketCount(
+      "DemoMode.ActiveApp",
+      DemoSessionMetricsRecorder::DemoModeApp::kGooglePhotos, 1);
+}
+
 // Verify popup windows are categorized as kOtherWindow.
 TEST_F(DemoSessionMetricsRecorderTest, PopupWindows) {
   std::unique_ptr<aura::Window> chrome_app_window =
@@ -516,6 +564,14 @@ TEST_F(DemoSessionMetricsRecorderTest,
   arc_window_1->SetProperty(kArcPackageNameKey,
                             new std::string("com.google.Photos"));
 
+  // Set the package name again. This shouldn't cause a double-recording
+  // of the stat.
+  arc_window_1->SetProperty(kArcPackageNameKey,
+                            new std::string("com.google.Photos"));
+
+  // Delete the window.
+  arc_window_1.reset();
+
   std::unique_ptr<aura::Window> arc_window_2 =
       CreateArcWindow("com.google.Maps");
   wm::ActivateWindow(arc_window_2.get());