Implement clipboard features in views textfield.

Implement clipboard features in views textfield.

BUG=none
TEST=Added new test.

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

[oshima, 2011-01-04 22:36:49]

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

http://codereview.chromium.org/6004010/diff/1/views/controls/textfield/native...
views/controls/textfield/native_textfield_views.cc:471: text_changed =
model_->HandleClipboardEvents(key_code);
I think it's more consistent to have Cut&Copy&Paste method on model, and have
NativeTextfieldViews to deal with key->function mapping. This way, model will be
independent of key code mapping. You can move this to switch and do things like
ctrl-a.

http://codereview.chromium.org/6004010/diff/1/views/controls/textfield/textfi...
File views/controls/textfield/textfield_views_model_unittest.cc (right):

http://codereview.chromium.org/6004010/diff/1/views/controls/textfield/textfi...
views/controls/textfield/textfield_views_model_unittest.cc:36: };
Looks like there are several ViewsDelegate impl for tests.
Don't have to be in this CL, but can you take the one in views_unittest and move
it to views/test?

http://codereview.chromium.org/6004010/diff/1/views/controls/textfield/textfi...
views/controls/textfield/textfield_views_model_unittest.cc:335: EXPECT_EQ(0U,
model.cursor_pos());
can you add test case for cut&copy with no selection to make sure it won't
update clipboard with empty string.

[rjkroege, 2011-01-05 15:31:22]

http://codereview.chromium.org/6004010/diff/1/views/controls/textfield/textfi...
File views/controls/textfield/textfield_views_model.cc (right):

http://codereview.chromium.org/6004010/diff/1/views/controls/textfield/textfi...
views/controls/textfield/textfield_views_model.cc:229: bool
TextfieldViewsModel::HandleClipboardEvents(app::KeyboardCode key_code) {
please remember that cut&paste is also invokable from menus, etc. I think you
need actions here (cut, paste, copy) that are invoked via the accelerator
framework for the control keys (or the menu entries)

[varunjain, 2011-01-05 16:19:31]

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

http://codereview.chromium.org/6004010/diff/1/views/controls/textfield/native...
views/controls/textfield/native_textfield_views.cc:471: text_changed =
model_->HandleClipboardEvents(key_code);
On 2011/01/04 22:36:49, oshima wrote:
> I think it's more consistent to have Cut&Copy&Paste method on model, and have
> NativeTextfieldViews to deal with key->function mapping. This way, model will
be
> independent of key code mapping. You can move this to switch and do things
like
> ctrl-a.

Done.

http://codereview.chromium.org/6004010/diff/1/views/controls/textfield/textfi...
File views/controls/textfield/textfield_views_model.cc (right):

http://codereview.chromium.org/6004010/diff/1/views/controls/textfield/textfi...
views/controls/textfield/textfield_views_model.cc:229: bool
TextfieldViewsModel::HandleClipboardEvents(app::KeyboardCode key_code) {
On 2011/01/05 15:31:22, rjkroege wrote:
> please remember that cut&paste is also invokable from menus, etc. I think you
> need actions here (cut, paste, copy) that are invoked via the accelerator
> framework for the control keys (or the menu entries)

Done.

http://codereview.chromium.org/6004010/diff/1/views/controls/textfield/textfi...
File views/controls/textfield/textfield_views_model_unittest.cc (right):

http://codereview.chromium.org/6004010/diff/1/views/controls/textfield/textfi...
views/controls/textfield/textfield_views_model_unittest.cc:36: };
On 2011/01/04 22:36:49, oshima wrote:
> Looks like there are several ViewsDelegate impl for tests.
> Don't have to be in this CL, but can you take the one in views_unittest and
move
> it to views/test?

Done.

http://codereview.chromium.org/6004010/diff/1/views/controls/textfield/textfi...
views/controls/textfield/textfield_views_model_unittest.cc:335: EXPECT_EQ(0U,
model.cursor_pos());
On 2011/01/04 22:36:49, oshima wrote:
> can you add test case for cut&copy with no selection to make sure it won't
> update clipboard with empty string.

Done.

[rjkroege, 2011-01-05 18:40:16]

Please be sure to add the ability to invoke the Cut/Copy/Paste entry points from
the wrench menu.

http://codereview.chromium.org/6004010/diff/8001/views/controls/textfield/tex...
File views/controls/textfield/textfield_views_model.h (right):

http://codereview.chromium.org/6004010/diff/8001/views/controls/textfield/tex...
views/controls/textfield/textfield_views_model.h:11: #include
"app/keyboard_codes.h"
you can remove this now yes?

[oshima, 2011-01-05 18:50:30]

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

http://codereview.chromium.org/6004010/diff/8001/views/controls/textfield/nat...
views/controls/textfield/native_textfield_views.cc:432: }
no need for {} for single line.

http://codereview.chromium.org/6004010/diff/8001/views/controls/textfield/tex...
File views/controls/textfield/textfield_views_model.h (right):

http://codereview.chromium.org/6004010/diff/8001/views/controls/textfield/tex...
views/controls/textfield/textfield_views_model.h:11: #include
"app/keyboard_codes.h"
should be.

On 2011/01/05 18:40:16, rjkroege wrote:
> you can remove this now yes?

http://codereview.chromium.org/6004010/diff/8001/views/view_unittest.cc
File views/view_unittest.cc (left):

http://codereview.chromium.org/6004010/diff/8001/views/view_unittest.cc#oldco...
views/view_unittest.cc:850: };
There are more in
bookmark_bar_view_test.cc, accessibility_event_router_views_unittest.cc

you can do it in follow up CL.

[oshima, 2011-01-05 18:53:01]

On 2011/01/05 18:40:16, rjkroege wrote:
> Please be sure to add the ability to invoke the Cut/Copy/Paste entry points
from
> the wrench menu.
It's currently implemented by emulating keys, so if ui_controls::SendKeyPress
works on touch, it should work on touch too.

> 
>
http://codereview.chromium.org/6004010/diff/8001/views/controls/textfield/tex...
> File views/controls/textfield/textfield_views_model.h (right):
> 
>
http://codereview.chromium.org/6004010/diff/8001/views/controls/textfield/tex...
> views/controls/textfield/textfield_views_model.h:11: #include
> "app/keyboard_codes.h"
> you can remove this now yes?

[varunjain, 2011-01-05 19:03:35]

@rjkroege: The wrench menu cut/copy/paste buttons dont work for the new
textfield. I will implement that in a separate CL (perhaps along with the
context menu)

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

http://codereview.chromium.org/6004010/diff/8001/views/controls/textfield/nat...
views/controls/textfield/native_textfield_views.cc:432: }
On 2011/01/05 18:50:30, oshima wrote:
> no need for {} for single line.

Done.

http://codereview.chromium.org/6004010/diff/8001/views/controls/textfield/tex...
File views/controls/textfield/textfield_views_model.h (right):

http://codereview.chromium.org/6004010/diff/8001/views/controls/textfield/tex...
views/controls/textfield/textfield_views_model.h:11: #include
"app/keyboard_codes.h"
On 2011/01/05 18:40:16, rjkroege wrote:
> you can remove this now yes?

Done.

[oshima, 2011-01-05 19:51:26]

LGTM

[rjkroege, 2011-01-07 15:09:33]

Please add a TODO about the wrench menu entry points.

LGTM.