Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2549163002:
RemoteContentSuggestions: Stores the time of the last successful background fetch in a pref (Closed)
|
Created:
4 years ago by markusheintz_ Modified:
4 years ago CC:
chromium-reviews, ntp-dev+reviews_chromium.org, fhorschig, jkrcal Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionRemoteContentSuggestions: Stores the time of the last successful background fetch in a pref.
Therefore we also need to pass the fetch success status to the callback passed to the NTPSnippetsFetcher.
The last successful background fetch time will be used by the scheduler in the future in order to decide when to do/schedule the next background fetch.
Passing back the success status to the callback will also allow to add error handling.
BUG=665873, 672422
Committed: https://crrev.com/f6f620269ee0af3e7f8da7542a0cf04275248912
Cr-Commit-Position: refs/heads/master@{#437233}
Patch Set 1 #Patch Set 2 : rebase #Patch Set 3 : Fix compile error after rebase #Patch Set 4 : Add a test #Patch Set 5 : Rebase #Patch Set 6 : Add a comment for the new pref #
Total comments: 24
Patch Set 7 : Address comments treib@ #
Total comments: 1
Patch Set 8 : Remove LOG(ERROR) dbg statment #
Total comments: 4
Patch Set 9 : Address comments treib #
Total comments: 4
Patch Set 10 : Address comments treib #
Total comments: 6
Messages
Total messages: 47 (26 generated)
Fix compile error after rebase
Description was changed from ========== Rough idea BUG=TBD ========== to ========== Stores the time of the last successful background fetch of remote snippets. BUG=TBD ==========
markusheintz@chromium.org changed reviewers: + treib@chromium.org
Add a test
The CQ bit was checked by markusheintz@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: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Rebase onto master
The CQ bit was checked by markusheintz@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...
Add a comment for the new pref
Description was changed from ========== Stores the time of the last successful background fetch of remote snippets. BUG=TBD ========== to ========== Stores the time of the last successful background fetch of remote snippets. BUG=665873 ==========
Description was changed from ========== Stores the time of the last successful background fetch of remote snippets. BUG=665873 ========== to ========== Stores the time of the last successful background fetch of remote snippets in a pref. Therefore we also need to pass the fetch success status to the callback passed to the NTPSnippetsFetcher. BUG=665873 ==========
Marc can you please take a look at this CL?
Thanks! Looks good overall, just a few nits. https://codereview.chromium.org/2549163002/diff/100001/components/ntp_snippet... File components/ntp_snippets/pref_names.cc (right): https://codereview.chromium.org/2549163002/diff/100001/components/ntp_snippet... components/ntp_snippets/pref_names.cc:45: "ntp_suggestions.articles.last_successfull_background_fetch_time"; s/articles/remote/ ? https://codereview.chromium.org/2549163002/diff/100001/components/ntp_snippet... File components/ntp_snippets/pref_names.h (right): https://codereview.chromium.org/2549163002/diff/100001/components/ntp_snippet... components/ntp_snippets/pref_names.h:44: extern const char kLastSuccessfullBackgroundFetchTime[]; nit: "successful", only one "l" https://codereview.chromium.org/2549163002/diff/100001/components/ntp_snippet... File components/ntp_snippets/remote/ntp_snippets_fetcher.h (right): https://codereview.chromium.org/2549163002/diff/100001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.h:98: // |snippets| contains parsed snippets if a fetch succeeded. If problems nit: Empty line before. https://codereview.chromium.org/2549163002/diff/100001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.h:103: FetchResult fetch_result)>; nit: I'd find it a bit more natural to have the FetchResult first, and the (optional) data second. WDYT? https://codereview.chromium.org/2549163002/diff/100001/components/ntp_snippet... File components/ntp_snippets/remote/ntp_snippets_fetcher_unittest.cc (right): https://codereview.chromium.org/2549163002/diff/100001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher_unittest.cc:729: EXPECT_CALL(mock_callback(), Run(_, _)) I think this should be an actual status, also in the other instances below. https://codereview.chromium.org/2549163002/diff/100001/components/ntp_snippet... File components/ntp_snippets/remote/remote_suggestions_provider.cc (right): https://codereview.chromium.org/2549163002/diff/100001/components/ntp_snippet... components/ntp_snippets/remote/remote_suggestions_provider.cc:254: tick_clock_(new base::DefaultTickClock()) { nit: base::MakeUnique<base::DefaultTickClock>() (to make clear that no raw pointer is involved) https://codereview.chromium.org/2549163002/diff/100001/components/ntp_snippet... File components/ntp_snippets/remote/remote_suggestions_provider_unittest.cc (right): https://codereview.chromium.org/2549163002/diff/100001/components/ntp_snippet... components/ntp_snippets/remote/remote_suggestions_provider_unittest.cc:1604: new base::SimpleTestTickClock(); I dislike raw pointers :) How about: - Make a unique_ptr - Get a raw ptr copy - Pass the unique_ptr to the provider ? https://codereview.chromium.org/2549163002/diff/100001/components/ntp_snippet... components/ntp_snippets/remote/remote_suggestions_provider_unittest.cc:1609: simple_test_tick_clock->SetNowTicks(test_now); Should this happen before passing the clock to the provider? (So that it always has a well-defined value) https://codereview.chromium.org/2549163002/diff/100001/components/ntp_snippet... components/ntp_snippets/remote/remote_suggestions_provider_unittest.cc:1611: // Test that the preference is correclty initialized with the default value 0. s/correclty/correctly/ https://codereview.chromium.org/2549163002/diff/100001/components/ntp_snippet... components/ntp_snippets/remote/remote_suggestions_provider_unittest.cc:1616: // background fetch on initialization since the snippets DB is still empty. This isn't really accurate - the background fetch attempt will happen anyway, independent of |set_empty_response|. https://codereview.chromium.org/2549163002/diff/100001/components/ntp_snippet... components/ntp_snippets/remote/remote_suggestions_provider_unittest.cc:1629: (test_now + TimeDelta::FromHours(1)).ToInternalValue(), This could just use simple_test_tick_clock->NowTicks(), then the |test_now| variable wouldn't be necessary, right?
Address comments treib@
The CQ bit was checked by markusheintz@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/2549163002/diff/100001/components/ntp_snippet... File components/ntp_snippets/pref_names.cc (right): https://codereview.chromium.org/2549163002/diff/100001/components/ntp_snippet... components/ntp_snippets/pref_names.cc:45: "ntp_suggestions.articles.last_successfull_background_fetch_time"; On 2016/12/07 14:42:12, Marc Treib wrote: > s/articles/remote/ > ? Done. https://codereview.chromium.org/2549163002/diff/100001/components/ntp_snippet... File components/ntp_snippets/pref_names.h (right): https://codereview.chromium.org/2549163002/diff/100001/components/ntp_snippet... components/ntp_snippets/pref_names.h:44: extern const char kLastSuccessfullBackgroundFetchTime[]; On 2016/12/07 14:42:13, Marc Treib wrote: > nit: "successful", only one "l" Done. https://codereview.chromium.org/2549163002/diff/100001/components/ntp_snippet... File components/ntp_snippets/remote/ntp_snippets_fetcher.h (right): https://codereview.chromium.org/2549163002/diff/100001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.h:98: // |snippets| contains parsed snippets if a fetch succeeded. If problems On 2016/12/07 14:42:13, Marc Treib wrote: > nit: Empty line before. Done. https://codereview.chromium.org/2549163002/diff/100001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.h:103: FetchResult fetch_result)>; On 2016/12/07 14:42:13, Marc Treib wrote: > nit: I'd find it a bit more natural to have the FetchResult first, and the > (optional) data second. WDYT? I have absolutely no hard feeling on this. But I guess I would pass on this one :-). If seen it both ways and I guess it does not really matter. while naturally you would want to check first the result status and then look at the results. But on a related note the name fetch_result is kind of not expressing that this is actually a success status. So I guess I can make you happy by changing the order, when finding a better name for the param? => Followup CL? https://codereview.chromium.org/2549163002/diff/100001/components/ntp_snippet... File components/ntp_snippets/remote/ntp_snippets_fetcher_unittest.cc (right): https://codereview.chromium.org/2549163002/diff/100001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher_unittest.cc:729: EXPECT_CALL(mock_callback(), Run(_, _)) On 2016/12/07 14:42:13, Marc Treib wrote: > I think this should be an actual status, also in the other instances below. Done. https://codereview.chromium.org/2549163002/diff/100001/components/ntp_snippet... File components/ntp_snippets/remote/remote_suggestions_provider.cc (right): https://codereview.chromium.org/2549163002/diff/100001/components/ntp_snippet... components/ntp_snippets/remote/remote_suggestions_provider.cc:254: tick_clock_(new base::DefaultTickClock()) { On 2016/12/07 14:42:13, Marc Treib wrote: > nit: base::MakeUnique<base::DefaultTickClock>() > (to make clear that no raw pointer is involved) Done. https://codereview.chromium.org/2549163002/diff/100001/components/ntp_snippet... File components/ntp_snippets/remote/remote_suggestions_provider_unittest.cc (right): https://codereview.chromium.org/2549163002/diff/100001/components/ntp_snippet... components/ntp_snippets/remote/remote_suggestions_provider_unittest.cc:1604: new base::SimpleTestTickClock(); On 2016/12/07 14:42:13, Marc Treib wrote: > I dislike raw pointers :) How about: > - Make a unique_ptr > - Get a raw ptr copy > - Pass the unique_ptr to the provider > ? Done https://codereview.chromium.org/2549163002/diff/100001/components/ntp_snippet... components/ntp_snippets/remote/remote_suggestions_provider_unittest.cc:1609: simple_test_tick_clock->SetNowTicks(test_now); On 2016/12/07 14:42:13, Marc Treib wrote: > Should this happen before passing the clock to the provider? (So that it always > has a well-defined value) SGTM done https://codereview.chromium.org/2549163002/diff/100001/components/ntp_snippet... components/ntp_snippets/remote/remote_suggestions_provider_unittest.cc:1611: // Test that the preference is correclty initialized with the default value 0. On 2016/12/07 14:42:13, Marc Treib wrote: > s/correclty/correctly/ Done. https://codereview.chromium.org/2549163002/diff/100001/components/ntp_snippet... components/ntp_snippets/remote/remote_suggestions_provider_unittest.cc:1616: // background fetch on initialization since the snippets DB is still empty. On 2016/12/07 14:42:13, Marc Treib wrote: > This isn't really accurate - the background fetch attempt will happen anyway, > independent of |set_empty_response|. This is not what I experienced when I wrote this. I'll remove the comment and investigate after the CL is done.
https://codereview.chromium.org/2549163002/diff/120001/components/ntp_snippet... File components/ntp_snippets/remote/ntp_snippets_fetcher.cc (right): https://codereview.chromium.org/2549163002/diff/120001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.cc:1000: LOG(ERROR) << " ================ ststus_code=" Removed
Remove LOG(ERROR) dbg statment
lgtm Thanks! LGTM with some last nits. (I'd prefer the parameters to be switched, but I won't force you ;) ) https://codereview.chromium.org/2549163002/diff/100001/components/ntp_snippet... File components/ntp_snippets/remote/ntp_snippets_fetcher.h (right): https://codereview.chromium.org/2549163002/diff/100001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.h:103: FetchResult fetch_result)>; On 2016/12/08 09:47:41, markusheintz_ wrote: > On 2016/12/07 14:42:13, Marc Treib wrote: > > nit: I'd find it a bit more natural to have the FetchResult first, and the > > (optional) data second. WDYT? > > I have absolutely no hard feeling on this. But I guess I would pass on this one > :-). If seen it both ways and I guess it does not really matter. > while naturally you would want to check first the result status and then look at > the results. > > But on a related note the name fetch_result is kind of not expressing that this > is actually a success status. So I guess I can make you happy by changing the > order, when finding a better name for the param? > => Followup CL? I think if we are gonna change the order at all, then we should do it in this CL which introduces the new param. Otherwise it's just more churn. So, I'd still prefer to have the result code first, but it's not important enough to fight over it ;) For the name, FetchResultCode maybe, to make clear that it's just a status code rather than the actual result data? (This is definitely for a separate CL though) https://codereview.chromium.org/2549163002/diff/140001/components/ntp_snippet... File components/ntp_snippets/pref_names.cc (right): https://codereview.chromium.org/2549163002/diff/140001/components/ntp_snippet... components/ntp_snippets/pref_names.cc:45: "ntp_suggestions.remote.last_successfull_background_fetch_time"; Also here: Only one "l" https://codereview.chromium.org/2549163002/diff/140001/components/ntp_snippet... File components/ntp_snippets/remote/remote_suggestions_provider_unittest.cc (right): https://codereview.chromium.org/2549163002/diff/140001/components/ntp_snippet... components/ntp_snippets/remote/remote_suggestions_provider_unittest.cc:1626: } Optional: You could re-create the service here, to simulate a Chrome restart, and make sure the previous value is still there.
Description was changed from ========== Stores the time of the last successful background fetch of remote snippets in a pref. Therefore we also need to pass the fetch success status to the callback passed to the NTPSnippetsFetcher. BUG=665873 ========== to ========== Stores the time of the last successful background fetch of remote snippets in a pref. Therefore we also need to pass the fetch success status to the callback passed to the NTPSnippetsFetcher. BUG=665873,672422 ==========
Per the recent discussion on CL descriptions: Could you - add "NTPSnippets: " or similar to the title, and - explain the "why"? (The current description only has the "what")
Description was changed from ========== Stores the time of the last successful background fetch of remote snippets in a pref. Therefore we also need to pass the fetch success status to the callback passed to the NTPSnippetsFetcher. BUG=665873,672422 ========== to ========== Stores the time of the last successful background fetch of remote snippets in a pref. Therefore we also need to pass the fetch success status to the callback passed to the NTPSnippetsFetcher. The last successful background fetch time will be used by the scheduler in the future in order to decide when to do/schedule the next background fetch. Passing back the success status to the callback will also allow to add error handling. BUG=665873,672422 ==========
https://codereview.chromium.org/2549163002/diff/100001/components/ntp_snippet... File components/ntp_snippets/remote/ntp_snippets_fetcher.h (right): https://codereview.chromium.org/2549163002/diff/100001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.h:103: FetchResult fetch_result)>; On 2016/12/08 10:33:48, Marc Treib wrote: > On 2016/12/08 09:47:41, markusheintz_ wrote: > > On 2016/12/07 14:42:13, Marc Treib wrote: > > > nit: I'd find it a bit more natural to have the FetchResult first, and the > > > (optional) data second. WDYT? > > > > I have absolutely no hard feeling on this. But I guess I would pass on this > one > > :-). If seen it both ways and I guess it does not really matter. > > while naturally you would want to check first the result status and then look > at > > the results. > > > > But on a related note the name fetch_result is kind of not expressing that > this > > is actually a success status. So I guess I can make you happy by changing the > > order, when finding a better name for the param? > > => Followup CL? > > I think if we are gonna change the order at all, then we should do it in this CL > which introduces the new param. Otherwise it's just more churn. > So, I'd still prefer to have the result code first, but it's not important > enough to fight over it ;) > > For the name, FetchResultCode maybe, to make clear that it's just a status code > rather than the actual result data? (This is definitely for a separate CL > though) Since Christmas is close here is my present for you :-) done https://codereview.chromium.org/2549163002/diff/140001/components/ntp_snippet... File components/ntp_snippets/pref_names.cc (right): https://codereview.chromium.org/2549163002/diff/140001/components/ntp_snippet... components/ntp_snippets/pref_names.cc:45: "ntp_suggestions.remote.last_successfull_background_fetch_time"; On 2016/12/08 10:33:49, Marc Treib wrote: > Also here: Only one "l" Done https://codereview.chromium.org/2549163002/diff/140001/components/ntp_snippet... File components/ntp_snippets/remote/remote_suggestions_provider_unittest.cc (right): https://codereview.chromium.org/2549163002/diff/140001/components/ntp_snippet... components/ntp_snippets/remote/remote_suggestions_provider_unittest.cc:1626: } On 2016/12/08 10:33:49, Marc Treib wrote: > Optional: You could re-create the service here, to simulate a Chrome restart, > and make sure the previous value is still there. Excellent idea. Once the scheduler refactoring is done this will be easier. I'll wait for this to happen. I created a bug to track it.
Description was changed from ========== Stores the time of the last successful background fetch of remote snippets in a pref. Therefore we also need to pass the fetch success status to the callback passed to the NTPSnippetsFetcher. The last successful background fetch time will be used by the scheduler in the future in order to decide when to do/schedule the next background fetch. Passing back the success status to the callback will also allow to add error handling. BUG=665873,672422 ========== to ========== RemoteContentSuggestions: Stores the time of the last successful background fetch in a pref. Therefore we also need to pass the fetch success status to the callback passed to the NTPSnippetsFetcher. The last successful background fetch time will be used by the scheduler in the future in order to decide when to do/schedule the next background fetch. Passing back the success status to the callback will also allow to add error handling. BUG=665873,672422 ==========
The CQ bit was checked by markusheintz@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...
Address comments treib
The CQ bit was checked by markusheintz@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/2549163002/diff/100001/components/ntp_snippet... File components/ntp_snippets/remote/ntp_snippets_fetcher.h (right): https://codereview.chromium.org/2549163002/diff/100001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.h:103: FetchResult fetch_result)>; On 2016/12/08 11:37:18, markusheintz_ wrote: > On 2016/12/08 10:33:48, Marc Treib wrote: > > On 2016/12/08 09:47:41, markusheintz_ wrote: > > > On 2016/12/07 14:42:13, Marc Treib wrote: > > > > nit: I'd find it a bit more natural to have the FetchResult first, and the > > > > (optional) data second. WDYT? > > > > > > I have absolutely no hard feeling on this. But I guess I would pass on this > > one > > > :-). If seen it both ways and I guess it does not really matter. > > > while naturally you would want to check first the result status and then > look > > at > > > the results. > > > > > > But on a related note the name fetch_result is kind of not expressing that > > this > > > is actually a success status. So I guess I can make you happy by changing > the > > > order, when finding a better name for the param? > > > => Followup CL? > > > > I think if we are gonna change the order at all, then we should do it in this > CL > > which introduces the new param. Otherwise it's just more churn. > > So, I'd still prefer to have the result code first, but it's not important > > enough to fight over it ;) > > > > For the name, FetchResultCode maybe, to make clear that it's just a status > code > > rather than the actual result data? (This is definitely for a separate CL > > though) > > Since Christmas is close here is my present for you :-) > done Woohoo! :D https://codereview.chromium.org/2549163002/diff/160001/components/ntp_snippet... File components/ntp_snippets/remote/ntp_snippets_fetcher_unittest.cc (right): https://codereview.chromium.org/2549163002/diff/160001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher_unittest.cc:63: ACTION_P(MoveArgumentPointeeTo, ptr) { nit: rename to MoveArgument1PointeeTo and/or add a comment that it's the second argument? https://codereview.chromium.org/2549163002/diff/160001/components/ntp_snippet... File components/ntp_snippets/remote/remote_suggestions_provider_unittest.cc (right): https://codereview.chromium.org/2549163002/diff/160001/components/ntp_snippet... components/ntp_snippets/remote/remote_suggestions_provider_unittest.cc:1627: // scheduler refactoring is done (b/672434). s/b/crbug/ :)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Address comments treib
https://codereview.chromium.org/2549163002/diff/160001/components/ntp_snippet... File components/ntp_snippets/remote/ntp_snippets_fetcher_unittest.cc (right): https://codereview.chromium.org/2549163002/diff/160001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher_unittest.cc:63: ACTION_P(MoveArgumentPointeeTo, ptr) { On 2016/12/08 12:15:40, Marc Treib wrote: > nit: rename to MoveArgument1PointeeTo and/or add a comment that it's the second > argument? Done. https://codereview.chromium.org/2549163002/diff/160001/components/ntp_snippet... File components/ntp_snippets/remote/remote_suggestions_provider_unittest.cc (right): https://codereview.chromium.org/2549163002/diff/160001/components/ntp_snippet... components/ntp_snippets/remote/remote_suggestions_provider_unittest.cc:1627: // scheduler refactoring is done (b/672434). On 2016/12/08 12:15:40, Marc Treib wrote: > s/b/crbug/ :) Done.
The CQ bit was checked by markusheintz@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from treib@chromium.org Link to the patchset: https://codereview.chromium.org/2549163002/#ps180001 (title: "Address comments treib")
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": 180001, "attempt_start_ts": 1481199836355630,
"parent_rev": "b759fa95b92b9a32f3b513140aab64cdd7ddcb5f", "commit_rev":
"7cc75e67dec999be368c4fab476d0f3909136264"}
Message was sent while issue was closed.
Description was changed from ========== RemoteContentSuggestions: Stores the time of the last successful background fetch in a pref. Therefore we also need to pass the fetch success status to the callback passed to the NTPSnippetsFetcher. The last successful background fetch time will be used by the scheduler in the future in order to decide when to do/schedule the next background fetch. Passing back the success status to the callback will also allow to add error handling. BUG=665873,672422 ========== to ========== RemoteContentSuggestions: Stores the time of the last successful background fetch in a pref. Therefore we also need to pass the fetch success status to the callback passed to the NTPSnippetsFetcher. The last successful background fetch time will be used by the scheduler in the future in order to decide when to do/schedule the next background fetch. Passing back the success status to the callback will also allow to add error handling. BUG=665873,672422 ==========
Message was sent while issue was closed.
Committed patchset #10 (id:180001)
Message was sent while issue was closed.
Description was changed from ========== RemoteContentSuggestions: Stores the time of the last successful background fetch in a pref. Therefore we also need to pass the fetch success status to the callback passed to the NTPSnippetsFetcher. The last successful background fetch time will be used by the scheduler in the future in order to decide when to do/schedule the next background fetch. Passing back the success status to the callback will also allow to add error handling. BUG=665873,672422 ========== to ========== RemoteContentSuggestions: Stores the time of the last successful background fetch in a pref. Therefore we also need to pass the fetch success status to the callback passed to the NTPSnippetsFetcher. The last successful background fetch time will be used by the scheduler in the future in order to decide when to do/schedule the next background fetch. Passing back the success status to the callback will also allow to add error handling. BUG=665873,672422 Committed: https://crrev.com/f6f620269ee0af3e7f8da7542a0cf04275248912 Cr-Commit-Position: refs/heads/master@{#437233} ==========
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/f6f620269ee0af3e7f8da7542a0cf04275248912 Cr-Commit-Position: refs/heads/master@{#437233}
Message was sent while issue was closed.
lgtm some follow-up / bystander comments. I was just curious and had a quick look. nothing too serious -- not even sure what's worth following up on. But I only realized you submitted after I finished the comments and don't want to simply delete them unread ;-) https://codereview.chromium.org/2549163002/diff/180001/components/ntp_snippet... File components/ntp_snippets/remote/ntp_snippets_fetcher_unittest.cc (right): https://codereview.chromium.org/2549163002/diff/180001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher_unittest.cc:730: .WillOnce(MoveArgument1PointeeTo(&fetched_categories)); nit: the matcher name is now a bit hard to understand. Alternatively you can use ::testing::WithArg<1> to extract the second argument and pass it into the funciton: .WillOnce(WithArg<0>(MoveArgumentPointeeTo(&...); If this feels to complicated, I'm also fine with renaming the helper, but maybe something easier to read like: MoveSecondArgumentPointeeTo(...); (the "1" is quite misleading as you don't expect 0-based indexing in usual language). https://codereview.chromium.org/2549163002/diff/180001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher_unittest.cc:965: .Times(1); nit: Times(1) can be omitted -- it's the default (https://wiki.corp.google.com/twiki/bin/view/Main/GMockGuide#Cardinalities_How...) So as long as this expectation is not a property tested by that test specificallly, we should probably omit. https://codereview.chromium.org/2549163002/diff/180001/components/ntp_snippet... File components/ntp_snippets/remote/remote_suggestions_provider.cc (right): https://codereview.chromium.org/2549163002/diff/180001/components/ntp_snippet... components/ntp_snippets/remote/remote_suggestions_provider.cc:721: pref_service_->SetInt64(prefs::kLastSuccessfulBackgroundFetchTime, just a comment -- no need to action: I guess that we might want to move this later to a place where we actually consume this data (guess that's going to be the remote-fetching-mastermind-class). You'll have to ask this class before we do any fetches and it seems then just symmetrical to also tell that class about the outcome of the fetch. No need to do this right now (not sure about the state of this), but if this sounds reasonable, a TODO() would be nice to find a proper home for this sort of book-keeping. https://codereview.chromium.org/2549163002/diff/180001/components/ntp_snippet... File components/ntp_snippets/remote/remote_suggestions_provider.h (right): https://codereview.chromium.org/2549163002/diff/180001/components/ntp_snippet... components/ntp_snippets/remote/remote_suggestions_provider.h:156: tick_clock_ = std::move(tick_clock); nit: Couldn't we dependency-inject this into the constructor instead? ForTesting() functions have a certain smell. https://codereview.chromium.org/2549163002/diff/180001/components/ntp_snippet... File components/ntp_snippets/remote/remote_suggestions_provider_unittest.cc (right): https://codereview.chromium.org/2549163002/diff/180001/components/ntp_snippet... components/ntp_snippets/remote/remote_suggestions_provider_unittest.cc:1612: WaitForSnippetsServiceInitialization(service.get(), can you add a comment explaining that a fetch request was already triggered (by which method? I guess MakeSnippetsServiceWithoutInitialization) but queued in the event-loop. So we're only going to see the effect after this function. https://codereview.chromium.org/2549163002/diff/180001/components/ntp_snippet... components/ntp_snippets/remote/remote_suggestions_provider_unittest.cc:1615: simple_test_tick_clock_ptr->NowTicks().ToInternalValue(), can you set up the clock's current time when initializing? How do I know it's not initialized to zero and this just works accidentally? |
