[Offline pages] Completely remove offliner factory.

[Offline pages] Completely remove offliner factory.

BUG=

Review-Url: https://codereview.chromium.org/2609713002
Cr-Commit-Position: refs/heads/master@{#442639}
Committed: https://chromium.googlesource.com/chromium/src/+/1f59c91a97dad5a9061d4b755544ade9f3af58a0

[chili, 2017-01-01 00:59:42]

chili@chromium.org changed reviewers:
+ dimich@chromium.org, dougarnett@chromium.org, petewil@chromium.org,
romax@chromium.org

[chili, 2017-01-01 00:59:43]

Per discussion regarding the offliner factories, this is a CL that removes all
of it.

Note that due to the dependency of Offliner on OfflinePageModel, this means that
we're always going to have an OPM sitting around when there is a RC.

https://codereview.chromium.org/2609713002/diff/1/components/offline_pages/co...
File components/offline_pages/core/background/request_coordinator.cc (right):

https://codereview.chromium.org/2609713002/diff/1/components/offline_pages/co...
components/offline_pages/core/background/request_coordinator.cc:777: DVLOG(0) <<
"Offliner crashed. Cannot background offline page.";
Maybe we should have an UMA here to more concretely record the instances where
prerenderer simply crashes?  Per a comment from Pete in the past, the reason we
had factories before was to account for possible case when the prerendering
offliner simply crashed due to prerenderer crashing

[dougarnett, 2017-01-03 17:04:07]

I have some vague recollection that the deferred construction was important -
unfortunately I don't recall why and whether still applicable, so end-to-end
testing of construction scenarios (including background scheduled construction)
would be good on this change if it goes forward.

One reason I expected to need to construct/reconstruct a PrerenderingOffliner
was to support incognito mode loads (need to wrap PrerenderManager with
incognito profile) - which do not yet do and hopefully will not need to with
this offliner impl.

The deferred construction could have memory footprint benefits which may be
something to consider wrt to fix/simplify the factories rather than kill them
for this refactor.

[Pete Williamson, 2017-01-03 20:48:15]

lgtm

Like DougArnett, I also have a vague recollection that we needed it, but I'm not
100% sure why.  I think it was in case the prerenderer crashed since we knew
that it could crash its renderer, but there might also have been another reason
(maybe satisfy some build order dependencies, or something like that)

https://codereview.chromium.org/2609713002/diff/1/components/offline_pages/co...
File components/offline_pages/core/background/request_coordinator.cc (right):

https://codereview.chromium.org/2609713002/diff/1/components/offline_pages/co...
components/offline_pages/core/background/request_coordinator.cc:777: DVLOG(0) <<
"Offliner crashed. Cannot background offline page.";
On 2017/01/01 00:59:43, chili wrote:
> Maybe we should have an UMA here to more concretely record the instances where
> prerenderer simply crashes?  Per a comment from Pete in the past, the reason
we
> had factories before was to account for possible case when the prerendering
> offliner simply crashed due to prerenderer crashing

Good idea, can you add a TODO to the code?

[chili, 2017-01-04 23:14:27]

The CQ bit was checked by chili@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-04 23:15:05]

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-01-04 23:23:40]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-04 23:23:41]

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...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[chili, 2017-01-05 21:05:39]

The CQ bit was checked by chili@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-05 21:06:12]

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-01-06 02:30:44]

https://codereview.chromium.org/2609713002/diff/1/components/offline_pages/co...
File components/offline_pages/core/background/request_coordinator.h (right):

https://codereview.chromium.org/2609713002/diff/1/components/offline_pages/co...
components/offline_pages/core/background/request_coordinator.h:72:
std::unique_ptr<Offliner> factory,
nit: factory -> offliner

https://codereview.chromium.org/2609713002/diff/40001/chrome/browser/android/...
File chrome/browser/android/offline_pages/request_coordinator_factory.cc
(right):

https://codereview.chromium.org/2609713002/diff/40001/chrome/browser/android/...
chrome/browser/android/offline_pages/request_coordinator_factory.cc:62:
offliner.reset(new BackgroundLoaderOffliner(context, policy.get(), model));
Neither of both offliners ever use 'policy' constructor parameter. Would be nice
not to pass a raw pointer like this at all.

[dougarnett, 2017-01-06 22:27:42]

not lgtm

Before landing, this CL needs to be able to successfully
background load a pending request via GcmNetworkManager
starting up chrome.

I tried an apk from chili and haven't yet see this work.

This can be a subtle point to notice so I don't think we
should land unless we confirm it works.

Steps:
  - open offline internals page
  - enter url to background load and confirm it loads
  - enter another url to background load and then kill chrome via swiping it
away from recent apps
  - verify pending notification
  - wait for it to complete without manually restarting chrome
    - to try to expedite, try turning airplane mode on then off and then making
phone idle (check back in 30 mins).

[dougarnett, 2017-01-06 23:28:39]

lgtm

Turns out I needed to get Play Services updated to work with more recent chrome.
It does work.

[Dmitry Titov, 2017-01-06 23:29:04]

On 2017/01/03 17:04:07, dougarnett wrote:
> I have some vague recollection that the deferred construction was important -
> unfortunately I don't recall why and whether still applicable, so end-to-end
> testing of construction scenarios (including background scheduled
construction)
> would be good on this change if it goes forward.
> 
> One reason I expected to need to construct/reconstruct a PrerenderingOffliner
> was to support incognito mode loads (need to wrap PrerenderManager with
> incognito profile) - which do not yet do and hopefully will not need to with
> this offliner impl.
> 
> The deferred construction could have memory footprint benefits which may be
> something to consider wrt to fix/simplify the factories rather than kill them
> for this refactor.

I wonder how much benefit the deferred construction of OPM could have. We
currently create OPM as soon as the first tab is shown. Also, we create it when
an instance of PrerenderingOffliner is created. It probably covers most
scenarios of Chrome startup. Is there specific cases when we have RC but no tabs
and no Offliner?

[Dmitry Titov, 2017-01-07 00:11:22]

On 2017/01/06 23:29:04, Dmitry Titov wrote:
> On 2017/01/03 17:04:07, dougarnett wrote:
> > I have some vague recollection that the deferred construction was important
-
> > unfortunately I don't recall why and whether still applicable, so end-to-end
> > testing of construction scenarios (including background scheduled
> construction)
> > would be good on this change if it goes forward.
> > 
> > One reason I expected to need to construct/reconstruct a
PrerenderingOffliner
> > was to support incognito mode loads (need to wrap PrerenderManager with
> > incognito profile) - which do not yet do and hopefully will not need to with
> > this offliner impl.
> > 
> > The deferred construction could have memory footprint benefits which may be
> > something to consider wrt to fix/simplify the factories rather than kill
them
> > for this refactor.
> 
> I wonder how much benefit the deferred construction of OPM could have. We
> currently create OPM as soon as the first tab is shown. Also, we create it
when
> an instance of PrerenderingOffliner is created. It probably covers most
> scenarios of Chrome startup. Is there specific cases when we have RC but no
tabs
> and no Offliner?

^^^ that was a Q for Doug :) sorry for not being clear.

[chromium-reviews, 2017-01-07 00:19:53]

Not sure of much benefit then. For true background loading, we might not
need to instantiate OPM until/unless first background load
successfully hits the RMD signal and so we need to archive.

We definitely have backup scheduled events and perhaps other cases where we
would instantiate RC, check if any requests are
applicable and if not, then we don't need OPM nor Offliner so nice to save
the instantiation resources for that type of case (true background).

On Fri, Jan 6, 2017 at 4:11 PM <dimich@chromium.org> wrote:

> On 2017/01/06 23:29:04, Dmitry Titov wrote:
> > On 2017/01/03 17:04:07, dougarnett wrote:
> > > I have some vague recollection that the deferred construction was
> important
> -
> > > unfortunately I don't recall why and whether still applicable, so
> end-to-end
> > > testing of construction scenarios (including background scheduled
> > construction)
> > > would be good on this change if it goes forward.
> > >
> > > One reason I expected to need to construct/reconstruct a
> PrerenderingOffliner
> > > was to support incognito mode loads (need to wrap PrerenderManager with
> > > incognito profile) - which do not yet do and hopefully will not need
> to with
> > > this offliner impl.
> > >
> > > The deferred construction could have memory footprint benefits which
> may be
> > > something to consider wrt to fix/simplify the factories rather than
> kill
> them
> > > for this refactor.
> >
> > I wonder how much benefit the deferred construction of OPM could have. We
> > currently create OPM as soon as the first tab is shown. Also, we create
> it
> when
> > an instance of PrerenderingOffliner is created. It probably covers most
> > scenarios of Chrome startup. Is there specific cases when we have RC but
> no
> tabs
> > and no Offliner?
>
> ^^^ that was a Q for Doug :) sorry for not being clear.
>
> https://codereview.chromium.org/2609713002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Dmitry Titov, 2017-01-07 01:03:54]

On 2017/01/07 00:19:53, chromium-reviews wrote:
> Not sure of much benefit then. For true background loading, we might not
> need to instantiate OPM until/unless first background load
> successfully hits the RMD signal and so we need to archive.
> 
> We definitely have backup scheduled events and perhaps other cases where we
> would instantiate RC, check if any requests are
> applicable and if not, then we don't need OPM nor Offliner so nice to save
> the instantiation resources for that type of case (true background).
> 
> On Fri, Jan 6, 2017 at 4:11 PM <mailto:dimich@chromium.org> wrote:
> 
> > On 2017/01/06 23:29:04, Dmitry Titov wrote:
> > > On 2017/01/03 17:04:07, dougarnett wrote:
> > > > I have some vague recollection that the deferred construction was
> > important
> > -
> > > > unfortunately I don't recall why and whether still applicable, so
> > end-to-end
> > > > testing of construction scenarios (including background scheduled
> > > construction)
> > > > would be good on this change if it goes forward.
> > > >
> > > > One reason I expected to need to construct/reconstruct a
> > PrerenderingOffliner
> > > > was to support incognito mode loads (need to wrap PrerenderManager with
> > > > incognito profile) - which do not yet do and hopefully will not need
> > to with
> > > > this offliner impl.
> > > >
> > > > The deferred construction could have memory footprint benefits which
> > may be
> > > > something to consider wrt to fix/simplify the factories rather than
> > kill
> > them
> > > > for this refactor.
> > >
> > > I wonder how much benefit the deferred construction of OPM could have. We
> > > currently create OPM as soon as the first tab is shown. Also, we create
> > it
> > when
> > > an instance of PrerenderingOffliner is created. It probably covers most
> > > scenarios of Chrome startup. Is there specific cases when we have RC but
> > no
> > tabs
> > > and no Offliner?
> >
> > ^^^ that was a Q for Doug :) sorry for not being clear.
> >
> > https://codereview.chromium.org/2609713002/
> >
> 
> -- 
> You received this message because you are subscribed to the Google Groups
> "Chromium-reviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

Cool, so it looks sufficient to mitigate the resource consumption by deferring
DB loading in OPM... Otherwise creation of the C++ instance should be ok.

lgtm, but please address comments in #11 before landing.

[chili, 2017-01-09 17:18:02]

The CQ bit was checked by chili@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-09 17:18:28]

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-01-09 17:26:26]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-09 17:26:27]

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...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[chili, 2017-01-09 18:05:10]

The CQ bit was checked by chili@chromium.org to run a CQ dry run

[chili, 2017-01-09 18:05:30]

Patchset #4 (id:60001) has been deleted

[commit-bot: I haz the power, 2017-01-09 18:05:35]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[chili, 2017-01-09 18:08:20]

Thanks for taking a look!

And thanks Doug for helping me run a manual test :)

https://codereview.chromium.org/2609713002/diff/1/components/offline_pages/co...
File components/offline_pages/core/background/request_coordinator.h (right):

https://codereview.chromium.org/2609713002/diff/1/components/offline_pages/co...
components/offline_pages/core/background/request_coordinator.h:72:
std::unique_ptr<Offliner> factory,
On 2017/01/06 02:30:44, Dmitry Titov wrote:
> nit: factory -> offliner

Done.

https://codereview.chromium.org/2609713002/diff/40001/chrome/browser/android/...
File chrome/browser/android/offline_pages/request_coordinator_factory.cc
(right):

https://codereview.chromium.org/2609713002/diff/40001/chrome/browser/android/...
chrome/browser/android/offline_pages/request_coordinator_factory.cc:62:
offliner.reset(new BackgroundLoaderOffliner(context, policy.get(), model));
On 2017/01/06 02:30:44, Dmitry Titov wrote:
> Neither of both offliners ever use 'policy' constructor parameter. Would be
nice
> not to pass a raw pointer like this at all.

Will do in a separate CL.  Want to keep this CL related to the offliner
factories

[Dmitry Titov, 2017-01-09 18:25:55]

> > Neither of both offliners ever use 'policy' constructor parameter. Would be
> nice
> > not to pass a raw pointer like this at all.
> 
> Will do in a separate CL.  Want to keep this CL related to the offliner
> factories

ok. still lgtm.

[commit-bot: I haz the power, 2017-01-09 19:31:10]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-09 19:31:10]

Dry run: This issue passed the CQ dry run.

[chili, 2017-01-09 20:23:22]

The CQ bit was checked by chili@chromium.org

[chili, 2017-01-09 20:23:22]

The patchset sent to the CQ was uploaded after l-g-t-m from
petewil@chromium.org, dougarnett@chromium.org
Link to the patchset: https://codereview.chromium.org/2609713002/#ps80001
(title: "rename")

[commit-bot: I haz the power, 2017-01-09 20:23:55]

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-01-09 20:41:15]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-09 20:41:17]

Failed to apply patch for chrome/browser/BUILD.gn:
While running git apply --index -p1;
  error: patch failed: chrome/browser/BUILD.gn:3126
  error: chrome/browser/BUILD.gn: patch does not apply

Patch:       chrome/browser/BUILD.gn
Index: chrome/browser/BUILD.gn
diff --git a/chrome/browser/BUILD.gn b/chrome/browser/BUILD.gn
index
30102a31bdf1c65112d8eda9ae4ee443728f9e08..880e0b3539d71aaad8ebebec051e307667ea03fb
100644
--- a/chrome/browser/BUILD.gn
+++ b/chrome/browser/BUILD.gn
@@ -3126,8 +3126,6 @@ split_static_library("browser") {
       "android/ntp/ntp_snippets_launcher.h",
       "android/offline_pages/background_loader_offliner.cc",
       "android/offline_pages/background_loader_offliner.h",
-      "android/offline_pages/background_loader_offliner_factory.cc",
-      "android/offline_pages/background_loader_offliner_factory.h",
       "android/offline_pages/background_scheduler_bridge.cc",
       "android/offline_pages/background_scheduler_bridge.h",
       "android/offline_pages/downloads/offline_page_download_bridge.cc",
@@ -3163,8 +3161,6 @@ split_static_library("browser") {
       "android/offline_pages/prerendering_loader.h",
       "android/offline_pages/prerendering_offliner.cc",
       "android/offline_pages/prerendering_offliner.h",
-      "android/offline_pages/prerendering_offliner_factory.cc",
-      "android/offline_pages/prerendering_offliner_factory.h",
       "android/offline_pages/recent_tab_helper.cc",
       "android/offline_pages/recent_tab_helper.h",
       "android/offline_pages/request_coordinator_factory.cc",

[chili, 2017-01-10 00:04:43]

The CQ bit was checked by chili@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-10 00:05: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-01-10 03:58:08]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-10 03:58:10]

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

[chili, 2017-01-10 18:00:31]

The CQ bit was checked by chili@chromium.org

[chili, 2017-01-10 18:00:32]

The patchset sent to the CQ was uploaded after l-g-t-m from
dougarnett@chromium.org, petewil@chromium.org, dimich@chromium.org
Link to the patchset: https://codereview.chromium.org/2609713002/#ps100001
(title: "rebase update")

[commit-bot: I haz the power, 2017-01-10 18:00:52]

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-01-10 18:49:36]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1484071231705170,
"parent_rev": "0a91e531b7f77bc2111e46c59f5e0d03afa1ffca", "commit_rev":
"1f59c91a97dad5a9061d4b755544ade9f3af58a0"}

[commit-bot: I haz the power, 2017-01-10 18:50:06]

Description was changed from

==========
[Offline pages] Completely remove offliner factory.

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

to

==========
[Offline pages] Completely remove offliner factory.

BUG=

Review-Url: https://codereview.chromium.org/2609713002
Cr-Commit-Position: refs/heads/master@{#442639}
Committed:
https://chromium.googlesource.com/chromium/src/+/1f59c91a97dad5a9061d4b755544...
==========

[commit-bot: I haz the power, 2017-01-10 18:50:07]

Committed patchset #5 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/1f59c91a97dad5a9061d4b755544...