Views: Replace resource ids with ui::TextEditCommand enum for text editing commands in Textfield.

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 b31dd2d0737218a3d623dd54b9eb3ba9f2be0594..bf0540a0a5f8b9e1d29fcd9e297ae2f7de25c1b3 100644
--- a/ui/views/controls/textfield/textfield.cc
+++ b/ui/views/controls/textfield/textfield.cc
@@ -61,8 +61,6 @@ namespace {
 // Default placeholder text color.
 const SkColor kDefaultPlaceholderTextColor = SK_ColorLTGRAY;
 
-const int kNoCommand = 0;
-
 void ConvertRectToScreen(const View* src, gfx::Rect* r) {
   DCHECK(src);
 
@@ -84,9 +82,9 @@ int GetDragSelectionDelay() {
 }
 
 // Get the default command for a given key |event|.
-int GetCommandForKeyEvent(const ui::KeyEvent& event) {
+ui::TextEditCommand GetCommandForKeyEvent(const ui::KeyEvent& event) {
   if (event.type() != ui::ET_KEY_PRESSED || event.IsUnicodeKeyCode())
-    return kNoCommand;
+    return ui::TextEditCommand::INVALID_COMMAND;
 
   const bool shift = event.IsShiftDown();
   const bool control = event.IsControlDown();
@@ -94,151 +92,173 @@ int GetCommandForKeyEvent(const ui::KeyEvent& event) {
   switch (event.key_code()) {
     case ui::VKEY_Z:
       if (control && !shift && !alt)
-        return IDS_APP_UNDO;
-      return (control && shift && !alt) ? IDS_APP_REDO : kNoCommand;
+        return ui::TextEditCommand::UNDO;
+      return (control && shift && !alt) ? ui::TextEditCommand::REDO
+                                        : ui::TextEditCommand::INVALID_COMMAND;
     case ui::VKEY_Y:
-      return (control && !alt) ? IDS_APP_REDO : kNoCommand;
+      return (control && !alt) ? ui::TextEditCommand::REDO
+                               : ui::TextEditCommand::INVALID_COMMAND;
     case ui::VKEY_A:
-      return (control && !alt) ? IDS_APP_SELECT_ALL : kNoCommand;
+      return (control && !alt) ? ui::TextEditCommand::SELECT_ALL
+                               : ui::TextEditCommand::INVALID_COMMAND;
     case ui::VKEY_X:
-      return (control && !alt) ? IDS_APP_CUT : kNoCommand;
+      return (control && !alt) ? ui::TextEditCommand::CUT
+                               : ui::TextEditCommand::INVALID_COMMAND;
     case ui::VKEY_C:
-      return (control && !alt) ? IDS_APP_COPY : kNoCommand;
+      return (control && !alt) ? ui::TextEditCommand::COPY
+                               : ui::TextEditCommand::INVALID_COMMAND;
     case ui::VKEY_V:
-      return (control && !alt) ? IDS_APP_PASTE : kNoCommand;
+      return (control && !alt) ? ui::TextEditCommand::PASTE
+                               : ui::TextEditCommand::INVALID_COMMAND;
     case ui::VKEY_RIGHT:
       // Ignore alt+right, which may be a browser navigation shortcut.
       if (alt)
-        return kNoCommand;
+        return ui::TextEditCommand::INVALID_COMMAND;
       if (!shift)
-        return control ? IDS_MOVE_WORD_RIGHT : IDS_MOVE_RIGHT;
-      return control ? IDS_MOVE_WORD_RIGHT_AND_MODIFY_SELECTION :
-                        IDS_MOVE_RIGHT_AND_MODIFY_SELECTION;
+        return control ? ui::TextEditCommand::MOVE_WORD_RIGHT
+                       : ui::TextEditCommand::MOVE_RIGHT;
+      return control ? ui::TextEditCommand::MOVE_WORD_RIGHT_AND_MODIFY_SELECTION
+                     : ui::TextEditCommand::MOVE_RIGHT_AND_MODIFY_SELECTION;
     case ui::VKEY_LEFT:
       // Ignore alt+left, which may be a browser navigation shortcut.
       if (alt)
-        return kNoCommand;
+        return ui::TextEditCommand::INVALID_COMMAND;
       if (!shift)
-        return control ? IDS_MOVE_WORD_LEFT : IDS_MOVE_LEFT;
-      return control ? IDS_MOVE_WORD_LEFT_AND_MODIFY_SELECTION :
-                        IDS_MOVE_LEFT_AND_MODIFY_SELECTION;
+        return control ? ui::TextEditCommand::MOVE_WORD_LEFT
+                       : ui::TextEditCommand::MOVE_LEFT;
+      return control ? ui::TextEditCommand::MOVE_WORD_LEFT_AND_MODIFY_SELECTION
+                     : ui::TextEditCommand::MOVE_LEFT_AND_MODIFY_SELECTION;
     case ui::VKEY_HOME:
-      return shift ? IDS_MOVE_TO_BEGINNING_OF_LINE_AND_MODIFY_SELECTION :
-                      IDS_MOVE_TO_BEGINNING_OF_LINE;
+      return shift ? ui::TextEditCommand::
+                         MOVE_TO_BEGINNING_OF_LINE_AND_MODIFY_SELECTION
+                   : ui::TextEditCommand::MOVE_TO_BEGINNING_OF_LINE;
     case ui::VKEY_END:
-      return shift ? IDS_MOVE_TO_END_OF_LINE_AND_MODIFY_SELECTION :
-                      IDS_MOVE_TO_END_OF_LINE;
+      return shift
+                 ? ui::TextEditCommand::MOVE_TO_END_OF_LINE_AND_MODIFY_SELECTION
+                 : ui::TextEditCommand::MOVE_TO_END_OF_LINE;
     case ui::VKEY_BACK:
       if (!control)
-        return IDS_DELETE_BACKWARD;
+        return ui::TextEditCommand::DELETE_BACKWARD;
 #if defined(OS_LINUX)
       // Only erase by line break on Linux and ChromeOS.
       if (shift)
-        return IDS_DELETE_TO_BEGINNING_OF_LINE;
+        return ui::TextEditCommand::DELETE_TO_BEGINNING_OF_LINE;
 #endif
-      return IDS_DELETE_WORD_BACKWARD;
+      return ui::TextEditCommand::DELETE_WORD_BACKWARD;
     case ui::VKEY_DELETE:
 #if defined(OS_LINUX)
       // Only erase by line break on Linux and ChromeOS.
       if (shift && control)
-        return IDS_DELETE_TO_END_OF_LINE;
+        return ui::TextEditCommand::DELETE_TO_END_OF_LINE;
 #endif
       if (control)
-        return IDS_DELETE_WORD_FORWARD;
-      return shift ? IDS_APP_CUT : IDS_DELETE_FORWARD;
+        return ui::TextEditCommand::DELETE_WORD_FORWARD;
+      return shift ? ui::TextEditCommand::CUT
+                   : ui::TextEditCommand::DELETE_FORWARD;
     case ui::VKEY_INSERT:
       if (control && !shift)
-        return IDS_APP_COPY;
-      return (shift && !control) ? IDS_APP_PASTE : kNoCommand;
+        return ui::TextEditCommand::COPY;
+      return (shift && !control) ? ui::TextEditCommand::PASTE
+                                 : ui::TextEditCommand::INVALID_COMMAND;
     default:
-      return kNoCommand;
+      return ui::TextEditCommand::INVALID_COMMAND;
   }
 }
 
 #if defined(OS_LINUX) && !defined(OS_CHROMEOS)
 // Convert a custom text edit |command| to the equivalent views command ID.
-int GetViewsCommand(const ui::TextEditCommandAuraLinux& command, bool rtl) {
+ui::TextEditCommand GetViewsCommand(const ui::TextEditCommandAuraLinux& command,

[tapted, 2016/05/31 04:18:16]

Can we delete TextEditCommandAuraLinux::CommandId entirely? This function should
become a lot simpler -- maybe something like 

AdjustEditCommandForRTL(..)

It does raise the question of what to do with the |extend_selection| boolean --
we should either remove it from TextEditCommandAuraLinux or incorporate the
concept into ui::TextEditCommand

[karandeepb, 2016/05/31 05:12:32]

Yeah I thought it might be easier to unify the two in a follow-up, since
TextEditCommandAuraLinux commands are sent to blink, and we'll have to be
careful to not cause any changes with that.

+                                    bool rtl) {
   const bool select = command.extend_selection();
   switch (command.command_id()) {
     case ui::TextEditCommandAuraLinux::COPY:
-      return IDS_APP_COPY;
+      return ui::TextEditCommand::COPY;
     case ui::TextEditCommandAuraLinux::CUT:
-      return IDS_APP_CUT;
+      return ui::TextEditCommand::CUT;
     case ui::TextEditCommandAuraLinux::DELETE_BACKWARD:
-      return IDS_DELETE_BACKWARD;
+      return ui::TextEditCommand::DELETE_BACKWARD;
     case ui::TextEditCommandAuraLinux::DELETE_FORWARD:
-      return IDS_DELETE_FORWARD;
+      return ui::TextEditCommand::DELETE_FORWARD;
     case ui::TextEditCommandAuraLinux::DELETE_TO_BEGINING_OF_LINE:
     case ui::TextEditCommandAuraLinux::DELETE_TO_BEGINING_OF_PARAGRAPH:
-      return IDS_DELETE_TO_BEGINNING_OF_LINE;
+      return ui::TextEditCommand::DELETE_TO_BEGINNING_OF_LINE;
     case ui::TextEditCommandAuraLinux::DELETE_TO_END_OF_LINE:
     case ui::TextEditCommandAuraLinux::DELETE_TO_END_OF_PARAGRAPH:
-      return IDS_DELETE_TO_END_OF_LINE;
+      return ui::TextEditCommand::DELETE_TO_END_OF_LINE;
     case ui::TextEditCommandAuraLinux::DELETE_WORD_BACKWARD:
-      return IDS_DELETE_WORD_BACKWARD;
+      return ui::TextEditCommand::DELETE_WORD_BACKWARD;
     case ui::TextEditCommandAuraLinux::DELETE_WORD_FORWARD:
-      return IDS_DELETE_WORD_FORWARD;
+      return ui::TextEditCommand::DELETE_WORD_FORWARD;
     case ui::TextEditCommandAuraLinux::INSERT_TEXT:
-      return kNoCommand;
+      return ui::TextEditCommand::INVALID_COMMAND;
     case ui::TextEditCommandAuraLinux::MOVE_BACKWARD:
       if (rtl)
-        return select ? IDS_MOVE_RIGHT_AND_MODIFY_SELECTION : IDS_MOVE_RIGHT;
-      return select ? IDS_MOVE_LEFT_AND_MODIFY_SELECTION : IDS_MOVE_LEFT;
+        return select ? ui::TextEditCommand::MOVE_RIGHT_AND_MODIFY_SELECTION
+                      : ui::TextEditCommand::MOVE_RIGHT;
+      return select ? ui::TextEditCommand::MOVE_LEFT_AND_MODIFY_SELECTION
+                    : ui::TextEditCommand::MOVE_LEFT;
     case ui::TextEditCommandAuraLinux::MOVE_DOWN:
-      return IDS_MOVE_DOWN;
+      return ui::TextEditCommand::MOVE_DOWN;
     case ui::TextEditCommandAuraLinux::MOVE_FORWARD:
       if (rtl)
-        return select ? IDS_MOVE_LEFT_AND_MODIFY_SELECTION : IDS_MOVE_LEFT;
-      return select ? IDS_MOVE_RIGHT_AND_MODIFY_SELECTION : IDS_MOVE_RIGHT;
+        return select ? ui::TextEditCommand::MOVE_LEFT_AND_MODIFY_SELECTION
+                      : ui::TextEditCommand::MOVE_LEFT;
+      return select ? ui::TextEditCommand::MOVE_RIGHT_AND_MODIFY_SELECTION
+                    : ui::TextEditCommand::MOVE_RIGHT;
     case ui::TextEditCommandAuraLinux::MOVE_LEFT:
-      return select ? IDS_MOVE_LEFT_AND_MODIFY_SELECTION : IDS_MOVE_LEFT;
+      return select ? ui::TextEditCommand::MOVE_LEFT_AND_MODIFY_SELECTION
+                    : ui::TextEditCommand::MOVE_LEFT;
     case ui::TextEditCommandAuraLinux::MOVE_PAGE_DOWN:
     case ui::TextEditCommandAuraLinux::MOVE_PAGE_UP:
-      return kNoCommand;
+      return ui::TextEditCommand::INVALID_COMMAND;
     case ui::TextEditCommandAuraLinux::MOVE_RIGHT:
-      return select ? IDS_MOVE_RIGHT_AND_MODIFY_SELECTION : IDS_MOVE_RIGHT;
+      return select ? ui::TextEditCommand::MOVE_RIGHT_AND_MODIFY_SELECTION
+                    : ui::TextEditCommand::MOVE_RIGHT;
     case ui::TextEditCommandAuraLinux::MOVE_TO_BEGINING_OF_DOCUMENT:
     case ui::TextEditCommandAuraLinux::MOVE_TO_BEGINING_OF_LINE:
     case ui::TextEditCommandAuraLinux::MOVE_TO_BEGINING_OF_PARAGRAPH:
-      return select ? IDS_MOVE_TO_BEGINNING_OF_LINE_AND_MODIFY_SELECTION :
-                      IDS_MOVE_TO_BEGINNING_OF_LINE;
+      return select ? ui::TextEditCommand::
+                          MOVE_TO_BEGINNING_OF_LINE_AND_MODIFY_SELECTION
+                    : ui::TextEditCommand::MOVE_TO_BEGINNING_OF_LINE;
     case ui::TextEditCommandAuraLinux::MOVE_TO_END_OF_DOCUMENT:
     case ui::TextEditCommandAuraLinux::MOVE_TO_END_OF_LINE:
     case ui::TextEditCommandAuraLinux::MOVE_TO_END_OF_PARAGRAPH:
-      return select ? IDS_MOVE_TO_END_OF_LINE_AND_MODIFY_SELECTION :
-                      IDS_MOVE_TO_END_OF_LINE;
+      return select
+                 ? ui::TextEditCommand::MOVE_TO_END_OF_LINE_AND_MODIFY_SELECTION
+                 : ui::TextEditCommand::MOVE_TO_END_OF_LINE;
     case ui::TextEditCommandAuraLinux::MOVE_UP:
-      return IDS_MOVE_UP;
+      return ui::TextEditCommand::MOVE_UP;
     case ui::TextEditCommandAuraLinux::MOVE_WORD_BACKWARD:
       if (rtl) {
-        return select ? IDS_MOVE_WORD_RIGHT_AND_MODIFY_SELECTION :
-                        IDS_MOVE_WORD_RIGHT;
+        return select
+                   ? ui::TextEditCommand::MOVE_WORD_RIGHT_AND_MODIFY_SELECTION
+                   : ui::TextEditCommand::MOVE_WORD_RIGHT;
       }
-      return select ? IDS_MOVE_WORD_LEFT_AND_MODIFY_SELECTION :
-                      IDS_MOVE_WORD_LEFT;
+      return select ? ui::TextEditCommand::MOVE_WORD_LEFT_AND_MODIFY_SELECTION
+                    : ui::TextEditCommand::MOVE_WORD_LEFT;
     case ui::TextEditCommandAuraLinux::MOVE_WORD_FORWARD:
       if (rtl) {
-        return select ? IDS_MOVE_WORD_LEFT_AND_MODIFY_SELECTION :
-                        IDS_MOVE_WORD_LEFT;
+        return select ? ui::TextEditCommand::MOVE_WORD_LEFT_AND_MODIFY_SELECTION
+                      : ui::TextEditCommand::MOVE_WORD_LEFT;
       }
-      return select ? IDS_MOVE_WORD_RIGHT_AND_MODIFY_SELECTION :
-                      IDS_MOVE_WORD_RIGHT;
+      return select ? ui::TextEditCommand::MOVE_WORD_RIGHT_AND_MODIFY_SELECTION
+                    : ui::TextEditCommand::MOVE_WORD_RIGHT;
     case ui::TextEditCommandAuraLinux::MOVE_WORD_LEFT:
-      return select ? IDS_MOVE_WORD_LEFT_AND_MODIFY_SELECTION :
-                      IDS_MOVE_WORD_LEFT;
+      return select ? ui::TextEditCommand::MOVE_WORD_LEFT_AND_MODIFY_SELECTION
+                    : ui::TextEditCommand::MOVE_WORD_LEFT;
     case ui::TextEditCommandAuraLinux::MOVE_WORD_RIGHT:
-      return select ? IDS_MOVE_WORD_RIGHT_AND_MODIFY_SELECTION :
-                      IDS_MOVE_WORD_RIGHT;
+      return select ? ui::TextEditCommand::MOVE_WORD_RIGHT_AND_MODIFY_SELECTION
+                    : ui::TextEditCommand::MOVE_WORD_RIGHT;
     case ui::TextEditCommandAuraLinux::PASTE:
-      return IDS_APP_PASTE;
+      return ui::TextEditCommand::PASTE;
     case ui::TextEditCommandAuraLinux::SELECT_ALL:
-      return IDS_APP_SELECT_ALL;
+      return ui::TextEditCommand::SELECT_ALL;
     case ui::TextEditCommandAuraLinux::SET_MARK:
     case ui::TextEditCommandAuraLinux::UNSELECT:
     case ui::TextEditCommandAuraLinux::INVALID_COMMAND:
-      return kNoCommand;
+      return ui::TextEditCommand::INVALID_COMMAND;
   }
-  return kNoCommand;
+  return ui::TextEditCommand::INVALID_COMMAND;
 }
 #endif
 
@@ -247,6 +267,33 @@ const gfx::FontList& GetDefaultFontList() {
   return rb.GetFontListWithDelta(ui::kLabelFontSizeDelta);
 }
 
+// Returns the equivalent ui::TextEditCommand for the given |command_id|
+// corresponding to a context menu action.
+ui::TextEditCommand GetTextEditCommandForMenuCommand(int command_id,
+                                                     bool has_selection) {
+  switch (command_id) {
+    case IDS_APP_UNDO:
+      return ui::TextEditCommand::UNDO;
+    case IDS_APP_CUT:
+      return ui::TextEditCommand::CUT;
+    case IDS_APP_COPY:
+      return ui::TextEditCommand::COPY;
+    case IDS_APP_PASTE:
+      return ui::TextEditCommand::PASTE;
+    case IDS_APP_DELETE:
+      // The menu command IDS_APP_DELETE should only work during an active
+      // selection.
+      if (has_selection)
+        return ui::TextEditCommand::DELETE_FORWARD;
+      break;
+    case IDS_APP_SELECT_ALL:
+      return ui::TextEditCommand::SELECT_ALL;
+    default:
+      break;
+  }
+  return ui::TextEditCommand::INVALID_COMMAND;
+}
+
 }  // namespace
 
 // static
@@ -267,7 +314,7 @@ size_t Textfield::GetCaretBlinkMs() {
 Textfield::Textfield()
     : model_(new TextfieldModel(this)),
       controller_(NULL),
-      scheduled_edit_command_(kNoCommand),
+      scheduled_edit_command_(ui::TextEditCommand::INVALID_COMMAND),
       read_only_(false),
       default_width_in_chars_(0),
       use_default_text_color_(true),
@@ -569,10 +616,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_;
 }
@@ -696,8 +739,8 @@ void Textfield::OnMouseReleased(const ui::MouseEvent& event) {
 }
 
 bool Textfield::OnKeyPressed(const ui::KeyEvent& event) {
-  int edit_command = scheduled_edit_command_;
-  scheduled_edit_command_ = kNoCommand;
+  ui::TextEditCommand edit_command = scheduled_edit_command_;
+  scheduled_edit_command_ = ui::TextEditCommand::INVALID_COMMAND;
 
   // Since HandleKeyEvent() might destroy |this|, get a weak pointer and verify
   // it isn't null before proceeding.
@@ -715,9 +758,11 @@ bool Textfield::OnKeyPressed(const ui::KeyEvent& event) {
   if (!handled && delegate && delegate->MatchEvent(event, &commands)) {
     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);
+      ui::TextEditCommand command = GetViewsCommand(commands[i], rtl);
+      // Ask do we want to use a virtual call? (probably not because we are
+      // bubbling up.)
+      if (Textfield::IsEditCommandEnabled(command)) {

[tapted, 2016/05/31 04:18:17]

Using the Textfield:: qualifier when not in that method's override doesn't look
right.  We should defer this to a follow-up

+        Textfield::ExecuteEditCommand(command);
         handled = true;
       }
     }
@@ -725,11 +770,11 @@ bool Textfield::OnKeyPressed(const ui::KeyEvent& event) {
   }
 #endif
 
-  if (edit_command == kNoCommand)
+  if (edit_command == ui::TextEditCommand::INVALID_COMMAND)
     edit_command = GetCommandForKeyEvent(event);
 
-  if (!handled && IsCommandIdEnabled(edit_command)) {
-    ExecuteCommand(edit_command);
+  if (!handled && Textfield::IsEditCommandEnabled(edit_command)) {
+    Textfield::ExecuteEditCommand(edit_command);
     handled = true;
   }
   return handled;
@@ -830,7 +875,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;
 }
 
@@ -851,7 +896,13 @@ 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)))
+      // Ask virtual, probably yes.
+      // Why the different codepaths for windows and Linux. On Linux, a Ctrl+V
+      // on a omnibox is processed via OnKeyPressed, but on Windows via
+      // Textfield::AcceleratorPressed. -
+      // https://codereview.chromium.org/213673009
+      // Shouldn't we do the same on other platforms.
+      if (IsEditCommandEnabled(GetViewsCommand(commands[i], rtl)))
         return true;
   }
 #endif
@@ -1224,49 +1275,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 IsEditCommandEnabled(
+      GetTextEditCommandForMenuCommand(command_id, HasSelection()));
 }
 
 bool Textfield::GetAcceleratorForCommandId(int command_id,
@@ -1298,124 +1308,8 @@ bool Textfield::GetAcceleratorForCommandId(int command_id,
 }
 
 void Textfield::ExecuteCommand(int command_id, int event_flags) {

[tapted, 2016/05/31 04:18:17]

What's still calling this? (can it be replaced with ExecuteEditCommand?)

It would be nice for review if we could just change the signature of this method
in-place. It's tricky to review when the body is also moving.

[karandeepb, 2016/05/31 05:12:32]

IsCommandIdEnabled and ExecuteCommand(command id, event_flags) are
ui::SimpleMenuModel::Delegate overrides. These are used for context menu actions
which can be seen when you right click on textfield/omnibox. Hence I don't think
that this can be removed. 

The problem currently is that all text editing commands in Textfield and
OmniboxViewsView are being routed through these two functions, while they were
probably meant to be used only for menu actions. So this CL is trying to
decouple text edit commands from menu commands, by using IsEditCommandEnabled
(ui::TextInputClient override) and ExecuteEditCommand (new virtual function) for
text editing.

-  DestroyTouchSelection();
-
-  // 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;
-    }
-  }
-
-  if (!IsCommandIdEnabled(command_id))
-    return;
-
-  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();
+  ExecuteEditCommand(
+      GetTextEditCommandForMenuCommand(command_id, HasSelection()));
 }
 
 ////////////////////////////////////////////////////////////////////////////////
@@ -1637,13 +1531,64 @@ void Textfield::ExtendSelectionAndDelete(size_t before, size_t after) {
 
 void Textfield::EnsureCaretInRect(const gfx::Rect& rect) {}
 
-bool Textfield::IsEditCommandEnabled(int command_id) {
-  return IsCommandIdEnabled(command_id);
+bool Textfield::IsEditCommandEnabled(ui::TextEditCommand command) const {
+  base::string16 result;
+  bool editable = !read_only();
+  bool readable = text_input_type_ != ui::TEXT_INPUT_TYPE_PASSWORD;
+  switch (command) {
+    case ui::TextEditCommand::INVALID_COMMAND:
+      return false;
+    case ui::TextEditCommand::UNDO:
+      return editable && model_->CanUndo();
+    case ui::TextEditCommand::REDO:
+      return editable && model_->CanRedo();
+    case ui::TextEditCommand::CUT:
+      return editable && readable && model_->HasSelection();
+    case ui::TextEditCommand::COPY:
+      return readable && model_->HasSelection();
+    case ui::TextEditCommand::PASTE:
+      ui::Clipboard::GetForCurrentThread()->ReadText(
+          ui::CLIPBOARD_TYPE_COPY_PASTE, &result);
+      return editable && !result.empty();
+    case ui::TextEditCommand::SELECT_ALL:
+      return !text().empty();
+    case ui::TextEditCommand::DELETE_BACKWARD:
+    case ui::TextEditCommand::DELETE_FORWARD:
+    case ui::TextEditCommand::DELETE_TO_END_OF_LINE:
+    case ui::TextEditCommand::DELETE_TO_BEGINNING_OF_LINE:
+    case ui::TextEditCommand::DELETE_WORD_BACKWARD:
+    case ui::TextEditCommand::DELETE_WORD_FORWARD:
+      return editable;
+    case ui::TextEditCommand::MOVE_LEFT:
+    case ui::TextEditCommand::MOVE_LEFT_AND_MODIFY_SELECTION:
+    case ui::TextEditCommand::MOVE_RIGHT:
+    case ui::TextEditCommand::MOVE_RIGHT_AND_MODIFY_SELECTION:
+    case ui::TextEditCommand::MOVE_WORD_LEFT:
+    case ui::TextEditCommand::MOVE_WORD_LEFT_AND_MODIFY_SELECTION:
+    case ui::TextEditCommand::MOVE_WORD_RIGHT:
+    case ui::TextEditCommand::MOVE_WORD_RIGHT_AND_MODIFY_SELECTION:
+    case ui::TextEditCommand::MOVE_TO_BEGINNING_OF_LINE:
+    case ui::TextEditCommand::MOVE_TO_BEGINNING_OF_LINE_AND_MODIFY_SELECTION:
+    case ui::TextEditCommand::MOVE_TO_END_OF_LINE:
+    case ui::TextEditCommand::MOVE_TO_END_OF_LINE_AND_MODIFY_SELECTION:
+      return true;
+    // Added code here.

[tapted, 2016/05/31 04:18:16]

Reviewing changes like this is quite tricky - I think we need to move this
implementation so that it sits under Textfield::IsCommandIdEnabled() with a //
TODO(..) Move this definition where it belongs.

Maybe it goes to OWNERS that way, and they say move it now, but at least the
changes will be clear. Alternatively, we should move the method first, but not
make any changes to it at all.

+    case ui::TextEditCommand::MOVE_UP:
+    case ui::TextEditCommand::MOVE_DOWN:
+    case ui::TextEditCommand::MOVE_PAGE_UP:
+    case ui::TextEditCommand::MOVE_PAGE_DOWN:
+#if defined(OS_MACOSX)
+      return true;
+#else
+      return false;
+#endif
+  }
+  return false;
 }
 
-void Textfield::SetEditCommandForNextKeyEvent(int command_id) {
-  DCHECK_EQ(kNoCommand, scheduled_edit_command_);
-  scheduled_edit_command_ = command_id;
+void Textfield::SetEditCommandForNextKeyEvent(ui::TextEditCommand command) {
+  DCHECK(scheduled_edit_command_ == ui::TextEditCommand::INVALID_COMMAND);
+  scheduled_edit_command_ = command;
 }
 
 ////////////////////////////////////////////////////////////////////////////////
@@ -1673,6 +1618,142 @@ base::string16 Textfield::GetSelectionClipboardText() const {
   return selection_clipboard_text;
 }
 
+void Textfield::ExecuteEditCommand(ui::TextEditCommand command) {
+  if (!Textfield::IsEditCommandEnabled(command))
+    return;
+
+  DestroyTouchSelection();
+
+  // Some codepaths may bypass GetCommandForKeyEvent, so any selection-dependent
+  // modifications of the command should happen here.
+  if (HasSelection()) {
+    switch (command) {
+      case ui::TextEditCommand::DELETE_WORD_BACKWARD:
+      case ui::TextEditCommand::DELETE_TO_BEGINNING_OF_LINE:
+        command = ui::TextEditCommand::DELETE_BACKWARD;
+        break;
+      case ui::TextEditCommand::DELETE_WORD_FORWARD:
+      case ui::TextEditCommand::DELETE_TO_END_OF_LINE:
+        command = ui::TextEditCommand::DELETE_FORWARD;
+        break;
+      default:
+        break;
+    }
+  }
+
+// Added code here.
+#if defined(OS_MACOSX)
+  switch (command) {
+    case ui::TextEditCommand::MOVE_UP:
+    case ui::TextEditCommand::MOVE_PAGE_UP:
+      command = ui::TextEditCommand::MOVE_TO_BEGINNING_OF_LINE;
+      break;
+    case ui::TextEditCommand::MOVE_DOWN:
+    case ui::TextEditCommand::MOVE_PAGE_DOWN:
+      command = ui::TextEditCommand::MOVE_TO_END_OF_LINE;
+      break;
+    default:
+      break;
+  }
+#endif
+
+  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) {
+    case ui::TextEditCommand::UNDO:
+      text_changed = cursor_changed = model_->Undo();
+      break;
+    case ui::TextEditCommand::REDO:
+      text_changed = cursor_changed = model_->Redo();
+      break;
+    case ui::TextEditCommand::CUT:
+      text_changed = cursor_changed = Cut();
+      break;
+    case ui::TextEditCommand::COPY:
+      Copy();
+      break;
+    case ui::TextEditCommand::PASTE:
+      text_changed = cursor_changed = Paste();
+      break;
+    case ui::TextEditCommand::SELECT_ALL:
+      SelectAll(false);
+      break;
+    case ui::TextEditCommand::DELETE_BACKWARD:
+      text_changed = cursor_changed = model_->Backspace();
+      break;
+    case ui::TextEditCommand::DELETE_FORWARD:
+      text_changed = cursor_changed = model_->Delete();
+      break;
+    case ui::TextEditCommand::DELETE_TO_END_OF_LINE:
+      model_->MoveCursor(gfx::LINE_BREAK, end, true);
+      text_changed = cursor_changed = model_->Delete();
+      break;
+    case ui::TextEditCommand::DELETE_TO_BEGINNING_OF_LINE:
+      model_->MoveCursor(gfx::LINE_BREAK, begin, true);
+      text_changed = cursor_changed = model_->Backspace();
+      break;
+    case ui::TextEditCommand::DELETE_WORD_BACKWARD:
+      model_->MoveCursor(gfx::WORD_BREAK, begin, true);
+      text_changed = cursor_changed = model_->Backspace();
+      break;
+    case ui::TextEditCommand::DELETE_WORD_FORWARD:
+      model_->MoveCursor(gfx::WORD_BREAK, end, true);
+      text_changed = cursor_changed = model_->Delete();
+      break;
+    case ui::TextEditCommand::MOVE_LEFT:
+      model_->MoveCursor(gfx::CHARACTER_BREAK, gfx::CURSOR_LEFT, false);
+      break;
+    case ui::TextEditCommand::MOVE_LEFT_AND_MODIFY_SELECTION:
+      model_->MoveCursor(gfx::CHARACTER_BREAK, gfx::CURSOR_LEFT, true);
+      break;
+    case ui::TextEditCommand::MOVE_RIGHT:
+      model_->MoveCursor(gfx::CHARACTER_BREAK, gfx::CURSOR_RIGHT, false);
+      break;
+    case ui::TextEditCommand::MOVE_RIGHT_AND_MODIFY_SELECTION:
+      model_->MoveCursor(gfx::CHARACTER_BREAK, gfx::CURSOR_RIGHT, true);
+      break;
+    case ui::TextEditCommand::MOVE_WORD_LEFT:
+      model_->MoveCursor(gfx::WORD_BREAK, gfx::CURSOR_LEFT, false);
+      break;
+    case ui::TextEditCommand::MOVE_WORD_LEFT_AND_MODIFY_SELECTION:
+      model_->MoveCursor(gfx::WORD_BREAK, gfx::CURSOR_LEFT, true);
+      break;
+    case ui::TextEditCommand::MOVE_WORD_RIGHT:
+      model_->MoveCursor(gfx::WORD_BREAK, gfx::CURSOR_RIGHT, false);
+      break;
+    case ui::TextEditCommand::MOVE_WORD_RIGHT_AND_MODIFY_SELECTION:
+      model_->MoveCursor(gfx::WORD_BREAK, gfx::CURSOR_RIGHT, true);
+      break;
+    case ui::TextEditCommand::MOVE_TO_BEGINNING_OF_LINE:
+      model_->MoveCursor(gfx::LINE_BREAK, begin, false);
+      break;
+    case ui::TextEditCommand::MOVE_TO_BEGINNING_OF_LINE_AND_MODIFY_SELECTION:
+      model_->MoveCursor(gfx::LINE_BREAK, begin, true);
+      break;
+    case ui::TextEditCommand::MOVE_TO_END_OF_LINE:
+      model_->MoveCursor(gfx::LINE_BREAK, end, false);
+      break;
+    case ui::TextEditCommand::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: