Update WebAPKs even if the WebAPK start URL has no Web Manifest part 2/3

Update WebAPKs even if the WebAPK start URL has no Web Manifest part 2/3

The class name ManifestUpgradeDetector does not make sense in the context of
the new ManifestUpgradeDetector#Callback. The new callback can be called up to
twice:
- Once after the initial page load. (Regardless of whether the page uses the
correct Web Manifest)
- Once after a page which uses the correct Web Manifest has finished loading.
If a Web Developer removes the Web Manifest from their site, the second call is
never done.

This CL:
- moves the check for "whether the fetched Web Manifest requires a WebAPK
upgrade" into WebApkUpdateManager.
- merges the manifest fetching parts of ManifestUpgradeDetector and
ManifestUpgradeDetectorFetcher into a new class WebApkUpdateDataFetcher

BUG=639536
Committed: https://crrev.com/b1e143c78604fdd43daa19e5123da514509ee091
Cr-Commit-Position: refs/heads/master@{#437641}

[pkotwicz, 2016-10-31 03:19:56]

Description was changed from

==========
Update WebAPKs even if the WebAPK start URL has no Web Manifest part 2/3

The class name ManifestUpgradeDetector does not make sense in the context of the
new ManifestUpgradeDetector#Callback. The new callback can be called up to
twice:
- Once after the initial page load. (Regardless of whether the page uses the
correct Web Manifest)
- Once after a page which uses the correct Web Manifest has finished loading. If
a Web Developer removes the Web Manifest from their site, the second call is
never done.

This CL:
- moves the check for "whether the fetched Web Manifest requires a WebAPK
upgrade" into WebApkUpdateManager.
- merges the manifest fetching parts of ManifestUpgradeDetector and
ManifestUpgradeDetectorFetcher into a new class WebApkUpdateManagerFetcher

BUG=639536
==========

to

==========
Update WebAPKs even if the WebAPK start URL has no Web Manifest part 2/3

The class name ManifestUpgradeDetector does not make sense in the context of
the new ManifestUpgradeDetector#Callback. The new callback can be called up to
twice:
- Once after the initial page load. (Regardless of whether the page uses the
correct Web Manifest)
- Once after a page which uses the correct Web Manifest has finished loading.
If a Web Developer removes the Web Manifest from their site, the second call is
never done.

This CL:
- moves the check for "whether the fetched Web Manifest requires a WebAPK
upgrade" into WebApkUpdateManager.
- merges the manifest fetching parts of ManifestUpgradeDetector and
ManifestUpgradeDetectorFetcher into a new class WebApkUpdateManagerFetcher

BUG=639536
==========

[pkotwicz, 2016-10-31 03:57:14]

Patchset #2 (id:20001) has been deleted

[pkotwicz, 2016-10-31 03:57:21]

Patchset #1 (id:1) has been deleted

[pkotwicz, 2016-10-31 03:58:01]

pkotwicz@chromium.org changed reviewers:
+ dominickn@chromium.org

[pkotwicz, 2016-10-31 03:58:02]

Dominick, can you please take a look? This CL depends on
https://codereview.chromium.org/2459023002/

[dominickn, 2016-11-01 03:37:16]

I'll wait till we've fully sorted out crrev.com/2459023002 to do the full
review, but for now.

https://codereview.chromium.org/2460253002/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkUpdateManagerFetcher.java
(right):

https://codereview.chromium.org/2460253002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkUpdateManagerFetcher.java:19:
public class WebApkUpdateManagerFetcher extends EmptyTabObserver {
Since we're renaming things in this class, can this be called
WebApkManifestFetcher? The name "ManagerFetcher" has never made that much sense
to me. Another alternative is the WebApkUpdateDataFetcher, but all this class
does is fetch a manifest.

[pkotwicz, 2016-12-07 23:08:06]

Patchset #3 (id:80001) has been deleted

[pkotwicz, 2016-12-07 23:08:14]

Patchset #2 (id:60001) has been deleted

[pkotwicz, 2016-12-08 01:15:56]

Patchset #2 (id:100001) has been deleted

[pkotwicz, 2016-12-08 01:52:19]

Patchset #2 (id:120001) has been deleted

[pkotwicz, 2016-12-08 01:53:07]

Dominick, can you please take another look? I have rebased this CL. I have also
renamed WebApkUpdateManagerFetcher to WebApkUpdateDataFetcher

[pkotwicz, 2016-12-08 01:53:30]

Description was changed from

==========
Update WebAPKs even if the WebAPK start URL has no Web Manifest part 2/3

The class name ManifestUpgradeDetector does not make sense in the context of
the new ManifestUpgradeDetector#Callback. The new callback can be called up to
twice:
- Once after the initial page load. (Regardless of whether the page uses the
correct Web Manifest)
- Once after a page which uses the correct Web Manifest has finished loading.
If a Web Developer removes the Web Manifest from their site, the second call is
never done.

This CL:
- moves the check for "whether the fetched Web Manifest requires a WebAPK
upgrade" into WebApkUpdateManager.
- merges the manifest fetching parts of ManifestUpgradeDetector and
ManifestUpgradeDetectorFetcher into a new class WebApkUpdateManagerFetcher

BUG=639536
==========

to

==========
Update WebAPKs even if the WebAPK start URL has no Web Manifest part 2/3

The class name ManifestUpgradeDetector does not make sense in the context of
the new ManifestUpgradeDetector#Callback. The new callback can be called up to
twice:
- Once after the initial page load. (Regardless of whether the page uses the
correct Web Manifest)
- Once after a page which uses the correct Web Manifest has finished loading.
If a Web Developer removes the Web Manifest from their site, the second call is
never done.

This CL:
- moves the check for "whether the fetched Web Manifest requires a WebAPK
upgrade" into WebApkUpdateManager.
- merges the manifest fetching parts of ManifestUpgradeDetector and
ManifestUpgradeDetectorFetcher into a new class WebApkUpdateDataFetcher

BUG=639536
==========

[pkotwicz, 2016-12-08 02:01:34]

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

[commit-bot: I haz the power, 2016-12-08 02:02:26]

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

[dominickn, 2016-12-08 05:41:49]

Looks pretty good. Just a couple of clarifying questions.

https://codereview.chromium.org/2460253002/diff/140001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkUpdateDataFetcher.java
(right):

https://codereview.chromium.org/2460253002/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkUpdateDataFetcher.java:69:
mTab.addObserver(this);
Out of an abundance of caution, should this observer be added after the native
side is initialized?

https://codereview.chromium.org/2460253002/diff/140001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkUpdateManager.java
(right):

https://codereview.chromium.org/2460253002/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkUpdateManager.java:208:
if (CommandLine.getInstance().hasSwitch(
Is it OK for this check to move above the unknown sources check? Or will the
Play store stuff replace the unknown sources?

[pkotwicz, 2016-12-08 15:13:39]

Dominick, can you please take another look?

https://codereview.chromium.org/2460253002/diff/140001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkUpdateDataFetcher.java
(right):

https://codereview.chromium.org/2460253002/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkUpdateDataFetcher.java:69:
mTab.addObserver(this);
Things are funky when as an observer/callback can be called synchronously after
calling a function. Thankfully Tab#addObserver() does not call the observer
synchronously.

https://codereview.chromium.org/2460253002/diff/140001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkUpdateManager.java
(right):

https://codereview.chromium.org/2460253002/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkUpdateManager.java:208:
if (CommandLine.getInstance().hasSwitch(
--check-for-web-manifest-update-on-startup is a test only command line flag. Yes
it is ok to change the precedence of the command line flag. The reason for this
change is to enable testing WebApkUpdateManager via instrumentation tests. I
don't think that an instrumentation test can toggle whether "installation from
unknown sources" is allowed

[dominickn, 2016-12-09 01:58:43]

lgtm. Thanks for clarifying.

[pkotwicz, 2016-12-09 15:50:06]

The CQ bit was checked by pkotwicz@chromium.org

[commit-bot: I haz the power, 2016-12-09 15:50:24]

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, 2016-12-09 15:59:19]

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

[commit-bot: I haz the power, 2016-12-09 15:59:20]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[pkotwicz, 2016-12-09 16:23:38]

pkotwicz@chromium.org changed reviewers:
+ dfalcantara@chromium.org

[pkotwicz, 2016-12-09 16:23:39]

dfalcantara@ for changes to chrome_jni_registrar.cc

[gone, 2016-12-09 19:24:16]

OWNERs lgtm

[pkotwicz, 2016-12-09 19:46:54]

The CQ bit was checked by pkotwicz@chromium.org

[commit-bot: I haz the power, 2016-12-09 19:48:15]

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, 2016-12-09 20:52:51]

CQ is committing da patch.

Bot data: {"patchset_id": 160001, "attempt_start_ts": 1481312814588830,
"parent_rev": "6096412b620320bc8776c7e0bfd38c6030f51a8a", "commit_rev":
"d0da6b4a767acd1e6c663d970c1c1edeeb156546"}

[commit-bot: I haz the power, 2016-12-09 20:53:17]

Description was changed from

==========
Update WebAPKs even if the WebAPK start URL has no Web Manifest part 2/3

The class name ManifestUpgradeDetector does not make sense in the context of
the new ManifestUpgradeDetector#Callback. The new callback can be called up to
twice:
- Once after the initial page load. (Regardless of whether the page uses the
correct Web Manifest)
- Once after a page which uses the correct Web Manifest has finished loading.
If a Web Developer removes the Web Manifest from their site, the second call is
never done.

This CL:
- moves the check for "whether the fetched Web Manifest requires a WebAPK
upgrade" into WebApkUpdateManager.
- merges the manifest fetching parts of ManifestUpgradeDetector and
ManifestUpgradeDetectorFetcher into a new class WebApkUpdateDataFetcher

BUG=639536
==========

to

==========
Update WebAPKs even if the WebAPK start URL has no Web Manifest part 2/3

The class name ManifestUpgradeDetector does not make sense in the context of
the new ManifestUpgradeDetector#Callback. The new callback can be called up to
twice:
- Once after the initial page load. (Regardless of whether the page uses the
correct Web Manifest)
- Once after a page which uses the correct Web Manifest has finished loading.
If a Web Developer removes the Web Manifest from their site, the second call is
never done.

This CL:
- moves the check for "whether the fetched Web Manifest requires a WebAPK
upgrade" into WebApkUpdateManager.
- merges the manifest fetching parts of ManifestUpgradeDetector and
ManifestUpgradeDetectorFetcher into a new class WebApkUpdateDataFetcher

BUG=639536

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

[commit-bot: I haz the power, 2016-12-09 20:53:18]

Committed patchset #3 (id:160001)

[commit-bot: I haz the power, 2016-12-12 14:40:24]

Description was changed from

==========
Update WebAPKs even if the WebAPK start URL has no Web Manifest part 2/3

The class name ManifestUpgradeDetector does not make sense in the context of
the new ManifestUpgradeDetector#Callback. The new callback can be called up to
twice:
- Once after the initial page load. (Regardless of whether the page uses the
correct Web Manifest)
- Once after a page which uses the correct Web Manifest has finished loading.
If a Web Developer removes the Web Manifest from their site, the second call is
never done.

This CL:
- moves the check for "whether the fetched Web Manifest requires a WebAPK
upgrade" into WebApkUpdateManager.
- merges the manifest fetching parts of ManifestUpgradeDetector and
ManifestUpgradeDetectorFetcher into a new class WebApkUpdateDataFetcher

BUG=639536

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

to

==========
Update WebAPKs even if the WebAPK start URL has no Web Manifest part 2/3

The class name ManifestUpgradeDetector does not make sense in the context of
the new ManifestUpgradeDetector#Callback. The new callback can be called up to
twice:
- Once after the initial page load. (Regardless of whether the page uses the
correct Web Manifest)
- Once after a page which uses the correct Web Manifest has finished loading.
If a Web Developer removes the Web Manifest from their site, the second call is
never done.

This CL:
- moves the check for "whether the fetched Web Manifest requires a WebAPK
upgrade" into WebApkUpdateManager.
- merges the manifest fetching parts of ManifestUpgradeDetector and
ManifestUpgradeDetectorFetcher into a new class WebApkUpdateDataFetcher

BUG=639536

Committed: https://crrev.com/b1e143c78604fdd43daa19e5123da514509ee091
Cr-Commit-Position: refs/heads/master@{#437641}
==========

[commit-bot: I haz the power, 2016-12-12 14:40:25]

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