Add click-action item to dbus status icon menu

Chrome status icons have a concept of both activation (usually left-clicking the
icon) and opening a menu (usually right-clicking the icon).  However, some
status notifier hosts open a menu on both left and right click.  To make sure
the user will always be able to run the activation action, explicitly add it to
the menu as an additional entry.  The behavior is the same as the
libappindicator codepath in libgtkui.

BUG=419673
R=thestig

Change-Id: Iffa26e0729cc91b7ea9875e6b87fba625a723c2c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1663195Reviewed-by: Lei Zhang <thestig@chromium.org>
Commit-Queue: Thomas Anderson <thomasanderson@chromium.org>
Cr-Commit-Position: refs/heads/master@{#670359}

chrome/browser/ui/BUILD.gn

@@ -2394,6 +2394,8 @@ jumbo_split_static_library("ui") {
 
     if (use_dbus) {
       sources += [
+        "views/status_icons/concat_menu_model.cc",
+        "views/status_icons/concat_menu_model.h",
         "views/status_icons/dbus_menu.cc",
         "views/status_icons/dbus_menu.h",
         "views/status_icons/dbus_properties_interface.cc",

chrome/browser/ui/views/status_icons/concat_menu_model.cc

+// Copyright 2019 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 "chrome/browser/ui/views/status_icons/concat_menu_model.h"
+
+ConcatMenuModel::ConcatMenuModel(ui::MenuModel* m1, ui::MenuModel* m2)
+    : m1_(m1), m2_(m2) {}
+
+ConcatMenuModel::~ConcatMenuModel() = default;
+
+bool ConcatMenuModel::HasIcons() const {
+  return m1_->HasIcons() || m2_->HasIcons();
+}
+
+int ConcatMenuModel::GetItemCount() const {
+  return m1_->GetItemCount() + m2_->GetItemCount();
+}
+
+ui::MenuModel::ItemType ConcatMenuModel::GetTypeAt(int index) const {
+  return GetterImpl(&ui::MenuModel::GetTypeAt, index);
+}
+
+ui::MenuSeparatorType ConcatMenuModel::GetSeparatorTypeAt(int index) const {
+  return GetterImpl(&ui::MenuModel::GetSeparatorTypeAt, index);
+}
+
+int ConcatMenuModel::GetCommandIdAt(int index) const {
+  return GetterImpl(&ui::MenuModel::GetCommandIdAt, index);
+}
+
+base::string16 ConcatMenuModel::GetLabelAt(int index) const {
+  return GetterImpl(&ui::MenuModel::GetLabelAt, index);
+}
+
+base::string16 ConcatMenuModel::GetSublabelAt(int index) const {
+  return GetterImpl(&ui::MenuModel::GetSublabelAt, index);
+}
+
+base::string16 ConcatMenuModel::GetMinorTextAt(int index) const {
+  return GetterImpl(&ui::MenuModel::GetMinorTextAt, index);
+}
+
+const gfx::VectorIcon* ConcatMenuModel::GetMinorIconAt(int index) const {
+  return GetterImpl(&ui::MenuModel::GetMinorIconAt, index);
+}
+
+bool ConcatMenuModel::IsItemDynamicAt(int index) const {
+  return GetterImpl(&ui::MenuModel::IsItemDynamicAt, index);
+}
+
+bool ConcatMenuModel::GetAcceleratorAt(int index,
+                                       ui::Accelerator* accelerator) const {
+  return GetterImpl(&ui::MenuModel::GetAcceleratorAt, index, accelerator);
+}
+
+bool ConcatMenuModel::IsItemCheckedAt(int index) const {
+  return GetterImpl(&ui::MenuModel::IsItemCheckedAt, index);
+}
+
+int ConcatMenuModel::GetGroupIdAt(int index) const {
+  return GetterImpl(&ui::MenuModel::GetGroupIdAt, index);
+}
+
+bool ConcatMenuModel::GetIconAt(int index, gfx::Image* icon) {
+  return GetterImpl(&ui::MenuModel::GetGroupIdAt, index);
+}
+
+ui::ButtonMenuItemModel* ConcatMenuModel::GetButtonMenuItemAt(int index) const {
+  return GetterImpl(&ui::MenuModel::GetButtonMenuItemAt, index);
+}
+
+bool ConcatMenuModel::IsEnabledAt(int index) const {
+  return GetterImpl(&ui::MenuModel::IsEnabledAt, index);
+}
+
+bool ConcatMenuModel::IsVisibleAt(int index) const {
+  return GetterImpl(&ui::MenuModel::IsVisibleAt, index);
+}
+
+void ConcatMenuModel::ActivatedAt(int index) {
+  GetMenuAndIndex(&index)->ActivatedAt(index);
+}
+
+void ConcatMenuModel::ActivatedAt(int index, int event_flags) {
+  GetMenuAndIndex(&index)->ActivatedAt(index, event_flags);
+}
+
+ui::MenuModel* ConcatMenuModel::GetSubmenuModelAt(int index) const {
+  return GetterImpl(&ui::MenuModel::GetSubmenuModelAt, index);
+}
+
+void ConcatMenuModel::MenuWillShow() {
+  m1_->MenuWillShow();
+  m2_->MenuWillShow();
+}
+
+void ConcatMenuModel::MenuWillClose() {
+  m1_->MenuWillClose();
+  m2_->MenuWillClose();
+}
+
+ui::MenuModel* ConcatMenuModel::GetMenuAndIndex(int* index) const {
+  int m1_count = m1_->GetItemCount();
+  if (*index < m1_count)
+    return m1_;
+
+  DCHECK_LT(*index - m1_count, m2_->GetItemCount());
+  *index -= m1_count;
+  return m2_;
+}

chrome/browser/ui/views/status_icons/concat_menu_model.h

+// Copyright 2019 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.
+
+#ifndef CHROME_BROWSER_UI_VIEWS_STATUS_ICONS_CONCAT_MENU_MODEL_H_
+#define CHROME_BROWSER_UI_VIEWS_STATUS_ICONS_CONCAT_MENU_MODEL_H_
+
+#include <utility>
+
+#include "base/macros.h"
+#include "ui/base/models/menu_model.h"
+
+// Combines two menu models (without using submenus).
+class ConcatMenuModel : public ui::MenuModel {
+ public:
+  ConcatMenuModel(ui::MenuModel* m1, ui::MenuModel* m2);
+  ~ConcatMenuModel() override;
+
+  // MenuModel:
+  bool HasIcons() const override;
+  int GetItemCount() const override;
+  ItemType GetTypeAt(int index) const override;
+  ui::MenuSeparatorType GetSeparatorTypeAt(int index) const override;
+  int GetCommandIdAt(int index) const override;
+  base::string16 GetLabelAt(int index) const override;
+  base::string16 GetSublabelAt(int index) const override;
+  base::string16 GetMinorTextAt(int index) const override;
+  const gfx::VectorIcon* GetMinorIconAt(int index) const override;
+  bool IsItemDynamicAt(int index) const override;
+  bool GetAcceleratorAt(int index, ui::Accelerator* accelerator) const override;
+  bool IsItemCheckedAt(int index) const override;
+  int GetGroupIdAt(int index) const override;
+  bool GetIconAt(int index, gfx::Image* icon) override;
+  ui::ButtonMenuItemModel* GetButtonMenuItemAt(int index) const override;
+  bool IsEnabledAt(int index) const override;
+  bool IsVisibleAt(int index) const override;
+  void ActivatedAt(int index) override;
+  void ActivatedAt(int index, int event_flags) override;
+  MenuModel* GetSubmenuModelAt(int index) const override;
+  void MenuWillShow() override;
+  void MenuWillClose() override;
+
+ private:
+  template <typename F, typename... Ts>
+  auto GetterImpl(F&& f, int index, Ts&&... args) const {
+    return (GetMenuAndIndex(&index)->*f)(index, args...);
+  }
+
+  // Returns either |m1_| or |m2_| for the input |index|.  |index| will be
+  // adjusted for the returned menu.
+  ui::MenuModel* GetMenuAndIndex(int* index) const;
+
+  ui::MenuModel* const m1_;
+  ui::MenuModel* const m2_;
+
+  DISALLOW_COPY_AND_ASSIGN(ConcatMenuModel);
+};
+
+#endif  // CHROME_BROWSER_UI_VIEWS_STATUS_ICONS_CONCAT_MENU_MODEL_H_

chrome/browser/ui/views/status_icons/status_icon_linux_dbus.cc

@@ -27,6 +27,9 @@
 #include "dbus/object_path.h"
 #include "dbus/object_proxy.h"
 #include "third_party/skia/include/core/SkBitmap.h"
+#include "ui/base/models/menu_model.h"
+#include "ui/base/models/menu_separator_types.h"
+#include "ui/base/models/simple_menu_model.h"
 #include "ui/gfx/image/image_skia.h"
 #include "ui/views/linux_ui/status_icon_linux.h"
 
@@ -168,6 +171,8 @@ void StatusIconLinuxDbus::SetToolTip(const base::string16& tool_tip) {
   if (!properties_)
     return;
 
+  UpdateMenuImpl(delegate_->GetMenuModel(), true);
+
   properties_->SetProperty(
       kInterfaceStatusNotifierItem, kPropertyToolTip,
       MakeDbusToolTip(base::UTF16ToUTF8(delegate_->GetToolTip())));
@@ -176,8 +181,7 @@ void StatusIconLinuxDbus::SetToolTip(const base::string16& tool_tip) {
 }
 
 void StatusIconLinuxDbus::UpdatePlatformContextMenu(ui::MenuModel* model) {
-  if (menu_)
-    menu_->SetModel(model, true);
+  UpdateMenuImpl(model, true);
 }
 
 void StatusIconLinuxDbus::RefreshPlatformContextMenu() {
@@ -185,7 +189,12 @@ void StatusIconLinuxDbus::RefreshPlatformContextMenu() {
   // icons, but also for layout changes like deleted items.
   // TODO(thomasanderson): Split this into two methods so we can avoid
   // rebuilding the menu for simple property changes.
-  UpdatePlatformContextMenu(delegate_->GetMenuModel());
+  UpdateMenuImpl(delegate_->GetMenuModel(), true);
+}
+
+void StatusIconLinuxDbus::ExecuteCommand(int command_id, int event_flags) {
+  DCHECK_EQ(command_id, 0);
+  delegate_->OnClick();
 }
 
 void StatusIconLinuxDbus::OnHostRegisteredResponse(dbus::Response* response) {
@@ -245,7 +254,7 @@ void StatusIconLinuxDbus::OnOwnership(const std::string& service_name,
 
   menu_ = std::make_unique<DbusMenu>(
       bus_->GetExportedObject(dbus::ObjectPath(kPathDbusMenu)), barrier_);
-  menu_->SetModel(delegate_->GetMenuModel(), false);
+  UpdateMenuImpl(delegate_->GetMenuModel(), false);
 
   properties_ = std::make_unique<DbusPropertiesInterface>(item_, barrier_);
   properties_->RegisterInterface(kInterfaceStatusNotifierItem);
@@ -326,3 +335,25 @@ void StatusIconLinuxDbus::OnSecondaryActivate(
   // to run the same handler as regular activations.
   sender.Run(dbus::Response::FromMethodCall(method_call));
 }
+
+void StatusIconLinuxDbus::UpdateMenuImpl(ui::MenuModel* model,
+                                         bool send_signal) {
+  if (!menu_)
+    return;
+
+  if (!model) {
+    empty_menu_ = std::make_unique<ui::SimpleMenuModel>(nullptr);
+    model = empty_menu_.get();
+  }
+
+  click_action_menu_ = std::make_unique<ui::SimpleMenuModel>(this);
+  if (delegate_->HasClickAction() && !delegate_->GetToolTip().empty()) {
+    click_action_menu_->AddItem(0, delegate_->GetToolTip());
+    if (model->GetItemCount())
+      click_action_menu_->AddSeparator(ui::MenuSeparatorType::NORMAL_SEPARATOR);
+  }
+
+  concat_menu_ =
+      std::make_unique<ConcatMenuModel>(click_action_menu_.get(), model);
+  menu_->SetModel(concat_menu_.get(), send_signal);
+}

chrome/browser/ui/views/status_icons/status_icon_linux_dbus.h

@@ -10,10 +10,12 @@
 #include "base/callback_forward.h"
 #include "base/macros.h"
 #include "base/memory/weak_ptr.h"
+#include "chrome/browser/ui/views/status_icons/concat_menu_model.h"
 #include "dbus/bus.h"
 #include "dbus/exported_object.h"
 #include "dbus/message.h"
 #include "dbus/object_proxy.h"
+#include "ui/base/models/simple_menu_model.h"
 #include "ui/views/linux_ui/status_icon_linux.h"
 
 namespace gfx {
@@ -25,7 +27,8 @@ class DbusPropertiesInterface;
 
 // A status icon following the StatusNotifierItem specification.
 // https://www.freedesktop.org/wiki/Specifications/StatusNotifierItem/StatusNotifierItem/
-class StatusIconLinuxDbus : public views::StatusIconLinux {
+class StatusIconLinuxDbus : public views::StatusIconLinux,
+                            public ui::SimpleMenuModel::Delegate {
  public:
   StatusIconLinuxDbus();
   ~StatusIconLinuxDbus() override;
@@ -36,6 +39,9 @@ class StatusIconLinuxDbus : public views::StatusIconLinux {
   void UpdatePlatformContextMenu(ui::MenuModel* model) override;
   void RefreshPlatformContextMenu() override;
 
+  // ui::SimpleMenuModel::Delegate:
+  void ExecuteCommand(int command_id, int event_flags) override;
+
  private:
   // Step 1: verify with the StatusNotifierWatcher that a StatusNotifierHost is
   // registered.
@@ -69,6 +75,8 @@ class StatusIconLinuxDbus : public views::StatusIconLinux {
   void OnSecondaryActivate(dbus::MethodCall* method_call,
                            dbus::ExportedObject::ResponseSender sender);
 
+  void UpdateMenuImpl(ui::MenuModel* model, bool send_signal);
+
   scoped_refptr<dbus::Bus> bus_;
 
   int service_id_ = 0;
@@ -80,6 +88,17 @@ class StatusIconLinuxDbus : public views::StatusIconLinux {
   std::unique_ptr<DbusPropertiesInterface> properties_;
 
   std::unique_ptr<DbusMenu> menu_;
+  // A menu that contains the click action (if there is a click action) and a
+  // separator (if there's a click action and delegate_->GetMenuModel() is
+  // non-empty).
+  std::unique_ptr<ui::SimpleMenuModel> click_action_menu_;
+  // An empty menu for use in |concat_menu_| if delegate_->GetMenuModel() is
+  // null.
+  std::unique_ptr<ui::SimpleMenuModel> empty_menu_;
+  // A concatenation of |click_action_menu_| and either
+  // delegate_->GetMenuModel() or |empty_menu_| if the delegate's menu is null.
+  // Appears after the other menus so that it gets destroyed first.
+  std::unique_ptr<ConcatMenuModel> concat_menu_;
 
   base::WeakPtrFactory<StatusIconLinuxDbus> weak_factory_;