Request upgraded WebAPK from WebAPK server if the ShellAPK version changes

Request upgraded WebAPK from WebAPK server if the ShellAPK version changes

The ShellAPK version is the version of the source code in
//chrome/android/webapk/shell_apk

This CL also makes WebApkUpdateManager get the WebAPK's <meta-data> from the
Android Manifest and pass it to ManifestUpgradeDetector. Repeatedly requesting
the <meta-data> from the PackageManager takes 0.5ms on a Nexus 5.

BUG=624834
Committed: https://crrev.com/eab4e851447a299a36458474d824b0ed569db55f
Cr-Commit-Position: refs/heads/master@{#415079}

[pkotwicz, 2016-08-18 03:29:19]

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

[pkotwicz, 2016-08-18 03:29:19]

Xi can you please take a look?

[pkotwicz, 2016-08-18 03:30:05]

Xi can you please take a look? This CL depends on
https://codereview.chromium.org/2184913005/

[pkotwicz, 2016-08-18 03:33:02]

Description was changed from

==========
Request upgraded WebAPK from WebAPK server if the ShellAPK version changes

The ShellAPK version is the version of the source code in
//chrome/android/webapk/shell_apk

BUG=624834
==========

to

==========
Request upgraded WebAPK from WebAPK server if the ShellAPK version changes

The ShellAPK version is the version of the source code in
//chrome/android/webapk/shell_apk

This CL also makes WebApkUpdateManager get the WebAPK's <meta-data> from the
Android Manifest and pass it to ManifestUpgradeDetector. Repeatedly requesting
the <meta-data> from the PackageManager takes 0.5ms on a Nexus 5.

BUG=624834
==========

[Yaron, 2016-08-19 14:02:12]

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

[Yaron, 2016-08-19 14:02:13]

https://codereview.chromium.org/2259713002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkUpdateManager.java
(right):

https://codereview.chromium.org/2259713002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkUpdateManager.java:81:
if (sinceLastCheckDuration > FULL_CHECK_UPDATE_INTERVAL || mForceUpgrade) {
two concerns:
1) if we are in "force upgrade" but the upgrade check hangs on a given page (I
thought I called out a case it could happen in another review) then we'll still
be stuck, not upgrading. Could we re-use assets after a timeout? Perhaps a
TODO/bug is appropriate for this since it technically shouldn't happen but I
think it currently does on a dev build of chromium on airhorner (since it
crashes on trunk)
2) i don't think "force" should work independent of time constraints. My worry
is that we push a new chrome and everyone starts upgrading and with some
failures to upgrade, they keep pinging rather aggressively because we don't
throttle on the client.

[Xi Han, 2016-08-19 14:53:56]

l-g-t-m after Yaron's concern is addressed.

https://codereview.chromium.org/2259713002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkUpdateManager.java
(right):

https://codereview.chromium.org/2259713002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkUpdateManager.java:81:
if (sinceLastCheckDuration > FULL_CHECK_UPDATE_INTERVAL || mForceUpgrade) {
On 2016/08/19 14:02:13, Yaron (limited availability) wrote:
> two concerns:
> 1) if we are in "force upgrade" but the upgrade check hangs on a given page (I
> thought I called out a case it could happen in another review) then we'll
still
> be stuck, not upgrading. Could we re-use assets after a timeout? Perhaps a
> TODO/bug is appropriate for this since it technically shouldn't happen but I
> think it currently does on a dev build of chromium on airhorner (since it
> crashes on trunk)
> 2) i don't think "force" should work independent of time constraints. My worry
> is that we push a new chrome and everyone starts upgrading and with some
> failures to upgrade, they keep pinging rather aggressively because we don't
> throttle on the client. 

+1

[pkotwicz, 2016-08-22 20:04:18]

Xi can you please take another look?

https://codereview.chromium.org/2259713002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkUpdateManager.java
(right):

https://codereview.chromium.org/2259713002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkUpdateManager.java:81:
if (sinceLastCheckDuration > FULL_CHECK_UPDATE_INTERVAL || mForceUpgrade) {
1) Thank you for catching this Yaron! With my code we might request an upgraded
WebAPK with non WebAPK-compatible data. I have filed a bug at
http://crbug.com/639536
2) I have made "force" try to do an upgrade every 12 hours

[Xi Han, 2016-08-23 17:28:56]

lgtm

[pkotwicz, 2016-08-23 18:24:57]

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

[pkotwicz, 2016-08-23 18:24:57]

dfalcantara@ for OWNERS

[gone, 2016-08-24 17:37:02]

https://chromiumcodereview.appspot.com/2259713002/diff/20001/chrome/android/j...
File
chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetector.java
(right):

https://chromiumcodereview.appspot.com/2259713002/diff/20001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetector.java:105:
public ManifestUpgradeDetector(Tab tab, WebappInfo info, Bundle metaData,
Callback callback) {
nit: metadata is one word, so don't add spaces or camelcase

https://chromiumcodereview.appspot.com/2259713002/diff/20001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetector.java:140:
mManifestUrl = metaData.getString(META_DATA_WEB_MANIFEST_URL);
IntentUtils.safeGetString()

https://chromiumcodereview.appspot.com/2259713002/diff/20001/chrome/android/j...
File
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkUpdateManager.java
(right):

https://chromiumcodereview.appspot.com/2259713002/diff/20001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkUpdateManager.java:81:
// TODO: Request upgrade if ShellAPK version changes if
TODO who?

[pkotwicz, 2016-08-24 23:03:14]

Dan can you please take another look?

[gone, 2016-08-24 23:04:49]

lgtm

[pkotwicz, 2016-08-29 14:12:40]

Patchset #4 (id:60001) has been deleted

[pkotwicz, 2016-08-29 14:12:46]

Patchset #4 (id:80001) has been deleted

[pkotwicz, 2016-08-29 14:12:49]

The CQ bit was checked by pkotwicz@chromium.org

[pkotwicz, 2016-08-29 14:12:49]

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

[commit-bot: I haz the power, 2016-08-29 14:13: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-08-29 15:43:35]

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

[commit-bot: I haz the power, 2016-08-29 15:43:36]

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-29 18:22:01]

The CQ bit was checked by pkotwicz@chromium.org

[commit-bot: I haz the power, 2016-08-29 18:37:59]

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-30 04:13:10]

Description was changed from

==========
Request upgraded WebAPK from WebAPK server if the ShellAPK version changes

The ShellAPK version is the version of the source code in
//chrome/android/webapk/shell_apk

This CL also makes WebApkUpdateManager get the WebAPK's <meta-data> from the
Android Manifest and pass it to ManifestUpgradeDetector. Repeatedly requesting
the <meta-data> from the PackageManager takes 0.5ms on a Nexus 5.

BUG=624834
==========

to

==========
Request upgraded WebAPK from WebAPK server if the ShellAPK version changes

The ShellAPK version is the version of the source code in
//chrome/android/webapk/shell_apk

This CL also makes WebApkUpdateManager get the WebAPK's <meta-data> from the
Android Manifest and pass it to ManifestUpgradeDetector. Repeatedly requesting
the <meta-data> from the PackageManager takes 0.5ms on a Nexus 5.

BUG=624834
==========

[commit-bot: I haz the power, 2016-08-30 04:13:11]

Committed patchset #4 (id:100001)

[commit-bot: I haz the power, 2016-08-30 04:16:14]

Description was changed from

==========
Request upgraded WebAPK from WebAPK server if the ShellAPK version changes

The ShellAPK version is the version of the source code in
//chrome/android/webapk/shell_apk

This CL also makes WebApkUpdateManager get the WebAPK's <meta-data> from the
Android Manifest and pass it to ManifestUpgradeDetector. Repeatedly requesting
the <meta-data> from the PackageManager takes 0.5ms on a Nexus 5.

BUG=624834
==========

to

==========
Request upgraded WebAPK from WebAPK server if the ShellAPK version changes

The ShellAPK version is the version of the source code in
//chrome/android/webapk/shell_apk

This CL also makes WebApkUpdateManager get the WebAPK's <meta-data> from the
Android Manifest and pass it to ManifestUpgradeDetector. Repeatedly requesting
the <meta-data> from the PackageManager takes 0.5ms on a Nexus 5.

BUG=624834

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

[commit-bot: I haz the power, 2016-08-30 04:16:16]

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