Textfield: Move selection logic from GetCommandForKeyEvent to ExecuteCommand.

ui/views/controls/textfield/textfield.cc

Index: ui/views/controls/textfield/textfield.cc
diff --git a/ui/views/controls/textfield/textfield.cc b/ui/views/controls/textfield/textfield.cc
index a42ec4dec17dcc9ca77a172926d3829a8d0eed1e..ea732bf848a06fa2b36942323e203d4a08ff15e7 100644
--- a/ui/views/controls/textfield/textfield.cc
+++ b/ui/views/controls/textfield/textfield.cc
@@ -83,8 +83,8 @@ int GetDragSelectionDelay() {
   return 100;
 }
 
-// Get the default command for a given key |event| and selection state.
-int GetCommandForKeyEvent(const ui::KeyEvent& event, bool has_selection) {
+// Get the default command for a given key |event|.
+int GetCommandForKeyEvent(const ui::KeyEvent& event) {
   if (event.type() != ui::ET_KEY_PRESSED || event.IsUnicodeKeyCode())
     return kNoCommand;
 
@@ -129,7 +129,7 @@ int GetCommandForKeyEvent(const ui::KeyEvent& event, bool has_selection) {
       return shift ? IDS_MOVE_TO_END_OF_LINE_AND_MODIFY_SELECTION :
                       IDS_MOVE_TO_END_OF_LINE;
     case ui::VKEY_BACK:
-      if (!control || has_selection)
+      if (!control)
         return IDS_DELETE_BACKWARD;
 #if defined(OS_LINUX)
       // Only erase by line break on Linux and ChromeOS.
@@ -138,14 +138,15 @@ int GetCommandForKeyEvent(const ui::KeyEvent& event, bool has_selection) {
 #endif
       return IDS_DELETE_WORD_BACKWARD;
     case ui::VKEY_DELETE:

[msw, 2016/04/28 18:22:02]

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;	

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

Done.

-      if (!control || has_selection)
-        return (shift && has_selection) ? IDS_APP_CUT : IDS_DELETE_FORWARD;
+      if (shift) {
 #if defined(OS_LINUX)
-      // Only erase by line break on Linux and ChromeOS.
-      if (shift)
-        return IDS_DELETE_TO_END_OF_LINE;
+        // Only erase by line break on Linux and ChromeOS.
+        if (control)
+          return IDS_DELETE_TO_END_OF_LINE;
 #endif
-      return IDS_DELETE_WORD_FORWARD;
+        return control ? IDS_DELETE_WORD_FORWARD : IDS_APP_CUT;

[msw, 2016/04/28 18:22:02]

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.

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

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]

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]

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]

Okay, sounds good to me!

+      }
+      return control ? IDS_DELETE_WORD_FORWARD : IDS_DELETE_FORWARD;
     case ui::VKEY_INSERT:
       if (control && !shift)
         return IDS_APP_COPY;
@@ -730,7 +731,7 @@ bool Textfield::OnKeyPressed(const ui::KeyEvent& event) {
 #endif
 
   if (edit_command == kNoCommand)
-    edit_command = GetCommandForKeyEvent(event, HasSelection());
+    edit_command = GetCommandForKeyEvent(event);
 
   if (!handled && IsCommandIdEnabled(edit_command)) {
     ExecuteCommand(edit_command);
@@ -834,7 +835,7 @@ void Textfield::OnGestureEvent(ui::GestureEvent* event) {
 bool Textfield::AcceleratorPressed(const ui::Accelerator& accelerator) {
   ui::KeyEvent event(accelerator.type(), accelerator.key_code(),
                      accelerator.modifiers());
-  ExecuteCommand(GetCommandForKeyEvent(event, HasSelection()));
+  ExecuteCommand(GetCommandForKeyEvent(event));
   return true;
 }
 
@@ -1303,6 +1304,20 @@ bool Textfield::GetAcceleratorForCommandId(int command_id,
 
 void Textfield::ExecuteCommand(int command_id, int event_flags) {
   DestroyTouchSelection();
+
+  if (HasSelection()) {

[msw, 2016/04/28 18:22:02]

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.

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

Done.

+    switch (command_id) {
+      case IDS_DELETE_WORD_BACKWARD:
+      case IDS_DELETE_TO_BEGINNING_OF_LINE:
+        command_id = IDS_DELETE_BACKWARD;

[msw, 2016/04/28 18:22:02]

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.

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

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.

+        break;
+      case IDS_DELETE_WORD_FORWARD:
+      case IDS_DELETE_TO_END_OF_LINE:
+        command_id = IDS_DELETE_FORWARD;
+        break;
+    }
+  }
+
   if (!IsCommandIdEnabled(command_id))
     return;