Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1164333002:
ToolbarView MaterialDesign in ThemeProvider (Closed)
DescriptionThis change moves layout constants from ToolbarView into ThemeProvider.
In ThemeProvider we determine the layout value to return based on the Material Design runtime flags.
Based on design discussion: https://docs.google.com/a/google.com/document/d/1h2RFyplJ_cNm9shT0pmtYUATI0R75dXeM6HN22eX5-E/edit?usp=sharing
Subsequent reviews will update the rest of top browser chrome following this format.
BUG=505800
TEST=Manual testing on device
Committed: https://crrev.com/28aa1291fb4cfb88d62872174e11f806a557f039
Cr-Commit-Position: refs/heads/master@{#338049}
Patch Set 1 #
Total comments: 5
Patch Set 2 : #Patch Set 3 : Rebase #Patch Set 4 : Add in Hybrid #
Total comments: 3
Patch Set 5 : #
Total comments: 8
Patch Set 6 : #
Total comments: 2
Patch Set 7 : #
Total comments: 8
Patch Set 8 : Switch to arrays #
Total comments: 2
Patch Set 9 : #
Total comments: 5
Patch Set 10 : #
Total comments: 2
Patch Set 11 : Change Enum from a Class #Patch Set 12 : Rebase #
Messages
Total messages: 47 (10 generated)
tdanderson@ FYI
tdanderson@chromium.org changed reviewers: + tdanderson@chromium.org
Hey Jon, a couple of thoughts below: https://codereview.chromium.org/1164333002/diff/1/chrome/browser/themes/theme... File chrome/browser/themes/theme_properties.cc (right): https://codereview.chromium.org/1164333002/diff/1/chrome/browser/themes/theme... chrome/browser/themes/theme_properties.cc:116: const int kToolbarViewLeftEdgeSpacingMD = 8; For readability and accounting purposes, I'm tempted to suggest dropping the check for the compile-time flag here and just formatting this section as: // Description of value. const int kValue = 3; const int kValueMD = 5; // Description of value. ... https://codereview.chromium.org/1164333002/diff/1/chrome/browser/themes/theme... chrome/browser/themes/theme_properties.cc:175: return -1; This will be a problem if any of our constants are actually equal to -1.
https://codereview.chromium.org/1164333002/diff/1/chrome/browser/themes/theme... File chrome/browser/themes/theme_properties.cc (right): https://codereview.chromium.org/1164333002/diff/1/chrome/browser/themes/theme... chrome/browser/themes/theme_properties.cc:175: return -1; On 2015/06/09 15:32:38, tdanderson wrote: > This will be a problem if any of our constants are actually equal to -1. An example of a negative layout constant is kTabHorizontalOffset: https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...
https://codereview.chromium.org/1164333002/diff/1/chrome/browser/themes/theme... File chrome/browser/themes/theme_properties.cc (right): https://codereview.chromium.org/1164333002/diff/1/chrome/browser/themes/theme... chrome/browser/themes/theme_properties.cc:116: const int kToolbarViewLeftEdgeSpacingMD = 8; On 2015/06/09 15:32:38, tdanderson wrote: > For readability and accounting purposes, I'm tempted to suggest dropping the > check for the compile-time flag here and just formatting this section as: > > // Description of value. > const int kValue = 3; > const int kValueMD = 5; > > // Description of value. > ... I personally prefer the compiler check. It acts as a grouping delimiter. However I dislike the current lack of descriptions. https://codereview.chromium.org/1164333002/diff/1/chrome/browser/themes/theme... chrome/browser/themes/theme_properties.cc:175: return -1; On 2015/06/09 17:50:07, tdanderson wrote: > On 2015/06/09 15:32:38, tdanderson wrote: > > This will be a problem if any of our constants are actually equal to -1. > > An example of a negative layout constant is kTabHorizontalOffset: > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/... I've moved away from this. I've pulled the old parameters out into a separate method, and have the MD call it for the default case.
jonross@chromium.org changed reviewers: + bruthig@chromium.org
Hi, This change has been updated to use the MaterialDesignController. Could you PTAL?
https://codereview.chromium.org/1164333002/diff/60001/chrome/browser/themes/t... File chrome/browser/themes/theme_properties.cc (right): https://codereview.chromium.org/1164333002/diff/60001/chrome/browser/themes/t... chrome/browser/themes/theme_properties.cc:397: default: Is this default necessary? The switch statement is properly handling all values of MaterialDesignController::Mode. I discourage the silent failure here, if the default is necessary then I suggest adding a NOTREACHED() with a message.
https://codereview.chromium.org/1164333002/diff/60001/chrome/browser/themes/t... File chrome/browser/themes/theme_properties.cc (right): https://codereview.chromium.org/1164333002/diff/60001/chrome/browser/themes/t... chrome/browser/themes/theme_properties.cc:397: default: On 2015/07/02 18:14:57, bruthig wrote: > Is this default necessary? The switch statement is properly handling all values > of MaterialDesignController::Mode. I discourage the silent failure here, if the > default is necessary then I suggest adding a NOTREACHED() with a message. I'll add the NOTREACHED(), some builds throw compilation errors if default is not there.
https://codereview.chromium.org/1164333002/diff/60001/chrome/browser/themes/t... File chrome/browser/themes/theme_properties.cc (right): https://codereview.chromium.org/1164333002/diff/60001/chrome/browser/themes/t... chrome/browser/themes/theme_properties.cc:397: default: On 2015/07/02 18:24:02, jonross wrote: > On 2015/07/02 18:14:57, bruthig wrote: > > Is this default necessary? The switch statement is properly handling all > values > > of MaterialDesignController::Mode. I discourage the silent failure here, if > the > > default is necessary then I suggest adding a NOTREACHED() with a message. > > I'll add the NOTREACHED(), some builds throw compilation errors if default is > not there. Done.
lgtm
jonross@chromium.org changed reviewers: + pkotwicz@chromium.org
Hey Peter, Could you provide an owners review for the changes in chrome/browser/themes/ ? This is a first stage of implementing Material Design layout spacing. Future changes to top chrome will follow this pattern. Thanks, Jon
I just looked again at the doc w.r.t to material design images (https://docs.google.com/document/d/1KRnwsoldzT2VcoNpscnxaQ6RFGs3j_0SWa1BNUW75...). Was a resolution reached? I realize that my question is unrelated to this CL but I want to get a bit more background information
I'm going to do my best to describe the approach that tdanderson@ has moved forward with since he is away on vacation. The approach decided on is option 3 combined with a modified option 4. In short the difference from option 4 is that the ResourceBundle will possibly have the "chrome_material_(100|200)_percent.pak" files loaded* as well as* the "chrome_(100|200)_percent.pak" depending on the runtime material flag. If the material pak file is loaded the ResourceBundle will search them first when looking for an asset and fall back to the "chrome_(100|200)_percent.pak" files if it wasn't found. Here are the CL's if you're interested in the nitty gritty details: 1. 506332 <http://www.crbug.com/506332> "Allow flagging of DataPacks which contain only material design assets" 2. 504111 <http://www.crbug.com/504111> "Load material design pak files into ResourceBundle on ChromeOS" 3. 504106 <http://www.crbug.com/504106> "chromite: Include material design pak files in Chrome on ChromeOS" Let me know if you need some clarifications. On Fri, Jul 3, 2015 at 10:37 AM, <pkotwicz@chromium.org> wrote: > I just looked again at the doc w.r.t to material design images > ( > https://docs.google.com/document/d/1KRnwsoldzT2VcoNpscnxaQ6RFGs3j_0SWa1BNUW75... > ). > Was a resolution reached? > > I realize that my question is unrelated to this CL but I want to get a bit > more > background information > > https://codereview.chromium.org/1164333002/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Jon, thanks for pointing me to the doc that this CL relates to. Can you add a link to the doc in the CL description. Your CL looks good. A few minor comments https://codereview.chromium.org/1164333002/diff/80001/chrome/browser/themes/t... File chrome/browser/themes/theme_properties.cc (right): https://codereview.chromium.org/1164333002/diff/80001/chrome/browser/themes/t... chrome/browser/themes/theme_properties.cc:7: #include "base/command_line.h" I don't think this include is necessary https://codereview.chromium.org/1164333002/diff/80001/chrome/browser/themes/t... chrome/browser/themes/theme_properties.cc:397: default: Why the default statement? https://codereview.chromium.org/1164333002/diff/80001/chrome/browser/themes/t... File chrome/browser/themes/theme_properties.h (right): https://codereview.chromium.org/1164333002/diff/80001/chrome/browser/themes/t... chrome/browser/themes/theme_properties.h:117: // Layout Properties for the Toolbar Can you add a TODO to remove all of these properties from this file once Chrome is 100% switched over to material design? It might be useful if you started a new enum "MaterialDesignDisplayProperty" perhaps. The benefit is that in theme_properties.cc you could get rid of a few default cases in the switch statements. In particular: int ThemeProperties::GetDefaultDisplayProperty(int id) { if (id >= MATERIAL_DESIGN_DISPLAY_PROPERTY_FIRST && id <= MATERIAL_DESIGN_DISPLAY_PROPERTY_LAST) { MaterialDesignDisplayProperty material_property = static_cast<MaterialDesignDisplayProperty>(id); ... } ... }
Jon, thanks for pointing me to the doc that this CL relates to. Can you add a link to the doc in the CL description. Your CL looks good. A few minor comments
https://codereview.chromium.org/1164333002/diff/80001/chrome/browser/themes/t... File chrome/browser/themes/theme_properties.cc (right): https://codereview.chromium.org/1164333002/diff/80001/chrome/browser/themes/t... chrome/browser/themes/theme_properties.cc:397: default: On 2015/07/03 15:39:22, pkotwicz wrote: > Why the default statement? I've had some builds throw compilation errors if I don't include these. Even though it will never be reached. https://codereview.chromium.org/1164333002/diff/80001/chrome/browser/themes/t... File chrome/browser/themes/theme_properties.h (right): https://codereview.chromium.org/1164333002/diff/80001/chrome/browser/themes/t... chrome/browser/themes/theme_properties.h:117: // Layout Properties for the Toolbar On 2015/07/03 15:39:22, pkotwicz wrote: > Can you add a TODO to remove all of these properties from this file once Chrome > is 100% switched over to material design? > > It might be useful if you started a new enum "MaterialDesignDisplayProperty" > perhaps. The benefit is that in theme_properties.cc you could get rid of a few > default cases in the switch statements. In particular: > > int ThemeProperties::GetDefaultDisplayProperty(int id) { > if (id >= MATERIAL_DESIGN_DISPLAY_PROPERTY_FIRST && > id <= MATERIAL_DESIGN_DISPLAY_PROPERTY_LAST) { > MaterialDesignDisplayProperty material_property = > static_cast<MaterialDesignDisplayProperty>(id); > ... > } > ... > } These might remain after we complete the implementation and remove the current defaults. We plan to test a normal Material Design layout, along with one for touch centric CrOS devices. I can move these values to their own enum, however we will still be switching in GetDefaultDisplayProperties based on the current mode of the MaterialDesignController. So it is not just a direct switch->if change.
I think that a TODO is still useful. At the very least, we should evaluate whether some of the properties can be moved out of theme_properties.* after the switch over to material design is complete. https://codereview.chromium.org/1164333002/diff/80001/chrome/browser/themes/t... File chrome/browser/themes/theme_properties.cc (right): https://codereview.chromium.org/1164333002/diff/80001/chrome/browser/themes/t... chrome/browser/themes/theme_properties.cc:397: default: This could have to do with C++11 enum classes. We definitely have switch statements without the default case. e.g. ShelfLayoutManager::CalculateShelfVisibility()
https://codereview.chromium.org/1164333002/diff/80001/chrome/browser/themes/t... File chrome/browser/themes/theme_properties.cc (right): https://codereview.chromium.org/1164333002/diff/80001/chrome/browser/themes/t... chrome/browser/themes/theme_properties.cc:7: #include "base/command_line.h" On 2015/07/03 15:39:22, pkotwicz wrote: > I don't think this include is necessary Done. https://codereview.chromium.org/1164333002/diff/80001/chrome/browser/themes/t... chrome/browser/themes/theme_properties.cc:397: default: On 2015/07/03 17:34:01, pkotwicz wrote: > This could have to do with C++11 enum classes. We definitely have switch > statements without the default case. e.g. > ShelfLayoutManager::CalculateShelfVisibility() Done.
LGTM Can you please reference a bug ID in the CL description? https://codereview.chromium.org/1164333002/diff/100001/chrome/browser/themes/... File chrome/browser/themes/theme_properties.h (right): https://codereview.chromium.org/1164333002/diff/100001/chrome/browser/themes/... chrome/browser/themes/theme_properties.h:118: // which of these properties can be moved out of ThemeProperties. Nit: Can you please add a new line?
https://codereview.chromium.org/1164333002/diff/100001/chrome/browser/themes/... File chrome/browser/themes/theme_properties.h (right): https://codereview.chromium.org/1164333002/diff/100001/chrome/browser/themes/... chrome/browser/themes/theme_properties.h:118: // which of these properties can be moved out of ThemeProperties. On 2015/07/03 18:45:21, pkotwicz wrote: > Nit: Can you please add a new line? Done.
jonross@chromium.org changed reviewers: + pkasting@chromium.org
pkasting@chromium.org: Please review changes in chrome/browser/ui/views/toolbar/toolbar_view.cc This review is the first stage of parameterizing layout in the browser's top chrome. Selecting layout values based on the current Material Design state. Follow up changes will implement the layout changes throughout the rest of the top chrome. Could you provide an owners review? Thanks, Jon
https://codereview.chromium.org/1164333002/diff/120001/chrome/browser/themes/... File chrome/browser/themes/theme_properties.cc (right): https://codereview.chromium.org/1164333002/diff/120001/chrome/browser/themes/... chrome/browser/themes/theme_properties.cc:100: // Defaults for layout properties, which are not stored in the browser theme Nit: No comma https://codereview.chromium.org/1164333002/diff/120001/chrome/browser/themes/... chrome/browser/themes/theme_properties.cc:115: const int kToolbarViewLeftEdgeSpacingMD = 4; This file can be simpler and easier to maintain. Instead of defining separate named constants for these, then writing separate property getter functions with dupes of the switch statements, make a set of constant arrays, each three elements long. The elements are the constants for the three modes, in the same order as MaterialDesignController::Mode. Then you can implement GetDefaultDisplayProperty() with a single switch statement a la: // The order of the constants in all following lines is: // MATERIAL, MATERIAL_HYBRID, NON_MATERIAL const int kToolbarViewLeftEdgeSpacing[] = { 4, 8, 3 }; ... // The cast in the next line is not needed if Mode becomes a standard // enum instead of an enum class. int mode = static_cast<int>(ui::MaterialDesignController::GetMode()); switch (id) { case ThemeProperties::PROPERTY_TOOLBAR_VIEW_LEFT_EDGE_SPACING: return kToolbarViewLeftEdgeSpacing[mode]; ... You will want to avoid protecting any of this code with ENABLE_TOPCHROME_MD, since doing that is not necessary to get the functionality right (GetMode() will be callable and do the right thing regardless), trying to add this #define will just make the declaration and definition of the above more complicated, and in general we should strive to avoid #ifdefs wherever possible. P.S. Why is NON_MATERIAL enum value 2 instead of 0? It would likely make not only this code but similar code in other places easier to read quickly if the "default, non-experimental" value was the first one in the enum. https://codereview.chromium.org/1164333002/diff/120001/chrome/browser/themes/... chrome/browser/themes/theme_properties.cc:168: int GetDefaultDisplayPropertyInternal(int id) { I'm not sure why the word "internal" is used here as these constants aren't more or less internal than the other ones, nor is the concept of material design "external". I would use "NonMaterial" or the like. That's also consistent with the enum names in material_design_controller.h. https://codereview.chromium.org/1164333002/diff/120001/chrome/browser/ui/view... File chrome/browser/ui/views/toolbar/toolbar_view.cc (right): https://codereview.chromium.org/1164333002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/toolbar_view.cc:801: return browser_->host_desktop_type() == chrome::HOST_DESKTOP_TYPE_ASH Nit: Simpler: return theme_provider->GetDisplayProperty( (browser_->host_desktop_type() == chrome::HOST_DESKTOP_TYPE_ASH) ? ThemeProperties::PROPERTY_TOOLBAR_VIEW_CONTENT_SHADOW_HEIGHT_ASH : ThemeProperties::PROPERTY_TOOLBAR_VIEW_CONTENT_SHADOW_HEIGHT);
https://codereview.chromium.org/1164333002/diff/120001/chrome/browser/themes/... File chrome/browser/themes/theme_properties.cc (right): https://codereview.chromium.org/1164333002/diff/120001/chrome/browser/themes/... chrome/browser/themes/theme_properties.cc:100: // Defaults for layout properties, which are not stored in the browser theme On 2015/07/03 20:13:08, Peter Kasting wrote: > Nit: No comma Done. https://codereview.chromium.org/1164333002/diff/120001/chrome/browser/themes/... chrome/browser/themes/theme_properties.cc:115: const int kToolbarViewLeftEdgeSpacingMD = 4; On 2015/07/03 20:13:08, Peter Kasting wrote: > This file can be simpler and easier to maintain. > > Instead of defining separate named constants for these, then writing separate > property getter functions with dupes of the switch statements, make a set of > constant arrays, each three elements long. The elements are the constants for > the three modes, in the same order as MaterialDesignController::Mode. Then you > can implement GetDefaultDisplayProperty() with a single switch statement a la: > > // The order of the constants in all following lines is: > // MATERIAL, MATERIAL_HYBRID, NON_MATERIAL > const int kToolbarViewLeftEdgeSpacing[] = { 4, 8, 3 }; > ... > // The cast in the next line is not needed if Mode becomes a standard > // enum instead of an enum class. > int mode = static_cast<int>(ui::MaterialDesignController::GetMode()); > switch (id) { > case ThemeProperties::PROPERTY_TOOLBAR_VIEW_LEFT_EDGE_SPACING: > return kToolbarViewLeftEdgeSpacing[mode]; > ... > > You will want to avoid protecting any of this code with ENABLE_TOPCHROME_MD, > since doing that is not necessary to get the functionality right (GetMode() will > be callable and do the right thing regardless), trying to add this #define will > just make the declaration and definition of the above more complicated, and in > general we should strive to avoid #ifdefs wherever possible. > > P.S. Why is NON_MATERIAL enum value 2 instead of 0? It would likely make not > only this code but similar code in other places easier to read quickly if the > "default, non-experimental" value was the first one in the enum. Great idea! I've switched this around to use the array. The switch case has been returned to GetDefaultDisplayProperty, and the methods I added below have been removed. I've updated MaterialDesignController's enum to have NON_MATERIAL be 0 to be consistent with it as the default value. https://codereview.chromium.org/1164333002/diff/120001/chrome/browser/themes/... chrome/browser/themes/theme_properties.cc:168: int GetDefaultDisplayPropertyInternal(int id) { On 2015/07/03 20:13:08, Peter Kasting wrote: > I'm not sure why the word "internal" is used here as these constants aren't more > or less internal than the other ones, nor is the concept of material design > "external". > > I would use "NonMaterial" or the like. That's also consistent with the enum > names in material_design_controller.h. These methods have been removed. https://codereview.chromium.org/1164333002/diff/120001/chrome/browser/ui/view... File chrome/browser/ui/views/toolbar/toolbar_view.cc (right): https://codereview.chromium.org/1164333002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/toolbar_view.cc:801: return browser_->host_desktop_type() == chrome::HOST_DESKTOP_TYPE_ASH On 2015/07/03 20:13:08, Peter Kasting wrote: > Nit: Simpler: > > return theme_provider->GetDisplayProperty( > (browser_->host_desktop_type() == chrome::HOST_DESKTOP_TYPE_ASH) > ? ThemeProperties::PROPERTY_TOOLBAR_VIEW_CONTENT_SHADOW_HEIGHT_ASH > : ThemeProperties::PROPERTY_TOOLBAR_VIEW_CONTENT_SHADOW_HEIGHT); Done.
LGTM https://codereview.chromium.org/1164333002/diff/140001/chrome/browser/themes/... File chrome/browser/themes/theme_properties.cc (right): https://codereview.chromium.org/1164333002/diff/140001/chrome/browser/themes/... chrome/browser/themes/theme_properties.cc:335: return -1; Nit: Style guide says the switch above should have a default case since you're not enumerating all possible values of an enum. So I'd move this return into such a case.
jonross@chromium.org changed reviewers: + sadrul@chromium.org
Hey Sadrul, Could you provide an owners review to the change in ui/base/resource/material_design/material_design_controller.h ? Thanks, Jon https://codereview.chromium.org/1164333002/diff/140001/chrome/browser/themes/... File chrome/browser/themes/theme_properties.cc (right): https://codereview.chromium.org/1164333002/diff/140001/chrome/browser/themes/... chrome/browser/themes/theme_properties.cc:335: return -1; On 2015/07/06 22:48:38, Peter Kasting wrote: > Nit: Style guide says the switch above should have a default case since you're > not enumerating all possible values of an enum. So I'd move this return into > such a case. Done.
Just a nit https://codereview.chromium.org/1164333002/diff/160001/chrome/browser/themes/... File chrome/browser/themes/theme_properties.cc (right): https://codereview.chromium.org/1164333002/diff/160001/chrome/browser/themes/... chrome/browser/themes/theme_properties.cc:101: // pack. The array indices map to ui::MaterialDesignController::Mode. Adding a comment with the mapping of the MaterialDesignController::Mode to index here would be hugely helpful. Otherwise anyone editing this file will have to also need to have material_design_controller.h open
https://codereview.chromium.org/1164333002/diff/160001/chrome/browser/themes/... File chrome/browser/themes/theme_properties.cc (right): https://codereview.chromium.org/1164333002/diff/160001/chrome/browser/themes/... chrome/browser/themes/theme_properties.cc:101: // pack. The array indices map to ui::MaterialDesignController::Mode. On 2015/07/07 14:30:27, pkotwicz wrote: > Adding a comment with the mapping of the MaterialDesignController::Mode to index > here would be hugely helpful. Otherwise anyone editing this file will have to > also need to have material_design_controller.h open OTOH if we do that we have copies of the same information in multiple places and we now need to make sure we always edit both lists at once or things rapidly become confusing. I'd probably just say something like: "The array indexes here are the values of ui::MaterialDesignController::Mode; see ui/base/resource/material_design/material_design_controller.h." This also reminds me: that enum should be updated to have at least "= 0" on the first value (and maybe explicit initializers for all values?) and a comment that the order can't be changed without changing the code over here.
https://codereview.chromium.org/1164333002/diff/160001/chrome/browser/themes/... File chrome/browser/themes/theme_properties.cc (right): https://codereview.chromium.org/1164333002/diff/160001/chrome/browser/themes/... chrome/browser/themes/theme_properties.cc:101: // pack. The array indices map to ui::MaterialDesignController::Mode. On 2015/07/07 19:01:16, Peter Kasting wrote: > On 2015/07/07 14:30:27, pkotwicz wrote: > > Adding a comment with the mapping of the MaterialDesignController::Mode to > index > > here would be hugely helpful. Otherwise anyone editing this file will have to > > also need to have material_design_controller.h open > > OTOH if we do that we have copies of the same information in multiple places and > we now need to make sure we always edit both lists at once or things rapidly > become confusing. > > I'd probably just say something like: "The array indexes here are the values of > ui::MaterialDesignController::Mode; see > ui/base/resource/material_design/material_design_controller.h." > > This also reminds me: that enum should be updated to have at least "= 0" on the > first value (and maybe explicit initializers for all values?) and a comment that > the order can't be changed without changing the code over here. Maybe have COMPILE_ASSERT()s here to make sure if the values change, it gets caught?
https://codereview.chromium.org/1164333002/diff/160001/chrome/browser/themes/... File chrome/browser/themes/theme_properties.cc (right): https://codereview.chromium.org/1164333002/diff/160001/chrome/browser/themes/... chrome/browser/themes/theme_properties.cc:101: // pack. The array indices map to ui::MaterialDesignController::Mode. On 2015/07/07 14:30:27, pkotwicz wrote: > Adding a comment with the mapping of the MaterialDesignController::Mode to index > here would be hugely helpful. Otherwise anyone editing this file will have to > also need to have material_design_controller.h open Done. https://codereview.chromium.org/1164333002/diff/160001/chrome/browser/themes/... chrome/browser/themes/theme_properties.cc:101: // pack. The array indices map to ui::MaterialDesignController::Mode. On 2015/07/07 21:53:04, sadrul wrote: > On 2015/07/07 19:01:16, Peter Kasting wrote: > > On 2015/07/07 14:30:27, pkotwicz wrote: > > > Adding a comment with the mapping of the MaterialDesignController::Mode to > > index > > > here would be hugely helpful. Otherwise anyone editing this file will have > to > > > also need to have material_design_controller.h open > > > > OTOH if we do that we have copies of the same information in multiple places > and > > we now need to make sure we always edit both lists at once or things rapidly > > become confusing. > > > > I'd probably just say something like: "The array indexes here are the values > of > > ui::MaterialDesignController::Mode; see > > ui/base/resource/material_design/material_design_controller.h." > > > > This also reminds me: that enum should be updated to have at least "= 0" on > the > > first value (and maybe explicit initializers for all values?) and a comment > that > > the order can't be changed without changing the code over here. > > Maybe have COMPILE_ASSERT()s here to make sure if the values change, it gets > caught? Updated the comments to point to the header. Added COMPILE_ASSERTS to check the indices matching Updated the enum to explicitly set the values
https://codereview.chromium.org/1164333002/diff/180001/chrome/browser/themes/... File chrome/browser/themes/theme_properties.cc (right): https://codereview.chromium.org/1164333002/diff/180001/chrome/browser/themes/... chrome/browser/themes/theme_properties.cc:104: COMPILE_ASSERT( I think these compile asserts are more big and ugly than helpful. They also don't catch adding a new value to the end of the enum. And we're very unlikely to change the order of things in the enum from now on to begin with; there's no motivation to do so. For all these reasons, I would prefer to leave them out. If you disagree and wish to keep them, then I again suggest changing Mode from an enum class to a pure enum, so you can remove all the static_casts here at least. It makes sense to me to make Mode an enum regardless if we actually intend to use its values as integral numbers somewhere.
https://codereview.chromium.org/1164333002/diff/180001/chrome/browser/themes/... File chrome/browser/themes/theme_properties.cc (right): https://codereview.chromium.org/1164333002/diff/180001/chrome/browser/themes/... chrome/browser/themes/theme_properties.cc:104: COMPILE_ASSERT( On 2015/07/08 21:18:42, Peter Kasting wrote: > I think these compile asserts are more big and ugly than helpful. They also > don't catch adding a new value to the end of the enum. And we're very unlikely > to change the order of things in the enum from now on to begin with; there's no > motivation to do so. > > For all these reasons, I would prefer to leave them out. > > If you disagree and wish to keep them, then I again suggest changing Mode from > an enum class to a pure enum, so you can remove all the static_casts here at > least. It makes sense to me to make Mode an enum regardless if we actually > intend to use its values as integral numbers somewhere. Done.
ui/ lgtm
The CQ bit was checked by jonross@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bruthig@chromium.org, pkotwicz@chromium.org, pkasting@chromium.org Link to the patchset: https://codereview.chromium.org/1164333002/#ps200001 (title: "Change Enum from a Class")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1164333002/200001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...) ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...) mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...) (exceeded global retry quota)
The CQ bit was checked by jonross@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from pkotwicz@chromium.org, bruthig@chromium.org, sadrul@chromium.org, pkasting@chromium.org Link to the patchset: https://codereview.chromium.org/1164333002/#ps220001 (title: "Rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1164333002/220001
Message was sent while issue was closed.
Committed patchset #12 (id:220001)
Message was sent while issue was closed.
Patchset 12 (id:??) landed as https://crrev.com/28aa1291fb4cfb88d62872174e11f806a557f039 Cr-Commit-Position: refs/heads/master@{#338049} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
