Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2131943002:
Change NTPSnippetsService to implement ContentSuggestionsProvider (Closed)
|
Created:
4 years, 5 months ago by Philipp Keck Modified:
4 years, 5 months ago CC:
chromium-reviews, ntp-dev+reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@neuerservice2 Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionChange NTPSnippetsService to implement ContentSuggestionsProvider
The NTPSnippetsService currently delivers snippets to the UI via the
Android bridge. Change it to additionally serve as a
ContentSuggestionsProvider implementation and deliver
ContentSuggestions for the ARTICLES category. These ContentSuggestions
are currently only delivered to the ContentSuggestionsService and not
served to the UI, yet.
Adjust NTPSnippetsServiceTest, SnippetsBridge and SnippetsInternals
because of a few renamed functions.
Change NTPSnippetsServiceFactory to register the NTPSnippetsService
as a provder with the ContentSuggestionsService.
BUG=619560
Committed: https://crrev.com/a6553c901b9663f2c3d63556f68fe4eccb730e03
Cr-Commit-Position: refs/heads/master@{#405103}
Patch Set 1 #
Total comments: 18
Patch Set 2 : Marcs comments #
Total comments: 4
Patch Set 3 : Change to constructor initialization #
Total comments: 4
Patch Set 4 : Marcs comments #
Total comments: 7
Patch Set 5 : Marcs new comments #Patch Set 6 : Fixed renaming of LOADING to INITIALIZING #
Total comments: 2
Patch Set 7 : Only notify category status change if actually changed #
Total comments: 7
Patch Set 8 : Move comment from implementation to class definition #Patch Set 9 : Ownership comment at ContentSuggestionsProvider::SetObserver #Patch Set 10 : Rebased #Patch Set 11 : Rename ERROR to LOADING_ERROR for compatibility with Windows compilers #Patch Set 12 : Remove unnecessary default-case #Patch Set 13 : Change from C-Style cast to static_cast #Patch Set 14 : Insert the default-case again because some compilers need it #
Total comments: 2
Dependent Patchsets: Messages
Total messages: 51 (17 generated)
pke@google.com changed reviewers: + treib@chromium.org
PTAL
Mostly looks good! https://codereview.chromium.org/2131943002/diff/1/components/ntp_snippets/con... File components/ntp_snippets/content_suggestions_category_status.h (right): https://codereview.chromium.org/2131943002/diff/1/components/ntp_snippets/con... components/ntp_snippets/content_suggestions_category_status.h:16: LOADING, Do we need both LOADING and AVAILABLE_LOADING? https://codereview.chromium.org/2131943002/diff/1/components/ntp_snippets/ntp... File components/ntp_snippets/ntp_snippets_service.cc (right): https://codereview.chromium.org/2131943002/diff/1/components/ntp_snippets/ntp... components/ntp_snippets/ntp_snippets_service.cc:186: // nested namespace. Eh, not sure if a nested namespace is necessary. https://codereview.chromium.org/2131943002/diff/1/components/ntp_snippets/ntp... components/ntp_snippets/ntp_snippets_service.cc:816: if (!snippet->is_discarded() && snippet->is_complete()) { snippets_ contains only non-discarded snippets, no need to check here. Also maybe "if (!complete) continue" to save a level of indentation? https://codereview.chromium.org/2131943002/diff/1/components/ntp_snippets/ntp... components/ntp_snippets/ntp_snippets_service.cc:832: FOR_EACH_OBSERVER(NTPSnippetsServiceObserver, observers_, nit: move this up, so you can do "if (!observer_) return" and save one level of indentation? Also maybe add a(nother) TODO to remove this? https://codereview.chromium.org/2131943002/diff/1/components/ntp_snippets/ntp... File components/ntp_snippets/ntp_snippets_service.h (right): https://codereview.chromium.org/2131943002/diff/1/components/ntp_snippets/ntp... components/ntp_snippets/ntp_snippets_service.h:267: ContentSuggestionsCategoryStatus::LOADING; Do we need both this and state_? Seems that it should be possible to infer one from the other? https://codereview.chromium.org/2131943002/diff/1/components/ntp_snippets/ntp... components/ntp_snippets/ntp_snippets_service.h:327: // TODO(pke): Remove this when snippets internals don't access this service The whole class should be removed, right? Then I'd move the comment up there.
https://codereview.chromium.org/2131943002/diff/1/components/ntp_snippets/con... File components/ntp_snippets/content_suggestions_category_status.h (right): https://codereview.chromium.org/2131943002/diff/1/components/ntp_snippets/con... components/ntp_snippets/content_suggestions_category_status.h:16: LOADING, On 2016/07/08 13:26:02, Marc Treib wrote: > Do we need both LOADING and AVAILABLE_LOADING? My idea was that a distinction between "MAY be available some time" and "WILL be available soon" could be useful. Without focusing too much on the current Android UI, any UI might display an empty section with a loading icon for AVAILABLE_LOADING, but not for LOADING. The article provider should do the following (though currently the order is the wrong way round): - Set status LOADING - Check dependencies, switch to some unavailable state if not satisfied. - Set status AVAILABLE_LOADING - Load data - Set status AVAILABLE The distinction is only useful if the dependencies checking is asynchronous or takes a long time. Similarly, if something with the dependencies changes and the new state needs to be clarified asynchronously, it could be required to remove the currently displayed suggestion cards, which would be achieved by switching to the LOADING state, which is not considered an available state and thus clears the cards from the UI. The Android UI would always treat LOADING and AVAILABLE_LOADING sections the same, so I guess we could just get rid of LOADING. https://codereview.chromium.org/2131943002/diff/1/components/ntp_snippets/ntp... File components/ntp_snippets/ntp_snippets_service.cc (right): https://codereview.chromium.org/2131943002/diff/1/components/ntp_snippets/ntp... components/ntp_snippets/ntp_snippets_service.cc:186: // nested namespace. On 2016/07/08 13:26:02, Marc Treib wrote: > Eh, not sure if a nested namespace is necessary. Done. Changed to "subdirectory" to be consistent with the "offline_pages" subdirectory that I have for the other provider. https://codereview.chromium.org/2131943002/diff/1/components/ntp_snippets/ntp... components/ntp_snippets/ntp_snippets_service.cc:816: if (!snippet->is_discarded() && snippet->is_complete()) { On 2016/07/08 13:26:02, Marc Treib wrote: > snippets_ contains only non-discarded snippets, no need to check here. > Also maybe "if (!complete) continue" to save a level of indentation? Done. https://codereview.chromium.org/2131943002/diff/1/components/ntp_snippets/ntp... components/ntp_snippets/ntp_snippets_service.cc:832: FOR_EACH_OBSERVER(NTPSnippetsServiceObserver, observers_, On 2016/07/08 13:26:02, Marc Treib wrote: > nit: move this up, so you can do "if (!observer_) return" and save one level of > indentation? Also maybe add a(nother) TODO to remove this? Done. https://codereview.chromium.org/2131943002/diff/1/components/ntp_snippets/ntp... File components/ntp_snippets/ntp_snippets_service.h (right): https://codereview.chromium.org/2131943002/diff/1/components/ntp_snippets/ntp... components/ntp_snippets/ntp_snippets_service.h:267: ContentSuggestionsCategoryStatus::LOADING; On 2016/07/08 13:26:02, Marc Treib wrote: > Do we need both this and state_? Seems that it should be possible to infer one > from the other? Yes, state_ and the "disabled reason" together are roughly equivalent to the category_status_. I didn't want to change too much of the code at the moment (especially because the NTPSnippetsStatusService is rather new). After the UI starts talking to the ContentSuggestionsService instead of the NTPSnippetsService, the state_ will become obsolete, the NTPSnippetsStatusService will become available to all providers as a background service (it gives providers the statuses of dependencies like sync, etc.) and the NTPSnippetsService (ArticleSuggestionsProvider) will use that status service to determine the correct category_status_. So then the entire old interface of the NTPSnippetsService including the old observers and state_ can be removed. https://codereview.chromium.org/2131943002/diff/1/components/ntp_snippets/ntp... components/ntp_snippets/ntp_snippets_service.h:327: // TODO(pke): Remove this when snippets internals don't access this service On 2016/07/08 13:26:02, Marc Treib wrote: > The whole class should be removed, right? Then I'd move the comment up there. Done.
https://codereview.chromium.org/2131943002/diff/1/components/ntp_snippets/con... File components/ntp_snippets/content_suggestions_category_status.h (right): https://codereview.chromium.org/2131943002/diff/1/components/ntp_snippets/con... components/ntp_snippets/content_suggestions_category_status.h:16: LOADING, On 2016/07/08 14:02:54, Philipp Keck wrote: > On 2016/07/08 13:26:02, Marc Treib wrote: > > Do we need both LOADING and AVAILABLE_LOADING? > > My idea was that a distinction between "MAY be available some time" and "WILL be > available soon" could be useful. Without focusing too much on the current > Android UI, any UI might display an empty section with a loading icon for > AVAILABLE_LOADING, but not for LOADING. > > The article provider should do the following (though currently the order is the > wrong way round): > - Set status LOADING > - Check dependencies, switch to some unavailable state if not satisfied. > - Set status AVAILABLE_LOADING > - Load data > - Set status AVAILABLE > > The distinction is only useful if the dependencies checking is asynchronous or > takes a long time. Similarly, if something with the dependencies changes and the > new state needs to be clarified asynchronously, it could be required to remove > the currently displayed suggestion cards, which would be achieved by switching > to the LOADING state, which is not considered an available state and thus clears > the cards from the UI. > > The Android UI would always treat LOADING and AVAILABLE_LOADING sections the > same, so I guess we could just get rid of LOADING. Hm, okay, I guess that makes sense. I'd like a different name though, to make the difference clearer. INITIALIZING? Or alternatively, if nobody is interested in that state (yet), we might as well not introduce it for now. https://codereview.chromium.org/2131943002/diff/1/components/ntp_snippets/ntp... File components/ntp_snippets/ntp_snippets_service.h (right): https://codereview.chromium.org/2131943002/diff/1/components/ntp_snippets/ntp... components/ntp_snippets/ntp_snippets_service.h:267: ContentSuggestionsCategoryStatus::LOADING; On 2016/07/08 14:02:55, Philipp Keck wrote: > On 2016/07/08 13:26:02, Marc Treib wrote: > > Do we need both this and state_? Seems that it should be possible to infer one > > from the other? > > Yes, state_ and the "disabled reason" together are roughly equivalent to the > category_status_. I didn't want to change too much of the code at the moment > (especially because the NTPSnippetsStatusService is rather new). After the UI > starts talking to the ContentSuggestionsService instead of the > NTPSnippetsService, the state_ will become obsolete, the > NTPSnippetsStatusService will become available to all providers as a background > service (it gives providers the statuses of dependencies like sync, etc.) and > the NTPSnippetsService (ArticleSuggestionsProvider) will use that status service > to determine the correct category_status_. So then the entire old interface of > the NTPSnippetsService including the old observers and state_ can be removed. Hm. We have fairly nice handling of state transitions though, while category_status_ is updated "wherever" right now. Can we unify updates to state_ and category_status_ somehow? https://codereview.chromium.org/2131943002/diff/20001/components/ntp_snippets... File components/ntp_snippets/ntp_snippets_service.h (right): https://codereview.chromium.org/2131943002/diff/20001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.h:267: ContentSuggestionsCategoryStatus::LOADING; Ah, I didn't know this style of initialization was allowed yet. I'd prefer to keep all initialization in one place though, i.e. the constructor. Maybe we can switch to this style at some point, but then we should switch all other members (for which it makes sense) too. https://codereview.chromium.org/2131943002/diff/20001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.h:288: Observer* observer_ = nullptr; Same here.
https://codereview.chromium.org/2131943002/diff/1/components/ntp_snippets/con... File components/ntp_snippets/content_suggestions_category_status.h (right): https://codereview.chromium.org/2131943002/diff/1/components/ntp_snippets/con... components/ntp_snippets/content_suggestions_category_status.h:16: LOADING, On 2016/07/08 15:17:23, Marc Treib wrote: > On 2016/07/08 14:02:54, Philipp Keck wrote: > > On 2016/07/08 13:26:02, Marc Treib wrote: > > > Do we need both LOADING and AVAILABLE_LOADING? > > > > My idea was that a distinction between "MAY be available some time" and "WILL > be > > available soon" could be useful. Without focusing too much on the current > > Android UI, any UI might display an empty section with a loading icon for > > AVAILABLE_LOADING, but not for LOADING. > > > > The article provider should do the following (though currently the order is > the > > wrong way round): > > - Set status LOADING > > - Check dependencies, switch to some unavailable state if not satisfied. > > - Set status AVAILABLE_LOADING > > - Load data > > - Set status AVAILABLE > > > > The distinction is only useful if the dependencies checking is asynchronous or > > takes a long time. Similarly, if something with the dependencies changes and > the > > new state needs to be clarified asynchronously, it could be required to remove > > the currently displayed suggestion cards, which would be achieved by switching > > to the LOADING state, which is not considered an available state and thus > clears > > the cards from the UI. > > > > The Android UI would always treat LOADING and AVAILABLE_LOADING sections the > > same, so I guess we could just get rid of LOADING. > > Hm, okay, I guess that makes sense. I'd like a different name though, to make > the difference clearer. INITIALIZING? > Or alternatively, if nobody is interested in that state (yet), we might as well > not introduce it for now. I was about to just remove it, but it might prove useful if the providers use this status enum also for their internal state handling (then calling it INITIALIZING would indeed make sense). The reason I initially wanted to avoid "init" is because I thought that a provider can also fall into this state during runtime, e.g., after discovering that sync was disabled and before it has figured out what its new state will be. I thought that happens asynchronously because of the way the NTPSnippetsStatusService communicates, but it is in fact synchronous (and most likely will always be synchronous). So the service would never need to go back to that "LOADING" state where it is unsure, but could directly determine the right ones of the disabled-states, if applicable, or be confident to go back to available. Depending on what we decide for state_ and category_status_ I will rename this to INITIALIZING only if it is used and otherwise remove it. https://codereview.chromium.org/2131943002/diff/1/components/ntp_snippets/ntp... File components/ntp_snippets/ntp_snippets_service.h (right): https://codereview.chromium.org/2131943002/diff/1/components/ntp_snippets/ntp... components/ntp_snippets/ntp_snippets_service.h:267: ContentSuggestionsCategoryStatus::LOADING; On 2016/07/08 15:17:23, Marc Treib wrote: > On 2016/07/08 14:02:55, Philipp Keck wrote: > > On 2016/07/08 13:26:02, Marc Treib wrote: > > > Do we need both this and state_? Seems that it should be possible to infer > one > > > from the other? > > > > Yes, state_ and the "disabled reason" together are roughly equivalent to the > > category_status_. I didn't want to change too much of the code at the moment > > (especially because the NTPSnippetsStatusService is rather new). After the UI > > starts talking to the ContentSuggestionsService instead of the > > NTPSnippetsService, the state_ will become obsolete, the > > NTPSnippetsStatusService will become available to all providers as a > background > > service (it gives providers the statuses of dependencies like sync, etc.) and > > the NTPSnippetsService (ArticleSuggestionsProvider) will use that status > service > > to determine the correct category_status_. So then the entire old interface of > > the NTPSnippetsService including the old observers and state_ can be removed. > > Hm. We have fairly nice handling of state transitions though, while > category_status_ is updated "wherever" right now. Can we unify updates to state_ > and category_status_ somehow? The NTPSnippetsService's state diagram is quite neat because it is so small. The service could use the statuses for its state (grouping a lot of status to the disabled state, using the new INITIALIZING state and using NOT_PROVIDED as the shut-down state). I'm not sure though if it's a good idea to change that already because the UI currently still accesses the state_ and expects it to behave in a certain way. While the state_ might be derivable easily from the category_status_, there are other changes to the state handling that could be made when transforming the NTPSnippetsService to a pure provider (i.e. when it does not have to deliver the state_ anymore): For example, the service could first check if its dependencies (especially sync service) are met, and only if that's the case it would load the database, etc. Changing that requires a lot of refactoring and then testing to make sure that it works in all cases. So I'd rather do it when I have to test anyway, which is when the UI is switched to talk to the ContentSuggestionsService and understand the new statuses that come from there. Currently, the statuses that the NTPSnippetsService reports to the new service might not be perfectly accurate, but since nobody really relies on that anyway (only me when viewing the debug outputs of ContentSuggestionsService), it doesn't matter too much. Finding a solid way to implement the states for the new ArticleSuggestionsProvider is more important than unifying state_ and category_status_ for the meantime. Maybe the new way will be cleaner if it doesn't take the historically grown "state" and "status" of the NTPSnippetsService as a reference. https://codereview.chromium.org/2131943002/diff/20001/components/ntp_snippets... File components/ntp_snippets/ntp_snippets_service.h (right): https://codereview.chromium.org/2131943002/diff/20001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.h:267: ContentSuggestionsCategoryStatus::LOADING; On 2016/07/08 15:17:23, Marc Treib wrote: > Ah, I didn't know this style of initialization was allowed yet. > I'd prefer to keep all initialization in one place though, i.e. the constructor. > Maybe we can switch to this style at some point, but then we should switch all > other members (for which it makes sense) too. Done. https://codereview.chromium.org/2131943002/diff/20001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.h:288: Observer* observer_ = nullptr; On 2016/07/08 15:17:23, Marc Treib wrote: > Same here. Done.
https://codereview.chromium.org/2131943002/diff/1/components/ntp_snippets/ntp... File components/ntp_snippets/ntp_snippets_service.h (right): https://codereview.chromium.org/2131943002/diff/1/components/ntp_snippets/ntp... components/ntp_snippets/ntp_snippets_service.h:267: ContentSuggestionsCategoryStatus::LOADING; On 2016/07/08 16:17:01, Philipp Keck wrote: > On 2016/07/08 15:17:23, Marc Treib wrote: > > On 2016/07/08 14:02:55, Philipp Keck wrote: > > > On 2016/07/08 13:26:02, Marc Treib wrote: > > > > Do we need both this and state_? Seems that it should be possible to infer > > one > > > > from the other? > > > > > > Yes, state_ and the "disabled reason" together are roughly equivalent to the > > > category_status_. I didn't want to change too much of the code at the moment > > > (especially because the NTPSnippetsStatusService is rather new). After the > UI > > > starts talking to the ContentSuggestionsService instead of the > > > NTPSnippetsService, the state_ will become obsolete, the > > > NTPSnippetsStatusService will become available to all providers as a > > background > > > service (it gives providers the statuses of dependencies like sync, etc.) > and > > > the NTPSnippetsService (ArticleSuggestionsProvider) will use that status > > service > > > to determine the correct category_status_. So then the entire old interface > of > > > the NTPSnippetsService including the old observers and state_ can be > removed. > > > > Hm. We have fairly nice handling of state transitions though, while > > category_status_ is updated "wherever" right now. Can we unify updates to > state_ > > and category_status_ somehow? > > The NTPSnippetsService's state diagram is quite neat because it is so small. The > service could use the statuses for its state (grouping a lot of status to the > disabled state, using the new INITIALIZING state and using NOT_PROVIDED as the > shut-down state). I'm not sure though if it's a good idea to change that already > because the UI currently still accesses the state_ and expects it to behave in a > certain way. While the state_ might be derivable easily from the > category_status_, there are other changes to the state handling that could be > made when transforming the NTPSnippetsService to a pure provider (i.e. when it > does not have to deliver the state_ anymore): For example, the service could > first check if its dependencies (especially sync service) are met, and only if > that's the case it would load the database, etc. Changing that requires a lot of > refactoring and then testing to make sure that it works in all cases. So I'd > rather do it when I have to test anyway, which is when the UI is switched to > talk to the ContentSuggestionsService and understand the new statuses that come > from there. Currently, the statuses that the NTPSnippetsService reports to the > new service might not be perfectly accurate, but since nobody really relies on > that anyway (only me when viewing the debug outputs of > ContentSuggestionsService), it doesn't matter too much. > > Finding a solid way to implement the states for the new > ArticleSuggestionsProvider is more important than unifying state_ and > category_status_ for the meantime. Maybe the new way will be cleaner if it > doesn't take the historically grown "state" and "status" of the > NTPSnippetsService as a reference. Alright. But I still don't like how category_status_ is updated in lots of random places, and each of them needs to remember to call Notify.. Can we move the updating and notifying into EnterState? It looks like state_ and category_status_ are always updated together now anyway. https://codereview.chromium.org/2131943002/diff/40001/components/ntp_snippets... File components/ntp_snippets/ntp_snippets_service.cc (right): https://codereview.chromium.org/2131943002/diff/40001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.cc:359: return category_status_; nit: Maybe DCHECK that |category| is ARTICLES? https://codereview.chromium.org/2131943002/diff/40001/components/ntp_snippets... File components/ntp_snippets/ntp_snippets_service.h (right): https://codereview.chromium.org/2131943002/diff/40001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.h:327: // should only be used by snippets-internals. nit: I'd remove this comment. As of now, this method is still used, and there is already a TODO to remove the whole class.
https://codereview.chromium.org/2131943002/diff/1/components/ntp_snippets/ntp... File components/ntp_snippets/ntp_snippets_service.h (right): https://codereview.chromium.org/2131943002/diff/1/components/ntp_snippets/ntp... components/ntp_snippets/ntp_snippets_service.h:267: ContentSuggestionsCategoryStatus::LOADING; On 2016/07/11 08:41:33, Marc Treib wrote: > On 2016/07/08 16:17:01, Philipp Keck wrote: > > On 2016/07/08 15:17:23, Marc Treib wrote: > > > On 2016/07/08 14:02:55, Philipp Keck wrote: > > > > On 2016/07/08 13:26:02, Marc Treib wrote: > > > > > Do we need both this and state_? Seems that it should be possible to > infer > > > one > > > > > from the other? > > > > > > > > Yes, state_ and the "disabled reason" together are roughly equivalent to > the > > > > category_status_. I didn't want to change too much of the code at the > moment > > > > (especially because the NTPSnippetsStatusService is rather new). After the > > UI > > > > starts talking to the ContentSuggestionsService instead of the > > > > NTPSnippetsService, the state_ will become obsolete, the > > > > NTPSnippetsStatusService will become available to all providers as a > > > background > > > > service (it gives providers the statuses of dependencies like sync, etc.) > > and > > > > the NTPSnippetsService (ArticleSuggestionsProvider) will use that status > > > service > > > > to determine the correct category_status_. So then the entire old > interface > > of > > > > the NTPSnippetsService including the old observers and state_ can be > > removed. > > > > > > Hm. We have fairly nice handling of state transitions though, while > > > category_status_ is updated "wherever" right now. Can we unify updates to > > state_ > > > and category_status_ somehow? > > > > The NTPSnippetsService's state diagram is quite neat because it is so small. > The > > service could use the statuses for its state (grouping a lot of status to the > > disabled state, using the new INITIALIZING state and using NOT_PROVIDED as the > > shut-down state). I'm not sure though if it's a good idea to change that > already > > because the UI currently still accesses the state_ and expects it to behave in > a > > certain way. While the state_ might be derivable easily from the > > category_status_, there are other changes to the state handling that could be > > made when transforming the NTPSnippetsService to a pure provider (i.e. when it > > does not have to deliver the state_ anymore): For example, the service could > > first check if its dependencies (especially sync service) are met, and only if > > that's the case it would load the database, etc. Changing that requires a lot > of > > refactoring and then testing to make sure that it works in all cases. So I'd > > rather do it when I have to test anyway, which is when the UI is switched to > > talk to the ContentSuggestionsService and understand the new statuses that > come > > from there. Currently, the statuses that the NTPSnippetsService reports to the > > new service might not be perfectly accurate, but since nobody really relies on > > that anyway (only me when viewing the debug outputs of > > ContentSuggestionsService), it doesn't matter too much. > > > > Finding a solid way to implement the states for the new > > ArticleSuggestionsProvider is more important than unifying state_ and > > category_status_ for the meantime. Maybe the new way will be cleaner if it > > doesn't take the historically grown "state" and "status" of the > > NTPSnippetsService as a reference. > > Alright. But I still don't like how category_status_ is updated in lots of > random places, and each of them needs to remember to call Notify.. > Can we move the updating and notifying into EnterState? It looks like state_ and > category_status_ are always updated together now anyway. Done. https://codereview.chromium.org/2131943002/diff/40001/components/ntp_snippets... File components/ntp_snippets/ntp_snippets_service.cc (right): https://codereview.chromium.org/2131943002/diff/40001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.cc:359: return category_status_; On 2016/07/11 08:41:33, Marc Treib wrote: > nit: Maybe DCHECK that |category| is ARTICLES? Done. https://codereview.chromium.org/2131943002/diff/40001/components/ntp_snippets... File components/ntp_snippets/ntp_snippets_service.h (right): https://codereview.chromium.org/2131943002/diff/40001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.h:327: // should only be used by snippets-internals. On 2016/07/11 08:41:33, Marc Treib wrote: > nit: I'd remove this comment. As of now, this method is still used, and there is > already a TODO to remove the whole class. Done. https://codereview.chromium.org/2131943002/diff/60001/components/ntp_snippets... File components/ntp_snippets/ntp_snippets_service.cc (right): https://codereview.chromium.org/2131943002/diff/60001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.cc:769: category_status_ = new_status; This code duplicates line 806. Alternative control structure with one more level of indentation and no early-returns: if (state != state_) { switch ... } if (new_status != category_status_) { update notify }
https://codereview.chromium.org/2131943002/diff/60001/components/ntp_snippets... File components/ntp_snippets/content_suggestions_category_status.h (right): https://codereview.chromium.org/2131943002/diff/60001/components/ntp_snippets... components/ntp_snippets/content_suggestions_category_status.h:16: LOADING, Should this be INITIALIZING now? https://codereview.chromium.org/2131943002/diff/60001/components/ntp_snippets... File components/ntp_snippets/ntp_snippets_service.cc (right): https://codereview.chromium.org/2131943002/diff/60001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.cc:758: new_status = ContentSuggestionsCategoryStatus::ERROR; nit: Can we get a consistent order of assigning new_state and new_status please? https://codereview.chromium.org/2131943002/diff/60001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.cc:769: category_status_ = new_status; On 2016/07/11 09:41:49, Philipp Keck wrote: > This code duplicates line 806. Alternative control structure with one more level > of indentation and no early-returns: > > if (state != state_) { > switch ... > } > > if (new_status != category_status_) { > update > notify > } Hm. I guess we don't want to notify about the changed status before we've updated the state too? If that were okay, you could just put the status update before the if. Otherwise, the version without early return is probably better. nit: Please either rename state to new_state, or remove the new_ from new_status.
https://codereview.chromium.org/2131943002/diff/60001/components/ntp_snippets... File components/ntp_snippets/content_suggestions_category_status.h (right): https://codereview.chromium.org/2131943002/diff/60001/components/ntp_snippets... components/ntp_snippets/content_suggestions_category_status.h:16: LOADING, On 2016/07/11 10:12:04, Marc Treib wrote: > Should this be INITIALIZING now? Done. https://codereview.chromium.org/2131943002/diff/60001/components/ntp_snippets... File components/ntp_snippets/ntp_snippets_service.cc (right): https://codereview.chromium.org/2131943002/diff/60001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.cc:758: new_status = ContentSuggestionsCategoryStatus::ERROR; On 2016/07/11 10:12:04, Marc Treib wrote: > nit: Can we get a consistent order of assigning new_state and new_status please? Done. https://codereview.chromium.org/2131943002/diff/60001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.cc:769: category_status_ = new_status; On 2016/07/11 10:12:04, Marc Treib wrote: > On 2016/07/11 09:41:49, Philipp Keck wrote: > > This code duplicates line 806. Alternative control structure with one more > level > > of indentation and no early-returns: > > > > if (state != state_) { > > switch ... > > } > > > > if (new_status != category_status_) { > > update > > notify > > } > > Hm. I guess we don't want to notify about the changed status before we've > updated the state too? If that were okay, you could just put the status update > before the if. > Otherwise, the version without early return is probably better. > > nit: Please either rename state to new_state, or remove the new_ from > new_status. That's what I thought initially, but actually it shouldn't matter too much if the status is updated beforehand because it is pretty independent anyway -- either someone accesses state+disabledReason or the status. Using your first suggestion, therefore.
lgtm https://codereview.chromium.org/2131943002/diff/100001/components/ntp_snippet... File components/ntp_snippets/ntp_snippets_service.cc (right): https://codereview.chromium.org/2131943002/diff/100001/components/ntp_snippet... components/ntp_snippets/ntp_snippets_service.cc:767: category_status_ = status; Should this be in an "if (status changed)" ?
https://codereview.chromium.org/2131943002/diff/100001/components/ntp_snippet... File components/ntp_snippets/ntp_snippets_service.cc (right): https://codereview.chromium.org/2131943002/diff/100001/components/ntp_snippet... components/ntp_snippets/ntp_snippets_service.cc:767: category_status_ = status; On 2016/07/11 10:40:02, Marc Treib wrote: > Should this be in an "if (status changed)" ? Yes!
pke@google.com changed reviewers: + bauerb@chromium.org
bauerb@chromium.org: Please review changes in snippets_internals_message_handler.cc.
lgtm https://codereview.chromium.org/2131943002/diff/120001/components/ntp_snippet... File components/ntp_snippets/ntp_snippets_service.cc (right): https://codereview.chromium.org/2131943002/diff/120001/components/ntp_snippet... components/ntp_snippets/ntp_snippets_service.cc:186: // a subdirectory. This TODO might belong to the class definition? https://codereview.chromium.org/2131943002/diff/120001/components/ntp_snippet... File components/ntp_snippets/ntp_snippets_service.h (right): https://codereview.chromium.org/2131943002/diff/120001/components/ntp_snippet... components/ntp_snippets/ntp_snippets_service.h:135: void SetObserver(Observer* observer) override; Can you add a comment about ownership of |observer|?
https://codereview.chromium.org/2131943002/diff/120001/components/ntp_snippet... File components/ntp_snippets/ntp_snippets_service.cc (right): https://codereview.chromium.org/2131943002/diff/120001/components/ntp_snippet... components/ntp_snippets/ntp_snippets_service.cc:186: // a subdirectory. On 2016/07/11 13:10:12, Bernhard Bauer wrote: > This TODO might belong to the class definition? Done. https://codereview.chromium.org/2131943002/diff/120001/components/ntp_snippet... File components/ntp_snippets/ntp_snippets_service.h (right): https://codereview.chromium.org/2131943002/diff/120001/components/ntp_snippet... components/ntp_snippets/ntp_snippets_service.h:135: void SetObserver(Observer* observer) override; On 2016/07/11 13:10:12, Bernhard Bauer wrote: > Can you add a comment about ownership of |observer|? You mean whether or not the implementation should take ownership of the object and destroy it when the provider shuts down? If that's not the question, please explain what you mean by "ownership". If it is the question, the answer would be no ownership because it points to the main service; and I could put a comment. Another -- possibly even nicer -- solution would be to move the observer_ and SetObserver() members to the base class ContentSuggestionsProvider, because any provider will need those anyway. Like that, the implementation wouldn't even need to think about the ownership question, assuming that the ~ContentSuggestionsProvider destructor would have to take care of it.
https://codereview.chromium.org/2131943002/diff/120001/components/ntp_snippet... File components/ntp_snippets/ntp_snippets_service.h (right): https://codereview.chromium.org/2131943002/diff/120001/components/ntp_snippet... components/ntp_snippets/ntp_snippets_service.h:135: void SetObserver(Observer* observer) override; On 2016/07/11 13:25:01, Philipp Keck wrote: > On 2016/07/11 13:10:12, Bernhard Bauer wrote: > > Can you add a comment about ownership of |observer|? > > You mean whether or not the implementation should take ownership of the object > and destroy it when the provider shuts down? > > If that's not the question, please explain what you mean by "ownership". > > If it is the question, the answer would be no ownership because it points to the > main service; and I could put a comment. > Another -- possibly even nicer -- solution would be to move the observer_ and > SetObserver() members to the base class ContentSuggestionsProvider, because any > provider will need those anyway. Like that, the implementation wouldn't even > need to think about the ownership question, assuming that the > ~ContentSuggestionsProvider destructor would have to take care of it. Pretty sure what Bernhard said was "Please add a comment saying that this doesn't take ownership, and the observer must outlive this object" :)
https://codereview.chromium.org/2131943002/diff/120001/components/ntp_snippet... File components/ntp_snippets/ntp_snippets_service.h (right): https://codereview.chromium.org/2131943002/diff/120001/components/ntp_snippet... components/ntp_snippets/ntp_snippets_service.h:135: void SetObserver(Observer* observer) override; On 2016/07/11 13:38:31, Marc Treib wrote: > On 2016/07/11 13:25:01, Philipp Keck wrote: > > On 2016/07/11 13:10:12, Bernhard Bauer wrote: > > > Can you add a comment about ownership of |observer|? > > > > You mean whether or not the implementation should take ownership of the object > > and destroy it when the provider shuts down? > > > > If that's not the question, please explain what you mean by "ownership". > > > > If it is the question, the answer would be no ownership because it points to > the > > main service; and I could put a comment. > > Another -- possibly even nicer -- solution would be to move the observer_ and > > SetObserver() members to the base class ContentSuggestionsProvider, because > any > > provider will need those anyway. Like that, the implementation wouldn't even > > need to think about the ownership question, assuming that the > > ~ContentSuggestionsProvider destructor would have to take care of it. > > Pretty sure what Bernhard said was "Please add a comment saying that this > doesn't take ownership, and the observer must outlive this object" :) Yup, that. Marc knows me well ^^
https://codereview.chromium.org/2131943002/diff/120001/components/ntp_snippet... File components/ntp_snippets/ntp_snippets_service.h (right): https://codereview.chromium.org/2131943002/diff/120001/components/ntp_snippet... components/ntp_snippets/ntp_snippets_service.h:135: void SetObserver(Observer* observer) override; On 2016/07/11 15:13:18, Bernhard Bauer wrote: > On 2016/07/11 13:38:31, Marc Treib wrote: > > On 2016/07/11 13:25:01, Philipp Keck wrote: > > > On 2016/07/11 13:10:12, Bernhard Bauer wrote: > > > > Can you add a comment about ownership of |observer|? > > > > > > You mean whether or not the implementation should take ownership of the > object > > > and destroy it when the provider shuts down? > > > > > > If that's not the question, please explain what you mean by "ownership". > > > > > > If it is the question, the answer would be no ownership because it points to > > the > > > main service; and I could put a comment. > > > Another -- possibly even nicer -- solution would be to move the observer_ > and > > > SetObserver() members to the base class ContentSuggestionsProvider, because > > any > > > provider will need those anyway. Like that, the implementation wouldn't even > > > need to think about the ownership question, assuming that the > > > ~ContentSuggestionsProvider destructor would have to take care of it. > > > > Pretty sure what Bernhard said was "Please add a comment saying that this > > doesn't take ownership, and the observer must outlive this object" :) > > Yup, that. Marc knows me well ^^ :-) Done.
The CQ bit was checked by pke@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from treib@chromium.org, bauerb@chromium.org Link to the patchset: https://codereview.chromium.org/2131943002/#ps160001 (title: "Ownership comment at ContentSuggestionsProvider::SetObserver")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-gn on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-gn on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn/...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
The CQ bit was checked by pke@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from treib@chromium.org, bauerb@chromium.org Link to the patchset: https://codereview.chromium.org/2131943002/#ps180001 (title: "Rebased")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
@treib PTAL at last patch set.
On 2016/07/12 07:58:54, Philipp Keck wrote: > @treib PTAL at last patch set. Windows compilers and their #defines... LGTM!
The CQ bit was checked by pke@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org Link to the patchset: https://codereview.chromium.org/2131943002/#ps200001 (title: "Rename ERROR to LOADING_ERROR for compatibility with Windows compilers")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Implemented two small changes discussed here: https://codereview.chromium.org/2145563002/diff/1/chrome/browser/ui/webui/sni...
The CQ bit was checked by pke@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from treib@chromium.org, bauerb@chromium.org Link to the patchset: https://codereview.chromium.org/2131943002/#ps240001 (title: "Change from C-Style cast to static_cast")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
The CQ bit was checked by pke@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from treib@chromium.org, bauerb@chromium.org Link to the patchset: https://codereview.chromium.org/2131943002/#ps260001 (title: "Insert the default-case again because some compilers need it")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2131943002/diff/260001/components/ntp_snippet... File components/ntp_snippets/ntp_snippets_service.cc (right): https://codereview.chromium.org/2131943002/diff/260001/components/ntp_snippet... components/ntp_snippets/ntp_snippets_service.cc:752: new_status = ContentSuggestionsCategoryStatus::LOADING_ERROR; The Win compiler doesn't really need the default case, it just wants new_state and new_status to be initialized, which you could do above (before the switch). (Not important enough to cancel the commit, we can change this later)
Message was sent while issue was closed.
Committed patchset #14 (id:260001)
Message was sent while issue was closed.
CQ bit was unchecked.
Message was sent while issue was closed.
Description was changed from ========== Change NTPSnippetsService to implement ContentSuggestionsProvider The NTPSnippetsService currently delivers snippets to the UI via the Android bridge. Change it to additionally serve as a ContentSuggestionsProvider implementation and deliver ContentSuggestions for the ARTICLES category. These ContentSuggestions are currently only delivered to the ContentSuggestionsService and not served to the UI, yet. Adjust NTPSnippetsServiceTest, SnippetsBridge and SnippetsInternals because of a few renamed functions. Change NTPSnippetsServiceFactory to register the NTPSnippetsService as a provder with the ContentSuggestionsService. BUG=619560 ========== to ========== Change NTPSnippetsService to implement ContentSuggestionsProvider The NTPSnippetsService currently delivers snippets to the UI via the Android bridge. Change it to additionally serve as a ContentSuggestionsProvider implementation and deliver ContentSuggestions for the ARTICLES category. These ContentSuggestions are currently only delivered to the ContentSuggestionsService and not served to the UI, yet. Adjust NTPSnippetsServiceTest, SnippetsBridge and SnippetsInternals because of a few renamed functions. Change NTPSnippetsServiceFactory to register the NTPSnippetsService as a provder with the ContentSuggestionsService. BUG=619560 Committed: https://crrev.com/a6553c901b9663f2c3d63556f68fe4eccb730e03 Cr-Commit-Position: refs/heads/master@{#405103} ==========
Message was sent while issue was closed.
Patchset 14 (id:??) landed as https://crrev.com/a6553c901b9663f2c3d63556f68fe4eccb730e03 Cr-Commit-Position: refs/heads/master@{#405103}
Message was sent while issue was closed.
https://codereview.chromium.org/2131943002/diff/260001/components/ntp_snippet... File components/ntp_snippets/ntp_snippets_service.cc (right): https://codereview.chromium.org/2131943002/diff/260001/components/ntp_snippet... components/ntp_snippets/ntp_snippets_service.cc:752: new_status = ContentSuggestionsCategoryStatus::LOADING_ERROR; On 2016/07/13 08:49:43, Marc Treib wrote: > The Win compiler doesn't really need the default case, it just wants new_state > and new_status to be initialized, which you could do above (before the switch). > (Not important enough to cancel the commit, we can change this later) I will include this in the next CL. |
