[TTS] Add onTouchDown to GestureStateListener.

[TTS] Add onTouchDown to GestureStateListener.

Adds messaging from ContentViewCore to GestureStateListeners when any
TouchDown occurs to allow timing of Tap events for Contextual Search.

Adds a heurisitc to Contextual Search to categorize a Tap by duration.
CS will use the Tap duration as a signal for machine learning
with the hope that accidental taps are short, and deliberate taps tend
to be longer in duration.  Log histograms for short and long duration
tap CTR, and histograms of durations for seen and unseen to UMA.

BUG=723194, 444114
Review-Url: https://codereview.chromium.org/2932623002
Cr-Commit-Position: refs/heads/master@{#486900}
Committed: https://chromium.googlesource.com/chromium/src/+/4d1e08b84b36729cf3d0fee9ab77028e9af0a8d8

[Donn Denman, 2017-06-08 00:46:17]

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

[commit-bot: I haz the power, 2017-06-08 00:46:34]

Dry run: 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-06-08 03:05:50]

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

[commit-bot: I haz the power, 2017-06-08 03:05:51]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[Donn Denman, 2017-06-30 01:08:29]

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

[commit-bot: I haz the power, 2017-06-30 01:08:44]

Dry run: CQ is trying da patch.

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

[Donn Denman, 2017-06-30 01:08:51]

donnd@chromium.org changed reviewers:
+ twellington@chromium.org

[Donn Denman, 2017-06-30 01:08:51]

Theresa, PTAL at your convenience.  Thanks!

[commit-bot: I haz the power, 2017-06-30 03:14:53]

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

[commit-bot: I haz the power, 2017-06-30 03:14:54]

Dry run: This issue passed the CQ dry run.

[Theresa, 2017-07-05 20:35:55]

https://codereview.chromium.org/2932623002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/TapDurationSuppression.java
(right):

https://codereview.chromium.org/2932623002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/TapDurationSuppression.java:42:
// TODO(donnd): Log the actual tap duration to Ranker once we have
privacy-approval!
Could we log the duration to UMA before we have privacy approval to get an early
indication of the distribution of tap lengths?

https://codereview.chromium.org/2932623002/diff/20001/content/public/android/...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
(right):

https://codereview.chromium.org/2932623002/diff/20001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:842:
mGestureStateListenersIterator.next().onTapDown();
Per the warning at the top of this file, we shouldn't be adding new stuff to
ContentViewCore. We could sync with the CVC owners to figure out whether it's
okay to add this here.

[Donn Denman, 2017-07-12 02:41:28]

donnd@chromium.org changed reviewers:
+ jinsukkim@chromium.org

[Donn Denman, 2017-07-12 02:41:28]

Jinsuk, PTAL at the overall idea of adding an onTapDown to GestureStateListener
given that it goes through CVC and you're refactoring that.

Theresa, still working on this.  I'll let you know when it's ready for a review.

https://codereview.chromium.org/2932623002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/TapDurationSuppression.java
(right):

https://codereview.chromium.org/2932623002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/TapDurationSuppression.java:42:
// TODO(donnd): Log the actual tap duration to Ranker once we have
privacy-approval!
On 2017/07/05 20:35:54, Theresa wrote:
> Could we log the duration to UMA before we have privacy approval to get an
early
> indication of the distribution of tap lengths?

Great idea, done.

https://codereview.chromium.org/2932623002/diff/20001/content/public/android/...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
(right):

https://codereview.chromium.org/2932623002/diff/20001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:842:
mGestureStateListenersIterator.next().onTapDown();
On 2017/07/05 20:35:54, Theresa wrote:
> Per the warning at the top of this file, we shouldn't be adding new stuff to
> ContentViewCore. We could sync with the CVC owners to figure out whether it's
> okay to add this here.

Adding jinsukkim@chromium.org to figure out whether it's OK.

[Donn Denman, 2017-07-12 23:26:08]

Description was changed from

==========
[TTS] Add onTapDown to GestureStateListener.

Adds messaging from ContentViewCore to GestureStateListeners when any
TapDown occurs to allow timing of Tap events for Contextual Search.

Adds a heurisitc to Contextual Search to categorize a Tap by duration.
CS will use the Tap duration as a signal for machine learning
with the hope that accidental taps are short, and deliberate taps tend
to be longer in duration.  Log histograms for short and long duration
tap CTR to UMA.

BUG=723194,444114
==========

to

==========
[TTS] Add onTouchDown to GestureStateListener.

Adds messaging from ContentViewCore to GestureStateListeners when any
TouchDown occurs to allow timing of Tap events for Contextual Search.

Adds a heurisitc to Contextual Search to categorize a Tap by duration.
CS will use the Tap duration as a signal for machine learning
with the hope that accidental taps are short, and deliberate taps tend
to be longer in duration.  Log histograms for short and long duration
tap CTR, and histograms of durations for seen and unseen to UMA.

BUG=723194,444114
==========

[Donn Denman, 2017-07-12 23:26:09]

donnd@chromium.org changed reviewers:
+ tedchoc@chromium.org
- jinsukkim@chromium.org

[Donn Denman, 2017-07-12 23:26:59]

Removed Jinsuk since OOO.

Ted, PTAL at ContentViewCore.java.

Theresa, PTAL at everything.

Thanks!

[Donn Denman, 2017-07-12 23:28:29]

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

[commit-bot: I haz the power, 2017-07-12 23:28:40]

Dry run: CQ is trying da patch.

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

[Donn Denman, 2017-07-12 23:29:15]

On 2017/07/12 23:26:59, Donn Denman wrote:
> Ted, PTAL at ContentViewCore.java.
And GestureStateListener.java please!

[Ted C, 2017-07-12 23:44:43]

On 2017/07/12 23:29:15, Donn Denman wrote:
> On 2017/07/12 23:26:59, Donn Denman wrote:
> > Ted, PTAL at ContentViewCore.java.
> And GestureStateListener.java please!

content/android - lgtm

[commit-bot: I haz the power, 2017-07-13 01:29:30]

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

[commit-bot: I haz the power, 2017-07-13 01:29:31]

Dry run: Try jobs failed on following builders:
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_TIMED_OUT, build has not started yet;
builder either lacks capacity or does not exist (misspelled?))

[Theresa, 2017-07-13 16:14:11]

https://codereview.chromium.org/2932623002/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchSelectionController.java
(left):

https://codereview.chromium.org/2932623002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchSelectionController.java:327:
mTapTimeNanoseconds = System.nanoTime();
It looks like we used this value for a few things. Do they all still work as
expected now that we record on touch down?

https://codereview.chromium.org/2932623002/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchUma.java
(right):

https://codereview.chromium.org/2932623002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchUma.java:835:
RecordHistogram.recordCustomCountHistogram(histogram, durationMs, min, max,
numBuckets);
Doing the

if (isTapShort) {
 
RecordHistogram.recordCustomCountHistogram("Search.ContextualSearchTapDurationSeen"...
} else {
 
RecordHistogram.recordCustomCountHistogram("Search.ContextualSearchTapDurationNotSeen"...
}

Allows our presubmit checks to detect missing histograms, so I have a slight
preference for in-lining the strings.

[Donn Denman, 2017-07-13 18:09:04]

donnd@chromium.org changed reviewers:
+ rkaplow@chromium.org

[Donn Denman, 2017-07-13 18:09:05]

Robert, PTAL at histograms.xml.  

Thanks everyone!

https://codereview.chromium.org/2932623002/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchSelectionController.java
(left):

https://codereview.chromium.org/2932623002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchSelectionController.java:327:
mTapTimeNanoseconds = System.nanoTime();
On 2017/07/13 16:14:11, Theresa wrote:
> It looks like we used this value for a few things. Do they all still work as
> expected now that we record on touch down?

I noticed this during development too and traced the usage down to the
before-scroll suppression and the tap-time sent to various heuristics.  These
all seemed like they could tolerate an earlier tap time.  

However, I just double-checked the before-scroll and see that it's not actually
enabled!  Filed a bug to fix that and double-check and adjust all these delays
and tasks (crbug.com/742425).  

I'll give everything a quick run-through before landing this.  Assuming that
goes well I'm pretty sure this change is OK, and I'll double-check everything
when I do the before-scroll and tuning, hopefully before the branch.

https://codereview.chromium.org/2932623002/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchUma.java
(right):

https://codereview.chromium.org/2932623002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchUma.java:835:
RecordHistogram.recordCustomCountHistogram(histogram, durationMs, min, max,
numBuckets);
On 2017/07/13 16:14:11, Theresa wrote:
> Doing the
> 
> if (isTapShort) {
>  
>
RecordHistogram.recordCustomCountHistogram("Search.ContextualSearchTapDurationSeen"...
> } else {
>  
>
RecordHistogram.recordCustomCountHistogram("Search.ContextualSearchTapDurationNotSeen"...
> }
> 
> Allows our presubmit checks to detect missing histograms, so I have a slight
> preference for in-lining the strings.

Oh, I didn't know our presubmits can catch missing histograms!  Do you happen to
know if this is a new presubmit, because a CL that I landed about a month ago
was missing a histogram (crbug.com/740725 has details on that).

Done!

[Theresa, 2017-07-13 21:11:28]

https://codereview.chromium.org/2932623002/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchUma.java
(right):

https://codereview.chromium.org/2932623002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchUma.java:835:
RecordHistogram.recordCustomCountHistogram(histogram, durationMs, min, max,
numBuckets);
On 2017/07/13 18:09:05, Donn Denman wrote:
> On 2017/07/13 16:14:11, Theresa wrote:
> > Doing the
> > 
> > if (isTapShort) {
> >  
> >
>
RecordHistogram.recordCustomCountHistogram("Search.ContextualSearchTapDurationSeen"...
> > } else {
> >  
> >
>
RecordHistogram.recordCustomCountHistogram("Search.ContextualSearchTapDurationNotSeen"...
> > }
> > 
> > Allows our presubmit checks to detect missing histograms, so I have a slight
> > preference for in-lining the strings.
> 
> Oh, I didn't know our presubmits can catch missing histograms!  Do you happen
to
> know if this is a new presubmit, because a CL that I landed about a month ago
> was missing a histogram (crbug.com/740725 has details on that).
> 
> Done!

Hm, no, it's been there as long as I can remember. Maybe it's broken right now.

https://codereview.chromium.org/2932623002/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchUma.java
(right):

https://codereview.chromium.org/2932623002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchUma.java:832:
if (durationMs < min || durationMs >= max) return;
I think we should let it record any overflow. We expect it to be at least 1 and
at most 500, but if it's more than 500 and ends up in the 500 bucket, that's
useful information for our distribution. Similarly, if it's less than 1ms (not
sure how, but maybe it's possible!) we need to capture that in our distribution
as well.

[Donn Denman, 2017-07-13 22:23:02]

donnd@chromium.org changed reviewers:
+ holte@chromium.org
- rkaplow@chromium.org

[Donn Denman, 2017-07-13 22:23:03]

-rkaplow +holte for histograms.xml

Theresa, PTAL.  Thanks!

https://codereview.chromium.org/2932623002/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchUma.java
(right):

https://codereview.chromium.org/2932623002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchUma.java:835:
RecordHistogram.recordCustomCountHistogram(histogram, durationMs, min, max,
numBuckets);
On 2017/07/13 21:11:27, Theresa wrote:
> On 2017/07/13 18:09:05, Donn Denman wrote:
> > On 2017/07/13 16:14:11, Theresa wrote:
> > > Doing the
> > > 
> > > if (isTapShort) {
> > >  
> > >
> >
>
RecordHistogram.recordCustomCountHistogram("Search.ContextualSearchTapDurationSeen"...
> > > } else {
> > >  
> > >
> >
>
RecordHistogram.recordCustomCountHistogram("Search.ContextualSearchTapDurationNotSeen"...
> > > }
> > > 
> > > Allows our presubmit checks to detect missing histograms, so I have a
slight
> > > preference for in-lining the strings.
> > 
> > Oh, I didn't know our presubmits can catch missing histograms!  Do you
happen
> to
> > know if this is a new presubmit, because a CL that I landed about a month
ago
> > was missing a histogram (crbug.com/740725 has details on that).
> > 
> > Done!
> 
> Hm, no, it's been there as long as I can remember. Maybe it's broken right
now.

OK.  Even if it's broken now I think we're doing the right thing to try to
facilitate this check.  Thanks!

https://codereview.chromium.org/2932623002/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchUma.java
(right):

https://codereview.chromium.org/2932623002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchUma.java:832:
if (durationMs < min || durationMs >= max) return;
On 2017/07/13 21:11:28, Theresa wrote:
> I think we should let it record any overflow. We expect it to be at least 1
and
> at most 500, but if it's more than 500 and ends up in the 500 bucket, that's
> useful information for our distribution. Similarly, if it's less than 1ms (not
> sure how, but maybe it's possible!) we need to capture that in our
distribution
> as well.

OK.  I'll just remove this range-check-and-return line, since overflow buckets
seem to be automatic.

Details: The Java documentation for recordCustomCountHistogram says the values
are required to be within the range of (min, max] and the min should be at least
1.  However I see from reading the C++ macro in histogram_macros.h that there
are overflow buckets that will take values outside the range. I see from our
ContextualSearchDurationBetweenTriggerAndScrollNotSeen histogram that there are
actually values in the 0 bucket even though we use 1 for the min, so I'll just
rely on the histograms to handle the overflow.

Also, changed the max to 1000 and buckets to 100 since 1000 is the upper limit
before a tap becomes a long-press.

[Theresa, 2017-07-13 22:26:12]

lgtm

https://codereview.chromium.org/2932623002/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchUma.java
(right):

https://codereview.chromium.org/2932623002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchUma.java:832:
if (durationMs < min || durationMs >= max) return;
On 2017/07/13 22:23:03, Donn Denman wrote:
> On 2017/07/13 21:11:28, Theresa wrote:
> > I think we should let it record any overflow. We expect it to be at least 1
> and
> > at most 500, but if it's more than 500 and ends up in the 500 bucket, that's
> > useful information for our distribution. Similarly, if it's less than 1ms
(not
> > sure how, but maybe it's possible!) we need to capture that in our
> distribution
> > as well.
> 
> OK.  I'll just remove this range-check-and-return line, since overflow buckets
> seem to be automatic.
> 
> Details: The Java documentation for recordCustomCountHistogram says the values
> are required to be within the range of (min, max] and the min should be at
least
> 1.  However I see from reading the C++ macro in histogram_macros.h that there
> are overflow buckets that will take values outside the range. I see from our
> ContextualSearchDurationBetweenTriggerAndScrollNotSeen histogram that there
are
> actually values in the 0 bucket even though we use 1 for the min, so I'll just
> rely on the histograms to handle the overflow.
> 
> Also, changed the max to 1000 and buckets to 100 since 1000 is the upper limit
> before a tap becomes a long-press.

Acknowledged.

[Donn Denman, 2017-07-14 16:40:24]

donnd@chromium.org changed reviewers:
+ rkaplow@chromium.org
- holte@chromium.org

[Donn Denman, 2017-07-14 16:40:25]

rkaplow@ PTAL at histograms.xml.  Thanks!

[Donn Denman, 2017-07-14 16:40:35]

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

[commit-bot: I haz the power, 2017-07-14 16:40:51]

Dry run: 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-07-14 18:42:42]

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

[commit-bot: I haz the power, 2017-07-14 18:42:43]

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

[Steven Holte, 2017-07-14 20:54:40]

holte@chromium.org changed reviewers:
+ holte@chromium.org

[Steven Holte, 2017-07-14 20:54:41]

histograms lgtm

[Donn Denman, 2017-07-14 20:55:27]

The CQ bit was checked by donnd@chromium.org

[Donn Denman, 2017-07-14 20:55:27]

The patchset sent to the CQ was uploaded after l-g-t-m from tedchoc@chromium.org
Link to the patchset: https://codereview.chromium.org/2932623002/#ps80001
(title: "Adjusted the custom histograms to write all values and use a max of
1000 with 100 buckets.")

[commit-bot: I haz the power, 2017-07-14 20:55:41]

CQ is trying da patch.

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

[Donn Denman, 2017-07-14 20:55:59]

Trying to commit now...Thanks Steven!

[commit-bot: I haz the power, 2017-07-14 21:56:03]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1500065727229050,
"parent_rev": "e9c5fa189911bbf346e19eae0aaa584ea655aa42", "commit_rev":
"4d1e08b84b36729cf3d0fee9ab77028e9af0a8d8"}

[commit-bot: I haz the power, 2017-07-14 21:56:24]

Description was changed from

==========
[TTS] Add onTouchDown to GestureStateListener.

Adds messaging from ContentViewCore to GestureStateListeners when any
TouchDown occurs to allow timing of Tap events for Contextual Search.

Adds a heurisitc to Contextual Search to categorize a Tap by duration.
CS will use the Tap duration as a signal for machine learning
with the hope that accidental taps are short, and deliberate taps tend
to be longer in duration.  Log histograms for short and long duration
tap CTR, and histograms of durations for seen and unseen to UMA.

BUG=723194,444114
==========

to

==========
[TTS] Add onTouchDown to GestureStateListener.

Adds messaging from ContentViewCore to GestureStateListeners when any
TouchDown occurs to allow timing of Tap events for Contextual Search.

Adds a heurisitc to Contextual Search to categorize a Tap by duration.
CS will use the Tap duration as a signal for machine learning
with the hope that accidental taps are short, and deliberate taps tend
to be longer in duration.  Log histograms for short and long duration
tap CTR, and histograms of durations for seen and unseen to UMA.

BUG=723194,444114

Review-Url: https://codereview.chromium.org/2932623002
Cr-Commit-Position: refs/heads/master@{#486900}
Committed:
https://chromium.googlesource.com/chromium/src/+/4d1e08b84b36729cf3d0fee9ab77...
==========

[commit-bot: I haz the power, 2017-07-14 21:56:26]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/4d1e08b84b36729cf3d0fee9ab77...