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

ui/views/controls/textfield/textfield.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "ui/views/controls/textfield/textfield.h"
 
 #include <string>
 #include <utility>
 
 #include "base/trace_event/trace_event.h"
@@ skipping 43 matching lines @@
 #include "ui/events/linux/text_edit_key_bindings_delegate_auralinux.h"
 #endif
 
 namespace views {
 
 namespace {
 
 // Default placeholder text color.
 const SkColor kDefaultPlaceholderTextColor = SK_ColorLTGRAY;
 
-const int kNoCommand = 0;
-
 void ConvertRectToScreen(const View* src, gfx::Rect* r) {
   DCHECK(src);
 
   gfx::Point new_origin = r->origin();
   View::ConvertPointToScreen(src, &new_origin);
   r->set_origin(new_origin);
 }
 
 // Get the drag selection timer delay, respecting animation scaling for testing.
 int GetDragSelectionDelay() {
   switch (ui::ScopedAnimationDurationScaleMode::duration_scale_mode()) {
       case ui::ScopedAnimationDurationScaleMode::NORMAL_DURATION: return 100;
       case ui::ScopedAnimationDurationScaleMode::FAST_DURATION:   return 25;
       case ui::ScopedAnimationDurationScaleMode::SLOW_DURATION:   return 400;
       case ui::ScopedAnimationDurationScaleMode::NON_ZERO_DURATION:   return 1;
       case ui::ScopedAnimationDurationScaleMode::ZERO_DURATION:   return 0;
     }
   return 100;
 }
 
 // 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();
   const bool alt = event.IsAltDown() || event.IsAltGrDown();
   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
 
 const gfx::FontList& GetDefaultFontList() {
   ui::ResourceBundle& rb = ui::ResourceBundle::GetSharedInstance();
   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
 const char Textfield::kViewClassName[] = "Textfield";
 const int Textfield::kTextPadding = 3;
 
 // static
 size_t Textfield::GetCaretBlinkMs() {
   static const size_t default_value = 500;
 #if defined(OS_WIN)
   static const size_t system_value = ::GetCaretBlinkTime();
   if (system_value != 0)
     return (system_value == INFINITE) ? 0 : system_value;
 #endif
   return default_value;
 }
 
 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),
       use_default_background_color_(true),
       use_default_selection_text_color_(true),
       use_default_selection_background_color_(true),
       text_color_(SK_ColorBLACK),
       background_color_(SK_ColorWHITE),
       selection_text_color_(SK_ColorWHITE),
       selection_background_color_(SK_ColorBLUE),
@@ skipping 281 matching lines @@
 }
 
 void Textfield::ClearEditHistory() {
   model_->ClearEditHistory();
 }
 
 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_;
 }
 
 ////////////////////////////////////////////////////////////////////////////////
 // Textfield, View overrides:
 
 gfx::Insets Textfield::GetInsets() const {
   gfx::Insets insets = View::GetInsets();
   insets += gfx::Insets(kTextPadding, kTextPadding, kTextPadding, kTextPadding);
@@ skipping 103 matching lines @@
   drag_selection_timer_.Stop();
   // Cancel suspected drag initiations, the user was clicking in the selection.
   if (initiating_drag_)
     MoveCursorTo(event.location(), false);
   initiating_drag_ = false;
   UpdateSelectionClipboard();
   OnAfterUserAction();
 }
 
 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.
   base::WeakPtr<Textfield> textfield(weak_ptr_factory_.GetWeakPtr());
 
   bool handled = controller_ && controller_->HandleKeyEvent(this, event);
 
   if (!textfield)
     return handled;
 
 #if defined(OS_LINUX) && !defined(OS_CHROMEOS)
   ui::TextEditKeyBindingsDelegateAuraLinux* delegate =
       ui::GetTextEditKeyBindingsDelegate();
   std::vector<ui::TextEditCommandAuraLinux> commands;
   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;
       }
     }
     return handled;
   }
 #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;
 }
 
 void Textfield::OnGestureEvent(ui::GestureEvent* event) {
   switch (event->type()) {
     case ui::ET_GESTURE_TAP_DOWN:
       RequestFocus();
       ShowImeIfNeeded();
@@ skipping 80 matching lines @@
       break;
     default:
       return;
   }
 }
 
 // This function is called by BrowserView to execute clipboard commands.
 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;
 }
 
 bool Textfield::CanHandleAccelerators() const {
   return GetRenderText()->focused() && View::CanHandleAccelerators();
 }
 
 void Textfield::AboutToRequestFocusFromTabTraversal(bool reverse) {
   SelectAll(false);
 }
 
 bool Textfield::SkipDefaultKeyEventProcessing(const ui::KeyEvent& event) {
 #if defined(OS_LINUX) && !defined(OS_CHROMEOS)
   // Skip any accelerator handling that conflicts with custom keybindings.
   ui::TextEditKeyBindingsDelegateAuraLinux* delegate =
       ui::GetTextEditKeyBindingsDelegate();
   std::vector<ui::TextEditCommandAuraLinux> commands;
   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
 
   // Skip backspace accelerator handling; editable textfields handle this key.
   // Also skip processing Windows [Alt]+<num-pad digit> Unicode alt-codes.
   const bool is_backspace = event.key_code() == ui::VKEY_BACK;
   return (is_backspace && !read_only()) || event.IsUnicodeKeyCode();
 }
 
@@ skipping 352 matching lines @@
 }
 
 ////////////////////////////////////////////////////////////////////////////////
 // Textfield, ui::SimpleMenuModel::Delegate overrides:
 
 bool Textfield::IsCommandIdChecked(int command_id) const {
   return true;
 }
 
 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,
                                            ui::Accelerator* accelerator) {
   switch (command_id) {
     case IDS_APP_UNDO:
       *accelerator = ui::Accelerator(ui::VKEY_Z, ui::EF_CONTROL_DOWN);
       return true;
 
     case IDS_APP_CUT:
@@ skipping 10 matching lines @@
 
     case IDS_APP_SELECT_ALL:
       *accelerator = ui::Accelerator(ui::VKEY_A, ui::EF_CONTROL_DOWN);
       return true;
 
     default:
       return false;
   }
 }
 
 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()));
 }
 
 ////////////////////////////////////////////////////////////////////////////////
 // Textfield, ui::TextInputClient overrides:
 
 void Textfield::SetCompositionText(const ui::CompositionText& composition) {
   if (GetTextInputType() == ui::TEXT_INPUT_TYPE_NONE)
     return;
 
   OnBeforeUserAction();
@@ skipping 201 matching lines @@
 
   range.set_start(range.start() - before);
   range.set_end(range.end() + after);
   gfx::Range text_range;
   if (GetTextRange(&text_range) && text_range.Contains(range))
     DeleteRange(range);
 }
 
 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;
 }
 
 ////////////////////////////////////////////////////////////////////////////////
 // Textfield, protected:
 
 void Textfield::DoInsertChar(base::char16 ch) {
   OnBeforeUserAction();
   skip_input_method_cancel_composition_ = true;
   if (GetRenderText()->insert_mode())
     model_->InsertChar(ch);
   else
     model_->ReplaceChar(ch);
   skip_input_method_cancel_composition_ = false;
 
   UpdateAfterChange(true, true);
   OnAfterUserAction();
 }
 
 gfx::RenderText* Textfield::GetRenderText() const {
   return model_->render_text();
 }
 
 base::string16 Textfield::GetSelectionClipboardText() const {
   base::string16 selection_clipboard_text;
   ui::Clipboard::GetForCurrentThread()->ReadText(
       ui::CLIPBOARD_TYPE_SELECTION, &selection_clipboard_text);
   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:
 
 void Textfield::AccessibilitySetValue(const base::string16& new_value) {
   if (!read_only()) {
     SetText(new_value);
     ClearSelection();
   }
 }
 
@@ skipping 239 matching lines @@
     RequestFocus();
   model_->MoveCursorTo(mouse);
   if (!selection_clipboard_text.empty()) {
     model_->InsertText(selection_clipboard_text);
     UpdateAfterChange(true, true);
   }
   OnAfterUserAction();
 }
 
 }  // namespace views