Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2023093002:
Reflow of the profile items in desktop user menu (Closed)
DescriptionReflow of the profile items in desktop user menu. Specifically:
1. Moved and resized the avatar icon as well as icon badges
1. Moved avatar name up to be on the right of the profile icon
2. Moved username and "Your accounts" link up and below the avatar name
3. Other dimension and padding changes
See design doc here ("Reflow the items in the first section"): https://docs.google.com/document/d/17yHrKd_EU6WoIAgljnZmNCSJLRlbM3O2Pxr7aPEnqik/edit?ts=57445a70#heading=h.6xesoh23gozz
See redlines here:
https://folio.googleplex.com/chrome-ux-specs-and-sources/Chrome%20Desktop%20Sign%20In/user_menu#
See screenshots here (marked with "3.*"):
https://drive.google.com/corp/drive/folders/0B7Fvv7JszRyGY2NrSEhheHptdG8
Downstream CL: https://codereview.chromium.org/2052473003/
BUG=615893
Committed: https://crrev.com/f9860fa8491d8f794cfeaa0f05d7240d3d89357e
Cr-Commit-Position: refs/heads/master@{#403190}
Patch Set 1 #Patch Set 2 : Added flags #Patch Set 3 : Consolidated with upstream CL #
Total comments: 7
Patch Set 4 : Addressed comments #
Total comments: 12
Patch Set 5 : Addressed formatting & naming comments #
Total comments: 18
Patch Set 6 : Rebased and addressed comments #
Total comments: 2
Patch Set 7 : Moved variables #Patch Set 8 : Box layout #
Total comments: 48
Patch Set 9 : More comments #Patch Set 10 : More comments x 2 #
Total comments: 2
Patch Set 11 : Clean up layout (and rebased) #
Total comments: 22
Patch Set 12 : More comments #Patch Set 13 : More comments x 2 #
Total comments: 8
Patch Set 14 : Clean up profile badge #
Total comments: 11
Patch Set 15 : Some cleanup #
Total comments: 3
Patch Set 16 : Profile badge clipping and color fixes #Patch Set 17 : Rebased and few changes in PaintChildren of EditableProfilePhoto #Patch Set 18 : Removed redundancy #
Total comments: 8
Patch Set 19 : Minor comments #
Total comments: 4
Patch Set 20 : Final nits #Patch Set 21 : Test fix #
Messages
Total messages: 74 (26 generated)
Description was changed from ========== Reflow of the items in desktop user menu BUG= ========== to ========== Reflow of the items in desktop user menu. Specifically: 1. Moved avatar name up to be on the right of the profile icon 2. Moved username and "Your accounts" link up as well 3. Signin promo remain as a separate row See design doc here: https://docs.google.com/document/d/17yHrKd_EU6WoIAgljnZmNCSJLRlbM3O2Pxr7aPEnq... This CL is dependent on https://codereview.chromium.org/2026043002/ BUG=615893 ==========
Description was changed from ========== Reflow of the items in desktop user menu. Specifically: 1. Moved avatar name up to be on the right of the profile icon 2. Moved username and "Your accounts" link up as well 3. Signin promo remain as a separate row See design doc here: https://docs.google.com/document/d/17yHrKd_EU6WoIAgljnZmNCSJLRlbM3O2Pxr7aPEnq... This CL is dependent on https://codereview.chromium.org/2026043002/ BUG=615893 ========== to ========== Reflow of the items in desktop user menu. Specifically: 1. Moved avatar name up to be on the right of the profile icon 2. Moved username and "Your accounts" link up as well 3. Other dimension and padding changes See design doc here: https://docs.google.com/document/d/17yHrKd_EU6WoIAgljnZmNCSJLRlbM3O2Pxr7aPEnq... See redlines here: https://folio.googleplex.com/chrome-ux-specs-and-sources/Chrome%20Desktop%20S... This CL is dependent on https://codereview.chromium.org/2026043002/ BUG=615893 ==========
Description was changed from ========== Reflow of the items in desktop user menu. Specifically: 1. Moved avatar name up to be on the right of the profile icon 2. Moved username and "Your accounts" link up as well 3. Other dimension and padding changes See design doc here: https://docs.google.com/document/d/17yHrKd_EU6WoIAgljnZmNCSJLRlbM3O2Pxr7aPEnq... See redlines here: https://folio.googleplex.com/chrome-ux-specs-and-sources/Chrome%20Desktop%20S... This CL is dependent on https://codereview.chromium.org/2026043002/ BUG=615893 ========== to ========== Reflow of the profile items in desktop user menu. Specifically: 1. Moved and resized the avatar icon as well as icon badges 1. Moved avatar name up to be on the right of the profile icon 2. Moved username and "Your accounts" link up and below the avatar name 3. Other dimension and padding changes See design doc here ("Reflow the items in the first section"): https://docs.google.com/document/d/17yHrKd_EU6WoIAgljnZmNCSJLRlbM3O2Pxr7aPEnq... See redlines here: https://folio.googleplex.com/chrome-ux-specs-and-sources/Chrome%20Desktop%20S... See screenshots here (marked with "3.*"): https://drive.google.com/corp/drive/folders/0B7Fvv7JszRyGY2NrSEhheHptdG8 BUG=615893 ==========
janeliulwq@google.com changed reviewers: + rogerta@chromium.org
janeliulwq@google.com changed required reviewers: + rogerta@chromium.org
https://codereview.chromium.org/2023093002/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/profiles/profile_chooser_view.cc (right): https://codereview.chromium.org/2023093002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/profiles/profile_chooser_view.cc:86: const int kFixedMenuWidth = 240; I couldn't figure out how to make this helper parameter dependent on IsMaterialDesignUserMenu(). Is there a simple way or can it remain like this?
lgtm Thanks for uploading the screenshots. https://codereview.chromium.org/2023093002/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/profiles/profile_chooser_view.cc (right): https://codereview.chromium.org/2023093002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/profiles/profile_chooser_view.cc:86: const int kFixedMenuWidth = 240; On 2016/06/06 at 16:56:46, Jane wrote: > I couldn't figure out how to make this helper parameter dependent on IsMaterialDesignUserMenu(). Is there a simple way or can it remain like this? You could write a function like: static int GetFixedMenuWidth() { return switches::IsMaterialDesignUserMenu() ? 240 : 250; } and then use GetFixedMenuWidth() wherever kFixedMenuWidth is used below. https://codereview.chromium.org/2023093002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/profiles/profile_chooser_view.cc:1114: CreateCurrentProfileView(item, false); I think lines 1113 and 1114 only need to be indented 4 more than line 1112. https://codereview.chromium.org/2023093002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/profiles/profile_chooser_view.cc:1708: layout->SetInsets(0, views::kMaterialHorizontalMargin, Nit: put views::kMaterialHorizontalMargin on next line.
janeliulwq@google.com changed reviewers: + sky@chromium.org
janeliulwq@google.com changed required reviewers: + sky@chromium.org
https://codereview.chromium.org/2023093002/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/profiles/profile_chooser_view.cc (right): https://codereview.chromium.org/2023093002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/profiles/profile_chooser_view.cc:86: const int kFixedMenuWidth = 240; On 2016/06/07 14:20:48, Roger Tawa wrote: > On 2016/06/06 at 16:56:46, Jane wrote: > > I couldn't figure out how to make this helper parameter dependent on > IsMaterialDesignUserMenu(). Is there a simple way or can it remain like this? > > You could write a function like: > > static int GetFixedMenuWidth() { > return switches::IsMaterialDesignUserMenu() ? 240 : 250; > } > > and then use GetFixedMenuWidth() wherever kFixedMenuWidth is used below. Done. https://codereview.chromium.org/2023093002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/profiles/profile_chooser_view.cc:1114: CreateCurrentProfileView(item, false); On 2016/06/07 14:20:48, Roger Tawa wrote: > I think lines 1113 and 1114 only need to be indented 4 more than line 1112. Done. https://codereview.chromium.org/2023093002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/profiles/profile_chooser_view.cc:1708: layout->SetInsets(0, views::kMaterialHorizontalMargin, On 2016/06/07 14:20:48, Roger Tawa wrote: > Nit: put views::kMaterialHorizontalMargin on next line. Done.
sky@chromium.org changed reviewers: + estade@chromium.org
+estade for ui/views/layout/layout_constants.h https://codereview.chromium.org/2023093002/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/profiles/profile_chooser_view.cc (right): https://codereview.chromium.org/2023093002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/profiles/profile_chooser_view.cc:315: const int kIconImageSide = switches::IsMaterialDesignUserMenu() Don't use kFoo style for things that aren't compile time constants. https://codereview.chromium.org/2023093002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/profiles/profile_chooser_view.cc:416: if (switches::IsMaterialDesignUserMenu()) { nit: no {} https://codereview.chromium.org/2023093002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/profiles/profile_chooser_view.cc:1190: views::GridLayout* layout = CreateSingleColumnLayout(view, GetFixedMenuWidth()); nit: > 80, run git cl format https://codereview.chromium.org/2023093002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/profiles/profile_chooser_view.cc:1510: supervised_icon->SetBounds( Is it expected that you're doing layout in this code too? Normally creation and layout are separate. https://codereview.chromium.org/2023093002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/profiles/profile_chooser_view.cc:1526: const int kTextfieldLeftMargin = Again, don't use kFoo for non compile time constants. https://codereview.chromium.org/2023093002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/profiles/profile_chooser_view.cc:1554: const int kXCoordinate = photo_bounds.width() + kProfileHorizontalSpacing; Same comment for all variables.
Patchset #5 (id:80001) has been deleted
Patchset #5 (id:100001) has been deleted
https://codereview.chromium.org/2023093002/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/profiles/profile_chooser_view.cc (right): https://codereview.chromium.org/2023093002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/profiles/profile_chooser_view.cc:315: const int kIconImageSide = switches::IsMaterialDesignUserMenu() On 2016/06/07 17:50:29, sky wrote: > Don't use kFoo style for things that aren't compile time constants. Done. Seems like a couple of the existing non-compile time constants in this class are named kFoo though, maybe refactoring would be necessary. https://codereview.chromium.org/2023093002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/profiles/profile_chooser_view.cc:416: if (switches::IsMaterialDesignUserMenu()) { On 2016/06/07 17:50:29, sky wrote: > nit: no {} Done. https://codereview.chromium.org/2023093002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/profiles/profile_chooser_view.cc:1190: views::GridLayout* layout = CreateSingleColumnLayout(view, GetFixedMenuWidth()); On 2016/06/07 17:50:29, sky wrote: > nit: > 80, run git cl format Done. https://codereview.chromium.org/2023093002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/profiles/profile_chooser_view.cc:1510: supervised_icon->SetBounds( On 2016/06/07 17:50:29, sky wrote: > Is it expected that you're doing layout in this code too? Normally creation and > layout are separate. This function is entirely based on the existing code of CreateCurrentProfileView. It follows the exact same flow (e.g. this line is basically L1389), and the changes only modify the layout of the profile view. Hence I assumed it is acceptable to do so. Let me know if refactoring is needed though. https://codereview.chromium.org/2023093002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/profiles/profile_chooser_view.cc:1526: const int kTextfieldLeftMargin = On 2016/06/07 17:50:29, sky wrote: > Again, don't use kFoo for non compile time constants. Done. https://codereview.chromium.org/2023093002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/profiles/profile_chooser_view.cc:1554: const int kXCoordinate = photo_bounds.width() + kProfileHorizontalSpacing; On 2016/06/07 17:50:29, sky wrote: > Same comment for all variables. Done.
LGTM - but wait for Evan
there are an alarming number of hardcoded constants in this file https://codereview.chromium.org/2023093002/diff/120001/chrome/browser/ui/view... File chrome/browser/ui/views/profiles/profile_chooser_view.cc (right): https://codereview.chromium.org/2023093002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/profile_chooser_view.cc:107: views::GridLayout* CreateSingleColumnLayout(views::View* view, int width) { I don't understand why this exists instead of using a BoxLayout? https://codereview.chromium.org/2023093002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/profile_chooser_view.cc:593: ? kSmallProfileBadgeSize : kProfileBadgeSize, is this the output of git cl format? https://codereview.chromium.org/2023093002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/profile_chooser_view.cc:1572: } else { combine else with if-else below https://codereview.chromium.org/2023093002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/profile_chooser_view.cc:1601: layout->AddView(profile_container); return view; then get rid of the next else https://codereview.chromium.org/2023093002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/profile_chooser_view.cc:1615: signin_current_profile_link_ = new views::BlueButton( don't use BlueButton for material design. What you want for now is MdTextButton::CreateSecondaryUiBlueButton or just MdTextButton with a strong call to action. https://codereview.chromium.org/2023093002/diff/120001/ui/views/layout/layout... File ui/views/layout/layout_constants.h (right): https://codereview.chromium.org/2023093002/diff/120001/ui/views/layout/layout... ui/views/layout/layout_constants.h:31: // menu items in material design. I have no idea what menu this comment is referring to https://codereview.chromium.org/2023093002/diff/120001/ui/views/layout/layout... ui/views/layout/layout_constants.h:32: const int kMaterialVerticalMargin = 16; this is a rather unhelpful name. Pretend we're in the world where material is default. In that world this becomes just kVerticalMargin.
Description was changed from
==========
Reflow of the profile items in desktop user menu. Specifically:
1. Moved and resized the avatar icon as well as icon badges
1. Moved avatar name up to be on the right of the profile icon
2. Moved username and "Your accounts" link up and below the avatar name
3. Other dimension and padding changes
See design doc here ("Reflow the items in the first section"):
https://docs.google.com/document/d/17yHrKd_EU6WoIAgljnZmNCSJLRlbM3O2Pxr7aPEnq...
See redlines here:
https://folio.googleplex.com/chrome-ux-specs-and-sources/Chrome%20Desktop%20S...
See screenshots here (marked with "3.*"):
https://drive.google.com/corp/drive/folders/0B7Fvv7JszRyGY2NrSEhheHptdG8
BUG=615893
==========
to
==========
Reflow of the profile items in desktop user menu. Specifically:
1. Moved and resized the avatar icon as well as icon badges
1. Moved avatar name up to be on the right of the profile icon
2. Moved username and "Your accounts" link up and below the avatar name
3. Other dimension and padding changes
See design doc here ("Reflow the items in the first section"):
https://docs.google.com/document/d/17yHrKd_EU6WoIAgljnZmNCSJLRlbM3O2Pxr7aPEnq...
See redlines here:
https://folio.googleplex.com/chrome-ux-specs-and-sources/Chrome%20Desktop%20S...
See screenshots here (marked with "3.*"):
https://drive.google.com/corp/drive/folders/0B7Fvv7JszRyGY2NrSEhheHptdG8
Downstream CL: https://codereview.chromium.org/2052473003/
BUG=615893
==========
Thanks! https://codereview.chromium.org/2023093002/diff/120001/chrome/browser/ui/view... File chrome/browser/ui/views/profiles/profile_chooser_view.cc (right): https://codereview.chromium.org/2023093002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/profile_chooser_view.cc:107: views::GridLayout* CreateSingleColumnLayout(views::View* view, int width) { On 2016/06/10 17:39:46, Evan Stade wrote: > I don't understand why this exists instead of using a BoxLayout? I wouldn't know why this got created initially either; I used this class following all the existing usages in this file. Do you think this is worth refactoring? Then maybe we should open another CL just for this. https://codereview.chromium.org/2023093002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/profile_chooser_view.cc:593: ? kSmallProfileBadgeSize : kProfileBadgeSize, On 2016/06/10 17:39:46, Evan Stade wrote: > is this the output of git cl format? Missed it, now it is. https://codereview.chromium.org/2023093002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/profile_chooser_view.cc:1572: } else { On 2016/06/10 17:39:46, Evan Stade wrote: > combine else with if-else below Done. FYI, these massive logic statements were basically copied from the function above, i.e. CreateCurrentProfileView, with some modifications on UI only. Also they are being completely reorganized in the downstream CL, and that's why these logic didn't get a close look. https://codereview.chromium.org/2023093002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/profile_chooser_view.cc:1601: layout->AddView(profile_container); On 2016/06/10 17:39:46, Evan Stade wrote: > return view; then get rid of the next else Done. But same as above. https://codereview.chromium.org/2023093002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/profile_chooser_view.cc:1615: signin_current_profile_link_ = new views::BlueButton( On 2016/06/10 17:39:46, Evan Stade wrote: > don't use BlueButton for material design. What you want for now is > MdTextButton::CreateSecondaryUiBlueButton or just MdTextButton with a strong > call to action. Done. Thanks, didn't know that. https://codereview.chromium.org/2023093002/diff/120001/ui/views/layout/layout... File ui/views/layout/layout_constants.h (right): https://codereview.chromium.org/2023093002/diff/120001/ui/views/layout/layout... ui/views/layout/layout_constants.h:31: // menu items in material design. On 2016/06/10 17:39:46, Evan Stade wrote: > I have no idea what menu this comment is referring to I thought this would be a common constant used by material design menus in general; but I definitely can be more specific for now. Done. https://codereview.chromium.org/2023093002/diff/120001/ui/views/layout/layout... ui/views/layout/layout_constants.h:32: const int kMaterialVerticalMargin = 16; On 2016/06/10 17:39:46, Evan Stade wrote: > this is a rather unhelpful name. Pretend we're in the world where material is > default. In that world this becomes just kVerticalMargin. Same thought but this name does need more details. Thanks for the suggestion and let me know if this name works.
On 2016/06/10 17:39:47, Evan Stade wrote: > there are an alarming number of hardcoded constants in this file > > https://codereview.chromium.org/2023093002/diff/120001/chrome/browser/ui/view... > File chrome/browser/ui/views/profiles/profile_chooser_view.cc (right): > > https://codereview.chromium.org/2023093002/diff/120001/chrome/browser/ui/view... > chrome/browser/ui/views/profiles/profile_chooser_view.cc:107: views::GridLayout* > CreateSingleColumnLayout(views::View* view, int width) { > I don't understand why this exists instead of using a BoxLayout? > > https://codereview.chromium.org/2023093002/diff/120001/chrome/browser/ui/view... > chrome/browser/ui/views/profiles/profile_chooser_view.cc:593: ? > kSmallProfileBadgeSize : kProfileBadgeSize, > is this the output of git cl format? > > https://codereview.chromium.org/2023093002/diff/120001/chrome/browser/ui/view... > chrome/browser/ui/views/profiles/profile_chooser_view.cc:1572: } else { > combine else with if-else below > > https://codereview.chromium.org/2023093002/diff/120001/chrome/browser/ui/view... > chrome/browser/ui/views/profiles/profile_chooser_view.cc:1601: > layout->AddView(profile_container); > return view; then get rid of the next else > > https://codereview.chromium.org/2023093002/diff/120001/chrome/browser/ui/view... > chrome/browser/ui/views/profiles/profile_chooser_view.cc:1615: > signin_current_profile_link_ = new views::BlueButton( > don't use BlueButton for material design. What you want for now is > MdTextButton::CreateSecondaryUiBlueButton or just MdTextButton with a strong > call to action. > > https://codereview.chromium.org/2023093002/diff/120001/ui/views/layout/layout... > File ui/views/layout/layout_constants.h (right): > > https://codereview.chromium.org/2023093002/diff/120001/ui/views/layout/layout... > ui/views/layout/layout_constants.h:31: // menu items in material design. > I have no idea what menu this comment is referring to > > https://codereview.chromium.org/2023093002/diff/120001/ui/views/layout/layout... > ui/views/layout/layout_constants.h:32: const int kMaterialVerticalMargin = 16; > this is a rather unhelpful name. Pretend we're in the world where material is > default. In that world this becomes just kVerticalMargin. I forgot to ask - can you be more specific on which hardcoded constants that you think are alarming/redundant? Part of the reason why more constants got added is because we are developing the material design user menu and adding new layout constants for that, while keeping the old user menu design and its constants for now. I also foresee some constants to come and go during the development process. Let me know if there is anything that I need to take notice of now, thanks!
> I forgot to ask - can you be more specific on which hardcoded constants that you > think are alarming/redundant? Part of the reason why more constants got added is > because we are developing the material design user menu and adding new layout > constants for that, while keeping the old user menu design and its constants for > now. I also foresee some constants to come and go during the development > process. Let me know if there is anything that I need to take notice of now, > thanks! Well, the old code had a ton of hardcoded constants too, so this isn't a new problem for this file. It's still nice to improve upon the old pattern by not adding too many new constants (and then when the old code is deleted, the world's a better place). Most layout constants in Chrome are cooked in, e.g. defined as part of an image, dependent on the base font size, etc., or shared, e.g. the default margins for bubbles/dialogs, or they're in ui/views/layout/layout_constants.h or somewhere shared. The point of reusing these constants is to ensure consistency so when there are a whole lot of unshared constants I'm worried unintended discrepancies will crop up. https://codereview.chromium.org/2023093002/diff/120001/chrome/browser/ui/view... File chrome/browser/ui/views/profiles/profile_chooser_view.cc (right): https://codereview.chromium.org/2023093002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/profile_chooser_view.cc:107: views::GridLayout* CreateSingleColumnLayout(views::View* view, int width) { On 2016/06/10 18:49:16, Jane wrote: > On 2016/06/10 17:39:46, Evan Stade wrote: > > I don't understand why this exists instead of using a BoxLayout? > > I wouldn't know why this got created initially either; I used this class > following all the existing usages in this file. Do you think this is worth > refactoring? Then maybe we should open another CL just for this. I'd at least not add another usage of it. Create a good pattern to follow, unless there's some non-obvious reason this needs to exist (in which case document it). https://codereview.chromium.org/2023093002/diff/140001/ui/views/layout/layout... File ui/views/layout/layout_constants.h (right): https://codereview.chromium.org/2023093002/diff/140001/ui/views/layout/layout... ui/views/layout/layout_constants.h:32: const int kMaterialMenuVerticalEdgeMargin = 16; I'm still confused about where this would be used besides in the user menu. Most menu related constants live in ui/views/controls/menu/menu_config.h I'd suggest moving this to the profile chooser view except I think the better idea is to use a shared constant that already exists, once again to ensure consistency.
On 2016/06/10 19:53:35, Evan Stade wrote: > > I forgot to ask - can you be more specific on which hardcoded constants that > you > > think are alarming/redundant? Part of the reason why more constants got added > is > > because we are developing the material design user menu and adding new layout > > constants for that, while keeping the old user menu design and its constants > for > > now. I also foresee some constants to come and go during the development > > process. Let me know if there is anything that I need to take notice of now, > > thanks! > > Well, the old code had a ton of hardcoded constants too, so this isn't a new > problem for this file. It's still nice to improve upon the old pattern by not > adding too many new constants (and then when the old code is deleted, the > world's a better place). Most layout constants in Chrome are cooked in, e.g. > defined as part of an image, dependent on the base font size, etc., or shared, > e.g. the default margins for bubbles/dialogs, or they're in > ui/views/layout/layout_constants.h or somewhere shared. The point of reusing > these constants is to ensure consistency so when there are a whole lot of > unshared constants I'm worried unintended discrepancies will crop up. > > https://codereview.chromium.org/2023093002/diff/120001/chrome/browser/ui/view... > File chrome/browser/ui/views/profiles/profile_chooser_view.cc (right): > > https://codereview.chromium.org/2023093002/diff/120001/chrome/browser/ui/view... > chrome/browser/ui/views/profiles/profile_chooser_view.cc:107: views::GridLayout* > CreateSingleColumnLayout(views::View* view, int width) { > On 2016/06/10 18:49:16, Jane wrote: > > On 2016/06/10 17:39:46, Evan Stade wrote: > > > I don't understand why this exists instead of using a BoxLayout? > > > > I wouldn't know why this got created initially either; I used this class > > following all the existing usages in this file. Do you think this is worth > > refactoring? Then maybe we should open another CL just for this. > > I'd at least not add another usage of it. Create a good pattern to follow, > unless there's some non-obvious reason this needs to exist (in which case > document it). > > https://codereview.chromium.org/2023093002/diff/140001/ui/views/layout/layout... > File ui/views/layout/layout_constants.h (right): > > https://codereview.chromium.org/2023093002/diff/140001/ui/views/layout/layout... > ui/views/layout/layout_constants.h:32: const int kMaterialMenuVerticalEdgeMargin > = 16; > I'm still confused about where this would be used besides in the user menu. Most > menu related constants live in ui/views/controls/menu/menu_config.h > > I'd suggest moving this to the profile chooser view except I think the better > idea is to use a shared constant that already exists, once again to ensure > consistency. Thanks! Your explanation on keeping constants organized makes sense, and I did try to keep the constants consistent while modifying the code. I just took another look at the constants that I added, and I think 1. most of them are specific to the user menu (and are used across different parts of the user menu), which is why they were not shared; 2. almost all of them are being added but not replacing old constants just because we are keeping the old user menu for now. I believe the number of constants will be restored to prior level once the development process for the new user menu is done and when the old user menu is removed. Let me know if any specific constant does not make sense though!
https://codereview.chromium.org/2023093002/diff/120001/chrome/browser/ui/view... File chrome/browser/ui/views/profiles/profile_chooser_view.cc (right): https://codereview.chromium.org/2023093002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/profile_chooser_view.cc:107: views::GridLayout* CreateSingleColumnLayout(views::View* view, int width) { On 2016/06/10 19:53:34, Evan Stade wrote: > On 2016/06/10 18:49:16, Jane wrote: > > On 2016/06/10 17:39:46, Evan Stade wrote: > > > I don't understand why this exists instead of using a BoxLayout? > > > > I wouldn't know why this got created initially either; I used this class > > following all the existing usages in this file. Do you think this is worth > > refactoring? Then maybe we should open another CL just for this. > > I'd at least not add another usage of it. Create a good pattern to follow, > unless there's some non-obvious reason this needs to exist (in which case > document it). That sounds like a reasonable suggestion. However, I think it's a pretty big decision and change to make for right now/this CL. I just tried switching to box layout, and there seem to be many different functions for adding insets, views/rows, paddings, etc. for box layout versus grid layout, making this potential change to be very involved. Do you think we can defer this change to later? https://codereview.chromium.org/2023093002/diff/140001/ui/views/layout/layout... File ui/views/layout/layout_constants.h (right): https://codereview.chromium.org/2023093002/diff/140001/ui/views/layout/layout... ui/views/layout/layout_constants.h:32: const int kMaterialMenuVerticalEdgeMargin = 16; On 2016/06/10 19:53:34, Evan Stade wrote: > I'm still confused about where this would be used besides in the user menu. Most > menu related constants live in ui/views/controls/menu/menu_config.h > > I'd suggest moving this to the profile chooser view except I think the better > idea is to use a shared constant that already exists, once again to ensure > consistency. Sorry, let me be more clear: I don't know exactly what menus will use this margin value, but I was making an assumption that this parameter would be used more commonly in the future when different Chrome components are converted to standardized material design styles. But yes this was just my assumption. I added in this parameter because I couldn't find anything existing to use. However, it does make sense to just move it to profile chooser view for now. Done that, let me know if this works.
> Your explanation on keeping constants organized makes sense, and I did try to > keep the constants consistent while modifying the code. I just took another look > at the constants that I added, and I think 1. most of them are specific to the > user menu (and are used across different parts of the user menu), which is why > they were not shared; 2. almost all of them are being added but not replacing > old constants just because we are keeping the old user menu for now. I believe > the number of constants will be restored to prior level once the development > process for the new user menu is done and when the old user menu is removed. I understand the reason so many new ones are being added. I don't believe the prior number of constants was sane, so "restoring to prior level" doesn't seem all that great. It's likely that the number of constants grew and grew over time, and no one bothered to reorganize/clean up/share where possible. A large UI refresh such as this one is really the best time to improve upon the status quo. Here's a specific example I don't like: button_->SetBorder(views::Border::CreateEmptyBorder(2, kTextfieldLabelHorizontalSpacing, 2, 0)); Really seems like this should be something along the lines of button_->SetBorder(views::Border::CreateEmptyBorder(Textfield::kTextPadding, views::kRelatedControlSmallVerticalSpacing, Textfield::kTextPadding, 0)); https://codereview.chromium.org/2023093002/diff/120001/chrome/browser/ui/view... File chrome/browser/ui/views/profiles/profile_chooser_view.cc (right): https://codereview.chromium.org/2023093002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/profile_chooser_view.cc:107: views::GridLayout* CreateSingleColumnLayout(views::View* view, int width) { On 2016/06/10 21:09:42, Jane wrote: > On 2016/06/10 19:53:34, Evan Stade wrote: > > On 2016/06/10 18:49:16, Jane wrote: > > > On 2016/06/10 17:39:46, Evan Stade wrote: > > > > I don't understand why this exists instead of using a BoxLayout? > > > > > > I wouldn't know why this got created initially either; I used this class > > > following all the existing usages in this file. Do you think this is worth > > > refactoring? Then maybe we should open another CL just for this. > > > > I'd at least not add another usage of it. Create a good pattern to follow, > > unless there's some non-obvious reason this needs to exist (in which case > > document it). > > That sounds like a reasonable suggestion. However, I think it's a pretty big > decision and change to make for right now/this CL. I just tried switching to box > layout, and there seem to be many different functions for adding insets, > views/rows, paddings, etc. for box layout versus grid layout, making this > potential change to be very involved. Do you think we can defer this change to > later? Are you talking about updates to existing code or the new code you're adding? If you're talking about the new code, I'd rather not defer. Create a good pattern to follow, use it, mark the old way deprecated. You don't have to change pre-existing code.
On 2016/06/13 16:36:02, Evan Stade wrote: > > Your explanation on keeping constants organized makes sense, and I did try to > > keep the constants consistent while modifying the code. I just took another > look > > at the constants that I added, and I think 1. most of them are specific to the > > user menu (and are used across different parts of the user menu), which is why > > they were not shared; 2. almost all of them are being added but not replacing > > old constants just because we are keeping the old user menu for now. I believe > > the number of constants will be restored to prior level once the development > > process for the new user menu is done and when the old user menu is removed. > > I understand the reason so many new ones are being added. I don't believe the > prior number of constants was sane, so "restoring to prior level" doesn't seem > all that great. It's likely that the number of constants grew and grew over > time, and no one bothered to reorganize/clean up/share where possible. A large > UI refresh such as this one is really the best time to improve upon the status > quo. Here's a specific example I don't like: > > button_->SetBorder(views::Border::CreateEmptyBorder(2, > kTextfieldLabelHorizontalSpacing, 2, 0)); > > Really seems like this should be something along the lines of > > button_->SetBorder(views::Border::CreateEmptyBorder(Textfield::kTextPadding, > views::kRelatedControlSmallVerticalSpacing, Textfield::kTextPadding, 0)); > > https://codereview.chromium.org/2023093002/diff/120001/chrome/browser/ui/view... > File chrome/browser/ui/views/profiles/profile_chooser_view.cc (right): > > https://codereview.chromium.org/2023093002/diff/120001/chrome/browser/ui/view... > chrome/browser/ui/views/profiles/profile_chooser_view.cc:107: views::GridLayout* > CreateSingleColumnLayout(views::View* view, int width) { > On 2016/06/10 21:09:42, Jane wrote: > > On 2016/06/10 19:53:34, Evan Stade wrote: > > > On 2016/06/10 18:49:16, Jane wrote: > > > > On 2016/06/10 17:39:46, Evan Stade wrote: > > > > > I don't understand why this exists instead of using a BoxLayout? > > > > > > > > I wouldn't know why this got created initially either; I used this class > > > > following all the existing usages in this file. Do you think this is worth > > > > refactoring? Then maybe we should open another CL just for this. > > > > > > I'd at least not add another usage of it. Create a good pattern to follow, > > > unless there's some non-obvious reason this needs to exist (in which case > > > document it). > > > > That sounds like a reasonable suggestion. However, I think it's a pretty big > > decision and change to make for right now/this CL. I just tried switching to > box > > layout, and there seem to be many different functions for adding insets, > > views/rows, paddings, etc. for box layout versus grid layout, making this > > potential change to be very involved. Do you think we can defer this change to > > later? > > Are you talking about updates to existing code or the new code you're adding? If > you're talking about the new code, I'd rather not defer. Create a good pattern > to follow, use it, mark the old way deprecated. You don't have to change > pre-existing code. Thanks for giving me a specific example, that's exactly where my confusion lies - what the specific improvements I could make for this CL. For this example, the reason why I created a new constant is because I thought the name of the existing constant, "kRelatedControlSmallVerticalSpacing", reflects a very different concept compared to the spacing that I need. My purpose is to account for the border width of the textfield so that when the textfield is triggered, the text inside the textfield lies in the same position as the original label that covers it. Also, the spacing I need is a horizontal spacing. Therefore, I thought it would be cleaner and more maintainable if I keep it as a separate constant. If you think it's still better to use the existing kRelatedControlSmallVerticalSpacing, I can definitely use that instead. Also FYI this constant is removed in the downstream CL when the textfield is removed altogether. Other than reasons like this one, a few other constants were added simply because no existing constants have the value, yet I have to follow the specifics of material design. It would be really helpful if you could point out other examples of the constants in this CL that you think should be replaced/removed, and I will learn my way from there. Thanks again!
> I understand the reason so many new ones are being added. I don't believe the > prior number of constants was sane, so "restoring to prior level" doesn't seem > all that great. It's likely that the number of constants grew and grew over > time, and no one bothered to reorganize/clean up/share where possible. A large > UI refresh such as this one is really the best time to improve upon the status > quo. Here's a specific example I don't like: > > button_->SetBorder(views::Border::CreateEmptyBorder(2, > kTextfieldLabelHorizontalSpacing, 2, 0)); > > Really seems like this should be something along the lines of > > button_->SetBorder(views::Border::CreateEmptyBorder(Textfield::kTextPadding, > views::kRelatedControlSmallVerticalSpacing, Textfield::kTextPadding, 0)); Thanks for giving me a specific example, that's exactly where my confusion lies - what the specific improvements I could make for this CL are. For this example, the reason why I created a new constant is because I thought the name of the existing constant, "kRelatedControlSmallVerticalSpacing", reflects a very different concept compared to the spacing that I need. My purpose is to account for the border width of the textfield so that when the textfield is triggered, the text inside the textfield lies in the same position as the original label that covers it. Also, the spacing I need is a horizontal spacing. Therefore, I thought it would be cleaner and more maintainable if I keep it as a separate constant. If you think it's still better to use the existing kRelatedControlSmallVerticalSpacing, I can definitely use that instead. Also FYI this constant is removed in the downstream CL when the textfield is removed altogether. Other than reasons like this one, a few other constants were added simply because no existing constants have the value, yet I have to follow the specifics of material design. It would be really helpful if you could point out other examples of the constants in this CL that you think should be replaced/removed, and I will learn my way from there. Thanks again! https://codereview.chromium.org/2023093002/diff/120001/chrome/browser/ui/view... File chrome/browser/ui/views/profiles/profile_chooser_view.cc (right): https://codereview.chromium.org/2023093002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/profile_chooser_view.cc:107: views::GridLayout* CreateSingleColumnLayout(views::View* view, int width) { On 2016/06/13 16:36:02, Evan Stade wrote: > On 2016/06/10 21:09:42, Jane wrote: > > On 2016/06/10 19:53:34, Evan Stade wrote: > > > On 2016/06/10 18:49:16, Jane wrote: > > > > On 2016/06/10 17:39:46, Evan Stade wrote: > > > > > I don't understand why this exists instead of using a BoxLayout? > > > > > > > > I wouldn't know why this got created initially either; I used this class > > > > following all the existing usages in this file. Do you think this is worth > > > > refactoring? Then maybe we should open another CL just for this. > > > > > > I'd at least not add another usage of it. Create a good pattern to follow, > > > unless there's some non-obvious reason this needs to exist (in which case > > > document it). > > > > That sounds like a reasonable suggestion. However, I think it's a pretty big > > decision and change to make for right now/this CL. I just tried switching to > box > > layout, and there seem to be many different functions for adding insets, > > views/rows, paddings, etc. for box layout versus grid layout, making this > > potential change to be very involved. Do you think we can defer this change to > > later? > > Are you talking about updates to existing code or the new code you're adding? If > you're talking about the new code, I'd rather not defer. Create a good pattern > to follow, use it, mark the old way deprecated. You don't have to change > pre-existing code. I replaced the GridLayout with BoxLayout in the latest patch #8. How does it look now? Although I still couldn't find a way to add a padding row in BoxLayout, something like GridLayout's StartRowWithPadding (see L1642 of patch #7). I need to add an extra padding below the blue button when it appears. Do you know of a way or an example? Thanks!
https://codereview.chromium.org/2023093002/diff/180001/chrome/browser/ui/view... File chrome/browser/ui/views/profiles/profile_chooser_view.cc (right): https://codereview.chromium.org/2023093002/diff/180001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/profile_chooser_view.cc:95: const int kProfileHorizontalSpacing = 16; pretty sure you should use kMaterialMenuHorizontalEdgeMargin in place of this --- looking at the redlines, I don't think it's a coincidence that they're both 16. https://codereview.chromium.org/2023093002/diff/180001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/profile_chooser_view.cc:424: label_ = new views::Label(text); since you asked me to find other constants I don't like, I am looking more closely at this file. Why is label_ a member var at all? https://codereview.chromium.org/2023093002/diff/180001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/profile_chooser_view.cc:425: label_->SetBorder(views::Border::CreateEmptyBorder(2, 0, 2, 0)); are these 2s distinct from the 2s below? https://codereview.chromium.org/2023093002/diff/180001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/profile_chooser_view.cc:453: button_->SetBorder(views::Border::CreateEmptyBorder( OK, I may have misunderstood the purpose of kTextfieldLabelHorizontalSpacing. Why can't you call Textfield::GetInsets() for these values? https://codereview.chromium.org/2023093002/diff/180001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/profile_chooser_view.cc:628: int offset = ((switches::IsMaterialDesignUserMenu() ? kSmallProfileBadgeSize this ternary appears 3 times https://codereview.chromium.org/2023093002/diff/180001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/profile_chooser_view.cc:1504: int column_width = are you sure the box layout can't grow wider than this? https://codereview.chromium.org/2023093002/diff/180001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/profile_chooser_view.cc:1517: kMediumImageSide, kMediumImageSide)); Yyou're doing layout at construction time, which is always going to be more complicated than using a layout manager or overriding Layout(). I don't think you even need to specify a size here because LabelButton will naturally use the image for sizing. give profile_container a horizontal box layout with inside border vertical spacing of 8 and between child spacing of kMaterialMenuHorizontalEdgeMargin https://codereview.chromium.org/2023093002/diff/180001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/profile_chooser_view.cc:1561: view->AddChildView(profile_container); can't you do this unconditionally? https://codereview.chromium.org/2023093002/diff/180001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/profile_chooser_view.cc:1571: const int x_coordinate = photo_bounds.width() + kProfileHorizontalSpacing; If profile_container had a layout manager this would be simpler. https://codereview.chromium.org/2023093002/diff/180001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/profile_chooser_view.cc:1598: auth_error_email_button_->set_request_focus_on_press(true); why do you want this non-default behavior https://codereview.chromium.org/2023093002/diff/180001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/profile_chooser_view.cc:1628: signin_current_profile_link_ = this is not really a link... https://codereview.chromium.org/2023093002/diff/180001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/profile_chooser_view.cc:1633: signin_current_profile_link_->SetMinSize(gfx::Size(0, 36)); here's a constant I don't understand the need for https://codereview.chromium.org/2023093002/diff/180001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/profile_chooser_view.cc:1637: view->AddChildView(padding_view); if you need padding under the md text button, perhaps you can add an empty border to |view|, or you can add an empty SizedContainer view
Patchset #9 (id:200001) has been deleted
https://codereview.chromium.org/2023093002/diff/180001/chrome/browser/ui/view... File chrome/browser/ui/views/profiles/profile_chooser_view.cc (right): https://codereview.chromium.org/2023093002/diff/180001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/profile_chooser_view.cc:95: const int kProfileHorizontalSpacing = 16; On 2016/06/13 23:14:41, Evan Stade wrote: > pretty sure you should use kMaterialMenuHorizontalEdgeMargin in place of this > --- looking at the redlines, I don't think it's a coincidence that they're both > 16. Done. You have a point; I didn't think this way before. https://codereview.chromium.org/2023093002/diff/180001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/profile_chooser_view.cc:424: label_ = new views::Label(text); On 2016/06/13 23:14:40, Evan Stade wrote: > since you asked me to find other constants I don't like, I am looking more > closely at this file. Why is label_ a member var at all? Done. I appreciate the close attention that you are paying on this! Now that you pointed it out, I agree with you that label_ has no apparent reason to be a member variable, but I wouldn't know why it was written this way - I simply didn't think it was necessary for me to go through other parts of this file that I'm not really touching and clean up existing code that other reviewers have approved. Also as a matter of fact, this class in particular will be completely removed once the new menu gets rolled out (because profile name will not be editable). But since this is a small change, I went in and made it a local variable. https://codereview.chromium.org/2023093002/diff/180001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/profile_chooser_view.cc:425: label_->SetBorder(views::Border::CreateEmptyBorder(2, 0, 2, 0)); On 2016/06/13 23:14:41, Evan Stade wrote: > are these 2s distinct from the 2s below? I assume so, but then again, I'm not touching any of the 2s myself. Are you saying it's necessary for me to make a constant for this? https://codereview.chromium.org/2023093002/diff/180001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/profile_chooser_view.cc:453: button_->SetBorder(views::Border::CreateEmptyBorder( On 2016/06/13 23:14:40, Evan Stade wrote: > OK, I may have misunderstood the purpose of kTextfieldLabelHorizontalSpacing. > Why can't you call Textfield::GetInsets() for these values? Done. Thanks, didn't know this before. (Although this variable will be removed in the downstream CL too... I realize that it would have probably been a better idea to consolidate them.) https://codereview.chromium.org/2023093002/diff/180001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/profile_chooser_view.cc:628: int offset = ((switches::IsMaterialDesignUserMenu() ? kSmallProfileBadgeSize On 2016/06/13 23:14:40, Evan Stade wrote: > this ternary appears 3 times I did realize this and I wanted to make it a const right at where kSmallProfileBadgeSize and kProfileBadgeSize are defined, but I get segfaults by calling the switches function there, so I thought maybe having them remain this way would be easier. However, if you think it's necessary, I can make a static function for this. https://codereview.chromium.org/2023093002/diff/180001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/profile_chooser_view.cc:1504: int column_width = On 2016/06/13 23:14:41, Evan Stade wrote: > are you sure the box layout can't grow wider than this? I'm only using column_width as the width for profile_container but not the box layout if that's what you are asking. https://codereview.chromium.org/2023093002/diff/180001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/profile_chooser_view.cc:1517: kMediumImageSide, kMediumImageSide)); On 2016/06/13 23:14:40, Evan Stade wrote: > Yyou're doing layout at construction time, which is always going to be more > complicated than using a layout manager or overriding Layout(). I don't think > you even need to specify a size here because LabelButton will naturally use the > image for sizing. > > give profile_container a horizontal box layout with inside border vertical > spacing of 8 and between child spacing of kMaterialMenuHorizontalEdgeMargin This is a very good suggestion. I thought about using a box layout and tried doing it, but I think this is a more complicated case: the profile photo might have a badge attached to it, so another layout/parent view is needed to hold them together; the avatar name and email address needs to in the same column too; and EditableProfileName needs to be closer to the profile photo than kMaterialMenuHorizontalEdgeMargin because the textfield insets need to be accounted. If anything, maybe I should create two columns for this. However, I really think it would be easier to make it happen in the downstream CL because some of these components will be simplified or removed. Let me know if you think this is ok for now and what I should do as next step. Also, I assume you are talking about EditableProfilePhoto when you said I don't need to specify a size for LabelButton. You make a point, but then again, I don't plan to investigate on this pre-existing code that will be removed soon, unless you deem it necessary. https://codereview.chromium.org/2023093002/diff/180001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/profile_chooser_view.cc:1561: view->AddChildView(profile_container); On 2016/06/13 23:14:40, Evan Stade wrote: > can't you do this unconditionally? No, because more child views will be added to profile_container if the profile is signed in. I can't do this all at the end either, because the guest view has to be returned right below. https://codereview.chromium.org/2023093002/diff/180001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/profile_chooser_view.cc:1571: const int x_coordinate = photo_bounds.width() + kProfileHorizontalSpacing; On 2016/06/13 23:14:40, Evan Stade wrote: > If profile_container had a layout manager this would be simpler. Same as above. https://codereview.chromium.org/2023093002/diff/180001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/profile_chooser_view.cc:1598: auth_error_email_button_->set_request_focus_on_press(true); On 2016/06/13 23:14:40, Evan Stade wrote: > why do you want this non-default behavior As mentioned before, this code is basically copied from the function above CreateCurrentProfileView (e.g. this line corresponds to L1456), and this code will be heavily modified as the user menu revamp project proceeds. I didn't take a close look at why this was done and I assumed it was reasonable as it is from existing code. As a matter of fact, the lines under this particular condition is getting removed in the downstream CL. I don't plan to investigate on this unless you think it's necessary. https://codereview.chromium.org/2023093002/diff/180001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/profile_chooser_view.cc:1628: signin_current_profile_link_ = On 2016/06/13 23:14:40, Evan Stade wrote: > this is not really a link... Done. Same as above, but since this code will live, I modified its name everywhere. https://codereview.chromium.org/2023093002/diff/180001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/profile_chooser_view.cc:1633: signin_current_profile_link_->SetMinSize(gfx::Size(0, 36)); On 2016/06/13 23:14:41, Evan Stade wrote: > here's a constant I don't understand the need for This was supposed to define the height of the button. Do you think this line is unnecessary? https://codereview.chromium.org/2023093002/diff/180001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/profile_chooser_view.cc:1637: view->AddChildView(padding_view); On 2016/06/13 23:14:40, Evan Stade wrote: > if you need padding under the md text button, perhaps you can add an empty > border to |view|, or you can add an empty SizedContainer view I tried adding: view->SetBorder(views::Border::CreateEmptyBorder( 0, 0, views::kRelatedControlVerticalSpacing, 0)); ...but no extra padding was shown. Why? So now I'm using a SizedContainer of height 1. Would you say it's ok to have an extra height of 1?
https://codereview.chromium.org/2023093002/diff/180001/chrome/browser/ui/view... File chrome/browser/ui/views/profiles/profile_chooser_view.cc (right): https://codereview.chromium.org/2023093002/diff/180001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/profile_chooser_view.cc:425: label_->SetBorder(views::Border::CreateEmptyBorder(2, 0, 2, 0)); On 2016/06/14 14:47:21, Jane wrote: > On 2016/06/13 23:14:41, Evan Stade wrote: > > are these 2s distinct from the 2s below? > > I assume so, but then again, I'm not touching any of the 2s myself. Are you > saying it's necessary for me to make a constant for this? I'm a bit perplexed by your stance on pre-existing code. When you add new code that is related to pre-existing code, you often make changes or refactors to share logic or constants, and this requires modifying said pre-existing code. So yes, it is necessary to evaluate and adjust the code that was in this file prior to this CL. Unless I misunderstand you, you're also implying that if one reviewer approves, all reviewers should approve. Sad to say, sometimes a reviewer misses something. If someone could review code perfectly they could write it perfectly as well and we'd probably not need a review process in the first place. https://codereview.chromium.org/2023093002/diff/180001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/profile_chooser_view.cc:628: int offset = ((switches::IsMaterialDesignUserMenu() ? kSmallProfileBadgeSize On 2016/06/14 14:47:21, Jane wrote: > On 2016/06/13 23:14:40, Evan Stade wrote: > > this ternary appears 3 times > > I did realize this and I wanted to make it a const right at where > kSmallProfileBadgeSize and kProfileBadgeSize are defined, but I get segfaults by > calling the switches function there, so I thought maybe having them remain this > way would be easier. However, if you think it's necessary, I can make a static > function for this. I think that would be an improvement. The badge size constants need not be defined separately from that function. https://codereview.chromium.org/2023093002/diff/180001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/profile_chooser_view.cc:1504: int column_width = On 2016/06/14 14:47:21, Jane wrote: > On 2016/06/13 23:14:41, Evan Stade wrote: > > are you sure the box layout can't grow wider than this? > > I'm only using column_width as the width for profile_container but not the box > layout if that's what you are asking. what sets the width of the box layout then? https://codereview.chromium.org/2023093002/diff/180001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/profile_chooser_view.cc:1517: kMediumImageSide, kMediumImageSide)); On 2016/06/14 14:47:21, Jane wrote: > On 2016/06/13 23:14:40, Evan Stade wrote: > > Yyou're doing layout at construction time, which is always going to be more > > complicated than using a layout manager or overriding Layout(). I don't think > > you even need to specify a size here because LabelButton will naturally use > the > > image for sizing. > > > > give profile_container a horizontal box layout with inside border vertical > > spacing of 8 and between child spacing of kMaterialMenuHorizontalEdgeMargin > > This is a very good suggestion. I thought about using a box layout and tried > doing it, but I think this is a more complicated case: the profile photo might > have a badge attached to it, so another layout/parent view is needed to hold > them together; the avatar name and email address needs to in the same column > too; and EditableProfileName needs to be closer to the profile photo than > kMaterialMenuHorizontalEdgeMargin because the textfield insets need to be > accounted. If anything, maybe I should create two columns for this. However, I > really think it would be easier to make it happen in the downstream CL because > some of these components will be simplified or removed. Let me know if you think > this is ok for now and what I should do as next step. When I said "or overriding Layout()", I guess I should have specified that this is the typical way of handling very custom layout. That said, I don't think it's too difficult to add a couple container views or conditionally adjust insets or borders. > Also, I assume you are talking about EditableProfilePhoto when you said I don't > need to specify a size for LabelButton. You make a point, but then again, I > don't plan to investigate on this pre-existing code that will be removed soon, > unless you deem it necessary. why are you adding code that uses EditableProfilePhoto if you're about to remove EditableProfilePhoto? https://codereview.chromium.org/2023093002/diff/180001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/profile_chooser_view.cc:1561: view->AddChildView(profile_container); On 2016/06/14 14:47:21, Jane wrote: > On 2016/06/13 23:14:40, Evan Stade wrote: > > can't you do this unconditionally? > > No, because more child views will be added to profile_container if the profile > is signed in. Not sure I understand. Why can't you add child views to profile_container after adding profile_container to view? https://codereview.chromium.org/2023093002/diff/180001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/profile_chooser_view.cc:1598: auth_error_email_button_->set_request_focus_on_press(true); On 2016/06/14 14:47:21, Jane wrote: > On 2016/06/13 23:14:40, Evan Stade wrote: > > why do you want this non-default behavior > > As mentioned before, this code is basically copied from the function above > CreateCurrentProfileView (e.g. this line corresponds to L1456), and this code > will be heavily modified as the user menu revamp project proceeds. I didn't take > a close look at why this was done and I assumed it was reasonable as it is from > existing code. As a matter of fact, the lines under this particular condition is > getting removed in the downstream CL. I don't plan to investigate on this unless > you think it's necessary. If there are large blocks of code you're about to delete, you should clearly mark them as such with disclaimers like TODO(yourname): Delete this entire block of code: crbug.com/mybug Generally speaking I'm not a huge fan of copying code without analyzing whether it still makes sense. https://codereview.chromium.org/2023093002/diff/180001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/profile_chooser_view.cc:1633: signin_current_profile_link_->SetMinSize(gfx::Size(0, 36)); On 2016/06/14 14:47:21, Jane wrote: > On 2016/06/13 23:14:41, Evan Stade wrote: > > here's a constant I don't understand the need for > > This was supposed to define the height of the button. Do you think this line is > unnecessary? yes, i think the height should be the same as all the other buttons in chrome https://codereview.chromium.org/2023093002/diff/180001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/profile_chooser_view.cc:1637: view->AddChildView(padding_view); On 2016/06/14 14:47:21, Jane wrote: > On 2016/06/13 23:14:40, Evan Stade wrote: > > if you need padding under the md text button, perhaps you can add an empty > > border to |view|, or you can add an empty SizedContainer view > > I tried adding: > > view->SetBorder(views::Border::CreateEmptyBorder( > 0, 0, views::kRelatedControlVerticalSpacing, 0)); > > ...but no extra padding was shown. Why? I dunno, is there something else setting a border on view? I suggest investigating because "why" is a good question and might have an interesting answer. > So now I'm using a SizedContainer of height 1. Would you say it's ok to have an > extra height of 1?
Patchset #10 (id:240001) has been deleted
Thanks again for the elaborate comments! I should point out that the only way I have been able to learn about the code base and work with it so far has been through reading existing code mostly from this file, which is why you would see certain patterns in my code that follow the existing patterns, which are not necessarily correct or the best practices. Your comments are definitely a good way for me to learn about rules and practices that don't exist in this file already. Please bear with me while I work this through! https://codereview.chromium.org/2023093002/diff/180001/chrome/browser/ui/view... File chrome/browser/ui/views/profiles/profile_chooser_view.cc (right): https://codereview.chromium.org/2023093002/diff/180001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/profile_chooser_view.cc:425: label_->SetBorder(views::Border::CreateEmptyBorder(2, 0, 2, 0)); On 2016/06/14 15:16:26, Evan Stade wrote: > On 2016/06/14 14:47:21, Jane wrote: > > On 2016/06/13 23:14:41, Evan Stade wrote: > > > are these 2s distinct from the 2s below? > > > > I assume so, but then again, I'm not touching any of the 2s myself. Are you > > saying it's necessary for me to make a constant for this? > > I'm a bit perplexed by your stance on pre-existing code. When you add new code > that is related to pre-existing code, you often make changes or refactors to > share logic or constants, and this requires modifying said pre-existing code. So > yes, it is necessary to evaluate and adjust the code that was in this file prior > to this CL. > > Unless I misunderstand you, you're also implying that if one reviewer approves, > all reviewers should approve. Sad to say, sometimes a reviewer misses something. > If someone could review code perfectly they could write it perfectly as well and > we'd probably not need a review process in the first place. Done. Makes sense, I set 2 to be border_width. https://codereview.chromium.org/2023093002/diff/180001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/profile_chooser_view.cc:628: int offset = ((switches::IsMaterialDesignUserMenu() ? kSmallProfileBadgeSize On 2016/06/14 15:16:26, Evan Stade wrote: > On 2016/06/14 14:47:21, Jane wrote: > > On 2016/06/13 23:14:40, Evan Stade wrote: > > > this ternary appears 3 times > > > > I did realize this and I wanted to make it a const right at where > > kSmallProfileBadgeSize and kProfileBadgeSize are defined, but I get segfaults > by > > calling the switches function there, so I thought maybe having them remain > this > > way would be easier. However, if you think it's necessary, I can make a static > > function for this. > > I think that would be an improvement. The badge size constants need not be > defined separately from that function. Done. https://codereview.chromium.org/2023093002/diff/180001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/profile_chooser_view.cc:1504: int column_width = On 2016/06/14 15:16:26, Evan Stade wrote: > On 2016/06/14 14:47:21, Jane wrote: > > On 2016/06/13 23:14:41, Evan Stade wrote: > > > are you sure the box layout can't grow wider than this? > > > > I'm only using column_width as the width for profile_container but not the box > > layout if that's what you are asking. > > what sets the width of the box layout then? I thought the width of the box layout would be determined by the width of its children and its inside_border_horizontal_spacing? https://codereview.chromium.org/2023093002/diff/180001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/profile_chooser_view.cc:1517: kMediumImageSide, kMediumImageSide)); On 2016/06/14 15:16:26, Evan Stade wrote: > On 2016/06/14 14:47:21, Jane wrote: > > On 2016/06/13 23:14:40, Evan Stade wrote: > > > Yyou're doing layout at construction time, which is always going to be more > > > complicated than using a layout manager or overriding Layout(). I don't > think > > > you even need to specify a size here because LabelButton will naturally use > > the > > > image for sizing. > > > > > > give profile_container a horizontal box layout with inside border vertical > > > spacing of 8 and between child spacing of kMaterialMenuHorizontalEdgeMargin > > > > This is a very good suggestion. I thought about using a box layout and tried > > doing it, but I think this is a more complicated case: the profile photo might > > have a badge attached to it, so another layout/parent view is needed to hold > > them together; the avatar name and email address needs to in the same column > > too; and EditableProfileName needs to be closer to the profile photo than > > kMaterialMenuHorizontalEdgeMargin because the textfield insets need to be > > accounted. If anything, maybe I should create two columns for this. However, I > > really think it would be easier to make it happen in the downstream CL because > > some of these components will be simplified or removed. Let me know if you > think > > this is ok for now and what I should do as next step. > > When I said "or overriding Layout()", I guess I should have specified that this > is the typical way of handling very custom layout. That said, I don't think it's > too difficult to add a couple container views or conditionally adjust insets or > borders. > Done. I added a couple of containers to the view, does it look better now? > > Also, I assume you are talking about EditableProfilePhoto when you said I > don't > > need to specify a size for LabelButton. You make a point, but then again, I > > don't plan to investigate on this pre-existing code that will be removed soon, > > unless you deem it necessary. > > why are you adding code that uses EditableProfilePhoto if you're about to remove > EditableProfilePhoto? Oops sorry, I got this mixed up with the name class. So the reason for setting the bound of the profile photo is mostly for the positioning variables. The current user menu needs to position the photo at the right x-position while the material design user menu needs to position the photo at the right y-position. https://codereview.chromium.org/2023093002/diff/180001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/profile_chooser_view.cc:1561: view->AddChildView(profile_container); On 2016/06/14 15:16:26, Evan Stade wrote: > On 2016/06/14 14:47:21, Jane wrote: > > On 2016/06/13 23:14:40, Evan Stade wrote: > > > can't you do this unconditionally? > > > > No, because more child views will be added to profile_container if the profile > > is signed in. > > Not sure I understand. Why can't you add child views to profile_container after > adding profile_container to view? Done. What I meant was more along the line of how the code would be easier to understand if the view is only added after all its child views are in place - that's the impression I got from existing code. But following your comment, I moved this line outside. https://codereview.chromium.org/2023093002/diff/180001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/profile_chooser_view.cc:1598: auth_error_email_button_->set_request_focus_on_press(true); On 2016/06/14 15:16:26, Evan Stade wrote: > On 2016/06/14 14:47:21, Jane wrote: > > On 2016/06/13 23:14:40, Evan Stade wrote: > > > why do you want this non-default behavior > > > > As mentioned before, this code is basically copied from the function above > > CreateCurrentProfileView (e.g. this line corresponds to L1456), and this code > > will be heavily modified as the user menu revamp project proceeds. I didn't > take > > a close look at why this was done and I assumed it was reasonable as it is > from > > existing code. As a matter of fact, the lines under this particular condition > is > > getting removed in the downstream CL. I don't plan to investigate on this > unless > > you think it's necessary. > > If there are large blocks of code you're about to delete, you should clearly > mark them as such with disclaimers like TODO(yourname): Delete this entire block > of code: crbug.com/mybug > > Generally speaking I'm not a huge fan of copying code without analyzing whether > it still makes sense. I just deleted this block instead, this is probably the easier solution. I understand that I should analyze the code while copying it, but the intention here was to break up a potentially huge CL in two, so that this CL deals more with getting the same set of functions going for the new menu while making some UI adjustments; the downstream CL that branches off this CL performs a more comprehensive revamp. This piece of code is deleted right at the said downstream CL. Again, I realize that it would have probably been a better idea to not split up the CL actually. Btw I can't own a bug. https://codereview.chromium.org/2023093002/diff/180001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/profile_chooser_view.cc:1633: signin_current_profile_link_->SetMinSize(gfx::Size(0, 36)); On 2016/06/14 15:16:26, Evan Stade wrote: > On 2016/06/14 14:47:21, Jane wrote: > > On 2016/06/13 23:14:41, Evan Stade wrote: > > > here's a constant I don't understand the need for > > > > This was supposed to define the height of the button. Do you think this line > is > > unnecessary? > > yes, i think the height should be the same as all the other buttons in chrome I put it here because SetMinSize of LabelButton sets the minimum height to 33, but the redlines specify 36. I will remove this line if you think it's safe to ignore. https://codereview.chromium.org/2023093002/diff/180001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/profile_chooser_view.cc:1637: view->AddChildView(padding_view); On 2016/06/14 15:16:26, Evan Stade wrote: > On 2016/06/14 14:47:21, Jane wrote: > > On 2016/06/13 23:14:40, Evan Stade wrote: > > > if you need padding under the md text button, perhaps you can add an empty > > > border to |view|, or you can add an empty SizedContainer view > > > > I tried adding: > > > > view->SetBorder(views::Border::CreateEmptyBorder( > > 0, 0, views::kRelatedControlVerticalSpacing, 0)); > > > > ...but no extra padding was shown. Why? > > I dunno, is there something else setting a border on view? I suggest > investigating because "why" is a good question and might have an interesting > answer. > > > So now I'm using a SizedContainer of height 1. Would you say it's ok to have > an > > extra height of 1? > Done. It works now.
Jane: you can assign any bugs you are working to me. Thanks, Roger On Jun 14, 2016 17:39, <janeliulwq@google.com> wrote: > Thanks again for the elaborate comments! > I should point out that the only way I have been able to learn about the > code > base and work with it so far has been through reading existing code mostly > from > this file, which is why you would see certain patterns in my code that > follow > the existing patterns, which are not necessarily correct or the best > practices. > Your comments are definitely a good way for me to learn about rules and > practices that don't exist in this file already. Please bear with me while > I > work this through! > > > > https://codereview.chromium.org/2023093002/diff/180001/chrome/browser/ui/view... > File chrome/browser/ui/views/profiles/profile_chooser_view.cc (right): > > > https://codereview.chromium.org/2023093002/diff/180001/chrome/browser/ui/view... > chrome/browser/ui/views/profiles/profile_chooser_view.cc:425: > label_->SetBorder(views::Border::CreateEmptyBorder(2, 0, 2, 0)); > On 2016/06/14 15:16:26, Evan Stade wrote: > > On 2016/06/14 14:47:21, Jane wrote: > > > On 2016/06/13 23:14:41, Evan Stade wrote: > > > > are these 2s distinct from the 2s below? > > > > > > I assume so, but then again, I'm not touching any of the 2s myself. > Are you > > > saying it's necessary for me to make a constant for this? > > > > I'm a bit perplexed by your stance on pre-existing code. When you add > new code > > that is related to pre-existing code, you often make changes or > refactors to > > share logic or constants, and this requires modifying said > pre-existing code. So > > yes, it is necessary to evaluate and adjust the code that was in this > file prior > > to this CL. > > > > Unless I misunderstand you, you're also implying that if one reviewer > approves, > > all reviewers should approve. Sad to say, sometimes a reviewer misses > something. > > If someone could review code perfectly they could write it perfectly > as well and > > we'd probably not need a review process in the first place. > > Done. Makes sense, I set 2 to be border_width. > > > https://codereview.chromium.org/2023093002/diff/180001/chrome/browser/ui/view... > chrome/browser/ui/views/profiles/profile_chooser_view.cc:628: int offset > = ((switches::IsMaterialDesignUserMenu() ? kSmallProfileBadgeSize > On 2016/06/14 15:16:26, Evan Stade wrote: > > On 2016/06/14 14:47:21, Jane wrote: > > > On 2016/06/13 23:14:40, Evan Stade wrote: > > > > this ternary appears 3 times > > > > > > I did realize this and I wanted to make it a const right at where > > > kSmallProfileBadgeSize and kProfileBadgeSize are defined, but I get > segfaults > > by > > > calling the switches function there, so I thought maybe having them > remain > > this > > > way would be easier. However, if you think it's necessary, I can > make a static > > > function for this. > > > > I think that would be an improvement. The badge size constants need > not be > > defined separately from that function. > > Done. > > > https://codereview.chromium.org/2023093002/diff/180001/chrome/browser/ui/view... > chrome/browser/ui/views/profiles/profile_chooser_view.cc:1504: int > column_width = > On 2016/06/14 15:16:26, Evan Stade wrote: > > On 2016/06/14 14:47:21, Jane wrote: > > > On 2016/06/13 23:14:41, Evan Stade wrote: > > > > are you sure the box layout can't grow wider than this? > > > > > > I'm only using column_width as the width for profile_container but > not the box > > > layout if that's what you are asking. > > > > what sets the width of the box layout then? > > I thought the width of the box layout would be determined by the width > of its children and its inside_border_horizontal_spacing? > > > https://codereview.chromium.org/2023093002/diff/180001/chrome/browser/ui/view... > chrome/browser/ui/views/profiles/profile_chooser_view.cc:1517: > kMediumImageSide, kMediumImageSide)); > On 2016/06/14 15:16:26, Evan Stade wrote: > > On 2016/06/14 14:47:21, Jane wrote: > > > On 2016/06/13 23:14:40, Evan Stade wrote: > > > > Yyou're doing layout at construction time, which is always going > to be more > > > > complicated than using a layout manager or overriding Layout(). I > don't > > think > > > > you even need to specify a size here because LabelButton will > naturally use > > > the > > > > image for sizing. > > > > > > > > give profile_container a horizontal box layout with inside border > vertical > > > > spacing of 8 and between child spacing of > kMaterialMenuHorizontalEdgeMargin > > > > > > This is a very good suggestion. I thought about using a box layout > and tried > > > doing it, but I think this is a more complicated case: the profile > photo might > > > have a badge attached to it, so another layout/parent view is needed > to hold > > > them together; the avatar name and email address needs to in the > same column > > > too; and EditableProfileName needs to be closer to the profile photo > than > > > kMaterialMenuHorizontalEdgeMargin because the textfield insets need > to be > > > accounted. If anything, maybe I should create two columns for this. > However, I > > > really think it would be easier to make it happen in the downstream > CL because > > > some of these components will be simplified or removed. Let me know > if you > > think > > > this is ok for now and what I should do as next step. > > > > When I said "or overriding Layout()", I guess I should have specified > that this > > is the typical way of handling very custom layout. That said, I don't > think it's > > too difficult to add a couple container views or conditionally adjust > insets or > > borders. > > > > Done. I added a couple of containers to the view, does it look better > now? > > > > Also, I assume you are talking about EditableProfilePhoto when you > said I > > don't > > > need to specify a size for LabelButton. You make a point, but then > again, I > > > don't plan to investigate on this pre-existing code that will be > removed soon, > > > unless you deem it necessary. > > > > why are you adding code that uses EditableProfilePhoto if you're about > to remove > > EditableProfilePhoto? > > Oops sorry, I got this mixed up with the name class. So the reason for > setting the bound of the profile photo is mostly for the positioning > variables. The current user menu needs to position the photo at the > right x-position while the material design user menu needs to position > the photo at the right y-position. > > > https://codereview.chromium.org/2023093002/diff/180001/chrome/browser/ui/view... > chrome/browser/ui/views/profiles/profile_chooser_view.cc:1561: > view->AddChildView(profile_container); > On 2016/06/14 15:16:26, Evan Stade wrote: > > On 2016/06/14 14:47:21, Jane wrote: > > > On 2016/06/13 23:14:40, Evan Stade wrote: > > > > can't you do this unconditionally? > > > > > > No, because more child views will be added to profile_container if > the profile > > > is signed in. > > > > Not sure I understand. Why can't you add child views to > profile_container after > > adding profile_container to view? > > Done. > What I meant was more along the line of how the code would be easier to > understand if the view is only added after all its child views are in > place - that's the impression I got from existing code. But following > your comment, I moved this line outside. > > > https://codereview.chromium.org/2023093002/diff/180001/chrome/browser/ui/view... > chrome/browser/ui/views/profiles/profile_chooser_view.cc:1598: > auth_error_email_button_->set_request_focus_on_press(true); > On 2016/06/14 15:16:26, Evan Stade wrote: > > On 2016/06/14 14:47:21, Jane wrote: > > > On 2016/06/13 23:14:40, Evan Stade wrote: > > > > why do you want this non-default behavior > > > > > > As mentioned before, this code is basically copied from the function > above > > > CreateCurrentProfileView (e.g. this line corresponds to L1456), and > this code > > > will be heavily modified as the user menu revamp project proceeds. I > didn't > > take > > > a close look at why this was done and I assumed it was reasonable as > it is > > from > > > existing code. As a matter of fact, the lines under this particular > condition > > is > > > getting removed in the downstream CL. I don't plan to investigate on > this > > unless > > > you think it's necessary. > > > > If there are large blocks of code you're about to delete, you should > clearly > > mark them as such with disclaimers like TODO(yourname): Delete this > entire block > > of code: crbug.com/mybug > > > > Generally speaking I'm not a huge fan of copying code without > analyzing whether > > it still makes sense. > > I just deleted this block instead, this is probably the easier solution. > I understand that I should analyze the code while copying it, but the > intention here was to break up a potentially huge CL in two, so that > this CL deals more with getting the same set of functions going for the > new menu while making some UI adjustments; the downstream CL that > branches off this CL performs a more comprehensive revamp. This piece of > code is deleted right at the said downstream CL. Again, I realize that > it would have probably been a better idea to not split up the CL > actually. > Btw I can't own a bug. > > > https://codereview.chromium.org/2023093002/diff/180001/chrome/browser/ui/view... > chrome/browser/ui/views/profiles/profile_chooser_view.cc:1633: > signin_current_profile_link_->SetMinSize(gfx::Size(0, 36)); > On 2016/06/14 15:16:26, Evan Stade wrote: > > On 2016/06/14 14:47:21, Jane wrote: > > > On 2016/06/13 23:14:41, Evan Stade wrote: > > > > here's a constant I don't understand the need for > > > > > > This was supposed to define the height of the button. Do you think > this line > > is > > > unnecessary? > > > > yes, i think the height should be the same as all the other buttons in > chrome > > I put it here because SetMinSize of LabelButton sets the minimum height > to 33, but the redlines specify 36. I will remove this line if you think > it's safe to ignore. > > > https://codereview.chromium.org/2023093002/diff/180001/chrome/browser/ui/view... > chrome/browser/ui/views/profiles/profile_chooser_view.cc:1637: > view->AddChildView(padding_view); > On 2016/06/14 15:16:26, Evan Stade wrote: > > On 2016/06/14 14:47:21, Jane wrote: > > > On 2016/06/13 23:14:40, Evan Stade wrote: > > > > if you need padding under the md text button, perhaps you can add > an empty > > > > border to |view|, or you can add an empty SizedContainer view > > > > > > I tried adding: > > > > > > view->SetBorder(views::Border::CreateEmptyBorder( > > > 0, 0, views::kRelatedControlVerticalSpacing, 0)); > > > > > > ...but no extra padding was shown. Why? > > > > I dunno, is there something else setting a border on view? I suggest > > investigating because "why" is a good question and might have an > interesting > > answer. > > > > > So now I'm using a SizedContainer of height 1. Would you say it's ok > to have > > an > > > extra height of 1? > > > > Done. It works now. > > https://codereview.chromium.org/2023093002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
https://codereview.chromium.org/2023093002/diff/180001/chrome/browser/ui/view... File chrome/browser/ui/views/profiles/profile_chooser_view.cc (right): https://codereview.chromium.org/2023093002/diff/180001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/profile_chooser_view.cc:1504: int column_width = On 2016/06/14 21:39:57, Jane wrote: > On 2016/06/14 15:16:26, Evan Stade wrote: > > On 2016/06/14 14:47:21, Jane wrote: > > > On 2016/06/13 23:14:41, Evan Stade wrote: > > > > are you sure the box layout can't grow wider than this? > > > > > > I'm only using column_width as the width for profile_container but not the > box > > > layout if that's what you are asking. > > > > what sets the width of the box layout then? > > I thought the width of the box layout would be determined by the width of its > children and its inside_border_horizontal_spacing? Well, that would set its preferred size. But in this case you're adding the box layout to a grid layout, and that grid layout is probably (or should be) making it fill a pre-set width, so I'm not entirely sure why this is necessary. https://codereview.chromium.org/2023093002/diff/180001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/profile_chooser_view.cc:1517: kMediumImageSide, kMediumImageSide)); On 2016/06/14 21:39:57, Jane wrote: > On 2016/06/14 15:16:26, Evan Stade wrote: > > On 2016/06/14 14:47:21, Jane wrote: > > > On 2016/06/13 23:14:40, Evan Stade wrote: > > > > Yyou're doing layout at construction time, which is always going to be > more > > > > complicated than using a layout manager or overriding Layout(). I don't > > think > > > > you even need to specify a size here because LabelButton will naturally > use > > > the > > > > image for sizing. > > > > > > > > give profile_container a horizontal box layout with inside border vertical > > > > spacing of 8 and between child spacing of > kMaterialMenuHorizontalEdgeMargin > > > > > > This is a very good suggestion. I thought about using a box layout and tried > > > doing it, but I think this is a more complicated case: the profile photo > might > > > have a badge attached to it, so another layout/parent view is needed to hold > > > them together; the avatar name and email address needs to in the same column > > > too; and EditableProfileName needs to be closer to the profile photo than > > > kMaterialMenuHorizontalEdgeMargin because the textfield insets need to be > > > accounted. If anything, maybe I should create two columns for this. However, > I > > > really think it would be easier to make it happen in the downstream CL > because > > > some of these components will be simplified or removed. Let me know if you > > think > > > this is ok for now and what I should do as next step. > > > > When I said "or overriding Layout()", I guess I should have specified that > this > > is the typical way of handling very custom layout. That said, I don't think > it's > > too difficult to add a couple container views or conditionally adjust insets > or > > borders. > > > > Done. I added a couple of containers to the view, does it look better now? SizedContainers are just a different way of hardcoding layout. > > > > Also, I assume you are talking about EditableProfilePhoto when you said I > > don't > > > need to specify a size for LabelButton. You make a point, but then again, I > > > don't plan to investigate on this pre-existing code that will be removed > soon, > > > unless you deem it necessary. > > > > why are you adding code that uses EditableProfilePhoto if you're about to > remove > > EditableProfilePhoto? > > Oops sorry, I got this mixed up with the name class. So the reason for setting > the bound of the profile photo is mostly for the positioning variables. The > current user menu needs to position the photo at the right x-position while the > material design user menu needs to position the photo at the right y-position. the right y-position? I'm not sure I follow, but I still think this is awkward. You should let the profile photo be sized according to its preferred size and positioned by the layout of its parent view. If you're using a horizontal box layout, for example, the width would automatically be set to the preferred width while the vertical centering would be achieved with set_cross_axis_alignment https://codereview.chromium.org/2023093002/diff/180001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/profile_chooser_view.cc:1633: signin_current_profile_link_->SetMinSize(gfx::Size(0, 36)); On 2016/06/14 21:39:57, Jane wrote: > On 2016/06/14 15:16:26, Evan Stade wrote: > > On 2016/06/14 14:47:21, Jane wrote: > > > On 2016/06/13 23:14:41, Evan Stade wrote: > > > > here's a constant I don't understand the need for > > > > > > This was supposed to define the height of the button. Do you think this line > > is > > > unnecessary? > > > > yes, i think the height should be the same as all the other buttons in chrome > > I put it here because SetMinSize of LabelButton sets the minimum height to 33, > but the redlines specify 36. I will remove this line if you think it's safe to > ignore. Yes, I do think you can/should. For example, there are changes in flight to make all MD buttons just 24px tall (a result of the UI leads providing feedback on the current, taller buttons). Whatever we land on for the rest of Chrome, you'll want the same here. https://codereview.chromium.org/2023093002/diff/260001/chrome/browser/ui/view... File chrome/browser/ui/views/profiles/profile_chooser_view.cc (right): https://codereview.chromium.org/2023093002/diff/260001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/profile_chooser_view.cc:1512: new SizedContainer(gfx::Size(column_width, profile_bar_height)); I don't really like SizedContainer (and you use it three times here) because it is just a way to hardcode sizes. You still have too many hardcoded sizes, e.g. you defined 56 for the profile bar, but this is actually redundant because you can derive this from the contents of that view (image height + padding).
Patchset #11 (id:280001) has been deleted
https://codereview.chromium.org/2023093002/diff/180001/chrome/browser/ui/view... File chrome/browser/ui/views/profiles/profile_chooser_view.cc (right): https://codereview.chromium.org/2023093002/diff/180001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/profile_chooser_view.cc:1504: int column_width = On 2016/06/16 21:56:34, Evan Stade wrote: > On 2016/06/14 21:39:57, Jane wrote: > > On 2016/06/14 15:16:26, Evan Stade wrote: > > > On 2016/06/14 14:47:21, Jane wrote: > > > > On 2016/06/13 23:14:41, Evan Stade wrote: > > > > > are you sure the box layout can't grow wider than this? > > > > > > > > I'm only using column_width as the width for profile_container but not the > > box > > > > layout if that's what you are asking. > > > > > > what sets the width of the box layout then? > > > > I thought the width of the box layout would be determined by the width of its > > children and its inside_border_horizontal_spacing? > > Well, that would set its preferred size. But in this case you're adding the box > layout to a grid layout, and that grid layout is probably (or should be) making > it fill a pre-set width, so I'm not entirely sure why this is necessary. Done. Removed column_width. https://codereview.chromium.org/2023093002/diff/180001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/profile_chooser_view.cc:1517: kMediumImageSide, kMediumImageSide)); On 2016/06/16 21:56:33, Evan Stade wrote: > On 2016/06/14 21:39:57, Jane wrote: > > On 2016/06/14 15:16:26, Evan Stade wrote: > > > On 2016/06/14 14:47:21, Jane wrote: > > > > On 2016/06/13 23:14:40, Evan Stade wrote: > > > > > Yyou're doing layout at construction time, which is always going to be > > more > > > > > complicated than using a layout manager or overriding Layout(). I don't > > > think > > > > > you even need to specify a size here because LabelButton will naturally > > use > > > > the > > > > > image for sizing. > > > > > > > > > > give profile_container a horizontal box layout with inside border > vertical > > > > > spacing of 8 and between child spacing of > > kMaterialMenuHorizontalEdgeMargin > > > > > > > > This is a very good suggestion. I thought about using a box layout and > tried > > > > doing it, but I think this is a more complicated case: the profile photo > > might > > > > have a badge attached to it, so another layout/parent view is needed to > hold > > > > them together; the avatar name and email address needs to in the same > column > > > > too; and EditableProfileName needs to be closer to the profile photo than > > > > kMaterialMenuHorizontalEdgeMargin because the textfield insets need to be > > > > accounted. If anything, maybe I should create two columns for this. > However, > > I > > > > really think it would be easier to make it happen in the downstream CL > > because > > > > some of these components will be simplified or removed. Let me know if you > > > think > > > > this is ok for now and what I should do as next step. > > > > > > When I said "or overriding Layout()", I guess I should have specified that > > this > > > is the typical way of handling very custom layout. That said, I don't think > > it's > > > too difficult to add a couple container views or conditionally adjust insets > > or > > > borders. > > > > > > > Done. I added a couple of containers to the view, does it look better now? > > SizedContainers are just a different way of hardcoding layout. > Point taken. See edits/comments in the newest patch. > > > > > > Also, I assume you are talking about EditableProfilePhoto when you said I > > > don't > > > > need to specify a size for LabelButton. You make a point, but then again, > I > > > > don't plan to investigate on this pre-existing code that will be removed > > soon, > > > > unless you deem it necessary. > > > > > > why are you adding code that uses EditableProfilePhoto if you're about to > > remove > > > EditableProfilePhoto? > > > > Oops sorry, I got this mixed up with the name class. So the reason for setting > > the bound of the profile photo is mostly for the positioning variables. The > > current user menu needs to position the photo at the right x-position while > the > > material design user menu needs to position the photo at the right y-position. > > > the right y-position? I'm not sure I follow, but I still think this is awkward. > You should let the profile photo be sized according to its preferred size and > positioned by the layout of its parent view. If you're using a horizontal box > layout, for example, the width would automatically be set to the preferred width > while the vertical centering would be achieved with set_cross_axis_alignment I meant "the correct y-position". This makes sense. I removed the last argument (bounds) for the constructor, and set its size to be preferred size instead. However, I couldn't think of a way to use box layout for profile_photo_container (see comment in the newer patch), so I'm still kind of hardcoding the position of EditableProfilePhoto using SetY. Let me know if you know of a cleaner way! https://codereview.chromium.org/2023093002/diff/180001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/profile_chooser_view.cc:1633: signin_current_profile_link_->SetMinSize(gfx::Size(0, 36)); On 2016/06/16 21:56:33, Evan Stade wrote: > On 2016/06/14 21:39:57, Jane wrote: > > On 2016/06/14 15:16:26, Evan Stade wrote: > > > On 2016/06/14 14:47:21, Jane wrote: > > > > On 2016/06/13 23:14:41, Evan Stade wrote: > > > > > here's a constant I don't understand the need for > > > > > > > > This was supposed to define the height of the button. Do you think this > line > > > is > > > > unnecessary? > > > > > > yes, i think the height should be the same as all the other buttons in > chrome > > > > I put it here because SetMinSize of LabelButton sets the minimum height to 33, > > but the redlines specify 36. I will remove this line if you think it's safe to > > ignore. > > Yes, I do think you can/should. For example, there are changes in flight to make > all MD buttons just 24px tall (a result of the UI leads providing feedback on > the current, taller buttons). Whatever we land on for the rest of Chrome, you'll > want the same here. Done. https://codereview.chromium.org/2023093002/diff/260001/chrome/browser/ui/view... File chrome/browser/ui/views/profiles/profile_chooser_view.cc (right): https://codereview.chromium.org/2023093002/diff/260001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/profile_chooser_view.cc:1512: new SizedContainer(gfx::Size(column_width, profile_bar_height)); On 2016/06/16 21:56:34, Evan Stade wrote: > I don't really like SizedContainer (and you use it three times here) because it > is just a way to hardcode sizes. > > You still have too many hardcoded sizes, e.g. you defined 56 for the profile > bar, but this is actually redundant because you can derive this from the > contents of that view (image height + padding). I modified profile_name_container and profile_container to be simply views::View(). Does this look better / is this what you meant? As for profile_photo_container, I'm not sure what the proper way is to construct profile_photo_container. The supervised icon badge needs to be positioned at a specific location (and currently it's achieved through SetBounds (as in CreateCurrentProfileView)), making box layout seem to be a bad option. If I simply use views::View, the preferred size comes out to be 0. I don't know how to set the proper size for this view without explicitly setting it through SizedContainer or something similar. Could you give me pointers on this? Thanks!
sorry, only had time for an incomplete review. Hopefully this is enough for now for you to keep making improvements. Keep up the good work! https://codereview.chromium.org/2023093002/diff/300001/chrome/browser/ui/view... File chrome/browser/ui/views/profiles/profile_chooser_view.cc (right): https://codereview.chromium.org/2023093002/diff/300001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/profile_chooser_view.cc:92: const int kMediumImageSide = 40; if this is going to be the only image size, I'd name this MdImageSide or something similar https://codereview.chromium.org/2023093002/diff/300001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/profile_chooser_view.cc:99: const int kMaterialMenuHorizontalEdgeMargin = 16; nit: I'd combine these constants https://codereview.chromium.org/2023093002/diff/300001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/profile_chooser_view.cc:323: : views::LabelButton(listener, base::string16()), photo_overlay_(NULL) { s/NULL/nullptr https://codereview.chromium.org/2023093002/diff/300001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/profile_chooser_view.cc:485: static const int border_width = 2; If you explained this already, I forgot (sorry): logically, why is this not profile_name_textfield_->GetInsets().top()/bottom()? https://codereview.chromium.org/2023093002/diff/300001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/profile_chooser_view.cc:1525: supervised_icon->SetBounds( seems like supervised_icon should be a child of current_profile_photo_ instead of profile_photo_container https://codereview.chromium.org/2023093002/diff/300001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/profile_chooser_view.cc:1541: name_container_v_spacing += views::kRelatedControlVerticalSpacing; nit: no curly braces
Thanks! https://codereview.chromium.org/2023093002/diff/300001/chrome/browser/ui/view... File chrome/browser/ui/views/profiles/profile_chooser_view.cc (right): https://codereview.chromium.org/2023093002/diff/300001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/profile_chooser_view.cc:92: const int kMediumImageSide = 40; On 2016/06/20 17:51:11, Evan Stade wrote: > if this is going to be the only image size, I'd name this MdImageSide or > something similar Done. https://codereview.chromium.org/2023093002/diff/300001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/profile_chooser_view.cc:99: const int kMaterialMenuHorizontalEdgeMargin = 16; On 2016/06/20 17:51:11, Evan Stade wrote: > nit: I'd combine these constants Done. https://codereview.chromium.org/2023093002/diff/300001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/profile_chooser_view.cc:323: : views::LabelButton(listener, base::string16()), photo_overlay_(NULL) { On 2016/06/20 17:51:11, Evan Stade wrote: > s/NULL/nullptr Done. https://codereview.chromium.org/2023093002/diff/300001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/profile_chooser_view.cc:485: static const int border_width = 2; On 2016/06/20 17:51:11, Evan Stade wrote: > If you explained this already, I forgot (sorry): logically, why is this not > profile_name_textfield_->GetInsets().top()/bottom()? Don't think we went over this, but it's because its insets would be 4 instead. 2 here is to account for the textfield border width on the top and bottom. Does this make sense? https://codereview.chromium.org/2023093002/diff/300001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/profile_chooser_view.cc:1525: supervised_icon->SetBounds( On 2016/06/20 17:51:11, Evan Stade wrote: > seems like supervised_icon should be a child of current_profile_photo_ instead > of profile_photo_container That's a good logical point. However, the current implementation of the EditableProfilePhoto class clips itself as a whole to display the profile photo as a circle, and also inherits from LabelButton so that the profile photo is clickable. This makes it hard for the badge to be added to current_profile_photo_ since it should neither be clipped nor clickable. This is why I made the decision to wrap current_profile_photo_ inside profile_photo_container, and place the badge in there too. Let me know if you think there's a cleaner way to achieve this! https://codereview.chromium.org/2023093002/diff/300001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/profile_chooser_view.cc:1541: name_container_v_spacing += views::kRelatedControlVerticalSpacing; On 2016/06/20 17:51:11, Evan Stade wrote: > nit: no curly braces Done.
https://codereview.chromium.org/2023093002/diff/300001/chrome/browser/ui/view... File chrome/browser/ui/views/profiles/profile_chooser_view.cc (right): https://codereview.chromium.org/2023093002/diff/300001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/profile_chooser_view.cc:364: views::LabelButton::OnPaint(canvas); canvas->Save(); // current fn body canvas->Restore(); if (browser_->profile()->IsSupervised()) copy some code from ProfileBadge::OnPaint normally copying is bad but if we remove ProfileBadge soon it doesn't make sense to share (although sharing would also not be hard here as you can probably construct a local ProfileBadge and call OnPaint manually) https://codereview.chromium.org/2023093002/diff/300001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/profile_chooser_view.cc:485: static const int border_width = 2; On 2016/06/20 18:53:26, Jane wrote: > On 2016/06/20 17:51:11, Evan Stade wrote: > > If you explained this already, I forgot (sorry): logically, why is this not > > profile_name_textfield_->GetInsets().top()/bottom()? > > Don't think we went over this, but it's because its insets would be 4 instead. 2 > here is to account for the textfield border width on the top and bottom. Does > this make sense? Not really, isn't the border 1px rather than 2? If you're saying 1+1=2 then why are you setting this on the top and bottom of button_? Can you use profile_name_textfield_->border()->GetInsets()? https://codereview.chromium.org/2023093002/diff/300001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/profile_chooser_view.cc:1525: supervised_icon->SetBounds( On 2016/06/20 18:53:26, Jane wrote: > On 2016/06/20 17:51:11, Evan Stade wrote: > > seems like supervised_icon should be a child of current_profile_photo_ instead > > of profile_photo_container > > That's a good logical point. However, the current implementation of the > EditableProfilePhoto class clips itself as a whole to display the profile photo > as a circle, and also inherits from LabelButton so that the profile photo is > clickable. This makes it hard for the badge to be added to > current_profile_photo_ since it should neither be clipped nor clickable. where does it say in the mocks it shouldn't be clickable? I'd have assumed it would do the same thing as clicking the profile photo. In fact you don't even really need a view for this. (see above) > This is > why I made the decision to wrap current_profile_photo_ inside > profile_photo_container, and place the badge in there too. Let me know if you > think there's a cleaner way to achieve this!
Patchset #13 (id:340001) has been deleted
https://codereview.chromium.org/2023093002/diff/300001/chrome/browser/ui/view... File chrome/browser/ui/views/profiles/profile_chooser_view.cc (right): https://codereview.chromium.org/2023093002/diff/300001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/profile_chooser_view.cc:364: views::LabelButton::OnPaint(canvas); On 2016/06/21 21:55:16, Evan Stade wrote: > canvas->Save(); > // current fn body > canvas->Restore(); > if (browser_->profile()->IsSupervised()) > copy some code from ProfileBadge::OnPaint > > normally copying is bad but if we remove ProfileBadge soon it doesn't make sense > to share (although sharing would also not be hard here as you can probably > construct a local ProfileBadge and call OnPaint manually) Thanks, I see what you are getting at. I'm not sure if I got how to implement it though, the profile badge doesn't show up with the code in the newest patch. Do you know what I'm missing? (I tried moving code around and inheriting from CanvasImageSource, etc., as well. Should I make the badge a childview instead? Sorry, I don't know much about this painting thing.) https://codereview.chromium.org/2023093002/diff/300001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/profile_chooser_view.cc:485: static const int border_width = 2; On 2016/06/21 21:55:16, Evan Stade wrote: > On 2016/06/20 18:53:26, Jane wrote: > > On 2016/06/20 17:51:11, Evan Stade wrote: > > > If you explained this already, I forgot (sorry): logically, why is this not > > > profile_name_textfield_->GetInsets().top()/bottom()? > > > > Don't think we went over this, but it's because its insets would be 4 instead. > 2 > > here is to account for the textfield border width on the top and bottom. Does > > this make sense? > > Not really, isn't the border 1px rather than 2? If you're saying 1+1=2 then why > are you setting this on the top and bottom of button_? Can you use > profile_name_textfield_->border()->GetInsets()? Done. I thought the original code had a purpose... But changing it to 1px doesn't make it look any different. https://codereview.chromium.org/2023093002/diff/300001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/profile_chooser_view.cc:1525: supervised_icon->SetBounds( On 2016/06/21 21:55:16, Evan Stade wrote: > On 2016/06/20 18:53:26, Jane wrote: > > On 2016/06/20 17:51:11, Evan Stade wrote: > > > seems like supervised_icon should be a child of current_profile_photo_ > instead > > > of profile_photo_container > > > > That's a good logical point. However, the current implementation of the > > EditableProfilePhoto class clips itself as a whole to display the profile > photo > > as a circle, and also inherits from LabelButton so that the profile photo is > > clickable. This makes it hard for the badge to be added to > > current_profile_photo_ since it should neither be clipped nor clickable. > > where does it say in the mocks it shouldn't be clickable? I'd have assumed it > would do the same thing as clicking the profile photo. > > In fact you don't even really need a view for this. (see above) > > > This is > > why I made the decision to wrap current_profile_photo_ inside > > profile_photo_container, and place the badge in there too. Let me know if you > > think there's a cleaner way to achieve this! > Sorry, let me be more clear about "clickable": I was actually referring to the current user menu behavior - you can't click the badge to edit the profile photo. Since the material design user menu can only be turned on with a flag right now, the current/old behavior has to be maintained, and that's why I had worries about it. It seems to me that your suggestion about putting the badge inside EditableProfilePhoto would have the same problem, since the badge will be part of the LabelButton. Am I missing something about this?
https://codereview.chromium.org/2023093002/diff/300001/chrome/browser/ui/view... File chrome/browser/ui/views/profiles/profile_chooser_view.cc (right): https://codereview.chromium.org/2023093002/diff/300001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/profile_chooser_view.cc:364: views::LabelButton::OnPaint(canvas); On 2016/06/22 15:38:33, Jane wrote: > On 2016/06/21 21:55:16, Evan Stade wrote: > > canvas->Save(); > > // current fn body > > canvas->Restore(); > > if (browser_->profile()->IsSupervised()) > > copy some code from ProfileBadge::OnPaint > > > > normally copying is bad but if we remove ProfileBadge soon it doesn't make > sense > > to share (although sharing would also not be hard here as you can probably > > construct a local ProfileBadge and call OnPaint manually) > > Thanks, I see what you are getting at. I'm not sure if I got how to implement it > though, the profile badge doesn't show up with the code in the newest patch. Do > you know what I'm missing? (I tried moving code around and inheriting from > CanvasImageSource, etc., as well. Should I make the badge a childview instead? > Sorry, I don't know much about this painting thing.) children are painted after |this| so you'll have to hook PaintChildren instead of OnPaint for this badge. https://codereview.chromium.org/2023093002/diff/360001/chrome/browser/ui/view... File chrome/browser/ui/views/profiles/profile_chooser_view.cc (right): https://codereview.chromium.org/2023093002/diff/360001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/profile_chooser_view.cc:379: const SkISize size = canvas->sk_canvas()->getBaseLayerSize(); use View::bounds(). Also the coordinate calculations below will need to be updated. https://codereview.chromium.org/2023093002/diff/360001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/profile_chooser_view.cc:384: paint.setColor(SK_ColorWHITE); This constant (color) should be removed in favor of GetNativeTheme()->GetSystemColor(kColorId_BubbleBackground) https://codereview.chromium.org/2023093002/diff/360001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/profile_chooser_view.cc:455: (new views::Textfield)->border()->GetInsets().top(); this view is leaking https://codereview.chromium.org/2023093002/diff/360001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/profile_chooser_view.cc:455: (new views::Textfield)->border()->GetInsets().top(); pretty sure this should be const int textfield_border_insets = views::TextField().border()->GetInsets(); then below (in several places) CreateEmptyBorder(textfield_border_insets) or CreateEmptyBorder(textfield_border_insets + gfx::Insets(0, icontextlabelwhatever, 0, 0))
Patchset #14 (id:380001) has been deleted
https://codereview.chromium.org/2023093002/diff/300001/chrome/browser/ui/view... File chrome/browser/ui/views/profiles/profile_chooser_view.cc (right): https://codereview.chromium.org/2023093002/diff/300001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/profile_chooser_view.cc:364: views::LabelButton::OnPaint(canvas); On 2016/06/22 17:15:43, Evan Stade wrote: > On 2016/06/22 15:38:33, Jane wrote: > > On 2016/06/21 21:55:16, Evan Stade wrote: > > > canvas->Save(); > > > // current fn body > > > canvas->Restore(); > > > if (browser_->profile()->IsSupervised()) > > > copy some code from ProfileBadge::OnPaint > > > > > > normally copying is bad but if we remove ProfileBadge soon it doesn't make > > sense > > > to share (although sharing would also not be hard here as you can probably > > > construct a local ProfileBadge and call OnPaint manually) > > > > Thanks, I see what you are getting at. I'm not sure if I got how to implement > it > > though, the profile badge doesn't show up with the code in the newest patch. > Do > > you know what I'm missing? (I tried moving code around and inheriting from > > CanvasImageSource, etc., as well. Should I make the badge a childview instead? > > Sorry, I don't know much about this painting thing.) > > children are painted after |this| so you'll have to hook PaintChildren instead > of OnPaint for this badge. Thanks! I think I implemented what you suggested in the newest patch. The only problem I couldn't solve is how to not clip the profile badge together with the profile icon. Would you point me to what I'm doing wrong there? I'm guessing this can be solved by reordering the painting/clipping steps. Appreciate the help! https://codereview.chromium.org/2023093002/diff/360001/chrome/browser/ui/view... File chrome/browser/ui/views/profiles/profile_chooser_view.cc (right): https://codereview.chromium.org/2023093002/diff/360001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/profile_chooser_view.cc:379: const SkISize size = canvas->sk_canvas()->getBaseLayerSize(); On 2016/06/22 17:15:43, Evan Stade wrote: > use View::bounds(). > > Also the coordinate calculations below will need to be updated. Not sure I got what you meant by View::bounds(). If it's about "size", I have modified it. https://codereview.chromium.org/2023093002/diff/360001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/profile_chooser_view.cc:384: paint.setColor(SK_ColorWHITE); On 2016/06/22 17:15:43, Evan Stade wrote: > This constant (color) should be removed in favor of > GetNativeTheme()->GetSystemColor(kColorId_BubbleBackground) Done. https://codereview.chromium.org/2023093002/diff/360001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/profile_chooser_view.cc:455: (new views::Textfield)->border()->GetInsets().top(); On 2016/06/22 17:15:43, Evan Stade wrote: > this view is leaking Done. https://codereview.chromium.org/2023093002/diff/360001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/profile_chooser_view.cc:455: (new views::Textfield)->border()->GetInsets().top(); On 2016/06/22 17:15:43, Evan Stade wrote: > pretty sure this should be > > const int textfield_border_insets = views::TextField().border()->GetInsets(); > > then below (in several places) CreateEmptyBorder(textfield_border_insets) or > CreateEmptyBorder(textfield_border_insets + gfx::Insets(0, > icontextlabelwhatever, 0, 0)) Done.
https://codereview.chromium.org/2023093002/diff/300001/chrome/browser/ui/view... File chrome/browser/ui/views/profiles/profile_chooser_view.cc (right): https://codereview.chromium.org/2023093002/diff/300001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/profile_chooser_view.cc:364: views::LabelButton::OnPaint(canvas); On 2016/06/22 22:02:32, Jane wrote: > On 2016/06/22 17:15:43, Evan Stade wrote: > > On 2016/06/22 15:38:33, Jane wrote: > > > On 2016/06/21 21:55:16, Evan Stade wrote: > > > > canvas->Save(); > > > > // current fn body > > > > canvas->Restore(); > > > > if (browser_->profile()->IsSupervised()) > > > > copy some code from ProfileBadge::OnPaint > > > > > > > > normally copying is bad but if we remove ProfileBadge soon it doesn't make > > > sense > > > > to share (although sharing would also not be hard here as you can probably > > > > construct a local ProfileBadge and call OnPaint manually) > > > > > > Thanks, I see what you are getting at. I'm not sure if I got how to > implement > > it > > > though, the profile badge doesn't show up with the code in the newest patch. > > Do > > > you know what I'm missing? (I tried moving code around and inheriting from > > > CanvasImageSource, etc., as well. Should I make the badge a childview > instead? > > > Sorry, I don't know much about this painting thing.) > > > > children are painted after |this| so you'll have to hook PaintChildren instead > > of OnPaint for this badge. > > Thanks! I think I implemented what you suggested in the newest patch. The only > problem I couldn't solve is how to not clip the profile badge together with the > profile icon. Would you point me to what I'm doing wrong there? I'm guessing > this can be solved by reordering the painting/clipping steps. Appreciate the > help! yes, views are clipped to their bounds. You have to make the view big enough to draw the icon and badge. https://codereview.chromium.org/2023093002/diff/400001/chrome/browser/ui/view... File chrome/browser/ui/views/profiles/profile_chooser_view.cc (right): https://codereview.chromium.org/2023093002/diff/400001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/profile_chooser_view.cc:409: canvas->DrawCircle(center_point, what is this?
Patchset #15 (id:420001) has been deleted
Thanks! https://codereview.chromium.org/2023093002/diff/300001/chrome/browser/ui/view... File chrome/browser/ui/views/profiles/profile_chooser_view.cc (right): https://codereview.chromium.org/2023093002/diff/300001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/profile_chooser_view.cc:364: views::LabelButton::OnPaint(canvas); On 2016/06/24 18:25:26, Evan Stade wrote: > On 2016/06/22 22:02:32, Jane wrote: > > On 2016/06/22 17:15:43, Evan Stade wrote: > > > On 2016/06/22 15:38:33, Jane wrote: > > > > On 2016/06/21 21:55:16, Evan Stade wrote: > > > > > canvas->Save(); > > > > > // current fn body > > > > > canvas->Restore(); > > > > > if (browser_->profile()->IsSupervised()) > > > > > copy some code from ProfileBadge::OnPaint > > > > > > > > > > normally copying is bad but if we remove ProfileBadge soon it doesn't > make > > > > sense > > > > > to share (although sharing would also not be hard here as you can > probably > > > > > construct a local ProfileBadge and call OnPaint manually) > > > > > > > > Thanks, I see what you are getting at. I'm not sure if I got how to > > implement > > > it > > > > though, the profile badge doesn't show up with the code in the newest > patch. > > > Do > > > > you know what I'm missing? (I tried moving code around and inheriting from > > > > CanvasImageSource, etc., as well. Should I make the badge a childview > > instead? > > > > Sorry, I don't know much about this painting thing.) > > > > > > children are painted after |this| so you'll have to hook PaintChildren > instead > > > of OnPaint for this badge. > > > > Thanks! I think I implemented what you suggested in the newest patch. The only > > problem I couldn't solve is how to not clip the profile badge together with > the > > profile icon. Would you point me to what I'm doing wrong there? I'm guessing > > this can be solved by reordering the painting/clipping steps. Appreciate the > > help! > > yes, views are clipped to their bounds. You have to make the view big enough to > draw the icon and badge. Not sure if I'm following, but my question is more about how to properly clip the children views as circles; their sizes don't have a problem right now. See my comment in the newest patch. https://codereview.chromium.org/2023093002/diff/400001/chrome/browser/ui/view... File chrome/browser/ui/views/profiles/profile_chooser_view.cc (right): https://codereview.chromium.org/2023093002/diff/400001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/profile_chooser_view.cc:409: canvas->DrawCircle(center_point, On 2016/06/24 18:25:26, Evan Stade wrote: > what is this? This is to draw the light blue circular background of the badge icon. https://codereview.chromium.org/2023093002/diff/440001/chrome/browser/ui/view... File chrome/browser/ui/views/profiles/profile_chooser_view.cc (right): https://codereview.chromium.org/2023093002/diff/440001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/profile_chooser_view.cc:381: Here is my question in more details: AFAIU, clip_recorder has to be constructed before PaintChildren so that the photo_overlay_ child can be clipped into a circle. PaintChildren has to be called before paint_recorder is constructed below or else the !context.inside_paint_recorder_ check will fail. If this is all true, then the clip_recorder will be alive when paint_recorder is construcuted, which means the profile badge being added will also be clipped inside the circle, which is not the desired behavior. My question is on how to work around this. Any way I can rearrange any of the painting/clipping to only clip photo_overlay_, but not the badge? Thanks for your help!
https://codereview.chromium.org/2023093002/diff/400001/chrome/browser/ui/view... File chrome/browser/ui/views/profiles/profile_chooser_view.cc (right): https://codereview.chromium.org/2023093002/diff/400001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/profile_chooser_view.cc:409: canvas->DrawCircle(center_point, On 2016/06/27 18:08:56, Jane wrote: > On 2016/06/24 18:25:26, Evan Stade wrote: > > what is this? > > This is to draw the light blue circular background of the badge icon. I don't see a light blue bg in the mocks. All I see is white silhouettes on a grey bg. https://codereview.chromium.org/2023093002/diff/400001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/profile_chooser_view.cc:417: SkColorSetRGB(0, 0x66, 0xff)); this should almost certainly be something from color_palette.h https://codereview.chromium.org/2023093002/diff/440001/chrome/browser/ui/view... File chrome/browser/ui/views/profiles/profile_chooser_view.cc (right): https://codereview.chromium.org/2023093002/diff/440001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/profile_chooser_view.cc:381: On 2016/06/27 18:08:57, Jane wrote: > Here is my question in more details: > AFAIU, clip_recorder has to be constructed before PaintChildren so that the > photo_overlay_ child can be clipped into a circle. PaintChildren has to be > called before paint_recorder is constructed below or else the > !context.inside_paint_recorder_ check will fail. > If this is all true, then the clip_recorder will be alive when paint_recorder is > construcuted, which means the profile badge being added will also be clipped > inside the circle, which is not the desired behavior. > My question is on how to work around this. Any way I can rearrange any of the > painting/clipping to only clip photo_overlay_, but not the badge? > Thanks for your help! I dunno offhand. Did you try using two different clip recorders? PaintChildren() { { ui::ClipRecorder clip_recorder(context); ... view::PaintChildren(context); } ui::ClipRecorder clip_recorder(context); // draw badge }
Patchset #16 (id:460001) has been deleted
Thanks! https://codereview.chromium.org/2023093002/diff/400001/chrome/browser/ui/view... File chrome/browser/ui/views/profiles/profile_chooser_view.cc (right): https://codereview.chromium.org/2023093002/diff/400001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/profile_chooser_view.cc:409: canvas->DrawCircle(center_point, On 2016/06/28 03:02:49, Evan Stade wrote: > On 2016/06/27 18:08:56, Jane wrote: > > On 2016/06/24 18:25:26, Evan Stade wrote: > > > what is this? > > > > This is to draw the light blue circular background of the badge icon. > > I don't see a light blue bg in the mocks. All I see is white silhouettes on a > grey bg. Oops, you are right, I totally missed that change. Btw, does this color also need to go into color_palette or no? It doesn't seem like a commonly used color. https://codereview.chromium.org/2023093002/diff/400001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/profile_chooser_view.cc:417: SkColorSetRGB(0, 0x66, 0xff)); On 2016/06/28 03:02:49, Evan Stade wrote: > this should almost certainly be something from color_palette.h (Changed it to white) I couldn't find any defined SkColor for white in color_palette or anywhere else. Should I add the definition over there? It doesn't seem to be used a lot. https://codereview.chromium.org/2023093002/diff/440001/chrome/browser/ui/view... File chrome/browser/ui/views/profiles/profile_chooser_view.cc (right): https://codereview.chromium.org/2023093002/diff/440001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/profile_chooser_view.cc:381: On 2016/06/28 03:02:50, Evan Stade wrote: > On 2016/06/27 18:08:57, Jane wrote: > > Here is my question in more details: > > AFAIU, clip_recorder has to be constructed before PaintChildren so that the > > photo_overlay_ child can be clipped into a circle. PaintChildren has to be > > called before paint_recorder is constructed below or else the > > !context.inside_paint_recorder_ check will fail. > > If this is all true, then the clip_recorder will be alive when paint_recorder > is > > construcuted, which means the profile badge being added will also be clipped > > inside the circle, which is not the desired behavior. > > My question is on how to work around this. Any way I can rearrange any of the > > painting/clipping to only clip photo_overlay_, but not the badge? > > Thanks for your help! > > I dunno offhand. Did you try using two different clip recorders? > > PaintChildren() { > { > ui::ClipRecorder clip_recorder(context); > ... > view::PaintChildren(context); > } > > ui::ClipRecorder clip_recorder(context); > // draw badge > } Done. Thanks, this worked!
Patchset #16 (id:480001) has been deleted
https://codereview.chromium.org/2023093002/diff/400001/chrome/browser/ui/view... File chrome/browser/ui/views/profiles/profile_chooser_view.cc (right): https://codereview.chromium.org/2023093002/diff/400001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/profile_chooser_view.cc:417: SkColorSetRGB(0, 0x66, 0xff)); On 2016/06/28 14:59:22, Jane wrote: > On 2016/06/28 03:02:49, Evan Stade wrote: > > this should almost certainly be something from color_palette.h > > (Changed it to white) > I couldn't find any defined SkColor for white in color_palette or anywhere else. > Should I add the definition over there? It doesn't seem to be used a lot. Actually, I think it makes sense for this color to be the same as the profile badge padding color, so I changed it to be ui::NativeTheme::kColorId_BubbleBackground.
https://codereview.chromium.org/2023093002/diff/400001/chrome/browser/ui/view... File chrome/browser/ui/views/profiles/profile_chooser_view.cc (right): https://codereview.chromium.org/2023093002/diff/400001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/profile_chooser_view.cc:409: canvas->DrawCircle(center_point, On 2016/06/28 14:59:22, Jane wrote: > On 2016/06/28 03:02:49, Evan Stade wrote: > > On 2016/06/27 18:08:56, Jane wrote: > > > On 2016/06/24 18:25:26, Evan Stade wrote: > > > > what is this? > > > > > > This is to draw the light blue circular background of the badge icon. > > > > I don't see a light blue bg in the mocks. All I see is white silhouettes on a > > grey bg. > > Oops, you are right, I totally missed that change. > Btw, does this color also need to go into color_palette or no? It doesn't seem > like a commonly used color. it already is in color_palette.h as kChromeIconGrey https://codereview.chromium.org/2023093002/diff/400001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/profile_chooser_view.cc:417: SkColorSetRGB(0, 0x66, 0xff)); On 2016/06/28 15:33:11, Jane wrote: > On 2016/06/28 14:59:22, Jane wrote: > > On 2016/06/28 03:02:49, Evan Stade wrote: > > > this should almost certainly be something from color_palette.h > > > > (Changed it to white) > > I couldn't find any defined SkColor for white in color_palette or anywhere > else. > > Should I add the definition over there? It doesn't seem to be used a lot. > > Actually, I think it makes sense for this color to be the same as the profile > badge padding color, so I changed it to be > ui::NativeTheme::kColorId_BubbleBackground. a) the definition for white is SK_ColorWHITE b) instead of manually drawing a circle and then a bubble-bg-colored icon on top, you should be just drawing an icon that has the circle and the silhouettes built in. In other words, you should be using ACCOUNT_CHILD instead of ACCOUNT_CHILD_INVERT (the former may not exist yet but is on go/icons) and SUPERVISOR_ACCOUNT_CIRCLE (which doesn't exist in chrome or on go/icons, so you should request it). You will also get rid of the weird alternate sizes because the size difference will be built into the icons.
Thanks! https://codereview.chromium.org/2023093002/diff/400001/chrome/browser/ui/view... File chrome/browser/ui/views/profiles/profile_chooser_view.cc (right): https://codereview.chromium.org/2023093002/diff/400001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/profile_chooser_view.cc:409: canvas->DrawCircle(center_point, On 2016/06/28 23:32:09, Evan Stade wrote: > On 2016/06/28 14:59:22, Jane wrote: > > On 2016/06/28 03:02:49, Evan Stade wrote: > > > On 2016/06/27 18:08:56, Jane wrote: > > > > On 2016/06/24 18:25:26, Evan Stade wrote: > > > > > what is this? > > > > > > > > This is to draw the light blue circular background of the badge icon. > > > > > > I don't see a light blue bg in the mocks. All I see is white silhouettes on > a > > > grey bg. > > > > Oops, you are right, I totally missed that change. > > Btw, does this color also need to go into color_palette or no? It doesn't seem > > like a commonly used color. > > it already is in color_palette.h as kChromeIconGrey Sorry, I got confused because of an inconsistency in the mockup. Changed to kChromeIconGrey now. https://codereview.chromium.org/2023093002/diff/400001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/profile_chooser_view.cc:417: SkColorSetRGB(0, 0x66, 0xff)); On 2016/06/28 23:32:09, Evan Stade wrote: > On 2016/06/28 15:33:11, Jane wrote: > > On 2016/06/28 14:59:22, Jane wrote: > > > On 2016/06/28 03:02:49, Evan Stade wrote: > > > > this should almost certainly be something from color_palette.h > > > > > > (Changed it to white) > > > I couldn't find any defined SkColor for white in color_palette or anywhere > > else. > > > Should I add the definition over there? It doesn't seem to be used a lot. > > > > Actually, I think it makes sense for this color to be the same as the profile > > badge padding color, so I changed it to be > > ui::NativeTheme::kColorId_BubbleBackground. > > a) the definition for white is SK_ColorWHITE > b) instead of manually drawing a circle and then a bubble-bg-colored icon on > top, you should be just drawing an icon that has the circle and the silhouettes > built in. In other words, you should be using ACCOUNT_CHILD instead of > ACCOUNT_CHILD_INVERT (the former may not exist yet but is on go/icons) and > SUPERVISOR_ACCOUNT_CIRCLE (which doesn't exist in chrome or on go/icons, so you > should request it). You will also get rid of the weird alternate sizes because > the size difference will be built into the icons. Good suggestion on simplifying the code! I think I will have to request both of the icons, since the current account_child icon doesn't have a white border either. However, I think it's more reasonable to defer it to a later CL, with the main reason being that the turnaround time for icon request is >2 weeks according to go/materialiconinfo. Also, I think it makes sense for this CL to be just the feature implementation, while code improvements like this could be done later. This piece is also from current code, so it should work stably for now. I have put this as a TODO here and will submit a icon request asap. If you think it's ok to defer, I will also file a bug for this, and replace the icons once they are ready. Let me know what you think!
On 2016/06/29 15:39:36, Jane wrote: > Thanks! > > https://codereview.chromium.org/2023093002/diff/400001/chrome/browser/ui/view... > File chrome/browser/ui/views/profiles/profile_chooser_view.cc (right): > > https://codereview.chromium.org/2023093002/diff/400001/chrome/browser/ui/view... > chrome/browser/ui/views/profiles/profile_chooser_view.cc:409: > canvas->DrawCircle(center_point, > On 2016/06/28 23:32:09, Evan Stade wrote: > > On 2016/06/28 14:59:22, Jane wrote: > > > On 2016/06/28 03:02:49, Evan Stade wrote: > > > > On 2016/06/27 18:08:56, Jane wrote: > > > > > On 2016/06/24 18:25:26, Evan Stade wrote: > > > > > > what is this? > > > > > > > > > > This is to draw the light blue circular background of the badge icon. > > > > > > > > I don't see a light blue bg in the mocks. All I see is white silhouettes > on > > a > > > > grey bg. > > > > > > Oops, you are right, I totally missed that change. > > > Btw, does this color also need to go into color_palette or no? It doesn't > seem > > > like a commonly used color. > > > > it already is in color_palette.h as kChromeIconGrey > > Sorry, I got confused because of an inconsistency in the mockup. Changed to > kChromeIconGrey now. > > https://codereview.chromium.org/2023093002/diff/400001/chrome/browser/ui/view... > chrome/browser/ui/views/profiles/profile_chooser_view.cc:417: SkColorSetRGB(0, > 0x66, 0xff)); > On 2016/06/28 23:32:09, Evan Stade wrote: > > On 2016/06/28 15:33:11, Jane wrote: > > > On 2016/06/28 14:59:22, Jane wrote: > > > > On 2016/06/28 03:02:49, Evan Stade wrote: > > > > > this should almost certainly be something from color_palette.h > > > > > > > > (Changed it to white) > > > > I couldn't find any defined SkColor for white in color_palette or anywhere > > > else. > > > > Should I add the definition over there? It doesn't seem to be used a lot. > > > > > > Actually, I think it makes sense for this color to be the same as the > profile > > > badge padding color, so I changed it to be > > > ui::NativeTheme::kColorId_BubbleBackground. > > > > a) the definition for white is SK_ColorWHITE > > b) instead of manually drawing a circle and then a bubble-bg-colored icon on > > top, you should be just drawing an icon that has the circle and the > silhouettes > > built in. In other words, you should be using ACCOUNT_CHILD instead of > > ACCOUNT_CHILD_INVERT (the former may not exist yet but is on go/icons) and > > SUPERVISOR_ACCOUNT_CIRCLE (which doesn't exist in chrome or on go/icons, so > you > > should request it). You will also get rid of the weird alternate sizes because > > the size difference will be built into the icons. > > Good suggestion on simplifying the code! I think I will have to request both of > the icons, since the current account_child icon doesn't have a white border > either. > However, I think it's more reasonable to defer it to a later CL, with the main > reason being that the turnaround time for icon request is >2 weeks according to > go/materialiconinfo. Also, I think it makes sense for this CL to be just the > feature implementation, while code improvements like this could be done later. > This piece is also from current code, so it should work stably for now. > I have put this as a TODO here and will submit a icon request asap. If you think > it's ok to defer, I will also file a bug for this, and replace the icons once > they are ready. > Let me know what you think! I appreciate the annoyance factor here in waiting for your new icons, but you should write the code as if you have the correct icons, then fix the icons later, rather than writing code to accommodate the lack of icons and fixing it up later. I don't think we should separate the notions of "feature implementation" and "good code" into two separate CLs. You don't need the icon to have the white background cooked in. That part can be drawn manually.
Point taken. The background would not be drawn for the profile badge in the new user menu any more. However, I hope it makes sense to still keep this code for the current user menu to maintain the current UI with the blue background.
thanks for bearing with this review. Mostly LG with a few minor changes left. https://codereview.chromium.org/2023093002/diff/540001/chrome/browser/ui/view... File chrome/browser/ui/views/profiles/profile_chooser_view.cc (right): https://codereview.chromium.org/2023093002/diff/540001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/profile_chooser_view.cc:392: // them are ready, which be inverted as silhouettes. can you elaborate a little on this TODO? Some indication of the code simplifications that will be possible with the new icons (since right now it just sounds like a simple icon swap). https://codereview.chromium.org/2023093002/diff/540001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/profile_chooser_view.cc:413: paint.setColor(SK_ColorWHITE); I liked the idea you had of using the theme's bubble bg color here instead of specifying white https://codereview.chromium.org/2023093002/diff/540001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/profile_chooser_view.cc:434: int badge_spacing = switches::IsMaterialDesignUserMenu() ? 4 : 0; these both need to be static functions rather than member variables (public member variables are banned in the style guide) https://codereview.chromium.org/2023093002/diff/540001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/profile_chooser_view.cc:696: gfx::ImageSkia CreateBadgeForProfile(Profile* profile) { is this still being used?
Thanks! https://codereview.chromium.org/2023093002/diff/540001/chrome/browser/ui/view... File chrome/browser/ui/views/profiles/profile_chooser_view.cc (right): https://codereview.chromium.org/2023093002/diff/540001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/profile_chooser_view.cc:392: // them are ready, which be inverted as silhouettes. On 2016/06/30 13:32:38, Evan Stade wrote: > can you elaborate a little on this TODO? Some indication of the code > simplifications that will be possible with the new icons (since right now it > just sounds like a simple icon swap). Done. https://codereview.chromium.org/2023093002/diff/540001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/profile_chooser_view.cc:413: paint.setColor(SK_ColorWHITE); On 2016/06/30 13:32:38, Evan Stade wrote: > I liked the idea you had of using the theme's bubble bg color here instead of > specifying white Done. https://codereview.chromium.org/2023093002/diff/540001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/profile_chooser_view.cc:434: int badge_spacing = switches::IsMaterialDesignUserMenu() ? 4 : 0; On 2016/06/30 13:32:38, Evan Stade wrote: > these both need to be static functions rather than member variables (public > member variables are banned in the style guide) Done. https://codereview.chromium.org/2023093002/diff/540001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/profile_chooser_view.cc:696: gfx::ImageSkia CreateBadgeForProfile(Profile* profile) { On 2016/06/30 13:32:38, Evan Stade wrote: > is this still being used? Done. Removed this and ProfileBadge class.
https://codereview.chromium.org/2023093002/diff/560001/chrome/browser/ui/view... File chrome/browser/ui/views/profiles/profile_chooser_view.cc (right): https://codereview.chromium.org/2023093002/diff/560001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/profile_chooser_view.cc:441: static int badge_spacing() { can you document the meaning of this one? https://codereview.chromium.org/2023093002/diff/560001/chrome/browser/ui/view... File chrome/browser/ui/views/profiles/profile_chooser_view.h (right): https://codereview.chromium.org/2023093002/diff/560001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/profile_chooser_view.h:69: static int GetFixedMenuWidth(); this doesn't need to be public. I'd just make it a file-local static in the .cc (in the anonymous namespace)
with those nits, lgtm
https://codereview.chromium.org/2023093002/diff/560001/chrome/browser/ui/view... File chrome/browser/ui/views/profiles/profile_chooser_view.cc (right): https://codereview.chromium.org/2023093002/diff/560001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/profile_chooser_view.cc:441: static int badge_spacing() { On 2016/06/30 14:36:45, Evan Stade wrote: > can you document the meaning of this one? Done. https://codereview.chromium.org/2023093002/diff/560001/chrome/browser/ui/view... File chrome/browser/ui/views/profiles/profile_chooser_view.h (right): https://codereview.chromium.org/2023093002/diff/560001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/profile_chooser_view.h:69: static int GetFixedMenuWidth(); On 2016/06/30 14:36:45, Evan Stade wrote: > this doesn't need to be public. I'd just make it a file-local static in the .cc > (in the anonymous namespace) Done.
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, sky@chromium.org, estade@chromium.org Link to the patchset: https://codereview.chromium.org/2023093002/#ps580001 (title: "Final nits")
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: linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
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, sky@chromium.org, estade@chromium.org Link to the patchset: https://codereview.chromium.org/2023093002/#ps600001 (title: "Test fix")
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
==========
Reflow of the profile items in desktop user menu. Specifically:
1. Moved and resized the avatar icon as well as icon badges
1. Moved avatar name up to be on the right of the profile icon
2. Moved username and "Your accounts" link up and below the avatar name
3. Other dimension and padding changes
See design doc here ("Reflow the items in the first section"):
https://docs.google.com/document/d/17yHrKd_EU6WoIAgljnZmNCSJLRlbM3O2Pxr7aPEnq...
See redlines here:
https://folio.googleplex.com/chrome-ux-specs-and-sources/Chrome%20Desktop%20S...
See screenshots here (marked with "3.*"):
https://drive.google.com/corp/drive/folders/0B7Fvv7JszRyGY2NrSEhheHptdG8
Downstream CL: https://codereview.chromium.org/2052473003/
BUG=615893
==========
to
==========
Reflow of the profile items in desktop user menu. Specifically:
1. Moved and resized the avatar icon as well as icon badges
1. Moved avatar name up to be on the right of the profile icon
2. Moved username and "Your accounts" link up and below the avatar name
3. Other dimension and padding changes
See design doc here ("Reflow the items in the first section"):
https://docs.google.com/document/d/17yHrKd_EU6WoIAgljnZmNCSJLRlbM3O2Pxr7aPEnq...
See redlines here:
https://folio.googleplex.com/chrome-ux-specs-and-sources/Chrome%20Desktop%20S...
See screenshots here (marked with "3.*"):
https://drive.google.com/corp/drive/folders/0B7Fvv7JszRyGY2NrSEhheHptdG8
Downstream CL: https://codereview.chromium.org/2052473003/
BUG=615893
==========
Message was sent while issue was closed.
Committed patchset #21 (id:600001)
Message was sent while issue was closed.
CQ bit was unchecked.
Message was sent while issue was closed.
Description was changed from
==========
Reflow of the profile items in desktop user menu. Specifically:
1. Moved and resized the avatar icon as well as icon badges
1. Moved avatar name up to be on the right of the profile icon
2. Moved username and "Your accounts" link up and below the avatar name
3. Other dimension and padding changes
See design doc here ("Reflow the items in the first section"):
https://docs.google.com/document/d/17yHrKd_EU6WoIAgljnZmNCSJLRlbM3O2Pxr7aPEnq...
See redlines here:
https://folio.googleplex.com/chrome-ux-specs-and-sources/Chrome%20Desktop%20S...
See screenshots here (marked with "3.*"):
https://drive.google.com/corp/drive/folders/0B7Fvv7JszRyGY2NrSEhheHptdG8
Downstream CL: https://codereview.chromium.org/2052473003/
BUG=615893
==========
to
==========
Reflow of the profile items in desktop user menu. Specifically:
1. Moved and resized the avatar icon as well as icon badges
1. Moved avatar name up to be on the right of the profile icon
2. Moved username and "Your accounts" link up and below the avatar name
3. Other dimension and padding changes
See design doc here ("Reflow the items in the first section"):
https://docs.google.com/document/d/17yHrKd_EU6WoIAgljnZmNCSJLRlbM3O2Pxr7aPEnq...
See redlines here:
https://folio.googleplex.com/chrome-ux-specs-and-sources/Chrome%20Desktop%20S...
See screenshots here (marked with "3.*"):
https://drive.google.com/corp/drive/folders/0B7Fvv7JszRyGY2NrSEhheHptdG8
Downstream CL: https://codereview.chromium.org/2052473003/
BUG=615893
Committed: https://crrev.com/f9860fa8491d8f794cfeaa0f05d7240d3d89357e
Cr-Commit-Position: refs/heads/master@{#403190}
==========
Message was sent while issue was closed.
Patchset 21 (id:??) landed as https://crrev.com/f9860fa8491d8f794cfeaa0f05d7240d3d89357e Cr-Commit-Position: refs/heads/master@{#403190} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
