Split Android-specific dependency from BackgroundLoaderOffliner. Create a subfolder of c/b/offline_…

Split Android-specific dependency from BackgroundLoaderOffliner. Create a subfolder of c/b/offline_pages which will contain
Android-specific code. Update DEPS files.

BUG=722467
Review-Url: https://codereview.chromium.org/2898393002
Cr-Commit-Position: refs/heads/master@{#474687}
Committed: https://chromium.googlesource.com/chromium/src/+/91494853f713cfb11c13ba2720689e6288639e4e

[Dmitry Titov, 2017-05-24 01:37:47]

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

[commit-bot: I haz the power, 2017-05-24 01:39:31]

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-24 01:44:28]

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

[commit-bot: I haz the power, 2017-05-24 01:44:29]

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_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)

[Dmitry Titov, 2017-05-24 01:50:21]

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

[commit-bot: I haz the power, 2017-05-24 01:51:26]

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-24 03:30:02]

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

[commit-bot: I haz the power, 2017-05-24 03:30:03]

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

[Dmitry Titov, 2017-05-24 04:47:14]

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

[commit-bot: I haz the power, 2017-05-24 04:47:56]

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-24 04:58:46]

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

[commit-bot: I haz the power, 2017-05-24 04:58:56]

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-24 05:03:13]

dimich@chromium.org changed reviewers:
+ chili@chromium.org, yfriedman@chromium.org

[Dmitry Titov, 2017-05-24 05:03:13]

Next step of refactoring to split platform-independent code and Android-specific
one into separate folders, with their own DEPS. More steps coming, eventually
moving all file out from chrome/browser/android/offline_pages into
chrome/browser/offline_pages or (just a few) to
chrome/browser/offline_pages/android


PTAL: 
chili@ : BackgroundLoaderOffliner, offline_pages
yfriedman@: DEPS mentioning base/android

[Dmitry Titov, 2017-05-24 05:04:52]

dimich@chromium.org changed reviewers:
+ dewittj@chromium.org

[Dmitry Titov, 2017-05-24 05:04:53]

+dewittj as OWNER of offline_pages

[commit-bot: I haz the power, 2017-05-24 06:48:02]

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

[commit-bot: I haz the power, 2017-05-24 06:48:02]

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

[chili, 2017-05-24 06:59:40]

offliner parts lgtm

[Dmitry Titov, 2017-05-24 08:14:15]

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

[commit-bot: I haz the power, 2017-05-24 08:14:41]

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-24 09:45:15]

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

[commit-bot: I haz the power, 2017-05-24 09:45:16]

Dry run: This issue passed the CQ dry run.

[dewittj, 2017-05-24 23:24:40]

lgtm

https://codereview.chromium.org/2898393002/diff/2/chrome/browser/android/offl...
File
chrome/browser/android/offline_pages/evaluation/offline_page_evaluation_bridge.cc
(right):

https://codereview.chromium.org/2898393002/diff/2/chrome/browser/android/offl...
chrome/browser/android/offline_pages/evaluation/offline_page_evaluation_bridge.cc:178:
nullptr));  // no need to connect LoadTerminatorListener for harness.
nit: LoadTerminationListener

https://codereview.chromium.org/2898393002/diff/2/chrome/browser/android/offl...
File chrome/browser/android/offline_pages/prerendering_offliner.cc (right):

https://codereview.chromium.org/2898393002/diff/2/chrome/browser/android/offl...
chrome/browser/android/offline_pages/prerendering_offliner.cc:282: // This is
not implemented since this offliner is deprecated and
Is this different behavior than we have today?

https://codereview.chromium.org/2898393002/diff/2/chrome/browser/android/offl...
File chrome/browser/android/offline_pages/request_coordinator_factory.cc
(right):

https://codereview.chromium.org/2898393002/diff/2/chrome/browser/android/offl...
chrome/browser/android/offline_pages/request_coordinator_factory.cc:20: #include
"chrome/browser/offline_pages/android/load_termination_listener_impl.h"
I assume that this file will eventually want to move out from c/b/a. Should we
start by adding #if defined(OS_ANDROID) for this?

https://codereview.chromium.org/2898393002/diff/2/chrome/browser/offline_page...
File chrome/browser/offline_pages/android/DEPS (right):

https://codereview.chromium.org/2898393002/diff/2/chrome/browser/offline_page...
chrome/browser/offline_pages/android/DEPS:5: # platform-inependent code and
Android-specific, and will go away.
independent

https://codereview.chromium.org/2898393002/diff/2/chrome/browser/offline_page...
File chrome/browser/offline_pages/android/load_termination_listener_impl.cc
(right):

https://codereview.chromium.org/2898393002/diff/2/chrome/browser/offline_page...
chrome/browser/offline_pages/android/load_termination_listener_impl.cc:18:
weak_ptr_factory_.GetWeakPtr()));
q: technically could this be base::Unretained(this) since we own the
app_listener_?

[Yaron, 2017-05-25 02:16:22]

base/android reference lgtm

[Dmitry Titov, 2017-05-25 15:45:05]

The CQ bit was checked by dimich@chromium.org

[Dmitry Titov, 2017-05-25 15:45:05]

The patchset sent to the CQ was uploaded after l-g-t-m from chili@chromium.org
Link to the patchset: https://codereview.chromium.org/2898393002/#ps2 (title:
"more build fixes")

[commit-bot: I haz the power, 2017-05-25 15:45:27]

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 17:36:08]

CQ is committing da patch.

Bot data: {"patchset_id": 2, "attempt_start_ts": 1495727105176990, "parent_rev":
"ce8204596559423fad5dae55acc6a3005e8e183e", "commit_rev":
"91494853f713cfb11c13ba2720689e6288639e4e"}

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

Description was changed from

==========
Split Android-specific dependency from BackgroundLoaderOffliner. Create a
subfolder of c/b/offline_pages which will contain
Android-specific code. Update DEPS files.

BUG=722467
==========

to

==========
Split Android-specific dependency from BackgroundLoaderOffliner. Create a
subfolder of c/b/offline_pages which will contain
Android-specific code. Update DEPS files.

BUG=722467

Review-Url: https://codereview.chromium.org/2898393002
Cr-Commit-Position: refs/heads/master@{#474687}
Committed:
https://chromium.googlesource.com/chromium/src/+/91494853f713cfb11c13ba272068...
==========

[commit-bot: I haz the power, 2017-05-25 17:36:24]

Committed patchset #6 (id:2) as
https://chromium.googlesource.com/chromium/src/+/91494853f713cfb11c13ba272068...

[Dmitry Titov, 2017-05-25 23:02:09]

oops sorry didn't notice Justin's suggestions while hitting Checkin button on
the phone :(
Preparing a follow-up CL for that!

[Dmitry Titov, 2017-05-25 23:24:13]

Justin, I've sent you a follow-up CL:
https://chromium-review.googlesource.com/c/516782/

Also some replies:

https://codereview.chromium.org/2898393002/diff/2/chrome/browser/android/offl...
File
chrome/browser/android/offline_pages/evaluation/offline_page_evaluation_bridge.cc
(right):

https://codereview.chromium.org/2898393002/diff/2/chrome/browser/android/offl...
chrome/browser/android/offline_pages/evaluation/offline_page_evaluation_bridge.cc:178:
nullptr));  // no need to connect LoadTerminatorListener for harness.
On 2017/05/24 23:24:39, dewittj wrote:
> nit: LoadTerminationListener

Done in followup CL

https://codereview.chromium.org/2898393002/diff/2/chrome/browser/android/offl...
File chrome/browser/android/offline_pages/prerendering_offliner.cc (right):

https://codereview.chromium.org/2898393002/diff/2/chrome/browser/android/offl...
chrome/browser/android/offline_pages/prerendering_offliner.cc:282: // This is
not implemented since this offliner is deprecated and
On 2017/05/24 23:24:40, dewittj wrote:
> Is this different behavior than we have today?

No, it directly uses Android class. I hope we will remove this file before it'll
be necessary to refactor it as well.

https://codereview.chromium.org/2898393002/diff/2/chrome/browser/android/offl...
File chrome/browser/android/offline_pages/request_coordinator_factory.cc
(right):

https://codereview.chromium.org/2898393002/diff/2/chrome/browser/android/offl...
chrome/browser/android/offline_pages/request_coordinator_factory.cc:20: #include
"chrome/browser/offline_pages/android/load_termination_listener_impl.h"
On 2017/05/24 23:24:40, dewittj wrote:
> I assume that this file will eventually want to move out from c/b/a. Should we
> start by adding #if defined(OS_ANDROID) for this?

My thought was that factories, as main dependency injection points, would be
living in leaf-most folders and directly inject specific dependencies, without
ifdefs. So we have a factory impl per platform, and include the right one in
BUILD.gn depending which platform we build.

So this specific one will eventually go to c/b/o_p/android.

https://codereview.chromium.org/2898393002/diff/2/chrome/browser/offline_page...
File chrome/browser/offline_pages/android/DEPS (right):

https://codereview.chromium.org/2898393002/diff/2/chrome/browser/offline_page...
chrome/browser/offline_pages/android/DEPS:5: # platform-inependent code and
Android-specific, and will go away.
On 2017/05/24 23:24:40, dewittj wrote:
> independent

Done in followup CL

https://codereview.chromium.org/2898393002/diff/2/chrome/browser/offline_page...
File chrome/browser/offline_pages/android/load_termination_listener_impl.cc
(right):

https://codereview.chromium.org/2898393002/diff/2/chrome/browser/offline_page...
chrome/browser/offline_pages/android/load_termination_listener_impl.cc:18:
weak_ptr_factory_.GetWeakPtr()));
On 2017/05/24 23:24:40, dewittj wrote:
> q: technically could this be base::Unretained(this) since we own the
> app_listener_?

It indeed would be, in this specific case. I'm hesitant to switch to Unretained
though because it feels as more load on casual reader to verify that lifetimes
are correct. In this case, it's a non-local lookup into the code... Let me know
if you think there is a reason to switch!