wrench_menu: use NativeTheme colors under aura

wrench_menu: use NativeTheme colors under aura

This changes the wrench menu's MenuButtonBackground to use NativeTheme colors under aura, and introduces kColorId_HotMenuItemBackgroundColor.

BUG=130810
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=190048

[Bobby Powers, 2013-03-16 15:36:17]

Hello, please take a look.  There are a few issues that I'm unsure of.  First,
it seems like the #ifdef I added is close to the point where it makes sense to
split off an aura-specific class.  Should I do this, or is it OK as is?  Second,
the wrench menu uses the Hot adjective where I've seen Hover used elsewhere.  Is
one preferred over the other?

This CL combined with #12518029 result in a nice native-looking wrench menu
under my dark GTK theme.

Elliot - let me know if you're the most appropriate reviewer for this sort of
stuff, or if there is someone else I should be bothering :)

yours,
Bobby

[Bobby Powers, 2013-03-18 17:00:02]

On 2013/03/16 15:36:17, Bobby Powers wrote:
> Hello, please take a look.  There are a few issues that I'm unsure of.  First,
> it seems like the #ifdef I added is close to the point where it makes sense to
> split off an aura-specific class.  Should I do this, or is it OK as is? 
Second,
> the wrench menu uses the Hot adjective where I've seen Hover used elsewhere. 
Is
> one preferred over the other?
> 
> This CL combined with #12518029 result in a nice native-looking wrench menu
> under my dark GTK theme.
> 
> Elliot - let me know if you're the most appropriate reviewer for this sort of
> stuff, or if there is someone else I should be bothering :)
> 
> yours,
> Bobby

I need to update this to call view::GetNativeTheme, rather than the global
ui::NativeTheme::instance.  I guess please hold off on review until that
happens, or keep it in mind.

[Elliot Glaysher, 2013-03-18 17:53:40]

A few comments fyi, anyway:

https://codereview.chromium.org/12483006/diff/2001/chrome/browser/ui/views/wr...
File chrome/browser/ui/views/wrench_menu.cc (right):

https://codereview.chromium.org/12483006/diff/2001/chrome/browser/ui/views/wr...
chrome/browser/ui/views/wrench_menu.cc:251: #if defined(USE_AURA)
Please remember that aura is not a linux-only component, but is currently
shipped on chromeos and will be shipped on Windows, probably simultaneously with
linux.

Querying a NativeTheme (preferably via Views::GetNativeTheme()) is fine, but
please push the current colors up into all the subclasses of NativeTheme, and
override the colors returned only in NativeThemeGtk2. (This is something that's
been on various people's plates for a while, but is now actually a thing that
has to happen for linux_aura.)

https://codereview.chromium.org/12483006/diff/2001/ui/native_theme/native_the...
File ui/native_theme/native_theme.h (right):

https://codereview.chromium.org/12483006/diff/2001/ui/native_theme/native_the...
ui/native_theme/native_theme.h:243: kColorId_HotMenuItemBackgroundColor,
Please follow the naming conventions from the State enum; this should be
"Hovered" instead of "Hot".

(I know that it's also currently wrong in the wrench_menu.cc class; if you do
end up pushing those colors in here (kColorId_HoveredMenuItemButtonBackground,
or whatnot), please don't copy the incorrect "hot" and "pushed" naming.)

[Bobby Powers, 2013-03-18 18:11:43]

https://codereview.chromium.org/12483006/diff/2001/chrome/browser/ui/views/wr...
File chrome/browser/ui/views/wrench_menu.cc (right):

https://codereview.chromium.org/12483006/diff/2001/chrome/browser/ui/views/wr...
chrome/browser/ui/views/wrench_menu.cc:251: #if defined(USE_AURA)
On 2013/03/18 17:53:40, Elliot Glaysher wrote:
> Please remember that aura is not a linux-only component, but is currently
> shipped on chromeos and will be shipped on Windows, probably simultaneously
with
> linux.
> 
> Querying a NativeTheme (preferably via Views::GetNativeTheme()) is fine, but
> please push the current colors up into all the subclasses of NativeTheme, and
> override the colors returned only in NativeThemeGtk2. (This is something
that's
> been on various people's plates for a while, but is now actually a thing that
> has to happen for linux_aura.)

{Unfocused,Focused,Menu}BorderColor were already defined for NativeThemeAura,
the only new color I added was HotMenuItemBackgroundColor.  I defined this for
NativeThemeAura (using the original color from wrench_menu.cc), and overrode it
in NativeThemeGtk2.  It seems like the best way to make sure it is available to
Windows & friends is to add it to CommonThemeGetSystemColor.  Does this make
sense, or should I explicitly override it NativeThemeWin?

https://codereview.chromium.org/12483006/diff/2001/ui/native_theme/native_the...
File ui/native_theme/native_theme.h (right):

https://codereview.chromium.org/12483006/diff/2001/ui/native_theme/native_the...
ui/native_theme/native_theme.h:243: kColorId_HotMenuItemBackgroundColor,
On 2013/03/18 17:53:40, Elliot Glaysher wrote:
> Please follow the naming conventions from the State enum; this should be
> "Hovered" instead of "Hot".
> 
> (I know that it's also currently wrong in the wrench_menu.cc class; if you do
> end up pushing those colors in here (kColorId_HoveredMenuItemButtonBackground,
> or whatnot), please don't copy the incorrect "hot" and "pushed" naming.)

Understood, will do.

[Bobby Powers, 2013-03-18 19:59:53]

PTAL.  The main thing is to pass a View down from ToolbarView::OnMenuItemClicked
into WrenchMenu::Init.  This is because at the point of WrenchMenu::Init(), the
WrenchMenu doesn't appear to be associated with a Widget, in which case
View::GetNativeTheme() returns ui::NativeTheme::instance()
(ui/views/view.cc:807), which is explicitly what we don't want.  Using
ToolbarView::OnMenuItemClicked's 'source' view fares better.

yours,
Bobby

https://codereview.chromium.org/12483006/diff/2001/chrome/browser/ui/views/wr...
File chrome/browser/ui/views/wrench_menu.cc (right):

https://codereview.chromium.org/12483006/diff/2001/chrome/browser/ui/views/wr...
chrome/browser/ui/views/wrench_menu.cc:251: #if defined(USE_AURA)
On 2013/03/18 18:11:43, Bobby Powers wrote:
> On 2013/03/18 17:53:40, Elliot Glaysher wrote:
> > Please remember that aura is not a linux-only component, but is currently
> > shipped on chromeos and will be shipped on Windows, probably simultaneously
> with
> > linux.
> > 
> > Querying a NativeTheme (preferably via Views::GetNativeTheme()) is fine, but
> > please push the current colors up into all the subclasses of NativeTheme,
and
> > override the colors returned only in NativeThemeGtk2. (This is something
> that's
> > been on various people's plates for a while, but is now actually a thing
that
> > has to happen for linux_aura.)
> 
> {Unfocused,Focused,Menu}BorderColor were already defined for NativeThemeAura,
> the only new color I added was HotMenuItemBackgroundColor.  I defined this for
> NativeThemeAura (using the original color from wrench_menu.cc), and overrode
it
> in NativeThemeGtk2.  It seems like the best way to make sure it is available
to
> Windows & friends is to add it to CommonThemeGetSystemColor.  Does this make
> sense, or should I explicitly override it NativeThemeWin?

I believe the current patch addresses this correctly.

https://codereview.chromium.org/12483006/diff/2001/ui/native_theme/native_the...
File ui/native_theme/native_theme.h (right):

https://codereview.chromium.org/12483006/diff/2001/ui/native_theme/native_the...
ui/native_theme/native_theme.h:243: kColorId_HotMenuItemBackgroundColor,
On 2013/03/18 18:11:43, Bobby Powers wrote:
> On 2013/03/18 17:53:40, Elliot Glaysher wrote:
> > Please follow the naming conventions from the State enum; this should be
> > "Hovered" instead of "Hot".
> > 
> > (I know that it's also currently wrong in the wrench_menu.cc class; if you
do
> > end up pushing those colors in here
(kColorId_HoveredMenuItemButtonBackground,
> > or whatnot), please don't copy the incorrect "hot" and "pushed" naming.)
> 
> Understood, will do.

Done.

[Elliot Glaysher, 2013-03-18 21:03:39]

https://codereview.chromium.org/12483006/diff/9001/chrome/browser/ui/views/wr...
File chrome/browser/ui/views/wrench_menu.cc (right):

https://codereview.chromium.org/12483006/diff/9001/chrome/browser/ui/views/wr...
chrome/browser/ui/views/wrench_menu.cc:200: background_color(view, state));
Please call {background,border}_color(view, state) once outside the case
statement instead of repetitively inside it. 

(Leaving the ?: calls in CENTER_BUTTON is OK.)

https://codereview.chromium.org/12483006/diff/9001/chrome/browser/ui/views/wr...
chrome/browser/ui/views/wrench_menu.cc:262: static SkColor border_color(View*
view, CustomButton::ButtonState state) {
This block changes the behavior of border_color on ChromeOS and Windows aura
(which we ship today on the canary channel).

Chromeos/Windows aura builds will now ask their NativeThemes for example,
kColorId_UnfocusedBorderColor, which is (0xd9, 0xd9, 0xd9) in NativeThemeWin and
NativeThemeAura, but kHotBorderColor is (72, 0, 0, 0) in this file.

The preferred solution is to create kColorId_HoveredMenuButtonBorder, add this
color into the various NativeTheme subclasses, with the Gtk2 one returning the
current kColorId_HoverMenuItemBackgroundColor.

[Bobby Powers, 2013-03-18 21:24:58]

Thanks for the reviews so far Elliot.  PTAL.

https://codereview.chromium.org/12483006/diff/9001/chrome/browser/ui/views/wr...
File chrome/browser/ui/views/wrench_menu.cc (right):

https://codereview.chromium.org/12483006/diff/9001/chrome/browser/ui/views/wr...
chrome/browser/ui/views/wrench_menu.cc:200: background_color(view, state));
On 2013/03/18 21:03:39, Elliot Glaysher wrote:
> Please call {background,border}_color(view, state) once outside the case
> statement instead of repetitively inside it. 
> 
> (Leaving the ?: calls in CENTER_BUTTON is OK.)

Done.

https://codereview.chromium.org/12483006/diff/9001/chrome/browser/ui/views/wr...
chrome/browser/ui/views/wrench_menu.cc:262: static SkColor border_color(View*
view, CustomButton::ButtonState state) {
On 2013/03/18 21:03:39, Elliot Glaysher wrote:
> This block changes the behavior of border_color on ChromeOS and Windows aura
> (which we ship today on the canary channel).
> 
> Chromeos/Windows aura builds will now ask their NativeThemes for example,
> kColorId_UnfocusedBorderColor, which is (0xd9, 0xd9, 0xd9) in NativeThemeWin
and
> NativeThemeAura, but kHotBorderColor is (72, 0, 0, 0) in this file.
> 
> The preferred solution is to create kColorId_HoveredMenuButtonBorder, add this
> color into the various NativeTheme subclasses, with the Gtk2 one returning the
> current kColorId_HoverMenuItemBackgroundColor.

Done.

[Elliot Glaysher, 2013-03-18 21:38:21]

https://codereview.chromium.org/12483006/diff/12001/chrome/browser/ui/views/w...
File chrome/browser/ui/views/wrench_menu.cc (right):

https://codereview.chromium.org/12483006/diff/12001/chrome/browser/ui/views/w...
chrome/browser/ui/views/wrench_menu.cc:251: #if defined(USE_AURA)
It's a start, but this #ifdef block still exists. All three colors should be
behind the GetSystemColor() interface so that you can delete the #else side of
this block.

https://codereview.chromium.org/12483006/diff/12001/ui/native_theme/common_th...
File ui/native_theme/common_theme.cc (right):

https://codereview.chromium.org/12483006/diff/12001/ui/native_theme/common_th...
ui/native_theme/common_theme.cc:45: *color = kMenuBorderColor;
kHoverBorderColor?

[Bobby Powers, 2013-03-19 13:27:01]

I think its starting to look better :)  I've added
{Enabled,Focused,Hover}MenuItemBorderColor everywhere with the values from
wrench_menu.cc.  FocusedMenuItemBackgroundColor already existed, but the values
CommonThemeGetSystemColor and NativeThemeAura use is almost the same as it was
in wrench_menu.cc (common theme is 0x0E000000, aura is non-alpha blended white
of 0xF1F1F1, compared to 0x0C000000 in wrench menu).  Similarly, menu background
color in the wrench menu was defined as white with an alpha of 102, but common
and aura themes use white without any alpha.

So without further ado, PTAL.

https://codereview.chromium.org/12483006/diff/12001/chrome/browser/ui/views/w...
File chrome/browser/ui/views/wrench_menu.cc (right):

https://codereview.chromium.org/12483006/diff/12001/chrome/browser/ui/views/w...
chrome/browser/ui/views/wrench_menu.cc:251: #if defined(USE_AURA)
On 2013/03/18 21:38:21, Elliot Glaysher wrote:
> It's a start, but this #ifdef block still exists. All three colors should be
> behind the GetSystemColor() interface so that you can delete the #else side of
> this block.

Done.

https://codereview.chromium.org/12483006/diff/12001/ui/native_theme/common_th...
File ui/native_theme/common_theme.cc (right):

https://codereview.chromium.org/12483006/diff/12001/ui/native_theme/common_th...
ui/native_theme/common_theme.cc:45: *color = kMenuBorderColor;
On 2013/03/18 21:38:21, Elliot Glaysher wrote:
> kHoverBorderColor?

Whoops, fat fingers.  Fixed.

[Elliot Glaysher, 2013-03-19 17:16:55]

lgtm. Adding sky to take a quick glance at the ui/ directory before stamp.

[sky, 2013-03-19 21:08:26]

https://codereview.chromium.org/12483006/diff/18011/chrome/browser/ui/views/w...
File chrome/browser/ui/views/wrench_menu.cc (right):

https://codereview.chromium.org/12483006/diff/18011/chrome/browser/ui/views/w...
chrome/browser/ui/views/wrench_menu.cc:250:
ui::NativeTheme::kColorId_HoverMenuItemBorderColor);
indent 4 here, 253 and all other similar places.

https://codereview.chromium.org/12483006/diff/18011/chrome/browser/ui/views/w...
File chrome/browser/ui/views/wrench_menu.h (right):

https://codereview.chromium.org/12483006/diff/18011/chrome/browser/ui/views/w...
chrome/browser/ui/views/wrench_menu.h:44: void Init(views::View* view,
ui::MenuModel* model);
It seems as though the only need you have for view is to get the native theme.
See code in WrenchMenu::GetMenuConfig() for how to get the NativeTheme from the
browser_.

https://codereview.chromium.org/12483006/diff/18011/ui/native_theme/native_th...
File ui/native_theme/native_theme.h (right):

https://codereview.chromium.org/12483006/diff/18011/ui/native_theme/native_th...
ui/native_theme/native_theme.h:243: kColorId_HoverMenuItemBackgroundColor,
These names imply they are used by menuitems, which isn't the case. They are
used for the MenuButton, which is totally different. They need to be named as
suhc.

[Bobby Powers, 2013-03-20 14:54:37]

Hi sky,

I believe I've addressed all of your comments, the only place I didn't do
exactly as you suggested is with HoverMenuItemBackgroundColor, but my reasoning
is below. If you don't agree I think the best thing to do would be to create
HoverMenuButtonBackgroundColor and FocusedMenuButtonBackgroundColor, lemme know.
PTAL.

Thanks!

yours,
Bobby

https://codereview.chromium.org/12483006/diff/18011/chrome/browser/ui/views/w...
File chrome/browser/ui/views/wrench_menu.cc (right):

https://codereview.chromium.org/12483006/diff/18011/chrome/browser/ui/views/w...
chrome/browser/ui/views/wrench_menu.cc:250:
ui::NativeTheme::kColorId_HoverMenuItemBorderColor);
On 2013/03/19 21:08:26, sky wrote:
> indent 4 here, 253 and all other similar places.

Done.

https://codereview.chromium.org/12483006/diff/18011/chrome/browser/ui/views/w...
File chrome/browser/ui/views/wrench_menu.h (right):

https://codereview.chromium.org/12483006/diff/18011/chrome/browser/ui/views/w...
chrome/browser/ui/views/wrench_menu.h:44: void Init(views::View* view,
ui::MenuModel* model);
On 2013/03/19 21:08:26, sky wrote:
> It seems as though the only need you have for view is to get the native theme.
> See code in WrenchMenu::GetMenuConfig() for how to get the NativeTheme from
the
> browser_.

Ah, yes!  Very good.  I've reworked the patch to do exactly that.

https://codereview.chromium.org/12483006/diff/18011/ui/native_theme/native_th...
File ui/native_theme/native_theme.h (right):

https://codereview.chromium.org/12483006/diff/18011/ui/native_theme/native_th...
ui/native_theme/native_theme.h:243: kColorId_HoverMenuItemBackgroundColor,
On 2013/03/19 21:08:26, sky wrote:
> These names imply they are used by menuitems, which isn't the case. They are
> used for the MenuButton, which is totally different. They need to be named as
> suhc.

I changed the all the MenuItemBorders to MenuButtonBorders.  For
HoverMenuItemBackgroundColor, I think it should be left - we want the background
of MenuButtons to match MenuItems, even though right now MenuItem doesn't
distinguish between the Hover and Focused states.  Maybe I'm misunderstanding,
though.

[sky, 2013-03-20 16:30:01]

https://codereview.chromium.org/12483006/diff/28001/chrome/browser/ui/views/w...
File chrome/browser/ui/views/wrench_menu.cc (right):

https://codereview.chromium.org/12483006/diff/28001/chrome/browser/ui/views/w...
chrome/browser/ui/views/wrench_menu.cc:245: static SkColor border_color(View*
view, CustomButton::ButtonState state) {
These should be renamed to CamelCase now that they are no longer cheap, eg
GetBorderColor and GetBackgroundColor (leave touch_background_color).

https://codereview.chromium.org/12483006/diff/28001/ui/native_theme/native_th...
File ui/native_theme/native_theme.h (right):

https://codereview.chromium.org/12483006/diff/28001/ui/native_theme/native_th...
ui/native_theme/native_theme.h:247: kColorId_EnabledMenuButtonBorderColor,
It isn't obvious what menu button border is. Add a description.

https://codereview.chromium.org/12483006/diff/28001/ui/native_theme/native_th...
File ui/native_theme/native_theme_gtk.cc (right):

https://codereview.chromium.org/12483006/diff/28001/ui/native_theme/native_th...
ui/native_theme/native_theme_gtk.cc:24: const SkColor
kFocusedMenuItemBackgroundColor = SkColorSetARGB(13, 0, 0, 0);
Can we not copy these values to three places?

[Bobby Powers, 2013-03-21 17:08:43]

Hello sky,

I believe the only thing I didn't yet address is the duplication of constants
between native theme implementations.  As noted below, this is an existing
problem, and I would suggest cleaning all of it up in a subsequent commit.  I'd
be happy to look at this, especially if given a little guidance.  It seems the
most straightforward way to do this would be to query CommonThemeGetSystemColor
for the values.  Is it that straightforward, or are there things I'm not
thinking of that would warrant a new common GetSystemColor method somewhere?

PTAL.

yours,
Bobby

https://codereview.chromium.org/12483006/diff/28001/chrome/browser/ui/views/w...
File chrome/browser/ui/views/wrench_menu.cc (right):

https://codereview.chromium.org/12483006/diff/28001/chrome/browser/ui/views/w...
chrome/browser/ui/views/wrench_menu.cc:245: static SkColor border_color(View*
view, CustomButton::ButtonState state) {
On 2013/03/20 16:30:02, sky wrote:
> These should be renamed to CamelCase now that they are no longer cheap, eg
> GetBorderColor and GetBackgroundColor (leave touch_background_color).

Done - I'll keep that in mind in the future.

https://codereview.chromium.org/12483006/diff/28001/ui/native_theme/native_th...
File ui/native_theme/native_theme.h (right):

https://codereview.chromium.org/12483006/diff/28001/ui/native_theme/native_th...
ui/native_theme/native_theme.h:247: kColorId_EnabledMenuButtonBorderColor,
On 2013/03/20 16:30:02, sky wrote:
> It isn't obvious what menu button border is. Add a description.

Done.

https://codereview.chromium.org/12483006/diff/28001/ui/native_theme/native_th...
File ui/native_theme/native_theme_gtk.cc (right):

https://codereview.chromium.org/12483006/diff/28001/ui/native_theme/native_th...
ui/native_theme/native_theme_gtk.cc:24: const SkColor
kFocusedMenuItemBackgroundColor = SkColorSetARGB(13, 0, 0, 0);
On 2013/03/20 16:30:02, sky wrote:
> Can we not copy these values to three places?

Copying is what Elliot originally suggested, and there is already a ton of
duplication among the various native_theme implementations (TextButton colors
are currently the same between _gtk and _win, for example).  What I think would
be best is to commit this as such, and clean up all of the duplication in a
follow up commit.

[sky, 2013-03-21 17:43:52]

LGTM - lets go with the copy for now and you can clean up in a later patch.

[Bobby Powers, 2013-03-21 17:44:34]

On 2013/03/21 17:43:52, sky wrote:
> LGTM - lets go with the copy for now and you can clean up in a later patch.

awesome, thanks :)

[Bobby Powers, 2013-03-22 15:09:51]

Elliot - can you kick the trybuild/commit machinery?

[commit-bot: I haz the power, 2013-03-22 16:40:12]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bobbypowers@gmail.com/12483006/34001

[commit-bot: I haz the power, 2013-03-22 18:16:40]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bobbypowers@gmail.com/12483006/34001

[Bobby Powers, 2013-03-22 19:55:35]

On 2013/03/22 18:16:40, I haz the power (commit-bot) wrote:
> CQ is trying da patch. Follow status at
> https://chromium-status.appspot.com/cq/bobbypowers%40gmail.com/12483006/34001

win_aura had a legit failure
(http://build.chromium.org/p/tryserver.chromium/builders/win7_aura/builds/2279...).
 I updated the patch, not sure if anything needs to be restarted/re-lgtm'ed.

[Elliot Glaysher, 2013-03-22 19:57:43]

On 2013/03/22 19:55:35, Bobby Powers wrote:
> On 2013/03/22 18:16:40, I haz the power (commit-bot) wrote:
> > CQ is trying da patch. Follow status at
> >
https://chromium-status.appspot.com/cq/bobbypowers%2540gmail.com/12483006/34001
> 
> win_aura had a legit failure
>
(http://build.chromium.org/p/tryserver.chromium/builders/win7_aura/builds/2279...).
>  I updated the patch, not sure if anything needs to be restarted/re-lgtm'ed.

clicking cq on patch set 10

[commit-bot: I haz the power, 2013-03-22 20:05:53]

Failed to trigger a try job on win_x64_rel
HTTP Error 400: Bad Request

[commit-bot: I haz the power, 2013-03-22 20:08:12]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bobbypowers@gmail.com/12483006/49001

[commit-bot: I haz the power, 2013-03-23 14:53:52]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bobbypowers@gmail.com/12483006/49001

[commit-bot: I haz the power, 2013-03-23 17:48:52]

Change committed as 190048

[cpu_(ooo_6.6-7.5), 2013-03-29 20:18:34]

Note that is change dchecks on windows if you are not in the new menu style
experiment.

It dchecks because CommonThemeGetSystemColor is not used and therefore we hit
NOTREACHED in the switch statement of

NativeThemeAura::GetSystemColor(..)

Because some colors are no longer defined on Windows.

I fixed by always using the new style menu if the build is Aura.

[Bobby Powers, 2013-03-30 03:13:59]

On 2013/03/29 20:18:34, cpu wrote:
> Note that is change dchecks on windows if you are not in the new menu style
> experiment.
> 
> It dchecks because CommonThemeGetSystemColor is not used and therefore we hit
> NOTREACHED in the switch statement of
> 
> NativeThemeAura::GetSystemColor(..)
> 
> Because some colors are no longer defined on Windows.
> 
> I fixed by always using the new style menu if the build is Aura.

hi cpu,

Unfortunately I don't think I understand your point.  I didn't remove any
dchecks, so we should still get the same behavior?  I must admit the logic
around the new menu style experiment if statements did somewhat confuse me, so I
certainly could have made a mistake.  If I did mess something up, I am happy to
promptly address it, once I understand the issue.

[Elliot Glaysher, 2013-04-01 17:34:43]

On 2013/03/30 03:13:59, Bobby Powers wrote:
> On 2013/03/29 20:18:34, cpu wrote:
> > Note that is change dchecks on windows if you are not in the new menu style
> > experiment.
> > 
> > It dchecks because CommonThemeGetSystemColor is not used and therefore we
hit
> > NOTREACHED in the switch statement of
> > 
> > NativeThemeAura::GetSystemColor(..)
> > 
> > Because some colors are no longer defined on Windows.
> > 
> > I fixed by always using the new style menu if the build is Aura.
> 
> hi cpu,
> 
> Unfortunately I don't think I understand your point.  I didn't remove any
> dchecks, so we should still get the same behavior?  I must admit the logic
> around the new menu style experiment if statements did somewhat confuse me, so
I
> certainly could have made a mistake.  If I did mess something up, I am happy
to
> promptly address it, once I understand the issue.

IIUC, it was hitting the NOTREACHED() at the end of the giant switch statement
in NativeThemeWin::GetSystemColor() because sometimes IsNewMenuStyleEnabled()
returned false on Windows.

[Bobby Powers, 2013-04-08 22:01:21]

Sorry for being absent - im in the middle of moving and starting a new
job.  I will have bandwidth to look at this tomorrow or wednesday
On Apr 1, 2013 1:34 PM, <erg@chromium.org> wrote:

> On 2013/03/30 03:13:59, Bobby Powers wrote:
>
>> On 2013/03/29 20:18:34, cpu wrote:
>> > Note that is change dchecks on windows if you are not in the new menu
>> style
>> > experiment.
>> >
>> > It dchecks because CommonThemeGetSystemColor is not used and therefore
>> we
>>
> hit
>
>> > NOTREACHED in the switch statement of
>> >
>> > NativeThemeAura::**GetSystemColor(..)
>> >
>> > Because some colors are no longer defined on Windows.
>> >
>> > I fixed by always using the new style menu if the build is Aura.
>>
>
>  hi cpu,
>>
>
>  Unfortunately I don't think I understand your point.  I didn't remove any
>> dchecks, so we should still get the same behavior?  I must admit the logic
>> around the new menu style experiment if statements did somewhat confuse
>> me, so
>>
> I
>
>> certainly could have made a mistake.  If I did mess something up, I am
>> happy
>>
> to
>
>> promptly address it, once I understand the issue.
>>
>
> IIUC, it was hitting the NOTREACHED() at the end of the giant switch
> statement
> in NativeThemeWin::**GetSystemColor() because sometimes
> IsNewMenuStyleEnabled()
> returned false on Windows.
>
>
https://chromiumcodereview.**appspot.com/12483006/<https://chromiumcodereview...
>