Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2702223004:
[Remote suggestions] Move all decisions to fetch to the scheduler (Closed)
Description[Remote suggestions] Move all decisions to fetch to the scheduler
This change will clarify responsibilities between the scheduler and the
provider. On top of that, it eliminates the need to fetch snippets upon
creating the provider (which is dangerous w.r.t. fetches in unexpected
situations).
This change needs the new types of notifications from the provider to
the scheduler:
- suggestions have been cleared (we should fetch on any later trigger),
- history has been cleared (we should NOT fetch for a while).
Thus, the provider is now given a pointer to the scheduler interface.
(the interface now mixes internal and external signals but I think it is
this way easier to understand then with two separate interfaces).
BUG=694324
Review-Url: https://codereview.chromium.org/2702223004
Cr-Commit-Position: refs/heads/master@{#454240}
Committed: https://chromium.googlesource.com/chromium/src/+/6dd692b945fc8599a4c7db22854d19c9b27d0d4c
Patch Set 1 #
Total comments: 4
Patch Set 2 : Tim's comments #1 #Patch Set 3 : Add unit-tests #Patch Set 4 : More unit-tests #
Total comments: 6
Patch Set 5 : Tim's comments #2 #
Total comments: 8
Patch Set 6 : Tim's comments #3 #
Total comments: 16
Patch Set 7 : Marc's comments #
Total comments: 3
Patch Set 8 : Tim's comments #4 #Patch Set 9 : Fix unit-tests #Patch Set 10 : Rebase #Patch Set 11 : Fixing an embarassing bug :) #Dependent Patchsets: Messages
Total messages: 64 (34 generated)
jkrcal@chromium.org changed reviewers: + tschumann@chromium.org
Tim, could you please take a quick look? The CL is not complete (I haven't added new unit-tests, yet). Before I do that, I would like to get your view on the extended interface between RemoteSuggestionsProvider and RemoteSuggestionsScheduler.
some quick comments, overall the direction looks good to me! https://codereview.chromium.org/2702223004/diff/1/components/ntp_snippets/rem... File components/ntp_snippets/remote/remote_suggestions_provider.h (right): https://codereview.chromium.org/2702223004/diff/1/components/ntp_snippets/rem... components/ntp_snippets/remote/remote_suggestions_provider.h:35: RemoteSuggestionsScheduler* scheduler) = 0; I'm not a fan of this cyclic dependency -- it feels weird that the provider could schedule calls for itself. Also, the new methods added to the RemoteSuggestionsScheduler could make a nice separate interface. Adding another Observer interface to the RemoteSuggestionsProvider however seems also odd because we already have an observer class on it's base class... Maybe it's best for now to go with your plan :-) https://codereview.chromium.org/2702223004/diff/1/components/ntp_snippets/rem... File components/ntp_snippets/remote/remote_suggestions_provider_impl.cc (right): https://codereview.chromium.org/2702223004/diff/1/components/ntp_snippets/rem... components/ntp_snippets/remote/remote_suggestions_provider_impl.cc:886: remote_suggestions_scheduler_->OnHistoryCleared(); this results in 2 calls to the scheduler -- would be nice to have the events exclusive, wdyt?
Addressed one of the comments, working on unit-tests now. https://codereview.chromium.org/2702223004/diff/1/components/ntp_snippets/rem... File components/ntp_snippets/remote/remote_suggestions_provider.h (right): https://codereview.chromium.org/2702223004/diff/1/components/ntp_snippets/rem... components/ntp_snippets/remote/remote_suggestions_provider.h:35: RemoteSuggestionsScheduler* scheduler) = 0; On 2017/02/21 11:57:32, tschumann wrote: > I'm not a fan of this cyclic dependency -- it feels weird that the provider > could schedule calls for itself. > > Also, the new methods added to the RemoteSuggestionsScheduler could make a nice > separate interface. > > Adding another Observer interface to the RemoteSuggestionsProvider however seems > also odd because we already have an observer class on it's base class... > > Maybe it's best for now to go with your plan :-) I am also not super happy about that. However, some generic Observer interface would obfuscate the meaning of these functions, IMO, as they are really targeted at someone who cares about when to fetch and when not to fetch - the scheduler. Thus (and for the reason you mention), I prefer to stick with the current design. https://codereview.chromium.org/2702223004/diff/1/components/ntp_snippets/rem... File components/ntp_snippets/remote/remote_suggestions_provider_impl.cc (right): https://codereview.chromium.org/2702223004/diff/1/components/ntp_snippets/rem... components/ntp_snippets/remote/remote_suggestions_provider_impl.cc:886: remote_suggestions_scheduler_->OnHistoryCleared(); On 2017/02/21 11:57:32, tschumann wrote: > this results in 2 calls to the scheduler -- would be nice to have the events > exclusive, wdyt? Good point! Renamed the other function and moved its call.
Uploaded unit-test changes, PTAL!
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: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
On 2017/02/21 17:56:30, jkrcal wrote: > Uploaded unit-test changes, PTAL! Tim, I've added some more unit-tests that I've forgotten about last time. PTAL! (other M58 changes are blocked on this CL)
didn't have a close look at the scheduler yet, though. https://codereview.chromium.org/2702223004/diff/60001/components/ntp_snippets... File components/ntp_snippets/remote/remote_suggestions_provider_impl.cc (right): https://codereview.chromium.org/2702223004/diff/60001/components/ntp_snippets... components/ntp_snippets/remote/remote_suggestions_provider_impl.cc:1090: remote_suggestions_scheduler_->OnProviderInactivated(); (outside this CL): We should investigate if we really need this state -- seems the same as DISABLED. https://codereview.chromium.org/2702223004/diff/60001/components/ntp_snippets... File components/ntp_snippets/remote/remote_suggestions_provider_impl.h (right): https://codereview.chromium.org/2702223004/diff/60001/components/ntp_snippets... components/ntp_snippets/remote/remote_suggestions_provider_impl.h:213: FRIEND_TEST_ALL_PREFIXES(RemoteSuggestionsProviderImplTest, please add a comment why these tests need to be friends. Honestly we should try to avoid them. Seems we only use these methods for injecting the error states. Honestly, I'd be happier if we could exercise the whole error path in tests (and make the provider get into the error condition by itself). Not necessarily in this CL, however, it would be nice if the new tests could already trigger the error through public interfaces. https://codereview.chromium.org/2702223004/diff/60001/components/ntp_snippets... components/ntp_snippets/remote/remote_suggestions_provider_impl.h:362: void ObsoleteSuggestions(); i'd like to avoid coining new terms -- it's pretty unclear what obsolete means. Why not simply ClearSuggestions()? -- the reason should be irrelevant. Then we have 2 distinct events -- ClearAnyHistory() [==leave no traces] and ClearSuggestions() [just removes what we currently have].
Thanks, PTAL again! https://codereview.chromium.org/2702223004/diff/60001/components/ntp_snippets... File components/ntp_snippets/remote/remote_suggestions_provider_impl.cc (right): https://codereview.chromium.org/2702223004/diff/60001/components/ntp_snippets... components/ntp_snippets/remote/remote_suggestions_provider_impl.cc:1090: remote_suggestions_scheduler_->OnProviderInactivated(); On 2017/02/22 11:31:34, tschumann wrote: > (outside this CL): We should investigate if we really need this state -- seems > the same as DISABLED. The difference is in the transition diagram in the header file. Not sure it is needed to keep this distinction. Added a todo. https://codereview.chromium.org/2702223004/diff/60001/components/ntp_snippets... File components/ntp_snippets/remote/remote_suggestions_provider_impl.h (right): https://codereview.chromium.org/2702223004/diff/60001/components/ntp_snippets... components/ntp_snippets/remote/remote_suggestions_provider_impl.h:213: FRIEND_TEST_ALL_PREFIXES(RemoteSuggestionsProviderImplTest, On 2017/02/22 11:31:34, tschumann wrote: > please add a comment why these tests need to be friends. > Honestly we should try to avoid them. Seems we only use these methods for > injecting the error states. Honestly, I'd be happier if we could exercise the > whole error path in tests (and make the provider get into the error condition by > itself). > > Not necessarily in this CL, however, it would be nice if the new tests could > already trigger the error through public interfaces. Agreed. I did a clean-up in the friends, removed some stale ones. Most of these friends (incl. the new ones) boil down to mocking the status service. This should be done. I vote for "not-in-this-CL". Added TODOs. https://codereview.chromium.org/2702223004/diff/60001/components/ntp_snippets... components/ntp_snippets/remote/remote_suggestions_provider_impl.h:362: void ObsoleteSuggestions(); On 2017/02/22 11:31:34, tschumann wrote: > i'd like to avoid coining new terms -- it's pretty unclear what obsolete means. > Why not simply ClearSuggestions()? -- the reason should be irrelevant. > Then we have 2 distinct events -- ClearAnyHistory() [==leave no traces] and > ClearSuggestions() [just removes what we currently have]. Makes sense. Done.
lgtm Some more nits. Once addressed feel free to submit. https://codereview.chromium.org/2702223004/diff/80001/components/ntp_snippets... File components/ntp_snippets/remote/remote_suggestions_provider_impl_unittest.cc (right): https://codereview.chromium.org/2702223004/diff/80001/components/ntp_snippets... components/ntp_snippets/remote/remote_suggestions_provider_impl_unittest.cc:720: LoadFromJSONString(service.get(), json_str); So, this change is needed because we don't do the fetch on start-up anymore? https://codereview.chromium.org/2702223004/diff/80001/components/ntp_snippets... File components/ntp_snippets/remote/scheduling_remote_suggestions_provider.cc (right): https://codereview.chromium.org/2702223004/diff/80001/components/ntp_snippets... components/ntp_snippets/remote/scheduling_remote_suggestions_provider.cc:380: ClearLastFetchAttemptTime(); This change is unexpected. Why is this needed now? Or was this a bug? https://codereview.chromium.org/2702223004/diff/80001/components/ntp_snippets... File components/ntp_snippets/remote/scheduling_remote_suggestions_provider_unittest.cc (right): https://codereview.chromium.org/2702223004/diff/80001/components/ntp_snippets... components/ntp_snippets/remote/scheduling_remote_suggestions_provider_unittest.cc:643: EXPECT_CALL(persistent_scheduler_, Schedule(_, _)); i'd much rather setup the expectations where they should happen. This also enforces an order but gives better context and makes reading the test easier (it's usually also easier to maintain as expectations and trigger actions are closer together). // First enable the scheduler -- this will trigger a fetch. EXPECT_CALL(persistent_scheduler_, Schedule(_, _)); ActivateUnderlyingProvider(); // Clear the history. scheduling_provider_->OnHistoryCleared(); // A trigger after 15 minutes is ignored. test_clock_->Advance(base::TimeDelta::FromMinutes(15)); scheduling_provider_->OnBrowserForegrounded(); // A trigger after another 16 minutes is performed (more than 30m after // clearing the history). EXPECT_CALL(*underlying_provider_, RefetchInTheBackground(_)); test_clock_->Advance(base::TimeDelta::FromMinutes(16)); scheduling_provider_->OnBrowserForegrounded(); https://codereview.chromium.org/2702223004/diff/80001/components/ntp_snippets... components/ntp_snippets/remote/scheduling_remote_suggestions_provider_unittest.cc:668: EXPECT_CALL(persistent_scheduler_, Schedule(_, _)); same here. Consider moving the EXPECT_CALL statements where they get triggered.
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! https://codereview.chromium.org/2702223004/diff/80001/components/ntp_snippets... File components/ntp_snippets/remote/remote_suggestions_provider_impl_unittest.cc (right): https://codereview.chromium.org/2702223004/diff/80001/components/ntp_snippets... components/ntp_snippets/remote/remote_suggestions_provider_impl_unittest.cc:720: LoadFromJSONString(service.get(), json_str); On 2017/02/22 20:46:04, tschumann wrote: > So, this change is needed because we don't do the fetch on start-up anymore? Exactly. https://codereview.chromium.org/2702223004/diff/80001/components/ntp_snippets... File components/ntp_snippets/remote/scheduling_remote_suggestions_provider.cc (right): https://codereview.chromium.org/2702223004/diff/80001/components/ntp_snippets... components/ntp_snippets/remote/scheduling_remote_suggestions_provider.cc:380: ClearLastFetchAttemptTime(); On 2017/02/22 20:46:04, tschumann wrote: > This change is unexpected. Why is this needed now? Or was this a bug? Not a bug, not strictly needed. Okay, I remove it as the improvement is arguable ;) https://codereview.chromium.org/2702223004/diff/80001/components/ntp_snippets... File components/ntp_snippets/remote/scheduling_remote_suggestions_provider_unittest.cc (right): https://codereview.chromium.org/2702223004/diff/80001/components/ntp_snippets... components/ntp_snippets/remote/scheduling_remote_suggestions_provider_unittest.cc:643: EXPECT_CALL(persistent_scheduler_, Schedule(_, _)); On 2017/02/22 20:46:04, tschumann wrote: > i'd much rather setup the expectations where they should happen. This also > enforces an order but gives better context and makes reading the test easier > (it's usually also easier to maintain as expectations and trigger actions are > closer together). > > // First enable the scheduler -- this will trigger a fetch. > EXPECT_CALL(persistent_scheduler_, Schedule(_, _)); > ActivateUnderlyingProvider(); > > // Clear the history. > scheduling_provider_->OnHistoryCleared(); > > // A trigger after 15 minutes is ignored. > test_clock_->Advance(base::TimeDelta::FromMinutes(15)); > scheduling_provider_->OnBrowserForegrounded(); > > // A trigger after another 16 minutes is performed (more than 30m after > // clearing the history). > EXPECT_CALL(*underlying_provider_, RefetchInTheBackground(_)); > test_clock_->Advance(base::TimeDelta::FromMinutes(16)); > scheduling_provider_->OnBrowserForegrounded(); Done. https://codereview.chromium.org/2702223004/diff/80001/components/ntp_snippets... components/ntp_snippets/remote/scheduling_remote_suggestions_provider_unittest.cc:668: EXPECT_CALL(persistent_scheduler_, Schedule(_, _)); On 2017/02/22 20:46:04, tschumann wrote: > same here. Consider moving the EXPECT_CALL statements where they get triggered. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by jkrcal@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tschumann@chromium.org Link to the patchset: https://codereview.chromium.org/2702223004/#ps100001 (title: "Tim's comments #3")
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
No L-G-T-M from a valid reviewer yet. CQ run can only be started by full committers or once the patch has received an L-G-T-M from a full committer. Even if an L-G-T-M may have been provided, it was from a non-committer, _not_ a full super star committer. Committers are members of the group "project-chromium-committers". Note that this has nothing to do with OWNERS files.
jkrcal@chromium.org changed reviewers: + treib@chromium.org
Marc, is it even possible that Tim is not a committer? Could you please approve the CL yourself as well?
Some comments/questions. I'm starting to seriously doubt the design of RemoteSuggestionsProviderImpl and SchedulingRemoteSuggestionsProvider implementing the same common interface. See below. https://codereview.chromium.org/2702223004/diff/100001/components/ntp_snippet... File components/ntp_snippets/remote/remote_suggestions_provider_impl.cc (right): https://codereview.chromium.org/2702223004/diff/100001/components/ntp_snippet... components/ntp_snippets/remote/remote_suggestions_provider_impl.cc:892: NukeAllSuggestions(); Should this also check ready()? Or if this can't be called before we're ready, DCHECK? Or add a comment for why we don't need to check? https://codereview.chromium.org/2702223004/diff/100001/components/ntp_snippet... components/ntp_snippets/remote/remote_suggestions_provider_impl.cc:894: remote_suggestions_scheduler_->OnSuggestionsCleared(); Here we first nuke, then notify; in the method above it's the other way around. Can we be consistent? https://codereview.chromium.org/2702223004/diff/100001/components/ntp_snippet... components/ntp_snippets/remote/remote_suggestions_provider_impl.cc:1087: remote_suggestions_scheduler_->OnProviderInactivated(); nit: s/Inactivated/Deactivated/ (I don't think inactivate is a word) https://codereview.chromium.org/2702223004/diff/100001/components/ntp_snippet... File components/ntp_snippets/remote/remote_suggestions_provider_impl.h (right): https://codereview.chromium.org/2702223004/diff/100001/components/ntp_snippet... components/ntp_snippets/remote/remote_suggestions_provider_impl.h:200: // TODO(jkrcal): Mock the database to trigger the error naturally (or remote s/remote/remove/ ? https://codereview.chromium.org/2702223004/diff/100001/components/ntp_snippet... components/ntp_snippets/remote/remote_suggestions_provider_impl.h:357: void ClearAnyHistory(); Should this be "AnyHistoryCleared"? It doesn't clear history itself; it's called when a history clear event occurs (right?) Or alternatively something like ClearHistoryDependentState? (with a less-awkward name, if possible :D) https://codereview.chromium.org/2702223004/diff/100001/components/ntp_snippet... File components/ntp_snippets/remote/scheduling_remote_suggestions_provider.cc (right): https://codereview.chromium.org/2702223004/diff/100001/components/ntp_snippet... components/ntp_snippets/remote/scheduling_remote_suggestions_provider.cc:197: provider_->SetRemoteSuggestionsScheduler(this); This is a bit weird: We now have two-way linking between the SchedulingProvider and the Impl. And this method is defined on the common interface, but it should never be called on this class. Also, do we need to set it back to nullptr at some point? What if the provider tries to call the scheduler during destruction, something like that? https://codereview.chromium.org/2702223004/diff/100001/components/ntp_snippet... components/ntp_snippets/remote/scheduling_remote_suggestions_provider.cc:231: // asks for new suggestions) do give sync the time to propagate the changes in s/do/to/ https://codereview.chromium.org/2702223004/diff/100001/components/ntp_snippet... components/ntp_snippets/remote/scheduling_remote_suggestions_provider.cc:279: void SchedulingRemoteSuggestionsProvider::SetRemoteSuggestionsScheduler( Can this method ever be called (on this particular class)?
Thanks, Marc, PTAL! https://codereview.chromium.org/2702223004/diff/100001/components/ntp_snippet... File components/ntp_snippets/remote/remote_suggestions_provider_impl.cc (right): https://codereview.chromium.org/2702223004/diff/100001/components/ntp_snippet... components/ntp_snippets/remote/remote_suggestions_provider_impl.cc:892: NukeAllSuggestions(); On 2017/02/23 13:52:24, Marc Treib wrote: > Should this also check ready()? Or if this can't be called before we're ready, > DCHECK? Or add a comment for why we don't need to check? Done. https://codereview.chromium.org/2702223004/diff/100001/components/ntp_snippet... components/ntp_snippets/remote/remote_suggestions_provider_impl.cc:894: remote_suggestions_scheduler_->OnSuggestionsCleared(); On 2017/02/23 13:52:24, Marc Treib wrote: > Here we first nuke, then notify; in the method above it's the other way around. > Can we be consistent? Done. https://codereview.chromium.org/2702223004/diff/100001/components/ntp_snippet... components/ntp_snippets/remote/remote_suggestions_provider_impl.cc:1087: remote_suggestions_scheduler_->OnProviderInactivated(); On 2017/02/23 13:52:24, Marc Treib wrote: > nit: s/Inactivated/Deactivated/ (I don't think inactivate is a word) Done. https://codereview.chromium.org/2702223004/diff/100001/components/ntp_snippet... File components/ntp_snippets/remote/remote_suggestions_provider_impl.h (right): https://codereview.chromium.org/2702223004/diff/100001/components/ntp_snippet... components/ntp_snippets/remote/remote_suggestions_provider_impl.h:200: // TODO(jkrcal): Mock the database to trigger the error naturally (or remote On 2017/02/23 13:52:24, Marc Treib wrote: > s/remote/remove/ ? Done. https://codereview.chromium.org/2702223004/diff/100001/components/ntp_snippet... components/ntp_snippets/remote/remote_suggestions_provider_impl.h:357: void ClearAnyHistory(); On 2017/02/23 13:52:24, Marc Treib wrote: > Should this be "AnyHistoryCleared"? It doesn't clear history itself; it's called > when a history clear event occurs (right?) > Or alternatively something like ClearHistoryDependentState? (with a less-awkward > name, if possible :D) I prefer the active form, stick to ClearHistoryDependentState(). https://codereview.chromium.org/2702223004/diff/100001/components/ntp_snippet... File components/ntp_snippets/remote/scheduling_remote_suggestions_provider.cc (right): https://codereview.chromium.org/2702223004/diff/100001/components/ntp_snippet... components/ntp_snippets/remote/scheduling_remote_suggestions_provider.cc:197: provider_->SetRemoteSuggestionsScheduler(this); On 2017/02/23 13:52:24, Marc Treib wrote: > This is a bit weird: We now have two-way linking between the SchedulingProvider > and the Impl. And this method is defined on the common interface, but it should > never be called on this class. > > Also, do we need to set it back to nullptr at some point? What if the provider > tries to call the scheduler during destruction, something like that? Added a bug and TODOs (after an offline discussion). I do not think we need to null it because this class has a unique_ptr to the provider_ so it is destructed first. https://codereview.chromium.org/2702223004/diff/100001/components/ntp_snippet... components/ntp_snippets/remote/scheduling_remote_suggestions_provider.cc:231: // asks for new suggestions) do give sync the time to propagate the changes in On 2017/02/23 13:52:24, Marc Treib wrote: > s/do/to/ Done. https://codereview.chromium.org/2702223004/diff/100001/components/ntp_snippet... components/ntp_snippets/remote/scheduling_remote_suggestions_provider.cc:279: void SchedulingRemoteSuggestionsProvider::SetRemoteSuggestionsScheduler( On 2017/02/23 13:52:24, Marc Treib wrote: > Can this method ever be called (on this particular class)? Replaced by NOTREACHED()
Once that issue is addressed, the CL looks good to me. Thanks marc for calling that out -- somehow completely missed that part in the diffs. https://codereview.chromium.org/2702223004/diff/120001/components/ntp_snippet... File components/ntp_snippets/remote/remote_suggestions_provider.h (right): https://codereview.chromium.org/2702223004/diff/120001/components/ntp_snippet... components/ntp_snippets/remote/remote_suggestions_provider.h:34: virtual void SetRemoteSuggestionsScheduler( Completely missed this (and thanks Marc for pointing it out). This method should not be on the RemoteSuggestionsProvider interface anymore. We only need it to tell the Impl about the Scheduler. We should move the function down to the Impl and then have the factory call it with the scheduler instance.
https://codereview.chromium.org/2702223004/diff/120001/components/ntp_snippet... File components/ntp_snippets/remote/remote_suggestions_provider.h (right): https://codereview.chromium.org/2702223004/diff/120001/components/ntp_snippet... components/ntp_snippets/remote/remote_suggestions_provider.h:34: virtual void SetRemoteSuggestionsScheduler( On 2017/02/24 09:37:12, tschumann wrote: > Completely missed this (and thanks Marc for pointing it out). > This method should not be on the RemoteSuggestionsProvider interface anymore. We > only need it to tell the Impl about the Scheduler. > > We should move the function down to the Impl and then have the factory call it > with the scheduler instance. I am puzzled: How is this different from the previous SetProviderStatusCallback? Why is it cleaner in the factory (which results in code duplication)?
https://codereview.chromium.org/2702223004/diff/120001/components/ntp_snippet... File components/ntp_snippets/remote/remote_suggestions_provider.h (right): https://codereview.chromium.org/2702223004/diff/120001/components/ntp_snippet... components/ntp_snippets/remote/remote_suggestions_provider.h:34: virtual void SetRemoteSuggestionsScheduler( On 2017/02/24 09:47:04, jkrcal wrote: > On 2017/02/24 09:37:12, tschumann wrote: > > Completely missed this (and thanks Marc for pointing it out). > > This method should not be on the RemoteSuggestionsProvider interface anymore. > We > > only need it to tell the Impl about the Scheduler. > > > > We should move the function down to the Impl and then have the factory call it > > with the scheduler instance. > > I am puzzled: How is this different from the previous SetProviderStatusCallback? > Why is it cleaner in the factory (which results in code duplication)? The main issue was here before (guess we missed that when we extracted the interface). It got slightly worse has we have now a dependency from a base class to its subclass. In the spirit of dependency injection, such wiring should be done as far outside as possible and a factory seems the proper place. A single instance should know as little as possible about how it's precisely wired to other instances. By having the wiring in a central place (where you actually can see the instances with their specific types) the system gets much easier to reason about.
Alright, LGTM since I don't want to further block this CL, assuming Tim is okay with this as an intermediate state. But IMO this whole design reaaally needs a re-think: The whole point of introducing the SchedulingProvider wrapper was that the Impl wouldn't have to know about the scheduling. If the Impl now knows about the Scheduler anyway, then IMO we should be consistent, i.e. provide all scheduling events in that way, and get rid of the wrapper.
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...
jkrcal@chromium.org changed reviewers: + noyau@chromium.org
Tim, I moved out the dependency injection in the factories, PTAL! Éric, could you PTAL at the ios factory?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
lgtm
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: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
Éric, friendly ping!
lgtm
The CQ bit was checked by jkrcal@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from treib@chromium.org, tschumann@chromium.org Link to the patchset: https://codereview.chromium.org/2702223004/#ps160001 (title: "Fix unit-tests")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
The CQ bit was checked by jkrcal@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
The CQ bit was checked by jkrcal@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
Patchset #10 (id:180001) has been deleted
The CQ bit was checked by jkrcal@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from treib@chromium.org, noyau@chromium.org, tschumann@chromium.org Link to the patchset: https://codereview.chromium.org/2702223004/#ps220001 (title: "Fixing an embarassing bug :)")
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": 220001, "attempt_start_ts": 1488456340847800,
"parent_rev": "0a7e111977232fbb33ad8db8bad857bbb3c68580", "commit_rev":
"6dd692b945fc8599a4c7db22854d19c9b27d0d4c"}
Message was sent while issue was closed.
Description was changed from ========== [Remote suggestions] Move all decisions to fetch to the scheduler This change will clarify responsibilities between the scheduler and the provider. On top of that, it eliminates the need to fetch snippets upon creating the provider (which is dangerous w.r.t. fetches in unexpected situations). This change needs the new types of notifications from the provider to the scheduler: - suggestions have been cleared (we should fetch on any later trigger), - history has been cleared (we should NOT fetch for a while). Thus, the provider is now given a pointer to the scheduler interface. (the interface now mixes internal and external signals but I think it is this way easier to understand then with two separate interfaces). BUG=694324 ========== to ========== [Remote suggestions] Move all decisions to fetch to the scheduler This change will clarify responsibilities between the scheduler and the provider. On top of that, it eliminates the need to fetch snippets upon creating the provider (which is dangerous w.r.t. fetches in unexpected situations). This change needs the new types of notifications from the provider to the scheduler: - suggestions have been cleared (we should fetch on any later trigger), - history has been cleared (we should NOT fetch for a while). Thus, the provider is now given a pointer to the scheduler interface. (the interface now mixes internal and external signals but I think it is this way easier to understand then with two separate interfaces). BUG=694324 Review-Url: https://codereview.chromium.org/2702223004 Cr-Commit-Position: refs/heads/master@{#454240} Committed: https://chromium.googlesource.com/chromium/src/+/6dd692b945fc8599a4c7db22854d... ==========
Message was sent while issue was closed.
Committed patchset #11 (id:220001) as https://chromium.googlesource.com/chromium/src/+/6dd692b945fc8599a4c7db22854d... |
