[OfflinePages] Hook the background loader offliner to the request coordinator based on finch flag s…

[OfflinePages] Hook the background loader offliner to the request coordinator based on finch flag settings.

This CL assumes that the background loader offliner can be re-used. Later changes will provide fix to this situation.

BUG=665242
Committed: https://crrev.com/7223bbf9c512c4061d8b56e4d5c103ae10e8bcfd
Cr-Commit-Position: refs/heads/master@{#440180}

[chili, 2016-12-15 23:58:56]

Patchset #2 (id:20001) has been deleted

[chili, 2016-12-16 18:30:04]

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

[chili, 2016-12-16 18:30:04]

This is the final portion to connect the offliner to flags and the RC.

Thanks!

[dougarnett, 2016-12-16 20:07:09]

lgtm

[Pete Williamson, 2016-12-17 01:26:26]

lgtm.  It looks good, but I would like to see a test for the factory (it can be
in a later patch)

https://codereview.chromium.org/2544533003/diff/60001/chrome/browser/android/...
File chrome/browser/android/offline_pages/background_loader_offliner_factory.cc
(right):

https://codereview.chromium.org/2544533003/diff/60001/chrome/browser/android/...
chrome/browser/android/offline_pages/background_loader_offliner_factory.cc:24:
Offliner* BackgroundLoaderOfflinerFactory::GetOffliner(
A test for this method would be good.  (It's OK to defer it to a later patch,
but I'd like to see one)

[Dmitry Titov, 2016-12-17 02:55:20]

https://codereview.chromium.org/2544533003/diff/60001/chrome/browser/android/...
File chrome/browser/android/offline_pages/background_loader_offliner_factory.h
(right):

https://codereview.chromium.org/2544533003/diff/60001/chrome/browser/android/...
chrome/browser/android/offline_pages/background_loader_offliner_factory.h:30:
BackgroundLoaderOffliner* offliner_;
Please take a look at my comment on Pete's patch that introduced the factories:
https://codereview.chromium.org/1929223002/

As is, those factories are not factories but rather wrappers around an offline
pointer with unclear lifetime responsibilities. Before we go this path, I
suggest we rethink the design a bit here.

[Dmitry Titov, 2016-12-19 19:17:06]

Per offline discussion, lets proceed and immediately follow with another CL that
would remove factories and just create a single instance of an offliner and
passes it into a new RequestCoordinator.

lgtm with that plan.

[dougarnett, 2016-12-19 19:42:15]

On 2016/12/19 19:17:06, Dmitry Titov wrote:
> Per offline discussion, lets proceed and immediately follow with another CL
that
> would remove factories and just create a single instance of an offliner and
> passes it into a new RequestCoordinator.
> 
> lgtm with that plan.

Warning - there may be some creation order issues with passing instance of
Offliner into newly constructed RequestCoordinator

[chili, 2016-12-21 18:00:33]

The CQ bit was checked by chili@chromium.org

[commit-bot: I haz the power, 2016-12-21 18:01:09]

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

[chili, 2016-12-21 18:01:48]

Thanks for looking at this!

I'll be sending followup patch shortly

https://codereview.chromium.org/2544533003/diff/60001/chrome/browser/android/...
File chrome/browser/android/offline_pages/background_loader_offliner_factory.cc
(right):

https://codereview.chromium.org/2544533003/diff/60001/chrome/browser/android/...
chrome/browser/android/offline_pages/background_loader_offliner_factory.cc:24:
Offliner* BackgroundLoaderOfflinerFactory::GetOffliner(
On 2016/12/17 01:26:26, Pete Williamson wrote:
> A test for this method would be good.  (It's OK to defer it to a later patch,
> but I'd like to see one)

Per discussion, I may be removing the factory altogether in follow up patch

https://codereview.chromium.org/2544533003/diff/60001/chrome/browser/android/...
File chrome/browser/android/offline_pages/background_loader_offliner_factory.h
(right):

https://codereview.chromium.org/2544533003/diff/60001/chrome/browser/android/...
chrome/browser/android/offline_pages/background_loader_offliner_factory.h:30:
BackgroundLoaderOffliner* offliner_;
On 2016/12/17 02:55:20, Dmitry Titov wrote:
> Please take a look at my comment on Pete's patch that introduced the
factories:
> https://codereview.chromium.org/1929223002/
> 
> As is, those factories are not factories but rather wrappers around an offline
> pointer with unclear lifetime responsibilities. Before we go this path, I
> suggest we rethink the design a bit here.

Per discussion, will resolve this in followup

[commit-bot: I haz the power, 2016-12-21 19:36:14]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1482343233020000,
"parent_rev": "c3d5345d57a616c3c80c010953aa2145cf04d320", "commit_rev":
"e36c32ac5ff9b20c78942268b9c2e15377c9e1e3"}

[commit-bot: I haz the power, 2016-12-21 19:36:39]

Description was changed from

==========
[OfflinePages] Hook the background loader offliner to the request coordinator
based on finch flag settings.

This CL assumes that the background loader offliner can be re-used. Later
changes will provide fix to this situation.

BUG=665242
==========

to

==========
[OfflinePages] Hook the background loader offliner to the request coordinator
based on finch flag settings.

This CL assumes that the background loader offliner can be re-used. Later
changes will provide fix to this situation.

BUG=665242

Review-Url: https://codereview.chromium.org/2544533003
==========

[commit-bot: I haz the power, 2016-12-21 19:36:40]

Committed patchset #3 (id:60001)

[commit-bot: I haz the power, 2016-12-21 19:40:32]

Description was changed from

==========
[OfflinePages] Hook the background loader offliner to the request coordinator
based on finch flag settings.

This CL assumes that the background loader offliner can be re-used. Later
changes will provide fix to this situation.

BUG=665242

Review-Url: https://codereview.chromium.org/2544533003
==========

to

==========
[OfflinePages] Hook the background loader offliner to the request coordinator
based on finch flag settings.

This CL assumes that the background loader offliner can be re-used. Later
changes will provide fix to this situation.

BUG=665242

Committed: https://crrev.com/7223bbf9c512c4061d8b56e4d5c103ae10e8bcfd
Cr-Commit-Position: refs/heads/master@{#440180}
==========

[commit-bot: I haz the power, 2016-12-21 19:40:33]

Patchset 3 (id:??) landed as
https://crrev.com/7223bbf9c512c4061d8b56e4d5c103ae10e8bcfd
Cr-Commit-Position: refs/heads/master@{#440180}