Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
268073005:
Bring back support for 38x31 avatars. (Closed)
|
Created:
6 years, 7 months ago by Mike Lerman Modified:
6 years, 7 months ago CC:
chromium-reviews, dbeam+watch-options_chromium.org, tfarina, arv+watch_chromium.org, Ben Goodger (Google) Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionBring back support for 38x31 avatars.
The avatar icons should not look skewed in any setting.
This CL is a partial revert of https://codereview.chromium.org/212603011/ (specifically, the portions that changed the dimensions of the avatar icons).
BUG=367022, 369495, 341608
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=270008
Patch Set 1 #
Total comments: 5
Patch Set 2 : Refactor crop/squaring code into ProfileAvatarIconUtil #
Total comments: 1
Patch Set 3 : Nits #
Total comments: 8
Patch Set 4 : Change return and singature of GetAvatarIconAsSquare #Patch Set 5 : Moved the call to profiles:: namespace outside the task thread. #
Total comments: 9
Patch Set 6 : Nits and refactor GetAvatarIconAsSquare to have a return value. #
Messages
Total messages: 24 (0 generated)
Hi Monica, Can you take a review of this, before I send it out to the other owners? Thanks!
If I understand this correctly, this CL is mostly reverting changes from a previous CL? Could you please include this in the description, and a link to that original CL, so that it doesn't look like actual new code? Also, I don't think the tabstrip icon bug is being fixed by this CL. It worked after reverting the avatar resources, so it should probably not be included in the list of bugs. https://codereview.chromium.org/268073005/diff/1/chrome/browser/profiles/prof... File chrome/browser/profiles/profile_shortcut_manager_win.cc (right): https://codereview.chromium.org/268073005/diff/1/chrome/browser/profiles/prof... chrome/browser/profiles/profile_shortcut_manager_win.cc:115: // avatar_menu_button::DrawTaskBarDecoration. Can we address this TODO? Maybe move the function out to profile_avatar_icon_util?
https://codereview.chromium.org/268073005/diff/1/chrome/browser/profiles/prof... File chrome/browser/profiles/profile_shortcut_manager_win.cc (right): https://codereview.chromium.org/268073005/diff/1/chrome/browser/profiles/prof... chrome/browser/profiles/profile_shortcut_manager_win.cc:75: const int kCurrentProfileIconVersion = 2; As calamity@ mentioned on issue 369495, you should increment this to force the static profile icons used for pinned taskbar shortcuts to be regenerated. (and you should manually test that this indeed works -- I think this is the first time we actually do this since it was put in place!).
https://codereview.chromium.org/268073005/diff/1/chrome/browser/profiles/prof... File chrome/browser/profiles/profile_shortcut_manager_win.cc (right): https://codereview.chromium.org/268073005/diff/1/chrome/browser/profiles/prof... chrome/browser/profiles/profile_shortcut_manager_win.cc:75: const int kCurrentProfileIconVersion = 2; Gab: Mike is actually reverting to the status quo of about 2 weeks ago, so in theory nothing actually has changed... On 2014/05/05 22:58:09, gab wrote: > As calamity@ mentioned on issue 369495, you should increment this to force the > static profile icons used for pinned taskbar shortcuts to be regenerated. > > (and you should manually test that this indeed works -- I think this is the > first time we actually do this since it was put in place!).
https://codereview.chromium.org/268073005/diff/1/chrome/browser/profiles/prof... File chrome/browser/profiles/profile_shortcut_manager_win.cc (right): https://codereview.chromium.org/268073005/diff/1/chrome/browser/profiles/prof... chrome/browser/profiles/profile_shortcut_manager_win.cc:115: // avatar_menu_button::DrawTaskBarDecoration. On 2014/05/05 16:10:53, Monica Dinculescu wrote: > Can we address this TODO? Maybe move the function out to > profile_avatar_icon_util? Done, refactored the code into profile_avatar_icon_util. I couldn't find anything in avatar_menu_button that looked similar to this, where I could use the new common method.
LGTM % nits. I think the refactor looks fine, but you should ask the taskbar_decorator_win.cc owner to double check it :) https://codereview.chromium.org/268073005/diff/60001/chrome/browser/profiles/... File chrome/browser/profiles/profile_avatar_icon_util.h (right): https://codereview.chromium.org/268073005/diff/60001/chrome/browser/profiles/... chrome/browser/profiles/profile_avatar_icon_util.h:89: // Returns a bitmap with a couple of columns shaved off so it is more square, nit: newline between functions.
Hi Ben@ and Estade@, Could I get reviews on: ben - c/b/ui/views/frame estade - c/n/resources/options Thanks!
css lgtm
https://codereview.chromium.org/268073005/diff/1/chrome/browser/profiles/prof... File chrome/browser/profiles/profile_shortcut_manager_win.cc (right): https://codereview.chromium.org/268073005/diff/1/chrome/browser/profiles/prof... chrome/browser/profiles/profile_shortcut_manager_win.cc:75: const int kCurrentProfileIconVersion = 2; On 2014/05/06 00:30:03, Monica Dinculescu wrote: > Gab: Mike is actually reverting to the status quo of about 2 weeks ago, so in > theory nothing actually has changed... > > On 2014/05/05 22:58:09, gab wrote: > > As calamity@ mentioned on issue 369495, you should increment this to force the > > static profile icons used for pinned taskbar shortcuts to be regenerated. > > > > (and you should manually test that this indeed works -- I think this is the > > first time we actually do this since it was put in place!). > Ah ok, ignore me then :)! Thanks!
Swapping reviewers. - ben + sky Sky, can you please review c/b/ui/views/frame? Thanks!
https://codereview.chromium.org/268073005/diff/80001/chrome/browser/profiles/... File chrome/browser/profiles/profile_avatar_icon_util.cc (right): https://codereview.chromium.org/268073005/diff/80001/chrome/browser/profiles/... chrome/browser/profiles/profile_avatar_icon_util.cc:281: } else { nit: no else after a return (see style guide). https://codereview.chromium.org/268073005/diff/80001/chrome/browser/ui/views/... File chrome/browser/ui/views/frame/taskbar_decorator_win.cc (right): https://codereview.chromium.org/268073005/diff/80001/chrome/browser/ui/views/... chrome/browser/ui/views/frame/taskbar_decorator_win.cc:43: const SkBitmap source_bitmap = Can you pass this bitmap into the function? That way you're minimizing what is called on a separate thread.
Thanks for the feedback, sky. Addressed in the latest patch. https://codereview.chromium.org/268073005/diff/80001/chrome/browser/profiles/... File chrome/browser/profiles/profile_avatar_icon_util.cc (right): https://codereview.chromium.org/268073005/diff/80001/chrome/browser/profiles/... chrome/browser/profiles/profile_avatar_icon_util.cc:281: } else { On 2014/05/08 16:04:36, sky wrote: > nit: no else after a return (see style guide). Done. https://codereview.chromium.org/268073005/diff/80001/chrome/browser/ui/views/... File chrome/browser/ui/views/frame/taskbar_decorator_win.cc (right): https://codereview.chromium.org/268073005/diff/80001/chrome/browser/ui/views/... chrome/browser/ui/views/frame/taskbar_decorator_win.cc:43: const SkBitmap source_bitmap = On 2014/05/08 16:04:36, sky wrote: > Can you pass this bitmap into the function? That way you're minimizing what is > called on a separate thread. Done.
https://codereview.chromium.org/268073005/diff/80001/chrome/browser/ui/views/... File chrome/browser/ui/views/frame/taskbar_decorator_win.cc (right): https://codereview.chromium.org/268073005/diff/80001/chrome/browser/ui/views/... chrome/browser/ui/views/frame/taskbar_decorator_win.cc:43: const SkBitmap source_bitmap = On 2014/05/08 18:46:39, Mike Lerman wrote: > On 2014/05/08 16:04:36, sky wrote: > > Can you pass this bitmap into the function? That way you're minimizing what is > > called on a separate thread. > > Done. Sorry if I wasn't being clear. Can you move this call to content::BrowserThread::GetBlockingPool()->PostWorkerTaskWithShutdownB so that this function is not calling profiles::GetAvatarIconAsSquare? Again, the reason for this is to avoid calling out to profile functions on random threads.
https://codereview.chromium.org/268073005/diff/80001/chrome/browser/ui/views/... File chrome/browser/ui/views/frame/taskbar_decorator_win.cc (right): https://codereview.chromium.org/268073005/diff/80001/chrome/browser/ui/views/... chrome/browser/ui/views/frame/taskbar_decorator_win.cc:44: profiles::GetAvatarIconAsSquare(*bitmap.get(), 1); I'm unsure about the threading issues you're referring to. This method used to contain SkBitmap altering code, until a CL (https://codereview.chromium.org/212603011) removed it. I need to partially revert the CL (the resized avatars, not the refactoring) and return the squaring-code, but am trying to refactor the SkBitmap manipulating code into a common location from this class and from profile_shortcut_manager_win (since they code that performed the same function). This is being refactored into profile_avatar_icon_util, because it contains avatar icon manipulating functions in the profiles namespace, but nothing is thread-specific AFAIK. But I'm still new here :) I assume ui/views/frame takes place on the UI-thread. Is the concern that the profiles:: namespace should not be called directly from the UI thread, but rather from the browser thread? If that's the case, I'd rather move the new profiles::GetAvatarIconAsSquare function to somewhere accessible to the UI thread. The new locations that call it are on the UI thread, and it's just supposed to be a refactored utility function.
https://codereview.chromium.org/268073005/diff/80001/chrome/browser/ui/views/... File chrome/browser/ui/views/frame/taskbar_decorator_win.cc (right): https://codereview.chromium.org/268073005/diff/80001/chrome/browser/ui/views/... chrome/browser/ui/views/frame/taskbar_decorator_win.cc:44: profiles::GetAvatarIconAsSquare(*bitmap.get(), 1); On 2014/05/08 20:07:32, Mike Lerman wrote: > I'm unsure about the threading issues you're referring to. > > This method used to contain SkBitmap altering code, until a CL > (https://codereview.chromium.org/212603011) removed it. I need to partially > revert the CL (the resized avatars, not the refactoring) and return the > squaring-code, but am trying to refactor the SkBitmap manipulating code into a > common location from this class and from profile_shortcut_manager_win (since > they code that performed the same function). This is being refactored into > profile_avatar_icon_util, because it contains avatar icon manipulating functions > in the profiles namespace, but nothing is thread-specific AFAIK. But I'm still > new here :) > > I assume ui/views/frame takes place on the UI-thread. Is the concern that the > profiles:: namespace should not be called directly from the UI thread, but > rather from the browser thread? If that's the case, I'd rather move the new > profiles::GetAvatarIconAsSquare function to somewhere accessible to the UI > thread. The new locations that call it are on the UI thread, and it's just > supposed to be a refactored utility function. This function, SetOverlayIcon, is executed on a background thread. I prefer to avoid calling out to random functions on background threads where possible. AFAICT you should be able to do the resizing that you want in DrawTaskbarDecoration, which is executed on the main thread (main thread and UI thread are used interchangeably and mean the same thing). In other words move this code to DrawTaskbarDecoration and I'm happy.
On 2014/05/08 20:23:50, sky wrote: > https://codereview.chromium.org/268073005/diff/80001/chrome/browser/ui/views/... > File chrome/browser/ui/views/frame/taskbar_decorator_win.cc (right): > > https://codereview.chromium.org/268073005/diff/80001/chrome/browser/ui/views/... > chrome/browser/ui/views/frame/taskbar_decorator_win.cc:44: > profiles::GetAvatarIconAsSquare(*bitmap.get(), 1); > On 2014/05/08 20:07:32, Mike Lerman wrote: > > I'm unsure about the threading issues you're referring to. > > > > This method used to contain SkBitmap altering code, until a CL > > (https://codereview.chromium.org/212603011) removed it. I need to partially > > revert the CL (the resized avatars, not the refactoring) and return the > > squaring-code, but am trying to refactor the SkBitmap manipulating code into a > > common location from this class and from profile_shortcut_manager_win (since > > they code that performed the same function). This is being refactored into > > profile_avatar_icon_util, because it contains avatar icon manipulating > functions > > in the profiles namespace, but nothing is thread-specific AFAIK. But I'm still > > new here :) > > > > I assume ui/views/frame takes place on the UI-thread. Is the concern that the > > profiles:: namespace should not be called directly from the UI thread, but > > rather from the browser thread? If that's the case, I'd rather move the new > > profiles::GetAvatarIconAsSquare function to somewhere accessible to the UI > > thread. The new locations that call it are on the UI thread, and it's just > > supposed to be a refactored utility function. > > This function, SetOverlayIcon, is executed on a background thread. I prefer to > avoid calling out to random functions on background threads where possible. > > AFAICT you should be able to do the resizing that you want in > DrawTaskbarDecoration, which is executed on the main thread (main thread and UI > thread are used interchangeably and mean the same thing). In other words move > this code to DrawTaskbarDecoration and I'm happy. Ah, thanks for the explanation, I can see how this makes the code better.
https://codereview.chromium.org/268073005/diff/80001/chrome/browser/ui/views/... File chrome/browser/ui/views/frame/taskbar_decorator_win.cc (right): https://codereview.chromium.org/268073005/diff/80001/chrome/browser/ui/views/... chrome/browser/ui/views/frame/taskbar_decorator_win.cc:44: profiles::GetAvatarIconAsSquare(*bitmap.get(), 1); On 2014/05/08 20:23:51, sky wrote: > On 2014/05/08 20:07:32, Mike Lerman wrote: > > I'm unsure about the threading issues you're referring to. > > > > This method used to contain SkBitmap altering code, until a CL > > (https://codereview.chromium.org/212603011) removed it. I need to partially > > revert the CL (the resized avatars, not the refactoring) and return the > > squaring-code, but am trying to refactor the SkBitmap manipulating code into a > > common location from this class and from profile_shortcut_manager_win (since > > they code that performed the same function). This is being refactored into > > profile_avatar_icon_util, because it contains avatar icon manipulating > functions > > in the profiles namespace, but nothing is thread-specific AFAIK. But I'm still > > new here :) > > > > I assume ui/views/frame takes place on the UI-thread. Is the concern that the > > profiles:: namespace should not be called directly from the UI thread, but > > rather from the browser thread? If that's the case, I'd rather move the new > > profiles::GetAvatarIconAsSquare function to somewhere accessible to the UI > > thread. The new locations that call it are on the UI thread, and it's just > > supposed to be a refactored utility function. > > This function, SetOverlayIcon, is executed on a background thread. I prefer to > avoid calling out to random functions on background threads where possible. > > AFAICT you should be able to do the resizing that you want in > DrawTaskbarDecoration, which is executed on the main thread (main thread and UI > thread are used interchangeably and mean the same thing). In other words move > this code to DrawTaskbarDecoration and I'm happy. Done.
https://codereview.chromium.org/268073005/diff/120001/chrome/browser/profiles... File chrome/browser/profiles/profile_avatar_icon_util.h (right): https://codereview.chromium.org/268073005/diff/120001/chrome/browser/profiles... chrome/browser/profiles/profile_avatar_icon_util.h:95: SkBitmap& square_bitmap); Style guide says refs passed to functions must be const. If you need non-const, then pass a pointer. That said, why not make this return a SkBitmap? SkBitmap has copy semantics. https://codereview.chromium.org/268073005/diff/120001/chrome/browser/ui/views... File chrome/browser/ui/views/frame/taskbar_decorator_win.cc (right): https://codereview.chromium.org/268073005/diff/120001/chrome/browser/ui/views... chrome/browser/ui/views/frame/taskbar_decorator_win.cc:43: // Maintain aspect ratio on resize. It is assumed that the image is wider assumed->DCHECK https://codereview.chromium.org/268073005/diff/120001/chrome/browser/ui/views... chrome/browser/ui/views/frame/taskbar_decorator_win.cc:46: size_t resized_height = Why the size_t? Resize takes ints. https://codereview.chromium.org/268073005/diff/120001/chrome/browser/ui/views... chrome/browser/ui/views/frame/taskbar_decorator_win.cc:89: SkBitmap* square_bitmap = new SkBitmap(); Make this a scoped_ptr. But if you go with the other issue I mentioned for GetAvatarIconAsSquare (make it return a SkBitmap), then this won't be an issue.
https://codereview.chromium.org/268073005/diff/120001/chrome/browser/profiles... File chrome/browser/profiles/profile_avatar_icon_util.h (right): https://codereview.chromium.org/268073005/diff/120001/chrome/browser/profiles... chrome/browser/profiles/profile_avatar_icon_util.h:95: SkBitmap& square_bitmap); On 2014/05/09 20:52:19, sky wrote: > Style guide says refs passed to functions must be const. If you need non-const, > then pass a pointer. That said, why not make this return a SkBitmap? SkBitmap > has copy semantics. I had it like this originally, but then changed it to a parameter. Back to a returned value. https://codereview.chromium.org/268073005/diff/120001/chrome/browser/ui/views... File chrome/browser/ui/views/frame/taskbar_decorator_win.cc (right): https://codereview.chromium.org/268073005/diff/120001/chrome/browser/ui/views... chrome/browser/ui/views/frame/taskbar_decorator_win.cc:43: // Maintain aspect ratio on resize. It is assumed that the image is wider On 2014/05/09 20:52:19, sky wrote: > assumed->DCHECK Done. https://codereview.chromium.org/268073005/diff/120001/chrome/browser/ui/views... chrome/browser/ui/views/frame/taskbar_decorator_win.cc:46: size_t resized_height = On 2014/05/09 20:52:19, sky wrote: > Why the size_t? Resize takes ints. Done. https://codereview.chromium.org/268073005/diff/120001/chrome/browser/ui/views... chrome/browser/ui/views/frame/taskbar_decorator_win.cc:89: SkBitmap* square_bitmap = new SkBitmap(); On 2014/05/09 20:52:19, sky wrote: > Make this a scoped_ptr. But if you go with the other issue I mentioned for > GetAvatarIconAsSquare (make it return a SkBitmap), then this won't be an issue. I have I think addressed this issue, but I'll admit I am not 100% clear about this comment. Do you want me to completely remove the scoped_ptr semantics from this method and the signature to SetOverlayIcon, and pass the SkBitmap directly into that method, invoking its copy semantics? I think you wanted me to keep a scoped_ptr, which I have kept as the variable bitmap. I've introduced a new SkBitmap() call because I'm concerned that the SkBitmap object returned from GetAvatarIconAsSquare will go out of scope once the method return has completed.
LGTM https://codereview.chromium.org/268073005/diff/120001/chrome/browser/ui/views... File chrome/browser/ui/views/frame/taskbar_decorator_win.cc (right): https://codereview.chromium.org/268073005/diff/120001/chrome/browser/ui/views... chrome/browser/ui/views/frame/taskbar_decorator_win.cc:89: SkBitmap* square_bitmap = new SkBitmap(); On 2014/05/12 15:35:47, Mike Lerman wrote: > On 2014/05/09 20:52:19, sky wrote: > > Make this a scoped_ptr. But if you go with the other issue I mentioned for > > GetAvatarIconAsSquare (make it return a SkBitmap), then this won't be an > issue. > > I have I think addressed this issue, but I'll admit I am not 100% clear about > this comment. Do you want me to completely remove the scoped_ptr semantics from > this method and the signature to SetOverlayIcon, and pass the SkBitmap directly > into that method, invoking its copy semantics? > I think you wanted me to keep a scoped_ptr, which I have kept as the variable > bitmap. I've introduced a new SkBitmap() call because I'm concerned that the > SkBitmap object returned from GetAvatarIconAsSquare will go out of scope once > the method return has completed. You got what I was after in your latest patch. You originally had: SkBitmap* square_bitmap = new SkBitmap(); Which is error prone and easy to leak.
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/268073005/140001
FYI, CQ is re-trying this CL (attempt #1). Please consider checking whether the failures are real, and report flakes to chrome-troopers@google.com. The failing builders are: win_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
Message was sent while issue was closed.
Change committed as 270008 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
