Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2611523004:
[Background fetching] Background fetching when opening an NTP. (Closed)
Description[Background fetching] Background fetching when opening an NTP.
This CL implements the most prominent soft background fetching trigger.
Whenever a new NTP is opened, we check whether a pre-specified time has
elapsed since the last successful fetch (+ a random jitter). Both the
minimum fetching frequency and the random jitter are controlled by
variation parameters.
BUG=672479
Committed: https://crrev.com/4d65d6bf235aa53c08304ebae6d39837e694d73f
Cr-Commit-Position: refs/heads/master@{#441619}
Patch Set 1 #
Total comments: 11
Patch Set 2 : Tim's comments #
Total comments: 2
Patch Set 3 : Tim's comments + unit-tests #
Total comments: 28
Patch Set 4 : Tim's comments #Patch Set 5 : Rebase #
Dependent Patchsets: Messages
Total messages: 38 (20 generated)
jkrcal@chromium.org changed reviewers: + tschumann@chromium.org
Tim, could you PTAL? I plan to add unit-tests in an hour or so in another patch set. https://codereview.chromium.org/2611523004/diff/1/components/ntp_snippets/rem... File components/ntp_snippets/remote/scheduling_remote_suggestions_provider.cc (left): https://codereview.chromium.org/2611523004/diff/1/components/ntp_snippets/rem... components/ntp_snippets/remote/scheduling_remote_suggestions_provider.cc:156: provider_->RefetchInTheBackground(std::move(callback)); This was a previous bug that OnFetchCompleted() (now RefetchInTheBackgroundFinished()) was not called after this call.
https://codereview.chromium.org/2611523004/diff/1/components/ntp_snippets/rem... File components/ntp_snippets/remote/scheduling_remote_suggestions_provider.cc (right): https://codereview.chromium.org/2611523004/diff/1/components/ntp_snippets/rem... components/ntp_snippets/remote/scheduling_remote_suggestions_provider.cc:28: SOFT_ACTIVE, hmm... what about: 'SOFT_ON_USAGE_EVENT' or just 'ON_USAGE_EVENT'? We already have a notion of 'active' (--> active users) so overloading this might be confusing. https://codereview.chromium.org/2611523004/diff/1/components/ntp_snippets/rem... components/ntp_snippets/remote/scheduling_remote_suggestions_provider.cc:64: DCHECK(index >=0 && index < static_cast<int>(FetchingInterval::MAX)); hmm... i guess what you really want to check is: DCHECK(interval != FetchingInterval::MAX); and DCHECK(index >=0 && index < arraysize(kDefaultFetchingIntervalRareNtpUser)); (and if you really care, compare against the other arrays as well).
jkrcal@chromium.org changed reviewers: + fhorschig@chromium.org
(adding Friedrich to keep him updated)
https://codereview.chromium.org/2611523004/diff/1/components/ntp_snippets/rem... File components/ntp_snippets/remote/scheduling_remote_suggestions_provider.cc (right): https://codereview.chromium.org/2611523004/diff/1/components/ntp_snippets/rem... components/ntp_snippets/remote/scheduling_remote_suggestions_provider.cc:166: RefetchInTheBackground(nullptr); please add /*callback=*/nullptr to document the parameter. Same in other places below. https://codereview.chromium.org/2611523004/diff/1/components/ntp_snippets/rem... components/ntp_snippets/remote/scheduling_remote_suggestions_provider.cc:377: if (!pref_service_->HasPrefPath(prefs::kSnippetNextActiveSoftFetch)) { i guess this checks whether the path has been registered. Do we do this somewhere?
Addressed your comments. Did not make it through unit-tests, yet. https://codereview.chromium.org/2611523004/diff/1/components/ntp_snippets/rem... File components/ntp_snippets/remote/scheduling_remote_suggestions_provider.cc (right): https://codereview.chromium.org/2611523004/diff/1/components/ntp_snippets/rem... components/ntp_snippets/remote/scheduling_remote_suggestions_provider.cc:28: SOFT_ACTIVE, On 2017/01/03 14:35:04, tschumann wrote: > hmm... what about: 'SOFT_ON_USAGE_EVENT' or just 'ON_USAGE_EVENT'? > We already have a notion of 'active' (--> active users) so overloading this > might be confusing. Done. https://codereview.chromium.org/2611523004/diff/1/components/ntp_snippets/rem... components/ntp_snippets/remote/scheduling_remote_suggestions_provider.cc:64: DCHECK(index >=0 && index < static_cast<int>(FetchingInterval::MAX)); On 2017/01/03 14:35:04, tschumann wrote: > hmm... i guess what you really want to check is: > DCHECK(interval != FetchingInterval::MAX); > and > DCHECK(index >=0 && index < arraysize(kDefaultFetchingIntervalRareNtpUser)); > > (and if you really care, compare against the other arrays as well). Done. https://codereview.chromium.org/2611523004/diff/1/components/ntp_snippets/rem... components/ntp_snippets/remote/scheduling_remote_suggestions_provider.cc:166: RefetchInTheBackground(nullptr); On 2017/01/03 15:16:13, tschumann wrote: > please add /*callback=*/nullptr > to document the parameter. > Same in other places below. Done. https://codereview.chromium.org/2611523004/diff/1/components/ntp_snippets/rem... components/ntp_snippets/remote/scheduling_remote_suggestions_provider.cc:377: if (!pref_service_->HasPrefPath(prefs::kSnippetNextActiveSoftFetch)) { On 2017/01/03 15:16:13, tschumann wrote: > i guess this checks whether the path has been registered. Do we do this > somewhere? No, a path should always be registered (I've forgotten to do so, thanks for pointing out). Here, I check that something has been stored there. I use it as a signal that background fetching is switched on / off.
https://codereview.chromium.org/2611523004/diff/1/components/ntp_snippets/rem... File components/ntp_snippets/remote/scheduling_remote_suggestions_provider.cc (right): https://codereview.chromium.org/2611523004/diff/1/components/ntp_snippets/rem... components/ntp_snippets/remote/scheduling_remote_suggestions_provider.cc:377: if (!pref_service_->HasPrefPath(prefs::kSnippetNextActiveSoftFetch)) { On 2017/01/03 18:36:04, jkrcal wrote: > On 2017/01/03 15:16:13, tschumann wrote: > > i guess this checks whether the path has been registered. Do we do this > > somewhere? > > No, a path should always be registered (I've forgotten to do so, thanks for > pointing out). Here, I check that something has been stored there. I use it as a > signal that background fetching is switched on / off. Ah, got it -- that's what ClearPref() is doing :-) It might be worth adding a comment to line 299 explaining that this is not only cleaning up but also signalling that soft fetches are stopped. https://codereview.chromium.org/2611523004/diff/20001/components/ntp_snippets... File components/ntp_snippets/remote/scheduling_remote_suggestions_provider.cc (right): https://codereview.chromium.org/2611523004/diff/20001/components/ntp_snippets... components/ntp_snippets/remote/scheduling_remote_suggestions_provider.cc:319: pref_service_->SetInt64(prefs::kSnippetLastBackgroundFetchAttempt, from an offline discussion: We also call this on StartScheduling() which essentially messes up the information of the actual last fetch. I'm fine with getting this addressed in a follow-up CL where we can also clean up the 'is-activated' logic in ShouldRefetchInTheBackgroundNow() -- we shouldn't abuse the last scheduled time-stamp for that.
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...
Tim, unit-tests have arrived! PTAL. https://codereview.chromium.org/2611523004/diff/1/components/ntp_snippets/rem... File components/ntp_snippets/remote/scheduling_remote_suggestions_provider.cc (right): https://codereview.chromium.org/2611523004/diff/1/components/ntp_snippets/rem... components/ntp_snippets/remote/scheduling_remote_suggestions_provider.cc:377: if (!pref_service_->HasPrefPath(prefs::kSnippetNextActiveSoftFetch)) { On 2017/01/03 18:54:24, tschumann wrote: > On 2017/01/03 18:36:04, jkrcal wrote: > > On 2017/01/03 15:16:13, tschumann wrote: > > > i guess this checks whether the path has been registered. Do we do this > > > somewhere? > > > > No, a path should always be registered (I've forgotten to do so, thanks for > > pointing out). Here, I check that something has been stored there. I use it as > a > > signal that background fetching is switched on / off. > > Ah, got it -- that's what ClearPref() is doing :-) It might be worth adding a > comment to line 299 explaining that this is not only cleaning up but also > signalling that soft fetches are stopped. I fixed it, in the end. https://codereview.chromium.org/2611523004/diff/20001/components/ntp_snippets... File components/ntp_snippets/remote/scheduling_remote_suggestions_provider.cc (right): https://codereview.chromium.org/2611523004/diff/20001/components/ntp_snippets... components/ntp_snippets/remote/scheduling_remote_suggestions_provider.cc:319: pref_service_->SetInt64(prefs::kSnippetLastBackgroundFetchAttempt, On 2017/01/03 18:54:25, tschumann wrote: > from an offline discussion: We also call this on StartScheduling() which > essentially messes up the information of the actual last fetch. > I'm fine with getting this addressed in a follow-up CL where we can also clean > up the 'is-activated' logic in ShouldRefetchInTheBackgroundNow() -- we shouldn't > abuse the last scheduled time-stamp for that. Fixed, in the end.
nice! https://codereview.chromium.org/2611523004/diff/40001/components/ntp_snippets... File components/ntp_snippets/remote/scheduling_remote_suggestions_provider.cc (right): https://codereview.chromium.org/2611523004/diff/40001/components/ntp_snippets... components/ntp_snippets/remote/scheduling_remote_suggestions_provider.cc:168: void SchedulingRemoteSuggestionsProvider::RescheduleFetching() { ultimatively, this method should go away, right? (why would somebody tell the scheduler to reschedule). Maybe add a TODO()?. https://codereview.chromium.org/2611523004/diff/40001/components/ntp_snippets... components/ntp_snippets/remote/scheduling_remote_suggestions_provider.cc:175: if (schedule_.is_empty()) { can we wrap this in a helper method BackgroundFetchesDisabled()? This would remove the necessity of the comments below, which is usually a nice indicator for introducing helper functions. https://codereview.chromium.org/2611523004/diff/40001/components/ntp_snippets... components/ntp_snippets/remote/scheduling_remote_suggestions_provider.cc:192: return; // Not enough time has elapsed since last fetch attempt. nit: let's remove this comment as it duplicates logic -- at this point we should only care whether we should fetch or not. The helper function owns the logic (and the reasons) for why we shouldn't. https://codereview.chromium.org/2611523004/diff/40001/components/ntp_snippets... components/ntp_snippets/remote/scheduling_remote_suggestions_provider.cc:325: void SchedulingRemoteSuggestionsProvider::ApplyFetchingSchedule( Rename to ApplyPersistentFetchingSchedule and get rid of the parameter? https://codereview.chromium.org/2611523004/diff/40001/components/ntp_snippets... components/ntp_snippets/remote/scheduling_remote_suggestions_provider.cc:387: callback.Run(fetch_status, std::move(suggestions)); should we also check for the empty callback? https://codereview.chromium.org/2611523004/diff/40001/components/ntp_snippets... File components/ntp_snippets/remote/scheduling_remote_suggestions_provider.h (right): https://codereview.chromium.org/2611523004/diff/40001/components/ntp_snippets... components/ntp_snippets/remote/scheduling_remote_suggestions_provider.h:106: clock_ = std::move(clock); can we dependency inject this into the constructor? I don't think we have many places to update and that would be the superior approach. (ForTesting() methods are crutches applicable as temporary solutions where the actual cleanup is too costly for the moment). https://codereview.chromium.org/2611523004/diff/40001/components/ntp_snippets... components/ntp_snippets/remote/scheduling_remote_suggestions_provider.h:112: base::TimeDelta interval_persistent_wifi; members go after methods. https://codereview.chromium.org/2611523004/diff/40001/components/ntp_snippets... components/ntp_snippets/remote/scheduling_remote_suggestions_provider.h:170: nit: in general, we should aim to limit vertical white space. In the members section, there's no comment addressing multiple members, so I'd vote for removing the blank lines between them. https://codereview.chromium.org/2611523004/diff/40001/components/ntp_snippets... components/ntp_snippets/remote/scheduling_remote_suggestions_provider.h:178: // Allow for an injectable clock for testing. this comment is not necessary. https://codereview.chromium.org/2611523004/diff/40001/components/ntp_snippets... File components/ntp_snippets/remote/scheduling_remote_suggestions_provider_unittest.cc (right): https://codereview.chromium.org/2611523004/diff/40001/components/ntp_snippets... components/ntp_snippets/remote/scheduling_remote_suggestions_provider_unittest.cc:65: class MockClock : public base::Clock { for clocks, you usually don't want behavioral testing (like mocks). Use base/test/simple_test_clock.h instead. https://codereview.chromium.org/2611523004/diff/40001/components/ntp_snippets... components/ntp_snippets/remote/scheduling_remote_suggestions_provider_unittest.cc:184: TEST_F(SchedulingRemoteSuggestionsProviderTest, there's one similar test missing: ShouldNotTriggerBackgroundFetchIfAlreadyInProgess https://codereview.chromium.org/2611523004/diff/40001/components/ntp_snippets... components/ntp_snippets/remote/scheduling_remote_suggestions_provider_unittest.cc:242: // Make the first soft fetch successful. that's not a soft fetch, right? https://codereview.chromium.org/2611523004/diff/40001/components/ntp_snippets... components/ntp_snippets/remote/scheduling_remote_suggestions_provider_unittest.cc:277: // Make the first soft fetch failed. first hard fetch?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
jkrcal@chromium.org changed reviewers: + noyau@chromium.org
Éric, could you PTAL at the iOS factory? Tim, PTAL, again! https://codereview.chromium.org/2611523004/diff/40001/components/ntp_snippets... File components/ntp_snippets/remote/scheduling_remote_suggestions_provider.cc (right): https://codereview.chromium.org/2611523004/diff/40001/components/ntp_snippets... components/ntp_snippets/remote/scheduling_remote_suggestions_provider.cc:168: void SchedulingRemoteSuggestionsProvider::RescheduleFetching() { On 2017/01/04 10:43:52, tschumann wrote: > ultimatively, this method should go away, right? (why would somebody tell the > scheduler to reschedule). Maybe add a TODO()?. No. This needs to be called when Chrome is updated. At that moment, existing persistent tasks are deleted while Chrome thinks (based on prefs) they are running. https://codereview.chromium.org/2611523004/diff/40001/components/ntp_snippets... components/ntp_snippets/remote/scheduling_remote_suggestions_provider.cc:175: if (schedule_.is_empty()) { On 2017/01/04 10:43:52, tschumann wrote: > can we wrap this in a helper method BackgroundFetchesDisabled()? > This would remove the necessity of the comments below, which is usually a nice > indicator for introducing helper functions. Done. https://codereview.chromium.org/2611523004/diff/40001/components/ntp_snippets... components/ntp_snippets/remote/scheduling_remote_suggestions_provider.cc:192: return; // Not enough time has elapsed since last fetch attempt. On 2017/01/04 10:43:52, tschumann wrote: > nit: let's remove this comment as it duplicates logic -- at this point we should > only care whether we should fetch or not. The helper function owns the logic > (and the reasons) for why we shouldn't. Good point, done. https://codereview.chromium.org/2611523004/diff/40001/components/ntp_snippets... components/ntp_snippets/remote/scheduling_remote_suggestions_provider.cc:325: void SchedulingRemoteSuggestionsProvider::ApplyFetchingSchedule( On 2017/01/04 10:43:52, tschumann wrote: > Rename to ApplyPersistentFetchingSchedule and get rid of the parameter? Done. https://codereview.chromium.org/2611523004/diff/40001/components/ntp_snippets... components/ntp_snippets/remote/scheduling_remote_suggestions_provider.cc:387: callback.Run(fetch_status, std::move(suggestions)); On 2017/01/04 10:43:52, tschumann wrote: > should we also check for the empty callback? Done. https://codereview.chromium.org/2611523004/diff/40001/components/ntp_snippets... File components/ntp_snippets/remote/scheduling_remote_suggestions_provider.h (right): https://codereview.chromium.org/2611523004/diff/40001/components/ntp_snippets... components/ntp_snippets/remote/scheduling_remote_suggestions_provider.h:106: clock_ = std::move(clock); On 2017/01/04 10:43:52, tschumann wrote: > can we dependency inject this into the constructor? I don't think we have many > places to update and that would be the superior approach. > (ForTesting() methods are crutches applicable as temporary solutions where the > actual cleanup is too costly for the moment). Done. https://codereview.chromium.org/2611523004/diff/40001/components/ntp_snippets... components/ntp_snippets/remote/scheduling_remote_suggestions_provider.h:112: base::TimeDelta interval_persistent_wifi; On 2017/01/04 10:43:52, tschumann wrote: > members go after methods. Done. https://codereview.chromium.org/2611523004/diff/40001/components/ntp_snippets... components/ntp_snippets/remote/scheduling_remote_suggestions_provider.h:170: On 2017/01/04 10:43:52, tschumann wrote: > nit: in general, we should aim to limit vertical white space. In the members > section, there's no comment addressing multiple members, so I'd vote for > removing the blank lines between them. Done. https://codereview.chromium.org/2611523004/diff/40001/components/ntp_snippets... components/ntp_snippets/remote/scheduling_remote_suggestions_provider.h:178: // Allow for an injectable clock for testing. On 2017/01/04 10:43:52, tschumann wrote: > this comment is not necessary. Done. https://codereview.chromium.org/2611523004/diff/40001/components/ntp_snippets... File components/ntp_snippets/remote/scheduling_remote_suggestions_provider_unittest.cc (right): https://codereview.chromium.org/2611523004/diff/40001/components/ntp_snippets... components/ntp_snippets/remote/scheduling_remote_suggestions_provider_unittest.cc:65: class MockClock : public base::Clock { On 2017/01/04 10:43:52, tschumann wrote: > for clocks, you usually don't want behavioral testing (like mocks). Use > base/test/simple_test_clock.h instead. Done. https://codereview.chromium.org/2611523004/diff/40001/components/ntp_snippets... components/ntp_snippets/remote/scheduling_remote_suggestions_provider_unittest.cc:184: TEST_F(SchedulingRemoteSuggestionsProviderTest, On 2017/01/04 10:43:52, tschumann wrote: > there's one similar test missing: > ShouldNotTriggerBackgroundFetchIfAlreadyInProgess Ah, sure! Done. EDIT: This added test showed there was a bug in the code. You are a unit-test magician! ;) https://codereview.chromium.org/2611523004/diff/40001/components/ntp_snippets... components/ntp_snippets/remote/scheduling_remote_suggestions_provider_unittest.cc:242: // Make the first soft fetch successful. On 2017/01/04 10:43:52, tschumann wrote: > that's not a soft fetch, right? Done. https://codereview.chromium.org/2611523004/diff/40001/components/ntp_snippets... components/ntp_snippets/remote/scheduling_remote_suggestions_provider_unittest.cc:277: // Make the first soft fetch failed. On 2017/01/04 10:43:52, tschumann wrote: > first hard fetch? Done.
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...
ios lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm https://codereview.chromium.org/2611523004/diff/40001/components/ntp_snippets... File components/ntp_snippets/remote/scheduling_remote_suggestions_provider.cc (right): https://codereview.chromium.org/2611523004/diff/40001/components/ntp_snippets... components/ntp_snippets/remote/scheduling_remote_suggestions_provider.cc:168: void SchedulingRemoteSuggestionsProvider::RescheduleFetching() { On 2017/01/04 14:19:03, jkrcal wrote: > On 2017/01/04 10:43:52, tschumann wrote: > > ultimatively, this method should go away, right? (why would somebody tell the > > scheduler to reschedule). Maybe add a TODO()?. > > No. This needs to be called when Chrome is updated. At that moment, existing > persistent tasks are deleted while Chrome thinks (based on prefs) they are > running. I see. Maybe we should rename this function then to tell the scheduler what happend (chrome-update) instead of what it should do to its internal state (reset the persistent scheduler). https://codereview.chromium.org/2611523004/diff/40001/components/ntp_snippets... File components/ntp_snippets/remote/scheduling_remote_suggestions_provider_unittest.cc (right): https://codereview.chromium.org/2611523004/diff/40001/components/ntp_snippets... components/ntp_snippets/remote/scheduling_remote_suggestions_provider_unittest.cc:184: TEST_F(SchedulingRemoteSuggestionsProviderTest, On 2017/01/04 14:19:03, jkrcal wrote: > On 2017/01/04 10:43:52, tschumann wrote: > > there's one similar test missing: > > ShouldNotTriggerBackgroundFetchIfAlreadyInProgess > > Ah, sure! Done. > > EDIT: This added test showed there was a bug in the code. You are a unit-test > magician! ;) hehehe... I just know that I cannot spot bugs well -- so I don't believe things are working until I see a test :-)
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
Failed to apply patch for
chrome/browser/ntp_snippets/content_suggestions_service_factory.cc:
While running git apply --index -p1;
error: patch failed:
chrome/browser/ntp_snippets/content_suggestions_service_factory.cc:10
error: chrome/browser/ntp_snippets/content_suggestions_service_factory.cc:
patch does not apply
Patch: chrome/browser/ntp_snippets/content_suggestions_service_factory.cc
Index: chrome/browser/ntp_snippets/content_suggestions_service_factory.cc
diff --git a/chrome/browser/ntp_snippets/content_suggestions_service_factory.cc
b/chrome/browser/ntp_snippets/content_suggestions_service_factory.cc
index
d065de4d250d36d8efd9be9dead54f0f974f21d7..3d2a1ae8c3ded11a1c1aef89a897633fa9be7f2e
100644
--- a/chrome/browser/ntp_snippets/content_suggestions_service_factory.cc
+++ b/chrome/browser/ntp_snippets/content_suggestions_service_factory.cc
@@ -10,6 +10,7 @@
#include "base/files/file_path.h"
#include "base/memory/ptr_util.h"
#include "base/memory/singleton.h"
+#include "base/time/default_clock.h"
#include "chrome/browser/bookmarks/bookmark_model_factory.h"
#include "chrome/browser/browser_process.h"
#include "chrome/browser/history/history_service_factory.h"
@@ -182,7 +183,7 @@ void RegisterArticleProvider(SigninManagerBase*
signin_manager,
auto scheduling_provider =
base::MakeUnique<SchedulingRemoteSuggestionsProvider>(
service, std::move(provider), scheduler, service->user_classifier(),
- pref_service);
+ pref_service, base::MakeUnique<base::DefaultClock>());
service->set_remote_suggestions_provider(scheduling_provider.get());
service->set_remote_suggestions_scheduler(scheduling_provider.get());
service->RegisterProvider(std::move(scheduling_provider));
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
Failed to apply patch for
chrome/browser/ntp_snippets/content_suggestions_service_factory.cc:
While running git apply --index -p1;
error: patch failed:
chrome/browser/ntp_snippets/content_suggestions_service_factory.cc:10
error: chrome/browser/ntp_snippets/content_suggestions_service_factory.cc:
patch does not apply
Patch: chrome/browser/ntp_snippets/content_suggestions_service_factory.cc
Index: chrome/browser/ntp_snippets/content_suggestions_service_factory.cc
diff --git a/chrome/browser/ntp_snippets/content_suggestions_service_factory.cc
b/chrome/browser/ntp_snippets/content_suggestions_service_factory.cc
index
d065de4d250d36d8efd9be9dead54f0f974f21d7..3d2a1ae8c3ded11a1c1aef89a897633fa9be7f2e
100644
--- a/chrome/browser/ntp_snippets/content_suggestions_service_factory.cc
+++ b/chrome/browser/ntp_snippets/content_suggestions_service_factory.cc
@@ -10,6 +10,7 @@
#include "base/files/file_path.h"
#include "base/memory/ptr_util.h"
#include "base/memory/singleton.h"
+#include "base/time/default_clock.h"
#include "chrome/browser/bookmarks/bookmark_model_factory.h"
#include "chrome/browser/browser_process.h"
#include "chrome/browser/history/history_service_factory.h"
@@ -182,7 +183,7 @@ void RegisterArticleProvider(SigninManagerBase*
signin_manager,
auto scheduling_provider =
base::MakeUnique<SchedulingRemoteSuggestionsProvider>(
service, std::move(provider), scheduler, service->user_classifier(),
- pref_service);
+ pref_service, base::MakeUnique<base::DefaultClock>());
service->set_remote_suggestions_provider(scheduling_provider.get());
service->set_remote_suggestions_scheduler(scheduling_provider.get());
service->RegisterProvider(std::move(scheduling_provider));
The CQ bit was checked by jkrcal@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from noyau@chromium.org, tschumann@chromium.org Link to the patchset: https://codereview.chromium.org/2611523004/#ps80001 (title: "Rebase")
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": 80001, "attempt_start_ts": 1483610518684630,
"parent_rev": "ba110aabd799fe4d5fc47d5df5beda989b7475d2", "commit_rev":
"2b438241d2a70ff52623936b8e7f32dede4be477"}
Message was sent while issue was closed.
Description was changed from ========== [Background fetching] Background fetching when opening an NTP. This CL implements the most prominent soft background fetching trigger. Whenever a new NTP is opened, we check whether a pre-specified time has elapsed since the last successful fetch (+ a random jitter). Both the minimum fetching frequency and the random jitter are controlled by variation parameters. BUG=672479 ========== to ========== [Background fetching] Background fetching when opening an NTP. This CL implements the most prominent soft background fetching trigger. Whenever a new NTP is opened, we check whether a pre-specified time has elapsed since the last successful fetch (+ a random jitter). Both the minimum fetching frequency and the random jitter are controlled by variation parameters. BUG=672479 Review-Url: https://codereview.chromium.org/2611523004 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== [Background fetching] Background fetching when opening an NTP. This CL implements the most prominent soft background fetching trigger. Whenever a new NTP is opened, we check whether a pre-specified time has elapsed since the last successful fetch (+ a random jitter). Both the minimum fetching frequency and the random jitter are controlled by variation parameters. BUG=672479 Review-Url: https://codereview.chromium.org/2611523004 ========== to ========== [Background fetching] Background fetching when opening an NTP. This CL implements the most prominent soft background fetching trigger. Whenever a new NTP is opened, we check whether a pre-specified time has elapsed since the last successful fetch (+ a random jitter). Both the minimum fetching frequency and the random jitter are controlled by variation parameters. BUG=672479 Committed: https://crrev.com/4d65d6bf235aa53c08304ebae6d39837e694d73f Cr-Commit-Position: refs/heads/master@{#441619} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/4d65d6bf235aa53c08304ebae6d39837e694d73f Cr-Commit-Position: refs/heads/master@{#441619} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
