Setting the touch wrench menu as default menu for ChromeOS

The touch menu for ChromeOS will be used for all flavors of ChromeOS while other operating systems should stay as they are.

Furthermore changed according to specifications which can be seen in the issue.

Cleaned up the menu separator constants.


BUG=137345
TEST=visually checked

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=153157

[Mr4D (OOO till 08-26), 2012-08-18 02:53:33]

Please have a look!

[sky, 2012-08-20 15:58:17]

http://codereview.chromium.org/10837317/diff/3001/ui/base/models/menu_model.h
File ui/base/models/menu_model.h (right):

http://codereview.chromium.org/10837317/diff/3001/ui/base/models/menu_model.h...
ui/base/models/menu_model.h:40: static const string16 NORMAL_SEPARATOR;
This should be an enum.

http://codereview.chromium.org/10837317/diff/3001/ui/base/models/simple_menu_...
File ui/base/models/simple_menu_model.cc (right):

http://codereview.chromium.org/10837317/diff/3001/ui/base/models/simple_menu_...
ui/base/models/simple_menu_model.cc:87: Item item = { kSeparatorId,
separator_type, gfx::ImageSkia(), TYPE_SEPARATOR,
Embedding the separator type in the label is a hack. Add a new field.

[Mr4D (OOO till 08-26), 2012-08-21 01:13:18]

Addressed. Have a look.

http://codereview.chromium.org/10837317/diff/3001/ui/base/models/simple_menu_...
File ui/base/models/simple_menu_model.cc (right):

http://codereview.chromium.org/10837317/diff/3001/ui/base/models/simple_menu_...
ui/base/models/simple_menu_model.cc:87: Item item = { kSeparatorId,
separator_type, gfx::ImageSkia(), TYPE_SEPARATOR,
Okay, done.

[sky, 2012-08-21 03:03:25]

http://codereview.chromium.org/10837317/diff/1035/ui/base/models/menu_model.h
File ui/base/models/menu_model.h (right):

http://codereview.chromium.org/10837317/diff/1035/ui/base/models/menu_model.h...
ui/base/models/menu_model.h:62: virtual ui::MenuSeparatorStyle
GetSeparatorStyleAt(int index) const = 0;
Why put this in a separate file instead of inside this class like the other? I'm
ok with the separate file as long you move ItemType too. It need not be with
this patch. I would name the file menu_types too.

http://codereview.chromium.org/10837317/diff/1035/ui/base/models/separator_ty...
File ui/base/models/separator_types.h (right):

http://codereview.chromium.org/10837317/diff/1035/ui/base/models/separator_ty...
ui/base/models/separator_types.h:11: enum MenuSeparatorStyle {
MenuSeparatorType.

http://codereview.chromium.org/10837317/diff/1035/ui/base/models/separator_ty...
ui/base/models/separator_types.h:12: NO_SEPARATOR = 0,
Can you remove this? If you keep it means AddSeparator(NO_SEPARATOR) is
possible, which is bogus.

http://codereview.chromium.org/10837317/diff/1035/ui/base/models/simple_menu_...
File ui/base/models/simple_menu_model.cc (right):

http://codereview.chromium.org/10837317/diff/1035/ui/base/models/simple_menu_...
ui/base/models/simple_menu_model.cc:85: #if defined(OS_WINDOWS)
if !defined(USE_AURA) right?

Also, instead of the DCHECK make it a NOTIMPLEMENTED.

http://codereview.chromium.org/10837317/diff/1035/ui/base/models/simple_menu_...
ui/base/models/simple_menu_model.cc:148: #if defined(OS_WINDOWS)
Same comment about define.

http://codereview.chromium.org/10837317/diff/1035/ui/views/controls/menu/menu...
File ui/views/controls/menu/menu_config.cc (left):

http://codereview.chromium.org/10837317/diff/1035/ui/views/controls/menu/menu...
ui/views/controls/menu/menu_config.cc:45: if (ui::GetDisplayLayout() ==
ui::LAYOUT_TOUCH) {
I thought we still wanted this for windows?

http://codereview.chromium.org/10837317/diff/1035/ui/views/controls/menu/menu...
File ui/views/controls/menu/menu_config.cc (right):

http://codereview.chromium.org/10837317/diff/1035/ui/views/controls/menu/menu...
ui/views/controls/menu/menu_config.cc:62: #if defined(OS_CHROMEOS)
USE_AURA

http://codereview.chromium.org/10837317/diff/1035/ui/views/controls/menu/menu...
File ui/views/controls/menu/menu_config.h (right):

http://codereview.chromium.org/10837317/diff/1035/ui/views/controls/menu/menu...
ui/views/controls/menu/menu_config.h:28: // However - for now only ChromeOS will
use it in all situations and the
aura

http://codereview.chromium.org/10837317/diff/1035/ui/views/controls/menu/menu...
File ui/views/controls/menu/menu_separator.h (right):

http://codereview.chromium.org/10837317/diff/1035/ui/views/controls/menu/menu...
ui/views/controls/menu/menu_separator.h:16: MenuSeparator(ui::MenuSeparatorStyle
type) : type_(type) {}
explicit

http://codereview.chromium.org/10837317/diff/1035/ui/views/controls/menu/menu...
ui/views/controls/menu/menu_separator.h:24: ui::MenuSeparatorStyle type_;
const

http://codereview.chromium.org/10837317/diff/1035/ui/views/controls/menu/menu...
File ui/views/controls/menu/menu_separator_views.cc (right):

http://codereview.chromium.org/10837317/diff/1035/ui/views/controls/menu/menu...
ui/views/controls/menu/menu_separator_views.cc:8: #include
"ui/base/models/menu_model.h"
remove this import

http://codereview.chromium.org/10837317/diff/1035/ui/views/controls/menu/menu...
ui/views/controls/menu/menu_separator_views.cc:41: int height =
MenuConfig::instance().separator_height;
Add 3 constants for the specific separator types.

[sky, 2012-08-21 15:27:55]

http://codereview.chromium.org/10837317/diff/1035/ui/base/models/menu_model.h
File ui/base/models/menu_model.h (right):

http://codereview.chromium.org/10837317/diff/1035/ui/base/models/menu_model.h...
ui/base/models/menu_model.h:62: virtual ui::MenuSeparatorStyle
GetSeparatorStyleAt(int index) const = 0;
On 2012/08/21 03:03:25, sky wrote:
> Why put this in a separate file instead of inside this class like the other?
I'm
> ok with the separate file as long you move ItemType too. It need not be with
> this patch. I would name the file menu_types too.

Leave this as you have it. While I prefer MenuSeparatorType, MenuSeparatorStyle
is fine. We can also move ItemType at a later date if we want to.

[Mr4D (OOO till 08-26), 2012-08-21 15:37:15]

Addressed. Have another look.

http://codereview.chromium.org/10837317/diff/1035/ui/base/models/menu_model.h
File ui/base/models/menu_model.h (right):

http://codereview.chromium.org/10837317/diff/1035/ui/base/models/menu_model.h...
ui/base/models/menu_model.h:62: virtual ui::MenuSeparatorStyle
GetSeparatorStyleAt(int index) const = 0;
Because in one of my latest CL's I was asked to do the same thing to avoid
including the header into so many other files. This enum shows exactly the same
problems so I didn't wait for the reviewer to tell me to do so. In contrast to
ItemType however MenuSeparatorStyle is used also in MenuItemView (and thus in a
lot more places). If you still want to get that done - let me know and I do it
in this CL.

http://codereview.chromium.org/10837317/diff/1035/ui/base/models/separator_ty...
File ui/base/models/separator_types.h (right):

http://codereview.chromium.org/10837317/diff/1035/ui/base/models/separator_ty...
ui/base/models/separator_types.h:11: enum MenuSeparatorStyle {
On 2012/08/21 03:03:25, sky wrote:
> MenuSeparatorType.

Done.

http://codereview.chromium.org/10837317/diff/1035/ui/base/models/separator_ty...
ui/base/models/separator_types.h:12: NO_SEPARATOR = 0,
To keep it clean I was adding the NO_SEPARATOR. But sure - I can remove this.

http://codereview.chromium.org/10837317/diff/1035/ui/base/models/simple_menu_...
File ui/base/models/simple_menu_model.cc (right):

http://codereview.chromium.org/10837317/diff/1035/ui/base/models/simple_menu_...
ui/base/models/simple_menu_model.cc:85: #if defined(OS_WINDOWS)
Actually - no? The new separator style should be used for any version of the
wrench menu. The only 'version' which cannot handle it is Windows, because the
separator is there used from the normal menu and therefore fixed. Please correct
me if this has changed.

Beside that: done.

http://codereview.chromium.org/10837317/diff/1035/ui/base/models/simple_menu_...
ui/base/models/simple_menu_model.cc:148: #if defined(OS_WINDOWS)
On 2012/08/21 03:03:25, sky wrote:
> Same comment about define.

Done.

http://codereview.chromium.org/10837317/diff/1035/ui/views/controls/menu/menu...
File ui/views/controls/menu/menu_config.cc (left):

http://codereview.chromium.org/10837317/diff/1035/ui/views/controls/menu/menu...
ui/views/controls/menu/menu_config.cc:45: if (ui::GetDisplayLayout() ==
ui::LAYOUT_TOUCH) {
I added this explicitly for the wrench menu. However - I do not have a Windows
system to test it out, and it seems safer to keep in then since the separator
changes do not apply there. Will add back in (for Windows only).

http://codereview.chromium.org/10837317/diff/1035/ui/views/controls/menu/menu...
File ui/views/controls/menu/menu_config.cc (right):

http://codereview.chromium.org/10837317/diff/1035/ui/views/controls/menu/menu...
ui/views/controls/menu/menu_config.cc:62: #if defined(OS_CHROMEOS)
As stated earlier - I am not so sure if this is correct, but you should know.
Done.

http://codereview.chromium.org/10837317/diff/1035/ui/views/controls/menu/menu...
File ui/views/controls/menu/menu_config.h (right):

http://codereview.chromium.org/10837317/diff/1035/ui/views/controls/menu/menu...
ui/views/controls/menu/menu_config.h:28: // However - for now only ChromeOS will
use it in all situations and the
Not entirely sure if that is (still) the same thing, but you should know it
best. Done.

http://codereview.chromium.org/10837317/diff/1035/ui/views/controls/menu/menu...
File ui/views/controls/menu/menu_separator.h (right):

http://codereview.chromium.org/10837317/diff/1035/ui/views/controls/menu/menu...
ui/views/controls/menu/menu_separator.h:16: MenuSeparator(ui::MenuSeparatorStyle
type) : type_(type) {}
On 2012/08/21 03:03:25, sky wrote:
> explicit

Done.

http://codereview.chromium.org/10837317/diff/1035/ui/views/controls/menu/menu...
ui/views/controls/menu/menu_separator.h:24: ui::MenuSeparatorStyle type_;
On 2012/08/21 03:03:25, sky wrote:
> const

Done.

http://codereview.chromium.org/10837317/diff/1035/ui/views/controls/menu/menu...
File ui/views/controls/menu/menu_separator_views.cc (right):

http://codereview.chromium.org/10837317/diff/1035/ui/views/controls/menu/menu...
ui/views/controls/menu/menu_separator_views.cc:8: #include
"ui/base/models/menu_model.h"
On 2012/08/21 03:03:25, sky wrote:
> remove this import

Done.

http://codereview.chromium.org/10837317/diff/1035/ui/views/controls/menu/menu...
ui/views/controls/menu/menu_separator_views.cc:41: int height =
MenuConfig::instance().separator_height;
On 2012/08/21 03:03:25, sky wrote:
> Add 3 constants for the specific separator types.

Done.

[sky, 2012-08-21 19:36:29]

http://codereview.chromium.org/10837317/diff/1035/ui/base/models/simple_menu_...
File ui/base/models/simple_menu_model.cc (right):

http://codereview.chromium.org/10837317/diff/1035/ui/base/models/simple_menu_...
ui/base/models/simple_menu_model.cc:85: #if defined(OS_WINDOWS)
On 2012/08/21 15:37:16, Mr4D wrote:
> Actually - no? The new separator style should be used for any version of the
> wrench menu. The only 'version' which cannot handle it is Windows, because the
> separator is there used from the normal menu and therefore fixed. Please
correct
> me if this has changed.
> 
> Beside that: done.

This is what NOTIMPLEMENTED is meant for, it means this code path isn't current
implemented but should be at some point.

http://codereview.chromium.org/10837317/diff/9001/chrome/browser/ui/views/wre...
File chrome/browser/ui/views/wrench_menu.cc (right):

http://codereview.chromium.org/10837317/diff/9001/chrome/browser/ui/views/wre...
chrome/browser/ui/views/wrench_menu.cc:964: height = kTouchItemHeight;
Rename this constant since it isn't just for touch anymore.

http://codereview.chromium.org/10837317/diff/9001/ui/base/models/menu_model.cc
File ui/base/models/menu_model.cc (right):

http://codereview.chromium.org/10837317/diff/9001/ui/base/models/menu_model.c...
ui/base/models/menu_model.cc:7: #include "base/utf_string_conversions.h"
Remove this include.

http://codereview.chromium.org/10837317/diff/9001/ui/base/models/menu_model.h
File ui/base/models/menu_model.h (right):

http://codereview.chromium.org/10837317/diff/9001/ui/base/models/menu_model.h...
ui/base/models/menu_model.h:62: virtual ui::MenuSeparatorType
GetSeparatorStyleAt(int index) const = 0;
Method should name return value: GetSeparatorTypeAt.

http://codereview.chromium.org/10837317/diff/9001/ui/base/models/separator_ty...
File ui/base/models/separator_types.h (right):

http://codereview.chromium.org/10837317/diff/9001/ui/base/models/separator_ty...
ui/base/models/separator_types.h:5: #ifndef UI_BASE_MODELS_SEPARATOR_TYPES_H_
This file should either be menu_types or menu_separator_types.

http://codereview.chromium.org/10837317/diff/9001/ui/base/models/separator_ty...
ui/base/models/separator_types.h:13: NORMAL_SEPARATOR = 0,
Is there a reason you're explicitly setting to 0? Typically we only set a
specific value if it is needed for something (saying writing to disk). I don't
think that's the case here.

http://codereview.chromium.org/10837317/diff/9001/ui/base/models/simple_menu_...
File ui/base/models/simple_menu_model.cc (right):

http://codereview.chromium.org/10837317/diff/9001/ui/base/models/simple_menu_...
ui/base/models/simple_menu_model.cc:85: #if defined(OS_WIN)
See earlier comment, !USE_AURA

http://codereview.chromium.org/10837317/diff/9001/ui/base/models/simple_menu_...
ui/base/models/simple_menu_model.cc:150: #if defined(OS_WIN)
Same thing here, !OS_AURA

http://codereview.chromium.org/10837317/diff/9001/ui/views/controls/menu/menu...
File ui/views/controls/menu/menu_config.cc (right):

http://codereview.chromium.org/10837317/diff/9001/ui/views/controls/menu/menu...
ui/views/controls/menu/menu_config.cc:47: #if defined (OS_WIN)
Do you even need the ifdef here? We don't use ui::LAYOUT_TOUCH on chromeos
anymore.

http://codereview.chromium.org/10837317/diff/9001/ui/views/controls/menu/menu...
ui/views/controls/menu/menu_config.cc:75: return ui::GetDisplayLayout() ==
ui::LAYOUT_TOUCH;
Why do we need this else?

http://codereview.chromium.org/10837317/diff/9001/ui/views/controls/menu/menu...
File ui/views/controls/menu/menu_config.h (right):

http://codereview.chromium.org/10837317/diff/9001/ui/views/controls/menu/menu...
ui/views/controls/menu/menu_config.h:27: // Going forward, every platform will
use the new style menu.
You don't need to get into the specific here, something like:

Returns true if the new menu style is used.

is fine, with a:

// TODO(skuhne): fix other OSs and remove this.

http://codereview.chromium.org/10837317/diff/9001/ui/views/controls/menu/menu...
File ui/views/controls/menu/menu_model_adapter_unittest.cc (right):

http://codereview.chromium.org/10837317/diff/9001/ui/views/controls/menu/menu...
ui/views/controls/menu/menu_model_adapter_unittest.cc:56: ui::NORMAL_SEPARATOR :
ui::NORMAL_SEPARATOR;
Both values return ui::NORMAL_SEPARATOR here.

http://codereview.chromium.org/10837317/diff/9001/ui/views/controls/menu/menu...
File ui/views/controls/menu/menu_separator_views.cc (right):

http://codereview.chromium.org/10837317/diff/9001/ui/views/controls/menu/menu...
ui/views/controls/menu/menu_separator_views.cc:23:
ui::NativeTheme::instance()->GetSystemColor(
Since all branches get the color, move it earlier on.

[Mr4D (OOO till 08-26), 2012-08-21 21:34:54]

Addressed. Please have another look!

http://codereview.chromium.org/10837317/diff/1035/ui/base/models/simple_menu_...
File ui/base/models/simple_menu_model.cc (right):

http://codereview.chromium.org/10837317/diff/1035/ui/base/models/simple_menu_...
ui/base/models/simple_menu_model.cc:85: #if defined(OS_WINDOWS)
No - you got me wrong here. I was not referring to NOTIMPLEMENTED. I referred to
USE_AURA. I thought that the changes should only be visible on ChromeOS and all
other OS'ses should remain as they are. Especially the flavor you were working
on last month.

http://codereview.chromium.org/10837317/diff/9001/chrome/browser/ui/views/wre...
File chrome/browser/ui/views/wrench_menu.cc (right):

http://codereview.chromium.org/10837317/diff/9001/chrome/browser/ui/views/wre...
chrome/browser/ui/views/wrench_menu.cc:964: height = kTouchItemHeight;
In deed! Done.

http://codereview.chromium.org/10837317/diff/9001/ui/base/models/menu_model.cc
File ui/base/models/menu_model.cc (right):

http://codereview.chromium.org/10837317/diff/9001/ui/base/models/menu_model.c...
ui/base/models/menu_model.cc:7: #include "base/utf_string_conversions.h"
On 2012/08/21 19:36:29, sky wrote:
> Remove this include.

Done.

http://codereview.chromium.org/10837317/diff/9001/ui/base/models/menu_model.h
File ui/base/models/menu_model.h (right):

http://codereview.chromium.org/10837317/diff/9001/ui/base/models/menu_model.h...
ui/base/models/menu_model.h:62: virtual ui::MenuSeparatorType
GetSeparatorStyleAt(int index) const = 0;
Hmpf. In deed. Changed. (sorry missed that).

http://codereview.chromium.org/10837317/diff/9001/ui/base/models/separator_ty...
File ui/base/models/separator_types.h (right):

http://codereview.chromium.org/10837317/diff/9001/ui/base/models/separator_ty...
ui/base/models/separator_types.h:5: #ifndef UI_BASE_MODELS_SEPARATOR_TYPES_H_
On 2012/08/21 19:36:29, sky wrote:
> This file should either be menu_types or menu_separator_types.

Done.

http://codereview.chromium.org/10837317/diff/9001/ui/base/models/separator_ty...
ui/base/models/separator_types.h:13: NORMAL_SEPARATOR = 0,
Not really. I guess I like at least one value clearly to be set. Removed.

http://codereview.chromium.org/10837317/diff/9001/ui/base/models/simple_menu_...
File ui/base/models/simple_menu_model.cc (right):

http://codereview.chromium.org/10837317/diff/9001/ui/base/models/simple_menu_...
ui/base/models/simple_menu_model.cc:85: #if defined(OS_WIN)
Done, but - I am not sure that this is correct. To my point of knowledge only
Windows cannot draw the separators as required. But again - I trust you that you
know these various flavors better then I do. (Sorry that I corrected that wrong,
somehow I must have reversed that change again while working on this for the
last review).

http://codereview.chromium.org/10837317/diff/9001/ui/base/models/simple_menu_...
ui/base/models/simple_menu_model.cc:150: #if defined(OS_WIN)
On 2012/08/21 19:36:29, sky wrote:
> Same thing here, !OS_AURA

Done.

http://codereview.chromium.org/10837317/diff/9001/ui/views/controls/menu/menu...
File ui/views/controls/menu/menu_config.cc (right):

http://codereview.chromium.org/10837317/diff/9001/ui/views/controls/menu/menu...
ui/views/controls/menu/menu_config.cc:47: #if defined (OS_WIN)
Well - fact wise you are right, but the code itself makes only sense for Windows
and it will be confusing for people looking at it otherwise. (not even
mentioning that it adds additional unneeded complexity). If you insist I remove
it of course.

http://codereview.chromium.org/10837317/diff/9001/ui/views/controls/menu/menu...
ui/views/controls/menu/menu_config.cc:75: return ui::GetDisplayLayout() ==
ui::LAYOUT_TOUCH;
Because I was told that for all other operating systems we want to keep
everything as it is. The lengthy comment for this function (which I have just
removed) was trying to explain this: I was told that for ChromeOS we always want
to have the new menu style, but for the moment we want to keep the menu for all
other OS'ses as it is. There were many eMails around this topic. So we should
really keep it as such...

http://codereview.chromium.org/10837317/diff/9001/ui/views/controls/menu/menu...
File ui/views/controls/menu/menu_config.h (right):

http://codereview.chromium.org/10837317/diff/9001/ui/views/controls/menu/menu...
ui/views/controls/menu/menu_config.h:27: // Going forward, every platform will
use the new style menu.
The TODO makes no sense. As long as there is no consensus how other other OS'ses
will be handled there is no TODO.

Done

http://codereview.chromium.org/10837317/diff/9001/ui/views/controls/menu/menu...
File ui/views/controls/menu/menu_model_adapter_unittest.cc (right):

http://codereview.chromium.org/10837317/diff/9001/ui/views/controls/menu/menu...
ui/views/controls/menu/menu_model_adapter_unittest.cc:56: ui::NORMAL_SEPARATOR :
ui::NORMAL_SEPARATOR;
Right. Happened after your last change request. While refactoring this function
slipped apparently through the cracks.

http://codereview.chromium.org/10837317/diff/9001/ui/views/controls/menu/menu...
File ui/views/controls/menu/menu_separator_views.cc (right):

http://codereview.chromium.org/10837317/diff/9001/ui/views/controls/menu/menu...
ui/views/controls/menu/menu_separator_views.cc:23:
ui::NativeTheme::instance()->GetSystemColor(
Not done it that way since that would have read the color for the spacing only
case as well. Instead I was using the position and moved the entire command to
the end.

[sky, 2012-08-21 22:35:22]

Now I'm a bit confused by your changes. Some of them will effect windows when in
a touch environment. Is that expected?

http://codereview.chromium.org/10837317/diff/1035/ui/base/models/simple_menu_...
File ui/base/models/simple_menu_model.cc (right):

http://codereview.chromium.org/10837317/diff/1035/ui/base/models/simple_menu_...
ui/base/models/simple_menu_model.cc:85: #if defined(OS_WINDOWS)
On 2012/08/21 21:34:54, Mr4D wrote:
> No - you got me wrong here. I was not referring to NOTIMPLEMENTED. I referred
to
> USE_AURA. I thought that the changes should only be visible on ChromeOS and
all
> other OS'ses should remain as they are. Especially the flavor you were working
> on last month.

Agreed. My first comment was this define should be !defined(USE_AURA), which is
what you have now.

http://codereview.chromium.org/10837317/diff/9001/ui/base/models/simple_menu_...
File ui/base/models/simple_menu_model.cc (right):

http://codereview.chromium.org/10837317/diff/9001/ui/base/models/simple_menu_...
ui/base/models/simple_menu_model.cc:85: #if defined(OS_WIN)
On 2012/08/21 21:34:54, Mr4D wrote:
> Done, but - I am not sure that this is correct. To my point of knowledge only
> Windows cannot draw the separators as required. But again - I trust you that
you
> know these various flavors better then I do. (Sorry that I corrected that
wrong,
> somehow I must have reversed that change again while working on this for the
> last review).

This class is used every where and it seems as though you're only supporting it
for aura. That's why I said !USE_AURA.

http://codereview.chromium.org/10837317/diff/10040/ui/views/controls/menu/men...
File ui/views/controls/menu/menu_config.cc (right):

http://codereview.chromium.org/10837317/diff/10040/ui/views/controls/menu/men...
ui/views/controls/menu/menu_config.cc:75: return ui::GetDisplayLayout() ==
ui::LAYOUT_TOUCH;
Window will hit this code path. I thought you only wanted the changes on
chromeos?

[sky, 2012-08-21 22:53:32]

http://codereview.chromium.org/10837317/diff/11013/ui/views/controls/menu/men...
File ui/views/controls/menu/menu_config.cc (right):

http://codereview.chromium.org/10837317/diff/11013/ui/views/controls/menu/men...
ui/views/controls/menu/menu_config.cc:47: #if defined (OS_WIN)
THIS IS WRONG! YOU DON"T NEED THE IFDEF!

[Mr4D (OOO till 08-26), 2012-08-21 23:16:01]

Added. As wished. Getting the buildbots building everything now and asked Carlos
to build it on Win to see how it looks.

http://codereview.chromium.org/10837317/diff/11013/ui/views/controls/menu/men...
File ui/views/controls/menu/menu_config.cc (right):

http://codereview.chromium.org/10837317/diff/11013/ui/views/controls/menu/men...
ui/views/controls/menu/menu_config.cc:47: #if defined (OS_WIN)
On 2012/08/21 22:53:32, sky wrote:
> THIS IS WRONG! YOU DON"T NEED THE IFDEF!

Done.

[sky, 2012-08-23 16:58:51]

http://codereview.chromium.org/10837317/diff/26002/chrome/browser/ui/toolbar/...
File chrome/browser/ui/toolbar/wrench_menu_model.cc (right):

http://codereview.chromium.org/10837317/diff/26002/chrome/browser/ui/toolbar/...
chrome/browser/ui/toolbar/wrench_menu_model.cc:59: #include
"ui/views/controls/menu/menu_config.h"
This is views specific, so you'll need to ifdef this.

[Mr4D (OOO till 08-26), 2012-08-23 17:46:31]

Done.

http://codereview.chromium.org/10837317/diff/26002/chrome/browser/ui/toolbar/...
File chrome/browser/ui/toolbar/wrench_menu_model.cc (right):

http://codereview.chromium.org/10837317/diff/26002/chrome/browser/ui/toolbar/...
chrome/browser/ui/toolbar/wrench_menu_model.cc:59: #include
"ui/views/controls/menu/menu_config.h"
On 2012/08/23 16:58:51, sky wrote:
> This is views specific, so you'll need to ifdef this.

Done.

[sky, 2012-08-23 19:59:17]

As long as you verified this is what we want on windows too: LGTM

[commit-bot: I haz the power, 2012-08-23 21:31:45]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/skuhne@chromium.org/10837317/17013

[commit-bot: I haz the power, 2012-08-23 23:30:21]

Try job failure for 10837317-17013 (retry) on linux_chromeos for step
"unit_tests".
It's a second try, previously, step "unit_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[commit-bot: I haz the power, 2012-08-23 23:31:12]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/skuhne@chromium.org/10837317/14089

[commit-bot: I haz the power, 2012-08-23 23:31:30]

Failed to apply patch for
chrome/browser/ui/ash/launcher/launcher_context_menu.cc:
While running patch -p1 --forward --force;
patching file chrome/browser/ui/ash/launcher/launcher_context_menu.cc
Hunk #1 FAILED at 27.
Hunk #2 succeeded at 65 (offset 4 lines).
1 out of 2 hunks FAILED -- saving rejects to file
chrome/browser/ui/ash/launcher/launcher_context_menu.cc.rej

[commit-bot: I haz the power, 2012-08-24 00:15:22]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/skuhne@chromium.org/10837317/18014

[commit-bot: I haz the power, 2012-08-24 04:29:12]

Change committed as 153157