Views: Modify Textfield::ExecuteCommand to use Textfield::IsCommandIdEnabled instead of a virtual ca

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 1284 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) {
+  if (!Textfield::IsCommandIdEnabled(command_id))
+    return;
+
   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;

[tapted, 2016/06/06 07:26:16]

The command_id can change down here, so this doesn't look right in isolation

[karandeepb, 2016/06/08 03:05:49]

Yeah I added this code in https://codereview.chromium.org/1923793002. It should
not matter since all these delete commands are enabled under the same
conditions.

[tapted, 2016/06/08 04:02:39]

Perhaps the CL needs a clearer rationale then, or this should be merged onto
another. It's not landable like this.

You might need a comment above Textfield::IsCommandIdEnabled(command_id).
Something like

// Even though |command_id| changes below, return early here so that something
something. Also, since something something, ensure Textfield's version of
IsCommandIdEnabled() is invoked rather than an override. If a subclass overrides
IsCommandIdEnabled, it should handle altered commands in an ExecuteCommand
override.

         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:
@@ skipping 594 matching lines @@
     RequestFocus();
   model_->MoveCursorTo(mouse);
   if (!selection_clipboard_text.empty()) {
     model_->InsertText(selection_clipboard_text);
     UpdateAfterChange(true, true);
   }
   OnAfterUserAction();
 }
 
 }  // namespace views