Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2206493002:
Update WebAPK if homescreen icon changed. (Closed)
DescriptionUpdate WebAPK if homescreen icon changed.
This CL makes ManifestUpgradeDetector refetch the homescreen icon. The icon
specified in the "icons" property of the Web Manifest
https://www.w3.org/TR/appmanifest/ is fetched if specified. If the icon URL
changed or the icon bitmap data has changed an upgrade request is sent to
the WebAPK server.
If the "icons" property of the Web Manifest is empty, the homescreen icon URL
that the WebAPK is currently using is refetched to check whether it has changed.
If it has changed an upgrade request is sent to the WebAPK server. For the sake
of simplicity, the ManifestUpgradeDetector does not determine the page's favicon
and fetch the favicon if the "icons" property of the Web Manifest is empty.
BUG=624834
TEST=ManifestUpgradeDetectorTest.*
Committed: https://crrev.com/3b92dfaaebf06b70f2401faeb29630ad6a076670
Cr-Commit-Position: refs/heads/master@{#412713}
Patch Set 1 : Merge branch 'webapk_updater_images0' into webapk_updater_images2 #
Total comments: 11
Patch Set 2 : Merge branch 'master' into webapk_updater_images2 #Patch Set 3 : Disable http/tests/serviceworker/registration.html #
Total comments: 2
Patch Set 4 : Disable http/tests/serviceworker/registration.html #
Total comments: 4
Patch Set 5 : Merge branch 'master' into webapk_updater_images2 #
Total comments: 2
Patch Set 6 : Merge branch 'master' into webapk_updater_images2 #
Total comments: 8
Patch Set 7 : Merge branch 'master' into webapk_updater_images2 #
Total comments: 2
Patch Set 8 : Merge branch 'master' into webapk_updater_images2 #
Total comments: 1
Patch Set 9 : Merge branch 'master' into webapk_updater_images2 #Messages
Total messages: 42 (12 generated)
Patchset #1 (id:1) has been deleted
Description was changed from ========== Update WebAPK if homescreen icon changed. This CL makes ManifestUpgradeDetector refetch the homescreen icon. The icon specified in the "icons" property of the Web Manifest https://www.w3.org/TR/appmanifest/ is fetched if specified. If the icon URL changed or the icon bitmap data has changed an upgrade request is sent to the WebAPK server. If the "icons" property of the Web Manifest is empty, the homescreen icon URL that the WebAPK is currently using is refetched to check whether it has changed. If it has changed an upgrade request is sent to the WebAPK server. For the sake of simplicity, the ManifestUpgradeDetector does not determine the page's favicon and fetch the favicon if the "icons" property of the Web Manifest is empty. BUG=624834 TEST=ManifestUpgradeDetectorTest.* ========== to ========== Update WebAPK if homescreen icon changed. This CL makes ManifestUpgradeDetector refetch the homescreen icon. The icon specified in the "icons" property of the Web Manifest https://www.w3.org/TR/appmanifest/ is fetched if specified. If the icon URL changed or the icon bitmap data has changed an upgrade request is sent to the WebAPK server. If the "icons" property of the Web Manifest is empty, the homescreen icon URL that the WebAPK is currently using is refetched to check whether it has changed. If it has changed an upgrade request is sent to the WebAPK server. For the sake of simplicity, the ManifestUpgradeDetector does not determine the page's favicon and fetch the favicon if the "icons" property of the Web Manifest is empty. BUG=624834 TEST=ManifestUpgradeDetectorTest.* ==========
pkotwicz@chromium.org changed reviewers: + hanxi@google.com
pkotwicz@chromium.org changed reviewers: + hanxi@chromium.org - hanxi@google.com
Xi can you please take a look?
Just one quick question, why the logic of fetching icon image is put in ShortcutHelper, not ManifestUpgradeFetcher? I assume all the fetching should be handled in the ManifestUpgradeFetcher/ManifestUpgradeDetector. ShortcutHelper is only for adding a shortcut, while the WebAPK's detector works for updating as well.
I actually thought about this question for a long time. ManifestUpgradeDetectorFetcher is for fetching the manifest. ManifestUpgradeDetectorFetcher observes the tab and the web contents. It is kind of weird to add a static function for fetching the app icon to ManifestUpgradeDetectorFetcher. I decided to put the function in ShortcutHelper because ShortcutHelper is the current place for random-webapp-related-static-functions. In particular, ShortcutHelper::FetchSplashScreenImage() is located in ShortcutHelper. I suspect that I will be asked to make ShortcutHelper::FetchSplashScreenImage() called from Java for the sake of consistency. We also need to figure out how the splash screen image will be updated for WebAPKs
The FetchSplashScreenImage is part of the adding shortcut process, but it isn't part of the fetching pipeline of the data for creating the shortcut, so I assume that is why it isn't in the data fetcher, but in ShortcutHelper. However, the icon fetch is part of change-detection-pipeline of ManifestUpgradeDataFetcher, the ManifestUpgradeDataFetcher should notify detector when the icon is fetched, I don't think it's necessary to hop to ShortcutHelper to make the detector know whether needs to request update.
Xi Ping!
Xi Ping!
pkotwicz@chromium.org changed reviewers: + yfriedman@chromium.org
Yaron, can you please take a look? Xi has a huge amount on her plate. She is reponsible for the #1 most critical and difficult task for making the M54 branch. Yaron, can you help out by helping out with some of the code review burden?
Xi just messaged telling me that she is looking at my CL
Mostly just comments. https://codereview.chromium.org/2206493002/diff/20001/chrome/android/junit/sr... File chrome/android/junit/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetectorTest.java (right): https://codereview.chromium.org/2206493002/diff/20001/chrome/android/junit/sr... chrome/android/junit/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetectorTest.java:216: fetchedData.iconMurmur2Hash = WEBAPK_ICON_MURMUR2_HASH + 1; As long as the |iconUrl| changes, the changes of |iconMurmur2Hash| and |icon| aren't necessary here. https://codereview.chromium.org/2206493002/diff/20001/chrome/android/junit/sr... chrome/android/junit/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetectorTest.java:228: * - There are no icons in Web Manifest when WebAPK is generated. Page favicon at If I understand correctly, the |WEBAPK_ICON_URL| was used when the WebAPK was generated, while there are no icons in the manifest now. "There are no icons in Web Manifest when WebAPK is generated." is confusing. How about: "There are no icons in Web Manifest anymore. Page favicon at ..." https://codereview.chromium.org/2206493002/diff/20001/chrome/android/junit/sr... chrome/android/junit/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetectorTest.java:250: * - There are no icons in Web Manifest when WebAPK is generated. Page favicon at Same here. https://codereview.chromium.org/2206493002/diff/20001/chrome/browser/android/... File chrome/browser/android/shortcut_helper.h (right): https://codereview.chromium.org/2206493002/diff/20001/chrome/browser/android/... chrome/browser/android/shortcut_helper.h:79: const base::android::ScopedJavaGlobalRef<jobject> j_callback, java_callback?
Fire a bug crbug.com/636413, since I am not sure whether we should continue fetching the old icon_url baked in the APK when there isn't "icons" in the web manifest. Or we need to request update directly for such case. Sorry for the late reply:)
Patchset #2 (id:40001) has been deleted
Xi can you please take another look? Most of the changes between "Patch Set #1" and "Patch Set #2" are due to rebasing onto master https://codereview.chromium.org/2206493002/diff/20001/chrome/android/junit/sr... File chrome/android/junit/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetectorTest.java (right): https://codereview.chromium.org/2206493002/diff/20001/chrome/android/junit/sr... chrome/android/junit/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetectorTest.java:216: fetchedData.iconMurmur2Hash = WEBAPK_ICON_MURMUR2_HASH + 1; I know that the other changes are unnecessary. I put in the other changes to emulate as accurately as possible the Web Manifest pointing to a new icon URL (It is unlikely that when the Web Manifest points to a new icon URL that the bitmap at the new icon URL would be identical to the bitmap at the old icon URL) https://codereview.chromium.org/2206493002/diff/20001/chrome/android/junit/sr... chrome/android/junit/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetectorTest.java:228: * - There are no icons in Web Manifest when WebAPK is generated. Page favicon at I am actually trying to test the scenario of: - Web Manifest did not have any icon URLs when the WebAPK was generated and still does not have any icon URLs. When the WebAPK was generated we fell back to using the favicon which is located at |WEBAPK_ICON_URL|. The bitmap at |WEBAPK_ICON_URL| has changed. You are right that if I were to write a test for the scenario of: - Web Manifest had an icon when WebAPK was generated. The icon was located at |WEBAPK_ICON_URL|. - Web Manifest was changed and no longer has any icon URLs - The bitmap at |WEBAPK_ICON_URL| has changed since the WebAPK was generated that the test would look exactly the same (lines 234 - 245 would be the same) I fixed up my grammar in the comments. I now use the past tense to refer to things which have occurred in the past. I hope that this makes the comments clearer. https://codereview.chromium.org/2206493002/diff/20001/chrome/browser/android/... File chrome/browser/android/shortcut_helper.h (right): https://codereview.chromium.org/2206493002/diff/20001/chrome/browser/android/... chrome/browser/android/shortcut_helper.h:79: const base::android::ScopedJavaGlobalRef<jobject> j_callback, On 2016/08/10 15:49:19, Xi Han wrote: > java_callback? Done.
https://codereview.chromium.org/2206493002/diff/20001/chrome/android/junit/sr... File chrome/android/junit/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetectorTest.java (right): https://codereview.chromium.org/2206493002/diff/20001/chrome/android/junit/sr... chrome/android/junit/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetectorTest.java:216: fetchedData.iconMurmur2Hash = WEBAPK_ICON_MURMUR2_HASH + 1; On 2016/08/10 19:03:47, pkotwicz wrote: > I know that the other changes are unnecessary. I put in the other changes to > emulate as accurately as possible the Web Manifest pointing to a new icon URL > (It is unlikely that when the Web Manifest points to a new icon URL that the > bitmap at the new icon URL would be identical to the bitmap at the old icon URL) We should avoid to add unnecessary changes to the test. If the test fails due to these changes, the test name won't reflect the realy reason it fails. https://codereview.chromium.org/2206493002/diff/20001/chrome/android/junit/sr... chrome/android/junit/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetectorTest.java:228: * - There are no icons in Web Manifest when WebAPK is generated. Page favicon at On 2016/08/10 19:03:47, pkotwicz wrote: > I am actually trying to test the scenario of: > - Web Manifest did not have any icon URLs when the WebAPK was generated and > still does not have any icon URLs. When the WebAPK was generated we fell back to > using the favicon which is located at |WEBAPK_ICON_URL|. The bitmap at > |WEBAPK_ICON_URL| has changed. > > You are right that if I were to write a test for the scenario of: > - Web Manifest had an icon when WebAPK was generated. The icon was located at > |WEBAPK_ICON_URL|. > - Web Manifest was changed and no longer has any icon URLs > - The bitmap at |WEBAPK_ICON_URL| has changed since the WebAPK was generated > > that the test would look exactly the same (lines 234 - 245 would be the same) > > I fixed up my grammar in the comments. I now use the past tense to refer to > things which have occurred in the past. I hope that this makes the comments > clearer. Hmmm, you still use the default data for the old WebAPK, but give it a different interpretation. It is not easy to understand that from the test description. It would be more clear if the test has a data for old WebAPK, and we can set like: icon_url = webapk_favicon_url; icon = favicon_icon; and then initializes the fetchedData. I think then the tests will look like how you designed. At least, add a TODO to add "mutable" data for old WebAPK.
Xi, can you please take another look? https://codereview.chromium.org/2206493002/diff/20001/chrome/android/junit/sr... File chrome/android/junit/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetectorTest.java (right): https://codereview.chromium.org/2206493002/diff/20001/chrome/android/junit/sr... chrome/android/junit/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetectorTest.java:216: fetchedData.iconMurmur2Hash = WEBAPK_ICON_MURMUR2_HASH + 1; Ok, removed the unnecessary changes https://codereview.chromium.org/2206493002/diff/20001/chrome/android/junit/sr... chrome/android/junit/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetectorTest.java:228: * - There are no icons in Web Manifest when WebAPK is generated. Page favicon at I have made the old data "mutable" I have made the old data and the new data use a variable named "faviconUrl" instead of a variable named "WEBAPK_ICON_URL"
https://codereview.chromium.org/2206493002/diff/80001/chrome/android/junit/sr... File chrome/android/junit/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetectorTest.java (right): https://codereview.chromium.org/2206493002/diff/80001/chrome/android/junit/sr... chrome/android/junit/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetectorTest.java:257: TestManifestUpgradeDetector detector = createDetector(fetchedData); createDetector(oldData, fetchedData)? https://codereview.chromium.org/2206493002/diff/80001/chrome/android/junit/sr... chrome/android/junit/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetectorTest.java:286: TestManifestUpgradeDetector detector = createDetectorWithFetchedData(fetchedData); createDetector(oldData, fetchedData)?
Thanks for updating the tests!
Sorry, I should have looked at the tests more closely. Xi can you please take another look?
lgtm
pkotwicz@chromium.org changed reviewers: + dominickn@chromium.org
Dominick, can you please take a look at the changes in ShortcutHelper.java and shortcut_helper.* ?
Summary: I'm getting concerned about the amount of duplication in the upgrade detector pathway compared to the installation pathway. Because the upgrade detector shares almost no code with installation, it seems like it would be very easy to accidentally skip a step (e.g. ShortcutHelper#createHomscreenIconFromWebIcon) when upgrading a WebAPK. Is it possible to use InstallableManager like the add to homescreen flow now uses? https://codereview.chromium.org/2206493002/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java (right): https://codereview.chromium.org/2206493002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java:431: * Fetches the image at {@link iconUrl} and scales it so that it can be used on the homescreen. This comment is confusing - does nativeFetchHomescreenImage do any scaling? Is the icon actually used on the homescreen? It doesn't seem to pass through ShortcutHelper#createHomescreenIconFromWebIcon, where all A2HS icons are sent for optional padding before being dropped in the launcher. https://codereview.chromium.org/2206493002/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetector.java (right): https://codereview.chromium.org/2206493002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetector.java:166: * - The page's favicon was used as the WebAPK's homescreen icon when the WebAPK was Should a WebAPK be generated if we used a page's favicon instead of a manifest icon? If we're using the same criteria as we do for app banners, then we shouldn't generate WebAPKs for this. It would also greatly simplify the ManifestUpgradeDetector because you would be guaranteed that a manifest icon was used (and hence you could just run InstallableManager.GetData). https://codereview.chromium.org/2206493002/diff/100001/chrome/browser/android... File chrome/browser/android/shortcut_helper.cc (right): https://codereview.chromium.org/2206493002/diff/100001/chrome/browser/android... chrome/browser/android/shortcut_helper.cc:43: const uint32_t kMurmur2HashSeed = 0; Should this be uint64_t? I guess the seed is 0 so it doesn't really matter. https://codereview.chromium.org/2206493002/diff/100001/chrome/browser/android... chrome/browser/android/shortcut_helper.cc:328: * can be used on the homscreen. Nit: Use //, not Java-style comment strings Nit: "homscreen" -> "homescreen" Also, see the note in ShortcutHelper.java about the comment being confusing. https://codereview.chromium.org/2206493002/diff/100001/chrome/browser/android... File chrome/browser/android/webapk/manifest_upgrade_detector_fetcher.cc (right): https://codereview.chromium.org/2206493002/diff/100001/chrome/browser/android... chrome/browser/android/webapk/manifest_upgrade_detector_fetcher.cc:93: web_contents()->GetManifest( Now that InstallableManager has landed, it would be ideal if this class could use it so we can reduce the amount of boilerplate fetching code. As I understand it, the flow currently goes: 1. [Java] Start the native side 2. [C++] GetManifest, ask Java for icon sizes 3. [Java] Return icon sizes 4. [C++] Select icon, update shortcut info, send to Java 5. [Java] Create FetchedManifestData, call back to C++ to download icon 6. [C++] Download icon, calculate hash, send to Java 7. [Java] Compare if we need to do an upgrade. Can this become: 1 [Java] Start the native side, sending over the icon sizes 2. [C++] Run installable manager, including downloading an icon and computing hash. Send everything to Java 3. [Java] Compare if we need to do an upgrade
Summary: I'm getting concerned about the amount of duplication in the upgrade detector pathway compared to the installation pathway. Because the upgrade detector shares almost no code with installation, it seems like it would be very easy to accidentally skip a step (e.g. ShortcutHelper#createHomscreenIconFromWebIcon) when upgrading a WebAPK. Is it possible to use InstallableManager like the add to homescreen flow now uses? https://codereview.chromium.org/2206493002/diff/100001/chrome/browser/android... File chrome/browser/android/shortcut_helper.cc (right): https://codereview.chromium.org/2206493002/diff/100001/chrome/browser/android... chrome/browser/android/shortcut_helper.cc:43: const uint32_t kMurmur2HashSeed = 0; Should this be uint64_t? I guess the seed is 0 so it doesn't really matter. https://codereview.chromium.org/2206493002/diff/100001/chrome/browser/android... chrome/browser/android/shortcut_helper.cc:328: * can be used on the homscreen. Nit: Use //, not Java-style comment strings Nit: "homscreen" -> "homescreen" Also, see the note in ShortcutHelper.java about the comment being confusing. https://codereview.chromium.org/2206493002/diff/100001/chrome/browser/android... File chrome/browser/android/webapk/manifest_upgrade_detector_fetcher.cc (right): https://codereview.chromium.org/2206493002/diff/100001/chrome/browser/android... chrome/browser/android/webapk/manifest_upgrade_detector_fetcher.cc:93: web_contents()->GetManifest( Now that InstallableManager has landed, it would be ideal if this class could use it so we can reduce the amount of boilerplate fetching code. As I understand it, the flow currently goes: 1. [Java] Start the native side 2. [C++] GetManifest, ask Java for icon sizes 3. [Java] Return icon sizes 4. [C++] Select icon, update shortcut info, send to Java 5. [Java] Create FetchedManifestData, call back to C++ to download icon 6. [C++] Download icon, calculate hash, send to Java 7. [Java] Compare if we need to do an upgrade. Can this become: 1 [Java] Start the native side, sending over the icon sizes 2. [C++] Run installable manager, including downloading an icon and computing hash. Send everything to Java 3. [Java] Compare if we need to do an upgrade
Dominick can you please take another look? https://codereview.chromium.org/2206493002/diff/100001/chrome/browser/android... File chrome/browser/android/webapk/manifest_upgrade_detector_fetcher.cc (right): https://codereview.chromium.org/2206493002/diff/100001/chrome/browser/android... chrome/browser/android/webapk/manifest_upgrade_detector_fetcher.cc:93: web_contents()->GetManifest( I have changed manifest_upgrade_detector_fetcher.cc to use InstallableManager. Patch Set #5 changes when a change in the homescreen icon triggers a request for an updated WebAPK (See the diff in ManifestUpgradeDetector#onGotManifestData() from the previous patch set) but I am OK with this change (and so is yfriedman@)
Looks great, non-owner lgtm % nits. Thanks for putting the installable manager in here, I hope it's helping you. :) Some things for the future CLs on top of this one: - making sure a WebAPK isn't generated in the A2HS flow when a favicon is chosen instead of a manifest icon - making sure that the icon used for an upgrade here is routed through ShortcutHelper#createHomescreenIconFromWebIcon
Forgot to publish nits. https://codereview.chromium.org/2206493002/diff/120001/chrome/browser/android... File chrome/browser/android/webapk/manifest_upgrade_detector_fetcher.cc (right): https://codereview.chromium.org/2206493002/diff/120001/chrome/browser/android... chrome/browser/android/webapk/manifest_upgrade_detector_fetcher.cc:107: params.ideal_icon_size_in_dp = Nit: Perhaps add TODO: when WebAPKs are restricted to sites which have a SW and installable manifest, add params.check_installable = true https://codereview.chromium.org/2206493002/diff/140001/chrome/browser/android... File chrome/browser/android/webapk/manifest_upgrade_detector_fetcher.cc (right): https://codereview.chromium.org/2206493002/diff/140001/chrome/browser/android... chrome/browser/android/webapk/manifest_upgrade_detector_fetcher.cc:7: #include <jni.h> Nit: #include <vector> https://codereview.chromium.org/2206493002/diff/140001/chrome/browser/android... chrome/browser/android/webapk/manifest_upgrade_detector_fetcher.cc:10: #include "chrome/browser/android/shortcut_helper.h" Nit: I couldn't see where ShortcutHelper is being used in this file now - remove the include? https://codereview.chromium.org/2206493002/diff/140001/chrome/browser/android... chrome/browser/android/webapk/manifest_upgrade_detector_fetcher.cc:106: InstallableParams params; Optional nit: put the construction of params in a function in the anonymous namespace. https://codereview.chromium.org/2206493002/diff/140001/chrome/browser/android... chrome/browser/android/webapk/manifest_upgrade_detector_fetcher.cc:107: params.ideal_icon_size_in_dp = Optional nit: make ideal_icon_size_in_dp and minimum_icon_size_in_dp members of this class, and pass them in from Java via Initialize(ShortcutHelper.getIdealHomescreenIconSizeInDp(), ShortcutHelper.getMinimumHomescreenIconSizeInDp()). https://codereview.chromium.org/2206493002/diff/140001/chrome/browser/android... chrome/browser/android/webapk/manifest_upgrade_detector_fetcher.cc:157: if (icon_bitmap.getSize()) { Perhaps call !icon_bitmap.drawsNothing() ? That is consistent with app banners and should subsume the getSize check.
pkotwicz@chromium.org changed reviewers: + dfalcantara@chromium.org
dfalcantara@ can you please take a look at the changes in chrome/android/java/src/org/chromium/chrome/browser/webapps https://codereview.chromium.org/2206493002/diff/120001/chrome/browser/android... File chrome/browser/android/webapk/manifest_upgrade_detector_fetcher.cc (right): https://codereview.chromium.org/2206493002/diff/120001/chrome/browser/android... chrome/browser/android/webapk/manifest_upgrade_detector_fetcher.cc:107: params.ideal_icon_size_in_dp = There's already a TODO w.r.t to this in ManifestUpgradeDetector.java http://crbug.com/627824 has been filed about this https://codereview.chromium.org/2206493002/diff/140001/chrome/browser/android... File chrome/browser/android/webapk/manifest_upgrade_detector_fetcher.cc (right): https://codereview.chromium.org/2206493002/diff/140001/chrome/browser/android... chrome/browser/android/webapk/manifest_upgrade_detector_fetcher.cc:7: #include <jni.h> On 2016/08/16 00:12:45, dominickn wrote: > Nit: #include <vector> Done. https://codereview.chromium.org/2206493002/diff/140001/chrome/browser/android... chrome/browser/android/webapk/manifest_upgrade_detector_fetcher.cc:10: #include "chrome/browser/android/shortcut_helper.h" We use ShortcutHelper to get the homescreen icon size in lines 108 and 110 https://codereview.chromium.org/2206493002/diff/140001/chrome/browser/android... chrome/browser/android/webapk/manifest_upgrade_detector_fetcher.cc:157: if (icon_bitmap.getSize()) { Yes, I need to do this. I prefer to do this in a separate CL though. I have filed http://crbug.com/636934 with respect to this.
https://chromiumcodereview.appspot.com/2206493002/diff/160001/chrome/android/... File chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetector.java (right): https://chromiumcodereview.appspot.com/2206493002/diff/160001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetector.java:83: private static long getLongFromBundle(Bundle bundle, String key) { I'm confused about why you think something as critical as Bundle#getLong() is broken for any reasonable values (everything less than Float.MAX_VALUE returns 0?). Do you have specific examples and code snippets showing how the standard code breaks? android.os.BaseBundle doesn't have any references to Floats.
Dan, can you please take another look? https://codereview.chromium.org/2206493002/diff/160001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetector.java (right): https://codereview.chromium.org/2206493002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetector.java:83: private static long getLongFromBundle(Bundle bundle, String key) { Thank you for asking. It looks like my comment was partially incorrect. Sorry for the confusion. I am talking specifically about extracting <meta-data> values from the Bundle. https://developer.android.com/guide/topics/manifest/meta-data-element.html Based on the documentation, it looks like only Bundle#getInt() and Bundle#getFloat() can be used for extracting <meta-data> values. Bundle#getFloat() returns 0 unless the value > Integer.MAX_VALUE We can't use Bundle#getFloat() because floats should not be used for precise values. Local testing confirms this. Previously, I had thought that Bundle#getLong() sometimes returns the <meta-data> value but this does not seem to be the case based on today's testing.
lgtm % comment https://codereview.chromium.org/2206493002/diff/180001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetector.java (right): https://codereview.chromium.org/2206493002/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetector.java:82: * could not be parsed. Link to the article you mentioned, and change the comment to indicate why you're not using Bundle#getLong() explicitly here. I could see that being accidentally changed in the future.
The CQ bit was checked by pkotwicz@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from hanxi@chromium.org, dominickn@chromium.org, dfalcantara@chromium.org Link to the patchset: https://codereview.chromium.org/2206493002/#ps200001 (title: "Merge branch 'master' into webapk_updater_images2")
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 ========== Update WebAPK if homescreen icon changed. This CL makes ManifestUpgradeDetector refetch the homescreen icon. The icon specified in the "icons" property of the Web Manifest https://www.w3.org/TR/appmanifest/ is fetched if specified. If the icon URL changed or the icon bitmap data has changed an upgrade request is sent to the WebAPK server. If the "icons" property of the Web Manifest is empty, the homescreen icon URL that the WebAPK is currently using is refetched to check whether it has changed. If it has changed an upgrade request is sent to the WebAPK server. For the sake of simplicity, the ManifestUpgradeDetector does not determine the page's favicon and fetch the favicon if the "icons" property of the Web Manifest is empty. BUG=624834 TEST=ManifestUpgradeDetectorTest.* ========== to ========== Update WebAPK if homescreen icon changed. This CL makes ManifestUpgradeDetector refetch the homescreen icon. The icon specified in the "icons" property of the Web Manifest https://www.w3.org/TR/appmanifest/ is fetched if specified. If the icon URL changed or the icon bitmap data has changed an upgrade request is sent to the WebAPK server. If the "icons" property of the Web Manifest is empty, the homescreen icon URL that the WebAPK is currently using is refetched to check whether it has changed. If it has changed an upgrade request is sent to the WebAPK server. For the sake of simplicity, the ManifestUpgradeDetector does not determine the page's favicon and fetch the favicon if the "icons" property of the Web Manifest is empty. BUG=624834 TEST=ManifestUpgradeDetectorTest.* ==========
Message was sent while issue was closed.
Committed patchset #9 (id:200001)
Message was sent while issue was closed.
Description was changed from ========== Update WebAPK if homescreen icon changed. This CL makes ManifestUpgradeDetector refetch the homescreen icon. The icon specified in the "icons" property of the Web Manifest https://www.w3.org/TR/appmanifest/ is fetched if specified. If the icon URL changed or the icon bitmap data has changed an upgrade request is sent to the WebAPK server. If the "icons" property of the Web Manifest is empty, the homescreen icon URL that the WebAPK is currently using is refetched to check whether it has changed. If it has changed an upgrade request is sent to the WebAPK server. For the sake of simplicity, the ManifestUpgradeDetector does not determine the page's favicon and fetch the favicon if the "icons" property of the Web Manifest is empty. BUG=624834 TEST=ManifestUpgradeDetectorTest.* ========== to ========== Update WebAPK if homescreen icon changed. This CL makes ManifestUpgradeDetector refetch the homescreen icon. The icon specified in the "icons" property of the Web Manifest https://www.w3.org/TR/appmanifest/ is fetched if specified. If the icon URL changed or the icon bitmap data has changed an upgrade request is sent to the WebAPK server. If the "icons" property of the Web Manifest is empty, the homescreen icon URL that the WebAPK is currently using is refetched to check whether it has changed. If it has changed an upgrade request is sent to the WebAPK server. For the sake of simplicity, the ManifestUpgradeDetector does not determine the page's favicon and fetch the favicon if the "icons" property of the Web Manifest is empty. BUG=624834 TEST=ManifestUpgradeDetectorTest.* Committed: https://crrev.com/3b92dfaaebf06b70f2401faeb29630ad6a076670 Cr-Commit-Position: refs/heads/master@{#412713} ==========
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/3b92dfaaebf06b70f2401faeb29630ad6a076670 Cr-Commit-Position: refs/heads/master@{#412713} |
