[NTP::SectionOrder] Propagate category rank from UI through the bridge.

[NTP::SectionOrder] Propagate category rank from UI through the bridge.

Provide category rank in |onSuggestionOpened| notification of the
bridge.

Reasons: we need it for metrics and the rank may have changed after the
NTP was opened, so UI is the only place to get "real" rank.

BUG=678623
Review-Url: https://codereview.chromium.org/2627603002
Cr-Commit-Position: refs/heads/master@{#444048}
Committed: https://chromium.googlesource.com/chromium/src/+/1e86736a8f02a0506d02148b112fcca44fb5ece4

[vitaliii, 2017-01-10 12:01:13]

Patchset #1 (id:1) has been deleted

[vitaliii, 2017-01-10 12:01:54]

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

[vitaliii, 2017-01-10 12:01:55]

Hi dgn@,

Could you have a look please?

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

How about this instead:

- Move the SnippetsBridge#onSuggestionOpened out of NtpManager#openSnippet, to
something like NtpManager#trackSnippetOpened

- Set the rank directly on categoryInfo rather than on the section itself.

- categoryInfo is available in SnippetViewHolder, get the rank from there.

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

https://codereview.chromium.org/2627603002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SectionList.java:65:
if (section != null) section.setCategoryRank(i);
nit: rename |i| to |categoryRank| or |categoryIndex| maybe?

[dgn, 2017-01-11 11:46:59]

context: I'm currently looking into cleaning up do-all methods like openSnippets
is currently, and having distinct responsibilities on the different objects.
Which is why I would prefer moving out UMA things from openSnippets and letting
the categoryInfo care about its rank.

[vitaliii, 2017-01-12 08:44:37]

On 2017/01/11 11:46:59, dgn wrote:
> context: I'm currently looking into cleaning up do-all methods like
openSnippets
> is currently, and having distinct responsibilities on the different objects.
> Which is why I would prefer moving out UMA things from openSnippets and
letting
> the categoryInfo care about its rank.

I did consider storing categoryRank in CategoryInfo, but it did not seem right.
The current CategoryInfo is propagated all the way from the provider:
 (1) provider -> (2) ContentSuggestionsService -> (3) bridge -> (4) UI

Setting categoryRank on any of these steps did not look appealing to me.

1) Provider does not know (and care) about rank;
2) Setting it in ContentSuggestionsService does not guarantee that it
corresponds to real UI position + then provider would have to leave categoryRank
empty;
3) Setting it in the bridge is just the same as where we started (providing it
to the metrics at the bridge) + same issues as for (2);
4) Setting it in UI would diverge CategoryInfo from its C++ counterpart. 

The option (4) seems the most reasonable.

Am I missing something? Or should I simply go for option 4?

[tschumann, 2017-01-12 09:47:29]

On 2017/01/12 08:44:37, vitaliii wrote:
> On 2017/01/11 11:46:59, dgn wrote:
> > context: I'm currently looking into cleaning up do-all methods like
> openSnippets
> > is currently, and having distinct responsibilities on the different objects.
> > Which is why I would prefer moving out UMA things from openSnippets and
> letting
> > the categoryInfo care about its rank.
> 
> I did consider storing categoryRank in CategoryInfo, but it did not seem
right.
> The current CategoryInfo is propagated all the way from the provider:
>  (1) provider -> (2) ContentSuggestionsService -> (3) bridge -> (4) UI
> 
> Setting categoryRank on any of these steps did not look appealing to me.
> 
> 1) Provider does not know (and care) about rank;
> 2) Setting it in ContentSuggestionsService does not guarantee that it
> corresponds to real UI position + then provider would have to leave
categoryRank
> empty;
> 3) Setting it in the bridge is just the same as where we started (providing it
> to the metrics at the bridge) + same issues as for (2);
> 4) Setting it in UI would diverge CategoryInfo from its C++ counterpart. 
> 
> The option (4) seems the most reasonable.
> 
> Am I missing something? Or should I simply go for option 4?

The CategoryInfo on the C++ side is considered to be quite static (quite because
it may change for server-side provided categories).
I'd be in favor to not change this concept between Java and C++.

Given the transient nature of the section rank (it might be different in every
NTP instance), it seems best to store it somewhere explicitly in the UI or just
bind it to corresponding callbacks. (but i also don't know the UI architecture
well enough to propose a proper place).

[dgn, 2017-01-12 10:46:57]

Yes of course, I didn't mean to change anything in the C++ side. The
categoryInfo in Java and C++ can have different things. For example in
SnippetArticle we have some static data but also some dynamic data that is there
only in the UI

I was thinking about setting that on SuggestionCategoryInfo right before passing
it to the section in SectionList#resetSection. It calls
SuggestionSource#getCategoryInfo which creates a new one at each call. The issue
might be that the fact that we create a new java object is an implementation
detail and the contract of the getCategoryInfo does not in theory say it will
not be shared with other NTPs, but for that the documentation can be changed.

But anyway, I'm also looking into replacing statically stored ranks that end up
being incorrect data as soon as users modify the list:
https://codereview.chromium.org/2618893003/ How exactly the rank is obtained
might change soon, so feel free to put it where convenient. I'd like the
tracking to be reported out of openSnippet though.

[tschumann, 2017-01-12 10:53:06]

On 2017/01/12 10:46:57, dgn wrote:
> Yes of course, I didn't mean to change anything in the C++ side. The
> categoryInfo in Java and C++ can have different things. For example in
> SnippetArticle we have some static data but also some dynamic data that is
there
> only in the UI
> 
> I was thinking about setting that on SuggestionCategoryInfo right before
passing
> it to the section in SectionList#resetSection. It calls
> SuggestionSource#getCategoryInfo which creates a new one at each call. The
issue
> might be that the fact that we create a new java object is an implementation
> detail and the contract of the getCategoryInfo does not in theory say it will
> not be shared with other NTPs, but for that the documentation can be changed.
> 
> But anyway, I'm also looking into replacing statically stored ranks that end
up
> being incorrect data as soon as users modify the list:
> https://codereview.chromium.org/2618893003/ How exactly the rank is obtained
> might change soon, so feel free to put it where convenient. I'd like the
> tracking to be reported out of openSnippet though.

I would prefer if things that have the same name on Java and C++ also have the
same semantics / responsibilities.
So objects of type CategoryInfo should be conceptually the same on both sides.
We might have inconsistencies but should try to remove those over time and not
create new ones.
IIUC, "section" is a UI term for a displayed category. If there's more state to
capture about a displayed section, then having a data structure like SectionInfo
(which might contain the category info but also dynamic parts) might be a better
place to put things.
given that I know little about the UI domain objects, this is just an example to
illustrate options, not necessarily the best ;-) [but i guess you get the idea]

[dgn, 2017-01-12 12:20:19]

Understood. As I said I don't feel too strongly about this specific point now as
I'm looking into removing the variable entirely anyway. Feel free to do as
convenient. 

Please split tracking out of openSnippet though.

[vitaliii, 2017-01-12 15:43:50]

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

[commit-bot: I haz the power, 2017-01-12 15:44:10]

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

[vitaliii, 2017-01-12 15:45:54]

Hi dgn@,

I addressed your comments.
However, the categoryRank is still stored in SuggestionsList.

Could you have a look?

Also I guess we are fine with static categoryRank.
At least I do not any benefit for our metric of taking into account dismissed
categories.

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

https://codereview.chromium.org/2627603002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SectionList.java:65:
if (section != null) section.setCategoryRank(i);
On 2017/01/11 11:44:18, dgn wrote:
> nit: rename |i| to |categoryRank| or |categoryIndex| maybe?

Done.

[commit-bot: I haz the power, 2017-01-12 15:46:30]

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

[commit-bot: I haz the power, 2017-01-12 15:46:31]

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-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[dgn, 2017-01-12 16:18:03]

lgtm

[vitaliii, 2017-01-16 06:59:46]

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

[commit-bot: I haz the power, 2017-01-16 07:00:00]

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

[vitaliii, 2017-01-16 07:00:57]

vitaliii@chromium.org changed reviewers:
+ jkrcal@chromium.org

[vitaliii, 2017-01-16 07:00:57]

Hi jkrcal@,

could you have a look at ntp_snippets_bridge.* please?

[commit-bot: I haz the power, 2017-01-16 07:25:05]

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

[commit-bot: I haz the power, 2017-01-16 07:25:05]

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

[vitaliii, 2017-01-16 07:43:34]

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

[commit-bot: I haz the power, 2017-01-16 07:43:45]

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-16 08:14:24]

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

[commit-bot: I haz the power, 2017-01-16 08:14:25]

Dry run: This issue passed the CQ dry run.

[jkrcal, 2017-01-16 09:47:59]

On 2017/01/16 07:00:57, vitaliii wrote:
> Hi jkrcal@,
> 
> could you have a look at ntp_snippets_bridge.* please?

lgtm

[vitaliii, 2017-01-16 09:56:08]

The CQ bit was checked by vitaliii@chromium.org

[vitaliii, 2017-01-16 09:56:08]

The patchset sent to the CQ was uploaded after l-g-t-m from dgn@chromium.org
Link to the patchset: https://codereview.chromium.org/2627603002/#ps120001
(title: "java tests.")

[commit-bot: I haz the power, 2017-01-16 09:56:19]

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-16 10:03:02]

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

[commit-bot: I haz the power, 2017-01-16 10:03:03]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[vitaliii, 2017-01-16 11:14:20]

vitaliii@chromium.org changed reviewers:
+ treib@chromium.org

[vitaliii, 2017-01-16 11:14:21]

Hi treib@,

could you have a look at ntp_snippets_bridge.*?

[Marc Treib, 2017-01-16 11:23:05]

https://codereview.chromium.org/2627603002/diff/120001/chrome/browser/android...
File chrome/browser/android/ntp/ntp_snippets_bridge.cc (right):

https://codereview.chromium.org/2627603002/diff/120001/chrome/browser/android...
chrome/browser/android/ntp/ntp_snippets_bridge.cc:355: DCHECK_GT(category_rank,
0);
Is this 1-based?!

[vitaliii, 2017-01-16 11:36:17]

Hi treib@,

PTAL.

https://codereview.chromium.org/2627603002/diff/120001/chrome/browser/android...
File chrome/browser/android/ntp/ntp_snippets_bridge.cc (right):

https://codereview.chromium.org/2627603002/diff/120001/chrome/browser/android...
chrome/browser/android/ntp/ntp_snippets_bridge.cc:355: DCHECK_GT(category_rank,
0);
On 2017/01/16 11:23:05, Marc Treib wrote:
> Is this 1-based?!

Yes, is this unusual?
It is not called "index".
I agree that "position" would be ambiguous.

Moreover, 
https://en.wikipedia.org/wiki/Mean_reciprocal_rank
implies that rank should 1-based.

[Marc Treib, 2017-01-16 12:28:52]

https://codereview.chromium.org/2627603002/diff/120001/chrome/browser/android...
File chrome/browser/android/ntp/ntp_snippets_bridge.cc (right):

https://codereview.chromium.org/2627603002/diff/120001/chrome/browser/android...
chrome/browser/android/ntp/ntp_snippets_bridge.cc:355: DCHECK_GT(category_rank,
0);
On 2017/01/16 11:36:17, vitaliii wrote:
> On 2017/01/16 11:23:05, Marc Treib wrote:
> > Is this 1-based?!
> 
> Yes, is this unusual?
> It is not called "index".
> I agree that "position" would be ambiguous.
> 
> Moreover, 
> https://en.wikipedia.org/wiki/Mean_reciprocal_rank
> implies that rank should 1-based.
> 

Well, most things we deal with are 0-based, including all of our other
histograms. So yes, I find this quite unusual.
If there's no particular reason for making it 1-based, then I'd much prefer 0.
If you really want to keep it as is, then lgtm, but please clearly mention this
in the histogram description and in comments on member variables.

[vitaliii, 2017-01-16 12:48:37]

tshchumann@,

It was your idea to use "rank", do you mind 0-based "index" instead to keep it
consistent with other histograms?

https://codereview.chromium.org/2627603002/diff/120001/chrome/browser/android...
File chrome/browser/android/ntp/ntp_snippets_bridge.cc (right):

https://codereview.chromium.org/2627603002/diff/120001/chrome/browser/android...
chrome/browser/android/ntp/ntp_snippets_bridge.cc:355: DCHECK_GT(category_rank,
0);
On 2017/01/16 12:28:52, Marc Treib wrote:
> On 2017/01/16 11:36:17, vitaliii wrote:
> > On 2017/01/16 11:23:05, Marc Treib wrote:
> > > Is this 1-based?!
> > 
> > Yes, is this unusual?
> > It is not called "index".
> > I agree that "position" would be ambiguous.
> > 
> > Moreover, 
> > https://en.wikipedia.org/wiki/Mean_reciprocal_rank
> > implies that rank should 1-based.
> > 
> 
> Well, most things we deal with are 0-based, including all of our other
> histograms. So yes, I find this quite unusual.
> If there's no particular reason for making it 1-based, then I'd much prefer 0.
> If you really want to keep it as is, then lgtm, but please clearly mention
this
> in the histogram description and in comments on member variables.

I like the consistency.
However, I do not want to have 0-based rank variable, so I would go for "index"
instead.

tschumann@, it was your idea to use "rank", do you mind 0-based "index" instead?

[tschumann, 2017-01-16 13:45:40]

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

[tschumann, 2017-01-16 13:45:41]

+mastiz for input on rank representation.

https://codereview.chromium.org/2627603002/diff/120001/chrome/browser/android...
File chrome/browser/android/ntp/ntp_snippets_bridge.cc (right):

https://codereview.chromium.org/2627603002/diff/120001/chrome/browser/android...
chrome/browser/android/ntp/ntp_snippets_bridge.cc:355: DCHECK_GT(category_rank,
0);
On 2017/01/16 12:48:37, vitaliii wrote:
> On 2017/01/16 12:28:52, Marc Treib wrote:
> > On 2017/01/16 11:36:17, vitaliii wrote:
> > > On 2017/01/16 11:23:05, Marc Treib wrote:
> > > > Is this 1-based?!
> > > 
> > > Yes, is this unusual?
> > > It is not called "index".
> > > I agree that "position" would be ambiguous.
> > > 
> > > Moreover, 
> > > https://en.wikipedia.org/wiki/Mean_reciprocal_rank
> > > implies that rank should 1-based.
> > > 
> > 
> > Well, most things we deal with are 0-based, including all of our other
> > histograms. So yes, I find this quite unusual.
> > If there's no particular reason for making it 1-based, then I'd much prefer
0.
> > If you really want to keep it as is, then lgtm, but please clearly mention
> this
> > in the histogram description and in comments on member variables.
> 
> I like the consistency.
> However, I do not want to have 0-based rank variable, so I would go for
"index"
> instead.
> 
> tschumann@, it was your idea to use "rank", do you mind 0-based "index"
instead?

+mastiz: What's the usual way ranks get handled in logs data etc. Are they
0-based or 1-based?

I'm against introducing a different name. I'm open to internally handle it zero
based, but I wonder if that wouldn't make things unnecessarily complicated as
the histograms are 1-based again anyways.

[jkrcal, 2017-01-16 14:08:11]

On 2017/01/16 13:45:41, tschumann wrote:
> +mastiz for input on rank representation.
> 
>
https://codereview.chromium.org/2627603002/diff/120001/chrome/browser/android...
> File chrome/browser/android/ntp/ntp_snippets_bridge.cc (right):
> 
>
https://codereview.chromium.org/2627603002/diff/120001/chrome/browser/android...
> chrome/browser/android/ntp/ntp_snippets_bridge.cc:355:
DCHECK_GT(category_rank,
> 0);
> On 2017/01/16 12:48:37, vitaliii wrote:
> > On 2017/01/16 12:28:52, Marc Treib wrote:
> > > On 2017/01/16 11:36:17, vitaliii wrote:
> > > > On 2017/01/16 11:23:05, Marc Treib wrote:
> > > > > Is this 1-based?!
> > > > 
> > > > Yes, is this unusual?
> > > > It is not called "index".
> > > > I agree that "position" would be ambiguous.
> > > > 
> > > > Moreover, 
> > > > https://en.wikipedia.org/wiki/Mean_reciprocal_rank
> > > > implies that rank should 1-based.
> > > > 
> > > 
> > > Well, most things we deal with are 0-based, including all of our other
> > > histograms. So yes, I find this quite unusual.
> > > If there's no particular reason for making it 1-based, then I'd much
prefer
> 0.
> > > If you really want to keep it as is, then lgtm, but please clearly mention
> > this
> > > in the histogram description and in comments on member variables.
> > 
> > I like the consistency.
> > However, I do not want to have 0-based rank variable, so I would go for
> "index"
> > instead.
> > 
> > tschumann@, it was your idea to use "rank", do you mind 0-based "index"
> instead?
> 
> +mastiz: What's the usual way ranks get handled in logs data etc. Are they
> 0-based or 1-based?
> 
> I'm against introducing a different name. I'm open to internally handle it
zero
> based, but I wonder if that wouldn't make things unnecessarily complicated as
> the histograms are 1-based again anyways.

Drive-by comment:

It is a common practice to report a 0-based histogram. 
(see
https://uma.googleplex.com/p/chrome/histograms/?endDate=20170114&dayCount=1&h...)

The underflow bucket is used for values "< 1" - i.e. for value "0" in this case.

[tschumann, 2017-01-16 14:16:23]

On 2017/01/16 14:08:11, jkrcal wrote:
> On 2017/01/16 13:45:41, tschumann wrote:
> > +mastiz for input on rank representation.
> > 
> >
>
https://codereview.chromium.org/2627603002/diff/120001/chrome/browser/android...
> > File chrome/browser/android/ntp/ntp_snippets_bridge.cc (right):
> > 
> >
>
https://codereview.chromium.org/2627603002/diff/120001/chrome/browser/android...
> > chrome/browser/android/ntp/ntp_snippets_bridge.cc:355:
> DCHECK_GT(category_rank,
> > 0);
> > On 2017/01/16 12:48:37, vitaliii wrote:
> > > On 2017/01/16 12:28:52, Marc Treib wrote:
> > > > On 2017/01/16 11:36:17, vitaliii wrote:
> > > > > On 2017/01/16 11:23:05, Marc Treib wrote:
> > > > > > Is this 1-based?!
> > > > > 
> > > > > Yes, is this unusual?
> > > > > It is not called "index".
> > > > > I agree that "position" would be ambiguous.
> > > > > 
> > > > > Moreover, 
> > > > > https://en.wikipedia.org/wiki/Mean_reciprocal_rank
> > > > > implies that rank should 1-based.
> > > > > 
> > > > 
> > > > Well, most things we deal with are 0-based, including all of our other
> > > > histograms. So yes, I find this quite unusual.
> > > > If there's no particular reason for making it 1-based, then I'd much
> prefer
> > 0.
> > > > If you really want to keep it as is, then lgtm, but please clearly
mention
> > > this
> > > > in the histogram description and in comments on member variables.
> > > 
> > > I like the consistency.
> > > However, I do not want to have 0-based rank variable, so I would go for
> > "index"
> > > instead.
> > > 
> > > tschumann@, it was your idea to use "rank", do you mind 0-based "index"
> > instead?
> > 
> > +mastiz: What's the usual way ranks get handled in logs data etc. Are they
> > 0-based or 1-based?
> > 
> > I'm against introducing a different name. I'm open to internally handle it
> zero
> > based, but I wonder if that wouldn't make things unnecessarily complicated
as
> > the histograms are 1-based again anyways.
> 
> Drive-by comment:
> 
> It is a common practice to report a 0-based histogram. 
> (see
>
https://uma.googleplex.com/p/chrome/histograms/?endDate=20170114&dayCount=1&h...)
> 
> The underflow bucket is used for values "< 1" - i.e. for value "0" in this
case.

Common -- certainly. Do we know if it's a good practice?
Thttps://codesearch.chromium.org/chromium/src/base/metrics/histogram.h?rcl=0&l=12

[jkrcal, 2017-01-16 14:28:35]

On 2017/01/16 14:16:23, tschumann wrote:
> On 2017/01/16 14:08:11, jkrcal wrote:
> > On 2017/01/16 13:45:41, tschumann wrote:
> > > +mastiz for input on rank representation.
> > > 
> > >
> >
>
https://codereview.chromium.org/2627603002/diff/120001/chrome/browser/android...
> > > File chrome/browser/android/ntp/ntp_snippets_bridge.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2627603002/diff/120001/chrome/browser/android...
> > > chrome/browser/android/ntp/ntp_snippets_bridge.cc:355:
> > DCHECK_GT(category_rank,
> > > 0);
> > > On 2017/01/16 12:48:37, vitaliii wrote:
> > > > On 2017/01/16 12:28:52, Marc Treib wrote:
> > > > > On 2017/01/16 11:36:17, vitaliii wrote:
> > > > > > On 2017/01/16 11:23:05, Marc Treib wrote:
> > > > > > > Is this 1-based?!
> > > > > > 
> > > > > > Yes, is this unusual?
> > > > > > It is not called "index".
> > > > > > I agree that "position" would be ambiguous.
> > > > > > 
> > > > > > Moreover, 
> > > > > > https://en.wikipedia.org/wiki/Mean_reciprocal_rank
> > > > > > implies that rank should 1-based.
> > > > > > 
> > > > > 
> > > > > Well, most things we deal with are 0-based, including all of our other
> > > > > histograms. So yes, I find this quite unusual.
> > > > > If there's no particular reason for making it 1-based, then I'd much
> > prefer
> > > 0.
> > > > > If you really want to keep it as is, then lgtm, but please clearly
> mention
> > > > this
> > > > > in the histogram description and in comments on member variables.
> > > > 
> > > > I like the consistency.
> > > > However, I do not want to have 0-based rank variable, so I would go for
> > > "index"
> > > > instead.
> > > > 
> > > > tschumann@, it was your idea to use "rank", do you mind 0-based "index"
> > > instead?
> > > 
> > > +mastiz: What's the usual way ranks get handled in logs data etc. Are they
> > > 0-based or 1-based?
> > > 
> > > I'm against introducing a different name. I'm open to internally handle it
> > zero
> > > based, but I wonder if that wouldn't make things unnecessarily complicated
> as
> > > the histograms are 1-based again anyways.
> > 
> > Drive-by comment:
> > 
> > It is a common practice to report a 0-based histogram. 
> > (see
> >
>
https://uma.googleplex.com/p/chrome/histograms/?endDate=20170114&dayCount=1&h...)
> > 
> > The underflow bucket is used for values "< 1" - i.e. for value "0" in this
> case.
> 
> Common -- certainly. Do we know if it's a good practice?
>
Thttps://codesearch.chromium.org/chromium/src/base/metrics/histogram.h?rcl=0&l=12

I think the way forward is to understand why they chose min_value>=1 and whether
they consider logging 0 a good / bad practice.

If they consider it a good practice, the documentation / variable names should
get clarified, I think.
If they consider it a bad practice, I wonder whether we should convince them to
change their mind&code.
I think "0" is a pretty useful value, in programming ;)

[Marc Treib, 2017-01-16 14:41:17]

On 2017/01/16 14:28:35, jkrcal wrote:
> On 2017/01/16 14:16:23, tschumann wrote:
> > On 2017/01/16 14:08:11, jkrcal wrote:
> > > On 2017/01/16 13:45:41, tschumann wrote:
> > > > +mastiz for input on rank representation.
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/2627603002/diff/120001/chrome/browser/android...
> > > > File chrome/browser/android/ntp/ntp_snippets_bridge.cc (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/2627603002/diff/120001/chrome/browser/android...
> > > > chrome/browser/android/ntp/ntp_snippets_bridge.cc:355:
> > > DCHECK_GT(category_rank,
> > > > 0);
> > > > On 2017/01/16 12:48:37, vitaliii wrote:
> > > > > On 2017/01/16 12:28:52, Marc Treib wrote:
> > > > > > On 2017/01/16 11:36:17, vitaliii wrote:
> > > > > > > On 2017/01/16 11:23:05, Marc Treib wrote:
> > > > > > > > Is this 1-based?!
> > > > > > > 
> > > > > > > Yes, is this unusual?
> > > > > > > It is not called "index".
> > > > > > > I agree that "position" would be ambiguous.
> > > > > > > 
> > > > > > > Moreover, 
> > > > > > > https://en.wikipedia.org/wiki/Mean_reciprocal_rank
> > > > > > > implies that rank should 1-based.
> > > > > > > 
> > > > > > 
> > > > > > Well, most things we deal with are 0-based, including all of our
other
> > > > > > histograms. So yes, I find this quite unusual.
> > > > > > If there's no particular reason for making it 1-based, then I'd much
> > > prefer
> > > > 0.
> > > > > > If you really want to keep it as is, then lgtm, but please clearly
> > mention
> > > > > this
> > > > > > in the histogram description and in comments on member variables.
> > > > > 
> > > > > I like the consistency.
> > > > > However, I do not want to have 0-based rank variable, so I would go
for
> > > > "index"
> > > > > instead.
> > > > > 
> > > > > tschumann@, it was your idea to use "rank", do you mind 0-based
"index"
> > > > instead?
> > > > 
> > > > +mastiz: What's the usual way ranks get handled in logs data etc. Are
they
> > > > 0-based or 1-based?
> > > > 
> > > > I'm against introducing a different name. I'm open to internally handle
it
> > > zero
> > > > based, but I wonder if that wouldn't make things unnecessarily
complicated
> > as
> > > > the histograms are 1-based again anyways.
> > > 
> > > Drive-by comment:
> > > 
> > > It is a common practice to report a 0-based histogram. 
> > > (see
> > >
> >
>
https://uma.googleplex.com/p/chrome/histograms/?endDate=20170114&dayCount=1&h...)
> > > 
> > > The underflow bucket is used for values "< 1" - i.e. for value "0" in this
> > case.
> > 
> > Common -- certainly. Do we know if it's a good practice?
> >
>
Thttps://codesearch.chromium.org/chromium/src/base/metrics/histogram.h?rcl=0&l=12
> 
> I think the way forward is to understand why they chose min_value>=1 and
whether
> they consider logging 0 a good / bad practice.
> 
> If they consider it a good practice, the documentation / variable names should
> get clarified, I think.
> If they consider it a bad practice, I wonder whether we should convince them
to
> change their mind&code.
> I think "0" is a pretty useful value, in programming ;)

The standard UMA_HISTOGRAM_ENUMERATION macro uses the 0 bucket normally. All our
position histograms make use of this.
Agreed that the UMA documentation/comments could be improved :)

[chromium-reviews, 2017-01-16 15:08:00]

As far as whether this is good or bad practice, it's explicitly said in the
documentation to do it that way:

"minimum should start from 1. 0 is as minimum is invalid. 0 is an
implicitdefault underflow bucket."
https://cs.chromium.org/chromium/src/base/metrics/histogram.h?sq=package:chro...


On Mon, Jan 16, 2017 at 2:41 PM <treib@chromium.org> wrote:

> On 2017/01/16 14:28:35, jkrcal wrote:
> > On 2017/01/16 14:16:23, tschumann wrote:
> > > On 2017/01/16 14:08:11, jkrcal wrote:
> > > > On 2017/01/16 13:45:41, tschumann wrote:
> > > > > +mastiz for input on rank representation.
> > > > >
> > > > >
> > > >
> > >
> >
>
>
https://codereview.chromium.org/2627603002/diff/120001/chrome/browser/android...
> > > > > File chrome/browser/android/ntp/ntp_snippets_bridge.cc (right):
> > > > >
> > > > >
> > > >
> > >
> >
>
>
https://codereview.chromium.org/2627603002/diff/120001/chrome/browser/android...
> > > > > chrome/browser/android/ntp/ntp_snippets_bridge.cc:355:
> > > > DCHECK_GT(category_rank,
> > > > > 0);
> > > > > On 2017/01/16 12:48:37, vitaliii wrote:
> > > > > > On 2017/01/16 12:28:52, Marc Treib wrote:
> > > > > > > On 2017/01/16 11:36:17, vitaliii wrote:
> > > > > > > > On 2017/01/16 11:23:05, Marc Treib wrote:
> > > > > > > > > Is this 1-based?!
> > > > > > > >
> > > > > > > > Yes, is this unusual?
> > > > > > > > It is not called "index".
> > > > > > > > I agree that "position" would be ambiguous.
> > > > > > > >
> > > > > > > > Moreover,
> > > > > > > > https://en.wikipedia.org/wiki/Mean_reciprocal_rank
> > > > > > > > implies that rank should 1-based.
> > > > > > > >
> > > > > > >
> > > > > > > Well, most things we deal with are 0-based, including all of
> our
> other
> > > > > > > histograms. So yes, I find this quite unusual.
> > > > > > > If there's no particular reason for making it 1-based, then
> I'd much
> > > > prefer
> > > > > 0.
> > > > > > > If you really want to keep it as is, then lgtm, but please
> clearly
> > > mention
> > > > > > this
> > > > > > > in the histogram description and in comments on member
> variables.
> > > > > >
> > > > > > I like the consistency.
> > > > > > However, I do not want to have 0-based rank variable, so I would
> go
> for
> > > > > "index"
> > > > > > instead.
> > > > > >
> > > > > > tschumann@, it was your idea to use "rank", do you mind 0-based
> "index"
> > > > > instead?
> > > > >
> > > > > +mastiz: What's the usual way ranks get handled in logs data etc.
> Are
> they
> > > > > 0-based or 1-based?
> > > > >
> > > > > I'm against introducing a different name. I'm open to internally
> handle
> it
> > > > zero
> > > > > based, but I wonder if that wouldn't make things unnecessarily
> complicated
> > > as
> > > > > the histograms are 1-based again anyways.
> > > >
> > > > Drive-by comment:
> > > >
> > > > It is a common practice to report a 0-based histogram.
> > > > (see
> > > >
> > >
> >
>
>
https://uma.googleplex.com/p/chrome/histograms/?endDate=20170114&dayCount=1&h...
> )
> > > >
> > > > The underflow bucket is used for values "< 1" - i.e. for value "0"
> in this
> > > case.
> > >
> > > Common -- certainly. Do we know if it's a good practice?
> > >
> >
> Thttps://
> codesearch.chromium.org/chromium/src/base/metrics/histogram.h?rcl=0&l=12
> >
> > I think the way forward is to understand why they chose min_value>=1 and
> whether
> > they consider logging 0 a good / bad practice.
> >
> > If they consider it a good practice, the documentation / variable names
> should
> > get clarified, I think.
> > If they consider it a bad practice, I wonder whether we should convince
> them
> to
> > change their mind&code.
> > I think "0" is a pretty useful value, in programming ;)
>
> The standard UMA_HISTOGRAM_ENUMERATION macro uses the 0 bucket normally.
> All our
> position histograms make use of this.
> Agreed that the UMA documentation/comments could be improved :)
>
> https://codereview.chromium.org/2627603002/
>
> --
> You received this message because you are subscribed to the Google Groups
> "New Tab Page development" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to ntp-dev+unsubscribe@chromium.org.
> To post to this group, send email to ntp-dev@chromium.org.
> To view this discussion on the web visit
>
https://groups.google.com/a/chromium.org/d/msgid/ntp-dev/001a113d3f0219f90605...
>
<https://groups.google.com/a/chromium.org/d/msgid/ntp-dev/001a113d3f0219f90605...>
> .
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[mastiz, 2017-01-16 15:22:52]

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

[mastiz, 2017-01-16 15:22:53]

https://codereview.chromium.org/2627603002/diff/120001/chrome/browser/android...
File chrome/browser/android/ntp/ntp_snippets_bridge.cc (right):

https://codereview.chromium.org/2627603002/diff/120001/chrome/browser/android...
chrome/browser/android/ntp/ntp_snippets_bridge.cc:355: DCHECK_GT(category_rank,
0);
On 2017/01/16 13:45:41, tschumann wrote:
> On 2017/01/16 12:48:37, vitaliii wrote:
> > On 2017/01/16 12:28:52, Marc Treib wrote:
> > > On 2017/01/16 11:36:17, vitaliii wrote:
> > > > On 2017/01/16 11:23:05, Marc Treib wrote:
> > > > > Is this 1-based?!
> > > > 
> > > > Yes, is this unusual?
> > > > It is not called "index".
> > > > I agree that "position" would be ambiguous.
> > > > 
> > > > Moreover, 
> > > > https://en.wikipedia.org/wiki/Mean_reciprocal_rank
> > > > implies that rank should 1-based.
> > > > 
> > > 
> > > Well, most things we deal with are 0-based, including all of our other
> > > histograms. So yes, I find this quite unusual.
> > > If there's no particular reason for making it 1-based, then I'd much
prefer
> 0.
> > > If you really want to keep it as is, then lgtm, but please clearly mention
> > this
> > > in the histogram description and in comments on member variables.
> > 
> > I like the consistency.
> > However, I do not want to have 0-based rank variable, so I would go for
> "index"
> > instead.
> > 
> > tschumann@, it was your idea to use "rank", do you mind 0-based "index"
> instead?
> 
> +mastiz: What's the usual way ranks get handled in logs data etc. Are they
> 0-based or 1-based?
> 
> I'm against introducing a different name. I'm open to internally handle it
zero
> based, but I wonder if that wouldn't make things unnecessarily complicated as
> the histograms are 1-based again anyways.

The term 'rank' suggests 1-based, otherwise logs usually use 'position' or
'index' which is zero-based (say, average click position).

[tschumann, 2017-01-16 15:29:23]

On 2017/01/16 15:22:53, mastiz wrote:
>
https://codereview.chromium.org/2627603002/diff/120001/chrome/browser/android...
> File chrome/browser/android/ntp/ntp_snippets_bridge.cc (right):
> 
>
https://codereview.chromium.org/2627603002/diff/120001/chrome/browser/android...
> chrome/browser/android/ntp/ntp_snippets_bridge.cc:355:
DCHECK_GT(category_rank,
> 0);
> On 2017/01/16 13:45:41, tschumann wrote:
> > On 2017/01/16 12:48:37, vitaliii wrote:
> > > On 2017/01/16 12:28:52, Marc Treib wrote:
> > > > On 2017/01/16 11:36:17, vitaliii wrote:
> > > > > On 2017/01/16 11:23:05, Marc Treib wrote:
> > > > > > Is this 1-based?!
> > > > > 
> > > > > Yes, is this unusual?
> > > > > It is not called "index".
> > > > > I agree that "position" would be ambiguous.
> > > > > 
> > > > > Moreover, 
> > > > > https://en.wikipedia.org/wiki/Mean_reciprocal_rank
> > > > > implies that rank should 1-based.
> > > > > 
> > > > 
> > > > Well, most things we deal with are 0-based, including all of our other
> > > > histograms. So yes, I find this quite unusual.
> > > > If there's no particular reason for making it 1-based, then I'd much
> prefer
> > 0.
> > > > If you really want to keep it as is, then lgtm, but please clearly
mention
> > > this
> > > > in the histogram description and in comments on member variables.
> > > 
> > > I like the consistency.
> > > However, I do not want to have 0-based rank variable, so I would go for
> > "index"
> > > instead.
> > > 
> > > tschumann@, it was your idea to use "rank", do you mind 0-based "index"
> > instead?
> > 
> > +mastiz: What's the usual way ranks get handled in logs data etc. Are they
> > 0-based or 1-based?
> > 
> > I'm against introducing a different name. I'm open to internally handle it
> zero
> > based, but I wonder if that wouldn't make things unnecessarily complicated
as
> > the histograms are 1-based again anyways.
> 
> The term 'rank' suggests 1-based, otherwise logs usually use 'position' or
> 'index' which is zero-based (say, average click position).

In this case, I'd vote for calling it rank and having it 1-based. We can add a
comment at central places (e.g. where members are defined) to make this obvious.

[Marc Treib, 2017-01-16 15:41:48]

On 2017/01/16 15:29:23, tschumann wrote:
> On 2017/01/16 15:22:53, mastiz wrote:
> >
>
https://codereview.chromium.org/2627603002/diff/120001/chrome/browser/android...
> > File chrome/browser/android/ntp/ntp_snippets_bridge.cc (right):
> > 
> >
>
https://codereview.chromium.org/2627603002/diff/120001/chrome/browser/android...
> > chrome/browser/android/ntp/ntp_snippets_bridge.cc:355:
> DCHECK_GT(category_rank,
> > 0);
> > On 2017/01/16 13:45:41, tschumann wrote:
> > > On 2017/01/16 12:48:37, vitaliii wrote:
> > > > On 2017/01/16 12:28:52, Marc Treib wrote:
> > > > > On 2017/01/16 11:36:17, vitaliii wrote:
> > > > > > On 2017/01/16 11:23:05, Marc Treib wrote:
> > > > > > > Is this 1-based?!
> > > > > > 
> > > > > > Yes, is this unusual?
> > > > > > It is not called "index".
> > > > > > I agree that "position" would be ambiguous.
> > > > > > 
> > > > > > Moreover, 
> > > > > > https://en.wikipedia.org/wiki/Mean_reciprocal_rank
> > > > > > implies that rank should 1-based.
> > > > > > 
> > > > > 
> > > > > Well, most things we deal with are 0-based, including all of our other
> > > > > histograms. So yes, I find this quite unusual.
> > > > > If there's no particular reason for making it 1-based, then I'd much
> > prefer
> > > 0.
> > > > > If you really want to keep it as is, then lgtm, but please clearly
> mention
> > > > this
> > > > > in the histogram description and in comments on member variables.
> > > > 
> > > > I like the consistency.
> > > > However, I do not want to have 0-based rank variable, so I would go for
> > > "index"
> > > > instead.
> > > > 
> > > > tschumann@, it was your idea to use "rank", do you mind 0-based "index"
> > > instead?
> > > 
> > > +mastiz: What's the usual way ranks get handled in logs data etc. Are they
> > > 0-based or 1-based?
> > > 
> > > I'm against introducing a different name. I'm open to internally handle it
> > zero
> > > based, but I wonder if that wouldn't make things unnecessarily complicated
> as
> > > the histograms are 1-based again anyways.
> > 
> > The term 'rank' suggests 1-based, otherwise logs usually use 'position' or
> > 'index' which is zero-based (say, average click position).
> 
> In this case, I'd vote for calling it rank and having it 1-based. We can add a
> comment at central places (e.g. where members are defined) to make this
obvious.

I still think it's very confusing to have the position of a *suggestion*
zero-based, but the position of a *category* one-based. I just don't see why one
would be different from the other. But I'll stop arguing now. :)

[jkrcal, 2017-01-16 15:44:57]

On 2017/01/16 15:41:48, Marc Treib wrote:
> On 2017/01/16 15:29:23, tschumann wrote:
> > On 2017/01/16 15:22:53, mastiz wrote:
> > >
> >
>
https://codereview.chromium.org/2627603002/diff/120001/chrome/browser/android...
> > > File chrome/browser/android/ntp/ntp_snippets_bridge.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2627603002/diff/120001/chrome/browser/android...
> > > chrome/browser/android/ntp/ntp_snippets_bridge.cc:355:
> > DCHECK_GT(category_rank,
> > > 0);
> > > On 2017/01/16 13:45:41, tschumann wrote:
> > > > On 2017/01/16 12:48:37, vitaliii wrote:
> > > > > On 2017/01/16 12:28:52, Marc Treib wrote:
> > > > > > On 2017/01/16 11:36:17, vitaliii wrote:
> > > > > > > On 2017/01/16 11:23:05, Marc Treib wrote:
> > > > > > > > Is this 1-based?!
> > > > > > > 
> > > > > > > Yes, is this unusual?
> > > > > > > It is not called "index".
> > > > > > > I agree that "position" would be ambiguous.
> > > > > > > 
> > > > > > > Moreover, 
> > > > > > > https://en.wikipedia.org/wiki/Mean_reciprocal_rank
> > > > > > > implies that rank should 1-based.
> > > > > > > 
> > > > > > 
> > > > > > Well, most things we deal with are 0-based, including all of our
other
> > > > > > histograms. So yes, I find this quite unusual.
> > > > > > If there's no particular reason for making it 1-based, then I'd much
> > > prefer
> > > > 0.
> > > > > > If you really want to keep it as is, then lgtm, but please clearly
> > mention
> > > > > this
> > > > > > in the histogram description and in comments on member variables.
> > > > > 
> > > > > I like the consistency.
> > > > > However, I do not want to have 0-based rank variable, so I would go
for
> > > > "index"
> > > > > instead.
> > > > > 
> > > > > tschumann@, it was your idea to use "rank", do you mind 0-based
"index"
> > > > instead?
> > > > 
> > > > +mastiz: What's the usual way ranks get handled in logs data etc. Are
they
> > > > 0-based or 1-based?
> > > > 
> > > > I'm against introducing a different name. I'm open to internally handle
it
> > > zero
> > > > based, but I wonder if that wouldn't make things unnecessarily
complicated
> > as
> > > > the histograms are 1-based again anyways.
> > > 
> > > The term 'rank' suggests 1-based, otherwise logs usually use 'position' or
> > > 'index' which is zero-based (say, average click position).
> > 
> > In this case, I'd vote for calling it rank and having it 1-based. We can add
a
> > comment at central places (e.g. where members are defined) to make this
> obvious.
> 
> I still think it's very confusing to have the position of a *suggestion*
> zero-based, but the position of a *category* one-based. I just don't see why
one
> would be different from the other. But I'll stop arguing now. :)

Unrelated to this particular conclusion (that I am fine with), I would like an
agreement whether we want to keep and use 0-base metrics in the future.
If so, should we push for clarification of the documentation?
If not, should we change all our existing metrics??

[tschumann, 2017-01-16 15:48:26]

On 2017/01/16 15:44:57, jkrcal wrote:
> On 2017/01/16 15:41:48, Marc Treib wrote:
> > On 2017/01/16 15:29:23, tschumann wrote:
> > > On 2017/01/16 15:22:53, mastiz wrote:
> > > >
> > >
> >
>
https://codereview.chromium.org/2627603002/diff/120001/chrome/browser/android...
> > > > File chrome/browser/android/ntp/ntp_snippets_bridge.cc (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/2627603002/diff/120001/chrome/browser/android...
> > > > chrome/browser/android/ntp/ntp_snippets_bridge.cc:355:
> > > DCHECK_GT(category_rank,
> > > > 0);
> > > > On 2017/01/16 13:45:41, tschumann wrote:
> > > > > On 2017/01/16 12:48:37, vitaliii wrote:
> > > > > > On 2017/01/16 12:28:52, Marc Treib wrote:
> > > > > > > On 2017/01/16 11:36:17, vitaliii wrote:
> > > > > > > > On 2017/01/16 11:23:05, Marc Treib wrote:
> > > > > > > > > Is this 1-based?!
> > > > > > > > 
> > > > > > > > Yes, is this unusual?
> > > > > > > > It is not called "index".
> > > > > > > > I agree that "position" would be ambiguous.
> > > > > > > > 
> > > > > > > > Moreover, 
> > > > > > > > https://en.wikipedia.org/wiki/Mean_reciprocal_rank
> > > > > > > > implies that rank should 1-based.
> > > > > > > > 
> > > > > > > 
> > > > > > > Well, most things we deal with are 0-based, including all of our
> other
> > > > > > > histograms. So yes, I find this quite unusual.
> > > > > > > If there's no particular reason for making it 1-based, then I'd
much
> > > > prefer
> > > > > 0.
> > > > > > > If you really want to keep it as is, then lgtm, but please clearly
> > > mention
> > > > > > this
> > > > > > > in the histogram description and in comments on member variables.
> > > > > > 
> > > > > > I like the consistency.
> > > > > > However, I do not want to have 0-based rank variable, so I would go
> for
> > > > > "index"
> > > > > > instead.
> > > > > > 
> > > > > > tschumann@, it was your idea to use "rank", do you mind 0-based
> "index"
> > > > > instead?
> > > > > 
> > > > > +mastiz: What's the usual way ranks get handled in logs data etc. Are
> they
> > > > > 0-based or 1-based?
> > > > > 
> > > > > I'm against introducing a different name. I'm open to internally
handle
> it
> > > > zero
> > > > > based, but I wonder if that wouldn't make things unnecessarily
> complicated
> > > as
> > > > > the histograms are 1-based again anyways.
> > > > 
> > > > The term 'rank' suggests 1-based, otherwise logs usually use 'position'
or
> > > > 'index' which is zero-based (say, average click position).
> > > 
> > > In this case, I'd vote for calling it rank and having it 1-based. We can
add
> a
> > > comment at central places (e.g. where members are defined) to make this
> > obvious.
> > 
> > I still think it's very confusing to have the position of a *suggestion*
> > zero-based, but the position of a *category* one-based. I just don't see why
> one
> > would be different from the other. But I'll stop arguing now. :)

hahaha! That's a very good observatio and we should address that. We might want
to fix the other metric as well.
Might be not the worse idea, now that we're agreeing on simpler semantics.

> 
> Unrelated to this particular conclusion (that I am fine with), I would like an
> agreement whether we want to keep and use 0-base metrics in the future.
> If so, should we push for clarification of the documentation?
> If not, should we change all our existing metrics??

We should reach out to the metrics folks and ask for their input.
I guess it's nice to have separate buckets for invalid data (maybe that's the
purpose of the zero-bucket). But they would know for sure.

[vitaliii, 2017-01-17 08:57:08]

On 2017/01/16 15:48:26, tschumann wrote:
> On 2017/01/16 15:44:57, jkrcal wrote:
> > On 2017/01/16 15:41:48, Marc Treib wrote:
> > > On 2017/01/16 15:29:23, tschumann wrote:
> > > > On 2017/01/16 15:22:53, mastiz wrote:
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/2627603002/diff/120001/chrome/browser/android...
> > > > > File chrome/browser/android/ntp/ntp_snippets_bridge.cc (right):
> > > > > 
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/2627603002/diff/120001/chrome/browser/android...
> > > > > chrome/browser/android/ntp/ntp_snippets_bridge.cc:355:
> > > > DCHECK_GT(category_rank,
> > > > > 0);
> > > > > On 2017/01/16 13:45:41, tschumann wrote:
> > > > > > On 2017/01/16 12:48:37, vitaliii wrote:
> > > > > > > On 2017/01/16 12:28:52, Marc Treib wrote:
> > > > > > > > On 2017/01/16 11:36:17, vitaliii wrote:
> > > > > > > > > On 2017/01/16 11:23:05, Marc Treib wrote:
> > > > > > > > > > Is this 1-based?!
> > > > > > > > > 
> > > > > > > > > Yes, is this unusual?
> > > > > > > > > It is not called "index".
> > > > > > > > > I agree that "position" would be ambiguous.
> > > > > > > > > 
> > > > > > > > > Moreover, 
> > > > > > > > > https://en.wikipedia.org/wiki/Mean_reciprocal_rank
> > > > > > > > > implies that rank should 1-based.
> > > > > > > > > 
> > > > > > > > 
> > > > > > > > Well, most things we deal with are 0-based, including all of our
> > other
> > > > > > > > histograms. So yes, I find this quite unusual.
> > > > > > > > If there's no particular reason for making it 1-based, then I'd
> much
> > > > > prefer
> > > > > > 0.
> > > > > > > > If you really want to keep it as is, then lgtm, but please
clearly
> > > > mention
> > > > > > > this
> > > > > > > > in the histogram description and in comments on member
variables.
> > > > > > > 
> > > > > > > I like the consistency.
> > > > > > > However, I do not want to have 0-based rank variable, so I would
go
> > for
> > > > > > "index"
> > > > > > > instead.
> > > > > > > 
> > > > > > > tschumann@, it was your idea to use "rank", do you mind 0-based
> > "index"
> > > > > > instead?
> > > > > > 
> > > > > > +mastiz: What's the usual way ranks get handled in logs data etc.
Are
> > they
> > > > > > 0-based or 1-based?
> > > > > > 
> > > > > > I'm against introducing a different name. I'm open to internally
> handle
> > it
> > > > > zero
> > > > > > based, but I wonder if that wouldn't make things unnecessarily
> > complicated
> > > > as
> > > > > > the histograms are 1-based again anyways.
> > > > > 
> > > > > The term 'rank' suggests 1-based, otherwise logs usually use
'position'
> or
> > > > > 'index' which is zero-based (say, average click position).
> > > > 
> > > > In this case, I'd vote for calling it rank and having it 1-based. We can
> add
> > a
> > > > comment at central places (e.g. where members are defined) to make this
> > > obvious.
> > > 
> > > I still think it's very confusing to have the position of a *suggestion*
> > > zero-based, but the position of a *category* one-based. I just don't see
why
> > one
> > > would be different from the other. But I'll stop arguing now. :)
> 
> hahaha! That's a very good observatio and we should address that. We might
want
> to fix the other metric as well.
> Might be not the worse idea, now that we're agreeing on simpler semantics.
> 
> > 
> > Unrelated to this particular conclusion (that I am fine with), I would like
an
> > agreement whether we want to keep and use 0-base metrics in the future.
> > If so, should we push for clarification of the documentation?
> > If not, should we change all our existing metrics??
> 
> We should reach out to the metrics folks and ask for their input.
> I guess it's nice to have separate buckets for invalid data (maybe that's the
> purpose of the zero-bucket). But they would know for sure.

According to asvitkine@, it is perfectly fine to use 0-based metrics.

I tend to rename the variable to categoryIndex and keep it 0-based.

Reasons:
1) 0-based, because all other position-related metrics are 0-based and it is
perfectly fine to have them 0-based.
2) categoryIndex, because "rank" is 1-based (as mastiz@ confirmed above).

WDYT?

[Marc Treib, 2017-01-17 09:00:50]

On 2017/01/17 08:57:08, vitaliii wrote:
> On 2017/01/16 15:48:26, tschumann wrote:
> > On 2017/01/16 15:44:57, jkrcal wrote:
> > > On 2017/01/16 15:41:48, Marc Treib wrote:
> > > > On 2017/01/16 15:29:23, tschumann wrote:
> > > > > On 2017/01/16 15:22:53, mastiz wrote:
> > > > > >
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/2627603002/diff/120001/chrome/browser/android...
> > > > > > File chrome/browser/android/ntp/ntp_snippets_bridge.cc (right):
> > > > > > 
> > > > > >
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/2627603002/diff/120001/chrome/browser/android...
> > > > > > chrome/browser/android/ntp/ntp_snippets_bridge.cc:355:
> > > > > DCHECK_GT(category_rank,
> > > > > > 0);
> > > > > > On 2017/01/16 13:45:41, tschumann wrote:
> > > > > > > On 2017/01/16 12:48:37, vitaliii wrote:
> > > > > > > > On 2017/01/16 12:28:52, Marc Treib wrote:
> > > > > > > > > On 2017/01/16 11:36:17, vitaliii wrote:
> > > > > > > > > > On 2017/01/16 11:23:05, Marc Treib wrote:
> > > > > > > > > > > Is this 1-based?!
> > > > > > > > > > 
> > > > > > > > > > Yes, is this unusual?
> > > > > > > > > > It is not called "index".
> > > > > > > > > > I agree that "position" would be ambiguous.
> > > > > > > > > > 
> > > > > > > > > > Moreover, 
> > > > > > > > > > https://en.wikipedia.org/wiki/Mean_reciprocal_rank
> > > > > > > > > > implies that rank should 1-based.
> > > > > > > > > > 
> > > > > > > > > 
> > > > > > > > > Well, most things we deal with are 0-based, including all of
our
> > > other
> > > > > > > > > histograms. So yes, I find this quite unusual.
> > > > > > > > > If there's no particular reason for making it 1-based, then
I'd
> > much
> > > > > > prefer
> > > > > > > 0.
> > > > > > > > > If you really want to keep it as is, then lgtm, but please
> clearly
> > > > > mention
> > > > > > > > this
> > > > > > > > > in the histogram description and in comments on member
> variables.
> > > > > > > > 
> > > > > > > > I like the consistency.
> > > > > > > > However, I do not want to have 0-based rank variable, so I would
> go
> > > for
> > > > > > > "index"
> > > > > > > > instead.
> > > > > > > > 
> > > > > > > > tschumann@, it was your idea to use "rank", do you mind 0-based
> > > "index"
> > > > > > > instead?
> > > > > > > 
> > > > > > > +mastiz: What's the usual way ranks get handled in logs data etc.
> Are
> > > they
> > > > > > > 0-based or 1-based?
> > > > > > > 
> > > > > > > I'm against introducing a different name. I'm open to internally
> > handle
> > > it
> > > > > > zero
> > > > > > > based, but I wonder if that wouldn't make things unnecessarily
> > > complicated
> > > > > as
> > > > > > > the histograms are 1-based again anyways.
> > > > > > 
> > > > > > The term 'rank' suggests 1-based, otherwise logs usually use
> 'position'
> > or
> > > > > > 'index' which is zero-based (say, average click position).
> > > > > 
> > > > > In this case, I'd vote for calling it rank and having it 1-based. We
can
> > add
> > > a
> > > > > comment at central places (e.g. where members are defined) to make
this
> > > > obvious.
> > > > 
> > > > I still think it's very confusing to have the position of a *suggestion*
> > > > zero-based, but the position of a *category* one-based. I just don't see
> why
> > > one
> > > > would be different from the other. But I'll stop arguing now. :)
> > 
> > hahaha! That's a very good observatio and we should address that. We might
> want
> > to fix the other metric as well.
> > Might be not the worse idea, now that we're agreeing on simpler semantics.
> > 
> > > 
> > > Unrelated to this particular conclusion (that I am fine with), I would
like
> an
> > > agreement whether we want to keep and use 0-base metrics in the future.
> > > If so, should we push for clarification of the documentation?
> > > If not, should we change all our existing metrics??
> > 
> > We should reach out to the metrics folks and ask for their input.
> > I guess it's nice to have separate buckets for invalid data (maybe that's
the
> > purpose of the zero-bucket). But they would know for sure.
> 
> According to asvitkine@, it is perfectly fine to use 0-based metrics.
> 
> I tend to rename the variable to categoryIndex and keep it 0-based.
> 
> Reasons:
> 1) 0-based, because all other position-related metrics are 0-based and it is
> perfectly fine to have them 0-based.
> 2) categoryIndex, because "rank" is 1-based (as mastiz@ confirmed above).
> 
> WDYT?

That SGTM, thanks!

[vitaliii, 2017-01-17 12:44:24]

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

[commit-bot: I haz the power, 2017-01-17 12:44:38]

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

[vitaliii, 2017-01-17 12:49:15]

I changed 1-based categoryRank to 0-based categoryIndex. 
Feel free to have a look.
Otherwise, I will submit in an hour or so.

[commit-bot: I haz the power, 2017-01-17 13:18:47]

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

[commit-bot: I haz the power, 2017-01-17 13:18:48]

Dry run: This issue passed the CQ dry run.

[Marc Treib, 2017-01-17 14:33:09]

On 2017/01/17 12:49:15, vitaliii wrote:
> I changed 1-based categoryRank to 0-based categoryIndex. 
> Feel free to have a look.
> Otherwise, I will submit in an hour or so.

LGTM

[vitaliii, 2017-01-17 14:39:57]

The CQ bit was checked by vitaliii@chromium.org

[vitaliii, 2017-01-17 14:39:58]

The patchset sent to the CQ was uploaded after l-g-t-m from jkrcal@chromium.org,
dgn@chromium.org
Link to the patchset: https://codereview.chromium.org/2627603002/#ps160001
(title: "1-based categoryRank -> 0-based categoryIndex.")

[commit-bot: I haz the power, 2017-01-17 14:40:12]

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 14:44:12]

CQ is committing da patch.

Bot data: {"patchset_id": 160001, "attempt_start_ts": 1484663997720880,
"parent_rev": "dc432f4211bfe92a50e6766d62ed8f833ab0c42d", "commit_rev":
"1e86736a8f02a0506d02148b112fcca44fb5ece4"}

[commit-bot: I haz the power, 2017-01-17 14:44:45]

Description was changed from

==========
[NTP::SectionOrder] Propagate category rank from UI through the bridge.

Provide category rank in |onSuggestionOpened| notification of the
bridge.

Reasons: we need it for metrics and the rank may have changed after the
NTP was opened, so UI is the only place to get "real" rank.

BUG=678623
==========

to

==========
[NTP::SectionOrder] Propagate category rank from UI through the bridge.

Provide category rank in |onSuggestionOpened| notification of the
bridge.

Reasons: we need it for metrics and the rank may have changed after the
NTP was opened, so UI is the only place to get "real" rank.

BUG=678623

Review-Url: https://codereview.chromium.org/2627603002
Cr-Commit-Position: refs/heads/master@{#444048}
Committed:
https://chromium.googlesource.com/chromium/src/+/1e86736a8f02a0506d02148b112f...
==========

[commit-bot: I haz the power, 2017-01-17 14:44:47]

Committed patchset #8 (id:160001) as
https://chromium.googlesource.com/chromium/src/+/1e86736a8f02a0506d02148b112f...

[dgn, 2017-01-17 17:53:18]

Sorry, realised a couple of bugs slipped in the CL a bit late. PTAL

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

https://codereview.chromium.org/2627603002/diff/160001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SectionList.java:65:
if (section != null) section.setCategoryIndex(categoryIndex);
here, we can decide not to show a section because it's empty. In this case, we
increment categoryIndex and for UMA purpose we are going to have a hole in the
reported values. 

shouldn't we use mSections.size() here?

https://codereview.chromium.org/2627603002/diff/160001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SectionList.java:98:
section = new SuggestionsSection(this, mNewTabPageManager, mOfflinePageBridge,
info);
here section has no categoryIndex set

[dgn, 2017-01-17 18:28:25]

On 2017/01/17 17:53:18, dgn wrote:
> Sorry, realised a couple of bugs slipped in the CL a bit late. PTAL
> 
>
https://codereview.chromium.org/2627603002/diff/160001/chrome/android/java/sr...
> File
> chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SectionList.java
> (right):
> 
>
https://codereview.chromium.org/2627603002/diff/160001/chrome/android/java/sr...
>
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SectionList.java:65:
> if (section != null) section.setCategoryIndex(categoryIndex);
> here, we can decide not to show a section because it's empty. In this case, we
> increment categoryIndex and for UMA purpose we are going to have a hole in the
> reported values. 
> 
> shouldn't we use mSections.size() here?
> 
>
https://codereview.chromium.org/2627603002/diff/160001/chrome/android/java/sr...
>
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SectionList.java:98:
> section = new SuggestionsSection(this, mNewTabPageManager, mOfflinePageBridge,
> info);
> here section has no categoryIndex set

Addressed in https://codereview.chromium.org/2618893003/#ps260001