Added the Ash.Tab.TimeBetweenSwitchToExistingTabUserActions histogram to track time between tab swit

Added the Ash.Tab.TimeBetweenSwitchToExistingTabUserActions histogram to track time between tab switches.

TEST=Manually verified histogram data added on ChromeOS.

BUG=478930
Committed: https://crrev.com/42faa2d8efff66a999da8291622ba43712b1ba8a
Cr-Commit-Position: refs/heads/master@{#330217}

[oshima, 2015-05-15 17:13:36]

oshima@chromium.org changed reviewers:
+ oshima@chromium.org

[oshima, 2015-05-15 17:13:36]

you should talk to sky about how to observe tab switch events.

https://codereview.chromium.org/1124153007/diff/20001/ash/metrics/user_metric...
File ash/metrics/user_metrics_recorder_proxy.h (right):

https://codereview.chromium.org/1124153007/diff/20001/ash/metrics/user_metric...
ash/metrics/user_metrics_recorder_proxy.h:15: class ASH_EXPORT
UserMetricsRecorderProxy {
you can call ash/metrics from chrome/browser/ui/ash, so you don't need extra
proxy.

[bruthig, 2015-05-15 19:11:26]

bruthig@chromium.org changed reviewers:
+ asvitkine@google.com, tdanderson@chromium.org

[bruthig, 2015-05-15 19:11:28]

oshima@: Can you please have a look at:
 - ash/*
 - chrome/*

asvitkine@: Can you please have a look at:
 - tools/metrics/histograms/histograms.xml

tdanderson@: Can you please have a look from a feature accuracy perspective?

https://codereview.chromium.org/1124153007/diff/20001/ash/metrics/user_metric...
File ash/metrics/user_metrics_recorder_proxy.h (right):

https://codereview.chromium.org/1124153007/diff/20001/ash/metrics/user_metric...
ash/metrics/user_metrics_recorder_proxy.h:15: class ASH_EXPORT
UserMetricsRecorderProxy {
On 2015/05/15 17:13:36, oshima wrote:
> you can call ash/metrics from chrome/browser/ui/ash, so you don't need extra
> proxy.

Done.

https://codereview.chromium.org/1124153007/diff/40001/chrome/browser/ui/ash/c...
File chrome/browser/ui/ash/chrome_shell_delegate_chromeos.cc (right):

https://codereview.chromium.org/1124153007/diff/40001/chrome/browser/ui/ash/c...
chrome/browser/ui/ash/chrome_shell_delegate_chromeos.cc:226:
user_metrics_recorder_proxy_.reset();
oshima@, is it safe to assume that all Browser's have called
UserMetricsRecorderProxy::OnBrowserRemoved(...) at this point?  Or does the
UserMetricsRecorderProxy class need to track which Browser's it has attached to
and disconnect the observer during ~UserMetricsRecorderProxy()?

[bruthig, 2015-05-15 19:13:13]

bruthig@chromium.org changed reviewers:
+ asvitkine@chromium.org
- asvitkine@google.com

[Alexei Svitkine (slow), 2015-05-15 19:19:56]

https://codereview.chromium.org/1124153007/diff/40001/ash/metrics/user_metric...
File ash/metrics/user_metrics_recorder.h (right):

https://codereview.chromium.org/1124153007/diff/40001/ash/metrics/user_metric...
ash/metrics/user_metrics_recorder.h:15: class ShellDelegate;
Do you still need changes to this file?

https://codereview.chromium.org/1124153007/diff/40001/chrome/browser/ui/ash/c...
File chrome/browser/ui/ash/chrome_shell_delegate.h (right):

https://codereview.chromium.org/1124153007/diff/40001/chrome/browser/ui/ash/c...
chrome/browser/ui/ash/chrome_shell_delegate.h:97: // Proxy's events from
chrome/browser to ash::UserMetricsRecorder.
Nit: Proxies.

But actually, I don't see this class logging any user actions. Is the intention
for this to come in future CLs? Otherwise, it doesn't seem to be accurately
named.

https://codereview.chromium.org/1124153007/diff/40001/chrome/browser/ui/ash/m...
File chrome/browser/ui/ash/metrics/user_metrics_recorder_proxy.h (right):

https://codereview.chromium.org/1124153007/diff/40001/chrome/browser/ui/ash/m...
chrome/browser/ui/ash/metrics/user_metrics_recorder_proxy.h:8: #include <string>
Is this include needed?

[tdanderson, 2015-05-15 19:23:09]

lgtm with the nits mentioned below.

https://codereview.chromium.org/1124153007/diff/40001/ash/metrics/user_metric...
File ash/metrics/user_metrics_recorder.h (right):

https://codereview.chromium.org/1124153007/diff/40001/ash/metrics/user_metric...
ash/metrics/user_metrics_recorder.h:10: #include "base/memory/scoped_ptr.h"
needed?

https://codereview.chromium.org/1124153007/diff/40001/ash/metrics/user_metric...
ash/metrics/user_metrics_recorder.h:15: class ShellDelegate;
needed?

https://codereview.chromium.org/1124153007/diff/40001/chrome/browser/ui/ash/m...
File chrome/browser/ui/ash/metrics/user_metrics_recorder_proxy.h (right):

https://codereview.chromium.org/1124153007/diff/40001/chrome/browser/ui/ash/m...
chrome/browser/ui/ash/metrics/user_metrics_recorder_proxy.h:38: // Called when a
new tab becomes active due to a user gesture.
nit: using the term "new tab" could be confusing since we're not measuring
opening new tabs as task switches.

https://codereview.chromium.org/1124153007/diff/40001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1124153007/diff/40001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:793: +    The number of seconds between
tab switches triggered by a user gesture (ie
nit: ie -> e.g.,

[bruthig, 2015-05-15 19:36:21]

I have addressed asvitkine@ and tdanderson@'s comments from patch set 3.

oshima@: Can you please have a look at:
 - ash/*
 - chrome/*

asvitkine@: Can you please have another look at:
 - tools/metrics/histograms/histograms.xml

tdanderson@: Can you please have another look from a feature accuracy
perspective?

https://codereview.chromium.org/1124153007/diff/40001/ash/metrics/user_metric...
File ash/metrics/user_metrics_recorder.h (right):

https://codereview.chromium.org/1124153007/diff/40001/ash/metrics/user_metric...
ash/metrics/user_metrics_recorder.h:10: #include "base/memory/scoped_ptr.h"
On 2015/05/15 19:23:09, tdanderson wrote:
> needed?

Done.

https://codereview.chromium.org/1124153007/diff/40001/ash/metrics/user_metric...
ash/metrics/user_metrics_recorder.h:15: class ShellDelegate;
On 2015/05/15 19:19:56, Alexei Svitkine wrote:
> Do you still need changes to this file?

Nope, done.

https://codereview.chromium.org/1124153007/diff/40001/ash/metrics/user_metric...
ash/metrics/user_metrics_recorder.h:15: class ShellDelegate;
On 2015/05/15 19:23:09, tdanderson wrote:
> needed?

Done.

https://codereview.chromium.org/1124153007/diff/40001/chrome/browser/ui/ash/c...
File chrome/browser/ui/ash/chrome_shell_delegate.h (right):

https://codereview.chromium.org/1124153007/diff/40001/chrome/browser/ui/ash/c...
chrome/browser/ui/ash/chrome_shell_delegate.h:97: // Proxy's events from
chrome/browser to ash::UserMetricsRecorder.
Fixed Proxy's to Proxies.

Summary of discussion offline.  UserMetricsRecorderProxy intentionally uses
'Metrics' as opposed to 'Actions' to hopefully include histograms as well.  The
actual recording is done by the ash::UserMetricsRecorder class.

https://codereview.chromium.org/1124153007/diff/40001/chrome/browser/ui/ash/m...
File chrome/browser/ui/ash/metrics/user_metrics_recorder_proxy.h (right):

https://codereview.chromium.org/1124153007/diff/40001/chrome/browser/ui/ash/m...
chrome/browser/ui/ash/metrics/user_metrics_recorder_proxy.h:8: #include <string>
On 2015/05/15 19:19:56, Alexei Svitkine wrote:
> Is this include needed?

Removed.

https://codereview.chromium.org/1124153007/diff/40001/chrome/browser/ui/ash/m...
chrome/browser/ui/ash/metrics/user_metrics_recorder_proxy.h:38: // Called when a
new tab becomes active due to a user gesture.
On 2015/05/15 19:23:09, tdanderson wrote:
> nit: using the term "new tab" could be confusing since we're not measuring
> opening new tabs as task switches.

Done.

https://codereview.chromium.org/1124153007/diff/40001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1124153007/diff/40001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:793: +    The number of seconds between
tab switches triggered by a user gesture (ie
On 2015/05/15 19:23:09, tdanderson wrote:
> nit: ie -> e.g.,

Done.

[Alexei Svitkine (slow), 2015-05-15 19:37:50]

lgtm

[oshima, 2015-05-15 19:45:36]

https://codereview.chromium.org/1124153007/diff/60001/chrome/browser/ui/ash/m...
File chrome/browser/ui/ash/metrics/user_metrics_recorder_proxy.cc (right):

https://codereview.chromium.org/1124153007/diff/60001/chrome/browser/ui/ash/m...
chrome/browser/ui/ash/metrics/user_metrics_recorder_proxy.cc:22:
BrowserList::RemoveObserver(this);
can you DCHECK if browser list is empty? Otherwise tab strip will have stale
observer.

https://codereview.chromium.org/1124153007/diff/60001/chrome/browser/ui/ash/m...
chrome/browser/ui/ash/metrics/user_metrics_recorder_proxy.cc:40: }
nuke {}

https://codereview.chromium.org/1124153007/diff/60001/chrome/browser/ui/ash/m...
chrome/browser/ui/ash/metrics/user_metrics_recorder_proxy.cc:44:
CHECK(ash::Shell::HasInstance());
DCHECK (on release build, you'll get crash report with the stack in next line.)

https://codereview.chromium.org/1124153007/diff/60001/chrome/browser/ui/ash/m...
File chrome/browser/ui/ash/metrics/user_metrics_recorder_proxy.h (right):

https://codereview.chromium.org/1124153007/diff/60001/chrome/browser/ui/ash/m...
chrome/browser/ui/ash/metrics/user_metrics_recorder_proxy.h:19: class
UserMetricsRecorderProxy : public chrome::BrowserListObserver,
ChromeUserMetricsRecorder / TabStripUserMetricsRecorder (or something like this)
would be better as proxy is too generic, and this records chrome's tab
operation.

[bruthig, 2015-05-15 20:02:43]

I've addressed oshima@'s comments from patch set 4.

oshima@, can you have another look?

https://codereview.chromium.org/1124153007/diff/60001/chrome/browser/ui/ash/m...
File chrome/browser/ui/ash/metrics/user_metrics_recorder_proxy.cc (right):

https://codereview.chromium.org/1124153007/diff/60001/chrome/browser/ui/ash/m...
chrome/browser/ui/ash/metrics/user_metrics_recorder_proxy.cc:22:
BrowserList::RemoveObserver(this);
On 2015/05/15 19:45:35, oshima wrote:
> can you DCHECK if browser list is empty? Otherwise tab strip will have stale
> observer.

Done.

https://codereview.chromium.org/1124153007/diff/60001/chrome/browser/ui/ash/m...
chrome/browser/ui/ash/metrics/user_metrics_recorder_proxy.cc:40: }
On 2015/05/15 19:45:35, oshima wrote:
> nuke {}

Done.

https://codereview.chromium.org/1124153007/diff/60001/chrome/browser/ui/ash/m...
chrome/browser/ui/ash/metrics/user_metrics_recorder_proxy.cc:44:
CHECK(ash::Shell::HasInstance());
On 2015/05/15 19:45:35, oshima wrote:
> DCHECK (on release build, you'll get crash report with the stack in next
line.)

Oh, I thought DCHECK's were removed on release builds and it was the CHECK's
that were kept.
Done.

https://codereview.chromium.org/1124153007/diff/60001/chrome/browser/ui/ash/m...
File chrome/browser/ui/ash/metrics/user_metrics_recorder_proxy.h (right):

https://codereview.chromium.org/1124153007/diff/60001/chrome/browser/ui/ash/m...
chrome/browser/ui/ash/metrics/user_metrics_recorder_proxy.h:19: class
UserMetricsRecorderProxy : public chrome::BrowserListObserver,
I intentionally named it generically so that other things that need to be passed
through can use this mechanism.  It may only do one thing now but there's no
reason it can't be extended to do more.

Let me know if you feel strongly about this and I will change it.

[oshima, 2015-05-15 20:12:55]

https://codereview.chromium.org/1124153007/diff/60001/chrome/browser/ui/ash/m...
File chrome/browser/ui/ash/metrics/user_metrics_recorder_proxy.cc (right):

https://codereview.chromium.org/1124153007/diff/60001/chrome/browser/ui/ash/m...
chrome/browser/ui/ash/metrics/user_metrics_recorder_proxy.cc:44:
CHECK(ash::Shell::HasInstance());
On 2015/05/15 20:02:43, bruthig wrote:
> On 2015/05/15 19:45:35, oshima wrote:
> > DCHECK (on release build, you'll get crash report with the stack in next
> line.)
> 
> Oh, I thought DCHECK's were removed on release builds and it was the CHECK's
> that were kept.
> Done.

What I meant is that you'll get segfault crash report on the next line on
release build, so no need to add CHECK here. CHECK is useful if
1) you want to catch nullptr early before being used later.
2) there is a security risk

https://codereview.chromium.org/1124153007/diff/60001/chrome/browser/ui/ash/m...
File chrome/browser/ui/ash/metrics/user_metrics_recorder_proxy.h (right):

https://codereview.chromium.org/1124153007/diff/60001/chrome/browser/ui/ash/m...
chrome/browser/ui/ash/metrics/user_metrics_recorder_proxy.h:19: class
UserMetricsRecorderProxy : public chrome::BrowserListObserver,
On 2015/05/15 20:02:43, bruthig wrote:
> I intentionally named it generically so that other things that need to be
passed
> through can use this mechanism.  It may only do one thing now but there's no
> reason it can't be extended to do more.
> 
> Let me know if you feel strongly about this and I will change it.

*Other things* should still be chrome browser related. Non chrome related stuff
should
be recorded in different class in different place.

[bruthig, 2015-05-15 20:40:26]

I've addressed oshima@'s comments from patch set 5.

I've moved ChromeUserMetricsRecorder out of the ash namespace.

oshima@ can you have another look?

https://codereview.chromium.org/1124153007/diff/60001/chrome/browser/ui/ash/m...
File chrome/browser/ui/ash/metrics/user_metrics_recorder_proxy.cc (right):

https://codereview.chromium.org/1124153007/diff/60001/chrome/browser/ui/ash/m...
chrome/browser/ui/ash/metrics/user_metrics_recorder_proxy.cc:44:
CHECK(ash::Shell::HasInstance());
On 2015/05/15 20:12:54, oshima wrote:
> On 2015/05/15 20:02:43, bruthig wrote:
> > On 2015/05/15 19:45:35, oshima wrote:
> > > DCHECK (on release build, you'll get crash report with the stack in next
> > line.)
> > 
> > Oh, I thought DCHECK's were removed on release builds and it was the CHECK's
> > that were kept.
> > Done.
> 
> What I meant is that you'll get segfault crash report on the next line on
> release build, so no need to add CHECK here. CHECK is useful if
> 1) you want to catch nullptr early before being used later.
> 2) there is a security risk
> 
> 

Done.

https://codereview.chromium.org/1124153007/diff/60001/chrome/browser/ui/ash/m...
File chrome/browser/ui/ash/metrics/user_metrics_recorder_proxy.h (right):

https://codereview.chromium.org/1124153007/diff/60001/chrome/browser/ui/ash/m...
chrome/browser/ui/ash/metrics/user_metrics_recorder_proxy.h:19: class
UserMetricsRecorderProxy : public chrome::BrowserListObserver,
On 2015/05/15 20:12:54, oshima wrote:
> On 2015/05/15 20:02:43, bruthig wrote:
> > I intentionally named it generically so that other things that need to be
> passed
> > through can use this mechanism.  It may only do one thing now but there's no
> > reason it can't be extended to do more.
> > 
> > Let me know if you feel strongly about this and I will change it.
> 
> *Other things* should still be chrome browser related. Non chrome related
stuff
> should
> be recorded in different class in different place.

Done.

[oshima, 2015-05-15 20:43:48]

lgtm

[tdanderson, 2015-05-15 20:44:14]

The CQ bit was checked by tdanderson@chromium.org

[tdanderson, 2015-05-15 20:44:14]

The patchset sent to the CQ was uploaded after l-g-t-m from
tdanderson@chromium.org, asvitkine@chromium.org
Link to the patchset: https://codereview.chromium.org/1124153007/#ps100001
(title: "Addressed oshima@'s comments from patch set 5.")

[commit-bot: I haz the power, 2015-05-15 20:45:34]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1124153007/100001

[commit-bot: I haz the power, 2015-05-15 22:24:57]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2015-05-18 11:28:29]

Patchset 6 (id:??) landed as
https://crrev.com/42faa2d8efff66a999da8291622ba43712b1ba8a
Cr-Commit-Position: refs/heads/master@{#330217}