[NTP Snippets] Metrics: switch over known categories

[NTP Snippets] Metrics: switch over known categories

This ensures that there'll be a compile error if we add a new category but don't update the metrics code.
Also, add a histogram suffix for PHYSICAL_WEB_PAGES which was missing.

BUG=none

Committed: https://crrev.com/c15e8abfcc306443d64e4c9c60182dc0737bb03f
Cr-Commit-Position: refs/heads/master@{#419420}

[Marc Treib, 2016-09-13 14:05:06]

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

[commit-bot: I haz the power, 2016-09-13 14:05:20]

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

[Marc Treib, 2016-09-13 15:02:50]

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

[Marc Treib, 2016-09-13 15:02:50]

PTAL!

[jkrcal, 2016-09-13 16:08:06]

https://codereview.chromium.org/2337103003/diff/20001/components/ntp_snippets...
File components/ntp_snippets/category.cc (right):

https://codereview.chromium.org/2337103003/diff/20001/components/ntp_snippets...
components/ntp_snippets/category.cc:21:
static_cast<int>(KnownCategories::ARTICLES)};
I am just wondering:

Cannot you add to the enum class elements SERVER_CATEGORIES_MAX and
UNKNOWN_SERVER_CATEGORIES_OFFSET in the end, consistently number unknown server
categories starting after the second offset and check here only for the
following?

0 <= id_ < LOCAL_CATEGORIES_COUNT || SERVER_CATEGORIES_OFFSET <= id_ <
SERVER_CATEGORIES_MAX

https://codereview.chromium.org/2337103003/diff/20001/components/ntp_snippets...
File components/ntp_snippets/category.h (right):

https://codereview.chromium.org/2337103003/diff/20001/components/ntp_snippets...
components/ntp_snippets/category.h:62: // Returns whether this category is any
of the known categories.
Without looking in the implementation, I would not understand what you mean by
"is any known category". Maybe expand by ", i.e. any element of the
KnowCategories enum except for LOCAL_CATEGORIES_COUNT and
REMOTE_CATEGORIES_OFFSET." 

It is also inconsistent with IsKnownCategory which accepts these two cases as
known categories. Admittedly, only an idiot would ask if this Category is the
LOCAL_CATEGORIES_COUNT-category. Still, I would prefer IsKnownCategory to
blacklist these two cases to get the code consistent and self-documented.

https://codereview.chromium.org/2337103003/diff/20001/components/ntp_snippets...
File components/ntp_snippets/content_suggestions_metrics.cc (right):

https://codereview.chromium.org/2337103003/diff/20001/components/ntp_snippets...
components/ntp_snippets/content_suggestions_metrics.cc:50: switch
(known_category) {
Way better, thanks!

[Marc Treib, 2016-09-13 16:28:51]

https://codereview.chromium.org/2337103003/diff/20001/components/ntp_snippets...
File components/ntp_snippets/category.cc (right):

https://codereview.chromium.org/2337103003/diff/20001/components/ntp_snippets...
components/ntp_snippets/category.cc:21:
static_cast<int>(KnownCategories::ARTICLES)};
On 2016/09/13 16:08:05, jkrcal wrote:
> I am just wondering:
> 
> Cannot you add to the enum class elements SERVER_CATEGORIES_MAX and
> UNKNOWN_SERVER_CATEGORIES_OFFSET in the end, consistently number unknown
server
> categories starting after the second offset and check here only for the
> following?
> 
> 0 <= id_ < LOCAL_CATEGORIES_COUNT || SERVER_CATEGORIES_OFFSET <= id_ <
> SERVER_CATEGORIES_MAX

SERVER_CATEGORIES_MAX would actually have to be KNOWN_SERVER_CATEGORIES_MAX, and
that would imply that all the known server categories are contiguous. We won't
necessarily be able to guarantee that.
We could do the range check for local categories, and then list only the known
server categories explicitly. WDYT?

https://codereview.chromium.org/2337103003/diff/20001/components/ntp_snippets...
File components/ntp_snippets/category.h (right):

https://codereview.chromium.org/2337103003/diff/20001/components/ntp_snippets...
components/ntp_snippets/category.h:62: // Returns whether this category is any
of the known categories.
On 2016/09/13 16:08:05, jkrcal wrote:
> Without looking in the implementation, I would not understand what you mean by
> "is any known category". Maybe expand by ", i.e. any element of the
> KnowCategories enum except for LOCAL_CATEGORIES_COUNT and
> REMOTE_CATEGORIES_OFFSET." 

Added, except for the "except..." part. It should be impossible to create such a
Category instance (see CategoryFactory::FromKnownCategory), so IMO that part of
the comment isn't necessary.

> It is also inconsistent with IsKnownCategory which accepts these two cases as
> known categories. Admittedly, only an idiot would ask if this Category is the
> LOCAL_CATEGORIES_COUNT-category. Still, I would prefer IsKnownCategory to
> blacklist these two cases to get the code consistent and self-documented.

Again, it shouldn't be possible to create those categories. Still, I've added
DCHECKs in IsKnownCategory to make clear that those aren't valid values.

[jkrcal, 2016-09-13 17:22:28]

lgtm inspite of the ongoing discussion below.

Feel free to continue discussing / adapt the code a bit and land / land.

https://codereview.chromium.org/2337103003/diff/20001/components/ntp_snippets...
File components/ntp_snippets/category.cc (right):

https://codereview.chromium.org/2337103003/diff/20001/components/ntp_snippets...
components/ntp_snippets/category.cc:21:
static_cast<int>(KnownCategories::ARTICLES)};
On 2016/09/13 16:28:50, Marc Treib wrote:
> On 2016/09/13 16:08:05, jkrcal wrote:
> > I am just wondering:
> > 
> > Cannot you add to the enum class elements SERVER_CATEGORIES_MAX and
> > UNKNOWN_SERVER_CATEGORIES_OFFSET in the end, consistently number unknown
> server
> > categories starting after the second offset and check here only for the
> > following?
> > 
> > 0 <= id_ < LOCAL_CATEGORIES_COUNT || SERVER_CATEGORIES_OFFSET <= id_ <
> > SERVER_CATEGORIES_MAX
> 
> SERVER_CATEGORIES_MAX would actually have to be KNOWN_SERVER_CATEGORIES_MAX,
and
> that would imply that all the known server categories are contiguous. We won't
> necessarily be able to guarantee that.

How come? Is it because we can introduce some category on the server without the
client knowing it and later want to add better client-side support by adding it
to KnownCategories? In such case, we can change the number of the category, to
put it in the contiguous range, cannot we?

I do not want to introduce any real burden on the development just because of
avoiding one duplicity in the code ;) So far, I just fail to see the burden /
impossibility of what I propose.

> We could do the range check for local categories, and then list only the known
> server categories explicitly. WDYT?

I think this is better than what we currently have. If you are convinced that
what I propose above is impossible.

https://codereview.chromium.org/2337103003/diff/20001/components/ntp_snippets...
File components/ntp_snippets/category.h (right):

https://codereview.chromium.org/2337103003/diff/20001/components/ntp_snippets...
components/ntp_snippets/category.h:62: // Returns whether this category is any
of the known categories.
On 2016/09/13 16:28:50, Marc Treib wrote:
> On 2016/09/13 16:08:05, jkrcal wrote:
> > Without looking in the implementation, I would not understand what you mean
by
> > "is any known category". Maybe expand by ", i.e. any element of the
> > KnowCategories enum except for LOCAL_CATEGORIES_COUNT and
> > REMOTE_CATEGORIES_OFFSET." 
> 
> Added, except for the "except..." part. It should be impossible to create such
a
> Category instance (see CategoryFactory::FromKnownCategory), so IMO that part
of
> the comment isn't necessary.
> 
> > It is also inconsistent with IsKnownCategory which accepts these two cases
as
> > known categories. Admittedly, only an idiot would ask if this Category is
the
> > LOCAL_CATEGORIES_COUNT-category. Still, I would prefer IsKnownCategory to
> > blacklist these two cases to get the code consistent and self-documented.
> 
> Again, it shouldn't be possible to create those categories. Still, I've added
> DCHECKs in IsKnownCategory to make clear that those aren't valid values.

Agreed, thanks.

[tschumann, 2016-09-14 08:41:35]

https://codereview.chromium.org/2337103003/diff/20001/components/ntp_snippets...
File components/ntp_snippets/category.cc (right):

https://codereview.chromium.org/2337103003/diff/20001/components/ntp_snippets...
components/ntp_snippets/category.cc:21:
static_cast<int>(KnownCategories::ARTICLES)};
On 2016/09/13 17:22:27, jkrcal wrote:
> On 2016/09/13 16:28:50, Marc Treib wrote:
> > On 2016/09/13 16:08:05, jkrcal wrote:
> > > I am just wondering:
> > > 
> > > Cannot you add to the enum class elements SERVER_CATEGORIES_MAX and
> > > UNKNOWN_SERVER_CATEGORIES_OFFSET in the end, consistently number unknown
> > server
> > > categories starting after the second offset and check here only for the
> > > following?
> > > 
> > > 0 <= id_ < LOCAL_CATEGORIES_COUNT || SERVER_CATEGORIES_OFFSET <= id_ <
> > > SERVER_CATEGORIES_MAX
> > 
> > SERVER_CATEGORIES_MAX would actually have to be KNOWN_SERVER_CATEGORIES_MAX,
> and
> > that would imply that all the known server categories are contiguous. We
won't
> > necessarily be able to guarantee that.
> 
> How come? Is it because we can introduce some category on the server without
the
> client knowing it and later want to add better client-side support by adding
it
> to KnownCategories? In such case, we can change the number of the category, to
> put it in the contiguous range, cannot we?
> 
> I do not want to introduce any real burden on the development just because of
> avoiding one duplicity in the code ;) So far, I just fail to see the burden /
> impossibility of what I propose.
> 
> > We could do the range check for local categories, and then list only the
known
> > server categories explicitly. WDYT?
> 
> I think this is better than what we currently have. If you are convinced that
> what I propose above is impossible.

Joining this discussion as a by-stander. Given that we explicitly ask if
something is a known category, I actually like the explicit list. However, we
should not rely on folks remembering to update this list -- compilation or test
failures would be really nice. I guess we're on the same page here.
Any idea how we could enforce this?
If we introduce a KnownCategories::NUM_CATEGORIES, then we could simply DCHECK()
that arraysize(kKnownCategoryIds) == KnownCategories::NUM_CATEGORIES.

We could hide the creation of the array in separate function and check there.

[Marc Treib, 2016-09-14 08:42:20]

https://codereview.chromium.org/2337103003/diff/20001/components/ntp_snippets...
File components/ntp_snippets/category.cc (right):

https://codereview.chromium.org/2337103003/diff/20001/components/ntp_snippets...
components/ntp_snippets/category.cc:21:
static_cast<int>(KnownCategories::ARTICLES)};
On 2016/09/13 17:22:27, jkrcal wrote:
> On 2016/09/13 16:28:50, Marc Treib wrote:
> > On 2016/09/13 16:08:05, jkrcal wrote:
> > > I am just wondering:
> > > 
> > > Cannot you add to the enum class elements SERVER_CATEGORIES_MAX and
> > > UNKNOWN_SERVER_CATEGORIES_OFFSET in the end, consistently number unknown
> > server
> > > categories starting after the second offset and check here only for the
> > > following?
> > > 
> > > 0 <= id_ < LOCAL_CATEGORIES_COUNT || SERVER_CATEGORIES_OFFSET <= id_ <
> > > SERVER_CATEGORIES_MAX
> > 
> > SERVER_CATEGORIES_MAX would actually have to be KNOWN_SERVER_CATEGORIES_MAX,
> and
> > that would imply that all the known server categories are contiguous. We
won't
> > necessarily be able to guarantee that.
> 
> How come? Is it because we can introduce some category on the server without
the
> client knowing it and later want to add better client-side support by adding
it
> to KnownCategories? In such case, we can change the number of the category, to
> put it in the contiguous range, cannot we?
> 
> I do not want to introduce any real burden on the development just because of
> avoiding one duplicity in the code ;) So far, I just fail to see the burden /
> impossibility of what I propose.

Say the server adds categories 2 and 3. Later, we decide that the client should
know about 3, so the list of known remote categories would have a "hole".
Now, we *could* re-assign the IDs on the server-side, but it's potentially
problematic, and IMO just generally bad practice - we'd be re-interpreting data
which has already been sent over the wire.

> > We could do the range check for local categories, and then list only the
known
> > server categories explicitly. WDYT?
> 
> I think this is better than what we currently have. If you are convinced that
> what I propose above is impossible.

Done.

[Marc Treib, 2016-09-14 08:45:37]

On 2016/09/14 08:41:35, tschumann wrote:
>
https://codereview.chromium.org/2337103003/diff/20001/components/ntp_snippets...
> File components/ntp_snippets/category.cc (right):
> 
>
https://codereview.chromium.org/2337103003/diff/20001/components/ntp_snippets...
> components/ntp_snippets/category.cc:21:
> static_cast<int>(KnownCategories::ARTICLES)};
> On 2016/09/13 17:22:27, jkrcal wrote:
> > On 2016/09/13 16:28:50, Marc Treib wrote:
> > > On 2016/09/13 16:08:05, jkrcal wrote:
> > > > I am just wondering:
> > > > 
> > > > Cannot you add to the enum class elements SERVER_CATEGORIES_MAX and
> > > > UNKNOWN_SERVER_CATEGORIES_OFFSET in the end, consistently number unknown
> > > server
> > > > categories starting after the second offset and check here only for the
> > > > following?
> > > > 
> > > > 0 <= id_ < LOCAL_CATEGORIES_COUNT || SERVER_CATEGORIES_OFFSET <= id_ <
> > > > SERVER_CATEGORIES_MAX
> > > 
> > > SERVER_CATEGORIES_MAX would actually have to be
KNOWN_SERVER_CATEGORIES_MAX,
> > and
> > > that would imply that all the known server categories are contiguous. We
> won't
> > > necessarily be able to guarantee that.
> > 
> > How come? Is it because we can introduce some category on the server without
> the
> > client knowing it and later want to add better client-side support by adding
> it
> > to KnownCategories? In such case, we can change the number of the category,
to
> > put it in the contiguous range, cannot we?
> > 
> > I do not want to introduce any real burden on the development just because
of
> > avoiding one duplicity in the code ;) So far, I just fail to see the burden
/
> > impossibility of what I propose.
> > 
> > > We could do the range check for local categories, and then list only the
> known
> > > server categories explicitly. WDYT?
> > 
> > I think this is better than what we currently have. If you are convinced
that
> > what I propose above is impossible.
> 
> Joining this discussion as a by-stander. Given that we explicitly ask if
> something is a known category, I actually like the explicit list. However, we
> should not rely on folks remembering to update this list -- compilation or
test
> failures would be really nice. I guess we're on the same page here.

Now I changed it to Jan's proposal :D  But IMO that's okay: Clearly all local
categories are known, so there's no need to explicitly list them.

Yes, compilation/test failure would be awesome, but I don't see a way to achieve
that, given that the enum is not contiguous (remote categories in particular).

> Any idea how we could enforce this?

Nope :(

> If we introduce a KnownCategories::NUM_CATEGORIES, then we could simply
DCHECK()
> that arraysize(kKnownCategoryIds) == KnownCategories::NUM_CATEGORIES.

But then you'll have to remember to update NUM_CATEGORIES. Arguably that's a
slight improvement, since at least it's closer to where you add the new
category...

> We could hide the creation of the array in separate function and check there.

[Marc Treib, 2016-09-14 12:26:42]

One other alternative, brought up by Mikel: Just cast the int to an enum, then
switch over that (and handle the "no-case-matched" case).
Con: It's not strictly standard-compliant
Pro: ...but it's fairly common practice, at least for non-class enums, so
unlikely to actually break. And it means we won't need an explicit list of
values (or rather, we'll get a compile error if the switch is missing one).
Jan, Tim, WDYT? I'm not a big fan of depending on undefined behavior, but maybe
it's worth the trade-off here?

[tschumann, 2016-09-14 13:23:34]

On 2016/09/14 12:26:42, Marc Treib wrote:
> One other alternative, brought up by Mikel: Just cast the int to an enum, then
> switch over that (and handle the "no-case-matched" case).
> Con: It's not strictly standard-compliant
> Pro: ...but it's fairly common practice, at least for non-class enums, so
> unlikely to actually break. And it means we won't need an explicit list of
> values (or rather, we'll get a compile error if the switch is missing one).
> Jan, Tim, WDYT? I'm not a big fan of depending on undefined behavior, but
maybe
> it's worth the trade-off here?

That's my preferred option. I wasn't sure if it's allowed to cast any int to an
enum. But if we define the underlying type explicitly as an integer, I don't see
any reason against it.

[Marc Treib, 2016-09-14 13:33:56]

On 2016/09/14 13:23:34, tschumann wrote:
> On 2016/09/14 12:26:42, Marc Treib wrote:
> > One other alternative, brought up by Mikel: Just cast the int to an enum,
then
> > switch over that (and handle the "no-case-matched" case).
> > Con: It's not strictly standard-compliant
> > Pro: ...but it's fairly common practice, at least for non-class enums, so
> > unlikely to actually break. And it means we won't need an explicit list of
> > values (or rather, we'll get a compile error if the switch is missing one).
> > Jan, Tim, WDYT? I'm not a big fan of depending on undefined behavior, but
> maybe
> > it's worth the trade-off here?
> 
> That's my preferred option. I wasn't sure if it's allowed to cast any int to
an
> enum. But if we define the underlying type explicitly as an integer, I don't
see
> any reason against it.

Well, it's *not* allowed, as in, it's not well-defined behavior in the C++
standard. This kind of thing has bitten people before. It has the potential of
breaking at any point in the future, when the compiler implements some smart new
optimization etc. So, again, maybe it's worth the trade-off here, but I'm not
convinced.

[jkrcal, 2016-09-14 13:48:47]

On 2016/09/14 13:33:56, Marc Treib wrote:
> On 2016/09/14 13:23:34, tschumann wrote:
> > On 2016/09/14 12:26:42, Marc Treib wrote:
> > > One other alternative, brought up by Mikel: Just cast the int to an enum,
> then
> > > switch over that (and handle the "no-case-matched" case).
> > > Con: It's not strictly standard-compliant
> > > Pro: ...but it's fairly common practice, at least for non-class enums, so
> > > unlikely to actually break. And it means we won't need an explicit list of
> > > values (or rather, we'll get a compile error if the switch is missing
one).
> > > Jan, Tim, WDYT? I'm not a big fan of depending on undefined behavior, but
> > maybe
> > > it's worth the trade-off here?
> > 
> > That's my preferred option. I wasn't sure if it's allowed to cast any int to
> an
> > enum. But if we define the underlying type explicitly as an integer, I don't
> see
> > any reason against it.
> 
> Well, it's *not* allowed, as in, it's not well-defined behavior in the C++
> standard. This kind of thing has bitten people before. It has the potential of
> breaking at any point in the future, when the compiler implements some smart
new
> optimization etc. So, again, maybe it's worth the trade-off here, but I'm not
> convinced.

Sorry, I do not have any strong opinion. I do not have enough c++ expertise to
judge this.

[Marc Treib, 2016-09-16 14:01:10]

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

[commit-bot: I haz the power, 2016-09-16 14:01:51]

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

[Marc Treib, 2016-09-16 14:02:59]

After some more investigation: It *is* in fact legal to cast from int to enum
*if* the enum's range can represent the int value (even if that value isn't one
of the listed enum values). For enum classes, the underlying type is int by
default, so we're good. I've added a static_assert to make sure.
So, no need to list all the values explicitly anywhere anymore :)

See e.g.
https://www.securecoding.cert.org/confluence/display/cplusplus/INT50-CPP.+Do+...

[Marc Treib, 2016-09-16 14:03:25]

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

[Marc Treib, 2016-09-16 14:03:26]

+holte for histograms.xml. PTAL!

[commit-bot: I haz the power, 2016-09-16 14:12:57]

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

[commit-bot: I haz the power, 2016-09-16 14:12:58]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Marc Treib, 2016-09-16 15:03:22]

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

[commit-bot: I haz the power, 2016-09-16 15:03:43]

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

[jkrcal, 2016-09-16 16:26:08]

On 2016/09/16 14:02:59, Marc Treib wrote:
> After some more investigation: It *is* in fact legal to cast from int to enum
> *if* the enum's range can represent the int value (even if that value isn't
one
> of the listed enum values). For enum classes, the underlying type is int by
> default, so we're good. I've added a static_assert to make sure.
> So, no need to list all the values explicitly anywhere anymore :)
> 
> See e.g.
>
https://www.securecoding.cert.org/confluence/display/cplusplus/INT50-CPP.+Do+...

Cool, thanks, Marc!

[commit-bot: I haz the power, 2016-09-16 16:53:16]

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

[commit-bot: I haz the power, 2016-09-16 16:53:17]

Dry run: This issue passed the CQ dry run.

[Steven Holte, 2016-09-16 18:52:21]

lgtm

[Marc Treib, 2016-09-19 08:17:39]

Description was changed from

==========
[NTP Snippets] Add Category::IsAnyKnownCategory

This allows us to switch over all known categories in
content_suggestions_metrics.cc.
Also, add a histogram suffix for PHYSICAL_WEB_PAGES which was missing.

BUG=none
==========

to

==========
[NTP Snippets] Metrics: switch over known categories

This ensures that there'll be a compile error if we add a new category but don't
update the metrics code.
Also, add a histogram suffix for PHYSICAL_WEB_PAGES which was missing.

BUG=none
==========

[Marc Treib, 2016-09-19 08:18:04]

The CQ bit was checked by treib@chromium.org

[Marc Treib, 2016-09-19 08:18:05]

The patchset sent to the CQ was uploaded after l-g-t-m from jkrcal@chromium.org
Link to the patchset: https://codereview.chromium.org/2337103003/#ps100001
(title: "base::underlying_type :-/")

[commit-bot: I haz the power, 2016-09-19 08:18:11]

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

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

Description was changed from

==========
[NTP Snippets] Metrics: switch over known categories

This ensures that there'll be a compile error if we add a new category but don't
update the metrics code.
Also, add a histogram suffix for PHYSICAL_WEB_PAGES which was missing.

BUG=none
==========

to

==========
[NTP Snippets] Metrics: switch over known categories

This ensures that there'll be a compile error if we add a new category but don't
update the metrics code.
Also, add a histogram suffix for PHYSICAL_WEB_PAGES which was missing.

BUG=none
==========

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

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-09-19 09:11:04]

Description was changed from

==========
[NTP Snippets] Metrics: switch over known categories

This ensures that there'll be a compile error if we add a new category but don't
update the metrics code.
Also, add a histogram suffix for PHYSICAL_WEB_PAGES which was missing.

BUG=none
==========

to

==========
[NTP Snippets] Metrics: switch over known categories

This ensures that there'll be a compile error if we add a new category but don't
update the metrics code.
Also, add a histogram suffix for PHYSICAL_WEB_PAGES which was missing.

BUG=none

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

[commit-bot: I haz the power, 2016-09-19 09:11:05]

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