📰 Tweak the suggestion ranks for UMA to handle fetchMore

[NTP Client] Tweak the suggestion ranks for UMA to handle fetchMore


There are 2 types of suggestion positions we report to UMA:

- Within a given section: because we were attributing ranks in
the incoming bundle, it was not taking into account the suggestions
that had been pushed before. This number is now always increasing,
so after fetching more suggestions 5 times, the ranks for the 5th
bundle can be in the 40 to 49 range.

- Global rank: we also now attribute the rank as suggestions are
added to the section, and keep track of that number. It would be the
position of the suggestion if we kept adding suggestions always at the
end of the list rather than grouping them by category.

This way for a given NTP, there can be no overlap of global rank or local
rank within a section.

BUG=675623
Review-Url: https://codereview.chromium.org/2618893003
Cr-Commit-Position: refs/heads/master@{#444354}
Committed: https://chromium.googlesource.com/chromium/src/+/5549f302eeb5cc43d3e13435c3826587721ddee8

[dgn, 2017-01-10 12:22:00]

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

[commit-bot: I haz the power, 2017-01-10 12:22:07]

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-01-10 14:02:47]

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

[commit-bot: I haz the power, 2017-01-10 14:02:48]

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

[dgn, 2017-01-11 12:22:01]

dgn@chromium.org changed reviewers:
+ bauerb@chromium.org

[dgn, 2017-01-11 12:22:01]

PTAL. Is that something like that you were thinking about?

[Bernhard Bauer, 2017-01-11 13:53:38]

https://codereview.chromium.org/2618893003/diff/60001/chrome/android/java/src...
File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java
(right):

https://codereview.chromium.org/2618893003/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java:332: if
(sSuggestionsSourceForTests != null) return;
Could we instead move these tracking methods into an interface? It could be
SuggestionsSource, or we could add a new interface that SnippetsBridge
implements, to keep SuggestionsSource from blowing up too much.

https://codereview.chromium.org/2618893003/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetsBridge.java
(right):

https://codereview.chromium.org/2618893003/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetsBridge.java:27:
private SuggestionsUma mUmaHelper;
I think I would make this an interface and have SectionList directly implement
it.

https://codereview.chromium.org/2618893003/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetsBridge.java:124:
int suggestionRank = mUmaHelper.getSuggestionRank(suggestion);
If getSuggestionRank returns -1 when the suggestion is not found, assert against
that at the call site?

https://codereview.chromium.org/2618893003/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetsBridge.java:210:
assert mUmaHelper == null;
TBH, I kind of like keeping simple set methods that don't do anything fancy.
It's not really necessary to set both the observer and the helper at the same
time, so we could just have them be two separate methods.

https://codereview.chromium.org/2618893003/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/suggestions/SuggestionsUma.java
(right):

https://codereview.chromium.org/2618893003/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/suggestions/SuggestionsUma.java:36:
return getSectionStartingRank(suggestion.mCategory) + localRank;
Would it be simpler to just return an android.util.Pair with the two ranks?

[dgn, 2017-01-11 17:56:34]

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

[commit-bot: I haz the power, 2017-01-11 17:57:01]

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-01-11 18:38:25]

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

[commit-bot: I haz the power, 2017-01-11 18:38:26]

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

[dgn, 2017-01-11 18:50:11]

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

[commit-bot: I haz the power, 2017-01-11 18:50:40]

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-01-11 20:42:54]

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

[commit-bot: I haz the power, 2017-01-11 20:42:55]

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

[dgn, 2017-01-12 14:28:28]

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

[commit-bot: I haz the power, 2017-01-12 14:28:41]

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

[dgn, 2017-01-12 14:29:58]

PTAL

https://codereview.chromium.org/2618893003/diff/60001/chrome/android/java/src...
File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java
(right):

https://codereview.chromium.org/2618893003/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java:332: if
(sSuggestionsSourceForTests != null) return;
On 2017/01/11 13:53:38, Bernhard Bauer wrote:
> Could we instead move these tracking methods into an interface? It could be
> SuggestionsSource, or we could add a new interface that SnippetsBridge
> implements, to keep SuggestionsSource from blowing up too much.

Sure! I was thinking about doing that in another patch at first, but here it
comes anyway :)

https://codereview.chromium.org/2618893003/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetsBridge.java
(right):

https://codereview.chromium.org/2618893003/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetsBridge.java:27:
private SuggestionsUma mUmaHelper;
On 2017/01/11 13:53:38, Bernhard Bauer wrote:
> I think I would make this an interface and have SectionList directly implement
> it.

Done.

https://codereview.chromium.org/2618893003/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetsBridge.java:124:
int suggestionRank = mUmaHelper.getSuggestionRank(suggestion);
On 2017/01/11 13:53:38, Bernhard Bauer wrote:
> If getSuggestionRank returns -1 when the suggestion is not found, assert
against
> that at the call site?

Done.

https://codereview.chromium.org/2618893003/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetsBridge.java:210:
assert mUmaHelper == null;
On 2017/01/11 13:53:38, Bernhard Bauer wrote:
> TBH, I kind of like keeping simple set methods that don't do anything fancy.
> It's not really necessary to set both the observer and the helper at the same
> time, so we could just have them be two separate methods.

Done.

https://codereview.chromium.org/2618893003/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/suggestions/SuggestionsUma.java
(right):

https://codereview.chromium.org/2618893003/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/suggestions/SuggestionsUma.java:36:
return getSectionStartingRank(suggestion.mCategory) + localRank;
On 2017/01/11 13:53:38, Bernhard Bauer wrote:
> Would it be simpler to just return an android.util.Pair with the two ranks?

Done.

[dgn, 2017-01-12 14:31:45]

Description was changed from

==========
[NTP Client] Make the positions reported to UMA more accurate

There are 2 types of suggestion positions we report to UMA:
- SnippetArticle#mPosition: was the position in the pushed bundle for
a given category. Changed to the overall position in the category: is
set when the suggestion is added to the category, keeps increasing even
when suggestions are removed or additional ones are added.
- SnippetArticle#mGlobalPosition: was using the above position, offset
by the number of preceding suggestions. Changed to the actual current
position in the suggestion stream. As some are added or removed, the
global position of the suggestions coming after is updated.

BUG=675623
==========

to

==========
[NTP Client] Make the positions reported to UMA more accurate

There are 2 types of suggestion positions we report to UMA:
- SnippetArticle#mPosition: was the position in the pushed bundle for
a given category. Changed to the overall position in the category: is
set when the suggestion is added to the category, keeps increasing even
when suggestions are removed or additional ones are added.
- SnippetArticle#mGlobalPosition: was using the above position, offset
by the number of preceding suggestions. Changed to the actual current
position in the suggestion stream. As some are added or removed, the
global position of the suggestions coming after is updated.

BUG=675623
==========

[commit-bot: I haz the power, 2017-01-12 16:13:55]

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

[commit-bot: I haz the power, 2017-01-12 16:13:55]

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

[Bernhard Bauer, 2017-01-13 10:56:07]

LGTM

https://codereview.chromium.org/2618893003/diff/140001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/suggestions/ContentSuggestionsActivity.java
(right):

https://codereview.chromium.org/2618893003/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/suggestions/ContentSuggestionsActivity.java:153:
return mSnippetsBridge;
Do we have to handle the SnippetsBridge being destroyed?

https://codereview.chromium.org/2618893003/diff/140001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/suggestions/SuggestionsMetricsReporter.java
(right):

https://codereview.chromium.org/2618893003/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/suggestions/SuggestionsMetricsReporter.java:23:
/** @return The rank pair for the action item of the section for the provided
category. */
This method doesn't return a pair.

[dgn, 2017-01-13 11:24:01]

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

[dgn, 2017-01-13 11:24:05]

PTAL

https://codereview.chromium.org/2618893003/diff/140001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/suggestions/ContentSuggestionsActivity.java
(right):

https://codereview.chromium.org/2618893003/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/suggestions/ContentSuggestionsActivity.java:153:
return mSnippetsBridge;
On 2017/01/13 10:56:07, Bernhard Bauer wrote:
> Do we have to handle the SnippetsBridge being destroyed?

added snippetsbridge destruction to #onDestroy(). getSuggestionsMetricsReporter
is @Nullable, but it looks like the calls are safe, so I think it can stay as it
is. This might have to change when we move to injecting the interfaces where
they are needed rather than pulling from the manager, as we won't be able to see
the reference was nulled. I'll noop them on destroy or something  like that.
Trying to make the patches smaller :/

https://codereview.chromium.org/2618893003/diff/140001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/suggestions/SuggestionsMetricsReporter.java
(right):

https://codereview.chromium.org/2618893003/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/suggestions/SuggestionsMetricsReporter.java:23:
/** @return The rank pair for the action item of the section for the provided
category. */
On 2017/01/13 10:56:07, Bernhard Bauer wrote:
> This method doesn't return a pair.

Oops thanks, done.

[commit-bot: I haz the power, 2017-01-13 11:24:19]

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-01-13 12:14:32]

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

[commit-bot: I haz the power, 2017-01-13 12:14:33]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Marc Treib, 2017-01-13 13:11:32]

Drive-by comments:

- The original reasoning for using the position at-instatiation-time was
dismissing: If you get 10 suggestions, dismiss the first 5, then click of the
first of the remaining ones, we want to log that you opened suggestion #5, not
#0.

- IIUC, this changes the semantics of stuff we log to UMA. That's somewhat
dangerous: The affected histograms will mix data from before and after this
change. Maybe in this case, it only rarely makes a difference so that might be
acceptable? But generally, this kind of thing is best to avoid.

[dgn, 2017-01-13 16:21:56]

On 2017/01/13 13:11:32, Marc Treib wrote:
> Drive-by comments:
> 
> - The original reasoning for using the position at-instatiation-time was
> dismissing: If you get 10 suggestions, dismiss the first 5, then click of the
> first of the remaining ones, we want to log that you opened suggestion #5, not
> #0.

Yes, but then if you tap More somewhere, the incoming start from 0 instead of
whatever they are at at this point, and everything coming after is also wrong
now. So it's accurate information with maybe a slightly different concept, or
inaccurate data. But I think the main issue is that our definition for the data
we want to gather is not very precise and does not handle all the possible
cases. I filed an issue some time ago but since no one replied, apparently we
are happy with the data not meaning much -_-


> 
> - IIUC, this changes the semantics of stuff we log to UMA. That's somewhat
> dangerous: The affected histograms will mix data from before and after this
> change. Maybe in this case, it only rarely makes a difference so that might be
> acceptable? But generally, this kind of thing is best to avoid.

Hum true, I should add histograms if I do that...

[tschumann, 2017-01-13 17:35:57]

On 2017/01/13 16:21:56, dgn wrote:
> On 2017/01/13 13:11:32, Marc Treib wrote:
> > Drive-by comments:
> > 
> > - The original reasoning for using the position at-instatiation-time was
> > dismissing: If you get 10 suggestions, dismiss the first 5, then click of
the
> > first of the remaining ones, we want to log that you opened suggestion #5,
not
> > #0.
> 
> Yes, but then if you tap More somewhere, the incoming start from 0 instead of
> whatever they are at at this point, and everything coming after is also wrong
> now. So it's accurate information with maybe a slightly different concept, or
> inaccurate data. But I think the main issue is that our definition for the
data
> we want to gather is not very precise and does not handle all the possible
> cases. I filed an issue some time ago but since no one replied, apparently we
> are happy with the data not meaning much -_-

My 2 cents: So far card dismissal is rare enough that we don't have to worry
about compatibility with old data.
I'd actually be in favor to keep the old behavior and use the ranks as they were
given when the page was built.
If the user discards position 1-3 and then clicks on the original #5 we should
realize that the ranking was worse
than if we put the position #5 on rank #2 right from the start.
I didn't look at the code, but would guess that adjusting the reported rank is
adding quite some complexity. Why not keep it simple?
I actually didn't think a lot about the "fetch more" case, but actually, you
could even justify using the rankings we got from that batch (start numbering
them all over again):
the reason is that the user is already looking at this new set and hence the
first suggestion returned by fetch-more is logically seen a rank #1.
Seems like we could keep the rank static from the initial batch and wouldn't
have to worry about many things.

> 
> 
> > 
> > - IIUC, this changes the semantics of stuff we log to UMA. That's somewhat
> > dangerous: The affected histograms will mix data from before and after this
> > change. Maybe in this case, it only rarely makes a difference so that might
be
> > acceptable? But generally, this kind of thing is best to avoid.
> 
> Hum true, I should add histograms if I do that...

[dgn, 2017-01-13 17:44:56]

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

[commit-bot: I haz the power, 2017-01-13 17:45:19]

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

[dgn, 2017-01-13 17:48:29]

On 2017/01/13 17:35:57, tschumann wrote:
> On 2017/01/13 16:21:56, dgn wrote:
> > On 2017/01/13 13:11:32, Marc Treib wrote:
> > > Drive-by comments:
> > > 
> > > - The original reasoning for using the position at-instatiation-time was
> > > dismissing: If you get 10 suggestions, dismiss the first 5, then click of
> the
> > > first of the remaining ones, we want to log that you opened suggestion #5,
> not
> > > #0.
> > 
> > Yes, but then if you tap More somewhere, the incoming start from 0 instead
of
> > whatever they are at at this point, and everything coming after is also
wrong
> > now. So it's accurate information with maybe a slightly different concept,
or
> > inaccurate data. But I think the main issue is that our definition for the
> data
> > we want to gather is not very precise and does not handle all the possible
> > cases. I filed an issue some time ago but since no one replied, apparently
we
> > are happy with the data not meaning much -_-
> 
> My 2 cents: So far card dismissal is rare enough that we don't have to worry
> about compatibility with old data.
> I'd actually be in favor to keep the old behavior and use the ranks as they
were
> given when the page was built.
> If the user discards position 1-3 and then clicks on the original #5 we should
> realize that the ranking was worse
> than if we put the position #5 on rank #2 right from the start.
> I didn't look at the code, but would guess that adjusting the reported rank is
> adding quite some complexity. Why not keep it simple?
> I actually didn't think a lot about the "fetch more" case, but actually, you
> could even justify using the rankings we got from that batch (start numbering
> them all over again):
> the reason is that the user is already looking at this new set and hence the
> first suggestion returned by fetch-more is logically seen a rank #1.
> Seems like we could keep the rank static from the initial batch and wouldn't
> have to worry about many things.
> 

Wdyt about the new patch?

Behaviour:
- rank is attributed in a continually increasing way as suggestions are added to
sections
- section rank is incremented for each suggestion added to the section
- global rank is incremented for each suggestion added anywhere.

Example:
Let's say I have 2 sections, each with 10. Ranks will be:
<0-9, 0-9>
<0-9, 10-19>

I dismiss the first one.

<1-9, 1-9>
<0-9, 10-19>

I fetch more in the first section:

<1-19, 0-9,20-29>
<0-9, 10-19>

I fetch more in the second section:

<1-19, 0-9,20-29>
<0-19, 10-19, 30-39>

Does that look like a sensible behaviour?

[dgn, 2017-01-13 18:06:01]

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

[commit-bot: I haz the power, 2017-01-13 18:06:36]

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

[tschumann, 2017-01-13 18:16:10]

On 2017/01/13 17:48:29, dgn wrote:
> On 2017/01/13 17:35:57, tschumann wrote:
> > On 2017/01/13 16:21:56, dgn wrote:
> > > On 2017/01/13 13:11:32, Marc Treib wrote:
> > > > Drive-by comments:
> > > > 
> > > > - The original reasoning for using the position at-instatiation-time was
> > > > dismissing: If you get 10 suggestions, dismiss the first 5, then click
of
> > the
> > > > first of the remaining ones, we want to log that you opened suggestion
#5,
> > not
> > > > #0.
> > > 
> > > Yes, but then if you tap More somewhere, the incoming start from 0 instead
> of
> > > whatever they are at at this point, and everything coming after is also
> wrong
> > > now. So it's accurate information with maybe a slightly different concept,
> or
> > > inaccurate data. But I think the main issue is that our definition for the
> > data
> > > we want to gather is not very precise and does not handle all the possible
> > > cases. I filed an issue some time ago but since no one replied, apparently
> we
> > > are happy with the data not meaning much -_-
> > 
> > My 2 cents: So far card dismissal is rare enough that we don't have to worry
> > about compatibility with old data.
> > I'd actually be in favor to keep the old behavior and use the ranks as they
> were
> > given when the page was built.
> > If the user discards position 1-3 and then clicks on the original #5 we
should
> > realize that the ranking was worse
> > than if we put the position #5 on rank #2 right from the start.
> > I didn't look at the code, but would guess that adjusting the reported rank
is
> > adding quite some complexity. Why not keep it simple?
> > I actually didn't think a lot about the "fetch more" case, but actually, you
> > could even justify using the rankings we got from that batch (start
numbering
> > them all over again):
> > the reason is that the user is already looking at this new set and hence the
> > first suggestion returned by fetch-more is logically seen a rank #1.
> > Seems like we could keep the rank static from the initial batch and wouldn't
> > have to worry about many things.
> > 
> 
> Wdyt about the new patch?
> 
> Behaviour:
> - rank is attributed in a continually increasing way as suggestions are added
to
> sections
> - section rank is incremented for each suggestion added to the section
> - global rank is incremented for each suggestion added anywhere.
> 
> Example:
To double check, each line is a separate NTP, right?
If each DD-DD interval shows one section, then I wonder why in the first state,
the second row has an entry "10-19".

> Let's say I have 2 sections, each with 10. Ranks will be:
> <0-9, 0-9>
> <0-9, 10-19>
> 
> I dismiss the first one.
> 
> <1-9, 1-9>
> <0-9, 10-19>
> 
> I fetch more in the first section:
>
I'd handle this differently (putting one section in parenthesis).
also, why would the second section change?
> <1-19, 0-9,20-29>
<(1-9,0-9), (1-9)>
> <0-9, 10-19>
> 
> I fetch more in the second section:
> 
> <1-19, 0-9,20-29>
> <0-19, 10-19, 30-39>
> 
> Does that look like a sensible behaviour?

Again, my goal is to make it as simple as possible. If this would complicate
things let me know.Maybe i'm also missing something (happy to have others chime
in).

[commit-bot: I haz the power, 2017-01-13 19:27:26]

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

[commit-bot: I haz the power, 2017-01-13 19:27:27]

Dry run: This issue passed the CQ dry run.

[dgn, 2017-01-13 20:39:52]

Rewriting in a more detailed way displaying the rank intervals :)

Let's say I have 2 sections, each with 10. Ranks will be:
section 1: localRanks=[0,9], globalRanks=[0-9]
section 2: localRanks=[0,9], globalRanks=[10-19]

I dismiss the first one.

section 1: localRanks=[1,9], globalRanks=[1-9]
section 2: localRanks=[0,9], globalRanks=[10-19]

I fetch more in the first section:

section 1: localRanks=[1,19], globalRanks=[0-9] U [20-29]
section 2: localRanks=[0,9], globalRanks=[10-19]

I fetch more in the second section:

section 1: localRanks=[1,19], globalRanks=[0-9] U [20-29]
section 2: localRanks=[0,19], globalRanks=[10-19] U [30-39]

That's the current implementation, a test in SectionListTest.java verifies this.

[dgn, 2017-01-13 20:51:39]

Description was changed from

==========
[NTP Client] Make the positions reported to UMA more accurate

There are 2 types of suggestion positions we report to UMA:
- SnippetArticle#mPosition: was the position in the pushed bundle for
a given category. Changed to the overall position in the category: is
set when the suggestion is added to the category, keeps increasing even
when suggestions are removed or additional ones are added.
- SnippetArticle#mGlobalPosition: was using the above position, offset
by the number of preceding suggestions. Changed to the actual current
position in the suggestion stream. As some are added or removed, the
global position of the suggestions coming after is updated.

BUG=675623
==========

to

==========
[NTP Client] Make the positions reported to UMA more accurate


There are 2 types of suggestion positions we report to UMA:

- Within a given section: because we were attributing ranks in
the incoming bundle, it was not taking into account the suggestions
that had been pushed before. This number is now always increasing,
so after fetching more suggestions 5 times, the ranks for the 5th
bundle can be in the 40 to 49 range.

- Global rank: we also now attribute the rank as suggestions are
added to the section, and keep track of that number. It would be the
position of the suggestion if we kept adding suggestions always at the
end of the list rather than grouping them by category.

This way for a given NTP, there can be no overlap of global rank or local
rank within a section.

BUG=675623
==========

[tschumann, 2017-01-16 09:47:35]

On 2017/01/13 20:39:52, dgn wrote:
> Rewriting in a more detailed way displaying the rank intervals :)

That's nice. So this would mean all ranks are statically assigned. I'd be also
fine with duplicating global ranks -- whatever is easiest for you (see example
below).
> 
> Let's say I have 2 sections, each with 10. Ranks will be:
> section 1: localRanks=[0,9], globalRanks=[0-9]
> section 2: localRanks=[0,9], globalRanks=[10-19]
> 
> I dismiss the first one.
> 
> section 1: localRanks=[1,9], globalRanks=[1-9]
> section 2: localRanks=[0,9], globalRanks=[10-19]
> 
> I fetch more in the first section:
> 
> section 1: localRanks=[1,19], globalRanks=[0-9] U [20-29]
one alternative would be. I'm having a slight preference for this, but we don't
make heavy use of the global rank anyways. If that's how the metric is
implemented in M56, it's probably better to leave unchanged.
section 1: localRanks=[1,19], globalRanks=[0-19]
section 2: localRanks=[0,9], globalRanks=[10-19]

> section 2: localRanks=[0,9], globalRanks=[10-19]
> 
> I fetch more in the second section:
> 
> section 1: localRanks=[1,19], globalRanks=[0-9] U [20-29]
> section 2: localRanks=[0,19], globalRanks=[10-19] U [30-39]
> 
> That's the current implementation, a test in SectionListTest.java verifies
this.

[dgn, 2017-01-16 10:33:16]

On 2017/01/16 09:47:35, tschumann wrote:
> On 2017/01/13 20:39:52, dgn wrote:
> > Rewriting in a more detailed way displaying the rank intervals :)
> 
> That's nice. So this would mean all ranks are statically assigned. I'd be also
> fine with duplicating global ranks -- whatever is easiest for you (see example
> below).
> > 
> > Let's say I have 2 sections, each with 10. Ranks will be:
> > section 1: localRanks=[0,9], globalRanks=[0-9]
> > section 2: localRanks=[0,9], globalRanks=[10-19]
> > 
> > I dismiss the first one.
> > 
> > section 1: localRanks=[1,9], globalRanks=[1-9]
> > section 2: localRanks=[0,9], globalRanks=[10-19]
> > 
> > I fetch more in the first section:
> > 
> > section 1: localRanks=[1,19], globalRanks=[0-9] U [20-29]
> one alternative would be. I'm having a slight preference for this, but we
don't
> make heavy use of the global rank anyways. If that's how the metric is
> implemented in M56, it's probably better to leave unchanged.
> section 1: localRanks=[1,19], globalRanks=[0-19]
> section 2: localRanks=[0,9], globalRanks=[10-19]
> 
> > section 2: localRanks=[0,9], globalRanks=[10-19]
> > 
> > I fetch more in the second section:
> > 
> > section 1: localRanks=[1,19], globalRanks=[0-9] U [20-29]
> > section 2: localRanks=[0,19], globalRanks=[10-19] U [30-39]
> > 
> > That's the current implementation, a test in SectionListTest.java verifies
> this.

Duplicating global ranks sounds more like a bug than a feature to me. If we
don't duplicate, perhaps we can get some more info out of the metric, otherwise
it feels very much like the same thing as the per-section rank. For example,
know that a user pressed "fetch more" a lot on different sections in the same
NTP? Alternatively, if we are not using that metric, and don't have a good
expression of what we want to know about, maybe we should just remove it?

In terms of what is easier to implement... well the current implementation is
non overlapping. If we want to have the overlap we would need to also keep track
of at which global rank each section starts.

[Marc Treib, 2017-01-16 10:54:48]

Trying to summarize:
- Local ranks are fine.
- Nobody can quite figure out what the global ranks should be after "more".
- We don't really have a use case for global ranks.

I'm tempted to say let's just get rid of global ranks altogether. We will soon
have the category rank of opened suggestions, that should mostly serve our
purposes.
WDYT?

[tschumann, 2017-01-16 14:08:26]

On 2017/01/16 10:54:48, Marc Treib wrote:
> Trying to summarize:
> - Local ranks are fine.
> - Nobody can quite figure out what the global ranks should be after "more".
> - We don't really have a use case for global ranks.
> 
> I'm tempted to say let's just get rid of global ranks altogether. We will soon
> have the category rank of opened suggestions, that should mostly serve our
> purposes.
> WDYT?

We'd need to log a different signal to indicate scroll-below-the-fold events.
I can see some value in keeping them to see the number of shown suggestions. But
then we should probably rename them according to the purpose -- as it's hard to
reason about a global rank.
Something like DisplayedOrder or similar might be better and get the semantics
correctly across which are already implemented.

[Marc Treib, 2017-01-16 14:31:23]

On 2017/01/16 14:08:26, tschumann wrote:
> On 2017/01/16 10:54:48, Marc Treib wrote:
> > Trying to summarize:
> > - Local ranks are fine.
> > - Nobody can quite figure out what the global ranks should be after "more".
> > - We don't really have a use case for global ranks.
> > 
> > I'm tempted to say let's just get rid of global ranks altogether. We will
soon
> > have the category rank of opened suggestions, that should mostly serve our
> > purposes.
> > WDYT?
> 
> We'd need to log a different signal to indicate scroll-below-the-fold events.
> I can see some value in keeping them to see the number of shown suggestions.
But
> then we should probably rename them according to the purpose -- as it's hard
to
> reason about a global rank.
> Something like DisplayedOrder or similar might be better and get the semantics
> correctly across which are already implemented.

I don't think it's worth the hassle to rename the histogram, but we can update
its description.

[dgn, 2017-01-17 09:55:55]

The current description (included below) fits the implementation proposed in
this patch, so I'd like to land this. I will send a follow up (since this patch
is big enough already) to update the descriptions and clarify what the goal for
this measurement is, and tweak the calculation if needed. PTAL.

"Android: The position of the snippets card on the NTP, that is clicked through
to the host website of the content. We track the position the snippet had in the
list when NTP was loaded. This tracked position is thus different from the
position observed by the user whenever before scrolling down to the given
snippet, the user discards some snippets in the top of the list."

[dgn, 2017-01-17 11:29:27]

Description was changed from

==========
[NTP Client] Make the positions reported to UMA more accurate


There are 2 types of suggestion positions we report to UMA:

- Within a given section: because we were attributing ranks in
the incoming bundle, it was not taking into account the suggestions
that had been pushed before. This number is now always increasing,
so after fetching more suggestions 5 times, the ranks for the 5th
bundle can be in the 40 to 49 range.

- Global rank: we also now attribute the rank as suggestions are
added to the section, and keep track of that number. It would be the
position of the suggestion if we kept adding suggestions always at the
end of the list rather than grouping them by category.

This way for a given NTP, there can be no overlap of global rank or local
rank within a section.

BUG=675623
==========

to

==========
[NTP Client] Tweak the suggestion ranks for UMA to handle fetchMore


There are 2 types of suggestion positions we report to UMA:

- Within a given section: because we were attributing ranks in
the incoming bundle, it was not taking into account the suggestions
that had been pushed before. This number is now always increasing,
so after fetching more suggestions 5 times, the ranks for the 5th
bundle can be in the 40 to 49 range.

- Global rank: we also now attribute the rank as suggestions are
added to the section, and keep track of that number. It would be the
position of the suggestion if we kept adding suggestions always at the
end of the list rather than grouping them by category.

This way for a given NTP, there can be no overlap of global rank or local
rank within a section.

BUG=675623
==========

[dgn, 2017-01-17 16:55:53]

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

[commit-bot: I haz the power, 2017-01-17 16:56:06]

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-01-17 16:57:55]

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

[commit-bot: I haz the power, 2017-01-17 16:57:56]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[Bernhard Bauer, 2017-01-17 17:42:31]

Wow, this CL has changed quite a lot :)

https://codereview.chromium.org/2618893003/diff/240001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/ActionItem.java
(right):

https://codereview.chromium.org/2618893003/diff/240001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/ActionItem.java:72:
public void setRank(int perSectionRank) {
The corresponding getter is called getPerSectionRank(). Can we name this one
setPerSectionRank() then?

https://codereview.chromium.org/2618893003/diff/240001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SectionList.java
(right):

https://codereview.chromium.org/2618893003/diff/240001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SectionList.java:258:
public static class SuggestionRanker {
Can we move this to a separate class? There isn't much here that requires it to
be in SectionList.

https://codereview.chromium.org/2618893003/diff/240001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SectionList.java:266:
public void rankItem(ActionItem actionItem, SuggestionsSection section) {
Can we rename these methods to rankActionItem() and rankSuggestion()?

https://codereview.chromium.org/2618893003/diff/240001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java
(right):

https://codereview.chromium.org/2618893003/diff/240001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java:259:
mSuggestionRanker.rankItem((ActionItem) getChildForPosition(position), this);
Could we have the ActionItem itself call this?

https://codereview.chromium.org/2618893003/diff/240001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java:270:
public void childSeen(int position) {
If this is only called for suggestions now, rename it accordingly?

[dgn, 2017-01-17 18:27:42]

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

[commit-bot: I haz the power, 2017-01-17 18:28:02]

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

[dgn, 2017-01-17 18:46:24]

yes sorry, I moved a few things around to minimize the follow up CL. PTAL again

https://codereview.chromium.org/2618893003/diff/240001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/ActionItem.java
(right):

https://codereview.chromium.org/2618893003/diff/240001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/ActionItem.java:72:
public void setRank(int perSectionRank) {
On 2017/01/17 17:42:31, Bernhard Bauer wrote:
> The corresponding getter is called getPerSectionRank(). Can we name this one
> setPerSectionRank() then?

Done.

https://codereview.chromium.org/2618893003/diff/240001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SectionList.java
(right):

https://codereview.chromium.org/2618893003/diff/240001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SectionList.java:258:
public static class SuggestionRanker {
On 2017/01/17 17:42:31, Bernhard Bauer wrote:
> Can we move this to a separate class? There isn't much here that requires it
to
> be in SectionList.

Done.

https://codereview.chromium.org/2618893003/diff/240001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SectionList.java:266:
public void rankItem(ActionItem actionItem, SuggestionsSection section) {
On 2017/01/17 17:42:31, Bernhard Bauer wrote:
> Can we rename these methods to rankActionItem() and rankSuggestion()?

Done.

https://codereview.chromium.org/2618893003/diff/240001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java
(right):

https://codereview.chromium.org/2618893003/diff/240001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java:259:
mSuggestionRanker.rankItem((ActionItem) getChildForPosition(position), this);
On 2017/01/17 17:42:31, Bernhard Bauer wrote:
> Could we have the ActionItem itself call this?

Done.

https://codereview.chromium.org/2618893003/diff/240001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java:270:
public void childSeen(int position) {
On 2017/01/17 17:42:31, Bernhard Bauer wrote:
> If this is only called for suggestions now, rename it accordingly?

Reverted since actionItem calls its ranker itself.

[vitaliii, 2017-01-18 08:46:27]

Drive-by comments.

https://codereview.chromium.org/2618893003/diff/280001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SectionList.java
(right):

https://codereview.chromium.org/2618893003/diff/280001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SectionList.java:104:
mSuggestionsRanker.registerCategory(category);
I discussed ignoring not shown empty sections with tschumann@.
This is fine as long as you count in visible empty section (which you do as of
now).
I do not have strong preference either.

https://codereview.chromium.org/2618893003/diff/280001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetsBridge.java
(right):

https://codereview.chromium.org/2618893003/diff/280001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetsBridge.java:160:
int categoryIndex = mSuggestionsRanker.getCategoryRank(suggestion.mCategory);
Rank is expected to be 1-based.
E.g. https://en.wikipedia.org/wiki/Mean_reciprocal_rank .

https://codereview.chromium.org/2618893003/diff/280001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/suggestions/SuggestionsRanker.java
(right):

https://codereview.chromium.org/2618893003/diff/280001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/suggestions/SuggestionsRanker.java:19:
* elements. See implementation for more details.
Remove "See implementation for more details."?

[Bernhard Bauer, 2017-01-18 10:26:22]

lgtm

[dgn, 2017-01-18 10:47:42]

https://codereview.chromium.org/2618893003/diff/280001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetsBridge.java
(right):

https://codereview.chromium.org/2618893003/diff/280001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetsBridge.java:160:
int categoryIndex = mSuggestionsRanker.getCategoryRank(suggestion.mCategory);
On 2017/01/18 08:46:27, vitaliii (OOO until 23rd) wrote:
> Rank is expected to be 1-based.
> E.g. https://en.wikipedia.org/wiki/Mean_reciprocal_rank .

Maybe, but "index" and "position" are both misleading as they are more or less
arbitrary values attributed to items here. I'd prefer to have the name be
inaccurate in that aspect rather than leading to confusion while trying to
manipulate actual indices of the items in the UI. The documentation for
SuggestionsRanker explicitly mentions that it uses rank as 0-based. 

I'm up for changing the name as long as it reflects the fact that the value is
not the actual position of the element, is not continuous, etc.

[dgn, 2017-01-18 13:54:36]

Thanks for the reviews! Landing now :)

https://codereview.chromium.org/2618893003/diff/280001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/suggestions/SuggestionsRanker.java
(right):

https://codereview.chromium.org/2618893003/diff/280001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/suggestions/SuggestionsRanker.java:19:
* elements. See implementation for more details.
On 2017/01/18 08:46:27, vitaliii (OOO until 23rd) wrote:
> Remove "See implementation for more details."?

Done.

[dgn, 2017-01-18 13:54:44]

The CQ bit was checked by dgn@chromium.org

[dgn, 2017-01-18 13:54:45]

The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org
Link to the patchset: https://codereview.chromium.org/2618893003/#ps300001
(title: "rebase, address comments")

[commit-bot: I haz the power, 2017-01-18 13:55: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-01-18 14:56:00]

CQ is committing da patch.

Bot data: {"patchset_id": 300001, "attempt_start_ts": 1484747684939850,
"parent_rev": "34b12e40418272d36ae5c94c58e50d97bd240697", "commit_rev":
"5549f302eeb5cc43d3e13435c3826587721ddee8"}

[commit-bot: I haz the power, 2017-01-18 14:56:39]

Description was changed from

==========
[NTP Client] Tweak the suggestion ranks for UMA to handle fetchMore


There are 2 types of suggestion positions we report to UMA:

- Within a given section: because we were attributing ranks in
the incoming bundle, it was not taking into account the suggestions
that had been pushed before. This number is now always increasing,
so after fetching more suggestions 5 times, the ranks for the 5th
bundle can be in the 40 to 49 range.

- Global rank: we also now attribute the rank as suggestions are
added to the section, and keep track of that number. It would be the
position of the suggestion if we kept adding suggestions always at the
end of the list rather than grouping them by category.

This way for a given NTP, there can be no overlap of global rank or local
rank within a section.

BUG=675623
==========

to

==========
[NTP Client] Tweak the suggestion ranks for UMA to handle fetchMore


There are 2 types of suggestion positions we report to UMA:

- Within a given section: because we were attributing ranks in
the incoming bundle, it was not taking into account the suggestions
that had been pushed before. This number is now always increasing,
so after fetching more suggestions 5 times, the ranks for the 5th
bundle can be in the 40 to 49 range.

- Global rank: we also now attribute the rank as suggestions are
added to the section, and keep track of that number. It would be the
position of the suggestion if we kept adding suggestions always at the
end of the list rather than grouping them by category.

This way for a given NTP, there can be no overlap of global rank or local
rank within a section.

BUG=675623

Review-Url: https://codereview.chromium.org/2618893003
Cr-Commit-Position: refs/heads/master@{#444354}
Committed:
https://chromium.googlesource.com/chromium/src/+/5549f302eeb5cc43d3e13435c382...
==========

[commit-bot: I haz the power, 2017-01-18 14:56:40]

Committed patchset #16 (id:300001) as
https://chromium.googlesource.com/chromium/src/+/5549f302eeb5cc43d3e13435c382...

[bikaboy1970, 2017-01-24 22:24:36]

bikaboy1970@googlemail.com changed reviewers:
+ bikaboy1970@googlemail.com

[bikaboy1970, 2017-01-24 22:24:37]

lgtm