Textfield: Move selection logic from GetCommandForKeyEvent to ExecuteCommand.

Textfield: Move selection logic from GetCommandForKeyEvent to ExecuteCommand.

Currently, all logic related to an active selection, resides in
GetCommandForKeyEvent. This leads to crbug.com/605492 on platforms like Linux
and Mac which may bypass GetCommandForKeyEvent.

This CL moves logic related to an active selection from GetCommandForKeyEvent to
ExecuteCommand. This also simplifies the logic in GetCommandForKeyEvent.

Also, this introduces some minor behavior changes on Windows-
-Ctrl+Shift+Delete(with selection) - used to be a IDS_APP_CUT operation earlier
but will now be resolved to a IDS_DELETE_FORWARD finally, similar to how
Ctrl+Shift+Backspace behaves.
-Shift+Delete(no selection) - used to be a IDS_DELETE_FORWARD operation earlier
but will now be resolved to a IDS_APP_CUT operation (and eventually a no-op,
since there is no active selection).

Also added a test "DeletionWithSelection" which fails on master for MacViews.

BUG=605492
TEST= On Linux and Mac, test that using a word delete keyboard shortcut in a
textfield with an active selection, deletes the selected text.

Committed: https://crrev.com/5e739d04c25b6f63ac9fe7f70005268006f25868
Cr-Commit-Position: refs/heads/master@{#390888}

[karandeepb, 2016-04-27 04:37:52]

Description was changed from

==========
kkk
==========

to

==========
Textfield: Move selection logic from GetCommandForKeyEvent to ExecuteCommand.

Currently, all logic related to an active selection, resides in 
GetCommandForKeyEvent. This leads to crbug.com/605492 on platforms like Linux 
and Mac which may bypass GetCommandForKeyEvent.

This CL moves logic related to an active selection from GetCommandForKeyEvent to
ExecuteCommand. This also simplifies the logic in GetCommandForKeyEvent.

Also, this introduces some minor behavior changes on Windows-
-Ctrl+Shift+Delete(with selection) - used to be a IDS_APP_CUT operation earlier 
but will now be resolved to a IDS_DELETE_FORWARD finally.
-Shift+Delete(no selection) - used to be a IDS_DELETE_FORWARD operation earlier
but will now be resolved to a IDS_APP_CUT operation (and eventually a no-op, 
since there is no active selection).

BUG=605492
TEST= On Linux and Mac, test that using a word delete keyboard shortcut in a 
textfield with an active selection, deletes the selected text.
==========

[karandeepb, 2016-04-27 04:38:58]

Description was changed from

==========
Textfield: Move selection logic from GetCommandForKeyEvent to ExecuteCommand.

Currently, all logic related to an active selection, resides in 
GetCommandForKeyEvent. This leads to crbug.com/605492 on platforms like Linux 
and Mac which may bypass GetCommandForKeyEvent.

This CL moves logic related to an active selection from GetCommandForKeyEvent to
ExecuteCommand. This also simplifies the logic in GetCommandForKeyEvent.

Also, this introduces some minor behavior changes on Windows-
-Ctrl+Shift+Delete(with selection) - used to be a IDS_APP_CUT operation earlier 
but will now be resolved to a IDS_DELETE_FORWARD finally.
-Shift+Delete(no selection) - used to be a IDS_DELETE_FORWARD operation earlier
but will now be resolved to a IDS_APP_CUT operation (and eventually a no-op, 
since there is no active selection).

BUG=605492
TEST= On Linux and Mac, test that using a word delete keyboard shortcut in a 
textfield with an active selection, deletes the selected text.
==========

to

==========
Textfield: Move selection logic from GetCommandForKeyEvent to ExecuteCommand.

Currently, all logic related to an active selection, resides in 
GetCommandForKeyEvent. This leads to crbug.com/605492 on platforms like Linux 
and Mac which may bypass GetCommandForKeyEvent.

This CL moves logic related to an active selection from GetCommandForKeyEvent to
ExecuteCommand. This also simplifies the logic in GetCommandForKeyEvent.

Also, this introduces some minor behavior changes on Windows-
-Ctrl+Shift+Delete(with selection) - used to be a IDS_APP_CUT operation earlier 
but will now be resolved to a IDS_DELETE_FORWARD finally.
-Shift+Delete(no selection) - used to be a IDS_DELETE_FORWARD operation earlier
but will now be resolved to a IDS_APP_CUT operation (and eventually a no-op, 
since there is no active selection).

Also added a test "DeletionWithSelection" which fails on master for MacViews.

BUG=605492
TEST= On Linux and Mac, test that using a word delete keyboard shortcut in a 
textfield with an active selection, deletes the selected text.
==========

[karandeepb, 2016-04-27 04:45:52]

Description was changed from

==========
Textfield: Move selection logic from GetCommandForKeyEvent to ExecuteCommand.

Currently, all logic related to an active selection, resides in 
GetCommandForKeyEvent. This leads to crbug.com/605492 on platforms like Linux 
and Mac which may bypass GetCommandForKeyEvent.

This CL moves logic related to an active selection from GetCommandForKeyEvent to
ExecuteCommand. This also simplifies the logic in GetCommandForKeyEvent.

Also, this introduces some minor behavior changes on Windows-
-Ctrl+Shift+Delete(with selection) - used to be a IDS_APP_CUT operation earlier 
but will now be resolved to a IDS_DELETE_FORWARD finally.
-Shift+Delete(no selection) - used to be a IDS_DELETE_FORWARD operation earlier
but will now be resolved to a IDS_APP_CUT operation (and eventually a no-op, 
since there is no active selection).

Also added a test "DeletionWithSelection" which fails on master for MacViews.

BUG=605492
TEST= On Linux and Mac, test that using a word delete keyboard shortcut in a 
textfield with an active selection, deletes the selected text.
==========

to

==========
Textfield: Move selection logic from GetCommandForKeyEvent to ExecuteCommand.

Currently, all logic related to an active selection, resides in 
GetCommandForKeyEvent. This leads to crbug.com/605492 on platforms like Linux 
and Mac which may bypass GetCommandForKeyEvent.

This CL moves logic related to an active selection from GetCommandForKeyEvent to
ExecuteCommand. This also simplifies the logic in GetCommandForKeyEvent.

Also, this introduces some minor behavior changes on Windows-
-Ctrl+Shift+Delete(with selection) - used to be a IDS_APP_CUT operation earlier 
but will now be resolved to a IDS_DELETE_FORWARD finally, similar to how
Ctrl+Shift+Backspace behaves.
-Shift+Delete(no selection) - used to be a IDS_DELETE_FORWARD operation earlier
but will now be resolved to a IDS_APP_CUT operation (and eventually a no-op, 
since there is no active selection).

Also added a test "DeletionWithSelection" which fails on master for MacViews.

BUG=605492
TEST= On Linux and Mac, test that using a word delete keyboard shortcut in a 
textfield with an active selection, deletes the selected text.
==========

[karandeepb, 2016-04-27 05:27:56]

karandeepb@chromium.org changed reviewers:
+ msw@chromium.org

[karandeepb, 2016-04-27 05:27:56]

PTAL @msw. Thanks.

https://codereview.chromium.org/1923793002/diff/1/ui/views/controls/textfield...
File ui/views/controls/textfield/textfield_unittest.cc (right):

https://codereview.chromium.org/1923793002/diff/1/ui/views/controls/textfield...
ui/views/controls/textfield/textfield_unittest.cc:872: TEST_F(TextfieldTest,
DeletionWithSelection) {
Ideally, this should fail on master for Linux. However, for tests on Linux, we
don't use a TextEditKeyBindingsDelegateAuraLinux, hence GetCommandForKeyEvent is
not bypassed in tests.

[karandeepb, 2016-04-28 06:51:04]

ping msw@.

[msw, 2016-04-28 18:22:03]

https://codereview.chromium.org/1923793002/diff/1/ui/views/controls/textfield...
File ui/views/controls/textfield/textfield.cc (right):

https://codereview.chromium.org/1923793002/diff/1/ui/views/controls/textfield...
ui/views/controls/textfield/textfield.cc:140: case ui::VKEY_DELETE:
Consider rewriting this case a little bit for simplicity:
#if defined(OS_LINUX)
      // Only erase by line break on Linux and ChromeOS.
      if (shift && control)
        return IDS_DELETE_TO_END_OF_LINE;
#endif
      if (control)
        return IDS_DELETE_WORD_FORWARD;
      return shift ? IDS_APP_CUT : IDS_DELETE_FORWARD;

https://codereview.chromium.org/1923793002/diff/1/ui/views/controls/textfield...
ui/views/controls/textfield/textfield.cc:147: return control ?
IDS_DELETE_WORD_FORWARD : IDS_APP_CUT;
It's weird to return IDS_APP_CUT without knowing if there's a selection, but I
guess we paste without knowing if the clipboard has text content... Would it be
preferable for IDS_APP_CUT to fall back to IDS_DELETE_FORWARD when there is no
selection, instead of a no-op? In abstract, that may not seem right, but it's
reasonable if you consider "we used to just delete-forward on SHIFT+DELETE with
no selection, now it does nothing." I'm not sure it's a big deal... On Windows,
Notepad does a backspace on SHIFT+DELETE, and Edge does nothing. +CC Peter in
case he has an opinion here.

https://codereview.chromium.org/1923793002/diff/1/ui/views/controls/textfield...
ui/views/controls/textfield/textfield.cc:1308: if (HasSelection()) {
nit: add an explanatory comment that Linux and Mac (or maybe 'some codepaths')
may bypass GetCommandForKeyEvent, so any selection-dependent modifications of
the command should happen here.

https://codereview.chromium.org/1923793002/diff/1/ui/views/controls/textfield...
ui/views/controls/textfield/textfield.cc:1312: command_id = IDS_DELETE_BACKWARD;
It seems a little odd that this function would supplant a given command. I
wonder if the 'correct' fix is to actually support knowledge of the selection
state in the places where Mac and Linux determine the commands to send (ie.
Gtk2UI::MatchEvent/Gtk2KeyBindingsHandler::BuildGdkEventKeyFromXEvent, but I'm
not sure where this is on Mac...). That said, changing the linux path looks far
less straightforward, so this is probably a reasonable fix.

https://codereview.chromium.org/1923793002/diff/1/ui/views/controls/textfield...
File ui/views/controls/textfield/textfield_unittest.cc (right):

https://codereview.chromium.org/1923793002/diff/1/ui/views/controls/textfield...
ui/views/controls/textfield/textfield_unittest.cc:872: TEST_F(TextfieldTest,
DeletionWithSelection) {
On 2016/04/27 05:27:56, karandeepb wrote:
> Ideally, this should fail on master for Linux. However, for tests on Linux, we
> don't use a TextEditKeyBindingsDelegateAuraLinux, hence GetCommandForKeyEvent
is
> not bypassed in tests.

Add a comment to that effect at a condition that should fail in the test.

https://codereview.chromium.org/1923793002/diff/1/ui/views/controls/textfield...
ui/views/controls/textfield/textfield_unittest.cc:872: TEST_F(TextfieldTest,
DeletionWithSelection) {
Since all cases have the same behavior, you could rewrite this test more
succinctly, perhaps something like:

  struct {
    ui::KeyboardCode key;
    bool shift;
  } cases[] = {
    { ui::VKEY_BACK, false }, { ui::VKEY_DELETE, false },
    { ui::VKEY_BACK, true }, { ui::VKEY_DELETE, true },
  };

  InitTextfield();
  // [Delete] and [Backspace] should delete a selection, regardless of [Shift].
  for (size_t i = 0; i < arraysize(cases); ++i) {
    SCOPED_TRACE(base::StringPrintf("Testing cases[%" PRIuS "]", i));
    textfield_->SetText(ASCIIToUTF16("one two three"));
    textfield_->SelectRange(gfx::Range(2, 6));
    SendWordEvent(cases[i].key, cases[i].shift);
    EXPECT_STR_EQ("ono three", textfield_->text());
    EXPECT_EQ(gfx::Range(2), textfield_->GetSelectedRange());
  }

https://codereview.chromium.org/1923793002/diff/1/ui/views/controls/textfield...
ui/views/controls/textfield/textfield_unittest.cc:887: // Delete to a line break
on Linux and ChromeOS, to a word break on Windows
nit: Shouldn't this comment say "[Shift]+[Backspace] should only delete the
active selection, when one exists." or similar?

https://codereview.chromium.org/1923793002/diff/1/ui/views/controls/textfield...
ui/views/controls/textfield/textfield_unittest.cc:910: // Delete to a line break
on Linux and ChromeOS, to a word break on Windows
ditto nit: Shouldn't this comment say "[Shift]+[Delete] should only delete the
active selection, when one exists." or similar?

[Peter Kasting, 2016-04-28 19:09:18]

pkasting@chromium.org changed reviewers:
+ pkasting@chromium.org

[Peter Kasting, 2016-04-28 19:09:18]

https://codereview.chromium.org/1923793002/diff/1/ui/views/controls/textfield...
File ui/views/controls/textfield/textfield.cc (right):

https://codereview.chromium.org/1923793002/diff/1/ui/views/controls/textfield...
ui/views/controls/textfield/textfield.cc:147: return control ?
IDS_DELETE_WORD_FORWARD : IDS_APP_CUT;
On 2016/04/28 18:22:02, msw wrote:
> It's weird to return IDS_APP_CUT without knowing if there's a selection, but I
> guess we paste without knowing if the clipboard has text content... Would it
be
> preferable for IDS_APP_CUT to fall back to IDS_DELETE_FORWARD when there is no
> selection, instead of a no-op? In abstract, that may not seem right, but it's
> reasonable if you consider "we used to just delete-forward on SHIFT+DELETE
with
> no selection, now it does nothing." I'm not sure it's a big deal... On
Windows,
> Notepad does a backspace on SHIFT+DELETE, and Edge does nothing. +CC Peter in
> case he has an opinion here.

Shift-delete with no selection should no-op.  This matches Wordpad and Edge, and
when Wordpad and Notepad disagree I normally go with Wordpad.

[msw, 2016-04-28 19:17:43]

https://codereview.chromium.org/1923793002/diff/1/ui/views/controls/textfield...
File ui/views/controls/textfield/textfield.cc (right):

https://codereview.chromium.org/1923793002/diff/1/ui/views/controls/textfield...
ui/views/controls/textfield/textfield.cc:147: return control ?
IDS_DELETE_WORD_FORWARD : IDS_APP_CUT;
On 2016/04/28 19:09:18, Peter Kasting wrote:
> On 2016/04/28 18:22:02, msw wrote:
> > It's weird to return IDS_APP_CUT without knowing if there's a selection, but
I
> > guess we paste without knowing if the clipboard has text content... Would it
> be
> > preferable for IDS_APP_CUT to fall back to IDS_DELETE_FORWARD when there is
no
> > selection, instead of a no-op? In abstract, that may not seem right, but
it's
> > reasonable if you consider "we used to just delete-forward on SHIFT+DELETE
> with
> > no selection, now it does nothing." I'm not sure it's a big deal... On
> Windows,
> > Notepad does a backspace on SHIFT+DELETE, and Edge does nothing. +CC Peter
in
> > case he has an opinion here.
> 
> Shift-delete with no selection should no-op.  This matches Wordpad and Edge,
and
> when Wordpad and Notepad disagree I normally go with Wordpad.

Okay, but note that Shift-Backspace acts as simply Backspace in Chrome, Wordpad,
Notepad, and Edge; it might be a little odd that Shift would make Delete no-op,
but do nothing to modify Backspace. I'm not too strongly opinionated here, but
I'd like it to be sensible/consistent.

[Peter Kasting, 2016-04-28 19:21:48]

https://codereview.chromium.org/1923793002/diff/1/ui/views/controls/textfield...
File ui/views/controls/textfield/textfield.cc (right):

https://codereview.chromium.org/1923793002/diff/1/ui/views/controls/textfield...
ui/views/controls/textfield/textfield.cc:147: return control ?
IDS_DELETE_WORD_FORWARD : IDS_APP_CUT;
On 2016/04/28 19:17:43, msw wrote:
> On 2016/04/28 19:09:18, Peter Kasting wrote:
> > On 2016/04/28 18:22:02, msw wrote:
> > > It's weird to return IDS_APP_CUT without knowing if there's a selection,
but
> I
> > > guess we paste without knowing if the clipboard has text content... Would
it
> > be
> > > preferable for IDS_APP_CUT to fall back to IDS_DELETE_FORWARD when there
is
> no
> > > selection, instead of a no-op? In abstract, that may not seem right, but
> it's
> > > reasonable if you consider "we used to just delete-forward on SHIFT+DELETE
> > with
> > > no selection, now it does nothing." I'm not sure it's a big deal... On
> > Windows,
> > > Notepad does a backspace on SHIFT+DELETE, and Edge does nothing. +CC Peter
> in
> > > case he has an opinion here.
> > 
> > Shift-delete with no selection should no-op.  This matches Wordpad and Edge,
> and
> > when Wordpad and Notepad disagree I normally go with Wordpad.
> 
> Okay, but note that Shift-Backspace acts as simply Backspace in Chrome,
Wordpad,
> Notepad, and Edge; it might be a little odd that Shift would make Delete
no-op,
> but do nothing to modify Backspace. I'm not too strongly opinionated here, but
> I'd like it to be sensible/consistent.

Shift-delete is normally "cut" whereas shift-backspace isn't normally anything. 
Having shift-delete no-op with no selection is consistent with it meaning "cut",
which is to me a more important consistency than being consistent with the way
that shift does not modify backspace.

[msw, 2016-04-28 19:23:41]

https://codereview.chromium.org/1923793002/diff/1/ui/views/controls/textfield...
File ui/views/controls/textfield/textfield.cc (right):

https://codereview.chromium.org/1923793002/diff/1/ui/views/controls/textfield...
ui/views/controls/textfield/textfield.cc:147: return control ?
IDS_DELETE_WORD_FORWARD : IDS_APP_CUT;
On 2016/04/28 19:21:48, Peter Kasting wrote:
> On 2016/04/28 19:17:43, msw wrote:
> > On 2016/04/28 19:09:18, Peter Kasting wrote:
> > > On 2016/04/28 18:22:02, msw wrote:
> > > > It's weird to return IDS_APP_CUT without knowing if there's a selection,
> but
> > I
> > > > guess we paste without knowing if the clipboard has text content...
Would
> it
> > > be
> > > > preferable for IDS_APP_CUT to fall back to IDS_DELETE_FORWARD when there
> is
> > no
> > > > selection, instead of a no-op? In abstract, that may not seem right, but
> > it's
> > > > reasonable if you consider "we used to just delete-forward on
SHIFT+DELETE
> > > with
> > > > no selection, now it does nothing." I'm not sure it's a big deal... On
> > > Windows,
> > > > Notepad does a backspace on SHIFT+DELETE, and Edge does nothing. +CC
Peter
> > in
> > > > case he has an opinion here.
> > > 
> > > Shift-delete with no selection should no-op.  This matches Wordpad and
Edge,
> > and
> > > when Wordpad and Notepad disagree I normally go with Wordpad.
> > 
> > Okay, but note that Shift-Backspace acts as simply Backspace in Chrome,
> Wordpad,
> > Notepad, and Edge; it might be a little odd that Shift would make Delete
> no-op,
> > but do nothing to modify Backspace. I'm not too strongly opinionated here,
but
> > I'd like it to be sensible/consistent.
> 
> Shift-delete is normally "cut" whereas shift-backspace isn't normally
anything. 
> Having shift-delete no-op with no selection is consistent with it meaning
"cut",
> which is to me a more important consistency than being consistent with the way
> that shift does not modify backspace.

Okay, sounds good to me!

[karandeepb, 2016-04-29 01:40:59]

PTAL msw@, pkasting@. Thanks.

https://codereview.chromium.org/1923793002/diff/1/ui/views/controls/textfield...
File ui/views/controls/textfield/textfield.cc (right):

https://codereview.chromium.org/1923793002/diff/1/ui/views/controls/textfield...
ui/views/controls/textfield/textfield.cc:140: case ui::VKEY_DELETE:
On 2016/04/28 18:22:02, msw wrote:
> Consider rewriting this case a little bit for simplicity:
> #if defined(OS_LINUX)
>       // Only erase by line break on Linux and ChromeOS.
>       if (shift && control)
>         return IDS_DELETE_TO_END_OF_LINE;
> #endif
>       if (control)
>         return IDS_DELETE_WORD_FORWARD;
>       return shift ? IDS_APP_CUT : IDS_DELETE_FORWARD;	

Done.

https://codereview.chromium.org/1923793002/diff/1/ui/views/controls/textfield...
ui/views/controls/textfield/textfield.cc:1308: if (HasSelection()) {
On 2016/04/28 18:22:02, msw wrote:
> nit: add an explanatory comment that Linux and Mac (or maybe 'some codepaths')
> may bypass GetCommandForKeyEvent, so any selection-dependent modifications of
> the command should happen here.

Done.

https://codereview.chromium.org/1923793002/diff/1/ui/views/controls/textfield...
ui/views/controls/textfield/textfield.cc:1312: command_id = IDS_DELETE_BACKWARD;
On 2016/04/28 18:22:02, msw wrote:
> It seems a little odd that this function would supplant a given command. I
> wonder if the 'correct' fix is to actually support knowledge of the selection
> state in the places where Mac and Linux determine the commands to send (ie.
> Gtk2UI::MatchEvent/Gtk2KeyBindingsHandler::BuildGdkEventKeyFromXEvent, but I'm
> not sure where this is on Mac...). That said, changing the linux path looks
far
> less straightforward, so this is probably a reasonable fix.

Yeah this should be easy to do on Mac at least. But I like this fix more.
Similar to how an OS translates keystrokes into commands, GetCommandForKeyEvent
should only be responsible for translating a given KeyEvent to the appropriate
command w/o considering whether there is an active selection.

So in case of [Ctrl]+[Delete] with an active selection, the actual command we
get should still be IDS_DELETE_WORD_FORWARD. It just so happens, that on an
active selection, we want IDS_DELETE_WORD_FORWARD to behave like
IDS_DELETE_FORWARD.

https://codereview.chromium.org/1923793002/diff/1/ui/views/controls/textfield...
File ui/views/controls/textfield/textfield_unittest.cc (right):

https://codereview.chromium.org/1923793002/diff/1/ui/views/controls/textfield...
ui/views/controls/textfield/textfield_unittest.cc:872: TEST_F(TextfieldTest,
DeletionWithSelection) {
On 2016/04/28 18:22:02, msw wrote:
> Since all cases have the same behavior, you could rewrite this test more
> succinctly, perhaps something like:
> 
>   struct {
>     ui::KeyboardCode key;
>     bool shift;
>   } cases[] = {
>     { ui::VKEY_BACK, false }, { ui::VKEY_DELETE, false },
>     { ui::VKEY_BACK, true }, { ui::VKEY_DELETE, true },
>   };
> 
>   InitTextfield();
>   // [Delete] and [Backspace] should delete a selection, regardless of
[Shift].
>   for (size_t i = 0; i < arraysize(cases); ++i) {
>     SCOPED_TRACE(base::StringPrintf("Testing cases[%" PRIuS "]", i));
>     textfield_->SetText(ASCIIToUTF16("one two three"));
>     textfield_->SelectRange(gfx::Range(2, 6));
>     SendWordEvent(cases[i].key, cases[i].shift);
>     EXPECT_STR_EQ("ono three", textfield_->text());
>     EXPECT_EQ(gfx::Range(2), textfield_->GetSelectedRange());
>   }

Done.

https://codereview.chromium.org/1923793002/diff/1/ui/views/controls/textfield...
ui/views/controls/textfield/textfield_unittest.cc:872: TEST_F(TextfieldTest,
DeletionWithSelection) {
On 2016/04/28 18:22:02, msw wrote:
> On 2016/04/27 05:27:56, karandeepb wrote:
> > Ideally, this should fail on master for Linux. However, for tests on Linux,
we
> > don't use a TextEditKeyBindingsDelegateAuraLinux, hence
GetCommandForKeyEvent
> is
> > not bypassed in tests.
> 
> Add a comment to that effect at a condition that should fail in the test.

I wasn't probably clear enough. I meant that it should have failed on the
current master. With the fix, it won't fail, even if we use a
TextEditKeyBindingsDelegateAuraLinux. Hence don't think a comment is needed.

https://codereview.chromium.org/1923793002/diff/1/ui/views/controls/textfield...
ui/views/controls/textfield/textfield_unittest.cc:887: // Delete to a line break
on Linux and ChromeOS, to a word break on Windows
On 2016/04/28 18:22:02, msw wrote:
> nit: Shouldn't this comment say "[Shift]+[Backspace] should only delete the
> active selection, when one exists." or similar?

Yeah, it should. Though it's actually Ctrl + Shift + Backspace.

https://codereview.chromium.org/1923793002/diff/1/ui/views/controls/textfield...
ui/views/controls/textfield/textfield_unittest.cc:910: // Delete to a line break
on Linux and ChromeOS, to a word break on Windows
On 2016/04/28 18:22:02, msw wrote:
> ditto nit: Shouldn't this comment say "[Shift]+[Delete] should only delete the
> active selection, when one exists." or similar?

Done.

[msw, 2016-04-29 16:59:22]

lgtm with minor nits

https://codereview.chromium.org/1923793002/diff/20001/ui/views/controls/textf...
File ui/views/controls/textfield/textfield_unittest.cc (right):

https://codereview.chromium.org/1923793002/diff/20001/ui/views/controls/textf...
ui/views/controls/textfield/textfield_unittest.cc:872: // Test that deletion
operations behave correctly in case there is an active
nit: one liner "// Test that deletion operations behave correctly with an active
selection."

https://codereview.chromium.org/1923793002/diff/20001/ui/views/controls/textf...
ui/views/controls/textfield/textfield_unittest.cc:880: {ui::VKEY_DELETE, false},
optional nit: swap cases so it's back, back, delete, delete.
(I guess I was optimistic about formatting two entries per line)

[karandeepb, 2016-05-02 02:58:02]

Patchset #3 (id:40001) has been deleted

[karandeepb, 2016-05-02 03:01:42]

https://codereview.chromium.org/1923793002/diff/20001/ui/views/controls/textf...
File ui/views/controls/textfield/textfield_unittest.cc (right):

https://codereview.chromium.org/1923793002/diff/20001/ui/views/controls/textf...
ui/views/controls/textfield/textfield_unittest.cc:872: // Test that deletion
operations behave correctly in case there is an active
On 2016/04/29 16:59:22, msw wrote:
> nit: one liner "// Test that deletion operations behave correctly with an
active
> selection."

Done.

https://codereview.chromium.org/1923793002/diff/20001/ui/views/controls/textf...
ui/views/controls/textfield/textfield_unittest.cc:880: {ui::VKEY_DELETE, false},
On 2016/04/29 16:59:22, msw wrote:
> optional nit: swap cases so it's back, back, delete, delete.
> (I guess I was optimistic about formatting two entries per line)

Done.

[karandeepb, 2016-05-02 03:03:01]

The CQ bit was checked by karandeepb@chromium.org

[karandeepb, 2016-05-02 03:03:02]

The patchset sent to the CQ was uploaded after l-g-t-m from msw@chromium.org
Link to the patchset: https://codereview.chromium.org/1923793002/#ps80001
(title: "Address review nits.")

[commit-bot: I haz the power, 2016-05-02 03:03:11]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1923793002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1923793002/80001

[commit-bot: I haz the power, 2016-05-02 03:48:29]

Description was changed from

==========
Textfield: Move selection logic from GetCommandForKeyEvent to ExecuteCommand.

Currently, all logic related to an active selection, resides in 
GetCommandForKeyEvent. This leads to crbug.com/605492 on platforms like Linux 
and Mac which may bypass GetCommandForKeyEvent.

This CL moves logic related to an active selection from GetCommandForKeyEvent to
ExecuteCommand. This also simplifies the logic in GetCommandForKeyEvent.

Also, this introduces some minor behavior changes on Windows-
-Ctrl+Shift+Delete(with selection) - used to be a IDS_APP_CUT operation earlier 
but will now be resolved to a IDS_DELETE_FORWARD finally, similar to how
Ctrl+Shift+Backspace behaves.
-Shift+Delete(no selection) - used to be a IDS_DELETE_FORWARD operation earlier
but will now be resolved to a IDS_APP_CUT operation (and eventually a no-op, 
since there is no active selection).

Also added a test "DeletionWithSelection" which fails on master for MacViews.

BUG=605492
TEST= On Linux and Mac, test that using a word delete keyboard shortcut in a 
textfield with an active selection, deletes the selected text.
==========

to

==========
Textfield: Move selection logic from GetCommandForKeyEvent to ExecuteCommand.

Currently, all logic related to an active selection, resides in 
GetCommandForKeyEvent. This leads to crbug.com/605492 on platforms like Linux 
and Mac which may bypass GetCommandForKeyEvent.

This CL moves logic related to an active selection from GetCommandForKeyEvent to
ExecuteCommand. This also simplifies the logic in GetCommandForKeyEvent.

Also, this introduces some minor behavior changes on Windows-
-Ctrl+Shift+Delete(with selection) - used to be a IDS_APP_CUT operation earlier 
but will now be resolved to a IDS_DELETE_FORWARD finally, similar to how
Ctrl+Shift+Backspace behaves.
-Shift+Delete(no selection) - used to be a IDS_DELETE_FORWARD operation earlier
but will now be resolved to a IDS_APP_CUT operation (and eventually a no-op, 
since there is no active selection).

Also added a test "DeletionWithSelection" which fails on master for MacViews.

BUG=605492
TEST= On Linux and Mac, test that using a word delete keyboard shortcut in a 
textfield with an active selection, deletes the selected text.
==========

[commit-bot: I haz the power, 2016-05-02 03:48:30]

Committed patchset #4 (id:80001)

[commit-bot: I haz the power, 2016-05-02 03:49:34]

Description was changed from

==========
Textfield: Move selection logic from GetCommandForKeyEvent to ExecuteCommand.

Currently, all logic related to an active selection, resides in 
GetCommandForKeyEvent. This leads to crbug.com/605492 on platforms like Linux 
and Mac which may bypass GetCommandForKeyEvent.

This CL moves logic related to an active selection from GetCommandForKeyEvent to
ExecuteCommand. This also simplifies the logic in GetCommandForKeyEvent.

Also, this introduces some minor behavior changes on Windows-
-Ctrl+Shift+Delete(with selection) - used to be a IDS_APP_CUT operation earlier 
but will now be resolved to a IDS_DELETE_FORWARD finally, similar to how
Ctrl+Shift+Backspace behaves.
-Shift+Delete(no selection) - used to be a IDS_DELETE_FORWARD operation earlier
but will now be resolved to a IDS_APP_CUT operation (and eventually a no-op, 
since there is no active selection).

Also added a test "DeletionWithSelection" which fails on master for MacViews.

BUG=605492
TEST= On Linux and Mac, test that using a word delete keyboard shortcut in a 
textfield with an active selection, deletes the selected text.
==========

to

==========
Textfield: Move selection logic from GetCommandForKeyEvent to ExecuteCommand.

Currently, all logic related to an active selection, resides in
GetCommandForKeyEvent. This leads to crbug.com/605492 on platforms like Linux
and Mac which may bypass GetCommandForKeyEvent.

This CL moves logic related to an active selection from GetCommandForKeyEvent to
ExecuteCommand. This also simplifies the logic in GetCommandForKeyEvent.

Also, this introduces some minor behavior changes on Windows-
-Ctrl+Shift+Delete(with selection) - used to be a IDS_APP_CUT operation earlier
but will now be resolved to a IDS_DELETE_FORWARD finally, similar to how
Ctrl+Shift+Backspace behaves.
-Shift+Delete(no selection) - used to be a IDS_DELETE_FORWARD operation earlier
but will now be resolved to a IDS_APP_CUT operation (and eventually a no-op,
since there is no active selection).

Also added a test "DeletionWithSelection" which fails on master for MacViews.

BUG=605492
TEST= On Linux and Mac, test that using a word delete keyboard shortcut in a
textfield with an active selection, deletes the selected text.

Committed: https://crrev.com/5e739d04c25b6f63ac9fe7f70005268006f25868
Cr-Commit-Position: refs/heads/master@{#390888}
==========

[commit-bot: I haz the power, 2016-05-02 03:49:35]

Patchset 4 (id:??) landed as
https://crrev.com/5e739d04c25b6f63ac9fe7f70005268006f25868
Cr-Commit-Position: refs/heads/master@{#390888}