Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
257843004:
Add in resize logic for the non-badged icons. (Closed)
|
Created:
6 years, 7 months ago by Mike Lerman Modified:
6 years, 7 months ago CC:
chromium-reviews, tfarina Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionAdd in resize logic for the non-badged icons.
BUG=367022
TEST=The shadow icon should now show properly in the avatar menu.
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=267816
Patch Set 1 #
Total comments: 4
Patch Set 2 : Change resize function used #
Total comments: 5
Patch Set 3 : Comment nit. #Patch Set 4 : Use avatar_icon_util method to resize badged and non-badged icons. #
Total comments: 2
Patch Set 5 : Nit #Messages
Total messages: 23 (0 generated)
Hi Mike, Can you review this quick patch? Thanks! Mike
https://codereview.chromium.org/257843004/diff/1/chrome/browser/ui/views/prof... File chrome/browser/ui/views/profiles/avatar_menu_bubble_view.cc (right): https://codereview.chromium.org/257843004/diff/1/chrome/browser/ui/views/prof... chrome/browser/ui/views/profiles/avatar_menu_bubble_view.cc:267: image_view_->SetImage(profiles::GetSizedAvatarIcon( The ImageView class comment says "If a size is provided, the ImageView will resize the provided image to fit if it is too big...", can you attempt specifying the desired size that way?
https://codereview.chromium.org/257843004/diff/1/chrome/browser/ui/views/prof... File chrome/browser/ui/views/profiles/avatar_menu_bubble_view.cc (right): https://codereview.chromium.org/257843004/diff/1/chrome/browser/ui/views/prof... chrome/browser/ui/views/profiles/avatar_menu_bubble_view.cc:267: image_view_->SetImage(profiles::GetSizedAvatarIcon( On 2014/04/25 19:50:45, msw wrote: > The ImageView class comment says "If a size is provided, the ImageView will > resize the provided image to fit if it is too big...", can you attempt > specifying the desired size that way? Done.
https://codereview.chromium.org/257843004/diff/20001/chrome/browser/ui/views/... File chrome/browser/ui/views/profiles/avatar_menu_bubble_view.cc (right): https://codereview.chromium.org/257843004/diff/20001/chrome/browser/ui/views/... chrome/browser/ui/views/profiles/avatar_menu_bubble_view.cc:266: // The image may be too large are require resizing nit: s/are/and/; also add a period at the end. https://codereview.chromium.org/257843004/diff/20001/chrome/browser/ui/views/... chrome/browser/ui/views/profiles/avatar_menu_bubble_view.cc:267: image_view_->SetImageSize( Should this be set all the time, not just in this else case?
https://codereview.chromium.org/257843004/diff/20001/chrome/browser/ui/views/... File chrome/browser/ui/views/profiles/avatar_menu_bubble_view.cc (right): https://codereview.chromium.org/257843004/diff/20001/chrome/browser/ui/views/... chrome/browser/ui/views/profiles/avatar_menu_bubble_view.cc:266: // The image may be too large are require resizing On 2014/04/28 17:56:52, msw wrote: > nit: s/are/and/; also add a period at the end. Done. https://codereview.chromium.org/257843004/diff/20001/chrome/browser/ui/views/... chrome/browser/ui/views/profiles/avatar_menu_bubble_view.cc:267: image_view_->SetImageSize( On 2014/04/28 17:56:52, msw wrote: > Should this be set all the time, not just in this else case? The other case, the GetBadgedIcon method, already does a scaling.
https://codereview.chromium.org/257843004/diff/20001/chrome/browser/ui/views/... File chrome/browser/ui/views/profiles/avatar_menu_bubble_view.cc (right): https://codereview.chromium.org/257843004/diff/20001/chrome/browser/ui/views/... chrome/browser/ui/views/profiles/avatar_menu_bubble_view.cc:267: image_view_->SetImageSize( On 2014/04/28 18:21:07, Mike Lerman wrote: > On 2014/04/28 17:56:52, msw wrote: > > Should this be set all the time, not just in this else case? > > The other case, the GetBadgedIcon method, already does a scaling. Yeah, I saw that, but BadgeImageSource::ComputeSize seems to increase the supplied sizes according to a badge overlap ratio; so their resulting composite images could also be too large, afaict. Perhaps if the ImageView itself scales the content, GetBadgedIcon could be simplified (but that's not strictly necessary for this CL).
On 2014/04/28 18:40:19, msw wrote: > https://codereview.chromium.org/257843004/diff/20001/chrome/browser/ui/views/... > File chrome/browser/ui/views/profiles/avatar_menu_bubble_view.cc (right): > > https://codereview.chromium.org/257843004/diff/20001/chrome/browser/ui/views/... > chrome/browser/ui/views/profiles/avatar_menu_bubble_view.cc:267: > image_view_->SetImageSize( > On 2014/04/28 18:21:07, Mike Lerman wrote: > > On 2014/04/28 17:56:52, msw wrote: > > > Should this be set all the time, not just in this else case? > > > > The other case, the GetBadgedIcon method, already does a scaling. > > Yeah, I saw that, but BadgeImageSource::ComputeSize seems to increase the > supplied sizes according to a badge overlap ratio; so their resulting composite > images could also be too large, afaict. Perhaps if the ImageView itself scales > the content, GetBadgedIcon could be simplified (but that's not strictly > necessary for this CL). Trying to define ahead of time the size of the ImageView is getting difficult - the results of GetBadgedIcon are intentionally sometimes larger than the base image (e.g. there's a little checkmark that extends beyond the profile_icon). If I pursue resizing the image_view I'll need to break out the logic used to created the BadgedIcon so that I can use the same logic to draw the image and to define the size of the view. I begin to lean back towards resizing the profile_icon rather than applying a resize to the image_view. In this case I would go back to resizing the profile_icon (in both clauses of the if statement) using profiles::GetSizedAvatarIcon (as per the original patch) or some other method you can recommend :) Thoughts?
On 2014/04/29 18:21:30, Mike Lerman wrote: > On 2014/04/28 18:40:19, msw wrote: > > > https://codereview.chromium.org/257843004/diff/20001/chrome/browser/ui/views/... > > File chrome/browser/ui/views/profiles/avatar_menu_bubble_view.cc (right): > > > > > https://codereview.chromium.org/257843004/diff/20001/chrome/browser/ui/views/... > > chrome/browser/ui/views/profiles/avatar_menu_bubble_view.cc:267: > > image_view_->SetImageSize( > > On 2014/04/28 18:21:07, Mike Lerman wrote: > > > On 2014/04/28 17:56:52, msw wrote: > > > > Should this be set all the time, not just in this else case? > > > > > > The other case, the GetBadgedIcon method, already does a scaling. > > > > Yeah, I saw that, but BadgeImageSource::ComputeSize seems to increase the > > supplied sizes according to a badge overlap ratio; so their resulting > composite > > images could also be too large, afaict. Perhaps if the ImageView itself scales > > the content, GetBadgedIcon could be simplified (but that's not strictly > > necessary for this CL). > > Trying to define ahead of time the size of the ImageView is getting difficult - > the results of GetBadgedIcon are intentionally sometimes larger than the base > image (e.g. there's a little checkmark that extends beyond the profile_icon). If > I pursue resizing the image_view I'll need to break out the logic used to > created the BadgedIcon so that I can use the same logic to draw the image and to > define the size of the view. I begin to lean back towards resizing the > profile_icon rather than applying a resize to the image_view. > > In this case I would go back to resizing the profile_icon (in both clauses of > the if statement) using profiles::GetSizedAvatarIcon (as per the original patch) > or some other method you can recommend :) > > Thoughts? I actually tried these changes locally, and my recommendation to use ImageView::SetImageSize is wrong (the images are stretched larger than intended). This code could use some cleanup, but for now please revert to your your original approach, which works as expected in my limited local testing. Please also address my indentation nit. Feel free to loop in a reviewer more familiar with this code. https://codereview.chromium.org/257843004/diff/1/chrome/browser/ui/views/prof... File chrome/browser/ui/views/profiles/avatar_menu_bubble_view.cc (right): https://codereview.chromium.org/257843004/diff/1/chrome/browser/ui/views/prof... chrome/browser/ui/views/profiles/avatar_menu_bubble_view.cc:269: ToImageSkia()); nit: I think this ought to be indented four more spaces.
Okay Mike, Back to the original method, with tweaks of course from what we've learned :) https://codereview.chromium.org/257843004/diff/1/chrome/browser/ui/views/prof... File chrome/browser/ui/views/profiles/avatar_menu_bubble_view.cc (right): https://codereview.chromium.org/257843004/diff/1/chrome/browser/ui/views/prof... chrome/browser/ui/views/profiles/avatar_menu_bubble_view.cc:269: ToImageSkia()); On 2014/04/29 19:25:46, msw wrote: > nit: I think this ought to be indented four more spaces. Done.
lgtm with a nit; sorry for the detour. https://codereview.chromium.org/257843004/diff/60001/chrome/browser/ui/views/... File chrome/browser/ui/views/profiles/avatar_menu_bubble_view.cc (right): https://codereview.chromium.org/257843004/diff/60001/chrome/browser/ui/views/... chrome/browser/ui/views/profiles/avatar_menu_bubble_view.cc:265: if (item_.active || item_.signin_required) { nit: remove unnecessary braces
Hi Monica, Mike asked me to rope in someone more familiar with the code for a review. Can you do me the honour? Thanks, Mike https://codereview.chromium.org/257843004/diff/60001/chrome/browser/ui/views/... File chrome/browser/ui/views/profiles/avatar_menu_bubble_view.cc (right): https://codereview.chromium.org/257843004/diff/60001/chrome/browser/ui/views/... chrome/browser/ui/views/profiles/avatar_menu_bubble_view.cc:265: if (item_.active || item_.signin_required) { On 2014/04/29 21:25:10, msw wrote: > nit: remove unnecessary braces Done.
I think it lgtm. Can you add some screenshots of how it's broken, and how it looks now when it's fixed, please? Just for a sanity check... Thanks.
The CQ bit was checked by mlerman@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mlerman@chromium.org/257843004/80001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_clang_dbg on tryserver.chromium
The CQ bit was checked by mlerman@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mlerman@chromium.org/257843004/80001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_rel on tryserver.chromium
The CQ bit was checked by mlerman@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mlerman@chromium.org/257843004/80001
Message was sent while issue was closed.
Change committed as 267816 |
