Changed when the UMA metric Ash.NumberOfVisibleWindowsInPrimaryDisplay is recorded.

Ash.NumberOfVisibleWindowsInPrimaryDisplay was incorrectly being recorded at the
login and lock screen.

TEST=UserMetricsRecorderTest.VerifyIsUserInActiveDesktopEnvironmentValues
TEST=UserMetricsRecorderTest.VerifyStatsRecordedWhenUserNotInActiveDesktopEnvironment
TEST=UserMetricsRecorderTest.VerifyStatsRecordedWhenUserInActiveDesktopEnvironment
TEST=UserMetricsRecorderTest.VerifyStatsRecordedByRecordPeriodicMetrics

BUG=477538

Review URL: https://codereview.chromium.org/1093483002

Cr-Commit-Position: refs/heads/master@{#327088}

ash/ash.gyp

@@ -717,6 +717,8 @@
       'test/test_volume_control_delegate.h',
       'test/ui_controls_factory_ash.cc',
       'test/ui_controls_factory_ash.h',
+      'test/user_metrics_recorder_test_api.cc',
+      'test/user_metrics_recorder_test_api.h',
       'test/virtual_keyboard_test_helper.cc',
       'test/virtual_keyboard_test_helper.h',
     ],
@@ -791,6 +793,7 @@
       'keyboard_overlay/keyboard_overlay_delegate_unittest.cc',
       'keyboard_overlay/keyboard_overlay_view_unittest.cc',
       'magnifier/magnification_controller_unittest.cc',
+      'metrics/user_metrics_recorder_unittest.cc',
       'popup_message_unittest.cc',
       'root_window_controller_unittest.cc',
       'screen_util_unittest.cc',

ash/metrics/user_metrics_recorder.cc

@@ -4,11 +4,13 @@
 
 #include "ash/metrics/user_metrics_recorder.h"
 
+#include "ash/session/session_state_delegate.h"
 #include "ash/shelf/shelf_layout_manager.h"
 #include "ash/shelf/shelf_view.h"
 #include "ash/shelf/shelf_widget.h"
 #include "ash/shell.h"
 #include "ash/shell_window_ids.h"
+#include "ash/system/tray/system_tray_delegate.h"
 #include "ash/wm/window_state.h"
 #include "base/metrics/histogram.h"
 #include "base/metrics/user_metrics.h"
@@ -64,6 +66,31 @@ ActiveWindowStateType GetActiveWindowState() {
   return active_window_state_type;
 }
 
+// Returns true if kiosk mode is active.
+bool IsKioskModeActive() {
+  return Shell::GetInstance()->system_tray_delegate()->GetUserLoginStatus() ==
+         user::LOGGED_IN_KIOSK_APP;
+}
+
+// Returns true if there is an active user and their session isn't currently
+// locked.
+bool IsUserActive() {
+  switch (Shell::GetInstance()->system_tray_delegate()->GetUserLoginStatus()) {
+    case user::LOGGED_IN_NONE:
+    case user::LOGGED_IN_LOCKED:
+      return false;
+    case user::LOGGED_IN_USER:
+    case user::LOGGED_IN_OWNER:
+    case user::LOGGED_IN_GUEST:
+    case user::LOGGED_IN_PUBLIC:
+    case user::LOGGED_IN_SUPERVISED:
+    case user::LOGGED_IN_KIOSK_APP:
+      return true;
+  }
+  NOTREACHED();
+  return false;
+}
+
 // Array of window container ids that contain visible windows to be counted for
 // UMA statistics. Note the containers are ordered from top most visible
 // container to the lowest to allow the |GetNumVisibleWindows| method to short
@@ -124,10 +151,12 @@ int GetNumVisibleWindowsInPrimaryDisplay() {
 }  // namespace
 
 UserMetricsRecorder::UserMetricsRecorder() {
-  timer_.Start(FROM_HERE,
-               base::TimeDelta::FromSeconds(kAshPeriodicMetricsTimeInSeconds),
-               this,
-               &UserMetricsRecorder::RecordPeriodicMetrics);
+  StartTimer();
+}
+
+UserMetricsRecorder::UserMetricsRecorder(bool record_periodic_metrics) {
+  if (record_periodic_metrics)
+    StartTimer();
 }
 
 UserMetricsRecorder::~UserMetricsRecorder() {
@@ -525,7 +554,10 @@ void UserMetricsRecorder::RecordUserMetricsAction(UserMetricsAction action) {
 void UserMetricsRecorder::RecordPeriodicMetrics() {
   ShelfLayoutManager* manager =
       ShelfLayoutManager::ForShelf(Shell::GetPrimaryRootWindow());
+  // TODO(bruthig): Investigating whether the check for |manager| is necessary
+  // and add tests if it is.
   if (manager) {
+    // TODO(bruthig): Consider tracking the time spent in each alignment.
     UMA_HISTOGRAM_ENUMERATION("Ash.ShelfAlignmentOverTime",
                               manager->SelectValueForShelfAlignment(
                                   SHELF_ALIGNMENT_UMA_ENUM_VALUE_BOTTOM,
@@ -535,12 +567,28 @@ void UserMetricsRecorder::RecordPeriodicMetrics() {
                               SHELF_ALIGNMENT_UMA_ENUM_VALUE_COUNT);
   }
 
-  UMA_HISTOGRAM_COUNTS_100("Ash.NumberOfVisibleWindowsInPrimaryDisplay",
-                           GetNumVisibleWindowsInPrimaryDisplay());
+  if (IsUserInActiveDesktopEnvironment()) {
+    UMA_HISTOGRAM_COUNTS_100("Ash.NumberOfVisibleWindowsInPrimaryDisplay",
+                             GetNumVisibleWindowsInPrimaryDisplay());
+  }
 
+  // TODO(bruthig): Find out if this should only be logged when the user is
+  // active.
+  // TODO(bruthig): Consider tracking how long a particular type of window is
+  // active at a time.
   UMA_HISTOGRAM_ENUMERATION("Ash.ActiveWindowShowTypeOverTime",
                             GetActiveWindowState(),
                             ACTIVE_WINDOW_STATE_TYPE_COUNT);
 }
 
+bool UserMetricsRecorder::IsUserInActiveDesktopEnvironment() const {
+  return IsUserActive() && !IsKioskModeActive();
+}
+
+void UserMetricsRecorder::StartTimer() {
+  timer_.Start(FROM_HERE,
+               base::TimeDelta::FromSeconds(kAshPeriodicMetricsTimeInSeconds),
+               this, &UserMetricsRecorder::RecordPeriodicMetrics);
+}
+
 }  // namespace ash

ash/metrics/user_metrics_recorder.h

@@ -10,6 +10,10 @@
 
 namespace ash {
 
+namespace test {
+class UserMetricsRecorderTestAPI;
+}
+
 enum UserMetricsAction {
   UMA_ACCEL_EXIT_FIRST_Q,
   UMA_ACCEL_EXIT_SECOND_Q,
@@ -129,14 +133,40 @@ enum UserMetricsAction {
 // (RecordUserMetricsAction) are passed through the UserMetricsRecorder.
 class ASH_EXPORT UserMetricsRecorder {
  public:
+  // Creates a UserMetricsRecorder that records metrics periodically. Equivalent
+  // to calling UserMetricsRecorder(true).
   UserMetricsRecorder();
-  ~UserMetricsRecorder();
 
+  virtual ~UserMetricsRecorder();
+
+  // Records an Ash owned user action.
   void RecordUserMetricsAction(ash::UserMetricsAction action);
+
  private:
+  friend class test::UserMetricsRecorderTestAPI;
+
+  // Creates a UserMetricsRecorder and will only record periodic metrics if
+  // |record_periodic_metrics| is true. This is used by tests that do not want
+  // the timer to be started.
+  // TODO(bruthig): Add a constructor that accepts a base::RepeatingTimer so
+  // that tests can inject a test double that can be controlled by the test. The
+  // missing piece is a suitable base::RepeatingTimer test double.
+  explicit UserMetricsRecorder(bool record_periodic_metrics);
+
+  // Records UMA metrics. Invoked periodically by the |timer_|.
   void RecordPeriodicMetrics();
 
+  // Returns true if the user's session is active and they are in a desktop
+  // environment.
+  bool IsUserInActiveDesktopEnvironment() const;
+
+  // Starts the |timer_| and binds it to |RecordPeriodicMetrics|.
+  void StartTimer();
+
+  // The periodic timer that triggers metrics to be recorded.
   base::RepeatingTimer<UserMetricsRecorder> timer_;
+
+  DISALLOW_COPY_AND_ASSIGN(UserMetricsRecorder);
 };
 
 }  // namespace ash

ash/metrics/user_metrics_recorder_unittest.cc

+// Copyright 2015 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 "ash/metrics/user_metrics_recorder.h"
+
+#include "ash/shell.h"
+#include "ash/system/user/login_status.h"
+#include "ash/test/ash_test_base.h"
+#include "ash/test/test_system_tray_delegate.h"
+#include "ash/test/user_metrics_recorder_test_api.h"
+#include "base/memory/scoped_ptr.h"
+#include "base/test/histogram_tester.h"
+#include "ui/aura/window.h"
+
+namespace ash {
+namespace {
+
+const char kAsh_NumberOfVisibleWindowsInPrimaryDisplay[] =
+    "Ash.NumberOfVisibleWindowsInPrimaryDisplay";
+
+const char kAsh_ActiveWindowShowTypeOverTime[] =
+    "Ash.ActiveWindowShowTypeOverTime";
+
+}  // namespace
+
+// Test fixture for the UserMetricsRecorder class.
+class UserMetricsRecorderTest : public test::AshTestBase {
+ public:
+  UserMetricsRecorderTest();
+  ~UserMetricsRecorderTest() override;
+
+  // test::AshTestBase:
+  void SetUp() override;
+  void TearDown() override;
+
+  // Sets the user login status.
+  void SetLoginStatus(user::LoginStatus login_status);
+
+  // Sets the current user session to be active or inactive in a desktop
+  // environment.
+  void SetUserInActiveDesktopEnvironment(bool is_active);
+
+  test::UserMetricsRecorderTestAPI* user_metrics_recorder_test_api() {
+    return user_metrics_recorder_test_api_.get();
+  }
+
+  base::HistogramTester& histograms() { return histograms_; }
+
+ private:
+  // Test API to access private members of the test target.
+  scoped_ptr<test::UserMetricsRecorderTestAPI> user_metrics_recorder_test_api_;
+
+  // Histogram value verifier.
+  base::HistogramTester histograms_;
+
+  // The active SystemTrayDelegate. Not owned.
+  test::TestSystemTrayDelegate* test_system_tray_delegate_;
+
+  DISALLOW_COPY_AND_ASSIGN(UserMetricsRecorderTest);
+};
+
+UserMetricsRecorderTest::UserMetricsRecorderTest() {
+}
+
+UserMetricsRecorderTest::~UserMetricsRecorderTest() {
+}
+
+void UserMetricsRecorderTest::SetUp() {
+  test::AshTestBase::SetUp();
+  user_metrics_recorder_test_api_.reset(new test::UserMetricsRecorderTestAPI());
+  test_system_tray_delegate_ = GetSystemTrayDelegate();
+}
+
+void UserMetricsRecorderTest::TearDown() {
+  test_system_tray_delegate_ = nullptr;
+  test::AshTestBase::TearDown();
+}
+
+void UserMetricsRecorderTest::SetLoginStatus(user::LoginStatus login_status) {
+  test_system_tray_delegate_->SetLoginStatus(login_status);
+}
+
+void UserMetricsRecorderTest::SetUserInActiveDesktopEnvironment(
+    bool is_active) {
+  if (is_active) {
+    SetLoginStatus(user::LOGGED_IN_USER);
+    ASSERT_TRUE(
+        user_metrics_recorder_test_api()->IsUserInActiveDesktopEnvironment());
+  } else {
+    SetLoginStatus(user::LOGGED_IN_LOCKED);
+    ASSERT_FALSE(
+        user_metrics_recorder_test_api()->IsUserInActiveDesktopEnvironment());
+  }
+}
+
+// Verifies the return value of IsUserInActiveDesktopEnvironment() for the
+// different login status values.
+TEST_F(UserMetricsRecorderTest, VerifyIsUserInActiveDesktopEnvironmentValues) {
+  SetLoginStatus(user::LOGGED_IN_NONE);
+  EXPECT_FALSE(
+      user_metrics_recorder_test_api()->IsUserInActiveDesktopEnvironment());
+
+  SetLoginStatus(user::LOGGED_IN_LOCKED);
+  EXPECT_FALSE(
+      user_metrics_recorder_test_api()->IsUserInActiveDesktopEnvironment());
+
+  SetLoginStatus(user::LOGGED_IN_USER);
+  EXPECT_TRUE(
+      user_metrics_recorder_test_api()->IsUserInActiveDesktopEnvironment());
+
+  SetLoginStatus(user::LOGGED_IN_OWNER);
+  EXPECT_TRUE(
+      user_metrics_recorder_test_api()->IsUserInActiveDesktopEnvironment());
+
+  SetLoginStatus(user::LOGGED_IN_GUEST);
+  EXPECT_TRUE(
+      user_metrics_recorder_test_api()->IsUserInActiveDesktopEnvironment());
+
+  SetLoginStatus(user::LOGGED_IN_PUBLIC);
+  EXPECT_TRUE(
+      user_metrics_recorder_test_api()->IsUserInActiveDesktopEnvironment());
+
+  SetLoginStatus(user::LOGGED_IN_SUPERVISED);
+  EXPECT_TRUE(
+      user_metrics_recorder_test_api()->IsUserInActiveDesktopEnvironment());
+
+  SetLoginStatus(user::LOGGED_IN_KIOSK_APP);
+  EXPECT_FALSE(
+      user_metrics_recorder_test_api()->IsUserInActiveDesktopEnvironment());
+}
+
+// Verifies that the IsUserInActiveDesktopEnvironment() dependent stats are not
+// recorded when a user is not active in a desktop environment.
+TEST_F(UserMetricsRecorderTest,
+       VerifyStatsRecordedWhenUserNotInActiveDesktopEnvironment) {
+  SetUserInActiveDesktopEnvironment(false);
+  user_metrics_recorder_test_api()->RecordPeriodicMetrics();
+
+  histograms().ExpectTotalCount(kAsh_NumberOfVisibleWindowsInPrimaryDisplay, 0);
+}
+
+// Verifies that the IsUserInActiveDesktopEnvironment() dependent stats are
+// recorded when a user is active in a desktop environment.
+TEST_F(UserMetricsRecorderTest,
+       VerifyStatsRecordedWhenUserInActiveDesktopEnvironment) {
+  SetUserInActiveDesktopEnvironment(true);
+  user_metrics_recorder_test_api()->RecordPeriodicMetrics();
+
+  histograms().ExpectTotalCount(kAsh_NumberOfVisibleWindowsInPrimaryDisplay, 1);
+}
+
+// Verifies recording of stats which are always recorded by
+// RecordPeriodicMetrics.
+TEST_F(UserMetricsRecorderTest, VerifyStatsRecordedByRecordPeriodicMetrics) {
+  SetUserInActiveDesktopEnvironment(true);
+  user_metrics_recorder_test_api()->RecordPeriodicMetrics();
+
+  histograms().ExpectTotalCount(kAsh_ActiveWindowShowTypeOverTime, 1);
+}
+
+}  // namespace ash

ash/test/user_metrics_recorder_test_api.cc

+// Copyright 2015 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 "ash/test/user_metrics_recorder_test_api.h"
+
+namespace ash {
+namespace test {
+
+UserMetricsRecorderTestAPI::UserMetricsRecorderTestAPI()
+    : user_metrics_recorder_(false) {
+}
+
+UserMetricsRecorderTestAPI::~UserMetricsRecorderTestAPI() {
+}
+
+void UserMetricsRecorderTestAPI::RecordPeriodicMetrics() {
+  user_metrics_recorder_.RecordPeriodicMetrics();
+}
+
+bool UserMetricsRecorderTestAPI::IsUserInActiveDesktopEnvironment() const {
+  return user_metrics_recorder_.IsUserInActiveDesktopEnvironment();
+}
+
+}  // namespace test
+}  // namespace ash

ash/test/user_metrics_recorder_test_api.h

+// Copyright 2015 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 ASH_TEST_USER_METRICS_RECORDER_TEST_API_H_
+#define ASH_TEST_USER_METRICS_RECORDER_TEST_API_H_
+
+#include "ash/metrics/user_metrics_recorder.h"
+#include "base/macros.h"
+#include "base/memory/scoped_ptr.h"
+
+namespace ash {
+namespace test {
+
+// Test API to access internals of the UserMetricsRecorder class.
+class UserMetricsRecorderTestAPI {
+ public:
+  UserMetricsRecorderTestAPI();
+  ~UserMetricsRecorderTestAPI();
+
+  // Accessor to UserMetricsRecorder::RecordPeriodicMetrics().
+  void RecordPeriodicMetrics();
+
+  // Accessor to UserMetricsRecorder::IsUserInActiveDesktopEnvironment().
+  bool IsUserInActiveDesktopEnvironment() const;
+
+ private:
+  // The UserMetricsRecorder that |this| is providing internal access to.
+  UserMetricsRecorder user_metrics_recorder_;
+
+  DISALLOW_COPY_AND_ASSIGN(UserMetricsRecorderTestAPI);
+};
+
+}  // namespace test
+}  // namespace ash
+
+#endif  // ASH_TEST_USER_METRICS_RECORDER_TEST_API_H_