[WillSplitBeforeLand] Add UMAs to measure how user interacts with tabs

[WillSplitBeforeLand] Add UMAs to measure how user interacts with tabs

BUG=517335

[kouhei (in TOK), 2015-08-06 06:49:36]

kouhei@chromium.org changed reviewers:
+ tzik@chromium.org

[kouhei (in TOK), 2015-08-06 06:49:36]

PTAL. I'll add histograms.xml entries once we finalize the UMA names

[kouhei (in TOK), 2015-08-06 06:49:47]

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

[commit-bot: I haz the power, 2015-08-06 06:49:52]

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

[kouhei (in TOK), 2015-08-06 06:51:17]

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

https://codereview.chromium.org/1277843002/diff/1/chrome/browser/ui/tabs/tab_...
chrome/browser/ui/tabs/tab_strip_model_stats_recorder.h:66: int current_index_;
Oh. Forgot to delete this. Should we track the tab index history?

[commit-bot: I haz the power, 2015-08-06 06:58:54]

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

[commit-bot: I haz the power, 2015-08-06 06:58:54]

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

[tzik, 2015-08-06 09:15:54]

looks good

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

https://codereview.chromium.org/1277843002/diff/1/chrome/browser/ui/tabs/tab_...
chrome/browser/ui/tabs/tab_strip_model_stats_recorder.h:66: int current_index_;
On 2015/08/06 06:51:17, kouhei wrote:
> Oh. Forgot to delete this. Should we track the tab index history?

I think we should. That will be the key to determine whether users tend to
switch to nearby tabs.

[tzik, 2015-08-07 07:17:42]

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

https://codereview.chromium.org/1277843002/diff/20001/chrome/browser/ui/tabs/...
chrome/browser/ui/tabs/tab_strip_model_stats_recorder.cc:1: // Copyright (c)
2015 The Chromium Authors. All rights reserved.
ditto

https://codereview.chromium.org/1277843002/diff/20001/chrome/browser/ui/tabs/...
chrome/browser/ui/tabs/tab_strip_model_stats_recorder.cc:40:
printf("TransferState: %d -> %d\n", current_state_, new_state);
remove or convert to VLOG? (here and elsewhere)

https://codereview.chromium.org/1277843002/diff/20001/chrome/browser/ui/tabs/...
chrome/browser/ui/tabs/tab_strip_model_stats_recorder.cc:163:
info.UpdateState(foreground ? TabState::ACTIVE : TabState::INACTIVE);
Can this be just like below?
tab_info_map_[contents].UpdateState(...);

https://codereview.chromium.org/1277843002/diff/20001/chrome/browser/ui/tabs/...
chrome/browser/ui/tabs/tab_strip_model_stats_recorder.cc:195: return;
not needed.

https://codereview.chromium.org/1277843002/diff/20001/chrome/browser/ui/tabs/...
chrome/browser/ui/tabs/tab_strip_model_stats_recorder.cc:283:
printf("TabReplacedAt %p %p %d\n", old_contents, new_contents, index);
Could you DCHECK old_contents != new_contents here?

https://codereview.chromium.org/1277843002/diff/20001/chrome/browser/ui/tabs/...
chrome/browser/ui/tabs/tab_strip_model_stats_recorder.cc:292:
tab_info_map_.erase(old_contents);
Can we use |it| instead of old_contents?

https://codereview.chromium.org/1277843002/diff/20001/chrome/browser/ui/tabs/...
chrome/browser/ui/tabs/tab_strip_model_stats_recorder.cc:302: }
// namespace chrome

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

https://codereview.chromium.org/1277843002/diff/20001/chrome/browser/ui/tabs/...
chrome/browser/ui/tabs/tab_strip_model_stats_recorder.h:1: // Copyright (c) 2015
The Chromium Authors. All rights reserved.
nit: Could you remove "(c)"?
https://www.chromium.org/developers/coding-style#TOC-File-headers

https://codereview.chromium.org/1277843002/diff/20001/chrome/browser/ui/tabs/...
chrome/browser/ui/tabs/tab_strip_model_stats_recorder.h:20: 
Could you add a brief comment to describe the class?

https://codereview.chromium.org/1277843002/diff/20001/chrome/browser/ui/tabs/...
chrome/browser/ui/tabs/tab_strip_model_stats_recorder.h:74: }
Please add "// namespace chrome" after "}", and a newline before "}"?

[kouhei (in TOK), 2015-08-11 00:34:19]

Thanks for review. Updated

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

https://codereview.chromium.org/1277843002/diff/20001/chrome/browser/ui/tabs/...
chrome/browser/ui/tabs/tab_strip_model_stats_recorder.cc:40:
printf("TransferState: %d -> %d\n", current_state_, new_state);
On 2015/08/07 07:17:42, tzik wrote:
> remove or convert to VLOG? (here and elsewhere)

Removed

https://codereview.chromium.org/1277843002/diff/20001/chrome/browser/ui/tabs/...
chrome/browser/ui/tabs/tab_strip_model_stats_recorder.cc:163:
info.UpdateState(foreground ? TabState::ACTIVE : TabState::INACTIVE);
On 2015/08/07 07:17:42, tzik wrote:
> Can this be just like below?
> tab_info_map_[contents].UpdateState(...);

Done.

https://codereview.chromium.org/1277843002/diff/20001/chrome/browser/ui/tabs/...
chrome/browser/ui/tabs/tab_strip_model_stats_recorder.cc:195: return;
On 2015/08/07 07:17:42, tzik wrote:
> not needed.

Done.

https://codereview.chromium.org/1277843002/diff/20001/chrome/browser/ui/tabs/...
chrome/browser/ui/tabs/tab_strip_model_stats_recorder.cc:283:
printf("TabReplacedAt %p %p %d\n", old_contents, new_contents, index);
On 2015/08/07 07:17:42, tzik wrote:
> Could you DCHECK old_contents != new_contents here?

Done.

https://codereview.chromium.org/1277843002/diff/20001/chrome/browser/ui/tabs/...
chrome/browser/ui/tabs/tab_strip_model_stats_recorder.cc:292:
tab_info_map_.erase(old_contents);
On 2015/08/07 07:17:42, tzik wrote:
> Can we use |it| instead of old_contents?

Done.

https://codereview.chromium.org/1277843002/diff/20001/chrome/browser/ui/tabs/...
chrome/browser/ui/tabs/tab_strip_model_stats_recorder.cc:302: }
On 2015/08/07 07:17:42, tzik wrote:
> // namespace chrome

Done.

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

https://codereview.chromium.org/1277843002/diff/20001/chrome/browser/ui/tabs/...
chrome/browser/ui/tabs/tab_strip_model_stats_recorder.h:1: // Copyright (c) 2015
The Chromium Authors. All rights reserved.
On 2015/08/07 07:17:42, tzik wrote:
> nit: Could you remove "(c)"?
> https://www.chromium.org/developers/coding-style#TOC-File-headers

Done.

https://codereview.chromium.org/1277843002/diff/20001/chrome/browser/ui/tabs/...
chrome/browser/ui/tabs/tab_strip_model_stats_recorder.h:20: 
On 2015/08/07 07:17:42, tzik wrote:
> Could you add a brief comment to describe the class?

Done.

https://codereview.chromium.org/1277843002/diff/20001/chrome/browser/ui/tabs/...
chrome/browser/ui/tabs/tab_strip_model_stats_recorder.h:74: }
On 2015/08/07 07:17:42, tzik wrote:
> Please add "// namespace chrome" after "}", and a newline before "}"?

Done.

[tzik, 2015-08-11 05:46:41]

lgtm