Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2263673003:
Pass icon and icon murmur2 hash to WebApkInstaller when updating WebAPKs. (Closed)
DescriptionPass icon and icon murmur2 hash to WebApkInstaller when updating WebAPKs.
The icon and icon murmur2 hash are sent to the WebApk Minting Server
when creating/updating a WebAPK. For updating, these two values are
fetched and computed by ManifestUpdateDetector, so WebApkInstaller
doesn't need to download the icon and compute icon hash again.
BUG=639000
Committed: https://crrev.com/b60dd9ba59956d1963c1a5eaef648c1eaa85a3bc
Cr-Commit-Position: refs/heads/master@{#415342}
Patch Set 1 #
Total comments: 20
Patch Set 2 : pkotwicz@'s comments. #
Total comments: 8
Patch Set 3 : Nits. #
Total comments: 2
Patch Set 4 : Rebase. #
Messages
Total messages: 31 (20 generated)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
hanxi@chromium.org changed reviewers: + pkotwicz@chromium.org
Hi Peter, could you please take a look? Thanks!
Just nits! https://codereview.chromium.org/2263673003/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetector.java (right): https://codereview.chromium.org/2263673003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetector.java:25: */ Nit: Can you make this interface public? I do not know what the convention is. My current principle is "not to use package scope unless there is a reason for it" https://codereview.chromium.org/2263673003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetector.java:47: */ Nit: "static class" -> "public static class" (My current principle is to avoid package scope. I do not know whether this principle is correct) https://codereview.chromium.org/2263673003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetector.java:113: Nit: getWebApkPakcageName() -> getWebApkPackageName() https://codereview.chromium.org/2263673003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetector.java:193: // TODO(hanxi): crbug.com/627824. Validate whether the new featched data is Nit: featched -> fetched https://codereview.chromium.org/2263673003/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkUpdateManager.java (right): https://codereview.chromium.org/2263673003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkUpdateManager.java:129: String shortName, String iconUrl, Bitmap icon, long iconMurmur2Hash, int displayMode, Nit: Switch the order of |iconMurmur2Hash| and |icon| to match manifest_upgrade_detector_fetcher.cc https://codereview.chromium.org/2263673003/diff/40001/chrome/android/junit/sr... File chrome/android/junit/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetectorTest.java (right): https://codereview.chromium.org/2263673003/diff/40001/chrome/android/junit/sr... chrome/android/junit/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetectorTest.java:230: FetchedManifestData oldData = createDefaultData(); It feels a bit weird to use FetchedManifestData for the data which is extracted from the WebAPK I would recommend either: - Keeping the Data class both the for "the data extracted from the WebAPK" and "the fetched data" - Creating a new class for "the data extracted from the WebAPK" https://codereview.chromium.org/2263673003/diff/40001/chrome/browser/android/... File chrome/browser/android/webapk/webapk_installer.cc (right): https://codereview.chromium.org/2263673003/diff/40001/chrome/browser/android/... chrome/browser/android/webapk/webapk_installer.cc:207: The purpose of the check is to ensure that we do not pass an invalid URL to WebApkIconHasher::DownloadAndComputeMurmur2Hash() because that causes a crash. Perhaps, move the check to DownloadAndComputeMurmur2Hash() and out of InstallAsyncWithURLRequestContextGetter() and add a comment My suggestion for the comment: "Safeguard. WebApkIconHasher crashes if asked to fetch an invalid URL." https://codereview.chromium.org/2263673003/diff/40001/chrome/browser/android/... File chrome/browser/android/webapk/webapk_installer_unittest.cc (right): https://codereview.chromium.org/2263673003/diff/40001/chrome/browser/android/... chrome/browser/android/webapk/webapk_installer_unittest.cc:103: const int webapk_version = 1; Nit: |webapk_version| -> |kWebApkVersion| https://codereview.chromium.org/2263673003/diff/40001/chrome/browser/android/... chrome/browser/android/webapk/webapk_installer_unittest.cc:104: const long icon_murmur2_hash = 0L; How about: "const std::string kIconMurmur2Hash = "0"; https://codereview.chromium.org/2263673003/diff/40001/chrome/browser/android/... File chrome/browser/android/webapk/webapk_update_manager.cc (right): https://codereview.chromium.org/2263673003/diff/40001/chrome/browser/android/... chrome/browser/android/webapk/webapk_update_manager.cc:97: std::to_string(java_icon_murmur2_hash), I think that using base::Uint64ToString() is preferred. However, I do not know what the difference between base::Uint64ToString() and std::to_string() is Note: Once I use webapk_icon_hasher.cc in manifest_upgrade_detector_fetcher.cc, the hash will be passed as a String to ManifestUpgradeDetector.java
Patchset #2 (id:60001) has been deleted
PTAL, thanks! https://codereview.chromium.org/2263673003/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetector.java (right): https://codereview.chromium.org/2263673003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetector.java:25: */ On 2016/08/19 23:38:17, pkotwicz wrote: > Nit: Can you make this interface public? > > I do not know what the convention is. My current principle is "not to use > package scope unless there is a reason for it" Done. https://codereview.chromium.org/2263673003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetector.java:47: */ On 2016/08/19 23:38:17, pkotwicz wrote: > Nit: "static class" -> "public static class" (My current principle is to avoid > package scope. I do not know whether this principle is correct) I don't know, personally I would prefer to give something that is considered as "internal" a scope that is enough for all its consumers. https://codereview.chromium.org/2263673003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetector.java:113: On 2016/08/19 23:38:17, pkotwicz wrote: > Nit: getWebApkPakcageName() -> getWebApkPackageName() Sorry for the typo. https://codereview.chromium.org/2263673003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetector.java:193: // TODO(hanxi): crbug.com/627824. Validate whether the new featched data is On 2016/08/19 23:38:17, pkotwicz wrote: > Nit: featched -> fetched Done. https://codereview.chromium.org/2263673003/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkUpdateManager.java (right): https://codereview.chromium.org/2263673003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkUpdateManager.java:129: String shortName, String iconUrl, Bitmap icon, long iconMurmur2Hash, int displayMode, On 2016/08/19 23:38:17, pkotwicz wrote: > Nit: Switch the order of |iconMurmur2Hash| and |icon| to match > manifest_upgrade_detector_fetcher.cc Done. https://codereview.chromium.org/2263673003/diff/40001/chrome/android/junit/sr... File chrome/android/junit/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetectorTest.java (right): https://codereview.chromium.org/2263673003/diff/40001/chrome/android/junit/sr... chrome/android/junit/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetectorTest.java:230: FetchedManifestData oldData = createDefaultData(); On 2016/08/19 23:38:17, pkotwicz wrote: > It feels a bit weird to use FetchedManifestData for the data which is extracted > from the WebAPK > > I would recommend either: > - Keeping the Data class both the for "the data extracted from the WebAPK" and > "the fetched data" > - Creating a new class for "the data extracted from the WebAPK" I add a WebappInfoCreationData to represent the "the data extracted from the WebAPK". https://codereview.chromium.org/2263673003/diff/40001/chrome/browser/android/... File chrome/browser/android/webapk/webapk_installer.cc (right): https://codereview.chromium.org/2263673003/diff/40001/chrome/browser/android/... chrome/browser/android/webapk/webapk_installer.cc:207: On 2016/08/19 23:38:17, pkotwicz wrote: > The purpose of the check is to ensure that we do not pass an invalid URL to > WebApkIconHasher::DownloadAndComputeMurmur2Hash() because that causes a crash. > Perhaps, move the check to DownloadAndComputeMurmur2Hash() and out of > InstallAsyncWithURLRequestContextGetter() and add a comment > > My suggestion for the comment: "Safeguard. WebApkIconHasher crashes if asked to > fetch an invalid URL." Done. https://codereview.chromium.org/2263673003/diff/40001/chrome/browser/android/... File chrome/browser/android/webapk/webapk_installer_unittest.cc (right): https://codereview.chromium.org/2263673003/diff/40001/chrome/browser/android/... chrome/browser/android/webapk/webapk_installer_unittest.cc:103: const int webapk_version = 1; On 2016/08/19 23:38:17, pkotwicz wrote: > Nit: |webapk_version| -> |kWebApkVersion| Done. https://codereview.chromium.org/2263673003/diff/40001/chrome/browser/android/... chrome/browser/android/webapk/webapk_installer_unittest.cc:104: const long icon_murmur2_hash = 0L; On 2016/08/19 23:38:17, pkotwicz wrote: > How about: "const std::string kIconMurmur2Hash = "0"; Done. https://codereview.chromium.org/2263673003/diff/40001/chrome/browser/android/... File chrome/browser/android/webapk/webapk_update_manager.cc (right): https://codereview.chromium.org/2263673003/diff/40001/chrome/browser/android/... chrome/browser/android/webapk/webapk_update_manager.cc:97: std::to_string(java_icon_murmur2_hash), On 2016/08/19 23:38:17, pkotwicz wrote: > I think that using base::Uint64ToString() is preferred. However, I do not know > what the difference between base::Uint64ToString() and std::to_string() is > > Note: Once I use webapk_icon_hasher.cc in manifest_upgrade_detector_fetcher.cc, > the hash will be passed as a String to ManifestUpgradeDetector.java I move the conversion to Java and pass a string directly to here. Thanks for the heads up.
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: This issue passed the CQ dry run.
LGTM with nits https://codereview.chromium.org/2263673003/diff/80001/chrome/android/junit/sr... File chrome/android/junit/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetectorTest.java (right): https://codereview.chromium.org/2263673003/diff/80001/chrome/android/junit/sr... chrome/android/junit/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetectorTest.java:130: * - Data fetched by ManifestUpgradeDetector. Nit: Remove the hyphen. How about: "The FetchedManifestData is the data fetched by ManifestUpgradeDetector." https://codereview.chromium.org/2263673003/diff/80001/chrome/android/junit/sr... chrome/android/junit/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetectorTest.java:138: /** Nit: Remove the hyphen. How about: "The WebappInfoCreationData is the data that the WebAPK was created with." https://codereview.chromium.org/2263673003/diff/80001/chrome/android/junit/sr... chrome/android/junit/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetectorTest.java:147: The function name is not representative of what the function does. Maybe rename the function to populateDataWithDefaults() ? https://codereview.chromium.org/2263673003/diff/80001/chrome/browser/android/... File chrome/browser/android/webapk/webapk_installer_unittest.cc (right): https://codereview.chromium.org/2263673003/diff/80001/chrome/browser/android/... chrome/browser/android/webapk/webapk_installer_unittest.cc:104: const std::string kIconMurmur2Hash = "0"; Nit: Order |kIconMurmur2Hash| before |kWebApkVersion| to match order that the arguments are passed to WebApkInstaller::UpdateAsyncWithURLRequestContextGetter()
Description was changed from ========== Pass icon and icon murmur2 hash to WebApkInstaller when updating WebAPKs. BUG=639000 ========== to ========== Pass icon and icon murmur2 hash to WebApkInstaller when updating WebAPKs. The icon and icon murmur2 hash are sent to the WebApk Minting Server when creating/updating a WebAPK. For updating, these two values are fetched and computed by ManifestUpdateDetector, so WebApkInstaller doesn't need to download the icon and compute icon hash again. BUG=639000 ==========
hanxi@chromium.org changed reviewers: + dominickn@chromium.org
Hi Dominick: could you please take a look? Thanks! https://codereview.chromium.org/2263673003/diff/80001/chrome/android/junit/sr... File chrome/android/junit/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetectorTest.java (right): https://codereview.chromium.org/2263673003/diff/80001/chrome/android/junit/sr... chrome/android/junit/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetectorTest.java:130: * - Data fetched by ManifestUpgradeDetector. On 2016/08/26 21:24:37, pkotwicz wrote: > Nit: Remove the hyphen. > > How about: "The FetchedManifestData is the data fetched by > ManifestUpgradeDetector." Done. https://codereview.chromium.org/2263673003/diff/80001/chrome/android/junit/sr... chrome/android/junit/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetectorTest.java:138: /** On 2016/08/26 21:24:37, pkotwicz wrote: > Nit: Remove the hyphen. > > How about: "The WebappInfoCreationData is the data that the WebAPK was created > with." Done. https://codereview.chromium.org/2263673003/diff/80001/chrome/android/junit/sr... chrome/android/junit/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetectorTest.java:147: On 2016/08/26 21:24:37, pkotwicz wrote: > The function name is not representative of what the function does. Maybe rename > the function to populateDataWithDefaults() ? Done. https://codereview.chromium.org/2263673003/diff/80001/chrome/browser/android/... File chrome/browser/android/webapk/webapk_installer_unittest.cc (right): https://codereview.chromium.org/2263673003/diff/80001/chrome/browser/android/... chrome/browser/android/webapk/webapk_installer_unittest.cc:104: const std::string kIconMurmur2Hash = "0"; On 2016/08/26 21:24:37, pkotwicz wrote: > Nit: Order |kIconMurmur2Hash| before |kWebApkVersion| to match order that the > arguments are passed to > WebApkInstaller::UpdateAsyncWithURLRequestContextGetter() 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: This issue passed the CQ dry run.
lgtm % optional nit https://codereview.chromium.org/2263673003/diff/100001/chrome/android/junit/s... File chrome/android/junit/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetectorTest.java (right): https://codereview.chromium.org/2263673003/diff/100001/chrome/android/junit/s... chrome/android/junit/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetectorTest.java:71: private static class WebappInfoCreationData extends FetchedManifestData {} Nit: is it necessary to have this abstraction? Can you just use FetchedManifestData directly?
Patchset #4 (id:120001) has been deleted
hanxi@chromium.org changed reviewers: + dfalcantara@chromium.org
Hi Dan: I need owners review for: -chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkUpdateManager.java Please take a look, thanks! https://codereview.chromium.org/2263673003/diff/100001/chrome/android/junit/s... File chrome/android/junit/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetectorTest.java (right): https://codereview.chromium.org/2263673003/diff/100001/chrome/android/junit/s... chrome/android/junit/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetectorTest.java:71: private static class WebappInfoCreationData extends FetchedManifestData {} On 2016/08/29 23:54:49, dominickn wrote: > Nit: is it necessary to have this abstraction? Can you just use > FetchedManifestData directly? I have discussed this with Peter, we think FetchedManifestData isn't a good name for the data that is used to generate a WebappInfo. We actually come with two options in this test class: 1) Introduce a Data class, which is exactly the same as FetchedManifestData but different name; It is used for both: data to generate a WebappInfo + fetched data; 2) Introduce WebappInfoCreationData and keep FetchedManifestData. I picked the second one, since it makes the meaning of each data more clear.
OWNERS lgtm
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, dominickn@chromium.org Link to the patchset: https://chromiumcodereview.appspot.com/2263673003/#ps140001 (title: "Rebase.")
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 ========== Pass icon and icon murmur2 hash to WebApkInstaller when updating WebAPKs. The icon and icon murmur2 hash are sent to the WebApk Minting Server when creating/updating a WebAPK. For updating, these two values are fetched and computed by ManifestUpdateDetector, so WebApkInstaller doesn't need to download the icon and compute icon hash again. BUG=639000 ========== to ========== Pass icon and icon murmur2 hash to WebApkInstaller when updating WebAPKs. The icon and icon murmur2 hash are sent to the WebApk Minting Server when creating/updating a WebAPK. For updating, these two values are fetched and computed by ManifestUpdateDetector, so WebApkInstaller doesn't need to download the icon and compute icon hash again. BUG=639000 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:140001)
Message was sent while issue was closed.
Description was changed from ========== Pass icon and icon murmur2 hash to WebApkInstaller when updating WebAPKs. The icon and icon murmur2 hash are sent to the WebApk Minting Server when creating/updating a WebAPK. For updating, these two values are fetched and computed by ManifestUpdateDetector, so WebApkInstaller doesn't need to download the icon and compute icon hash again. BUG=639000 ========== to ========== Pass icon and icon murmur2 hash to WebApkInstaller when updating WebAPKs. The icon and icon murmur2 hash are sent to the WebApk Minting Server when creating/updating a WebAPK. For updating, these two values are fetched and computed by ManifestUpdateDetector, so WebApkInstaller doesn't need to download the icon and compute icon hash again. BUG=639000 Committed: https://crrev.com/b60dd9ba59956d1963c1a5eaef648c1eaa85a3bc Cr-Commit-Position: refs/heads/master@{#415342} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/b60dd9ba59956d1963c1a5eaef648c1eaa85a3bc Cr-Commit-Position: refs/heads/master@{#415342} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
