Upstream TabUma.

Upstream TabUma.

BUG=463125
Committed: https://crrev.com/8adb770df93622d77821cc9ed584b1f87418c2af
Cr-Commit-Position: refs/heads/master@{#322090}

[hartmanng, 2015-03-23 22:55:39]

Patchset #1 (id:1) has been deleted

[hartmanng, 2015-03-23 22:55:47]

Patchset #1 (id:20001) has been deleted

[hartmanng, 2015-03-23 23:05:11]

hartmanng@chromium.org changed reviewers:
+ yfriedman@chromium.org

[hartmanng, 2015-03-23 23:05:12]

Please take a look.

[hartmanng, 2015-03-24 13:28:25]

Patchset #2 (id:60001) has been deleted

[Yaron, 2015-03-24 13:52:30]

yfriedman@chromium.org changed reviewers:
+ dtrainor@chromium.org

[Yaron, 2015-03-24 13:52:30]

lgtm but please wait for David

https://codereview.chromium.org/1032513003/diff/40001/chrome/android/java/src...
File chrome/android/java/src/org/chromium/chrome/browser/Tab.java (right):

https://codereview.chromium.org/1032513003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/Tab.java:741: public void
setTabUma(TabUma tabUma) {
Make protected

https://codereview.chromium.org/1032513003/diff/40001/chrome/android/java/src...
File chrome/android/java/src/org/chromium/chrome/browser/TabUma.java (right):

https://codereview.chromium.org/1032513003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/TabUma.java:1: // Copyright
2013 The Chromium Authors. All rights reserved.
Update copyright

https://codereview.chromium.org/1032513003/diff/40001/chrome/android/javatest...
File chrome/android/javatests/src/org/chromium/chrome/browser/TabUmaTest.java
(right):

https://codereview.chromium.org/1032513003/diff/40001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/TabUmaTest.java:1: //
Copyright 2014 The Chromium Authors. All rights reserved.
ditto

[hartmanng, 2015-03-24 14:05:16]

https://codereview.chromium.org/1032513003/diff/40001/chrome/android/java/src...
File chrome/android/java/src/org/chromium/chrome/browser/Tab.java (right):

https://codereview.chromium.org/1032513003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/Tab.java:741: public void
setTabUma(TabUma tabUma) {
On 2015/03/24 13:52:30, Yaron wrote:
> Make protected

Done.

https://codereview.chromium.org/1032513003/diff/40001/chrome/android/java/src...
File chrome/android/java/src/org/chromium/chrome/browser/TabUma.java (right):

https://codereview.chromium.org/1032513003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/TabUma.java:1: // Copyright
2013 The Chromium Authors. All rights reserved.
On 2015/03/24 13:52:30, Yaron wrote:
> Update copyright

Done.

https://codereview.chromium.org/1032513003/diff/40001/chrome/android/javatest...
File chrome/android/javatests/src/org/chromium/chrome/browser/TabUmaTest.java
(right):

https://codereview.chromium.org/1032513003/diff/40001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/TabUmaTest.java:1: //
Copyright 2014 The Chromium Authors. All rights reserved.
On 2015/03/24 13:52:30, Yaron wrote:
> ditto

Done.

[David Trainor- moved to gerrit, 2015-03-24 20:24:57]

Thanks for upstreaming this!  A few comments on Tab.java.  Are
TabUma.java/TabUmaTest.java basically simple file moves?

https://chromiumcodereview.appspot.com/1032513003/diff/80001/chrome/android/j...
File chrome/android/java/src/org/chromium/chrome/browser/Tab.java (right):

https://chromiumcodereview.appspot.com/1032513003/diff/80001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/Tab.java:741: protected void
setTabUma(TabUma tabUma) {
Are we changing how we build/set up TabUma?  It looks like in ChromeTab we build
it internally.

https://chromiumcodereview.appspot.com/1032513003/diff/80001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/Tab.java:1248: if (mTabUma
!= null) mTabUma.onShow(type, getTimestampMillis());
Can we move this to show()?

https://chromiumcodereview.appspot.com/1032513003/diff/80001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/Tab.java:2661: * cases, such
as when incognito mode is active.
Is this true?  The static create methods sem to build one whether or not the
incognito boolean is set.  If we're trying to make these guarantees do we need
the setTabUma method?

[hartmanng, 2015-03-24 20:49:17]

On 2015/03/24 20:24:57, David Trainor wrote:
> Thanks for upstreaming this!  A few comments on Tab.java.  Are
> TabUma.java/TabUmaTest.java basically simple file moves?

Mostly, yes. In particular, TabUma.java is almost an exact move (with the
exception that I replaced all occurrences of ChromeTab with Tab).

TabUmaTest.java required a little bit more, but not too much. The setUp() method
in there is new, and the base class that TabUmaTest inherits from is different.
Apart from that, just a few type renames (ChromeTab->Tab,
UmaHistogramDelta->HistogramDelta).

https://codereview.chromium.org/1032513003/diff/80001/chrome/android/java/src...
File chrome/android/java/src/org/chromium/chrome/browser/Tab.java (right):

https://codereview.chromium.org/1032513003/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/Tab.java:741: protected void
setTabUma(TabUma tabUma) {
On 2015/03/24 20:24:57, David Trainor wrote:
> Are we changing how we build/set up TabUma?  It looks like in ChromeTab we
build
> it internally.

Upstreaming the instantiation of TabUma looked a bit tricky, so I'm trying to
skip it for now. Specifically, we need to provide a TabCreationState and a
TabModel that isn't always easily available in Tab.

I planned to continue to let ChromeTab handle creating a TabUma and setting it
for now (see https://chrome-internal-review.googlesource.com/#/c/210006/).

https://codereview.chromium.org/1032513003/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/Tab.java:1248: if (mTabUma
!= null) mTabUma.onShow(type, getTimestampMillis());
On 2015/03/24 20:24:57, David Trainor wrote:
> Can we move this to show()?

Done.

https://codereview.chromium.org/1032513003/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/Tab.java:2661: * cases, such
as when incognito mode is active.
On 2015/03/24 20:24:57, David Trainor wrote:
> Is this true?  The static create methods sem to build one whether or not the
> incognito boolean is set.  If we're trying to make these guarantees do we need
> the setTabUma method?

mTabUma can be null, but you're right that incognito doesn't seem to be a factor
in this. The only restriction I see is that a TabUma shouldn't get created if
the context is null or if the TabCreationState is null.

I've updated this code to check for a null context (and updated the comment to
remove mention of incognito), but also see
https://chrome-internal-review.googlesource.com/#/c/210006/ for the "regular"
instantiation point of TabUma going forward. I don't actually know under what
circumstances a Tab would be created with a null context, but I'm not confident
that it can't happen.

These two "create" functions are (currently) only being used in TabUmaTest.

https://codereview.chromium.org/1032513003/diff/80001/chrome/android/javatest...
File chrome/android/javatests/src/org/chromium/chrome/browser/TabUmaTest.java
(right):

https://codereview.chromium.org/1032513003/diff/80001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/TabUmaTest.java:37: //
This is to fake a "tab show" event that should be happening
This is new code.

[David Trainor- moved to gerrit, 2015-03-24 21:28:19]

Thanks for the explanation.  lgtm! :)

[hartmanng, 2015-03-24 21:40:23]

Thanks!

[hartmanng, 2015-03-24 21:40:27]

The CQ bit was checked by hartmanng@chromium.org

[hartmanng, 2015-03-24 21:40:27]

The patchset sent to the CQ was uploaded after l-g-t-m from
yfriedman@chromium.org
Link to the patchset: https://codereview.chromium.org/1032513003/#ps100001
(title: "dtrainor's comments")

[commit-bot: I haz the power, 2015-03-24 21:40:54]

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

[commit-bot: I haz the power, 2015-03-24 23:33:00]

Committed patchset #3 (id:100001)

[commit-bot: I haz the power, 2015-03-24 23:33:41]

Patchset 3 (id:??) landed as
https://crrev.com/8adb770df93622d77821cc9ed584b1f87418c2af
Cr-Commit-Position: refs/heads/master@{#322090}