Introduce TabStripModelStatsRecorder class to record tab interaction stats

Introduce TabStripModelStatsRecorder class to record tab interaction stats

This CL introduces TabStripModelStatsRecorder class.
TabStripModelStatsRecorder will be used to hook TabStripModelObserver
callbacks to record user tab interaction stats.

The TabStripModelStatsRecorder class is instantiated once per browser
process via UMABrowsingActivityObserver, and it observes BrowserList
to register itself as TabStripModelObserver for all chrome windows.

BUG=517335
Committed: https://crrev.com/ae80287741a5781a048bdd2a336ec49598100970
Cr-Commit-Position: refs/heads/master@{#343620}

[kouhei (in TOK), 2015-08-11 00:58:43]

kouhei@chromium.org changed reviewers:
+ georgesak@chromium.org, sky@chromium.org, tzik@chromium.org

[kouhei (in TOK), 2015-08-11 00:59:02]

CL split from https://codereview.chromium.org/1277843002/

Would you take a look?

[kouhei (in TOK), 2015-08-11 00:59:22]

The CQ bit was checked by kouhei@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-08-11 00:59:56]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1286433004/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1286433004/1

[commit-bot: I haz the power, 2015-08-11 01:38:44]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-08-11 01:38:44]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[tzik, 2015-08-11 05:43:48]

lgtm

https://codereview.chromium.org/1286433004/diff/1/chrome/chrome_browser_ui.gypi
File chrome/chrome_browser_ui.gypi (right):

https://codereview.chromium.org/1286433004/diff/1/chrome/chrome_browser_ui.gy...
chrome/chrome_browser_ui.gypi:1714:
'browser/ui/tabs/tab_strip_model_stats_recorder.h',
could you also update BUILD.gn too?

[kouhei (in TOK), 2015-08-11 08:11:45]

https://codereview.chromium.org/1286433004/diff/1/chrome/chrome_browser_ui.gypi
File chrome/chrome_browser_ui.gypi (right):

https://codereview.chromium.org/1286433004/diff/1/chrome/chrome_browser_ui.gy...
chrome/chrome_browser_ui.gypi:1714:
'browser/ui/tabs/tab_strip_model_stats_recorder.h',
On 2015/08/11 05:43:48, tzik wrote:
> could you also update BUILD.gn too?

Looks like BUILD.gn is auto-generated from this gypi. so it isn't needed

[kouhei (in TOK), 2015-08-11 08:12:37]

sky: Would you take a look?

[sky, 2015-08-11 15:27:42]

https://codereview.chromium.org/1286433004/diff/1/chrome/browser/ui/tabs/tab_...
File chrome/browser/ui/tabs/tab_strip_model_stats_recorder.h (right):

https://codereview.chromium.org/1286433004/diff/1/chrome/browser/ui/tabs/tab_...
chrome/browser/ui/tabs/tab_strip_model_stats_recorder.h:15: namespace chrome {
Don't use chrome namespace. If you want motivation search for threads on
chromium-dev.

https://codereview.chromium.org/1286433004/diff/1/chrome/browser/ui/tabs/tab_...
chrome/browser/ui/tabs/tab_strip_model_stats_recorder.h:20: class
TabStripModelStatsRecorder : public BrowserListObserver,
Combine this with chrome/browser/ui/ash/metrics/chrome_user_metrics_recorder.h .

https://codereview.chromium.org/1286433004/diff/1/chrome/browser/ui/tabs/tab_...
chrome/browser/ui/tabs/tab_strip_model_stats_recorder.h:26: //
chrome::BrowserListObserver implementation.
Make this private, add DISALLOW_COPY_AND_ASSIGN.

[kouhei (in TOK), 2015-08-12 04:38:29]

Thanks for your comments!

https://codereview.chromium.org/1286433004/diff/1/chrome/browser/ui/tabs/tab_...
File chrome/browser/ui/tabs/tab_strip_model_stats_recorder.h (right):

https://codereview.chromium.org/1286433004/diff/1/chrome/browser/ui/tabs/tab_...
chrome/browser/ui/tabs/tab_strip_model_stats_recorder.h:15: namespace chrome {
On 2015/08/11 15:27:42, sky wrote:
> Don't use chrome namespace. If you want motivation search for threads on
> chromium-dev.

Done.

https://codereview.chromium.org/1286433004/diff/1/chrome/browser/ui/tabs/tab_...
chrome/browser/ui/tabs/tab_strip_model_stats_recorder.h:20: class
TabStripModelStatsRecorder : public BrowserListObserver,
On 2015/08/11 15:27:42, sky wrote:
> Combine this with chrome/browser/ui/ash/metrics/chrome_user_metrics_recorder.h
.

I'm not sure what is the best solution here.

I found ChromeUserMetricsRecorder:::OnTabSwitchedByUserGesture accessing ash
classes:
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...
so we can't simply move the function body.

We can add OnTabSwitchedByUserGesture callback on TabStripModelStatsRecorder so
that ash/ can inject, but it would be TabStripModelObserverObserver essentially.

https://codereview.chromium.org/1286433004/diff/1/chrome/browser/ui/tabs/tab_...
chrome/browser/ui/tabs/tab_strip_model_stats_recorder.h:26: //
chrome::BrowserListObserver implementation.
On 2015/08/11 15:27:42, sky wrote:
> Make this private, add DISALLOW_COPY_AND_ASSIGN.

Done.

[sky, 2015-08-12 16:21:36]

https://codereview.chromium.org/1286433004/diff/20001/chrome/browser/ui/tabs/...
File chrome/browser/ui/tabs/tab_strip_model_stats_recorder.cc (right):

https://codereview.chromium.org/1286433004/diff/20001/chrome/browser/ui/tabs/...
chrome/browser/ui/tabs/tab_strip_model_stats_recorder.cc:18:
BrowserList::RemoveObserver(this);
I'm not clear on the lifetime. Are you guaranteed that by the time you get here
this isn't obervering any browsers?

[kouhei (in TOK), 2015-08-13 01:45:55]

https://codereview.chromium.org/1286433004/diff/20001/chrome/browser/ui/tabs/...
File chrome/browser/ui/tabs/tab_strip_model_stats_recorder.cc (right):

https://codereview.chromium.org/1286433004/diff/20001/chrome/browser/ui/tabs/...
chrome/browser/ui/tabs/tab_strip_model_stats_recorder.cc:18:
BrowserList::RemoveObserver(this);
On 2015/08/12 16:21:36, sky wrote:
> I'm not clear on the lifetime. Are you guaranteed that by the time you get
here
> this isn't obervering any browsers?
The instance is guaranteed to be always alive, so we don't have to worry about
browser->tab_strip_model() calling into already destructed instance of
TabStripModelStatsRecoder.

TabStripModelStatsRecorder is owned by UMABrowsingActivityObserver, which is
never destructed, thus we will never get here (dtor).
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...

[sky, 2015-08-13 17:15:20]

On Wed, Aug 12, 2015 at 6:45 PM,  <kouhei@chromium.org> wrote:
>
>
https://codereview.chromium.org/1286433004/diff/20001/chrome/browser/ui/tabs/...
> File chrome/browser/ui/tabs/tab_strip_model_stats_recorder.cc (right):
>
>
https://codereview.chromium.org/1286433004/diff/20001/chrome/browser/ui/tabs/...
> chrome/browser/ui/tabs/tab_strip_model_stats_recorder.cc:18:
> BrowserList::RemoveObserver(this);
> On 2015/08/12 16:21:36, sky wrote:
>>
>> I'm not clear on the lifetime. Are you guaranteed that by the time you
>
> get here
>>
>> this isn't obervering any browsers?
>
> The instance is guaranteed to be always alive, so we don't have to worry
> about browser->tab_strip_model() calling into already destructed
> instance of TabStripModelStatsRecoder.

That's true now, but it's hard to know that from the class and easy to
break. Please add explicit cleanup.

  -Scott

>
> TabStripModelStatsRecorder is owned by UMABrowsingActivityObserver,
> which is never destructed, thus we will never get here (dtor).
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...
>
> https://codereview.chromium.org/1286433004/

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[kouhei (in TOK), 2015-08-14 01:15:33]

Added explicit observer removal in d-tor. Would you take a look?

On 2015/08/13 17:15:20, sky wrote:
> On Wed, Aug 12, 2015 at 6:45 PM,  <mailto:kouhei@chromium.org> wrote:
> >
> >
>
https://codereview.chromium.org/1286433004/diff/20001/chrome/browser/ui/tabs/...
> > File chrome/browser/ui/tabs/tab_strip_model_stats_recorder.cc (right):
> >
> >
>
https://codereview.chromium.org/1286433004/diff/20001/chrome/browser/ui/tabs/...
> > chrome/browser/ui/tabs/tab_strip_model_stats_recorder.cc:18:
> > BrowserList::RemoveObserver(this);
> > On 2015/08/12 16:21:36, sky wrote:
> >>
> >> I'm not clear on the lifetime. Are you guaranteed that by the time you
> >
> > get here
> >>
> >> this isn't obervering any browsers?
> >
> > The instance is guaranteed to be always alive, so we don't have to worry
> > about browser->tab_strip_model() calling into already destructed
> > instance of TabStripModelStatsRecoder.
> 
> That's true now, but it's hard to know that from the class and easy to
> break. Please add explicit cleanup.
> 
>   -Scott
> 
> >
> > TabStripModelStatsRecorder is owned by UMABrowsingActivityObserver,
> > which is never destructed, thus we will never get here (dtor).
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...
> >
> > https://codereview.chromium.org/1286433004/

[sky, 2015-08-14 14:57:46]

LGTM

https://codereview.chromium.org/1286433004/diff/60001/chrome/browser/ui/tabs/...
File chrome/browser/ui/tabs/tab_strip_model_stats_recorder.cc (right):

https://codereview.chromium.org/1286433004/diff/60001/chrome/browser/ui/tabs/...
chrome/browser/ui/tabs/tab_strip_model_stats_recorder.cc:19: for
(chrome::BrowserIterator iterator; !iterator.done(); iterator.Next()) {
nit: no {}

[kouhei (in TOK), 2015-08-17 02:07:59]

Thanks for your review!

https://codereview.chromium.org/1286433004/diff/60001/chrome/browser/ui/tabs/...
File chrome/browser/ui/tabs/tab_strip_model_stats_recorder.cc (right):

https://codereview.chromium.org/1286433004/diff/60001/chrome/browser/ui/tabs/...
chrome/browser/ui/tabs/tab_strip_model_stats_recorder.cc:19: for
(chrome::BrowserIterator iterator; !iterator.done(); iterator.Next()) {
On 2015/08/14 14:57:45, sky wrote:
> nit: no {}

Done.

[kouhei (in TOK), 2015-08-17 02:09:57]

The CQ bit was checked by kouhei@chromium.org

[kouhei (in TOK), 2015-08-17 02:09:57]

The patchset sent to the CQ was uploaded after l-g-t-m from tzik@chromium.org,
sky@chromium.org
Link to the patchset: https://codereview.chromium.org/1286433004/#ps80001
(title: "remove {} / rebase")

[commit-bot: I haz the power, 2015-08-17 02:09:59]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1286433004/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1286433004/80001

[commit-bot: I haz the power, 2015-08-17 03:01:56]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-08-17 03:02:40]

Patchset 5 (id:??) landed as
https://crrev.com/ae80287741a5781a048bdd2a336ec49598100970
Cr-Commit-Position: refs/heads/master@{#343620}