Do not request upgrade if Web Manifest is no longer WebAPK-capable

Do not request upgrade if Web Manifest is no longer WebAPK-capable

For instance, an upgraded WebAPK should not be requested if the Web Manifest was
updated to no longer have a start_url

BUG=627824
TEST=ManifestUpgradeDetectorTest.testWebManifestNotWebApkCapableShouldNotUpgrade

Committed: https://crrev.com/090b6f3b15c5b11a304db2c7d61f9ff8672a8f1d
Cr-Commit-Position: refs/heads/master@{#414149}

[pkotwicz, 2016-08-19 01:00:07]

Description was changed from

==========
Do not request upgrade if Web Manifest is no longer Web-APK capable

For instance, an upgraded WebAPK should not be requested if the Web Manifest was
updated to no longer have a start_url

BUG=627824
==========

to

==========
Do not request upgrade if Web Manifest is no longer Web-APK capable

For instance, an upgraded WebAPK should not be requested if the Web Manifest was
updated to no longer have a start_url

BUG=627824
TEST=ManifestUpgradeDetectorTest.testWebManifestNotWebApkCapableShouldNotUpgrade
==========

[pkotwicz, 2016-08-19 01:00:07]

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

[pkotwicz, 2016-08-19 01:00:21]

Description was changed from

==========
Do not request upgrade if Web Manifest is no longer Web-APK capable

For instance, an upgraded WebAPK should not be requested if the Web Manifest was
updated to no longer have a start_url

BUG=627824
TEST=ManifestUpgradeDetectorTest.testWebManifestNotWebApkCapableShouldNotUpgrade
==========

to

==========
Do not request upgrade if Web Manifest is no longer WebAPK capable

For instance, an upgraded WebAPK should not be requested if the Web Manifest was
updated to no longer have a start_url

BUG=627824
TEST=ManifestUpgradeDetectorTest.testWebManifestNotWebApkCapableShouldNotUpgrade
==========

[pkotwicz, 2016-08-19 01:00:40]

Yaron, can you please take a look?

[pkotwicz, 2016-08-19 01:00:57]

Description was changed from

==========
Do not request upgrade if Web Manifest is no longer WebAPK capable

For instance, an upgraded WebAPK should not be requested if the Web Manifest was
updated to no longer have a start_url

BUG=627824
TEST=ManifestUpgradeDetectorTest.testWebManifestNotWebApkCapableShouldNotUpgrade
==========

to

==========
Do not request upgrade if Web Manifest is no longer WebAPK-capable

For instance, an upgraded WebAPK should not be requested if the Web Manifest was
updated to no longer have a start_url

BUG=627824
TEST=ManifestUpgradeDetectorTest.testWebManifestNotWebApkCapableShouldNotUpgrade
==========

[Yaron, 2016-08-19 13:57:10]

https://codereview.chromium.org/2262583002/diff/1/chrome/browser/android/weba...
File chrome/browser/android/webapk/manifest_upgrade_detector_fetcher.cc (right):

https://codereview.chromium.org/2262583002/diff/1/chrome/browser/android/weba...
chrome/browser/android/webapk/manifest_upgrade_detector_fetcher.cc:145: bool
is_installable,
if it's not installable, we're going to do a bunch of marshalling and
unmarshalling for nothing (we end up short-circuiting later).  How about instead
having a separate flow for when installation isn't possible?

[pkotwicz, 2016-08-23 16:52:19]

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

[pkotwicz, 2016-08-23 16:52:19]

Xi what do you think?

The "new web manifest being installable" case is the common case. The "new web
manifest being not installable" is an edge case. I do not mind if the "edge
case" is as slow as the "common case".

[Xi Han, 2016-08-23 19:11:52]

https://codereview.chromium.org/2262583002/diff/1/chrome/browser/android/weba...
File chrome/browser/android/webapk/manifest_upgrade_detector_fetcher.cc (right):

https://codereview.chromium.org/2262583002/diff/1/chrome/browser/android/weba...
chrome/browser/android/webapk/manifest_upgrade_detector_fetcher.cc:145: bool
is_installable,
On 2016/08/19 13:57:10, Yaron (ooo) wrote:
> if it's not installable, we're going to do a bunch of marshalling and
> unmarshalling for nothing (we end up short-circuiting later).  How about
instead
> having a separate flow for when installation isn't possible?

Yaron's suggestion makes sense, even though I didn't have this idea at the first
place. It makes code looks neat. Maybe we can have another path that just calls:
Java_ManifestUpgradeDetectorFetcher_onInstallableCheckFailed(). Hopefully it
won't add too much complex for testing.

[Xi Han, 2016-08-23 19:13:32]

You need to rebase this CL since the CL
(https://codereview.chromium.org/2184913005/) has landed.

[pkotwicz, 2016-08-23 21:06:02]

Xi can you please take another look?

Adding a code path which calls
Java_ManifestUpgradeDetectorFetcher_onInstallableCheckFailed() is a good idea. I
will do this as part of http://crbug.com/639536 I will add tests also as part of
http://crbug.com/639536

[Xi Han, 2016-08-23 21:26:02]

lgtm

[pkotwicz, 2016-08-23 21:55:41]

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

[pkotwicz, 2016-08-23 21:55:41]

Dominick for OWNERS

[dominickn, 2016-08-24 06:44:37]

lgtm

[pkotwicz, 2016-08-24 13:32:14]

The CQ bit was checked by pkotwicz@chromium.org

[commit-bot: I haz the power, 2016-08-24 13:32: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-08-24 14:47:48]

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

[commit-bot: I haz the power, 2016-08-24 14:47:49]

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

[pkotwicz, 2016-08-24 16:46:02]

Dominick, can you please take another look? I have fixed
ManifestUpgradeDetectorFetcherTest. Patch Set #2 made the test fail because the
callback is now only called if the Web Manifest is installable. For a page to be
installable it must have a registered service worker.

I am going to add tests for this new behavior as part of fixing
http://crbug.com/639536

[dominickn, 2016-08-24 16:55:17]

On 2016/08/24 16:46:02, pkotwicz wrote:
> Dominick, can you please take another look? I have fixed
> ManifestUpgradeDetectorFetcherTest. Patch Set #2 made the test fail because
the
> callback is now only called if the Web Manifest is installable. For a page to
be
> installable it must have a registered service worker.
> 
> I am going to add tests for this new behavior as part of fixing
> http://crbug.com/639536

still lgtm % question

Any reason to have the reduced manifest for this test? Seems like you could just
use the existing manifest_test_page.html in chrome/test/data/banners and not
need to create new files.

[pkotwicz, 2016-08-24 17:23:17]

https://codereview.chromium.org/2262583002/diff/40001/chrome/android/javatest...
File
chrome/android/javatests/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetectorFetcherTest.java
(right):

https://codereview.chromium.org/2262583002/diff/40001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetectorFetcherTest.java:122:
public void testLaunchWithDifferentManifestUrl() throws Exception {
I need two test pages which use different manifest URLs for this test

[pkotwicz, 2016-08-24 17:23:20]

The CQ bit was checked by pkotwicz@chromium.org

[pkotwicz, 2016-08-24 17:23:21]

The patchset sent to the CQ was uploaded after l-g-t-m from hanxi@chromium.org
Link to the patchset: https://codereview.chromium.org/2262583002/#ps40001
(title: "Merge branch 'master' into webapk_updater_non_installable")

[commit-bot: I haz the power, 2016-08-24 17:24:09]

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

[pkotwicz, 2016-08-24 17:59:26]

The CQ bit was unchecked by pkotwicz@chromium.org

[pkotwicz, 2016-08-24 18:11:44]

The CQ bit was checked by pkotwicz@chromium.org

[pkotwicz, 2016-08-24 18:11:44]

The patchset sent to the CQ was uploaded after l-g-t-m from
dominickn@chromium.org, hanxi@chromium.org
Link to the patchset: https://codereview.chromium.org/2262583002/#ps60001
(title: "Merge branch 'master' into webapk_updater_non_installable")

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

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-08-24 20:15:17]

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

[commit-bot: I haz the power, 2016-08-24 20:15:18]

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

[pkotwicz, 2016-08-24 20:29:00]

The CQ bit was checked by pkotwicz@chromium.org

[commit-bot: I haz the power, 2016-08-24 20:30:42]

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-08-24 21:24:27]

Description was changed from

==========
Do not request upgrade if Web Manifest is no longer WebAPK-capable

For instance, an upgraded WebAPK should not be requested if the Web Manifest was
updated to no longer have a start_url

BUG=627824
TEST=ManifestUpgradeDetectorTest.testWebManifestNotWebApkCapableShouldNotUpgrade
==========

to

==========
Do not request upgrade if Web Manifest is no longer WebAPK-capable

For instance, an upgraded WebAPK should not be requested if the Web Manifest was
updated to no longer have a start_url

BUG=627824
TEST=ManifestUpgradeDetectorTest.testWebManifestNotWebApkCapableShouldNotUpgrade
==========

[commit-bot: I haz the power, 2016-08-24 21:24:29]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-08-24 21:26:44]

Description was changed from

==========
Do not request upgrade if Web Manifest is no longer WebAPK-capable

For instance, an upgraded WebAPK should not be requested if the Web Manifest was
updated to no longer have a start_url

BUG=627824
TEST=ManifestUpgradeDetectorTest.testWebManifestNotWebApkCapableShouldNotUpgrade
==========

to

==========
Do not request upgrade if Web Manifest is no longer WebAPK-capable

For instance, an upgraded WebAPK should not be requested if the Web Manifest was
updated to no longer have a start_url

BUG=627824
TEST=ManifestUpgradeDetectorTest.testWebManifestNotWebApkCapableShouldNotUpgrade

Committed: https://crrev.com/090b6f3b15c5b11a304db2c7d61f9ff8672a8f1d
Cr-Commit-Position: refs/heads/master@{#414149}
==========

[commit-bot: I haz the power, 2016-08-24 21:26:45]

Patchset 4 (id:??) landed as
https://crrev.com/090b6f3b15c5b11a304db2c7d61f9ff8672a8f1d
Cr-Commit-Position: refs/heads/master@{#414149}