New issue
Advanced search Search tips

Issue 639536 link

Starred by 3 users

Issue metadata

Status: Fixed
Owner:
Closed: Jan 2017
Components:
EstimatedDays: ----
NextAction: ----
OS: Android
Pri: 3
Type: Bug

Blocking:
issue 609258



Sign in to add a comment

Upgrade WebAPK if ShellApk version is updated if WebAPK web manifest is deleted

Project Member Reported by pkotw...@chromium.org, Aug 20 2016

Issue description

Currently, ManifestUpgradeDetector must first fetch the site's updated Web Manifest in order to upgrade the WebAPK. If the web page no longer refers to a Web Manifest, ManifestUpgradeDetector does not call its callback.

Even though the site is no longer WebAPK-able we still want to update the WebAPK's ShellAPK version if the user currently has a WebAPK for the site installed.
 
Blocking: 609258
Tentatively marking this bug as blocking the dev release
Labels: good-starter-bug
This would be a bug
Labels: -good-starter-bug Hotlist-GoodFirstBug
Owner: pkotw...@chromium.org
Status: Assigned (was: Untriaged)
Project Member

Comment 5 by bugdroid1@chromium.org, Oct 13 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/cd85a3b3a92fb04a89b586110bea55ce3f0bfa00

commit cd85a3b3a92fb04a89b586110bea55ce3f0bfa00
Author: pkotwicz <pkotwicz@chromium.org>
Date: Thu Oct 13 18:51:43 2016

Make the timing of ManifestUpgradeDetectorFetcher::Start() call less subtle

Currently ManifestUpgradeDetectorFetcher assumes that
ManifestUpgradeDetectorFetcher::Start() will be called prior to the initial page
load finishing (a lot of web apps are a single page sites so there is only
one page load ever).

This assumumption relies on:
- The specifics of when ChromeActivity#onDeferredStartup() is called.
- How long the WebappRegistry#getWebappDataStorage() invocation in
  WebApkUpdateManager#updateIfNeeded() takes to call the callback.

This CL makes ManifestUpgradeDetectorFetcher::Start() work even if it is called
after the initial page load finishes

BUG= 639536 

Review-Url: https://codereview.chromium.org/2402903002
Cr-Commit-Position: refs/heads/master@{#425110}

[modify] https://crrev.com/cd85a3b3a92fb04a89b586110bea55ce3f0bfa00/chrome/browser/android/webapk/manifest_upgrade_detector_fetcher.cc
[modify] https://crrev.com/cd85a3b3a92fb04a89b586110bea55ce3f0bfa00/chrome/browser/android/webapk/manifest_upgrade_detector_fetcher.h

Status: Started (was: Assigned)
Project Member

Comment 7 by bugdroid1@chromium.org, Oct 21 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/62ed2afce28cc27f6acd19115a7715f41dcc5e8f

commit 62ed2afce28cc27f6acd19115a7715f41dcc5e8f
Author: pkotwicz <pkotwicz@chromium.org>
Date: Fri Oct 21 19:54:10 2016

Fix WebappDataStorage#updateDidLastWebApkUpdateRequestSucceed() corner cases

This CL fixes three corner cases:
1) This CL sets the "WebAPK update" as having succeeded if:
  - the previous WebAPK update failed
  AND
  - A WebAPK update is no longer needed
  Doing this is important because the is-webapk-update-needed check is
  done much more frequently if the "previous WebAPK update failed."
2) This CL ignores whether the "WebAPK update" has previously succeeded if
   there have been no update attempts.
   The goal is to wait at least WebApkUpdateManager#FULL_CHECK_UPDATE_INTERVAL
   since the WebAPK's first launch before checking for updates.
3) This CL sets the "WebAPK update" as having failed if Chrome is killed
  after the time that the WebAPK update is requested but before
  WebApkUpdateManager#onBuiltWebApk() is called.

BUG= 639536 
TEST=WebApkManagerTest.*
R=dominickn, mlamouri
TBR=tedchoc (For new dependencies to chrome_junit_tests in BUILD.gn)

Review-Url: https://chromiumcodereview.appspot.com/2430773002
Cr-Commit-Position: refs/heads/master@{#426868}

[modify] https://crrev.com/62ed2afce28cc27f6acd19115a7715f41dcc5e8f/chrome/android/BUILD.gn
[modify] https://crrev.com/62ed2afce28cc27f6acd19115a7715f41dcc5e8f/chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java
[modify] https://crrev.com/62ed2afce28cc27f6acd19115a7715f41dcc5e8f/chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkUpdateManager.java
[modify] https://crrev.com/62ed2afce28cc27f6acd19115a7715f41dcc5e8f/chrome/android/java_sources.gni
[modify] https://crrev.com/62ed2afce28cc27f6acd19115a7715f41dcc5e8f/chrome/android/junit/src/org/chromium/chrome/browser/webapps/WebApkMetaDataUtilsTest.java
[add] https://crrev.com/62ed2afce28cc27f6acd19115a7715f41dcc5e8f/chrome/android/junit/src/org/chromium/chrome/browser/webapps/WebApkUpdateManagerTest.java
[modify] https://crrev.com/62ed2afce28cc27f6acd19115a7715f41dcc5e8f/chrome/android/webapk/shell_apk/BUILD.gn
[modify] https://crrev.com/62ed2afce28cc27f6acd19115a7715f41dcc5e8f/chrome/android/webapk/shell_apk/junit/DEPS
[modify] https://crrev.com/62ed2afce28cc27f6acd19115a7715f41dcc5e8f/chrome/android/webapk/shell_apk/junit/src/org/chromium/webapk/shell_apk/MainActivityTest.java
[add] https://crrev.com/62ed2afce28cc27f6acd19115a7715f41dcc5e8f/chrome/android/webapk/test/BUILD.gn
[add] https://crrev.com/62ed2afce28cc27f6acd19115a7715f41dcc5e8f/chrome/android/webapk/test/src/org/chromium/webapk/test/WebApkTestHelper.java

Project Member

Comment 8 by bugdroid1@chromium.org, Oct 28 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/529d60f1e0addf28fe0a08a9643eb4d04688fb63

commit 529d60f1e0addf28fe0a08a9643eb4d04688fb63
Author: pkotwicz <pkotwicz@chromium.org>
Date: Fri Oct 28 15:36:42 2016

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

WebAPKs are updated when either:
- The web developer changes the Web Manifest
OR
- The code in chrome/android/webapk/shell_apk/ is modified to provide new
  features (because of a new version of Chrome)

If a Web Developer removes the WebAPK's Web Manifest from their site, it is
still necessary for the WebAPK to update when the code in
chrome/android/webapk/shell_apk/ changes. Updating a WebAPK in this scenario is
desirable so that users can take advantage of new WebAPK-related features.

This CL:
1) Makes the WebAPK request an update from the WebAPK server with parameters
from the WebAPK's Android manifest if the Web Manifest cannot be fetched.

2)Adds WebApkUpdateManager#onFinishedFetchingWebManifestForInitialUrl() and
WebApkUpdateManager#onGotManifestData()
This CL adds the implementation for
#onFinishedFetchingWebManifestForInitialUrl() but does not add the code which
calls #onFinishedFetchingWebManifestForInitialUrl()

#onFinishedFetchingWebManifestForInitialUrl() will be called after the Web
Manifest is fetched (successfully or unsuccessfully) after the initial page
load. If an update is needed because the code in
chrome/android/webapk/shell_apk/ changed, an update is requested by
#onFinishedFetchingWebManifestForInitialUrl() regardless of whether fetching
the Web Manifest was successful. It is possible that an update is requested
with a stale Web Manifest (because the Web Manifest changed as well) but at
least an update is guaranteed to be requested.

#onGotManifestData() will be called once/if the Web Manifest is fetched
SUCCESSFULLY on a page load after the first page load. The page at the WebAPK's
start_url might not have a Web Manifest but a page linked from the start_url
might have a Web Manifest. For instance, the page at start_url might be a
simple JavaScript redirect. Having #onGotManifestData() enables updating the
WebAPK even if the page at the WebAPK's start_url does not have a Web Manifest.

BUG= 639536 
TEST=WebApkUpdateManagerTest.*

Review-Url: https://codereview.chromium.org/2435383002
Cr-Commit-Position: refs/heads/master@{#428381}

[modify] https://crrev.com/529d60f1e0addf28fe0a08a9643eb4d04688fb63/chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetector.java
[add] https://crrev.com/529d60f1e0addf28fe0a08a9643eb4d04688fb63/chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkMetaData.java
[modify] https://crrev.com/529d60f1e0addf28fe0a08a9643eb4d04688fb63/chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkMetaDataUtils.java
[modify] https://crrev.com/529d60f1e0addf28fe0a08a9643eb4d04688fb63/chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkUpdateManager.java
[modify] https://crrev.com/529d60f1e0addf28fe0a08a9643eb4d04688fb63/chrome/android/java_sources.gni
[modify] https://crrev.com/529d60f1e0addf28fe0a08a9643eb4d04688fb63/chrome/android/javatests/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetectorTest.java
[modify] https://crrev.com/529d60f1e0addf28fe0a08a9643eb4d04688fb63/chrome/android/junit/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetectorTest.java
[modify] https://crrev.com/529d60f1e0addf28fe0a08a9643eb4d04688fb63/chrome/android/junit/src/org/chromium/chrome/browser/webapps/WebApkUpdateManagerTest.java

Project Member

Comment 9 by bugdroid1@chromium.org, Dec 5 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/78660e873fd72044bcbcd970fd7456453680b564

commit 78660e873fd72044bcbcd970fd7456453680b564
Author: pkotwicz <pkotwicz@chromium.org>
Date: Mon Dec 05 17:02:28 2016

Make ManifestUpgradeDetectorTest use the intent version of WebApkInfo#create()

- This makes the tests more integration-test-like.
- This is a step towards merging ManifestUpgradeDetectorTest and
  WebApkUpdateManagerTest (See https://codereview.chromium.org/2460253002/)

BUG= 639536 

Review-Url: https://codereview.chromium.org/2543073005
Cr-Commit-Position: refs/heads/master@{#436326}

[modify] https://crrev.com/78660e873fd72044bcbcd970fd7456453680b564/chrome/android/junit/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetectorTest.java

Project Member

Comment 10 by bugdroid1@chromium.org, Dec 9 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/b1e143c78604fdd43daa19e5123da514509ee091

commit b1e143c78604fdd43daa19e5123da514509ee091
Author: pkotwicz <pkotwicz@chromium.org>
Date: Fri Dec 09 20:52:46 2016

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
Cr-Commit-Position: refs/heads/master@{#437641}

[modify] https://crrev.com/b1e143c78604fdd43daa19e5123da514509ee091/chrome/android/BUILD.gn
[delete] https://crrev.com/9c5e2946272ea6778fbf23596142c7265d452cc4/chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetector.java
[rename] https://crrev.com/b1e143c78604fdd43daa19e5123da514509ee091/chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkUpdateDataFetcher.java
[modify] https://crrev.com/b1e143c78604fdd43daa19e5123da514509ee091/chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkUpdateManager.java
[modify] https://crrev.com/b1e143c78604fdd43daa19e5123da514509ee091/chrome/android/java_sources.gni
[rename] https://crrev.com/b1e143c78604fdd43daa19e5123da514509ee091/chrome/android/javatests/src/org/chromium/chrome/browser/webapps/WebApkUpdateDataFetcherTest.java
[rename] https://crrev.com/b1e143c78604fdd43daa19e5123da514509ee091/chrome/android/javatests/src/org/chromium/chrome/browser/webapps/WebApkUpdateManagerTest.java
[modify] https://crrev.com/b1e143c78604fdd43daa19e5123da514509ee091/chrome/android/junit/src/org/chromium/chrome/browser/webapps/WebApkUpdateManagerTest.java
[modify] https://crrev.com/b1e143c78604fdd43daa19e5123da514509ee091/chrome/browser/BUILD.gn
[modify] https://crrev.com/b1e143c78604fdd43daa19e5123da514509ee091/chrome/browser/android/chrome_jni_registrar.cc
[rename] https://crrev.com/b1e143c78604fdd43daa19e5123da514509ee091/chrome/browser/android/webapk/webapk_update_data_fetcher.cc
[rename] https://crrev.com/b1e143c78604fdd43daa19e5123da514509ee091/chrome/browser/android/webapk/webapk_update_data_fetcher.h

Project Member

Comment 11 by bugdroid1@chromium.org, Dec 11 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/cf064891c24ce454379b95b3a2a312b4e252c539

commit cf064891c24ce454379b95b3a2a312b4e252c539
Author: pkotwicz <pkotwicz@chromium.org>
Date: Sun Dec 11 02:58:04 2016

[WebAPKs] Compute the default scope from the manifest start URL.

This CL changes how the scope is computed if the scope is unspecified in the
Android Manifest.
We used to compute the default scope based on the "URL the WebAPK navigates to
when launched." Deep links can open a WebAPK at an arbirtrary URL.
This CL changes the computation to be based on the Web Manifest start URL.

This change enables cleaning up
ManifestUpgradeDetectorFetcher#onDataAvailable(). We can now create the fetched
WebApkInfo using the current WebAPK's WebApkInfo#uri().

BUG= 639536 

Review-Url: https://codereview.chromium.org/2548333006
Cr-Commit-Position: refs/heads/master@{#437785}

[modify] https://crrev.com/cf064891c24ce454379b95b3a2a312b4e252c539/chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkInfo.java
[modify] https://crrev.com/cf064891c24ce454379b95b3a2a312b4e252c539/chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkUpdateDataFetcher.java
[modify] https://crrev.com/cf064891c24ce454379b95b3a2a312b4e252c539/chrome/android/javatests/src/org/chromium/chrome/browser/webapps/WebApkUpdateDataFetcherTest.java
[modify] https://crrev.com/cf064891c24ce454379b95b3a2a312b4e252c539/chrome/android/junit/src/org/chromium/chrome/browser/webapps/WebApkInfoTest.java

Project Member

Comment 12 by bugdroid1@chromium.org, Dec 22 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/f20ef410db15c47df68f101b8a105cbe98d0c437

commit f20ef410db15c47df68f101b8a105cbe98d0c437
Author: hanxi <hanxi@chromium.org>
Date: Thu Dec 22 01:34:05 2016

Update WebAPKs even if the WebAPK start URL has no Web Manifest

This CL makes WebApkUpdateDataFetcher call back to WebApkUpdateManager on the
WebAPK's initial load regardless of whether the URL has an associated Web
Manifest.

If
- The WebAPK's start URL no longer has an associated Web Manifest or the
updated Web Manifest is not WebAPK-able
AND
- An update is needed because the WebAPK's Java code is out of date
Then a WebAPK update is requested using the data in the WebAPK's
AndroidManifest.xml

If the WebAPK's Java code is not out of date, WebApkUpdateDataFetcher is not
destroyed. The
WebApkUpdateDataFetcher will check whether the page has a WebAPK-able Web
Manifest on each subsequent page load till a WebAPK-able Web Manifest is found.
When a WebAPK-able Web Manifest is found, WebApkUpdateManager sends a WebAPK
update request if the new Web Manifest differs from the Web Manifest data
in AndroidManifest.xml

BUG= 639536 
TEST=WebApkDataFetcherTest.testNoWebManifest

Review-Url: https://codereview.chromium.org/2594813002
Cr-Commit-Position: refs/heads/master@{#440302}

[modify] https://crrev.com/f20ef410db15c47df68f101b8a105cbe98d0c437/chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkUpdateDataFetcher.java
[modify] https://crrev.com/f20ef410db15c47df68f101b8a105cbe98d0c437/chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkUpdateManager.java
[modify] https://crrev.com/f20ef410db15c47df68f101b8a105cbe98d0c437/chrome/android/javatests/src/org/chromium/chrome/browser/webapps/WebApkUpdateDataFetcherTest.java
[modify] https://crrev.com/f20ef410db15c47df68f101b8a105cbe98d0c437/chrome/android/javatests/src/org/chromium/chrome/browser/webapps/WebApkUpdateManagerTest.java
[modify] https://crrev.com/f20ef410db15c47df68f101b8a105cbe98d0c437/chrome/android/junit/src/org/chromium/chrome/browser/webapps/WebApkUpdateManagerTest.java
[modify] https://crrev.com/f20ef410db15c47df68f101b8a105cbe98d0c437/chrome/browser/android/webapk/webapk_update_data_fetcher.cc
[modify] https://crrev.com/f20ef410db15c47df68f101b8a105cbe98d0c437/chrome/browser/android/webapk/webapk_update_data_fetcher.h

Status: Fixed (was: Started)
Marking this bug as fixed. There isn't a good way of manually testing this bug fix without modifying Chrome's code. In order to update the ShellAPK version, we need to modify Chrome's code

Sign in to add a comment