Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1469423002:
Modify toolbar action bar layout for material design (Closed)
|
Created:
5 years ago by tdanderson Modified:
5 years ago CC:
chromium-reviews, tfarina Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionModify toolbar action bar layout for material design
Modifies the spacing between buttons in the
toolbar action bar (extensions) when material
design is enabled, and ensures that the area
directly to the right of the location bar
in LTR can be used to resize the action bar
width.
BUG=555031, 550816
TEST=manual
Committed: https://crrev.com/a1ce3730e40062ab80ec0faf52d6c876186a4c44
Cr-Commit-Position: refs/heads/master@{#364123}
Patch Set 1 #
Total comments: 36
Patch Set 2 : comments addressed #Patch Set 3 : rebase #
Total comments: 23
Patch Set 4 : more comments addressed #Messages
Total messages: 24 (7 generated)
Description was changed from ========== Modify toolbar action bar layout for material design WIP. BUG=555031,550816 TEST=manual ========== to ========== Modify toolbar action bar layout for material design Modifies the spacing between buttons in the toolbar action bar (extensions) when material design is enabled, and ensures that the area directly to the right of the location bar in LTR can be used to resize the action bar width. BUG=555031,550816 TEST=manual ==========
tdanderson@chromium.org changed reviewers: + pkasting@chromium.org, rdevlin.cronin@chromium.org
Devlin and Peter, can you please take a look? https://codereview.chromium.org/1469423002/diff/1/chrome/browser/ui/toolbar/t... File chrome/browser/ui/toolbar/toolbar_actions_bar.cc (right): https://codereview.chromium.org/1469423002/diff/1/chrome/browser/ui/toolbar/t... chrome/browser/ui/toolbar/toolbar_actions_bar.cc:62: int GetRightPadding() { Since all of these will return the same value (3, 4, or 8 for non-material, material, and material hybrid respectively) can we combine them into one?
leaving toolbar_view.* to Peter. https://codereview.chromium.org/1469423002/diff/1/chrome/browser/ui/toolbar/t... File chrome/browser/ui/toolbar/toolbar_actions_bar.cc (right): https://codereview.chromium.org/1469423002/diff/1/chrome/browser/ui/toolbar/t... chrome/browser/ui/toolbar/toolbar_actions_bar.cc:62: int GetRightPadding() { On 2015/11/24 19:34:31, tdanderson wrote: > Since all of these will return the same value (3, 4, or 8 for non-material, > material, and material hybrid respectively) can we combine them into one? Probably, with an updated comment saying something along the lines of "item spacing is the number of pixels between each item, between the start of the container and the first item, and the last item and the end of the container" (feel free to reword). https://codereview.chromium.org/1469423002/diff/1/chrome/browser/ui/toolbar/t... chrome/browser/ui/toolbar/toolbar_actions_bar.cc:183: return GetIconDimension(WIDTH) + (include_padding ? GetItemSpacing() : 0); nit: this should probably just use platform_settings_.item_spacing. https://codereview.chromium.org/1469423002/diff/1/chrome/browser/ui/toolbar/t... chrome/browser/ui/toolbar/toolbar_actions_bar.cc:226: return GetLeftPadding(); ditto - just use platform_settings_ (and below) https://codereview.chromium.org/1469423002/diff/1/chrome/browser/ui/views/too... File chrome/browser/ui/views/toolbar/browser_actions_container.cc (right): https://codereview.chromium.org/1469423002/diff/1/chrome/browser/ui/views/too... chrome/browser/ui/views/toolbar/browser_actions_container.cc:52: return GetLayoutConstant(TOOLBAR_STANDARD_SPACING) - 2; GetLayoutConstant(TOOLBAR_STANDARD_SPACING) is used a lot. Maybe worth caching here for efficiency and readability? https://codereview.chromium.org/1469423002/diff/1/chrome/browser/ui/views/too... File chrome/browser/ui/views/toolbar/browser_actions_container.h (right): https://codereview.chromium.org/1469423002/diff/1/chrome/browser/ui/views/too... chrome/browser/ui/views/toolbar/browser_actions_container.h:60: // _: GetItemSpacing() pixels of empty space. "GetItemSpacing()" isn't very specific here. Maybe ToolbarActionsBar::PlatformSettings::item_spacing? Side note: this comment is a) waaaaaaay too long, b) probably already out of date. But a can be fixed separately, and we shouldn't make b worse :)
https://codereview.chromium.org/1469423002/diff/1/chrome/browser/ui/toolbar/t... File chrome/browser/ui/toolbar/toolbar_actions_bar.cc (right): https://codereview.chromium.org/1469423002/diff/1/chrome/browser/ui/toolbar/t... chrome/browser/ui/toolbar/toolbar_actions_bar.cc:43: // Matches GetLayoutConstant(TOOLBAR_STANDARD_SPACING). Nuke all these constants and all the helpers below and replace everything in this file that references them with direct calls to GetLayoutConstant(TOOLBAR_STANDARD_SPACING). https://codereview.chromium.org/1469423002/diff/1/chrome/browser/ui/toolbar/t... chrome/browser/ui/toolbar/toolbar_actions_bar.cc:82: if (ui::MaterialDesignController::IsModeMaterial()) { The old browser_action_normal.png was square, so this function always used to return the same value whether WIDTH or HEIGHT was passed to it. Given this, the following changes will simplify this and still work: * Nuke the DimensionType enum. * Rename this to GetIconSize(). * Instead of using IDR_BROWSER_ACTION below, use IDR_BROWSER_ACTIONS_OVERFLOW. * Always return the height of that image. * Eliminate the MD-specific codepath you're adding here as that icon's height is 28 in MD. https://codereview.chromium.org/1469423002/diff/1/chrome/browser/ui/views/too... File chrome/browser/ui/views/toolbar/browser_actions_container.cc (right): https://codereview.chromium.org/1469423002/diff/1/chrome/browser/ui/views/too... chrome/browser/ui/views/toolbar/browser_actions_container.cc:50: // and vertical spacing as the other action buttons. Where does the magic '2' come from below? It's hard to know what that means especially in light of this comment that seems to suggest it shouldn't be there. https://codereview.chromium.org/1469423002/diff/1/chrome/browser/ui/views/too... chrome/browser/ui/views/toolbar/browser_actions_container.cc:52: return GetLayoutConstant(TOOLBAR_STANDARD_SPACING) - 2; On 2015/11/24 21:17:11, Devlin wrote: > GetLayoutConstant(TOOLBAR_STANDARD_SPACING) is used a lot. Maybe worth caching > here for efficiency and readability? No. That call is a very simple operation. Don't cache this as the efficiency win is irrelevant and readability is actually lowered by doing so. https://codereview.chromium.org/1469423002/diff/1/chrome/browser/ui/views/too... chrome/browser/ui/views/toolbar/browser_actions_container.cc:478: // GetLayoutConstant(TOOLBAR_STANDARD_SPACING). If we're right-to-left, we Nit: You can probably say "the standard toolbar spacing" here, or even just remove the last clause in this sentence entirely as the code is fairly clear. https://codereview.chromium.org/1469423002/diff/1/chrome/browser/ui/views/too... File chrome/browser/ui/views/toolbar/browser_actions_container.h (right): https://codereview.chromium.org/1469423002/diff/1/chrome/browser/ui/views/too... chrome/browser/ui/views/toolbar/browser_actions_container.h:59: // wide as the IDR_BROWSER_ACTION image. Nit: If you make my suggested changes elsewhere, this can be "An icon. This is treated as a square whose side is the height of IDR_BROWSER_ACTIONS_OVERFLOW." https://codereview.chromium.org/1469423002/diff/1/chrome/browser/ui/views/too... chrome/browser/ui/views/toolbar/browser_actions_container.h:60: // _: GetItemSpacing() pixels of empty space. On 2015/11/24 21:17:11, Devlin wrote: > "GetItemSpacing()" isn't very specific here. Maybe > ToolbarActionsBar::PlatformSettings::item_spacing? > > Side note: this comment is a) waaaaaaay too long, b) probably already out of > date. But a can be fixed separately, and we shouldn't make b worse :) Disagree that the comment is too long at all (let alone "waaaaaay"). The comment is informative and useful. https://codereview.chromium.org/1469423002/diff/1/chrome/browser/ui/views/too... File chrome/browser/ui/views/toolbar/toolbar_view.cc (right): https://codereview.chromium.org/1469423002/diff/1/chrome/browser/ui/views/too... chrome/browser/ui/views/toolbar/toolbar_view.cc:495: (browser_actions_width == 0 ? right_padding : 0) + You reversed the behavior here and two places below, and I don't understand why. https://codereview.chromium.org/1469423002/diff/1/chrome/browser/ui/views/too... chrome/browser/ui/views/toolbar/toolbar_view.cc:590: browser_actions_->GetPreferredSize().width(); Nit: Might as well just inline this as it's only used once below. https://codereview.chromium.org/1469423002/diff/1/chrome/browser/ui/views/too... chrome/browser/ui/views/toolbar/toolbar_view.cc:599: Nit: No blank line here
Peter and Devlin, can you please take another look? https://codereview.chromium.org/1469423002/diff/1/chrome/browser/ui/toolbar/t... File chrome/browser/ui/toolbar/toolbar_actions_bar.cc (right): https://codereview.chromium.org/1469423002/diff/1/chrome/browser/ui/toolbar/t... chrome/browser/ui/toolbar/toolbar_actions_bar.cc:43: // Matches GetLayoutConstant(TOOLBAR_STANDARD_SPACING). On 2015/11/24 23:07:46, Peter Kasting wrote: > Nuke all these constants and all the helpers below and replace everything in > this file that references them with direct calls to > GetLayoutConstant(TOOLBAR_STANDARD_SPACING). This file is platform-independent, which IIUC means that we can't (or at least shouldn't) #include c/b/u/views/layout_constants.h since Mac uses this file. https://codereview.chromium.org/1469423002/diff/1/chrome/browser/ui/toolbar/t... chrome/browser/ui/toolbar/toolbar_actions_bar.cc:62: int GetRightPadding() { On 2015/11/24 21:17:11, Devlin wrote: > On 2015/11/24 19:34:31, tdanderson wrote: > > Since all of these will return the same value (3, 4, or 8 for non-material, > > material, and material hybrid respectively) can we combine them into one? > > Probably, with an updated comment saying something along the lines of "item > spacing is the number of pixels between each item, between the start of the > container and the first item, and the last item and the end of the container" > (feel free to reword). Done. https://codereview.chromium.org/1469423002/diff/1/chrome/browser/ui/toolbar/t... chrome/browser/ui/toolbar/toolbar_actions_bar.cc:82: if (ui::MaterialDesignController::IsModeMaterial()) { On 2015/11/24 23:07:46, Peter Kasting wrote: > The old browser_action_normal.png was square, so this function always used to > return the same value whether WIDTH or HEIGHT was passed to it. > > Given this, the following changes will simplify this and still work: > * Nuke the DimensionType enum. > * Rename this to GetIconSize(). > * Instead of using IDR_BROWSER_ACTION below, use IDR_BROWSER_ACTIONS_OVERFLOW. > * Always return the height of that image. > * Eliminate the MD-specific codepath you're adding here as that icon's height is > 28 in MD. I would rather not make the changes you suggest for two reasons. First, independent of MD the chevron is being phased out in favor of the new action bar design, and your change would add dependency on the chevron asset. Second, in MD we have been trying to avoid relying on image assets to tell us size values which are better specified as constants. https://codereview.chromium.org/1469423002/diff/1/chrome/browser/ui/toolbar/t... chrome/browser/ui/toolbar/toolbar_actions_bar.cc:183: return GetIconDimension(WIDTH) + (include_padding ? GetItemSpacing() : 0); On 2015/11/24 21:17:11, Devlin wrote: > nit: this should probably just use platform_settings_.item_spacing. For this particular instance I am leaving it as-is since IconWidth() is static. https://codereview.chromium.org/1469423002/diff/1/chrome/browser/ui/toolbar/t... chrome/browser/ui/toolbar/toolbar_actions_bar.cc:226: return GetLeftPadding(); On 2015/11/24 21:17:11, Devlin wrote: > ditto - just use platform_settings_ (and below) Done. https://codereview.chromium.org/1469423002/diff/1/chrome/browser/ui/views/too... File chrome/browser/ui/views/toolbar/browser_actions_container.cc (right): https://codereview.chromium.org/1469423002/diff/1/chrome/browser/ui/views/too... chrome/browser/ui/views/toolbar/browser_actions_container.cc:50: // and vertical spacing as the other action buttons. On 2015/11/24 23:07:46, Peter Kasting wrote: > Where does the magic '2' come from below? It's hard to know what that means > especially in light of this comment that seems to suggest it shouldn't be there. I'm just carrying it over to avoid breaking any existing behavior in non-MD (this gives TOOLBAR_STANDARD_SPACING - 2 = 1 px of spacing before the chevron). It is likely that this TODO will become obsolete before MD is ready, since the redesign of the browser actions container does not use a chevron. https://codereview.chromium.org/1469423002/diff/1/chrome/browser/ui/views/too... chrome/browser/ui/views/toolbar/browser_actions_container.cc:52: return GetLayoutConstant(TOOLBAR_STANDARD_SPACING) - 2; On 2015/11/24 23:07:46, Peter Kasting wrote: > On 2015/11/24 21:17:11, Devlin wrote: > > GetLayoutConstant(TOOLBAR_STANDARD_SPACING) is used a lot. Maybe worth caching > > here for efficiency and readability? > > No. That call is a very simple operation. Don't cache this as the efficiency > win is irrelevant and readability is actually lowered by doing so. Leaving this as-is. https://codereview.chromium.org/1469423002/diff/1/chrome/browser/ui/views/too... chrome/browser/ui/views/toolbar/browser_actions_container.cc:478: // GetLayoutConstant(TOOLBAR_STANDARD_SPACING). If we're right-to-left, we On 2015/11/24 23:07:46, Peter Kasting wrote: > Nit: You can probably say "the standard toolbar spacing" here, or even just > remove the last clause in this sentence entirely as the code is fairly clear. Done. https://codereview.chromium.org/1469423002/diff/1/chrome/browser/ui/views/too... File chrome/browser/ui/views/toolbar/browser_actions_container.h (right): https://codereview.chromium.org/1469423002/diff/1/chrome/browser/ui/views/too... chrome/browser/ui/views/toolbar/browser_actions_container.h:59: // wide as the IDR_BROWSER_ACTION image. On 2015/11/24 23:07:47, Peter Kasting wrote: > Nit: If you make my suggested changes elsewhere, this can be "An icon. This is > treated as a square whose side is the height of IDR_BROWSER_ACTIONS_OVERFLOW." I have replied to your other comment. https://codereview.chromium.org/1469423002/diff/1/chrome/browser/ui/views/too... chrome/browser/ui/views/toolbar/browser_actions_container.h:60: // _: GetItemSpacing() pixels of empty space. On 2015/11/24 23:07:46, Peter Kasting wrote: > On 2015/11/24 21:17:11, Devlin wrote: > > "GetItemSpacing()" isn't very specific here. Maybe > > ToolbarActionsBar::PlatformSettings::item_spacing? > > > > Side note: this comment is a) waaaaaaay too long, b) probably already out of > > date. But a can be fixed separately, and we shouldn't make b worse :) > > Disagree that the comment is too long at all (let alone "waaaaaay"). The > comment is informative and useful. Comment changed, and I believe I have updated the comment wherever necessary to keep it up to date as of this CL. https://codereview.chromium.org/1469423002/diff/1/chrome/browser/ui/views/too... File chrome/browser/ui/views/toolbar/toolbar_view.cc (right): https://codereview.chromium.org/1469423002/diff/1/chrome/browser/ui/views/too... chrome/browser/ui/views/toolbar/toolbar_view.cc:495: (browser_actions_width == 0 ? right_padding : 0) + On 2015/11/24 23:07:47, Peter Kasting wrote: > You reversed the behavior here and two places below, and I don't understand why. For the purposes of explanation, let: L = location bar r = TOOLBAR_LOCATION_BAR_RIGHT_PADDING = {0, 4, 8} C = browser actions container A = app menu button e = TOOLBAR_ELEMENT_PADDING = {0, 0, 8} The existing layout is LrCeA if C is non-empty, and LrA if C is empty. Note that C currently has leftmost and rightmost padding of 3 in all cases. In this CL I am changing the leftmost and rightmost padding within C to be {3, 4, 8} for non-material, material, hybrid respectively (in toolbar_actions_bar.cc). As a result, this CL changes the layout to be LCA if C is non-empty, and LrA if C is empty. This guarantees the correct spacing in both cases and at the same time allows resizing the actions container from the leftmost edge of C (or what the user would perceive to be the rightmost edge of L). https://codereview.chromium.org/1469423002/diff/1/chrome/browser/ui/views/too... chrome/browser/ui/views/toolbar/toolbar_view.cc:590: browser_actions_->GetPreferredSize().width(); On 2015/11/24 23:07:47, Peter Kasting wrote: > Nit: Might as well just inline this as it's only used once below. Done. https://codereview.chromium.org/1469423002/diff/1/chrome/browser/ui/views/too... chrome/browser/ui/views/toolbar/toolbar_view.cc:599: On 2015/11/24 23:07:47, Peter Kasting wrote: > Nit: No blank line here Done.
https://codereview.chromium.org/1469423002/diff/1/chrome/browser/ui/toolbar/t... File chrome/browser/ui/toolbar/toolbar_actions_bar.cc (right): https://codereview.chromium.org/1469423002/diff/1/chrome/browser/ui/toolbar/t... chrome/browser/ui/toolbar/toolbar_actions_bar.cc:43: // Matches GetLayoutConstant(TOOLBAR_STANDARD_SPACING). On 2015/11/26 17:04:01, tdanderson wrote: > On 2015/11/24 23:07:46, Peter Kasting wrote: > > Nuke all these constants and all the helpers below and replace everything in > > this file that references them with direct calls to > > GetLayoutConstant(TOOLBAR_STANDARD_SPACING). > > This file is platform-independent, which IIUC means that we can't (or at least > shouldn't) #include c/b/u/views/layout_constants.h since Mac uses this file. Then we should do one of the following to make this available: * Move layout_constants.h from c/b/ui/views/ to c/b/ui/ * Split this constant out of the views-specific file and put it in a new c/b/ui/-level file. This requires figuring out how to name things so as not to have conflicts. We'd have needed to solve this eventually anyway, so we might as well solve it now. The first bullet is easier, though if it publicizes things that should only ever be views-specific it might arguably be considered a layering violation. The second bullet could be done in a variety of ways; one way might be: In c/b/ui/layout_constants.h: enum LayoutConstant { TOOLBAR_STANDARD_SPACING, ... NUM_LAYOUT_CONSTANTS, }; int GetLayoutConstant(int constant_id); In c/b/ui/layout_constants.cc: #include "chrome/browser/ui/layout_constants.h" ... #if defined(TOOLKIT_VIEWS) #include "chrome/browser/ui/views/layout_constants.h" #endif int GetLayoutConstant(int constant id) { switch (constant_id) { case TOOLBAR_STANDARD_SPACING: return ...; ... } #if defined(TOOLKIT_VIEWS) return GetViewsLayoutConstant(constant_id); #else NOTREACHED(); return 0; #endif } In c/b/ui/views/layout_constants.h: #include "chrome/browser/ui/layout_constants.h" enum ViewsLayoutConstant { XXX_BLAH = NUM_LAYOUT_CONSTANTS, ... } int GetViewsLayoutConstant(int constant_id); c/b/ui/views/layout_constants.cc would be implemented as you'd expect. This would allow callers to just #include and call the cross-platform function, without exposing views-specific constants to non-views callers either in the header or the implementation. https://codereview.chromium.org/1469423002/diff/1/chrome/browser/ui/toolbar/t... chrome/browser/ui/toolbar/toolbar_actions_bar.cc:82: if (ui::MaterialDesignController::IsModeMaterial()) { On 2015/11/26 17:04:01, tdanderson wrote: > On 2015/11/24 23:07:46, Peter Kasting wrote: > > The old browser_action_normal.png was square, so this function always used to > > return the same value whether WIDTH or HEIGHT was passed to it. > > > > Given this, the following changes will simplify this and still work: > > * Nuke the DimensionType enum. > > * Rename this to GetIconSize(). > > * Instead of using IDR_BROWSER_ACTION below, use IDR_BROWSER_ACTIONS_OVERFLOW. > > * Always return the height of that image. > > * Eliminate the MD-specific codepath you're adding here as that icon's height > is > > 28 in MD. > > I would rather not make the changes you suggest for two reasons. First, > independent of MD the chevron is being phased out in favor of the new action bar > design, and your change would add dependency on the chevron asset. Second, in MD > we have been trying to avoid relying on image assets to tell us size values > which are better specified as constants. If you are using an image at all, you should always get the size from the asset, and never specify the size as a constant which has to remain in-sync with the image. If you're not going to be using any images at all in MD for any of this, then hardcoding the constant is OK, but in that case why is this asset present in the MD folder? It would be nice to first make whatever design change eliminates that asset. https://codereview.chromium.org/1469423002/diff/1/chrome/browser/ui/views/too... File chrome/browser/ui/views/toolbar/toolbar_view.cc (right): https://codereview.chromium.org/1469423002/diff/1/chrome/browser/ui/views/too... chrome/browser/ui/views/toolbar/toolbar_view.cc:495: (browser_actions_width == 0 ? right_padding : 0) + On 2015/11/26 17:04:02, tdanderson wrote: > On 2015/11/24 23:07:47, Peter Kasting wrote: > > You reversed the behavior here and two places below, and I don't understand > why. > > For the purposes of explanation, let: > > L = location bar > r = TOOLBAR_LOCATION_BAR_RIGHT_PADDING = {0, 4, 8} > C = browser actions container > A = app menu button > e = TOOLBAR_ELEMENT_PADDING = {0, 0, 8} > > The existing layout is LrCeA if C is non-empty, and LrA if C is empty. Note that > C currently has leftmost and rightmost padding of 3 in all cases. > > In this CL I am changing the leftmost and rightmost padding within C to be {3, > 4, 8} for non-material, material, hybrid respectively (in > toolbar_actions_bar.cc). As a result, this CL changes the layout to be LCA if C > is non-empty, and LrA if C is empty. This guarantees the correct spacing in both > cases and at the same time allows resizing the actions container from the > leftmost edge of C (or what the user would perceive to be the rightmost edge of > L). OK, so to be clear then, the total spacing around the browser actions in the material modes does change in this CL -- from (7, 3) to (4, 4) for non-hybrid and from (11, 11) to (8, 8) for hybrid. (I got these by adding the "3 in all cases" that used to be in C to the old r and e values.)
estade@chromium.org changed reviewers: + estade@chromium.org
https://codereview.chromium.org/1469423002/diff/1/chrome/browser/ui/toolbar/t... File chrome/browser/ui/toolbar/toolbar_actions_bar.cc (right): https://codereview.chromium.org/1469423002/diff/1/chrome/browser/ui/toolbar/t... chrome/browser/ui/toolbar/toolbar_actions_bar.cc:43: // Matches GetLayoutConstant(TOOLBAR_STANDARD_SPACING). On 2015/11/30 23:52:14, Peter Kasting wrote: > On 2015/11/26 17:04:01, tdanderson wrote: > > On 2015/11/24 23:07:46, Peter Kasting wrote: > > > Nuke all these constants and all the helpers below and replace everything in > > > this file that references them with direct calls to > > > GetLayoutConstant(TOOLBAR_STANDARD_SPACING). > > > > This file is platform-independent, which IIUC means that we can't (or at least > > shouldn't) #include c/b/u/views/layout_constants.h since Mac uses this file. > > Then we should do one of the following to make this available: > > * Move layout_constants.h from c/b/ui/views/ to c/b/ui/ > * Split this constant out of the views-specific file and put it in a new > c/b/ui/-level file. This requires figuring out how to name things so as not to > have conflicts. > > We'd have needed to solve this eventually anyway, so we might as well solve it > now. The first bullet is easier, though if it publicizes things that should > only ever be views-specific it might arguably be considered a layering > violation. The second bullet could be done in a variety of ways; one way might > be: > > In c/b/ui/layout_constants.h: > > enum LayoutConstant { > TOOLBAR_STANDARD_SPACING, > ... > NUM_LAYOUT_CONSTANTS, > }; > > int GetLayoutConstant(int constant_id); > > > In c/b/ui/layout_constants.cc: > > #include "chrome/browser/ui/layout_constants.h" > > ... > #if defined(TOOLKIT_VIEWS) > #include "chrome/browser/ui/views/layout_constants.h" > #endif > > int GetLayoutConstant(int constant id) { > switch (constant_id) { > case TOOLBAR_STANDARD_SPACING: > return ...; > ... > } > #if defined(TOOLKIT_VIEWS) > return GetViewsLayoutConstant(constant_id); > #else > NOTREACHED(); > return 0; > #endif > } > > > In c/b/ui/views/layout_constants.h: > > #include "chrome/browser/ui/layout_constants.h" > > enum ViewsLayoutConstant { > XXX_BLAH = NUM_LAYOUT_CONSTANTS, > ... > } > > int GetViewsLayoutConstant(int constant_id); > > > c/b/ui/views/layout_constants.cc would be implemented as you'd expect. > > This would allow callers to just #include and call the cross-platform function, > without exposing views-specific constants to non-views callers either in the > header or the implementation. Peter and I chatted and I'm going to move the file for now (I also need it in non-views code). We can address the platform specific constants problem as a follow up.
Peter can you please take another look? https://codereview.chromium.org/1469423002/diff/1/chrome/browser/ui/toolbar/t... File chrome/browser/ui/toolbar/toolbar_actions_bar.cc (right): https://codereview.chromium.org/1469423002/diff/1/chrome/browser/ui/toolbar/t... chrome/browser/ui/toolbar/toolbar_actions_bar.cc:43: // Matches GetLayoutConstant(TOOLBAR_STANDARD_SPACING). On 2015/12/01 23:00:55, Evan Stade wrote: > On 2015/11/30 23:52:14, Peter Kasting wrote: > > On 2015/11/26 17:04:01, tdanderson wrote: > > > On 2015/11/24 23:07:46, Peter Kasting wrote: > > > > Nuke all these constants and all the helpers below and replace everything > in > > > > this file that references them with direct calls to > > > > GetLayoutConstant(TOOLBAR_STANDARD_SPACING). > > > > > > This file is platform-independent, which IIUC means that we can't (or at > least > > > shouldn't) #include c/b/u/views/layout_constants.h since Mac uses this file. > > > > Then we should do one of the following to make this available: > > > > * Move layout_constants.h from c/b/ui/views/ to c/b/ui/ > > * Split this constant out of the views-specific file and put it in a new > > c/b/ui/-level file. This requires figuring out how to name things so as not > to > > have conflicts. > > > > We'd have needed to solve this eventually anyway, so we might as well solve it > > now. The first bullet is easier, though if it publicizes things that should > > only ever be views-specific it might arguably be considered a layering > > violation. The second bullet could be done in a variety of ways; one way > might > > be: > > > > In c/b/ui/layout_constants.h: > > > > enum LayoutConstant { > > TOOLBAR_STANDARD_SPACING, > > ... > > NUM_LAYOUT_CONSTANTS, > > }; > > > > int GetLayoutConstant(int constant_id); > > > > > > In c/b/ui/layout_constants.cc: > > > > #include "chrome/browser/ui/layout_constants.h" > > > > ... > > #if defined(TOOLKIT_VIEWS) > > #include "chrome/browser/ui/views/layout_constants.h" > > #endif > > > > int GetLayoutConstant(int constant id) { > > switch (constant_id) { > > case TOOLBAR_STANDARD_SPACING: > > return ...; > > ... > > } > > #if defined(TOOLKIT_VIEWS) > > return GetViewsLayoutConstant(constant_id); > > #else > > NOTREACHED(); > > return 0; > > #endif > > } > > > > > > In c/b/ui/views/layout_constants.h: > > > > #include "chrome/browser/ui/layout_constants.h" > > > > enum ViewsLayoutConstant { > > XXX_BLAH = NUM_LAYOUT_CONSTANTS, > > ... > > } > > > > int GetViewsLayoutConstant(int constant_id); > > > > > > c/b/ui/views/layout_constants.cc would be implemented as you'd expect. > > > > This would allow callers to just #include and call the cross-platform > function, > > without exposing views-specific constants to non-views callers either in the > > header or the implementation. > > Peter and I chatted and I'm going to move the file for now (I also need it in > non-views code). We can address the platform specific constants problem as a > follow up. I am going to leave this alone and would like to land it as-is in this CL. How to handle this in the general case is being tracked by Peter in crbug.com/564340 https://codereview.chromium.org/1469423002/diff/1/chrome/browser/ui/toolbar/t... chrome/browser/ui/toolbar/toolbar_actions_bar.cc:82: if (ui::MaterialDesignController::IsModeMaterial()) { On 2015/11/30 23:52:14, Peter Kasting wrote: > On 2015/11/26 17:04:01, tdanderson wrote: > > On 2015/11/24 23:07:46, Peter Kasting wrote: > > > The old browser_action_normal.png was square, so this function always used > to > > > return the same value whether WIDTH or HEIGHT was passed to it. > > > > > > Given this, the following changes will simplify this and still work: > > > * Nuke the DimensionType enum. > > > * Rename this to GetIconSize(). > > > * Instead of using IDR_BROWSER_ACTION below, use > IDR_BROWSER_ACTIONS_OVERFLOW. > > > * Always return the height of that image. > > > * Eliminate the MD-specific codepath you're adding here as that icon's > height > > is > > > 28 in MD. > > > > I would rather not make the changes you suggest for two reasons. First, > > independent of MD the chevron is being phased out in favor of the new action > bar > > design, and your change would add dependency on the chevron asset. Second, in > MD > > we have been trying to avoid relying on image assets to tell us size values > > which are better specified as constants. > > If you are using an image at all, you should always get the size from the asset, > and never specify the size as a constant which has to remain in-sync with the > image. > > If you're not going to be using any images at all in MD for any of this, then > hardcoding the constant is OK, but in that case why is this asset present in the > MD folder? It would be nice to first make whatever design change eliminates > that asset. The image asset is slated to be removed by estade@, and the change here was already made in https://codereview.chromium.org/1472863004 https://codereview.chromium.org/1469423002/diff/1/chrome/browser/ui/views/too... File chrome/browser/ui/views/toolbar/toolbar_view.cc (right): https://codereview.chromium.org/1469423002/diff/1/chrome/browser/ui/views/too... chrome/browser/ui/views/toolbar/toolbar_view.cc:495: (browser_actions_width == 0 ? right_padding : 0) + On 2015/11/30 23:52:14, Peter Kasting wrote: > On 2015/11/26 17:04:02, tdanderson wrote: > > On 2015/11/24 23:07:47, Peter Kasting wrote: > > > You reversed the behavior here and two places below, and I don't understand > > why. > > > > For the purposes of explanation, let: > > > > L = location bar > > r = TOOLBAR_LOCATION_BAR_RIGHT_PADDING = {0, 4, 8} > > C = browser actions container > > A = app menu button > > e = TOOLBAR_ELEMENT_PADDING = {0, 0, 8} > > > > The existing layout is LrCeA if C is non-empty, and LrA if C is empty. Note > that > > C currently has leftmost and rightmost padding of 3 in all cases. > > > > In this CL I am changing the leftmost and rightmost padding within C to be {3, > > 4, 8} for non-material, material, hybrid respectively (in > > toolbar_actions_bar.cc). As a result, this CL changes the layout to be LCA if > C > > is non-empty, and LrA if C is empty. This guarantees the correct spacing in > both > > cases and at the same time allows resizing the actions container from the > > leftmost edge of C (or what the user would perceive to be the rightmost edge > of > > L). > > OK, so to be clear then, the total spacing around the browser actions in the > material modes does change in this CL -- from (7, 3) to (4, 4) for non-hybrid > and from (11, 11) to (8, 8) for hybrid. (I got these by adding the "3 in all > cases" that used to be in C to the old r and e values.) Yes, that is correct, this CL does adjust the spacing as you describe for material and material hybrid (which brings it in line with the spec).
LGTM https://codereview.chromium.org/1469423002/diff/40001/chrome/browser/ui/toolb... File chrome/browser/ui/toolbar/toolbar_actions_bar.cc (right): https://codereview.chromium.org/1469423002/diff/40001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/toolbar_actions_bar.cc:41: // Matches GetLayoutConstant(TOOLBAR_STANDARD_SPACING). While I'm on the hook for the long-term solution here, I think you should do the short-term thing Evan was going to do, which is to just move layout_constants.* to chrome/browser/ui/. Then you can go ahead and call this function directly here. You're welcome to do that as a separate change. https://codereview.chromium.org/1469423002/diff/40001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/toolbar_actions_bar.cc:222: const int side_padding = platform_settings_.item_spacing * 2; Nit: I'd probably just inline this below. https://codereview.chromium.org/1469423002/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/toolbar/toolbar_view.cc (right): https://codereview.chromium.org/1469423002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/toolbar_view.cc:482: GetLayoutConstant(TOOLBAR_LOCATION_BAR_RIGHT_PADDING); Tiny nit: Define this below |browser_actions_width| to match the order they're used in the next statement (2 places)
lgtm https://codereview.chromium.org/1469423002/diff/40001/chrome/browser/ui/toolb... File chrome/browser/ui/toolbar/toolbar_actions_bar.cc (right): https://codereview.chromium.org/1469423002/diff/40001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/toolbar_actions_bar.cc:213: const bool display_chevron = nit: IMO, having POD like this be const is overkill, and doesn't really ever help much (it just means that when someone does want it to change, they need to take it out). Unless there's reason to do it, I'd just have these stick as bool, int. https://codereview.chromium.org/1469423002/diff/40001/chrome/browser/ui/toolb... File chrome/browser/ui/toolbar/toolbar_actions_bar_unittest.cc (right): https://codereview.chromium.org/1469423002/diff/40001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/toolbar_actions_bar_unittest.cc:209: 3 * ToolbarActionsBar::IconWidth(true) + platform_settings.item_spacing; nit: for these, a super-short comment explaining why the calculation is correct is helpful. https://codereview.chromium.org/1469423002/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/toolbar/browser_actions_container.cc (right): https://codereview.chromium.org/1469423002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/browser_actions_container.cc:50: // and vertical spacing as the other action buttons. My hope is that the chevron is gone before MD launches, but we'll see. https://codereview.chromium.org/1469423002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/browser_actions_container.cc:716: const int drop_icon_x = GetLayoutConstant(TOOLBAR_STANDARD_SPACING) + ditto on the consts. https://codereview.chromium.org/1469423002/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/toolbar/toolbar_view.cc (right): https://codereview.chromium.org/1469423002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/toolbar_view.cc:593: (browser_actions_->GetPreferredSize().width() == 0 ? right_padding : 0) - A short comment explaining this calculation would be useful.
https://codereview.chromium.org/1469423002/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/toolbar/toolbar_view.cc (right): https://codereview.chromium.org/1469423002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/toolbar_view.cc:496: app_menu_button_->GetPreferredSize().width(); these two functions copy each other a lot. Can we share the code (perhaps with the use of fn pointers or base::Callbacks)? https://codereview.chromium.org/1469423002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/toolbar_view.cc:520: (browser_actions_width == 0 ? right_padding : 0) + nit: these two lines could perhaps be (browser_actions_width > 0 ? browser_actions_width : right_padding) + https://codereview.chromium.org/1469423002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/toolbar_view.cc:593: (browser_actions_->GetPreferredSize().width() == 0 ? right_padding : 0) - On 2015/12/08 22:13:44, Devlin wrote: > A short comment explaining this calculation would be useful. perhaps better as GetPreferredSize().IsEmpty()
https://codereview.chromium.org/1469423002/diff/40001/chrome/browser/ui/toolb... File chrome/browser/ui/toolbar/toolbar_actions_bar.cc (right): https://codereview.chromium.org/1469423002/diff/40001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/toolbar_actions_bar.cc:41: // Matches GetLayoutConstant(TOOLBAR_STANDARD_SPACING). On 2015/12/08 21:56:24, Peter Kasting wrote: > While I'm on the hook for the long-term solution here, I think you should do the > short-term thing Evan was going to do, which is to just move layout_constants.* > to chrome/browser/ui/. Then you can go ahead and call this function directly > here. > > You're welcome to do that as a separate change. Evan landed https://codereview.chromium.org/1508203003/ so I will instead call GetLayoutConstant() here. https://codereview.chromium.org/1469423002/diff/40001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/toolbar_actions_bar.cc:213: const bool display_chevron = On 2015/12/08 22:13:44, Devlin wrote: > nit: IMO, having POD like this be const is overkill, and doesn't really ever > help much (it just means that when someone does want it to change, they need to > take it out). Unless there's reason to do it, I'd just have these stick as > bool, int. IMO in general doing this improves readability. When tracing or debugging a method I like knowing upfront which locals can/will change without needing to scan through the entire body. It also guards against someone else accidentally or incorrectly mutating a variable that was originally not meant to change. And if they do want it to change, IMO the cost of removing 'const' does not outweigh the benefit of having it there in the first place. My preference would be to leave the const in here (and in browser_actions_container.cc). If you still disagree I'm happy to discuss further / change them back in a follow-up CL. https://codereview.chromium.org/1469423002/diff/40001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/toolbar_actions_bar.cc:222: const int side_padding = platform_settings_.item_spacing * 2; On 2015/12/08 21:56:24, Peter Kasting wrote: > Nit: I'd probably just inline this below. IMO inlining these three values would make the return statement unnecessarily hard to read, so I prefer to leave this as-is. I'm happy to discuss as a follow-up if you feel strongly otherwise. https://codereview.chromium.org/1469423002/diff/40001/chrome/browser/ui/toolb... File chrome/browser/ui/toolbar/toolbar_actions_bar_unittest.cc (right): https://codereview.chromium.org/1469423002/diff/40001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/toolbar_actions_bar_unittest.cc:209: 3 * ToolbarActionsBar::IconWidth(true) + platform_settings.item_spacing; On 2015/12/08 22:13:44, Devlin wrote: > nit: for these, a super-short comment explaining why the calculation is correct > is helpful. Done. https://codereview.chromium.org/1469423002/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/toolbar/browser_actions_container.cc (right): https://codereview.chromium.org/1469423002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/browser_actions_container.cc:50: // and vertical spacing as the other action buttons. On 2015/12/08 22:13:44, Devlin wrote: > My hope is that the chevron is gone before MD launches, but we'll see. Acknowledged, I'll keep an eye on this and fix the TODO if needed before MD launch. https://codereview.chromium.org/1469423002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/browser_actions_container.cc:716: const int drop_icon_x = GetLayoutConstant(TOOLBAR_STANDARD_SPACING) + On 2015/12/08 22:13:44, Devlin wrote: > ditto on the consts. See my reply to your original comment. https://codereview.chromium.org/1469423002/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/toolbar/toolbar_view.cc (right): https://codereview.chromium.org/1469423002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/toolbar_view.cc:482: GetLayoutConstant(TOOLBAR_LOCATION_BAR_RIGHT_PADDING); On 2015/12/08 21:56:25, Peter Kasting wrote: > Tiny nit: Define this below |browser_actions_width| to match the order they're > used in the next statement (2 places) Done. https://codereview.chromium.org/1469423002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/toolbar_view.cc:496: app_menu_button_->GetPreferredSize().width(); On 2015/12/09 01:05:10, Evan Stade wrote: > these two functions copy each other a lot. Can we share the code (perhaps with > the use of fn pointers or base::Callbacks)? I'll refactor in a follow-on CL once this one lands. https://codereview.chromium.org/1469423002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/toolbar_view.cc:520: (browser_actions_width == 0 ? right_padding : 0) + On 2015/12/09 01:05:09, Evan Stade wrote: > nit: these two lines could perhaps be > > (browser_actions_width > 0 ? browser_actions_width : right_padding) + Done. https://codereview.chromium.org/1469423002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/toolbar_view.cc:593: (browser_actions_->GetPreferredSize().width() == 0 ? right_padding : 0) - On 2015/12/09 01:05:09, Evan Stade wrote: > On 2015/12/08 22:13:44, Devlin wrote: > > A short comment explaining this calculation would be useful. > > perhaps better as GetPreferredSize().IsEmpty() Done and done.
The CQ bit was checked by tdanderson@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rdevlin.cronin@chromium.org, pkasting@chromium.org Link to the patchset: https://codereview.chromium.org/1469423002/#ps60001 (title: "more comments addressed")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1469423002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1469423002/60001
https://codereview.chromium.org/1469423002/diff/40001/chrome/browser/ui/toolb... File chrome/browser/ui/toolbar/toolbar_actions_bar.cc (right): https://codereview.chromium.org/1469423002/diff/40001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/toolbar_actions_bar.cc:213: const bool display_chevron = On 2015/12/09 18:22:58, tdanderson wrote: > On 2015/12/08 22:13:44, Devlin wrote: > > nit: IMO, having POD like this be const is overkill, and doesn't really ever > > help much (it just means that when someone does want it to change, they need > to > > take it out). Unless there's reason to do it, I'd just have these stick as > > bool, int. > > IMO in general doing this improves readability. When tracing or debugging a > method I like knowing upfront which locals can/will change without needing to > scan through the entire body. It also guards against someone else accidentally > or incorrectly mutating a variable that was originally not meant to change. And > if they do want it to change, IMO the cost of removing 'const' does not outweigh > the benefit of having it there in the first place. > > My preference would be to leave the const in here (and in > browser_actions_container.cc). If you still disagree I'm happy to discuss > further / change them back in a follow-up CL. TL;DR: I'd prefer them removed. The chance of someone "accidentally" modifying a POD is pretty slim, I think (the reason it's so beneficial for objects is that if I call Foo(), I may not know if Foo() has some side-effect - we should know that foo = bar modifies the value of foo :)). So in my opinion it's just making extra work for someone else down the road, and doesn't really add much in the way of readability. The style guide also encourages const in three main cases - function arguments by reference or ptr, methods, and data members; this doesn't fall into those categories.
Message was sent while issue was closed.
Description was changed from ========== Modify toolbar action bar layout for material design Modifies the spacing between buttons in the toolbar action bar (extensions) when material design is enabled, and ensures that the area directly to the right of the location bar in LTR can be used to resize the action bar width. BUG=555031,550816 TEST=manual ========== to ========== Modify toolbar action bar layout for material design Modifies the spacing between buttons in the toolbar action bar (extensions) when material design is enabled, and ensures that the area directly to the right of the location bar in LTR can be used to resize the action bar width. BUG=555031,550816 TEST=manual ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Modify toolbar action bar layout for material design Modifies the spacing between buttons in the toolbar action bar (extensions) when material design is enabled, and ensures that the area directly to the right of the location bar in LTR can be used to resize the action bar width. BUG=555031,550816 TEST=manual ========== to ========== Modify toolbar action bar layout for material design Modifies the spacing between buttons in the toolbar action bar (extensions) when material design is enabled, and ensures that the area directly to the right of the location bar in LTR can be used to resize the action bar width. BUG=555031,550816 TEST=manual Committed: https://crrev.com/a1ce3730e40062ab80ec0faf52d6c876186a4c44 Cr-Commit-Position: refs/heads/master@{#364123} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/a1ce3730e40062ab80ec0faf52d6c876186a4c44 Cr-Commit-Position: refs/heads/master@{#364123}
Message was sent while issue was closed.
https://codereview.chromium.org/1469423002/diff/40001/chrome/browser/ui/toolb... File chrome/browser/ui/toolbar/toolbar_actions_bar.cc (right): https://codereview.chromium.org/1469423002/diff/40001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/toolbar_actions_bar.cc:213: const bool display_chevron = On 2015/12/09 19:22:27, Devlin wrote: > On 2015/12/09 18:22:58, tdanderson wrote: > > On 2015/12/08 22:13:44, Devlin wrote: > > > nit: IMO, having POD like this be const is overkill, and doesn't really ever > > > help much (it just means that when someone does want it to change, they need > > to > > > take it out). Unless there's reason to do it, I'd just have these stick as > > > bool, int. > > > > IMO in general doing this improves readability. When tracing or debugging a > > method I like knowing upfront which locals can/will change without needing to > > scan through the entire body. It also guards against someone else accidentally > > or incorrectly mutating a variable that was originally not meant to change. > And > > if they do want it to change, IMO the cost of removing 'const' does not > outweigh > > the benefit of having it there in the first place. > > > > My preference would be to leave the const in here (and in > > browser_actions_container.cc). If you still disagree I'm happy to discuss > > further / change them back in a follow-up CL. > > TL;DR: I'd prefer them removed. > > The chance of someone "accidentally" modifying a POD is pretty slim, I think > (the reason it's so beneficial for objects is that if I call Foo(), I may not > know if Foo() has some side-effect - we should know that foo = bar modifies the > value of foo :)). So in my opinion it's just making extra work for someone else > down the road, and doesn't really add much in the way of readability. The style > guide also encourages const in three main cases - function arguments by > reference or ptr, methods, and data members; this doesn't fall into those > categories. OK, fair enough. I have removed them in: https://codereview.chromium.org/1505403003/ |
