[Contextual Search] Do not include tap suppression in aggregate heuristic logging

[Contextual Search] Do not include tap suppression in aggregate heuristic logging

Also default 'false' for shouldAggregateLogForTapSuppression and isConditionSatisfiedForAggregateLogging

BUG=642897
Committed: https://crrev.com/717e57dbbc6470b61dd01f3c80f7378638a33504
Cr-Commit-Position: refs/heads/master@{#422932}

[Theresa, 2016-10-04 00:27:33]

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

[Theresa, 2016-10-04 00:27:45]

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

[Theresa, 2016-10-04 00:27:45]

ptal

[commit-bot: I haz the power, 2016-10-04 00:28:05]

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, 2016-10-04 01:55:03]

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

[commit-bot: I haz the power, 2016-10-04 01:55:04]

Dry run: This issue passed the CQ dry run.

[Theresa, 2016-10-04 07:15:40]

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

[commit-bot: I haz the power, 2016-10-04 07:15:54]

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, 2016-10-04 07:42:52]

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

[commit-bot: I haz the power, 2016-10-04 07:42:52]

Dry run: This issue passed the CQ dry run.

[Donn Denman, 2016-10-04 17:18:45]

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

[Donn Denman, 2016-10-04 17:18:46]

lgtm

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

https://codereview.chromium.org/2386413002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/TapSuppression.java:68:
protected boolean shouldAggregateLogForTapSuppression() {
Do you think it would make sense for the base implementation to return false?

[Theresa, 2016-10-04 17:22:04]

On 2016/10/04 17:18:46, Donn Denman wrote:
> lgtm
> 
>
https://codereview.chromium.org/2386413002/diff/40001/chrome/android/java/src...
> File
>
chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/TapSuppression.java
> (right):
> 
>
https://codereview.chromium.org/2386413002/diff/40001/chrome/android/java/src...
>
chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/TapSuppression.java:68:
> protected boolean shouldAggregateLogForTapSuppression() {
> Do you think it would make sense for the base implementation to return false?

I could change shouldAggregateLogForTappSuppression and
isConditionSatisfiedForAggregateLogging to both default return to false.

[Donn Denman, 2016-10-04 17:47:43]

On 2016/10/04 17:22:04, Theresa Wellington wrote:
> On 2016/10/04 17:18:46, Donn Denman wrote:
> > lgtm
> > 
> >
>
https://codereview.chromium.org/2386413002/diff/40001/chrome/android/java/src...
> > File
> >
>
chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/TapSuppression.java
> > (right):
> > 
> >
>
https://codereview.chromium.org/2386413002/diff/40001/chrome/android/java/src...
> >
>
chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/TapSuppression.java:68:
> > protected boolean shouldAggregateLogForTapSuppression() {
> > Do you think it would make sense for the base implementation to return
false?
> 
> I could change shouldAggregateLogForTappSuppression and
> isConditionSatisfiedForAggregateLogging to both default return to false.

I don't feel strongly either way, up to you!

LGTM

[Theresa, 2016-10-04 18:22:43]

Description was changed from

==========
[Contextual Search] Do not include tap suppression in aggregate heuristic
logging

BUG=642897
==========

to

==========
[Contextual Search] Do not include tap suppression in aggregate heuristic
logging

Also default 'false' for shouldAggregateLogForTapSuppression and
isConditionSatisfiedForAggregateLogging

BUG=642897
==========

[Theresa, 2016-10-04 18:24:02]

Donn, will you please take another look at this? I changed it to return false
for shouldAggregateLogForTapSuppression() and
isConditionSatisfiedForAggregateLogging()

[Theresa, 2016-10-04 18:24:05]

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

[commit-bot: I haz the power, 2016-10-04 18:24:33]

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, 2016-10-04 18:44:06]

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

[commit-bot: I haz the power, 2016-10-04 18:44:08]

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...)

[Theresa, 2016-10-04 18:57:11]

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

[commit-bot: I haz the power, 2016-10-04 18:57:25]

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, 2016-10-04 20:03:21]

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

[commit-bot: I haz the power, 2016-10-04 20:03:22]

Dry run: This issue passed the CQ dry run.

[Donn Denman, 2016-10-04 20:55:30]

LGTM with one suggestion.

https://codereview.chromium.org/2386413002/diff/120001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/NearTopTapSuppression.java
(right):

https://codereview.chromium.org/2386413002/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/NearTopTapSuppression.java:49:
// TODO(twellington): Define a default value to use when determining if the
condition is
Let's just do this so we can get rid of the TODO?

I think this would work:
protected boolean isConditionSatisfiedForAggregateLogging() {
  return (mIsConditionSatisfied && mExperiementThresholdDps == 0); 
}

[Donn Denman, 2016-10-04 20:55:51]

donnd@chromium.org changed reviewers:
- donnd@google.com

[Theresa, 2016-10-04 21:10:37]

https://codereview.chromium.org/2386413002/diff/120001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/NearTopTapSuppression.java
(right):

https://codereview.chromium.org/2386413002/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/NearTopTapSuppression.java:49:
// TODO(twellington): Define a default value to use when determining if the
condition is
On 2016/10/04 20:55:29, Donn Denman wrote:
> Let's just do this so we can get rid of the TODO?
> 
> I think this would work:
> protected boolean isConditionSatisfiedForAggregateLogging() {
>   return (mIsConditionSatisfied && mExperiementThresholdDps == 0); 
> }

If mExperimentThreshold == 0 then mIsConditionSatisfied will always be false. I
think we need to do some analysis on the histograms we're collecting for near
top tap suppression to figure out a good default value. This is something simple
we may be able to merge back after we analyze.

[Theresa, 2016-10-04 21:10:54]

The CQ bit was checked by twellington@chromium.org

[commit-bot: I haz the power, 2016-10-04 21:11:46]

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-10-04 21:23:14]

Description was changed from

==========
[Contextual Search] Do not include tap suppression in aggregate heuristic
logging

Also default 'false' for shouldAggregateLogForTapSuppression and
isConditionSatisfiedForAggregateLogging

BUG=642897
==========

to

==========
[Contextual Search] Do not include tap suppression in aggregate heuristic
logging

Also default 'false' for shouldAggregateLogForTapSuppression and
isConditionSatisfiedForAggregateLogging

BUG=642897
==========

[commit-bot: I haz the power, 2016-10-04 21:23:16]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-10-04 21:25:10]

Description was changed from

==========
[Contextual Search] Do not include tap suppression in aggregate heuristic
logging

Also default 'false' for shouldAggregateLogForTapSuppression and
isConditionSatisfiedForAggregateLogging

BUG=642897
==========

to

==========
[Contextual Search] Do not include tap suppression in aggregate heuristic
logging

Also default 'false' for shouldAggregateLogForTapSuppression and
isConditionSatisfiedForAggregateLogging

BUG=642897

Committed: https://crrev.com/717e57dbbc6470b61dd01f3c80f7378638a33504
Cr-Commit-Position: refs/heads/master@{#422932}
==========

[commit-bot: I haz the power, 2016-10-04 21:25:11]

Patchset 7 (id:??) landed as
https://crrev.com/717e57dbbc6470b61dd01f3c80f7378638a33504
Cr-Commit-Position: refs/heads/master@{#422932}