[Multipaste] Refactoring on the command execution

It is confusing that the count of items in the menu model is different
from that of the item views in the context menu. In the
current code, we have to insert the deletion command because
MenuModelAdapter::ExecuteCommand() checks whether the command to execute
is contained in the menu model.

In this CL, the special command id (i.e. the item deletion command id)
is removed. Instead, We use the command id to indicate which menu item
is activated. When the clipboard history controller is notified of the
activation, it queries the menu item specified by the command id about
the action to take.

This CL should not introduce any visual difference

Bug: 1131556
Change-Id: I51b6c56aea656c1a2c764d756e616823c0bbf850
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2531063
Commit-Queue: Andrew Xu <andrewxu@chromium.org>
Reviewed-by: David Black <dmblack@google.com>
Cr-Commit-Position: refs/heads/master@{#828040}

ash/clipboard/clipboard_history_controller_impl.cc

@@ -135,8 +135,7 @@ class ClipboardHistoryControllerImpl::AcceleratorTarget
 
   void HandleDeleteSelected(int event_flags) {
     DCHECK(controller_->IsMenuShowing());
-    controller_->ExecuteCommand(ClipboardHistoryUtil::kDeleteCommandId,
-                                event_flags);
+    controller_->DeleteSelectedMenuItemIfAny();
   }
 
   void HandleTab() {
@@ -278,21 +277,30 @@ void ClipboardHistoryControllerImpl::ExecuteSelectedMenuItem(int event_flags) {
   auto command = context_menu_->GetSelectedMenuItemCommand();
 
   // If no menu item is currently selected, we'll fallback to the first item.
-  menu_delegate_->ExecuteCommand(
-      command.value_or(ClipboardHistoryUtil::kFirstItemCommandId), event_flags);
+  PasteMenuItemData(command.value_or(ClipboardHistoryUtil::kFirstItemCommandId),
+                    event_flags);
 }
 
 void ClipboardHistoryControllerImpl::ExecuteCommand(int command_id,
                                                     int event_flags) {
   DCHECK(context_menu_);
 
-  if (command_id == ClipboardHistoryUtil::kDeleteCommandId) {
-    DeleteSelectedMenuItemIfAny();
-    return;
-  }
-
   DCHECK_GE(command_id, ClipboardHistoryUtil::kFirstItemCommandId);
-  PasteMenuItemData(command_id, event_flags & ui::EF_SHIFT_DOWN);
+  DCHECK_LE(command_id, ClipboardHistoryUtil::kMaxItemCommandId);
+
+  using Action = ClipboardHistoryUtil::Action;
+  Action action = context_menu_->GetActionForCommandId(command_id);
+  switch (action) {
+    case Action::kPaste:
+      PasteMenuItemData(command_id, event_flags & ui::EF_SHIFT_DOWN);
+      return;
+    case Action::kDelete:
+      DeleteItemWithCommandId(command_id);
+      return;
+    case Action::kEmpty:
+      NOTREACHED();
+      return;
+  }
 }
 
 void ClipboardHistoryControllerImpl::PasteMenuItemData(int command_id,
@@ -375,10 +383,11 @@ void ClipboardHistoryControllerImpl::DeleteSelectedMenuItemIfAny() {
   if (!selected_command.has_value())
     return;
 
-  DCHECK_GE(*selected_command, ClipboardHistoryUtil::kFirstItemCommandId);
+  DeleteItemWithCommandId(*selected_command);
+}
 
-  const auto& to_be_deleted_item =
-      context_menu_->GetItemFromCommandId(*selected_command);
+void ClipboardHistoryControllerImpl::DeleteItemWithCommandId(int command_id) {
+  DCHECK(context_menu_);
 
   // Pressing VKEY_DELETE is handled here via AcceleratorTarget because the
   // contextual menu consumes the key event. Record the "pressing the delete
@@ -386,6 +395,8 @@ void ClipboardHistoryControllerImpl::DeleteSelectedMenuItemIfAny() {
   // activating the button directly via click/tap. There is no special handling
   // for pasting an item via VKEY_RETURN because in that case the menu does not
   // process the key event.
+  const auto& to_be_deleted_item =
+      context_menu_->GetItemFromCommandId(command_id);
   ClipboardHistoryUtil::RecordClipboardHistoryItemDeleted(to_be_deleted_item);
   clipboard_history_->RemoveItemForId(to_be_deleted_item.id());
 
@@ -396,7 +407,7 @@ void ClipboardHistoryControllerImpl::DeleteSelectedMenuItemIfAny() {
     return;
   }
 
-  context_menu_->RemoveSelectedMenuItem();
+  context_menu_->RemoveMenuItemWithCommandId(command_id);
 }
 
 void ClipboardHistoryControllerImpl::AdvancePseudoFocus(bool reverse) {

ash/clipboard/clipboard_history_controller_impl.h

@@ -106,6 +106,9 @@ class ASH_EXPORT ClipboardHistoryControllerImpl
   // is selected, do nothing.
   void DeleteSelectedMenuItemIfAny();
 
+  // Delete the menu item specified by `command_id` and its corresponding data.
+  void DeleteItemWithCommandId(int command_id);
+
   // Advances the pseudo focus (backward if `reverse` is true).
   void AdvancePseudoFocus(bool reverse);
 

ash/clipboard/clipboard_history_menu_model_adapter.cc

@@ -28,8 +28,8 @@
 namespace ash {
 
 namespace {
-bool IsDataReadAllowed(ui::DataTransferEndpoint* source,
-                       ui::DataTransferEndpoint* destination) {
+bool IsDataReadAllowed(const ui::DataTransferEndpoint* source,
+                       const ui::DataTransferEndpoint* destination) {
   ui::DataTransferPolicyController* policy_controller =
       ui::DataTransferPolicyController::Get();
   if (!policy_controller)
@@ -69,6 +69,9 @@ void ClipboardHistoryMenuModelAdapter::Run(
   // not shown.
   UMA_HISTOGRAM_COUNTS_100(
       "Ash.ClipboardHistory.ContextMenu.NumberOfItemsShown", items.size());
+
+  const ui::DataTransferEndpoint data_dst(ui::EndpointType::kDefault,
+                                          /*notify_if_restricted=*/false);
   for (const auto& item : items) {
     model_->AddItem(command_id, base::string16());
 
@@ -76,8 +79,6 @@ void ClipboardHistoryMenuModelAdapter::Run(
     // clipboard history item is allowed to read or not.
     // This clipboard read isn't initiated by the user, that's why it shouldn't
     // notify if the clipboard is restricted.
-    ui::DataTransferEndpoint data_dst(ui::EndpointType::kDefault,
-                                      /*notify_if_restricted=*/false);
     model_->SetEnabledAt(model_->GetIndexOfCommandId(command_id),
                          IsDataReadAllowed(item.data().source(), &data_dst));
 
@@ -85,9 +86,6 @@ void ClipboardHistoryMenuModelAdapter::Run(
     ++command_id;
   }
 
-  // Enable the command execution through the model delegate.
-  model_->AddItem(ClipboardHistoryUtil::kDeleteCommandId, base::string16());
-
   // Start async rendering of HTML, if any exists.
   ClipboardImageModelFactory::Get()->Activate();
 
@@ -145,16 +143,13 @@ void ClipboardHistoryMenuModelAdapter::SelectMenuItemWithCommandId(
       selected_menu_item);
 }
 
-void ClipboardHistoryMenuModelAdapter::RemoveSelectedMenuItem() {
-  base::Optional<int> current_selected_command_id =
-      GetSelectedMenuItemCommand();
-  DCHECK(current_selected_command_id.has_value());
-
+void ClipboardHistoryMenuModelAdapter::RemoveMenuItemWithCommandId(
+    int command_id) {
   // Calculate `new_selected_command_id` before removing
-  // `current_selected_command_id` from data structures because the latter is
-  // needed in calculation.
+  // the item specified by `command_id` from data structures because the item to
+  // be removed is needed in calculation.
   base::Optional<int> new_selected_command_id =
-      CalculateSelectedCommandIdAfterDeletion();
+      CalculateSelectedCommandIdAfterDeletion(command_id);
 
   // Update the menu item selection.
   if (new_selected_command_id.has_value()) {
@@ -164,17 +159,16 @@ void ClipboardHistoryMenuModelAdapter::RemoveSelectedMenuItem() {
         root_view_);
   }
 
-  auto item_view_to_delete =
-      item_views_by_command_id_.find(*current_selected_command_id);
+  auto item_view_to_delete = item_views_by_command_id_.find(command_id);
   DCHECK(item_view_to_delete != item_views_by_command_id_.cend());
 
   // Disable views to be removed in order to prevent them from handling events.
-  root_view_->GetMenuItemByID(*current_selected_command_id)->SetEnabled(false);
+  root_view_->GetMenuItemByID(command_id)->SetEnabled(false);
   item_view_to_delete->second->SetEnabled(false);
 
   item_views_by_command_id_.erase(item_view_to_delete);
 
-  auto item_to_delete = item_snapshots_.find(*current_selected_command_id);
+  auto item_to_delete = item_snapshots_.find(command_id);
   DCHECK(item_to_delete != item_snapshots_.end());
   item_snapshots_.erase(item_to_delete);
 
@@ -183,8 +177,7 @@ void ClipboardHistoryMenuModelAdapter::RemoveSelectedMenuItem() {
   base::SequencedTaskRunnerHandle::Get()->PostTask(
       FROM_HERE,
       base::BindOnce(&ClipboardHistoryMenuModelAdapter::RemoveItemView,
-                     weak_ptr_factory_.GetWeakPtr(),
-                     *current_selected_command_id));
+                     weak_ptr_factory_.GetWeakPtr(), command_id));
 }
 
 void ClipboardHistoryMenuModelAdapter::AdvancePseudoFocus(bool reverse) {
@@ -202,6 +195,14 @@ void ClipboardHistoryMenuModelAdapter::AdvancePseudoFocus(bool reverse) {
   AdvancePseudoFocusFromSelectedItem(reverse);
 }
 
+ClipboardHistoryUtil::Action
+ClipboardHistoryMenuModelAdapter::GetActionForCommandId(int command_id) const {
+  auto selected_item_iter = item_views_by_command_id_.find(command_id);
+  DCHECK(selected_item_iter != item_views_by_command_id_.cend());
+
+  return selected_item_iter->second->action();
+}
+
 gfx::Rect ClipboardHistoryMenuModelAdapter::GetMenuBoundsInScreenForTest()
     const {
   DCHECK(root_view_);
@@ -267,16 +268,13 @@ void ClipboardHistoryMenuModelAdapter::AdvancePseudoFocusFromSelectedItem(
 }
 
 base::Optional<int>
-ClipboardHistoryMenuModelAdapter::CalculateSelectedCommandIdAfterDeletion()
-    const {
-  base::Optional<int> command_id = GetSelectedMenuItemCommand();
-  DCHECK(command_id.has_value());
-
+ClipboardHistoryMenuModelAdapter::CalculateSelectedCommandIdAfterDeletion(
+    int command_id) const {
   // If the menu item view to be deleted is the last one, Cancel()
   // should be called so this function should not be hit.
   DCHECK_GT(item_snapshots_.size(), 1u);
 
-  auto start_item = item_snapshots_.find(*command_id);
+  auto start_item = item_snapshots_.find(command_id);
   DCHECK(start_item != item_snapshots_.cend());
 
   // Search in the forward direction.
@@ -323,10 +321,6 @@ views::MenuItemView* ClipboardHistoryMenuModelAdapter::AppendMenuItem(
     int index) {
   const int command_id = model->GetCommandIdAt(index);
 
-  // Do not create the view for the deletion command.
-  if (command_id == ClipboardHistoryUtil::kDeleteCommandId)
-    return nullptr;
-
   views::MenuItemView* container = menu->AppendMenuItem(command_id);
 
   // Ignore `container` in accessibility events handling. Let `item_view`

ash/clipboard/clipboard_history_menu_model_adapter.h

@@ -25,6 +25,10 @@ class MenuRunner;
 
 namespace ash {
 
+namespace ClipboardHistoryUtil {
+enum class Action;
+}  // namespace ClipboardHistoryUtil
+
 class ClipboardHistory;
 class ClipboardHistoryItem;
 class ClipboardHistoryItemView;
@@ -71,12 +75,15 @@ class ASH_EXPORT ClipboardHistoryMenuModelAdapter : views::MenuModelAdapter {
   // Selects the menu item specified by `command_id`.
   void SelectMenuItemWithCommandId(int command_id);
 
-  // Removes the selected menu item.
-  void RemoveSelectedMenuItem();
+  // Removes the menu item specified by `command_id`.
+  void RemoveMenuItemWithCommandId(int command_id);
 
   // Advances the pseudo focus (backward if `reverse` is true).
   void AdvancePseudoFocus(bool reverse);
 
+  // Returns the action to take on the menu item specified by `command_id`.
+  ClipboardHistoryUtil::Action GetActionForCommandId(int command_id) const;
+
   // Returns menu bounds in screen coordinates.
   gfx::Rect GetMenuBoundsInScreenForTest() const;
 
@@ -98,9 +105,10 @@ class ASH_EXPORT ClipboardHistoryMenuModelAdapter : views::MenuModelAdapter {
   void AdvancePseudoFocusFromSelectedItem(bool reverse);
 
   // Returns the command id of the menu item to be selected if any after the
-  // current selected item is deleted. If no menu item is selectable
-  // after deletion, an absent value is returned.
-  base::Optional<int> CalculateSelectedCommandIdAfterDeletion() const;
+  // menu item specified by `command_id` is deleted. If no menu item is
+  // selectable after deletion, an absent value is returned.
+  base::Optional<int> CalculateSelectedCommandIdAfterDeletion(
+      int command_id) const;
 
   // Removes the item view specified by `command_id` from the root menu.
   void RemoveItemView(int command_id);

ash/clipboard/clipboard_history_util.h

@@ -19,17 +19,31 @@ class ClipboardHistoryItem;
 
 namespace ClipboardHistoryUtil {
 
-// The command id for deletion.
-constexpr int kDeleteCommandId = 0;
-
 // The first available command id for normal clipboard history menu items.
 constexpr int kFirstItemCommandId = 1;
 
+// The maximum available command id for normal clipboard history menu items.
+constexpr int kMaxItemCommandId = 5;
+
 // The max number of items stored in ClipboardHistory.
-constexpr int kMaxClipboardItemsShared = 5;
+constexpr int kMaxClipboardItemsShared =
+    kMaxItemCommandId - kFirstItemCommandId + 1;
 
 // The max command ID, used to record histograms.
 constexpr int kMaxCommandId = kFirstItemCommandId + kMaxClipboardItemsShared;
+
+// The type of the action to take when the clipboard history menu item is
+// activated.
+enum class Action {
+  kEmpty,
+
+  // Pastes the activated item's corresponding clipboard data.
+  kPaste,
+
+  // Deletes the activated item.
+  kDelete
+};
+
 // Used in histograms, each value corresponds with an underlying format
 // displayed by a ClipboardHistoryItemView. Do not reorder entries, if you must
 // add to it, add at the end.

ash/clipboard/views/clipboard_history_item_view.cc

@@ -12,6 +12,7 @@
 #include "ash/resources/vector_icons/vector_icons.h"
 #include "ash/style/ash_color_provider.h"
 #include "ash/style/scoped_light_mode_as_default.h"
+#include "base/auto_reset.h"
 #include "base/metrics/histogram_macros.h"
 #include "base/strings/utf_string_conversions.h"
 #include "third_party/skia/include/core/SkBitmap.h"
@@ -29,6 +30,7 @@
 #include "ui/views/layout/fill_layout.h"
 
 namespace {
+using Action = ash::ClipboardHistoryUtil::Action;
 
 // The insets within the contents view.
 constexpr gfx::Insets kContentsInsets(/*vertical=*/4, /*horizontal=*/16);
@@ -126,10 +128,11 @@ class ash::ClipboardHistoryItemView::MainButton : public views::Button {
 
 ClipboardHistoryItemView::DeleteButton::DeleteButton(
     ClipboardHistoryItemView* listener)
-    : views::ImageButton(
-          base::BindRepeating(&ClipboardHistoryItemView::ExecuteCommand,
-                              base::Unretained(listener),
-                              ClipboardHistoryUtil::kDeleteCommandId)) {
+    : views::ImageButton(base::BindRepeating(
+          [](ClipboardHistoryItemView* item_view, const ui::Event& event) {
+            item_view->Activate(Action::kDelete, event.flags());
+          },
+          base::Unretained(listener))) {
   SetFocusBehavior(FocusBehavior::ACCESSIBLE_ONLY);
   SetAccessibleName(base::ASCIIToUTF16(std::string(GetClassName())));
   SetImageHorizontalAlignment(views::ImageButton::ALIGN_CENTER);
@@ -203,7 +206,8 @@ void ClipboardHistoryItemView::Init() {
         if (event.type() == ui::ET_GESTURE_TAP)
           item->pseudo_focus_ = PseudoFocus::kMainButton;
 
-        item->ExecuteCommand(item->CalculateCommandId(), event);
+        item->Activate(item->CalculateActionForMainButtonClick(),
+                       event.flags());
       },
       base::Unretained(this)));
 
@@ -263,16 +267,20 @@ bool ClipboardHistoryItemView::AdvancePseudoFocus(bool reverse) {
   return true;
 }
 
-void ClipboardHistoryItemView::RecordButtonPressedHistogram(
-    bool is_delete_button) {
-  if (is_delete_button) {
-    ClipboardHistoryUtil::RecordClipboardHistoryItemDeleted(
-        *clipboard_history_item_);
-    return;
+void ClipboardHistoryItemView::RecordButtonPressedHistogram() const {
+  switch (action_) {
+    case Action::kDelete:
+      ClipboardHistoryUtil::RecordClipboardHistoryItemDeleted(
+          *clipboard_history_item_);
+      return;
+    case Action::kPaste:
+      ClipboardHistoryUtil::RecordClipboardHistoryItemPasted(
+          *clipboard_history_item_);
+      return;
+    case Action::kEmpty:
+      NOTREACHED();
+      return;
   }
-
-  ClipboardHistoryUtil::RecordClipboardHistoryItemPasted(
-      *clipboard_history_item_);
 }
 
 bool ClipboardHistoryItemView::IsItemEnabled() const {
@@ -289,25 +297,36 @@ void ClipboardHistoryItemView::GetAccessibleNodeData(ui::AXNodeData* data) {
   data->SetName(GetAccessibleName());
 }
 
-void ClipboardHistoryItemView::ExecuteCommand(int command_id,
-                                              const ui::Event& event) {
-  RecordButtonPressedHistogram(/*is_delete_button=*/command_id ==
-                               ClipboardHistoryUtil::kDeleteCommandId);
+void ClipboardHistoryItemView::Activate(Action action, int event_flags) {
+  DCHECK(Action::kEmpty == action_ && action_ != action);
+
+  base::AutoReset<Action> action_to_take(&action_, action);
+  RecordButtonPressedHistogram();
+
   views::MenuDelegate* delegate = container_->GetDelegate();
+  const int command_id = container_->GetCommand();
   DCHECK(delegate->IsCommandEnabled(command_id));
-  container_->GetDelegate()->ExecuteCommand(command_id, event.flags());
+  delegate->ExecuteCommand(command_id, event_flags);
 }
 
-int ClipboardHistoryItemView::CalculateCommandId() const {
+Action ClipboardHistoryItemView::CalculateActionForMainButtonClick() const {
+  // `main_button_` may be clicked when the delete button is under the pseudo
+  // focus. It happens when a user presses the ENTER key. Note that the menu
+  // controller sends the accelerator to the hot-tracked view and `main_button_`
+  // is hot-tracked when the delete button is under the pseudo focus. The menu
+  // controller should not hot-track the delete button. Otherwise, pressing the
+  // up/down arrow key will select a delete button instead of a neighboring
+  // menu item.
+
   switch (pseudo_focus_) {
     case PseudoFocus::kMainButton:
-      return container_->GetCommand();
+      return Action::kPaste;
     case PseudoFocus::kDeleteButton:
-      return ClipboardHistoryUtil::kDeleteCommandId;
+      return Action::kDelete;
     case PseudoFocus::kEmpty:
     case PseudoFocus::kMaxValue:
       NOTREACHED();
-      return -1;
+      return Action::kEmpty;
   }
 }
 

ash/clipboard/views/clipboard_history_item_view.h

@@ -5,6 +5,7 @@
 #ifndef ASH_CLIPBOARD_VIEWS_CLIPBOARD_HISTORY_ITEM_VIEW_H_
 #define ASH_CLIPBOARD_VIEWS_CLIPBOARD_HISTORY_ITEM_VIEW_H_
 
+#include "ash/clipboard/clipboard_history_util.h"
 #include "ui/views/controls/button/image_button.h"
 #include "ui/views/view_targeter_delegate.h"
 
@@ -40,6 +41,8 @@ class ClipboardHistoryItemView : public views::View {
   // the view still keeps the pseudo focus.
   bool AdvancePseudoFocus(bool reverse);
 
+  ClipboardHistoryUtil::Action action() const { return action_; }
+
   const views::View* delete_button_for_test() const {
     return contents_view_->delete_button();
   }
@@ -99,7 +102,7 @@ class ClipboardHistoryItemView : public views::View {
                            views::MenuItemView* container);
 
   // Records histograms after the button is pressed.
-  void RecordButtonPressedHistogram(bool is_delete_button);
+  void RecordButtonPressedHistogram() const;
 
   // Creates the contents view.
   virtual std::unique_ptr<ContentsView> CreateContentsView() = 0;
@@ -139,11 +142,11 @@ class ClipboardHistoryItemView : public views::View {
   gfx::Size CalculatePreferredSize() const override;
   void GetAccessibleNodeData(ui::AXNodeData* data) override;
 
-  // Executes |command_id| on the delegate.
-  void ExecuteCommand(int command_id, const ui::Event& event);
+  // Activates the menu item with the specified action and event flags.
+  void Activate(ClipboardHistoryUtil::Action action, int event_flags);
 
-  // Calculates the command id, which indicates the response to user actions.
-  int CalculateCommandId() const;
+  // Calculates the action type when `main_button_` is clicked.
+  ClipboardHistoryUtil::Action CalculateActionForMainButtonClick() const;
 
   // Returns whether the highlight background should show.
   bool ShouldHighlight() const;
@@ -167,6 +170,10 @@ class ClipboardHistoryItemView : public views::View {
 
   PseudoFocus pseudo_focus_ = PseudoFocus::kEmpty;
 
+  // Indicates the action to take. It is set when the menu item is activated
+  // through `main_button_` or the delete button.
+  ClipboardHistoryUtil::Action action_ = ClipboardHistoryUtil::Action::kEmpty;
+
   views::PropertyChangedSubscription subscription_;
 };