Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2059203002:
Add ContentSuggestion, ContentSuggestionCategory and ContentSuggestionProviderType (Closed)
|
Created:
4 years, 6 months ago by Philipp Keck Modified:
4 years, 5 months ago Reviewers:
Marc Treib CC:
chromium-reviews, zine-eng+reviews_google.com Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd ContentSuggestion, ContentSuggestionCategory and ContentSuggestionProviderType
Add new classes to be used by the upcoming ContentSuggestionService.
These new classes are added in a way that does not break the existing
NTPSnippets code.
BUG=619560
Committed: https://crrev.com/bf8042c0dfb265e5c39d1a7e69fe0aa7625867d3
Cr-Commit-Position: refs/heads/master@{#402164}
Patch Set 1 #
Total comments: 31
Patch Set 2 : Fixed comments and copyright headers in NTPSnippet, NTPSnippetsService and new Snippet classes, rem… #
Total comments: 15
Patch Set 3 : Minor changes to comments #Patch Set 4 : Include Snippet#url in constructor, remove setters for fields passed in through constructor #
Total comments: 2
Patch Set 5 : Added unused comment to snippet.h #Patch Set 6 : Remove Offline Pages from SnippetCategory and SnippetProviderType #
Total comments: 2
Patch Set 7 : Remove unnecessary constructor comment from snippet #Patch Set 8 : Rename Snippet (not NTPSnippet) to ContentSuggestion #
Total comments: 4
Patch Set 9 : Removed ContentSuggestion::PtrVector, Corrected include guard #Patch Set 10 : Renamed ContentSuggestionProviderType to ContentSuggestionsProviderType #
Total comments: 4
Patch Set 11 : Fixed includes #
Messages
Total messages: 31 (6 generated)
treib@chromium.org changed reviewers: + treib@chromium.org
I filed a bug for you: crbug.com/619560, please reference that in the CL description (like, BUG=619560). https://codereview.chromium.org/2059203002/diff/1/components/ntp_snippets/ntp... File components/ntp_snippets/ntp_snippet.h (right): https://codereview.chromium.org/2059203002/diff/1/components/ntp_snippets/ntp... components/ntp_snippets/ntp_snippet.h:36: // TODO IMPORTANT NOTE: This class will move to a sub-namespace and be renamed TODOs should always have someone assigned, as in: // TODO(pke): ... (this exact format, for easy greppability etc) I'd split this into two parts, the "IMPORTANT NOTE" (for other devs), and then a TODO for yourself to actually do the rename. However: Right now, this comment references a class that doesn't exist yet ("SnippetService"). That's the kind of inconsistency we'd like to avoid. So, two options: - Add the new SnippetsService class in this CL too, or - remove this comment again, and add it back in the CL that actually adds the SnippetsService. https://codereview.chromium.org/2059203002/diff/1/components/ntp_snippets/ntp... components/ntp_snippets/ntp_snippet.h:136: // TODO (pke) Instead of converting, ArticleSnippet might inherit Snippet? Probably not :) But anyway, this is the kind of thing that's better served by a personal todo list, rather than a comment in the code. https://codereview.chromium.org/2059203002/diff/1/components/ntp_snippets/ntp... File components/ntp_snippets/ntp_snippets_service.cc (right): https://codereview.chromium.org/2059203002/diff/1/components/ntp_snippets/ntp... components/ntp_snippets/ntp_snippets_service.cc:41: // "SnippetService". Not necessary here, the comment in the header is enough. https://codereview.chromium.org/2059203002/diff/1/components/ntp_snippets/ntp... File components/ntp_snippets/ntp_snippets_service.h (right): https://codereview.chromium.org/2059203002/diff/1/components/ntp_snippets/ntp... components/ntp_snippets/ntp_snippets_service.h:68: // "SnippetService". Same thing here. https://codereview.chromium.org/2059203002/diff/1/components/ntp_snippets/sni... File components/ntp_snippets/snippet.h (right): https://codereview.chromium.org/2059203002/diff/1/components/ntp_snippets/sni... components/ntp_snippets/snippet.h:8: #include <components/ntp_snippets/snippet_provider_type.h> Remove this line https://codereview.chromium.org/2059203002/diff/1/components/ntp_snippets/sni... components/ntp_snippets/snippet.h:67: // directly. Hm, does the UI need to know about the image URL? IIRC, it just calls a "FetchImage(snippet_id)" method? https://codereview.chromium.org/2059203002/diff/1/components/ntp_snippets/sni... components/ntp_snippets/snippet.h:73: // When the page pointed by this text_extract was published. Er, what? How is this related to the text extract? https://codereview.chromium.org/2059203002/diff/1/components/ntp_snippets/sni... components/ntp_snippets/snippet.h:87: // IMPORTANT: The score may simply be 0 for text_extracts from providers which Also here: Why text_extract? https://codereview.chromium.org/2059203002/diff/1/components/ntp_snippets/sni... File components/ntp_snippets/snippet_category.h (right): https://codereview.chromium.org/2059203002/diff/1/components/ntp_snippets/sni... components/ntp_snippets/snippet_category.h:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. :) https://codereview.chromium.org/2059203002/diff/1/components/ntp_snippets/sni... components/ntp_snippets/snippet_category.h:14: enum class SnippetCategory : int { ARTICLE, OFFLINE_PAGE, READING_LIST }; Please remove the categories that don't exist yet. https://codereview.chromium.org/2059203002/diff/1/components/ntp_snippets/sni... File components/ntp_snippets/snippet_provider_type.h (right): https://codereview.chromium.org/2059203002/diff/1/components/ntp_snippets/sni... components/ntp_snippets/snippet_provider_type.h:1: // Copyright (c) 2016 The Chromium Authors. All rights reserved. Not that it matters much, but the current version of the copyright header can be found here: http://dev.chromium.org/developers/coding-style (In particular: no "(c)") https://codereview.chromium.org/2059203002/diff/1/components/ntp_snippets/sni... components/ntp_snippets/snippet_provider_type.h:15: enum class SnippetProviderType : int { ARTICLES, OFFLINE_PAGES, READING_LIST }; Also here, please remove the ones that don't exist yet.
Description was changed from ========== Add Snippet, SnippetCategory and SnippetProviderType; not yet linked to existing NTPSnippets code BUG= ========== to ========== Add Snippet, SnippetCategory and SnippetProviderType; not yet linked to existing NTPSnippets code BUG=619560 ==========
https://codereview.chromium.org/2059203002/diff/1/components/ntp_snippets/ntp... File components/ntp_snippets/ntp_snippet.h (right): https://codereview.chromium.org/2059203002/diff/1/components/ntp_snippets/ntp... components/ntp_snippets/ntp_snippet.h:36: // TODO IMPORTANT NOTE: This class will move to a sub-namespace and be renamed On 2016/06/13 13:08:17, Marc Treib wrote: > TODOs should always have someone assigned, as in: > // TODO(pke): ... > (this exact format, for easy greppability etc) > > I'd split this into two parts, the "IMPORTANT NOTE" (for other devs), and then a > TODO for yourself to actually do the rename. > > However: Right now, this comment references a class that doesn't exist yet > ("SnippetService"). That's the kind of inconsistency we'd like to avoid. So, two > options: > - Add the new SnippetsService class in this CL too, or > - remove this comment again, and add it back in the CL that actually adds the > SnippetsService. Done. https://codereview.chromium.org/2059203002/diff/1/components/ntp_snippets/ntp... components/ntp_snippets/ntp_snippet.h:36: // TODO IMPORTANT NOTE: This class will move to a sub-namespace and be renamed On 2016/06/13 13:08:17, Marc Treib wrote: > TODOs should always have someone assigned, as in: > // TODO(pke): ... > (this exact format, for easy greppability etc) > > I'd split this into two parts, the "IMPORTANT NOTE" (for other devs), and then a > TODO for yourself to actually do the rename. > > However: Right now, this comment references a class that doesn't exist yet > ("SnippetService"). That's the kind of inconsistency we'd like to avoid. So, two > options: > - Add the new SnippetsService class in this CL too, or > - remove this comment again, and add it back in the CL that actually adds the > SnippetsService. Ok. I don't need the TODO comment for myself because that's the next thing I'm doing anyway, so it's on my private todo list. I just added it because many IDEs will then highlight the line, but in that case I'll just remove the TODO marker. I will remove those comments and re-add them when necessary. https://codereview.chromium.org/2059203002/diff/1/components/ntp_snippets/ntp... components/ntp_snippets/ntp_snippet.h:136: // TODO (pke) Instead of converting, ArticleSnippet might inherit Snippet? On 2016/06/13 13:08:17, Marc Treib wrote: > Probably not :) But anyway, this is the kind of thing that's better served by a > personal todo list, rather than a comment in the code. Done. https://codereview.chromium.org/2059203002/diff/1/components/ntp_snippets/ntp... File components/ntp_snippets/ntp_snippets_service.cc (right): https://codereview.chromium.org/2059203002/diff/1/components/ntp_snippets/ntp... components/ntp_snippets/ntp_snippets_service.cc:41: // "SnippetService". On 2016/06/13 13:08:17, Marc Treib wrote: > Not necessary here, the comment in the header is enough. Done. https://codereview.chromium.org/2059203002/diff/1/components/ntp_snippets/ntp... File components/ntp_snippets/ntp_snippets_service.h (right): https://codereview.chromium.org/2059203002/diff/1/components/ntp_snippets/ntp... components/ntp_snippets/ntp_snippets_service.h:68: // "SnippetService". On 2016/06/13 13:08:17, Marc Treib wrote: > Same thing here. Done. https://codereview.chromium.org/2059203002/diff/1/components/ntp_snippets/sni... File components/ntp_snippets/snippet.h (right): https://codereview.chromium.org/2059203002/diff/1/components/ntp_snippets/sni... components/ntp_snippets/snippet.h:8: #include <components/ntp_snippets/snippet_provider_type.h> On 2016/06/13 13:08:17, Marc Treib wrote: > Remove this line Done. https://codereview.chromium.org/2059203002/diff/1/components/ntp_snippets/sni... components/ntp_snippets/snippet.h:67: // directly. On 2016/06/13 13:08:17, Marc Treib wrote: > Hm, does the UI need to know about the image URL? IIRC, it just calls a > "FetchImage(snippet_id)" method? That's right. If we leave it up to the providers to provide the images, we can remove this field. Then I should also remove it from SnippetsArticle.java later when I change the bridge to fetch snippets from the new service. https://codereview.chromium.org/2059203002/diff/1/components/ntp_snippets/sni... components/ntp_snippets/snippet.h:73: // When the page pointed by this text_extract was published. On 2016/06/13 13:08:17, Marc Treib wrote: > Er, what? How is this related to the text extract? Done. https://codereview.chromium.org/2059203002/diff/1/components/ntp_snippets/sni... components/ntp_snippets/snippet.h:87: // IMPORTANT: The score may simply be 0 for text_extracts from providers which On 2016/06/13 13:08:17, Marc Treib wrote: > Also here: Why text_extract? Done. https://codereview.chromium.org/2059203002/diff/1/components/ntp_snippets/sni... File components/ntp_snippets/snippet_category.h (right): https://codereview.chromium.org/2059203002/diff/1/components/ntp_snippets/sni... components/ntp_snippets/snippet_category.h:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. On 2016/06/13 13:08:17, Marc Treib wrote: > :) Done. https://codereview.chromium.org/2059203002/diff/1/components/ntp_snippets/sni... components/ntp_snippets/snippet_category.h:14: enum class SnippetCategory : int { ARTICLE, OFFLINE_PAGE, READING_LIST }; On 2016/06/13 13:08:17, Marc Treib wrote: > Please remove the categories that don't exist yet. Done. https://codereview.chromium.org/2059203002/diff/1/components/ntp_snippets/sni... File components/ntp_snippets/snippet_provider_type.h (right): https://codereview.chromium.org/2059203002/diff/1/components/ntp_snippets/sni... components/ntp_snippets/snippet_provider_type.h:1: // Copyright (c) 2016 The Chromium Authors. All rights reserved. On 2016/06/13 13:08:17, Marc Treib wrote: > Not that it matters much, but the current version of the copyright header can be > found here: http://dev.chromium.org/developers/coding-style > > (In particular: no "(c)") Done. https://codereview.chromium.org/2059203002/diff/1/components/ntp_snippets/sni... components/ntp_snippets/snippet_provider_type.h:15: enum class SnippetProviderType : int { ARTICLES, OFFLINE_PAGES, READING_LIST }; On 2016/06/13 13:08:17, Marc Treib wrote: > Also here, please remove the ones that don't exist yet. Done.
A few more comments; almost there! https://codereview.chromium.org/2059203002/diff/1/components/ntp_snippets/sni... File components/ntp_snippets/snippet.h (right): https://codereview.chromium.org/2059203002/diff/1/components/ntp_snippets/sni... components/ntp_snippets/snippet.h:67: // directly. On 2016/06/13 14:13:14, pke wrote: > On 2016/06/13 13:08:17, Marc Treib wrote: > > Hm, does the UI need to know about the image URL? IIRC, it just calls a > > "FetchImage(snippet_id)" method? > > That's right. If we leave it up to the providers to provide the images, we can > remove this field. Then I should also remove it from SnippetsArticle.java later > when I change the bridge to fetch snippets from the new service. Acknowledged. https://codereview.chromium.org/2059203002/diff/1/components/ntp_snippets/sni... File components/ntp_snippets/snippet_category.h (right): https://codereview.chromium.org/2059203002/diff/1/components/ntp_snippets/sni... components/ntp_snippets/snippet_category.h:14: enum class SnippetCategory : int { ARTICLE, OFFLINE_PAGE, READING_LIST }; On 2016/06/13 14:13:14, pke wrote: > On 2016/06/13 13:08:17, Marc Treib wrote: > > Please remove the categories that don't exist yet. > > Done. OFFLINE_PAGE also doesn't exist yet. https://codereview.chromium.org/2059203002/diff/1/components/ntp_snippets/sni... File components/ntp_snippets/snippet_provider_type.h (right): https://codereview.chromium.org/2059203002/diff/1/components/ntp_snippets/sni... components/ntp_snippets/snippet_provider_type.h:15: enum class SnippetProviderType : int { ARTICLES, OFFLINE_PAGES, READING_LIST }; On 2016/06/13 14:13:14, pke wrote: > On 2016/06/13 13:08:17, Marc Treib wrote: > > Also here, please remove the ones that don't exist yet. > > Done. Also here. https://codereview.chromium.org/2059203002/diff/20001/components/ntp_snippets... File components/ntp_snippets/snippet.h (right): https://codereview.chromium.org/2059203002/diff/20001/components/ntp_snippets... components/ntp_snippets/snippet.h:21: // a snippet from another source. nit: I wouldn't say "source", since that term is too overloaded. Maybe just "which can be e.g. an article or an offline page." ? https://codereview.chromium.org/2059203002/diff/20001/components/ntp_snippets... components/ntp_snippets/snippet.h:29: const SnippetCategory category); Is the idea here to pass in all strictly-required fields? If so, URL should probably be added. In general, we should clearly define which fields we consider mandatory and which are optional. That can happen later though (the old NTPSnippet also doesn't really do that). https://codereview.chromium.org/2059203002/diff/20001/components/ntp_snippets... components/ntp_snippets/snippet.h:43: void set_category(const SnippetCategory category) { category_ = category; } I think this setter isn't required? https://codereview.chromium.org/2059203002/diff/20001/components/ntp_snippets... File components/ntp_snippets/snippet_category.h (right): https://codereview.chromium.org/2059203002/diff/20001/components/ntp_snippets... components/ntp_snippets/snippet_category.h:13: // and how to display the snippet. "where" yes (as in, under which section), but not "how" - all snippets will have the same look. https://codereview.chromium.org/2059203002/diff/20001/components/ntp_snippets... File components/ntp_snippets/snippet_provider_type.h (right): https://codereview.chromium.org/2059203002/diff/20001/components/ntp_snippets... components/ntp_snippets/snippet_provider_type.h:13: // to identify the source of a snippet and to direct calls from the UI like nit: extra space between "UI" and "like"
https://codereview.chromium.org/2059203002/diff/20001/components/ntp_snippets... File components/ntp_snippets/snippet.h (right): https://codereview.chromium.org/2059203002/diff/20001/components/ntp_snippets... components/ntp_snippets/snippet.h:21: // a snippet from another source. On 2016/06/13 15:11:10, Marc Treib wrote: > nit: I wouldn't say "source", since that term is too overloaded. Maybe just > "which can be e.g. an article or an offline page." ? Done. https://codereview.chromium.org/2059203002/diff/20001/components/ntp_snippets... components/ntp_snippets/snippet.h:29: const SnippetCategory category); On 2016/06/13 15:11:10, Marc Treib wrote: > Is the idea here to pass in all strictly-required fields? If so, URL should > probably be added. > In general, we should clearly define which fields we consider mandatory and > which are optional. That can happen later though (the old NTPSnippet also > doesn't really do that). If the two enum values are not passed in, they need to be initialized with something else (like 0). Would that be okay? Otherwise, I see no other solution but to pass them in here. Then adding all required fields to the constructor seems like a good idea. My intention was: Constructor gets everything for the object to be able to exist (though required fields maybe should be part of that) and setters exist for all fields that are not necessarily constant. In Java, I'd make all fields final, no setters, and overloaded constructors or factory methods to account for optional fields. But C++ style seems to be different. https://codereview.chromium.org/2059203002/diff/20001/components/ntp_snippets... components/ntp_snippets/snippet.h:43: void set_category(const SnippetCategory category) { category_ = category; } On 2016/06/13 15:11:10, Marc Treib wrote: > I think this setter isn't required? Depends on decision above. https://codereview.chromium.org/2059203002/diff/20001/components/ntp_snippets... File components/ntp_snippets/snippet_category.h (right): https://codereview.chromium.org/2059203002/diff/20001/components/ntp_snippets... components/ntp_snippets/snippet_category.h:13: // and how to display the snippet. On 2016/06/13 15:11:10, Marc Treib wrote: > "where" yes (as in, under which section), but not "how" - all snippets will have > the same look. Done. https://codereview.chromium.org/2059203002/diff/20001/components/ntp_snippets... File components/ntp_snippets/snippet_provider_type.h (right): https://codereview.chromium.org/2059203002/diff/20001/components/ntp_snippets... components/ntp_snippets/snippet_provider_type.h:13: // to identify the source of a snippet and to direct calls from the UI like On 2016/06/13 15:11:10, Marc Treib wrote: > nit: extra space between "UI" and "like" Done.
https://codereview.chromium.org/2059203002/diff/20001/components/ntp_snippets... File components/ntp_snippets/snippet.h (right): https://codereview.chromium.org/2059203002/diff/20001/components/ntp_snippets... components/ntp_snippets/snippet.h:29: const SnippetCategory category); On 2016/06/13 15:27:50, pke wrote: > On 2016/06/13 15:11:10, Marc Treib wrote: > > Is the idea here to pass in all strictly-required fields? If so, URL should > > probably be added. > > In general, we should clearly define which fields we consider mandatory and > > which are optional. That can happen later though (the old NTPSnippet also > > doesn't really do that). > > If the two enum values are not passed in, they need to be initialized with > something else (like 0). Would that be okay? > Otherwise, I see no other solution but to pass them in here. Then adding all > required fields to the constructor seems like a good idea. > > My intention was: Constructor gets everything for the object to be able to exist > (though required fields maybe should be part of that) and setters exist for all > fields that are not necessarily constant. > > In Java, I'd make all fields final, no setters, and overloaded constructors or > factory methods to account for optional fields. But C++ style seems to be > different. No, initializing to 0 doesn't seem like a good idea. But I wasn't complaining about those being passed in, I was saying that maybe more things should be passed to the constructor :) Your intention was exactly right, the question is just what exactly we consider "able to exist". I'd argue that at least the URL is necessary for that.
https://codereview.chromium.org/2059203002/diff/20001/components/ntp_snippets... File components/ntp_snippets/snippet.h (right): https://codereview.chromium.org/2059203002/diff/20001/components/ntp_snippets... components/ntp_snippets/snippet.h:29: const SnippetCategory category); On 2016/06/13 15:30:42, Marc Treib wrote: > On 2016/06/13 15:27:50, pke wrote: > > On 2016/06/13 15:11:10, Marc Treib wrote: > > > Is the idea here to pass in all strictly-required fields? If so, URL should > > > probably be added. > > > In general, we should clearly define which fields we consider mandatory and > > > which are optional. That can happen later though (the old NTPSnippet also > > > doesn't really do that). > > > > If the two enum values are not passed in, they need to be initialized with > > something else (like 0). Would that be okay? > > Otherwise, I see no other solution but to pass them in here. Then adding all > > required fields to the constructor seems like a good idea. > > > > My intention was: Constructor gets everything for the object to be able to > exist > > (though required fields maybe should be part of that) and setters exist for > all > > fields that are not necessarily constant. > > > > In Java, I'd make all fields final, no setters, and overloaded constructors or > > factory methods to account for optional fields. But C++ style seems to be > > different. > > No, initializing to 0 doesn't seem like a good idea. > But I wasn't complaining about those being passed in, I was saying that maybe > more things should be passed to the constructor :) > Your intention was exactly right, the question is just what exactly we consider > "able to exist". I'd argue that at least the URL is necessary for that. Maybe also the title, as it doesn't make much sense to display a card on the UI without a title. Unless there might be cards with only text_extract but without title? Seems unlikely, the only cards of this kind that Google Now has, for example, are "read more" cards, and sth. like that probably wouldn't be backed by the Snippet class. amp_url and score are clearly optional and the remaining fields are almost always present, but not "required to be able to display a meaningful card".
https://codereview.chromium.org/2059203002/diff/20001/components/ntp_snippets... File components/ntp_snippets/snippet.h (right): https://codereview.chromium.org/2059203002/diff/20001/components/ntp_snippets... components/ntp_snippets/snippet.h:29: const SnippetCategory category); On 2016/06/13 15:38:56, pke wrote: > On 2016/06/13 15:30:42, Marc Treib wrote: > > On 2016/06/13 15:27:50, pke wrote: > > > On 2016/06/13 15:11:10, Marc Treib wrote: > > > > Is the idea here to pass in all strictly-required fields? If so, URL > should > > > > probably be added. > > > > In general, we should clearly define which fields we consider mandatory > and > > > > which are optional. That can happen later though (the old NTPSnippet also > > > > doesn't really do that). > > > > > > If the two enum values are not passed in, they need to be initialized with > > > something else (like 0). Would that be okay? > > > Otherwise, I see no other solution but to pass them in here. Then adding all > > > required fields to the constructor seems like a good idea. > > > > > > My intention was: Constructor gets everything for the object to be able to > > exist > > > (though required fields maybe should be part of that) and setters exist for > > all > > > fields that are not necessarily constant. > > > > > > In Java, I'd make all fields final, no setters, and overloaded constructors > or > > > factory methods to account for optional fields. But C++ style seems to be > > > different. > > > > No, initializing to 0 doesn't seem like a good idea. > > But I wasn't complaining about those being passed in, I was saying that maybe > > more things should be passed to the constructor :) > > Your intention was exactly right, the question is just what exactly we > consider > > "able to exist". I'd argue that at least the URL is necessary for that. > > Maybe also the title, as it doesn't make much sense to display a card on the UI > without a title. Unless there might be cards with only text_extract but without > title? Seems unlikely, the only cards of this kind that Google Now has, for > example, are "read more" cards, and sth. like that probably wouldn't be backed > by the Snippet class. > > amp_url and score are clearly optional and the remaining fields are almost > always present, but not "required to be able to display a meaningful card". text_extract will definitely be optional (since Offline Pages cards won't have it at first); not sure about title. Maybe it's best to keep the ctor as-is (pass in only the things that don't have any reasonable "empty" state), and maybe add an "is_complete" method, like NTPSnippet has? WDYT?
https://codereview.chromium.org/2059203002/diff/20001/components/ntp_snippets... File components/ntp_snippets/snippet.h (right): https://codereview.chromium.org/2059203002/diff/20001/components/ntp_snippets... components/ntp_snippets/snippet.h:29: const SnippetCategory category); On 2016/06/13 15:54:35, Marc Treib wrote: > On 2016/06/13 15:38:56, pke wrote: > > On 2016/06/13 15:30:42, Marc Treib wrote: > > > On 2016/06/13 15:27:50, pke wrote: > > > > On 2016/06/13 15:11:10, Marc Treib wrote: > > > > > Is the idea here to pass in all strictly-required fields? If so, URL > > should > > > > > probably be added. > > > > > In general, we should clearly define which fields we consider mandatory > > and > > > > > which are optional. That can happen later though (the old NTPSnippet > also > > > > > doesn't really do that). > > > > > > > > If the two enum values are not passed in, they need to be initialized with > > > > something else (like 0). Would that be okay? > > > > Otherwise, I see no other solution but to pass them in here. Then adding > all > > > > required fields to the constructor seems like a good idea. > > > > > > > > My intention was: Constructor gets everything for the object to be able to > > > exist > > > > (though required fields maybe should be part of that) and setters exist > for > > > all > > > > fields that are not necessarily constant. > > > > > > > > In Java, I'd make all fields final, no setters, and overloaded > constructors > > or > > > > factory methods to account for optional fields. But C++ style seems to be > > > > different. > > > > > > No, initializing to 0 doesn't seem like a good idea. > > > But I wasn't complaining about those being passed in, I was saying that > maybe > > > more things should be passed to the constructor :) > > > Your intention was exactly right, the question is just what exactly we > > consider > > > "able to exist". I'd argue that at least the URL is necessary for that. > > > > Maybe also the title, as it doesn't make much sense to display a card on the > UI > > without a title. Unless there might be cards with only text_extract but > without > > title? Seems unlikely, the only cards of this kind that Google Now has, for > > example, are "read more" cards, and sth. like that probably wouldn't be backed > > by the Snippet class. > > > > amp_url and score are clearly optional and the remaining fields are almost > > always present, but not "required to be able to display a meaningful card". > > text_extract will definitely be optional (since Offline Pages cards won't have > it at first); not sure about title. > Maybe it's best to keep the ctor as-is (pass in only the things that don't have > any reasonable "empty" state), and maybe add an "is_complete" method, like > NTPSnippet has? WDYT? I will go by your previous suggestion. The constructor will take ID, provider_type, category and url. Those will be unchangeable (no setters). An additional reason for this is the fact that the mixer service or the UI may need to rely on the URL being unchangeable (just like those other three fields) for properly merging identical content from different providers. https://codereview.chromium.org/2059203002/diff/20001/components/ntp_snippets... components/ntp_snippets/snippet.h:43: void set_category(const SnippetCategory category) { category_ = category; } On 2016/06/13 15:27:50, pke wrote: > On 2016/06/13 15:11:10, Marc Treib wrote: > > I think this setter isn't required? > > Depends on decision above. Done.
https://codereview.chromium.org/2059203002/diff/1/components/ntp_snippets/sni... File components/ntp_snippets/snippet_category.h (right): https://codereview.chromium.org/2059203002/diff/1/components/ntp_snippets/sni... components/ntp_snippets/snippet_category.h:14: enum class SnippetCategory : int { ARTICLE, OFFLINE_PAGE, READING_LIST }; On 2016/06/13 15:11:10, Marc Treib wrote: > On 2016/06/13 14:13:14, pke wrote: > > On 2016/06/13 13:08:17, Marc Treib wrote: > > > Please remove the categories that don't exist yet. > > > > Done. > > OFFLINE_PAGE also doesn't exist yet. This isn't resolved yet; also for the ProviderType. https://codereview.chromium.org/2059203002/diff/60001/components/ntp_snippets... File components/ntp_snippets/snippet.h (right): https://codereview.chromium.org/2059203002/diff/60001/components/ntp_snippets... components/ntp_snippets/snippet.h:22: class Snippet { Could you add a comment here saying that this isn't actually used yet? To prevent any confusion if someone else reads this code before the new service lands.
https://codereview.chromium.org/2059203002/diff/60001/components/ntp_snippets... File components/ntp_snippets/snippet.h (right): https://codereview.chromium.org/2059203002/diff/60001/components/ntp_snippets... components/ntp_snippets/snippet.h:22: class Snippet { On 2016/06/14 09:29:44, Marc Treib wrote: > Could you add a comment here saying that this isn't actually used yet? To > prevent any confusion if someone else reads this code before the new service > lands. Done.
Read the full message you got; not all comments are necessarily on the latest patch set :)
Okay, I found the comments now ... Offline pages are still in the header comment of snippet.h as an example. https://codereview.chromium.org/2059203002/diff/1/components/ntp_snippets/sni... File components/ntp_snippets/snippet_category.h (right): https://codereview.chromium.org/2059203002/diff/1/components/ntp_snippets/sni... components/ntp_snippets/snippet_category.h:14: enum class SnippetCategory : int { ARTICLE, OFFLINE_PAGE, READING_LIST }; On 2016/06/14 09:29:44, Marc Treib wrote: > On 2016/06/13 15:11:10, Marc Treib wrote: > > On 2016/06/13 14:13:14, pke wrote: > > > On 2016/06/13 13:08:17, Marc Treib wrote: > > > > Please remove the categories that don't exist yet. > > > > > > Done. > > > > OFFLINE_PAGE also doesn't exist yet. > > This isn't resolved yet; also for the ProviderType. Done. https://codereview.chromium.org/2059203002/diff/1/components/ntp_snippets/sni... File components/ntp_snippets/snippet_provider_type.h (right): https://codereview.chromium.org/2059203002/diff/1/components/ntp_snippets/sni... components/ntp_snippets/snippet_provider_type.h:15: enum class SnippetProviderType : int { ARTICLES, OFFLINE_PAGES, READING_LIST }; On 2016/06/13 15:11:10, Marc Treib wrote: > On 2016/06/13 14:13:14, pke wrote: > > On 2016/06/13 13:08:17, Marc Treib wrote: > > > Also here, please remove the ones that don't exist yet. > > > > Done. > > Also here. Done.
LGTM with one more nit. (As you've seen in the terms doc, we *might* have to rename this again... ugh. But let's go on with this for now, and then do a renaming later if we really must.) https://codereview.chromium.org/2059203002/diff/100001/components/ntp_snippet... File components/ntp_snippets/snippet.h (right): https://codereview.chromium.org/2059203002/diff/100001/components/ntp_snippet... components/ntp_snippets/snippet.h:27: // Creates a new snippet with the given |id|. nit: Not really correct, since it takes more than an id. Also it's kinda obvious, given that this is the ctor - I'd just remove the comment.
On 2016/06/14 12:57:28, Marc Treib wrote: > LGTM with one more nit. > > (As you've seen in the terms doc, we *might* have to rename this again... ugh. > But let's go on with this for now, and then do a renaming later if we really > must.) > > https://codereview.chromium.org/2059203002/diff/100001/components/ntp_snippet... > File components/ntp_snippets/snippet.h (right): > > https://codereview.chromium.org/2059203002/diff/100001/components/ntp_snippet... > components/ntp_snippets/snippet.h:27: // Creates a new snippet with the given > |id|. > nit: Not really correct, since it takes more than an id. Also it's kinda > obvious, given that this is the ctor - I'd just remove the comment. Or let's speed up the final clarification of terms and not merge this until tomorrow. There is no harm in not merging it because it's all new files, so no matter what other people code in the meanwhile, I won't have rebasing problems.
https://codereview.chromium.org/2059203002/diff/100001/components/ntp_snippet... File components/ntp_snippets/snippet.h (right): https://codereview.chromium.org/2059203002/diff/100001/components/ntp_snippet... components/ntp_snippets/snippet.h:27: // Creates a new snippet with the given |id|. On 2016/06/14 12:57:28, Marc Treib wrote: > nit: Not really correct, since it takes more than an id. Also it's kinda > obvious, given that this is the ctor - I'd just remove the comment. Done.
Description was changed from
==========
Add Snippet, SnippetCategory and SnippetProviderType; not yet linked to existing
NTPSnippets code
BUG=619560
==========
to
==========
Add ContentSuggestion, ContentSuggestionCategory and
ContentSuggestionProviderType
Add new classes to be used by the upcoming ContentSuggestionService.
These new classes are added in a way that does not break the existing
NTPSnippets code.
BUG=619560
==========
On 2016/06/14 13:02:17, Philipp Keck wrote: > https://codereview.chromium.org/2059203002/diff/100001/components/ntp_snippet... > File components/ntp_snippets/snippet.h (right): > > https://codereview.chromium.org/2059203002/diff/100001/components/ntp_snippet... > components/ntp_snippets/snippet.h:27: // Creates a new snippet with the given > |id|. > On 2016/06/14 12:57:28, Marc Treib wrote: > > nit: Not really correct, since it takes more than an id. Also it's kinda > > obvious, given that this is the ctor - I'd just remove the comment. > > Done. PTAL again. I renamed everything in the new code to match the new zine-terms, still without touching the existing code.
LGTM! https://codereview.chromium.org/2059203002/diff/140001/components/ntp_snippet... File components/ntp_snippets/content_suggestion.h (right): https://codereview.chromium.org/2059203002/diff/140001/components/ntp_snippet... components/ntp_snippets/content_suggestion.h:26: using PtrVector = std::vector<std::unique_ptr<ContentSuggestion>>; Hm, we could consider passing around vectors of instances instead of vectors of pointers. Any good reason for using pointers? https://codereview.chromium.org/2059203002/diff/140001/components/ntp_snippet... File components/ntp_snippets/content_suggestion_category.h (right): https://codereview.chromium.org/2059203002/diff/140001/components/ntp_snippet... components/ntp_snippets/content_suggestion_category.h:6: #define COMPONENTS_NTP_SNIPPETS_CONTENT_SUGGESTION_CATEGORY_TYPE_H_ nit: no TYPE
https://codereview.chromium.org/2059203002/diff/140001/components/ntp_snippet... File components/ntp_snippets/content_suggestion.h (right): https://codereview.chromium.org/2059203002/diff/140001/components/ntp_snippet... components/ntp_snippets/content_suggestion.h:26: using PtrVector = std::vector<std::unique_ptr<ContentSuggestion>>; On 2016/06/27 11:35:30, Marc Treib wrote: > Hm, we could consider passing around vectors of instances instead of vectors of > pointers. Any good reason for using pointers? Done. https://codereview.chromium.org/2059203002/diff/140001/components/ntp_snippet... File components/ntp_snippets/content_suggestion_category.h (right): https://codereview.chromium.org/2059203002/diff/140001/components/ntp_snippet... components/ntp_snippets/content_suggestion_category.h:6: #define COMPONENTS_NTP_SNIPPETS_CONTENT_SUGGESTION_CATEGORY_TYPE_H_ On 2016/06/27 11:35:30, Marc Treib wrote: > nit: no TYPE Done.
On 2016/06/27 11:35:31, Marc Treib wrote: > LGTM! > > https://codereview.chromium.org/2059203002/diff/140001/components/ntp_snippet... > File components/ntp_snippets/content_suggestion.h (right): > > https://codereview.chromium.org/2059203002/diff/140001/components/ntp_snippet... > components/ntp_snippets/content_suggestion.h:26: using PtrVector = > std::vector<std::unique_ptr<ContentSuggestion>>; > Hm, we could consider passing around vectors of instances instead of vectors of > pointers. Any good reason for using pointers? > > https://codereview.chromium.org/2059203002/diff/140001/components/ntp_snippet... > File components/ntp_snippets/content_suggestion_category.h (right): > > https://codereview.chromium.org/2059203002/diff/140001/components/ntp_snippet... > components/ntp_snippets/content_suggestion_category.h:6: #define > COMPONENTS_NTP_SNIPPETS_CONTENT_SUGGESTION_CATEGORY_TYPE_H_ > nit: no TYPE Note Patch Set 9, had to rename another file.
https://codereview.chromium.org/2059203002/diff/180001/components/ntp_snippet... File components/ntp_snippets/content_suggestion.h (right): https://codereview.chromium.org/2059203002/diff/180001/components/ntp_snippet... components/ntp_snippets/content_suggestion.h:8: #include <components/ntp_snippets/content_suggestions_provider_type.h> This is in the wrong place, and should use quotes https://codereview.chromium.org/2059203002/diff/180001/components/ntp_snippet... File components/ntp_snippets/ntp_snippet.cc (right): https://codereview.chromium.org/2059203002/diff/180001/components/ntp_snippet... components/ntp_snippets/ntp_snippet.cc:5: #include <components/ntp_snippets/content_suggestions_provider_type.h> Here too
Oh, of course. Never trust your IDE with seemingly simply tasks like renaming ... https://codereview.chromium.org/2059203002/diff/180001/components/ntp_snippet... File components/ntp_snippets/content_suggestion.h (right): https://codereview.chromium.org/2059203002/diff/180001/components/ntp_snippet... components/ntp_snippets/content_suggestion.h:8: #include <components/ntp_snippets/content_suggestions_provider_type.h> On 2016/06/27 12:25:36, Marc Treib wrote: > This is in the wrong place, and should use quotes Done. https://codereview.chromium.org/2059203002/diff/180001/components/ntp_snippet... File components/ntp_snippets/ntp_snippet.cc (right): https://codereview.chromium.org/2059203002/diff/180001/components/ntp_snippet... components/ntp_snippets/ntp_snippet.cc:5: #include <components/ntp_snippets/content_suggestions_provider_type.h> On 2016/06/27 12:25:36, Marc Treib wrote: > Here too Done.
:D LGTM again
The CQ bit was checked by pke@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from
==========
Add ContentSuggestion, ContentSuggestionCategory and
ContentSuggestionProviderType
Add new classes to be used by the upcoming ContentSuggestionService.
These new classes are added in a way that does not break the existing
NTPSnippets code.
BUG=619560
==========
to
==========
Add ContentSuggestion, ContentSuggestionCategory and
ContentSuggestionProviderType
Add new classes to be used by the upcoming ContentSuggestionService.
These new classes are added in a way that does not break the existing
NTPSnippets code.
BUG=619560
==========
Message was sent while issue was closed.
Committed patchset #11 (id:200001)
Message was sent while issue was closed.
Description was changed from
==========
Add ContentSuggestion, ContentSuggestionCategory and
ContentSuggestionProviderType
Add new classes to be used by the upcoming ContentSuggestionService.
These new classes are added in a way that does not break the existing
NTPSnippets code.
BUG=619560
==========
to
==========
Add ContentSuggestion, ContentSuggestionCategory and
ContentSuggestionProviderType
Add new classes to be used by the upcoming ContentSuggestionService.
These new classes are added in a way that does not break the existing
NTPSnippets code.
BUG=619560
Committed: https://crrev.com/bf8042c0dfb265e5c39d1a7e69fe0aa7625867d3
Cr-Commit-Position: refs/heads/master@{#402164}
==========
Message was sent while issue was closed.
Patchset 11 (id:??) landed as https://crrev.com/bf8042c0dfb265e5c39d1a7e69fe0aa7625867d3 Cr-Commit-Position: refs/heads/master@{#402164} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
