Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2676863002:
Update WebApkInstaller to support badge icon in installation. (Closed)
DescriptionUpdate WebApkInstaller to support badge icon in installation.
This CL updates WebApkInstaller so that the murmur2 hash of the best
badge icon would be calculated and passed to the WAM server in WebAPK
proto along with the url of the best badge icon.
BUG=649771
Review-Url: https://codereview.chromium.org/2676863002
Cr-Commit-Position: refs/heads/master@{#462963}
Committed: https://chromium.googlesource.com/chromium/src/+/1fd959d6b3a68e527f7b2be9e9ef93d996839c00
Patch Set 1 : Revert git cl format #
Total comments: 6
Patch Set 2 : Rebase #
Total comments: 17
Patch Set 3 : Addressing comments #
Total comments: 16
Patch Set 4 : Addressing comments #
Total comments: 10
Patch Set 5 : Addressing comments: modifying tests, etc. #
Total comments: 6
Patch Set 6 : Addressing commentswq #Patch Set 7 : Addressing commentswq #
Total comments: 10
Patch Set 8 : addressing comments #Messages
Total messages: 76 (53 generated)
The CQ bit was checked by zpeng@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...
Patchset #3 (id:40001) has been deleted
Patchset #2 (id:20001) has been deleted
Patchset #1 (id:1) has been deleted
Patchset #1 (id:60001) has been deleted
The CQ bit was checked by zpeng@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...
Patchset #1 (id:80001) has been deleted
The CQ bit was checked by zpeng@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...
Description was changed from ========== Installer WIP BUG= ========== to ========== Update WebApkInstaller to support badge icon in installation. This CL updates WebApkInstaller so that the murmur2 hash of the best badge icon would be calculated and passed to the WAM server in WebAPK proto along with the url of the best badge icon. BUG=649771 ==========
zpeng@chromium.org changed reviewers: + dominickn@chromium.org, pkotwicz@chromium.org
Hi Dominick & Peter, PTAL. Thanks!
https://codereview.chromium.org/2676863002/diff/100001/chrome/browser/android... File chrome/browser/android/webapk/webapk_installer.cc (right): https://codereview.chromium.org/2676863002/diff/100001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer.cc:161: std::string best_badge_icon_url = This variable looks redundant (you already have it at line 156?) https://codereview.chromium.org/2676863002/diff/100001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer.cc:518: void WebApkInstaller::DownloadBadgeIconAndComputeMurmur2Hash() { Instead of mostly duplicating DownloadPrimaryIconAndComputeMurmur2Hash(), can you make it something like: DownloadIconAndComputeMurmur2Hash(const GURL& icon_url, std::string* hash_result); And then explicitly call it twice in ::InstallAsync()? At the very least, the downloading of the icon and hash computation should be identical and thus one method. The only difference is where you save the final result. https://codereview.chromium.org/2676863002/diff/100001/chrome/browser/android... File chrome/browser/android/webapk/webapk_installer.h (right): https://codereview.chromium.org/2676863002/diff/100001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer.h:99: // TODO TODO what?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Patchset #2 (id:120001) has been deleted
Thanks Dominick & Peter! PTAL https://codereview.chromium.org/2676863002/diff/100001/chrome/browser/android... File chrome/browser/android/webapk/webapk_installer.cc (right): https://codereview.chromium.org/2676863002/diff/100001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer.cc:161: std::string best_badge_icon_url = On 2017/02/23 02:08:52, dominickn wrote: > This variable looks redundant (you already have it at line 156?) Oops. Thanks! https://codereview.chromium.org/2676863002/diff/100001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer.cc:518: void WebApkInstaller::DownloadBadgeIconAndComputeMurmur2Hash() { On 2017/02/23 02:08:52, dominickn wrote: > Instead of mostly duplicating DownloadPrimaryIconAndComputeMurmur2Hash(), can > you make it something like: > > DownloadIconAndComputeMurmur2Hash(const GURL& icon_url, std::string* > hash_result); > > And then explicitly call it twice in ::InstallAsync()? > > At the very least, the downloading of the icon and hash computation should be > identical and thus one method. The only difference is where you save the final > result. After the refactoring of WebApkIconHasher, this has become much cleaner. WDUT? https://codereview.chromium.org/2676863002/diff/100001/chrome/browser/android... File chrome/browser/android/webapk/webapk_installer.h (right): https://codereview.chromium.org/2676863002/diff/100001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer.h:99: // TODO On 2017/02/23 02:08:52, dominickn wrote: > TODO what? Oops.
Still looking at the CL. Some initial comments https://codereview.chromium.org/2676863002/diff/140001/chrome/browser/android... File chrome/browser/android/webapk/webapk_installer.cc (right): https://codereview.chromium.org/2676863002/diff/140001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer.cc:180: image->set_hash(entry.second); Can we simplify this logic to: if (entry.first == shortcut_info.best_primary_icon_url.spec()) { SetImageData(image, primary_icon); image->add_usages(webapk::Image::PRIMARY_ICON); } else { SetImageData(image, badge_icon); image->add_usages(webapk::Image::BADGE_ICON); } where SetImageData() is a new function in the anonymous namespace https://codereview.chromium.org/2676863002/diff/140001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer.cc:499: MapIconUrlToMurmur2Hash(); Can we keep the flow linear and call OnGotBadgeIconMurmur2Hash() with an empty hash instead? This would enable you to collapse OnGotBadgeIconMurmur2Hash() and MapIconUrlToMurmur2Hash() into a single function
Here is the rest of the comments https://codereview.chromium.org/2676863002/diff/140001/chrome/browser/android... File chrome/browser/android/shortcut_helper.h (right): https://codereview.chromium.org/2676863002/diff/140001/chrome/browser/android... chrome/browser/android/shortcut_helper.h:72: // created by |AddToLauncherWithSkBitmap|; this method should be passed as a Nit: |AddToLauncherWithSkBitmap| -> AddToLauncherWithSkBitmap() https://codereview.chromium.org/2676863002/diff/140001/chrome/browser/android... File chrome/browser/android/webapk/webapk_installer.cc (right): https://codereview.chromium.org/2676863002/diff/140001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer.cc:265: context, shortcut_info, primary_icon, empty_badge_icon); I assume that passing a badge icon for updates is part of a follow up CL https://codereview.chromium.org/2676863002/diff/140001/chrome/browser/android... File chrome/browser/android/webapk/webapk_installer_unittest.cc (right): https://codereview.chromium.org/2676863002/diff/140001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer_unittest.cc:390: SetBestBadgeIconUrl(best_badge_icon_url); Nit: You can merge lines 389 and 390 https://codereview.chromium.org/2676863002/diff/140001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer_unittest.cc:494: runner->BuildSync(GURL(""), GURL(""), icon_url_to_murmur2_hash, Nit: You can use the no-arg constructor of GURL() instead https://codereview.chromium.org/2676863002/diff/140001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer_unittest.cc:542: ASSERT_EQ(2, manifest.icons_size()); Don't you need to update this test? i.e. ASSERT_EQ(3, manifest.icons_size()) etc. https://codereview.chromium.org/2676863002/diff/140001/chrome/browser/android... File chrome/browser/android/webapps/add_to_homescreen_manager.cc (right): https://codereview.chromium.org/2676863002/diff/140001/chrome/browser/android... chrome/browser/android/webapps/add_to_homescreen_manager.cc:197: base::MakeUnique<SkBitmap>(primary_icon), You don't need to do base::MakeUnique<SkBitmap> for SkBitmaps. SkBitmaps are cheeply copyable (The Chrome codebase is written with that assumption)
https://codereview.chromium.org/2676863002/diff/140001/chrome/browser/android... File chrome/browser/android/webapk/webapk_installer.cc (right): https://codereview.chromium.org/2676863002/diff/140001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer.cc:265: context, shortcut_info, primary_icon, empty_badge_icon); After some thought, we actually need to fetch the badge icon in the update path. The WebAPK server deletes its data after a while. We need to be able to update OneOffWebApks with badge icons (We don't want a WebAPK with a badge icon to regress to a WebAPK without a badge icon when it gets updated) It is not necessary to trigger updates when the badge icon changes. However, we need to pass the badge icon to UpdateAsync() when we request an update. WebApkUpdateDataFetcher is probably the easiest place to fetch the badge icon.
https://codereview.chromium.org/2676863002/diff/140001/chrome/browser/android... File chrome/browser/android/webapk/webapk_installer.cc (right): https://codereview.chromium.org/2676863002/diff/140001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer.cc:265: context, shortcut_info, primary_icon, empty_badge_icon); After some thought, we actually need to fetch the badge icon in the update path. The WebAPK server deletes its data after a while. We need to be able to update OneOffWebApks with badge icons (We don't want a WebAPK with a badge icon to regress to a WebAPK without a badge icon when it gets updated) It is not necessary to trigger updates when the badge icon changes. However, we need to pass the badge icon to UpdateAsync() when we request an update. WebApkUpdateDataFetcher is probably the easiest place to fetch the badge icon.
I'll wait till you resolve Peter's comments before taking another look.
Patchset #3 (id:160001) has been deleted
The CQ bit was checked by zpeng@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...
Patchset #3 (id:180001) has been deleted
The CQ bit was checked by zpeng@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...
Patchset #3 (id:200001) has been deleted
Dry run: Try jobs failed on following builders: android_cronet on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...) android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...) cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...) cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
Patchset #3 (id:220001) has been deleted
The CQ bit was checked by zpeng@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...
Thanks Dominick & Peter! PTAL https://codereview.chromium.org/2676863002/diff/140001/chrome/browser/android... File chrome/browser/android/shortcut_helper.h (right): https://codereview.chromium.org/2676863002/diff/140001/chrome/browser/android... chrome/browser/android/shortcut_helper.h:72: // created by |AddToLauncherWithSkBitmap|; this method should be passed as a On 2017/03/30 21:49:50, pkotwicz wrote: > Nit: > > |AddToLauncherWithSkBitmap| -> AddToLauncherWithSkBitmap() Done. https://codereview.chromium.org/2676863002/diff/140001/chrome/browser/android... File chrome/browser/android/webapk/webapk_installer.cc (right): https://codereview.chromium.org/2676863002/diff/140001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer.cc:180: image->set_hash(entry.second); On 2017/03/30 21:19:37, pkotwicz wrote: > Can we simplify this logic to: > > if (entry.first == shortcut_info.best_primary_icon_url.spec()) { > SetImageData(image, primary_icon); > image->add_usages(webapk::Image::PRIMARY_ICON); > } else { > SetImageData(image, badge_icon); > image->add_usages(webapk::Image::BADGE_ICON); > } > > where SetImageData() is a new function in the anonymous namespace Resolved by offline. Doing everything inside the loop would break "update when web manifest is no longer available" https://codereview.chromium.org/2676863002/diff/140001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer.cc:265: context, shortcut_info, primary_icon, empty_badge_icon); On 2017/03/31 18:03:27, pkotwicz wrote: > After some thought, we actually need to fetch the badge icon in the update path. > > The WebAPK server deletes its data after a while. We need to be able to update > OneOffWebApks with badge icons (We don't want a WebAPK with a badge icon to > regress to a WebAPK without a badge icon when it gets updated) > > It is not necessary to trigger updates when the badge icon changes. However, we > need to pass the badge icon to UpdateAsync() when we request an update. > WebApkUpdateDataFetcher is probably the easiest place to fetch the badge icon. I'll do it in follow up CLs. Added TODO in header file. https://codereview.chromium.org/2676863002/diff/140001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer.cc:499: MapIconUrlToMurmur2Hash(); On 2017/03/30 21:19:37, pkotwicz wrote: > Can we keep the flow linear and call OnGotBadgeIconMurmur2Hash() with an empty > hash instead? > > This would enable you to collapse OnGotBadgeIconMurmur2Hash() and > MapIconUrlToMurmur2Hash() into a single function Done. https://codereview.chromium.org/2676863002/diff/140001/chrome/browser/android... File chrome/browser/android/webapk/webapk_installer_unittest.cc (right): https://codereview.chromium.org/2676863002/diff/140001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer_unittest.cc:390: SetBestBadgeIconUrl(best_badge_icon_url); On 2017/03/30 21:49:50, pkotwicz wrote: > Nit: You can merge lines 389 and 390 Done. https://codereview.chromium.org/2676863002/diff/140001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer_unittest.cc:494: runner->BuildSync(GURL(""), GURL(""), icon_url_to_murmur2_hash, On 2017/03/30 21:49:50, pkotwicz wrote: > Nit: You can use the no-arg constructor of GURL() instead Done. https://codereview.chromium.org/2676863002/diff/140001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer_unittest.cc:542: ASSERT_EQ(2, manifest.icons_size()); On 2017/03/30 21:49:50, pkotwicz wrote: > Don't you need to update this test? > > i.e. ASSERT_EQ(3, manifest.icons_size()) etc. Done. In this test case, primary icon and badge icon happen to point to the same icon, so there are still 2 icons. https://codereview.chromium.org/2676863002/diff/140001/chrome/browser/android... File chrome/browser/android/webapps/add_to_homescreen_manager.cc (right): https://codereview.chromium.org/2676863002/diff/140001/chrome/browser/android... chrome/browser/android/webapps/add_to_homescreen_manager.cc:197: base::MakeUnique<SkBitmap>(primary_icon), On 2017/03/30 21:49:50, pkotwicz wrote: > You don't need to do base::MakeUnique<SkBitmap> for SkBitmaps. SkBitmaps are > cheeply copyable > > (The Chrome codebase is written with that assumption) Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2676863002/diff/240001/chrome/browser/android... File chrome/browser/android/banners/app_banner_infobar_delegate_android.cc (left): https://codereview.chromium.org/2676863002/diff/240001/chrome/browser/android... chrome/browser/android/banners/app_banner_infobar_delegate_android.cc:6: #include <utility> for std::move https://codereview.chromium.org/2676863002/diff/240001/chrome/browser/android... File chrome/browser/android/banners/app_banner_manager_android.cc (right): https://codereview.chromium.org/2676863002/diff/240001/chrome/browser/android... chrome/browser/android/banners/app_banner_manager_android.cc:8: #include <utility> <utility> is no longer needed https://codereview.chromium.org/2676863002/diff/240001/chrome/browser/android... File chrome/browser/android/webapk/webapk_installer.cc (right): https://codereview.chromium.org/2676863002/diff/240001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer.cc:492: // An empty hash indicates that |icon_hasher_| encountered an error. Nit: comment is no longer accurate (no more icon_hasher_) https://codereview.chromium.org/2676863002/diff/240001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer.cc:501: shortcut_info_.best_badge_icon_url.spec() != Do you need to compare the .spec(), or can you directly compare the GURL objects? https://codereview.chromium.org/2676863002/diff/240001/chrome/browser/android... File chrome/browser/android/webapk/webapk_installer.h (right): https://codereview.chromium.org/2676863002/diff/240001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer.h:246: std::string primary_icon_hash_; Instead of having primary_icon_hash_ and badge_icon_hash_ as members, can you add a std::string |primary_icon_hash| argument to OnGotBadgeIconMurmur2Hash()? Then you don't need these, since |badge_icon_hash_| is set and then used in the same method (and neither are touched again). https://codereview.chromium.org/2676863002/diff/240001/chrome/browser/android... File chrome/browser/android/webapk/webapk_installer_unittest.cc (right): https://codereview.chromium.org/2676863002/diff/240001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer_unittest.cc:46: const char* kBestPrimaryIconUrl = "/simple.html"; If these two have the same value, can you use have one kIconUrl constant? https://codereview.chromium.org/2676863002/diff/240001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer_unittest.cc:541: // /icon.png This comment is a bit lonely. Can you expand it or remove it please? https://codereview.chromium.org/2676863002/diff/240001/chrome/browser/banners... File chrome/browser/banners/app_banner_manager.h (right): https://codereview.chromium.org/2676863002/diff/240001/chrome/browser/banners... chrome/browser/banners/app_banner_manager.h:8: #include <memory> This include is no longer needed.
The CQ bit was checked by zpeng@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...
Thanks Dominick & Peter! PTAL https://codereview.chromium.org/2676863002/diff/240001/chrome/browser/android... File chrome/browser/android/banners/app_banner_infobar_delegate_android.cc (left): https://codereview.chromium.org/2676863002/diff/240001/chrome/browser/android... chrome/browser/android/banners/app_banner_infobar_delegate_android.cc:6: On 2017/04/04 05:25:33, dominickn wrote: > #include <utility> for std::move Done. https://codereview.chromium.org/2676863002/diff/240001/chrome/browser/android... File chrome/browser/android/banners/app_banner_manager_android.cc (right): https://codereview.chromium.org/2676863002/diff/240001/chrome/browser/android... chrome/browser/android/banners/app_banner_manager_android.cc:8: #include <utility> On 2017/04/04 05:25:33, dominickn wrote: > <utility> is no longer needed Done. https://codereview.chromium.org/2676863002/diff/240001/chrome/browser/android... File chrome/browser/android/webapk/webapk_installer.cc (right): https://codereview.chromium.org/2676863002/diff/240001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer.cc:492: // An empty hash indicates that |icon_hasher_| encountered an error. On 2017/04/04 05:25:33, dominickn wrote: > Nit: comment is no longer accurate (no more icon_hasher_) Done. https://codereview.chromium.org/2676863002/diff/240001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer.cc:501: shortcut_info_.best_badge_icon_url.spec() != On 2017/04/04 05:25:33, dominickn wrote: > Do you need to compare the .spec(), or can you directly compare the GURL > objects? Done. https://codereview.chromium.org/2676863002/diff/240001/chrome/browser/android... File chrome/browser/android/webapk/webapk_installer.h (right): https://codereview.chromium.org/2676863002/diff/240001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer.h:246: std::string primary_icon_hash_; On 2017/04/04 05:25:33, dominickn wrote: > Instead of having primary_icon_hash_ and badge_icon_hash_ as members, can you > add a std::string |primary_icon_hash| argument to OnGotBadgeIconMurmur2Hash()? > Then you don't need these, since |badge_icon_hash_| is set and then used in the > same method (and neither are touched again). Done. https://codereview.chromium.org/2676863002/diff/240001/chrome/browser/android... File chrome/browser/android/webapk/webapk_installer_unittest.cc (right): https://codereview.chromium.org/2676863002/diff/240001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer_unittest.cc:46: const char* kBestPrimaryIconUrl = "/simple.html"; On 2017/04/04 05:25:33, dominickn wrote: > If these two have the same value, can you use have one kIconUrl constant? Done. https://codereview.chromium.org/2676863002/diff/240001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer_unittest.cc:541: // /icon.png On 2017/04/04 05:25:33, dominickn wrote: > This comment is a bit lonely. Can you expand it or remove it please? Done. https://codereview.chromium.org/2676863002/diff/240001/chrome/browser/banners... File chrome/browser/banners/app_banner_manager.h (right): https://codereview.chromium.org/2676863002/diff/240001/chrome/browser/banners... chrome/browser/banners/app_banner_manager.h:8: #include <memory> On 2017/04/04 05:25:33, dominickn wrote: > This include is no longer needed. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm, thanks
https://codereview.chromium.org/2676863002/diff/260001/chrome/browser/android... File chrome/browser/android/webapk/webapk_installer.cc (right): https://codereview.chromium.org/2676863002/diff/260001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer.cc:162: } This is an elegant solution. Congrats! https://codereview.chromium.org/2676863002/diff/260001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer.cc:172: SetImageData(image, badge_icon); Should we call SetImageData() only if: shortcut_info.best_primary_icon_url != shortcut_info.best_badge_icon_url SetImageData() is a pretty expensive call. PNG encoding is much slower than PNG decoding https://codereview.chromium.org/2676863002/diff/260001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer.cc:261: context, shortcut_info, primary_icon, empty_badge_icon); Nit: inline the |empty_badge_icon| initialization. Thus: new WebApkInstaller(context, shortcut_info, primary_icon, SkBitmap()); https://codereview.chromium.org/2676863002/diff/260001/chrome/browser/android... File chrome/browser/android/webapk/webapk_installer.h (right): https://codereview.chromium.org/2676863002/diff/260001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer.h:169: // |did_fetch_badge_icon| to indicate whether there is an attempt to fetch Nit: "there is an attempt" -> "there was an attempt" https://codereview.chromium.org/2676863002/diff/260001/chrome/browser/android... File chrome/browser/android/webapk/webapk_installer_unittest.cc (right): https://codereview.chromium.org/2676863002/diff/260001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer_unittest.cc:526: best_badge_icon_murmur2_hash; This is not very useful since |best_badge_icon_url| == |best_primary_icon_url| The test would be more useful if the URLs were different
The CQ bit was checked by zpeng@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...
Thanks Dominick & Peter! PTAL https://codereview.chromium.org/2676863002/diff/260001/chrome/browser/android... File chrome/browser/android/webapk/webapk_installer.cc (right): https://codereview.chromium.org/2676863002/diff/260001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer.cc:162: } On 2017/04/05 03:44:29, pkotwicz wrote: > This is an elegant solution. Congrats! Thanks :) https://codereview.chromium.org/2676863002/diff/260001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer.cc:172: SetImageData(image, badge_icon); On 2017/04/05 03:44:29, pkotwicz wrote: > Should we call SetImageData() only if: > shortcut_info.best_primary_icon_url != shortcut_info.best_badge_icon_url > > SetImageData() is a pretty expensive call. PNG encoding is much slower than PNG > decoding Done. https://codereview.chromium.org/2676863002/diff/260001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer.cc:261: context, shortcut_info, primary_icon, empty_badge_icon); On 2017/04/05 03:44:29, pkotwicz wrote: > Nit: inline the |empty_badge_icon| initialization. > > Thus: > new WebApkInstaller(context, shortcut_info, primary_icon, SkBitmap()); Done. https://codereview.chromium.org/2676863002/diff/260001/chrome/browser/android... File chrome/browser/android/webapk/webapk_installer.h (right): https://codereview.chromium.org/2676863002/diff/260001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer.h:169: // |did_fetch_badge_icon| to indicate whether there is an attempt to fetch On 2017/04/05 03:44:29, pkotwicz wrote: > Nit: "there is an attempt" -> "there was an attempt" Done. https://codereview.chromium.org/2676863002/diff/260001/chrome/browser/android... File chrome/browser/android/webapk/webapk_installer_unittest.cc (right): https://codereview.chromium.org/2676863002/diff/260001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer_unittest.cc:526: best_badge_icon_murmur2_hash; On 2017/04/05 03:44:30, pkotwicz wrote: > This is not very useful since |best_badge_icon_url| == |best_primary_icon_url| > > The test would be more useful if the URLs were different Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...) cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
https://codereview.chromium.org/2676863002/diff/280001/chrome/browser/android... File chrome/browser/android/webapk/webapk_installer_unittest.cc (right): https://codereview.chromium.org/2676863002/diff/280001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer_unittest.cc:514: TEST_F(WebApkInstallerTest, BuildWebApkProtoWhenManifestIsAvailable) { Please split this test up into two separate tests BuildWebApkProtoWhenManifestIsAvailable and BuildWebApkProtoPrimaryIconAndBadgeIconSameUrl https://codereview.chromium.org/2676863002/diff/280001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer_unittest.cc:524: icon_url_to_murmur2_hash[icon_url_1.spec()] = icon_murmur2_hash_1; How about: std::string icon_url_1 = test_server()->GetURL("/icon.png").spec(); ... icon_url_to_murmur2_hash[icon_url_1] = "0" ... EXPECT_EQ(icon_url_to_murmur2_hash[icon_url_1], icons[0].src());
The CQ bit was checked by zpeng@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...
Thanks Dominick & Peter! PTAL https://codereview.chromium.org/2676863002/diff/280001/chrome/browser/android... File chrome/browser/android/webapk/webapk_installer_unittest.cc (right): https://codereview.chromium.org/2676863002/diff/280001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer_unittest.cc:514: TEST_F(WebApkInstallerTest, BuildWebApkProtoWhenManifestIsAvailable) { On 2017/04/06 18:58:31, pkotwicz wrote: > Please split this test up into two separate tests > > BuildWebApkProtoWhenManifestIsAvailable and > BuildWebApkProtoPrimaryIconAndBadgeIconSameUrl Done. https://codereview.chromium.org/2676863002/diff/280001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer_unittest.cc:524: icon_url_to_murmur2_hash[icon_url_1.spec()] = icon_murmur2_hash_1; On 2017/04/06 18:58:31, pkotwicz wrote: > How about: > std::string icon_url_1 = test_server()->GetURL("/icon.png").spec(); > > ... > > icon_url_to_murmur2_hash[icon_url_1] = "0" > > ... > > EXPECT_EQ(icon_url_to_murmur2_hash[icon_url_1], icons[0].src()); Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2676863002/diff/280001/chrome/browser/android... File chrome/browser/android/webapk/webapk_installer_unittest.cc (right): https://codereview.chromium.org/2676863002/diff/280001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer_unittest.cc:524: icon_url_to_murmur2_hash[icon_url_1.spec()] = icon_murmur2_hash_1; Please make the same change for |best_primary_icon_url| and |best_badge_icon_url|
The CQ bit was checked by zpeng@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...
Thanks Dominick & Peter! PTAL https://codereview.chromium.org/2676863002/diff/280001/chrome/browser/android... File chrome/browser/android/webapk/webapk_installer_unittest.cc (right): https://codereview.chromium.org/2676863002/diff/280001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer_unittest.cc:524: icon_url_to_murmur2_hash[icon_url_1.spec()] = icon_murmur2_hash_1; On 2017/04/06 22:08:52, pkotwicz wrote: > Please make the same change for |best_primary_icon_url| and > |best_badge_icon_url| Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM https://codereview.chromium.org/2676863002/diff/320001/chrome/browser/android... File chrome/browser/android/webapk/webapk_installer_unittest.cc (right): https://codereview.chromium.org/2676863002/diff/320001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer_unittest.cc:498: EXPECT_EQ(webapk::Image::BADGE_ICON, icons[1].usages(0)); Nit: You can use EXPECT_THAT(icon[1].usages(), testing::ElementsAre(webapk::Image::BADGE_ICON)); and have one line instead of 497 and 498 According to http://stackoverflow.com/questions/1460703/comparison-of-arrays-in-google-test it looks like ElementsAre() works even though icons() does not return an STL container https://codereview.chromium.org/2676863002/diff/320001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer_unittest.cc:505: EXPECT_EQ(webapk::Image::PRIMARY_ICON, icons[2].usages(0)); Nit: You can use EXPECT_THAT(icon[2].usages(), testing::ElementsAre(webapk::Image::PRIMARY_ICON)); and have one line instead of 504 and 505 https://codereview.chromium.org/2676863002/diff/320001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer_unittest.cc:542: EXPECT_EQ(webapk::Image::BADGE_ICON, icons[1].usages(1)); Nit: You can use EXPECT_THAT(icons[1].usages(), testing::ElementsAre(webapk::Image::PRIMARY_ICON, webapk::Image::BADGE_ICON));
https://codereview.chromium.org/2676863002/diff/320001/chrome/browser/android... File chrome/browser/android/webapk/webapk_installer_unittest.cc (right): https://codereview.chromium.org/2676863002/diff/320001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer_unittest.cc:491: EXPECT_EQ(icon_url_to_murmur2_hash[icon_url_1], icons[0].hash()); We should also check that the usages are not specified https://codereview.chromium.org/2676863002/diff/320001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer_unittest.cc:534: EXPECT_EQ(icon_url_to_murmur2_hash[icon_url_1], icons[0].hash()); We should also check that the usages are not specified
The CQ bit was checked by zpeng@chromium.org to run a CQ dry run
Thanks Dominick & Peter! PTAL https://codereview.chromium.org/2676863002/diff/320001/chrome/browser/android... File chrome/browser/android/webapk/webapk_installer_unittest.cc (right): https://codereview.chromium.org/2676863002/diff/320001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer_unittest.cc:491: EXPECT_EQ(icon_url_to_murmur2_hash[icon_url_1], icons[0].hash()); On 2017/04/07 15:37:55, pkotwicz wrote: > We should also check that the usages are not specified Done. https://codereview.chromium.org/2676863002/diff/320001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer_unittest.cc:498: EXPECT_EQ(webapk::Image::BADGE_ICON, icons[1].usages(0)); On 2017/04/07 15:34:05, pkotwicz wrote: > Nit: You can use > > EXPECT_THAT(icon[1].usages(), testing::ElementsAre(webapk::Image::BADGE_ICON)); > > and have one line instead of 497 and 498 > > According to > http://stackoverflow.com/questions/1460703/comparison-of-arrays-in-google-test > it looks like ElementsAre() works even though icons() does not return an STL > container Done. https://codereview.chromium.org/2676863002/diff/320001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer_unittest.cc:505: EXPECT_EQ(webapk::Image::PRIMARY_ICON, icons[2].usages(0)); On 2017/04/07 15:34:04, pkotwicz wrote: > Nit: You can use > > EXPECT_THAT(icon[2].usages(), > testing::ElementsAre(webapk::Image::PRIMARY_ICON)); > > and have one line instead of 504 and 505 Done. https://codereview.chromium.org/2676863002/diff/320001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer_unittest.cc:534: EXPECT_EQ(icon_url_to_murmur2_hash[icon_url_1], icons[0].hash()); On 2017/04/07 15:37:55, pkotwicz wrote: > We should also check that the usages are not specified Done. https://codereview.chromium.org/2676863002/diff/320001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer_unittest.cc:542: EXPECT_EQ(webapk::Image::BADGE_ICON, icons[1].usages(1)); On 2017/04/07 15:34:04, pkotwicz wrote: > Nit: You can use > > EXPECT_THAT(icons[1].usages(), testing::ElementsAre(webapk::Image::PRIMARY_ICON, > webapk::Image::BADGE_ICON)); Done.
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.
The CQ bit was checked by zpeng@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dominickn@chromium.org, pkotwicz@chromium.org Link to the patchset: https://codereview.chromium.org/2676863002/#ps340001 (title: "addressing comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 340001, "attempt_start_ts": 1491593158324710,
"parent_rev": "9f64905e0a14a51135c1c937be146271a3fb057e", "commit_rev":
"1fd959d6b3a68e527f7b2be9e9ef93d996839c00"}
Message was sent while issue was closed.
Description was changed from ========== Update WebApkInstaller to support badge icon in installation. This CL updates WebApkInstaller so that the murmur2 hash of the best badge icon would be calculated and passed to the WAM server in WebAPK proto along with the url of the best badge icon. BUG=649771 ========== to ========== Update WebApkInstaller to support badge icon in installation. This CL updates WebApkInstaller so that the murmur2 hash of the best badge icon would be calculated and passed to the WAM server in WebAPK proto along with the url of the best badge icon. BUG=649771 Review-Url: https://codereview.chromium.org/2676863002 Cr-Commit-Position: refs/heads/master@{#462963} Committed: https://chromium.googlesource.com/chromium/src/+/1fd959d6b3a68e527f7b2be9e9ef... ==========
Message was sent while issue was closed.
Committed patchset #8 (id:340001) as https://chromium.googlesource.com/chromium/src/+/1fd959d6b3a68e527f7b2be9e9ef... |
