[Offline Pages] Add a GCMAppHandler for offline page prefetch.

[Offline Pages] Add a GCMAppHandler for offline page prefetch.

This does not yet react to such messages but hooks up the plumbing so that we are ready for them

Because the app handlers need to be registered before any GCM messages are received, this app handler is added upon construction of the GCMProfileService.

BUG=701939
Review-Url: https://codereview.chromium.org/2864293003
Cr-Commit-Position: refs/heads/master@{#475735}
Committed: https://chromium.googlesource.com/chromium/src/+/d564808ec725e2eb36bef109d17fe641885e0ad0

[dewittj, 2017-05-08 20:59:28]

The CQ bit was checked by [email protected] to run a CQ dry run

[commit-bot: I haz the power, 2017-05-08 21:00:43]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dewittj, 2017-05-08 23:00:37]

[email protected] changed reviewers:
+ [email protected]

[dewittj, 2017-05-08 23:00:38]

+dimich

This is not yet complete, but wanted to get your sense of whether
PrefetchService inheriting from GCMAppHandler was a bad idea.  In meeting we
discussed maybe a specific object should do this.  Do you prefer a subclass such
as PrefetchService::GCMHandler or a separate class with a generic delegate?

[commit-bot: I haz the power, 2017-05-09 00:21:47]

The CQ bit was unchecked by [email protected]

[commit-bot: I haz the power, 2017-05-09 00:21:47]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[Dmitry Titov, 2017-05-09 00:22:25]

https://codereview.chromium.org/2864293003/diff/1/chrome/browser/android/offl...
File chrome/browser/android/offline_pages/prefetch/prefetch_service_factory.cc
(right):

https://codereview.chromium.org/2864293003/diff/1/chrome/browser/android/offl...
chrome/browser/android/offline_pages/prefetch/prefetch_service_factory.cc:26:
DependsOn(gcm::GCMProfileServiceFactory::GetInstance());
I wonder if this is the other way around - technically speaking the GCM needs
the client threfore it depends on it...  I may be mistaken here.

btw, if you reverse dependency, you naturally don't need
ServiceIsCreatedWithBrowserContext anywhere.

[Dmitry Titov, 2017-05-09 00:23:50]

Also, I do think having a separate object would be good, and I'd expose a getter
for the instance on the PrefetchDervice, so the hookup of it with the service
was encapsulated in the service impl.

[dewittj, 2017-05-09 16:59:46]

Description was changed from

==========
[Offline Pages] Add a GCMAppHandler for offline page prefetch.

BUG=
==========

to

==========
[Offline Pages] Add a GCMAppHandler for offline page prefetch.

This does not yet react to such messages but hooks up the plumbing so that we
are ready for them

Because the app handlers need to be registered before any GCM messages are
received, this app handler is added upon construction of the GCMProfileService.

BUG=701939
==========

[dewittj, 2017-05-09 16:59:46]

[email protected] changed reviewers:
+ [email protected]

[dewittj, 2017-05-09 16:59:54]

The CQ bit was checked by [email protected] to run a CQ dry run

[commit-bot: I haz the power, 2017-05-09 17:00:30]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dewittj, 2017-05-09 17:07:54]

dimich: PTAL, does this look better?
fgorski: Would you be okay with the dependency as written or do you prefer the
strategy in PatchSet?

[Dmitry Titov, 2017-05-11 03:35:20]

Almost there!

1. I'm going to move the prefetch_service_factory to c/b/o_p/prefetch,
preliminary patch (2875853003) is there. 
2. I think we can use the traditional schema:
- declare interface in comp/o_p/prefetch, separate file.
- derive a real gcmHandlerImpl somewhere in c/b/.. 
- create a real gcmHandlerImpl in PrefetchServiceFactory which is itself in
c/b..., pass it to PrefetchService so it keeps it in unique_ptr and provides raw
pointer to anybody
- pull that raw pointer in GCMProfileServiceFactory and stuff it into
GCMDriver::AddHandler()

This way, we can always create a PrefetchService with fake Impls, and test it.
Also, we can keep most impl in comp/o_p. WDYT?

https://codereview.chromium.org/2864293003/diff/40001/chrome/browser/gcm/gcm_...
File chrome/browser/gcm/gcm_profile_service_factory.cc (right):

https://codereview.chromium.org/2864293003/diff/40001/chrome/browser/gcm/gcm_...
chrome/browser/gcm/gcm_profile_service_factory.cc:78: service =
base::WrapUnique(
Could be just base::MakeUnique<type>(params);

https://codereview.chromium.org/2864293003/diff/40001/components/offline_page...
File components/offline_pages/core/prefetch/prefetch_service_impl.cc (right):

https://codereview.chromium.org/2864293003/diff/40001/components/offline_page...
components/offline_pages/core/prefetch/prefetch_service_impl.cc:70:
driver->AddAppHandler(kPrefetchingOfflinePagesAppId, &app_handler_);
If you do this in the GCMProfileServiceFactory, you would avoid making
offline_pages DEPS for GCMDriver.

[dewittj, 2017-05-15 23:35:39]

The CQ bit was checked by [email protected] to run a CQ dry run

[commit-bot: I haz the power, 2017-05-15 23:36:25]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dewittj, 2017-05-16 00:00:54]

The CQ bit was checked by [email protected] to run a CQ dry run

[commit-bot: I haz the power, 2017-05-16 00:01:44]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dewittj, 2017-05-16 00:03:35]

The CQ bit was checked by [email protected] to run a CQ dry run

[commit-bot: I haz the power, 2017-05-16 00:04:47]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dewittj, 2017-05-16 00:08:44]

The CQ bit was checked by [email protected] to run a CQ dry run

[commit-bot: I haz the power, 2017-05-16 00:10:17]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-16 01:54:19]

The CQ bit was unchecked by [email protected]

[commit-bot: I haz the power, 2017-05-16 01:54:20]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[dewittj, 2017-05-16 17:23:02]

The CQ bit was checked by [email protected] to run a CQ dry run

[commit-bot: I haz the power, 2017-05-16 17:23:43]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dewittj, 2017-05-16 17:52:46]

The CQ bit was checked by [email protected] to run a CQ dry run

[commit-bot: I haz the power, 2017-05-16 17:53:54]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dewittj, 2017-05-16 17:56:44]

PTAL, rewrote this to have teh following properties:
* We assume that PrefetchService is very lightweight to create
* We depend on GCMProfileService, and create PrefetchService at startup, to
register the app handler
* The app handler is owned by PrefetchService.

Thanks!

https://codereview.chromium.org/2864293003/diff/120001/chrome/test/BUILD.gn
File chrome/test/BUILD.gn (right):

https://codereview.chromium.org/2864293003/diff/120001/chrome/test/BUILD.gn#n...
chrome/test/BUILD.gn:3432:
"../browser/offline_pages/prefetch/suggested_articles_observer_unittest.cc",
note: I put this in |is_android| because tests have not been ported to use
|enable_offline_pages|

[Dmitry Titov, 2017-05-16 19:33:23]

https://codereview.chromium.org/2864293003/diff/120001/chrome/browser/offline...
File chrome/browser/offline_pages/prefetch/prefetch_service_factory.cc (right):

https://codereview.chromium.org/2864293003/diff/120001/chrome/browser/offline...
chrome/browser/offline_pages/prefetch/prefetch_service_factory.cc:41: if
(!offline_pages::IsPrefetchingOfflinePagesEnabled())
It feels this check belongs to delayed initialization rather than to initial
lightweight creation of main objects. 

Also, I'm relying on PS being available unconditionally for my next CL of stats
accumulation (OfflineDaysAccumulator lives on PS).

I think all the objects injected here should be fine to be created regardless of
the flag. The actual work might not happening, but at least we don't need null
checks everywhere.

https://codereview.chromium.org/2864293003/diff/120001/chrome/browser/offline...
chrome/browser/offline_pages/prefetch/prefetch_service_factory.cc:53: bool
PrefetchServiceFactory::ServiceIsCreatedWithBrowserContext() const {
Could you add a comment as to lightweight creation/delayed init?

https://codereview.chromium.org/2864293003/diff/120001/components/offline_pag...
File components/offline_pages/core/prefetch/prefetch_service_impl.h (right):

https://codereview.chromium.org/2864293003/diff/120001/components/offline_pag...
components/offline_pages/core/prefetch/prefetch_service_impl.h:20: class
GCMAppHandler {
Does it deserve a separate file? Most of the subobjects of PS (Zine, Dispatcher,
etc) will have a separate declaration, perhaps it's better to just use one way
of creating those subobjects.

[fgorski, 2017-05-16 20:03:02]

https://codereview.chromium.org/2864293003/diff/120001/chrome/browser/offline...
File chrome/browser/offline_pages/prefetch/prefetch_gcm_app_handler.h (right):

https://codereview.chromium.org/2864293003/diff/120001/chrome/browser/offline...
chrome/browser/offline_pages/prefetch/prefetch_gcm_app_handler.h:8: #include
<vector>
#include <string> instead is more appropriate I think

https://codereview.chromium.org/2864293003/diff/120001/components/offline_pag...
File components/offline_pages/core/prefetch/prefetch_service_impl.h (right):

https://codereview.chromium.org/2864293003/diff/120001/components/offline_pag...
components/offline_pages/core/prefetch/prefetch_service_impl.h:8: #include
<vector>
#include <memory>

https://codereview.chromium.org/2864293003/diff/120001/components/offline_pag...
components/offline_pages/core/prefetch/prefetch_service_impl.h:20: class
GCMAppHandler {
If this is only about life cycle, why not user data?

If this is going to be extended to provide interface to the functionality I am
OK with that.

https://codereview.chromium.org/2864293003/diff/120001/components/offline_pag...
components/offline_pages/core/prefetch/prefetch_service_impl.h:25:
PrefetchServiceImpl(std::unique_ptr<GCMAppHandler> gcm_app_handler);
explicit in case you stick with the interface

[commit-bot: I haz the power, 2017-05-16 22:22:11]

The CQ bit was unchecked by [email protected]

[commit-bot: I haz the power, 2017-05-16 22:22:12]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[dewittj, 2017-05-16 22:57:34]

The CQ bit was checked by [email protected] to run a CQ dry run

[dewittj, 2017-05-16 22:58:22]

ptal, thanks!

https://codereview.chromium.org/2864293003/diff/120001/chrome/browser/offline...
File chrome/browser/offline_pages/prefetch/prefetch_gcm_app_handler.h (right):

https://codereview.chromium.org/2864293003/diff/120001/chrome/browser/offline...
chrome/browser/offline_pages/prefetch/prefetch_gcm_app_handler.h:8: #include
<vector>
On 2017/05/16 20:03:01, fgorski wrote:
> #include <string> instead is more appropriate I think

Done.

https://codereview.chromium.org/2864293003/diff/120001/chrome/browser/offline...
File chrome/browser/offline_pages/prefetch/prefetch_service_factory.cc (right):

https://codereview.chromium.org/2864293003/diff/120001/chrome/browser/offline...
chrome/browser/offline_pages/prefetch/prefetch_service_factory.cc:41: if
(!offline_pages::IsPrefetchingOfflinePagesEnabled())
On 2017/05/16 19:33:23, Dmitry Titov wrote:
> It feels this check belongs to delayed initialization rather than to initial
> lightweight creation of main objects. 
> 
> Also, I'm relying on PS being available unconditionally for my next CL of
stats
> accumulation (OfflineDaysAccumulator lives on PS).
> 
> I think all the objects injected here should be fine to be created regardless
of
> the flag. The actual work might not happening, but at least we don't need null
> checks everywhere.

Done.

https://codereview.chromium.org/2864293003/diff/120001/chrome/browser/offline...
chrome/browser/offline_pages/prefetch/prefetch_service_factory.cc:53: bool
PrefetchServiceFactory::ServiceIsCreatedWithBrowserContext() const {
On 2017/05/16 19:33:23, Dmitry Titov wrote:
> Could you add a comment as to lightweight creation/delayed init?

Done.

https://codereview.chromium.org/2864293003/diff/120001/components/offline_pag...
File components/offline_pages/core/prefetch/prefetch_service_impl.h (right):

https://codereview.chromium.org/2864293003/diff/120001/components/offline_pag...
components/offline_pages/core/prefetch/prefetch_service_impl.h:20: class
GCMAppHandler {
On 2017/05/16 20:03:02, fgorski wrote:
> If this is only about life cycle, why not user data?
> 
> If this is going to be extended to provide interface to the functionality I am
> OK with that.

Added TODO and left interface here.

https://codereview.chromium.org/2864293003/diff/120001/components/offline_pag...
components/offline_pages/core/prefetch/prefetch_service_impl.h:25:
PrefetchServiceImpl(std::unique_ptr<GCMAppHandler> gcm_app_handler);
On 2017/05/16 20:03:01, fgorski wrote:
> explicit in case you stick with the interface

Done.

[commit-bot: I haz the power, 2017-05-16 22:58:36]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-16 23:06:11]

The CQ bit was unchecked by [email protected]

[commit-bot: I haz the power, 2017-05-16 23:06:11]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
  android_cronet on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...)
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)

[dewittj, 2017-05-17 16:21:49]

The CQ bit was checked by [email protected] to run a CQ dry run

[commit-bot: I haz the power, 2017-05-17 16:22:46]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-17 17:34:42]

The CQ bit was unchecked by [email protected]

[commit-bot: I haz the power, 2017-05-17 17:34:43]

Dry run: 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...)

[dewittj, 2017-05-17 17:36:07]

The CQ bit was checked by [email protected] to run a CQ dry run

[commit-bot: I haz the power, 2017-05-17 17:37:15]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Dmitry Titov, 2017-05-17 18:21:09]

lgtm upon moving gcm handler and zine listener to components/ since they don't
necessarily need to be in c/b

[dewittj, 2017-05-17 20:19:02]

The CQ bit was checked by [email protected] to run a CQ dry run

[commit-bot: I haz the power, 2017-05-17 20:20:53]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-17 20:42:12]

The CQ bit was unchecked by [email protected]

[commit-bot: I haz the power, 2017-05-17 20:42:14]

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...)

[dewittj, 2017-05-17 21:39:29]

The CQ bit was checked by [email protected] to run a CQ dry run

[commit-bot: I haz the power, 2017-05-17 21:40:55]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dewittj, 2017-05-17 21:43:19]

PTAL again,

When I moved the suggested_articles_observer there were some substantial
changes:
* It hangs off the PrefetchService now
* It does not have a delegate anymore, I changed the way to inject suggestions
* I rearranged the imports in ContentSuggestionsService to use the new BUILDFLAG

Thanks!

[fgorski, 2017-05-17 23:15:02]

lgtm

https://codereview.chromium.org/2864293003/diff/220001/components/offline_pag...
File components/offline_pages/core/prefetch/prefetch_service_impl.h (right):

https://codereview.chromium.org/2864293003/diff/220001/components/offline_pag...
components/offline_pages/core/prefetch/prefetch_service_impl.h:23: // lifetime
purposes. TODO(dewittj): Add an interface method for getting an
This comment is duplicated below, please remove one.

[Dmitry Titov, 2017-05-18 01:13:41]

still lgtm

[dewittj, 2017-05-18 06:15:46]

[email protected] changed reviewers:
+ [email protected]

[dewittj, 2017-05-18 06:23:08]

[email protected] changed reviewers:
+ [email protected]

[dewittj, 2017-05-18 06:23:09]

+jochen - TBR but at your leisure please look at components/BUILD.gn, moving
files around removed a target.
+bauerb - Please look at ntp_snippets and
chrome_browser_main_extra_parts_profiles

The main changes in ntp_snippets are around the ownership model of the
prefetching ContentSuggestionsObserver, it is now owned by PrefetchService,
which is a lightweight lifetime management object for prefetching.  It won't do
anything significant in initialization.

[Bernhard Bauer, 2017-05-18 19:11:31]

LGTM

https://codereview.chromium.org/2864293003/diff/220001/chrome/browser/ntp_sni...
File chrome/browser/ntp_snippets/content_suggestions_service_factory.cc (right):

https://codereview.chromium.org/2864293003/diff/220001/chrome/browser/ntp_sni...
chrome/browser/ntp_snippets/content_suggestions_service_factory.cc:78: #if
BUILDFLAG(ENABLE_OFFLINE_PAGES)
Nit: If you're touching this code anyway, I would do all includes first (first
unconditional, then conditional), followed by all using declarations, instead of
grouped by #define.

[jochen (gone - plz use gerrit), 2017-05-18 20:30:23]

lgtm

[dewittj, 2017-05-22 21:52:44]

The CQ bit was checked by [email protected] to run a CQ dry run

[commit-bot: I haz the power, 2017-05-22 21:53:15]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dewittj, 2017-05-22 21:59:46]

The CQ bit was checked by [email protected]

[dewittj, 2017-05-22 21:59:47]

The patchset sent to the CQ was uploaded after l-g-t-m from [email protected],
[email protected], [email protected], [email protected]
Link to the patchset: https://codereview.chromium.org/2864293003/#ps260001
(title: "Rebase.")

[dewittj, 2017-05-22 22:00:08]

https://codereview.chromium.org/2864293003/diff/220001/chrome/browser/ntp_sni...
File chrome/browser/ntp_snippets/content_suggestions_service_factory.cc (right):

https://codereview.chromium.org/2864293003/diff/220001/chrome/browser/ntp_sni...
chrome/browser/ntp_snippets/content_suggestions_service_factory.cc:78: #if
BUILDFLAG(ENABLE_OFFLINE_PAGES)
On 2017/05/18 19:11:31, Bernhard Bauer wrote:
> Nit: If you're touching this code anyway, I would do all includes first (first
> unconditional, then conditional), followed by all using declarations, instead
of
> grouped by #define.

Done.

https://codereview.chromium.org/2864293003/diff/220001/components/offline_pag...
File components/offline_pages/core/prefetch/prefetch_service_impl.h (right):

https://codereview.chromium.org/2864293003/diff/220001/components/offline_pag...
components/offline_pages/core/prefetch/prefetch_service_impl.h:23: // lifetime
purposes. TODO(dewittj): Add an interface method for getting an
On 2017/05/17 23:15:02, fgorski wrote:
> This comment is duplicated below, please remove one.

Done.

[commit-bot: I haz the power, 2017-05-22 22:00:12]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-23 00:48:03]

The CQ bit was unchecked by [email protected]

[commit-bot: I haz the power, 2017-05-23 00:48:05]

Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[dewittj, 2017-05-23 17:20:01]

The CQ bit was checked by [email protected]

[commit-bot: I haz the power, 2017-05-23 17:20:45]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-23 20:06:54]

The CQ bit was unchecked by [email protected]

[commit-bot: I haz the power, 2017-05-23 20:06:56]

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...)

[dewittj, 2017-05-24 20:31:51]

The CQ bit was checked by [email protected] to run a CQ dry run

[commit-bot: I haz the power, 2017-05-24 20:32:20]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dewittj, 2017-05-24 21:09:56]

The CQ bit was checked by [email protected] to run a CQ dry run

[commit-bot: I haz the power, 2017-05-24 21:10:27]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dewittj, 2017-05-24 21:11:13]

fgorski, dimich: PTAL, I reworked the lifetime of PrefetchService as we
discussed in the office.

[commit-bot: I haz the power, 2017-05-24 21:53:32]

The CQ bit was unchecked by [email protected]

[commit-bot: I haz the power, 2017-05-24 21:53:33]

Dry run: Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[dewittj, 2017-05-24 23:43:15]

The CQ bit was checked by [email protected] to run a CQ dry run

[commit-bot: I haz the power, 2017-05-24 23:44:27]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-25 00:20:48]

The CQ bit was unchecked by [email protected]

[commit-bot: I haz the power, 2017-05-25 00:20:49]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[dewittj, 2017-05-30 15:55:09]

The CQ bit was checked by [email protected] to run a CQ dry run

[commit-bot: I haz the power, 2017-05-30 15:55:36]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-30 16:43:30]

The CQ bit was unchecked by [email protected]

[commit-bot: I haz the power, 2017-05-30 16:43:33]

Dry run: Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[dewittj, 2017-05-30 17:39:07]

The CQ bit was checked by [email protected] to run a CQ dry run

[commit-bot: I haz the power, 2017-05-30 17:39:43]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[fgorski, 2017-05-30 18:20:05]

https://codereview.chromium.org/2864293003/diff/320001/chrome/browser/offline...
File chrome/browser/offline_pages/prefetch/prefetch_service_factory.cc (right):

https://codereview.chromium.org/2864293003/diff/320001/chrome/browser/offline...
chrome/browser/offline_pages/prefetch/prefetch_service_factory.cc:9: #include
"chrome/browser/gcm/gcm_profile_service_factory.h"
are these includes needed?

https://codereview.chromium.org/2864293003/diff/320001/components/offline_pag...
File components/offline_pages/core/prefetch/prefetch_service_impl.cc (right):

https://codereview.chromium.org/2864293003/diff/320001/components/offline_pag...
components/offline_pages/core/prefetch/prefetch_service_impl.cc:20:
ntp_snippets::ContentSuggestionsService* service) {
CHECK or DCHECK to ensure we are not passing in the nullptr in production?

[commit-bot: I haz the power, 2017-05-30 19:31:41]

The CQ bit was unchecked by [email protected]

[commit-bot: I haz the power, 2017-05-30 19:31:43]

Dry run: This issue passed the CQ dry run.

[dewittj, 2017-05-30 20:58:19]

The CQ bit was checked by [email protected] to run a CQ dry run

[dewittj, 2017-05-30 20:58:30]

Patchset #4 (id:60001) has been deleted

[dewittj, 2017-05-30 20:58:55]

Patchset #4 (id:80001) has been deleted

[commit-bot: I haz the power, 2017-05-30 20:59:02]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dewittj, 2017-05-30 21:00:21]

Patchset #4 (id:100001) has been deleted

[dewittj, 2017-05-30 21:00:47]

Patchset #5 (id:140001) has been deleted

[dewittj, 2017-05-30 21:01:31]

Patchset #7 (id:200001) has been deleted

[dewittj, 2017-05-30 21:01:48]

Patchset #6 (id:180001) has been deleted

[dewittj, 2017-05-30 21:02:15]

Patchset #5 (id:160001) has been deleted

[dewittj, 2017-05-30 21:07:18]

The CQ bit was checked by [email protected] to run a CQ dry run

[commit-bot: I haz the power, 2017-05-30 21:07:49]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dewittj, 2017-05-30 21:09:19]

fgorski, dimich: Updated as discussed, ptal.

https://codereview.chromium.org/2864293003/diff/320001/chrome/browser/offline...
File chrome/browser/offline_pages/prefetch/prefetch_service_factory.cc (right):

https://codereview.chromium.org/2864293003/diff/320001/chrome/browser/offline...
chrome/browser/offline_pages/prefetch/prefetch_service_factory.cc:9: #include
"chrome/browser/gcm/gcm_profile_service_factory.h"
On 2017/05/30 18:20:05, fgorski wrote:
> are these includes needed?

Done.

https://codereview.chromium.org/2864293003/diff/320001/components/offline_pag...
File components/offline_pages/core/prefetch/prefetch_service_impl.cc (right):

https://codereview.chromium.org/2864293003/diff/320001/components/offline_pag...
components/offline_pages/core/prefetch/prefetch_service_impl.cc:20:
ntp_snippets::ContentSuggestionsService* service) {
On 2017/05/30 18:20:05, fgorski wrote:
> CHECK or DCHECK to ensure we are not passing in the nullptr in production?

Not going to change this code in this patch.

[Dmitry Titov, 2017-05-30 21:21:23]

LGTM

with understanding that we need to do a quick refactor of suggestion listener to
the same pattern as other subobjects...

[commit-bot: I haz the power, 2017-05-30 21:56:09]

The CQ bit was unchecked by [email protected]

[commit-bot: I haz the power, 2017-05-30 21:56:10]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[dewittj, 2017-05-30 22:31:59]

The CQ bit was checked by [email protected] to run a CQ dry run

[commit-bot: I haz the power, 2017-05-30 22:32:52]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dewittj, 2017-05-30 23:52:22]

The CQ bit was unchecked by [email protected]

[dewittj, 2017-05-30 23:52:28]

The CQ bit was checked by [email protected]

[dewittj, 2017-05-30 23:52:28]

The patchset sent to the CQ was uploaded after l-g-t-m from [email protected],
[email protected], [email protected], [email protected]
Link to the patchset: https://codereview.chromium.org/2864293003/#ps400001
(title: "Fix windows")

[commit-bot: I haz the power, 2017-05-30 23:52:59]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-31 01:57:37]

CQ is committing da patch.

Bot data: {"patchset_id": 400001, "attempt_start_ts": 1496188348053270,
"parent_rev": "867e554db2e9d5b2cd58d684544b81c8d170144b", "commit_rev":
"d564808ec725e2eb36bef109d17fe641885e0ad0"}

[commit-bot: I haz the power, 2017-05-31 01:57:51]

Description was changed from

==========
[Offline Pages] Add a GCMAppHandler for offline page prefetch.

This does not yet react to such messages but hooks up the plumbing so that we
are ready for them

Because the app handlers need to be registered before any GCM messages are
received, this app handler is added upon construction of the GCMProfileService.

BUG=701939
==========

to

==========
[Offline Pages] Add a GCMAppHandler for offline page prefetch.

This does not yet react to such messages but hooks up the plumbing so that we
are ready for them

Because the app handlers need to be registered before any GCM messages are
received, this app handler is added upon construction of the GCMProfileService.

BUG=701939

Review-Url: https://codereview.chromium.org/2864293003
Cr-Commit-Position: refs/heads/master@{#475735}
Committed:
https://chromium.googlesource.com/chromium/src/+/d564808ec725e2eb36bef109d17f...
==========

[commit-bot: I haz the power, 2017-05-31 01:57:53]

Committed patchset #14 (id:400001) as
https://chromium.googlesource.com/chromium/src/+/d564808ec725e2eb36bef109d17f...