Implementing context menu for the new views textfield.

Implementing context menu for the new views textfield.

BUG=none
TEST=right click on the new textfield should show the context menu and all commands on the menu should work.

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

[oshima, 2011-01-12 18:46:25]

http://codereview.chromium.org/6155011/diff/1/views/controls/textfield/native...
File views/controls/textfield/native_textfield_views.cc (right):

http://codereview.chromium.org/6155011/diff/1/views/controls/textfield/native...
views/controls/textfield/native_textfield_views.cc:82: if
(e.IsLeftMouseButton()) {
q: should this be "Only"?

http://codereview.chromium.org/6155011/diff/1/views/controls/textfield/native...
views/controls/textfield/native_textfield_views.cc:365: default:
NOTREACHED() << "unkonwn command:" << command_id;

http://codereview.chromium.org/6155011/diff/1/views/controls/textfield/native...
File views/controls/textfield/native_textfield_views.h (right):

http://codereview.chromium.org/6155011/diff/1/views/controls/textfield/native...
views/controls/textfield/native_textfield_views.h:1: // Copyright (c) 2010 The
Chromium Authors. All rights reserved.
2011  (other files too)

http://codereview.chromium.org/6155011/diff/1/views/controls/textfield/native...
views/controls/textfield/native_textfield_views.h:31: // * Context Menu.
remove this

http://codereview.chromium.org/6155011/diff/1/views/controls/textfield/native...
views/controls/textfield/native_textfield_views.h:168: void CreateContextMenu();
 CreateXXX is typically used to return instance, so I'd call it
"InitContextMenu'.

Or you can rename it to "EnsureContxtMenu" and include checks. This way, Test do
not have to check the scoped pointer.

http://codereview.chromium.org/6155011/diff/1/views/controls/textfield/native...
File views/controls/textfield/native_textfield_views_unittest.cc (right):

http://codereview.chromium.org/6155011/diff/1/views/controls/textfield/native...
views/controls/textfield/native_textfield_views_unittest.cc:415:
views::ViewsDelegate::views_delegate = new TestViewsDelegate();
don't you have to delete this?

http://codereview.chromium.org/6155011/diff/1/views/controls/textfield/native...
views/controls/textfield/native_textfield_views_unittest.cc:426: MenuWrapper*
MenuWrapper::CreateWrapper(Menu2* menu) {
why do we need this here?

[sadrul, 2011-01-12 19:09:16]

http://codereview.chromium.org/6155011/diff/1/views/controls/textfield/native...
File views/controls/textfield/native_textfield_views.cc (right):

http://codereview.chromium.org/6155011/diff/1/views/controls/textfield/native...
views/controls/textfield/native_textfield_views.cc:87: }
Are there plans for middle-click-to-paste? Or is that only if X selection is
used?

http://codereview.chromium.org/6155011/diff/1/views/controls/textfield/native...
views/controls/textfield/native_textfield_views.cc:777:
context_menu_contents_->AddItemWithStringId(IDS_APP_CUT, IDS_APP_CUT);
IDC_xxx should be used for command-id, and IDS_xxx should be used for string-id.
I notice that this is incorrect in native_textfield_win too. Perhaps fix both?

[rjkroege, 2011-01-12 19:16:01]

On 2011/01/12 19:09:16, sadrul wrote:
>
http://codereview.chromium.org/6155011/diff/1/views/controls/textfield/native...
> File views/controls/textfield/native_textfield_views.cc (right):
> 
>
http://codereview.chromium.org/6155011/diff/1/views/controls/textfield/native...
> views/controls/textfield/native_textfield_views.cc:87: }
> Are there plans for middle-click-to-paste? Or is that only if X selection is
> used?

AFAIK, the views text field will be used first only on chromeos (no click to
paste) and then windows (no click to paste) so this isn't a priority.

> 
>
http://codereview.chromium.org/6155011/diff/1/views/controls/textfield/native...
> views/controls/textfield/native_textfield_views.cc:777:
> context_menu_contents_->AddItemWithStringId(IDS_APP_CUT, IDS_APP_CUT);
> IDC_xxx should be used for command-id, and IDS_xxx should be used for
string-id.
> I notice that this is incorrect in native_textfield_win too. Perhaps fix both?

+1

[tfarina, 2011-01-12 19:19:19]

http://codereview.chromium.org/6155011/diff/1/views/controls/textfield/native...
File views/controls/textfield/native_textfield_views_unittest.cc (right):

http://codereview.chromium.org/6155011/diff/1/views/controls/textfield/native...
views/controls/textfield/native_textfield_views_unittest.cc:415:
views::ViewsDelegate::views_delegate = new TestViewsDelegate();
On 2011/01/12 18:46:25, oshima wrote:
> don't you have to delete this?

It seems, refer to this CL for a fix?
http://codereview.chromium.org/6132009/diff/1/views/controls/textfield/textfi...

[oshima, 2011-01-12 19:25:31]

http://codereview.chromium.org/6155011/diff/1/views/controls/textfield/native...
File views/controls/textfield/native_textfield_views.cc (right):

http://codereview.chromium.org/6155011/diff/1/views/controls/textfield/native...
views/controls/textfield/native_textfield_views.cc:87: }
On 2011/01/12 19:09:16, sadrul wrote:
> Are there plans for middle-click-to-paste? Or is that only if X selection is
> used?

It's X selection and different from cut&paste. I've heard that we don't want
x-selection, but not 100% sure.

[varunjain, 2011-01-12 19:59:04]

http://codereview.chromium.org/6155011/diff/1/views/controls/textfield/native...
File views/controls/textfield/native_textfield_views.cc (right):

http://codereview.chromium.org/6155011/diff/1/views/controls/textfield/native...
views/controls/textfield/native_textfield_views.cc:82: if
(e.IsLeftMouseButton()) {
On 2011/01/12 18:46:25, oshima wrote:
> q: should this be "Only"?

the behaviour of the current textfield in chrome's external build suggests not.
If left click is combined with any other button, only left click seems to be
effective.

http://codereview.chromium.org/6155011/diff/1/views/controls/textfield/native...
views/controls/textfield/native_textfield_views.cc:365: default:
On 2011/01/12 18:46:25, oshima wrote:
> NOTREACHED() << "unkonwn command:" << command_id;

Done.

http://codereview.chromium.org/6155011/diff/1/views/controls/textfield/native...
views/controls/textfield/native_textfield_views.cc:777:
context_menu_contents_->AddItemWithStringId(IDS_APP_CUT, IDS_APP_CUT);
On 2011/01/12 19:09:16, sadrul wrote:
> IDC_xxx should be used for command-id, and IDS_xxx should be used for
string-id.
> I notice that this is incorrect in native_textfield_win too. Perhaps fix both?

I was confused by this... IDCs are defined in chrome tree so I didnot want to
include a dependency here in views. The IDSs I have used here are from app/

http://codereview.chromium.org/6155011/diff/1/views/controls/textfield/native...
File views/controls/textfield/native_textfield_views.h (right):

http://codereview.chromium.org/6155011/diff/1/views/controls/textfield/native...
views/controls/textfield/native_textfield_views.h:1: // Copyright (c) 2010 The
Chromium Authors. All rights reserved.
On 2011/01/12 18:46:25, oshima wrote:
> 2011  (other files too)

These files were created in 2010... I think this is fine.

http://codereview.chromium.org/6155011/diff/1/views/controls/textfield/native...
views/controls/textfield/native_textfield_views.h:31: // * Context Menu.
On 2011/01/12 18:46:25, oshima wrote:
> remove this

Done.

http://codereview.chromium.org/6155011/diff/1/views/controls/textfield/native...
views/controls/textfield/native_textfield_views.h:168: void CreateContextMenu();
On 2011/01/12 18:46:25, oshima wrote:
>  CreateXXX is typically used to return instance, so I'd call it
> "InitContextMenu'.

Changed to Init

> 
> Or you can rename it to "EnsureContxtMenu" and include checks. This way, Test
do
> not have to check the scoped pointer.

Done

http://codereview.chromium.org/6155011/diff/1/views/controls/textfield/native...
File views/controls/textfield/native_textfield_views_unittest.cc (right):

http://codereview.chromium.org/6155011/diff/1/views/controls/textfield/native...
views/controls/textfield/native_textfield_views_unittest.cc:415:
views::ViewsDelegate::views_delegate = new TestViewsDelegate();
On 2011/01/12 19:19:19, tfarina wrote:
> On 2011/01/12 18:46:25, oshima wrote:
> > don't you have to delete this?
> 
> It seems, refer to this CL for a fix?
>
http://codereview.chromium.org/6132009/diff/1/views/controls/textfield/textfi...

Fixed... I cannot believe I did this again (especially because I reviewed that
CL this morning!!)... need coffee :(

http://codereview.chromium.org/6155011/diff/1/views/controls/textfield/native...
views/controls/textfield/native_textfield_views_unittest.cc:426: MenuWrapper*
MenuWrapper::CreateWrapper(Menu2* menu) {
On 2011/01/12 18:46:25, oshima wrote:
> why do we need this here?

for touchui=1 && chromeos=1, we do not get an implementation of CreateWrapper.
However, this is wrong in its current form. Fixed.

[tfarina, 2011-01-12 20:01:17]

http://codereview.chromium.org/6155011/diff/1/views/controls/textfield/native...
File views/controls/textfield/native_textfield_views.h (right):

http://codereview.chromium.org/6155011/diff/1/views/controls/textfield/native...
views/controls/textfield/native_textfield_views.h:1: // Copyright (c) 2010 The
Chromium Authors. All rights reserved.
On 2011/01/12 19:59:05, varunjain wrote:
> On 2011/01/12 18:46:25, oshima wrote:
> > 2011  (other files too)
> 
> These files were created in 2010... I think this is fine.

No, you have to update the copyright here, when you change the code.

[varunjain, 2011-01-12 20:13:47]

On 2011/01/12 20:01:17, tfarina wrote:
>
http://codereview.chromium.org/6155011/diff/1/views/controls/textfield/native...
> File views/controls/textfield/native_textfield_views.h (right):
> 
>
http://codereview.chromium.org/6155011/diff/1/views/controls/textfield/native...
> views/controls/textfield/native_textfield_views.h:1: // Copyright (c) 2010 The
> Chromium Authors. All rights reserved.
> On 2011/01/12 19:59:05, varunjain wrote:
> > On 2011/01/12 18:46:25, oshima wrote:
> > > 2011  (other files too)
> > 
> > These files were created in 2010... I think this is fine.
> 
> No, you have to update the copyright here, when you change the code.

Done.

[sadrul, 2011-01-12 20:24:59]

http://codereview.chromium.org/6155011/diff/1/views/controls/textfield/native...
File views/controls/textfield/native_textfield_views.cc (right):

http://codereview.chromium.org/6155011/diff/1/views/controls/textfield/native...
views/controls/textfield/native_textfield_views.cc:777:
context_menu_contents_->AddItemWithStringId(IDS_APP_CUT, IDS_APP_CUT);
On 2011/01/12 19:59:05, varunjain wrote:
> On 2011/01/12 19:09:16, sadrul wrote:
> > IDC_xxx should be used for command-id, and IDS_xxx should be used for
> string-id.
> > I notice that this is incorrect in native_textfield_win too. Perhaps fix
both?
> 
> I was confused by this... IDCs are defined in chrome tree so I didnot want to
> include a dependency here in views. The IDSs I have used here are from app/

I fixed up some cases of IDS_/IDC_ mixups in chrome/ a few weeks back, but the
case for views/ do seem more difficult enough to belong in a separate fix. I
wouldn't hold this against this CL.

[tfarina, 2011-01-12 20:26:48]

On 2011/01/12 20:24:59, sadrul wrote:
> I fixed up some cases of IDS_/IDC_ mixups in chrome/ a few weeks back, but the
> case for views/ do seem more difficult enough to belong in a separate fix. I
> wouldn't hold this against this CL.

+1, this seems to be beyond the scope of this CL.

[oshima, 2011-01-12 20:51:40]

On Wed, Jan 12, 2011 at 12:24 PM, <sadrul@chromium.org> wrote:

>
>
>
http://codereview.chromium.org/6155011/diff/1/views/controls/textfield/native...
> File views/controls/textfield/native_textfield_views.cc (right):
>
>
>
>
http://codereview.chromium.org/6155011/diff/1/views/controls/textfield/native...
> views/controls/textfield/native_textfield_views.cc:777:
> context_menu_contents_->AddItemWithStringId(IDS_APP_CUT, IDS_APP_CUT);
> On 2011/01/12 19:59:05, varunjain wrote:
>
>> On 2011/01/12 19:09:16, sadrul wrote:
>> > IDC_xxx should be used for command-id, and IDS_xxx should be used
>>
> for
>
>> string-id.
>> > I notice that this is incorrect in native_textfield_win too. Perhaps
>>
> fix both?
>
>  I was confused by this... IDCs are defined in chrome tree so I didnot
>>
> want to
>
>> include a dependency here in views. The IDSs I have used here are from
>>
> app/
>
> I fixed up some cases of IDS_/IDC_ mixups in chrome/ a few weeks back,
> but the case for views/ do seem more difficult enough to belong in a
> separate fix. I wouldn't hold this against this CL.

Agreed. Let's address this separately.

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

[oshima, 2011-01-12 21:01:47]

http://codereview.chromium.org/6155011/diff/1/views/controls/textfield/native...
File views/controls/textfield/native_textfield_views.h (right):

http://codereview.chromium.org/6155011/diff/1/views/controls/textfield/native...
views/controls/textfield/native_textfield_views.h:1: // Copyright (c) 2010 The
Chromium Authors. All rights reserved.
On 2011/01/12 20:01:22, tfarina wrote:
> On 2011/01/12 19:59:05, varunjain wrote:
> > On 2011/01/12 18:46:25, oshima wrote:
> > > 2011  (other files too)
> > 
> > These files were created in 2010... I think this is fine.
> 
> No, you have to update the copyright here, when you change the code.

tfarina is correct. chrome updates the copyright when modified. This is one of a
few differences from google3.

http://codereview.chromium.org/6155011/diff/1/views/controls/textfield/native...
File views/controls/textfield/native_textfield_views_unittest.cc (right):

http://codereview.chromium.org/6155011/diff/1/views/controls/textfield/native...
views/controls/textfield/native_textfield_views_unittest.cc:426: MenuWrapper*
MenuWrapper::CreateWrapper(Menu2* menu) {
On 2011/01/12 19:59:05, varunjain wrote:
> On 2011/01/12 18:46:25, oshima wrote:
> > why do we need this here?
> 
> for touchui=1 && chromeos=1, we do not get an implementation of CreateWrapper.
> However, this is wrong in its current form. Fixed.

That sounds like general issue with touchui&chromeos=1 build. Shouldn't we fix
it in native_menu_x.cc? What would happen to menu in chrome if I build with
touchui=1 and chromeos=1?

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

http://codereview.chromium.org/6155011/diff/6002/views/controls/textfield/nat...
views/controls/textfield/native_textfield_views.cc:335: default:
NOTREACHED()

http://codereview.chromium.org/6155011/diff/6002/views/controls/textfield/nat...
views/controls/textfield/native_textfield_views.cc:338: return false;
won't compiler complain this? I think this is unreachable.

http://codereview.chromium.org/6155011/diff/6002/views/controls/textfield/nat...
File views/controls/textfield/native_textfield_views_unittest.cc (right):

http://codereview.chromium.org/6155011/diff/6002/views/controls/textfield/nat...
views/controls/textfield/native_textfield_views_unittest.cc:427:
views::ViewsDelegate::views_delegate = original_delegate;
can you use base/auto_reset.h ?

[varunjain, 2011-01-12 21:28:11]

http://codereview.chromium.org/6155011/diff/1/views/controls/textfield/native...
File views/controls/textfield/native_textfield_views.h (right):

http://codereview.chromium.org/6155011/diff/1/views/controls/textfield/native...
views/controls/textfield/native_textfield_views.h:1: // Copyright (c) 2010 The
Chromium Authors. All rights reserved.
On 2011/01/12 21:01:47, oshima wrote:
> On 2011/01/12 20:01:22, tfarina wrote:
> > On 2011/01/12 19:59:05, varunjain wrote:
> > > On 2011/01/12 18:46:25, oshima wrote:
> > > > 2011  (other files too)
> > > 
> > > These files were created in 2010... I think this is fine.
> > 
> > No, you have to update the copyright here, when you change the code.
> 
> tfarina is correct. chrome updates the copyright when modified. This is one of
a
> few differences from google3.

Done.

http://codereview.chromium.org/6155011/diff/1/views/controls/textfield/native...
File views/controls/textfield/native_textfield_views_unittest.cc (right):

http://codereview.chromium.org/6155011/diff/1/views/controls/textfield/native...
views/controls/textfield/native_textfield_views_unittest.cc:426: MenuWrapper*
MenuWrapper::CreateWrapper(Menu2* menu) {
On 2011/01/12 21:01:47, oshima wrote:
> On 2011/01/12 19:59:05, varunjain wrote:
> > On 2011/01/12 18:46:25, oshima wrote:
> > > why do we need this here?
> > 
> > for touchui=1 && chromeos=1, we do not get an implementation of
CreateWrapper.
> > However, this is wrong in its current form. Fixed.
> 
> That sounds like general issue with touchui&chromeos=1 build. Shouldn't we fix
> it in native_menu_x.cc? What would happen to menu in chrome if I build with
> touchui=1 and chromeos=1?

In the general case, when touchui=1&&chromeos=1, the CreateWrapper
implementation comes from chromeos/views/native_menu_domui.cc. However, this is
a views testcase and cannot depend on the file from chromeos tree. Hence I
specified the implementation here.

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

http://codereview.chromium.org/6155011/diff/6002/views/controls/textfield/nat...
views/controls/textfield/native_textfield_views.cc:335: default:
On 2011/01/12 21:01:47, oshima wrote:
> NOTREACHED()

Done.

http://codereview.chromium.org/6155011/diff/6002/views/controls/textfield/nat...
views/controls/textfield/native_textfield_views.cc:338: return false;
On 2011/01/12 21:01:47, oshima wrote:
> won't compiler complain this? I think this is unreachable.

the compiler doesnot seem to care. I removed it anyways

http://codereview.chromium.org/6155011/diff/6002/views/controls/textfield/nat...
File views/controls/textfield/native_textfield_views_unittest.cc (right):

http://codereview.chromium.org/6155011/diff/6002/views/controls/textfield/nat...
views/controls/textfield/native_textfield_views_unittest.cc:427:
views::ViewsDelegate::views_delegate = original_delegate;
On 2011/01/12 21:01:47, oshima wrote:
> can you use base/auto_reset.h ?

Done.

[oshima, 2011-01-12 21:39:56]

LGTM

On Wed, Jan 12, 2011 at 1:28 PM, <varunjain@chromium.org> wrote:

>
>
>
http://codereview.chromium.org/6155011/diff/1/views/controls/textfield/native...
> File views/controls/textfield/native_textfield_views.h (right):
>
>
>
http://codereview.chromium.org/6155011/diff/1/views/controls/textfield/native...
> views/controls/textfield/native_textfield_views.h:1: // Copyright (c)
> 2010 The Chromium Authors. All rights reserved.
> On 2011/01/12 21:01:47, oshima wrote:
>
>> On 2011/01/12 20:01:22, tfarina wrote:
>> > On 2011/01/12 19:59:05, varunjain wrote:
>> > > On 2011/01/12 18:46:25, oshima wrote:
>> > > > 2011  (other files too)
>> > >
>> > > These files were created in 2010... I think this is fine.
>> >
>> > No, you have to update the copyright here, when you change the code.
>>
>
>  tfarina is correct. chrome updates the copyright when modified. This
>>
> is one of a
>
>> few differences from google3.
>>
>
> Done.
>
>
>
>
http://codereview.chromium.org/6155011/diff/1/views/controls/textfield/native...
> File views/controls/textfield/native_textfield_views_unittest.cc
> (right):
>
>
>
http://codereview.chromium.org/6155011/diff/1/views/controls/textfield/native...
> views/controls/textfield/native_textfield_views_unittest.cc:426:
> MenuWrapper* MenuWrapper::CreateWrapper(Menu2* menu) {
> On 2011/01/12 21:01:47, oshima wrote:
>
>> On 2011/01/12 19:59:05, varunjain wrote:
>> > On 2011/01/12 18:46:25, oshima wrote:
>> > > why do we need this here?
>> >
>> > for touchui=1 && chromeos=1, we do not get an implementation of
>>
> CreateWrapper.
>
>> > However, this is wrong in its current form. Fixed.
>>
>
>  That sounds like general issue with touchui&chromeos=1 build.
>>
> Shouldn't we fix
>
>> it in native_menu_x.cc? What would happen to menu in chrome if I build
>>
> with
>
>> touchui=1 and chromeos=1?
>>
>
> In the general case, when touchui=1&&chromeos=1, the CreateWrapper
> implementation comes from chromeos/views/native_menu_domui.cc. However,
> this is a views testcase and cannot depend on the file from chromeos
> tree. Hence I specified the implementation here.


Ok. native_menu_domui.cc needs to be retired (at least from regular build)
and
i'll address this separately.


>
>
>
>
http://codereview.chromium.org/6155011/diff/6002/views/controls/textfield/nat...
> File views/controls/textfield/native_textfield_views.cc (right):
>
>
>
http://codereview.chromium.org/6155011/diff/6002/views/controls/textfield/nat...
> views/controls/textfield/native_textfield_views.cc:335: default:
> On 2011/01/12 21:01:47, oshima wrote:
>
>> NOTREACHED()
>>
>
> Done.
>
>
>
>
http://codereview.chromium.org/6155011/diff/6002/views/controls/textfield/nat...
> views/controls/textfield/native_textfield_views.cc:338: return false;
> On 2011/01/12 21:01:47, oshima wrote:
>
>> won't compiler complain this? I think this is unreachable.
>>
>
> the compiler doesnot seem to care. I removed it anyways
>
>
>
>
http://codereview.chromium.org/6155011/diff/6002/views/controls/textfield/nat...
> File views/controls/textfield/native_textfield_views_unittest.cc
> (right):
>
>
>
http://codereview.chromium.org/6155011/diff/6002/views/controls/textfield/nat...
> views/controls/textfield/native_textfield_views_unittest.cc:427:
> views::ViewsDelegate::views_delegate = original_delegate;
> On 2011/01/12 21:01:47, oshima wrote:
>
>> can you use base/auto_reset.h ?
>>
>
> Done.
>
>
> http://codereview.chromium.org/6155011/
>

[varunjain, 2011-01-14 17:37:18]

Made changes to fix clashing implementations of MenuWrapper::CreateWrapper for
different platforms. The CL now passes all trybots. PTAL.

[oshima, 2011-01-14 18:17:58]

LGTM