Changed when the UMA metric Ash.NumberOfVisibleWindowsInPrimaryDisplay is recorded.

ash/metrics/user_metrics_recorder.cc

Index: ash/metrics/user_metrics_recorder.cc
diff --git a/ash/metrics/user_metrics_recorder.cc b/ash/metrics/user_metrics_recorder.cc
index c1c9e631aeaeff5ec581bd8a35b96c74bd6f5340..a1ff9b321fa7aa007bde7bf9ed189993b3e2c57b 100644
--- a/ash/metrics/user_metrics_recorder.cc
+++ b/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"
@@ -124,10 +126,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() {
@@ -529,12 +533,30 @@ void UserMetricsRecorder::RecordPeriodicMetrics() {
                               SHELF_ALIGNMENT_UMA_ENUM_VALUE_COUNT);
   }
 
-  UMA_HISTOGRAM_COUNTS_100("Ash.NumberOfVisibleWindowsInPrimaryDisplay",
-                           GetNumVisibleWindowsInPrimaryDisplay());
+  if (UserIsActive()) {

[pkotwicz, 2015/04/17 13:45:38]

Should we also ignore kiosk mode? In kiosk mode, a user cannot change
GetNumVisibleWindowsInPrimaryDisplay(). (Unless it is possible to open popup
windows in kiosk mode?)

[bruthig, 2015/04/17 14:41:23]

Good point, I agree it should probably not be logged during kiosk mode either.  

tdanderson@ do you have any input here?

[tdanderson, 2015/04/17 17:59:57]

Agreed, let's ignore kiosk mode.

[bruthig, 2015/04/21 14:34:05]

Done.

+    UMA_HISTOGRAM_COUNTS_100("Ash.NumberOfVisibleWindowsInPrimaryDisplay",
+                             GetNumVisibleWindowsInPrimaryDisplay());
+  }
 
+  // TODO(bruthig): Find out if this should only be logged when the user is
+  // active.
   UMA_HISTOGRAM_ENUMERATION("Ash.ActiveWindowShowTypeOverTime",

[tdanderson, 2015/04/17 17:59:57]

IMO, we should only be collecting periodic metrics related to the UI if the user
is active (so that applies to all of the histograms here). Double-check with the
histogram owners, though, before making any such change.

[bruthig, 2015/04/21 14:34:05]

I will check with the owner's on these and will make the change in a follow-up
CL if necessary.

                             GetActiveWindowState(),
                             ACTIVE_WINDOW_STATE_TYPE_COUNT);
 }

[sadrul, 2015/04/17 22:55:39]

This function is ... interesting. What are the values of these histograms? I
think it would be more useful to know how long the user spent with left/bottom
etc. shelf alignment before switching to a different orientation, how long the
user has X number of windows open etc. before opening/closing a window, how long
a particular type of window is active at a time etc., rather than collecting
this info every 30 minutes. The owner for these seem to be kuscher@ Can you guys
talk to him to see if that would make more sense?

[bruthig, 2015/04/21 14:34:05]

I've added some TODO's to consider these suggestions for the two histograms that
already exist. Unfortunately we don't currently have the time to push these
forward.

As for the histogram that is being added, tdanderson@ and I discussed and agree
that tracking 'the amount of time X number of windows is visible' may be a
better thing to do, we felt that the additional work to implement this was not
worth the effort.  The immediate questions we are trying to answer revolve
around whether we need to invest time implementing window management in
TouchView.

sadrul@, did you have any major concerns as to why tracking it this way would be
bad?

[sadrul, 2015/04/21 17:24:56]

The only concern I have is that this data is not useful enough, and may even be
misleading, depending on how we interpret the data. For example, if some user
always has two open windows, but frequently opens a short-lived (in order of
minutes) third/fourth window, then we would potentially completely miss that.

It would be better to collect more useful data, or not collect this at all. I
will leave it to you to consult with the owner.

In theory, it shouldn't need too much effort to change the code to collect the
more useful data.

[oshima, 2015/04/25 01:06:58]

You can listen to activation change plus login state change, and record
accumulative time instead of doing this periodic. 

 
+bool UserMetricsRecorder::UserIsActive() {
+  return Shell::GetInstance()
+             ->session_state_delegate()
+             ->IsActiveUserSessionStarted() &&
+         Shell::GetInstance()->system_tray_delegate()->GetUserLoginStatus() !=
+             user::LOGGED_IN_LOCKED;

[tdanderson, 2015/04/17 17:59:57]

Is the first clause necessary here?

[bruthig, 2015/04/21 14:34:05]

Removed.

+}
+
+void UserMetricsRecorder::StartTimer() {
+  timer_.Start(FROM_HERE,
+               base::TimeDelta::FromSeconds(kAshPeriodicMetricsTimeInSeconds),
+               this, &UserMetricsRecorder::RecordPeriodicMetrics);
+}
+
 }  // namespace ash