Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2557363002:
[NTP Snippets] Refactor background scheduling for remote suggestions (Closed)
Description[NTP Snippets] Refactor background scheduling for remote suggestions
This CL splits off scheduling functionality from
RemoteSuggestionsProvider into a separate class.
RemoteSuggestionsProvider is a complex class. This is to prepare for
further work on soft-scheduled updates (wich makes scheduling slightly
more complex).
The CL also renames the getter for RemoteSuggestionsProvider as it would anyway touch almost all of its callers.
BUG=672479
Committed: https://crrev.com/093410c469a8105341cdf0062e65a5c5ac0ef4b8
Cr-Commit-Position: refs/heads/master@{#440116}
Patch Set 1 #
Total comments: 8
Patch Set 2 : Rebase #Patch Set 3 : Marc's comments #1 #
Total comments: 5
Patch Set 4 : Make unit-tests pass #
Total comments: 82
Patch Set 5 : Take two #Patch Set 6 : Small fixes #Patch Set 7 : Small fixes #2 #Patch Set 8 : Rebase #
Total comments: 36
Patch Set 9 : Tim's comments some unit-test changes ( rebase, sorry about it!) #Patch Set 10 : Rebase #2 #Patch Set 11 : Tim's comments and splitting RemoteSuggestionsProvider and RemoteSuggestionsProviderImpl #
Total comments: 77
Patch Set 12 : Addressing many comments #
Total comments: 4
Patch Set 13 : Rebase #
Total comments: 2
Patch Set 14 : Changes to make unit-tests pass #
Total comments: 1
Patch Set 15 : A minor fix #
Total comments: 2
Patch Set 16 : Rebase #Patch Set 17 : Eric's comment #
Total comments: 10
Patch Set 18 : Tim's comments + fix ios compilation #
Total comments: 23
Patch Set 19 : Tim's and Bernhard's comments #Patch Set 20 : Tim's and Bernhard's comments #2 #Patch Set 21 : Fixing the last changes :) #Dependent Patchsets: Messages
Total messages: 68 (25 generated)
Description was changed from ========== [NTP Snippets] Refactor background scheduling for remote suggestions This CL splits off scheduling functionality from RemoteSuggestionsProvider into a separate class. RemoteSuggestionsProvider is a complex class. This is to prepare for further work on soft-scheduled updates (wich makes scheduling slightly more complex). BUG=672479 ========== to ========== [NTP Snippets] Refactor background scheduling for remote suggestions This CL splits off scheduling functionality from RemoteSuggestionsProvider into a separate class. RemoteSuggestionsProvider is a complex class. This is to prepare for further work on soft-scheduled updates (wich makes scheduling slightly more complex). The CL also renames the getter for RemoteSuggestionsProvider as it would anyway touch almost all of its callers. BUG=672479 ==========
jkrcal@chromium.org changed reviewers: + treib@chromium.org
Marc, could you PTAL? (It has grown a bit, I am sorry about it. I tried to rethink the interface to the scheduler to make it as simple as possible.)
First few comments, but mostly: The diff is incomplete; looks like it's based on some local changes that weren't uploaded. Could you fix that and upload again? https://codereview.chromium.org/2557363002/diff/1/components/ntp_snippets/rem... File components/ntp_snippets/remote/remote_suggestions_hard_scheduler.h (right): https://codereview.chromium.org/2557363002/diff/1/components/ntp_snippets/rem... components/ntp_snippets/remote/remote_suggestions_hard_scheduler.h:6: #define COMPONENTS_NTP_SNIPPETS_REMOTE_REMOTE_SUGGESTIONS_HARD_SCHEDULER_H_ The diff is weird (missing) here... do you maybe have a local base branch without a CL? https://codereview.chromium.org/2557363002/diff/1/components/ntp_snippets/rem... File components/ntp_snippets/remote/remote_suggestions_provider.cc (right): https://codereview.chromium.org/2557363002/diff/1/components/ntp_snippets/rem... components/ntp_snippets/remote/remote_suggestions_provider.cc:192: this /* scheduler_updater */, /*scheduler_updater=*/this That said, it's a bit dangerous to pass out the |this| pointer here - if the scheduler tries to do anything with it, things will probably fail in interesting ways, because this object isn't initialized yet. https://codereview.chromium.org/2557363002/diff/1/components/ntp_snippets/rem... File components/ntp_snippets/remote/remote_suggestions_provider.h (right): https://codereview.chromium.org/2557363002/diff/1/components/ntp_snippets/rem... components/ntp_snippets/remote/remote_suggestions_provider.h:96: // ContentSuggestionsScheduler::Updater implementation RemoteSuggestionsScheduler. Also, private? https://codereview.chromium.org/2557363002/diff/1/components/ntp_snippets/rem... File components/ntp_snippets/remote/remote_suggestions_scheduler.h (right): https://codereview.chromium.org/2557363002/diff/1/components/ntp_snippets/rem... components/ntp_snippets/remote/remote_suggestions_scheduler.h:32: virtual void UpdateRemoteSuggestionsBySchedule(); Also here: There's some part of the patch missing. Seems something went wrong with the upload.
Marc, thanks! PTAL, again. https://codereview.chromium.org/2557363002/diff/1/components/ntp_snippets/rem... File components/ntp_snippets/remote/remote_suggestions_hard_scheduler.h (right): https://codereview.chromium.org/2557363002/diff/1/components/ntp_snippets/rem... components/ntp_snippets/remote/remote_suggestions_hard_scheduler.h:6: #define COMPONENTS_NTP_SNIPPETS_REMOTE_REMOTE_SUGGESTIONS_HARD_SCHEDULER_H_ On 2016/12/08 16:14:58, Marc Treib wrote: > The diff is weird (missing) here... do you maybe have a local base branch > without a CL? Oh, shit. I've messed up :) The two files you've spotted have leaked into a totally independent CL https://codereview.chromium.org/2558743003/ (on which I based this CL because I use the general parsing functions, now). That CL has unfortunately already landed. No problem for trunk at the moment as these are new files, not referenced. Can you see any reasonably simple way to fix it? Otherwise, can you please just mentally treat both files in review as completely new, please? https://codereview.chromium.org/2557363002/diff/1/components/ntp_snippets/rem... File components/ntp_snippets/remote/remote_suggestions_provider.cc (right): https://codereview.chromium.org/2557363002/diff/1/components/ntp_snippets/rem... components/ntp_snippets/remote/remote_suggestions_provider.cc:192: this /* scheduler_updater */, On 2016/12/08 16:14:58, Marc Treib wrote: > /*scheduler_updater=*/this > > That said, it's a bit dangerous to pass out the |this| pointer here - if the > scheduler tries to do anything with it, things will probably fail in interesting > ways, because this object isn't initialized yet. Fair enough. Removed it from the constructor and added another method to pass it in later. https://codereview.chromium.org/2557363002/diff/1/components/ntp_snippets/rem... File components/ntp_snippets/remote/remote_suggestions_provider.h (right): https://codereview.chromium.org/2557363002/diff/1/components/ntp_snippets/rem... components/ntp_snippets/remote/remote_suggestions_provider.h:96: // ContentSuggestionsScheduler::Updater implementation On 2016/12/08 16:14:58, Marc Treib wrote: > RemoteSuggestionsScheduler. Also, private? Done both. (thanks, was not aware that implementations of public functions can be private ;)
+Markus as cc (sorry, I've forgotten)
Comment re previously added headers below. Reviewing now. https://codereview.chromium.org/2557363002/diff/1/components/ntp_snippets/rem... File components/ntp_snippets/remote/remote_suggestions_hard_scheduler.h (right): https://codereview.chromium.org/2557363002/diff/1/components/ntp_snippets/rem... components/ntp_snippets/remote/remote_suggestions_hard_scheduler.h:6: #define COMPONENTS_NTP_SNIPPETS_REMOTE_REMOTE_SUGGESTIONS_HARD_SCHEDULER_H_ On 2016/12/09 09:20:37, jkrcal wrote: > On 2016/12/08 16:14:58, Marc Treib wrote: > > The diff is weird (missing) here... do you maybe have a local base branch > > without a CL? > > Oh, shit. I've messed up :) The two files you've spotted have leaked into a > totally independent CL https://codereview.chromium.org/2558743003/ (on which I > based this CL because I use the general parsing functions, now). > > That CL has unfortunately already landed. No problem for trunk at the moment as > these are new files, not referenced. > Can you see any reasonably simple way to fix it? Well, I guess the only way would be to land another CL that removes them again. But since they don't really create any problems, it's probably not worth the churn. Let's just get this one landed quickly :) > Otherwise, can you please just mentally treat both files in review as completely > new, please? Sure, will do.
The CQ bit was checked by jkrcal@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2557363002/diff/40001/components/ntp_snippets... File components/ntp_snippets/remote/remote_suggestions_hard_scheduler.h (right): https://codereview.chromium.org/2557363002/diff/40001/components/ntp_snippets... components/ntp_snippets/remote/remote_suggestions_hard_scheduler.h:16: class RemoteSuggestionsHardScheduler { I'm not very happy with this name. For one, "hard" is very clear. But more importantly, this name sounds like it's "next to" RemoteSuggestionsScheduler, maybe an alternative to it or something. But in fact, it's one level below, and its responsibilities are very different (much more limited). But I'm also not sure what a good name would be :) Maybe something very explicit like SchedulingOSBridge? That's really all it is. https://codereview.chromium.org/2557363002/diff/40001/components/ntp_snippets... File components/ntp_snippets/remote/remote_suggestions_provider.h (right): https://codereview.chromium.org/2557363002/diff/40001/components/ntp_snippets... components/ntp_snippets/remote/remote_suggestions_provider.h:112: RemoteSuggestionsScheduler* scheduler() { Comment on why this needs to be exposed? https://codereview.chromium.org/2557363002/diff/60001/components/ntp_snippets... File components/ntp_snippets/remote/remote_suggestions_provider.cc (left): https://codereview.chromium.org/2557363002/diff/60001/components/ntp_snippets... components/ntp_snippets/remote/remote_suggestions_provider.cc:418: if (state_ != State::NOT_INITED || force) { Is this case still handled in the new world? https://codereview.chromium.org/2557363002/diff/60001/components/ntp_snippets... File components/ntp_snippets/remote/remote_suggestions_provider.cc (right): https://codereview.chromium.org/2557363002/diff/60001/components/ntp_snippets... components/ntp_snippets/remote/remote_suggestions_provider.cc:194: pref_service), I think this fits on one line? Maybe for a later CL: Would we want to inject the whole scheduler? Might make testing easier. https://codereview.chromium.org/2557363002/diff/60001/components/ntp_snippets... File components/ntp_snippets/remote/remote_suggestions_provider_unittest.cc (left): https://codereview.chromium.org/2557363002/diff/60001/components/ntp_snippets... components/ntp_snippets/remote/remote_suggestions_provider_unittest.cc:557: TEST_F(RemoteSuggestionsProviderTest, ScheduleOnStart) { I think some of the removed tests kinda still apply - what's the new behavior of RemoteSuggestionsProvider vs the scheduler? I think injecting a mock scheduler (the full thing, not just the "hard" one) might be good, then we could test that the provider calls the right things at the right time. https://codereview.chromium.org/2557363002/diff/60001/components/ntp_snippets... File components/ntp_snippets/remote/remote_suggestions_scheduler.cc (right): https://codereview.chromium.org/2557363002/diff/60001/components/ntp_snippets... components/ntp_snippets/remote/remote_suggestions_scheduler.cc:43: const char* param_name = ""; nullptr? The "empty" case can't happen anyway https://codereview.chromium.org/2557363002/diff/60001/components/ntp_snippets... components/ntp_snippets/remote/remote_suggestions_scheduler.cc:84: DCHECK(updater) << "non-null Updater must be provided"; Since we have to handle a null updater_ anyway, maybe this isn't needed? https://codereview.chromium.org/2557363002/diff/60001/components/ntp_snippets... components/ntp_snippets/remote/remote_suggestions_scheduler.cc:102: updater_->UpdateRemoteSuggestionsBySchedule(); Hm. So in practice, the OS-scheduled update gets here via ContentSuggestionsService->remote_suggestions_provider()->scheduler(), and this effectively calls back into the RemoteSuggestionsProvider. Should we get rid of this roundtrip through here? https://codereview.chromium.org/2557363002/diff/60001/components/ntp_snippets... components/ntp_snippets/remote/remote_suggestions_scheduler.cc:116: last_schedule.interval_fallback != schedule.interval_fallback) { optional: Could put operator==/!= on the struct? IMO that'd improve changes that this will be updated when a member is added. https://codereview.chromium.org/2557363002/diff/60001/components/ntp_snippets... components/ntp_snippets/remote/remote_suggestions_scheduler.cc:130: last_schedule.interval_fallback.is_zero()) { Similar here, could put an accessor on the struct. Also, you probably don't want to check interval_fallback twice :) (Should we have a test to catch this?) https://codereview.chromium.org/2557363002/diff/60001/components/ntp_snippets... components/ntp_snippets/remote/remote_suggestions_scheduler.cc:137: base::TimeDelta::FromHours(0)}); Maybe make an UpdateSchedule::Empty() ? https://codereview.chromium.org/2557363002/diff/60001/components/ntp_snippets... components/ntp_snippets/remote/remote_suggestions_scheduler.cc:157: RemoteSuggestionsScheduler::GetCurrentUpdateSchedule() { Hm, IMO this name is a bit misleading. I'd interpret it as "give me the values that are currently scheduled". https://codereview.chromium.org/2557363002/diff/60001/components/ntp_snippets... File components/ntp_snippets/remote/remote_suggestions_scheduler.h (right): https://codereview.chromium.org/2557363002/diff/60001/components/ntp_snippets... components/ntp_snippets/remote/remote_suggestions_scheduler.h:40: explicit RemoteSuggestionsScheduler( "explicit" not needed https://codereview.chromium.org/2557363002/diff/60001/components/ntp_snippets... components/ntp_snippets/remote/remote_suggestions_scheduler.h:49: // is skipped. Document the lifetime of |updater|: Must outlive us. https://codereview.chromium.org/2557363002/diff/60001/components/ntp_snippets... components/ntp_snippets/remote/remote_suggestions_scheduler.h:83: UpdateSchedule GetLastUpdateSchedule(); const? https://codereview.chromium.org/2557363002/diff/60001/components/ntp_snippets... components/ntp_snippets/remote/remote_suggestions_scheduler.h:86: // Interface for scheduling hard fetches, OS dependent. Not owned. May be null. https://codereview.chromium.org/2557363002/diff/60001/components/ntp_snippets... components/ntp_snippets/remote/remote_suggestions_scheduler.h:92: // Interface for doing the actual updates, when they are due. Not owned. Update comment https://codereview.chromium.org/2557363002/diff/60001/components/ntp_snippets... File components/ntp_snippets/remote/remote_suggestions_scheduler_unittest.cc (right): https://codereview.chromium.org/2557363002/diff/60001/components/ntp_snippets... components/ntp_snippets/remote/remote_suggestions_scheduler_unittest.cc:1: // Copyright 2015 The Chromium Authors. All rights reserved. 2016 https://codereview.chromium.org/2557363002/diff/60001/components/ntp_snippets... components/ntp_snippets/remote/remote_suggestions_scheduler_unittest.cc:81: if (enabled) { Hm. Might be better to do this in the individual tests directly - WDYT? https://codereview.chromium.org/2557363002/diff/60001/components/ntp_snippets... components/ntp_snippets/remote/remote_suggestions_scheduler_unittest.cc:90: MockHardScheduler& mock_scheduler() { return hard_scheduler_; } Please rename, otherwise it's difficult to keep the two "schedulers" apart. (Per the other comment, maybe this class should be renamed anyway.) Non-const references are discouraged at best in Chromium, so maybe return pointers instead? https://codereview.chromium.org/2557363002/diff/60001/components/ntp_snippets... components/ntp_snippets/remote/remote_suggestions_scheduler_unittest.cc:110: EXPECT_CALL(mock_scheduler(), Schedule(_, _)).Times(1); Probably you mostly copy-pasted this code, but: Times(1) is the default, so no need to specify, unless it's really crucial to this test. https://codereview.chromium.org/2557363002/diff/60001/components/ntp_snippets... components/ntp_snippets/remote/remote_suggestions_scheduler_unittest.cc:113: Mock::VerifyAndClearExpectations(&mock_scheduler()); FYI: Tim considers VerifyAndClearExpectations an anti-pattern, though in this case it seems okay to me. I guess an alternative would be testing::InSequence, which would however provide slightly less strict guarantees here. https://codereview.chromium.org/2557363002/diff/60001/components/ntp_snippets... components/ntp_snippets/remote/remote_suggestions_scheduler_unittest.cc:124: scheduler.reset(); Not really necessary; it'll go out of scope anyway. Maybe worth a comment though? https://codereview.chromium.org/2557363002/diff/60001/components/ntp_snippets... components/ntp_snippets/remote/remote_suggestions_scheduler_unittest.cc:127: TEST_F(RemoteSuggestionsSchedulerTest, UnscheduleOnlyOnce) { nitty nit: UnschedulesOnlyOnce (add an "s") https://codereview.chromium.org/2557363002/diff/60001/components/ntp_snippets... components/ntp_snippets/remote/remote_suggestions_scheduler_unittest.cc:143: TEST_F(RemoteSuggestionsSchedulerTest, ScheduleOnlyOnce) { Also here: +s (and a few more below) https://codereview.chromium.org/2557363002/diff/60001/components/ntp_snippets... components/ntp_snippets/remote/remote_suggestions_scheduler_unittest.cc:214: // Simulate a succesfull update. nitty nit: successful (only one "l") https://codereview.chromium.org/2557363002/diff/60001/components/ntp_snippets... components/ntp_snippets/remote/remote_suggestions_scheduler_unittest.cc:217: scheduler->OnSuccessfulUpdate(); Why do we need both of these calls? OnSuccessfulUpdate should really be called by the updater, right? So maybe a Fake implementation of that would be the right choice here?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
tschumann@chromium.org changed reviewers: + tschumann@chromium.org
some bystander comments on overall design. https://codereview.chromium.org/2557363002/diff/60001/chrome/browser/android/... File chrome/browser/android/ntp/ntp_snippets_bridge.cc (right): https://codereview.chromium.org/2557363002/diff/60001/chrome/browser/android/... chrome/browser/android/ntp/ntp_snippets_bridge.cc:141: remote_suggestions_provider->scheduler()->PerformHardUpdate(); this is a smell. If this is a funtionality this provider supports, then the RemoteSuggestionsProvider should have get a method to support this and not reveal implementation details like the scheduler. https://codereview.chromium.org/2557363002/diff/60001/chrome/browser/android/... chrome/browser/android/ntp/ntp_snippets_bridge.cc:156: remote_suggestions_provider->scheduler()->Unschedule(); same here: just add a single Reschedule() function on the provider itself and don't expose the scheduler. https://codereview.chromium.org/2557363002/diff/60001/components/ntp_snippets... File components/ntp_snippets/remote/remote_suggestions_provider.cc (right): https://codereview.chromium.org/2557363002/diff/60001/components/ntp_snippets... components/ntp_snippets/remote/remote_suggestions_provider.cc:701: scheduler_.OnSuccessfulUpdate(); this has a smell, especially when reading the comment. If I understand the motivation correctly, the RemoteSuggestionsProvider shouldn't be involved in scheduling decisions anymore -- that's in contrast to the comment above. Why not just always signal the result of a fetch to the scheduler? I guess knowing about failed attempts might also be useful. For sure, it would make the provider simpler. Rename to OnFetchFinished()? That said, I think we should make the scheduled fetch properly asynchronous and thus simplify the scheduler's interface. Instead of requiring a specific call to the scheduler once the fetch is done, why not simply pass a done callback into the UpdateRemoteSuggestionsBySchedule() function and require it to be called when done? That's something we can move in a separate CL, but we should stop implemetning asynchronous actions with synchronous functions ;-) https://codereview.chromium.org/2557363002/diff/60001/components/ntp_snippets... File components/ntp_snippets/remote/remote_suggestions_provider.h (right): https://codereview.chromium.org/2557363002/diff/60001/components/ntp_snippets... components/ntp_snippets/remote/remote_suggestions_provider.h:64: public RemoteSuggestionsScheduler::Updater { can we try to not inherit / implement yet another interface ;-) ? Inheritance is a very tight coupling and makes it super hard to understand the relationships and interfaces (it also often muddies the 1 class - 1 purpose rule). This interface has only a single method, can't we register a callback instead? So simply change scheduler_.SetUpdater(this); to take a callback (and rename to something like OnUpdate() or SetUpdateCallback(). IMO, the ideal world would be that RemoteSuggestionsProvider only implements a single interface, namely ContentSuggestionsProvider (that's the only is-a relationship that seems justified). https://codereview.chromium.org/2557363002/diff/60001/components/ntp_snippets... components/ntp_snippets/remote/remote_suggestions_provider.h:112: RemoteSuggestionsScheduler* scheduler() { nit: who actually needs access to this scheduler? Seems like you're exposing implementation details which would be better hidden behind (possibly adapted) facade methods. https://codereview.chromium.org/2557363002/diff/60001/components/ntp_snippets... File components/ntp_snippets/remote/remote_suggestions_scheduler.cc (right): https://codereview.chromium.org/2557363002/diff/60001/components/ntp_snippets... components/ntp_snippets/remote/remote_suggestions_scheduler.cc:96: void RemoteSuggestionsScheduler::PerformHardUpdate() { i'm a bit confused. Does this now perform an update or simply schedule one? https://codereview.chromium.org/2557363002/diff/60001/components/ntp_snippets... components/ntp_snippets/remote/remote_suggestions_scheduler.cc:149: void RemoteSuggestionsScheduler::ResetUpdateScheduleFromNow( the 'fromNow' suffix seems superfluous. Actually, I find it super hard to figure out what this function does from just looking at it's code... https://codereview.chromium.org/2557363002/diff/60001/components/ntp_snippets... File components/ntp_snippets/remote/remote_suggestions_scheduler.h (right): https://codereview.chromium.org/2557363002/diff/60001/components/ntp_snippets... components/ntp_snippets/remote/remote_suggestions_scheduler.h:53: // the scheduler to work correctly. that's odd. I'd assume that if I don't call Schedule/Unschedule, i'd have it in the same state as after calling Unschedule. Can we fix this? https://codereview.chromium.org/2557363002/diff/60001/components/ntp_snippets... components/ntp_snippets/remote/remote_suggestions_scheduler.h:57: void Schedule(); without the comments, I would have guessed completely different semantics of Schedule()/Unschedule(). Rename them to StartScheduling() / StopScheduling()?
markusheintz@chromium.org changed reviewers: + markusheintz@chromium.org
https://codereview.chromium.org/2557363002/diff/60001/components/ntp_snippets... File components/ntp_snippets/remote/remote_suggestions_provider.cc (right): https://codereview.chromium.org/2557363002/diff/60001/components/ntp_snippets... components/ntp_snippets/remote/remote_suggestions_provider.cc:179: RemoteSuggestionsHardScheduler* hard_scheduler, Should we pass in the scheduler instead of the hard scheduler? I assume this would also make testing easier. We would need to build the scheduler externally then. https://codereview.chromium.org/2557363002/diff/60001/components/ntp_snippets... File components/ntp_snippets/remote/remote_suggestions_provider.h (right): https://codereview.chromium.org/2557363002/diff/60001/components/ntp_snippets... components/ntp_snippets/remote/remote_suggestions_provider.h:64: public RemoteSuggestionsScheduler::Updater { On 2016/12/09 17:27:28, tschumann wrote: > can we try to not inherit / implement yet another interface ;-) ? > Inheritance is a very tight coupling and makes it super hard to understand the > relationships and interfaces (it also often muddies the 1 class - 1 purpose > rule). > > This interface has only a single method, can't we register a callback instead? > So simply change scheduler_.SetUpdater(this); to take a callback (and rename to > something like OnUpdate() or SetUpdateCallback(). > > IMO, the ideal world would be that RemoteSuggestionsProvider only implements a > single interface, namely ContentSuggestionsProvider (that's the only is-a > relationship that seems justified). +1 https://codereview.chromium.org/2557363002/diff/60001/components/ntp_snippets... File components/ntp_snippets/remote/remote_suggestions_scheduler.cc (right): https://codereview.chromium.org/2557363002/diff/60001/components/ntp_snippets... components/ntp_snippets/remote/remote_suggestions_scheduler.cc:102: updater_->UpdateRemoteSuggestionsBySchedule(); On 2016/12/09 12:25:27, Marc Treib wrote: > Hm. So in practice, the OS-scheduled update gets here via > ContentSuggestionsService->remote_suggestions_provider()->scheduler(), and this > effectively calls back into the RemoteSuggestionsProvider. > Should we get rid of this roundtrip through here? +1 this hides a simple call and somehow implies that there needs some special work to be done for the hard updates. Which does not seem to be the case. https://codereview.chromium.org/2557363002/diff/60001/components/ntp_snippets... File components/ntp_snippets/remote/remote_suggestions_scheduler.h (right): https://codereview.chromium.org/2557363002/diff/60001/components/ntp_snippets... components/ntp_snippets/remote/remote_suggestions_scheduler.h:30: class Updater { I think we don't need this interface. Instead we can call the RemoteSuggestionsProvider directly. https://codereview.chromium.org/2557363002/diff/60001/components/ntp_snippets... components/ntp_snippets/remote/remote_suggestions_scheduler.h:57: void Schedule(); On 2016/12/09 17:27:28, tschumann wrote: > without the comments, I would have guessed completely different semantics of > Schedule()/Unschedule(). Rename them to StartScheduling() / StopScheduling()? I would actually rename them to "OnChromeStartup" because the Scheduler needs to do something on this particular "event/occasion". https://codereview.chromium.org/2557363002/diff/60001/components/ntp_snippets... components/ntp_snippets/remote/remote_suggestions_scheduler.h:66: void PerformHardUpdate(); I think this method should live somewhere else. It's not the responsibility of the scheduler to perform updates. https://codereview.chromium.org/2557363002/diff/60001/components/ntp_snippets... components/ntp_snippets/remote/remote_suggestions_scheduler.h:72: static void RegisterProfilePrefs(PrefRegistrySimple* registry); nit: I'm not sure whether we (inside the team) have agreed on a particular order in class files. I'm not used to having static methods at the top (of the public section).
On a second thought I guess the relation ship between the individual components is more tricky. I think we should distinguish more between callbacks that are call once as response to an async call and listener interfaces that are called for "event" that are emitted by a component. I used to over do the listener interface part. I think some components are simply tightly couple e.g. RemoteSuggestionsProvider and RemoteSuggestionsScheduler, that we may not need to introduce a listener interface. It should be ok for the RemoteSuggestionsScheduler to know the RemoteSuggestionsProvider. However I guess we still nee "On<EventName>" methods. We probably should discuss a good naming scheme to distinguisg these methods from methods that are passed as callbacks which could be "<AsyncMethodName>Finished". https://codereview.chromium.org/2557363002/diff/60001/components/ntp_snippets... File components/ntp_snippets/remote/remote_suggestions_provider.cc (right): https://codereview.chromium.org/2557363002/diff/60001/components/ntp_snippets... components/ntp_snippets/remote/remote_suggestions_provider.cc:191: user_classifier_(user_classifier), This is not used in this class any more. So we can get rid of it. https://codereview.chromium.org/2557363002/diff/60001/components/ntp_snippets... File components/ntp_snippets/remote/remote_suggestions_provider.h (right): https://codereview.chromium.org/2557363002/diff/60001/components/ntp_snippets... components/ntp_snippets/remote/remote_suggestions_provider.h:64: public RemoteSuggestionsScheduler::Updater { On 2016/12/09 17:27:28, tschumann wrote: > can we try to not inherit / implement yet another interface ;-) ? > Inheritance is a very tight coupling and makes it super hard to understand the > relationships and interfaces (it also often muddies the 1 class - 1 purpose > rule). > > This interface has only a single method, can't we register a callback instead? > So simply change scheduler_.SetUpdater(this); to take a callback (and rename to > something like OnUpdate() or SetUpdateCallback(). > > IMO, the ideal world would be that RemoteSuggestionsProvider only implements a > single interface, namely ContentSuggestionsProvider (that's the only is-a > relationship that seems justified). +1 I would even call the method that is then passed as callback to the scheduler "OnScheduledUpdate" because this is the "event" that the RemoteSuggestionsScheduler should "emit". https://codereview.chromium.org/2557363002/diff/60001/components/ntp_snippets... File components/ntp_snippets/remote/remote_suggestions_scheduler.h (right): https://codereview.chromium.org/2557363002/diff/60001/components/ntp_snippets... components/ntp_snippets/remote/remote_suggestions_scheduler.h:57: void Schedule(); On 2016/12/12 14:36:16, markusheintz_ wrote: > On 2016/12/09 17:27:28, tschumann wrote: > > without the comments, I would have guessed completely different semantics of > > Schedule()/Unschedule(). Rename them to StartScheduling() / StopScheduling()? > > I would actually rename them to "OnChromeStartup" because the Scheduler needs to > do something on this particular "event/occasion". > On a second though I guess this can be "OnEnterReadyState" or something similar. Because this is the "event" the scheduler reacts to. The "Unschedule" below could be "OnEnterDisableState" or something similar.
Tim, Marc, Markus,
[The CL is not complete, I did adapt fix unit-tests nor addressed your comments
to unit-tests. All other comments should be addressed.]
I've refactored the code once again I would like to get quick feedback before I
dive into the unit-tests. I need feedback mainly on
[scheduling_]remote_suggestions_provider.* but feel free to comment on anything
else.
Is this a promising direction?
The main change is that I've put SchedulingRemoteSuggestionsProvider as a layer
on top of current RemoteSuggestionsProvider. My issues:
- I do not like the amount of boilerplate code in
SchedulingRemoteSuggestionsProvider,
- RemoteSuggestionsProvider has a public function RefetchInTheBackground()
called solely by SchedulingRemoteSuggestionsProvider.
- RemoteSuggestionsProvider still initiates some fetches on its own and thus
the Scheduling layer does not know:
a) fetch on startup when there are no suggestions - to be removed when I
introduce soft fetches triggered by startup of Chrome
b) fetch on sign-in status change (we need to clear the cache and fetch
again) - this could be fixed by another refactoring - putting sign-in status
with the whole status_service in another layer on top of the scheduling. WDYT?
If we start gluing multiple layers, it would be nice to have between them a
clear interface:
RemoteSuggestionsProvider : public ContentSuggestionsProvider
Other changes:
- NtpSnippetsBridge now accesses stuff only via ContentSuggestionService and
via ntp_snippets::PersistentScheduler::Listener* registered at
ContentSuggestionService
- As a side effect, I needed to change the treatment of a weird UI case where
you dismiss all snippets and categories and then you press the refresh button on
the screen with the nice picture. Previously
RemoteSuggestionsProvider::FetchSnippetsForAllCategories() was called, now it
goes to ContentSuggestionsService::ReloadSuggestions() and from there to all
providers.
https://codereview.chromium.org/2557363002/diff/40001/components/ntp_snippets...
File components/ntp_snippets/remote/remote_suggestions_hard_scheduler.h (right):
https://codereview.chromium.org/2557363002/diff/40001/components/ntp_snippets...
components/ntp_snippets/remote/remote_suggestions_hard_scheduler.h:16: class
RemoteSuggestionsHardScheduler {
On 2016/12/09 12:25:26, Marc Treib wrote:
> I'm not very happy with this name. For one, "hard" is very clear. But more
> importantly, this name sounds like it's "next to" RemoteSuggestionsScheduler,
> maybe an alternative to it or something. But in fact, it's one level below,
and
> its responsibilities are very different (much more limited).
>
> But I'm also not sure what a good name would be :) Maybe something very
> explicit like SchedulingOSBridge? That's really all it is.
What about PersistentScheduler?
https://codereview.chromium.org/2557363002/diff/40001/components/ntp_snippets...
File components/ntp_snippets/remote/remote_suggestions_provider.h (right):
https://codereview.chromium.org/2557363002/diff/40001/components/ntp_snippets...
components/ntp_snippets/remote/remote_suggestions_provider.h:112:
RemoteSuggestionsScheduler* scheduler() {
On 2016/12/09 12:25:26, Marc Treib wrote:
> Comment on why this needs to be exposed?
Done.
https://codereview.chromium.org/2557363002/diff/60001/chrome/browser/android/...
File chrome/browser/android/ntp/ntp_snippets_bridge.cc (right):
https://codereview.chromium.org/2557363002/diff/60001/chrome/browser/android/...
chrome/browser/android/ntp/ntp_snippets_bridge.cc:141:
remote_suggestions_provider->scheduler()->PerformHardUpdate();
On 2016/12/09 17:27:28, tschumann wrote:
> this is a smell. If this is a funtionality this provider supports, then the
> RemoteSuggestionsProvider should have get a method to support this and not
> reveal implementation details like the scheduler.
Done.
https://codereview.chromium.org/2557363002/diff/60001/chrome/browser/android/...
chrome/browser/android/ntp/ntp_snippets_bridge.cc:156:
remote_suggestions_provider->scheduler()->Unschedule();
On 2016/12/09 17:27:28, tschumann wrote:
> same here: just add a single Reschedule() function on the provider itself and
> don't expose the scheduler.
Done.
https://codereview.chromium.org/2557363002/diff/60001/components/ntp_snippets...
File components/ntp_snippets/remote/remote_suggestions_provider.cc (left):
https://codereview.chromium.org/2557363002/diff/60001/components/ntp_snippets...
components/ntp_snippets/remote/remote_suggestions_provider.cc:418: if (state_ !=
State::NOT_INITED || force) {
On 2016/12/09 12:25:26, Marc Treib wrote:
> Is this case still handled in the new world?
It is.
- Unschedule() is not called on state change when not inited.
- forced is handled by calling Unschedule() and Schedule() in a sequence.
https://codereview.chromium.org/2557363002/diff/60001/components/ntp_snippets...
File components/ntp_snippets/remote/remote_suggestions_provider.cc (right):
https://codereview.chromium.org/2557363002/diff/60001/components/ntp_snippets...
components/ntp_snippets/remote/remote_suggestions_provider.cc:179:
RemoteSuggestionsHardScheduler* hard_scheduler,
On 2016/12/12 14:36:16, markusheintz_ wrote:
> Should we pass in the scheduler instead of the hard scheduler?
>
> I assume this would also make testing easier. We would need to build the
> scheduler externally then.
Done.
https://codereview.chromium.org/2557363002/diff/60001/components/ntp_snippets...
components/ntp_snippets/remote/remote_suggestions_provider.cc:191:
user_classifier_(user_classifier),
On 2016/12/12 15:32:28, markusheintz_ wrote:
> This is not used in this class any more. So we can get rid of it.
Done.
https://codereview.chromium.org/2557363002/diff/60001/components/ntp_snippets...
components/ntp_snippets/remote/remote_suggestions_provider.cc:194:
pref_service),
On 2016/12/09 12:25:26, Marc Treib wrote:
> I think this fits on one line?
>
> Maybe for a later CL: Would we want to inject the whole scheduler? Might make
> testing easier.
Done.
https://codereview.chromium.org/2557363002/diff/60001/components/ntp_snippets...
components/ntp_snippets/remote/remote_suggestions_provider.cc:701:
scheduler_.OnSuccessfulUpdate();
On 2016/12/09 17:27:28, tschumann wrote:
> this has a smell, especially when reading the comment. If I understand the
> motivation correctly, the RemoteSuggestionsProvider shouldn't be involved in
> scheduling decisions anymore -- that's in contrast to the comment above.
> Why not just always signal the result of a fetch to the scheduler? I guess
> knowing about failed attempts might also be useful.
>
> For sure, it would make the provider simpler. Rename to OnFetchFinished()?
>
> That said, I think we should make the scheduled fetch properly asynchronous
and
> thus simplify the scheduler's interface. Instead of requiring a specific call
to
> the scheduler once the fetch is done, why not simply pass a done callback into
> the UpdateRemoteSuggestionsBySchedule() function and require it to be called
> when done? That's something we can move in a separate CL, but we should stop
> implemetning asynchronous actions with synchronous functions ;-)
Done.
https://codereview.chromium.org/2557363002/diff/60001/components/ntp_snippets...
File components/ntp_snippets/remote/remote_suggestions_provider.h (right):
https://codereview.chromium.org/2557363002/diff/60001/components/ntp_snippets...
components/ntp_snippets/remote/remote_suggestions_provider.h:64: public
RemoteSuggestionsScheduler::Updater {
On 2016/12/12 15:32:28, markusheintz_ wrote:
> On 2016/12/09 17:27:28, tschumann wrote:
> > can we try to not inherit / implement yet another interface ;-) ?
> > Inheritance is a very tight coupling and makes it super hard to understand
the
> > relationships and interfaces (it also often muddies the 1 class - 1 purpose
> > rule).
> >
> > This interface has only a single method, can't we register a callback
instead?
> > So simply change scheduler_.SetUpdater(this); to take a callback (and rename
> to
> > something like OnUpdate() or SetUpdateCallback().
> >
> > IMO, the ideal world would be that RemoteSuggestionsProvider only implements
a
> > single interface, namely ContentSuggestionsProvider (that's the only is-a
> > relationship that seems justified).
>
> +1
>
> I would even call the method that is then passed as callback to the scheduler
> "OnScheduledUpdate" because this is the "event" that the
> RemoteSuggestionsScheduler should "emit".
Done.
https://codereview.chromium.org/2557363002/diff/60001/components/ntp_snippets...
components/ntp_snippets/remote/remote_suggestions_provider.h:112:
RemoteSuggestionsScheduler* scheduler() {
On 2016/12/09 17:27:28, tschumann wrote:
> nit: who actually needs access to this scheduler? Seems like you're exposing
> implementation details which would be better hidden behind (possibly adapted)
> facade methods.
Done.
https://codereview.chromium.org/2557363002/diff/60001/components/ntp_snippets...
File components/ntp_snippets/remote/remote_suggestions_scheduler.cc (right):
https://codereview.chromium.org/2557363002/diff/60001/components/ntp_snippets...
components/ntp_snippets/remote/remote_suggestions_scheduler.cc:43: const char*
param_name = "";
On 2016/12/09 12:25:27, Marc Treib wrote:
> nullptr? The "empty" case can't happen anyway
Done.
https://codereview.chromium.org/2557363002/diff/60001/components/ntp_snippets...
components/ntp_snippets/remote/remote_suggestions_scheduler.cc:84:
DCHECK(updater) << "non-null Updater must be provided";
On 2016/12/09 12:25:27, Marc Treib wrote:
> Since we have to handle a null updater_ anyway, maybe this isn't needed?
Done.
https://codereview.chromium.org/2557363002/diff/60001/components/ntp_snippets...
components/ntp_snippets/remote/remote_suggestions_scheduler.cc:96: void
RemoteSuggestionsScheduler::PerformHardUpdate() {
On 2016/12/09 17:27:28, tschumann wrote:
> i'm a bit confused. Does this now perform an update or simply schedule one?
It performs it. Is is clearer now in the new design? (OnFetchDue())
https://codereview.chromium.org/2557363002/diff/60001/components/ntp_snippets...
components/ntp_snippets/remote/remote_suggestions_scheduler.cc:102:
updater_->UpdateRemoteSuggestionsBySchedule();
On 2016/12/09 12:25:27, Marc Treib wrote:
> Hm. So in practice, the OS-scheduled update gets here via
> ContentSuggestionsService->remote_suggestions_provider()->scheduler(), and
this
> effectively calls back into the RemoteSuggestionsProvider.
> Should we get rid of this roundtrip through here?
Done.
https://codereview.chromium.org/2557363002/diff/60001/components/ntp_snippets...
components/ntp_snippets/remote/remote_suggestions_scheduler.cc:116:
last_schedule.interval_fallback != schedule.interval_fallback) {
On 2016/12/09 12:25:26, Marc Treib wrote:
> optional: Could put operator==/!= on the struct? IMO that'd improve changes
that
> this will be updated when a member is added.
Done.
https://codereview.chromium.org/2557363002/diff/60001/components/ntp_snippets...
components/ntp_snippets/remote/remote_suggestions_scheduler.cc:130:
last_schedule.interval_fallback.is_zero()) {
On 2016/12/09 12:25:27, Marc Treib wrote:
> Similar here, could put an accessor on the struct.
> Also, you probably don't want to check interval_fallback twice :) (Should we
> have a test to catch this?)
Done.
https://codereview.chromium.org/2557363002/diff/60001/components/ntp_snippets...
components/ntp_snippets/remote/remote_suggestions_scheduler.cc:137:
base::TimeDelta::FromHours(0)});
On 2016/12/09 12:25:26, Marc Treib wrote:
> Maybe make an UpdateSchedule::Empty() ?
Done.
https://codereview.chromium.org/2557363002/diff/60001/components/ntp_snippets...
components/ntp_snippets/remote/remote_suggestions_scheduler.cc:149: void
RemoteSuggestionsScheduler::ResetUpdateScheduleFromNow(
On 2016/12/09 17:27:28, tschumann wrote:
> the 'fromNow' suffix seems superfluous.
> Actually, I find it super hard to figure out what this function does from just
> looking at it's code...
Renamed to ApplyFetchingSchedule(). WDYT?
https://codereview.chromium.org/2557363002/diff/60001/components/ntp_snippets...
components/ntp_snippets/remote/remote_suggestions_scheduler.cc:157:
RemoteSuggestionsScheduler::GetCurrentUpdateSchedule() {
On 2016/12/09 12:25:27, Marc Treib wrote:
> Hm, IMO this name is a bit misleading. I'd interpret it as "give me the values
> that are currently scheduled".
Renamed to GetDesiredUpdateSchedule(). Better?
https://codereview.chromium.org/2557363002/diff/60001/components/ntp_snippets...
File components/ntp_snippets/remote/remote_suggestions_scheduler.h (right):
https://codereview.chromium.org/2557363002/diff/60001/components/ntp_snippets...
components/ntp_snippets/remote/remote_suggestions_scheduler.h:30: class Updater
{
On 2016/12/12 14:36:16, markusheintz_ wrote:
> I think we don't need this interface. Instead we can call the
> RemoteSuggestionsProvider directly.
I do not want to couple the scheduler with the provider. I'll do it via a
registered callback, as Tim suggests.
https://codereview.chromium.org/2557363002/diff/60001/components/ntp_snippets...
components/ntp_snippets/remote/remote_suggestions_scheduler.h:40: explicit
RemoteSuggestionsScheduler(
On 2016/12/09 12:25:27, Marc Treib wrote:
> "explicit" not needed
Done.
https://codereview.chromium.org/2557363002/diff/60001/components/ntp_snippets...
components/ntp_snippets/remote/remote_suggestions_scheduler.h:49: // is skipped.
On 2016/12/09 12:25:27, Marc Treib wrote:
> Document the lifetime of |updater|: Must outlive us.
Done.
https://codereview.chromium.org/2557363002/diff/60001/components/ntp_snippets...
components/ntp_snippets/remote/remote_suggestions_scheduler.h:53: // the
scheduler to work correctly.
On 2016/12/09 17:27:28, tschumann wrote:
> that's odd. I'd assume that if I don't call Schedule/Unschedule, i'd have it
in
> the same state as after calling Unschedule. Can we fix this?
I am not sure I understand. Maybe not relevant any more?
https://codereview.chromium.org/2557363002/diff/60001/components/ntp_snippets...
components/ntp_snippets/remote/remote_suggestions_scheduler.h:57: void
Schedule();
On 2016/12/09 17:27:28, tschumann wrote:
> without the comments, I would have guessed completely different semantics of
> Schedule()/Unschedule(). Rename them to StartScheduling() / StopScheduling()?
I like Start/StopScheduling quite a lot.
https://codereview.chromium.org/2557363002/diff/60001/components/ntp_snippets...
components/ntp_snippets/remote/remote_suggestions_scheduler.h:66: void
PerformHardUpdate();
On 2016/12/12 14:36:16, markusheintz_ wrote:
> I think this method should live somewhere else.
>
> It's not the responsibility of the scheduler to perform updates.
Actually, it stayed in the scheduling layer. Do you still oppose it?
https://codereview.chromium.org/2557363002/diff/60001/components/ntp_snippets...
components/ntp_snippets/remote/remote_suggestions_scheduler.h:72: static void
RegisterProfilePrefs(PrefRegistrySimple* registry);
On 2016/12/12 14:36:16, markusheintz_ wrote:
> nit: I'm not sure whether we (inside the team) have agreed on a particular
order
> in class files.
>
> I'm not used to having static methods at the top (of the public section).
Did you mean that you _are_ used to have them at the top?
Anyway, done.
https://codereview.chromium.org/2557363002/diff/60001/components/ntp_snippets...
components/ntp_snippets/remote/remote_suggestions_scheduler.h:83: UpdateSchedule
GetLastUpdateSchedule();
On 2016/12/09 12:25:27, Marc Treib wrote:
> const?
Done.
https://codereview.chromium.org/2557363002/diff/60001/components/ntp_snippets...
components/ntp_snippets/remote/remote_suggestions_scheduler.h:86: // Interface
for scheduling hard fetches, OS dependent. Not owned.
On 2016/12/09 12:25:27, Marc Treib wrote:
> May be null.
Done.
https://codereview.chromium.org/2557363002/diff/60001/components/ntp_snippets...
components/ntp_snippets/remote/remote_suggestions_scheduler.h:92: // Interface
for doing the actual updates, when they are due. Not owned.
On 2016/12/09 12:25:27, Marc Treib wrote:
> Update comment
Done.
On 2016/12/14 09:42:10, jkrcal wrote: > The main change is that I've put SchedulingRemoteSuggestionsProvider as a layer > on top of current RemoteSuggestionsProvider. My issues: > - I do not like the amount of boilerplate code in > SchedulingRemoteSuggestionsProvider, Certainly not great, but IMO tolerable. I don't see a better alternative. > - RemoteSuggestionsProvider has a public function RefetchInTheBackground() > called solely by SchedulingRemoteSuggestionsProvider. IMO that's okay, at least for now. If we ever have another client for the SchedulingProvider, then we can extract an interface here. > - RemoteSuggestionsProvider still initiates some fetches on its own and thus > the Scheduling layer does not know: > a) fetch on startup when there are no suggestions - to be removed when I > introduce soft fetches triggered by startup of Chrome IMO fine for now; it'll be fixed at exactly the point where it'll start to matter. > b) fetch on sign-in status change (we need to clear the cache and fetch > again) - this could be fixed by another refactoring - putting sign-in status > with the whole status_service in another layer on top of the scheduling. WDYT? Hm, this is tricky. The one wrapping layer is already not great, having to add yet another one is a bad sign IMO. I don't have a good idea offhand. Time for another whiteboard session? > If we start gluing multiple layers, it would be nice to have between them a > clear interface: > RemoteSuggestionsProvider : public ContentSuggestionsProvider > > > Other changes: > - NtpSnippetsBridge now accesses stuff only via ContentSuggestionService and > via ntp_snippets::PersistentScheduler::Listener* registered at > ContentSuggestionService Nice! > - As a side effect, I needed to change the treatment of a weird UI case where > you dismiss all snippets and categories and then you press the refresh button on > the screen with the nice picture. Previously > RemoteSuggestionsProvider::FetchSnippetsForAllCategories() was called, now it > goes to ContentSuggestionsService::ReloadSuggestions() and from there to all > providers. As discussed, SGTM.
https://codereview.chromium.org/2557363002/diff/40001/components/ntp_snippets... File components/ntp_snippets/remote/remote_suggestions_hard_scheduler.h (right): https://codereview.chromium.org/2557363002/diff/40001/components/ntp_snippets... components/ntp_snippets/remote/remote_suggestions_hard_scheduler.h:16: class RemoteSuggestionsHardScheduler { On 2016/12/14 09:42:09, jkrcal wrote: > On 2016/12/09 12:25:26, Marc Treib wrote: > > I'm not very happy with this name. For one, "hard" is very clear. But more > > importantly, this name sounds like it's "next to" RemoteSuggestionsScheduler, > > maybe an alternative to it or something. But in fact, it's one level below, > and > > its responsibilities are very different (much more limited). > > > > But I'm also not sure what a good name would be :) Maybe something very > > explicit like SchedulingOSBridge? That's really all it is. > > What about PersistentScheduler? Hm. Certainly better than "hard", and since the new "scheduler" isn't actually called "scheduler" anymore in the new design, I guess there's no risk of confusion. So sure, SGTM! https://codereview.chromium.org/2557363002/diff/60001/components/ntp_snippets... File components/ntp_snippets/remote/remote_suggestions_scheduler.cc (right): https://codereview.chromium.org/2557363002/diff/60001/components/ntp_snippets... components/ntp_snippets/remote/remote_suggestions_scheduler.cc:157: RemoteSuggestionsScheduler::GetCurrentUpdateSchedule() { On 2016/12/14 09:42:10, jkrcal wrote: > On 2016/12/09 12:25:27, Marc Treib wrote: > > Hm, IMO this name is a bit misleading. I'd interpret it as "give me the values > > that are currently scheduled". > > Renamed to GetDesiredUpdateSchedule(). Better? Yup, thanks!
On 2016/12/14 09:42:10, jkrcal wrote: > Tim, Marc, Markus, > > [The CL is not complete, I did adapt fix unit-tests nor addressed your comments > to unit-tests. All other comments should be addressed.] > > I've refactored the code once again I would like to get quick feedback before I > dive into the unit-tests. I need feedback mainly on > [scheduling_]remote_suggestions_provider.* but feel free to comment on anything > else. > > Is this a promising direction? > > > The main change is that I've put SchedulingRemoteSuggestionsProvider as a layer > on top of current RemoteSuggestionsProvider. My issues: > - I do not like the amount of boilerplate code in > SchedulingRemoteSuggestionsProvider, > - RemoteSuggestionsProvider has a public function RefetchInTheBackground() > called solely by SchedulingRemoteSuggestionsProvider. > - RemoteSuggestionsProvider still initiates some fetches on its own and thus > the Scheduling layer does not know: > a) fetch on startup when there are no suggestions - to be removed when I > introduce soft fetches triggered by startup of Chrome I think this can go into the Scheduling layer. Similar to the soft fetches, Startup is one of the events that triggers scheduling a fetch. > b) fetch on sign-in status change (we need to clear the cache and fetch > again) - this could be fixed by another refactoring - putting sign-in status > with the whole status_service in another layer on top of the scheduling. WDYT? I think this could also go into the scheduling layer for the same reason as above. Sign-in is yet another event that triggers scheduling a fetch (even if it it an immediate fetch) > If we start gluing multiple layers, it would be nice to have between them a > clear interface: > RemoteSuggestionsProvider : public ContentSuggestionsProvider > > > Other changes: > - NtpSnippetsBridge now accesses stuff only via ContentSuggestionService and > via ntp_snippets::PersistentScheduler::Listener* registered at > ContentSuggestionService > - As a side effect, I needed to change the treatment of a weird UI case where > you dismiss all snippets and categories and then you press the refresh button on > the screen with the nice picture. Previously > RemoteSuggestionsProvider::FetchSnippetsForAllCategories() was called, now it > goes to ContentSuggestionsService::ReloadSuggestions() and from there to all > providers. > > https://codereview.chromium.org/2557363002/diff/40001/components/ntp_snippets... > File components/ntp_snippets/remote/remote_suggestions_hard_scheduler.h (right): > > https://codereview.chromium.org/2557363002/diff/40001/components/ntp_snippets... > components/ntp_snippets/remote/remote_suggestions_hard_scheduler.h:16: class > RemoteSuggestionsHardScheduler { > On 2016/12/09 12:25:26, Marc Treib wrote: > > I'm not very happy with this name. For one, "hard" is very clear. But more > > importantly, this name sounds like it's "next to" RemoteSuggestionsScheduler, > > maybe an alternative to it or something. But in fact, it's one level below, > and > > its responsibilities are very different (much more limited). > > > > But I'm also not sure what a good name would be :) Maybe something very > > explicit like SchedulingOSBridge? That's really all it is. > > What about PersistentScheduler? I could also imagine naming it: A) {System|Platform}Scheduler as it user the underlying Platform APIs B) FixedIntervalScheduler which refers to what it tries to do
https://codereview.chromium.org/2557363002/diff/140001/chrome/browser/android... File chrome/browser/android/ntp/ntp_snippets_launcher.h (right): https://codereview.chromium.org/2557363002/diff/140001/chrome/browser/android... chrome/browser/android/ntp/ntp_snippets_launcher.h:18: : public ntp_snippets::PersistentScheduler { sorry for joining the naming discussion so late. Will there be further implementations of the PersistentScheduler interface? Would the soft schedules also happen through an implementation of this? I think what we really have here is a PeriodicTask. IMO, we are still mixing up the concepts a bit about what are signals that influence the fetch-scheduling and what is part of the actual scheduler. (see further comments on PersistentScheduler::Listener). I also see that we need to make progress here and volunteer to help with further renamings. But that would be a topic I'd quickly like to discuss first, and then see what's the quickest way to get us there. https://codereview.chromium.org/2557363002/diff/140001/components/ntp_snippet... File components/ntp_snippets/remote/persistent_scheduler.h (right): https://codereview.chromium.org/2557363002/diff/140001/components/ntp_snippet... components/ntp_snippets/remote/persistent_scheduler.h:19: class Listener { this feels like the wrong place to implement this interface. In my mind, the Android system scheduler and possibly the tbd soft scheduler are like clock generators (that's why I'd call it PerodicTask or the like). In fact, they are not scheduling a thing -- the NTPSnippetsLauncher *gets* scheduled by the system and it triggers a fetch. What we want to introduce is a smarter fetch logic so that it only maybe triggers a fetch -- and that logic is in SchedulingRemoteSuggestionsProvider which actually maintains the schedule (and hence is "Scheduling"). That said, I'd move the Listener interfac either into scheduling_remote_suggestions_provider.h or content_suggestions_service.h We can call it RemoteSuggestionsScheduler. I think for now a simply "DoFetch()" would be sufficient -- later we might introduce more ones to better distinguish between optional fetches etc. The RescheduleFetching() seems odd -- in the case of the NTPSnippetsLauncher, it would ultimately result in a call on the NTPSnippetsLauncher which feels like a weird round-trip. Can we just leave this one out and make sure the SchedulingRemoteSuggestionsProvider has access to the scheduler?
some more comments on the other code. some might be obsolete, but I wanted to have a look at the larger picture to avoid late feedback. https://codereview.chromium.org/2557363002/diff/140001/components/ntp_snippet... File components/ntp_snippets/content_suggestions_provider.h (right): https://codereview.chromium.org/2557363002/diff/140001/components/ntp_snippet... components/ntp_snippets/content_suggestions_provider.h:107: // Reloads suggestions from all categories. Makes sense to call only after Can we find a better name for this method? To me it conveys pretty much no semantics. I find this comment confusing, why doesn't it make sense to call if no cards are dismissed? It would still fetch new ones, right? Isn't this just a FetchForAllCategories() functionality? https://codereview.chromium.org/2557363002/diff/140001/components/ntp_snippet... components/ntp_snippets/content_suggestions_provider.h:112: virtual void ReloadSuggestions() {} hm... we should make this pure virtual -- add a TODO()? https://codereview.chromium.org/2557363002/diff/140001/components/ntp_snippet... File components/ntp_snippets/content_suggestions_service.h (right): https://codereview.chromium.org/2557363002/diff/140001/components/ntp_snippet... components/ntp_snippets/content_suggestions_service.h:150: // Reloads suggestions from all categories. Makes sense to call only after what does "reload" mean? Would this include fetching new suggestions? Or is this for restoring dismissed suggestions? Isn't this just a FetchAll() functionality? https://codereview.chromium.org/2557363002/diff/140001/components/ntp_snippet... components/ntp_snippets/content_suggestions_service.h:207: void set_remote_suggestions_provider( please add a TODO() to remove this setter. These instances should be injected into the constructor. Not sure which debugging use cases would justify changing those internals. I see that right now, we have a cyclic dependency as the service gets injected as an observer into the RemoteSuggestionProvider. IMO, it would be more appropriate to hand the RemoteSuggetsionsProvider into the ContentSuggestionsService's constructor and have a setter on the RemoteSuggestionsProvider to set the observer. But like I said, that's a TODO(). https://codereview.chromium.org/2557363002/diff/140001/components/ntp_snippet... File components/ntp_snippets/remote/remote_suggestions_provider.h (right): https://codereview.chromium.org/2557363002/diff/140001/components/ntp_snippet... components/ntp_snippets/remote/remote_suggestions_provider.h:93: void RegisterActivenessObserver(const ProviderActiveCallback& callback); naming is hard. Maybe we change this to rename the callback to ProviderStatusCallback() and probably introduce an enum for readability: enum ProviderStatus { ACTIVE, INACTIVE } or ENABLED, DISABLED, whatever. then we can call this: SetProviderStatusListener or SetProviderStatusCallback(). As we only store a single callback, we should call it Set and not Register. How do we distinguish if such a callback was provided? How does the default constructed callback react if we call it? I guess it's best to make this function take a unique_ptr<> and store the unique ptr so that we can a) check against nullptr b) clients can unregister themselves before they get deleted (we don't want callbacks into deleted objects). https://codereview.chromium.org/2557363002/diff/140001/components/ntp_snippet... File components/ntp_snippets/remote/scheduling_remote_suggestions_provider.cc (right): https://codereview.chromium.org/2557363002/diff/140001/components/ntp_snippet... components/ntp_snippets/remote/scheduling_remote_suggestions_provider.cc:85: provider->RegisterActivenessObserver(base::BindRepeating( doing this in the constructor is tricky -- we might get called back while still in the constructor which is an issue in case this class gets subclassed. I believe the RemoteSuggestionsProvider has the same issue. Let's add a todo and mark this class final for now. (a proper way would be to have the constructor private and a static Create() function which first constructs the object and then calls an init() function.) https://codereview.chromium.org/2557363002/diff/140001/components/ntp_snippet... components/ntp_snippets/remote/scheduling_remote_suggestions_provider.cc:262: // Intercept |status_code| before calling the provided |callback|. no need for the comment -- it's quite obvious :-) https://codereview.chromium.org/2557363002/diff/140001/components/ntp_snippet... File components/ntp_snippets/remote/scheduling_remote_suggestions_provider.h (right): https://codereview.chromium.org/2557363002/diff/140001/components/ntp_snippet... components/ntp_snippets/remote/scheduling_remote_suggestions_provider.h:92: struct FetchingSchedule { let's move this into the .cc file and just forward declare here, to keep the header short. https://codereview.chromium.org/2557363002/diff/140001/components/ntp_snippet... File components/ntp_snippets/remote/scheduling_remote_suggestions_provider_unittest.cc (right): https://codereview.chromium.org/2557363002/diff/140001/components/ntp_snippet... components/ntp_snippets/remote/scheduling_remote_suggestions_provider_unittest.cc:79: scheduler->SetUpdater(&updater()); &updater_ https://codereview.chromium.org/2557363002/diff/140001/components/ntp_snippet... components/ntp_snippets/remote/scheduling_remote_suggestions_provider_unittest.cc:81: if (enabled) { let's not do this inside the MakeScheduler() function. Less convenience, less magic, easier to read the tests. https://codereview.chromium.org/2557363002/diff/140001/components/ntp_snippet... components/ntp_snippets/remote/scheduling_remote_suggestions_provider_unittest.cc:90: MockHardScheduler& mock_scheduler() { return hard_scheduler_; } i know that we followed this pattern in other tests, but honestly, I don't see a lot of reason in this. The test fixture should stay simple and there shouldn't be a need to artificially encapsulate these members. Just make them all protected and remove the accessors -- they don't add value IMO. https://codereview.chromium.org/2557363002/diff/140001/components/ntp_snippet... components/ntp_snippets/remote/scheduling_remote_suggestions_provider_unittest.cc:96: NiceMock<MockHardScheduler> hard_scheduler_; are you sure you want them to be nice? From my experience, you should default to StrictMocks. https://codereview.chromium.org/2557363002/diff/140001/components/ntp_snippet... components/ntp_snippets/remote/scheduling_remote_suggestions_provider_unittest.cc:103: EXPECT_CALL(mock_scheduler(), Schedule(_, _)).Times(1); .Times(1) is implicit. https://codereview.chromium.org/2557363002/diff/140001/components/ntp_snippet... components/ntp_snippets/remote/scheduling_remote_suggestions_provider_unittest.cc:104: EXPECT_CALL(mock_scheduler(), Unschedule()).Times(0); with a strict-mock that's the default behavior (it fails if it was called unexpectedly). I'd argue that that's the better choice.
PTAL, again. When diving into unit-tests, I realized that I cannot mock the RemoteSuggestionsProvider to pass to mock into SchedulingRemoteSuggestionsProvider. So I've split RemoteSuggestionsProvider from RemoteSuggestionsProviderImpl. Externally, only the RemoteSuggestionsProvider interface is used. The unit-tests are still not complete (and not all comments are addressed). I will try to finish the unit-tests ASAP. https://codereview.chromium.org/2557363002/diff/60001/components/ntp_snippets... File components/ntp_snippets/remote/remote_suggestions_provider_unittest.cc (left): https://codereview.chromium.org/2557363002/diff/60001/components/ntp_snippets... components/ntp_snippets/remote/remote_suggestions_provider_unittest.cc:557: TEST_F(RemoteSuggestionsProviderTest, ScheduleOnStart) { On 2016/12/09 12:25:26, Marc Treib wrote: > I think some of the removed tests kinda still apply - what's the new behavior of > RemoteSuggestionsProvider vs the scheduler? > I think injecting a mock scheduler (the full thing, not just the "hard" one) > might be good, then we could test that the provider calls the right things at > the right time. Not relevant any more. I've added some more tests for notifying status changes. https://codereview.chromium.org/2557363002/diff/60001/components/ntp_snippets... File components/ntp_snippets/remote/remote_suggestions_scheduler_unittest.cc (right): https://codereview.chromium.org/2557363002/diff/60001/components/ntp_snippets... components/ntp_snippets/remote/remote_suggestions_scheduler_unittest.cc:1: // Copyright 2015 The Chromium Authors. All rights reserved. On 2016/12/09 12:25:27, Marc Treib wrote: > 2016 Done. https://codereview.chromium.org/2557363002/diff/60001/components/ntp_snippets... components/ntp_snippets/remote/remote_suggestions_scheduler_unittest.cc:90: MockHardScheduler& mock_scheduler() { return hard_scheduler_; } On 2016/12/09 12:25:27, Marc Treib wrote: > Please rename, otherwise it's difficult to keep the two "schedulers" apart. (Per > the other comment, maybe this class should be renamed anyway.) > > Non-const references are discouraged at best in Chromium, so maybe return > pointers instead? Done. https://codereview.chromium.org/2557363002/diff/60001/components/ntp_snippets... components/ntp_snippets/remote/remote_suggestions_scheduler_unittest.cc:110: EXPECT_CALL(mock_scheduler(), Schedule(_, _)).Times(1); On 2016/12/09 12:25:27, Marc Treib wrote: > Probably you mostly copy-pasted this code, but: Times(1) is the default, so no > need to specify, unless it's really crucial to this test. Done. https://codereview.chromium.org/2557363002/diff/60001/components/ntp_snippets... components/ntp_snippets/remote/remote_suggestions_scheduler_unittest.cc:124: scheduler.reset(); On 2016/12/09 12:25:27, Marc Treib wrote: > Not really necessary; it'll go out of scope anyway. Maybe worth a comment > though? Done. https://codereview.chromium.org/2557363002/diff/60001/components/ntp_snippets... components/ntp_snippets/remote/remote_suggestions_scheduler_unittest.cc:127: TEST_F(RemoteSuggestionsSchedulerTest, UnscheduleOnlyOnce) { On 2016/12/09 12:25:27, Marc Treib wrote: > nitty nit: UnschedulesOnlyOnce (add an "s") Done. https://codereview.chromium.org/2557363002/diff/60001/components/ntp_snippets... components/ntp_snippets/remote/remote_suggestions_scheduler_unittest.cc:143: TEST_F(RemoteSuggestionsSchedulerTest, ScheduleOnlyOnce) { On 2016/12/09 12:25:27, Marc Treib wrote: > Also here: +s (and a few more below) Done. https://codereview.chromium.org/2557363002/diff/60001/components/ntp_snippets... components/ntp_snippets/remote/remote_suggestions_scheduler_unittest.cc:214: // Simulate a succesfull update. On 2016/12/09 12:25:27, Marc Treib wrote: > nitty nit: successful (only one "l") Done. https://codereview.chromium.org/2557363002/diff/60001/components/ntp_snippets... components/ntp_snippets/remote/remote_suggestions_scheduler_unittest.cc:217: scheduler->OnSuccessfulUpdate(); On 2016/12/09 12:25:27, Marc Treib wrote: > Why do we need both of these calls? > OnSuccessfulUpdate should really be called by the updater, right? So maybe a > Fake implementation of that would be the right choice here? Done. https://codereview.chromium.org/2557363002/diff/140001/chrome/browser/android... File chrome/browser/android/ntp/ntp_snippets_launcher.h (right): https://codereview.chromium.org/2557363002/diff/140001/chrome/browser/android... chrome/browser/android/ntp/ntp_snippets_launcher.h:18: : public ntp_snippets::PersistentScheduler { On 2016/12/15 14:12:11, tschumann wrote: > sorry for joining the naming discussion so late. > Will there be further implementations of the PersistentScheduler interface? > > Would the soft schedules also happen through an implementation of this? > I think what we really have here is a PeriodicTask. IMO, we are still mixing up > the concepts a bit about what are signals that influence the fetch-scheduling > and what is part of the actual scheduler. > > (see further comments on PersistentScheduler::Listener). > > I also see that we need to make progress here and volunteer to help with further > renamings. But that would be a topic I'd quickly like to discuss first, and then > see what's the quickest way to get us there. No, this is meant just for the OS-dependent scheduler for persistent periodic tasks. Based on an offline discussion, kept as it is. https://codereview.chromium.org/2557363002/diff/140001/components/ntp_snippet... File components/ntp_snippets/content_suggestions_provider.h (right): https://codereview.chromium.org/2557363002/diff/140001/components/ntp_snippet... components/ntp_snippets/content_suggestions_provider.h:107: // Reloads suggestions from all categories. Makes sense to call only after On 2016/12/15 19:27:00, tschumann wrote: > Can we find a better name for this method? To me it conveys pretty much no > semantics. > I find this comment confusing, why doesn't it make sense to call if no cards are > dismissed? It would still fetch new ones, right? > > Isn't this just a FetchForAllCategories() functionality? Removed the comment. As regards naming, this is a mess :) It is kind-of FetchForAllCategories but it has a different API so I did not want to name the function analogously. Fetch has fetch-and-return-by-callback semantics (and has not a clear semantics about whether the additional suggestions should be cached in the provider). This is fetch-for-all-and-notify-the-observer semantics. Markus is about to clean things up a bit so this function may become redundant and get renamed. Do you feel strongly about some name? https://codereview.chromium.org/2557363002/diff/140001/components/ntp_snippet... components/ntp_snippets/content_suggestions_provider.h:112: virtual void ReloadSuggestions() {} On 2016/12/15 19:27:00, tschumann wrote: > hm... we should make this pure virtual -- add a TODO()? Done. https://codereview.chromium.org/2557363002/diff/140001/components/ntp_snippet... File components/ntp_snippets/content_suggestions_service.h (right): https://codereview.chromium.org/2557363002/diff/140001/components/ntp_snippet... components/ntp_snippets/content_suggestions_service.h:150: // Reloads suggestions from all categories. Makes sense to call only after On 2016/12/15 19:27:00, tschumann wrote: > what does "reload" mean? Would this include fetching new suggestions? Or is this > for restoring dismissed suggestions? > > Isn't this just a FetchAll() functionality? I've rewritten the comment. Hope it is clearer now. The FetchAll naming is discussed above. Nevertheless, I am open to renaming suggestions. https://codereview.chromium.org/2557363002/diff/140001/components/ntp_snippet... components/ntp_snippets/content_suggestions_service.h:207: void set_remote_suggestions_provider( On 2016/12/15 19:27:00, tschumann wrote: > please add a TODO() to remove this setter. These instances should be injected > into the constructor. Not sure which debugging use cases would justify changing > those internals. > > I see that right now, we have a cyclic dependency as the service gets injected > as an observer into the RemoteSuggestionProvider. IMO, it would be more > appropriate to hand the RemoteSuggetsionsProvider into the > ContentSuggestionsService's constructor and have a setter on the > RemoteSuggestionsProvider to set the observer. But like I said, that's a TODO(). Yes, there is a cyclic dependency. Adding a setter for the observer actually influences all providers whereas this setter influences just one provider. Why do you feel the former way is better? (maybe better to discuss offline) https://codereview.chromium.org/2557363002/diff/140001/components/ntp_snippet... File components/ntp_snippets/remote/persistent_scheduler.h (right): https://codereview.chromium.org/2557363002/diff/140001/components/ntp_snippet... components/ntp_snippets/remote/persistent_scheduler.h:19: class Listener { On 2016/12/15 14:12:11, tschumann wrote: > this feels like the wrong place to implement this interface. > In my mind, the Android system scheduler and possibly the tbd soft scheduler are > like clock generators (that's why I'd call it PerodicTask or the like). In fact, > they are not scheduling a thing -- the NTPSnippetsLauncher *gets* scheduled by > the system and it triggers a fetch. What we want to introduce is a smarter fetch > logic so that it only maybe triggers a fetch -- and that logic is in > SchedulingRemoteSuggestionsProvider which actually maintains the schedule (and > hence is "Scheduling"). > > That said, I'd move the Listener interfac either into > scheduling_remote_suggestions_provider.h or content_suggestions_service.h > We can call it RemoteSuggestionsScheduler. I think for now a simply "DoFetch()" > would be sufficient -- later we might introduce more ones to better distinguish > between optional fetches etc. > > The RescheduleFetching() seems odd -- in the case of the NTPSnippetsLauncher, it > would ultimately result in a call on the NTPSnippetsLauncher which feels like a > weird round-trip. Can we just leave this one out and make sure the > SchedulingRemoteSuggestionsProvider has access to the scheduler? Done. https://codereview.chromium.org/2557363002/diff/140001/components/ntp_snippet... File components/ntp_snippets/remote/remote_suggestions_provider.h (right): https://codereview.chromium.org/2557363002/diff/140001/components/ntp_snippet... components/ntp_snippets/remote/remote_suggestions_provider.h:93: void RegisterActivenessObserver(const ProviderActiveCallback& callback); On 2016/12/15 19:27:00, tschumann wrote: > naming is hard. Maybe we change this to > rename the callback to > ProviderStatusCallback() and probably introduce an enum for readability: > enum ProviderStatus { ACTIVE, INACTIVE } or ENABLED, DISABLED, whatever. > > then we can call this: > SetProviderStatusListener or SetProviderStatusCallback(). > > As we only store a single callback, we should call it Set and not Register. > > How do we distinguish if such a callback was provided? How does the default > constructed callback react if we call it? I guess it's best to make this > function take a unique_ptr<> and store the unique ptr so that we can > a) check against nullptr > b) clients can unregister themselves before they get deleted (we don't want > callbacks into deleted objects). Renamed as suggested. Totally agree with both "Set" and "ProviderStatus". I believe that Callback internally handles this (keeping a pointer to a context, being nullptr initially). Do you still want me to change it here to clarify the contract here? I do not think that clearing / uneregistering the callback is needed here. The only client so far keeps RemoteSuggestionsProviderImpl as a unique_ptr so it will delete the whole class before deleting itself. https://codereview.chromium.org/2557363002/diff/140001/components/ntp_snippet... File components/ntp_snippets/remote/scheduling_remote_suggestions_provider.cc (right): https://codereview.chromium.org/2557363002/diff/140001/components/ntp_snippet... components/ntp_snippets/remote/scheduling_remote_suggestions_provider.cc:85: provider->RegisterActivenessObserver(base::BindRepeating( On 2016/12/15 19:27:00, tschumann wrote: > doing this in the constructor is tricky -- we might get called back while still > in the constructor which is an issue in case this class gets subclassed. I > believe the RemoteSuggestionsProvider has the same issue. Let's add a todo and > mark this class final for now. > (a proper way would be to have the constructor private and a static Create() > function which first constructs the object and then calls an init() function.) Added the TODO. https://codereview.chromium.org/2557363002/diff/140001/components/ntp_snippet... components/ntp_snippets/remote/scheduling_remote_suggestions_provider.cc:262: // Intercept |status_code| before calling the provided |callback|. On 2016/12/15 19:27:00, tschumann wrote: > no need for the comment -- it's quite obvious :-) Done. https://codereview.chromium.org/2557363002/diff/140001/components/ntp_snippet... File components/ntp_snippets/remote/scheduling_remote_suggestions_provider.h (right): https://codereview.chromium.org/2557363002/diff/140001/components/ntp_snippet... components/ntp_snippets/remote/scheduling_remote_suggestions_provider.h:92: struct FetchingSchedule { On 2016/12/15 19:27:00, tschumann wrote: > let's move this into the .cc file and just forward declare here, to keep the > header short. Done.
https://codereview.chromium.org/2557363002/diff/200001/components/ntp_snippet... File components/ntp_snippets/content_suggestions_service.cc (right): https://codereview.chromium.org/2557363002/diff/200001/components/ntp_snippet... components/ntp_snippets/content_suggestions_service.cc:33: remote_suggestions_scheduler_(nullptr), why do we need both? Shouldn't the SchedulingRemoteSuggestionsProvider be sufficient? https://codereview.chromium.org/2557363002/diff/200001/components/ntp_snippet... File components/ntp_snippets/remote/remote_suggestions_provider.h (right): https://codereview.chromium.org/2557363002/diff/200001/components/ntp_snippet... components/ntp_snippets/remote/remote_suggestions_provider.h:18: // provides them as content suggestions. Minimal additional interface that needs feel free to drop the second sentence. I don't think it adds a lot of value to readers / users of this interface. https://codereview.chromium.org/2557363002/diff/200001/components/ntp_snippet... File components/ntp_snippets/remote/remote_suggestions_provider_impl_unittest.cc (right): https://codereview.chromium.org/2557363002/diff/200001/components/ntp_snippet... components/ntp_snippets/remote/remote_suggestions_provider_impl_unittest.cc:1633: EXPECT_CALL(activeness_observer, Call(_)).Times(0); this is a bit backwards. Instead, I'd recommend a StrictMock<MockFunction<>> and don't set any expectations (strict-mocks fail on unexpected calls). https://codereview.chromium.org/2557363002/diff/200001/components/ntp_snippet... components/ntp_snippets/remote/remote_suggestions_provider_impl_unittest.cc:1643: // Call the callback when becoming ready. nit: The comment being written in "active" form is a bit misleading -- we expect it to be called. // Should be called when becoming ready? Same in the tests below. https://codereview.chromium.org/2557363002/diff/200001/components/ntp_snippet... File components/ntp_snippets/remote/remote_suggestions_scheduler.h (right): https://codereview.chromium.org/2557363002/diff/200001/components/ntp_snippet... components/ntp_snippets/remote/remote_suggestions_scheduler.h:17: virtual void OnFetchDue() = 0; This sounds a bit odd on this interface. The scheduler is making the scheduling decisions, so it's weird that somebody else tells it to fetch. Why not name the signal very explicitly, like OnPersistentSchedulerWakeUp() or something like that? Would be good to transport the proper context of the signal so that the scheduling decisions are easy to follow. https://codereview.chromium.org/2557363002/diff/200001/components/ntp_snippet... File components/ntp_snippets/remote/scheduling_remote_suggestions_provider.h (right): https://codereview.chromium.org/2557363002/diff/200001/components/ntp_snippet... components/ntp_snippets/remote/scheduling_remote_suggestions_provider.h:32: // - regular fetches according to its schedule (when notified via i'd drop the part in parenthesis. It's an implementation detail which might change soon. It's fine to have comments like that at the function definition, but I'd consider them noise in headers. https://codereview.chromium.org/2557363002/diff/200001/components/ntp_snippet... components/ntp_snippets/remote/scheduling_remote_suggestions_provider.h:71: // RemoteSuggestionsScheduler implementation nit: Comments should end with a period. https://codereview.chromium.org/2557363002/diff/200001/components/ntp_snippet... File components/ntp_snippets/remote/scheduling_remote_suggestions_provider_unittest.cc (right): https://codereview.chromium.org/2557363002/diff/200001/components/ntp_snippet... components/ntp_snippets/remote/scheduling_remote_suggestions_provider_unittest.cc:144: drop consecutive blank lines. same in line 106/107
following up on the other comments (missed them when only looking at the latest snapshot). https://codereview.chromium.org/2557363002/diff/140001/components/ntp_snippet... File components/ntp_snippets/content_suggestions_service.h (right): https://codereview.chromium.org/2557363002/diff/140001/components/ntp_snippet... components/ntp_snippets/content_suggestions_service.h:207: void set_remote_suggestions_provider( On 2016/12/19 09:40:24, jkrcal wrote: > On 2016/12/15 19:27:00, tschumann wrote: > > please add a TODO() to remove this setter. These instances should be injected > > into the constructor. Not sure which debugging use cases would justify > changing > > those internals. > > > > I see that right now, we have a cyclic dependency as the service gets injected > > as an observer into the RemoteSuggestionProvider. IMO, it would be more > > appropriate to hand the RemoteSuggetsionsProvider into the > > ContentSuggestionsService's constructor and have a setter on the > > RemoteSuggestionsProvider to set the observer. But like I said, that's a > TODO(). > > Yes, there is a cyclic dependency. Adding a setter for the observer actually > influences all providers whereas this setter influences just one provider. Why > do you feel the former way is better? (maybe better to discuss offline) The depdencies just feel wrong: Needing the service first before being able to construct the provider only complicates things. The most important flow is that the service forwards data to the providers.The depdency back from the provider to the service is of lower importance IMO. But I might also see this from a biased view. Probably best to leave it as is for now :-) (because you can argue that the provider's job is to push data to observers and not being called by the service ;-)). https://codereview.chromium.org/2557363002/diff/140001/components/ntp_snippet... File components/ntp_snippets/remote/remote_suggestions_provider.h (right): https://codereview.chromium.org/2557363002/diff/140001/components/ntp_snippet... components/ntp_snippets/remote/remote_suggestions_provider.h:93: void RegisterActivenessObserver(const ProviderActiveCallback& callback); On 2016/12/19 09:40:24, jkrcal wrote: > On 2016/12/15 19:27:00, tschumann wrote: > > naming is hard. Maybe we change this to > > rename the callback to > > ProviderStatusCallback() and probably introduce an enum for readability: > > enum ProviderStatus { ACTIVE, INACTIVE } or ENABLED, DISABLED, whatever. > > > > then we can call this: > > SetProviderStatusListener or SetProviderStatusCallback(). > > > > As we only store a single callback, we should call it Set and not Register. > > > > How do we distinguish if such a callback was provided? How does the default > > constructed callback react if we call it? I guess it's best to make this > > function take a unique_ptr<> and store the unique ptr so that we can > > a) check against nullptr > > b) clients can unregister themselves before they get deleted (we don't want > > callbacks into deleted objects). > > Renamed as suggested. Totally agree with both "Set" and "ProviderStatus". > > I believe that Callback internally handles this (keeping a pointer to a context, > being nullptr initially). Do you still want me to change it here to clarify the > contract here? > > I do not think that clearing / uneregistering the callback is needed here. The > only client so far keeps RemoteSuggestionsProviderImpl as a unique_ptr so it > will delete the whole class before deleting itself. I see. Well in this case we're in good shape. If that changes, that party can still call SetProviderStatusCallback() with a dummy callback to unregister. can you add a unit test making sure we play nicely with the default constructed callback?
Bunch of comments, but nothing major. The general design looks good, thanks! https://codereview.chromium.org/2557363002/diff/200001/components/ntp_snippet... File components/ntp_snippets/content_suggestions_service.cc (right): https://codereview.chromium.org/2557363002/diff/200001/components/ntp_snippet... components/ntp_snippets/content_suggestions_service.cc:33: remote_suggestions_scheduler_(nullptr), On 2016/12/19 11:07:19, tschumann wrote: > why do we need both? Shouldn't the SchedulingRemoteSuggestionsProvider be > sufficient? But then that'd have to expose both members. And since they're exposed for different reasons (the provider only for the internals page, the scheduler only because of plumbing reasons, to make it accessible from the scheduled wake-ups), I kinda like them being separate. Not feeling very strongly though. https://codereview.chromium.org/2557363002/diff/200001/components/ntp_snippet... components/ntp_snippets/content_suggestions_service.cc:230: category_provider_pair.second->ReloadSuggestions(); If a provider provides multiple categories, then it'll get called multiple times here. I think this should just iterate over providers_ (and maybe skip those that haven't registered any categories?) https://codereview.chromium.org/2557363002/diff/200001/components/ntp_snippet... File components/ntp_snippets/remote/persistent_scheduler.h (right): https://codereview.chromium.org/2557363002/diff/200001/components/ntp_snippet... components/ntp_snippets/remote/persistent_scheduler.h:33: // Cancel any scheduled tasks. Equivalent to Schedule(0, 0). Hm, we could just get rid of Unschedule. WDYT? https://codereview.chromium.org/2557363002/diff/200001/components/ntp_snippet... File components/ntp_snippets/remote/remote_suggestions_provider.cc (right): https://codereview.chromium.org/2557363002/diff/200001/components/ntp_snippet... components/ntp_snippets/remote/remote_suggestions_provider.cc:1: // Copyright 2016 The Chromium Authors. All rights reserved. It doesn't really matter, but we don't generally update the copyright years. https://codereview.chromium.org/2557363002/diff/200001/components/ntp_snippet... File components/ntp_snippets/remote/remote_suggestions_provider.h (right): https://codereview.chromium.org/2557363002/diff/200001/components/ntp_snippet... components/ntp_snippets/remote/remote_suggestions_provider.h:22: enum ProviderStatus { ACTIVE, INACTIVE }; Hrm. I'm not thrilled about introducing yet another "status" enum. But I guess there's no good way around it... https://codereview.chromium.org/2557363002/diff/200001/components/ntp_snippet... components/ntp_snippets/remote/remote_suggestions_provider.h:49: CategoryFactory* category_factory); Heads-up: This will need a rebase onto https://codereview.chromium.org/2568033005/ https://codereview.chromium.org/2557363002/diff/200001/components/ntp_snippet... File components/ntp_snippets/remote/remote_suggestions_provider_impl.cc (left): https://codereview.chromium.org/2557363002/diff/200001/components/ntp_snippet... components/ntp_snippets/remote/remote_suggestions_provider_impl.cc:412: return; I think the "return" should stay? https://codereview.chromium.org/2557363002/diff/200001/components/ntp_snippet... File components/ntp_snippets/remote/remote_suggestions_provider_impl.cc (right): https://codereview.chromium.org/2557363002/diff/200001/components/ntp_snippet... components/ntp_snippets/remote/remote_suggestions_provider_impl.cc:344: switch (state_) { Maybe pull this logic into a helper method ("NotifyStateChanged") that can be reused in EnterState? https://codereview.chromium.org/2557363002/diff/200001/components/ntp_snippet... components/ntp_snippets/remote/remote_suggestions_provider_impl.cc:959: FetchSnippets(fetch_when_ready_interactive_, fetch_when_ready_callback_); If we get here because our article suggestions are empty, then fetch_when_ready_callback_ won't be initialized. Is that okay? https://codereview.chromium.org/2557363002/diff/200001/components/ntp_snippet... File components/ntp_snippets/remote/remote_suggestions_provider_impl.h (right): https://codereview.chromium.org/2557363002/diff/200001/components/ntp_snippet... components/ntp_snippets/remote/remote_suggestions_provider_impl.h:111: using ProviderActiveCallback = base::RepeatingCallback<void(bool active)>; AFAIK Callback and RepeatingCallback are the same thing. We should be consistent. Also seems that ProviderActiveCallback isn't actually used? https://codereview.chromium.org/2557363002/diff/200001/components/ntp_snippet... components/ntp_snippets/remote/remote_suggestions_provider_impl.h:112: // |application_language_code| should be a ISO 639-1 compliant string, e.g. nit: empty line before https://codereview.chromium.org/2557363002/diff/200001/components/ntp_snippet... components/ntp_snippets/remote/remote_suggestions_provider_impl.h:136: // Returns whether the service is successfuly initialized. While this is successfully (two "l"s) https://codereview.chromium.org/2557363002/diff/200001/components/ntp_snippet... components/ntp_snippets/remote/remote_suggestions_provider_impl.h:431: FetchStatusCallback fetch_when_ready_callback_; Maybe package all these into a struct, and have a base::Optional instance of it here? https://codereview.chromium.org/2557363002/diff/200001/components/ntp_snippet... File components/ntp_snippets/remote/remote_suggestions_provider_impl_unittest.cc (right): https://codereview.chromium.org/2557363002/diff/200001/components/ntp_snippet... components/ntp_snippets/remote/remote_suggestions_provider_impl_unittest.cc:1583: // is nit: "wrong" line break https://codereview.chromium.org/2557363002/diff/200001/components/ntp_snippet... components/ntp_snippets/remote/remote_suggestions_provider_impl_unittest.cc:1633: EXPECT_CALL(activeness_observer, Call(_)).Times(0); On 2016/12/19 11:07:19, tschumann wrote: > this is a bit backwards. Instead, I'd recommend a StrictMock<MockFunction<>> and > don't set any expectations (strict-mocks fail on unexpected calls). The ".Times(0)" pattern is somewhat common, though mostly with Mock classes rather than functions. I guess having a StrictMock in the first place is usually better. Something to add to our testing guidelines doc? https://codereview.chromium.org/2557363002/diff/200001/components/ntp_snippet... File components/ntp_snippets/remote/remote_suggestions_scheduler.h (right): https://codereview.chromium.org/2557363002/diff/200001/components/ntp_snippet... components/ntp_snippets/remote/remote_suggestions_scheduler.h:12: // Interface for informing the scheduler of fetches of remote suggestions about nit: "scheduler of fetches of remote suggestions" is hard to parse for me. I think just "scheduler" is enough; the rest is in the class name anyway. https://codereview.chromium.org/2557363002/diff/200001/components/ntp_snippet... components/ntp_snippets/remote/remote_suggestions_scheduler.h:21: }; DISALLOW_COPY_AND_ASSIGN? https://codereview.chromium.org/2557363002/diff/200001/components/ntp_snippet... File components/ntp_snippets/remote/scheduling_remote_suggestions_provider.cc (right): https://codereview.chromium.org/2557363002/diff/200001/components/ntp_snippet... components/ntp_snippets/remote/scheduling_remote_suggestions_provider.cc:39: base::TimeDelta GetCurrentUpdateInterval( GetDesiredUpdateInterval https://codereview.chromium.org/2557363002/diff/200001/components/ntp_snippet... components/ntp_snippets/remote/scheduling_remote_suggestions_provider.cc:77: base::TimeDelta::FromHours(0)}; Just use the default ctor of TimeDelta. (In fact, this will do the exact same thing as just instantiating a FetchingSchedule without explicitly initializing it, but I guess it's still nicer to have it explicit) https://codereview.chromium.org/2557363002/diff/200001/components/ntp_snippet... components/ntp_snippets/remote/scheduling_remote_suggestions_provider.cc:80: bool operator!=(const FetchingSchedule& other) const { nit: If it has !=, it should probably also have == (you can implement one in terms of the other) https://codereview.chromium.org/2557363002/diff/200001/components/ntp_snippet... components/ntp_snippets/remote/scheduling_remote_suggestions_provider.cc:103: DCHECK(pref_service) << "non-null pref service is needed"; nit: Comments aren't really necessary; they're obvious from the DCHECK itself. https://codereview.chromium.org/2557363002/diff/200001/components/ntp_snippet... components/ntp_snippets/remote/scheduling_remote_suggestions_provider.cc:205: } NOTREACHED(); https://codereview.chromium.org/2557363002/diff/200001/components/ntp_snippet... components/ntp_snippets/remote/scheduling_remote_suggestions_provider.cc:304: ApplyFetchingSchedule(GetLastFetchingSchedule()); Just call RescheduleFetching? https://codereview.chromium.org/2557363002/diff/200001/components/ntp_snippet... File components/ntp_snippets/remote/scheduling_remote_suggestions_provider.h (right): https://codereview.chromium.org/2557363002/diff/200001/components/ntp_snippet... components/ntp_snippets/remote/scheduling_remote_suggestions_provider.h:54: // The passed in |updater| is called when an update is due according to the There's no |updater| https://codereview.chromium.org/2557363002/diff/200001/components/ntp_snippet... components/ntp_snippets/remote/scheduling_remote_suggestions_provider.h:100: void OnProviderStatusChanged( Comment? https://codereview.chromium.org/2557363002/diff/200001/components/ntp_snippet... components/ntp_snippets/remote/scheduling_remote_suggestions_provider.h:120: FetchingSchedule GetDesiredFetchingSchedule(); Can this be const? https://codereview.chromium.org/2557363002/diff/200001/components/ntp_snippet... components/ntp_snippets/remote/scheduling_remote_suggestions_provider.h:124: // Common function to call after each fetch. misaligned https://codereview.chromium.org/2557363002/diff/200001/components/ntp_snippet... components/ntp_snippets/remote/scheduling_remote_suggestions_provider.h:130: // Interface for doing the actual updates, when they are due. Not owned. If it's a unique_ptr, it's owned by definition.
https://codereview.chromium.org/2557363002/diff/200001/components/ntp_snippet... File components/ntp_snippets/content_suggestions_service.cc (right): https://codereview.chromium.org/2557363002/diff/200001/components/ntp_snippet... components/ntp_snippets/content_suggestions_service.cc:33: remote_suggestions_scheduler_(nullptr), On 2016/12/19 12:59:23, Marc Treib (OOO until Jan 12) wrote: > On 2016/12/19 11:07:19, tschumann wrote: > > why do we need both? Shouldn't the SchedulingRemoteSuggestionsProvider be > > sufficient? > > But then that'd have to expose both members. And since they're exposed for > different reasons (the provider only for the internals page, the scheduler only > because of plumbing reasons, to make it accessible from the scheduled wake-ups), > I kinda like them being separate. Not feeling very strongly though. hehe... so my question is more: why do we still need to expose the wrapped remote-suggestions-provider. Nothing we need to fix now, but it would be good to understand and clean-up later.
Tim, finally I finished the CL to a state where it passes unit-tests and have no pending comments :) Tim, Markus, PTAL, again! I changed to callbacks to unique_ptr because the way I did it previously, did not work (calling an empty callback indeed crashes in an unit-test). https://codereview.chromium.org/2557363002/diff/140001/components/ntp_snippet... File components/ntp_snippets/content_suggestions_service.h (right): https://codereview.chromium.org/2557363002/diff/140001/components/ntp_snippet... components/ntp_snippets/content_suggestions_service.h:207: void set_remote_suggestions_provider( On 2016/12/19 11:19:23, tschumann wrote: > On 2016/12/19 09:40:24, jkrcal wrote: > > On 2016/12/15 19:27:00, tschumann wrote: > > > please add a TODO() to remove this setter. These instances should be > injected > > > into the constructor. Not sure which debugging use cases would justify > > changing > > > those internals. > > > > > > I see that right now, we have a cyclic dependency as the service gets > injected > > > as an observer into the RemoteSuggestionProvider. IMO, it would be more > > > appropriate to hand the RemoteSuggetsionsProvider into the > > > ContentSuggestionsService's constructor and have a setter on the > > > RemoteSuggestionsProvider to set the observer. But like I said, that's a > > TODO(). > > > > Yes, there is a cyclic dependency. Adding a setter for the observer actually > > influences all providers whereas this setter influences just one provider. Why > > do you feel the former way is better? (maybe better to discuss offline) > > The depdencies just feel wrong: Needing the service first before being able to > construct the provider only complicates things. The most important flow is that > the service forwards data to the providers.The depdency back from the provider > to the service is of lower importance IMO. > But I might also see this from a biased view. Probably best to leave it as is > for now :-) > (because you can argue that the provider's job is to push data to observers and > not being called by the service ;-)). Thanks for the explanation. Still not totally convinced it is worth the effort. Added a (mildly phrased) TODO so that we do not forget this discussion. https://codereview.chromium.org/2557363002/diff/140001/components/ntp_snippet... File components/ntp_snippets/remote/remote_suggestions_provider.h (right): https://codereview.chromium.org/2557363002/diff/140001/components/ntp_snippet... components/ntp_snippets/remote/remote_suggestions_provider.h:93: void RegisterActivenessObserver(const ProviderActiveCallback& callback); On 2016/12/19 11:19:23, tschumann wrote: > On 2016/12/19 09:40:24, jkrcal wrote: > > On 2016/12/15 19:27:00, tschumann wrote: > > > naming is hard. Maybe we change this to > > > rename the callback to > > > ProviderStatusCallback() and probably introduce an enum for readability: > > > enum ProviderStatus { ACTIVE, INACTIVE } or ENABLED, DISABLED, whatever. > > > > > > then we can call this: > > > SetProviderStatusListener or SetProviderStatusCallback(). > > > > > > As we only store a single callback, we should call it Set and not Register. > > > > > > How do we distinguish if such a callback was provided? How does the default > > > constructed callback react if we call it? I guess it's best to make this > > > function take a unique_ptr<> and store the unique ptr so that we can > > > a) check against nullptr > > > b) clients can unregister themselves before they get deleted (we don't want > > > callbacks into deleted objects). > > > > Renamed as suggested. Totally agree with both "Set" and "ProviderStatus". > > > > I believe that Callback internally handles this (keeping a pointer to a > context, > > being nullptr initially). Do you still want me to change it here to clarify > the > > contract here? > > > > I do not think that clearing / uneregistering the callback is needed here. The > > only client so far keeps RemoteSuggestionsProviderImpl as a unique_ptr so it > > will delete the whole class before deleting itself. > > I see. Well in this case we're in good shape. If that changes, that party can > still call SetProviderStatusCallback() with a dummy callback to unregister. > > can you add a unit test making sure we play nicely with the default constructed > callback? Done. https://codereview.chromium.org/2557363002/diff/140001/components/ntp_snippet... File components/ntp_snippets/remote/scheduling_remote_suggestions_provider_unittest.cc (right): https://codereview.chromium.org/2557363002/diff/140001/components/ntp_snippet... components/ntp_snippets/remote/scheduling_remote_suggestions_provider_unittest.cc:79: scheduler->SetUpdater(&updater()); On 2016/12/15 19:27:01, tschumann wrote: > &updater_ Not relevant any more. https://codereview.chromium.org/2557363002/diff/140001/components/ntp_snippet... components/ntp_snippets/remote/scheduling_remote_suggestions_provider_unittest.cc:81: if (enabled) { On 2016/12/15 19:27:01, tschumann wrote: > let's not do this inside the MakeScheduler() function. Less convenience, less > magic, easier to read the tests. Done. https://codereview.chromium.org/2557363002/diff/140001/components/ntp_snippet... components/ntp_snippets/remote/scheduling_remote_suggestions_provider_unittest.cc:90: MockHardScheduler& mock_scheduler() { return hard_scheduler_; } On 2016/12/15 19:27:01, tschumann wrote: > i know that we followed this pattern in other tests, but honestly, I don't see a > lot of reason in this. The test fixture should stay simple and there shouldn't > be a need to artificially encapsulate these members. Just make them all > protected and remove the accessors -- they don't add value IMO. Done. https://codereview.chromium.org/2557363002/diff/140001/components/ntp_snippet... components/ntp_snippets/remote/scheduling_remote_suggestions_provider_unittest.cc:96: NiceMock<MockHardScheduler> hard_scheduler_; On 2016/12/15 19:27:01, tschumann wrote: > are you sure you want them to be nice? From my experience, you should default > to StrictMocks. Done. https://codereview.chromium.org/2557363002/diff/140001/components/ntp_snippet... components/ntp_snippets/remote/scheduling_remote_suggestions_provider_unittest.cc:103: EXPECT_CALL(mock_scheduler(), Schedule(_, _)).Times(1); On 2016/12/15 19:27:01, tschumann wrote: > .Times(1) is implicit. Done. https://codereview.chromium.org/2557363002/diff/140001/components/ntp_snippet... components/ntp_snippets/remote/scheduling_remote_suggestions_provider_unittest.cc:104: EXPECT_CALL(mock_scheduler(), Unschedule()).Times(0); On 2016/12/15 19:27:01, tschumann wrote: > with a strict-mock that's the default behavior (it fails if it was called > unexpectedly). I'd argue that that's the better choice. Done. https://codereview.chromium.org/2557363002/diff/200001/components/ntp_snippet... File components/ntp_snippets/content_suggestions_service.cc (right): https://codereview.chromium.org/2557363002/diff/200001/components/ntp_snippet... components/ntp_snippets/content_suggestions_service.cc:33: remote_suggestions_scheduler_(nullptr), On 2016/12/19 11:07:19, tschumann wrote: > why do we need both? Shouldn't the SchedulingRemoteSuggestionsProvider be > sufficient? These are different interfaces, one is used for debugging and testing, the other from the bridge. I would like to at least keep the getters separate. We can still unify the setters and the pointers. However, I am not convinced if the service should necessarily know who is implementing what. (At the moment, SchedulingRemoteSuggestionsProvider still does not implement RemoteSuggestionsProvider. This should be fixed anyway, maybe in another CL?) https://codereview.chromium.org/2557363002/diff/200001/components/ntp_snippet... components/ntp_snippets/content_suggestions_service.cc:33: remote_suggestions_scheduler_(nullptr), On 2016/12/19 12:59:23, Marc Treib (OOO until Jan 12) wrote: > On 2016/12/19 11:07:19, tschumann wrote: > > why do we need both? Shouldn't the SchedulingRemoteSuggestionsProvider be > > sufficient? > > But then that'd have to expose both members. And since they're exposed for > different reasons (the provider only for the internals page, the scheduler only > because of plumbing reasons, to make it accessible from the scheduled wake-ups), > I kinda like them being separate. Not feeling very strongly though. Thanks, agreed :) https://codereview.chromium.org/2557363002/diff/200001/components/ntp_snippet... components/ntp_snippets/content_suggestions_service.cc:33: remote_suggestions_scheduler_(nullptr), On 2016/12/19 14:17:08, tschumann wrote: > On 2016/12/19 12:59:23, Marc Treib (OOO until Jan 12) wrote: > > On 2016/12/19 11:07:19, tschumann wrote: > > > why do we need both? Shouldn't the SchedulingRemoteSuggestionsProvider be > > > sufficient? > > > > But then that'd have to expose both members. And since they're exposed for > > different reasons (the provider only for the internals page, the scheduler > only > > because of plumbing reasons, to make it accessible from the scheduled > wake-ups), > > I kinda like them being separate. Not feeling very strongly though. > > hehe... so my question is more: why do we still need to expose the wrapped > remote-suggestions-provider. Nothing we need to fix now, but it would be good to > understand and clean-up later. As I say above, the only reason for the moment is that the scheduling wrapper does not implement RemoteSuggestionsProvider). I think we can just expose the scheduling wrapper, using two getters. I am not sure whether to have just one setter or two setters (so that content_suggestions_service does not need to know about implementation details). https://codereview.chromium.org/2557363002/diff/200001/components/ntp_snippet... components/ntp_snippets/content_suggestions_service.cc:230: category_provider_pair.second->ReloadSuggestions(); On 2016/12/19 12:59:23, Marc Treib (OOO until Jan 12) wrote: > If a provider provides multiple categories, then it'll get called multiple times > here. > I think this should just iterate over providers_ (and maybe skip those that > haven't registered any categories?) Indeed, thanks! I do not see how to implement skipping those without any categories without introducing another data or without heavy iteration. I do not see the added value worth the effort. https://codereview.chromium.org/2557363002/diff/200001/components/ntp_snippet... File components/ntp_snippets/remote/persistent_scheduler.h (right): https://codereview.chromium.org/2557363002/diff/200001/components/ntp_snippet... components/ntp_snippets/remote/persistent_scheduler.h:33: // Cancel any scheduled tasks. Equivalent to Schedule(0, 0). On 2016/12/19 12:59:23, Marc Treib (OOO until Jan 12) wrote: > Hm, we could just get rid of Unschedule. WDYT? We could. I prefer them being split, though. It makes the application clearer, IMO. It also makes writing expectations in unit-tests way clearer. If you feel strongly, I am willing to remove it. https://codereview.chromium.org/2557363002/diff/200001/components/ntp_snippet... File components/ntp_snippets/remote/remote_suggestions_provider.cc (right): https://codereview.chromium.org/2557363002/diff/200001/components/ntp_snippet... components/ntp_snippets/remote/remote_suggestions_provider.cc:1: // Copyright 2016 The Chromium Authors. All rights reserved. On 2016/12/19 12:59:23, Marc Treib (OOO until Jan 12) wrote: > It doesn't really matter, but we don't generally update the copyright years. I treat this as a new file (whereas I treat the Impl as the 2015 one). Does it make sense? https://codereview.chromium.org/2557363002/diff/200001/components/ntp_snippet... File components/ntp_snippets/remote/remote_suggestions_provider.h (right): https://codereview.chromium.org/2557363002/diff/200001/components/ntp_snippet... components/ntp_snippets/remote/remote_suggestions_provider.h:18: // provides them as content suggestions. Minimal additional interface that needs On 2016/12/19 11:07:19, tschumann wrote: > feel free to drop the second sentence. I don't think it adds a lot of value to > readers / users of this interface. Done. https://codereview.chromium.org/2557363002/diff/200001/components/ntp_snippet... components/ntp_snippets/remote/remote_suggestions_provider.h:22: enum ProviderStatus { ACTIVE, INACTIVE }; On 2016/12/19 12:59:23, Marc Treib (OOO until Jan 12) wrote: > Hrm. I'm not thrilled about introducing yet another "status" enum. But I guess > there's no good way around it... Good point, I've added a TODO to unify the statuses when refactoring RemoteSuggestionsStatusService. https://codereview.chromium.org/2557363002/diff/200001/components/ntp_snippet... components/ntp_snippets/remote/remote_suggestions_provider.h:49: CategoryFactory* category_factory); On 2016/12/19 12:59:23, Marc Treib (OOO until Jan 12) wrote: > Heads-up: This will need a rebase onto > https://codereview.chromium.org/2568033005/ Thanks! Huh. Inevitable fate of too large CLs ;) (and punishment for authors not able to restrain from writing them) https://codereview.chromium.org/2557363002/diff/200001/components/ntp_snippet... File components/ntp_snippets/remote/remote_suggestions_provider_impl.cc (left): https://codereview.chromium.org/2557363002/diff/200001/components/ntp_snippet... components/ntp_snippets/remote/remote_suggestions_provider_impl.cc:412: return; On 2016/12/19 12:59:23, Marc Treib (OOO until Jan 12) wrote: > I think the "return" should stay? Indeed! https://codereview.chromium.org/2557363002/diff/200001/components/ntp_snippet... File components/ntp_snippets/remote/remote_suggestions_provider_impl.cc (right): https://codereview.chromium.org/2557363002/diff/200001/components/ntp_snippet... components/ntp_snippets/remote/remote_suggestions_provider_impl.cc:344: switch (state_) { On 2016/12/19 12:59:23, Marc Treib (OOO until Jan 12) wrote: > Maybe pull this logic into a helper method ("NotifyStateChanged") that can be > reused in EnterState? Done. https://codereview.chromium.org/2557363002/diff/200001/components/ntp_snippet... components/ntp_snippets/remote/remote_suggestions_provider_impl.cc:959: FetchSnippets(fetch_when_ready_interactive_, fetch_when_ready_callback_); On 2016/12/19 12:59:23, Marc Treib (OOO until Jan 12) wrote: > If we get here because our article suggestions are empty, then > fetch_when_ready_callback_ won't be initialized. Is that okay? I believe so. Callbacks keep internally a pointer to the context and the default initializer sets the pointer to nullptr. Nothing should be called in such a case. https://codereview.chromium.org/2557363002/diff/200001/components/ntp_snippet... File components/ntp_snippets/remote/remote_suggestions_provider_impl.h (right): https://codereview.chromium.org/2557363002/diff/200001/components/ntp_snippet... components/ntp_snippets/remote/remote_suggestions_provider_impl.h:111: using ProviderActiveCallback = base::RepeatingCallback<void(bool active)>; On 2016/12/19 12:59:23, Marc Treib (OOO until Jan 12) wrote: > AFAIK Callback and RepeatingCallback are the same thing. We should be > consistent. > Also seems that ProviderActiveCallback isn't actually used? Removed ProviderActiveCallback. Even FetchStatusCallback should be defined here. Still your comment applies to their definition in RemoteSuggestionsProvider. I added a TODO to change the Callback to OnceCallback there (I had problems with move-constructors and google mock that avoided me from doing it right away). Explanation (from the docs): The legacy `Callback<>` is currently aliased to `RepeatingCallback<>`. In new code, prefer `OnceCallback<>` where possible, and use `RepeatingCallback<>` otherwise. Once the migration is complete, the type alias will be removed and `OnceCallback<>` will be renamed to `Callback<>` to emphasize that it should be preferred. https://codereview.chromium.org/2557363002/diff/200001/components/ntp_snippet... components/ntp_snippets/remote/remote_suggestions_provider_impl.h:112: // |application_language_code| should be a ISO 639-1 compliant string, e.g. On 2016/12/19 12:59:23, Marc Treib (OOO until Jan 12) wrote: > nit: empty line before Done. https://codereview.chromium.org/2557363002/diff/200001/components/ntp_snippet... components/ntp_snippets/remote/remote_suggestions_provider_impl.h:136: // Returns whether the service is successfuly initialized. While this is On 2016/12/19 12:59:23, Marc Treib (OOO until Jan 12) wrote: > successfully (two "l"s) Huh, I have repetitive problems with "success" ;D https://codereview.chromium.org/2557363002/diff/200001/components/ntp_snippet... components/ntp_snippets/remote/remote_suggestions_provider_impl.h:431: FetchStatusCallback fetch_when_ready_callback_; On 2016/12/19 12:59:23, Marc Treib (OOO until Jan 12) wrote: > Maybe package all these into a struct, and have a base::Optional instance of it > here? I added a TODO. I really want to finish this piece. (Actually I struggled for a while and reverted back. Not so straightforward because the OnceCallback needs to be moved.) https://codereview.chromium.org/2557363002/diff/200001/components/ntp_snippet... File components/ntp_snippets/remote/remote_suggestions_provider_impl_unittest.cc (right): https://codereview.chromium.org/2557363002/diff/200001/components/ntp_snippet... components/ntp_snippets/remote/remote_suggestions_provider_impl_unittest.cc:1583: // is On 2016/12/19 12:59:23, Marc Treib (OOO until Jan 12) wrote: > nit: "wrong" line break Done. https://codereview.chromium.org/2557363002/diff/200001/components/ntp_snippet... components/ntp_snippets/remote/remote_suggestions_provider_impl_unittest.cc:1633: EXPECT_CALL(activeness_observer, Call(_)).Times(0); On 2016/12/19 11:07:19, tschumann wrote: > this is a bit backwards. Instead, I'd recommend a StrictMock<MockFunction<>> and > don't set any expectations (strict-mocks fail on unexpected calls). Done. https://codereview.chromium.org/2557363002/diff/200001/components/ntp_snippet... components/ntp_snippets/remote/remote_suggestions_provider_impl_unittest.cc:1633: EXPECT_CALL(activeness_observer, Call(_)).Times(0); On 2016/12/19 12:59:23, Marc Treib (OOO until Jan 12) wrote: > On 2016/12/19 11:07:19, tschumann wrote: > > this is a bit backwards. Instead, I'd recommend a StrictMock<MockFunction<>> > and > > don't set any expectations (strict-mocks fail on unexpected calls). > > The ".Times(0)" pattern is somewhat common, though mostly with Mock classes > rather than functions. I guess having a StrictMock in the first place is usually > better. Something to add to our testing guidelines doc? Agreed, I've added it into the testing agenda for Jan 12. https://codereview.chromium.org/2557363002/diff/200001/components/ntp_snippet... components/ntp_snippets/remote/remote_suggestions_provider_impl_unittest.cc:1643: // Call the callback when becoming ready. On 2016/12/19 11:07:19, tschumann wrote: > nit: The comment being written in "active" form is a bit misleading -- we expect > it to be called. > // Should be called when becoming ready? > > Same in the tests below. Done. https://codereview.chromium.org/2557363002/diff/200001/components/ntp_snippet... File components/ntp_snippets/remote/remote_suggestions_scheduler.h (right): https://codereview.chromium.org/2557363002/diff/200001/components/ntp_snippet... components/ntp_snippets/remote/remote_suggestions_scheduler.h:12: // Interface for informing the scheduler of fetches of remote suggestions about On 2016/12/19 12:59:23, Marc Treib (OOO until Jan 12) wrote: > nit: "scheduler of fetches of remote suggestions" is hard to parse for me. I > think just "scheduler" is enough; the rest is in the class name anyway. Done. https://codereview.chromium.org/2557363002/diff/200001/components/ntp_snippet... components/ntp_snippets/remote/remote_suggestions_scheduler.h:17: virtual void OnFetchDue() = 0; On 2016/12/19 11:07:19, tschumann wrote: > This sounds a bit odd on this interface. The scheduler is making the scheduling > decisions, so it's weird that somebody else tells it to fetch. > > Why not name the signal very explicitly, like OnPersistentSchedulerWakeUp() or > something like that? > > Would be good to transport the proper context of the signal so that the > scheduling decisions are easy to follow. Done. Great suggestions, thanks! https://codereview.chromium.org/2557363002/diff/200001/components/ntp_snippet... components/ntp_snippets/remote/remote_suggestions_scheduler.h:21: }; On 2016/12/19 12:59:23, Marc Treib (OOO until Jan 12) wrote: > DISALLOW_COPY_AND_ASSIGN? Is it needed? Are these default constructors created when I have no constructor at all? (If I add this macro, it forces me to also add a constructor here.) https://codereview.chromium.org/2557363002/diff/200001/components/ntp_snippet... File components/ntp_snippets/remote/scheduling_remote_suggestions_provider.cc (right): https://codereview.chromium.org/2557363002/diff/200001/components/ntp_snippet... components/ntp_snippets/remote/scheduling_remote_suggestions_provider.cc:39: base::TimeDelta GetCurrentUpdateInterval( On 2016/12/19 12:59:24, Marc Treib (OOO until Jan 12) wrote: > GetDesiredUpdateInterval Yep, thanks! https://codereview.chromium.org/2557363002/diff/200001/components/ntp_snippet... components/ntp_snippets/remote/scheduling_remote_suggestions_provider.cc:77: base::TimeDelta::FromHours(0)}; On 2016/12/19 12:59:23, Marc Treib (OOO until Jan 12) wrote: > Just use the default ctor of TimeDelta. > (In fact, this will do the exact same thing as just instantiating a > FetchingSchedule without explicitly initializing it, but I guess it's still > nicer to have it explicit) Done. https://codereview.chromium.org/2557363002/diff/200001/components/ntp_snippet... components/ntp_snippets/remote/scheduling_remote_suggestions_provider.cc:80: bool operator!=(const FetchingSchedule& other) const { On 2016/12/19 12:59:23, Marc Treib (OOO until Jan 12) wrote: > nit: If it has !=, it should probably also have == (you can implement one in > terms of the other) Done. https://codereview.chromium.org/2557363002/diff/200001/components/ntp_snippet... components/ntp_snippets/remote/scheduling_remote_suggestions_provider.cc:103: DCHECK(pref_service) << "non-null pref service is needed"; On 2016/12/19 12:59:23, Marc Treib (OOO until Jan 12) wrote: > nit: Comments aren't really necessary; they're obvious from the DCHECK itself. Done. https://codereview.chromium.org/2557363002/diff/200001/components/ntp_snippet... components/ntp_snippets/remote/scheduling_remote_suggestions_provider.cc:205: } On 2016/12/19 12:59:23, Marc Treib (OOO until Jan 12) wrote: > NOTREACHED(); Done. Is this really needed? Does not the compiler warn me if I forget something? Or is it to check that I haven't forgotten any return? https://codereview.chromium.org/2557363002/diff/200001/components/ntp_snippet... components/ntp_snippets/remote/scheduling_remote_suggestions_provider.cc:304: ApplyFetchingSchedule(GetLastFetchingSchedule()); On 2016/12/19 12:59:23, Marc Treib (OOO until Jan 12) wrote: > Just call RescheduleFetching? I prefer it this way because RescheduleFetching() does unnecessary work compared to this line. If you find it too cryptic, raise you voice again :) https://codereview.chromium.org/2557363002/diff/200001/components/ntp_snippet... File components/ntp_snippets/remote/scheduling_remote_suggestions_provider.h (right): https://codereview.chromium.org/2557363002/diff/200001/components/ntp_snippet... components/ntp_snippets/remote/scheduling_remote_suggestions_provider.h:32: // - regular fetches according to its schedule (when notified via On 2016/12/19 11:07:19, tschumann wrote: > i'd drop the part in parenthesis. It's an implementation detail which might > change soon. It's fine to have comments like that at the function definition, > but I'd consider them noise in headers. Done. https://codereview.chromium.org/2557363002/diff/200001/components/ntp_snippet... components/ntp_snippets/remote/scheduling_remote_suggestions_provider.h:54: // The passed in |updater| is called when an update is due according to the On 2016/12/19 12:59:24, Marc Treib (OOO until Jan 12) wrote: > There's no |updater| Done. https://codereview.chromium.org/2557363002/diff/200001/components/ntp_snippet... components/ntp_snippets/remote/scheduling_remote_suggestions_provider.h:71: // RemoteSuggestionsScheduler implementation On 2016/12/19 11:07:19, tschumann wrote: > nit: Comments should end with a period. Done. https://codereview.chromium.org/2557363002/diff/200001/components/ntp_snippet... components/ntp_snippets/remote/scheduling_remote_suggestions_provider.h:100: void OnProviderStatusChanged( On 2016/12/19 12:59:24, Marc Treib (OOO until Jan 12) wrote: > Comment? Done. https://codereview.chromium.org/2557363002/diff/200001/components/ntp_snippet... components/ntp_snippets/remote/scheduling_remote_suggestions_provider.h:124: // Common function to call after each fetch. On 2016/12/19 12:59:24, Marc Treib (OOO until Jan 12) wrote: > misaligned Done. https://codereview.chromium.org/2557363002/diff/200001/components/ntp_snippet... components/ntp_snippets/remote/scheduling_remote_suggestions_provider.h:130: // Interface for doing the actual updates, when they are due. Not owned. On 2016/12/19 12:59:24, Marc Treib (OOO until Jan 12) wrote: > If it's a unique_ptr, it's owned by definition. Done (outdated comment). https://codereview.chromium.org/2557363002/diff/200001/components/ntp_snippet... File components/ntp_snippets/remote/scheduling_remote_suggestions_provider_unittest.cc (right): https://codereview.chromium.org/2557363002/diff/200001/components/ntp_snippet... components/ntp_snippets/remote/scheduling_remote_suggestions_provider_unittest.cc:144: On 2016/12/19 11:07:19, tschumann wrote: > drop consecutive blank lines. > same in line 106/107 Done. https://codereview.chromium.org/2557363002/diff/240001/chrome/browser/ntp_sni... File chrome/browser/ntp_snippets/content_suggestions_service_factory.cc (right): https://codereview.chromium.org/2557363002/diff/240001/chrome/browser/ntp_sni... chrome/browser/ntp_snippets/content_suggestions_service_factory.cc:185: service, category_factory, std::move(provider), scheduler, A mistake in previous rebase. https://codereview.chromium.org/2557363002/diff/240001/components/ntp_snippet... File components/ntp_snippets/remote/remote_suggestions_provider_impl_unittest.cc (right): https://codereview.chromium.org/2557363002/diff/240001/components/ntp_snippet... components/ntp_snippets/remote/remote_suggestions_provider_impl_unittest.cc:478: &user_classifier_, std::move(snippets_fetcher), Also an omission in the rebase. https://codereview.chromium.org/2557363002/diff/260001/components/ntp_snippet... File components/ntp_snippets/remote/scheduling_remote_suggestions_provider_unittest.cc (right): https://codereview.chromium.org/2557363002/diff/260001/components/ntp_snippet... components/ntp_snippets/remote/scheduling_remote_suggestions_provider_unittest.cc:144: &persistent_scheduler_, &user_classifier_, utils_.pref_service()); I now construct both providers in the constructor because the previous "auto provider = MakeSchedulingProvider();" was a mess in some tests. Namely in tests, where I construct the provider and never use it in the test (because I only talk to underlying_provider_, for instance). I think it is clearer this way.
jkrcal@chromium.org changed reviewers: + bauerb@chromium.org, noyau@chromium.org
Bernhard, could you PTAL at chrome/ stuff? (if you do not make it before going ooo, please let me know asap) Éric, could you PTAL at the ios factory?
https://codereview.chromium.org/2557363002/diff/280001/ios/chrome/browser/ntp... File ios/chrome/browser/ntp_snippets/ios_chrome_content_suggestions_service_factory.cc (right): https://codereview.chromium.org/2557363002/diff/280001/ios/chrome/browser/ntp... ios/chrome/browser/ntp_snippets/ios_chrome_content_suggestions_service_factory.cc:184: /*persistent_scheduler=*/nullptr, service->user_classifier(), This code creates a SchedulingRemoteSuggestionsProvider with a null scheduler. Mmm, is this thing going to schedule anything on iOS, or is it just so the compilation pass? What is the behavior on iOS, will this trigger a fetch? Or repeat a fetch? Why does the implementation of something like a scheduler needs to be platform dependent? Before lgtming this I'd like to see what's the plan is to implement a scheduler on iOS. An idea on a design, and why the various schedulers we already have in Chrome to fetch data are not suitable for this usage.
https://codereview.chromium.org/2557363002/diff/280001/ios/chrome/browser/ntp... File ios/chrome/browser/ntp_snippets/ios_chrome_content_suggestions_service_factory.cc (right): https://codereview.chromium.org/2557363002/diff/280001/ios/chrome/browser/ntp... ios/chrome/browser/ntp_snippets/ios_chrome_content_suggestions_service_factory.cc:184: /*persistent_scheduler=*/nullptr, service->user_classifier(), On 2016/12/20 17:02:20, noyau wrote: > This code creates a SchedulingRemoteSuggestionsProvider with a null scheduler. > Mmm, is this thing going to schedule anything on iOS, or is it just so the > compilation pass? What is the behavior on iOS, will this trigger a fetch? Or > repeat a fetch? Why does the implementation of something like a scheduler needs > to be platform dependent? > > Before lgtming this I'd like to see what's the plan is to implement a scheduler > on iOS. An idea on a design, and why the various schedulers we already have in > Chrome to fetch data are not suitable for this usage. Éric, this is a persistent scheduler which cannot be easily implemented on iOS, AFAIK. The one that triggers the task no matter whether Chrome is running or not. This whole refactoring takes place to allow us adding "soft scheduling", which means signals to trigger a fetch, while Chrome is running. This will work on iOS as well.
On 2016/12/20 17:06:16, jkrcal wrote: > https://codereview.chromium.org/2557363002/diff/280001/ios/chrome/browser/ntp... > File > ios/chrome/browser/ntp_snippets/ios_chrome_content_suggestions_service_factory.cc > (right): > > https://codereview.chromium.org/2557363002/diff/280001/ios/chrome/browser/ntp... > ios/chrome/browser/ntp_snippets/ios_chrome_content_suggestions_service_factory.cc:184: > /*persistent_scheduler=*/nullptr, service->user_classifier(), > On 2016/12/20 17:02:20, noyau wrote: > > This code creates a SchedulingRemoteSuggestionsProvider with a null scheduler. > > Mmm, is this thing going to schedule anything on iOS, or is it just so the > > compilation pass? What is the behavior on iOS, will this trigger a fetch? Or > > repeat a fetch? Why does the implementation of something like a scheduler > needs > > to be platform dependent? > > > > Before lgtming this I'd like to see what's the plan is to implement a > scheduler > > on iOS. An idea on a design, and why the various schedulers we already have in > > Chrome to fetch data are not suitable for this usage. > > Éric, this is a persistent scheduler which cannot be easily implemented on iOS, > AFAIK. The one that triggers the task no matter whether Chrome is running or > not. > Background fetch is probably the iOS feature you need here. Sorry for the long URL: https://developer.apple.com/library/content/documentation/iPhone/Conceptual/i... This feature from iOS pings the apps at regular interval to check and download new content. > This whole refactoring takes place to allow us adding "soft scheduling", which > means signals to trigger a fetch, while Chrome is running. This will work on iOS > as well. > Is there a design doc on fetch strategies and scheduling I could take a look at?
On 2016/12/20 17:20:26, noyau wrote: > On 2016/12/20 17:06:16, jkrcal wrote: > > > https://codereview.chromium.org/2557363002/diff/280001/ios/chrome/browser/ntp... > > File > > > ios/chrome/browser/ntp_snippets/ios_chrome_content_suggestions_service_factory.cc > > (right): > > > > > https://codereview.chromium.org/2557363002/diff/280001/ios/chrome/browser/ntp... > > > ios/chrome/browser/ntp_snippets/ios_chrome_content_suggestions_service_factory.cc:184: > > /*persistent_scheduler=*/nullptr, service->user_classifier(), > > On 2016/12/20 17:02:20, noyau wrote: > > > This code creates a SchedulingRemoteSuggestionsProvider with a null > scheduler. > > > Mmm, is this thing going to schedule anything on iOS, or is it just so the > > > compilation pass? What is the behavior on iOS, will this trigger a fetch? Or > > > repeat a fetch? Why does the implementation of something like a scheduler > > needs > > > to be platform dependent? > > > > > > Before lgtming this I'd like to see what's the plan is to implement a > > scheduler > > > on iOS. An idea on a design, and why the various schedulers we already have > in > > > Chrome to fetch data are not suitable for this usage. > > > > Éric, this is a persistent scheduler which cannot be easily implemented on > iOS, > > AFAIK. The one that triggers the task no matter whether Chrome is running or > > not. > > > Background fetch is probably the iOS feature you need here. Sorry for the long > URL: > https://developer.apple.com/library/content/documentation/iPhone/Conceptual/i... > > > This feature from iOS pings the apps at regular interval to check and download > new content. > > > This whole refactoring takes place to allow us adding "soft scheduling", which > > means signals to trigger a fetch, while Chrome is running. This will work on > iOS > > as well. > > > Is there a design doc on fetch strategies and scheduling I could take a look at? Let's not let this block this CL. Can you add a TODO to the factory code, and file an associated bug? We can continue the conversation there.
lgtm, once the TODO and bugs have been added.
On 2016/12/20 17:59:04, noyau wrote: > lgtm, once the TODO and bugs have been added. Thanks! Added the todo and the bug (crbug.com/676249). The design doc is not in a finished state: https://docs.google.com/document/d/1AVJuNuXC1Ygg3apUvvxG7rTdwWJ1Z0oSoE4yN15Aj... The best source of info at the moment is probably crbug.com/672479
The CQ bit was checked by jkrcal@chromium.org to run a CQ dry run
Dry run: 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
Dry run: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
following up on comments. will review scheduling_remote_suggestions_provider_unittest.cc soon ;-) https://codereview.chromium.org/2557363002/diff/200001/components/ntp_snippet... File components/ntp_snippets/content_suggestions_service.cc (right): https://codereview.chromium.org/2557363002/diff/200001/components/ntp_snippet... components/ntp_snippets/content_suggestions_service.cc:33: remote_suggestions_scheduler_(nullptr), On 2016/12/20 16:39:46, jkrcal wrote: > On 2016/12/19 14:17:08, tschumann wrote: > > On 2016/12/19 12:59:23, Marc Treib (OOO until Jan 12) wrote: > > > On 2016/12/19 11:07:19, tschumann wrote: > > > > why do we need both? Shouldn't the SchedulingRemoteSuggestionsProvider be > > > > sufficient? > > > > > > But then that'd have to expose both members. And since they're exposed for > > > different reasons (the provider only for the internals page, the scheduler > > only > > > because of plumbing reasons, to make it accessible from the scheduled > > wake-ups), > > > I kinda like them being separate. Not feeling very strongly though. > > > > hehe... so my question is more: why do we still need to expose the wrapped > > remote-suggestions-provider. Nothing we need to fix now, but it would be good > to > > understand and clean-up later. > > As I say above, the only reason for the moment is that the scheduling wrapper > does not implement RemoteSuggestionsProvider). I think we can just expose the > scheduling wrapper, using two getters. I am not sure whether to have just one > setter or two setters (so that content_suggestions_service does not need to know > about implementation details). Oh yeah, totally. The scheduling remote suggestions provider should under no circumstances expose it's underlying provider -- that's the whole idea of the abstraction. I was just wondering which pieces of the remotesuggestionsprovider are still needed by the ContentSuggestionsService -- in an ideal world there would be no need to make that provider anything special. Thanks for clarifying! https://codereview.chromium.org/2557363002/diff/200001/components/ntp_snippet... components/ntp_snippets/content_suggestions_service.cc:230: category_provider_pair.second->ReloadSuggestions(); On 2016/12/20 16:39:46, jkrcal wrote: > On 2016/12/19 12:59:23, Marc Treib (OOO until Jan 12) wrote: > > If a provider provides multiple categories, then it'll get called multiple > times > > here. > > I think this should just iterate over providers_ (and maybe skip those that > > haven't registered any categories?) > > Indeed, thanks! > > I do not see how to implement skipping those without any categories without > introducing another data or without heavy iteration. I do not see the added > value worth the effort. +1 for such providers that would be a no-op anyways, right? https://codereview.chromium.org/2557363002/diff/220001/components/ntp_snippet... File components/ntp_snippets/remote/remote_suggestions_provider_impl.cc (right): https://codereview.chromium.org/2557363002/diff/220001/components/ntp_snippet... components/ntp_snippets/remote/remote_suggestions_provider_impl.cc:342: // Call the observer right away if we've reached any final state. you're missing a call to NotifyStatusChanged() now. https://codereview.chromium.org/2557363002/diff/220001/components/ntp_snippet... File components/ntp_snippets/remote/remote_suggestions_provider_impl_unittest.cc (right): https://codereview.chromium.org/2557363002/diff/220001/components/ntp_snippet... components/ntp_snippets/remote/remote_suggestions_provider_impl_unittest.cc:1668: ShouldNotCrashWhenCallingEmptyCallback) { nit: try not to mention internals here. The empty callback is quite a detail. What you're testing is that state transitions work fine when registering an empty callback. ShouldHandleDefaultConstructedProviderStatusCallbacks ?
The CQ bit was checked by jkrcal@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks, Tim! https://codereview.chromium.org/2557363002/diff/200001/components/ntp_snippet... File components/ntp_snippets/content_suggestions_service.cc (right): https://codereview.chromium.org/2557363002/diff/200001/components/ntp_snippet... components/ntp_snippets/content_suggestions_service.cc:33: remote_suggestions_scheduler_(nullptr), On 2016/12/21 08:21:18, tschumann wrote: > On 2016/12/20 16:39:46, jkrcal wrote: > > On 2016/12/19 14:17:08, tschumann wrote: > > > On 2016/12/19 12:59:23, Marc Treib (OOO until Jan 12) wrote: > > > > On 2016/12/19 11:07:19, tschumann wrote: > > > > > why do we need both? Shouldn't the SchedulingRemoteSuggestionsProvider > be > > > > > sufficient? > > > > > > > > But then that'd have to expose both members. And since they're exposed for > > > > different reasons (the provider only for the internals page, the scheduler > > > only > > > > because of plumbing reasons, to make it accessible from the scheduled > > > wake-ups), > > > > I kinda like them being separate. Not feeling very strongly though. > > > > > > hehe... so my question is more: why do we still need to expose the wrapped > > > remote-suggestions-provider. Nothing we need to fix now, but it would be > good > > to > > > understand and clean-up later. > > > > As I say above, the only reason for the moment is that the scheduling wrapper > > does not implement RemoteSuggestionsProvider). I think we can just expose the > > scheduling wrapper, using two getters. I am not sure whether to have just one > > setter or two setters (so that content_suggestions_service does not need to > know > > about implementation details). > > Oh yeah, totally. The scheduling remote suggestions provider should under no > circumstances expose it's underlying provider -- that's the whole idea of the > abstraction. > I was just wondering which pieces of the remotesuggestionsprovider are still > needed by the ContentSuggestionsService -- in an ideal world there would be no > need to make that provider anything special. > > Thanks for clarifying! Done, in the end. Scheduling... now implements RemoteSuggestionsProvider and only this guy is exposed. Via two setters and getters. https://codereview.chromium.org/2557363002/diff/200001/components/ntp_snippet... components/ntp_snippets/content_suggestions_service.cc:230: category_provider_pair.second->ReloadSuggestions(); On 2016/12/21 08:21:18, tschumann wrote: > On 2016/12/20 16:39:46, jkrcal wrote: > > On 2016/12/19 12:59:23, Marc Treib (OOO until Jan 12) wrote: > > > If a provider provides multiple categories, then it'll get called multiple > > times > > > here. > > > I think this should just iterate over providers_ (and maybe skip those that > > > haven't registered any categories?) > > > > Indeed, thanks! > > > > I do not see how to implement skipping those without any categories without > > introducing another data or without heavy iteration. I do not see the added > > value worth the effort. > > +1 for such providers that would be a no-op anyways, right? Acknowledged. https://codereview.chromium.org/2557363002/diff/220001/components/ntp_snippet... File components/ntp_snippets/remote/remote_suggestions_provider_impl.cc (right): https://codereview.chromium.org/2557363002/diff/220001/components/ntp_snippet... components/ntp_snippets/remote/remote_suggestions_provider_impl.cc:342: // Call the observer right away if we've reached any final state. On 2016/12/21 08:21:18, tschumann wrote: > you're missing a call to NotifyStatusChanged() now. Fixed in the version I uploaded yesterday :) (Cought by unit-tests, hurray!) https://codereview.chromium.org/2557363002/diff/220001/components/ntp_snippet... File components/ntp_snippets/remote/remote_suggestions_provider_impl_unittest.cc (right): https://codereview.chromium.org/2557363002/diff/220001/components/ntp_snippet... components/ntp_snippets/remote/remote_suggestions_provider_impl_unittest.cc:1668: ShouldNotCrashWhenCallingEmptyCallback) { On 2016/12/21 08:21:18, tschumann wrote: > nit: try not to mention internals here. The empty callback is quite a detail. > What you're testing is that state transitions work fine when registering an > empty callback. > > ShouldHandleDefaultConstructedProviderStatusCallbacks ? Removed in the last version. This unit-test actually conceptually fails so I had to redo the thing :)
nice tests! just some nits around the mock implementation. https://codereview.chromium.org/2557363002/diff/320001/components/ntp_snippet... File components/ntp_snippets/remote/scheduling_remote_suggestions_provider_unittest.cc (right): https://codereview.chromium.org/2557363002/diff/320001/components/ntp_snippet... components/ntp_snippets/remote/scheduling_remote_suggestions_provider_unittest.cc:55: const char* kDifferentInterval = "2"; as it is, this as very little context. I'd probably define this constant inside the 2 tests which actually use them so there's better context. https://codereview.chromium.org/2557363002/diff/320001/components/ntp_snippet... components/ntp_snippets/remote/scheduling_remote_suggestions_provider_unittest.cc:65: class MockRemoteSuggestionsProvider : public RemoteSuggestionsProvider { this guy should probably go into it's on library so that other tests can use it as well. Not necessarily in this CL but add a TODO() to follow up. https://codereview.chromium.org/2557363002/diff/320001/components/ntp_snippet... components/ntp_snippets/remote/scheduling_remote_suggestions_provider_unittest.cc:72: void SetProviderStatusCallback( a comment might be nice, something like: // SchedulingRemoteSuggestionsProvider calls this to stay in the loop of status changes. also shouldn't that be marked 'override'? While I'm fine with this for now, this should actually be a mock method (using the same pattern i explained in RefetchInTheBackground()). So that test code can listen to events, capture the callback and run it by itself. If that's too much boilerplate in the single test, you can add that EXPECT call in the test's constructor and capture the callback inside the test class. The reasoning is that the mock is not responsible for any action, including capturing that callback. https://codereview.chromium.org/2557363002/diff/320001/components/ntp_snippet... components/ntp_snippets/remote/scheduling_remote_suggestions_provider_unittest.cc:85: callback) { that method is overriding RemoteSuggestionsProvider's method right? Please add the 'override' keyword. Also, please extend the comment, e.g.: // Move-only params are not supported by GMock. We want to mock out RefetchInTheBackground() which takes a unique_ptr<>. Instead, we add a new mock function which takes a plain pointer and override the RemoteSuggestionsProvider's method to forward the call into the new mock function. https://codereview.chromium.org/2557363002/diff/320001/components/ntp_snippet... components/ntp_snippets/remote/scheduling_remote_suggestions_provider_unittest.cc:87: fetch_status_callback_ = std::move(callback); this is fishy as you're adding fake semantics. The TEST code should run the callback itself. Why not change the new MockMethod to take the parameter by value and we simply copy it? void RefetchInTheBackground( std::unique_ptr<RemoteSuggestionsProvider::FetchStatusCallback> callback) { RefetchInTheBackground(*callback); } Test code like this // Trigger a fetch. EXPECT_CALL(*underlying_provider_, RefetchInTheBackground(_)); looks really fishy as it seems you're dropping callbacks. The code should more look like this: RemoteSuggestionsProvider::FetchStatusCallback signal_fetch_done; EXPECT_CALL(*underlying_provider_, RefetchInTheBackground(_)). .WillOnce(SaveArgPointee<0>(&signal_fetch_done)); // do stuff ... signal_fetch_done.Run(Status::Success());
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2557363002/diff/340001/chrome/browser/ui/webu... File chrome/browser/ui/webui/snippets_internals_message_handler.cc (right): https://codereview.chromium.org/2557363002/diff/340001/chrome/browser/ui/webu... chrome/browser/ui/webui/snippets_internals_message_handler.cc:272: std::unique_ptr<RemoteSuggestionsProvider::FetchStatusCallback>(nullptr)); Can you see whether just passing nullptr directly works here? I vaguely remember scoped_ptr having an implicit nullptr_t constructor, so std::unique_ptr should have it too. https://codereview.chromium.org/2557363002/diff/340001/chrome/browser/ui/webu... chrome/browser/ui/webui/snippets_internals_message_handler.cc:305: ->snippets_fetcher_for_testing_and_debugging() Maybe pull this out into a local variable? https://codereview.chromium.org/2557363002/diff/340001/components/ntp_snippet... File components/ntp_snippets/content_suggestions_service.h (right): https://codereview.chromium.org/2557363002/diff/340001/components/ntp_snippet... components/ntp_snippets/content_suggestions_service.h:212: // Tim. Consider swapping the dependencies: first constructing all providers, Nit: With no disrespect to Tim, I'm not sure we should immortalize him here like that ;-) I think just "...feels wrong" would be sufficient. https://codereview.chromium.org/2557363002/diff/340001/components/ntp_snippet... File components/ntp_snippets/remote/remote_suggestions_provider.h (right): https://codereview.chromium.org/2557363002/diff/340001/components/ntp_snippet... components/ntp_snippets/remote/remote_suggestions_provider.h:26: enum ProviderStatus { ACTIVE, INACTIVE }; Nit: Can you make this an enum class? https://codereview.chromium.org/2557363002/diff/340001/components/ntp_snippet... File components/ntp_snippets/remote/remote_suggestions_provider_impl.cc (right): https://codereview.chromium.org/2557363002/diff/340001/components/ntp_snippet... components/ntp_snippets/remote/remote_suggestions_provider_impl.cc:356: std::unique_ptr<FetchStatusCallback>(nullptr)); You should also be able to just use nullptr here (and in other places in this CL). https://codereview.chromium.org/2557363002/diff/340001/components/ntp_snippet... components/ntp_snippets/remote/remote_suggestions_provider_impl.cc:958: fetch_when_ready_callback_.reset(); Is this necessary if you move the callback anyway? https://codereview.chromium.org/2557363002/diff/340001/components/ntp_snippet... File components/ntp_snippets/remote/remote_suggestions_provider_impl.h (right): https://codereview.chromium.org/2557363002/diff/340001/components/ntp_snippet... components/ntp_snippets/remote/remote_suggestions_provider_impl.h:435: // The parameters for the fetch to perform later. Nit: empty line before comments plz :)
Thanks for all the helpful comments! PTAl, again. https://codereview.chromium.org/2557363002/diff/320001/components/ntp_snippet... File components/ntp_snippets/remote/scheduling_remote_suggestions_provider_unittest.cc (right): https://codereview.chromium.org/2557363002/diff/320001/components/ntp_snippet... components/ntp_snippets/remote/scheduling_remote_suggestions_provider_unittest.cc:55: const char* kDifferentInterval = "2"; On 2016/12/21 09:41:02, tschumann wrote: > as it is, this as very little context. I'd probably define this constant inside > the 2 tests which actually use them so there's better context. Agreed, I think this is actually clear enough with the literals, directly. https://codereview.chromium.org/2557363002/diff/320001/components/ntp_snippet... components/ntp_snippets/remote/scheduling_remote_suggestions_provider_unittest.cc:65: class MockRemoteSuggestionsProvider : public RemoteSuggestionsProvider { On 2016/12/21 09:41:01, tschumann wrote: > this guy should probably go into it's on library so that other tests can use it > as well. Not necessarily in this CL but add a TODO() to follow up. I am not sure which other tests need it in the close future. Added a TODO. https://codereview.chromium.org/2557363002/diff/320001/components/ntp_snippet... components/ntp_snippets/remote/scheduling_remote_suggestions_provider_unittest.cc:72: void SetProviderStatusCallback( On 2016/12/21 09:41:02, tschumann wrote: > a comment might be nice, something like: > > // SchedulingRemoteSuggestionsProvider calls this to stay in the loop of status > changes. > > also shouldn't that be marked 'override'? > > While I'm fine with this for now, this should actually be a mock method (using > the same pattern i explained in RefetchInTheBackground()). > So that test code can listen to events, capture the callback and run it by > itself. > > If that's too much boilerplate in the single test, you can add that EXPECT call > in the test's constructor and capture the callback inside the test class. > > The reasoning is that the mock is not responsible for any action, including > capturing that callback. Added the comment. https://codereview.chromium.org/2557363002/diff/320001/components/ntp_snippet... components/ntp_snippets/remote/scheduling_remote_suggestions_provider_unittest.cc:85: callback) { On 2016/12/21 09:41:02, tschumann wrote: > that method is overriding RemoteSuggestionsProvider's method right? > Please add the 'override' keyword. > > Also, please extend the comment, e.g.: > // Move-only params are not supported by GMock. We want to mock out > RefetchInTheBackground() which takes a unique_ptr<>. Instead, we add a new mock > function which takes a plain pointer and override the > RemoteSuggestionsProvider's method to forward the call into the new mock > function. Done. https://codereview.chromium.org/2557363002/diff/320001/components/ntp_snippet... components/ntp_snippets/remote/scheduling_remote_suggestions_provider_unittest.cc:87: fetch_status_callback_ = std::move(callback); On 2016/12/21 09:41:01, tschumann wrote: > this is fishy as you're adding fake semantics. > The TEST code should run the callback itself. > Why not change the new MockMethod to take the parameter by value and we simply > copy it? > > void RefetchInTheBackground( > std::unique_ptr<RemoteSuggestionsProvider::FetchStatusCallback> > callback) { > RefetchInTheBackground(*callback); > } > > Test code like this > > // Trigger a fetch. > EXPECT_CALL(*underlying_provider_, RefetchInTheBackground(_)); > > looks really fishy as it seems you're dropping callbacks. > > The code should more look like this: > > RemoteSuggestionsProvider::FetchStatusCallback signal_fetch_done; > EXPECT_CALL(*underlying_provider_, RefetchInTheBackground(_)). > .WillOnce(SaveArgPointee<0>(&signal_fetch_done)); > // do stuff ... > signal_fetch_done.Run(Status::Success()); I am puzzled by your solution. Would not the unique pointer and the callback itself get destroyed after the function RefetchInTheBackground(std::unique_ptr<RemoteSuggestionsProvider::FetchStatusCallback> callback) is left? https://codereview.chromium.org/2557363002/diff/340001/chrome/browser/ui/webu... File chrome/browser/ui/webui/snippets_internals_message_handler.cc (right): https://codereview.chromium.org/2557363002/diff/340001/chrome/browser/ui/webu... chrome/browser/ui/webui/snippets_internals_message_handler.cc:272: std::unique_ptr<RemoteSuggestionsProvider::FetchStatusCallback>(nullptr)); On 2016/12/21 10:38:42, Bernhard Bauer wrote: > Can you see whether just passing nullptr directly works here? I vaguely remember > scoped_ptr having an implicit nullptr_t constructor, so std::unique_ptr should > have it too. Much better, thanks! https://codereview.chromium.org/2557363002/diff/340001/chrome/browser/ui/webu... chrome/browser/ui/webui/snippets_internals_message_handler.cc:305: ->snippets_fetcher_for_testing_and_debugging() On 2016/12/21 10:38:42, Bernhard Bauer wrote: > Maybe pull this out into a local variable? Done. https://codereview.chromium.org/2557363002/diff/340001/components/ntp_snippet... File components/ntp_snippets/content_suggestions_service.h (right): https://codereview.chromium.org/2557363002/diff/340001/components/ntp_snippet... components/ntp_snippets/content_suggestions_service.h:212: // Tim. Consider swapping the dependencies: first constructing all providers, On 2016/12/21 10:38:42, Bernhard Bauer wrote: > Nit: With no disrespect to Tim, I'm not sure we should immortalize him here like > that ;-) I think just "...feels wrong" would be sufficient. Done. https://codereview.chromium.org/2557363002/diff/340001/components/ntp_snippet... File components/ntp_snippets/remote/remote_suggestions_provider.h (right): https://codereview.chromium.org/2557363002/diff/340001/components/ntp_snippet... components/ntp_snippets/remote/remote_suggestions_provider.h:26: enum ProviderStatus { ACTIVE, INACTIVE }; On 2016/12/21 10:38:42, Bernhard Bauer wrote: > Nit: Can you make this an enum class? Done. Why? Is it a general rule in chromium / Zine to always use "enum class" instead of "enum"? https://codereview.chromium.org/2557363002/diff/340001/components/ntp_snippet... File components/ntp_snippets/remote/remote_suggestions_provider_impl.cc (right): https://codereview.chromium.org/2557363002/diff/340001/components/ntp_snippet... components/ntp_snippets/remote/remote_suggestions_provider_impl.cc:356: std::unique_ptr<FetchStatusCallback>(nullptr)); On 2016/12/21 10:38:42, Bernhard Bauer wrote: > You should also be able to just use nullptr here (and in other places in this > CL). Done. https://codereview.chromium.org/2557363002/diff/340001/components/ntp_snippet... components/ntp_snippets/remote/remote_suggestions_provider_impl.cc:958: fetch_when_ready_callback_.reset(); On 2016/12/21 10:38:42, Bernhard Bauer wrote: > Is this necessary if you move the callback anyway? Not needed, you are right. I just remembered the general semantics rule that std::moving something leaves it in an unspecified state. std::unique_ptr luckily gives a more concrete contract :) https://codereview.chromium.org/2557363002/diff/340001/components/ntp_snippet... File components/ntp_snippets/remote/remote_suggestions_provider_impl.h (right): https://codereview.chromium.org/2557363002/diff/340001/components/ntp_snippet... components/ntp_snippets/remote/remote_suggestions_provider_impl.h:435: // The parameters for the fetch to perform later. On 2016/12/21 10:38:42, Bernhard Bauer wrote: > Nit: empty line before comments plz :) Done.
https://codereview.chromium.org/2557363002/diff/340001/components/ntp_snippet... File components/ntp_snippets/remote/remote_suggestions_provider.h (right): https://codereview.chromium.org/2557363002/diff/340001/components/ntp_snippet... components/ntp_snippets/remote/remote_suggestions_provider.h:26: enum ProviderStatus { ACTIVE, INACTIVE }; On 2016/12/21 11:54:40, jkrcal wrote: > On 2016/12/21 10:38:42, Bernhard Bauer wrote: > > Nit: Can you make this an enum class? > > Done. > > Why? Is it a general rule in chromium / Zine to always use "enum class" instead > of "enum"? No, but there are some advantages, like the additional namespacing (RemoteSuggestionsProvider::ProviderStatus::ACTIVE makes a bit more sense to me than justRemoteSuggestionsProvider::ACTIVE [if it's too long, we could remove the "Provider" from ProviderStatus, as that's redundant]) or the fact that it doesn't have implicit conversions to bool. That being said, there are also disadvantages, so I'm not going to push for either. https://codereview.chromium.org/2557363002/diff/340001/components/ntp_snippet... File components/ntp_snippets/remote/remote_suggestions_provider_impl.cc (right): https://codereview.chromium.org/2557363002/diff/340001/components/ntp_snippet... components/ntp_snippets/remote/remote_suggestions_provider_impl.cc:958: fetch_when_ready_callback_.reset(); On 2016/12/21 11:54:40, jkrcal wrote: > On 2016/12/21 10:38:42, Bernhard Bauer wrote: > > Is this necessary if you move the callback anyway? > > Not needed, you are right. > > I just remembered the general semantics rule that std::moving something leaves > it in an unspecified state. std::unique_ptr luckily gives a more concrete > contract :) Oh hey, actually now that you mention it: We could store the callbacks directly instead of in unique_ptr<>s (and also pass them around by value as per https://chromium.googlesource.com/chromium/src/+/master/docs/callback.md#Memo...). I think Callback also resets cleanly when it's moved (its only member is a scoped_refptr to the state, which explicitly resets, see https://cs.chromium.org/chromium/src/base/memory/ref_counted.h?sq=package:chr...).
I also implemented the unit-test changes suggested by Tim (letting the Mock be just a Mock). https://codereview.chromium.org/2557363002/diff/340001/components/ntp_snippet... File components/ntp_snippets/remote/remote_suggestions_provider.h (right): https://codereview.chromium.org/2557363002/diff/340001/components/ntp_snippet... components/ntp_snippets/remote/remote_suggestions_provider.h:26: enum ProviderStatus { ACTIVE, INACTIVE }; On 2016/12/21 12:24:36, Bernhard Bauer wrote: > On 2016/12/21 11:54:40, jkrcal wrote: > > On 2016/12/21 10:38:42, Bernhard Bauer wrote: > > > Nit: Can you make this an enum class? > > > > Done. > > > > Why? Is it a general rule in chromium / Zine to always use "enum class" > instead > > of "enum"? > > No, but there are some advantages, like the additional namespacing > (RemoteSuggestionsProvider::ProviderStatus::ACTIVE makes a bit more sense to me > than justRemoteSuggestionsProvider::ACTIVE [if it's too long, we could remove > the "Provider" from ProviderStatus, as that's redundant]) or the fact that it > doesn't have implicit conversions to bool. That being said, there are also > disadvantages, so I'm not going to push for either. Thanks for the explanation! https://codereview.chromium.org/2557363002/diff/340001/components/ntp_snippet... File components/ntp_snippets/remote/remote_suggestions_provider_impl.cc (right): https://codereview.chromium.org/2557363002/diff/340001/components/ntp_snippet... components/ntp_snippets/remote/remote_suggestions_provider_impl.cc:958: fetch_when_ready_callback_.reset(); On 2016/12/21 12:24:36, Bernhard Bauer wrote: > On 2016/12/21 11:54:40, jkrcal wrote: > > On 2016/12/21 10:38:42, Bernhard Bauer wrote: > > > Is this necessary if you move the callback anyway? > > > > Not needed, you are right. > > > > I just remembered the general semantics rule that std::moving something leaves > > it in an unspecified state. std::unique_ptr luckily gives a more concrete > > contract :) > > Oh hey, actually now that you mention it: We could store the callbacks directly > instead of in unique_ptr<>s (and also pass them around by value as per > https://chromium.googlesource.com/chromium/src/+/master/docs/callback.md#Memo...). > I think Callback also resets cleanly when it's moved (its only member is a > scoped_refptr to the state, which explicitly resets, see > https://cs.chromium.org/chromium/src/base/memory/ref_counted.h?sq=package:chr...). I had it like that before but changed to unique_ptr because I needed to support empty callbacks. Maybe I did not try hard (I find documentation / the right source code for callbacks hard to find). Does it behave like a pointer? Can I write auto callback = Callback<type>() to create an empty callback and test its non-emptyness by if (callback) { callback.Run(); } ? If so, the last question: can I leave it for a later CL, please? ;) (the time is pressing and I would like to actually start implementing the soft fetches)
lgtm https://codereview.chromium.org/2557363002/diff/340001/components/ntp_snippet... File components/ntp_snippets/remote/remote_suggestions_provider_impl.cc (right): https://codereview.chromium.org/2557363002/diff/340001/components/ntp_snippet... components/ntp_snippets/remote/remote_suggestions_provider_impl.cc:958: fetch_when_ready_callback_.reset(); On 2016/12/21 12:24:36, Bernhard Bauer wrote: > On 2016/12/21 11:54:40, jkrcal wrote: > > On 2016/12/21 10:38:42, Bernhard Bauer wrote: > > > Is this necessary if you move the callback anyway? > > > > Not needed, you are right. > > > > I just remembered the general semantics rule that std::moving something leaves > > it in an unspecified state. std::unique_ptr luckily gives a more concrete > > contract :) > > Oh hey, actually now that you mention it: We could store the callbacks directly > instead of in unique_ptr<>s (and also pass them around by value as per > https://chromium.googlesource.com/chromium/src/+/master/docs/callback.md#Memo...). > I think Callback also resets cleanly when it's moved (its only member is a > scoped_refptr to the state, which explicitly resets, see > https://cs.chromium.org/chromium/src/base/memory/ref_counted.h?sq=package:chr...). let's not rely on undocumented properties here. Unless it's explicitly stated in the callback's documentation that the moved-out state is as we expect it, we shouldn't rely on it. It's tricky enough. IMO, the unique_ptr<> doesn't add a lot of noise but makes it very clear to the reader what the possible states are. Another option would be to use Optional, but IMO that's a bit clumsy to use. The nice thing about the unique_ptr is that callers can also unregister themselves cleanly.
The CQ bit was checked by jkrcal@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
LGTM https://codereview.chromium.org/2557363002/diff/340001/components/ntp_snippet... File components/ntp_snippets/remote/remote_suggestions_provider_impl.cc (right): https://codereview.chromium.org/2557363002/diff/340001/components/ntp_snippet... components/ntp_snippets/remote/remote_suggestions_provider_impl.cc:958: fetch_when_ready_callback_.reset(); On 2016/12/21 12:41:20, jkrcal wrote: > On 2016/12/21 12:24:36, Bernhard Bauer wrote: > > On 2016/12/21 11:54:40, jkrcal wrote: > > > On 2016/12/21 10:38:42, Bernhard Bauer wrote: > > > > Is this necessary if you move the callback anyway? > > > > > > Not needed, you are right. > > > > > > I just remembered the general semantics rule that std::moving something > leaves > > > it in an unspecified state. std::unique_ptr luckily gives a more concrete > > > contract :) > > > > Oh hey, actually now that you mention it: We could store the callbacks > directly > > instead of in unique_ptr<>s (and also pass them around by value as per > > > https://chromium.googlesource.com/chromium/src/+/master/docs/callback.md#Memo...). > > I think Callback also resets cleanly when it's moved (its only member is a > > scoped_refptr to the state, which explicitly resets, see > > > https://cs.chromium.org/chromium/src/base/memory/ref_counted.h?sq=package:chr...). > > I had it like that before but changed to unique_ptr because I needed to support > empty callbacks. Maybe I did not try hard (I find documentation / the right > source code for callbacks hard to find). Does it behave like a pointer? > > Can I write > auto callback = Callback<type>() > to create an empty callback and test its non-emptyness by > if (callback) { > callback.Run(); > } > ? Yup. (There is an explicit is_null() method, but also a bool() operator, so using it directly works.) > If so, the last question: can I leave it for a later CL, please? ;) > (the time is pressing and I would like to actually start implementing the soft > fetches) Yeah, sure. https://codereview.chromium.org/2557363002/diff/340001/components/ntp_snippet... components/ntp_snippets/remote/remote_suggestions_provider_impl.cc:958: fetch_when_ready_callback_.reset(); On 2016/12/21 12:42:51, tschumann wrote: > On 2016/12/21 12:24:36, Bernhard Bauer wrote: > > On 2016/12/21 11:54:40, jkrcal wrote: > > > On 2016/12/21 10:38:42, Bernhard Bauer wrote: > > > > Is this necessary if you move the callback anyway? > > > > > > Not needed, you are right. > > > > > > I just remembered the general semantics rule that std::moving something > leaves > > > it in an unspecified state. std::unique_ptr luckily gives a more concrete > > > contract :) > > > > Oh hey, actually now that you mention it: We could store the callbacks > directly > > instead of in unique_ptr<>s (and also pass them around by value as per > > > https://chromium.googlesource.com/chromium/src/+/master/docs/callback.md#Memo...). > > I think Callback also resets cleanly when it's moved (its only member is a > > scoped_refptr to the state, which explicitly resets, see > > > https://cs.chromium.org/chromium/src/base/memory/ref_counted.h?sq=package:chr...). > > let's not rely on undocumented properties here. Unless it's explicitly stated in > the callback's documentation that the moved-out state is as we expect it, we > shouldn't rely on it. It's tricky enough. > IMO, the unique_ptr<> doesn't add a lot of noise but makes it very clear to the > reader what the possible states are. Another option would be to use Optional, > but IMO that's a bit clumsy to use. The nice thing about the unique_ptr is that > callers can also unregister themselves cleanly. The explicit recommendation is to pass callbacks by value, and for OnceCallbacks std::move()ing them is the only way to get an rvalue, so, yes, I would assume that my colleagues are in fact not incompetent and have made moving callbacks possible without undefined behavior. If you're concerned about the lack of documentation, I'm more than happy to clarify the behavior with the authors and add documentation.
The CQ bit was unchecked by jkrcal@chromium.org
lgtm https://codereview.chromium.org/2557363002/diff/340001/components/ntp_snippet... File components/ntp_snippets/remote/remote_suggestions_provider_impl.cc (right): https://codereview.chromium.org/2557363002/diff/340001/components/ntp_snippet... components/ntp_snippets/remote/remote_suggestions_provider_impl.cc:958: fetch_when_ready_callback_.reset(); On 2016/12/21 13:16:33, Bernhard Bauer wrote: > On 2016/12/21 12:42:51, tschumann wrote: > > On 2016/12/21 12:24:36, Bernhard Bauer wrote: > > > On 2016/12/21 11:54:40, jkrcal wrote: > > > > On 2016/12/21 10:38:42, Bernhard Bauer wrote: > > > > > Is this necessary if you move the callback anyway? > > > > > > > > Not needed, you are right. > > > > > > > > I just remembered the general semantics rule that std::moving something > > leaves > > > > it in an unspecified state. std::unique_ptr luckily gives a more concrete > > > > contract :) > > > > > > Oh hey, actually now that you mention it: We could store the callbacks > > directly > > > instead of in unique_ptr<>s (and also pass them around by value as per > > > > > > https://chromium.googlesource.com/chromium/src/+/master/docs/callback.md#Memo...). > > > I think Callback also resets cleanly when it's moved (its only member is a > > > scoped_refptr to the state, which explicitly resets, see > > > > > > https://cs.chromium.org/chromium/src/base/memory/ref_counted.h?sq=package:chr...). > > > > let's not rely on undocumented properties here. Unless it's explicitly stated > in > > the callback's documentation that the moved-out state is as we expect it, we > > shouldn't rely on it. It's tricky enough. > > IMO, the unique_ptr<> doesn't add a lot of noise but makes it very clear to > the > > reader what the possible states are. Another option would be to use Optional, > > but IMO that's a bit clumsy to use. The nice thing about the unique_ptr is > that > > callers can also unregister themselves cleanly. > > The explicit recommendation is to pass callbacks by value, and for OnceCallbacks > std::move()ing them is the only way to get an rvalue, so, yes, I would assume > that my colleagues are in fact not incompetent and have made moving callbacks > possible without undefined behavior. If you're concerned about the lack of > documentation, I'm more than happy to clarify the behavior with the authors and > add documentation. hehehe... not questioning competence; but usually people tend to not think too much of the moved-out state as most best practices are about not touching it then. However, I'm not sure we have the problem of the moved-out state anyways. Happy to have it changed back to value semantics given the is_null() check we missed before :-)
Thanks! https://codereview.chromium.org/2557363002/diff/340001/components/ntp_snippet... File components/ntp_snippets/remote/remote_suggestions_provider_impl.cc (right): https://codereview.chromium.org/2557363002/diff/340001/components/ntp_snippet... components/ntp_snippets/remote/remote_suggestions_provider_impl.cc:958: fetch_when_ready_callback_.reset(); On 2016/12/21 13:33:47, tschumann wrote: > On 2016/12/21 13:16:33, Bernhard Bauer wrote: > > On 2016/12/21 12:42:51, tschumann wrote: > > > On 2016/12/21 12:24:36, Bernhard Bauer wrote: > > > > On 2016/12/21 11:54:40, jkrcal wrote: > > > > > On 2016/12/21 10:38:42, Bernhard Bauer wrote: > > > > > > Is this necessary if you move the callback anyway? > > > > > > > > > > Not needed, you are right. > > > > > > > > > > I just remembered the general semantics rule that std::moving something > > > leaves > > > > > it in an unspecified state. std::unique_ptr luckily gives a more > concrete > > > > > contract :) > > > > > > > > Oh hey, actually now that you mention it: We could store the callbacks > > > directly > > > > instead of in unique_ptr<>s (and also pass them around by value as per > > > > > > > > > > https://chromium.googlesource.com/chromium/src/+/master/docs/callback.md#Memo...). > > > > I think Callback also resets cleanly when it's moved (its only member is a > > > > scoped_refptr to the state, which explicitly resets, see > > > > > > > > > > https://cs.chromium.org/chromium/src/base/memory/ref_counted.h?sq=package:chr...). > > > > > > let's not rely on undocumented properties here. Unless it's explicitly > stated > > in > > > the callback's documentation that the moved-out state is as we expect it, we > > > shouldn't rely on it. It's tricky enough. > > > IMO, the unique_ptr<> doesn't add a lot of noise but makes it very clear to > > the > > > reader what the possible states are. Another option would be to use > Optional, > > > but IMO that's a bit clumsy to use. The nice thing about the unique_ptr is > > that > > > callers can also unregister themselves cleanly. > > > > The explicit recommendation is to pass callbacks by value, and for > OnceCallbacks > > std::move()ing them is the only way to get an rvalue, so, yes, I would assume > > that my colleagues are in fact not incompetent and have made moving callbacks > > possible without undefined behavior. If you're concerned about the lack of > > documentation, I'm more than happy to clarify the behavior with the authors > and > > add documentation. > > hehehe... not questioning competence; but usually people tend to not think too > much of the moved-out state as most best practices are about not touching it > then. > However, I'm not sure we have the problem of the moved-out state anyways. Happy > to have it changed back to value semantics given the is_null() check we missed > before :-) I file a bug for this.
The CQ bit was checked by fhorschig@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from noyau@chromium.org Link to the patchset: https://codereview.chromium.org/2557363002/#ps400001 (title: "Fixing the last changes :)")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 400001, "attempt_start_ts": 1482336339316720,
"parent_rev": "b8be1bdcff23493268faecd7cd9e34739efcc846", "commit_rev":
"508a4f908f592de44f3cdf054aebbd51367eafb5"}
Message was sent while issue was closed.
Description was changed from ========== [NTP Snippets] Refactor background scheduling for remote suggestions This CL splits off scheduling functionality from RemoteSuggestionsProvider into a separate class. RemoteSuggestionsProvider is a complex class. This is to prepare for further work on soft-scheduled updates (wich makes scheduling slightly more complex). The CL also renames the getter for RemoteSuggestionsProvider as it would anyway touch almost all of its callers. BUG=672479 ========== to ========== [NTP Snippets] Refactor background scheduling for remote suggestions This CL splits off scheduling functionality from RemoteSuggestionsProvider into a separate class. RemoteSuggestionsProvider is a complex class. This is to prepare for further work on soft-scheduled updates (wich makes scheduling slightly more complex). The CL also renames the getter for RemoteSuggestionsProvider as it would anyway touch almost all of its callers. BUG=672479 Review-Url: https://codereview.chromium.org/2557363002 ==========
Message was sent while issue was closed.
Committed patchset #21 (id:400001)
Message was sent while issue was closed.
Description was changed from ========== [NTP Snippets] Refactor background scheduling for remote suggestions This CL splits off scheduling functionality from RemoteSuggestionsProvider into a separate class. RemoteSuggestionsProvider is a complex class. This is to prepare for further work on soft-scheduled updates (wich makes scheduling slightly more complex). The CL also renames the getter for RemoteSuggestionsProvider as it would anyway touch almost all of its callers. BUG=672479 Review-Url: https://codereview.chromium.org/2557363002 ========== to ========== [NTP Snippets] Refactor background scheduling for remote suggestions This CL splits off scheduling functionality from RemoteSuggestionsProvider into a separate class. RemoteSuggestionsProvider is a complex class. This is to prepare for further work on soft-scheduled updates (wich makes scheduling slightly more complex). The CL also renames the getter for RemoteSuggestionsProvider as it would anyway touch almost all of its callers. BUG=672479 Committed: https://crrev.com/093410c469a8105341cdf0062e65a5c5ac0ef4b8 Cr-Commit-Position: refs/heads/master@{#440116} ==========
Message was sent while issue was closed.
Patchset 21 (id:??) landed as https://crrev.com/093410c469a8105341cdf0062e65a5c5ac0ef4b8 Cr-Commit-Position: refs/heads/master@{#440116} |
