Convert NativeTextFieldViews context menu from Menu2 to MenuItemView.

Convert NativeTextFieldViews context menu from Menu2 to MenuItemView.

BUG=chromium-os:13887
TEST=none


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

[oshima, 2011-06-03 17:06:23]

http://codereview.chromium.org/7104013/diff/2001/views/controls/textfield/nat...
File views/controls/textfield/native_textfield_views.cc (right):

http://codereview.chromium.org/7104013/diff/2001/views/controls/textfield/nat...
views/controls/textfield/native_textfield_views.cc:268: views::MenuItemView
menu(&menu_delegate);
Any reason to change from member variable to stack? (especially menu doesn't
change in the textfield)

http://codereview.chromium.org/7104013/diff/2001/views/controls/textfield/nat...
views/controls/textfield/native_textfield_views.cc:272:
views::MenuItemView::TOPLEFT,
RTL?

[rhashimoto, 2011-06-03 19:20:43]

http://codereview.chromium.org/7104013/diff/2001/views/controls/textfield/nat...
File views/controls/textfield/native_textfield_views.cc (right):

http://codereview.chromium.org/7104013/diff/2001/views/controls/textfield/nat...
views/controls/textfield/native_textfield_views.cc:268: views::MenuItemView
menu(&menu_delegate);
On 2011/06/03 17:06:23, oshima wrote:
> Any reason to change from member variable to stack? (especially menu doesn't
> change in the textfield)

The menu needs to be updated because item enabled state changes depending on if
anything is selected.  I've moved the adapter and the menu back into class
members and renamed InitContextMenuIfRequired() to UpdateContextMenu().

http://codereview.chromium.org/7104013/diff/2001/views/controls/textfield/nat...
views/controls/textfield/native_textfield_views.cc:272:
views::MenuItemView::TOPLEFT,
On 2011/06/03 17:06:23, oshima wrote:
> RTL?

Should I make it TOPRIGHT for RTL?  Menu2 context menus are always TOPLEFT so
that's why I chose that, to preserve the previous behavior.  I can make it
depend on IsRTL() if that is better.

[oshima, 2011-06-03 21:44:39]

On 2011/06/03 19:20:43, rhashimoto wrote:
>
http://codereview.chromium.org/7104013/diff/2001/views/controls/textfield/nat...
> File views/controls/textfield/native_textfield_views.cc (right):
> 
>
http://codereview.chromium.org/7104013/diff/2001/views/controls/textfield/nat...
> views/controls/textfield/native_textfield_views.cc:268: views::MenuItemView
> menu(&menu_delegate);
> On 2011/06/03 17:06:23, oshima wrote:
> > Any reason to change from member variable to stack? (especially menu doesn't
> > change in the textfield)
> 
> The menu needs to be updated because item enabled state changes depending on
if
> anything is selected.  I've moved the adapter and the menu back into class
> members and renamed InitContextMenuIfRequired() to UpdateContextMenu().

Ah, it doesn't like Rebuild like menu2 does. ok.

>
http://codereview.chromium.org/7104013/diff/2001/views/controls/textfield/nat...
> views/controls/textfield/native_textfield_views.cc:272:
> views::MenuItemView::TOPLEFT,
> On 2011/06/03 17:06:23, oshima wrote:
> > RTL?
> 
> Should I make it TOPRIGHT for RTL?  Menu2 context menus are always TOPLEFT so
> that's why I chose that, to preserve the previous behavior.  I can make it
> depend on IsRTL() if that is better.

Menu2 handles RTL positioning inside native_menu_gtk.cc but
*I believe* views menu doesn't. Can you double check? May be we should add that
to views menu?

[rhashimoto, 2011-06-06 16:45:39]

On 2011/06/03 21:44:39, oshima wrote:
>
http://codereview.chromium.org/7104013/diff/2001/views/controls/textfield/nat...
> > views/controls/textfield/native_textfield_views.cc:272:
> > views::MenuItemView::TOPLEFT,
> > On 2011/06/03 17:06:23, oshima wrote:
> > > RTL?
> > 
> > Should I make it TOPRIGHT for RTL?  Menu2 context menus are always TOPLEFT
so
> > that's why I chose that, to preserve the previous behavior.  I can make it
> > depend on IsRTL() if that is better.
> 
> Menu2 handles RTL positioning inside native_menu_gtk.cc but
> *I believe* views menu doesn't. Can you double check? May be we should add
that
> to views menu?

The views menu doesn't seem to change alignment for the top level menu (RTL does
affect positioning of submenus).

It seems wrong to me to specify TOPLEFT and have the menu use TOPRIGHT based on
some other state.  The enums should be named something else if that is the
behavior.  I would rather check RTL state at the site of the call.

[oshima, 2011-06-06 19:53:35]

On Mon, Jun 6, 2011 at 9:45 AM, <rhashimoto@chromium.org> wrote:

> On 2011/06/03 21:44:39, oshima wrote:
>
>
>
http://codereview.chromium.org/7104013/diff/2001/views/controls/textfield/nat...
>
>> > views/controls/textfield/native_textfield_views.cc:272:
>> > views::MenuItemView::TOPLEFT,
>> > On 2011/06/03 17:06:23, oshima wrote:
>> > > RTL?
>> >
>> > Should I make it TOPRIGHT for RTL?  Menu2 context menus are always
>> TOPLEFT
>>
> so
>
>> > that's why I chose that, to preserve the previous behavior.  I can make
>> it
>> > depend on IsRTL() if that is better.
>>
>
>  Menu2 handles RTL positioning inside native_menu_gtk.cc but
>> *I believe* views menu doesn't. Can you double check? May be we should add
>>
> that
>
>> to views menu?
>>
>
> The views menu doesn't seem to change alignment for the top level menu (RTL
> does
> affect positioning of submenus).
>
> It seems wrong to me to specify TOPLEFT and have the menu use TOPRIGHT
> based on
> some other state.  The enums should be named something else if that is the
> behavior.  I would rather check RTL state at the site of the call.


You can see it as an default direction used in LTR, which will be
reversed in RTL. That's how menu2 works and also in other places in views,
so
it's not so strange. Alternative way is to remove the parameter and
completely hide it
from client.

Doing this on client side means that it will be duplicated in several
places. Right now,
all views menu doesn't work properly in RTL (status  menus, system menu),
and there will be
at least to mores (textfield and contents), so I thin it's better for the
menu to handle it. (one way or another).



>
>
> http://codereview.chromium.org/7104013/
>

[rhashimoto, 2011-06-06 23:49:50]

It still doesn't feel right to me, but I defer to you and sky.  I'm reversing
the specified MenuItemView::AnchorPosition for RTL in a separate CL
(http://codereview.chromium.org/7115005/).

Thanks,
Roy

On 2011/06/06 19:53:35, oshima wrote:
> On Mon, Jun 6, 2011 at 9:45 AM, <mailto:rhashimoto@chromium.org> wrote:
> 
> > On 2011/06/03 21:44:39, oshima wrote:
> >
> >
> >
>
http://codereview.chromium.org/7104013/diff/2001/views/controls/textfield/nat...
> >
> >> > views/controls/textfield/native_textfield_views.cc:272:
> >> > views::MenuItemView::TOPLEFT,
> >> > On 2011/06/03 17:06:23, oshima wrote:
> >> > > RTL?
> >> >
> >> > Should I make it TOPRIGHT for RTL?  Menu2 context menus are always
> >> TOPLEFT
> >>
> > so
> >
> >> > that's why I chose that, to preserve the previous behavior.  I can make
> >> it
> >> > depend on IsRTL() if that is better.
> >>
> >
> >  Menu2 handles RTL positioning inside native_menu_gtk.cc but
> >> *I believe* views menu doesn't. Can you double check? May be we should add
> >>
> > that
> >
> >> to views menu?
> >>
> >
> > The views menu doesn't seem to change alignment for the top level menu (RTL
> > does
> > affect positioning of submenus).
> >
> > It seems wrong to me to specify TOPLEFT and have the menu use TOPRIGHT
> > based on
> > some other state.  The enums should be named something else if that is the
> > behavior.  I would rather check RTL state at the site of the call.
> 
> 
> You can see it as an default direction used in LTR, which will be
> reversed in RTL. That's how menu2 works and also in other places in views,
> so
> it's not so strange. Alternative way is to remove the parameter and
> completely hide it
> from client.
> 
> Doing this on client side means that it will be duplicated in several
> places. Right now,
> all views menu doesn't work properly in RTL (status  menus, system menu),
> and there will be
> at least to mores (textfield and contents), so I thin it's better for the
> menu to handle it. (one way or another).
> 
> 
> 
> >
> >
> > http://codereview.chromium.org/7104013/
> >

[oshima, 2011-06-07 17:21:44]

LGTM with one nit

http://codereview.chromium.org/7104013/diff/8004/views/controls/textfield/nat...
File views/controls/textfield/native_textfield_views.cc (right):

http://codereview.chromium.org/7104013/diff/8004/views/controls/textfield/nat...
views/controls/textfield/native_textfield_views.cc:264:
context_menu_menu_->RunMenuAt(GetWindow()->GetNativeWindow(), NULL,
move NULL to next line

[rhashimoto, 2011-06-07 21:07:22]

http://codereview.chromium.org/7104013/diff/8004/views/controls/textfield/nat...
File views/controls/textfield/native_textfield_views.cc (right):

http://codereview.chromium.org/7104013/diff/8004/views/controls/textfield/nat...
views/controls/textfield/native_textfield_views.cc:264:
context_menu_menu_->RunMenuAt(GetWindow()->GetNativeWindow(), NULL,
On 2011/06/07 17:21:44, oshima wrote:
> move NULL to next line

Done.

[commit-bot: I haz the power, 2011-06-08 16:34:11]

Change committed as 88340