Views Textfield: copy-on-select and paste-on-middle-click.

Views Textfield: copy-on-select and paste-on-middle-click.

Add Textfield::PasteSelectionClipboard helper function.
(used to paste on middle click, update cursor/selection)
Add file-local UpdateSelectionClipboard helper function.
(used to write non-empty text to the selection clipboard)

Call UpdateAfterChange consistently after changes.
(not just OnCaretBoundsChanged, SchedulePaint, etc.)
Cleanup ExecuteCommand, OnMousePressed/Dragged, etc.

Add TextfieldTest.SelectionClipboard unit tests.
Make Get/SetClipboardText helpers file-local.

BUG=319011
R=erg@chromium.org,oshima@chromium.org
TEST=Copy-on-select and paste-on-middle-click work for Linux Aura omnibox and browser textfields (find-in-page, bookmark bubble).
NOTRY=true

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

[Markus (顧孟勤), 2013-11-15 22:05:25]

We also need the inverse, don't we. I.e. any time text is selected anywhere, we
need to add it to the clipboard. Is there already an open bug for that?

[msw, 2013-11-15 22:15:51]

On 2013/11/15 22:05:25, Markus (顧孟勤) wrote:
> We also need the inverse, don't we. I.e. any time text is selected anywhere,
we
> need to add it to the clipboard. Is there already an open bug for that?

You're correct, let's track that defect with Issue 319011 too.
I'll try to implement both select copy and paste in this CL.

[msw, 2014-01-22 02:13:18]

Okay! This is finally ready for review!
Oshima and Elliot, please take a look; thanks!

[Elliot Glaysher, 2014-01-22 20:07:46]

I've read through this patch twice. Functionally, it lgtm, but I'm not enough of
an expert on this code to be confident in that.

[oshima, 2014-01-22 21:40:55]

https://codereview.chromium.org/73403002/diff/320001/ui/views/controls/textfi...
File ui/views/controls/textfield/textfield.cc (right):

https://codereview.chromium.org/73403002/diff/320001/ui/views/controls/textfi...
ui/views/controls/textfield/textfield.cc:57: void UpdateSelectionClipboard(const
base::string16& text) {
do you need to pass text? Can't you just use GetSelectedText() for all cases?

https://codereview.chromium.org/73403002/diff/320001/ui/views/controls/textfi...
ui/views/controls/textfield/textfield.cc:59: if (!text.empty())
nit: I believe you should have {} in this case.

https://codereview.chromium.org/73403002/diff/320001/ui/views/controls/textfi...
ui/views/controls/textfield/textfield.cc:417:
UpdateSelectionClipboard(GetSelectedText());
Moving cursor clears the selection, so this should clear the selection. Is that
correct?

https://codereview.chromium.org/73403002/diff/320001/ui/views/controls/textfi...
ui/views/controls/textfield/textfield.cc:509:
ui::CLIPBOARD_TYPE_SELECTION).WriteText(base::string16());
.. and you can just call UpdateSelectionClipboard() for consistency?

[msw, 2014-01-22 23:15:35]

Comments addressed, please take another look at your earliest convenience.

https://codereview.chromium.org/73403002/diff/320001/ui/views/controls/textfi...
File ui/views/controls/textfield/textfield.cc (right):

https://codereview.chromium.org/73403002/diff/320001/ui/views/controls/textfi...
ui/views/controls/textfield/textfield.cc:57: void UpdateSelectionClipboard(const
base::string16& text) {
On 2014/01/22 21:40:55, oshima wrote:
> do you need to pass text? Can't you just use GetSelectedText() for all cases?

I changed this file-local to be a class function and took your suggestion.

https://codereview.chromium.org/73403002/diff/320001/ui/views/controls/textfi...
ui/views/controls/textfield/textfield.cc:59: if (!text.empty())
On 2014/01/22 21:40:55, oshima wrote:
> nit: I believe you should have {} in this case. 

Done.

https://codereview.chromium.org/73403002/diff/320001/ui/views/controls/textfi...
ui/views/controls/textfield/textfield.cc:417:
UpdateSelectionClipboard(GetSelectedText());
On 2014/01/22 21:40:55, oshima wrote:
> Moving cursor clears the selection, so this should clear the selection. Is
that
> correct?

If |shift| is true, MoveCursor may yield a selected range.
UpdateSelectionClipboard will set the clipboard contents to any non-empty
selected text, if there is no selected range, the clipboard contents will be
unchanged. I believe this is the intended behavior.

https://codereview.chromium.org/73403002/diff/320001/ui/views/controls/textfi...
ui/views/controls/textfield/textfield.cc:509:
ui::CLIPBOARD_TYPE_SELECTION).WriteText(base::string16());
On 2014/01/22 21:40:55, oshima wrote:
> .. and you can just call UpdateSelectionClipboard() for consistency?

No; here we specifically want to clear the selection clipboard; in contrast,
UpdateSelectionClipboard should no-op if there is no selection. LMK if you'd
prefer a separate SetSelectionClipboard helper in addition to
UpdateSelectionClipboard, or something else.

[oshima, 2014-01-22 23:21:52]

lgtm

[commit-bot: I haz the power, 2014-01-22 23:22:24]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/msw@chromium.org/73403002/420001

[commit-bot: I haz the power, 2014-01-23 01:35:52]

Commit queue rejected this change because the description was changed
between the time the change entered the commit queue and the time it
was ready to commit. You can safely check the commit box again.

[commit-bot: I haz the power, 2014-01-23 01:36:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/msw@chromium.org/73403002/420001

[commit-bot: I haz the power, 2014-01-23 01:40:44]

Change committed as 246478