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

chrome/browser/ui/views/omnibox/omnibox_view_views.cc

Index: chrome/browser/ui/views/omnibox/omnibox_view_views.cc
diff --git a/chrome/browser/ui/views/omnibox/omnibox_view_views.cc b/chrome/browser/ui/views/omnibox/omnibox_view_views.cc
index 8852dfe48c57321a8a6cbf13ad68624259255622..1dd62b46c7a8ab94ed00bac72847a95961d15956 100644
--- a/chrome/browser/ui/views/omnibox/omnibox_view_views.cc
+++ b/chrome/browser/ui/views/omnibox/omnibox_view_views.cc
@@ -366,14 +366,10 @@ void OmniboxViewViews::ExecuteCommand(int command_id, int event_flags) {
     case IDC_EDIT_SEARCH_ENGINES:
       location_bar_view_->command_updater()->ExecuteCommand(command_id);
       return;
-    case IDS_MOVE_DOWN:
-    case IDS_MOVE_UP:
-      model()->OnUpOrDownKeyPressed(command_id == IDS_MOVE_DOWN ? 1 : -1);
-      return;
 
     // These commands do invoke the popup.
     case IDS_APP_PASTE:
-      OnPaste();
+      ExecuteEditCommand(IDS_APP_PASTE);
       return;
     default:
       if (Textfield::IsCommandIdEnabled(command_id)) {
@@ -758,14 +754,14 @@ bool OmniboxViewViews::OnKeyPressed(const ui::KeyEvent& event) {
         model()->popup_model()->TryDeletingCurrentItem();
       break;
     case ui::VKEY_UP:
-      if (!read_only()) {
-        model()->OnUpOrDownKeyPressed(-1);
+      if (IsEditCommandEnabled(IDS_MOVE_UP)) {
+        ExecuteEditCommand(IDS_MOVE_UP);
         return true;
       }
       break;
     case ui::VKEY_DOWN:
-      if (!read_only()) {
-        model()->OnUpOrDownKeyPressed(1);
+      if (IsEditCommandEnabled(IDS_MOVE_DOWN)) {
+        ExecuteEditCommand(IDS_MOVE_DOWN);
         return true;
       }
       break;
@@ -780,14 +776,14 @@ bool OmniboxViewViews::OnKeyPressed(const ui::KeyEvent& event) {
       model()->OnUpOrDownKeyPressed(model()->result().size());
       return true;
     case ui::VKEY_V:
-      if (control && !alt && !read_only()) {
-        ExecuteCommand(IDS_APP_PASTE, 0);
+      if (control && !alt && IsEditCommandEnabled(IDS_APP_PASTE)) {
+        ExecuteEditCommand(IDS_APP_PASTE);
         return true;
       }
       break;
     case ui::VKEY_INSERT:
-      if (shift && !control && !read_only()) {
-        ExecuteCommand(IDS_APP_PASTE, 0);
+      if (shift && !control && IsEditCommandEnabled(IDS_APP_PASTE)) {
+        ExecuteEditCommand(IDS_APP_PASTE);
         return true;
       }
       break;
@@ -908,8 +904,7 @@ bool OmniboxViewViews::IsCommandIdEnabled(int command_id) const {
     return !read_only() && model()->CanPasteAndGo(GetClipboardText());
   if (command_id == IDS_SHOW_URL)
     return controller()->GetToolbarModel()->WouldReplaceURL();
-  return command_id == IDS_MOVE_DOWN || command_id == IDS_MOVE_UP ||
-         Textfield::IsCommandIdEnabled(command_id) ||
+  return Textfield::IsCommandIdEnabled(command_id) ||
          location_bar_view_->command_updater()->IsCommandEnabled(command_id);
 }
 
@@ -925,6 +920,42 @@ void OmniboxViewViews::DoInsertChar(base::char16 ch) {
   Textfield::DoInsertChar(ch);
 }
 
+bool OmniboxViewViews::IsEditCommandEnabled(int command_id) const {
+  switch (command_id) {
+    case IDS_MOVE_UP:
+    case IDS_MOVE_DOWN:
+    case IDS_APP_PASTE:

[tapted, 2016/06/06 07:42:27]

Is Paste not handled by Textfield::IsCommandIdEnabled already? it also disables
the command when the clipboard is empty.

[karandeepb, 2016/06/08 03:06:57]

Added the clipboard condition. Yeah it is. But Omnibox uses a custom
GetClipboardText() implementation
https://cs.chromium.org/chromium/src/chrome/browser/ui/omnibox/clipboard_util...

+      return !read_only();
+    default:
+      return Textfield::IsEditCommandEnabled(command_id);
+  }
+}
+
+void OmniboxViewViews::ExecuteEditCommand(int command_id) {
+  if (!IsEditCommandEnabled(command_id))

[tapted, 2016/06/06 07:42:27]

could this DCHECK? (if we can't we should probably add a comment, since it looks
a bit unusual)

[karandeepb, 2016/06/08 03:06:57]

A normal if condition looks safer to me. Why does it look unusual though,
because it's a protected method?

+    return;
+
+  // In the base class, touch text selection is deactivated when a command is
+  // executed. Since we are not always calling the base class implementation
+  // here, we need to deactivate touch text selection here, too.
+  DestroyTouchSelection();
+
+  switch (command_id) {
+    case IDS_MOVE_UP:
+      model()->OnUpOrDownKeyPressed(-1);
+      break;
+    case IDS_MOVE_DOWN:
+      model()->OnUpOrDownKeyPressed(1);
+      break;
+    case IDS_APP_PASTE:
+      OnPaste();
+      break;
+    default:
+      Textfield::ExecuteEditCommand(command_id);
+      break;
+  }
+}
+
 #if defined(OS_CHROMEOS)
 void OmniboxViewViews::CandidateWindowOpened(
       chromeos::input_method::InputMethodManager* manager) {