Issue 2187233002: Add ContentSuggestionsCategoryFactory; Store categories as ints

Philipp Keck

pke@google.com changed reviewers: + treib@chromium.org

4 years, 4 months ago (2016-07-28 08:21:54 UTC) #1

Marc Treib

Might be worth filing a separate bug for dynamic categories/ordering, WDYT? https://codereview.chromium.org/2187233002/diff/1/chrome/browser/android/ntp/ntp_snippets_bridge.h File chrome/browser/android/ntp/ntp_snippets_bridge.h (right): ...

4 years, 4 months ago (2016-07-28 11:41:46 UTC) #3

Philipp Keck

Description was changed from ========== Add ContentSuggestionsCategoryFactory; Store categories as ints Replace the ContentSuggestionsCategory enum ...

4 years, 4 months ago (2016-07-28 11:51:48 UTC) #4

tschumann

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

4 years, 4 months ago (2016-07-28 12:50:13 UTC) #5

tschumann

https://codereview.chromium.org/2187233002/diff/1/components/ntp_snippets/content_suggestions_category.h File components/ntp_snippets/content_suggestions_category.h (right): https://codereview.chromium.org/2187233002/diff/1/components/ntp_snippets/content_suggestions_category.h#newcode49 components/ntp_snippets/content_suggestions_category.h:49: inline bool operator==(const KnownSuggestionsCategories& left, On 2016/07/28 11:41:45, Marc ...

4 years, 4 months ago (2016-07-28 12:50:14 UTC) #6

Marc Treib

https://codereview.chromium.org/2187233002/diff/1/components/ntp_snippets/content_suggestions_category.h File components/ntp_snippets/content_suggestions_category.h (right): https://codereview.chromium.org/2187233002/diff/1/components/ntp_snippets/content_suggestions_category.h#newcode69 components/ntp_snippets/content_suggestions_category.h:69: std::ostream& operator<<(std::ostream& os, On 2016/07/28 12:50:14, tschumann wrote: > ...

4 years, 4 months ago (2016-07-28 12:54:02 UTC) #7

Philipp Keck

Description was changed from ========== Add ContentSuggestionsCategoryFactory; Store categories as ints Replace the ContentSuggestionsCategory enum ...

4 years, 4 months ago (2016-07-28 13:48:45 UTC) #8

Philipp Keck

Thanks for your comments. Created a new bug. https://codereview.chromium.org/2187233002/diff/1/chrome/browser/android/ntp/ntp_snippets_bridge.h File chrome/browser/android/ntp/ntp_snippets_bridge.h (right): https://codereview.chromium.org/2187233002/diff/1/chrome/browser/android/ntp/ntp_snippets_bridge.h#newcode81 chrome/browser/android/ntp/ntp_snippets_bridge.h:81: ntp_snippets::ContentSuggestionsCategoryFactory* ...

4 years, 4 months ago (2016-07-28 13:50:55 UTC) #9

Thanks for your comments.
Created a new bug.

https://codereview.chromium.org/2187233002/diff/1/chrome/browser/android/ntp/...
File chrome/browser/android/ntp/ntp_snippets_bridge.h (right):

https://codereview.chromium.org/2187233002/diff/1/chrome/browser/android/ntp/...
chrome/browser/android/ntp/ntp_snippets_bridge.h:81:
ntp_snippets::ContentSuggestionsCategoryFactory* category_factory_;
On 2016/07/28 11:41:45, Marc Treib wrote:
> Any particular reason for storing this separately, rather than just getting it
> from the service when you need it?

Done.

https://codereview.chromium.org/2187233002/diff/1/chrome/browser/ntp_snippets...
File chrome/browser/ntp_snippets/ntp_snippets_service_factory.cc (right):

https://codereview.chromium.org/2187233002/diff/1/chrome/browser/ntp_snippets...
chrome/browser/ntp_snippets/ntp_snippets_service_factory.cc:137:
content_suggestions_service->category_factory());
On 2016/07/28 11:41:45, Marc Treib wrote:
> misaligned

That's what the formatter does and I believe it's correct. 137 starts a new
parameter to the call in 122, so it needs to be indented one more than 122, e.g.
indented as 135.

https://codereview.chromium.org/2187233002/diff/1/chrome/browser/ui/webui/sni...
File chrome/browser/ui/webui/snippets_internals_message_handler.cc (right):

https://codereview.chromium.org/2187233002/diff/1/chrome/browser/ui/webui/sni...
chrome/browser/ui/webui/snippets_internals_message_handler.cc:78: std::string
MapCategoryName(ContentSuggestionsCategory category) {
On 2016/07/28 11:41:45, Marc Treib wrote:
> Should this be called GetCategoryName? Not sure what the "map" is saying here.

Done. GetCategoryTitle.

https://codereview.chromium.org/2187233002/diff/1/chrome/browser/ui/webui/sni...
chrome/browser/ui/webui/snippets_internals_message_handler.cc:79: // TODO(pke):
Replace this with the category's title.
On 2016/07/28 11:41:45, Marc Treib wrote:
> I guess this should be a comment on the function?

Done.

https://codereview.chromium.org/2187233002/diff/1/chrome/browser/ui/webui/sni...
chrome/browser/ui/webui/snippets_internals_message_handler.cc:89: std::string
MapCategoryStatus(ContentSuggestionsCategoryStatus status) {
On 2016/07/28 11:41:45, Marc Treib wrote:
> Similar here - GetCategoryStatusName?

Done.

https://codereview.chromium.org/2187233002/diff/1/components/ntp_snippets/con...
File components/ntp_snippets/content_suggestions_category.h (right):

https://codereview.chromium.org/2187233002/diff/1/components/ntp_snippets/con...
components/ntp_snippets/content_suggestions_category.h:22:
REMOTE_CATEGORIES_OFFSET = 10000,
On 2016/07/28 11:41:45, Marc Treib wrote:
> Should REMOTE_CATEGORIES_OFFSET itself be a valid category number? (Probably
> either way is fine, but right now I can't tell which it is)

No it shouldn't and the factory enforces that. I documented it in the header
comment here.

https://codereview.chromium.org/2187233002/diff/1/components/ntp_snippets/con...
components/ntp_snippets/content_suggestions_category.h:30: int id() const {
return id_; };
On 2016/07/28 11:41:45, Marc Treib wrote:
> nit: extra semicolon at the end

Done.

https://codereview.chromium.org/2187233002/diff/1/components/ntp_snippets/con...
components/ntp_snippets/content_suggestions_category.h:34:
ContentSuggestionsCategory(int id);
On 2016/07/28 11:41:45, Marc Treib wrote:
> empty line before

Done.

https://codereview.chromium.org/2187233002/diff/1/components/ntp_snippets/con...
components/ntp_snippets/content_suggestions_category.h:39: inline bool
operator==(const ContentSuggestionsCategory& left,
On 2016/07/28 11:41:45, Marc Treib wrote:
> All these shouldn't be inline.

Done.

https://codereview.chromium.org/2187233002/diff/1/components/ntp_snippets/con...
components/ntp_snippets/content_suggestions_category.h:46: return left.id() !=
right.id();
On 2016/07/28 11:41:45, Marc Treib wrote:
> It's a common practice to define != in terms of ==, i.e.
> return !(left == right);
> In this case, it doesn't save any lines of code, but it does make sure that ==
> and != are consistent.

Done.

https://codereview.chromium.org/2187233002/diff/1/components/ntp_snippets/con...
components/ntp_snippets/content_suggestions_category.h:49: inline bool
operator==(const KnownSuggestionsCategories& left,
On 2016/07/28 11:41:45, Marc Treib wrote:
> Hmm, not quite sure about these... maybe an explicit IsKnownCategory method
> would be clearer? Depends on how much we want a Category and a KnownCategory
to
> be "the same" I guess...

Done.

https://codereview.chromium.org/2187233002/diff/1/components/ntp_snippets/con...
components/ntp_snippets/content_suggestions_category.h:49: inline bool
operator==(const KnownSuggestionsCategories& left,
On 2016/07/28 12:50:14, tschumann wrote:
> On 2016/07/28 11:41:45, Marc Treib wrote:
> > Hmm, not quite sure about these... maybe an explicit IsKnownCategory method
> > would be clearer? Depends on how much we want a Category and a KnownCategory
> to
> > be "the same" I guess...
> 
> Yeah, this also seems a bit overkill to me. Ideally, we wouldn't support those
> comparisons and you would simply create a ContentSuggestionsCategory instance
> from the KnownSuggestionsCategories and compare with that.
> 
> the whole point of this class is that you can treat all categories the same.
> 
> That aside, if a IsKnownCategory function is sufficient, that's even simpler.

See below.

https://codereview.chromium.org/2187233002/diff/1/components/ntp_snippets/con...
components/ntp_snippets/content_suggestions_category.h:69: std::ostream&
operator<<(std::ostream& os,
On 2016/07/28 12:54:02, Marc Treib wrote:
> On 2016/07/28 12:50:14, tschumann wrote:
> > On 2016/07/28 11:41:45, Marc Treib wrote:
> > > What's this needed for - output in tests?
> > 
> > not sure about chrome, but in google3 you'd have a function like:
> > string DebugString() const;
> > 
> > if logging id() is not sufficient.
> 
> Chrome's LOG, DCHECK etc macros make use of operator<<. You can get by without
> it, but this makes things a bit more comfortable. IMO it's fine to define
this.

It's used in DCHECK_EQ, but it also comes in handy for debugging outputs in
ContentSuggestionsService. As long as the operator is consistently only used for
debugging and never for assembling strings for the user (which seems to be the
case in all the code I saw so far), it makes for nicely readable debugging
output statements. The style guide also encourages that: "Don't go out of your
way to avoid defining operator overloads. For example, prefer to define ==, =,
and <<, rather than Equals(), CopyFrom(), and PrintTo()".

That's also why I initially implemented the other comparison operators here.
Normally, comparing two things of different but similar types doesn't seem to be
a bad thing and an overloaded operator makes for nicely readable code. Now, the
code reads "someCategory.IsKnownCategory(KnownSuggestionsCategories::XYZ)". I
think there is little danger in accidentally using those wrong. But then the
operators do some conversions that are based on implementation details of the
two involved types, so they're not as straightforward as the implementation
suggests. That's why I'm also fine with an IsKnownCategory method.

https://codereview.chromium.org/2187233002/diff/1/components/ntp_snippets/con...
File components/ntp_snippets/content_suggestions_category_factory.cc (right):

https://codereview.chromium.org/2187233002/diff/1/components/ntp_snippets/con...
components/ntp_snippets/content_suggestions_category_factory.cc:22:
DCHECK_GE(static_cast<int>(known_category), 0);
On 2016/07/28 11:41:45, Marc Treib wrote:
> What is this for? We're getting a KnownSuggestionsCategories, so we can assume
> it has a valid value.

This is here because C++ allows you to do things like
"static_cast<KnownSuggestionsCategories>(999)" and it even delivers 999 if you
cast it back to int. A provider might read the value from a cache/DB or sth like
that and not even do the cast explicitly.

https://codereview.chromium.org/2187233002/diff/1/components/ntp_snippets/con...
components/ntp_snippets/content_suggestions_category_factory.cc:25:
DCHECK(categories_by_id_.find(static_cast<int>(known_category)) !=
On 2016/07/28 11:41:45, Marc Treib wrote:
> IMO slightly clearer written as
> DCHECK(categories_by_id_.count(...));

Done. And shorter. But when categories_by_id_ is gone, it's a long
std::find(_if).

https://codereview.chromium.org/2187233002/diff/1/components/ntp_snippets/con...
components/ntp_snippets/content_suggestions_category_factory.cc:28:
DCHECK_GT(known_category,
On 2016/07/28 11:41:45, Marc Treib wrote:
> Hm, so this basically checks that the enum has no values between
> LOCAL_CATEGORIES_COUNT and REMOTE_CATEGORIES_OFFSET. Since that's known at
> compile time, it'd be nice to check it statically, but I guess that's not
> possible...
> Could we have a unit test for it? Is there a way to iterate over all values of
> an enum class?

:D As far as I know, and I googled a lot, there is no way to enumerate an enum
in C++, even though that's what gave them their name..
If that was (or is?) possible, we could even make our design a bit nicer in some
places, and of course get rid of this DCHECK.

And additionally, this DCHECK protects against unlucky casts, just as the one in
line 22.

https://codereview.chromium.org/2187233002/diff/1/components/ntp_snippets/con...
components/ntp_snippets/content_suggestions_category_factory.cc:71: auto entry =
categories_by_id_.find(id);
On 2016/07/28 11:41:45, Marc Treib wrote:
> nit: call this |it|? As-is, it sounds like it's the actual element rather than
> an iterator.

Done.

https://codereview.chromium.org/2187233002/diff/1/components/ntp_snippets/con...
File components/ntp_snippets/content_suggestions_category_factory.h (right):

https://codereview.chromium.org/2187233002/diff/1/components/ntp_snippets/con...
components/ntp_snippets/content_suggestions_category_factory.h:8: #include
<stddef.h>
On 2016/07/28 11:41:45, Marc Treib wrote:
> Was this added automatically by Eclipse or something? Doesn't really matter,
> just curious.

This indirectly comes from ntp_snippets_service.h, I now deleted it here and in
content_suggestions_service.h, where it isn't needed.

https://codereview.chromium.org/2187233002/diff/1/components/ntp_snippets/con...
components/ntp_snippets/content_suggestions_category_factory.h:26:
ContentSuggestionsCategory FromRemoteCategory(int remote_id);
On 2016/07/28 12:50:14, tschumann wrote:
> i think passing in the index from the server response would be good, so that
we
> can keep the order we received from the server.

We can add that once we need it. Currently, the server-provider would just add
them in the order it received them. And the order cannot change (at least that's
what we agreed on).
Passing the index in here is only useful if you also pass in the provider, to
keep track of per-provider indices. And then you'd need some ordering among
providers. Currently, the ContentSuggestionsCategory and -Factory are nicely
decoupled from all the other stuff that's going on, but receiving the provider
instance here would lead to circular dependencies. We could use a provider_id
for that, but we don't have that yet. Neither do we need the indices yet, so
let's just worry about that once we need it. It would only be one provider that
needs to be adjusted then.

https://codereview.chromium.org/2187233002/diff/1/components/ntp_snippets/con...
components/ntp_snippets/content_suggestions_category_factory.h:27:
ContentSuggestionsCategory FromIDValue(int id);
On 2016/07/28 11:41:45, Marc Treib wrote:
> Please add comments for these. E.g. what must |remote_id| satisfy.

Done.

https://codereview.chromium.org/2187233002/diff/1/components/ntp_snippets/con...
components/ntp_snippets/content_suggestions_category_factory.h:29: bool
CompareCategories(const ContentSuggestionsCategory& left,
On 2016/07/28 11:41:45, Marc Treib wrote:
> Please add a comment on what exactly this does - I guess it's a "less than"?

Done.

https://codereview.chromium.org/2187233002/diff/1/components/ntp_snippets/con...
components/ntp_snippets/content_suggestions_category_factory.h:36:
std::vector<ContentSuggestionsCategory> categories_;
On 2016/07/28 11:41:46, Marc Treib wrote:
> Add a comment saying that these are in order?

Done.

https://codereview.chromium.org/2187233002/diff/1/components/ntp_snippets/con...
components/ntp_snippets/content_suggestions_category_factory.h:37: std::map<int,
ContentSuggestionsCategory> categories_by_id_;
On 2016/07/28 11:41:45, Marc Treib wrote:
> Hm. We could also leave this out and just do std::find on the vector, then we
> don't need two data structures. WDYT?

That's possible. I was hoping that the map is faster because it sorts the keys
(although that doesn't really matter). And also, it would either need to be a
find_if with a lambda expression, or we have to create a new
ContentSuggestionsCategory instance and then find that.

https://codereview.chromium.org/2187233002/diff/1/components/ntp_snippets/con...
File components/ntp_snippets/content_suggestions_category_status.h (right):

https://codereview.chromium.org/2187233002/diff/1/components/ntp_snippets/con...
components/ntp_snippets/content_suggestions_category_status.h:46: // or updating
them.
On 2016/07/28 11:41:46, Marc Treib wrote:
> nit: fits on the previous line

Done.

https://codereview.chromium.org/2187233002/diff/1/components/ntp_snippets/con...
File components/ntp_snippets/content_suggestions_provider.h (right):

https://codereview.chromium.org/2187233002/diff/1/components/ntp_snippets/con...
components/ntp_snippets/content_suggestions_provider.h:72: virtual
std::vector<ContentSuggestionsCategory> provided_categories() = 0;
On 2016/07/28 11:41:46, Marc Treib wrote:
> GetProvidedCategories - let's use hacker_style methods as rarely as possible
:)

Done.

https://codereview.chromium.org/2187233002/diff/1/components/ntp_snippets/con...
components/ntp_snippets/content_suggestions_provider.h:119:
ContentSuggestionsCategoryFactory* category_factory_;
On 2016/07/28 11:41:46, Marc Treib wrote:
> Non-private members aren't allowed. You'll have to make it private, and offer
a
> protected accessor.

Done.

https://codereview.chromium.org/2187233002/diff/1/components/ntp_snippets/con...
File components/ntp_snippets/content_suggestions_service.cc (right):

https://codereview.chromium.org/2187233002/diff/1/components/ntp_snippets/con...
components/ntp_snippets/content_suggestions_service.cc:168: for
(ContentSuggestion& suggestion : new_suggestions) {
On 2016/07/28 11:41:46, Marc Treib wrote:
> Why'd you remove the "const"?

That was unintentional.

https://codereview.chromium.org/2187233002/diff/1/components/ntp_snippets/con...
File components/ntp_snippets/content_suggestions_service.h (right):

https://codereview.chromium.org/2187233002/diff/1/components/ntp_snippets/con...
components/ntp_snippets/content_suggestions_service.h:68: struct
CompareCategoriesByID {
On 2016/07/28 11:41:46, Marc Treib wrote:
> This should be private.

Done.

https://codereview.chromium.org/2187233002/diff/1/components/ntp_snippets/con...
components/ntp_snippets/content_suggestions_service.h:130:
ContentSuggestionsCategoryFactory* category_factory() {
On 2016/07/28 11:41:46, Marc Treib wrote:
> Clients aren't supposed to register new categories with this, right? And also
> they shouldn't keep the pointer around? In that case, you could return a const
> ref.

Yes, the providers use this to register categories. And they do keep the pointer
around (none of the current providers does, but the server-provider will have to
do so, as it's the only way to access the factory).

https://codereview.chromium.org/2187233002/diff/1/components/ntp_snippets/con...
components/ntp_snippets/content_suggestions_service.h:154: // Provides new
categories and an order for them.
On 2016/07/28 11:41:46, Marc Treib wrote:
> Provides all categories really, no?

True. I put "new and existing" because "all" could confusingly also mean that
all categories are in there, even the inactive ones.

https://codereview.chromium.org/2187233002/diff/1/components/ntp_snippets/con...
File components/ntp_snippets/content_suggestions_service_unittest.cc (right):

https://codereview.chromium.org/2187233002/diff/1/components/ntp_snippets/con...
components/ntp_snippets/content_suggestions_service_unittest.cc:433: //
TODO(pke): Write a test for dynamic sections and ordering.
On 2016/07/28 11:41:46, Marc Treib wrote:
> TODOs in the code aren't the best tool for a personal task list ;)

Removed.

https://codereview.chromium.org/2187233002/diff/1/components/ntp_snippets/ntp...
File components/ntp_snippets/ntp_snippets_service.h (right):

https://codereview.chromium.org/2187233002/diff/1/components/ntp_snippets/ntp...
components/ntp_snippets/ntp_snippets_service.h:87:
ContentSuggestionsCategoryFactory* category_factory);
On 2016/07/28 11:41:46, Marc Treib wrote:
> I'd move this up to the other non-owned things, after suggestions_service.

Done.

https://codereview.chromium.org/2187233002/diff/1/components/ntp_snippets/ntp...
components/ntp_snippets/ntp_snippets_service.h:329: ContentSuggestionsCategory
provided_category_;
On 2016/07/28 11:41:46, Marc Treib wrote:
> const?

Done.

https://codereview.chromium.org/2187233002/diff/1/components/ntp_snippets/off...
File components/ntp_snippets/offline_pages/offline_page_suggestions_provider.h
(right):

https://codereview.chromium.org/2187233002/diff/1/components/ntp_snippets/off...
components/ntp_snippets/offline_pages/offline_page_suggestions_provider.h:77:
ContentSuggestionsCategory provided_category_;
On 2016/07/28 11:41:46, Marc Treib wrote:
> const?

Done.

Marc Treib

LGTM, thanks! https://codereview.chromium.org/2187233002/diff/1/chrome/browser/ntp_snippets/ntp_snippets_service_factory.cc File chrome/browser/ntp_snippets/ntp_snippets_service_factory.cc (right): https://codereview.chromium.org/2187233002/diff/1/chrome/browser/ntp_snippets/ntp_snippets_service_factory.cc#newcode137 chrome/browser/ntp_snippets/ntp_snippets_service_factory.cc:137: content_suggestions_service->category_factory()); On 2016/07/28 13:50:53, Philipp Keck wrote: ...

4 years, 4 months ago (2016-07-28 14:31:17 UTC) #10

LGTM, thanks!

https://codereview.chromium.org/2187233002/diff/1/chrome/browser/ntp_snippets...
File chrome/browser/ntp_snippets/ntp_snippets_service_factory.cc (right):

https://codereview.chromium.org/2187233002/diff/1/chrome/browser/ntp_snippets...
chrome/browser/ntp_snippets/ntp_snippets_service_factory.cc:137:
content_suggestions_service->category_factory());
On 2016/07/28 13:50:53, Philipp Keck wrote:
> On 2016/07/28 11:41:45, Marc Treib wrote:
> > misaligned
> 
> That's what the formatter does and I believe it's correct. 137 starts a new
> parameter to the call in 122, so it needs to be indented one more than 122,
e.g.
> indented as 135.

Ah true, I missed a paren.
This ctor has WAY too many parameters...

https://codereview.chromium.org/2187233002/diff/1/chrome/browser/ui/webui/sni...
File chrome/browser/ui/webui/snippets_internals_message_handler.cc (right):

https://codereview.chromium.org/2187233002/diff/1/chrome/browser/ui/webui/sni...
chrome/browser/ui/webui/snippets_internals_message_handler.cc:78: std::string
MapCategoryName(ContentSuggestionsCategory category) {
On 2016/07/28 13:50:53, Philipp Keck wrote:
> On 2016/07/28 11:41:45, Marc Treib wrote:
> > Should this be called GetCategoryName? Not sure what the "map" is saying
here.
> 
> Done. GetCategoryTitle.

It's not actually the title though, it's just a debug name. Then again, there's
a TODO there to make it the title, so okay.

https://codereview.chromium.org/2187233002/diff/1/components/ntp_snippets/con...
File components/ntp_snippets/content_suggestions_category_factory.cc (right):

https://codereview.chromium.org/2187233002/diff/1/components/ntp_snippets/con...
components/ntp_snippets/content_suggestions_category_factory.cc:22:
DCHECK_GE(static_cast<int>(known_category), 0);
On 2016/07/28 13:50:54, Philipp Keck wrote:
> On 2016/07/28 11:41:45, Marc Treib wrote:
> > What is this for? We're getting a KnownSuggestionsCategories, so we can
assume
> > it has a valid value.
> 
> This is here because C++ allows you to do things like
> "static_cast<KnownSuggestionsCategories>(999)" and it even delivers 999 if you
> cast it back to int. A provider might read the value from a cache/DB or sth
like
> that and not even do the cast explicitly.

For some value of "allows" - it might work in practice, but I'm pretty sure
that's undefined behavior.
Anyway, the DCHECK below would catch that case, so I'd remove this one.

https://codereview.chromium.org/2187233002/diff/1/components/ntp_snippets/con...
components/ntp_snippets/content_suggestions_category_factory.cc:28:
DCHECK_GT(known_category,
On 2016/07/28 13:50:54, Philipp Keck wrote:
> On 2016/07/28 11:41:45, Marc Treib wrote:
> > Hm, so this basically checks that the enum has no values between
> > LOCAL_CATEGORIES_COUNT and REMOTE_CATEGORIES_OFFSET. Since that's known at
> > compile time, it'd be nice to check it statically, but I guess that's not
> > possible...
> > Could we have a unit test for it? Is there a way to iterate over all values
of
> > an enum class?
> 
> :D As far as I know, and I googled a lot, there is no way to enumerate an enum
> in C++, even though that's what gave them their name..
> If that was (or is?) possible, we could even make our design a bit nicer in
some
> places, and of course get rid of this DCHECK.

Hm, I guess the closest thing is something like this (from StackOverflow):
enum class Color {
    blue,
    red,
    green = 5,
    purple
};
const std::array<Color,4> all_colors = {blue,red,green,purple};
...and then have a unit test that verifies that indeed all values are included,
using a switch. But that might be more trouble than it's worth.

> And additionally, this DCHECK protects against unlucky casts, just as the one
in
> line 22.

https://codereview.chromium.org/2187233002/diff/1/components/ntp_snippets/con...
File components/ntp_snippets/content_suggestions_category_factory.h (right):

https://codereview.chromium.org/2187233002/diff/1/components/ntp_snippets/con...
components/ntp_snippets/content_suggestions_category_factory.h:37: std::map<int,
ContentSuggestionsCategory> categories_by_id_;
On 2016/07/28 13:50:54, Philipp Keck wrote:
> On 2016/07/28 11:41:45, Marc Treib wrote:
> > Hm. We could also leave this out and just do std::find on the vector, then
we
> > don't need two data structures. WDYT?
> 
> That's possible. I was hoping that the map is faster because it sorts the keys
> (although that doesn't really matter). And also, it would either need to be a
> find_if with a lambda expression, or we have to create a new
> ContentSuggestionsCategory instance and then find that.

Rule of thumb: For < 100 small elements, vector will be faster than anything
else, because cache coherency. Anyway, performance won't be a problem either
way, I was just thinking of simplicity. I'll leave it to you :)

https://codereview.chromium.org/2187233002/diff/1/components/ntp_snippets/con...
File components/ntp_snippets/content_suggestions_service.h (right):

https://codereview.chromium.org/2187233002/diff/1/components/ntp_snippets/con...
components/ntp_snippets/content_suggestions_service.h:130:
ContentSuggestionsCategoryFactory* category_factory() {
On 2016/07/28 13:50:54, Philipp Keck wrote:
> On 2016/07/28 11:41:46, Marc Treib wrote:
> > Clients aren't supposed to register new categories with this, right? And
also
> > they shouldn't keep the pointer around? In that case, you could return a
const
> > ref.
> 
> Yes, the providers use this to register categories. And they do keep the
pointer
> around (none of the current providers does, but the server-provider will have
to
> do so, as it's the only way to access the factory).

Ah - technically the providers just get a Factory ptr passed in, but the
ProviderFactory gets it from here. Alright, carry on then.

Philipp Keck

https://codereview.chromium.org/2187233002/diff/1/components/ntp_snippets/content_suggestions_category_factory.cc File components/ntp_snippets/content_suggestions_category_factory.cc (right): https://codereview.chromium.org/2187233002/diff/1/components/ntp_snippets/content_suggestions_category_factory.cc#newcode22 components/ntp_snippets/content_suggestions_category_factory.cc:22: DCHECK_GE(static_cast<int>(known_category), 0); On 2016/07/28 14:31:16, Marc Treib wrote: > ...

4 years, 4 months ago (2016-07-28 14:55:47 UTC) #11

https://codereview.chromium.org/2187233002/diff/1/components/ntp_snippets/con...
File components/ntp_snippets/content_suggestions_category_factory.cc (right):

https://codereview.chromium.org/2187233002/diff/1/components/ntp_snippets/con...
components/ntp_snippets/content_suggestions_category_factory.cc:22:
DCHECK_GE(static_cast<int>(known_category), 0);
On 2016/07/28 14:31:16, Marc Treib wrote:
> On 2016/07/28 13:50:54, Philipp Keck wrote:
> > On 2016/07/28 11:41:45, Marc Treib wrote:
> > > What is this for? We're getting a KnownSuggestionsCategories, so we can
> assume
> > > it has a valid value.
> > 
> > This is here because C++ allows you to do things like
> > "static_cast<KnownSuggestionsCategories>(999)" and it even delivers 999 if
you
> > cast it back to int. A provider might read the value from a cache/DB or sth
> like
> > that and not even do the cast explicitly.
> 
> For some value of "allows" - it might work in practice, but I'm pretty sure
> that's undefined behavior.
> Anyway, the DCHECK below would catch that case, so I'd remove this one.

True. Done.

https://codereview.chromium.org/2187233002/diff/1/components/ntp_snippets/con...
components/ntp_snippets/content_suggestions_category_factory.cc:28:
DCHECK_GT(known_category,
On 2016/07/28 14:31:16, Marc Treib wrote:
> On 2016/07/28 13:50:54, Philipp Keck wrote:
> > On 2016/07/28 11:41:45, Marc Treib wrote:
> > > Hm, so this basically checks that the enum has no values between
> > > LOCAL_CATEGORIES_COUNT and REMOTE_CATEGORIES_OFFSET. Since that's known at
> > > compile time, it'd be nice to check it statically, but I guess that's not
> > > possible...
> > > Could we have a unit test for it? Is there a way to iterate over all
values
> of
> > > an enum class?
> > 
> > :D As far as I know, and I googled a lot, there is no way to enumerate an
enum
> > in C++, even though that's what gave them their name..
> > If that was (or is?) possible, we could even make our design a bit nicer in
> some
> > places, and of course get rid of this DCHECK.
> 
> Hm, I guess the closest thing is something like this (from StackOverflow):
> enum class Color {
>     blue,
>     red,
>     green = 5,
>     purple
> };
> const std::array<Color,4> all_colors = {blue,red,green,purple};
> ...and then have a unit test that verifies that indeed all values are
included,
> using a switch. But that might be more trouble than it's worth.
> 
> > And additionally, this DCHECK protects against unlucky casts, just as the
one
> in
> > line 22.

Acknowledged.

https://codereview.chromium.org/2187233002/diff/1/components/ntp_snippets/con...
File components/ntp_snippets/content_suggestions_category_factory.h (right):

https://codereview.chromium.org/2187233002/diff/1/components/ntp_snippets/con...
components/ntp_snippets/content_suggestions_category_factory.h:37: std::map<int,
ContentSuggestionsCategory> categories_by_id_;
On 2016/07/28 14:31:17, Marc Treib wrote:
> On 2016/07/28 13:50:54, Philipp Keck wrote:
> > On 2016/07/28 11:41:45, Marc Treib wrote:
> > > Hm. We could also leave this out and just do std::find on the vector, then
> we
> > > don't need two data structures. WDYT?
> > 
> > That's possible. I was hoping that the map is faster because it sorts the
keys
> > (although that doesn't really matter). And also, it would either need to be
a
> > find_if with a lambda expression, or we have to create a new
> > ContentSuggestionsCategory instance and then find that.
> 
> Rule of thumb: For < 100 small elements, vector will be faster than anything
> else, because cache coherency. Anyway, performance won't be a problem either
> way, I was just thinking of simplicity. I'll leave it to you :)

I changed it and introduced a CategoryExists helper function.

https://codereview.chromium.org/2187233002/diff/1/components/ntp_snippets/con...
File components/ntp_snippets/content_suggestions_service.h (right):

https://codereview.chromium.org/2187233002/diff/1/components/ntp_snippets/con...
components/ntp_snippets/content_suggestions_service.h:130:
ContentSuggestionsCategoryFactory* category_factory() {
On 2016/07/28 14:31:17, Marc Treib wrote:
> On 2016/07/28 13:50:54, Philipp Keck wrote:
> > On 2016/07/28 11:41:46, Marc Treib wrote:
> > > Clients aren't supposed to register new categories with this, right? And
> also
> > > they shouldn't keep the pointer around? In that case, you could return a
> const
> > > ref.
> > 
> > Yes, the providers use this to register categories. And they do keep the
> pointer
> > around (none of the current providers does, but the server-provider will
have
> to
> > do so, as it's the only way to access the factory).
> 
> Ah - technically the providers just get a Factory ptr passed in, but the
> ProviderFactory gets it from here. Alright, carry on then.

Acknowledged.

Philipp Keck

pke@google.com changed reviewers: + bauerb@chromium.org

4 years, 4 months ago (2016-07-28 14:56:28 UTC) #12

Philipp Keck

bauerb@chromium.org: Please review changes in chrome/browser/resources/snippets_internals.html chrome/browser/ui/webui/snippets_internals_message_handler.cc

4 years, 4 months ago (2016-07-28 14:56:29 UTC) #13

tschumann

https://codereview.chromium.org/2187233002/diff/1/components/ntp_snippets/content_suggestions_category.h File components/ntp_snippets/content_suggestions_category.h (right): https://codereview.chromium.org/2187233002/diff/1/components/ntp_snippets/content_suggestions_category.h#newcode69 components/ntp_snippets/content_suggestions_category.h:69: std::ostream& operator<<(std::ostream& os, On 2016/07/28 13:50:54, Philipp Keck wrote: ...

4 years, 4 months ago (2016-07-28 15:03:45 UTC) #14

https://codereview.chromium.org/2187233002/diff/1/components/ntp_snippets/con...
File components/ntp_snippets/content_suggestions_category.h (right):

https://codereview.chromium.org/2187233002/diff/1/components/ntp_snippets/con...
components/ntp_snippets/content_suggestions_category.h:69: std::ostream&
operator<<(std::ostream& os,
On 2016/07/28 13:50:54, Philipp Keck wrote:
> On 2016/07/28 12:54:02, Marc Treib wrote:
> > On 2016/07/28 12:50:14, tschumann wrote:
> > > On 2016/07/28 11:41:45, Marc Treib wrote:
> > > > What's this needed for - output in tests?
> > > 
> > > not sure about chrome, but in google3 you'd have a function like:
> > > string DebugString() const;
> > > 
> > > if logging id() is not sufficient.
> > 
> > Chrome's LOG, DCHECK etc macros make use of operator<<. You can get by
without
> > it, but this makes things a bit more comfortable. IMO it's fine to define
> this.
> 
> It's used in DCHECK_EQ, but it also comes in handy for debugging outputs in
> ContentSuggestionsService. As long as the operator is consistently only used
for
> debugging and never for assembling strings for the user (which seems to be the
> case in all the code I saw so far), it makes for nicely readable debugging
> output statements. The style guide also encourages that: "Don't go out of your
> way to avoid defining operator overloads. For example, prefer to define ==, =,
> and <<, rather than Equals(), CopyFrom(), and PrintTo()".
> 
> That's also why I initially implemented the other comparison operators here.
> Normally, comparing two things of different but similar types doesn't seem to
be
> a bad thing and an overloaded operator makes for nicely readable code. Now,
the
> code reads "someCategory.IsKnownCategory(KnownSuggestionsCategories::XYZ)". I
> think there is little danger in accidentally using those wrong. But then the
> operators do some conversions that are based on implementation details of the
> two involved types, so they're not as straightforward as the implementation
> suggests. That's why I'm also fine with an IsKnownCategory method.

ok, so this means no operator== comparing KnownSuggestionsCategories with
ContentSuggestionsCategory?

operator<<() makes sense. I wasn't up-to-date with the style guide. It used to
be discouraged.

https://codereview.chromium.org/2187233002/diff/1/components/ntp_snippets/con...
File components/ntp_snippets/content_suggestions_category_factory.cc (right):

https://codereview.chromium.org/2187233002/diff/1/components/ntp_snippets/con...
components/ntp_snippets/content_suggestions_category_factory.cc:22:
DCHECK_GE(static_cast<int>(known_category), 0);
On 2016/07/28 14:31:16, Marc Treib wrote:
> On 2016/07/28 13:50:54, Philipp Keck wrote:
> > On 2016/07/28 11:41:45, Marc Treib wrote:
> > > What is this for? We're getting a KnownSuggestionsCategories, so we can
> assume
> > > it has a valid value.
> > 
> > This is here because C++ allows you to do things like
> > "static_cast<KnownSuggestionsCategories>(999)" and it even delivers 999 if
you
> > cast it back to int. A provider might read the value from a cache/DB or sth
> like
> > that and not even do the cast explicitly.
> 
> For some value of "allows" - it might work in practice, but I'm pretty sure
> that's undefined behavior.
> Anyway, the DCHECK below would catch that case, so I'd remove this one.

nit: I believe it's unspecified ;-) But yes, we'd be in troubles anyways if that
happens. Let's remove that check.

https://codereview.chromium.org/2187233002/diff/1/components/ntp_snippets/con...
File components/ntp_snippets/content_suggestions_category_factory.h (right):

https://codereview.chromium.org/2187233002/diff/1/components/ntp_snippets/con...
components/ntp_snippets/content_suggestions_category_factory.h:26:
ContentSuggestionsCategory FromRemoteCategory(int remote_id);
On 2016/07/28 13:50:54, Philipp Keck wrote:
> On 2016/07/28 12:50:14, tschumann wrote:
> > i think passing in the index from the server response would be good, so that
> we
> > can keep the order we received from the server.
> 
> We can add that once we need it. Currently, the server-provider would just add
> them in the order it received them. And the order cannot change (at least
that's
> what we agreed on).
> Passing the index in here is only useful if you also pass in the provider, to
> keep track of per-provider indices. And then you'd need some ordering among
> providers. Currently, the ContentSuggestionsCategory and -Factory are nicely
> decoupled from all the other stuff that's going on, but receiving the provider
> instance here would lead to circular dependencies. We could use a provider_id
> for that, but we don't have that yet. Neither do we need the indices yet, so
> let's just worry about that once we need it. It would only be one provider
that
> needs to be adjusted then.

The problem i'm seeing is: Keeping the order as we received it from the service
is a requirement and it only works due to undocumented implementation details.
If for some reason we change how the provider processes the server response,
we'd violate the order requirement.
I'd rather hard-code that we only support one remote-provider. If you pass the
second instance, we could have the following semantics:
-- the first time we see a remote_id, we use the given index as a secondary
sort-key (i.e. as a tie breaker when comparing remote categories.
-- if another remote_id already used that secondary sort-key, we'd assign a new
one (the next available secondary key) essentially sorting at the end. This is
needed in case we get updates in a different order.

another nit: should we rename 'remote_id' to 'remote_category'? (would be in
line with 'known_category'.

Marc Treib

https://codereview.chromium.org/2187233002/diff/1/components/ntp_snippets/content_suggestions_category_factory.h File components/ntp_snippets/content_suggestions_category_factory.h (right): https://codereview.chromium.org/2187233002/diff/1/components/ntp_snippets/content_suggestions_category_factory.h#newcode26 components/ntp_snippets/content_suggestions_category_factory.h:26: ContentSuggestionsCategory FromRemoteCategory(int remote_id); On 2016/07/28 15:03:45, tschumann wrote: > ...

4 years, 4 months ago (2016-07-28 15:13:26 UTC) #15

https://codereview.chromium.org/2187233002/diff/1/components/ntp_snippets/con...
File components/ntp_snippets/content_suggestions_category_factory.h (right):

https://codereview.chromium.org/2187233002/diff/1/components/ntp_snippets/con...
components/ntp_snippets/content_suggestions_category_factory.h:26:
ContentSuggestionsCategory FromRemoteCategory(int remote_id);
On 2016/07/28 15:03:45, tschumann wrote:
> On 2016/07/28 13:50:54, Philipp Keck wrote:
> > On 2016/07/28 12:50:14, tschumann wrote:
> > > i think passing in the index from the server response would be good, so
that
> > we
> > > can keep the order we received from the server.
> > 
> > We can add that once we need it. Currently, the server-provider would just
add
> > them in the order it received them. And the order cannot change (at least
> that's
> > what we agreed on).
> > Passing the index in here is only useful if you also pass in the provider,
to
> > keep track of per-provider indices. And then you'd need some ordering among
> > providers. Currently, the ContentSuggestionsCategory and -Factory are nicely
> > decoupled from all the other stuff that's going on, but receiving the
provider
> > instance here would lead to circular dependencies. We could use a
provider_id
> > for that, but we don't have that yet. Neither do we need the indices yet, so
> > let's just worry about that once we need it. It would only be one provider
> that
> > needs to be adjusted then.
> 
> The problem i'm seeing is: Keeping the order as we received it from the
service
> is a requirement and it only works due to undocumented implementation details.
> If for some reason we change how the provider processes the server response,
> we'd violate the order requirement.
> I'd rather hard-code that we only support one remote-provider. If you pass the
> second instance, we could have the following semantics:
> -- the first time we see a remote_id, we use the given index as a secondary
> sort-key (i.e. as a tie breaker when comparing remote categories.
> -- if another remote_id already used that secondary sort-key, we'd assign a
new
> one (the next available secondary key) essentially sorting at the end. This is
> needed in case we get updates in a different order.

Or, alternatively: Document that remote categories will be ordered by order of
creation. That way, we don't introduce any extra unused plumbing right now, and
longer-term, the ordering will change anyway and likely those secondary
sort-keys won't cover it. YAGNI

> another nit: should we rename 'remote_id' to 'remote_category'? (would be in
> line with 'known_category'.

That makes sense!

Philipp Keck

https://codereview.chromium.org/2187233002/diff/1/components/ntp_snippets/content_suggestions_category.h File components/ntp_snippets/content_suggestions_category.h (right): https://codereview.chromium.org/2187233002/diff/1/components/ntp_snippets/content_suggestions_category.h#newcode69 components/ntp_snippets/content_suggestions_category.h:69: std::ostream& operator<<(std::ostream& os, On 2016/07/28 15:03:45, tschumann wrote: > ...

4 years, 4 months ago (2016-07-28 15:15:25 UTC) #16

https://codereview.chromium.org/2187233002/diff/1/components/ntp_snippets/con...
File components/ntp_snippets/content_suggestions_category.h (right):

https://codereview.chromium.org/2187233002/diff/1/components/ntp_snippets/con...
components/ntp_snippets/content_suggestions_category.h:69: std::ostream&
operator<<(std::ostream& os,
On 2016/07/28 15:03:45, tschumann wrote:
> On 2016/07/28 13:50:54, Philipp Keck wrote:
> > On 2016/07/28 12:54:02, Marc Treib wrote:
> > > On 2016/07/28 12:50:14, tschumann wrote:
> > > > On 2016/07/28 11:41:45, Marc Treib wrote:
> > > > > What's this needed for - output in tests?
> > > > 
> > > > not sure about chrome, but in google3 you'd have a function like:
> > > > string DebugString() const;
> > > > 
> > > > if logging id() is not sufficient.
> > > 
> > > Chrome's LOG, DCHECK etc macros make use of operator<<. You can get by
> without
> > > it, but this makes things a bit more comfortable. IMO it's fine to define
> > this.
> > 
> > It's used in DCHECK_EQ, but it also comes in handy for debugging outputs in
> > ContentSuggestionsService. As long as the operator is consistently only used
> for
> > debugging and never for assembling strings for the user (which seems to be
the
> > case in all the code I saw so far), it makes for nicely readable debugging
> > output statements. The style guide also encourages that: "Don't go out of
your
> > way to avoid defining operator overloads. For example, prefer to define ==,
=,
> > and <<, rather than Equals(), CopyFrom(), and PrintTo()".
> > 
> > That's also why I initially implemented the other comparison operators here.
> > Normally, comparing two things of different but similar types doesn't seem
to
> be
> > a bad thing and an overloaded operator makes for nicely readable code. Now,
> the
> > code reads "someCategory.IsKnownCategory(KnownSuggestionsCategories::XYZ)".
I
> > think there is little danger in accidentally using those wrong. But then the
> > operators do some conversions that are based on implementation details of
the
> > two involved types, so they're not as straightforward as the implementation
> > suggests. That's why I'm also fine with an IsKnownCategory method.
> 
> ok, so this means no operator== comparing KnownSuggestionsCategories with
> ContentSuggestionsCategory?
> 
> operator<<() makes sense. I wasn't up-to-date with the style guide. It used to
> be discouraged.

Yes, I removed the operator== comparing KnownSuggestionsCategories with
ContentSuggestionsCategory.

https://codereview.chromium.org/2187233002/diff/1/components/ntp_snippets/con...
File components/ntp_snippets/content_suggestions_category_factory.cc (right):

https://codereview.chromium.org/2187233002/diff/1/components/ntp_snippets/con...
components/ntp_snippets/content_suggestions_category_factory.cc:22:
DCHECK_GE(static_cast<int>(known_category), 0);
On 2016/07/28 15:03:45, tschumann wrote:
> On 2016/07/28 14:31:16, Marc Treib wrote:
> > On 2016/07/28 13:50:54, Philipp Keck wrote:
> > > On 2016/07/28 11:41:45, Marc Treib wrote:
> > > > What is this for? We're getting a KnownSuggestionsCategories, so we can
> > assume
> > > > it has a valid value.
> > > 
> > > This is here because C++ allows you to do things like
> > > "static_cast<KnownSuggestionsCategories>(999)" and it even delivers 999 if
> you
> > > cast it back to int. A provider might read the value from a cache/DB or
sth
> > like
> > > that and not even do the cast explicitly.
> > 
> > For some value of "allows" - it might work in practice, but I'm pretty sure
> > that's undefined behavior.
> > Anyway, the DCHECK below would catch that case, so I'd remove this one.
> 
> nit: I believe it's unspecified ;-) But yes, we'd be in troubles anyways if
that
> happens. Let's remove that check.

Acknowledged.

https://codereview.chromium.org/2187233002/diff/1/components/ntp_snippets/con...
File components/ntp_snippets/content_suggestions_category_factory.h (right):

https://codereview.chromium.org/2187233002/diff/1/components/ntp_snippets/con...
components/ntp_snippets/content_suggestions_category_factory.h:26:
ContentSuggestionsCategory FromRemoteCategory(int remote_id);
On 2016/07/28 15:03:45, tschumann wrote:
> On 2016/07/28 13:50:54, Philipp Keck wrote:
> > On 2016/07/28 12:50:14, tschumann wrote:
> > > i think passing in the index from the server response would be good, so
that
> > we
> > > can keep the order we received from the server.
> > 
> > We can add that once we need it. Currently, the server-provider would just
add
> > them in the order it received them. And the order cannot change (at least
> that's
> > what we agreed on).
> > Passing the index in here is only useful if you also pass in the provider,
to
> > keep track of per-provider indices. And then you'd need some ordering among
> > providers. Currently, the ContentSuggestionsCategory and -Factory are nicely
> > decoupled from all the other stuff that's going on, but receiving the
provider
> > instance here would lead to circular dependencies. We could use a
provider_id
> > for that, but we don't have that yet. Neither do we need the indices yet, so
> > let's just worry about that once we need it. It would only be one provider
> that
> > needs to be adjusted then.
> 
> The problem i'm seeing is: Keeping the order as we received it from the
service
> is a requirement and it only works due to undocumented implementation details.
> If for some reason we change how the provider processes the server response,
> we'd violate the order requirement.
> I'd rather hard-code that we only support one remote-provider. If you pass the
> second instance, we could have the following semantics:
> -- the first time we see a remote_id, we use the given index as a secondary
> sort-key (i.e. as a tie breaker when comparing remote categories.
> -- if another remote_id already used that secondary sort-key, we'd assign a
new
> one (the next available secondary key) essentially sorting at the end. This is
> needed in case we get updates in a different order.
> 
> another nit: should we rename 'remote_id' to 'remote_category'? (would be in
> line with 'known_category'.

We could document the way it works in this file. It's also in the design doc.
There currently is no sorting at all, categories are simply kept in the order in
which they were added. So it wouldn't be an implementation detail of the
provider, but it's a requirement that the provider has to fulfill when using
this interface: If the provider wants its categories to appear in the right
order, it needs to sort them the way it needs them before calling
FromRemoteCategory for each of them.

I don't see where and how we could hard-code the one remote-provider (or do we
just trust that only one provider ever uses this method?) and where we would
store that secondary sort-key. We would need a separate data structure for that,
or we would have to pack it into the Category itself. I could create a follow-up
CL which attempts that and we can discuss it there? But since the current
approach works, maybe documenting it is sufficient?

I'd call something a "category" if it unambiguously is a category. And something
is a "[category] id" if it's an int that somehow refers to a category. There is
a different between the "ids" we store in ContentSuggestionsCategory::id_ and
the 'remote_id' passed in here -- namely a numerical difference of 10000. So the
parameter is not the category itself, it's something that refers to it.
Maybe the method should be named "FromRemoteCategoryID" and the one below should
be named "FromCategoryID"?

Philipp Keck

https://codereview.chromium.org/2187233002/diff/1/components/ntp_snippets/content_suggestions_category_factory.h File components/ntp_snippets/content_suggestions_category_factory.h (right): https://codereview.chromium.org/2187233002/diff/1/components/ntp_snippets/content_suggestions_category_factory.h#newcode26 components/ntp_snippets/content_suggestions_category_factory.h:26: ContentSuggestionsCategory FromRemoteCategory(int remote_id); On 2016/07/28 15:13:26, Marc Treib wrote: ...

4 years, 4 months ago (2016-07-28 15:25:59 UTC) #17

https://codereview.chromium.org/2187233002/diff/1/components/ntp_snippets/con...
File components/ntp_snippets/content_suggestions_category_factory.h (right):

https://codereview.chromium.org/2187233002/diff/1/components/ntp_snippets/con...
components/ntp_snippets/content_suggestions_category_factory.h:26:
ContentSuggestionsCategory FromRemoteCategory(int remote_id);
On 2016/07/28 15:13:26, Marc Treib wrote:
> On 2016/07/28 15:03:45, tschumann wrote:
> > On 2016/07/28 13:50:54, Philipp Keck wrote:
> > > On 2016/07/28 12:50:14, tschumann wrote:
> > > > i think passing in the index from the server response would be good, so
> that
> > > we
> > > > can keep the order we received from the server.
> > > 
> > > We can add that once we need it. Currently, the server-provider would just
> add
> > > them in the order it received them. And the order cannot change (at least
> > that's
> > > what we agreed on).
> > > Passing the index in here is only useful if you also pass in the provider,
> to
> > > keep track of per-provider indices. And then you'd need some ordering
among
> > > providers. Currently, the ContentSuggestionsCategory and -Factory are
nicely
> > > decoupled from all the other stuff that's going on, but receiving the
> provider
> > > instance here would lead to circular dependencies. We could use a
> provider_id
> > > for that, but we don't have that yet. Neither do we need the indices yet,
so
> > > let's just worry about that once we need it. It would only be one provider
> > that
> > > needs to be adjusted then.
> > 
> > The problem i'm seeing is: Keeping the order as we received it from the
> service
> > is a requirement and it only works due to undocumented implementation
details.
> > If for some reason we change how the provider processes the server response,
> > we'd violate the order requirement.
> > I'd rather hard-code that we only support one remote-provider. If you pass
the
> > second instance, we could have the following semantics:
> > -- the first time we see a remote_id, we use the given index as a secondary
> > sort-key (i.e. as a tie breaker when comparing remote categories.
> > -- if another remote_id already used that secondary sort-key, we'd assign a
> new
> > one (the next available secondary key) essentially sorting at the end. This
is
> > needed in case we get updates in a different order.
> 
> Or, alternatively: Document that remote categories will be ordered by order of
> creation. That way, we don't introduce any extra unused plumbing right now,
and
> longer-term, the ordering will change anyway and likely those secondary
> sort-keys won't cover it. YAGNI
Agree. I documented it here and on CompareCategories below.

> 
> > another nit: should we rename 'remote_id' to 'remote_category'? (would be in
> > line with 'known_category'.
> 
> That makes sense!

I'm fine with renaming it, but I still think there is a difference between
categories and their (different kinds of) IDs.

tschumann

https://codereview.chromium.org/2187233002/diff/1/components/ntp_snippets/content_suggestions_category_factory.h File components/ntp_snippets/content_suggestions_category_factory.h (right): https://codereview.chromium.org/2187233002/diff/1/components/ntp_snippets/content_suggestions_category_factory.h#newcode26 components/ntp_snippets/content_suggestions_category_factory.h:26: ContentSuggestionsCategory FromRemoteCategory(int remote_id); On 2016/07/28 15:25:58, Philipp Keck wrote: ...

4 years, 4 months ago (2016-07-28 15:36:10 UTC) #18

https://codereview.chromium.org/2187233002/diff/1/components/ntp_snippets/con...
File components/ntp_snippets/content_suggestions_category_factory.h (right):

https://codereview.chromium.org/2187233002/diff/1/components/ntp_snippets/con...
components/ntp_snippets/content_suggestions_category_factory.h:26:
ContentSuggestionsCategory FromRemoteCategory(int remote_id);
On 2016/07/28 15:25:58, Philipp Keck wrote:
> On 2016/07/28 15:13:26, Marc Treib wrote:
> > On 2016/07/28 15:03:45, tschumann wrote:
> > > On 2016/07/28 13:50:54, Philipp Keck wrote:
> > > > On 2016/07/28 12:50:14, tschumann wrote:
> > > > > i think passing in the index from the server response would be good,
so
> > that
> > > > we
> > > > > can keep the order we received from the server.
> > > > 
> > > > We can add that once we need it. Currently, the server-provider would
just
> > add
> > > > them in the order it received them. And the order cannot change (at
least
> > > that's
> > > > what we agreed on).
> > > > Passing the index in here is only useful if you also pass in the
provider,
> > to
> > > > keep track of per-provider indices. And then you'd need some ordering
> among
> > > > providers. Currently, the ContentSuggestionsCategory and -Factory are
> nicely
> > > > decoupled from all the other stuff that's going on, but receiving the
> > provider
> > > > instance here would lead to circular dependencies. We could use a
> > provider_id
> > > > for that, but we don't have that yet. Neither do we need the indices
yet,
> so
> > > > let's just worry about that once we need it. It would only be one
provider
> > > that
> > > > needs to be adjusted then.
> > > 
> > > The problem i'm seeing is: Keeping the order as we received it from the
> > service
> > > is a requirement and it only works due to undocumented implementation
> details.
> > > If for some reason we change how the provider processes the server
response,
> > > we'd violate the order requirement.
> > > I'd rather hard-code that we only support one remote-provider. If you pass
> the
> > > second instance, we could have the following semantics:
> > > -- the first time we see a remote_id, we use the given index as a
secondary
> > > sort-key (i.e. as a tie breaker when comparing remote categories.
> > > -- if another remote_id already used that secondary sort-key, we'd assign
a
> > new
> > > one (the next available secondary key) essentially sorting at the end.
This
> is
> > > needed in case we get updates in a different order.
> > 
> > Or, alternatively: Document that remote categories will be ordered by order
of
> > creation. That way, we don't introduce any extra unused plumbing right now,
> and
> > longer-term, the ordering will change anyway and likely those secondary
> > sort-keys won't cover it. YAGNI
> Agree. I documented it here and on CompareCategories below.
> 
> > 
> > > another nit: should we rename 'remote_id' to 'remote_category'? (would be
in
> > > line with 'known_category'.
> > 
> > That makes sense!
> 
> I'm fine with renaming it, but I still think there is a difference between
> categories and their (different kinds of) IDs.
yes, hence the different type and factory function. if you call it a id, then it
gets confusing with FromIDValue() which in addition has the same type as
'remote_id' -- and we really want to keep those things separated.

https://codereview.chromium.org/2187233002/diff/1/components/ntp_snippets/con...
components/ntp_snippets/content_suggestions_category_factory.h:26:
ContentSuggestionsCategory FromRemoteCategory(int remote_id);
On 2016/07/28 15:15:25, Philipp Keck wrote:
> On 2016/07/28 15:03:45, tschumann wrote:
> > On 2016/07/28 13:50:54, Philipp Keck wrote:
> > > On 2016/07/28 12:50:14, tschumann wrote:
> > > > i think passing in the index from the server response would be good, so
> that
> > > we
> > > > can keep the order we received from the server.
> > > 
> > > We can add that once we need it. Currently, the server-provider would just
> add
> > > them in the order it received them. And the order cannot change (at least
> > that's
> > > what we agreed on).
> > > Passing the index in here is only useful if you also pass in the provider,
> to
> > > keep track of per-provider indices. And then you'd need some ordering
among
> > > providers. Currently, the ContentSuggestionsCategory and -Factory are
nicely
> > > decoupled from all the other stuff that's going on, but receiving the
> provider
> > > instance here would lead to circular dependencies. We could use a
> provider_id
> > > for that, but we don't have that yet. Neither do we need the indices yet,
so
> > > let's just worry about that once we need it. It would only be one provider
> > that
> > > needs to be adjusted then.
> > 
> > The problem i'm seeing is: Keeping the order as we received it from the
> service
> > is a requirement and it only works due to undocumented implementation
details.
> > If for some reason we change how the provider processes the server response,
> > we'd violate the order requirement.
> > I'd rather hard-code that we only support one remote-provider. If you pass
the
> > second instance, we could have the following semantics:
> > -- the first time we see a remote_id, we use the given index as a secondary
> > sort-key (i.e. as a tie breaker when comparing remote categories.
> > -- if another remote_id already used that secondary sort-key, we'd assign a
> new
> > one (the next available secondary key) essentially sorting at the end. This
is
> > needed in case we get updates in a different order.
> > 
> > another nit: should we rename 'remote_id' to 'remote_category'? (would be in
> > line with 'known_category'.
> 
> We could document the way it works in this file. It's also in the design doc.
> There currently is no sorting at all, categories are simply kept in the order
in
> which they were added. So it wouldn't be an implementation detail of the
> provider, but it's a requirement that the provider has to fulfill when using
> this interface: If the provider wants its categories to appear in the right
> order, it needs to sort them the way it needs them before calling
> FromRemoteCategory for each of them.
> 
> I don't see where and how we could hard-code the one remote-provider (or do we
> just trust that only one provider ever uses this method?) and where we would
> store that secondary sort-key. We would need a separate data structure for
that,
> or we would have to pack it into the Category itself. I could create a
follow-up
> CL which attempts that and we can discuss it there? But since the current
> approach works, maybe documenting it is sufficient?
> 
> I'd call something a "category" if it unambiguously is a category. And
something
> is a "[category] id" if it's an int that somehow refers to a category. There
is
> a different between the "ids" we store in ContentSuggestionsCategory::id_ and
> the 'remote_id' passed in here -- namely a numerical difference of 10000. So
the
> parameter is not the category itself, it's something that refers to it.
> Maybe the method should be named "FromRemoteCategoryID" and the one below
should
> be named "FromCategoryID"?

just read-up on the implementation (didn't have a careful look before) and tend
to agree that simply documenting that the insertion order will be reflected in
the sort order seems to be good enough for now. Especially as it naturally
handles the case of changing orders.

https://codereview.chromium.org/2187233002/diff/40001/components/ntp_snippets...
File components/ntp_snippets/content_suggestions_category.h (right):

https://codereview.chromium.org/2187233002/diff/40001/components/ntp_snippets...
components/ntp_snippets/content_suggestions_category.h:33: int id() const {
return id_; }
we should document this id. To make clear which properties clients can assume
(hopefully none besides uniquely identifying the category).

https://codereview.chromium.org/2187233002/diff/40001/components/ntp_snippets...
File components/ntp_snippets/content_suggestions_category_factory.h (right):

https://codereview.chromium.org/2187233002/diff/40001/components/ntp_snippets...
components/ntp_snippets/content_suggestions_category_factory.h:30: //
|REMOTE_CATEGORIES_OFFSET + remote_id|. It may, but does not have to match
IMO, the factory should not document this detail (about how the category id is
computed for remote categories) about how we represent the categories. That's a
detail that should be only known by the category and factory implementation.

https://codereview.chromium.org/2187233002/diff/40001/components/ntp_snippets...
components/ntp_snippets/content_suggestions_category_factory.h:51:
std::vector<ContentSuggestionsCategory> categories_;
should we rename this to ordered_categories_? Just a thought, not feeling
strongly.

Bernhard Bauer

lgtm https://codereview.chromium.org/2187233002/diff/1/components/ntp_snippets/content_suggestions_category_factory.cc File components/ntp_snippets/content_suggestions_category_factory.cc (right): https://codereview.chromium.org/2187233002/diff/1/components/ntp_snippets/content_suggestions_category_factory.cc#newcode22 components/ntp_snippets/content_suggestions_category_factory.cc:22: DCHECK_GE(static_cast<int>(known_category), 0); On 2016/07/28 15:15:25, Philipp Keck wrote: ...

4 years, 4 months ago (2016-07-28 15:39:27 UTC) #19

lgtm

https://codereview.chromium.org/2187233002/diff/1/components/ntp_snippets/con...
File components/ntp_snippets/content_suggestions_category_factory.cc (right):

https://codereview.chromium.org/2187233002/diff/1/components/ntp_snippets/con...
components/ntp_snippets/content_suggestions_category_factory.cc:22:
DCHECK_GE(static_cast<int>(known_category), 0);
On 2016/07/28 15:15:25, Philipp Keck wrote:
> On 2016/07/28 15:03:45, tschumann wrote:
> > On 2016/07/28 14:31:16, Marc Treib wrote:
> > > On 2016/07/28 13:50:54, Philipp Keck wrote:
> > > > On 2016/07/28 11:41:45, Marc Treib wrote:
> > > > > What is this for? We're getting a KnownSuggestionsCategories, so we
can
> > > assume
> > > > > it has a valid value.
> > > > 
> > > > This is here because C++ allows you to do things like
> > > > "static_cast<KnownSuggestionsCategories>(999)" and it even delivers 999
if
> > you
> > > > cast it back to int. A provider might read the value from a cache/DB or
> sth
> > > like
> > > > that and not even do the cast explicitly.

Not to add too much bikeshedding, but if you are actually concerned about a
corrupted DB or something, you should do something stronger than a DCHECK that
will be compiled out in an official build. 

> > > For some value of "allows" - it might work in practice, but I'm pretty
sure
> > > that's undefined behavior.
> > > Anyway, the DCHECK below would catch that case, so I'd remove this one.
> > 
> > nit: I believe it's unspecified ;-) But yes, we'd be in troubles anyways if
> that
> > happens. Let's remove that check.
> 
> Acknowledged.

https://codereview.chromium.org/2187233002/diff/60001/components/ntp_snippets...
File components/ntp_snippets/content_suggestions_category.h (right):

https://codereview.chromium.org/2187233002/diff/60001/components/ntp_snippets...
components/ntp_snippets/content_suggestions_category.h:25:
REMOTE_CATEGORIES_OFFSET = 10000,
Stupid question: If the local categories are fixed anyway, why do we need the
gap between local and remote categories? Are we going to store the values on
disk?

https://codereview.chromium.org/2187233002/diff/60001/components/ntp_snippets...
components/ntp_snippets/content_suggestions_category.h:42: int id_;
Nit: It's quite common to add a comment that copying and assignment are allowed,
just to make sure that no one will try and "fix" the lack of a
DISALLOW_COPY_AND_ASSIGN :)

https://codereview.chromium.org/2187233002/diff/60001/components/ntp_snippets...
File components/ntp_snippets/content_suggestions_category_factory.cc (right):

https://codereview.chromium.org/2187233002/diff/60001/components/ntp_snippets...
components/ntp_snippets/content_suggestions_category_factory.cc:76: if (it !=
categories_.end()) {
Nit: no braces for single-line bodies.

Philipp Keck

https://codereview.chromium.org/2187233002/diff/1/components/ntp_snippets/content_suggestions_category_factory.cc File components/ntp_snippets/content_suggestions_category_factory.cc (right): https://codereview.chromium.org/2187233002/diff/1/components/ntp_snippets/content_suggestions_category_factory.cc#newcode22 components/ntp_snippets/content_suggestions_category_factory.cc:22: DCHECK_GE(static_cast<int>(known_category), 0); On 2016/07/28 15:39:26, Bernhard Bauer wrote: > ...

4 years, 4 months ago (2016-07-28 16:59:59 UTC) #20

https://codereview.chromium.org/2187233002/diff/1/components/ntp_snippets/con...
File components/ntp_snippets/content_suggestions_category_factory.cc (right):

https://codereview.chromium.org/2187233002/diff/1/components/ntp_snippets/con...
components/ntp_snippets/content_suggestions_category_factory.cc:22:
DCHECK_GE(static_cast<int>(known_category), 0);
On 2016/07/28 15:39:26, Bernhard Bauer wrote:
> On 2016/07/28 15:15:25, Philipp Keck wrote:
> > On 2016/07/28 15:03:45, tschumann wrote:
> > > On 2016/07/28 14:31:16, Marc Treib wrote:
> > > > On 2016/07/28 13:50:54, Philipp Keck wrote:
> > > > > On 2016/07/28 11:41:45, Marc Treib wrote:
> > > > > > What is this for? We're getting a KnownSuggestionsCategories, so we
> can
> > > > assume
> > > > > > it has a valid value.
> > > > > 
> > > > > This is here because C++ allows you to do things like
> > > > > "static_cast<KnownSuggestionsCategories>(999)" and it even delivers
999
> if
> > > you
> > > > > cast it back to int. A provider might read the value from a cache/DB
or
> > sth
> > > > like
> > > > > that and not even do the cast explicitly.
> 
> Not to add too much bikeshedding, but if you are actually concerned about a
> corrupted DB or something, you should do something stronger than a DCHECK that
> will be compiled out in an official build. 
> 
> > > > For some value of "allows" - it might work in practice, but I'm pretty
> sure
> > > > that's undefined behavior.
> > > > Anyway, the DCHECK below would catch that case, so I'd remove this one.
> > > 
> > > nit: I believe it's unspecified ;-) But yes, we'd be in troubles anyways
if
> > that
> > > happens. Let's remove that check.
> > 
> > Acknowledged.
> 

That's also true.

https://codereview.chromium.org/2187233002/diff/1/components/ntp_snippets/con...
File components/ntp_snippets/content_suggestions_category_factory.h (right):

https://codereview.chromium.org/2187233002/diff/1/components/ntp_snippets/con...
components/ntp_snippets/content_suggestions_category_factory.h:26:
ContentSuggestionsCategory FromRemoteCategory(int remote_id);
On 2016/07/28 15:36:10, tschumann wrote:
> On 2016/07/28 15:15:25, Philipp Keck wrote:
> > On 2016/07/28 15:03:45, tschumann wrote:
> > > On 2016/07/28 13:50:54, Philipp Keck wrote:
> > > > On 2016/07/28 12:50:14, tschumann wrote:
> > > > > i think passing in the index from the server response would be good,
so
> > that
> > > > we
> > > > > can keep the order we received from the server.
> > > > 
> > > > We can add that once we need it. Currently, the server-provider would
just
> > add
> > > > them in the order it received them. And the order cannot change (at
least
> > > that's
> > > > what we agreed on).
> > > > Passing the index in here is only useful if you also pass in the
provider,
> > to
> > > > keep track of per-provider indices. And then you'd need some ordering
> among
> > > > providers. Currently, the ContentSuggestionsCategory and -Factory are
> nicely
> > > > decoupled from all the other stuff that's going on, but receiving the
> > provider
> > > > instance here would lead to circular dependencies. We could use a
> > provider_id
> > > > for that, but we don't have that yet. Neither do we need the indices
yet,
> so
> > > > let's just worry about that once we need it. It would only be one
provider
> > > that
> > > > needs to be adjusted then.
> > > 
> > > The problem i'm seeing is: Keeping the order as we received it from the
> > service
> > > is a requirement and it only works due to undocumented implementation
> details.
> > > If for some reason we change how the provider processes the server
response,
> > > we'd violate the order requirement.
> > > I'd rather hard-code that we only support one remote-provider. If you pass
> the
> > > second instance, we could have the following semantics:
> > > -- the first time we see a remote_id, we use the given index as a
secondary
> > > sort-key (i.e. as a tie breaker when comparing remote categories.
> > > -- if another remote_id already used that secondary sort-key, we'd assign
a
> > new
> > > one (the next available secondary key) essentially sorting at the end.
This
> is
> > > needed in case we get updates in a different order.
> > > 
> > > another nit: should we rename 'remote_id' to 'remote_category'? (would be
in
> > > line with 'known_category'.
> > 
> > We could document the way it works in this file. It's also in the design
doc.
> > There currently is no sorting at all, categories are simply kept in the
order
> in
> > which they were added. So it wouldn't be an implementation detail of the
> > provider, but it's a requirement that the provider has to fulfill when using
> > this interface: If the provider wants its categories to appear in the right
> > order, it needs to sort them the way it needs them before calling
> > FromRemoteCategory for each of them.
> > 
> > I don't see where and how we could hard-code the one remote-provider (or do
we
> > just trust that only one provider ever uses this method?) and where we would
> > store that secondary sort-key. We would need a separate data structure for
> that,
> > or we would have to pack it into the Category itself. I could create a
> follow-up
> > CL which attempts that and we can discuss it there? But since the current
> > approach works, maybe documenting it is sufficient?
> > 
> > I'd call something a "category" if it unambiguously is a category. And
> something
> > is a "[category] id" if it's an int that somehow refers to a category. There
> is
> > a different between the "ids" we store in ContentSuggestionsCategory::id_
and
> > the 'remote_id' passed in here -- namely a numerical difference of 10000. So
> the
> > parameter is not the category itself, it's something that refers to it.
> > Maybe the method should be named "FromRemoteCategoryID" and the one below
> should
> > be named "FromCategoryID"?
> 
> just read-up on the implementation (didn't have a careful look before) and
tend
> to agree that simply documenting that the insertion order will be reflected in
> the sort order seems to be good enough for now. Especially as it naturally
> handles the case of changing orders.

Acknowledged.

https://codereview.chromium.org/2187233002/diff/1/components/ntp_snippets/con...
components/ntp_snippets/content_suggestions_category_factory.h:26:
ContentSuggestionsCategory FromRemoteCategory(int remote_id);
On 2016/07/28 15:36:10, tschumann wrote:
> On 2016/07/28 15:15:25, Philipp Keck wrote:
> > On 2016/07/28 15:03:45, tschumann wrote:
> > > On 2016/07/28 13:50:54, Philipp Keck wrote:
> > > > On 2016/07/28 12:50:14, tschumann wrote:
> > > > > i think passing in the index from the server response would be good,
so
> > that
> > > > we
> > > > > can keep the order we received from the server.
> > > > 
> > > > We can add that once we need it. Currently, the server-provider would
just
> > add
> > > > them in the order it received them. And the order cannot change (at
least
> > > that's
> > > > what we agreed on).
> > > > Passing the index in here is only useful if you also pass in the
provider,
> > to
> > > > keep track of per-provider indices. And then you'd need some ordering
> among
> > > > providers. Currently, the ContentSuggestionsCategory and -Factory are
> nicely
> > > > decoupled from all the other stuff that's going on, but receiving the
> > provider
> > > > instance here would lead to circular dependencies. We could use a
> > provider_id
> > > > for that, but we don't have that yet. Neither do we need the indices
yet,
> so
> > > > let's just worry about that once we need it. It would only be one
provider
> > > that
> > > > needs to be adjusted then.
> > > 
> > > The problem i'm seeing is: Keeping the order as we received it from the
> > service
> > > is a requirement and it only works due to undocumented implementation
> details.
> > > If for some reason we change how the provider processes the server
response,
> > > we'd violate the order requirement.
> > > I'd rather hard-code that we only support one remote-provider. If you pass
> the
> > > second instance, we could have the following semantics:
> > > -- the first time we see a remote_id, we use the given index as a
secondary
> > > sort-key (i.e. as a tie breaker when comparing remote categories.
> > > -- if another remote_id already used that secondary sort-key, we'd assign
a
> > new
> > > one (the next available secondary key) essentially sorting at the end.
This
> is
> > > needed in case we get updates in a different order.
> > > 
> > > another nit: should we rename 'remote_id' to 'remote_category'? (would be
in
> > > line with 'known_category'.
> > 
> > We could document the way it works in this file. It's also in the design
doc.
> > There currently is no sorting at all, categories are simply kept in the
order
> in
> > which they were added. So it wouldn't be an implementation detail of the
> > provider, but it's a requirement that the provider has to fulfill when using
> > this interface: If the provider wants its categories to appear in the right
> > order, it needs to sort them the way it needs them before calling
> > FromRemoteCategory for each of them.
> > 
> > I don't see where and how we could hard-code the one remote-provider (or do
we
> > just trust that only one provider ever uses this method?) and where we would
> > store that secondary sort-key. We would need a separate data structure for
> that,
> > or we would have to pack it into the Category itself. I could create a
> follow-up
> > CL which attempts that and we can discuss it there? But since the current
> > approach works, maybe documenting it is sufficient?
> > 
> > I'd call something a "category" if it unambiguously is a category. And
> something
> > is a "[category] id" if it's an int that somehow refers to a category. There
> is
> > a different between the "ids" we store in ContentSuggestionsCategory::id_
and
> > the 'remote_id' passed in here -- namely a numerical difference of 10000. So
> the
> > parameter is not the category itself, it's something that refers to it.
> > Maybe the method should be named "FromRemoteCategoryID" and the one below
> should
> > be named "FromCategoryID"?
> 
> just read-up on the implementation (didn't have a careful look before) and
tend
> to agree that simply documenting that the insertion order will be reflected in
> the sort order seems to be good enough for now. Especially as it naturally
> handles the case of changing orders.

Ok, I renamed it.

https://codereview.chromium.org/2187233002/diff/40001/components/ntp_snippets...
File components/ntp_snippets/content_suggestions_category.h (right):

https://codereview.chromium.org/2187233002/diff/40001/components/ntp_snippets...
components/ntp_snippets/content_suggestions_category.h:33: int id() const {
return id_; }
On 2016/07/28 15:36:10, tschumann wrote:
> we should document this id. To make clear which properties clients can assume
> (hopefully none besides uniquely identifying the category).

Done.

https://codereview.chromium.org/2187233002/diff/40001/components/ntp_snippets...
File components/ntp_snippets/content_suggestions_category_factory.h (right):

https://codereview.chromium.org/2187233002/diff/40001/components/ntp_snippets...
components/ntp_snippets/content_suggestions_category_factory.h:30: //
|REMOTE_CATEGORIES_OFFSET + remote_id|. It may, but does not have to match
On 2016/07/28 15:36:10, tschumann wrote:
> IMO, the factory should not document this detail (about how the category id is
> computed for remote categories) about how we represent the categories. That's
a
> detail that should be only known by the category and factory implementation.

Yeah, this might be the wrong place to document this. It's not totally an
implementation detail though because the same relation is hard-coded in
KnownSuggestionsCategories for at least some of those categories.
I removed the comment here, it should become clear there anyway and it's only
relevant there (to keep in sync with the enum on the server).

https://codereview.chromium.org/2187233002/diff/40001/components/ntp_snippets...
components/ntp_snippets/content_suggestions_category_factory.h:51:
std::vector<ContentSuggestionsCategory> categories_;
On 2016/07/28 15:36:10, tschumann wrote:
> should we rename this to ordered_categories_? Just a thought, not feeling
> strongly.

Done.

https://codereview.chromium.org/2187233002/diff/60001/components/ntp_snippets...
File components/ntp_snippets/content_suggestions_category.h (right):

https://codereview.chromium.org/2187233002/diff/60001/components/ntp_snippets...
components/ntp_snippets/content_suggestions_category.h:25:
REMOTE_CATEGORIES_OFFSET = 10000,
On 2016/07/28 15:39:27, Bernhard Bauer wrote:
> Stupid question: If the local categories are fixed anyway, why do we need the
> gap between local and remote categories? Are we going to store the values on
> disk?

It's because they're also hard-coded on the server (as ARTICLES = 1, ...) and
they need to match the numbers here, regardless of what Chromium version it is
(and we may have added other local categories in the meantime, which would shift
the values).

https://codereview.chromium.org/2187233002/diff/60001/components/ntp_snippets...
components/ntp_snippets/content_suggestions_category.h:42: int id_;
On 2016/07/28 15:39:26, Bernhard Bauer wrote:
> Nit: It's quite common to add a comment that copying and assignment are
allowed,
> just to make sure that no one will try and "fix" the lack of a
> DISALLOW_COPY_AND_ASSIGN :)

I added it, not sure about the wording and location of the comment.

https://codereview.chromium.org/2187233002/diff/60001/components/ntp_snippets...
File components/ntp_snippets/content_suggestions_category_factory.cc (right):

https://codereview.chromium.org/2187233002/diff/60001/components/ntp_snippets...
components/ntp_snippets/content_suggestions_category_factory.cc:76: if (it !=
categories_.end()) {
On 2016/07/28 15:39:27, Bernhard Bauer wrote:
> Nit: no braces for single-line bodies.

Done. The style guide still allows them "if you like them" -- but since you
don't ... ;-)

Philipp Keck

The CQ bit was checked by pke@google.com to run a CQ dry run

4 years, 4 months ago (2016-07-28 17:00:18 UTC) #21

commit-bot: I haz the power

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

4 years, 4 months ago (2016-07-28 17:00:42 UTC) #22

tschumann

lgtm didn't look close at the implementation (trusting Marc :-)) but interface and direction looks ...

4 years, 4 months ago (2016-07-28 17:05:16 UTC) #23

commit-bot: I haz the power

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

4 years, 4 months ago (2016-07-28 17:07:42 UTC) #24

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: ios-simulator-gn on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn/builds/43257)

4 years, 4 months ago (2016-07-28 17:07:43 UTC) #25

Marc Treib

https://codereview.chromium.org/2187233002/diff/60001/components/ntp_snippets/content_suggestions_category.h File components/ntp_snippets/content_suggestions_category.h (right): https://codereview.chromium.org/2187233002/diff/60001/components/ntp_snippets/content_suggestions_category.h#newcode25 components/ntp_snippets/content_suggestions_category.h:25: REMOTE_CATEGORIES_OFFSET = 10000, On 2016/07/28 16:59:58, Philipp Keck wrote: ...

4 years, 4 months ago (2016-07-29 08:20:35 UTC) #26

Philipp Keck

https://codereview.chromium.org/2187233002/diff/60001/components/ntp_snippets/content_suggestions_category.h File components/ntp_snippets/content_suggestions_category.h (right): https://codereview.chromium.org/2187233002/diff/60001/components/ntp_snippets/content_suggestions_category.h#newcode25 components/ntp_snippets/content_suggestions_category.h:25: REMOTE_CATEGORIES_OFFSET = 10000, On 2016/07/29 08:20:35, Marc Treib wrote: ...

4 years, 4 months ago (2016-07-29 08:24:28 UTC) #27

Philipp Keck

The build previously failed because of the IOS factory. That is now removed (https://codereview.chromium.org/2194793004/), so ...

4 years, 4 months ago (2016-08-01 10:41:44 UTC) #28

Philipp Keck

The patchset sent to the CQ was uploaded after l-g-t-m from treib@chromium.org, bauerb@chromium.org, tschumann@chromium.org Link ...

4 years, 4 months ago (2016-08-01 10:42:13 UTC) #30

commit-bot: I haz the power

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

4 years, 4 months ago (2016-08-01 10:42:21 UTC) #31

commit-bot: I haz the power

Description was changed from ========== Add ContentSuggestionsCategoryFactory; Store categories as ints Replace the ContentSuggestionsCategory enum ...

4 years, 4 months ago (2016-08-01 11:37:09 UTC) #33

commit-bot: I haz the power

4 years, 4 months ago (2016-08-01 11:37:11 UTC) #34

Message was sent while issue was closed.

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

Issue 2187233002: Add ContentSuggestionsCategoryFactory; Store categories as ints (Closed)

Description

Patch Set 1 #

Patch Set 2 : Marc's comments #

Patch Set 3 : Marc's new comments #

Patch Set 4 : Document remote categories order #

Patch Set 5 : Tim's and Bernhard's comments #

Patch Set 6 : Rebase #

Messages