Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2057203002:
Bringing back fast user switching on desktop user menu (Closed)
DescriptionSpecifically:
1. Migrated the fast user switching buttons triggered by right clicks back to user menu's main page
2. Added a "Guest" button
3. Added a "Close all your windows" button
4. Made UI modifications such as circular icons, paddings, etc.
See screenshot:
https://drive.google.com/a/google.com/file/d/0B7Fvv7JszRyGY2tBWklPOFVKS28/view?usp=sharing
Mockup:
https://folio.googleplex.com/chrome-ux-specs-and-sources/Chrome%20Desktop%20Sign%20In/user_menu#%2Fspec-2.png (Note the appropriate icons for "Guest" and "Close all your windows" will be added as a separate CL.)
BUG=615893
Committed: https://crrev.com/0a96c46f7536d7dcd037738eb5cb8d653c8ef857
Cr-Commit-Position: refs/heads/master@{#402607}
Patch Set 1 #Patch Set 2 : Minor cleanup #
Total comments: 9
Patch Set 3 : Rebased and added enum #Patch Set 4 : ifdef for Android #
Total comments: 6
Patch Set 5 : Modifying ifdef #Patch Set 6 : More ifdef for failed tests #Patch Set 7 : Added check for guest button #
Total comments: 3
Patch Set 8 : More #ifdef #Patch Set 9 : DCHECK and comment #
Total comments: 4
Patch Set 10 : Minor comment and consolidating "Close all windows" change #
Total comments: 9
Patch Set 11 : Addressed sky's comments #
Total comments: 7
Patch Set 12 : Addressed Anthony's comments #
Messages
Total messages: 40 (19 generated)
Description was changed from ========== Bringing back fast user switching BUG= ========== to ========== Specifically: 1. Migrated the fast user switching buttons triggered by right clicks back to user menu's main page 2. Added a "Guest" button 3. Made UI modifications such as circular icons, paddings, etc. See screenshot: https://drive.google.com/a/google.com/file/d/0B7Fvv7JszRyGdXNERWVEMzZhdlk/vie... Mockup: https://folio.googleplex.com/chrome-ux-specs-and-sources/Chrome%20Desktop%20S... BUG=615893 ==========
janeliulwq@google.com changed reviewers: + rogerta@chromium.org
janeliulwq@google.com changed required reviewers: + rogerta@chromium.org
https://codereview.chromium.org/2057203002/diff/20001/chrome/browser/profiles... File chrome/browser/profiles/profile_avatar_icon_util.cc (right): https://codereview.chromium.org/2057203002/diff/20001/chrome/browser/profiles... chrome/browser/profiles/profile_avatar_icon_util.cc:120: gfx::Path circular_mask; Question: Why am I getting "error: undefined reference to 'gfx::Path::Path()'" for Android tests?
https://codereview.chromium.org/2057203002/diff/20001/chrome/browser/profiles... File chrome/browser/profiles/profile_avatar_icon_util.cc (right): https://codereview.chromium.org/2057203002/diff/20001/chrome/browser/profiles... chrome/browser/profiles/profile_avatar_icon_util.cc:77: bool is_circular = false); No spaces around = for default args. https://codereview.chromium.org/2057203002/diff/20001/chrome/browser/profiles... chrome/browser/profiles/profile_avatar_icon_util.cc:120: gfx::Path circular_mask; On 2016/06/10 at 17:28:55, Jane wrote: > Question: Why am I getting "error: undefined reference to 'gfx::Path::Path()'" for Android tests? Sorry, not sure. Try asking Justin (junov@) https://codereview.chromium.org/2057203002/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/profiles/profile_chooser_view.cc (right): https://codereview.chromium.org/2057203002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/profiles/profile_chooser_view.cc:1531: item_icon, true, kProfileIconSize, kProfileIconSize, true); Instead of using true|false for last arg (the new one you just added), define enums like "circular" or "square". This makes the code more readable. https://codereview.chromium.org/2057203002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/profiles/profile_chooser_view.cc:1600: views::kRelatedControlVerticalSpacing); Add { and } when the block of the if statement spans more than one line.
https://codereview.chromium.org/2057203002/diff/20001/chrome/browser/profiles... File chrome/browser/profiles/profile_avatar_icon_util.cc (right): https://codereview.chromium.org/2057203002/diff/20001/chrome/browser/profiles... chrome/browser/profiles/profile_avatar_icon_util.cc:77: bool is_circular = false); On 2016/06/13 15:20:44, Roger Tawa wrote: > No spaces around = for default args. Done. https://codereview.chromium.org/2057203002/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/profiles/profile_chooser_view.cc (right): https://codereview.chromium.org/2057203002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/profiles/profile_chooser_view.cc:1531: item_icon, true, kProfileIconSize, kProfileIconSize, true); On 2016/06/13 15:20:44, Roger Tawa wrote: > Instead of using true|false for last arg (the new one you just added), define > enums like "circular" or "square". This makes the code more readable. Done. https://codereview.chromium.org/2057203002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/profiles/profile_chooser_view.cc:1600: views::kRelatedControlVerticalSpacing); On 2016/06/13 15:20:44, Roger Tawa wrote: > Add { and } when the block of the if statement spans more than one line. Done.
https://codereview.chromium.org/2057203002/diff/20001/chrome/browser/profiles... File chrome/browser/profiles/profile_avatar_icon_util.cc (right): https://codereview.chromium.org/2057203002/diff/20001/chrome/browser/profiles... chrome/browser/profiles/profile_avatar_icon_util.cc:120: gfx::Path circular_mask; On 2016/06/13 15:20:44, Roger Tawa wrote: > On 2016/06/10 at 17:28:55, Jane wrote: > > Question: Why am I getting "error: undefined reference to 'gfx::Path::Path()'" > for Android tests? > > Sorry, not sure. Try asking Justin (junov@) Added #ifdef and the Android unit tests passed! PTAL.
Looking good Jane. A couple of questions below. https://codereview.chromium.org/2057203002/diff/60001/chrome/browser/profiles... File chrome/browser/profiles/profile_avatar_icon_util.cc (right): https://codereview.chromium.org/2057203002/diff/60001/chrome/browser/profiles... chrome/browser/profiles/profile_avatar_icon_util.cc:134: } else if (position_ == POSITION_CENTER) { If someone happens to use SHAPE_CIRCLE in clank, they won't get an error, but it probably won't draw correctly. I think it would be better to put SHAPE_CIRCLE inside the #ifdef too, so you just can't do it on android. https://codereview.chromium.org/2057203002/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/profiles/profile_chooser_view.cc (right): https://codereview.chromium.org/2057203002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/profiles/profile_chooser_view.cc:637: switches::IsMaterialDesignUserMenu())) { I'm not sure about placement of ( and ). did you mean: if ((view_mode == profiles::BUBBLE_VIEW_MODE_FAST_PROFILE_CHOOSER && !profiles::HasProfileSwitchTargets(browser->profile()) || switches::IsMaterialDesignUserMenu())) { I moved the ( from line 636 to 635.
Patchset #5 (id:80001) has been deleted
https://codereview.chromium.org/2057203002/diff/60001/chrome/browser/profiles... File chrome/browser/profiles/profile_avatar_icon_util.cc (right): https://codereview.chromium.org/2057203002/diff/60001/chrome/browser/profiles... chrome/browser/profiles/profile_avatar_icon_util.cc:134: } else if (position_ == POSITION_CENTER) { On 2016/06/14 15:25:13, Roger Tawa wrote: > If someone happens to use SHAPE_CIRCLE in clank, they won't get an error, but it > probably won't draw correctly. I think it would be better to put SHAPE_CIRCLE > inside the #ifdef too, so you just can't do it on android. Done. I assume you meant putting the if condition inside the same #ifdef, is that right? https://codereview.chromium.org/2057203002/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/profiles/profile_chooser_view.cc (right): https://codereview.chromium.org/2057203002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/profiles/profile_chooser_view.cc:637: switches::IsMaterialDesignUserMenu())) { On 2016/06/14 15:25:13, Roger Tawa wrote: > I'm not sure about placement of ( and ). did you mean: > > if ((view_mode == profiles::BUBBLE_VIEW_MODE_FAST_PROFILE_CHOOSER && > !profiles::HasProfileSwitchTargets(browser->profile()) || > switches::IsMaterialDesignUserMenu())) { > > I moved the ( from line 636 to 635. I meant what I wrote. AFAIU, this function shows bubble for fast profile chooser and other view modes; this if statement specifically blocks the fast profile chooser view if 1. there is no profile to switch to 2. user menu is the material design version. Does this make sense?
Hi Jane, see comments below. Let me know if that makes sense. Thanks. https://codereview.chromium.org/2057203002/diff/60001/chrome/browser/profiles... File chrome/browser/profiles/profile_avatar_icon_util.cc (right): https://codereview.chromium.org/2057203002/diff/60001/chrome/browser/profiles... chrome/browser/profiles/profile_avatar_icon_util.cc:134: } else if (position_ == POSITION_CENTER) { On 2016/06/15 14:21:13, Jane wrote: > On 2016/06/14 15:25:13, Roger Tawa wrote: > > If someone happens to use SHAPE_CIRCLE in clank, they won't get an error, but > it > > probably won't draw correctly. I think it would be better to put SHAPE_CIRCLE > > inside the #ifdef too, so you just can't do it on android. > > Done. I assume you meant putting the if condition inside the same #ifdef, is > that right? As is, the CIRCLE enum is still defined on android. So someone code could still try to use it. See comment in header file. https://codereview.chromium.org/2057203002/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/profiles/profile_chooser_view.cc (right): https://codereview.chromium.org/2057203002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/profiles/profile_chooser_view.cc:637: switches::IsMaterialDesignUserMenu())) { On 2016/06/15 14:21:13, Jane wrote: > On 2016/06/14 15:25:13, Roger Tawa wrote: > > I'm not sure about placement of ( and ). did you mean: > > > > if ((view_mode == profiles::BUBBLE_VIEW_MODE_FAST_PROFILE_CHOOSER && > > !profiles::HasProfileSwitchTargets(browser->profile()) || > > switches::IsMaterialDesignUserMenu())) { > > > > I moved the ( from line 636 to 635. > > I meant what I wrote. AFAIU, this function shows bubble for fast profile chooser > and other view modes; this if statement specifically blocks the fast profile > chooser view if 1. there is no profile to switch to 2. user menu is the material > design version. Does this make sense? Got it, I misunderstood the condition. https://codereview.chromium.org/2057203002/diff/140001/chrome/browser/profile... File chrome/browser/profiles/profile_avatar_icon_util.h (right): https://codereview.chromium.org/2057203002/diff/140001/chrome/browser/profile... chrome/browser/profiles/profile_avatar_icon_util.h:42: SHAPE_CIRCLE, You should remove the enum value here with an #ifdef. Alternatively, add a comment here that says CIRCLE is only available on desktop platforms and then add a DCHECK to enforce this in the cc file.
Patchset #8 (id:160001) has been deleted
Patchset #8 (id:180001) has been deleted
Patchset #9 (id:220001) has been deleted
Patchset #9 (id:240001) has been deleted
Thanks! https://codereview.chromium.org/2057203002/diff/140001/chrome/browser/profile... File chrome/browser/profiles/profile_avatar_icon_util.h (right): https://codereview.chromium.org/2057203002/diff/140001/chrome/browser/profile... chrome/browser/profiles/profile_avatar_icon_util.h:42: SHAPE_CIRCLE, On 2016/06/17 14:35:57, Roger Tawa wrote: > You should remove the enum value here with an #ifdef. > > Alternatively, add a comment here that says CIRCLE is only available on desktop > platforms and then add a DCHECK to enforce this in the cc file. I tried putting the enum definition under #ifdef (see patch 8), but the Android tests failed with the error below. I'm confused because I don't see how the compiler would think it's ambiguous given the two candidate definitions. Besides, the first definition in the header file shouldn't even have AvatarShape for OS_ANDROID, and should be the same definition as the one in .cc. Do you know why this is happening and how I could fix it? Thanks! ../../chrome/browser/profiles/profile_avatar_icon_util.cc: In function 'gfx::Image profiles::GetAvatarIconForMenu(const gfx::Image&, bool)': ../../chrome/browser/profiles/profile_avatar_icon_util.cc:310:63: error: call of overloaded 'GetSizedAvatarIcon(const gfx::Image&, bool&, const int&, const int&)' is ambiguous image, is_rectangle, kAvatarIconWidth, kAvatarIconHeight); ^ ../../chrome/browser/profiles/profile_avatar_icon_util.cc:310:63: note: candidates are: In file included from ../../chrome/browser/profiles/profile_avatar_icon_util.cc:5:0: ../../chrome/browser/profiles/profile_avatar_icon_util.h:56:12: note: gfx::Image profiles::GetSizedAvatarIcon(const gfx::Image&, bool, int, int, profiles::AvatarShape) gfx::Image GetSizedAvatarIcon(const gfx::Image& image, ^ ../../chrome/browser/profiles/profile_avatar_icon_util.cc:277:12: note: gfx::Image profiles::GetSizedAvatarIcon(const gfx::Image&, bool, int, int) gfx::Image GetSizedAvatarIcon(const gfx::Image& image, I also tried the DCHECK way (see patch 9) and the test passed. I would say the code looks cleaner, but definitely not as rigorous.
lgtm Thanks for investigating all my questions Jane! https://codereview.chromium.org/2057203002/diff/140001/chrome/browser/profile... File chrome/browser/profiles/profile_avatar_icon_util.h (right): https://codereview.chromium.org/2057203002/diff/140001/chrome/browser/profile... chrome/browser/profiles/profile_avatar_icon_util.h:42: SHAPE_CIRCLE, On 2016/06/20 13:10:24, Jane wrote: > On 2016/06/17 14:35:57, Roger Tawa wrote: > > You should remove the enum value here with an #ifdef. > > > > Alternatively, add a comment here that says CIRCLE is only available on > desktop > > platforms and then add a DCHECK to enforce this in the cc file. > > I tried putting the enum definition under #ifdef (see patch 8), but the Android > tests failed with the error below. I'm confused because I don't see how the > compiler would think it's ambiguous given the two candidate definitions. > Besides, the first definition in the header file shouldn't even have AvatarShape > for OS_ANDROID, and should be the same definition as the one in .cc. Do you know > why this is happening and how I could fix it? Thanks! > > ../../chrome/browser/profiles/profile_avatar_icon_util.cc: In function > 'gfx::Image profiles::GetAvatarIconForMenu(const gfx::Image&, bool)': > ../../chrome/browser/profiles/profile_avatar_icon_util.cc:310:63: error: call of > overloaded 'GetSizedAvatarIcon(const gfx::Image&, bool&, const int&, const > int&)' is ambiguous > image, is_rectangle, kAvatarIconWidth, kAvatarIconHeight); > ^ > ../../chrome/browser/profiles/profile_avatar_icon_util.cc:310:63: note: > candidates are: > In file included from > ../../chrome/browser/profiles/profile_avatar_icon_util.cc:5:0: > ../../chrome/browser/profiles/profile_avatar_icon_util.h:56:12: note: gfx::Image > profiles::GetSizedAvatarIcon(const gfx::Image&, bool, int, int, > profiles::AvatarShape) > gfx::Image GetSizedAvatarIcon(const gfx::Image& image, > ^ > ../../chrome/browser/profiles/profile_avatar_icon_util.cc:277:12: note: > gfx::Image profiles::GetSizedAvatarIcon(const gfx::Image&, bool, int, int) > gfx::Image GetSizedAvatarIcon(const gfx::Image& image, > > > I also tried the DCHECK way (see patch 9) and the test passed. I would say the > code looks cleaner, but definitely not as rigorous. Not sure about ambiguity. https://codereview.chromium.org/2057203002/diff/260001/chrome/browser/profile... File chrome/browser/profiles/profile_avatar_icon_util.cc (right): https://codereview.chromium.org/2057203002/diff/260001/chrome/browser/profile... chrome/browser/profiles/profile_avatar_icon_util.cc:133: // Circular shape is only available on desktop platforms Nit: end comment with period. https://codereview.chromium.org/2057203002/diff/260001/chrome/browser/profile... chrome/browser/profiles/profile_avatar_icon_util.cc:258: { { should stay on previous line.
Description was changed from ========== Specifically: 1. Migrated the fast user switching buttons triggered by right clicks back to user menu's main page 2. Added a "Guest" button 3. Made UI modifications such as circular icons, paddings, etc. See screenshot: https://drive.google.com/a/google.com/file/d/0B7Fvv7JszRyGdXNERWVEMzZhdlk/vie... Mockup: https://folio.googleplex.com/chrome-ux-specs-and-sources/Chrome%20Desktop%20S... BUG=615893 ========== to ========== Specifically: 1. Migrated the fast user switching buttons triggered by right clicks back to user menu's main page 2. Added a "Guest" button 3. Added a "Close all your windows" button 4. Made UI modifications such as circular icons, paddings, etc. See screenshot: https://drive.google.com/a/google.com/file/d/0B7Fvv7JszRyGY2tBWklPOFVKS28/vie... Mockup: https://folio.googleplex.com/chrome-ux-specs-and-sources/Chrome%20Desktop%20S... BUG=615893 ==========
Description was changed from ========== Specifically: 1. Migrated the fast user switching buttons triggered by right clicks back to user menu's main page 2. Added a "Guest" button 3. Added a "Close all your windows" button 4. Made UI modifications such as circular icons, paddings, etc. See screenshot: https://drive.google.com/a/google.com/file/d/0B7Fvv7JszRyGY2tBWklPOFVKS28/vie... Mockup: https://folio.googleplex.com/chrome-ux-specs-and-sources/Chrome%20Desktop%20S... BUG=615893 ========== to ========== Specifically: 1. Migrated the fast user switching buttons triggered by right clicks back to user menu's main page 2. Added a "Guest" button 3. Added a "Close all your windows" button 4. Made UI modifications such as circular icons, paddings, etc. See screenshot: https://drive.google.com/a/google.com/file/d/0B7Fvv7JszRyGY2tBWklPOFVKS28/vie... Mockup: https://folio.googleplex.com/chrome-ux-specs-and-sources/Chrome%20Desktop%20S... (Note the appropriate icons for "Guest" and "Close all your windows" will be added as a separate CL.) BUG=615893 ==========
janeliulwq@google.com changed reviewers: + anthonyvd@chromium.org, sky@chromium.org
janeliulwq@google.com changed required reviewers: + anthonyvd@chromium.org, sky@chromium.org
Hi Anthony, please take a look at the profile-related changes; and sky, please review the UI-related changes. Thanks! https://codereview.chromium.org/2057203002/diff/260001/chrome/browser/profile... File chrome/browser/profiles/profile_avatar_icon_util.cc (right): https://codereview.chromium.org/2057203002/diff/260001/chrome/browser/profile... chrome/browser/profiles/profile_avatar_icon_util.cc:133: // Circular shape is only available on desktop platforms On 2016/06/20 14:04:44, Roger Tawa wrote: > Nit: end comment with period. Done. https://codereview.chromium.org/2057203002/diff/260001/chrome/browser/profile... chrome/browser/profiles/profile_avatar_icon_util.cc:258: { On 2016/06/20 14:04:44, Roger Tawa wrote: > { should stay on previous line. Done.
https://codereview.chromium.org/2057203002/diff/280001/chrome/browser/ui/view... File chrome/browser/ui/views/profiles/profile_chooser_view.cc (right): https://codereview.chromium.org/2057203002/diff/280001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/profile_chooser_view.cc:705: guest_profile_button_ = NULL; nullptr (feel free to change all NULLs in this function if you want). https://codereview.chromium.org/2057203002/diff/280001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/profile_chooser_view.cc:911: if (sender == guest_profile_button_) { guest_profile_button_ and/or close_all_windows_button_ may be null right? You should guard against that. https://codereview.chromium.org/2057203002/diff/280001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/profile_chooser_view.cc:917: // The UI should have prevented the user from allowing the selection of 914 should be a DCHECK and not an if. https://codereview.chromium.org/2057203002/diff/280001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/profile_chooser_view.cc:1621: if (switches::IsMaterialDesignUserMenu()) { nit: no {}
Patchset #11 (id:300001) has been deleted
Thanks! https://codereview.chromium.org/2057203002/diff/280001/chrome/browser/ui/view... File chrome/browser/ui/views/profiles/profile_chooser_view.cc (right): https://codereview.chromium.org/2057203002/diff/280001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/profile_chooser_view.cc:705: guest_profile_button_ = NULL; On 2016/06/27 22:49:36, sky wrote: > nullptr (feel free to change all NULLs in this function if you want). Done. https://codereview.chromium.org/2057203002/diff/280001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/profile_chooser_view.cc:911: if (sender == guest_profile_button_) { On 2016/06/27 22:49:36, sky wrote: > guest_profile_button_ and/or close_all_windows_button_ may be null right? You > should guard against that. Sorry, I'm confused. The sender button can't be null, so if guest_profile_button_ is null, this condition will just be false, right? Where should there be a guard? I also referred to the go_incognito_button_ below, which can also be null, but there doesn't seem to be any guards. https://codereview.chromium.org/2057203002/diff/280001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/profile_chooser_view.cc:917: // The UI should have prevented the user from allowing the selection of On 2016/06/27 22:49:36, sky wrote: > 914 should be a DCHECK and not an if. Done. https://codereview.chromium.org/2057203002/diff/280001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/profile_chooser_view.cc:1621: if (switches::IsMaterialDesignUserMenu()) { On 2016/06/27 22:49:36, sky wrote: > nit: no {} Done.
c/b/profiles/ looking good, only a few comments. Thanks Jane! https://codereview.chromium.org/2057203002/diff/320001/chrome/browser/profile... File chrome/browser/profiles/profile_avatar_icon_util.cc (right): https://codereview.chromium.org/2057203002/diff/320001/chrome/browser/profile... chrome/browser/profiles/profile_avatar_icon_util.cc:79: profiles::AvatarShape shape=profiles::SHAPE_SQUARE); nit: spaces here too https://codereview.chromium.org/2057203002/diff/320001/chrome/browser/profile... chrome/browser/profiles/profile_avatar_icon_util.cc:137: // Draw the avatar on the bottom center of the canvas; overrides the Could you explain why in this comment please? https://codereview.chromium.org/2057203002/diff/320001/chrome/browser/profile... File chrome/browser/profiles/profile_avatar_icon_util.h (right): https://codereview.chromium.org/2057203002/diff/320001/chrome/browser/profile... chrome/browser/profiles/profile_avatar_icon_util.h:55: AvatarShape shape=SHAPE_SQUARE); nit: I think you want spaces around = here We also tend to avoid default arguments in favor of overloading where possible (https://google.github.io/styleguide/cppguide.html#Default_Arguments). You could instead overload the constructor and use delegating constructors. For example: gfx:Image GetSizedAvatarIcon(..., AvatarShape shape) { ... } gfx:Image GetSizedAvatarIcon(...) : GetSizedAvatarIcon(..., shape) { }
Thanks! https://codereview.chromium.org/2057203002/diff/320001/chrome/browser/profile... File chrome/browser/profiles/profile_avatar_icon_util.cc (right): https://codereview.chromium.org/2057203002/diff/320001/chrome/browser/profile... chrome/browser/profiles/profile_avatar_icon_util.cc:79: profiles::AvatarShape shape=profiles::SHAPE_SQUARE); On 2016/06/28 14:50:46, anthonyvd wrote: > nit: spaces here too Changed this to function overloads. https://codereview.chromium.org/2057203002/diff/320001/chrome/browser/profile... chrome/browser/profiles/profile_avatar_icon_util.cc:137: // Draw the avatar on the bottom center of the canvas; overrides the On 2016/06/28 14:50:46, anthonyvd wrote: > Could you explain why in this comment please? Done. Does this make sense? https://codereview.chromium.org/2057203002/diff/320001/chrome/browser/profile... File chrome/browser/profiles/profile_avatar_icon_util.h (right): https://codereview.chromium.org/2057203002/diff/320001/chrome/browser/profile... chrome/browser/profiles/profile_avatar_icon_util.h:55: AvatarShape shape=SHAPE_SQUARE); On 2016/06/28 14:50:46, anthonyvd wrote: > nit: I think you want spaces around = here > > We also tend to avoid default arguments in favor of overloading where possible > (https://google.github.io/styleguide/cppguide.html#Default_Arguments). You could > instead overload the constructor and use delegating constructors. > > For example: > > gfx:Image GetSizedAvatarIcon(..., AvatarShape shape) { ... } > gfx:Image GetSizedAvatarIcon(...) : GetSizedAvatarIcon(..., shape) { } Thanks! Good to learn. I modified both functions to follow this format. Regarding the space, git cl format does force that, but Roger told me to remove them in a previous comment...
LGTM https://codereview.chromium.org/2057203002/diff/280001/chrome/browser/ui/view... File chrome/browser/ui/views/profiles/profile_chooser_view.cc (right): https://codereview.chromium.org/2057203002/diff/280001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/profile_chooser_view.cc:911: if (sender == guest_profile_button_) { On 2016/06/28 13:44:25, Jane wrote: > On 2016/06/27 22:49:36, sky wrote: > > guest_profile_button_ and/or close_all_windows_button_ may be null right? You > > should guard against that. > > Sorry, I'm confused. The sender button can't be null, so if > guest_profile_button_ is null, this condition will just be false, right? Where > should there be a guard? > I also referred to the go_incognito_button_ below, which can also be null, but > there doesn't seem to be any guards. I find it confusing that you can be checking against null multiple times, but you're right. Ok, ignore this.
lgtm, thanks! https://codereview.chromium.org/2057203002/diff/320001/chrome/browser/profile... File chrome/browser/profiles/profile_avatar_icon_util.cc (right): https://codereview.chromium.org/2057203002/diff/320001/chrome/browser/profile... chrome/browser/profiles/profile_avatar_icon_util.cc:137: // Draw the avatar on the bottom center of the canvas; overrides the On 2016/06/28 at 16:32:53, Jane wrote: > On 2016/06/28 14:50:46, anthonyvd wrote: > > Could you explain why in this comment please? > > Done. Does this make sense? Yep, perfect :)
The CQ bit was checked by janeliulwq@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from rogerta@chromium.org Link to the patchset: https://codereview.chromium.org/2057203002/#ps340001 (title: "Addressed Anthony's comments")
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
Try jobs failed on following builders: android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
The CQ bit was checked by janeliulwq@google.com
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 ========== Specifically: 1. Migrated the fast user switching buttons triggered by right clicks back to user menu's main page 2. Added a "Guest" button 3. Added a "Close all your windows" button 4. Made UI modifications such as circular icons, paddings, etc. See screenshot: https://drive.google.com/a/google.com/file/d/0B7Fvv7JszRyGY2tBWklPOFVKS28/vie... Mockup: https://folio.googleplex.com/chrome-ux-specs-and-sources/Chrome%20Desktop%20S... (Note the appropriate icons for "Guest" and "Close all your windows" will be added as a separate CL.) BUG=615893 ========== to ========== Specifically: 1. Migrated the fast user switching buttons triggered by right clicks back to user menu's main page 2. Added a "Guest" button 3. Added a "Close all your windows" button 4. Made UI modifications such as circular icons, paddings, etc. See screenshot: https://drive.google.com/a/google.com/file/d/0B7Fvv7JszRyGY2tBWklPOFVKS28/vie... Mockup: https://folio.googleplex.com/chrome-ux-specs-and-sources/Chrome%20Desktop%20S... (Note the appropriate icons for "Guest" and "Close all your windows" will be added as a separate CL.) BUG=615893 ==========
Message was sent while issue was closed.
Committed patchset #12 (id:340001)
Message was sent while issue was closed.
Description was changed from ========== Specifically: 1. Migrated the fast user switching buttons triggered by right clicks back to user menu's main page 2. Added a "Guest" button 3. Added a "Close all your windows" button 4. Made UI modifications such as circular icons, paddings, etc. See screenshot: https://drive.google.com/a/google.com/file/d/0B7Fvv7JszRyGY2tBWklPOFVKS28/vie... Mockup: https://folio.googleplex.com/chrome-ux-specs-and-sources/Chrome%20Desktop%20S... (Note the appropriate icons for "Guest" and "Close all your windows" will be added as a separate CL.) BUG=615893 ========== to ========== Specifically: 1. Migrated the fast user switching buttons triggered by right clicks back to user menu's main page 2. Added a "Guest" button 3. Added a "Close all your windows" button 4. Made UI modifications such as circular icons, paddings, etc. See screenshot: https://drive.google.com/a/google.com/file/d/0B7Fvv7JszRyGY2tBWklPOFVKS28/vie... Mockup: https://folio.googleplex.com/chrome-ux-specs-and-sources/Chrome%20Desktop%20S... (Note the appropriate icons for "Guest" and "Close all your windows" will be added as a separate CL.) BUG=615893 Committed: https://crrev.com/0a96c46f7536d7dcd037738eb5cb8d653c8ef857 Cr-Commit-Position: refs/heads/master@{#402607} ==========
Message was sent while issue was closed.
Patchset 12 (id:??) landed as https://crrev.com/0a96c46f7536d7dcd037738eb5cb8d653c8ef857 Cr-Commit-Position: refs/heads/master@{#402607} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
