Added MenuConfig setter to MenuItemView, updated rest of the code to use set MenuConfig if it avail…

Added MenuConfig setter to MenuItemView, updated rest of the code to use set MenuConfig if it available.


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

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

[sky, 2012-10-05 21:33:40]

http://codereview.chromium.org/11026076/diff/1/ui/views/controls/menu/menu_it...
File ui/views/controls/menu/menu_item_view.cc (right):

http://codereview.chromium.org/11026076/diff/1/ui/views/controls/menu/menu_it...
ui/views/controls/menu/menu_item_view.cc:620: SetTitle(ASCIIToUTF16("blah")); 
// Text doesn't matter here.
I don't like doing this, it may have side effects.

http://codereview.chromium.org/11026076/diff/1/ui/views/controls/menu/menu_it...
File ui/views/controls/menu/menu_item_view.h (right):

http://codereview.chromium.org/11026076/diff/1/ui/views/controls/menu/menu_it...
ui/views/controls/menu/menu_item_view.h:324: void set_menu_config(MenuConfig*
menu_config) { menu_config_ = menu_config; }
This approach encourages leaking. MenuItemView should own the MenuConfig.

[yefimt, 2012-10-08 20:02:47]

http://codereview.chromium.org/11026076/diff/1/ui/views/controls/menu/menu_it...
File ui/views/controls/menu/menu_item_view.cc (right):

http://codereview.chromium.org/11026076/diff/1/ui/views/controls/menu/menu_it...
ui/views/controls/menu/menu_item_view.cc:620: SetTitle(ASCIIToUTF16("blah")); 
// Text doesn't matter here.
OK, I think this should be better

On 2012/10/05 21:33:40, sky wrote:
> I don't like doing this, it may have side effects.

http://codereview.chromium.org/11026076/diff/1/ui/views/controls/menu/menu_it...
File ui/views/controls/menu/menu_item_view.h (right):

http://codereview.chromium.org/11026076/diff/1/ui/views/controls/menu/menu_it...
ui/views/controls/menu/menu_item_view.h:324: void set_menu_config(MenuConfig*
menu_config) { menu_config_ = menu_config; }
On 2012/10/05 21:33:40, sky wrote:
> This approach encourages leaking. MenuItemView should own the MenuConfig.

Yes, was going to do it but forgot :(
The best would be reference counting, but i didn't find suitable class. So now
MenuItem owns menu_config, only any new Menu items have to have parent set.

[sky, 2012-10-08 21:32:25]

http://codereview.chromium.org/11026076/diff/11001/chrome/browser/ui/views/ac...
File chrome/browser/ui/views/action_box_menu.cc (right):

http://codereview.chromium.org/11026076/diff/11001/chrome/browser/ui/views/ac...
chrome/browser/ui/views/action_box_menu.cc:25: struct ActionBoxMenuConfig :
public views::MenuConfig {
MenuConfig isn't set up for subclassing for two reasons:
. there's no virtual destructor, so this code leaks.
. The state is set in Create(), not the constructor.

I don't think we need to subclass MenuConfig. Instead create a MenuConfig and
set the values you need to override. In order to do this you need to move
functionality from MenuConfig::Create to the constructor. That will likely
enable you to get rid of all the static methods there now
(Reset/Create/instance).

http://codereview.chromium.org/11026076/diff/11001/chrome/browser/ui/views/ac...
chrome/browser/ui/views/action_box_menu.cc:34: views::MenuConfig* menu_config_;
There's no need for this now, right?

http://codereview.chromium.org/11026076/diff/11001/ui/views/controls/menu/men...
File ui/views/controls/menu/menu_item_view.h (right):

http://codereview.chromium.org/11026076/diff/11001/ui/views/controls/menu/men...
ui/views/controls/menu/menu_item_view.h:513: const MenuConfig* menu_config_;
scoped_ptr

[yefimt, 2012-10-08 22:32:00]

http://codereview.chromium.org/11026076/diff/11001/chrome/browser/ui/views/ac...
File chrome/browser/ui/views/action_box_menu.cc (right):

http://codereview.chromium.org/11026076/diff/11001/chrome/browser/ui/views/ac...
chrome/browser/ui/views/action_box_menu.cc:25: struct ActionBoxMenuConfig :
public views::MenuConfig {
On 2012/10/08 21:32:26, sky wrote:
> MenuConfig isn't set up for subclassing for two reasons:
> . there's no virtual destructor, so this code leaks.
> . The state is set in Create(), not the constructor.
> 
> I don't think we need to subclass MenuConfig. Instead create a MenuConfig and
> set the values you need to override. In order to do this you need to move
> functionality from MenuConfig::Create to the constructor. That will likely
> enable you to get rid of all the static methods there now
> (Reset/Create/instance).

Done.

http://codereview.chromium.org/11026076/diff/11001/chrome/browser/ui/views/ac...
chrome/browser/ui/views/action_box_menu.cc:34: views::MenuConfig* menu_config_;
On 2012/10/08 21:32:26, sky wrote:
> There's no need for this now, right?

Done.

http://codereview.chromium.org/11026076/diff/11001/ui/views/controls/menu/men...
File ui/views/controls/menu/menu_item_view.h (right):

http://codereview.chromium.org/11026076/diff/11001/ui/views/controls/menu/men...
ui/views/controls/menu/menu_item_view.h:513: const MenuConfig* menu_config_;
On 2012/10/08 21:32:26, sky wrote:
> scoped_ptr

Done.

[sky, 2012-10-09 00:40:42]

LGTM

http://codereview.chromium.org/11026076/diff/14002/ui/views/controls/menu/men...
File ui/views/controls/menu/menu_config.h (right):

http://codereview.chromium.org/11026076/diff/14002/ui/views/controls/menu/men...
ui/views/controls/menu/menu_config.h:126: // Creates and configures a new
MenuConfig as appropriate for the current
Update comment.

http://codereview.chromium.org/11026076/diff/14002/ui/views/controls/menu/men...
File ui/views/controls/menu/menu_item_view.h (right):

http://codereview.chromium.org/11026076/diff/14002/ui/views/controls/menu/men...
ui/views/controls/menu/menu_item_view.h:323: // Sets MenuConfig to be used
instead of default one.
Document NULL resets to default.

[yefimt, 2012-10-09 00:53:25]

http://codereview.chromium.org/11026076/diff/14002/ui/views/controls/menu/men...
File ui/views/controls/menu/menu_config.h (right):

http://codereview.chromium.org/11026076/diff/14002/ui/views/controls/menu/men...
ui/views/controls/menu/menu_config.h:126: // Creates and configures a new
MenuConfig as appropriate for the current
On 2012/10/09 00:40:42, sky wrote:
> Update comment.

Done.

http://codereview.chromium.org/11026076/diff/14002/ui/views/controls/menu/men...
File ui/views/controls/menu/menu_item_view.h (right):

http://codereview.chromium.org/11026076/diff/14002/ui/views/controls/menu/men...
ui/views/controls/menu/menu_item_view.h:323: // Sets MenuConfig to be used
instead of default one.
On 2012/10/09 00:40:42, sky wrote:
> Document NULL resets to default.

Done.

[commit-bot: I haz the power, 2012-10-09 00:53:41]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yefim@chromium.org/11026076/17023

[commit-bot: I haz the power, 2012-10-09 01:22:46]

Sorry for I got bad news for ya.
Compile failed with a clobber build.
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2012-10-09 16:53:36]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yefim@chromium.org/11026076/14023

[commit-bot: I haz the power, 2012-10-09 18:56:11]

Change committed as 160886