[Contextual Search] Exclude suppressed taps from aggregate suppression heuristics logging

[Contextual Search] Exclude suppressed taps from aggregate suppression heuristics logging

We would like the aggregate suppression heuristic logging to determine
the impact  of signals we are collecting but not yet using to suppress.

Also changes BarOverlapTapSuppression to only consider the y-position
of the tap.

BUG=642897, 642962
Committed: https://crrev.com/e233f01811cda4d7e9e5f9d3cd5bd536492e5003
Cr-Commit-Position: refs/heads/master@{#420736}

[Theresa, 2016-09-23 20:12:05]

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

[Theresa, 2016-09-23 20:12:12]

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

[Theresa, 2016-09-23 20:12:13]

ptal

[commit-bot: I haz the power, 2016-09-23 20:13:01]

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

[Donn Denman, 2016-09-23 20:40:13]

LGTM with a few nits.

If you don't mind doing a little more cleanup, I think we should rename
isConditionSatisfied to something like isConditionSatisfiedAndEnabled and update
the javadoc for that interface.  If you'd rather not, that's fine, otherwise
that would be great!

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

https://codereview.chromium.org/2365653007/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchHeuristic.java:49:
*         return false if the feature is enabled through
VariationsAssociatedData.
Nit: please rephrase this second sentence to better connect with the previous
regarding enabling, e.g. "If the feature is enabled through
VariationsAssociatedData then this method should return false."

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

https://codereview.chromium.org/2365653007/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/RecentScrollTapSuppression.java:16:
private int mExperimentThresholdMs;
Nit: can't this stay final?  Maybe the constructor just needs a single
assignment statement for this to work -- sometimes I just put one at the end.

[commit-bot: I haz the power, 2016-09-23 20:45:18]

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

[commit-bot: I haz the power, 2016-09-23 20:45:19]

Dry run: This issue passed the CQ dry run.

[Theresa, 2016-09-23 20:57:30]

Did the renaming to isConditionSatisfiedAndEnabled() as well and updated the
comment

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

https://codereview.chromium.org/2365653007/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchHeuristic.java:49:
*         return false if the feature is enabled through
VariationsAssociatedData.
On 2016/09/23 20:40:13, Donn Denman wrote:
> Nit: please rephrase this second sentence to better connect with the previous
> regarding enabling, e.g. "If the feature is enabled through
> VariationsAssociatedData then this method should return false."

Done.

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

https://codereview.chromium.org/2365653007/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/RecentScrollTapSuppression.java:16:
private int mExperimentThresholdMs;
On 2016/09/23 20:40:13, Donn Denman wrote:
> Nit: can't this stay final?  Maybe the constructor just needs a single
> assignment statement for this to work -- sometimes I just put one at the end.

Done.

[Theresa, 2016-09-23 20:57:43]

The CQ bit was checked by twellington@chromium.org

[Theresa, 2016-09-23 20:57:43]

The patchset sent to the CQ was uploaded after l-g-t-m from donnd@chromium.org
Link to the patchset: https://codereview.chromium.org/2365653007/#ps40001
(title: "Changes from donnd@ review")

[commit-bot: I haz the power, 2016-09-23 20:58:20]

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

[Theresa, 2016-09-23 20:59:02]

The CQ bit was unchecked by twellington@chromium.org

[Donn Denman, 2016-09-23 20:59:04]

On 2016/09/23 20:57:30, Theresa Wellington wrote:
> Did the renaming to isConditionSatisfiedAndEnabled() as well and updated the
> comment
> 
>
https://codereview.chromium.org/2365653007/diff/20001/chrome/android/java/src...
> File
>
chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchHeuristic.java
> (right):
> 
>
https://codereview.chromium.org/2365653007/diff/20001/chrome/android/java/src...
>
chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchHeuristic.java:49:
> *         return false if the feature is enabled through
> VariationsAssociatedData.
> On 2016/09/23 20:40:13, Donn Denman wrote:
> > Nit: please rephrase this second sentence to better connect with the
previous
> > regarding enabling, e.g. "If the feature is enabled through
> > VariationsAssociatedData then this method should return false."
> 
> Done.
> 
>
https://codereview.chromium.org/2365653007/diff/20001/chrome/android/java/src...
> File
>
chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/RecentScrollTapSuppression.java
> (right):
> 
>
https://codereview.chromium.org/2365653007/diff/20001/chrome/android/java/src...
>
chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/RecentScrollTapSuppression.java:16:
> private int mExperimentThresholdMs;
> On 2016/09/23 20:40:13, Donn Denman wrote:
> > Nit: can't this stay final?  Maybe the constructor just needs a single
> > assignment statement for this to work -- sometimes I just put one at the
end.
> 
> Done.

Thanks so much for the cleanup Theresa!

[Theresa, 2016-09-23 20:59:50]

The CQ bit was checked by twellington@chromium.org

[Theresa, 2016-09-23 20:59:50]

The patchset sent to the CQ was uploaded after l-g-t-m from donnd@chromium.org
Link to the patchset: https://codereview.chromium.org/2365653007/#ps60001
(title: "Fix punctuation")

[commit-bot: I haz the power, 2016-09-23 21:00:27]

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, 2016-09-23 21:30:26]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-09-23 21:32:35]

Description was changed from

==========
[Contextual Search] Exclude suppressed taps from aggregate suppression
heuristics logging

We would like the aggregate suppression heuristic logging to determine
the impact  of signals we are collecting but not yet using to suppress.

Also changes BarOverlapTapSuppression to only consider the y-position
of the tap.

BUG=642897,642962
==========

to

==========
[Contextual Search] Exclude suppressed taps from aggregate suppression
heuristics logging

We would like the aggregate suppression heuristic logging to determine
the impact  of signals we are collecting but not yet using to suppress.

Also changes BarOverlapTapSuppression to only consider the y-position
of the tap.

BUG=642897,642962

Committed: https://crrev.com/e233f01811cda4d7e9e5f9d3cd5bd536492e5003
Cr-Commit-Position: refs/heads/master@{#420736}
==========

[commit-bot: I haz the power, 2016-09-23 21:32:36]

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