[TTS] Record CTR by week and 28-day intervals.

[TTS] Record CTR by week and 28-day intervals.

Adds recording of CTR by user for Contextual Search.
Updates the TapSuppressionHeuristics to support destroy() so native destructors can be called.
Updates ContextualSearchSelectionController to call destroy() on the heuristics.
Adds 4 new histograms for Impressions and CTR by week and 28day interval.

BUG=609668
Committed: https://crrev.com/1d909b62c4c6103d5f513500e054d1a95a1f8277
Cr-Commit-Position: refs/heads/master@{#422200}

[Donn Denman, 2016-08-26 16:49:23]

Theresa and Mark, just cc-ing you on this patch which uses the CTR aggregator so
you can see example usage for Android.

[Theresa, 2016-08-26 23:32:19]

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

[Theresa, 2016-08-26 23:32:20]

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

https://codereview.chromium.org/2285633004/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchUma.java:1146:
public static void logPreviousWeekCTR(float previousWeekCTR) {
Should we do something to keep track of whether the previous week CTR has
already been logged so that we don't record it numerous times?

[Donn Denman, 2016-08-29 21:37:41]

Description was changed from

==========
[TTS] Record CTR by week and 28-day intervals.

Adds recording of impressions and CTR by user for Contextual Search.
Still needed: write histograms for users CTR once their data is complete.

BUG=609668
==========

to

==========
[TTS] Record CTR by week and 28-day intervals.

Adds recording of CTR by user for the previous week and 28-day interval,
for Contextual Search.

BUG=609668
==========

[Donn Denman, 2016-08-29 23:07:25]

Theresa, I think this is ready for a real review now.

Also added the histograms to histograms.xml.

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

https://codereview.chromium.org/2285633004/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchUma.java:1146:
public static void logPreviousWeekCTR(float previousWeekCTR) {
On 2016/08/26 23:32:20, Theresa Wellington wrote:
> Should we do something to keep track of whether the previous week CTR has
> already been logged so that we don't record it numerous times?

Good idea, but I don't think the caching should be done here.  I added code to
CTRRecord.java to do cache.

[Theresa, 2016-08-29 23:52:14]

A couple of comments, will do a more thorough review when the native patch lands
(will look at that one now)

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

https://codereview.chromium.org/2285633004/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/CTRRecorder.java:67:
sPreviousWeekCTRCache = previousWeekCTR;
Multiple writes will still be possible, but not across a single session, right?
I think that's okay, we just need to be aware that this will be logged multiple
times per user and that we probably want to look only at user counts.

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

https://codereview.chromium.org/2285633004/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchUma.java:1147:
RecordHistogram.recordCount100Histogram(
Replace recordCount100Histogram() with recordPercentageHistogram()?

[Donn Denman, 2016-08-30 00:15:54]

Thanks for the review, PTAL when not too busy.

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

https://codereview.chromium.org/2285633004/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/CTRRecorder.java:67:
sPreviousWeekCTRCache = previousWeekCTR;
On 2016/08/29 23:52:14, Theresa Wellington wrote:
> Multiple writes will still be possible, but not across a single session,
right?
> I think that's okay, we just need to be aware that this will be logged
multiple
> times per user and that we probably want to look only at user counts.

Yep, I was thinking the same thing.  Updated the comment above to reflect this.

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

https://codereview.chromium.org/2285633004/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchUma.java:1147:
RecordHistogram.recordCount100Histogram(
On 2016/08/29 23:52:14, Theresa Wellington wrote:
> Replace recordCount100Histogram() with recordPercentageHistogram()?

Done for both.  Good catch, I think this actually treats the value 100 correctly
but the recordCount100Histogram may not.

[marq (ping after 24h), 2016-08-30 08:38:12]

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

[marq (ping after 24h), 2016-08-30 08:38:13]

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

https://codereview.chromium.org/2285633004/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/CTRRecorder.java:61:
protected void logConditionState() {
Is there some reason this logic is in the Android layer and not part of the
shared component?

https://codereview.chromium.org/2285633004/diff/60001/chrome/chrome_browser.gypi
File chrome/chrome_browser.gypi (right):

https://codereview.chromium.org/2285633004/diff/60001/chrome/chrome_browser.g...
chrome/chrome_browser.gypi:805:
'browser/android/contextualsearch/ctr_recorder.cc',
BUILD.gn change corresponding to this?

[Donn Denman, 2016-08-30 16:49:24]

Thanks for the comments Mark!

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

https://codereview.chromium.org/2285633004/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/CTRRecorder.java:61:
protected void logConditionState() {
On 2016/08/30 08:38:13, marq wrote:
> Is there some reason this logic is in the Android layer and not part of the
> shared component?

Good question.  Our current native code doesn't know anything about whether the
panel was seen or not, and that's the critical part of what we're logging here. 
 Having said that, we are planning to move the Tap suppression logic to native
this cycle, and we plan to move the "log-results-seen" logic too.  We should
take note that the "log-signals" should also be moved, so code like this doesn't
linger in the Android-only section when it could be shared.

Make sense?

https://codereview.chromium.org/2285633004/diff/60001/chrome/chrome_browser.gypi
File chrome/chrome_browser.gypi (right):

https://codereview.chromium.org/2285633004/diff/60001/chrome/chrome_browser.g...
chrome/chrome_browser.gypi:805:
'browser/android/contextualsearch/ctr_recorder.cc',
On 2016/08/30 08:38:13, marq wrote:
> BUILD.gn change corresponding to this?

I think that's automatically generated by this file.  I build using gn and it
seems to work.  I guess we'll find out for sure when I try to land this CL. :-)

[Donn Denman, 2016-08-31 04:37:54]

FYI new patch uploaded.  I'll ask you to PTAL when the base patch lands.

[Donn Denman, 2016-09-26 23:33:19]

Description was changed from

==========
[TTS] Record CTR by week and 28-day intervals.

Adds recording of CTR by user for the previous week and 28-day interval,
for Contextual Search.

BUG=609668
==========

to

==========
[TTS] Record CTR by week and 28-day intervals.

Adds recording of CTR by user for Contextual Search.
Updates the TapSuppressionHeuristics to support destroy() so native destructors
can be called.
Updates ContextualSearchSelectionController to call destroy() on the heuristics.
Adds 4 new histograms for Impressions and CTR by week and 28day interval.

BUG=609668
==========

[Donn Denman, 2016-09-26 23:33:19]

donnd@chromium.org changed reviewers:
- marq@chromium.org

[Donn Denman, 2016-09-26 23:34:02]

Theresa, this is ready for a review now (no rush).

[Theresa, 2016-09-27 20:53:57]

https://codereview.chromium.org/2285633004/diff/100001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/CTRRecorder.java
(right):

https://codereview.chromium.org/2285633004/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/CTRRecorder.java:21:
public class CTRRecorder extends ContextualSearchHeuristic {
Eventually this will be used for tap suppression, right? Maybe it should have a
different name than CTRRecorder, e.g. AggregatedCTRSuppression and the class
JavaDoc can indicate that it's currently only being used for logging and not
suppression.

https://codereview.chromium.org/2285633004/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/CTRRecorder.java:70:
// multiple writes to the UMA log.  Multiple writes will still happen across
multiple
Does it make sense to use a shared preference to keep track of the last week
recorded so that we aren't logging multiple times?

https://codereview.chromium.org/2285633004/diff/100001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchUma.java
(right):

https://codereview.chromium.org/2285633004/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchUma.java:1175:
* @param previousWeekImpressions The number of impressions the user saw.
nit: "The number of times the user saw the Contextual Search Bar."?

https://codereview.chromium.org/2285633004/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchUma.java:1179:
RecordHistogram.recordPercentageHistogram(
Impressions is just a regular number, not a percentage, right?

https://codereview.chromium.org/2285633004/diff/100001/chrome/browser/android...
File chrome/browser/android/contextualsearch/ctr_recorder.h (right):

https://codereview.chromium.org/2285633004/diff/100001/chrome/browser/android...
chrome/browser/android/contextualsearch/ctr_recorder.h:29: void
RecordImpression(JNIEnv* env,
For the methods that just call through to
components/contextual_search/browser/ctr_aggregator.h, I think it's fine to
group them together and reference that class in one comment rather than
duplicating the comments in the other ctr_aggregator.h.

https://codereview.chromium.org/2285633004/diff/100001/components/contextual_...
File components/contextual_search/browser/ctr_aggregator.h (right):

https://codereview.chromium.org/2285633004/diff/100001/components/contextual_...
components/contextual_search/browser/ctr_aggregator.h:47: // Returns the week
number for the current week, useful for checking when the
nit: "Returns the number for the current week. Useful for checking..."
?

[Donn Denman, 2016-09-28 00:38:44]

Theresa, PTAL:  Argh!  I realized during the renaming that using "CTR" instead
of "Ctr" is wrong, at least for Java class names
(http://source.android.com/source/code-style.html -- Treat acronyms as words),
and probably for C++ too.  So I renamed everything.  Luckily the C++ filenames
don't change.  Also good that I got my Eclipse setup going again, so using
"refactor" helped.

https://codereview.chromium.org/2285633004/diff/100001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/CTRRecorder.java
(right):

https://codereview.chromium.org/2285633004/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/CTRRecorder.java:21:
public class CTRRecorder extends ContextualSearchHeuristic {
On 2016/09/27 20:53:56, Theresa Wellington wrote:
> Eventually this will be used for tap suppression, right? Maybe it should have
a
> different name than CTRRecorder, e.g. AggregatedCTRSuppression and the class
> JavaDoc can indicate that it's currently only being used for logging and not
> suppression.

Changed to CTRSuppression.

https://codereview.chromium.org/2285633004/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/CTRRecorder.java:70:
// multiple writes to the UMA log.  Multiple writes will still happen across
multiple
On 2016/09/27 20:53:56, Theresa Wellington wrote:
> Does it make sense to use a shared preference to keep track of the last week
> recorded so that we aren't logging multiple times?

I don't think it's very bad to log multiple times to UMA, since we already need
to look at user-counts for some of our UMA data.

https://codereview.chromium.org/2285633004/diff/100001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchUma.java
(right):

https://codereview.chromium.org/2285633004/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchUma.java:1175:
* @param previousWeekImpressions The number of impressions the user saw.
On 2016/09/27 20:53:56, Theresa Wellington wrote:
> nit: "The number of times the user saw the Contextual Search Bar."?

Done, here and below.

https://codereview.chromium.org/2285633004/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchUma.java:1179:
RecordHistogram.recordPercentageHistogram(
On 2016/09/27 20:53:56, Theresa Wellington wrote:
> Impressions is just a regular number, not a percentage, right?

Nice catch!  Done here and below.

https://codereview.chromium.org/2285633004/diff/100001/chrome/browser/android...
File chrome/browser/android/contextualsearch/ctr_recorder.h (right):

https://codereview.chromium.org/2285633004/diff/100001/chrome/browser/android...
chrome/browser/android/contextualsearch/ctr_recorder.h:29: void
RecordImpression(JNIEnv* env,
On 2016/09/27 20:53:56, Theresa Wellington wrote:
> For the methods that just call through to
> components/contextual_search/browser/ctr_aggregator.h, I think it's fine to
> group them together and reference that class in one comment rather than
> duplicating the comments in the other ctr_aggregator.h.

Done.

https://codereview.chromium.org/2285633004/diff/100001/components/contextual_...
File components/contextual_search/browser/ctr_aggregator.h (right):

https://codereview.chromium.org/2285633004/diff/100001/components/contextual_...
components/contextual_search/browser/ctr_aggregator.h:47: // Returns the week
number for the current week, useful for checking when the
On 2016/09/27 20:53:57, Theresa Wellington wrote:
> nit: "Returns the number for the current week. Useful for checking..."
> ?

Done.

[Donn Denman, 2016-09-28 00:39:52]

Theresa, no rush on this review, can easily wait till tomorrow.

[Theresa, 2016-09-28 16:24:15]

lgtm

[Donn Denman, 2016-09-28 17:24:37]

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

[commit-bot: I haz the power, 2016-09-28 17:24:59]

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-28 17:28:56]

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

[Donn Denman, 2016-09-28 17:28:57]

+tedchoc@ for chrome_jni_registrar.cc
+rkaplow@ for histograms.xml

[commit-bot: I haz the power, 2016-09-28 18:53:45]

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

[commit-bot: I haz the power, 2016-09-28 18:53:46]

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

[Ted C, 2016-09-28 20:04:31]

On 2016/09/28 17:28:57, Donn Denman wrote:
> +tedchoc@ for chrome_jni_registrar.cc
> +rkaplow@ for histograms.xml

chrome_jni_registrar.cc - lgtm (although it seems you need to address a couple
findbugs issues from the clang bot)

[rkaplow, 2016-09-28 21:16:03]

lgtm

https://codereview.chromium.org/2285633004/diff/140001/tools/metrics/histogra...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2285633004/diff/140001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:53813: +    percentage. May be logged
multiple times for each user. Implemented for
can you mention how often is it recorded? SImilar for the other metrics

[Theresa, 2016-09-29 04:07:37]

https://codereview.chromium.org/2285633004/diff/140001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/CtrSuppression.java
(right):

https://codereview.chromium.org/2285633004/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/CtrSuppression.java:15:
* TODO(donnd): add suppression logic.
I had an additional thought on this. When we add suppression, we should consider
how to handle impressions of quick answers/insights/actions panels, especially
as we continue to add more of them.

For M55, since the caption is set with doesAnswer = false, panels that show a
"quick insight" will count toward the taps since last open. Maybe something we
should discuss in our next meeting?

[Donn Denman, 2016-09-30 03:05:54]

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

[commit-bot: I haz the power, 2016-09-30 03:06:17]

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-09-30 04:41:09]

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

[commit-bot: I haz the power, 2016-09-30 04:41:10]

Dry run: This issue passed the CQ dry run.

[Donn Denman, 2016-09-30 16:23:19]

Theresa, would you mind double-checking my work in the latest patch set?  

Thanks everyone!

https://codereview.chromium.org/2285633004/diff/140001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/CtrSuppression.java
(right):

https://codereview.chromium.org/2285633004/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/CtrSuppression.java:15:
* TODO(donnd): add suppression logic.
On 2016/09/29 04:07:37, Theresa Wellington wrote:
> I had an additional thought on this. When we add suppression, we should
consider
> how to handle impressions of quick answers/insights/actions panels, especially
> as we continue to add more of them.
> 
> For M55, since the caption is set with doesAnswer = false, panels that show a
> "quick insight" will count toward the taps since last open. Maybe something we
> should discuss in our next meeting?

Yes, let's discuss this in our next meeting.

https://codereview.chromium.org/2285633004/diff/140001/tools/metrics/histogra...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2285633004/diff/140001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:53813: +    percentage. May be logged
multiple times for each user. Implemented for
On 2016/09/28 21:16:03, rkaplow wrote:
> can you mention how often is it recorded? SImilar for the other metrics

Done.

[Theresa, 2016-09-30 19:23:17]

lgtm

[Donn Denman, 2016-09-30 20:08:29]

The CQ bit was checked by donnd@chromium.org

[Donn Denman, 2016-09-30 20:08:29]

The patchset sent to the CQ was uploaded after l-g-t-m from
tedchoc@chromium.org, rkaplow@chromium.org
Link to the patchset: https://codereview.chromium.org/2285633004/#ps160001
(title: "Reworked the cache to use a stored preference instead of session static
data.")

[commit-bot: I haz the power, 2016-09-30 20:08:57]

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-30 20:15:17]

Description was changed from

==========
[TTS] Record CTR by week and 28-day intervals.

Adds recording of CTR by user for Contextual Search.
Updates the TapSuppressionHeuristics to support destroy() so native destructors
can be called.
Updates ContextualSearchSelectionController to call destroy() on the heuristics.
Adds 4 new histograms for Impressions and CTR by week and 28day interval.

BUG=609668
==========

to

==========
[TTS] Record CTR by week and 28-day intervals.

Adds recording of CTR by user for Contextual Search.
Updates the TapSuppressionHeuristics to support destroy() so native destructors
can be called.
Updates ContextualSearchSelectionController to call destroy() on the heuristics.
Adds 4 new histograms for Impressions and CTR by week and 28day interval.

BUG=609668
==========

[commit-bot: I haz the power, 2016-09-30 20:15:20]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2016-09-30 20:18:01]

Description was changed from

==========
[TTS] Record CTR by week and 28-day intervals.

Adds recording of CTR by user for Contextual Search.
Updates the TapSuppressionHeuristics to support destroy() so native destructors
can be called.
Updates ContextualSearchSelectionController to call destroy() on the heuristics.
Adds 4 new histograms for Impressions and CTR by week and 28day interval.

BUG=609668
==========

to

==========
[TTS] Record CTR by week and 28-day intervals.

Adds recording of CTR by user for Contextual Search.
Updates the TapSuppressionHeuristics to support destroy() so native destructors
can be called.
Updates ContextualSearchSelectionController to call destroy() on the heuristics.
Adds 4 new histograms for Impressions and CTR by week and 28day interval.

BUG=609668

Committed: https://crrev.com/1d909b62c4c6103d5f513500e054d1a95a1f8277
Cr-Commit-Position: refs/heads/master@{#422200}
==========

[commit-bot: I haz the power, 2016-09-30 20:18:03]

Patchset 9 (id:??) landed as
https://crrev.com/1d909b62c4c6103d5f513500e054d1a95a1f8277
Cr-Commit-Position: refs/heads/master@{#422200}