Add ability to automatically refresh a menu created using MenuRunner's MenuModel-based constructor.

One way to create a MenuRunner is through its MenuModel-based constructor: https://cs.chromium.org/chromium/src/ui/views/controls/menu/menu_runner.h?l=109&rcl=75b927b4f2d4193d77c1721d8415aadac33f5b4e

This uses the BuildMenu method in MenuModelAdapter to create the MenuItemView based on the MenuModel.
However, if the MenuModel later changes, the MenuItemView doesn't get updated accordingly, since it was only built once on construction.

This CL makes MenuRunnerImplAdapter the delegate of its MenuModel, and when it sees a change, it makes it rebuild the menu.

Bug: 923660
Change-Id: I9df794f87cef268fa6fbba2c47fbe29a745ae566
Reviewed-on: https://chromium-review.googlesource.com/c/1452356
Commit-Queue: Edin Kadric <edinkadric@google.com>
Reviewed-by: Scott Violet <sky@chromium.org>
Cr-Commit-Position: refs/heads/master@{#630635}

chrome/browser/ui/toolbar/back_forward_menu_model.cc

@@ -216,15 +216,6 @@ bool BackForwardMenuModel::IsSeparator(int index) const {
   return index == history_items;
 }
 
-void BackForwardMenuModel::SetMenuModelDelegate(
-      ui::MenuModelDelegate* menu_model_delegate) {
-  menu_model_delegate_ = menu_model_delegate;
-}
-
-ui::MenuModelDelegate* BackForwardMenuModel::GetMenuModelDelegate() const {
-  return menu_model_delegate_;
-}
-
 void BackForwardMenuModel::FetchFavicon(NavigationEntry* entry) {
   // If the favicon has already been requested for this menu, don't do
   // anything.

chrome/browser/ui/toolbar/back_forward_menu_model.h

@@ -73,14 +73,6 @@ class BackForwardMenuModel : public ui::MenuModel {
   // Is the item at |index| a separator?
   bool IsSeparator(int index) const;
 
-  // Set the delegate for triggering OnIconChanged.
-  void SetMenuModelDelegate(
-      ui::MenuModelDelegate* menu_model_delegate) override;
-  ui::MenuModelDelegate* GetMenuModelDelegate() const override;
-
- protected:
-  ui::MenuModelDelegate* menu_model_delegate() { return menu_model_delegate_; }
-
  private:
   friend class BackFwdMenuModelTest;
   FRIEND_TEST_ALL_PREFIXES(BackFwdMenuModelTest, BasicCase);
@@ -200,9 +192,6 @@ class BackForwardMenuModel : public ui::MenuModel {
   // Used for loading favicons.
   base::CancelableTaskTracker cancelable_task_tracker_;
 
-  // Used for receiving notifications when an icon is changed.
-  ui::MenuModelDelegate* menu_model_delegate_ = nullptr;
-
   DISALLOW_COPY_AND_ASSIGN(BackForwardMenuModel);
 };
 

chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.cc

@@ -605,9 +605,9 @@ void RecentTabsSubMenuModel::OnFaviconDataAvailable(
   int index_in_menu = GetIndexOfCommandId(command_id);
   DCHECK_GT(index_in_menu, -1);
   SetIcon(index_in_menu, image_result.image);
-  ui::MenuModelDelegate* menu_model_delegate = GetMenuModelDelegate();
-  if (menu_model_delegate)
-    menu_model_delegate->OnIconChanged(index_in_menu);
+  ui::MenuModelDelegate* delegate = menu_model_delegate();
+  if (delegate)
+    delegate->OnIconChanged(index_in_menu);
 }
 
 int RecentTabsSubMenuModel::CommandIdToTabVectorIndex(
@@ -662,9 +662,9 @@ void RecentTabsSubMenuModel::TabRestoreServiceChanged(
 
   BuildLocalEntries();
 
-  ui::MenuModelDelegate* menu_model_delegate = GetMenuModelDelegate();
-  if (menu_model_delegate)
-    menu_model_delegate->OnMenuStructureChanged();
+  ui::MenuModelDelegate* delegate = menu_model_delegate();
+  if (delegate)
+    delegate->OnMenuStructureChanged();
 }
 
 void RecentTabsSubMenuModel::TabRestoreServiceDestroyed(
@@ -677,7 +677,7 @@ void RecentTabsSubMenuModel::OnForeignSessionUpdated() {
 
   BuildTabsFromOtherDevices();
 
-  ui::MenuModelDelegate* menu_model_delegate = GetMenuModelDelegate();
-  if (menu_model_delegate)
-    menu_model_delegate->OnMenuStructureChanged();
+  ui::MenuModelDelegate* delegate = menu_model_delegate();
+  if (delegate)
+    delegate->OnMenuStructureChanged();
 }

chrome/browser/ui/toolbar/recent_tabs_sub_menu_model_unittest.cc

@@ -91,7 +91,7 @@ class TestRecentTabsMenuModelDelegate : public ui::MenuModelDelegate {
   }
 
   ~TestRecentTabsMenuModelDelegate() override {
-    model_->SetMenuModelDelegate(NULL);
+    model_->SetMenuModelDelegate(nullptr);
   }
 
   // ui::MenuModelDelegate implementation:

chrome/browser/ui/views/menu_model_adapter_test.cc

@@ -75,12 +75,6 @@ class CommonMenuModel : public ui::MenuModel {
 
   void ActivatedAt(int index) override {}
 
-  void SetMenuModelDelegate(ui::MenuModelDelegate* delegate) override {}
-
-  ui::MenuModelDelegate* GetMenuModelDelegate() const override {
-    return nullptr;
-  }
-
  private:
   DISALLOW_COPY_AND_ASSIGN(CommonMenuModel);
 };

chrome/browser/ui/views/toolbar/toolbar_button_unittest.cc

@@ -52,10 +52,6 @@ class CheckActiveWebContentsMenuModel : public ui::MenuModel {
   bool IsEnabledAt(int index) const override { return false; }
   ui::MenuModel* GetSubmenuModelAt(int index) const override { return nullptr; }
   void ActivatedAt(int index) override {}
-  void SetMenuModelDelegate(ui::MenuModelDelegate* delegate) override {}
-  ui::MenuModelDelegate* GetMenuModelDelegate() const override {
-    return nullptr;
-  }
 
  private:
   TabStripModel* const tab_strip_model_;

ui/base/models/menu_model.cc

@@ -6,6 +6,13 @@
 
 namespace ui {
 
+MenuModel::MenuModel() : menu_model_delegate_(nullptr) {}
+
+MenuModel::~MenuModel() {
+  if (menu_model_delegate_)
+    menu_model_delegate_->OnMenuClearingDelegate();
+}
+
 bool MenuModel::IsVisibleAt(int index) const {
   return true;
 }
@@ -60,4 +67,12 @@ void MenuModel::ActivatedAt(int index, int event_flags) {
   ActivatedAt(index);
 }
 
+void MenuModel::SetMenuModelDelegate(MenuModelDelegate* delegate) {
+  // A non-null delegate overwriting our non-null delegate is not allowed.
+  DCHECK(!(menu_model_delegate_ && delegate));
+  if (menu_model_delegate_)
+    menu_model_delegate_->OnMenuClearingDelegate();
+  menu_model_delegate_ = delegate;
+}
+
 }  // namespace ui

ui/base/models/menu_model.h

@@ -40,7 +40,9 @@ class UI_BASE_EXPORT MenuModel {
                        // background matches the menu's rounded corners.
   };
 
-  virtual ~MenuModel() {}
+  MenuModel();
+
+  virtual ~MenuModel();
 
   // Returns true if any of the items within the model have icons. Not all
   // platforms support icons in menus natively and so this is a hint for
@@ -127,11 +129,12 @@ class UI_BASE_EXPORT MenuModel {
   // should not be deleted here.
   virtual void MenuWillClose() {}
 
-  // Set the MenuModelDelegate. Owned by the caller of this function.
-  virtual void SetMenuModelDelegate(MenuModelDelegate* delegate) = 0;
+  // Set the MenuModelDelegate, owned by the caller of this function. We allow
+  // setting a new one or clearing the current one.
+  void SetMenuModelDelegate(MenuModelDelegate* delegate);
 
   // Gets the MenuModelDelegate.
-  virtual MenuModelDelegate* GetMenuModelDelegate() const = 0;
+  MenuModelDelegate* menu_model_delegate() { return menu_model_delegate_; }
 
   // Retrieves the model and index that contains a specific command id. Returns
   // true if an item with the specified command id is found. |model| is inout,
@@ -139,6 +142,10 @@ class UI_BASE_EXPORT MenuModel {
   static bool GetModelAndIndexForCommandId(int command_id,
                                            MenuModel** model,
                                            int* index);
+
+ private:
+  // MenuModelDelegate. Weak. Could be null.
+  MenuModelDelegate* menu_model_delegate_;
 };
 
 }  // namespace ui

ui/base/models/menu_model_delegate.h

@@ -16,6 +16,10 @@ class MenuModelDelegate {
   // delegate should assume the entire contents of the model has changed.
   virtual void OnMenuStructureChanged() {}
 
+  // Invoked when |MenuModel| is clearing its current delegate field. This
+  // indicates to |this| that it is not that MenuModel's delegate anymore.
+  virtual void OnMenuClearingDelegate() {}
+
  protected:
   virtual ~MenuModelDelegate() {}
 };

ui/base/models/simple_menu_model.cc

@@ -66,7 +66,6 @@ bool SimpleMenuModel::Delegate::GetAcceleratorForCommandId(
 
 SimpleMenuModel::SimpleMenuModel(Delegate* delegate)
     : delegate_(delegate),
-      menu_model_delegate_(nullptr),
       method_factory_(this) {}
 
 SimpleMenuModel::~SimpleMenuModel() {
@@ -450,15 +449,6 @@ void SimpleMenuModel::MenuWillClose() {
                                 method_factory_.GetWeakPtr()));
 }
 
-void SimpleMenuModel::SetMenuModelDelegate(
-      ui::MenuModelDelegate* menu_model_delegate) {
-  menu_model_delegate_ = menu_model_delegate;
-}
-
-MenuModelDelegate* SimpleMenuModel::GetMenuModelDelegate() const {
-  return menu_model_delegate_;
-}
-
 void SimpleMenuModel::OnMenuClosed() {
   if (delegate_)
     delegate_->MenuClosed(this);

ui/base/models/simple_menu_model.h

@@ -186,9 +186,6 @@ class UI_BASE_EXPORT SimpleMenuModel : public MenuModel {
   MenuModel* GetSubmenuModelAt(int index) const override;
   void MenuWillShow() override;
   void MenuWillClose() override;
-  void SetMenuModelDelegate(
-      ui::MenuModelDelegate* menu_model_delegate) override;
-  MenuModelDelegate* GetMenuModelDelegate() const override;
 
   // Sets |histogram_name_|.
   void set_histogram_name(const std::string& histogram_name) {
@@ -245,8 +242,6 @@ class UI_BASE_EXPORT SimpleMenuModel : public MenuModel {
 
   Delegate* delegate_;
 
-  MenuModelDelegate* menu_model_delegate_;
-
   // The UMA histogram name that is be used to log command ids.
   std::string histogram_name_;
 

ui/views/controls/combobox/combobox.cc

@@ -197,13 +197,6 @@ class Combobox::ComboboxMenuModel : public ui::MenuModel,
 
   MenuModel* GetSubmenuModelAt(int index) const override { return nullptr; }
 
-  void SetMenuModelDelegate(
-      ui::MenuModelDelegate* menu_model_delegate) override {}
-
-  ui::MenuModelDelegate* GetMenuModelDelegate() const override {
-    return nullptr;
-  }
-
   // Overridden from ComboboxModelObserver:
   void OnComboboxModelChanged(ui::ComboboxModel* model) override {
     owner_->ModelChanged();

ui/views/controls/menu/menu_model_adapter.cc

@@ -22,9 +22,13 @@ MenuModelAdapter::MenuModelAdapter(ui::MenuModel* menu_model,
                                ui::EF_RIGHT_MOUSE_BUTTON),
       on_menu_closed_callback_(on_menu_closed_callback) {
   DCHECK(menu_model);
+  menu_model_->SetMenuModelDelegate(nullptr);
+  menu_model_->SetMenuModelDelegate(this);
 }
 
 MenuModelAdapter::~MenuModelAdapter() {
+  if (menu_model_)
+    menu_model_->SetMenuModelDelegate(nullptr);
 }
 
 void MenuModelAdapter::BuildMenu(MenuItemView* menu) {
@@ -50,9 +54,9 @@ void MenuModelAdapter::BuildMenu(MenuItemView* menu) {
 }
 
 MenuItemView* MenuModelAdapter::CreateMenu() {
-  MenuItemView* item = new MenuItemView(this);
-  BuildMenu(item);
-  return item;
+  menu_ = new MenuItemView(this);
+  BuildMenu(menu_);
+  return menu_;
 }
 
 // Static.
@@ -249,6 +253,16 @@ void MenuModelAdapter::OnMenuClosed(MenuItemView* menu) {
     on_menu_closed_callback_.Run();
 }
 
+// MenuModelDelegate overrides:
+void MenuModelAdapter::OnMenuStructureChanged() {
+  if (menu_)
+    BuildMenu(menu_);
+}
+
+void MenuModelAdapter::OnMenuClearingDelegate() {
+  menu_model_ = nullptr;
+}
+
 // MenuModelAdapter, private:
 
 void MenuModelAdapter::BuildMenuImpl(MenuItemView* menu, ui::MenuModel* model) {

ui/views/controls/menu/menu_model_adapter.h

@@ -9,6 +9,7 @@
 
 #include "base/callback.h"
 #include "base/macros.h"
+#include "ui/base/models/menu_model_delegate.h"
 #include "ui/views/controls/menu/menu_delegate.h"
 
 namespace ui {
@@ -18,12 +19,15 @@ class MenuModel;
 namespace views {
 class MenuItemView;
 
-// This class wraps an instance of ui::MenuModel with the
-// views::MenuDelegate interface required by views::MenuItemView.
-class VIEWS_EXPORT MenuModelAdapter : public MenuDelegate {
+// This class wraps an instance of ui::MenuModel with the views::MenuDelegate
+// interface required by views::MenuItemView.
+class VIEWS_EXPORT MenuModelAdapter : public MenuDelegate,
+                                      public ui::MenuModelDelegate {
  public:
-  // The caller retains ownership of the ui::MenuModel instance and
-  // must ensure it exists for the lifetime of the adapter.
+  // The caller retains ownership of the ui::MenuModel instance and must ensure
+  // it exists for the lifetime of the adapter. |this| will become the new
+  // MenuModelDelegate of |menu_model| so that subsequent changes to it get
+  // reflected in the created MenuItemView.
   explicit MenuModelAdapter(ui::MenuModel* menu_model);
   MenuModelAdapter(ui::MenuModel* menu_model,
                    const base::Closure& on_menu_closed_callback);
@@ -57,6 +61,11 @@ class VIEWS_EXPORT MenuModelAdapter : public MenuDelegate {
                                                MenuItemView* menu,
                                                int item_id);
 
+  // MenuModelDelegate:
+  void OnIconChanged(int index) override{};
+  void OnMenuStructureChanged() override;
+  void OnMenuClearingDelegate() override;
+
  protected:
   // Create and add a menu item to |menu| for the item at index |index| in
   // |model|. Subclasses override this to allow custom items to be added to the
@@ -88,6 +97,10 @@ class VIEWS_EXPORT MenuModelAdapter : public MenuDelegate {
   // passed to the constructor.
   ui::MenuModel* menu_model_;
 
+  // Pointer to the MenuItemView created and updated by |this|, but not owned by
+  // |this|.
+  MenuItemView* menu_;
+
   // Mouse event flags which can trigger menu actions.
   int triggerable_event_flags_;
 

ui/views/controls/menu/menu_model_adapter_unittest.cc

@@ -90,10 +90,6 @@ class MenuModelBase : public ui::MenuModel {
 
   void MenuWillClose() override {}
 
-  void SetMenuModelDelegate(ui::MenuModelDelegate* delegate) override {}
-
-  ui::MenuModelDelegate* GetMenuModelDelegate() const override { return NULL; }
-
   // Item definition.
   struct Item {
     Item(ItemType item_type,

ui/views/controls/menu/menu_runner.h

@@ -106,6 +106,7 @@ class VIEWS_EXPORT MenuRunner {
   // |on_menu_closed_callback| is invoked when the menu is closed.
   // Note that with a native menu (e.g. on Mac), the ASYNC flag in |run_types|
   // may be ignored. See http://crbug.com/682544.
+  // The MenuModelDelegate of |menu_model| will be overwritten by this call.
   MenuRunner(ui::MenuModel* menu_model,
              int32_t run_types,
              const base::Closure& on_menu_closed_callback = base::Closure());

ui/views/controls/menu/menu_runner_impl_adapter.cc

@@ -42,8 +42,7 @@ base::TimeTicks MenuRunnerImplAdapter::GetClosingEventTime() const {
   return impl_->GetClosingEventTime();
 }
 
-MenuRunnerImplAdapter::~MenuRunnerImplAdapter() {
-}
+MenuRunnerImplAdapter::~MenuRunnerImplAdapter() = default;
 
 }  // namespace internal
 }  // namespace views