Add a UMA counter for tap disambiguation result.

Add a UMA counter for tap disambiguation result.

This is to collect data informing a possible removal of this feature.
In particular, only if the user tapped inside the popup can it be
considered to be a (possibly) successful UX; if the user dismissed it by
tapping outside or pressing the back button, then it can be considered
to have caused more harm than good.

BUG=705117
Review-Url: https://codereview.chromium.org/2775013002
Cr-Commit-Position: refs/heads/master@{#460013}
Committed: https://chromium.googlesource.com/chromium/src/+/642318401fc47bea352b860ab8786854abb7c314

[aelias_OOO_until_Jul13, 2017-03-24 22:06:37]

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

[commit-bot: I haz the power, 2017-03-24 22:07:00]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[aelias_OOO_until_Jul13, 2017-03-24 22:09:03]

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

[aelias_OOO_until_Jul13, 2017-03-24 22:09:03]

Hi isherman@, PTAL.

[Ilya Sherman, 2017-03-24 22:16:57]

https://codereview.chromium.org/2775013002/diff/1/content/public/android/java...
File
content/public/android/java/src/org/chromium/content/browser/PopupZoomer.java
(right):

https://codereview.chromium.org/2775013002/diff/1/content/public/android/java...
content/public/android/java/src/org/chromium/content/browser/PopupZoomer.java:53:
// tools/metrics/histograms/histograms.xml.
Please also document that, since it is used to back an UMA histogram, the enum
should be treated as append-only.

https://codereview.chromium.org/2775013002/diff/1/content/public/android/java...
content/public/android/java/src/org/chromium/content/browser/PopupZoomer.java:56:
private static final int UMA_TAPDISAMBIGUATION_TAPPEDOUTSIDE = 1;
nit: Why do you not have underscores between "TAP" and "DISAMBIGUATION" and
between "TAPPED" and "OUTSIDE"?

https://codereview.chromium.org/2775013002/diff/1/content/public/android/java...
content/public/android/java/src/org/chromium/content/browser/PopupZoomer.java:550:
UMA_TAPDISAMBIGUATION_TAPPEDOUTSIDE, UMA_TAPDISAMBIGUATION_COUNT);
nit: It's generally a best-practice to have one call-site per histogram, i.e. to
create a small wrapper function.  In Java, this mostly just reduces the chances
of code path drift.  (In C++, there are also some binary size improvements; not
sure whether those apply to Java to some extent as well.)

https://codereview.chromium.org/2775013002/diff/1/tools/metrics/histograms/hi...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2775013002/diff/1/tools/metrics/histograms/hi...
tools/metrics/histograms/histograms.xml:51512: +<histogram
name="PopupZoomer.TapDisambiguation" enum="TapDisambiguation">
It looks like you're adding a new top-level category name, "PopupZoomer".  This
doesn't seem like a sufficiently general name to be a top-level category. 
Please consider whether there isn't an existing category that's a good fit
already; and if not, please try to come up with a top-level category name that's
more general/intuitive for someone who isn't on your immediate team.

[aelias_OOO_until_Jul13, 2017-03-24 22:29:25]

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

[commit-bot: I haz the power, 2017-03-24 22:29:49]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[aelias_OOO_until_Jul13, 2017-03-24 22:30:31]

Thanks for the quick review.

https://codereview.chromium.org/2775013002/diff/1/content/public/android/java...
File
content/public/android/java/src/org/chromium/content/browser/PopupZoomer.java
(right):

https://codereview.chromium.org/2775013002/diff/1/content/public/android/java...
content/public/android/java/src/org/chromium/content/browser/PopupZoomer.java:53:
// tools/metrics/histograms/histograms.xml.
On 2017/03/24 at 22:16:56, Ilya Sherman wrote:
> Please also document that, since it is used to back an UMA histogram, the enum
should be treated as append-only.

Done.

https://codereview.chromium.org/2775013002/diff/1/content/public/android/java...
content/public/android/java/src/org/chromium/content/browser/PopupZoomer.java:56:
private static final int UMA_TAPDISAMBIGUATION_TAPPEDOUTSIDE = 1;
On 2017/03/24 at 22:16:56, Ilya Sherman wrote:
> nit: Why do you not have underscores between "TAP" and "DISAMBIGUATION" and
between "TAPPED" and "OUTSIDE"?

I prefer to use the underscore as a category separator (type vs value) rather
than as a word separator.

https://codereview.chromium.org/2775013002/diff/1/content/public/android/java...
content/public/android/java/src/org/chromium/content/browser/PopupZoomer.java:550:
UMA_TAPDISAMBIGUATION_TAPPEDOUTSIDE, UMA_TAPDISAMBIGUATION_COUNT);
On 2017/03/24 at 22:16:57, Ilya Sherman wrote:
> nit: It's generally a best-practice to have one call-site per histogram, i.e.
to create a small wrapper function.  In Java, this mostly just reduces the
chances of code path drift.  (In C++, there are also some binary size
improvements; not sure whether those apply to Java to some extent as well.)

Done.

https://codereview.chromium.org/2775013002/diff/1/tools/metrics/histograms/hi...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2775013002/diff/1/tools/metrics/histograms/hi...
tools/metrics/histograms/histograms.xml:51512: +<histogram
name="PopupZoomer.TapDisambiguation" enum="TapDisambiguation">
On 2017/03/24 at 22:16:57, Ilya Sherman wrote:
> It looks like you're adding a new top-level category name, "PopupZoomer". 
This doesn't seem like a sufficiently general name to be a top-level category. 
Please consider whether there isn't an existing category that's a good fit
already; and if not, please try to come up with a top-level category name that's
more general/intuitive for someone who isn't on your immediate team.

OK, switched to existing category "Touchscreen".

[Ilya Sherman, 2017-03-24 22:46:35]

Metrics LGTM

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

https://codereview.chromium.org/2775013002/diff/20001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:112316: +  <int value="0"
label="TappedInside"/>
Optional nit: It's fine to write this as "Tapped inside" if you'd like.

[aelias_OOO_until_Jul13, 2017-03-24 22:52:14]

The CQ bit was checked by aelias@chromium.org

[aelias_OOO_until_Jul13, 2017-03-24 22:52:14]

The patchset sent to the CQ was uploaded after l-g-t-m from
isherman@chromium.org
Link to the patchset: https://codereview.chromium.org/2775013002/#ps40001
(title: "Put spaces in labels and move Other to 0 for cleaner appends")

[commit-bot: I haz the power, 2017-03-24 22:53:02]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-25 01:56:01]

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

[commit-bot: I haz the power, 2017-03-25 01:56:02]

Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[aelias_OOO_until_Jul13, 2017-03-28 01:10:28]

The CQ bit was checked by aelias@chromium.org

[aelias_OOO_until_Jul13, 2017-03-28 01:10:29]

The patchset sent to the CQ was uploaded after l-g-t-m from
isherman@chromium.org
Link to the patchset: https://codereview.chromium.org/2775013002/#ps60001
(title: "Make TappedInside the last value for possible append of subdivision of
it later")

[commit-bot: I haz the power, 2017-03-28 01:11:02]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-28 04:41:09]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1490663428444900,
"parent_rev": "dadec57db629fb81fea6341d949d5a3f0f27eb9c", "commit_rev":
"642318401fc47bea352b860ab8786854abb7c314"}

[commit-bot: I haz the power, 2017-03-28 04:41:52]

Description was changed from

==========
Add a UMA counter for tap disambiguation result.

This is to collect data informing a possible removal of this feature.
In particular, only if the user tapped inside the popup can it be
considered to be a (possibly) successful UX; if the user dismissed it by
tapping outside or pressing the back button, then it can be considered
to have caused more harm than good.

BUG=705117
==========

to

==========
Add a UMA counter for tap disambiguation result.

This is to collect data informing a possible removal of this feature.
In particular, only if the user tapped inside the popup can it be
considered to be a (possibly) successful UX; if the user dismissed it by
tapping outside or pressing the back button, then it can be considered
to have caused more harm than good.

BUG=705117

Review-Url: https://codereview.chromium.org/2775013002
Cr-Commit-Position: refs/heads/master@{#460013}
Committed:
https://chromium.googlesource.com/chromium/src/+/642318401fc47bea352b860ab878...
==========

[commit-bot: I haz the power, 2017-03-28 04:41:53]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/642318401fc47bea352b860ab878...