Views::Textfield: Decouple handling of menu and text editing commands.

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 40b15f83e154670490e7f2fe0c40853438643cc0..ca71dc70f7424bfee59a398580d64d1b0610b6ef 100644
--- a/ui/views/controls/textfield/textfield.cc
+++ b/ui/views/controls/textfield/textfield.cc
@@ -248,6 +248,21 @@ const gfx::FontList& GetDefaultFontList() {
   return rb.GetFontListWithDelta(ui::kLabelFontSizeDelta);
 }
 
+// Returns true if |command_id| is a menu action installed by this textfield.
+// Keep in sync with UpdateContextMenu.
+bool IsMenuCommand(int command_id) {
+  switch (command_id) {
+    case IDS_APP_UNDO:
+    case IDS_APP_CUT:
+    case IDS_APP_COPY:
+    case IDS_APP_PASTE:
+    case IDS_APP_DELETE:
+    case IDS_APP_SELECT_ALL:
+      return true;
+  }
+  return false;
+}
+
 }  // namespace
 
 // static
@@ -570,10 +585,6 @@ void Textfield::SetAccessibleName(const base::string16& name) {
   accessible_name_ = name;
 }
 
-void Textfield::ExecuteCommand(int command_id) {
-  ExecuteCommand(command_id, ui::EF_NONE);
-}
-
 bool Textfield::HasTextBeingDragged() {
   return initiating_drag_;
 }
@@ -717,8 +728,8 @@ bool Textfield::OnKeyPressed(const ui::KeyEvent& event) {
     const bool rtl = GetTextDirection() == base::i18n::RIGHT_TO_LEFT;
     for (size_t i = 0; i < commands.size(); ++i) {
       const int command = GetViewsCommand(commands[i], rtl);
-      if (IsCommandIdEnabled(command)) {
-        ExecuteCommand(command);
+      if (IsEditCommandEnabled(command)) {
+        ExecuteEditCommand(command);
         handled = true;
       }
     }
@@ -729,8 +740,8 @@ bool Textfield::OnKeyPressed(const ui::KeyEvent& event) {
   if (edit_command == kNoCommand)
     edit_command = GetCommandForKeyEvent(event);
 
-  if (!handled && IsCommandIdEnabled(edit_command)) {
-    ExecuteCommand(edit_command);
+  if (!handled && IsEditCommandEnabled(edit_command)) {
+    ExecuteEditCommand(edit_command);
     handled = true;
   }
   return handled;
@@ -834,7 +845,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));
+  ExecuteEditCommand(GetCommandForKeyEvent(event));
   return true;
 }
 
@@ -855,7 +866,7 @@ bool Textfield::SkipDefaultKeyEventProcessing(const ui::KeyEvent& event) {
   if (delegate && delegate->MatchEvent(event, &commands)) {
     const bool rtl = GetTextDirection() == base::i18n::RIGHT_TO_LEFT;
     for (size_t i = 0; i < commands.size(); ++i)
-      if (IsCommandIdEnabled(GetViewsCommand(commands[i], rtl)))
+      if (IsEditCommandEnabled(GetViewsCommand(commands[i], rtl)))
         return true;
   }
 #endif
@@ -1228,49 +1239,8 @@ bool Textfield::IsCommandIdChecked(int command_id) const {
 }
 
 bool Textfield::IsCommandIdEnabled(int command_id) const {
-  base::string16 result;
-  bool editable = !read_only();
-  bool readable = text_input_type_ != ui::TEXT_INPUT_TYPE_PASSWORD;
-  switch (command_id) {
-    case IDS_APP_UNDO:
-      return editable && model_->CanUndo();
-    case IDS_APP_REDO:
-      return editable && model_->CanRedo();
-    case IDS_APP_CUT:
-      return editable && readable && model_->HasSelection();
-    case IDS_APP_COPY:
-      return readable && model_->HasSelection();
-    case IDS_APP_PASTE:
-      ui::Clipboard::GetForCurrentThread()->ReadText(
-          ui::CLIPBOARD_TYPE_COPY_PASTE, &result);
-      return editable && !result.empty();
-    case IDS_APP_DELETE:
-      return editable && model_->HasSelection();
-    case IDS_APP_SELECT_ALL:
-      return !text().empty();
-    case IDS_DELETE_FORWARD:
-    case IDS_DELETE_BACKWARD:
-    case IDS_DELETE_TO_BEGINNING_OF_LINE:
-    case IDS_DELETE_TO_END_OF_LINE:
-    case IDS_DELETE_WORD_BACKWARD:
-    case IDS_DELETE_WORD_FORWARD:
-      return editable;
-    case IDS_MOVE_LEFT:
-    case IDS_MOVE_LEFT_AND_MODIFY_SELECTION:
-    case IDS_MOVE_RIGHT:
-    case IDS_MOVE_RIGHT_AND_MODIFY_SELECTION:
-    case IDS_MOVE_WORD_LEFT:
-    case IDS_MOVE_WORD_LEFT_AND_MODIFY_SELECTION:
-    case IDS_MOVE_WORD_RIGHT:
-    case IDS_MOVE_WORD_RIGHT_AND_MODIFY_SELECTION:
-    case IDS_MOVE_TO_BEGINNING_OF_LINE:
-    case IDS_MOVE_TO_BEGINNING_OF_LINE_AND_MODIFY_SELECTION:
-    case IDS_MOVE_TO_END_OF_LINE:
-    case IDS_MOVE_TO_END_OF_LINE_AND_MODIFY_SELECTION:
-      return true;
-    default:
-      return false;
-  }
+  return IsMenuCommand(command_id) &&

[tapted, 2016/06/09 04:10:03]

I'm still dubious about the merit of the `IsMenuCommand` check. If there's no
bugs by omitting it, then the benefit is that we don't have the maintenance
problem of keeping IsMenuCommand() in sync with UpdateContextMenu.

Per my prior comment. I think an improvement on this would be:

  // Only commands in the context menu go through this.
  DCHECK(context_menu_contents_);
  DCHECK(context_menu_contents_->GetIndexOfCommandId(command_id) >= 0);
  return Textfield::IsEditCommandEnabled(command_id);

Is there a reason not to do this?

[karandeepb, 2016/06/09 04:51:42]

IsMenuCommand is replaced by ui::TextEditCommand
GetTextEditCommandFromMenuCommand(int command_id) in the next CL
(https://codereview.chromium.org/2027133002/), i.e. I'll need a way to map a
menu action to a ui::TextEditCommand. So we'll always need to keep it in sync
with UpdateContextMenu. I can remove it in this CL but we'll need an analogous
function in the next anyway.

[tapted, 2016/06/09 05:02:01]

Gotcha - it makes a lot more sense with that context. This is worth mentioning.
E.g., in the CL description,

Adds `bool IsMenuCommand(int)` which be updated in a follow-up CL to return a
text editing command from the enum rather than a bool. (i.e.
`ui::TextEditCommand GetTextEditCommandFromMenuCommand(int command_id)`).

+         Textfield::IsEditCommandEnabled(command_id);
 }
 
 bool Textfield::GetAcceleratorForCommandId(int command_id,
@@ -1302,126 +1272,9 @@ bool Textfield::GetAcceleratorForCommandId(int command_id,
 }
 
 void Textfield::ExecuteCommand(int command_id, int event_flags) {
-  // Some codepaths may bypass GetCommandForKeyEvent, so any selection-dependent
-  // modifications of the command should happen here.
-  if (HasSelection()) {
-    switch (command_id) {
-      case IDS_DELETE_WORD_BACKWARD:
-      case IDS_DELETE_TO_BEGINNING_OF_LINE:
-        command_id = IDS_DELETE_BACKWARD;
-        break;
-      case IDS_DELETE_WORD_FORWARD:
-      case IDS_DELETE_TO_END_OF_LINE:
-        command_id = IDS_DELETE_FORWARD;
-        break;
-    }
-  }
-
-  // We only execute the commands enabled in Textfield::IsCommandIdEnabled
-  // below. Hence don't do a virtual IsCommandIdEnabled call.
   if (!Textfield::IsCommandIdEnabled(command_id))
     return;
-
-  DestroyTouchSelection();
-
-  bool text_changed = false;
-  bool cursor_changed = false;
-  bool rtl = GetTextDirection() == base::i18n::RIGHT_TO_LEFT;
-  gfx::VisualCursorDirection begin = rtl ? gfx::CURSOR_RIGHT : gfx::CURSOR_LEFT;
-  gfx::VisualCursorDirection end = rtl ? gfx::CURSOR_LEFT : gfx::CURSOR_RIGHT;
-  gfx::SelectionModel selection_model = GetSelectionModel();
-
-  OnBeforeUserAction();
-  switch (command_id) {
-    case IDS_APP_UNDO:
-      text_changed = cursor_changed = model_->Undo();
-      break;
-    case IDS_APP_REDO:
-      text_changed = cursor_changed = model_->Redo();
-      break;
-    case IDS_APP_CUT:
-      text_changed = cursor_changed = Cut();
-      break;
-    case IDS_APP_COPY:
-      Copy();
-      break;
-    case IDS_APP_PASTE:
-      text_changed = cursor_changed = Paste();
-      break;
-    case IDS_APP_DELETE:
-      text_changed = cursor_changed = model_->Delete();
-      break;
-    case IDS_APP_SELECT_ALL:
-      SelectAll(false);
-      break;
-    case IDS_DELETE_BACKWARD:
-      text_changed = cursor_changed = model_->Backspace();
-      break;
-    case IDS_DELETE_FORWARD:
-      text_changed = cursor_changed = model_->Delete();
-      break;
-    case IDS_DELETE_TO_END_OF_LINE:
-      model_->MoveCursor(gfx::LINE_BREAK, end, true);
-      text_changed = cursor_changed = model_->Delete();
-      break;
-    case IDS_DELETE_TO_BEGINNING_OF_LINE:
-      model_->MoveCursor(gfx::LINE_BREAK, begin, true);
-      text_changed = cursor_changed = model_->Backspace();
-      break;
-    case IDS_DELETE_WORD_BACKWARD:
-      model_->MoveCursor(gfx::WORD_BREAK, begin, true);
-      text_changed = cursor_changed = model_->Backspace();
-      break;
-    case IDS_DELETE_WORD_FORWARD:
-      model_->MoveCursor(gfx::WORD_BREAK, end, true);
-      text_changed = cursor_changed = model_->Delete();
-      break;
-    case IDS_MOVE_LEFT:
-      model_->MoveCursor(gfx::CHARACTER_BREAK, gfx::CURSOR_LEFT, false);
-      break;
-    case IDS_MOVE_LEFT_AND_MODIFY_SELECTION:
-      model_->MoveCursor(gfx::CHARACTER_BREAK, gfx::CURSOR_LEFT, true);
-      break;
-    case IDS_MOVE_RIGHT:
-      model_->MoveCursor(gfx::CHARACTER_BREAK, gfx::CURSOR_RIGHT, false);
-      break;
-    case IDS_MOVE_RIGHT_AND_MODIFY_SELECTION:
-      model_->MoveCursor(gfx::CHARACTER_BREAK, gfx::CURSOR_RIGHT, true);
-      break;
-    case IDS_MOVE_WORD_LEFT:
-      model_->MoveCursor(gfx::WORD_BREAK, gfx::CURSOR_LEFT, false);
-      break;
-    case IDS_MOVE_WORD_LEFT_AND_MODIFY_SELECTION:
-      model_->MoveCursor(gfx::WORD_BREAK, gfx::CURSOR_LEFT, true);
-      break;
-    case IDS_MOVE_WORD_RIGHT:
-      model_->MoveCursor(gfx::WORD_BREAK, gfx::CURSOR_RIGHT, false);
-      break;
-    case IDS_MOVE_WORD_RIGHT_AND_MODIFY_SELECTION:
-      model_->MoveCursor(gfx::WORD_BREAK, gfx::CURSOR_RIGHT, true);
-      break;
-    case IDS_MOVE_TO_BEGINNING_OF_LINE:
-      model_->MoveCursor(gfx::LINE_BREAK, begin, false);
-      break;
-    case IDS_MOVE_TO_BEGINNING_OF_LINE_AND_MODIFY_SELECTION:
-      model_->MoveCursor(gfx::LINE_BREAK, begin, true);
-      break;
-    case IDS_MOVE_TO_END_OF_LINE:
-      model_->MoveCursor(gfx::LINE_BREAK, end, false);
-      break;
-    case IDS_MOVE_TO_END_OF_LINE_AND_MODIFY_SELECTION:
-      model_->MoveCursor(gfx::LINE_BREAK, end, true);
-      break;
-    default:
-      NOTREACHED();
-      break;
-  }
-
-  cursor_changed |= GetSelectionModel() != selection_model;
-  if (cursor_changed)
-    UpdateSelectionClipboard();
-  UpdateAfterChange(text_changed, cursor_changed);
-  OnAfterUserAction();
+  Textfield::ExecuteEditCommand(command_id);
 }
 
 ////////////////////////////////////////////////////////////////////////////////
@@ -1644,7 +1497,49 @@ void Textfield::ExtendSelectionAndDelete(size_t before, size_t after) {
 void Textfield::EnsureCaretInRect(const gfx::Rect& rect) {}
 
 bool Textfield::IsEditCommandEnabled(int command_id) const {
-  return IsCommandIdEnabled(command_id);
+  base::string16 result;
+  bool editable = !read_only();
+  bool readable = text_input_type_ != ui::TEXT_INPUT_TYPE_PASSWORD;
+  switch (command_id) {
+    case IDS_APP_UNDO:
+      return editable && model_->CanUndo();
+    case IDS_APP_REDO:
+      return editable && model_->CanRedo();
+    case IDS_APP_CUT:
+      return editable && readable && model_->HasSelection();
+    case IDS_APP_COPY:
+      return readable && model_->HasSelection();
+    case IDS_APP_PASTE:
+      ui::Clipboard::GetForCurrentThread()->ReadText(
+          ui::CLIPBOARD_TYPE_COPY_PASTE, &result);
+      return editable && !result.empty();
+    case IDS_APP_DELETE:
+      return editable && model_->HasSelection();
+    case IDS_APP_SELECT_ALL:
+      return !text().empty();
+    case IDS_DELETE_FORWARD:
+    case IDS_DELETE_BACKWARD:
+    case IDS_DELETE_TO_BEGINNING_OF_LINE:
+    case IDS_DELETE_TO_END_OF_LINE:
+    case IDS_DELETE_WORD_BACKWARD:
+    case IDS_DELETE_WORD_FORWARD:
+      return editable;
+    case IDS_MOVE_LEFT:
+    case IDS_MOVE_LEFT_AND_MODIFY_SELECTION:
+    case IDS_MOVE_RIGHT:
+    case IDS_MOVE_RIGHT_AND_MODIFY_SELECTION:
+    case IDS_MOVE_WORD_LEFT:
+    case IDS_MOVE_WORD_LEFT_AND_MODIFY_SELECTION:
+    case IDS_MOVE_WORD_RIGHT:
+    case IDS_MOVE_WORD_RIGHT_AND_MODIFY_SELECTION:
+    case IDS_MOVE_TO_BEGINNING_OF_LINE:
+    case IDS_MOVE_TO_BEGINNING_OF_LINE_AND_MODIFY_SELECTION:
+    case IDS_MOVE_TO_END_OF_LINE:
+    case IDS_MOVE_TO_END_OF_LINE_AND_MODIFY_SELECTION:
+      return true;
+    default:
+      return false;
+  }
 }
 
 void Textfield::SetEditCommandForNextKeyEvent(int command_id) {
@@ -1679,6 +1574,129 @@ base::string16 Textfield::GetSelectionClipboardText() const {
   return selection_clipboard_text;
 }
 
+void Textfield::ExecuteEditCommand(int command_id) {
+  // Some codepaths may bypass GetCommandForKeyEvent, so any selection-dependent
+  // modifications of the command should happen here.
+  if (HasSelection()) {
+    switch (command_id) {
+      case IDS_DELETE_WORD_BACKWARD:
+      case IDS_DELETE_TO_BEGINNING_OF_LINE:
+        command_id = IDS_DELETE_BACKWARD;
+        break;
+      case IDS_DELETE_WORD_FORWARD:
+      case IDS_DELETE_TO_END_OF_LINE:
+        command_id = IDS_DELETE_FORWARD;
+        break;
+    }
+  }
+
+  // We only execute the commands enabled in Textfield::IsEditCommandEnabled
+  // below. Hence don't do a virtual IsEditCommandEnabled call.
+  if (!Textfield::IsEditCommandEnabled(command_id))
+    return;
+
+  DestroyTouchSelection();
+
+  bool text_changed = false;
+  bool cursor_changed = false;
+  bool rtl = GetTextDirection() == base::i18n::RIGHT_TO_LEFT;
+  gfx::VisualCursorDirection begin = rtl ? gfx::CURSOR_RIGHT : gfx::CURSOR_LEFT;
+  gfx::VisualCursorDirection end = rtl ? gfx::CURSOR_LEFT : gfx::CURSOR_RIGHT;
+  gfx::SelectionModel selection_model = GetSelectionModel();
+
+  OnBeforeUserAction();
+  switch (command_id) {
+    case IDS_APP_UNDO:
+      text_changed = cursor_changed = model_->Undo();
+      break;
+    case IDS_APP_REDO:
+      text_changed = cursor_changed = model_->Redo();
+      break;
+    case IDS_APP_CUT:
+      text_changed = cursor_changed = Cut();
+      break;
+    case IDS_APP_COPY:
+      Copy();
+      break;
+    case IDS_APP_PASTE:
+      text_changed = cursor_changed = Paste();
+      break;
+    case IDS_APP_DELETE:
+      text_changed = cursor_changed = model_->Delete();
+      break;
+    case IDS_APP_SELECT_ALL:
+      SelectAll(false);
+      break;
+    case IDS_DELETE_BACKWARD:
+      text_changed = cursor_changed = model_->Backspace();
+      break;
+    case IDS_DELETE_FORWARD:
+      text_changed = cursor_changed = model_->Delete();
+      break;
+    case IDS_DELETE_TO_END_OF_LINE:
+      model_->MoveCursor(gfx::LINE_BREAK, end, true);
+      text_changed = cursor_changed = model_->Delete();
+      break;
+    case IDS_DELETE_TO_BEGINNING_OF_LINE:
+      model_->MoveCursor(gfx::LINE_BREAK, begin, true);
+      text_changed = cursor_changed = model_->Backspace();
+      break;
+    case IDS_DELETE_WORD_BACKWARD:
+      model_->MoveCursor(gfx::WORD_BREAK, begin, true);
+      text_changed = cursor_changed = model_->Backspace();
+      break;
+    case IDS_DELETE_WORD_FORWARD:
+      model_->MoveCursor(gfx::WORD_BREAK, end, true);
+      text_changed = cursor_changed = model_->Delete();
+      break;
+    case IDS_MOVE_LEFT:
+      model_->MoveCursor(gfx::CHARACTER_BREAK, gfx::CURSOR_LEFT, false);
+      break;
+    case IDS_MOVE_LEFT_AND_MODIFY_SELECTION:
+      model_->MoveCursor(gfx::CHARACTER_BREAK, gfx::CURSOR_LEFT, true);
+      break;
+    case IDS_MOVE_RIGHT:
+      model_->MoveCursor(gfx::CHARACTER_BREAK, gfx::CURSOR_RIGHT, false);
+      break;
+    case IDS_MOVE_RIGHT_AND_MODIFY_SELECTION:
+      model_->MoveCursor(gfx::CHARACTER_BREAK, gfx::CURSOR_RIGHT, true);
+      break;
+    case IDS_MOVE_WORD_LEFT:
+      model_->MoveCursor(gfx::WORD_BREAK, gfx::CURSOR_LEFT, false);
+      break;
+    case IDS_MOVE_WORD_LEFT_AND_MODIFY_SELECTION:
+      model_->MoveCursor(gfx::WORD_BREAK, gfx::CURSOR_LEFT, true);
+      break;
+    case IDS_MOVE_WORD_RIGHT:
+      model_->MoveCursor(gfx::WORD_BREAK, gfx::CURSOR_RIGHT, false);
+      break;
+    case IDS_MOVE_WORD_RIGHT_AND_MODIFY_SELECTION:
+      model_->MoveCursor(gfx::WORD_BREAK, gfx::CURSOR_RIGHT, true);
+      break;
+    case IDS_MOVE_TO_BEGINNING_OF_LINE:
+      model_->MoveCursor(gfx::LINE_BREAK, begin, false);
+      break;
+    case IDS_MOVE_TO_BEGINNING_OF_LINE_AND_MODIFY_SELECTION:
+      model_->MoveCursor(gfx::LINE_BREAK, begin, true);
+      break;
+    case IDS_MOVE_TO_END_OF_LINE:
+      model_->MoveCursor(gfx::LINE_BREAK, end, false);
+      break;
+    case IDS_MOVE_TO_END_OF_LINE_AND_MODIFY_SELECTION:
+      model_->MoveCursor(gfx::LINE_BREAK, end, true);
+      break;
+    default:
+      NOTREACHED();
+      break;
+  }
+
+  cursor_changed |= GetSelectionModel() != selection_model;
+  if (cursor_changed)
+    UpdateSelectionClipboard();
+  UpdateAfterChange(text_changed, cursor_changed);
+  OnAfterUserAction();
+}
+
 ////////////////////////////////////////////////////////////////////////////////
 // Textfield, private:
 
@@ -1855,6 +1873,9 @@ void Textfield::UpdateContextMenu() {
     context_menu_contents_->AddSeparator(ui::NORMAL_SEPARATOR);
     context_menu_contents_->AddItemWithStringId(IDS_APP_SELECT_ALL,
                                                 IDS_APP_SELECT_ALL);
+
+    // If the controller adds menu commands, also override ExecuteCommand() and
+    // IsCommandIdEnabled() as appropriate, for the commands added.

[tapted, 2016/06/09 04:10:03]

I don't think it makes sense for UpdateContextMenu to be on TextFieldController,
if it needs to be paired with overrides on Textfield.
OmniboxViewViews::UpdateContextMenu is the only override, which also inherits
from Textfield already, so it would be easy to move. See what msw thinks.

[karandeepb, 2016/06/09 04:51:43]

Yeah, I agree. Will wait for owner's review on this.

     if (controller_)
       controller_->UpdateContextMenu(context_menu_contents_.get());
   }