Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2124513002:
Introduce ManifestUpgradeDetector for WebAPK to detect web manifest changes. (Closed)
DescriptionIntroduce ManifestUpgradeDetector for WebAPK to detect web manifest changes.
The ManifestUpgradeDetector fetches the latest web manifest for a WebAPK, compare
with cached one and make a decision of whether to request re-mint.
The ManifestUpgradeDetector is initilized for WebApkActivity when it needs to check
resources changes. Once the detector starts, it observes WebContents's change,
and stops when the manifest file of the WebApk is found and fetched. This is
because not all of the urls within the WebAPK's scope link to its manifest file.
Therefore, the detector needs to follow WebContents's change until find
the manifest file.
TEST=org.chromium.chrome.browser.webapps.ManifestUpgradeDetectorTest
BUG=624834
Committed: https://crrev.com/e72182e94924a4a8af00b016ed75fb1bdc554500
Cr-Commit-Position: refs/heads/master@{#408636}
Patch Set 1 : Introduce manifest upgrade detector. #
Total comments: 17
Patch Set 2 : Remove data fetcher and doesn't fetch icon for now. #
Total comments: 22
Patch Set 3 : Remove changes of short name. #
Total comments: 12
Patch Set 4 : Remove ManifestUpgradeDetector.Observer. #
Total comments: 44
Patch Set 5 : Fix the scope. #
Total comments: 18
Patch Set 6 : pkotwicz@'s comments. #
Total comments: 2
Patch Set 7 : Remove IconUrls. #
Total comments: 46
Patch Set 8 : Get start_url from WebAPK's metadata and nits. #
Total comments: 61
Patch Set 9 : pkotwicz@ and yfriedman@'s comments. #
Total comments: 25
Patch Set 10 : Every change in web manifest will request to re-mint WebAPK. #
Total comments: 29
Patch Set 11 : Clean up. #
Total comments: 40
Patch Set 12 : Clean up and remove unit_tests. #
Total comments: 13
Patch Set 13 : Nits. #
Total comments: 3
Patch Set 14 : Remvoe tests in WebappDataStorageTests. #Messages
Total messages: 87 (48 generated)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:40001) has been deleted
Patchset #1 (id:60001) has been deleted
Patchset #1 (id:80001) has been deleted
Patchset #1 (id:100001) has been deleted
Description was changed from ========== Introduce manifest upgrade detector. BUG=624834 ========== to ========== Introduce ManifestUpgradeDetector for WebAPK to detect web manifest changes. Resources changes in web manifest file will require to re-mint a WebAPK. For example, start Url, background color and Webapp Name. These infos are backed into WebAPK's AndroidManifest.xml. Some other resources, like orientaion etc., are stored in Chrome's metadata. The ManifestUpgradeDetector fetches the latest web manifest for a WebAPK, compare with cached one and make a decision of whether to request re-mint. It holds a data fetcher to fetch manifest file stored in the renderer. We reuses the AddToHomescreenDataFetcher. The ManifestUpgradeDetector is one per tab, and only be initilized for tabs of WebApkActivity. Once the detector starts, it observes WebContents's change, and stops when the manifest file of the WebApk is found and fetched. This is because not all of the urls within the WebAPK's scope link to its manifest file, the detector needs to follow WebContents's change until find the manifest file. BUG=624834 ==========
Description was changed from ========== Introduce ManifestUpgradeDetector for WebAPK to detect web manifest changes. Resources changes in web manifest file will require to re-mint a WebAPK. For example, start Url, background color and Webapp Name. These infos are backed into WebAPK's AndroidManifest.xml. Some other resources, like orientaion etc., are stored in Chrome's metadata. The ManifestUpgradeDetector fetches the latest web manifest for a WebAPK, compare with cached one and make a decision of whether to request re-mint. It holds a data fetcher to fetch manifest file stored in the renderer. We reuses the AddToHomescreenDataFetcher. The ManifestUpgradeDetector is one per tab, and only be initilized for tabs of WebApkActivity. Once the detector starts, it observes WebContents's change, and stops when the manifest file of the WebApk is found and fetched. This is because not all of the urls within the WebAPK's scope link to its manifest file, the detector needs to follow WebContents's change until find the manifest file. BUG=624834 ========== to ========== Introduce ManifestUpgradeDetector for WebAPK to detect web manifest changes. Resources changes in web manifest file will require to re-mint a WebAPK. For example, start Url, background color and Webapp Name. These infos are baked into WebAPK's AndroidManifest.xml. Some other resources, like orientaion etc., are stored in Chrome's metadata. The ManifestUpgradeDetector fetches the latest web manifest for a WebAPK, compare with cached one and make a decision of whether to request re-mint. It holds a data fetcher to fetch manifest file stored in the renderer. We reuses the AddToHomescreenDataFetcher. The ManifestUpgradeDetector is one per tab, and only be initilized for tabs of WebApkActivity. Once the detector starts, it observes WebContents's change, and stops when the manifest file of the WebApk is found and fetched. This is because not all of the urls within the WebAPK's scope link to its manifest file, the detector needs to follow WebContents's change until find the manifest file. BUG=624834 ==========
Description was changed from ========== Introduce ManifestUpgradeDetector for WebAPK to detect web manifest changes. Resources changes in web manifest file will require to re-mint a WebAPK. For example, start Url, background color and Webapp Name. These infos are baked into WebAPK's AndroidManifest.xml. Some other resources, like orientaion etc., are stored in Chrome's metadata. The ManifestUpgradeDetector fetches the latest web manifest for a WebAPK, compare with cached one and make a decision of whether to request re-mint. It holds a data fetcher to fetch manifest file stored in the renderer. We reuses the AddToHomescreenDataFetcher. The ManifestUpgradeDetector is one per tab, and only be initilized for tabs of WebApkActivity. Once the detector starts, it observes WebContents's change, and stops when the manifest file of the WebApk is found and fetched. This is because not all of the urls within the WebAPK's scope link to its manifest file, the detector needs to follow WebContents's change until find the manifest file. BUG=624834 ========== to ========== Introduce ManifestUpgradeDetector for WebAPK to detect web manifest changes. Resources changes in web manifest file will require to re-mint a WebAPK. For example, start Url, background color and Webapp Name. These infos are baked into WebAPK's AndroidManifest.xml. Some other resources, like orientaion etc., are stored in Chrome's metadata. The ManifestUpgradeDetector fetches the latest web manifest for a WebAPK, compare with cached one and make a decision of whether to request re-mint. It holds a data fetcher to fetch manifest file stored in the renderer. We reuses the AddToHomescreenDataFetcher. The ManifestUpgradeDetector is one per tab, and only be initilized for tabs of WebApkActivity. Once the detector starts, it observes WebContents's change, and stops when the manifest file of the WebApk is found and fetched. This is because not all of the urls within the WebAPK's scope link to its manifest file. Therefore, the detector needs to follow WebContents's change until find the manifest file. BUG=624834 ==========
Description was changed from ========== Introduce ManifestUpgradeDetector for WebAPK to detect web manifest changes. Resources changes in web manifest file will require to re-mint a WebAPK. For example, start Url, background color and Webapp Name. These infos are baked into WebAPK's AndroidManifest.xml. Some other resources, like orientaion etc., are stored in Chrome's metadata. The ManifestUpgradeDetector fetches the latest web manifest for a WebAPK, compare with cached one and make a decision of whether to request re-mint. It holds a data fetcher to fetch manifest file stored in the renderer. We reuses the AddToHomescreenDataFetcher. The ManifestUpgradeDetector is one per tab, and only be initilized for tabs of WebApkActivity. Once the detector starts, it observes WebContents's change, and stops when the manifest file of the WebApk is found and fetched. This is because not all of the urls within the WebAPK's scope link to its manifest file. Therefore, the detector needs to follow WebContents's change until find the manifest file. BUG=624834 ========== to ========== Introduce ManifestUpgradeDetector for WebAPK to detect web manifest changes. Resources changes in web manifest file will require to re-mint a WebAPK. For example, start Url, background color and Webapp Name. These infos are baked into WebAPK's AndroidManifest.xml. Some other resources, like orientaion etc., are stored in Chrome's metadata. The ManifestUpgradeDetector fetches the latest web manifest for a WebAPK, compare with cached one and make a decision of whether to request re-mint. It holds a data fetcher to fetch manifest file stored in the renderer. We reuses the AddToHomescreenDataFetcher. The ManifestUpgradeDetector is one per tab, and only be initilized for tabs of WebApkActivity. Once the detector starts, it observes WebContents's change, and stops when the manifest file of the WebApk is found and fetched. This is because not all of the urls within the WebAPK's scope link to its manifest file. Therefore, the detector needs to follow WebContents's change until find the manifest file. BUG=624834 ==========
Description was changed from ========== Introduce ManifestUpgradeDetector for WebAPK to detect web manifest changes. Resources changes in web manifest file will require to re-mint a WebAPK. For example, start Url, background color and Webapp Name. These infos are baked into WebAPK's AndroidManifest.xml. Some other resources, like orientaion etc., are stored in Chrome's metadata. The ManifestUpgradeDetector fetches the latest web manifest for a WebAPK, compare with cached one and make a decision of whether to request re-mint. It holds a data fetcher to fetch manifest file stored in the renderer. We reuses the AddToHomescreenDataFetcher. The ManifestUpgradeDetector is one per tab, and only be initilized for tabs of WebApkActivity. Once the detector starts, it observes WebContents's change, and stops when the manifest file of the WebApk is found and fetched. This is because not all of the urls within the WebAPK's scope link to its manifest file. Therefore, the detector needs to follow WebContents's change until find the manifest file. BUG=624834 ========== to ========== Introduce ManifestUpgradeDetector for WebAPK to detect web manifest changes. Resources changes in web manifest file will require to re-mint a WebAPK. For example, start Url, background color and Webapp Name. These infos are baked into WebAPK's AndroidManifest.xml. Some other resources, like orientaion etc., are stored in Chrome's metadata. The ManifestUpgradeDetector fetches the latest web manifest for a WebAPK, compare with cached one and make a decision of whether to request re-mint. It holds a data fetcher to fetch manifest file stored in the renderer. We reuses the AddToHomescreenDataFetcher. The ManifestUpgradeDetector is one per tab, and only be initilized for tabs of WebApkActivity. Once the detector starts, it observes WebContents's change, and stops when the manifest file of the WebApk is found and fetched. This is because not all of the urls within the WebAPK's scope link to its manifest file. Therefore, the detector needs to follow WebContents's change until find the manifest file. TEST=org.chromium.chrome.browser.webapps.ManifestUpgradeDetectorTest BUG=624834 ==========
hanxi@chromium.org changed reviewers: + pkotwicz@chromium.org
Hi Peter, could you please take a look? Thanks!
Drive-by FYI 1: I'm removing WebContents::HasManifest in crrev.com/2140463002 (I've made manifest_url.is_empty() in WebContents::GetManifest mean the same thing to skip the extra IPCs. Thanks pkotwicz for adding the url!) Drive-by FYI 2: this may be simplified when I land the new InstallableChecker (hopefully this week! Or next week!). It will be a WebContents-scoped fetcher/cache for the manifest (and other things like the app icon etc.). Since we're already fetching the manifest on every page load for banners, it makes sense just to cache the manifest in the browser process and avoid the extra IPC of calling WebContents::GetManifest again. Once I land it, it'll also remove all of the manifest/icon fetching in add_to_homescreen_data_fetcher.cc; it will just query the checker. You'll just need to add the Java side hooks you have here to forward the manifest changes to WebAPK code.
https://codereview.chromium.org/2124513002/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetector.java (right): https://codereview.chromium.org/2124513002/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetector.java:123: final Set<String> iconUrlSet = new HashSet<String>(Arrays.asList(iconUrls)); This if() statement should be in its own function. It took me a while to remember why we were checking only some of WebappInfo's attributes. https://codereview.chromium.org/2124513002/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetector.java:130: } Should we always call WebappDataStorage#updateFromWebappInfo() regardless of whether the Web Manifest has changed? I think this will make things simpler https://codereview.chromium.org/2124513002/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java (right): https://codereview.chromium.org/2124513002/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java:86: mWebappInfo.updateThemeColor(storage.getThemeColor()); Shouldn't we also update |mWebappInfo|'s short_name? https://codereview.chromium.org/2124513002/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java:189: if (shouldCheckUpdate()) { Can the ManifestUpgradeDetector be created in this function? I must be missing something. Why does Tab hold an instance of ManifestUpgradeDetector You can get the Tab associated with the WebAPK by calling WebApkActivity#getActivityTab() https://codereview.chromium.org/2124513002/diff/120001/chrome/browser/android... File chrome/browser/android/webapps/manifest_upgrade_detector.cc (right): https://codereview.chromium.org/2124513002/diff/120001/chrome/browser/android... chrome/browser/android/webapps/manifest_upgrade_detector.cc:56: const GURL& validated_url) { Random thought: I wonder how this works with 301 HTTP redirects. For instance: WebAPK is minted with start URL www.example.com/home1.html www.example.com/home1.html is changed to HTTP 301 redirect to www.example.com/home2.html www.example.com/home2.html uses the same web manifest URL as www.example.com/home1.html uses to https://codereview.chromium.org/2124513002/diff/120001/chrome/browser/android... chrome/browser/android/webapps/manifest_upgrade_detector.cc:90: true, this); For the sake of landing something this week, I think that it makes sense to revert the image-related checking. (Don't delete it, keep it in a separate git branch) For the image-related checking we should extract the useful parts out of AddToHomescreenDataFetcher into a separate class Without the image related checking, this class needs to just call: content::WebContents::GetManifest() (which is simple enough that it should not interfere with dominickn@'s work) I think that it should be simple enough that you do not need a dedicated fetcher class I do not think that it is worth calling WebContents::HasManifest(). Calling WebContents::HasManifest() prior to WebContents::GetManifest() does not save much/any computation. https://codereview.chromium.org/2124513002/diff/120001/chrome/browser/android... chrome/browser/android/webapps/manifest_upgrade_detector.cc:117: We should check whether the manifest URL is the same one as in the WebAPK manifest. If 1) a user minted a WebAPK for funnysite.com 2) the developer for funnysite.com makes their home page do an HTTP redirect to google.com This logic should not cause a OneOffWebAPK for funnysite.com to be updated into a OneOffWebAPK for google.com https://codereview.chromium.org/2124513002/diff/120001/chrome/browser/android... chrome/browser/android/webapps/manifest_upgrade_detector.cc:146: start_fetching_ = true; Nit: Rename to |started_|
This is a complicated piece of logic. Xi, I really appreciate you taking this task on!
Patchset #2 (id:140001) has been deleted
Patchset #2 (id:160001) has been deleted
Patchset #2 (id:180001) has been deleted
Description was changed from ========== Introduce ManifestUpgradeDetector for WebAPK to detect web manifest changes. Resources changes in web manifest file will require to re-mint a WebAPK. For example, start Url, background color and Webapp Name. These infos are baked into WebAPK's AndroidManifest.xml. Some other resources, like orientaion etc., are stored in Chrome's metadata. The ManifestUpgradeDetector fetches the latest web manifest for a WebAPK, compare with cached one and make a decision of whether to request re-mint. It holds a data fetcher to fetch manifest file stored in the renderer. We reuses the AddToHomescreenDataFetcher. The ManifestUpgradeDetector is one per tab, and only be initilized for tabs of WebApkActivity. Once the detector starts, it observes WebContents's change, and stops when the manifest file of the WebApk is found and fetched. This is because not all of the urls within the WebAPK's scope link to its manifest file. Therefore, the detector needs to follow WebContents's change until find the manifest file. TEST=org.chromium.chrome.browser.webapps.ManifestUpgradeDetectorTest BUG=624834 ========== to ========== Introduce ManifestUpgradeDetector for WebAPK to detect web manifest changes. Some attributes changes in web manifest file will require to re-mint a WebAPK. For example, start Url, background color and Webapp Name. These infos are baked into WebAPK's AndroidManifest.xml. Other attributes, like orientaion etc., their changes are stored in Chrome's metadata, but don't request to re-mint. The ManifestUpgradeDetector fetches the latest web manifest for a WebAPK, compare with cached one and make a decision of whether to request re-mint. The ManifestUpgradeDetector is initilized for WebApkActivity when it needs to check resources changes. Once the detector starts, it observes WebContents's change, and stops when the manifest file of the WebApk is found and fetched. This is because not all of the urls within the WebAPK's scope link to its manifest file. Therefore, the detector needs to follow WebContents's change until find the manifest file. TEST=org.chromium.chrome.browser.webapps.ManifestUpgradeDetectorTest BUG=624834 ==========
Description was changed from ========== Introduce ManifestUpgradeDetector for WebAPK to detect web manifest changes. Some attributes changes in web manifest file will require to re-mint a WebAPK. For example, start Url, background color and Webapp Name. These infos are baked into WebAPK's AndroidManifest.xml. Other attributes, like orientaion etc., their changes are stored in Chrome's metadata, but don't request to re-mint. The ManifestUpgradeDetector fetches the latest web manifest for a WebAPK, compare with cached one and make a decision of whether to request re-mint. The ManifestUpgradeDetector is initilized for WebApkActivity when it needs to check resources changes. Once the detector starts, it observes WebContents's change, and stops when the manifest file of the WebApk is found and fetched. This is because not all of the urls within the WebAPK's scope link to its manifest file. Therefore, the detector needs to follow WebContents's change until find the manifest file. TEST=org.chromium.chrome.browser.webapps.ManifestUpgradeDetectorTest BUG=624834 ========== to ========== Introduce ManifestUpgradeDetector for WebAPK to detect web manifest changes. Some attributes changes in web manifest file will require to re-mint a WebAPK. For example, start Url, background color and Webapp Name. These infos are baked into WebAPK's AndroidManifest.xml. Other attributes, like orientaion etc., their changes are stored in Chrome's metadata, but don't request to re-mint. The ManifestUpgradeDetector fetches the latest web manifest for a WebAPK, compare with cached one and make a decision of whether to request re-mint. The ManifestUpgradeDetector is initilized for WebApkActivity when it needs to check resources changes. Once the detector starts, it observes WebContents's change, and stops when the manifest file of the WebApk is found and fetched. This is because not all of the urls within the WebAPK's scope link to its manifest file. Therefore, the detector needs to follow WebContents's change until find the manifest file. This CL depends on CL(https://codereview.chromium.org/2140273002/). TEST=org.chromium.chrome.browser.webapps.ManifestUpgradeDetectorTest BUG=624834 ==========
hanxi@chromium.org changed reviewers: + dominickn@chromium.org
To Dominick: Thanks for the heads up. Very excited to see your patch. Meanwhile, I adopted Peter's suggestion to remove the icon check as well as the usage of AddToHomescreenDataFetcher. Instead, call WebContents#GetManifest() directly in the ManifestUpdateDetector for now. So it won't conflict with your changes. Thanks Peter! To Peter: I adopted your suggestion and added web_manifest_url check in the c++ side of the detector. Please take another look, thank you both! https://codereview.chromium.org/2124513002/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetector.java (right): https://codereview.chromium.org/2124513002/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetector.java:123: final Set<String> iconUrlSet = new HashSet<String>(Arrays.asList(iconUrls)); On 2016/07/12 01:23:48, pkotwicz wrote: > This if() statement should be in its own function. It took me a while to > remember why we were checking only some of WebappInfo's attributes. Done. https://codereview.chromium.org/2124513002/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetector.java:130: } On 2016/07/12 01:23:48, pkotwicz wrote: > Should we always call WebappDataStorage#updateFromWebappInfo() regardless of > whether the Web Manifest has changed? I think this will make things simpler We could, yes, then we don't need to check all those non-critical resources. Updated. https://codereview.chromium.org/2124513002/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java (right): https://codereview.chromium.org/2124513002/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java:86: mWebappInfo.updateThemeColor(storage.getThemeColor()); On 2016/07/12 01:23:49, pkotwicz wrote: > Shouldn't we also update |mWebappInfo|'s short_name? Done. https://codereview.chromium.org/2124513002/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java:189: if (shouldCheckUpdate()) { On 2016/07/12 01:23:49, pkotwicz wrote: > Can the ManifestUpgradeDetector be created in this function? I must be missing > something. Why does Tab hold an instance of ManifestUpgradeDetector > > You can get the Tab associated with the WebAPK by calling > WebApkActivity#getActivityTab() > That is a good idea. I once put the checkUpdates before initializeUI() so getActivityTab() return null. I will remove the unnecessary changes in Tab and TabDelegateFactory, since I don't think the detector should be there too. https://codereview.chromium.org/2124513002/diff/120001/chrome/browser/android... File chrome/browser/android/webapps/manifest_upgrade_detector.cc (right): https://codereview.chromium.org/2124513002/diff/120001/chrome/browser/android... chrome/browser/android/webapps/manifest_upgrade_detector.cc:56: const GURL& validated_url) { On 2016/07/12 01:23:49, pkotwicz wrote: > Random thought: I wonder how this works with 301 HTTP redirects. > > For instance: > WebAPK is minted with start URL http://www.example.com/home1.html > http://www.example.com/home1.html is changed to HTTP 301 redirect to > http://www.example.com/home2.html > > http://www.example.com/home2.html uses the same web manifest URL as > http://www.example.com/home1.html uses to If both urls link to the same manifest, no matter which url is loaded and points us to the manifest, should be considered as the manifest is found, and we are ok. But if they don't point to the same manifest, then we have to check whether the manifest is the one that we look for. Do I get your point? https://codereview.chromium.org/2124513002/diff/120001/chrome/browser/android... chrome/browser/android/webapps/manifest_upgrade_detector.cc:90: true, this); On 2016/07/12 01:23:49, pkotwicz wrote: > For the sake of landing something this week, I think that it makes sense to > revert the image-related checking. (Don't delete it, keep it in a separate git > branch) > > For the image-related checking we should extract the useful parts out of > AddToHomescreenDataFetcher into a separate class > > Without the image related checking, this class needs to just call: > content::WebContents::GetManifest() > (which is simple enough that it should not interfere with dominickn@'s work) > I think that it should be simple enough that you do not need a dedicated fetcher > class > > I do not think that it is worth calling WebContents::HasManifest(). Calling > WebContents::HasManifest() prior to WebContents::GetManifest() does not save > much/any computation. That is definitely a good idea. https://codereview.chromium.org/2124513002/diff/120001/chrome/browser/android... chrome/browser/android/webapps/manifest_upgrade_detector.cc:117: On 2016/07/12 01:23:49, pkotwicz wrote: > We should check whether the manifest URL is the same one as in the WebAPK > manifest. > > If > 1) a user minted a WebAPK for http://funnysite.com > 2) the developer for http://funnysite.com makes their home page do an HTTP redirect to > http://google.com > This logic should not cause a OneOffWebAPK for http://funnysite.com to be updated into > a OneOffWebAPK for http://google.com That was what I thought. Create a CL(https://codereview.chromium.org/2140273002/) to plumb web_manifest_url to webappInfo. We will check whether the manifest url loaded matches this baked one. https://codereview.chromium.org/2124513002/diff/120001/chrome/browser/android... chrome/browser/android/webapps/manifest_upgrade_detector.cc:146: start_fetching_ = true; On 2016/07/12 01:23:49, pkotwicz wrote: > Nit: Rename to |started_| Done.
https://codereview.chromium.org/2124513002/diff/120001/chrome/browser/android... File chrome/browser/android/webapps/manifest_upgrade_detector.cc (right): https://codereview.chromium.org/2124513002/diff/120001/chrome/browser/android... chrome/browser/android/webapps/manifest_upgrade_detector.cc:56: const GURL& validated_url) { My concern is that if the start URL is a 301 HTTP redirect we will get ManifestUpgradeDetector::DidFinishLoad() but fetching the manifest will fail. Do we even get ManifestUpgradeDetector::DidFinishLoad() for 301 HTTP redirects? https://codereview.chromium.org/2124513002/diff/200001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java (right): https://codereview.chromium.org/2124513002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java:87: mWebappInfo.updateShortName(storage.getShortName()); This change should be part of a separate CL which comes prior to this CL
https://codereview.chromium.org/2124513002/diff/200001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetector.java (right): https://codereview.chromium.org/2124513002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetector.java:48: public ManifestUpgradeDetector(Tab tab, WebappInfo info) { Can the observer be passed into the constructor? https://codereview.chromium.org/2124513002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetector.java:126: mWebappInfo.webManifestUrl()); We need to validate that the new WebappInfo is WebAPK-compatible. For instance the new short_name might be empty. Doing this checking will probably be easier once https://codereview.chromium.org/2050933002/ lands. I am OK if you file crbug and put a comment for now. https://codereview.chromium.org/2124513002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetector.java:163: || !mWebappInfo.name().equals(newInfo.name()) We should check whether the "short_name" is different not the "name" because it is the "short_name" which is used for the app's name in the app list https://codereview.chromium.org/2124513002/diff/200001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java (right): https://codereview.chromium.org/2124513002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java:87: mWebappInfo.updateShortName(storage.getShortName()); The name should be the one being updated. The short_name is used for the Web APK's app name. https://codereview.chromium.org/2124513002/diff/200001/chrome/browser/android... File chrome/browser/android/webapps/manifest_upgrade_detector.cc (right): https://codereview.chromium.org/2124513002/diff/200001/chrome/browser/android... chrome/browser/android/webapps/manifest_upgrade_detector.cc:72: content::WebContents::FromRenderFrameHost(render_frame_host); Nit: You can get the WebContents via WebContentsObserver::web_contents() https://codereview.chromium.org/2124513002/diff/200001/chrome/browser/android... chrome/browser/android/webapps/manifest_upgrade_detector.cc:77: } You should override WebContentsObserver::DidNavigateMainFrame() and fail if the user navigates outside of the WebAPK's scope (It is possible to navigate outside of the WebAPK scope without bouncing out of the WebAPK) I do not think that it is an issue; is there any weirdness which occurs if the WebContents::GetManifest() request is sent prior to a navigation and the response is received after a navigation? https://codereview.chromium.org/2124513002/diff/200001/chrome/browser/android... chrome/browser/android/webapps/manifest_upgrade_detector.cc:83: ||!banners::AppBannerManager::URLsAreForTheSamePage( Check: web_manifest_url_ != manifest_url instead banners::AppBannerManager::URLsAreForTheSamePage() checks whether the URLs match except for their fragments. Fragment navigations do not make sense in the context of Web App Manifests https://codereview.chromium.org/2124513002/diff/200001/chrome/browser/android... chrome/browser/android/webapps/manifest_upgrade_detector.cc:93: Ignore icon URLs for now too :) https://codereview.chromium.org/2124513002/diff/200001/chrome/browser/android... chrome/browser/android/webapps/manifest_upgrade_detector.cc:109: base::android::ConvertUTF16ToJavaString(env, info.user_title); |java_user_title| is not used? https://codereview.chromium.org/2124513002/diff/220001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetector.java (right): https://codereview.chromium.org/2124513002/diff/220001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetector.java:50: tab.addObserver(this); Adding the observer should be done in start() We should stop observing that tab in onDataAvailable() https://codereview.chromium.org/2124513002/diff/220001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetector.java:100: */ This method does not seem to have any callers https://codereview.chromium.org/2124513002/diff/220001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java (right): https://codereview.chromium.org/2124513002/diff/220001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java:62: // still installed, so re-register it. - I think that registering the WebAPK on the first run should be moved to the ManifestUpgradeDetector.Observer. This puts all of the places which modify WebappDataRegistry together - CRAAAZY IDEA: Does ManifestUpgradeDetector need to call back to WebApkActivity at all? In particular: - Can ManifestUpgradeDetector be renamed to ManifestUpgradeManager - ManifestUpgradeManager#onWebappDataRetrieved() would perform all of the logic currently in the ManifestUpgradeDetector.Observer https://codereview.chromium.org/2124513002/diff/220001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java:188: Nit: Remove this method for now https://codereview.chromium.org/2124513002/diff/220001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java:218: protected void onDestroyInternal() { Nit: Call super.onDestroyInternal()
https://codereview.chromium.org/2124513002/diff/220001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetector.java (right): https://codereview.chromium.org/2124513002/diff/220001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetector.java:164: || mWebappInfo.backgroundColor() != newInfo.backgroundColor(); We need to also rebuild the WebAPK if the scope changes. (Because the scope affects the intent filter in the Android Manifest) ... It would be so much simpler if any change caused us to generate a new WebAPK.
Patchset #4 (id:240001) has been deleted
Hi Peter, PTAL, thanks! https://codereview.chromium.org/2124513002/diff/200001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetector.java (right): https://codereview.chromium.org/2124513002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetector.java:48: public ManifestUpgradeDetector(Tab tab, WebappInfo info) { On 2016/07/13 01:08:19, pkotwicz wrote: > Can the observer be passed into the constructor? Done. https://codereview.chromium.org/2124513002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetector.java:126: mWebappInfo.webManifestUrl()); On 2016/07/13 01:08:19, pkotwicz wrote: > We need to validate that the new WebappInfo is WebAPK-compatible. For instance > the new short_name might be empty. Doing this checking will probably be easier > once https://codereview.chromium.org/2050933002/ lands. I am OK if you file > crbug and put a comment for now. Done. https://codereview.chromium.org/2124513002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetector.java:163: || !mWebappInfo.name().equals(newInfo.name()) On 2016/07/13 01:08:20, pkotwicz wrote: > We should check whether the "short_name" is different not the "name" because it > is the "short_name" which is used for the app's name in the app list Done. https://codereview.chromium.org/2124513002/diff/200001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java (right): https://codereview.chromium.org/2124513002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java:87: mWebappInfo.updateShortName(storage.getShortName()); On 2016/07/12 21:04:28, pkotwicz wrote: > This change should be part of a separate CL which comes prior to this CL Removed. https://codereview.chromium.org/2124513002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java:87: mWebappInfo.updateShortName(storage.getShortName()); On 2016/07/13 01:08:20, pkotwicz wrote: > The name should be the one being updated. The short_name is used for the Web > APK's app name. Will do. https://codereview.chromium.org/2124513002/diff/200001/chrome/browser/android... File chrome/browser/android/webapps/manifest_upgrade_detector.cc (right): https://codereview.chromium.org/2124513002/diff/200001/chrome/browser/android... chrome/browser/android/webapps/manifest_upgrade_detector.cc:72: content::WebContents::FromRenderFrameHost(render_frame_host); On 2016/07/13 01:08:20, pkotwicz wrote: > Nit: You can get the WebContents via WebContentsObserver::web_contents() Thanks! https://codereview.chromium.org/2124513002/diff/200001/chrome/browser/android... chrome/browser/android/webapps/manifest_upgrade_detector.cc:77: } On 2016/07/13 01:08:20, pkotwicz wrote: > You should override WebContentsObserver::DidNavigateMainFrame() and fail if the > user navigates outside of the WebAPK's scope (It is possible to navigate outside > of the WebAPK scope without bouncing out of the WebAPK) > > I do not think that it is an issue; is there any weirdness which occurs if the > WebContents::GetManifest() request is sent prior to a navigation and the > response is received after a navigation? Add the scope to check whether the url loaded is within the WebAPK's scope. As discussed offline, we still use the DidFinishLoad() for now. https://codereview.chromium.org/2124513002/diff/200001/chrome/browser/android... chrome/browser/android/webapps/manifest_upgrade_detector.cc:83: ||!banners::AppBannerManager::URLsAreForTheSamePage( On 2016/07/13 01:08:20, pkotwicz wrote: > Check: > web_manifest_url_ != manifest_url > instead > > banners::AppBannerManager::URLsAreForTheSamePage() checks whether the URLs match > except for their fragments. Fragment navigations do not make sense in the > context of Web App Manifests It makes sense, updated. https://codereview.chromium.org/2124513002/diff/200001/chrome/browser/android... chrome/browser/android/webapps/manifest_upgrade_detector.cc:93: On 2016/07/13 01:08:20, pkotwicz wrote: > Ignore icon URLs for now too :) We could have the icon_urls, since they are obtained in the manifest directly, without any additional fetches. https://codereview.chromium.org/2124513002/diff/200001/chrome/browser/android... chrome/browser/android/webapps/manifest_upgrade_detector.cc:109: base::android::ConvertUTF16ToJavaString(env, info.user_title); On 2016/07/13 01:08:20, pkotwicz wrote: > |java_user_title| is not used? Removed. https://codereview.chromium.org/2124513002/diff/220001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetector.java (right): https://codereview.chromium.org/2124513002/diff/220001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetector.java:50: tab.addObserver(this); On 2016/07/13 01:08:20, pkotwicz wrote: > Adding the observer should be done in start() > We should stop observing that tab in onDataAvailable() Good point, updated. https://codereview.chromium.org/2124513002/diff/220001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetector.java:100: */ On 2016/07/13 01:08:20, pkotwicz wrote: > This method does not seem to have any callers Forgot to remove it when removing instance of ManifestUpgradeDetector from Tab. Removed. https://codereview.chromium.org/2124513002/diff/220001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetector.java:164: || mWebappInfo.backgroundColor() != newInfo.backgroundColor(); On 2016/07/13 03:37:34, pkotwicz wrote: > We need to also rebuild the WebAPK if the scope changes. (Because the scope > affects the intent filter in the Android Manifest) Since scope hasn't been added to the WebappInfo, add a TODO here. > ... It would be so much simpler if any change caused us to generate a new > WebAPK. Hmm, it would be simpler, but I am not sure whether we expect to do that. https://codereview.chromium.org/2124513002/diff/220001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java (right): https://codereview.chromium.org/2124513002/diff/220001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java:62: // still installed, so re-register it. On 2016/07/13 01:08:20, pkotwicz wrote: > - I think that registering the WebAPK on the first run should be moved to the > ManifestUpgradeDetector.Observer. This puts all of the places which modify > WebappDataRegistry together > > - CRAAAZY IDEA: Does ManifestUpgradeDetector need to call back to WebApkActivity > at all? In particular: > - Can ManifestUpgradeDetector be renamed to ManifestUpgradeManager > - ManifestUpgradeManager#onWebappDataRetrieved() would perform all of the > logic currently in the ManifestUpgradeDetector.Observer Moved the logic in the observer to the ManifestUpgradeDetector, and update tests. Also, add a flag to make sure the detector is always running after the WebAPK's registration is done. I would prefer to reserve the name "ManifestUpgradeManager", since we need such a global manager to tell a WebAPK when it should check updates and so on. https://codereview.chromium.org/2124513002/diff/220001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java:188: On 2016/07/13 01:08:20, pkotwicz wrote: > Nit: Remove this method for now Leave the TODO to remind me for the following tasks. https://codereview.chromium.org/2124513002/diff/220001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java:218: protected void onDestroyInternal() { On 2016/07/13 01:08:20, pkotwicz wrote: > Nit: Call super.onDestroyInternal() Done.
https://codereview.chromium.org/2124513002/diff/260001/chrome/browser/android... File chrome/browser/android/webapps/manifest_upgrade_detector.cc (right): https://codereview.chromium.org/2124513002/diff/260001/chrome/browser/android... chrome/browser/android/webapps/manifest_upgrade_detector.cc:13: #include "chrome/browser/banners/app_banner_manager.h" Nit: Remove include :) https://codereview.chromium.org/2124513002/diff/260001/chrome/browser/android... chrome/browser/android/webapps/manifest_upgrade_detector.cc:21: using content::Manifest; In my opinion (others will probably disagree), a high threshold of usefulness must be attained before using the "using" keyword. In functions like ManifestUpgradeDetector::OnDidGetManifest() you are including the "content" namespace. Maybe just remove the "using" statement? https://codereview.chromium.org/2124513002/diff/260001/chrome/browser/android... chrome/browser/android/webapps/manifest_upgrade_detector.cc:23: namespace { Nit: new line https://codereview.chromium.org/2124513002/diff/260001/chrome/browser/android... chrome/browser/android/webapps/manifest_upgrade_detector.cc:24: // Returns whether the given |url| is within the scope of the |scope| url. Super nit: I think that it is clearer if you switch the order of the |scope| and |url| parameters. That way "Is" refers to the first function parameter. https://codereview.chromium.org/2124513002/diff/260001/chrome/browser/android... chrome/browser/android/webapps/manifest_upgrade_detector.cc:29: return res.first == scope_str.end(); Can you use base::StartsWith() ? https://codereview.chromium.org/2124513002/diff/260001/chrome/browser/android... chrome/browser/android/webapps/manifest_upgrade_detector.cc:32: } // namespace Nit: 2 spaces before // https://codereview.chromium.org/2124513002/diff/260001/chrome/browser/android... chrome/browser/android/webapps/manifest_upgrade_detector.cc:86: if (start_ && IsInScope(scope_, validated_url)) { Nit: For the sake of consistency early return if (!started_ || !IsInScope(validated_url, scope_)) return; https://codereview.chromium.org/2124513002/diff/260001/chrome/browser/android... chrome/browser/android/webapps/manifest_upgrade_detector.cc:97: const content::Manifest& manifest) { Can you please add a comment why we are ignoring the manifest if it is empty. i.e. why isn't |started_| set to false on line 98? I think that it is the right thing to do but the reason why should be commented https://codereview.chromium.org/2124513002/diff/260001/chrome/browser/android... chrome/browser/android/webapps/manifest_upgrade_detector.cc:103: ShortcutInfo info(GURL::EmptyGURL()); Can you use GURL() instead of GURL::EmptyGURL() ? https://codereview.chromium.org/2124513002/diff/260001/chrome/browser/android... chrome/browser/android/webapps/manifest_upgrade_detector.cc:146: const JavaParamRef<jobject>& obj) { Nit: Order the functions in this file to match the order in the .h file https://codereview.chromium.org/2124513002/diff/260001/chrome/browser/android... File chrome/browser/android/webapps/manifest_upgrade_detector.h (right): https://codereview.chromium.org/2124513002/diff/260001/chrome/browser/android... chrome/browser/android/webapps/manifest_upgrade_detector.h:3: // found in the LICENSE file. I started putting C++ files in chrome/browser/android/webapk Maybe we should move this file to that folder too. One day we will move the WebAPK Java code to a WebAPK specific folder too https://codereview.chromium.org/2124513002/diff/260001/chrome/browser/android... chrome/browser/android/webapps/manifest_upgrade_detector.h:19: } // namespace content Nit: We do not add comments for forward declarations. A long time ago, I made the mistake of writing a script to "fix" a number of forward declarations by adding a "namespace comment" https://codereview.chromium.org/2124513002/diff/260001/chrome/browser/android... chrome/browser/android/webapps/manifest_upgrade_detector.h:27: // call and MUST BE DESTROYED via Destroy(). Nit: Remove "The object is owned by the Java object." I think that the following sentence says the same thing https://codereview.chromium.org/2124513002/diff/260001/chrome/browser/android... chrome/browser/android/webapps/manifest_upgrade_detector.h:29: public: Can the constructor be private? https://codereview.chromium.org/2124513002/diff/260001/chrome/browser/android... chrome/browser/android/webapps/manifest_upgrade_detector.h:32: content::WebContents* web_contents, Nit: switch the order of |web_manifest_url| and |scope| https://codereview.chromium.org/2124513002/diff/260001/chrome/browser/android... chrome/browser/android/webapps/manifest_upgrade_detector.h:36: // Observes a new WebContents, if necessary. How about: "Replaces the WebContents that is being observed." https://codereview.chromium.org/2124513002/diff/260001/chrome/browser/android... chrome/browser/android/webapps/manifest_upgrade_detector.h:57: // content::WebContentsObserver. Nit: "content::WebContentsObserver." -> "content::WebContentsObserver:" https://codereview.chromium.org/2124513002/diff/260001/chrome/browser/android... chrome/browser/android/webapps/manifest_upgrade_detector.h:66: Nit: |start_| -> |started_|
Mostly nits https://codereview.chromium.org/2124513002/diff/200001/chrome/browser/android... File chrome/browser/android/webapps/manifest_upgrade_detector.cc (right): https://codereview.chromium.org/2124513002/diff/200001/chrome/browser/android... chrome/browser/android/webapps/manifest_upgrade_detector.cc:93: I am suggesting skipping the icons for now because I am not yet convinced that we will make use of multiple icon URLs in ManifestUpgradeDetector We need to see how confusing things are once we introduce the bitmap checking code https://codereview.chromium.org/2124513002/diff/280001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetector.java (right): https://codereview.chromium.org/2124513002/diff/280001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetector.java:39: if (webManifestUrl == null || webManifestUrl.isEmpty()) return; I think that this logic should be in start() instead of in the constructor. Yes that means that native will be initialized in the unlikely case the "manifest URL" is missing from the Android manifest. It just feels weird for the constructor to have an early return in it Nit: You can use TextUtils#isEmpty() to check whether the string is null or empty in just one function call https://codereview.chromium.org/2124513002/diff/280001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetector.java:96: // WebAPK-compatible. Nit: Validates -> Validate https://codereview.chromium.org/2124513002/diff/280001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetector.java:97: mTab.removeObserver(this); Nit: Move this call before the comment. https://codereview.chromium.org/2124513002/diff/280001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetector.java:99: null, name, shortName, displayMode, orientation, mWebappInfo.source(), Nit: mWebappInfo.icon() instead of null? https://codereview.chromium.org/2124513002/diff/280001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetector.java:137: return !mWebappInfo.uri().toString().equals(startUrl) Notes: - Can't you get the "startUrl" from |newInfo| too? - If a user launches a WebAPK by tapping a link which falls within the WebAPK's scope in Chrome, |mWebappInfo|'s uri will be different than the start URL in the Android manifest. - You should include a comment for each property as to why a change in the property requires an upgrade. For instance for "short_name", "Used as the WebAPK's name in the Android app list" https://codereview.chromium.org/2124513002/diff/280001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetector.java:155: final Set<String> iconUrls, WebappInfo newInfo) { Nit: Introduce a dedicated function which gets called when an upgrade is required. Call the method ManifestUpgradeDetector#upgrade() perhaps? https://codereview.chromium.org/2124513002/diff/280001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java (right): https://codereview.chromium.org/2124513002/diff/280001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java:31: private static final String TAG = "cr_WebApkActivity"; |mManifestUpgradeDetector| needs a comment https://codereview.chromium.org/2124513002/diff/280001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java:72: mIsWebApkRegistered = true; Shouldn't you call checkUpdates() sometime after setting mIsWebApkRegistered to true in case the initial call failed? https://codereview.chromium.org/2124513002/diff/280001/chrome/android/webapk/... File chrome/android/webapk/libs/runtime_library/src/org/chromium/webapk/lib/runtime_library/HostBrowserLauncher.java (right): https://codereview.chromium.org/2124513002/diff/280001/chrome/android/webapk/... chrome/android/webapk/libs/runtime_library/src/org/chromium/webapk/lib/runtime_library/HostBrowserLauncher.java:140: if (value == null || !value.endsWith("L")) { Why this change? Shouldn't the colors always be specified in the Android manifest? If you want to make this change, you should rename this function to getColorFromBundle()
Patchset #6 (id:300001) has been deleted
Patchset #6 (id:320001) has been deleted
Description was changed from ========== Introduce ManifestUpgradeDetector for WebAPK to detect web manifest changes. Some attributes changes in web manifest file will require to re-mint a WebAPK. For example, start Url, background color and Webapp Name. These infos are baked into WebAPK's AndroidManifest.xml. Other attributes, like orientaion etc., their changes are stored in Chrome's metadata, but don't request to re-mint. The ManifestUpgradeDetector fetches the latest web manifest for a WebAPK, compare with cached one and make a decision of whether to request re-mint. The ManifestUpgradeDetector is initilized for WebApkActivity when it needs to check resources changes. Once the detector starts, it observes WebContents's change, and stops when the manifest file of the WebApk is found and fetched. This is because not all of the urls within the WebAPK's scope link to its manifest file. Therefore, the detector needs to follow WebContents's change until find the manifest file. This CL depends on CL(https://codereview.chromium.org/2140273002/). TEST=org.chromium.chrome.browser.webapps.ManifestUpgradeDetectorTest BUG=624834 ========== to ========== Introduce ManifestUpgradeDetector for WebAPK to detect web manifest changes. Some attributes changes in web manifest file will require to re-mint a WebAPK. For example, start Url, background color and Webapp Name. These infos are baked into WebAPK's AndroidManifest.xml. Other attributes, like orientaion etc., their changes are stored in Chrome's metadata, but don't request to re-mint. The ManifestUpgradeDetector fetches the latest web manifest for a WebAPK, compare with cached one and make a decision of whether to request re-mint. The ManifestUpgradeDetector is initilized for WebApkActivity when it needs to check resources changes. Once the detector starts, it observes WebContents's change, and stops when the manifest file of the WebApk is found and fetched. This is because not all of the urls within the WebAPK's scope link to its manifest file. Therefore, the detector needs to follow WebContents's change until find the manifest file. This CL depends on CL(https://codereview.chromium.org/2140273002/) and CL(https://codereview.chromium.org/2147323002/). TEST=org.chromium.chrome.browser.webapps.ManifestUpgradeDetectorTest BUG=624834 ==========
Patchset #6 (id:340001) has been deleted
Hi Peter, I think I addressed all of your comments, PTAL, thanks! https://codereview.chromium.org/2124513002/diff/200001/chrome/browser/android... File chrome/browser/android/webapps/manifest_upgrade_detector.cc (right): https://codereview.chromium.org/2124513002/diff/200001/chrome/browser/android... chrome/browser/android/webapps/manifest_upgrade_detector.cc:93: On 2016/07/13 21:40:38, pkotwicz wrote: > I am suggesting skipping the icons for now because I am not yet convinced that > we will make use of multiple icon URLs in ManifestUpgradeDetector > > We need to see how confusing things are once we introduce the bitmap checking > code No matter how we treat them later, these urls are something that we can get directly from Web Manifest. Their changes most likely mean the images are updated. I still prefer to keep them for now. https://codereview.chromium.org/2124513002/diff/260001/chrome/browser/android... File chrome/browser/android/webapps/manifest_upgrade_detector.cc (right): https://codereview.chromium.org/2124513002/diff/260001/chrome/browser/android... chrome/browser/android/webapps/manifest_upgrade_detector.cc:13: #include "chrome/browser/banners/app_banner_manager.h" On 2016/07/13 21:01:15, pkotwicz wrote: > Nit: Remove include :) Done. https://codereview.chromium.org/2124513002/diff/260001/chrome/browser/android... chrome/browser/android/webapps/manifest_upgrade_detector.cc:21: using content::Manifest; On 2016/07/13 21:01:15, pkotwicz wrote: > In my opinion (others will probably disagree), a high threshold of usefulness > must be attained before using the "using" keyword. > > In functions like ManifestUpgradeDetector::OnDidGetManifest() you are including > the "content" namespace. Maybe just remove the "using" statement? Sure, just forgot to remove it when some functions that used it got removed. https://codereview.chromium.org/2124513002/diff/260001/chrome/browser/android... chrome/browser/android/webapps/manifest_upgrade_detector.cc:23: namespace { On 2016/07/13 21:01:15, pkotwicz wrote: > Nit: new line Done. https://codereview.chromium.org/2124513002/diff/260001/chrome/browser/android... chrome/browser/android/webapps/manifest_upgrade_detector.cc:24: // Returns whether the given |url| is within the scope of the |scope| url. On 2016/07/13 21:01:15, pkotwicz wrote: > Super nit: I think that it is clearer if you switch the order of the |scope| and > |url| parameters. That way "Is" refers to the first function parameter. Done. https://codereview.chromium.org/2124513002/diff/260001/chrome/browser/android... chrome/browser/android/webapps/manifest_upgrade_detector.cc:29: return res.first == scope_str.end(); On 2016/07/13 21:01:15, pkotwicz wrote: > Can you use base::StartsWith() ? Have been looking for this function but haven't found. Updated. https://codereview.chromium.org/2124513002/diff/260001/chrome/browser/android... chrome/browser/android/webapps/manifest_upgrade_detector.cc:32: } // namespace On 2016/07/13 21:01:15, pkotwicz wrote: > Nit: 2 spaces before // Done. https://codereview.chromium.org/2124513002/diff/260001/chrome/browser/android... chrome/browser/android/webapps/manifest_upgrade_detector.cc:86: if (start_ && IsInScope(scope_, validated_url)) { On 2016/07/13 21:01:15, pkotwicz wrote: > Nit: For the sake of consistency early return > > if (!started_ || !IsInScope(validated_url, scope_)) > return; Done. https://codereview.chromium.org/2124513002/diff/260001/chrome/browser/android... chrome/browser/android/webapps/manifest_upgrade_detector.cc:97: const content::Manifest& manifest) { On 2016/07/13 21:01:15, pkotwicz wrote: > Can you please add a comment why we are ignoring the manifest if it is empty. > i.e. why isn't |started_| set to false on line 98? I think that it is the right > thing to do but the reason why should be commented Done. https://codereview.chromium.org/2124513002/diff/260001/chrome/browser/android... chrome/browser/android/webapps/manifest_upgrade_detector.cc:103: ShortcutInfo info(GURL::EmptyGURL()); On 2016/07/13 21:01:15, pkotwicz wrote: > Can you use GURL() instead of GURL::EmptyGURL() ? No, we will end up with compiler error, since the constructor of ShortcutInfo needs a const GURL&. https://codereview.chromium.org/2124513002/diff/260001/chrome/browser/android... chrome/browser/android/webapps/manifest_upgrade_detector.cc:146: const JavaParamRef<jobject>& obj) { On 2016/07/13 21:01:15, pkotwicz wrote: > Nit: Order the functions in this file to match the order in the .h file Done. https://codereview.chromium.org/2124513002/diff/260001/chrome/browser/android... File chrome/browser/android/webapps/manifest_upgrade_detector.h (right): https://codereview.chromium.org/2124513002/diff/260001/chrome/browser/android... chrome/browser/android/webapps/manifest_upgrade_detector.h:3: // found in the LICENSE file. On 2016/07/13 21:01:16, pkotwicz wrote: > I started putting C++ files in chrome/browser/android/webapk Maybe we should > move this file to that folder too. > > One day we will move the WebAPK Java code to a WebAPK specific folder too Missed this comment last time, moved now. https://codereview.chromium.org/2124513002/diff/260001/chrome/browser/android... chrome/browser/android/webapps/manifest_upgrade_detector.h:19: } // namespace content On 2016/07/13 21:01:16, pkotwicz wrote: > Nit: We do not add comments for forward declarations. A long time ago, I made > the mistake of writing a script to "fix" a number of forward declarations by > adding a "namespace comment" Done. https://codereview.chromium.org/2124513002/diff/260001/chrome/browser/android... chrome/browser/android/webapps/manifest_upgrade_detector.h:27: // call and MUST BE DESTROYED via Destroy(). On 2016/07/13 21:01:16, pkotwicz wrote: > Nit: Remove "The object is owned by the Java object." > > I think that the following sentence says the same thing Done. https://codereview.chromium.org/2124513002/diff/260001/chrome/browser/android... chrome/browser/android/webapps/manifest_upgrade_detector.h:29: public: On 2016/07/13 21:01:15, pkotwicz wrote: > Can the constructor be private? It can't, because it is called by the jni function to initialize the c++ object. https://codereview.chromium.org/2124513002/diff/260001/chrome/browser/android... chrome/browser/android/webapps/manifest_upgrade_detector.h:32: content::WebContents* web_contents, On 2016/07/13 21:01:16, pkotwicz wrote: > Nit: switch the order of |web_manifest_url| and |scope| As discussed offline, keep this order for now:) https://codereview.chromium.org/2124513002/diff/260001/chrome/browser/android... chrome/browser/android/webapps/manifest_upgrade_detector.h:36: // Observes a new WebContents, if necessary. On 2016/07/13 21:01:16, pkotwicz wrote: > How about: "Replaces the WebContents that is being observed." Done. https://codereview.chromium.org/2124513002/diff/260001/chrome/browser/android... chrome/browser/android/webapps/manifest_upgrade_detector.h:57: // content::WebContentsObserver. On 2016/07/13 21:01:15, pkotwicz wrote: > Nit: "content::WebContentsObserver." -> "content::WebContentsObserver:" Done. https://codereview.chromium.org/2124513002/diff/260001/chrome/browser/android... chrome/browser/android/webapps/manifest_upgrade_detector.h:66: On 2016/07/13 21:01:16, pkotwicz wrote: > Nit: |start_| -> |started_| Done. https://codereview.chromium.org/2124513002/diff/280001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetector.java (right): https://codereview.chromium.org/2124513002/diff/280001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetector.java:39: if (webManifestUrl == null || webManifestUrl.isEmpty()) return; On 2016/07/13 21:40:38, pkotwicz wrote: > I think that this logic should be in start() instead of in the constructor. > > Yes that means that native will be initialized in the unlikely case the > "manifest URL" is missing from the Android manifest. It just feels weird for the > constructor to have an early return in it > > Nit: You can use TextUtils#isEmpty() to check whether the string is null or > empty in just one function call That is a good point, I moved all the following logic into start(). So we won't initialize a native object with an empty manifest. https://codereview.chromium.org/2124513002/diff/280001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetector.java:96: // WebAPK-compatible. On 2016/07/13 21:40:38, pkotwicz wrote: > Nit: Validates -> Validate Done. https://codereview.chromium.org/2124513002/diff/280001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetector.java:97: mTab.removeObserver(this); On 2016/07/13 21:40:38, pkotwicz wrote: > Nit: Move this call before the comment. Done. https://codereview.chromium.org/2124513002/diff/280001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetector.java:99: null, name, shortName, displayMode, orientation, mWebappInfo.source(), On 2016/07/13 21:40:39, pkotwicz wrote: > Nit: mWebappInfo.icon() instead of null? Done. https://codereview.chromium.org/2124513002/diff/280001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetector.java:137: return !mWebappInfo.uri().toString().equals(startUrl) On 2016/07/13 21:40:38, pkotwicz wrote: > Notes: > - Can't you get the "startUrl" from |newInfo| too? > - If a user launches a WebAPK by tapping a link which falls within the WebAPK's > scope in Chrome, |mWebappInfo|'s uri will be different than the start URL in the > Android manifest. I intentionally do not include the start_url into this CL, but since you point it out, I added it in WebappInfo and compare the real start_url with the new fetched one. > - You should include a comment for each property as to why a change in the > property requires an upgrade. For instance for "short_name", "Used as the > WebAPK's name in the Android app list" Done. https://codereview.chromium.org/2124513002/diff/280001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetector.java:155: final Set<String> iconUrls, WebappInfo newInfo) { On 2016/07/13 21:40:38, pkotwicz wrote: > Nit: Introduce a dedicated function which gets called when an upgrade is > required. Call the method ManifestUpgradeDetector#upgrade() perhaps? Done. https://codereview.chromium.org/2124513002/diff/280001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java (right): https://codereview.chromium.org/2124513002/diff/280001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java:31: private static final String TAG = "cr_WebApkActivity"; On 2016/07/13 21:40:39, pkotwicz wrote: > |mManifestUpgradeDetector| needs a comment Done. https://codereview.chromium.org/2124513002/diff/280001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java:72: mIsWebApkRegistered = true; On 2016/07/13 21:40:39, pkotwicz wrote: > Shouldn't you call checkUpdates() sometime after setting mIsWebApkRegistered to > true in case the initial call failed? Removed this flag, and calls registerWebapp() when storage is null in ManifestUpgradeDetector#onWebappDataRetrieved(). https://codereview.chromium.org/2124513002/diff/280001/chrome/android/webapk/... File chrome/android/webapk/libs/runtime_library/src/org/chromium/webapk/lib/runtime_library/HostBrowserLauncher.java (right): https://codereview.chromium.org/2124513002/diff/280001/chrome/android/webapk/... chrome/android/webapk/libs/runtime_library/src/org/chromium/webapk/lib/runtime_library/HostBrowserLauncher.java:140: if (value == null || !value.endsWith("L")) { On 2016/07/13 21:40:39, pkotwicz wrote: > Why this change? Shouldn't the colors always be specified in the Android > manifest? If you want to make this change, you should rename this function to > getColorFromBundle() Good catch. In case the color is missing, we'd better to return the default value for a missing color. Renamed this function to getColorFromBundle() and updated its description.
Hi Peter, as discussed offline, I removed the iconUrls check. PTAL, thanks!
https://codereview.chromium.org/2124513002/diff/260001/chrome/browser/android... File chrome/browser/android/webapps/manifest_upgrade_detector.cc (right): https://codereview.chromium.org/2124513002/diff/260001/chrome/browser/android... chrome/browser/android/webapps/manifest_upgrade_detector.cc:103: ShortcutInfo info(GURL::EmptyGURL()); I am willing to bet that we will not have a compiler error. Nothing special needs to be done to pass variables by reference. This is valid: void main() { foo(GURL()); } void foo(const GURL& url) { ... } This is a bug: std::pair<const GURL&, int> pair_; void bar() { pair_ = std::make_pair(GURL(), 0); } This is also a bug: const GURL& rod() { return GURL(); } https://codereview.chromium.org/2124513002/diff/260001/chrome/browser/android... File chrome/browser/android/webapps/manifest_upgrade_detector.h (right): https://codereview.chromium.org/2124513002/diff/260001/chrome/browser/android... chrome/browser/android/webapps/manifest_upgrade_detector.h:29: public: Yes, I see now. I thought that Initialize() was a static member function of ManifestUpgradeDetector but it's not. https://codereview.chromium.org/2124513002/diff/360001/chrome/browser/android... File chrome/browser/android/webapk/manifest_upgrade_detector.h (right): https://codereview.chromium.org/2124513002/diff/360001/chrome/browser/android... chrome/browser/android/webapk/manifest_upgrade_detector.h:26: // from there via a JNI (Initialize) call and MUST BE DESTROYED via Destroy(). Nit: "from there via a JNI" -> "via a JNI" https://codereview.chromium.org/2124513002/diff/380001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetector.java (right): https://codereview.chromium.org/2124513002/diff/380001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetector.java:18: * This class interacts with c++ to detect whether resources in web manifest of a WebAPK has been Nits: - "whether resources in web manifest" -> "whether a resource in the web manifest" - "c++" -> C++ (⏫ Uppercase) https://codereview.chromium.org/2124513002/diff/380001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetector.java:30: private boolean mIsInitialized; Given how you are using |mIsInitialized| I think that renaming it to |mWasStarted| makes sense https://codereview.chromium.org/2124513002/diff/380001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetector.java:37: mContext = context; Nit: Use ContextUtils.getApplicationContext() instead of passing in context I don't mind caching the context in |mContext| put please be consistent on whether you use |mContext| or ContextUtils.getApplicationContext() https://codereview.chromium.org/2124513002/diff/380001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetector.java:42: * fetching pipeline. How about: "Starts fetching the web manifest resources." I think that the specifics of how this is implemented in native is an implementation detail https://codereview.chromium.org/2124513002/diff/380001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetector.java:80: private void updatePointers(Tab tab) { Nit: Use |mTab| and remove |tab| parameter https://codereview.chromium.org/2124513002/diff/380001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetector.java:99: if (requireUpgrade(newInfo)) { Can you do: if (requireUpgrade(newInfo)) { upgrade(); } updateWebappDataStorage(); If you want, you can call updateWebappDataStorage() from upgrade() https://codereview.chromium.org/2124513002/diff/380001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetector.java:102: } Can you do this from updateWebappDataStorage() instead of from here. (I think that this saves a few lines of code) https://codereview.chromium.org/2124513002/diff/380001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetector.java:115: private void onWebappDataRetrieved(WebappDataStorage storage, final WebappInfo newInfo) { I am confused why both ManifestUpgradeDetector#onWebappDataRetrieved() and WebApkActivity#onDataStorageFetched() call WebappRegistry#registerWebapp() https://codereview.chromium.org/2124513002/diff/380001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetector.java:145: // log to find such cases. Nit: Sometime -> Sometimes https://codereview.chromium.org/2124513002/diff/380001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetector.java:146: boolean scopeMatch = mWebappInfo.scopeUri().compareTo(newInfo.scopeUri()) == 0; Sorry, I should have been clearer. I meant adding comments in line with the code. How about: "Scope is used by WebAPK's intent filter. It determines which URLs are opened by the WebAPK instead of Chrome." https://codereview.chromium.org/2124513002/diff/380001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetector.java:152: } How about: // Start URL is the initial URL when the WebAPK is launched. A change in the start URL requires the WebAPK to be re-installed because the initial URL starts loading prior to the WebAPK's WebappDataStorage being fetched. if (!mWebappInfo.startUri().equals(newInfo.startUri())) { return true; } // Short name is used as the WebAPK's name in the app list. if (!mWebappInfo.shortName().equals(newInfo.shortName())) { return true; } // Background color is the splash screen's background color. A change in the background color requires the WebAPK to be re-installed because a background-color-only splash screen is displayed prior to the WebAPK's WebappDataStorage being fetched. There can be a several second delay between the background-color-only splash screen being displayed and the WebAPK's WebappDataStorage being fetched. if (mWebappInfo.backgroundColor() != newInfo.backgroundColor()) { return true; } Comments: - In my version I explain why an upgrade is necessary for each property which triggers an upgrade - Using the "start_url" as the "scope" if the "scope" is empty is not interesting in the context of this function. The fallback behavior is needed to handle home screen webapp shortcuts which were added a long time ago. https://codereview.chromium.org/2124513002/diff/380001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetector.java:167: // updates before updating Chrome's sharedPreference. I do not think that it matters whether WebappDataStorage is updated prior to to requesting the upgrade from the WebAPK server or after requesting the upgrade. The WebAPK server will treat an update as a request to install a new WebAPK. We will pass the WebAPK package name to the server so that server knows which WebAPK it is updating. I am leaning towards requiring WebappDataStorage to be updated prior to requesting the update from the WebAPK server just because it makes things easier to think about. (Request WebAppDataStorage -> WebAppDataStorage response -> Update WebappDataStorage -> Request Upgrade) https://codereview.chromium.org/2124513002/diff/380001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java (right): https://codereview.chromium.org/2124513002/diff/380001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java:66: storage.updateFromShortcutIntent(getIntent(), true); Why is true passed here? If this is the first time that WebappDataStorage#updateFromShortcutIntent() is called won't the version be 0? https://codereview.chromium.org/2124513002/diff/380001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java (right): https://codereview.chromium.org/2124513002/diff/380001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java:373: editor.putBoolean(KEY_IS_ICON_GENERATED, info.isIconGenerated()); Why are EXTRA_WEBAPK_PACKAGE_NAME and EXTRA_SOURCE not set? They are also part of WebappInfo. I wonder whether calling "put" is expensive. I don't why there's the early return in updateFromShortcutIntent(). Perhaps KEY_ICON is the only expensive one? https://codereview.chromium.org/2124513002/diff/380001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappInfo.java (right): https://codereview.chromium.org/2124513002/diff/380001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappInfo.java:127: String startUrl = IntentUtils.safeGetStringExtra(intent, WebApkConstants.EXTRA_START_URL); Aside: In a separate CL I am going to investigate not adding startUrl to WebAppInfo and instead read all of the properties missing from WebappInfo from the meta bundle directly in ManifestUpgradeDetector. It seems kind of a waste to plumb properties through 3 - 4 classes when only ManifestUpgradeDetector needs them https://codereview.chromium.org/2124513002/diff/380001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappInfo.java:158: * @param startUrl Start URL of the WebAPK associated with the webapp. Nit: |startUrl| should be before |scope| to match order of arguments in function https://codereview.chromium.org/2124513002/diff/380001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappInfo.java:356: } Don't you need to put WebApkConstants.EXTRA_START_URL here? https://codereview.chromium.org/2124513002/diff/380001/chrome/android/webapk/... File chrome/android/webapk/libs/runtime_library/src/org/chromium/webapk/lib/runtime_library/HostBrowserLauncher.java (right): https://codereview.chromium.org/2124513002/diff/380001/chrome/android/webapk/... chrome/android/webapk/libs/runtime_library/src/org/chromium/webapk/lib/runtime_library/HostBrowserLauncher.java:147: return ((long) Integer.MAX_VALUE) + 1; Nit: This should be a constant https://codereview.chromium.org/2124513002/diff/380001/chrome/browser/android... File chrome/browser/android/webapk/manifest_upgrade_detector.cc (right): https://codereview.chromium.org/2124513002/diff/380001/chrome/browser/android... chrome/browser/android/webapk/manifest_upgrade_detector.cc:12: #include "content/public/browser/browser_thread.h" This include does not seem to be used https://codereview.chromium.org/2124513002/diff/380001/chrome/browser/android... chrome/browser/android/webapk/manifest_upgrade_detector.cc:82: const JavaParamRef<jobject>& obj) { Nit: indentation on line 82 looks wrong If you don't use it already clank-format makes fixing indentation really easy (https://chromium.googlesource.com/chromium/src/+/master/docs/clang_format.md) https://codereview.chromium.org/2124513002/diff/380001/chrome/browser/android... chrome/browser/android/webapk/manifest_upgrade_detector.cc:107: // links to the Web Manifest that we are looking for. You should also address why we continue fetching when the Web Manifest is different. You should ask Yaron for a good comment explaining why. (I do not have any suggestions because I do not understand why). I think that we decided a long time ago that it is invalid for a web developer to change a WebAPK's Web Manifest location (If a web developer changes the Web Manifest location we will continue using the old Web Manifest) but that it is valid for a web developer to change a WebAPK's start URL in the Web Manifest.
pkotwicz@chromium.org changed reviewers: + yfriedman@chromium.org
Adding Yaron to the CL in case that he has a completely different opinion on "what is the right architecture" than I do
Patchset #9 (id:420001) has been deleted
Patchset #8 (id:400001) has been deleted
Hi Peter, PTAL, thanks! https://codereview.chromium.org/2124513002/diff/260001/chrome/browser/android... File chrome/browser/android/webapps/manifest_upgrade_detector.cc (right): https://codereview.chromium.org/2124513002/diff/260001/chrome/browser/android... chrome/browser/android/webapps/manifest_upgrade_detector.cc:103: ShortcutInfo info(GURL::EmptyGURL()); On 2016/07/16 00:56:45, pkotwicz wrote: > I am willing to bet that we will not have a compiler error. Nothing special > needs to be done to pass variables by reference. > > This is valid: > void main() { > foo(GURL()); > } > > void foo(const GURL& url) { > ... > } > > This is a bug: > std::pair<const GURL&, int> pair_; > void bar() { > pair_ = std::make_pair(GURL(), 0); > } > > This is also a bug: > const GURL& rod() { > return GURL(); > } When passing a reference, the variable should be something reachable when the function is done. It cannot be just an arbitrary variable within the function. This is my understanding. https://codereview.chromium.org/2124513002/diff/260001/chrome/browser/android... File chrome/browser/android/webapps/manifest_upgrade_detector.h (right): https://codereview.chromium.org/2124513002/diff/260001/chrome/browser/android... chrome/browser/android/webapps/manifest_upgrade_detector.h:29: public: On 2016/07/16 00:56:45, pkotwicz wrote: > Yes, I see now. I thought that Initialize() was a static member function of > ManifestUpgradeDetector but it's not. Acknowledged. https://codereview.chromium.org/2124513002/diff/360001/chrome/browser/android... File chrome/browser/android/webapk/manifest_upgrade_detector.h (right): https://codereview.chromium.org/2124513002/diff/360001/chrome/browser/android... chrome/browser/android/webapk/manifest_upgrade_detector.h:26: // from there via a JNI (Initialize) call and MUST BE DESTROYED via Destroy(). On 2016/07/16 00:56:45, pkotwicz wrote: > Nit: "from there via a JNI" -> "via a JNI" Done. https://codereview.chromium.org/2124513002/diff/380001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetector.java (right): https://codereview.chromium.org/2124513002/diff/380001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetector.java:18: * This class interacts with c++ to detect whether resources in web manifest of a WebAPK has been On 2016/07/16 00:56:45, pkotwicz wrote: > Nits: > - "whether resources in web manifest" -> "whether a resource in the web > manifest" > - "c++" -> C++ (⏫ Uppercase) Done. https://codereview.chromium.org/2124513002/diff/380001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetector.java:30: private boolean mIsInitialized; On 2016/07/16 00:56:46, pkotwicz wrote: > Given how you are using |mIsInitialized| I think that renaming it to > |mWasStarted| makes sense Done. https://codereview.chromium.org/2124513002/diff/380001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetector.java:37: mContext = context; On 2016/07/16 00:56:45, pkotwicz wrote: > Nit: Use ContextUtils.getApplicationContext() instead of passing in context > > I don't mind caching the context in |mContext| put please be consistent on > whether you use |mContext| or ContextUtils.getApplicationContext() Done. https://codereview.chromium.org/2124513002/diff/380001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetector.java:42: * fetching pipeline. On 2016/07/16 00:56:46, pkotwicz wrote: > How about: "Starts fetching the web manifest resources." > > I think that the specifics of how this is implemented in native is an > implementation detail Done. https://codereview.chromium.org/2124513002/diff/380001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetector.java:80: private void updatePointers(Tab tab) { On 2016/07/16 00:56:45, pkotwicz wrote: > Nit: Use |mTab| and remove |tab| parameter Done. https://codereview.chromium.org/2124513002/diff/380001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetector.java:99: if (requireUpgrade(newInfo)) { On 2016/07/16 00:56:45, pkotwicz wrote: > Can you do: > > if (requireUpgrade(newInfo)) { > upgrade(); > } > updateWebappDataStorage(); > > If you want, you can call updateWebappDataStorage() from upgrade() The logic above is based on an assumption that we never compare any new fetched things with the ones within the SharedPreference. Since currently this seems right, and the above logic is simpler, updated to the suggested ones. https://codereview.chromium.org/2124513002/diff/380001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetector.java:102: } On 2016/07/16 00:56:46, pkotwicz wrote: > Can you do this from updateWebappDataStorage() instead of from here. (I think > that this saves a few lines of code) Done. https://codereview.chromium.org/2124513002/diff/380001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetector.java:115: private void onWebappDataRetrieved(WebappDataStorage storage, final WebappInfo newInfo) { On 2016/07/16 00:56:46, pkotwicz wrote: > I am confused why both ManifestUpgradeDetector#onWebappDataRetrieved() and > WebApkActivity#onDataStorageFetched() call WebappRegistry#registerWebapp() This is How I removed the flag mIsWebApkRegistered. This guarantees that we will call WebApkActivity#checkUpdates() even when the callback of registerWebapp() failed in WebApkActivity#onDataStorageFetched(). https://codereview.chromium.org/2124513002/diff/380001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetector.java:145: // log to find such cases. On 2016/07/16 00:56:45, pkotwicz wrote: > Nit: Sometime -> Sometimes Done. https://codereview.chromium.org/2124513002/diff/380001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetector.java:146: boolean scopeMatch = mWebappInfo.scopeUri().compareTo(newInfo.scopeUri()) == 0; On 2016/07/16 00:56:45, pkotwicz wrote: > Sorry, I should have been clearer. I meant adding comments in line with the > code. > > How about: "Scope is used by WebAPK's intent filter. It determines which URLs > are opened by the WebAPK instead of Chrome." Done. https://codereview.chromium.org/2124513002/diff/380001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetector.java:152: } On 2016/07/16 00:56:46, pkotwicz wrote: > How about: > > // Start URL is the initial URL when the WebAPK is launched. A change in the > start URL requires the WebAPK to be re-installed because the initial URL starts > loading prior to the WebAPK's WebappDataStorage being fetched. > if (!mWebappInfo.startUri().equals(newInfo.startUri())) { > return true; > } > > // Short name is used as the WebAPK's name in the app list. > if (!mWebappInfo.shortName().equals(newInfo.shortName())) { > return true; > } > > // Background color is the splash screen's background color. A change in the > background color requires the WebAPK to be re-installed because a > background-color-only splash screen is displayed prior to the WebAPK's > WebappDataStorage being fetched. There can be a several second delay between the > background-color-only splash screen being displayed and the WebAPK's > WebappDataStorage being fetched. > if (mWebappInfo.backgroundColor() != newInfo.backgroundColor()) { > return true; > } > > Comments: > - In my version I explain why an upgrade is necessary for each property which > triggers an upgrade > - Using the "start_url" as the "scope" if the "scope" is empty is not > interesting in the context of this function. The fallback behavior is needed to > handle home screen webapp shortcuts which were added a long time ago. Thanks Peter, your explanations does sound better. Updated. https://codereview.chromium.org/2124513002/diff/380001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetector.java:167: // updates before updating Chrome's sharedPreference. On 2016/07/16 00:56:46, pkotwicz wrote: > I do not think that it matters whether WebappDataStorage is updated prior to to > requesting the upgrade from the WebAPK server or after requesting the upgrade. > The WebAPK server will treat an update as a request to install a new WebAPK. We > will pass the WebAPK package name to the server so that server knows which > WebAPK it is updating. I am leaning towards requiring WebappDataStorage to be > updated prior to requesting the update from the WebAPK server just because it > makes things easier to think about. (Request WebAppDataStorage -> > WebAppDataStorage response -> Update WebappDataStorage -> Request Upgrade) I talked to Scott and the server side also does upgrade check based on the info that the client provides. So the client is supposed to send the current values of these properties to the server. It is ok to update the SharedPreferences before sending out the upgrade request, but we have to cache the old values before they are gone. https://codereview.chromium.org/2124513002/diff/380001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java (right): https://codereview.chromium.org/2124513002/diff/380001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java:66: storage.updateFromShortcutIntent(getIntent(), true); On 2016/07/16 00:56:46, pkotwicz wrote: > Why is true passed here? If this is the first time that > WebappDataStorage#updateFromShortcutIntent() is called won't the version be 0? Yes, but I'd prefer to pass "true" for webapk. https://codereview.chromium.org/2124513002/diff/380001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java (right): https://codereview.chromium.org/2124513002/diff/380001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java:373: editor.putBoolean(KEY_IS_ICON_GENERATED, info.isIconGenerated()); On 2016/07/16 00:56:46, pkotwicz wrote: > Why are EXTRA_WEBAPK_PACKAGE_NAME and EXTRA_SOURCE not set? They are also part > of WebappInfo. > > I wonder whether calling "put" is expensive. I don't why there's the early > return in updateFromShortcutIntent(). Perhaps KEY_ICON is the only expensive > one? Added back, we do need to update them when registerWebapp() is called and updates the sharedPreference from webappInfo. https://codereview.chromium.org/2124513002/diff/380001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappInfo.java (right): https://codereview.chromium.org/2124513002/diff/380001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappInfo.java:158: * @param startUrl Start URL of the WebAPK associated with the webapp. On 2016/07/16 00:56:46, pkotwicz wrote: > Nit: |startUrl| should be before |scope| to match order of arguments in function Done. https://codereview.chromium.org/2124513002/diff/380001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappInfo.java:356: } On 2016/07/16 00:56:46, pkotwicz wrote: > Don't you need to put WebApkConstants.EXTRA_START_URL here? Done. https://codereview.chromium.org/2124513002/diff/380001/chrome/android/webapk/... File chrome/android/webapk/libs/runtime_library/src/org/chromium/webapk/lib/runtime_library/HostBrowserLauncher.java (right): https://codereview.chromium.org/2124513002/diff/380001/chrome/android/webapk/... chrome/android/webapk/libs/runtime_library/src/org/chromium/webapk/lib/runtime_library/HostBrowserLauncher.java:147: return ((long) Integer.MAX_VALUE) + 1; On 2016/07/16 00:56:46, pkotwicz wrote: > Nit: This should be a constant Done. https://codereview.chromium.org/2124513002/diff/380001/chrome/browser/android... File chrome/browser/android/webapk/manifest_upgrade_detector.cc (right): https://codereview.chromium.org/2124513002/diff/380001/chrome/browser/android... chrome/browser/android/webapk/manifest_upgrade_detector.cc:12: #include "content/public/browser/browser_thread.h" On 2016/07/16 00:56:46, pkotwicz wrote: > This include does not seem to be used Done. https://codereview.chromium.org/2124513002/diff/380001/chrome/browser/android... chrome/browser/android/webapk/manifest_upgrade_detector.cc:82: const JavaParamRef<jobject>& obj) { On 2016/07/16 00:56:46, pkotwicz wrote: > Nit: indentation on line 82 looks wrong > > If you don't use it already clank-format makes fixing indentation really easy > (https://chromium.googlesource.com/chromium/src/+/master/docs/clang_format.md) Done. https://codereview.chromium.org/2124513002/diff/380001/chrome/browser/android... chrome/browser/android/webapk/manifest_upgrade_detector.cc:107: // links to the Web Manifest that we are looking for. On 2016/07/16 00:56:46, pkotwicz wrote: > You should also address why we continue fetching when the Web Manifest is > different. > > You should ask Yaron for a good comment explaining why. (I do not have any > suggestions because I do not understand why). I think that we decided a long > time ago that it is invalid for a web developer to change a WebAPK's Web > Manifest location (If a web developer changes the Web Manifest location we will > continue using the old Web Manifest) but that it is valid for a web developer to > change a WebAPK's start URL in the Web Manifest. Added a comment here. I think it is because no matter where the start url goes, the webapp still links to the same web manifest file location. If the location changes, we will treat it as a different webapp. But you remind me that we need some logic to decide when to stop observing if the location does change.
Mostly nits ▶️▶️▶️ Yaron your turn https://codereview.chromium.org/2124513002/diff/260001/chrome/android/webapk/... File chrome/android/webapk/shell_apk/BUILD.gn (right): https://codereview.chromium.org/2124513002/diff/260001/chrome/android/webapk/... chrome/android/webapk/shell_apk/BUILD.gn:25: webapk_theme_color = "2147483648L" Can you add a comment as to what this number represents? Perhaps: webapk_theme_color = "2147483648L" # HostBrowserLauncher#MANIFEST_COLOR_INVALID_OR_MISSING https://codereview.chromium.org/2124513002/diff/260001/chrome/browser/android... File chrome/browser/android/webapps/manifest_upgrade_detector.cc (right): https://codereview.chromium.org/2124513002/diff/260001/chrome/browser/android... chrome/browser/android/webapps/manifest_upgrade_detector.cc:103: ShortcutInfo info(GURL::EmptyGURL()); From offline discussion, I believe that the error you were getting is https://en.wikipedia.org/wiki/Most_vexing_parse There are a lot of places in the Chromium code which do things similar to this. e.g. RectTest, ManhattanDistanceToPoint in ui/gfx/geometry/rect_unittest.cc Notice that Rect::ManhattanDistanceToPoint() takes a const ref to a gfx::Point https://codereview.chromium.org/2124513002/diff/380001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetector.java (right): https://codereview.chromium.org/2124513002/diff/380001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetector.java:115: private void onWebappDataRetrieved(WebappDataStorage storage, final WebappInfo newInfo) { My concern was not that WebappRegistry#registerWebapp() would fail but rather about timing. It would be nice if it was clear that WebApkActivity#onWebappDataRetrieved() occurs prior to UpgradeManifestDetector#onDataAvailable() I will defer to Yaron who knows more about when things are called https://codereview.chromium.org/2124513002/diff/380001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetector.java:167: // updates before updating Chrome's sharedPreference. I do not understand what you mean by "we have to cache the old values before they are gone" https://codereview.chromium.org/2124513002/diff/380001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java (right): https://codereview.chromium.org/2124513002/diff/380001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java:66: storage.updateFromShortcutIntent(getIntent(), true); I think that this is the only call of WebappDataStorage#updateFromShortcutIntent() which passes true. It would be nicer if we did not need to pass in the boolean at all. I am also geniunely interested in the reason for the early return in WebappDataStorage#updateFromShortcutIntent(). Is there a performance reason? Can we just remove the early return and generally simplify things? https://codereview.chromium.org/2124513002/diff/440001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetector.java (right): https://codereview.chromium.org/2124513002/diff/440001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetector.java:27: "switch-use-start-url-for-testing"; Please add a comment for META_DATA_START_URL https://codereview.chromium.org/2124513002/diff/440001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetector.java:56: || TextUtils.isEmpty(mWebappInfo.webManifestUri().toString())) { How about: || mWebappInfo.webManifestUri().equals(Uri.EMPTY)) { According the Uri#equals() my suggestion seems to do the right thing https://codereview.chromium.org/2124513002/diff/440001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetector.java:60: if (!mWasStarted) { Why not early return if {@link mWasStarted} is true? https://codereview.chromium.org/2124513002/diff/440001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetector.java:82: return; Nit: The return is kind of useless 😀 https://codereview.chromium.org/2124513002/diff/440001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetector.java:122: mTab.removeObserver(this); Should we call destroy() instead? This would prevent memory leaks in the case of: - ManifestUpgradeDetector#start() being called - ManifestUpgradeDetector#onDataAvailable() callback is called by native - ManifestUpgradeDetector#start() is called again ManifestUpgradeDetector#destroy() can also call mTab#removeObserver() https://codereview.chromium.org/2124513002/diff/440001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetector.java:124: // WebAPK-compatible. crubug.com -> crbug.com https://codereview.chromium.org/2124513002/diff/440001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetector.java:139: */ How about: "Checks whether an upgraded WebAPK needs to be downloaded from the WebAPK server." https://codereview.chromium.org/2124513002/diff/440001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetector.java:143: boolean scopeMatch = mWebappInfo.scopeUri().compareTo(newInfo.scopeUri()) == 0; Can you use Uri#equals() instead? https://codereview.chromium.org/2124513002/diff/440001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetector.java:179: * Updates the SharedPreference. How about: "Updates WebappDataStorage." https://codereview.chromium.org/2124513002/diff/440001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java (right): https://codereview.chromium.org/2124513002/diff/440001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java:141: checkUpdates(); Can WebApkActivity override finishNativeInitialization() and call checkUpdates() instead? https://codereview.chromium.org/2124513002/diff/440001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java (right): https://codereview.chromium.org/2124513002/diff/440001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java:365: editor.putString(KEY_SCOPE, ShortcutHelper.getScopeFromUrl(url)); Should we use WebappInfo#scope()? https://codereview.chromium.org/2124513002/diff/440001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetectorTest.java (right): https://codereview.chromium.org/2124513002/diff/440001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetectorTest.java:180: public void testManifestDoesNotUpgrade() throws Exception { I think that all of these tests can be done as JUnit tests instead. You just need to make ManifestUpgradeDetector#requireUpgrade() static We should also have one test which check that WebappDataStorage is updated in the no upgrade scenario. You can wait on the test. I think that the refactor that I am doing in https://codereview.chromium.org/2160663002/ will make it possible to write a JUnit test for this https://codereview.chromium.org/2124513002/diff/440001/chrome/android/webapk/... File chrome/android/webapk/libs/runtime_library/src/org/chromium/webapk/lib/runtime_library/HostBrowserLauncher.java (left): https://codereview.chromium.org/2124513002/diff/440001/chrome/android/webapk/... chrome/android/webapk/libs/runtime_library/src/org/chromium/webapk/lib/runtime_library/HostBrowserLauncher.java:138: * Gets a long from a Bundle. The long should be terminated with 'L'. This function is more Nit: Keep the part about "This function is more reliable than..." because it is non obvious https://codereview.chromium.org/2124513002/diff/440001/chrome/android/webapk/... File chrome/android/webapk/libs/runtime_library/src/org/chromium/webapk/lib/runtime_library/HostBrowserLauncher.java (right): https://codereview.chromium.org/2124513002/diff/440001/chrome/android/webapk/... chrome/android/webapk/libs/runtime_library/src/org/chromium/webapk/lib/runtime_library/HostBrowserLauncher.java:38: private static final String META_DATA_WEB_MANIFEST_URL = "webManifestUrl"; Nit: new line https://codereview.chromium.org/2124513002/diff/440001/chrome/android/webapk/... chrome/android/webapk/libs/runtime_library/src/org/chromium/webapk/lib/runtime_library/HostBrowserLauncher.java:140: * Gets a long value of a color from a Bundle. The long should be terminated with 'L'. Nit: "a long value" -> "the long value" https://codereview.chromium.org/2124513002/diff/440001/chrome/browser/android... File chrome/browser/android/webapk/manifest_upgrade_detector.cc (right): https://codereview.chromium.org/2124513002/diff/440001/chrome/browser/android... chrome/browser/android/webapk/manifest_upgrade_detector.cc:98: content::WebContentsObserver::web_contents(); Nit: Remove lines 97 - 98 and replace line 99 with: web_contents()->GetManifest( https://codereview.chromium.org/2124513002/diff/440001/chrome/browser/android... chrome/browser/android/webapk/manifest_upgrade_detector.cc:101: weak_ptr_factory_.GetWeakPtr())); Nit: indentation looks off. (Not sure though) https://codereview.chromium.org/2124513002/diff/440001/chrome/browser/android... chrome/browser/android/webapk/manifest_upgrade_detector.cc:113: // web developers to cahnge the Web Manifest location. When it does cahnge -> change https://codereview.chromium.org/2124513002/diff/440001/chrome/browser/android... chrome/browser/android/webapk/manifest_upgrade_detector.cc:114: // change, we will treat the new Web Manifest as the one of another WebAPK. This logic should probably be unit tested https://codereview.chromium.org/2124513002/diff/440001/chrome/browser/android... chrome/browser/android/webapk/manifest_upgrade_detector.cc:139: Java_ManifestUpgradeDetector_onDataAvailable( Nit: 4 spaces indentation
https://codereview.chromium.org/2124513002/diff/440001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetector.java (right): https://codereview.chromium.org/2124513002/diff/440001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetector.java:37: private boolean mWasStarted; I don't think you need a separate variable for this - it's just a proxy for mNativePointer != 0 https://codereview.chromium.org/2124513002/diff/440001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetector.java:132: updateWebappDataStorage(newInfo); i'm a little surprised that we do this unconditionally. isn't there an existing mechanism to keep this up to date? I (perhaps) naively thought this component should only look for upgrades to webapks https://codereview.chromium.org/2124513002/diff/440001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetector.java:173: } todo: add icon comparison (or at a minimum, icon url so at least developers have a way of forcing update) https://codereview.chromium.org/2124513002/diff/440001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java (right): https://codereview.chromium.org/2124513002/diff/440001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java:141: checkUpdates(); On 2016/07/19 18:02:38, pkotwicz wrote: > Can WebApkActivity override finishNativeInitialization() and call checkUpdates() > instead? +1 https://codereview.chromium.org/2124513002/diff/440001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java (right): https://codereview.chromium.org/2124513002/diff/440001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java:290: public void updateFromShortcutIntent(Intent shortcutIntent, boolean bypassVersionCheck) { what's this about bypassing the version check? https://codereview.chromium.org/2124513002/diff/440001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java:365: editor.putString(KEY_SCOPE, ShortcutHelper.getScopeFromUrl(url)); On 2016/07/19 18:02:38, pkotwicz wrote: > Should we use WebappInfo#scope()? I think that's right https://codereview.chromium.org/2124513002/diff/440001/chrome/browser/android... File chrome/browser/android/webapk/manifest_upgrade_detector.cc (right): https://codereview.chromium.org/2124513002/diff/440001/chrome/browser/android... chrome/browser/android/webapk/manifest_upgrade_detector.cc:56: start_url_(start_url), where is this used? https://codereview.chromium.org/2124513002/diff/440001/chrome/browser/android... chrome/browser/android/webapk/manifest_upgrade_detector.cc:109: // and continue observing the WebContents's loading until we find a page that i'm a bit confused by this. A page should only have one manifest - or does this apply across multiple page loads?
Patchset #10 (id:480001) has been deleted
Patchset #9 (id:460001) has been deleted
Patchset #9 (id:500001) has been deleted
Hi Peter and Yaron, I think addressed all of your comments, PTAL, thanks! https://codereview.chromium.org/2124513002/diff/260001/chrome/android/webapk/... File chrome/android/webapk/shell_apk/BUILD.gn (right): https://codereview.chromium.org/2124513002/diff/260001/chrome/android/webapk/... chrome/android/webapk/shell_apk/BUILD.gn:25: webapk_theme_color = "2147483648L" On 2016/07/19 18:02:37, pkotwicz wrote: > Can you add a comment as to what this number represents? > Perhaps: > webapk_theme_color = "2147483648L" # > HostBrowserLauncher#MANIFEST_COLOR_INVALID_OR_MISSING Done. https://codereview.chromium.org/2124513002/diff/260001/chrome/browser/android... File chrome/browser/android/webapps/manifest_upgrade_detector.cc (right): https://codereview.chromium.org/2124513002/diff/260001/chrome/browser/android... chrome/browser/android/webapps/manifest_upgrade_detector.cc:103: ShortcutInfo info(GURL::EmptyGURL()); On 2016/07/19 18:02:37, pkotwicz wrote: > From offline discussion, I believe that the error you were getting is > https://en.wikipedia.org/wiki/Most_vexing_parse > > There are a lot of places in the Chromium code which do things similar to this. > e.g. RectTest, ManhattanDistanceToPoint in ui/gfx/geometry/rect_unittest.cc > Notice that Rect::ManhattanDistanceToPoint() takes a const ref to a gfx::Point Yes, an extra "()" works: ShortcutInfo info((GURL())). https://codereview.chromium.org/2124513002/diff/380001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetector.java (right): https://codereview.chromium.org/2124513002/diff/380001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetector.java:167: // updates before updating Chrome's sharedPreference. On 2016/07/19 18:02:38, pkotwicz wrote: > I do not understand what you mean by "we have to cache the old values > before they are gone" This part has been removed. https://codereview.chromium.org/2124513002/diff/380001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java (right): https://codereview.chromium.org/2124513002/diff/380001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java:66: storage.updateFromShortcutIntent(getIntent(), true); On 2016/07/19 18:02:38, pkotwicz wrote: > I think that this is the only call of > WebappDataStorage#updateFromShortcutIntent() which passes true. It would be > nicer if we did not need to pass in the boolean at all. > > I am also geniunely interested in the reason for the early return in > WebappDataStorage#updateFromShortcutIntent(). Is there a performance reason? Can > we just remove the early return and generally simplify things? After a second thought, I set a different version value for webAPK, since I don't want to change the behavior of this function. https://codereview.chromium.org/2124513002/diff/440001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetector.java (right): https://codereview.chromium.org/2124513002/diff/440001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetector.java:27: "switch-use-start-url-for-testing"; On 2016/07/19 18:02:38, pkotwicz wrote: > Please add a comment for META_DATA_START_URL Done. https://codereview.chromium.org/2124513002/diff/440001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetector.java:37: private boolean mWasStarted; On 2016/07/20 02:27:58, Yaron wrote: > I don't think you need a separate variable for this - it's just a proxy for > mNativePointer != 0 It makes sense to me. Removed. https://codereview.chromium.org/2124513002/diff/440001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetector.java:56: || TextUtils.isEmpty(mWebappInfo.webManifestUri().toString())) { On 2016/07/19 18:02:38, pkotwicz wrote: > How about: > || mWebappInfo.webManifestUri().equals(Uri.EMPTY)) { > > According the Uri#equals() my suggestion seems to do the right thing Done. https://codereview.chromium.org/2124513002/diff/440001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetector.java:82: return; On 2016/07/19 18:02:38, pkotwicz wrote: > Nit: The return is kind of useless 😀 Copy and paste, but forget to remove:( https://codereview.chromium.org/2124513002/diff/440001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetector.java:122: mTab.removeObserver(this); On 2016/07/19 18:02:38, pkotwicz wrote: > Should we call destroy() instead? > This would prevent memory leaks in the case of: > - ManifestUpgradeDetector#start() being called > - ManifestUpgradeDetector#onDataAvailable() callback is called by native > - ManifestUpgradeDetector#start() is called again > > ManifestUpgradeDetector#destroy() can also call mTab#removeObserver() Done. https://codereview.chromium.org/2124513002/diff/440001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetector.java:124: // WebAPK-compatible. On 2016/07/19 18:02:38, pkotwicz wrote: > http://crubug.com -> http://crbug.com Sorry for the typo. https://codereview.chromium.org/2124513002/diff/440001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetector.java:132: updateWebappDataStorage(newInfo); On 2016/07/20 02:27:58, Yaron wrote: > i'm a little surprised that we do this unconditionally. isn't there an existing > mechanism to keep this up to date? I (perhaps) naively thought this component > should only look for upgrades to webapks We also update the sharedPreference, since there are properties like orientation etc. that don't require to re-mint but keep the updated values in WebappDataStorage. Initially I checked every property to see if any of them changed. Peter suggests to simplify the logic by always updating the WebappDataStorage. https://codereview.chromium.org/2124513002/diff/440001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetector.java:139: */ On 2016/07/19 18:02:38, pkotwicz wrote: > How about: "Checks whether an upgraded WebAPK needs to be downloaded from the > WebAPK server." Done. https://codereview.chromium.org/2124513002/diff/440001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetector.java:143: boolean scopeMatch = mWebappInfo.scopeUri().compareTo(newInfo.scopeUri()) == 0; On 2016/07/19 18:02:38, pkotwicz wrote: > Can you use Uri#equals() instead? Done. https://codereview.chromium.org/2124513002/diff/440001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetector.java:173: } On 2016/07/20 02:27:58, Yaron wrote: > todo: add icon comparison (or at a minimum, icon url so at least developers have > a way of forcing update) Done. https://codereview.chromium.org/2124513002/diff/440001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetector.java:179: * Updates the SharedPreference. On 2016/07/19 18:02:38, pkotwicz wrote: > How about: "Updates WebappDataStorage." Done. https://codereview.chromium.org/2124513002/diff/440001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java (right): https://codereview.chromium.org/2124513002/diff/440001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java:141: checkUpdates(); On 2016/07/19 18:02:38, pkotwicz wrote: > Can WebApkActivity override finishNativeInitialization() and call checkUpdates() > instead? Done. https://codereview.chromium.org/2124513002/diff/440001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java (right): https://codereview.chromium.org/2124513002/diff/440001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java:290: public void updateFromShortcutIntent(Intent shortcutIntent, boolean bypassVersionCheck) { On 2016/07/20 02:27:59, Yaron wrote: > what's this about bypassing the version check? The version of Webapp is always set to ShortcutHelper.WEBAPP_SHORTCUT_VERSION the first time that updateFromShortcutIntent() is called. This means once the webapp is set, we won't be able to update fields except url, scope. So for WebAPK, I try to avoid calling this function. We either: 1) call updateFromWebappInfo(WebappInfo info) when udpates are detected and we need to update the WebappDataStorage; 2) or call updateFromShortcutIntent() when WebAPK hasn't registered. However, we have tests call this functions. I don't want to change the behavior of this function, instead, the version value of WebAPK is set differently to bypass this check. https://codereview.chromium.org/2124513002/diff/440001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java:365: editor.putString(KEY_SCOPE, ShortcutHelper.getScopeFromUrl(url)); On 2016/07/19 18:02:38, pkotwicz wrote: > Should we use WebappInfo#scope()? Good catch! https://codereview.chromium.org/2124513002/diff/440001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetectorTest.java (right): https://codereview.chromium.org/2124513002/diff/440001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetectorTest.java:180: public void testManifestDoesNotUpgrade() throws Exception { On 2016/07/19 18:02:38, pkotwicz wrote: > I think that all of these tests can be done as JUnit tests instead. You just > need to make ManifestUpgradeDetector#requireUpgrade() static > > We should also have one test which check that WebappDataStorage is updated in > the no upgrade scenario. You can wait on the test. I think that the refactor > that I am doing in https://codereview.chromium.org/2160663002/ will make it > possible to write a JUnit test for this Honestly, I am not sure whether we can write these tests as JUnit tests. The ManifestUpgradeDetector observe the tab's loading, so is it really possible to avoid creating a ChromeActivity? Besides, the TestManifestUpgradeDetector overrides requireUpgrade() to check the manifest fetch results, so we cannot make it static. It is worthy discussing the feasibility offline:) https://codereview.chromium.org/2124513002/diff/440001/chrome/android/webapk/... File chrome/android/webapk/libs/runtime_library/src/org/chromium/webapk/lib/runtime_library/HostBrowserLauncher.java (left): https://codereview.chromium.org/2124513002/diff/440001/chrome/android/webapk/... chrome/android/webapk/libs/runtime_library/src/org/chromium/webapk/lib/runtime_library/HostBrowserLauncher.java:138: * Gets a long from a Bundle. The long should be terminated with 'L'. This function is more On 2016/07/19 18:02:38, pkotwicz wrote: > Nit: Keep the part about "This function is more reliable than..." because it is > non obvious Done. https://codereview.chromium.org/2124513002/diff/440001/chrome/android/webapk/... File chrome/android/webapk/libs/runtime_library/src/org/chromium/webapk/lib/runtime_library/HostBrowserLauncher.java (right): https://codereview.chromium.org/2124513002/diff/440001/chrome/android/webapk/... chrome/android/webapk/libs/runtime_library/src/org/chromium/webapk/lib/runtime_library/HostBrowserLauncher.java:38: private static final String META_DATA_WEB_MANIFEST_URL = "webManifestUrl"; On 2016/07/19 18:02:38, pkotwicz wrote: > Nit: new line Done. https://codereview.chromium.org/2124513002/diff/440001/chrome/android/webapk/... chrome/android/webapk/libs/runtime_library/src/org/chromium/webapk/lib/runtime_library/HostBrowserLauncher.java:140: * Gets a long value of a color from a Bundle. The long should be terminated with 'L'. On 2016/07/19 18:02:38, pkotwicz wrote: > Nit: "a long value" -> "the long value" Done. https://codereview.chromium.org/2124513002/diff/440001/chrome/browser/android... File chrome/browser/android/webapk/manifest_upgrade_detector.cc (right): https://codereview.chromium.org/2124513002/diff/440001/chrome/browser/android... chrome/browser/android/webapk/manifest_upgrade_detector.cc:56: start_url_(start_url), On 2016/07/20 02:27:59, Yaron wrote: > where is this used? Thanks for catching this. I forgot why I initially passed it to native, but now it isn't used anymore. https://codereview.chromium.org/2124513002/diff/440001/chrome/browser/android... chrome/browser/android/webapk/manifest_upgrade_detector.cc:98: content::WebContentsObserver::web_contents(); On 2016/07/19 18:02:39, pkotwicz wrote: > Nit: Remove lines 97 - 98 and replace line 99 with: > web_contents()->GetManifest( Done. https://codereview.chromium.org/2124513002/diff/440001/chrome/browser/android... chrome/browser/android/webapk/manifest_upgrade_detector.cc:101: weak_ptr_factory_.GetWeakPtr())); On 2016/07/19 18:02:39, pkotwicz wrote: > Nit: indentation looks off. (Not sure though) Done. https://codereview.chromium.org/2124513002/diff/440001/chrome/browser/android... chrome/browser/android/webapk/manifest_upgrade_detector.cc:109: // and continue observing the WebContents's loading until we find a page that On 2016/07/20 02:27:59, Yaron wrote: > i'm a bit confused by this. A page should only have one manifest - or does this > apply across multiple page loads? Yes, it aims to handle multiple pages navigation within a webapp, since we can't assume there is no such case occurs. For example, user clicks an arbitrary link within the scope of a WebAPK, and it bounces into the WebAPK. The WebAPK is launched not from the start url, but a url that doesn't link to its web manifest. https://codereview.chromium.org/2124513002/diff/440001/chrome/browser/android... chrome/browser/android/webapk/manifest_upgrade_detector.cc:113: // web developers to cahnge the Web Manifest location. When it does On 2016/07/19 18:02:39, pkotwicz wrote: > cahnge -> change Done. https://codereview.chromium.org/2124513002/diff/440001/chrome/browser/android... chrome/browser/android/webapk/manifest_upgrade_detector.cc:114: // change, we will treat the new Web Manifest as the one of another WebAPK. On 2016/07/19 18:02:39, pkotwicz wrote: > This logic should probably be unit tested Done. https://codereview.chromium.org/2124513002/diff/440001/chrome/browser/android... chrome/browser/android/webapk/manifest_upgrade_detector.cc:139: Java_ManifestUpgradeDetector_onDataAvailable( On 2016/07/19 18:02:39, pkotwicz wrote: > Nit: 4 spaces indentation Done.
The CQ bit was checked by hanxi@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: 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...)
Patchset #9 (id:520001) has been deleted
Patchset #9 (id:540001) has been deleted
Still going through the CL. Decided to publish my comments so far because I think that you can make that javatests JUnit tests https://codereview.chromium.org/2124513002/diff/440001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetectorTest.java (right): https://codereview.chromium.org/2124513002/diff/440001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetectorTest.java:180: public void testManifestDoesNotUpgrade() throws Exception { It is definitely possible to write a JUnit test because I am suggesting making ManifestUpgradeDetector#requireUpgrade() static https://codereview.chromium.org/2124513002/diff/560001/chrome/browser/android... File chrome/browser/android/webapk/manifest_upgrade_detector.cc (right): https://codereview.chromium.org/2124513002/diff/560001/chrome/browser/android... chrome/browser/android/webapk/manifest_upgrade_detector.cc:26: } // namespace Nit: "namespace" -> "anonymous namespace" I think that this convention changed a few months ago https://codereview.chromium.org/2124513002/diff/560001/chrome/browser/android... chrome/browser/android/webapk/manifest_upgrade_detector.cc:31: const JavaParamRef<jstring>& java_scope, Nit: |java_scope| -> |java_scope_url| for the sake of consistency with |java_web_manifest_url| https://codereview.chromium.org/2124513002/diff/560001/chrome/browser/android... chrome/browser/android/webapk/manifest_upgrade_detector.cc:117: ShortcutInfo info((GURL())); I think that GURL::EmptyGURL() is slightly more readable. What you have now is fine too though. https://codereview.chromium.org/2124513002/diff/560001/chrome/browser/android... File chrome/browser/android/webapk/manifest_upgrade_detector.h (right): https://codereview.chromium.org/2124513002/diff/560001/chrome/browser/android... chrome/browser/android/webapk/manifest_upgrade_detector.h:8: #include <vector> Nit: This include is unnecessary https://codereview.chromium.org/2124513002/diff/560001/chrome/browser/android... chrome/browser/android/webapk/manifest_upgrade_detector.h:75: Nit: Add comments for each of the member variables (except |weak_ptr_factory_|) https://codereview.chromium.org/2124513002/diff/560001/chrome/browser/android... File chrome/browser/android/webapk/manifest_upgrade_detector_unittest.cc (right): https://codereview.chromium.org/2124513002/diff/560001/chrome/browser/android... chrome/browser/android/webapk/manifest_upgrade_detector_unittest.cc:12: OnDidGetManifestReturnsFalseWhenTheFetchedManifestUrlIsEmpty) { This is on the border of what should be unit tested. The logic in ManifestUpgradeDetector::DidFinishLoad() is simple but non obvious. I did not realize when I asked you to write the unit tests that the JNI call would be a problem. You could add a method ManifestUpgradeDetector::ShouldStop(const GURL& manifest_url, const content::Manifest& manifest) and unit test that I will defer to Yaron as to whether manifest_upgrade_detector\.cc should be unit tested. Right now I am leaning towards no. Sorry for asking you to write all of this code :(
Some more comments. I talked to Yaron offline a bit and he has some "overarching architecture questions". Nothing about your CL, just questions about certain decisions that we made a long time ago. https://codereview.chromium.org/2124513002/diff/440001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetector.java (right): https://codereview.chromium.org/2124513002/diff/440001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetector.java:60: if (!mWasStarted) { Bump https://codereview.chromium.org/2124513002/diff/440001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetector.java:139: */ Bump https://codereview.chromium.org/2124513002/diff/560001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetector.java (right): https://codereview.chromium.org/2124513002/diff/560001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetector.java:21: * This class interacts with C++ to detect whether resources in web manifest of a WebAPK has been Nit: has -> have https://codereview.chromium.org/2124513002/diff/560001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetector.java:29: * The META_* is the metadata stored in WebAPK's AndroidManifest.xml.The values must stay in How about: "The names of <meta-data> in the WebAPK's AndroidManifest.xml whose values are needed to determine whether a WebAPK needs to be upgraded but which are not present in {@link WebappInfo}. The names must stay in sync with {@link org.chromium.webapk.lib.runtime_library.HostBrowserLauncher}." https://codereview.chromium.org/2124513002/diff/560001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetector.java:129: mWebappInfo.source(), themeColor, backgroundColor, false, Can we do mWebappInfo.isIconGenerated() instead of passing false? https://codereview.chromium.org/2124513002/diff/560001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java (right): https://codereview.chromium.org/2124513002/diff/560001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java:154: // TODO(hanxi): Ask WebApk's update manager whether to check resources updates. Nit: "resources updates" -> "resource updates"
PTAL, thanks! https://codereview.chromium.org/2124513002/diff/440001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetector.java (right): https://codereview.chromium.org/2124513002/diff/440001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetector.java:60: if (!mWasStarted) { On 2016/07/20 21:47:52, pkotwicz wrote: > Bump I have changed this to a check "if (mNativePointer == null)". This check is only for if the native pointer is initialized, not if the detection pipeline is starting. https://codereview.chromium.org/2124513002/diff/440001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetector.java:139: */ On 2016/07/20 21:47:52, pkotwicz wrote: > Bump > ? The description has been updated. https://codereview.chromium.org/2124513002/diff/440001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetectorTest.java (right): https://codereview.chromium.org/2124513002/diff/440001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetectorTest.java:180: public void testManifestDoesNotUpgrade() throws Exception { On 2016/07/20 20:57:04, pkotwicz wrote: > It is definitely possible to write a JUnit test because I am suggesting making > ManifestUpgradeDetector#requireUpgrade() static As discussed offline, we will still keep the Java Tests. https://codereview.chromium.org/2124513002/diff/560001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetector.java (right): https://codereview.chromium.org/2124513002/diff/560001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetector.java:21: * This class interacts with C++ to detect whether resources in web manifest of a WebAPK has been On 2016/07/20 21:47:53, pkotwicz wrote: > Nit: has -> have Done. https://codereview.chromium.org/2124513002/diff/560001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetector.java:29: * The META_* is the metadata stored in WebAPK's AndroidManifest.xml.The values must stay in On 2016/07/20 21:47:53, pkotwicz wrote: > How about: "The names of <meta-data> in the WebAPK's AndroidManifest.xml whose > values are needed to determine whether a WebAPK needs to be upgraded but which > are not present in {@link WebappInfo}. The names must stay in sync with {@link > org.chromium.webapk.lib.runtime_library.HostBrowserLauncher}." Done. https://codereview.chromium.org/2124513002/diff/560001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetector.java:129: mWebappInfo.source(), themeColor, backgroundColor, false, On 2016/07/20 21:47:53, pkotwicz wrote: > Can we do mWebappInfo.isIconGenerated() instead of passing false? Done. https://codereview.chromium.org/2124513002/diff/560001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java (right): https://codereview.chromium.org/2124513002/diff/560001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java:154: // TODO(hanxi): Ask WebApk's update manager whether to check resources updates. On 2016/07/20 21:47:53, pkotwicz wrote: > Nit: "resources updates" -> "resource updates" Done. https://codereview.chromium.org/2124513002/diff/560001/chrome/browser/android... File chrome/browser/android/webapk/manifest_upgrade_detector.cc (right): https://codereview.chromium.org/2124513002/diff/560001/chrome/browser/android... chrome/browser/android/webapk/manifest_upgrade_detector.cc:26: } // namespace On 2016/07/20 20:57:04, pkotwicz wrote: > Nit: "namespace" -> "anonymous namespace" > > I think that this convention changed a few months ago Done. https://codereview.chromium.org/2124513002/diff/560001/chrome/browser/android... chrome/browser/android/webapk/manifest_upgrade_detector.cc:31: const JavaParamRef<jstring>& java_scope, On 2016/07/20 20:57:04, pkotwicz wrote: > Nit: |java_scope| -> |java_scope_url| for the sake of consistency with > |java_web_manifest_url| This keeps consistent with the naming in web manifest. Classes like ShortcutInfo, struct Manifest all use "scope". I updated it to |java_scope_url|, but keep the member variable name unchanged. https://codereview.chromium.org/2124513002/diff/560001/chrome/browser/android... chrome/browser/android/webapk/manifest_upgrade_detector.cc:117: ShortcutInfo info((GURL())); On 2016/07/20 20:57:04, pkotwicz wrote: > I think that GURL::EmptyGURL() is slightly more readable. What you have now is > fine too though. Ok, revert it back. https://codereview.chromium.org/2124513002/diff/560001/chrome/browser/android... File chrome/browser/android/webapk/manifest_upgrade_detector.h (right): https://codereview.chromium.org/2124513002/diff/560001/chrome/browser/android... chrome/browser/android/webapk/manifest_upgrade_detector.h:8: #include <vector> On 2016/07/20 20:57:04, pkotwicz wrote: > Nit: This include is unnecessary Removed. https://codereview.chromium.org/2124513002/diff/560001/chrome/browser/android... chrome/browser/android/webapk/manifest_upgrade_detector.h:75: On 2016/07/20 20:57:04, pkotwicz wrote: > Nit: Add comments for each of the member variables (except |weak_ptr_factory_|) Done.
L-G-T-M after you address these comments https://codereview.chromium.org/2124513002/diff/440001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetector.java (right): https://codereview.chromium.org/2124513002/diff/440001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetector.java:60: if (!mWasStarted) { You should call Tab#addObserver() only if (mNativePointer == null) (Technically your code is correct due to the implementation of ObserverList#addObserver() but I think that it is clearer if you call Tab#addObserver() only if (mNativePointer == null). https://codereview.chromium.org/2124513002/diff/560001/chrome/browser/android... File chrome/browser/android/webapk/manifest_upgrade_detector.cc (right): https://codereview.chromium.org/2124513002/diff/560001/chrome/browser/android... chrome/browser/android/webapk/manifest_upgrade_detector.cc:31: const JavaParamRef<jstring>& java_scope, In that case, keep the parameter name |java_scope| https://codereview.chromium.org/2124513002/diff/560001/chrome/browser/android... File chrome/browser/android/webapk/manifest_upgrade_detector_unittest.cc (right): https://codereview.chromium.org/2124513002/diff/560001/chrome/browser/android... chrome/browser/android/webapk/manifest_upgrade_detector_unittest.cc:12: OnDidGetManifestReturnsFalseWhenTheFetchedManifestUrlIsEmpty) { Still deferring to Yaron https://codereview.chromium.org/2124513002/diff/580001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetector.java (right): https://codereview.chromium.org/2124513002/diff/580001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetector.java:75: private void getMetaDataFromAndroidManifest() { Here's an idea. You could have a constructor which takes in the startUrl that you use when testing e.g. ManifestUpgradeDetector(Tab tab, WebappInfo info, String startUrl) and a static method which takes in fewer arguments. e.g. ManifestUpgradeDetector#create(Tab tab, WebappInfo info) {} This avoids the command line flag. The "manifest URL", "icon URL" and "icon MD5 hash" will all be specified in the manifest. That would be a lot of command line flags 😱 https://codereview.chromium.org/2124513002/diff/580001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetector.java:119: * Checks whether an upgraded WebAPK needs to be downloaded from the WebAPK server. How about: "Called when the updated Web Manifest has been fetched." I think my comment better matches the function name. https://codereview.chromium.org/2124513002/diff/580001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetector.java:140: * if yes. How about: "Checks whether the WebAPK needs to be upgraded provided the new Web Manifest info." My version does not include: "Request to re-mint the WebAPK if yes" because "what occurs when an upgrade is required" is unrelated to what the function does https://codereview.chromium.org/2124513002/diff/580001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetector.java:142: protected boolean requireUpgrade(WebappInfo newInfo, String startUrl) { Now that we are upgrading if any property changes you can remove all of my lengthy comments https://codereview.chromium.org/2124513002/diff/580001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java (right): https://codereview.chromium.org/2124513002/diff/580001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java:30: private static final String TAG = "cr_WebApkActivity"; Nit: The tag is unused https://codereview.chromium.org/2124513002/diff/580001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java (right): https://codereview.chromium.org/2124513002/diff/580001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java:331: editor.putString(KEY_WEBAPK_PACKAGE_NAME, webApkPakcageName); This change is no longer necessary? https://codereview.chromium.org/2124513002/diff/580001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetectorTest.java (right): https://codereview.chromium.org/2124513002/diff/580001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetectorTest.java:57: protected boolean requireUpgrade(WebappInfo newInfo, String startUrl) { I think there is a nicer way of doing this test but I am too new to Clank to know what the right wayTM of doing it is https://codereview.chromium.org/2124513002/diff/580001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetectorTest.java:101: .putExtra(ShortcutHelper.EXTRA_URL, mStartUrl) Can this be "manifest_test_page.html" instead? It is valid for a WebAPK to be launched at URL other than the start URL https://codereview.chromium.org/2124513002/diff/580001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetectorTest.java:107: mTestServer.getURL(WEBAPK_WEB_MANIFEST_URL)); Don't you also need to set: - display mdoe - the color - background color Would it be simpler to call WebappInfo#create() ? https://codereview.chromium.org/2124513002/diff/580001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetectorTest.java:112: registerWebApk(intent); Nit: Registering the WebAPK is no longer necessary https://codereview.chromium.org/2124513002/diff/580001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetectorTest.java:126: CriteriaHelper.pollUiThread(new Criteria() { I think that CallbackHelper is the new hotness https://codereview.chromium.org/2124513002/diff/580001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetectorTest.java:142: @MediumTest I think that you need only one test to check whether "updating the Web Manifest" causes an upgrade request (You can delete testWebappShortNameChangeReturnsUpgradeTrue() and testWebappBackgroundColorChangeReturnsUpgradeTrue(). testStartUrlChangeShouldReturnUpgradeTrue() gets to stay because it is first. No other reason) https://codereview.chromium.org/2124513002/diff/580001/chrome/browser/android... File chrome/browser/android/webapk/manifest_upgrade_detector.h (right): https://codereview.chromium.org/2124513002/diff/580001/chrome/browser/android... chrome/browser/android/webapk/manifest_upgrade_detector.h:74: // A flag to indicate if the detection pipeline is started. Nit: is -> was https://codereview.chromium.org/2124513002/diff/580001/chrome/browser/android... chrome/browser/android/webapk/manifest_upgrade_detector.h:77: // The detector will Only fetch the URL within the Scope of the WebAPK. Nits: - Only -> only - Scope -> scope
Patchset #12 (id:620001) has been deleted
Patchset #12 (id:640001) has been deleted
Patchset #11 (id:600001) has been deleted
Patchset #11 (id:660001) has been deleted
PTAL, thanks! https://codereview.chromium.org/2124513002/diff/440001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetector.java (right): https://codereview.chromium.org/2124513002/diff/440001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetector.java:60: if (!mWasStarted) { On 2016/07/22 20:13:05, pkotwicz wrote: > You should call Tab#addObserver() only if (mNativePointer == null) > > (Technically your code is correct due to the implementation of > ObserverList#addObserver() but I think that it is clearer if you call > Tab#addObserver() only if (mNativePointer == null). Done. https://codereview.chromium.org/2124513002/diff/560001/chrome/browser/android... File chrome/browser/android/webapk/manifest_upgrade_detector.cc (right): https://codereview.chromium.org/2124513002/diff/560001/chrome/browser/android... chrome/browser/android/webapk/manifest_upgrade_detector.cc:31: const JavaParamRef<jstring>& java_scope, On 2016/07/22 20:13:05, pkotwicz wrote: > In that case, keep the parameter name |java_scope| Done. https://codereview.chromium.org/2124513002/diff/580001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetector.java (right): https://codereview.chromium.org/2124513002/diff/580001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetector.java:75: private void getMetaDataFromAndroidManifest() { On 2016/07/22 20:13:05, pkotwicz wrote: > Here's an idea. > > You could have a constructor which takes in the startUrl that you use when > testing e.g. > ManifestUpgradeDetector(Tab tab, WebappInfo info, String startUrl) > > and a static method which takes in fewer arguments. e.g. > ManifestUpgradeDetector#create(Tab tab, WebappInfo info) {} > > This avoids the command line flag. The "manifest URL", "icon URL" and "icon MD5 > hash" will all be specified in the manifest. That would be a lot of command line > flags 😱 Hmmm, I would prefer to rename the command line flag to be more generic, and change the setStartUrl() to setMetadataForTesting(). When "icon URL" etc. are added, we could simply passing more parameters to this function. https://codereview.chromium.org/2124513002/diff/580001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetector.java:119: * Checks whether an upgraded WebAPK needs to be downloaded from the WebAPK server. On 2016/07/22 20:13:05, pkotwicz wrote: > How about: "Called when the updated Web Manifest has been fetched." > > I think my comment better matches the function name. Done. https://codereview.chromium.org/2124513002/diff/580001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetector.java:140: * if yes. On 2016/07/22 20:13:05, pkotwicz wrote: > How about: "Checks whether the WebAPK needs to be upgraded provided the new Web > Manifest info." > > My version does not include: "Request to re-mint the WebAPK if yes" because > "what occurs when an upgrade is required" is unrelated to what the function does Done. https://codereview.chromium.org/2124513002/diff/580001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetector.java:142: protected boolean requireUpgrade(WebappInfo newInfo, String startUrl) { On 2016/07/22 20:13:05, pkotwicz wrote: > Now that we are upgrading if any property changes you can remove all of my > lengthy comments Done. https://codereview.chromium.org/2124513002/diff/580001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java (right): https://codereview.chromium.org/2124513002/diff/580001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java:30: private static final String TAG = "cr_WebApkActivity"; On 2016/07/22 20:13:06, pkotwicz wrote: > Nit: The tag is unused Done. https://codereview.chromium.org/2124513002/diff/580001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java (right): https://codereview.chromium.org/2124513002/diff/580001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java:331: editor.putString(KEY_WEBAPK_PACKAGE_NAME, webApkPakcageName); On 2016/07/22 20:13:06, pkotwicz wrote: > This change is no longer necessary? Reverted. https://codereview.chromium.org/2124513002/diff/580001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetectorTest.java (right): https://codereview.chromium.org/2124513002/diff/580001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetectorTest.java:101: .putExtra(ShortcutHelper.EXTRA_URL, mStartUrl) On 2016/07/22 20:13:06, pkotwicz wrote: > Can this be "manifest_test_page.html" instead? It is valid for a WebAPK to be > launched at URL other than the start URL It could be, since the url doesn't matter here. I am ok to use that. https://codereview.chromium.org/2124513002/diff/580001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetectorTest.java:107: mTestServer.getURL(WEBAPK_WEB_MANIFEST_URL)); On 2016/07/22 20:13:06, pkotwicz wrote: > Don't you also need to set: > - display mdoe > - the color > - background color > > Would it be simpler to call WebappInfo#create() ? I didn't add them in the first place because the manifest.json file itself doesn't define theme color and background color. Add them in the intent as well. It is better to use CreateIntent(): 1) we only add the extra that we care about, not unnecessary one like webApkPackageName; 2) it is clear to update some of them for the purpose to detect upgrade in the following tests. https://codereview.chromium.org/2124513002/diff/580001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetectorTest.java:112: registerWebApk(intent); On 2016/07/22 20:13:06, pkotwicz wrote: > Nit: Registering the WebAPK is no longer necessary Good catch! https://codereview.chromium.org/2124513002/diff/580001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetectorTest.java:126: CriteriaHelper.pollUiThread(new Criteria() { On 2016/07/22 20:13:06, pkotwicz wrote: > I think that CallbackHelper is the new hotness Thanks for pointing out it. I don't mint to change it to CallbackHelper, but it will take longer time to update this CL. I don't think it is worthy the effort for a test. https://codereview.chromium.org/2124513002/diff/580001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetectorTest.java:142: @MediumTest On 2016/07/22 20:13:06, pkotwicz wrote: > I think that you need only one test to check whether "updating the Web Manifest" > causes an upgrade request > > (You can delete testWebappShortNameChangeReturnsUpgradeTrue() and > testWebappBackgroundColorChangeReturnsUpgradeTrue(). > testStartUrlChangeShouldReturnUpgradeTrue() gets to stay because it is first. No > other reason) Done. https://codereview.chromium.org/2124513002/diff/580001/chrome/browser/android... File chrome/browser/android/webapk/manifest_upgrade_detector.h (right): https://codereview.chromium.org/2124513002/diff/580001/chrome/browser/android... chrome/browser/android/webapk/manifest_upgrade_detector.h:77: // The detector will Only fetch the URL within the Scope of the WebAPK. On 2016/07/22 20:13:06, pkotwicz wrote: > Nits: > - Only -> only > - Scope -> scope Done.
lgtm https://codereview.chromium.org/2124513002/diff/680001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetector.java (right): https://codereview.chromium.org/2124513002/diff/680001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetector.java:53: public void setMetadataForTesting(String startUrl) { If you rename this to: overrideMetadataForTesting and have it set a bool, you can remove the dependency on CommandLine and the new switch https://codereview.chromium.org/2124513002/diff/680001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetector.java:156: || !mWebappInfo.shortName().equals(newInfo.shortName()) If these can be null (first-glance seems no, but not sure), then use TextUtils.equals() https://codereview.chromium.org/2124513002/diff/680001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetector.java:156: || !mWebappInfo.shortName().equals(newInfo.shortName()) we should consider adding a helper method to WebAppInfo but this is specific to WebApks and not webapp shortcuts. Actually, probably would've been nice to do this in native if we still had access to the manifest but I see we don't. I'm ok with his for now. https://codereview.chromium.org/2124513002/diff/680001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java (right): https://codereview.chromium.org/2124513002/diff/680001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java:186: mManifestUpgradeDetector.destroy(); null it out.
Patchset #12 (id:700001) has been deleted
Mostly test related comments Yaron, some questions for you too https://codereview.chromium.org/2124513002/diff/560001/chrome/browser/android... File chrome/browser/android/webapk/manifest_upgrade_detector_unittest.cc (right): https://codereview.chromium.org/2124513002/diff/560001/chrome/browser/android... chrome/browser/android/webapk/manifest_upgrade_detector_unittest.cc:12: OnDidGetManifestReturnsFalseWhenTheFetchedManifestUrlIsEmpty) { Yaron bump! https://codereview.chromium.org/2124513002/diff/580001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetector.java (right): https://codereview.chromium.org/2124513002/diff/580001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetector.java:75: private void getMetaDataFromAndroidManifest() { Fair enough https://codereview.chromium.org/2124513002/diff/580001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetectorTest.java (right): https://codereview.chromium.org/2124513002/diff/580001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetectorTest.java:126: CriteriaHelper.pollUiThread(new Criteria() { Can you please make the change? Both CallbackHelper and CriteriaHelper are only used fro tests https://codereview.chromium.org/2124513002/diff/680001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetector.java (right): https://codereview.chromium.org/2124513002/diff/680001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetector.java:26: public static final String SWITCH_DO_NOT_USE_META_DATA_FROM_WEBAPK_FOR_TESTING = Nit: Remove "switch-" from switch https://codereview.chromium.org/2124513002/diff/680001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetectorTest.java (right): https://codereview.chromium.org/2124513002/diff/680001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetectorTest.java:29: public class ManifestUpgradeDetectorTest extends ChromeTabbedActivityTestBase { Please add a comment where that the field values come from {@link WEBAPK_WEB_MANIFEST_URL} https://codereview.chromium.org/2124513002/diff/680001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetectorTest.java:41: private ManifestUpgradeDetector mDetector; Nit: Change the type to TestManifestUpgradeDetector. This should enable removing several casts https://codereview.chromium.org/2124513002/diff/680001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetectorTest.java:43: Nit: Please add JavaDoc for the test class. https://codereview.chromium.org/2124513002/diff/680001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetectorTest.java:45: private boolean mIsUpgraded; Please make these variables public to reflect how they are being used I looked it up. Here is an explanation of why an outer class can access an inner class' privates http://stackoverflow.com/questions/19747812/why-can-the-private-member-of-an-... https://codereview.chromium.org/2124513002/diff/680001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetectorTest.java:48: private boolean mIsDataAvaliable = false; Nit: new line https://codereview.chromium.org/2124513002/diff/680001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetectorTest.java:61: mIsDataAvaliable = true; mIsDataAvaliable -> mIsDataAvailable https://codereview.chromium.org/2124513002/diff/680001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetectorTest.java:62: mInfo = newInfo; This does not seem to be used https://codereview.chromium.org/2124513002/diff/680001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetectorTest.java:86: Please add a comment say that this creates an intent with the properties in chrome/test/data/webapps/manifest.json https://codereview.chromium.org/2124513002/diff/680001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetectorTest.java:89: intent.setData(Uri.parse(WebappActivity.WEBAPP_SCHEME + "://" + WEBAPK_ID)); Is setting the data necessary? https://codereview.chromium.org/2124513002/diff/680001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetectorTest.java:108: final Tab tab = getActivity().getActivityTab(); Can you please add a comment why ManifestUpgradeDetector#start() must be called on the UI thread? (I actually do not know why it is necessary) The ManifestUpgradeDetector constructor and ManifestUpgradeDetector#setMetadataForTesting() do not need to be called on the UI thread. Can you move those outside of the ThreadUtils#runOnUiThreadBlocking() block? https://codereview.chromium.org/2124513002/diff/680001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetectorTest.java:112: mDetector = new TestManifestUpgradeDetector(tab, WebappInfo.create(createIntent())); This function is not using the intent parameter at all. I think that you should: - Use the passed in WebappInfo (not an intent) - Pass in the startUrl as a parameter as well Document that the "passed in WebappInfo" and the "passed in start URL" are "the old WebAPK data" https://codereview.chromium.org/2124513002/diff/680001/chrome/browser/android... File chrome/browser/android/webapk/manifest_upgrade_detector.cc (right): https://codereview.chromium.org/2124513002/diff/680001/chrome/browser/android... chrome/browser/android/webapk/manifest_upgrade_detector.cc:108: // change, we will treat the new Web Manifest as the one of another WebAPK. Yaron do you have a good comment to put here to explain why we early return when |web_manifest_url_| != manifest_url https://codereview.chromium.org/2124513002/diff/680001/chrome/test/data/webap... File chrome/test/data/webapps/manifest_test_page_test_start_url_change.html (right): https://codereview.chromium.org/2124513002/diff/680001/chrome/test/data/webap... chrome/test/data/webapps/manifest_test_page_test_start_url_change.html:1: <html> It should be possible to delete this file now?
https://codereview.chromium.org/2124513002/diff/560001/chrome/browser/android... File chrome/browser/android/webapk/manifest_upgrade_detector_unittest.cc (right): https://codereview.chromium.org/2124513002/diff/560001/chrome/browser/android... chrome/browser/android/webapk/manifest_upgrade_detector_unittest.cc:12: OnDidGetManifestReturnsFalseWhenTheFetchedManifestUrlIsEmpty) { On 2016/07/25 17:35:52, pkotwicz wrote: > Yaron bump! Not sure why JNI code is a problem - the Java code should be registered with the test fixture and if it's not, that's a bug. I think this testing is pretty minimal - by itself it's not interesting but if you covered the other cases (e.g. manifest doesn't match, then it might). You could actually close the feedback loop - by having the java ManifestUpgradeDetector notify native whether or not upgrade is necessary, you could test the variations from here and possibly not add ManifestUpgradeDetectorTest? https://codereview.chromium.org/2124513002/diff/680001/chrome/browser/android... File chrome/browser/android/webapk/manifest_upgrade_detector.cc (right): https://codereview.chromium.org/2124513002/diff/680001/chrome/browser/android... chrome/browser/android/webapk/manifest_upgrade_detector.cc:108: // change, we will treat the new Web Manifest as the one of another WebAPK. On 2016/07/25 17:35:53, pkotwicz wrote: > Yaron do you have a good comment to put here to explain why we early return when > |web_manifest_url_| != manifest_url I think Xi's comment is correct. As currently planned, when you install a WebApk, it will not follow you if the manifest url changes. But we have to reflect the fact that the old WebApk was launched for the previous manifest url and we wait to see if the user discovers a new version of it. Actually if it doesn't match, it really shouldn't return FetchedManifestEmpty - it's a no-op.
Patchset #12 (id:720001) has been deleted
Patchset #12 (id:740001) has been deleted
PTAL, thanks! https://codereview.chromium.org/2124513002/diff/560001/chrome/browser/android... File chrome/browser/android/webapk/manifest_upgrade_detector_unittest.cc (right): https://codereview.chromium.org/2124513002/diff/560001/chrome/browser/android... chrome/browser/android/webapk/manifest_upgrade_detector_unittest.cc:12: OnDidGetManifestReturnsFalseWhenTheFetchedManifestUrlIsEmpty) { On 2016/07/25 18:00:21, Yaron wrote: > On 2016/07/25 17:35:52, pkotwicz wrote: > > Yaron bump! > > Not sure why JNI code is a problem - the Java code should be registered with the > test fixture and if it's not, that's a bug. > > I think this testing is pretty minimal - by itself it's not interesting but if > you covered the other cases (e.g. manifest doesn't match, then it might). You > could actually close the feedback loop - by having the java > ManifestUpgradeDetector notify native whether or not upgrade is necessary, you > could test the variations from here and possibly not add > ManifestUpgradeDetectorTest? I would prefer to simply to remove this test. For the case of having a non-empty web_manifest_url but doesn't match the current one, we can't really tell if this is another manifest url (within the webapp's scope) or the old url is changed. So we just return here, won't continue fetching until get the result of if upgrade or not. https://codereview.chromium.org/2124513002/diff/580001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetectorTest.java (right): https://codereview.chromium.org/2124513002/diff/580001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetectorTest.java:126: CriteriaHelper.pollUiThread(new Criteria() { On 2016/07/25 17:35:52, pkotwicz wrote: > Can you please make the change? Both CallbackHelper and CriteriaHelper are only > used fro tests Done. https://codereview.chromium.org/2124513002/diff/680001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetector.java (right): https://codereview.chromium.org/2124513002/diff/680001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetector.java:26: public static final String SWITCH_DO_NOT_USE_META_DATA_FROM_WEBAPK_FOR_TESTING = On 2016/07/25 17:35:53, pkotwicz wrote: > Nit: Remove "switch-" from switch Done. https://codereview.chromium.org/2124513002/diff/680001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetector.java:53: public void setMetadataForTesting(String startUrl) { On 2016/07/25 15:55:02, Yaron wrote: > If you rename this to: overrideMetadataForTesting and have it set a bool, you > can remove the dependency on CommandLine and the new switch Done. https://codereview.chromium.org/2124513002/diff/680001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetector.java:156: || !mWebappInfo.shortName().equals(newInfo.shortName()) On 2016/07/25 15:55:02, Yaron wrote: > we should consider adding a helper method to WebAppInfo but this is specific to > WebApks and not webapp shortcuts. Actually, probably would've been nice to do > this in native if we still had access to the manifest but I see we don't. I'm ok > with his for now. Acknowledged. https://codereview.chromium.org/2124513002/diff/680001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetector.java:156: || !mWebappInfo.shortName().equals(newInfo.shortName()) On 2016/07/25 15:55:01, Yaron wrote: > If these can be null (first-glance seems no, but not sure), then use > TextUtils.equals() It can't be null, will fallback to "". But I am ok to use TextUtils.equals(), which looks safer. https://codereview.chromium.org/2124513002/diff/680001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java (right): https://codereview.chromium.org/2124513002/diff/680001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java:186: mManifestUpgradeDetector.destroy(); On 2016/07/25 15:55:02, Yaron wrote: > null it out. Since we need to call super.onDestroyInternal() at last, early exist doesn't make code simper. https://codereview.chromium.org/2124513002/diff/680001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetectorTest.java (right): https://codereview.chromium.org/2124513002/diff/680001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetectorTest.java:29: public class ManifestUpgradeDetectorTest extends ChromeTabbedActivityTestBase { On 2016/07/25 17:35:53, pkotwicz wrote: > Please add a comment where that the field values come from {@link > WEBAPK_WEB_MANIFEST_URL} Done. https://codereview.chromium.org/2124513002/diff/680001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetectorTest.java:41: private ManifestUpgradeDetector mDetector; On 2016/07/25 17:35:53, pkotwicz wrote: > Nit: Change the type to TestManifestUpgradeDetector. This should enable removing > several casts Done. https://codereview.chromium.org/2124513002/diff/680001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetectorTest.java:43: On 2016/07/25 17:35:53, pkotwicz wrote: > Nit: Please add JavaDoc for the test class. Done. https://codereview.chromium.org/2124513002/diff/680001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetectorTest.java:45: private boolean mIsUpgraded; On 2016/07/25 17:35:53, pkotwicz wrote: > Please make these variables public to reflect how they are being used > > I looked it up. Here is an explanation of why an outer class can access an inner > class' privates > http://stackoverflow.com/questions/19747812/why-can-the-private-member-of-an-... Done. https://codereview.chromium.org/2124513002/diff/680001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetectorTest.java:48: private boolean mIsDataAvaliable = false; On 2016/07/25 17:35:53, pkotwicz wrote: > Nit: new line Done. https://codereview.chromium.org/2124513002/diff/680001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetectorTest.java:61: mIsDataAvaliable = true; On 2016/07/25 17:35:53, pkotwicz wrote: > mIsDataAvaliable -> mIsDataAvailable Done. https://codereview.chromium.org/2124513002/diff/680001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetectorTest.java:62: mInfo = newInfo; On 2016/07/25 17:35:53, pkotwicz wrote: > This does not seem to be used Removed, since most of the tests that used it had been removed. https://codereview.chromium.org/2124513002/diff/680001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetectorTest.java:86: On 2016/07/25 17:35:53, pkotwicz wrote: > Please add a comment say that this creates an intent with the properties in > chrome/test/data/webapps/manifest.json Done. https://codereview.chromium.org/2124513002/diff/680001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetectorTest.java:89: intent.setData(Uri.parse(WebappActivity.WEBAPP_SCHEME + "://" + WEBAPK_ID)); On 2016/07/25 17:35:53, pkotwicz wrote: > Is setting the data necessary? It also works without it. removed. https://codereview.chromium.org/2124513002/diff/680001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetectorTest.java:108: final Tab tab = getActivity().getActivityTab(); On 2016/07/25 17:35:53, pkotwicz wrote: > Can you please add a comment why ManifestUpgradeDetector#start() must be called > on the UI thread? (I actually do not know why it is necessary) > > The ManifestUpgradeDetector constructor and > ManifestUpgradeDetector#setMetadataForTesting() do not need to be called on the > UI thread. Can you move those outside of the ThreadUtils#runOnUiThreadBlocking() > block? Move the rest and add a comment for detector.start(). I think it is because it makes JNI calls to initialize pointer in c++. https://codereview.chromium.org/2124513002/diff/680001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetectorTest.java:112: mDetector = new TestManifestUpgradeDetector(tab, WebappInfo.create(createIntent())); On 2016/07/25 17:35:53, pkotwicz wrote: > This function is not using the intent parameter at all. > > I think that you should: > - Use the passed in WebappInfo (not an intent) > - Pass in the startUrl as a parameter as well > > Document that the "passed in WebappInfo" and the "passed in start URL" are "the > old WebAPK data" This refactoring is due to the removing of other tests. I am ok with it for now. https://codereview.chromium.org/2124513002/diff/680001/chrome/browser/android... File chrome/browser/android/webapk/manifest_upgrade_detector.cc (right): https://codereview.chromium.org/2124513002/diff/680001/chrome/browser/android... chrome/browser/android/webapk/manifest_upgrade_detector.cc:108: // change, we will treat the new Web Manifest as the one of another WebAPK. On 2016/07/25 18:00:21, Yaron wrote: > On 2016/07/25 17:35:53, pkotwicz wrote: > > Yaron do you have a good comment to put here to explain why we early return > when > > |web_manifest_url_| != manifest_url > > I think Xi's comment is correct. As currently planned, when you install a > WebApk, it will not follow you if the manifest url changes. But we have to > reflect the fact that the old WebApk was launched for the previous manifest url > and we wait to see if the user discovers a new version of it. > > Actually if it doesn't match, it really shouldn't return FetchedManifestEmpty - > it's a no-op. Good catch, the FetchedManifestEmpty is only covered the case of manifest.IsEmpty():( https://codereview.chromium.org/2124513002/diff/680001/chrome/test/data/webap... File chrome/test/data/webapps/manifest_test_page_test_start_url_change.html (right): https://codereview.chromium.org/2124513002/diff/680001/chrome/test/data/webap... chrome/test/data/webapps/manifest_test_page_test_start_url_change.html:1: <html> On 2016/07/25 17:35:53, pkotwicz wrote: > It should be possible to delete this file now? I am afraid not, the test testStartUrlChangeShouldReturnUpgradeTrue needs to load this URL.
lgtm with one comment change and assumign Peter is happy https://codereview.chromium.org/2124513002/diff/680001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetector.java (right): https://codereview.chromium.org/2124513002/diff/680001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetector.java:53: public void setMetadataForTesting(String startUrl) { On 2016/07/25 20:01:37, Xi Han wrote: > On 2016/07/25 15:55:02, Yaron wrote: > > If you rename this to: overrideMetadataForTesting and have it set a bool, you > > can remove the dependency on CommandLine and the new switch > > Done. I meant that you can collapse these two into one function. if you are setting the start url for testing, you are implicitly overriding the metadata (since that's where startUrl would typically come from). But I guess if you re-use a detector across tests it could leak more easily so I'm fine with it. https://codereview.chromium.org/2124513002/diff/760001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetectorTest.java (right): https://codereview.chromium.org/2124513002/diff/760001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetectorTest.java:120: // JNI call to initialize the detector in c++. I don't think that's the reason but mDetector.start interacts with several objects that are typically used on UI thread only.
LGTM!!!!!!!!!!!!! Thank you for bearing with me https://codereview.chromium.org/2124513002/diff/680001/chrome/test/data/webap... File chrome/test/data/webapps/manifest_test_page_test_start_url_change.html (right): https://codereview.chromium.org/2124513002/diff/680001/chrome/test/data/webap... chrome/test/data/webapps/manifest_test_page_test_start_url_change.html:1: <html> You are right. You need the test page for your test. https://codereview.chromium.org/2124513002/diff/760001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetector.java (right): https://codereview.chromium.org/2124513002/diff/760001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetector.java:44: * A flag of whether metadata is overriding via {@link set*ForTesting()}, rather than loading Nits: - "of whether" -> "for whether" - "overriding via" -> "set via" - "rather than loading from" -> "rather than set from" https://codereview.chromium.org/2124513002/diff/760001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetectorTest.java (right): https://codereview.chromium.org/2124513002/diff/760001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetectorTest.java:104: * Waits until the manifest upgrade detector gets the new fetched Web Manifest. How about: "Navigates to {@link startUrl} and waits till the ManifestUpgradeDetector gets the Web Manifest data." https://codereview.chromium.org/2124513002/diff/760001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetectorTest.java:106: * @param startUrl: the start URL of the old Webapp info. How about: "URL to navigate to and start URL of the old Webapp info. https://codereview.chromium.org/2124513002/diff/760001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetectorTest.java:109: loadUrlInNewTab("about:blank"); I don't think that the call to loadUrlInNewTab() is necessary. startMainActivity() loads about:blank and waits for the page to finish loading https://codereview.chromium.org/2124513002/diff/760001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetectorTest.java:120: // JNI call to initialize the detector in c++. Maybe remove the comment. I forgot that we needed to run on the UI thread whenever we do anything UI related. For instance loadUrlInNewTab() runs on the UI thread. ManifestUpgradeDetector#start() runs on the UI thread in prod. It is legal to do JNI calls off of the UI thread. One example is ShortcutHelper. (I probably have misread your comment. My understanding of your comment is "you can't do JNI off of the UI thread") https://codereview.chromium.org/2124513002/diff/760001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetectorTest.java:128: callbackHelper.waitForCallback(curCallCount); Nit: Replace |curCallCount| with 0. That's what everyone does.
Hi Dominick, could you please take a look? Thanks! https://codereview.chromium.org/2124513002/diff/680001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetector.java (right): https://codereview.chromium.org/2124513002/diff/680001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetector.java:53: public void setMetadataForTesting(String startUrl) { On 2016/07/25 22:29:06, Yaron (OOO) wrote: > On 2016/07/25 20:01:37, Xi Han wrote: > > On 2016/07/25 15:55:02, Yaron wrote: > > > If you rename this to: overrideMetadataForTesting and have it set a bool, > you > > > can remove the dependency on CommandLine and the new switch > > > > Done. > > I meant that you can collapse these two into one function. if you are setting > the start url for testing, you are implicitly overriding the metadata (since > that's where startUrl would typically come from). But I guess if you re-use a > detector across tests it could leak more easily so I'm fine with it. Acknowledged. https://codereview.chromium.org/2124513002/diff/760001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetector.java (right): https://codereview.chromium.org/2124513002/diff/760001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetector.java:44: * A flag of whether metadata is overriding via {@link set*ForTesting()}, rather than loading On 2016/07/25 22:32:08, pkotwicz wrote: > Nits: > - "of whether" -> "for whether" > - "overriding via" -> "set via" > - "rather than loading from" -> "rather than set from" Done. https://codereview.chromium.org/2124513002/diff/760001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetectorTest.java (right): https://codereview.chromium.org/2124513002/diff/760001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetectorTest.java:104: * Waits until the manifest upgrade detector gets the new fetched Web Manifest. On 2016/07/25 22:32:08, pkotwicz wrote: > How about: "Navigates to {@link startUrl} and waits till the > ManifestUpgradeDetector gets the Web Manifest data." Done. https://codereview.chromium.org/2124513002/diff/760001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetectorTest.java:106: * @param startUrl: the start URL of the old Webapp info. On 2016/07/25 22:32:08, pkotwicz wrote: > How about: "URL to navigate to and start URL of the old Webapp info. Done. https://codereview.chromium.org/2124513002/diff/760001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetectorTest.java:109: loadUrlInNewTab("about:blank"); On 2016/07/25 22:32:08, pkotwicz wrote: > I don't think that the call to loadUrlInNewTab() is necessary. > startMainActivity() loads about:blank and waits for the page to finish loading You are right, good catch! I thought the tab has to be created via loadUrlInNewTab(), but obvious it doesn't need to. https://codereview.chromium.org/2124513002/diff/760001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetectorTest.java:120: // JNI call to initialize the detector in c++. On 2016/07/25 22:32:08, pkotwicz wrote: > Maybe remove the comment. I forgot that we needed to run on the UI thread > whenever we do anything UI related. For instance loadUrlInNewTab() runs on the > UI thread. ManifestUpgradeDetector#start() runs on the UI thread in prod. > > It is legal to do JNI calls off of the UI thread. One example is ShortcutHelper. > (I probably have misread your comment. My understanding of your comment is "you > can't do JNI off of the UI thread") Removed. I think the key point there isn't to let the code running in UI thread, but to block UI thread until the mDetector.start() finished. Otherwise, tests fails. https://codereview.chromium.org/2124513002/diff/760001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetectorTest.java:128: callbackHelper.waitForCallback(curCallCount); On 2016/07/25 22:32:08, pkotwicz wrote: > Nit: Replace |curCallCount| with 0. That's what everyone does. Done.
Description was changed from ========== Introduce ManifestUpgradeDetector for WebAPK to detect web manifest changes. Some attributes changes in web manifest file will require to re-mint a WebAPK. For example, start Url, background color and Webapp Name. These infos are baked into WebAPK's AndroidManifest.xml. Other attributes, like orientaion etc., their changes are stored in Chrome's metadata, but don't request to re-mint. The ManifestUpgradeDetector fetches the latest web manifest for a WebAPK, compare with cached one and make a decision of whether to request re-mint. The ManifestUpgradeDetector is initilized for WebApkActivity when it needs to check resources changes. Once the detector starts, it observes WebContents's change, and stops when the manifest file of the WebApk is found and fetched. This is because not all of the urls within the WebAPK's scope link to its manifest file. Therefore, the detector needs to follow WebContents's change until find the manifest file. This CL depends on CL(https://codereview.chromium.org/2140273002/) and CL(https://codereview.chromium.org/2147323002/). TEST=org.chromium.chrome.browser.webapps.ManifestUpgradeDetectorTest BUG=624834 ========== to ========== Introduce ManifestUpgradeDetector for WebAPK to detect web manifest changes. The ManifestUpgradeDetector fetches the latest web manifest for a WebAPK, compare with cached one and make a decision of whether to request re-mint. The ManifestUpgradeDetector is initilized for WebApkActivity when it needs to check resources changes. Once the detector starts, it observes WebContents's change, and stops when the manifest file of the WebApk is found and fetched. This is because not all of the urls within the WebAPK's scope link to its manifest file. Therefore, the detector needs to follow WebContents's change until find the manifest file. This CL depends on CL(https://codereview.chromium.org/2140273002/) and CL(https://codereview.chromium.org/2147323002/). TEST=org.chromium.chrome.browser.webapps.ManifestUpgradeDetectorTest BUG=624834 ==========
Description was changed from ========== Introduce ManifestUpgradeDetector for WebAPK to detect web manifest changes. The ManifestUpgradeDetector fetches the latest web manifest for a WebAPK, compare with cached one and make a decision of whether to request re-mint. The ManifestUpgradeDetector is initilized for WebApkActivity when it needs to check resources changes. Once the detector starts, it observes WebContents's change, and stops when the manifest file of the WebApk is found and fetched. This is because not all of the urls within the WebAPK's scope link to its manifest file. Therefore, the detector needs to follow WebContents's change until find the manifest file. This CL depends on CL(https://codereview.chromium.org/2140273002/) and CL(https://codereview.chromium.org/2147323002/). TEST=org.chromium.chrome.browser.webapps.ManifestUpgradeDetectorTest BUG=624834 ========== to ========== Introduce ManifestUpgradeDetector for WebAPK to detect web manifest changes. The ManifestUpgradeDetector fetches the latest web manifest for a WebAPK, compare with cached one and make a decision of whether to request re-mint. The ManifestUpgradeDetector is initilized for WebApkActivity when it needs to check resources changes. Once the detector starts, it observes WebContents's change, and stops when the manifest file of the WebApk is found and fetched. This is because not all of the urls within the WebAPK's scope link to its manifest file. Therefore, the detector needs to follow WebContents's change until find the manifest file. TEST=org.chromium.chrome.browser.webapps.ManifestUpgradeDetectorTest BUG=624834 ==========
Dominick ping.
Sorry for the delay here - been a crazy week! lgtm % nits https://codereview.chromium.org/2124513002/diff/780001/chrome/android/junit/s... File chrome/android/junit/src/org/chromium/chrome/browser/webapps/WebappDataStorageTest.java (right): https://codereview.chromium.org/2124513002/diff/780001/chrome/android/junit/s... chrome/android/junit/src/org/chromium/chrome/browser/webapps/WebappDataStorageTest.java:324: @Test Can you add a brief comment here clarifying the difference between this test and the existing testIntentUpdate() (and also the test below)? As far as I can tell, this is testing that when a WebappDataStorage is created for a WebAPK, it can be restored from an intent correctly. Is that right? https://codereview.chromium.org/2124513002/diff/780001/chrome/android/junit/s... chrome/android/junit/src/org/chromium/chrome/browser/webapps/WebappDataStorageTest.java:365: Nit: extra newline
The CQ bit was checked by hanxi@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from pkotwicz@chromium.org, yfriedman@chromium.org, dominickn@chromium.org Link to the patchset: https://codereview.chromium.org/2124513002/#ps800001 (title: "Remvoe tests in WebappDataStorageTests.")
https://codereview.chromium.org/2124513002/diff/780001/chrome/android/junit/s... File chrome/android/junit/src/org/chromium/chrome/browser/webapps/WebappDataStorageTest.java (right): https://codereview.chromium.org/2124513002/diff/780001/chrome/android/junit/s... chrome/android/junit/src/org/chromium/chrome/browser/webapps/WebappDataStorageTest.java:324: @Test On 2016/07/29 00:03:39, dominickn wrote: > Can you add a brief comment here clarifying the difference between this test and > the existing testIntentUpdate() (and also the test below)? As far as I can tell, > this is testing that when a WebappDataStorage is created for a WebAPK, it can be > restored from an intent correctly. Is that right? This is my bad. I think these two tests must confuse you, since I reverted the changes in WebappDataStorage, but forgot to remove these two tests:( Thank you for putting comments here, and they are removed now:)
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Introduce ManifestUpgradeDetector for WebAPK to detect web manifest changes. The ManifestUpgradeDetector fetches the latest web manifest for a WebAPK, compare with cached one and make a decision of whether to request re-mint. The ManifestUpgradeDetector is initilized for WebApkActivity when it needs to check resources changes. Once the detector starts, it observes WebContents's change, and stops when the manifest file of the WebApk is found and fetched. This is because not all of the urls within the WebAPK's scope link to its manifest file. Therefore, the detector needs to follow WebContents's change until find the manifest file. TEST=org.chromium.chrome.browser.webapps.ManifestUpgradeDetectorTest BUG=624834 ========== to ========== Introduce ManifestUpgradeDetector for WebAPK to detect web manifest changes. The ManifestUpgradeDetector fetches the latest web manifest for a WebAPK, compare with cached one and make a decision of whether to request re-mint. The ManifestUpgradeDetector is initilized for WebApkActivity when it needs to check resources changes. Once the detector starts, it observes WebContents's change, and stops when the manifest file of the WebApk is found and fetched. This is because not all of the urls within the WebAPK's scope link to its manifest file. Therefore, the detector needs to follow WebContents's change until find the manifest file. TEST=org.chromium.chrome.browser.webapps.ManifestUpgradeDetectorTest BUG=624834 ==========
Message was sent while issue was closed.
Committed patchset #14 (id:800001)
Message was sent while issue was closed.
Description was changed from ========== Introduce ManifestUpgradeDetector for WebAPK to detect web manifest changes. The ManifestUpgradeDetector fetches the latest web manifest for a WebAPK, compare with cached one and make a decision of whether to request re-mint. The ManifestUpgradeDetector is initilized for WebApkActivity when it needs to check resources changes. Once the detector starts, it observes WebContents's change, and stops when the manifest file of the WebApk is found and fetched. This is because not all of the urls within the WebAPK's scope link to its manifest file. Therefore, the detector needs to follow WebContents's change until find the manifest file. TEST=org.chromium.chrome.browser.webapps.ManifestUpgradeDetectorTest BUG=624834 ========== to ========== Introduce ManifestUpgradeDetector for WebAPK to detect web manifest changes. The ManifestUpgradeDetector fetches the latest web manifest for a WebAPK, compare with cached one and make a decision of whether to request re-mint. The ManifestUpgradeDetector is initilized for WebApkActivity when it needs to check resources changes. Once the detector starts, it observes WebContents's change, and stops when the manifest file of the WebApk is found and fetched. This is because not all of the urls within the WebAPK's scope link to its manifest file. Therefore, the detector needs to follow WebContents's change until find the manifest file. TEST=org.chromium.chrome.browser.webapps.ManifestUpgradeDetectorTest BUG=624834 Committed: https://crrev.com/e72182e94924a4a8af00b016ed75fb1bdc554500 Cr-Commit-Position: refs/heads/master@{#408636} ==========
Message was sent while issue was closed.
Patchset 14 (id:??) landed as https://crrev.com/e72182e94924a4a8af00b016ed75fb1bdc554500 Cr-Commit-Position: refs/heads/master@{#408636} |
