Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2669133003:
Update AddToHomescreenDataFetcher to accept badge icon for WebAPK. (Closed)
DescriptionUpdate AddToHomescreenDataFetcher to accept badge icon for WebAPK.
Update AddToHomescreenDataFetcher to accept badge icon if data is
requested for WebAPK. As badge icon is processed for notification
purpose in NotificationBuilderBase::setSmallIcon, we do not process the
badge icon the same way we process the primary/launcher icon.
BUG=649771
Review-Url: https://codereview.chromium.org/2669133003
Cr-Commit-Position: refs/heads/master@{#447870}
Committed: https://chromium.googlesource.com/chromium/src/+/f504c224a1f45f413cbe4c0ab0908033ba83ca3e
Patch Set 1 : Format #
Total comments: 23
Patch Set 2 : Addressing comments #Patch Set 3 : Addressing comments #
Messages
Total messages: 33 (24 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...
The CQ bit was unchecked by zpeng@chromium.org
Patchset #1 (id:1) 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 ========== Name BUG= ========== to ========== Update AddToHomescreenDataFetcher to accept badge icon for WebAPK. Update AddToHomescreenDataFetcher to accept badge icon if data is requested for WebAPK. As badge icon is processed for notification purpose in NotificationBuilderBase::setSmallIcon, we do not process the badge icon the same way we process the primary/launcher icon. BUG=649771 ==========
zpeng@chromium.org changed reviewers: + dominickn@chromium.org, pkotwicz@chromium.org
Hi Dominick & Peter, PTAL. Thanks!
I'll take a look after my sheriff shift
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2669133003/diff/20001/chrome/browser/android/... File chrome/browser/android/shortcut_info.h (right): https://codereview.chromium.org/2669133003/diff/20001/chrome/browser/android/... chrome/browser/android/shortcut_info.h:56: GURL best_icon_url; Perhaps rename this best_primary_icon_url? https://codereview.chromium.org/2669133003/diff/20001/chrome/browser/android/... File chrome/browser/android/webapps/add_to_homescreen_data_fetcher.cc (right): https://codereview.chromium.org/2669133003/diff/20001/chrome/browser/android/... chrome/browser/android/webapps/add_to_homescreen_data_fetcher.cc:49: bool check_webapk_compatibility, It's weird to me to see the bool in the middle of the argument list, particularly since it's last elsewhere. https://codereview.chromium.org/2669133003/diff/20001/chrome/browser/android/... chrome/browser/android/webapps/add_to_homescreen_data_fetcher.cc:228: data.badge_icon->deepCopyTo(&shortcut_badge_); Why does this need to be a deep copy as opposed to just using the copy constructor? https://codereview.chromium.org/2669133003/diff/20001/chrome/browser/android/... File chrome/browser/android/webapps/add_to_homescreen_data_fetcher.h (right): https://codereview.chromium.org/2669133003/diff/20001/chrome/browser/android/... chrome/browser/android/webapps/add_to_homescreen_data_fetcher.h:62: const SkBitmap& icon, primary_icon? badge_icon? https://codereview.chromium.org/2669133003/diff/20001/chrome/browser/android/... chrome/browser/android/webapps/add_to_homescreen_data_fetcher.h:130: SkBitmap shortcut_icon_; primary_icon_? badge_icon? https://codereview.chromium.org/2669133003/diff/20001/chrome/browser/android/... chrome/browser/android/webapps/add_to_homescreen_data_fetcher.h:142: const int ideal_badge_size_in_px_; These appear to always be set to the same value. Can we just have one member? A corollary is that it seems to me that we don't have this in-between zone for badge icons: the minimal and ideal sizes are always the same. If that's the case, why don't we just have one size parameter passed into InstallableManager as well? https://codereview.chromium.org/2669133003/diff/20001/chrome/browser/android/... File chrome/browser/android/webapps/add_to_homescreen_data_fetcher_unittest.cc (right): https://codereview.chromium.org/2669133003/diff/20001/chrome/browser/android/... chrome/browser/android/webapps/add_to_homescreen_data_fetcher_unittest.cc:134: const SkBitmap& icon, primary_icon? badge_icon? https://codereview.chromium.org/2669133003/diff/20001/chrome/browser/android/... File chrome/browser/android/webapps/add_to_homescreen_manager.h (right): https://codereview.chromium.org/2669133003/diff/20001/chrome/browser/android/... chrome/browser/android/webapps/add_to_homescreen_manager.h:63: const SkBitmap& icon, primary_icon? basge_icon?
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, PTAL! https://codereview.chromium.org/2669133003/diff/20001/chrome/browser/android/... File chrome/browser/android/shortcut_info.h (right): https://codereview.chromium.org/2669133003/diff/20001/chrome/browser/android/... chrome/browser/android/shortcut_info.h:56: GURL best_icon_url; On 2017/02/02 19:16:50, dominickn wrote: > Perhaps rename this best_primary_icon_url? After looking at the places that use ShortcutInfo, I propose that we address the renaming of best_icon_url in a separate CL. WDUT? https://codereview.chromium.org/2669133003/diff/20001/chrome/browser/android/... File chrome/browser/android/webapps/add_to_homescreen_data_fetcher.cc (right): https://codereview.chromium.org/2669133003/diff/20001/chrome/browser/android/... chrome/browser/android/webapps/add_to_homescreen_data_fetcher.cc:49: bool check_webapk_compatibility, On 2017/02/02 19:16:50, dominickn wrote: > It's weird to me to see the bool in the middle of the argument list, > particularly since it's last elsewhere. Done. https://codereview.chromium.org/2669133003/diff/20001/chrome/browser/android/... chrome/browser/android/webapps/add_to_homescreen_data_fetcher.cc:228: data.badge_icon->deepCopyTo(&shortcut_badge_); On 2017/02/02 19:16:50, dominickn wrote: > Why does this need to be a deep copy as opposed to just using the copy > constructor? My bad. I wasn't sure of the life span of SkBitmap owned by InstallableManager. https://codereview.chromium.org/2669133003/diff/20001/chrome/browser/android/... File chrome/browser/android/webapps/add_to_homescreen_data_fetcher.h (right): https://codereview.chromium.org/2669133003/diff/20001/chrome/browser/android/... chrome/browser/android/webapps/add_to_homescreen_data_fetcher.h:62: const SkBitmap& icon, On 2017/02/02 19:16:50, dominickn wrote: > primary_icon? > badge_icon? Done. https://codereview.chromium.org/2669133003/diff/20001/chrome/browser/android/... chrome/browser/android/webapps/add_to_homescreen_data_fetcher.h:130: SkBitmap shortcut_icon_; On 2017/02/02 19:16:50, dominickn wrote: > primary_icon_? > badge_icon? Done. https://codereview.chromium.org/2669133003/diff/20001/chrome/browser/android/... chrome/browser/android/webapps/add_to_homescreen_data_fetcher.h:142: const int ideal_badge_size_in_px_; On 2017/02/02 19:16:50, dominickn wrote: > These appear to always be set to the same value. Can we just have one member? > > A corollary is that it seems to me that we don't have this in-between zone for > badge icons: the minimal and ideal sizes are always the same. If that's the > case, why don't we just have one size parameter passed into InstallableManager > as well? My understanding is that ShortcutHelper wraps the device-related information while InstallableManager is more on the logic side. So I think InstallableManager should allow setting a minimal value, though right now we decide to provide the same value for both minimal and ideal. This way, setting the values is AddToHomescreenManager's concern, and data fetcher & InstallableManager provide a more generic interface. WDUT? https://codereview.chromium.org/2669133003/diff/20001/chrome/browser/android/... File chrome/browser/android/webapps/add_to_homescreen_data_fetcher_unittest.cc (right): https://codereview.chromium.org/2669133003/diff/20001/chrome/browser/android/... chrome/browser/android/webapps/add_to_homescreen_data_fetcher_unittest.cc:134: const SkBitmap& icon, On 2017/02/02 19:16:50, dominickn wrote: > primary_icon? > badge_icon? Done. https://codereview.chromium.org/2669133003/diff/20001/chrome/browser/android/... File chrome/browser/android/webapps/add_to_homescreen_manager.h (right): https://codereview.chromium.org/2669133003/diff/20001/chrome/browser/android/... chrome/browser/android/webapps/add_to_homescreen_manager.h:63: const SkBitmap& icon, On 2017/02/02 19:16:50, dominickn wrote: > primary_icon? > basge_icon? Done.
https://codereview.chromium.org/2669133003/diff/20001/chrome/browser/android/... File chrome/browser/android/shortcut_info.h (right): https://codereview.chromium.org/2669133003/diff/20001/chrome/browser/android/... chrome/browser/android/shortcut_info.h:56: GURL best_icon_url; On 2017/02/02 21:05:51, F wrote: > On 2017/02/02 19:16:50, dominickn wrote: > > Perhaps rename this best_primary_icon_url? > > After looking at the places that use ShortcutInfo, I propose that we address the > renaming of best_icon_url in a separate CL. WDUT? Acknowledged. https://codereview.chromium.org/2669133003/diff/20001/chrome/browser/android/... File chrome/browser/android/webapps/add_to_homescreen_data_fetcher.cc (right): https://codereview.chromium.org/2669133003/diff/20001/chrome/browser/android/... chrome/browser/android/webapps/add_to_homescreen_data_fetcher.cc:228: data.badge_icon->deepCopyTo(&shortcut_badge_); On 2017/02/02 21:05:51, F wrote: > On 2017/02/02 19:16:50, dominickn wrote: > > Why does this need to be a deep copy as opposed to just using the copy > > constructor? > My bad. I wasn't sure of the life span of SkBitmap owned by InstallableManager. > We use the copy constructor for the primary icon here so it should be fine. InstallableManager's icon is cached for the lifetime of the navigation. :) https://codereview.chromium.org/2669133003/diff/20001/chrome/browser/android/... File chrome/browser/android/webapps/add_to_homescreen_data_fetcher.h (right): https://codereview.chromium.org/2669133003/diff/20001/chrome/browser/android/... chrome/browser/android/webapps/add_to_homescreen_data_fetcher.h:130: SkBitmap shortcut_icon_; On 2017/02/02 21:05:51, F wrote: > On 2017/02/02 19:16:50, dominickn wrote: > > primary_icon_? > > badge_icon? > > Done. I don't think we need the shortcut_ prefix here - primary_icon_ and badge_icon_ should suffice (and make some methods fit on one line). E.g. see splash_screen_url_ https://codereview.chromium.org/2669133003/diff/20001/chrome/browser/android/... chrome/browser/android/webapps/add_to_homescreen_data_fetcher.h:142: const int ideal_badge_size_in_px_; On 2017/02/02 21:05:51, F wrote: > On 2017/02/02 19:16:50, dominickn wrote: > > These appear to always be set to the same value. Can we just have one member? > > > > A corollary is that it seems to me that we don't have this in-between zone for > > badge icons: the minimal and ideal sizes are always the same. If that's the > > case, why don't we just have one size parameter passed into InstallableManager > > as well? > > My understanding is that ShortcutHelper wraps the device-related information > while InstallableManager is more on the logic side. So I think > InstallableManager should allow setting a minimal value, though right now we > decide to provide the same value for both minimal and ideal. > > This way, setting the values is AddToHomescreenManager's concern, and data > fetcher & InstallableManager provide a more generic interface. > > WDUT? If we're going to provide the same value for now, we should just have one value. Later on if we decide to have different values we can have the separate variables (I doubt that we'll ever have a minimal badge icon size different from the ideal). I'm okay with you leaving InstallableManager alone, but in this class here I at least think having one badge_size_in_px_ should suffice.
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 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:60001) has been deleted
The CQ bit was checked by zpeng@chromium.org to run a CQ dry run
Thanks Dominick, PTAL! https://codereview.chromium.org/2669133003/diff/20001/chrome/browser/android/... File chrome/browser/android/webapps/add_to_homescreen_data_fetcher.cc (right): https://codereview.chromium.org/2669133003/diff/20001/chrome/browser/android/... chrome/browser/android/webapps/add_to_homescreen_data_fetcher.cc:228: data.badge_icon->deepCopyTo(&shortcut_badge_); On 2017/02/02 21:17:11, dominickn wrote: > On 2017/02/02 21:05:51, F wrote: > > On 2017/02/02 19:16:50, dominickn wrote: > > > Why does this need to be a deep copy as opposed to just using the copy > > > constructor? > > My bad. I wasn't sure of the life span of SkBitmap owned by > InstallableManager. > > > > We use the copy constructor for the primary icon here so it should be fine. > InstallableManager's icon is cached for the lifetime of the navigation. :) Thanks for the info :) https://codereview.chromium.org/2669133003/diff/20001/chrome/browser/android/... File chrome/browser/android/webapps/add_to_homescreen_data_fetcher.h (right): https://codereview.chromium.org/2669133003/diff/20001/chrome/browser/android/... chrome/browser/android/webapps/add_to_homescreen_data_fetcher.h:130: SkBitmap shortcut_icon_; On 2017/02/02 21:17:11, dominickn wrote: > On 2017/02/02 21:05:51, F wrote: > > On 2017/02/02 19:16:50, dominickn wrote: > > > primary_icon_? > > > badge_icon? > > > > Done. > > I don't think we need the shortcut_ prefix here - primary_icon_ and badge_icon_ > should suffice (and make some methods fit on one line). E.g. see > splash_screen_url_ Done. https://codereview.chromium.org/2669133003/diff/20001/chrome/browser/android/... chrome/browser/android/webapps/add_to_homescreen_data_fetcher.h:142: const int ideal_badge_size_in_px_; On 2017/02/02 21:17:11, dominickn wrote: > On 2017/02/02 21:05:51, F wrote: > > On 2017/02/02 19:16:50, dominickn wrote: > > > These appear to always be set to the same value. Can we just have one > member? > > > > > > A corollary is that it seems to me that we don't have this in-between zone > for > > > badge icons: the minimal and ideal sizes are always the same. If that's the > > > case, why don't we just have one size parameter passed into > InstallableManager > > > as well? > > > > My understanding is that ShortcutHelper wraps the device-related information > > while InstallableManager is more on the logic side. So I think > > InstallableManager should allow setting a minimal value, though right now we > > decide to provide the same value for both minimal and ideal. > > > > This way, setting the values is AddToHomescreenManager's concern, and data > > fetcher & InstallableManager provide a more generic interface. > > > > WDUT? > > If we're going to provide the same value for now, we should just have one value. > Later on if we decide to have different values we can have the separate > variables (I doubt that we'll ever have a minimal badge icon size different from > the ideal). > > I'm okay with you leaving InstallableManager alone, but in this class here I at > least think having one badge_size_in_px_ should suffice. Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm, thanks!
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
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": 80001, "attempt_start_ts": 1486077138898100,
"parent_rev": "673948a5da3938412ada2feea06f29e2f80f8299", "commit_rev":
"f504c224a1f45f413cbe4c0ab0908033ba83ca3e"}
Message was sent while issue was closed.
Description was changed from ========== Update AddToHomescreenDataFetcher to accept badge icon for WebAPK. Update AddToHomescreenDataFetcher to accept badge icon if data is requested for WebAPK. As badge icon is processed for notification purpose in NotificationBuilderBase::setSmallIcon, we do not process the badge icon the same way we process the primary/launcher icon. BUG=649771 ========== to ========== Update AddToHomescreenDataFetcher to accept badge icon for WebAPK. Update AddToHomescreenDataFetcher to accept badge icon if data is requested for WebAPK. As badge icon is processed for notification purpose in NotificationBuilderBase::setSmallIcon, we do not process the badge icon the same way we process the primary/launcher icon. BUG=649771 Review-Url: https://codereview.chromium.org/2669133003 Cr-Commit-Position: refs/heads/master@{#447870} Committed: https://chromium.googlesource.com/chromium/src/+/f504c224a1f45f413cbe4c0ab090... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:80001) as https://chromium.googlesource.com/chromium/src/+/f504c224a1f45f413cbe4c0ab090... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
