|
|
Created:
3 years, 8 months ago by dewittj Modified:
3 years, 8 months ago CC:
chili+watch_chromium.org, chromium-reviews, dewittj+watch_chromium.org, dimich+watch_chromium.org, Dmitry Titov, fgorski+watch_chromium.org, noyau+watch_chromium.org, ntp-dev+reviews_chromium.org, petewil+watch_chromium.org, romax+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
Description[Offline Pages] Set up the initial prefetching service.
This service does nothing by itself, but does listen to suggestions from
the ContentSuggestionsService and forwards suggestions that are not in
the offline page model to the empty prefetching service.
All of this is gated by the Offline Pages Prefetching about:flags entry.
BUG=701939
Review-Url: https://codereview.chromium.org/2811813002
Cr-Commit-Position: refs/heads/master@{#466747}
Committed: https://chromium.googlesource.com/chromium/src/+/094047d6fd5b831902dbb9a35416634024895e95
Patch Set 1 #
Total comments: 33
Patch Set 2 : class comment. #Patch Set 3 : Remove offline page model. #
Total comments: 24
Patch Set 4 : Adds client id #Patch Set 5 : fixes. #Patch Set 6 : some comments #Patch Set 7 : Implement invalidating a single ContentSuggestion. #
Total comments: 30
Patch Set 8 : comments. #
Total comments: 6
Patch Set 9 : moar comments #
Total comments: 2
Patch Set 10 : Go back to previous SetUserData API. #
Total comments: 4
Patch Set 11 : Move observer and prefetch service factory to components. #
Total comments: 8
Patch Set 12 : Comments. #Patch Set 13 : Make unit tests only non-ios #Patch Set 14 : fix compile >:-( #Messages
Total messages: 88 (59 generated)
The CQ bit was checked by dewittj@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: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by dewittj@chromium.org to run a CQ dry run
Patchset #1 (id:1) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== [Offline Pages] Set up the initial prefetching service. This service does nothing by itself, but does listen to suggestions from the ContentSuggestionsService and forwards suggestions that are not in the offline page model to the empty prefetching service. BUG= ========== to ========== [Offline Pages] Set up the initial prefetching service. This service does nothing by itself, but does listen to suggestions from the ContentSuggestionsService and forwards suggestions that are not in the offline page model to the empty prefetching service. BUG= ==========
dewittj@chromium.org changed reviewers: + carlosk@chromium.org, dimich@chromium.org
Description was changed from ========== [Offline Pages] Set up the initial prefetching service. This service does nothing by itself, but does listen to suggestions from the ContentSuggestionsService and forwards suggestions that are not in the offline page model to the empty prefetching service. BUG= ========== to ========== [Offline Pages] Set up the initial prefetching service. This service does nothing by itself, but does listen to suggestions from the ContentSuggestionsService and forwards suggestions that are not in the offline page model to the empty prefetching service. BUG=701939 ==========
Description was changed from ========== [Offline Pages] Set up the initial prefetching service. This service does nothing by itself, but does listen to suggestions from the ContentSuggestionsService and forwards suggestions that are not in the offline page model to the empty prefetching service. BUG=701939 ========== to ========== [Offline Pages] Set up the initial prefetching service. This service does nothing by itself, but does listen to suggestions from the ContentSuggestionsService and forwards suggestions that are not in the offline page model to the empty prefetching service. All of this is gated by the Offline Pages Prefetching about:flags entry. BUG=701939 ==========
dimich, carlosk: PTAL, this sets up the initial infrastructure for prefetching, and accepts suggestions from Zine. I have not finished the tests but want architecture suggestions if you have any before I go too far. The most "interesting" part of this is that the OfflinePageSuggestionsObserver doesn't instantiate a PrefetchService until it knows it has suggestions to forward. I thought this would be a nice efficiency step since I can assume that not all signals from Zine will be related to articles.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
dewittj@chromium.org changed reviewers: + jianli@chromium.org
+jianli
Nice! https://codereview.chromium.org/2811813002/diff/20001/chrome/browser/android/... File chrome/browser/android/offline_pages/offline_page_suggestions_observer.cc (right): https://codereview.chromium.org/2811813002/diff/20001/chrome/browser/android/... chrome/browser/android/offline_pages/offline_page_suggestions_observer.cc:157: for (const auto& page : pages) { That seems too strict. What if a user manually downloaded a page from X many months ago - this will prevent us downloading a fresh copy (which has content caused it to get into the suggestions today). I think the 'recently downloaded' schema that we discussed today is better in preventing 'duplicates' while not stopping the feature to download same URLs if they reappear in suggestions. https://codereview.chromium.org/2811813002/diff/20001/chrome/browser/android/... File chrome/browser/android/offline_pages/offline_page_suggestions_observer.h (right): https://codereview.chromium.org/2811813002/diff/20001/chrome/browser/android/... chrome/browser/android/offline_pages/offline_page_suggestions_observer.h:27: class OfflinePageSuggestionsObserver Perhaps this can be just SuggestionObserver. https://codereview.chromium.org/2811813002/diff/20001/chrome/browser/android/... chrome/browser/android/offline_pages/offline_page_suggestions_observer.h:50: OfflinePageSuggestionsObserver(content::BrowserContext* browser_context, nice to add an empty line before ctor. https://codereview.chromium.org/2811813002/diff/20001/chrome/browser/android/... File chrome/browser/android/offline_pages/prefetch_service_factory.cc (right): https://codereview.chromium.org/2811813002/diff/20001/chrome/browser/android/... chrome/browser/android/offline_pages/prefetch_service_factory.cc:42: scoped_refptr<base::SequencedTaskRunner> background_task_runner = this seems not used... https://codereview.chromium.org/2811813002/diff/20001/components/offline_page... File components/offline_pages/core/prefetch/prefetch_service.h (right): https://codereview.chromium.org/2811813002/diff/20001/components/offline_page... components/offline_pages/core/prefetch/prefetch_service.h:20: virtual void OnNewURLSuggestions(std::vector<GURL> suggested_urls) = 0; Not a request, but for consideration: Lets say at some point we want to use this similar to current RequestCoordinator, to ask to offline/download a page that perhaps user requested. What if we split the "suggestions-related" functionality into separate small class (maybe into the suggestions observer), so that PrefetchService would simply expose something like DownloadPages(list-of-urls). Eventually there will be a need for an Observer of some sort (either for updating some in-progress UI, popping up re-engagement notification etc) For this patch, if it is easy to avoid 'suggestions' to propagate into this class, it'd be cool. https://codereview.chromium.org/2811813002/diff/20001/components/offline_page... File components/offline_pages/core/prefetch/prefetch_service_impl.h (right): https://codereview.chromium.org/2811813002/diff/20001/components/offline_page... components/offline_pages/core/prefetch/prefetch_service_impl.h:25: void Shutdown() override; Is this ever called in Clank? I thought it's all 'sudden death' on Android.
Thanks for starting this. https://codereview.chromium.org/2811813002/diff/20001/chrome/browser/android/... File chrome/browser/android/offline_pages/offline_page_suggestions_observer.cc (right): https://codereview.chromium.org/2811813002/diff/20001/chrome/browser/android/... chrome/browser/android/offline_pages/offline_page_suggestions_observer.cc:19: int kOfflinePageSuggestionsObserverUserDataKey; Where is this initialized? https://codereview.chromium.org/2811813002/diff/20001/chrome/browser/android/... chrome/browser/android/offline_pages/offline_page_suggestions_observer.cc:24: // Suggestions Observer. This is unused in tests. nit: s/unused/replaced/ https://codereview.chromium.org/2811813002/diff/20001/chrome/browser/android/... chrome/browser/android/offline_pages/offline_page_suggestions_observer.cc:53: for (const auto& suggestion : nit: explicitly mentioning ContentSuggestion here instead of auto is clearer. https://codereview.chromium.org/2811813002/diff/20001/chrome/browser/android/... chrome/browser/android/offline_pages/offline_page_suggestions_observer.cc:145: service->RemoveAllUnprocessedURLSuggestions(); From reading the doc on this observer method [1] it seems we should also fetch new URLs following this cleanup. [1] https://cs.chromium.org/chromium/src/components/ntp_snippets/content_suggesti... https://codereview.chromium.org/2811813002/diff/20001/chrome/browser/android/... chrome/browser/android/offline_pages/offline_page_suggestions_observer.cc:157: for (const auto& page : pages) { On 2017/04/11 01:17:13, Dmitry Titov wrote: > That seems too strict. What if a user manually downloaded a page from X many > months ago - this will prevent us downloading a fresh copy (which has content > caused it to get into the suggestions today). > > I think the 'recently downloaded' schema that we discussed today is better in > preventing 'duplicates' while not stopping the feature to download same URLs if > they reappear in suggestions. I think the main point here is not re-downloading archives that are still available. Maybe restrict to only prefetched offline pages? I would not worry too much about the manually downloaded coincidences as they seem very unlikely. https://codereview.chromium.org/2811813002/diff/20001/chrome/browser/android/... File chrome/browser/android/offline_pages/offline_page_suggestions_observer.h (right): https://codereview.chromium.org/2811813002/diff/20001/chrome/browser/android/... chrome/browser/android/offline_pages/offline_page_suggestions_observer.h:27: class OfflinePageSuggestionsObserver Please add class comment. https://codereview.chromium.org/2811813002/diff/20001/chrome/browser/ntp_snip... File chrome/browser/ntp_snippets/content_suggestions_service_factory.cc (right): https://codereview.chromium.org/2811813002/diff/20001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/content_suggestions_service_factory.cc:369: ObserveContentSuggestionsService(profile, offline_page_model, service); How about renaming the Observer* method to MaybeObserve* and moving the feature check inside it? I think the logic deciding if an observer should observe or not should be closer to the observer (and not the observed). https://codereview.chromium.org/2811813002/diff/20001/components/ntp_snippets... File components/ntp_snippets/content_suggestions_service.h (right): https://codereview.chromium.org/2811813002/diff/20001/components/ntp_snippets... components/ntp_snippets/content_suggestions_service.h:51: public base::SupportsUserData, This change is to ease linking our observer lifetime to this service's, right? https://codereview.chromium.org/2811813002/diff/20001/components/offline_page... File components/offline_pages/core/prefetch/prefetch_service.h (right): https://codereview.chromium.org/2811813002/diff/20001/components/offline_page... components/offline_pages/core/prefetch/prefetch_service.h:14: class PrefetchService : public KeyedService { Please add a comment explaining what this class represents. https://codereview.chromium.org/2811813002/diff/20001/components/offline_page... components/offline_pages/core/prefetch/prefetch_service.h:18: // This will be called by OfflinePageSuggestionsObserver when suggestions nit: the potential caller should not be mentioned here. https://codereview.chromium.org/2811813002/diff/20001/components/offline_page... File components/offline_pages/core/prefetch/prefetch_service_impl_unittest.cc (right): https://codereview.chromium.org/2811813002/diff/20001/components/offline_page... components/offline_pages/core/prefetch/prefetch_service_impl_unittest.cc:16: service.RemoveAllUnprocessedURLSuggestions(); Question: how come debug build tests don't crash here because of the NOTIMPLEMENTED calls?
Patchset #2 (id:40001) has been deleted
PTAL, thanks! https://codereview.chromium.org/2811813002/diff/20001/chrome/browser/android/... File chrome/browser/android/offline_pages/offline_page_suggestions_observer.cc (right): https://codereview.chromium.org/2811813002/diff/20001/chrome/browser/android/... chrome/browser/android/offline_pages/offline_page_suggestions_observer.cc:19: int kOfflinePageSuggestionsObserverUserDataKey; On 2017/04/11 17:31:35, carlosk wrote: > Where is this initialized? Nowhere, its address is used for KeyedService purposes. Its value is unused. https://codereview.chromium.org/2811813002/diff/20001/chrome/browser/android/... chrome/browser/android/offline_pages/offline_page_suggestions_observer.cc:24: // Suggestions Observer. This is unused in tests. On 2017/04/11 17:31:36, carlosk wrote: > nit: s/unused/replaced/ DefaultDelegate is unused :) Reworded. https://codereview.chromium.org/2811813002/diff/20001/chrome/browser/android/... chrome/browser/android/offline_pages/offline_page_suggestions_observer.cc:53: for (const auto& suggestion : On 2017/04/11 17:31:35, carlosk wrote: > nit: explicitly mentioning ContentSuggestion here instead of auto is clearer. Done. https://codereview.chromium.org/2811813002/diff/20001/chrome/browser/android/... chrome/browser/android/offline_pages/offline_page_suggestions_observer.cc:145: service->RemoveAllUnprocessedURLSuggestions(); On 2017/04/11 17:31:35, carlosk wrote: > From reading the doc on this observer method [1] it seems we should also fetch > new URLs following this cleanup. > > [1] > https://cs.chromium.org/chromium/src/components/ntp_snippets/content_suggesti... Done. https://codereview.chromium.org/2811813002/diff/20001/chrome/browser/android/... chrome/browser/android/offline_pages/offline_page_suggestions_observer.cc:157: for (const auto& page : pages) { On 2017/04/11 17:31:35, carlosk wrote: > On 2017/04/11 01:17:13, Dmitry Titov wrote: > > That seems too strict. What if a user manually downloaded a page from X many > > months ago - this will prevent us downloading a fresh copy (which has content > > caused it to get into the suggestions today). > > > > I think the 'recently downloaded' schema that we discussed today is better in > > preventing 'duplicates' while not stopping the feature to download same URLs > if > > they reappear in suggestions. > > I think the main point here is not re-downloading archives that are still > available. Maybe restrict to only prefetched offline pages? > > I would not worry too much about the manually downloaded coincidences as they > seem very unlikely. Maybe it is premature to do the offline page filtering in this patch. I've removed it and will leave filtering to a future patch (perhaps adding a CheckForDuplicatesOrZombies function to the PrefetchService). https://codereview.chromium.org/2811813002/diff/20001/chrome/browser/android/... File chrome/browser/android/offline_pages/offline_page_suggestions_observer.h (right): https://codereview.chromium.org/2811813002/diff/20001/chrome/browser/android/... chrome/browser/android/offline_pages/offline_page_suggestions_observer.h:27: class OfflinePageSuggestionsObserver On 2017/04/11 01:17:13, Dmitry Titov wrote: > Perhaps this can be just SuggestionObserver. Done. https://codereview.chromium.org/2811813002/diff/20001/chrome/browser/android/... chrome/browser/android/offline_pages/offline_page_suggestions_observer.h:27: class OfflinePageSuggestionsObserver On 2017/04/11 17:31:36, carlosk wrote: > Please add class comment. Done. https://codereview.chromium.org/2811813002/diff/20001/chrome/browser/android/... chrome/browser/android/offline_pages/offline_page_suggestions_observer.h:50: OfflinePageSuggestionsObserver(content::BrowserContext* browser_context, On 2017/04/11 01:17:13, Dmitry Titov wrote: > nice to add an empty line before ctor. Done. https://codereview.chromium.org/2811813002/diff/20001/chrome/browser/android/... File chrome/browser/android/offline_pages/prefetch_service_factory.cc (right): https://codereview.chromium.org/2811813002/diff/20001/chrome/browser/android/... chrome/browser/android/offline_pages/prefetch_service_factory.cc:42: scoped_refptr<base::SequencedTaskRunner> background_task_runner = On 2017/04/11 01:17:13, Dmitry Titov wrote: > this seems not used... Done. https://codereview.chromium.org/2811813002/diff/20001/chrome/browser/ntp_snip... File chrome/browser/ntp_snippets/content_suggestions_service_factory.cc (right): https://codereview.chromium.org/2811813002/diff/20001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/content_suggestions_service_factory.cc:369: ObserveContentSuggestionsService(profile, offline_page_model, service); On 2017/04/11 17:31:36, carlosk wrote: > How about renaming the Observer* method to MaybeObserve* and moving the feature > check inside it? I think the logic deciding if an observer should observe or not > should be closer to the observer (and not the observed). Done, though I don't think the Maybe is strictly necessary. PTAL. https://codereview.chromium.org/2811813002/diff/20001/components/ntp_snippets... File components/ntp_snippets/content_suggestions_service.h (right): https://codereview.chromium.org/2811813002/diff/20001/components/ntp_snippets... components/ntp_snippets/content_suggestions_service.h:51: public base::SupportsUserData, On 2017/04/11 17:31:36, carlosk wrote: > This change is to ease linking our observer lifetime to this service's, right? yes. https://codereview.chromium.org/2811813002/diff/20001/components/offline_page... File components/offline_pages/core/prefetch/prefetch_service.h (right): https://codereview.chromium.org/2811813002/diff/20001/components/offline_page... components/offline_pages/core/prefetch/prefetch_service.h:14: class PrefetchService : public KeyedService { On 2017/04/11 17:31:36, carlosk wrote: > Please add a comment explaining what this class represents. Done. https://codereview.chromium.org/2811813002/diff/20001/components/offline_page... components/offline_pages/core/prefetch/prefetch_service.h:18: // This will be called by OfflinePageSuggestionsObserver when suggestions On 2017/04/11 17:31:36, carlosk wrote: > nit: the potential caller should not be mentioned here. Done. https://codereview.chromium.org/2811813002/diff/20001/components/offline_page... components/offline_pages/core/prefetch/prefetch_service.h:20: virtual void OnNewURLSuggestions(std::vector<GURL> suggested_urls) = 0; On 2017/04/11 01:17:14, Dmitry Titov wrote: > Not a request, but for consideration: Lets say at some point we want to use this > similar to current RequestCoordinator, to ask to offline/download a page that > perhaps user requested. What if we split the "suggestions-related" functionality > into separate small class (maybe into the suggestions observer), so that > PrefetchService would simply expose something like DownloadPages(list-of-urls). > > Eventually there will be a need for an Observer of some sort (either for > updating some in-progress UI, popping up re-engagement notification etc) > > For this patch, if it is easy to avoid 'suggestions' to propagate into this > class, it'd be cool. Done. https://codereview.chromium.org/2811813002/diff/20001/components/offline_page... File components/offline_pages/core/prefetch/prefetch_service_impl.h (right): https://codereview.chromium.org/2811813002/diff/20001/components/offline_page... components/offline_pages/core/prefetch/prefetch_service_impl.h:25: void Shutdown() override; On 2017/04/11 01:17:14, Dmitry Titov wrote: > Is this ever called in Clank? I thought it's all 'sudden death' on Android. Possibly not, but since this code lives in Components (and I expect that this could enable offline features on iOS in particular) I don't want to assume too much. https://codereview.chromium.org/2811813002/diff/20001/components/offline_page... File components/offline_pages/core/prefetch/prefetch_service_impl_unittest.cc (right): https://codereview.chromium.org/2811813002/diff/20001/components/offline_page... components/offline_pages/core/prefetch/prefetch_service_impl_unittest.cc:16: service.RemoveAllUnprocessedURLSuggestions(); On 2017/04/11 17:31:36, carlosk wrote: > Question: how come debug build tests don't crash here because of the > NOTIMPLEMENTED calls? NOTIMPLEMENTED does not crash, it just logs. NOTREACHED() crashes.
https://codereview.chromium.org/2811813002/diff/100001/chrome/browser/android... File chrome/browser/android/offline_pages/offline_page_suggestions_observer.cc (right): https://codereview.chromium.org/2811813002/diff/100001/chrome/browser/android... chrome/browser/android/offline_pages/offline_page_suggestions_observer.cc:114: service->RemoveAllUnprocessedURLsToPrefetch(); Do you mean to have a TODO here to actually remove only the ones from a category? https://codereview.chromium.org/2811813002/diff/100001/chrome/browser/android... File chrome/browser/android/offline_pages/offline_page_suggestions_observer.h (right): https://codereview.chromium.org/2811813002/diff/100001/chrome/browser/android... chrome/browser/android/offline_pages/offline_page_suggestions_observer.h:6: #define CHROME_BROWSER_ANDROID_OFFLINE_PAGES_OFFLINE_PAGE_SUGGESTIONS_OBSERVER_H_ These ifdefs and file name need to match the class name. https://codereview.chromium.org/2811813002/diff/100001/components/offline_pag... File components/offline_pages/core/prefetch/prefetch_service.h (right): https://codereview.chromium.org/2811813002/diff/100001/components/offline_pag... components/offline_pages/core/prefetch/prefetch_service.h:17: class PrefetchService : public KeyedService { As formulated, the service may only have a single client. In this case, Zine Suggestions are assumed. If we ever want to use this capability for anything else (like offlining on Svelte), we can't because there is no 'segregation' between URLs - for example, RemoveAllUnprocessedURLsToPrefetch() indiscriminately deletes all URLs. I wonder if it is easier to add either a 'namespace' (string or enum) or a guid paired with each url or even a ClientId-like struct, so the client may do 'remove by category' for example? https://codereview.chromium.org/2811813002/diff/100001/components/offline_pag... components/offline_pages/core/prefetch/prefetch_service.h:22: // the only consumer is article suggestions. can we just remove use of 'suggestions' and just use 'pages' or 'urls'?
https://codereview.chromium.org/2811813002/diff/100001/chrome/browser/android... File chrome/browser/android/offline_pages/offline_page_suggestions_observer.cc (right): https://codereview.chromium.org/2811813002/diff/100001/chrome/browser/android... chrome/browser/android/offline_pages/offline_page_suggestions_observer.cc:74: suggestions_observer.release()); Who is responsible to clean this up? https://codereview.chromium.org/2811813002/diff/100001/chrome/browser/android... chrome/browser/android/offline_pages/offline_page_suggestions_observer.cc:79: std::unique_ptr<Delegate> delegate, nit: normally we pass delegate as the last argument https://codereview.chromium.org/2811813002/diff/100001/chrome/browser/android... chrome/browser/android/offline_pages/offline_page_suggestions_observer.cc:81: : delegate_(std::move(delegate)), nit: it would be better to declare these member variables in same order as parameters, if possible. https://codereview.chromium.org/2811813002/diff/100001/chrome/browser/android... chrome/browser/android/offline_pages/offline_page_suggestions_observer.cc:107: category_status_ = new_status; Do we want a DCHECK to ensure new_status is different? https://codereview.chromium.org/2811813002/diff/100001/chrome/browser/android... File chrome/browser/android/offline_pages/offline_page_suggestions_observer.h (right): https://codereview.chromium.org/2811813002/diff/100001/chrome/browser/android... chrome/browser/android/offline_pages/offline_page_suggestions_observer.h:56: // ContentSuggestionsService::Observer overrides nit: end with ":" https://codereview.chromium.org/2811813002/diff/100001/chrome/browser/android... File chrome/browser/android/offline_pages/prefetch_service_factory.h (right): https://codereview.chromium.org/2811813002/diff/100001/chrome/browser/android... chrome/browser/android/offline_pages/prefetch_service_factory.h:23: // TODO(fgorski): Add an integration test that ensures incognito users don't nit: update comment https://codereview.chromium.org/2811813002/diff/100001/components/offline_pag... File components/offline_pages/core/prefetch/prefetch_service.h (right): https://codereview.chromium.org/2811813002/diff/100001/components/offline_pag... components/offline_pages/core/prefetch/prefetch_service.h:23: virtual void OnNewURLsToPrefetch(std::vector<GURL> suggested_urls) = 0; nit: Please add const &. https://codereview.chromium.org/2811813002/diff/100001/components/offline_pag... components/offline_pages/core/prefetch/prefetch_service.h:28: virtual void RemoveAllUnprocessedURLsToPrefetch() = 0; Since class name is PrefetchService, we can probably drop "ToPrefetch" from method name.
The CQ bit was checked by dewittj@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/2811813002/diff/100001/chrome/browser/android... File chrome/browser/android/offline_pages/offline_page_suggestions_observer.cc (right): https://codereview.chromium.org/2811813002/diff/100001/chrome/browser/android... chrome/browser/android/offline_pages/offline_page_suggestions_observer.cc:74: suggestions_observer.release()); On 2017/04/11 22:39:55, jianli wrote: > Who is responsible to clean this up? UserData::Data is owned by the class that inherits from base::SupportsUserData. https://codereview.chromium.org/2811813002/diff/100001/chrome/browser/android... chrome/browser/android/offline_pages/offline_page_suggestions_observer.cc:79: std::unique_ptr<Delegate> delegate, On 2017/04/11 22:39:55, jianli wrote: > nit: normally we pass delegate as the last argument Done. https://codereview.chromium.org/2811813002/diff/100001/chrome/browser/android... chrome/browser/android/offline_pages/offline_page_suggestions_observer.cc:81: : delegate_(std::move(delegate)), On 2017/04/11 22:39:55, jianli wrote: > nit: it would be better to declare these member variables in same order as > parameters, if possible. Done. https://codereview.chromium.org/2811813002/diff/100001/chrome/browser/android... chrome/browser/android/offline_pages/offline_page_suggestions_observer.cc:107: category_status_ = new_status; On 2017/04/11 22:39:55, jianli wrote: > Do we want a DCHECK to ensure new_status is different? It seems aggressive to do that. Perhaps an early return is better. https://codereview.chromium.org/2811813002/diff/100001/chrome/browser/android... chrome/browser/android/offline_pages/offline_page_suggestions_observer.cc:114: service->RemoveAllUnprocessedURLsToPrefetch(); On 2017/04/11 21:50:46, Dmitry Titov wrote: > Do you mean to have a TODO here to actually remove only the ones from a > category? Done. https://codereview.chromium.org/2811813002/diff/100001/chrome/browser/android... File chrome/browser/android/offline_pages/offline_page_suggestions_observer.h (right): https://codereview.chromium.org/2811813002/diff/100001/chrome/browser/android... chrome/browser/android/offline_pages/offline_page_suggestions_observer.h:6: #define CHROME_BROWSER_ANDROID_OFFLINE_PAGES_OFFLINE_PAGE_SUGGESTIONS_OBSERVER_H_ On 2017/04/11 21:50:46, Dmitry Titov wrote: > These ifdefs and file name need to match the class name. Done. https://codereview.chromium.org/2811813002/diff/100001/chrome/browser/android... chrome/browser/android/offline_pages/offline_page_suggestions_observer.h:56: // ContentSuggestionsService::Observer overrides On 2017/04/11 22:39:55, jianli wrote: > nit: end with ":" Done. https://codereview.chromium.org/2811813002/diff/100001/chrome/browser/android... File chrome/browser/android/offline_pages/prefetch_service_factory.h (right): https://codereview.chromium.org/2811813002/diff/100001/chrome/browser/android... chrome/browser/android/offline_pages/prefetch_service_factory.h:23: // TODO(fgorski): Add an integration test that ensures incognito users don't On 2017/04/11 22:39:55, jianli wrote: > nit: update comment Done. https://codereview.chromium.org/2811813002/diff/100001/components/offline_pag... File components/offline_pages/core/prefetch/prefetch_service.h (right): https://codereview.chromium.org/2811813002/diff/100001/components/offline_pag... components/offline_pages/core/prefetch/prefetch_service.h:17: class PrefetchService : public KeyedService { On 2017/04/11 21:50:46, Dmitry Titov wrote: > As formulated, the service may only have a single client. In this case, Zine > Suggestions are assumed. If we ever want to use this capability for anything > else (like offlining on Svelte), we can't because there is no 'segregation' > between URLs - for example, RemoveAllUnprocessedURLsToPrefetch() > indiscriminately deletes all URLs. > > I wonder if it is easier to add either a 'namespace' (string or enum) or a guid > paired with each url or even a ClientId-like struct, so the client may do > 'remove by category' for example? Done. https://codereview.chromium.org/2811813002/diff/100001/components/offline_pag... components/offline_pages/core/prefetch/prefetch_service.h:22: // the only consumer is article suggestions. On 2017/04/11 21:50:46, Dmitry Titov wrote: > can we just remove use of 'suggestions' and just use 'pages' or 'urls'? Done. https://codereview.chromium.org/2811813002/diff/100001/components/offline_pag... components/offline_pages/core/prefetch/prefetch_service.h:23: virtual void OnNewURLsToPrefetch(std::vector<GURL> suggested_urls) = 0; On 2017/04/11 22:39:55, jianli wrote: > nit: Please add const &. Done. https://codereview.chromium.org/2811813002/diff/100001/components/offline_pag... components/offline_pages/core/prefetch/prefetch_service.h:28: virtual void RemoveAllUnprocessedURLsToPrefetch() = 0; On 2017/04/11 22:39:55, jianli wrote: > Since class name is PrefetchService, we can probably drop "ToPrefetch" from > method name. I updated it to refer to the PrefetchURL struct instead. WDYT
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
dewittj@chromium.org changed reviewers: + jkrcal@chromium.org
+ jkrcal - NTP integration, including ContentSuggestionsService and SuggestionsObserver Please let me know if I'm holding it right.
Patchset #2 (id:60001) has been deleted
friendly ping :)
The CQ bit was checked by dewittj@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: This issue passed the CQ dry run.
dewittj@chromium.org changed reviewers: - dimich@chromium.org
-dimich to CC since he's OOO
https://codereview.chromium.org/2811813002/diff/180001/chrome/browser/android... File chrome/browser/android/offline_pages/prefetch_service_factory.h (right): https://codereview.chromium.org/2811813002/diff/180001/chrome/browser/android... chrome/browser/android/offline_pages/prefetch_service_factory.h:20: // A factory to create one unique PrefetchService. Prefetching Offline Pages is nit: "one unique PrefetchService" => "PrefetchService instances per BrowserContext" https://codereview.chromium.org/2811813002/diff/180001/chrome/browser/android... File chrome/browser/android/offline_pages/suggestions_observer.cc (right): https://codereview.chromium.org/2811813002/diff/180001/chrome/browser/android... chrome/browser/android/offline_pages/suggestions_observer.cc:137: service->RemoveAllUnprocessedPrefetchURLs(kSuggestedArticlesNamespace); Are all PrefetchService operations asynchronous? If so, do we need to wait until is finish before we can call OnNewSuggestions? https://codereview.chromium.org/2811813002/diff/180001/chrome/browser/android... File chrome/browser/android/offline_pages/suggestions_observer.h (right): https://codereview.chromium.org/2811813002/diff/180001/chrome/browser/android... chrome/browser/android/offline_pages/suggestions_observer.h:39: class Delegate { It just seems to me that using this technique though make the test code a bit easy to write, but it makes the production code more complicated and a bit hard to follow. Can we use SetTestingFactoryAndUse for GetPrefetchService? For GetSuggestions, can we pass a ContentSuggestionService instance which can be mocked in the test code? https://codereview.chromium.org/2811813002/diff/180001/components/ntp_snippet... File components/ntp_snippets/content_suggestions_service.h (right): https://codereview.chromium.org/2811813002/diff/180001/components/ntp_snippet... components/ntp_snippets/content_suggestions_service.h:51: public base::SupportsUserData, Why is this needed? https://codereview.chromium.org/2811813002/diff/180001/components/offline_pag... File components/offline_pages/core/prefetch/prefetch_service.h (right): https://codereview.chromium.org/2811813002/diff/180001/components/offline_pag... components/offline_pages/core/prefetch/prefetch_service.h:33: virtual void OnNewURLsToPrefetch( Can we rename this to something like PrefetchURLs since OnNewURLsToPrefetch sounds a bit permissive to understand?
https://codereview.chromium.org/2811813002/diff/180001/chrome/browser/android... File chrome/browser/android/offline_pages/prefetch_service_factory.h (right): https://codereview.chromium.org/2811813002/diff/180001/chrome/browser/android... chrome/browser/android/offline_pages/prefetch_service_factory.h:20: // A factory to create one unique PrefetchService. Prefetching Offline Pages is On 2017/04/17 21:52:58, jianli wrote: > nit: "one unique PrefetchService" => "PrefetchService instances per > BrowserContext" Done. https://codereview.chromium.org/2811813002/diff/180001/chrome/browser/android... File chrome/browser/android/offline_pages/suggestions_observer.cc (right): https://codereview.chromium.org/2811813002/diff/180001/chrome/browser/android... chrome/browser/android/offline_pages/suggestions_observer.cc:137: service->RemoveAllUnprocessedPrefetchURLs(kSuggestedArticlesNamespace); On 2017/04/17 21:52:58, jianli wrote: > Are all PrefetchService operations asynchronous? If so, do we need to wait until > is finish before we can call OnNewSuggestions? They are, but hopefully they will be serialized using task queue. So unless the observer requires a result, we should not need to wait. https://codereview.chromium.org/2811813002/diff/180001/chrome/browser/android... File chrome/browser/android/offline_pages/suggestions_observer.h (right): https://codereview.chromium.org/2811813002/diff/180001/chrome/browser/android... chrome/browser/android/offline_pages/suggestions_observer.h:39: class Delegate { On 2017/04/17 21:52:58, jianli wrote: > It just seems to me that using this technique though make the test code a bit > easy to write, but it makes the production code more complicated and a bit hard > to follow. Can we use SetTestingFactoryAndUse for GetPrefetchService? For > GetSuggestions, can we pass a ContentSuggestionService instance which can be > mocked in the test code? Mocking ContentSuggestionService turned out to be a lot of code. I actually implemented that before, and it required extensive harness setup. So once we have a delegate or even a raw base::Callback<...> for content suggestions it doesn't make sense to have multiple strategies for injection. https://codereview.chromium.org/2811813002/diff/180001/components/ntp_snippet... File components/ntp_snippets/content_suggestions_service.h (right): https://codereview.chromium.org/2811813002/diff/180001/components/ntp_snippet... components/ntp_snippets/content_suggestions_service.h:51: public base::SupportsUserData, On 2017/04/17 21:52:58, jianli wrote: > Why is this needed? This is needed so that ContentSuggestionsService can own the SuggestionsObserver. Other implementations might require ContentSuggestionsService to actually know about the SuggestionsObserver, or for the Observer to manage its own lifetime using DeleteSoon. I had that type of API before and it did not seem very reliable. https://codereview.chromium.org/2811813002/diff/180001/components/offline_pag... File components/offline_pages/core/prefetch/prefetch_service.h (right): https://codereview.chromium.org/2811813002/diff/180001/components/offline_pag... components/offline_pages/core/prefetch/prefetch_service.h:33: virtual void OnNewURLsToPrefetch( On 2017/04/17 21:52:58, jianli wrote: > Can we rename this to something like PrefetchURLs since OnNewURLsToPrefetch > sounds a bit permissive to understand? Done.
https://codereview.chromium.org/2811813002/diff/180001/chrome/browser/android... File chrome/browser/android/offline_pages/suggestions_observer.cc (right): https://codereview.chromium.org/2811813002/diff/180001/chrome/browser/android... chrome/browser/android/offline_pages/suggestions_observer.cc:23: int kOfflinePageSuggestionsObserverUserDataKey; Shouldn't this be inside the anonymous namespace too? https://codereview.chromium.org/2811813002/diff/180001/chrome/browser/android... chrome/browser/android/offline_pages/suggestions_observer.cc:25: namespace { Are anonymous namespaces within named namespaces also guaranteed to eliminate side effects from its encompassed members to other compilation units? https://codereview.chromium.org/2811813002/diff/180001/chrome/browser/android... chrome/browser/android/offline_pages/suggestions_observer.cc:77: suggestions_observer.release()); Is it impossible to use std::move here? https://codereview.chromium.org/2811813002/diff/180001/chrome/browser/android... File chrome/browser/android/offline_pages/suggestions_observer.h (right): https://codereview.chromium.org/2811813002/diff/180001/chrome/browser/android... chrome/browser/android/offline_pages/suggestions_observer.h:37: // as possible, since it will only be covered by instrumentation/browser nit: if by covered you mean overridden or re-implemented I think these latter two words are clearer. https://codereview.chromium.org/2811813002/diff/180001/chrome/browser/android... File chrome/browser/android/offline_pages/suggestions_observer_unittest.cc (right): https://codereview.chromium.org/2811813002/diff/180001/chrome/browser/android... chrome/browser/android/offline_pages/suggestions_observer_unittest.cc:151: EXPECT_EQ(test_url_1, test_prefetch_service()->latest_prefetch_urls[0].url); Duplicate line. https://codereview.chromium.org/2811813002/diff/180001/components/offline_pag... File components/offline_pages/core/client_policy_controller.cc (right): https://codereview.chromium.org/2811813002/diff/180001/components/offline_pag... components/offline_pages/core/client_policy_controller.cc:61: kUnlimitedPages) Shouldn't the limit of pages per URL be 1? https://codereview.chromium.org/2811813002/diff/180001/components/offline_pag... File components/offline_pages/core/prefetch/prefetch_service.h (right): https://codereview.chromium.org/2811813002/diff/180001/components/offline_pag... components/offline_pages/core/prefetch/prefetch_service.h:32: // Called when a consumer has new URLs for the system to prefetch. nit: s/has new/has potentially new/ ? I'm not really sure how to phrase it but I think it should be made clear here that PrefetchService must be able to accept repeated URL suggestions. https://codereview.chromium.org/2811813002/diff/180001/components/offline_pag... components/offline_pages/core/prefetch/prefetch_service.h:38: // have not yet started downloading within that namespace. IRT "have not yet started downloading": why not delete ongoing downloads too? Or even why not delete all prefetched pages? Has the scope of content to be deleted upon this event been discussed?
https://codereview.chromium.org/2811813002/diff/200001/chrome/browser/android... File chrome/browser/android/offline_pages/suggestions_observer.cc (right): https://codereview.chromium.org/2811813002/diff/200001/chrome/browser/android... chrome/browser/android/offline_pages/suggestions_observer.cc:102: for (auto& suggestion : suggestions) { nit: const auto& https://codereview.chromium.org/2811813002/diff/200001/chrome/browser/android... File chrome/browser/android/offline_pages/suggestions_observer.h (right): https://codereview.chromium.org/2811813002/diff/200001/chrome/browser/android... chrome/browser/android/offline_pages/suggestions_observer.h:26: // single category (initially ARTICLES). When those suggestions arrive, it Do we have a plan to make SuggestionsObserver support other category in the future? If not, this class should be strictly associated with articles, like renaming to SuggestedArticlesObserver. Otherwise, we should make it more flexible by passing the hard-coded values from outside. https://codereview.chromium.org/2811813002/diff/200001/components/offline_pag... File components/offline_pages/core/prefetch/prefetch_service.h (right): https://codereview.chromium.org/2811813002/diff/200001/components/offline_pag... components/offline_pages/core/prefetch/prefetch_service.h:33: virtual void OnNewPrefetchURLs( I think some URLs might not be new since PrefetchService might be still processing them. My understanding is that a consumer has a list of URLs to ask PrefetchService to do the prefetch.
Thanks! PTAL https://codereview.chromium.org/2811813002/diff/180001/chrome/browser/android... File chrome/browser/android/offline_pages/suggestions_observer.cc (right): https://codereview.chromium.org/2811813002/diff/180001/chrome/browser/android... chrome/browser/android/offline_pages/suggestions_observer.cc:23: int kOfflinePageSuggestionsObserverUserDataKey; On 2017/04/18 00:36:09, carlosk wrote: > Shouldn't this be inside the anonymous namespace too? Done. https://codereview.chromium.org/2811813002/diff/180001/chrome/browser/android... chrome/browser/android/offline_pages/suggestions_observer.cc:25: namespace { On 2017/04/18 00:36:09, carlosk wrote: > Are anonymous namespaces within named namespaces also guaranteed to eliminate > side effects from its encompassed members to other compilation units? I'm not sure what you are trying to ask. In my experience it is only used to prevent names from polluting the global namespaces. https://codereview.chromium.org/2811813002/diff/180001/chrome/browser/android... chrome/browser/android/offline_pages/suggestions_observer.cc:77: suggestions_observer.release()); On 2017/04/18 00:36:09, carlosk wrote: > Is it impossible to use std::move here? Done. Note that the obvious way seemed to fail because the suggestions_observer is a derived class from base::SupportsUserData::Data. https://codereview.chromium.org/2811813002/diff/180001/chrome/browser/android... File chrome/browser/android/offline_pages/suggestions_observer.h (right): https://codereview.chromium.org/2811813002/diff/180001/chrome/browser/android... chrome/browser/android/offline_pages/suggestions_observer.h:37: // as possible, since it will only be covered by instrumentation/browser On 2017/04/18 00:36:09, carlosk wrote: > nit: if by covered you mean overridden or re-implemented I think these latter > two words are clearer. no, I mean test coverage in particular. https://codereview.chromium.org/2811813002/diff/180001/chrome/browser/android... File chrome/browser/android/offline_pages/suggestions_observer_unittest.cc (right): https://codereview.chromium.org/2811813002/diff/180001/chrome/browser/android... chrome/browser/android/offline_pages/suggestions_observer_unittest.cc:151: EXPECT_EQ(test_url_1, test_prefetch_service()->latest_prefetch_urls[0].url); On 2017/04/18 00:36:09, carlosk wrote: > Duplicate line. Done. https://codereview.chromium.org/2811813002/diff/180001/components/offline_pag... File components/offline_pages/core/client_policy_controller.cc (right): https://codereview.chromium.org/2811813002/diff/180001/components/offline_pag... components/offline_pages/core/client_policy_controller.cc:61: kUnlimitedPages) On 2017/04/18 00:36:09, carlosk wrote: > Shouldn't the limit of pages per URL be 1? I don't think we should enforce this here. I think the case where a URL is suggested, then a week later is not suggested, then a week later becomes suggested again should be supported. Right? https://codereview.chromium.org/2811813002/diff/180001/components/offline_pag... File components/offline_pages/core/prefetch/prefetch_service.h (right): https://codereview.chromium.org/2811813002/diff/180001/components/offline_pag... components/offline_pages/core/prefetch/prefetch_service.h:32: // Called when a consumer has new URLs for the system to prefetch. On 2017/04/18 00:36:09, carlosk wrote: > nit: s/has new/has potentially new/ ? > I'm not really sure how to phrase it but I think it should be made clear here > that PrefetchService must be able to accept repeated URL suggestions. Done. https://codereview.chromium.org/2811813002/diff/180001/components/offline_pag... components/offline_pages/core/prefetch/prefetch_service.h:38: // have not yet started downloading within that namespace. On 2017/04/18 00:36:09, carlosk wrote: > IRT "have not yet started downloading": why not delete ongoing downloads too? Or > even why not delete all prefetched pages? Has the scope of content to be deleted > upon this event been discussed? I recall we want to treat pages that have started downloading as somewhat more permanent, since we have spent potentially significant user data/power on it already. There is some discussion on the design doc. I prefer to land this then modify the comment if we decide on something different. https://codereview.chromium.org/2811813002/diff/200001/chrome/browser/android... File chrome/browser/android/offline_pages/suggestions_observer.cc (right): https://codereview.chromium.org/2811813002/diff/200001/chrome/browser/android... chrome/browser/android/offline_pages/suggestions_observer.cc:102: for (auto& suggestion : suggestions) { On 2017/04/18 00:41:00, jianli wrote: > nit: const auto& Done. https://codereview.chromium.org/2811813002/diff/200001/chrome/browser/android... File chrome/browser/android/offline_pages/suggestions_observer.h (right): https://codereview.chromium.org/2811813002/diff/200001/chrome/browser/android... chrome/browser/android/offline_pages/suggestions_observer.h:26: // single category (initially ARTICLES). When those suggestions arrive, it On 2017/04/18 00:41:00, jianli wrote: > Do we have a plan to make SuggestionsObserver support other category in the > future? If not, this class should be strictly associated with articles, like > renaming to SuggestedArticlesObserver. Otherwise, we should make it more > flexible by passing the hard-coded values from outside. Done. https://codereview.chromium.org/2811813002/diff/200001/components/offline_pag... File components/offline_pages/core/prefetch/prefetch_service.h (right): https://codereview.chromium.org/2811813002/diff/200001/components/offline_pag... components/offline_pages/core/prefetch/prefetch_service.h:33: virtual void OnNewPrefetchURLs( On 2017/04/18 00:41:00, jianli wrote: > I think some URLs might not be new since PrefetchService might be still > processing them. My understanding is that a consumer has a list of URLs to ask > PrefetchService to do the prefetch. Changed to AddCandidatePrefetchURLs.
LGTM with one nit. https://codereview.chromium.org/2811813002/diff/180001/chrome/browser/android... File chrome/browser/android/offline_pages/suggestions_observer.cc (right): https://codereview.chromium.org/2811813002/diff/180001/chrome/browser/android... chrome/browser/android/offline_pages/suggestions_observer.cc:25: namespace { On 2017/04/18 18:20:25, dewittj wrote: > On 2017/04/18 00:36:09, carlosk wrote: > > Are anonymous namespaces within named namespaces also guaranteed to eliminate > > side effects from its encompassed members to other compilation units? > > I'm not sure what you are trying to ask. In my experience it is only used to > prevent names from polluting the global namespaces. Acknowledged. https://codereview.chromium.org/2811813002/diff/180001/chrome/browser/android... File chrome/browser/android/offline_pages/suggestions_observer.h (right): https://codereview.chromium.org/2811813002/diff/180001/chrome/browser/android... chrome/browser/android/offline_pages/suggestions_observer.h:37: // as possible, since it will only be covered by instrumentation/browser On 2017/04/18 18:20:25, dewittj wrote: > On 2017/04/18 00:36:09, carlosk wrote: > > nit: if by covered you mean overridden or re-implemented I think these latter > > two words are clearer. > > no, I mean test coverage in particular. Acknowledged. https://codereview.chromium.org/2811813002/diff/180001/components/offline_pag... File components/offline_pages/core/client_policy_controller.cc (right): https://codereview.chromium.org/2811813002/diff/180001/components/offline_pag... components/offline_pages/core/client_policy_controller.cc:61: kUnlimitedPages) On 2017/04/18 18:20:25, dewittj wrote: > On 2017/04/18 00:36:09, carlosk wrote: > > Shouldn't the limit of pages per URL be 1? > > I don't think we should enforce this here. I think the case where a URL is > suggested, then a week later is not suggested, then a week later becomes > suggested again should be supported. Right? OK. We can change this later on should we decide to not re-download pages that are already in the model database. https://codereview.chromium.org/2811813002/diff/180001/components/offline_pag... File components/offline_pages/core/prefetch/prefetch_service.h (right): https://codereview.chromium.org/2811813002/diff/180001/components/offline_pag... components/offline_pages/core/prefetch/prefetch_service.h:38: // have not yet started downloading within that namespace. On 2017/04/18 18:20:25, dewittj wrote: > On 2017/04/18 00:36:09, carlosk wrote: > > IRT "have not yet started downloading": why not delete ongoing downloads too? > Or > > even why not delete all prefetched pages? Has the scope of content to be > deleted > > upon this event been discussed? > > I recall we want to treat pages that have started downloading as somewhat more > permanent, since we have spent potentially significant user data/power on it > already. There is some discussion on the design doc. I prefer to land this then > modify the comment if we decide on something different. Acknowledged. https://codereview.chromium.org/2811813002/diff/220001/chrome/browser/android... File chrome/browser/android/offline_pages/suggested_articles_observer.cc (right): https://codereview.chromium.org/2811813002/diff/220001/chrome/browser/android... chrome/browser/android/offline_pages/suggested_articles_observer.cc:77: base::WrapUnique<base::SupportsUserData::Data>(suggestions_observer)); nit: this looks worse than what it was before (especially the raw pointer to heap memory above). I'm sorry about that suggestion. Feel free to revert if you wish.
The CQ bit was checked by dewittj@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: This issue passed the CQ dry run.
lgtm https://codereview.chromium.org/2811813002/diff/240001/chrome/browser/android... File chrome/browser/android/offline_pages/suggested_articles_observer.cc (right): https://codereview.chromium.org/2811813002/diff/240001/chrome/browser/android... chrome/browser/android/offline_pages/suggested_articles_observer.cc:85: category_( nit: there is not need to save category now. Just define and use a constant. https://codereview.chromium.org/2811813002/diff/240001/chrome/browser/android... File chrome/browser/android/offline_pages/suggested_articles_observer.h (right): https://codereview.chromium.org/2811813002/diff/240001/chrome/browser/android... chrome/browser/android/offline_pages/suggested_articles_observer.h:26: // single category (initially ARTICLES). When those suggestions arrive, it nit: now we can say :listening for new suggestions of ARTICLES.
Patchset #11 (id:260001) has been deleted
The CQ bit was checked by dewittj@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_cronet on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...) cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Patchset #11 (id:280001) has been deleted
The CQ bit was checked by dewittj@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...
dewittj@chromium.org changed reviewers: + jochen@chromium.org
+jochen: //components/offline_pages/content/DEPS, //components/BUILD.gn PTAL, thanks. https://codereview.chromium.org/2811813002/diff/240001/chrome/browser/android... File chrome/browser/android/offline_pages/suggested_articles_observer.cc (right): https://codereview.chromium.org/2811813002/diff/240001/chrome/browser/android... chrome/browser/android/offline_pages/suggested_articles_observer.cc:85: category_( On 2017/04/18 22:19:51, jianli wrote: > nit: there is not need to save category now. Just define and use a constant. I don't want a static initializer, so how do you like the solution I have now? https://codereview.chromium.org/2811813002/diff/240001/chrome/browser/android... File chrome/browser/android/offline_pages/suggested_articles_observer.h (right): https://codereview.chromium.org/2811813002/diff/240001/chrome/browser/android... chrome/browser/android/offline_pages/suggested_articles_observer.h:26: // single category (initially ARTICLES). When those suggestions arrive, it On 2017/04/18 22:19:52, jianli wrote: > nit: now we can say :listening for new suggestions of ARTICLES. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
https://codereview.chromium.org/2811813002/diff/300001/components/offline_pag... File components/offline_pages/content/DEPS (right): https://codereview.chromium.org/2811813002/diff/300001/components/offline_pag... components/offline_pages/content/DEPS:4: "+content/public", please restrict this to content/public/browser
lgtm % comments below https://codereview.chromium.org/2811813002/diff/300001/components/offline_pag... File components/offline_pages/content/suggested_articles_observer.cc (right): https://codereview.chromium.org/2811813002/diff/300001/components/offline_pag... components/offline_pages/content/suggested_articles_observer.cc:94: if (category != ArticlesCategory() || This special-casing to KnownCategories::ARTICLES is unfortunate. We run and plan to run various server-side experiments with providing other remote categories (with different IDs). There are conceptually articles to be prefetched, just not ARTICLES. Please add a TODO to change this to check whether a given category is not a _remote_ category. This involves changes in category.h, so lets leave it for a follow-up CL. https://codereview.chromium.org/2811813002/diff/300001/components/offline_pag... components/offline_pages/content/suggested_articles_observer.cc:110: PrefetchService* service = delegate_->GetPrefetchService(browser_context_); Shouldn't you check for nullptr? Does the factory always create the service? What about incognito profile? In incognito profile there is also no content suggestions service. I am just wondering whether the guarantees are not too subtle. https://codereview.chromium.org/2811813002/diff/300001/components/offline_pag... components/offline_pages/content/suggested_articles_observer.cc:126: PrefetchService* service = delegate_->GetPrefetchService(browser_context_); ditto
The CQ bit was checked by dewittj@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 for the reviews! jochen: PTAL at DEPS, thanks!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
The CQ bit was checked by dewittj@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_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by dewittj@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: This issue passed the CQ dry run.
https://codereview.chromium.org/2811813002/diff/220001/chrome/browser/android... File chrome/browser/android/offline_pages/suggested_articles_observer.cc (right): https://codereview.chromium.org/2811813002/diff/220001/chrome/browser/android... chrome/browser/android/offline_pages/suggested_articles_observer.cc:77: base::WrapUnique<base::SupportsUserData::Data>(suggestions_observer)); On 2017/04/18 20:17:23, carlosk wrote: > nit: this looks worse than what it was before (especially the raw pointer to > heap memory above). I'm sorry about that suggestion. Feel free to revert if you > wish. Done. https://codereview.chromium.org/2811813002/diff/300001/components/offline_pag... File components/offline_pages/content/DEPS (right): https://codereview.chromium.org/2811813002/diff/300001/components/offline_pag... components/offline_pages/content/DEPS:4: "+content/public", On 2017/04/21 08:40:10, jochen wrote: > please restrict this to content/public/browser Done. https://codereview.chromium.org/2811813002/diff/300001/components/offline_pag... File components/offline_pages/content/suggested_articles_observer.cc (right): https://codereview.chromium.org/2811813002/diff/300001/components/offline_pag... components/offline_pages/content/suggested_articles_observer.cc:94: if (category != ArticlesCategory() || On 2017/04/21 13:57:06, jkrcal wrote: > This special-casing to KnownCategories::ARTICLES is unfortunate. We run and plan > to run various server-side experiments with providing other remote categories > (with different IDs). There are conceptually articles to be prefetched, just not > ARTICLES. > > Please add a TODO to change this to check whether a given category is not a > _remote_ category. This involves changes in category.h, so lets leave it for a > follow-up CL. Done. https://codereview.chromium.org/2811813002/diff/300001/components/offline_pag... components/offline_pages/content/suggested_articles_observer.cc:110: PrefetchService* service = delegate_->GetPrefetchService(browser_context_); On 2017/04/21 13:57:07, jkrcal wrote: > Shouldn't you check for nullptr? > Does the factory always create the service? What about incognito profile? In > incognito profile there is also no content suggestions service. I am just > wondering whether the guarantees are not too subtle. Done. https://codereview.chromium.org/2811813002/diff/300001/components/offline_pag... components/offline_pages/content/suggested_articles_observer.cc:126: PrefetchService* service = delegate_->GetPrefetchService(browser_context_); On 2017/04/21 13:57:07, jkrcal wrote: > ditto Done.
lgtm
The CQ bit was checked by dewittj@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from carlosk@chromium.org, jianli@chromium.org, jkrcal@chromium.org Link to the patchset: https://codereview.chromium.org/2811813002/#ps360001 (title: "fix compile >:-(")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by dewittj@chromium.org
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": 360001, "attempt_start_ts": 1493062146433590, "parent_rev": "2f0527c9fcfdb3b53628acec1b3195563f71ec51", "commit_rev": "094047d6fd5b831902dbb9a35416634024895e95"}
Message was sent while issue was closed.
Description was changed from ========== [Offline Pages] Set up the initial prefetching service. This service does nothing by itself, but does listen to suggestions from the ContentSuggestionsService and forwards suggestions that are not in the offline page model to the empty prefetching service. All of this is gated by the Offline Pages Prefetching about:flags entry. BUG=701939 ========== to ========== [Offline Pages] Set up the initial prefetching service. This service does nothing by itself, but does listen to suggestions from the ContentSuggestionsService and forwards suggestions that are not in the offline page model to the empty prefetching service. All of this is gated by the Offline Pages Prefetching about:flags entry. BUG=701939 Review-Url: https://codereview.chromium.org/2811813002 Cr-Commit-Position: refs/heads/master@{#466747} Committed: https://chromium.googlesource.com/chromium/src/+/094047d6fd5b831902dbb9a35416... ==========
Message was sent while issue was closed.
Committed patchset #14 (id:360001) as https://chromium.googlesource.com/chromium/src/+/094047d6fd5b831902dbb9a35416...
Message was sent while issue was closed.
Findit(https://goo.gl/kROfz5) identified this CL at revision 466747 as the culprit for failures in the build cycles as shown on: https://findit-for-me.appspot.com/waterfall/culprit?key=ag9zfmZpbmRpdC1mb3Itb... |