customtabs: UMA histogram tracking whether clients call warmup().

customtabs: UMA histogram tracking whether clients call warmup().

This adds a UMA histogram to learn about the behavior of client
applications. This is useful as calling warmup() is a net performance
win for Custom Tabs, and clients should be encouraged to do so.

This histogram has 5 states, since we cannot always reliably know
whether a client has called warmup(). Indeed, warmup can only query the
UID of the client, and a client can send a Custom Tabs intent without
creating a session. So, we have 5 states:
1. No session, and no-one has called warmup
2. No session, but someone has called warmup
3. Session, no warmup call from this session, but one from someone else.
4. Session, no warmup call from this session, and no-one has called it
5. Session, and warmup() call from this client.

The good cases for Chrome are the sum of (2), (3) and (5), but we should
encourage (5). (3) and (4) are bad client behaviors (you cared to
connect to the service but not to call warmup()).

BUG=527788
Committed: https://crrev.com/a08156afab9cf7f36223058a8ff753441f0b81ed
Cr-Commit-Position: refs/heads/master@{#351543}

[Benoit L, 2015-09-03 12:32:53]

lizeb@chromium.org changed reviewers:
+ pasko@chromium.org

[Benoit L, 2015-09-03 12:32:54]

Hi,

It's unfortunately a bit more complicated than I hoped, so we have 5 states...

[pasko, 2015-09-03 18:02:20]

https://codereview.chromium.org/1325213002/diff/30001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnection.java
(right):

https://codereview.chromium.org/1325213002/diff/30001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnection.java:83:
// Values for the "CustomTabs.CalledWarmup" UMA histogram. Append-only.
nittynit: empty line above would look nicer

https://codereview.chromium.org/1325213002/diff/30001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnection.java:202:
private final Object mLock = new Object();
please add a comment saying something like: 'mLock protects all the members
below'.

Locking is a bit complex in this class. How can we simplify reading it?

I think it will be cleaner to create a private inner class to keep this state,
remove all other synchronized blocks and make the class thread-safe.

Something like: SessionManager {
newSession(...)
updateForMayLaunchUrl(...)
evaluatePredictionOnLaunch(...)
getCallbackForSession(...)
...
// not sure what to do about keepalive, for example: if it runs on an arbitrary
thread, why is there no race when accessing mApplication?

We could create another class to manage the UI thread data. And this class would
just manage sending data to appropriate threads.

WDYT?

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

https://codereview.chromium.org/1325213002/diff/30001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:5140: +<histogram
name="CustomTabs.CalledWarmup" enum="CalledWarmup">
name suggestion:
CustomTabs.WarmupStateOnLaunch

https://codereview.chromium.org/1325213002/diff/30001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:5143: +    For Custom Tabs, records
whether warmup() has been called, and if so,
please explain explicitly at what moment the histogram is recorded

[Benoit L, 2015-09-29 09:04:23]

Thank you for the review.
The refactor you suggested was done and landed in a separate CL.

PTAL.

https://codereview.chromium.org/1325213002/diff/30001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnection.java
(right):

https://codereview.chromium.org/1325213002/diff/30001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnection.java:83:
// Values for the "CustomTabs.CalledWarmup" UMA histogram. Append-only.
On 2015/09/03 18:02:19, pasko wrote:
> nittynit: empty line above would look nicer

Ack.

https://codereview.chromium.org/1325213002/diff/30001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnection.java:202:
private final Object mLock = new Object();
On 2015/09/03 18:02:20, pasko wrote:
> please add a comment saying something like: 'mLock protects all the members
> below'.
> 
> Locking is a bit complex in this class. How can we simplify reading it?
> 
> I think it will be cleaner to create a private inner class to keep this state,
> remove all other synchronized blocks and make the class thread-safe.
> 
> Something like: SessionManager {
> newSession(...)
> updateForMayLaunchUrl(...)
> evaluatePredictionOnLaunch(...)
> getCallbackForSession(...)
> ...
> // not sure what to do about keepalive, for example: if it runs on an
arbitrary
> thread, why is there no race when accessing mApplication?
> 
> We could create another class to manage the UI thread data. And this class
would
> just manage sending data to appropriate threads.
> 
> WDYT?

Refactored this, so... done?

[pasko, 2015-09-29 09:50:07]

lgtm with a few naming suggestions to make it easier to read the dashboard.

thanks!

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

https://codereview.chromium.org/1325213002/diff/70001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:73375: +  <int value="2" label="Session,
No Warmup, Called elsewhere"/>
can we make 'elsewhere' clearer? Like:
"Session, No Warmup, Warmup called from another Uid"

https://codereview.chromium.org/1325213002/diff/70001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:73376: +  <int value="3" label="Session,
No Warmup, Not called"/>
I thin this would be clearer:

s/, Not called//

[Benoit L, 2015-09-29 10:07:41]

lizeb@chromium.org changed reviewers:
+ isherman@chromium.org

[Benoit L, 2015-09-29 10:07:42]

Hello isherman,

This is a new histogram to more precisely assess how client application use the
new Custom Tabs feature on Android.
Please take a look, thanks.

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

https://codereview.chromium.org/1325213002/diff/70001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:73375: +  <int value="2" label="Session,
No Warmup, Called elsewhere"/>
On 2015/09/29 09:50:07, pasko wrote:
> can we make 'elsewhere' clearer? Like:
> "Session, No Warmup, Warmup called from another Uid"

Done.

https://codereview.chromium.org/1325213002/diff/70001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:73376: +  <int value="3" label="Session,
No Warmup, Not called"/>
On 2015/09/29 09:50:07, pasko wrote:
> I thin this would be clearer:
> 
> s/, Not called//

Done.

[Ilya Sherman, 2015-09-30 04:10:26]

histograms lgtm

[Benoit L, 2015-09-30 11:16:28]

The CQ bit was checked by lizeb@chromium.org

[Benoit L, 2015-09-30 11:16:29]

The patchset sent to the CQ was uploaded after l-g-t-m from pasko@chromium.org,
isherman@chromium.org
Link to the patchset: https://codereview.chromium.org/1325213002/#ps110001
(title: "Make Findbugs happy.")

[commit-bot: I haz the power, 2015-09-30 11:17:02]

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

[commit-bot: I haz the power, 2015-09-30 11:25:42]

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

[commit-bot: I haz the power, 2015-09-30 11:25:43]

Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[Benoit L, 2015-09-30 11:46:03]

The CQ bit was checked by lizeb@chromium.org

[Benoit L, 2015-09-30 11:46:04]

The patchset sent to the CQ was uploaded after l-g-t-m from pasko@chromium.org,
isherman@chromium.org
Link to the patchset: https://codereview.chromium.org/1325213002/#ps130001
(title: "Rebase.")

[commit-bot: I haz the power, 2015-09-30 11:46:22]

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

[commit-bot: I haz the power, 2015-09-30 13:35:39]

Committed patchset #8 (id:130001)

[commit-bot: I haz the power, 2015-09-30 13:36:51]

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