Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(133)

Issue 2811813002: [Offline Pages] Set up the initial prefetching service. (Closed)

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 >:-( #

Unified diffs Side-by-side diffs Delta from patch set Stats (+763 lines, -0 lines) Patch
M chrome/browser/BUILD.gn View 1 2 3 4 5 6 7 8 9 10 1 chunk +2 lines, -0 lines 0 comments Download
M chrome/browser/ntp_snippets/content_suggestions_service_factory.cc View 1 2 3 4 5 6 7 8 9 10 2 chunks +4 lines, -0 lines 0 comments Download
M components/BUILD.gn View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +1 line, -0 lines 0 comments Download
M components/ntp_snippets/content_suggestions_service.h View 1 2 3 4 5 6 2 chunks +2 lines, -0 lines 0 comments Download
A components/offline_pages/content/BUILD.gn View 1 2 3 4 5 6 7 8 9 10 1 chunk +44 lines, -0 lines 0 comments Download
A components/offline_pages/content/DEPS View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +6 lines, -0 lines 0 comments Download
A components/offline_pages/content/prefetch_service_factory.h View 1 2 3 4 5 6 7 8 9 10 1 chunk +43 lines, -0 lines 0 comments Download
A components/offline_pages/content/prefetch_service_factory.cc View 1 2 3 4 5 6 7 8 9 10 1 chunk +36 lines, -0 lines 0 comments Download
A components/offline_pages/content/suggested_articles_observer.h View 1 2 3 4 5 6 7 8 9 10 1 chunk +80 lines, -0 lines 0 comments Download
A components/offline_pages/content/suggested_articles_observer.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 1 chunk +170 lines, -0 lines 0 comments Download
A components/offline_pages/content/suggested_articles_observer_unittest.cc View 1 2 3 4 5 6 7 8 9 10 1 chunk +194 lines, -0 lines 0 comments Download
M components/offline_pages/core/BUILD.gn View 1 2 3 1 chunk +1 line, -0 lines 0 comments Download
M components/offline_pages/core/client_namespace_constants.h View 1 2 3 1 chunk +1 line, -0 lines 0 comments Download
M components/offline_pages/core/client_namespace_constants.cc View 1 2 3 1 chunk +1 line, -0 lines 0 comments Download
M components/offline_pages/core/client_policy_controller.cc View 1 2 3 1 chunk +8 lines, -0 lines 0 comments Download
A components/offline_pages/core/prefetch/BUILD.gn View 1 2 3 4 5 6 1 chunk +36 lines, -0 lines 0 comments Download
A components/offline_pages/core/prefetch/prefetch_service.h View 1 2 3 4 5 6 7 8 1 chunk +50 lines, -0 lines 0 comments Download
A components/offline_pages/core/prefetch/prefetch_service_impl.h View 1 2 3 4 5 6 7 8 1 chunk +35 lines, -0 lines 0 comments Download
A components/offline_pages/core/prefetch/prefetch_service_impl.cc View 1 2 3 4 5 6 7 8 1 chunk +28 lines, -0 lines 0 comments Download
A components/offline_pages/core/prefetch/prefetch_service_impl_unittest.cc View 1 2 3 4 5 6 7 8 1 chunk +21 lines, -0 lines 0 comments Download

Messages

Total messages: 88 (59 generated)
dewittj
dimich, carlosk: PTAL, this sets up the initial infrastructure for prefetching, and accepts suggestions from ...
3 years, 8 months ago (2017-04-10 17:48:52 UTC) #12
dewittj
+jianli
3 years, 8 months ago (2017-04-10 22:29:54 UTC) #16
Dmitry Titov
Nice! https://codereview.chromium.org/2811813002/diff/20001/chrome/browser/android/offline_pages/offline_page_suggestions_observer.cc File chrome/browser/android/offline_pages/offline_page_suggestions_observer.cc (right): https://codereview.chromium.org/2811813002/diff/20001/chrome/browser/android/offline_pages/offline_page_suggestions_observer.cc#newcode157 chrome/browser/android/offline_pages/offline_page_suggestions_observer.cc:157: for (const auto& page : pages) { That ...
3 years, 8 months ago (2017-04-11 01:17:14 UTC) #17
carlosk
Thanks for starting this. https://codereview.chromium.org/2811813002/diff/20001/chrome/browser/android/offline_pages/offline_page_suggestions_observer.cc File chrome/browser/android/offline_pages/offline_page_suggestions_observer.cc (right): https://codereview.chromium.org/2811813002/diff/20001/chrome/browser/android/offline_pages/offline_page_suggestions_observer.cc#newcode19 chrome/browser/android/offline_pages/offline_page_suggestions_observer.cc:19: int kOfflinePageSuggestionsObserverUserDataKey; Where is this ...
3 years, 8 months ago (2017-04-11 17:31:36 UTC) #18
dewittj
PTAL, thanks! https://codereview.chromium.org/2811813002/diff/20001/chrome/browser/android/offline_pages/offline_page_suggestions_observer.cc File chrome/browser/android/offline_pages/offline_page_suggestions_observer.cc (right): https://codereview.chromium.org/2811813002/diff/20001/chrome/browser/android/offline_pages/offline_page_suggestions_observer.cc#newcode19 chrome/browser/android/offline_pages/offline_page_suggestions_observer.cc:19: int kOfflinePageSuggestionsObserverUserDataKey; On 2017/04/11 17:31:35, carlosk wrote: ...
3 years, 8 months ago (2017-04-11 21:25:49 UTC) #20
Dmitry Titov
https://codereview.chromium.org/2811813002/diff/100001/chrome/browser/android/offline_pages/offline_page_suggestions_observer.cc File chrome/browser/android/offline_pages/offline_page_suggestions_observer.cc (right): https://codereview.chromium.org/2811813002/diff/100001/chrome/browser/android/offline_pages/offline_page_suggestions_observer.cc#newcode114 chrome/browser/android/offline_pages/offline_page_suggestions_observer.cc:114: service->RemoveAllUnprocessedURLsToPrefetch(); Do you mean to have a TODO here ...
3 years, 8 months ago (2017-04-11 21:50:47 UTC) #21
jianli
https://codereview.chromium.org/2811813002/diff/100001/chrome/browser/android/offline_pages/offline_page_suggestions_observer.cc File chrome/browser/android/offline_pages/offline_page_suggestions_observer.cc (right): https://codereview.chromium.org/2811813002/diff/100001/chrome/browser/android/offline_pages/offline_page_suggestions_observer.cc#newcode74 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/offline_pages/offline_page_suggestions_observer.cc#newcode79 ...
3 years, 8 months ago (2017-04-11 22:39:55 UTC) #22
dewittj
https://codereview.chromium.org/2811813002/diff/100001/chrome/browser/android/offline_pages/offline_page_suggestions_observer.cc File chrome/browser/android/offline_pages/offline_page_suggestions_observer.cc (right): https://codereview.chromium.org/2811813002/diff/100001/chrome/browser/android/offline_pages/offline_page_suggestions_observer.cc#newcode74 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 ...
3 years, 8 months ago (2017-04-12 00:22:49 UTC) #25
dewittj
+ jkrcal - NTP integration, including ContentSuggestionsService and SuggestionsObserver Please let me know if I'm ...
3 years, 8 months ago (2017-04-12 18:07:50 UTC) #29
dewittj
friendly ping :)
3 years, 8 months ago (2017-04-13 22:32:12 UTC) #31
dewittj
-dimich to CC since he's OOO
3 years, 8 months ago (2017-04-17 16:14:08 UTC) #37
jianli
https://codereview.chromium.org/2811813002/diff/180001/chrome/browser/android/offline_pages/prefetch_service_factory.h File chrome/browser/android/offline_pages/prefetch_service_factory.h (right): https://codereview.chromium.org/2811813002/diff/180001/chrome/browser/android/offline_pages/prefetch_service_factory.h#newcode20 chrome/browser/android/offline_pages/prefetch_service_factory.h:20: // A factory to create one unique PrefetchService. Prefetching ...
3 years, 8 months ago (2017-04-17 21:52:58 UTC) #38
dewittj
https://codereview.chromium.org/2811813002/diff/180001/chrome/browser/android/offline_pages/prefetch_service_factory.h File chrome/browser/android/offline_pages/prefetch_service_factory.h (right): https://codereview.chromium.org/2811813002/diff/180001/chrome/browser/android/offline_pages/prefetch_service_factory.h#newcode20 chrome/browser/android/offline_pages/prefetch_service_factory.h:20: // A factory to create one unique PrefetchService. Prefetching ...
3 years, 8 months ago (2017-04-17 22:22:18 UTC) #39
carlosk
https://codereview.chromium.org/2811813002/diff/180001/chrome/browser/android/offline_pages/suggestions_observer.cc File chrome/browser/android/offline_pages/suggestions_observer.cc (right): https://codereview.chromium.org/2811813002/diff/180001/chrome/browser/android/offline_pages/suggestions_observer.cc#newcode23 chrome/browser/android/offline_pages/suggestions_observer.cc:23: int kOfflinePageSuggestionsObserverUserDataKey; Shouldn't this be inside the anonymous namespace ...
3 years, 8 months ago (2017-04-18 00:36:09 UTC) #40
jianli
https://codereview.chromium.org/2811813002/diff/200001/chrome/browser/android/offline_pages/suggestions_observer.cc File chrome/browser/android/offline_pages/suggestions_observer.cc (right): https://codereview.chromium.org/2811813002/diff/200001/chrome/browser/android/offline_pages/suggestions_observer.cc#newcode102 chrome/browser/android/offline_pages/suggestions_observer.cc:102: for (auto& suggestion : suggestions) { nit: const auto& ...
3 years, 8 months ago (2017-04-18 00:41:00 UTC) #41
dewittj
Thanks! PTAL https://codereview.chromium.org/2811813002/diff/180001/chrome/browser/android/offline_pages/suggestions_observer.cc File chrome/browser/android/offline_pages/suggestions_observer.cc (right): https://codereview.chromium.org/2811813002/diff/180001/chrome/browser/android/offline_pages/suggestions_observer.cc#newcode23 chrome/browser/android/offline_pages/suggestions_observer.cc:23: int kOfflinePageSuggestionsObserverUserDataKey; On 2017/04/18 00:36:09, carlosk wrote: ...
3 years, 8 months ago (2017-04-18 18:20:26 UTC) #42
carlosk
LGTM with one nit. https://codereview.chromium.org/2811813002/diff/180001/chrome/browser/android/offline_pages/suggestions_observer.cc File chrome/browser/android/offline_pages/suggestions_observer.cc (right): https://codereview.chromium.org/2811813002/diff/180001/chrome/browser/android/offline_pages/suggestions_observer.cc#newcode25 chrome/browser/android/offline_pages/suggestions_observer.cc:25: namespace { On 2017/04/18 18:20:25, ...
3 years, 8 months ago (2017-04-18 20:17:23 UTC) #43
jianli
lgtm https://codereview.chromium.org/2811813002/diff/240001/chrome/browser/android/offline_pages/suggested_articles_observer.cc File chrome/browser/android/offline_pages/suggested_articles_observer.cc (right): https://codereview.chromium.org/2811813002/diff/240001/chrome/browser/android/offline_pages/suggested_articles_observer.cc#newcode85 chrome/browser/android/offline_pages/suggested_articles_observer.cc:85: category_( nit: there is not need to save ...
3 years, 8 months ago (2017-04-18 22:19:52 UTC) #48
dewittj
+jochen: //components/offline_pages/content/DEPS, //components/BUILD.gn PTAL, thanks. https://codereview.chromium.org/2811813002/diff/240001/chrome/browser/android/offline_pages/suggested_articles_observer.cc File chrome/browser/android/offline_pages/suggested_articles_observer.cc (right): https://codereview.chromium.org/2811813002/diff/240001/chrome/browser/android/offline_pages/suggested_articles_observer.cc#newcode85 chrome/browser/android/offline_pages/suggested_articles_observer.cc:85: category_( On 2017/04/18 22:19:51, ...
3 years, 8 months ago (2017-04-20 18:34:18 UTC) #58
jochen (gone - plz use gerrit)
https://codereview.chromium.org/2811813002/diff/300001/components/offline_pages/content/DEPS File components/offline_pages/content/DEPS (right): https://codereview.chromium.org/2811813002/diff/300001/components/offline_pages/content/DEPS#newcode4 components/offline_pages/content/DEPS:4: "+content/public", please restrict this to content/public/browser
3 years, 8 months ago (2017-04-21 08:40:10 UTC) #61
jkrcal
lgtm % comments below https://codereview.chromium.org/2811813002/diff/300001/components/offline_pages/content/suggested_articles_observer.cc File components/offline_pages/content/suggested_articles_observer.cc (right): https://codereview.chromium.org/2811813002/diff/300001/components/offline_pages/content/suggested_articles_observer.cc#newcode94 components/offline_pages/content/suggested_articles_observer.cc:94: if (category != ArticlesCategory() || ...
3 years, 8 months ago (2017-04-21 13:57:07 UTC) #62
dewittj
Thanks for the reviews! jochen: PTAL at DEPS, thanks!
3 years, 8 months ago (2017-04-21 15:00:11 UTC) #65
dewittj
https://codereview.chromium.org/2811813002/diff/220001/chrome/browser/android/offline_pages/suggested_articles_observer.cc File chrome/browser/android/offline_pages/suggested_articles_observer.cc (right): https://codereview.chromium.org/2811813002/diff/220001/chrome/browser/android/offline_pages/suggested_articles_observer.cc#newcode77 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 ...
3 years, 8 months ago (2017-04-21 21:59:04 UTC) #76
jochen (gone - plz use gerrit)
lgtm
3 years, 8 months ago (2017-04-24 13:54:58 UTC) #77
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2811813002/360001
3 years, 8 months ago (2017-04-24 16:42:50 UTC) #80
commit-bot: I haz the power
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_android_rel_ng/builds/278365)
3 years, 8 months ago (2017-04-24 19:08:09 UTC) #82
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2811813002/360001
3 years, 8 months ago (2017-04-24 19:30:04 UTC) #84
commit-bot: I haz the power
Committed patchset #14 (id:360001) as https://chromium.googlesource.com/chromium/src/+/094047d6fd5b831902dbb9a35416634024895e95
3 years, 8 months ago (2017-04-24 20:37:53 UTC) #87
findit-for-me
3 years, 8 months ago (2017-04-24 22:25:44 UTC) #88
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...

Powered by Google App Engine
This is Rietveld 408576698