Do popup modernization without switching to the new API

https://crrev.com/c/6173642 switched to new API for doing popup menus,
but it also did some associated cleanup. While the new API was buggy
(see https://crbug.com/401443090) and therefore had to be reverted, the
cleanup aspects are still a good idea, so re-land them.

Specifically:
- Switching WebMenuRunner to have explicit testing callbacks rather
  than requiring other code to swizzle NSView with the knowledge of
  the WebMenuRunner internals.
- Removal of the "useWithPopUpButtonCell" parameter on
  MenuControllerCocoa, which was not actually used for
  PopUpButtonCells! (The WebMenuRunner builds its own menus.)
- Removal of the default -init on MenuControllerCocoa due to having no
  callers; now MenuControllerCocoa always eagerly builds the menu,
  simplifying things.
- Light code modernization.

Bug: 389067059
Include-Ci-Only-Tests: chromium.mac:mac14-tests|browser_tests
Change-Id: If62fd54001c9ad2c50e6bd96a97c894b00a7bc61
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6351721
Commit-Queue: Avi Drissman <avi@chromium.org>
Reviewed-by: Keren Zhu <kerenzhu@chromium.org>
Reviewed-by: Leonard Grey <lgrey@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1433321}

chrome/browser/apps/guest_view/web_view_interactive_browsertest.cc

@@ -85,20 +85,16 @@ using guest_view::TestGuestViewManagerFactory;
 #endif
 
 #if BUILDFLAG(IS_MAC)
-// This class observes the RenderWidgetHostViewCocoa corresponding to the outer
-// most WebContents provided for newly added subviews. The added subview
-// corresponds to a NSPopUpButtonCell which will be removed shortly after being
-// shown.
-class NewSubViewAddedObserver : content::RenderWidgetHostViewCocoaObserver {
+class PopupShowAttemptObserver : content::RenderWidgetHostViewCocoaObserver {
  public:
-  explicit NewSubViewAddedObserver(content::WebContents* web_contents)
+  explicit PopupShowAttemptObserver(content::WebContents* web_contents)
       : content::RenderWidgetHostViewCocoaObserver(web_contents) {}
 
-  NewSubViewAddedObserver(const NewSubViewAddedObserver&) = delete;
-  NewSubViewAddedObserver& operator=(const NewSubViewAddedObserver&) = delete;
-  ~NewSubViewAddedObserver() override = default;
+  PopupShowAttemptObserver(const PopupShowAttemptObserver&) = delete;
+  PopupShowAttemptObserver& operator=(const PopupShowAttemptObserver&) = delete;
+  ~PopupShowAttemptObserver() override = default;
 
-  void WaitForNextSubView() {
+  void WaitForPopup() {
     if (did_receive_rect_)
       return;
 
@@ -109,12 +105,13 @@ class NewSubViewAddedObserver : content::RenderWidgetHostViewCocoaObserver {
   const gfx::Rect& view_bounds_in_screen() const { return bounds_; }
 
  private:
-  void DidAddSubviewWillBeDismissed(
-      const gfx::Rect& bounds_in_root_view) override {
+  void DidAttemptToShowPopup(const gfx::Rect& bounds,
+                             int selected_item) override {
     did_receive_rect_ = true;
-    bounds_ = bounds_in_root_view;
-    if (run_loop_)
+    bounds_ = bounds;
+    if (run_loop_) {
       run_loop_->Quit();
+    }
   }
 
   bool did_receive_rect_ = false;
@@ -126,11 +123,7 @@ class NewSubViewAddedObserver : content::RenderWidgetHostViewCocoaObserver {
 class WebViewInteractiveTest : public extensions::PlatformAppBrowserTest {
  public:
   WebViewInteractiveTest()
-      : guest_view_(nullptr),
-        embedder_web_contents_(nullptr),
-        corner_(gfx::Point()),
-        mouse_click_result_(false),
-        first_click_(true) {}
+      : guest_view_(nullptr), embedder_web_contents_(nullptr) {}
 
   void SetUpCommandLine(base::CommandLine* command_line) override {
     extensions::PlatformAppBrowserTest::SetUpCommandLine(command_line);
@@ -491,8 +484,8 @@ class WebViewInteractiveTest : public extensions::PlatformAppBrowserTest {
       embedder_web_contents_;
 
   gfx::Point corner_;
-  bool mouse_click_result_;
-  bool first_click_;
+  bool mouse_click_result_ = false;
+  bool first_click_ = true;
 };
 
 class WebViewImeInteractiveTest : public WebViewInteractiveTest {
@@ -1586,7 +1579,7 @@ IN_PROC_BROWSER_TEST_F(WebViewImeInteractiveTest, CompositionRangeUpdates) {
 // This test verifies that drop-down lists appear correctly inside OOPIF-based
 // webviews which have offset inside embedder. This is a test for all guest
 // views as the logic for showing such popups is inside content/ layer. For more
-// context see https://crbug.com/772840.
+// context see https://crbug.com/41348804.
 IN_PROC_BROWSER_TEST_F(WebViewFocusInteractiveTest,
                        DropDownPopupInCorrectPosition) {
   TestHelper("testSelectPopupPositionInMac", "web_view/shim", NO_TEST_SERVER);
@@ -1603,19 +1596,19 @@ IN_PROC_BROWSER_TEST_F(WebViewFocusInteractiveTest,
                .Length() >= distance_from_root_view_origin;
   }));
 
-  // Now trigger the popup and wait until it is displayed. The popup will get
-  // dismissed after being shown.
-  NewSubViewAddedObserver popup_observer(embedder_web_contents_);
+  // Now trigger the popup and wait for the callback that indicates that it
+  // would have been displayed.
+  PopupShowAttemptObserver popup_observer(embedder_web_contents_);
   // Now send a mouse click and wait until the <select> tag is focused.
   SimulateRWHMouseClick(guest_rwhv->GetRenderWidgetHost(),
                         blink::WebMouseEvent::Button::kLeft, 5, 5);
-  popup_observer.WaitForNextSubView();
+  popup_observer.WaitForPopup();
 
-  // Verify the popup bounds intersect with those of the guest. Since the popup
-  // is relatively small (the width is determined by the <select> element's
-  // width and the hight is a factor of font-size and number of items), the
-  // intersection alone is a good indication the popup is shown properly inside
-  // the screen.
+  // Verify that the popup bounds (where it would have been displayed) intersect
+  // with the bounds of the guest. Since the popup is relatively small (the
+  // width is determined by the <select> element's width and the height is a
+  // factor of font-size and number of items), the intersection alone is a good
+  // indication the popup is shown properly inside the screen.
   gfx::Rect guest_bounds_in_embedder(
       guest_rwhv->TransformPointToRootCoordSpace(gfx::Point()),
       guest_rwhv->GetViewBounds().size());

chrome/browser/ui/cocoa/renderer_context_menu/render_view_context_menu_mac_cocoa.mm

@@ -81,8 +81,7 @@ void RenderViewContextMenuMacCocoa::Show() {
       initWithParams:MenuControllerParamsForWidget(widget)];
   menu_controller_ =
       [[MenuControllerCocoa alloc] initWithModel:&menu_model_
-                                        delegate:menu_controller_delegate_
-                          useWithPopUpButtonCell:NO];
+                                        delegate:menu_controller_delegate_];
 
   NSPoint position =
       NSMakePoint(params_.x, NSHeight(parent_view_.bounds) - params_.y);

chrome/browser/ui/cocoa/status_icons/status_icon_mac.mm

@@ -228,8 +228,6 @@ void StatusIconMac::UpdatePlatformContextMenu(StatusIconMenuModel* model) {
 
 void StatusIconMac::CreateMenu(ui::MenuModel* model) {
   DCHECK(model);
-  menu_ = [[MenuControllerCocoa alloc] initWithModel:model
-                                            delegate:nil
-                              useWithPopUpButtonCell:NO];
+  menu_ = [[MenuControllerCocoa alloc] initWithModel:model delegate:nil];
   item().menu = menu_.menu;
 }

components/remote_cocoa/app_shim/context_menu_runner.mm

@@ -47,7 +47,7 @@ ContextMenuRunner::ContextMenuRunner(
 
 ContextMenuRunner::~ContextMenuRunner() {
   if (menu_controller_) {
-    CHECK(!menu_controller_.isMenuOpen);
+    CHECK(!menu_controller_.menuOpen);
   }
 }
 
@@ -60,8 +60,7 @@ void ContextMenuRunner::ShowMenu(mojom::ContextMenuPtr menu,
       initWithParams:std::move(menu->params)];
   menu_controller_ =
       [[MenuControllerCocoa alloc] initWithModel:menu_model_.get()
-                                        delegate:menu_delegate_
-                          useWithPopUpButtonCell:NO];
+                                        delegate:menu_delegate_];
 
   if (!target_view) {
     target_view = window.contentView;

content/app_shim_remote_cocoa/render_widget_host_ns_view_bridge.mm

@@ -397,7 +397,7 @@ void RenderWidgetHostNSViewBridge::DisplayPopupMenu(
     // menu to finish showing to get the nested run loop of the stack.
     // Attempting to show a new menu while the old menu is still visible or
     // fading out confuses AppKit, since we're still in the nested event loop of
-    // DisplayPopupMenu(). See https://crbug.com/812260.
+    // DisplayPopupMenu(). See https://crbug.com/41370640.
     pending_menus_.emplace_back(std::move(menu), std::move(callback));
     return;
   }
@@ -413,9 +413,9 @@ void RenderWidgetHostNSViewBridge::DisplayPopupMenu(
   }
 
   // Retain the Cocoa view for the duration of the pop-up so that it can't be
-  // dealloced if the widget is destroyed while the pop-up's up (which
-  // would in turn delete me, causing a crash once the -runMenuInView
-  // call returns. That's what was happening in <http://crbug.com/33250>).
+  // dealloced if the widget is destroyed while the pop-up's up (which would in
+  // turn delete me, causing a crash once the -runMenuInView call returns.
+  // That's what was happening in <https://crbug.com/40346793>).
   RenderWidgetHostViewCocoa* cocoa_view = cocoa_view_;
 
   // Get a weak pointer to `this`, so we can detect if we get destroyed while
@@ -466,16 +466,7 @@ void RenderWidgetHostNSViewBridge::DisplayPopupMenu(
     return;
   }
 
-  if (runner.menuItemWasChosen) {
-    int index = runner.indexOfSelectedItem;
-    if (index < 0) {
-      std::move(callback).Run(std::nullopt);
-    } else {
-      std::move(callback).Run(index);
-    }
-  } else {
-    std::move(callback).Run(std::nullopt);
-  }
+  std::move(callback).Run(runner.selectedMenuItemIndex);
 
   std::vector<PendingPopupMenu> next_menus = std::exchange(pending_menus_, {});
   if (!next_menus.empty()) {

content/app_shim_remote_cocoa/render_widget_host_view_cocoa.h

@@ -13,6 +13,7 @@
 
 #include "base/containers/flat_set.h"
 #include "content/browser/renderer_host/input/mouse_wheel_rails_filter_mac.h"
+#include "content/common/content_export.h"
 #include "content/common/render_widget_host_ns_view.mojom.h"
 #include "mojo/public/cpp/bindings/remote.h"
 #include "third_party/blink/public/mojom/input/input_handler.mojom-shared.h"
@@ -49,6 +50,7 @@ struct DidOverscrollParams;
 // but that means that the view needs to own the delegate and will dispose of it
 // when it's removed from the view system.
 // TODO(ccameron): Hide this interface behind RenderWidgetHostNSViewBridge.
+CONTENT_EXPORT
 @interface RenderWidgetHostViewCocoa
     : ToolTipBaseView <CommandDispatcherTarget,
                        RenderWidgetHostNSViewHostOwner,

content/app_shim_remote_cocoa/web_menu_runner_mac.h

@@ -7,26 +7,27 @@
 
 #import <Cocoa/Cocoa.h>
 
+#include <optional>
 #include <vector>
 
+#include "base/functional/callback_forward.h"
+#include "content/common/content_export.h"
 #include "third_party/blink/public/mojom/choosers/popup_menu.mojom.h"
 
 // WebMenuRunner ---------------------------------------------------------------
-// A class for determining whether an item was selected from an HTML select
-// control, or if the menu was dismissed without making a selection. If a menu
-// item is selected, MenuDelegate is informed and sets a flag which can be
-// queried after the menu has finished running.
-
+// A class for presenting a menu when a HTML select element is clicked, and
+// returning the user selection or dismissal without selection.
+CONTENT_EXPORT
 @interface WebMenuRunner : NSObject
 
-// Initializes the MenuDelegate with a list of items sent from WebKit.
 - (id)initWithItems:(const std::vector<blink::mojom::MenuItemPtr>&)items
            fontSize:(CGFloat)fontSize
        rightAligned:(BOOL)rightAligned;
 
-// Returns YES if an item was selected from the menu, NO if the menu was
-// dismissed.
-- (BOOL)menuItemWasChosen;
+// If the menu has been run, and an item was selected, has the index of the
+// selected menu item. If the menu has not yet been run, or if the menu was run
+// but the user did not select an item from the menu, this is nullopt.
+@property(readonly) std::optional<int> selectedMenuItemIndex;
 
 // Displays and runs a native popup menu.
 - (void)runMenuInView:(NSView*)view
@@ -38,13 +39,22 @@
 // contents of the menu changed so the menu has to be rebuilt). Because this is
 // driven by Blink, and in some cases Blink will immediately re-issue the menu,
 // this is a synchronous cancellation with no animation. See
-// https://crbug.com/812260.
+// https://crbug.com/41370640.
 - (void)cancelSynchronously;
 
-// Returns the index of selected menu item, or its initial value (-1) if no item
-// was selected.
-- (int)indexOfSelectedItem;
-
 @end  // @interface WebMenuRunner
 
+// The callback for testing. Parameters are the same as on
+// -runMenuInView:withBounds:initialIndex:.
+using MenuWasRunCallback = base::RepeatingCallback<void(NSView*, NSRect, int)>;
+
+@interface WebMenuRunner (TestingAPI)
+// Register a callback to be called if a popup menu is invoked for a specific
+// view. If a callback is registered for a view, the menu will not be invoked
+// but instead, the callback will be run.
++ (void)registerForTestingMenuRunCallback:(MenuWasRunCallback)callback
+                                  forView:(NSView*)view;
++ (void)unregisterForTestingMenuRunCallbackForView:(NSView*)view;
+@end
+
 #endif  // CONTENT_APP_SHIM_REMOTE_COCOA_WEB_MENU_RUNNER_MAC_H_

content/app_shim_remote_cocoa/web_menu_runner_mac.mm

@@ -4,34 +4,38 @@
 
 #include "content/app_shim_remote_cocoa/web_menu_runner_mac.h"
 
+#include <AppKit/AppKit.h>
+#include <Foundation/Foundation.h>
+#include <objc/runtime.h>
 #include <stddef.h>
 
+#include <optional>
+
 #include "base/base64.h"
 #include "base/mac/mac_util.h"
 #include "base/strings/sys_string_conversions.h"
 
-@interface WebMenuRunner (PrivateAPI)
+namespace {
 
-// Worker function used during initialization.
-- (void)addItem:(const blink::mojom::MenuItemPtr&)item;
+// A key to attach a MenuWasRunCallbackHolder to the NSView*.
+static const char kMenuWasRunCallbackKey = 0;
 
-// A callback for the menu controller object to call when an item is selected
-// from the menu. This is not called if the menu is dismissed without a
-// selection.
-- (void)menuItemSelected:(id)sender;
+}  // namespace
 
-@end  // WebMenuRunner (PrivateAPI)
+@interface MenuWasRunCallbackHolder : NSObject
+@property MenuWasRunCallback callback;
+@end
+
+@implementation MenuWasRunCallbackHolder
+@synthesize callback = _callback;
+@end
 
 @implementation WebMenuRunner {
-  // The native menu control.
+  // The native menu.
   NSMenu* __strong _menu;
 
-  // A flag set to YES if a menu item was chosen, or NO if the menu was
-  // dismissed without selecting an item.
-  BOOL _menuItemWasChosen;
-
   // The index of the selected menu item.
-  int _index;
+  std::optional<int> _selectedMenuItemIndex;
 
   // The font size being used for the menu.
   CGFloat _fontSize;
@@ -46,7 +50,6 @@
   if ((self = [super init])) {
     _menu = [[NSMenu alloc] initWithTitle:@""];
     _menu.autoenablesItems = NO;
-    _index = -1;
     _fontSize = fontSize;
     _rightAligned = rightAligned;
     for (const auto& item : items) {
@@ -62,28 +65,35 @@
     return;
   }
 
-  NSString* title = base::SysUTF8ToNSString(item->label.value_or(""));
-  // https://crbug.com/1140620: SysUTF8ToNSString will return nil if the bits
+  std::string label = item->label.value_or("");
+  NSString* title = base::SysUTF8ToNSString(label);
+  // https://crbug.com/40726719: SysUTF8ToNSString will return nil if the bits
   // that it is passed cannot be turned into a CFString. If this nil value is
-  // passed to -[NSMenuItem addItemWithTitle:action:keyEquivalent], Chromium
+  // passed to -[NSMenuItem addItemWithTitle:action:keyEquivalent:], Chromium
   // will crash. Therefore, for debugging, if the result is nil, substitute in
   // the raw bytes, encoded for safety in base64, to allow for investigation.
   if (!title) {
-    title = base::SysUTF8ToNSString(base::Base64Encode(*item->label));
+    title = base::SysUTF8ToNSString(base::Base64Encode(label));
   }
+
+  // TODO(https://crbug.com/389084419): Figure out how to handle
+  // blink::mojom::MenuItem::Type::kGroup items. This should use the macOS 14+
+  // support for section headers, but popup menus have to resize themselves to
+  // match the scale of the page, and there's no good way (currently) to get the
+  // font used for section header items in order to scale it and set it.
   NSMenuItem* menuItem = [_menu addItemWithTitle:title
                                           action:@selector(menuItemSelected:)
                                    keyEquivalent:@""];
+
   if (item->tool_tip.has_value()) {
-    NSString* toolTip = base::SysUTF8ToNSString(item->tool_tip.value());
-    [menuItem setToolTip:toolTip];
+    menuItem.toolTip = base::SysUTF8ToNSString(item->tool_tip.value());
   }
-  [menuItem setEnabled:(item->enabled &&
-                        item->type != blink::mojom::MenuItem::Type::kGroup)];
-  [menuItem setTarget:self];
+  menuItem.enabled =
+      item->enabled && item->type != blink::mojom::MenuItem::Type::kGroup;
+  menuItem.target = self;
 
   // Set various alignment/language attributes.
-  NSMutableDictionary* attrs = [[NSMutableDictionary alloc] initWithCapacity:3];
+  NSMutableDictionary* attrs = [NSMutableDictionary dictionary];
   NSMutableParagraphStyle* paragraphStyle =
       [[NSMutableParagraphStyle alloc] init];
   paragraphStyle.alignment =
@@ -113,35 +123,50 @@
   //
   // This is the approach that WebKit uses; see PopupMenuMac::populate():
   // https://github.com/search?q=repo%3AWebKit/WebKit%20PopupMenuMac%3A%3Apopulate&type=code
-  NSCharacterSet* whitespaceSet = [NSCharacterSet whitespaceCharacterSet];
-  [menuItem setTitle:[title stringByTrimmingCharactersInSet:whitespaceSet]];
+  NSCharacterSet* whitespaceSet = NSCharacterSet.whitespaceCharacterSet;
+  menuItem.title = [title stringByTrimmingCharactersInSet:whitespaceSet];
 
-  [menuItem setTag:[_menu numberOfItems] - 1];
+  menuItem.tag = _menu.numberOfItems - 1;
 }
 
-// Reflects the result of the user's interaction with the popup menu. If NO, the
-// menu was dismissed without the user choosing an item, which can happen if the
-// user clicked outside the menu region or hit the escape key. If YES, the user
-// selected an item from the menu.
-- (BOOL)menuItemWasChosen {
-  return _menuItemWasChosen;
+- (std::optional<int>)selectedMenuItemIndex {
+  return _selectedMenuItemIndex;
 }
 
 - (void)menuItemSelected:(id)sender {
-  _menuItemWasChosen = YES;
+  _selectedMenuItemIndex = [sender tag];
 }
 
 - (void)runMenuInView:(NSView*)view
            withBounds:(NSRect)bounds
          initialIndex:(int)index {
+  // In a testing situation, make the callback and early-exit.
+  MenuWasRunCallbackHolder* holder =
+      objc_getAssociatedObject(view, &kMenuWasRunCallbackKey);
+  if (holder) {
+    holder.callback.Run(view, bounds, index);
+    return;
+  }
+
+  // Using NSPopUpButtonCell in this way is not SPI, but there is new(er) API to
+  // show a pop-up menu in a way that avoids the hassle of instantiating a cell
+  // just to use its innards.
+  //
+  // However, that API, -[NSMenu popUpMenuPositioningItem:atLocation:inView:],
+  // is broken and displays menus that are the incorrect width and which
+  // improperly truncate their contents (see https://crbug.com/401443090).
+  //
+  // This has been filed as FB16843355. TODO(https://crbug.com/389067059): When
+  // this FB is resolved, switch to the new API.
+
   // Set up the button cell, converting to NSView coordinates. The menu is
   // positioned such that the currently selected menu item appears over the
   // popup button, which is the expected Mac popup menu behavior.
   NSPopUpButtonCell* cell = [[NSPopUpButtonCell alloc] initTextCell:@""
                                                           pullsDown:NO];
   cell.menu = _menu;
-  // We use selectItemWithTag below so if the index is out-of-bounds nothing
-  // bad happens.
+  // Use -selectItemWithTag: so if the index is out-of-bounds nothing bad
+  // happens.
   [cell selectItemWithTag:index];
 
   if (_rightAligned) {
@@ -151,27 +176,28 @@
         NSUserInterfaceLayoutDirectionRightToLeft;
   }
 
-  // When popping up a menu near the Dock, Cocoa restricts the menu
-  // size to not overlap the Dock, with a scroll arrow.  Below a
-  // certain point this doesn't work.  At that point the menu is
-  // popped up above the element, so that the current item can be
-  // selected without mouse-tracking selecting a different item
-  // immediately.
+  // When popping up a menu near the Dock, Cocoa restricts the menu size to not
+  // overlap the Dock, with a scroll arrow. At a certain point, though, this
+  // doesn't work, so the menu is repositioned, so that the current item can be
+  // selected without mouse-tracking selecting a different item immediately.
   //
-  // Unfortunately, instead of popping up above the passed |bounds|,
-  // it pops up above the bounds of the view passed to inView:.  Use a
-  // dummy view to fake this out.
-  NSView* dummyView = [[NSView alloc] initWithFrame:bounds];
-  [view addSubview:dummyView];
+  // Unfortunately, in that situation, the cell will try to reposition the menu
+  // relative to the view passed in, as it believes that the view is the
+  // NSPopUpButton control. However, `view` is the view containing the entire
+  // web page, so if it were to be passed in, the menu would be repositioned
+  // relative to that, and would end up being wildly misplaced.
+  //
+  // Therefore, set up a fake "control" view corresponding to the visual bounds
+  // of the HTML element, so that if the menu needs to be repositioned, it is
+  // repositioned relative to that.
+  NSView* fakeControlView = [[NSView alloc] initWithFrame:bounds];
+  [view addSubview:fakeControlView];
 
-  // Display the menu, and set a flag if a menu item was chosen.
-  [cell attachPopUpWithFrame:dummyView.bounds inView:dummyView];
-  [cell performClickWithFrame:dummyView.bounds inView:dummyView];
+  // Display the menu.
+  [cell attachPopUpWithFrame:fakeControlView.bounds inView:fakeControlView];
+  [cell performClickWithFrame:fakeControlView.bounds inView:fakeControlView];
 
-  [dummyView removeFromSuperview];
-
-  if ([self menuItemWasChosen])
-    _index = [cell indexOfSelectedItem];
+  [fakeControlView removeFromSuperview];
 }
 
 - (void)cancelSynchronously {
@@ -219,8 +245,17 @@
   }
 }
 
-- (int)indexOfSelectedItem {
-  return _index;
++ (void)registerForTestingMenuRunCallback:(MenuWasRunCallback)callback
+                                  forView:(NSView*)view {
+  MenuWasRunCallbackHolder* holder = [[MenuWasRunCallbackHolder alloc] init];
+  holder.callback = callback;
+  objc_setAssociatedObject(view, &kMenuWasRunCallbackKey, holder,
+                           OBJC_ASSOCIATION_RETAIN);
+}
+
++ (void)unregisterForTestingMenuRunCallbackForView:(NSView*)view {
+  objc_setAssociatedObject(view, &kMenuWasRunCallbackKey, nil,
+                           OBJC_ASSOCIATION_RETAIN);
 }
 
 @end  // WebMenuRunner

content/public/test/content_browser_test_utils.h

@@ -177,8 +177,6 @@ class ShellAddedObserver {
 // corresponding to the page.
 class RenderWidgetHostViewCocoaObserver {
  public:
-  // The method name for 'didAddSubview'.
-  static constexpr char kDidAddSubview[] = "didAddSubview:";
   static constexpr char kShowDefinitionForAttributedString[] =
       "showDefinitionForAttributedString:atPoint:";
 
@@ -202,12 +200,12 @@ class RenderWidgetHostViewCocoaObserver {
 
   virtual ~RenderWidgetHostViewCocoaObserver();
 
-  // Called when a new NSView is added as a subview of RWHVCocoa.
-  // |rect_in_root_view| represents the bounds of the NSView in RWHVCocoa
-  // coordinates. The view will be dismissed shortly after this call.
-  virtual void DidAddSubviewWillBeDismissed(
-      const gfx::Rect& rect_in_root_view) {}
-  // Called when RenderWidgeHostViewCocoa is asked to show definition of
+  // Called when a popup was attempted to be displayed, conveying the bounds of
+  // the popup rectangle (in the RenderWidgetHostViewCocoa coordinate system)
+  // and the initially-selected item. The popup will not actually be triggered.
+  virtual void DidAttemptToShowPopup(const gfx::Rect& bounds,
+                                     int selected_item) {}
+  // Called when RenderWidgetHostViewCocoa is asked to show definition of
   // |for_word| using Mac's dictionary popup.
   virtual void OnShowDefinitionForAttributedString(
       const std::string& for_word) {}

content/public/test/content_browser_test_utils_mac.mm

@@ -9,16 +9,19 @@
 
 #include <memory>
 
+#include "base/apple/foundation_util.h"
 #include "base/apple/scoped_objc_class_swizzler.h"
 #include "base/functional/bind.h"
 #include "base/lazy_instance.h"
 #include "base/strings/sys_string_conversions.h"
 #import "content/app_shim_remote_cocoa/render_widget_host_view_cocoa.h"
+#include "content/app_shim_remote_cocoa/web_menu_runner_mac.h"
 #include "content/browser/renderer_host/render_widget_host_view_mac.h"
 #include "content/browser/renderer_host/text_input_client_mac.h"
 #include "content/browser/web_contents/web_contents_impl.h"
 #include "content/common/mac/attributed_string_type_converters.h"
 #include "content/public/browser/render_widget_host.h"
+#include "content/public/browser/render_widget_host_view.h"
 #include "content/public/browser/web_contents.h"
 #include "ui/base/mojom/attributed_string.mojom.h"
 #include "ui/gfx/geometry/point.h"
@@ -28,7 +31,6 @@
 // The interface class used to override the implementation of some of
 // RenderWidgetHostViewCocoa methods for tests.
 @interface RenderWidgetHostViewCocoaSwizzler : NSObject
-- (void)didAddSubview:(NSView*)view;
 - (void)showDefinitionForAttributedString:(NSAttributedString*)attrString
                                   atPoint:(NSPoint)textBaselineOrigin;
 @end
@@ -38,7 +40,6 @@ namespace content {
 using base::apple::ScopedObjCClassSwizzler;
 
 // static
-constexpr char RenderWidgetHostViewCocoaObserver::kDidAddSubview[];
 constexpr char
     RenderWidgetHostViewCocoaObserver::kShowDefinitionForAttributedString[];
 
@@ -52,17 +53,33 @@ std::map<WebContents*, RenderWidgetHostViewCocoaObserver*>
 
 namespace {
 
+content::RenderWidgetHostViewMac* GetRenderWidgetHostViewMac(
+    WebContents* contents) {
+  auto* rwhv_base = static_cast<RenderWidgetHostViewBase*>(
+      contents->GetRenderWidgetHostView());
+  if (rwhv_base && !rwhv_base->IsRenderWidgetHostViewChildFrame()) {
+    return static_cast<RenderWidgetHostViewMac*>(rwhv_base);
+  }
+  return nil;
+}
+
+RenderWidgetHostViewCocoa* GetRenderWidgetHostViewCocoa(WebContents* contents) {
+  content::RenderWidgetHostViewMac* rwhv_mac =
+      GetRenderWidgetHostViewMac(contents);
+  if (!rwhv_mac) {
+    return nil;
+  }
+  return rwhv_mac->GetInProcessNSView();
+}
+
 content::RenderWidgetHostViewMac* GetRenderWidgetHostViewMac(NSObject* object) {
   for (auto* contents : WebContentsImpl::GetAllWebContents()) {
-    auto* rwhv_base = static_cast<RenderWidgetHostViewBase*>(
-        contents->GetRenderWidgetHostView());
-    if (rwhv_base && !rwhv_base->IsRenderWidgetHostViewChildFrame()) {
-      auto* rwhv_mac = static_cast<RenderWidgetHostViewMac*>(rwhv_base);
-      if (rwhv_mac->GetInProcessNSView() == object)
-        return rwhv_mac;
+    content::RenderWidgetHostViewMac* rwhv_mac =
+        GetRenderWidgetHostViewMac(contents);
+    if (rwhv_mac && rwhv_mac->GetInProcessNSView() == object) {
+      return rwhv_mac;
     }
   }
-
   return nullptr;
 }
 
@@ -84,30 +101,43 @@ RenderWidgetHostViewCocoaObserver::GetObserver(WebContents* web_contents) {
 RenderWidgetHostViewCocoaObserver::RenderWidgetHostViewCocoaObserver(
     WebContents* web_contents)
     : web_contents_(web_contents) {
-  if (rwhvcocoa_swizzlers_.empty())
+  if (rwhvcocoa_swizzlers_.empty()) {
     SetUpSwizzlers();
+  }
+
+  MenuWasRunCallback callback = base::BindRepeating(
+      [](RenderWidgetHostViewCocoaObserver* observer, NSView* view,
+         NSRect bounds, int index) {
+        RenderWidgetHostViewCocoa* rwhv_cocoa =
+            base::apple::ObjCCast<RenderWidgetHostViewCocoa>(view);
+        gfx::Rect rect = [rwhv_cocoa flipNSRectToRect:bounds];
+        observer->DidAttemptToShowPopup(rect, index);
+      },
+      this);
+  [WebMenuRunner registerForTestingMenuRunCallback:callback
+                                           forView:GetRenderWidgetHostViewCocoa(
+                                                       web_contents)];
 
   DCHECK(!observers_.count(web_contents));
   observers_[web_contents] = this;
 }
 
 RenderWidgetHostViewCocoaObserver::~RenderWidgetHostViewCocoaObserver() {
+  [WebMenuRunner
+      unregisterForTestingMenuRunCallbackForView:GetRenderWidgetHostViewCocoa(
+                                                     web_contents_)];
+
   observers_.erase(web_contents_);
 
-  if (observers_.empty())
+  if (observers_.empty()) {
     rwhvcocoa_swizzlers_.clear();
+  }
 }
 
 void RenderWidgetHostViewCocoaObserver::SetUpSwizzlers() {
-  if (!rwhvcocoa_swizzlers_.empty())
+  if (!rwhvcocoa_swizzlers_.empty()) {
     return;
-
-  // [RenderWidgetHostViewCocoa didAddSubview:NSView*].
-  rwhvcocoa_swizzlers_[kDidAddSubview] =
-      std::make_unique<ScopedObjCClassSwizzler>(
-          GetRenderWidgetHostViewCocoaClassForTesting(),
-          [RenderWidgetHostViewCocoaSwizzler class],
-          NSSelectorFromString(@(kDidAddSubview)));
+  }
 
   // [RenderWidgetHostViewCocoa showDefinitionForAttributedString:atPoint].
   rwhvcocoa_swizzlers_[kShowDefinitionForAttributedString] =
@@ -174,46 +204,6 @@ void GetStringFromRangeForRenderWidget(
 }  // namespace content
 
 @implementation RenderWidgetHostViewCocoaSwizzler
-- (void)didAddSubview:(NSView*)view {
-  content::RenderWidgetHostViewCocoaObserver::GetSwizzler(
-      content::RenderWidgetHostViewCocoaObserver::kDidAddSubview)
-      ->InvokeOriginal<void, NSView*>(self, _cmd, view);
-
-  content::RenderWidgetHostViewMac* rwhv_mac =
-      content::GetRenderWidgetHostViewMac(self);
-
-  if (!rwhv_mac)
-    return;
-
-  content::RenderWidgetHostViewCocoaObserver* observer =
-      content::RenderWidgetHostViewCocoaObserver::GetObserver(
-          rwhv_mac->GetWebContents());
-
-  if (!observer)
-    return;
-
-  NSRect bounds_in_cocoa_view =
-      [view convertRect:view.bounds toView:rwhv_mac->GetInProcessNSView()];
-
-  gfx::Rect rect =
-      [rwhv_mac->GetInProcessNSView() flipNSRectToRect:bounds_in_cocoa_view];
-
-  observer->DidAddSubviewWillBeDismissed(rect);
-
-  // This override is useful for testing popups. To make sure the run loops end
-  // after the call it is best to dismiss the popup soon.
-  NSEvent* dismissal_event =
-      [NSEvent mouseEventWithType:NSEventTypeLeftMouseDown
-                         location:NSZeroPoint
-                    modifierFlags:0
-                        timestamp:0.0
-                     windowNumber:0
-                          context:nil
-                      eventNumber:0
-                       clickCount:1
-                         pressure:1.0];
-  [NSApplication.sharedApplication postEvent:dismissal_event atStart:false];
-}
 
 - (void)showDefinitionForAttributedString:(NSAttributedString*)attrString
                                   atPoint:(NSPoint)textBaselineOrigin {
@@ -228,9 +218,11 @@ void GetStringFromRangeForRenderWidget(
   auto* observer = content::RenderWidgetHostViewCocoaObserver::GetObserver(
       rwhv_mac->GetWebContents());
 
-  if (!observer)
+  if (!observer) {
     return;
+  }
   observer->OnShowDefinitionForAttributedString(
       base::SysNSStringToUTF8(attrString.string));
 }
+
 @end

ui/base/interaction/interaction_test_util_mac.mm

@@ -38,7 +38,7 @@ ActionResult InteractionTestUtilSimulatorMac::SelectMenuItem(
     LOG(ERROR) << "Cannot retrieve MenuControllerCocoa from menu.";
     return ActionResult::kFailed;
   }
-  ui::MenuModel* const model = [controller model];
+  ui::MenuModel* const model = controller.model;
   if (!model) {
     LOG(ERROR) << "Cannot retrieve MenuModel from controller.";
     return ActionResult::kFailed;

ui/menus/cocoa/menu_controller.h

@@ -33,41 +33,24 @@ COMPONENT_EXPORT(UI_MENUS)
 @interface MenuControllerCocoa
     : NSObject <NSMenuDelegate, NSUserInterfaceValidations>
 
-// Note that changing this will have no effect if you use
-// |-initWithModel:useWithPopUpButtonCell:| or after the first call to |-menu|.
-@property(nonatomic, assign) BOOL useWithPopUpButtonCell;
+- (instancetype)init NS_UNAVAILABLE;
 
-// NIB-based initializer. This does not create a menu. Clients can set the
-// properties of the object and the menu will be created upon the first call to
-// |-maybeBuildWithColorProvider:| or |-menu|.
-- (instancetype)init;
-
-// Builds a NSMenu from the pre-built model (must not be nil). Changes made
-// to the contents of the model after calling this will not be noticed. If
-// the menu will be displayed by a NSPopUpButtonCell, it needs to be of a
-// slightly different form (0th item is empty).
+// Builds a NSMenu from the model which must not be null. Changes made to the
+// contents of the model after calling this will not be noticed.
 - (instancetype)initWithModel:(ui::MenuModel*)model
-                     delegate:(id<MenuControllerCocoaDelegate>)delegate
-       useWithPopUpButtonCell:(BOOL)useWithCell;
+                     delegate:(id<MenuControllerCocoaDelegate>)delegate;
 
 // Programmatically close the constructed menu.
 - (void)cancel;
 
-- (ui::MenuModel*)model;
-- (void)setModel:(ui::MenuModel*)model;
+@property(readwrite) ui::MenuModel* model;
 
-// Access to the constructed menu if the complex initializer was used. If the
-// menu has not bee built yet it will be built on the first call.
-- (NSMenu*)menu;
+// Access to the constructed menu.
+@property(readonly) NSMenu* menu;
 
 // Whether the menu is currently open.
-- (BOOL)isMenuOpen;
+@property(readonly, getter=isMenuOpen) BOOL menuOpen;
 
 @end
 
-@interface MenuControllerCocoa (TestingAPI)
-// Whether the NSMenu has been built.
-- (BOOL)isMenuBuiltForTesting;
-@end
-
 #endif  // UI_MENUS_COCOA_MENU_CONTROLLER_H_

ui/menus/cocoa/menu_controller.mm

@@ -4,6 +4,8 @@
 
 #import "ui/menus/cocoa/menu_controller.h"
 
+#include <AppKit/AppKit.h>
+
 #include "base/apple/bridging.h"
 #include "base/apple/foundation_util.h"
 #include "base/apple/owned_objc.h"
@@ -53,9 +55,8 @@ bool MenuHasVisibleItems(const ui::MenuModel* model) {
 // which allows it to be stored in the representedObject field of an NSMenuItem.
 @interface WeakPtrToMenuModelAsNSObject : NSObject
 + (instancetype)weakPtrForModel:(ui::MenuModel*)model;
-+ (ui::MenuModel*)getFrom:(id)instance;
-- (instancetype)initWithModel:(ui::MenuModel*)model;
-- (ui::MenuModel*)menuModel;
++ (ui::MenuModel*)menuModelForNSMenuItem:(NSMenuItem*)menuItem;
+@property(readonly) ui::MenuModel* menuModel;
 @end
 
 @implementation WeakPtrToMenuModelAsNSObject {
@@ -66,9 +67,10 @@ bool MenuHasVisibleItems(const ui::MenuModel* model) {
   return [[WeakPtrToMenuModelAsNSObject alloc] initWithModel:model];
 }
 
-+ (ui::MenuModel*)getFrom:(id)instance {
-  return [base::apple::ObjCCastStrict<WeakPtrToMenuModelAsNSObject>(instance)
-      menuModel];
++ (ui::MenuModel*)menuModelForNSMenuItem:(NSMenuItem*)menuItem {
+  return base::apple::ObjCCastStrict<WeakPtrToMenuModelAsNSObject>(
+             menuItem.representedObject)
+      .menuModel;
 }
 
 - (instancetype)initWithModel:(ui::MenuModel*)model {
@@ -114,13 +116,10 @@ bool MenuHasVisibleItems(const ui::MenuModel* model) {
 @implementation MenuControllerCocoa {
   base::WeakPtr<ui::MenuModel> _model;
   NSMenu* __strong _menu;
-  BOOL _useWithPopUpButtonCell;  // If YES, 0th item is blank
   BOOL _isMenuOpen;
   id<MenuControllerCocoaDelegate> __weak _delegate;
 }
 
-@synthesize useWithPopUpButtonCell = _useWithPopUpButtonCell;
-
 - (ui::MenuModel*)model {
   return _model.get();
 }
@@ -129,19 +128,12 @@ bool MenuHasVisibleItems(const ui::MenuModel* model) {
   _model = model->AsWeakPtr();
 }
 
-- (instancetype)init {
-  self = [super init];
-  return self;
-}
-
 - (instancetype)initWithModel:(ui::MenuModel*)model
-                     delegate:(id<MenuControllerCocoaDelegate>)delegate
-       useWithPopUpButtonCell:(BOOL)useWithCell {
+                     delegate:(id<MenuControllerCocoaDelegate>)delegate {
   if ((self = [super init])) {
     _model = model->AsWeakPtr();
     _delegate = delegate;
-    _useWithPopUpButtonCell = useWithCell;
-    [self maybeBuild];
+    [self buildMenu];
   }
   return self;
 }
@@ -215,12 +207,13 @@ bool MenuHasVisibleItems(const ui::MenuModel* model) {
     item.target = nil;
     item.action = nil;
     item.submenu = submenu;
-    // [item setSubmenu] updates target and action which means clicking on a
-    // submenu entry will not call [self validateUserInterfaceItem].
+    // [item setSubmenu:] updates target and action which means clicking on a
+    // submenu entry will not call [self validateUserInterfaceItem:].
     DCHECK_EQ(item.action, @selector(submenuAction:));
     DCHECK_EQ(item.target, submenu);
     // Set the enabled state here as submenu entries do not call into
-    // validateUserInterfaceItem. See crbug.com/981294 and crbug.com/991472.
+    // validateUserInterfaceItem. See https://crbug.com/40634897 and
+    // https://crbug.com/41474827.
     [item setEnabled:model->IsEnabledAt(index)];
   } else {
     // The MenuModel works on indexes so we can't just set the command id as the
@@ -235,9 +228,7 @@ bool MenuHasVisibleItems(const ui::MenuModel* model) {
     // On the Mac, context menus do not have accelerators except when
     // |force_show_accelerator_for_item| is set to true. Consult with the Mac
     // team before using the flag!
-    // Menus constructed for context use have useWithPopUpButtonCell_ set to NO.
-    if (_useWithPopUpButtonCell ||
-        model->GetForceShowAcceleratorForItemAt(index)) {
+    if (model->GetForceShowAcceleratorForItemAt(index)) {
       ui::Accelerator accelerator;
       if (model->GetAcceleratorAt(index, &accelerator)) {
         KeyEquivalentAndModifierMask* equivalent =
@@ -264,7 +255,7 @@ bool MenuHasVisibleItems(const ui::MenuModel* model) {
   }
 
   ui::MenuModel* model =
-      [WeakPtrToMenuModelAsNSObject getFrom:menuItem.representedObject];
+      [WeakPtrToMenuModelAsNSObject menuModelForNSMenuItem:menuItem];
   if (!model) {
     return NO;
   }
@@ -299,7 +290,7 @@ bool MenuHasVisibleItems(const ui::MenuModel* model) {
   NSMenuItem* menuItem = base::apple::ObjCCastStrict<NSMenuItem>(sender);
 
   ui::MenuModel* model =
-      [WeakPtrToMenuModelAsNSObject getFrom:menuItem.representedObject];
+      [WeakPtrToMenuModelAsNSObject menuModelForNSMenuItem:menuItem];
   DCHECK(model);
   const size_t modelIndex = base::checked_cast<size_t>(menuItem.tag);
   const ui::ElementIdentifier identifier =
@@ -314,10 +305,10 @@ bool MenuHasVisibleItems(const ui::MenuModel* model) {
   // Note: |self| may be destroyed by the call to ActivatedAt().
 }
 
-- (void)maybeBuild {
-  if (_menu || !_model) {
-    return;
-  }
+- (void)buildMenu {
+  // The menu is eagerly built in the initializer, so the model cannot be null
+  // at this point.
+  CHECK(_model);
 
   _menu = [self menuFromModel:_model.get()];
   _menu.delegate = self;
@@ -329,21 +320,9 @@ bool MenuHasVisibleItems(const ui::MenuModel* model) {
   if (_delegate) {
     [_delegate controllerWillAddMenu:_menu fromModel:_model.get()];
   }
-
-  // If this is to be used with a NSPopUpButtonCell, add an item at the 0th
-  // position that's empty. Doing it after the menu has been constructed won't
-  // complicate creation logic, and since the tags are model indexes, they
-  // are unaffected by the extra item.
-  if (_useWithPopUpButtonCell) {
-    NSMenuItem* blankItem = [[NSMenuItem alloc] initWithTitle:@""
-                                                       action:nil
-                                                keyEquivalent:@""];
-    [_menu insertItem:blankItem atIndex:0];
-  }
 }
 
 - (NSMenu*)menu {
-  [self maybeBuild];
   return _menu;
 }
 
@@ -368,11 +347,3 @@ bool MenuHasVisibleItems(const ui::MenuModel* model) {
 }
 
 @end
-
-@implementation MenuControllerCocoa (TestingAPI)
-
-- (BOOL)isMenuBuiltForTesting {
-  return _menu != nil;
-}
-
-@end

ui/menus/cocoa/menu_controller_unittest.mm

@@ -171,8 +171,7 @@ class OwningDelegate : public Delegate {
       : did_delete_(did_delete), model_(this) {
     model_.AddItem(1, u"foo");
     controller_ = [[WatchedLifetimeMenuController alloc] initWithModel:&model_
-                                                              delegate:nil
-                                                useWithPopUpButtonCell:NO];
+                                                              delegate:nil];
     [controller_ setDeallocCalled:did_dealloc];
   }
 
@@ -227,9 +226,8 @@ TEST_F(MenuControllerTest, EmptyMenu) {
   Delegate delegate;
   SimpleMenuModel model(&delegate);
   MenuControllerCocoa* menu = [[MenuControllerCocoa alloc] initWithModel:&model
-                                                                delegate:nil
-                                                  useWithPopUpButtonCell:NO];
-  EXPECT_EQ(0, [[menu menu] numberOfItems]);
+                                                                delegate:nil];
+  EXPECT_EQ(0, menu.menu.numberOfItems);
 }
 
 TEST_F(MenuControllerTest, BasicCreation) {
@@ -243,18 +241,17 @@ TEST_F(MenuControllerTest, BasicCreation) {
   model.AddItem(5, u"five");
 
   MenuControllerCocoa* menu = [[MenuControllerCocoa alloc] initWithModel:&model
-                                                                delegate:nil
-                                                  useWithPopUpButtonCell:NO];
-  EXPECT_EQ(6, [[menu menu] numberOfItems]);
+                                                                delegate:nil];
+  EXPECT_EQ(6, menu.menu.numberOfItems);
 
   // Check the title, tag, and represented object are correct for a random
   // element.
-  NSMenuItem* itemTwo = [[menu menu] itemAtIndex:2];
-  NSString* title = [itemTwo title];
+  NSMenuItem* itemTwo = [menu.menu itemAtIndex:2];
+  NSString* title = itemTwo.title;
   EXPECT_EQ(u"three", base::SysNSStringToUTF16(title));
-  EXPECT_EQ(2, [itemTwo tag]);
+  EXPECT_EQ(2, itemTwo.tag);
 
-  EXPECT_TRUE([[[menu menu] itemAtIndex:3] isSeparatorItem]);
+  EXPECT_TRUE([menu.menu itemAtIndex:3].separatorItem);
 }
 
 TEST_F(MenuControllerTest, Submenus) {
@@ -269,30 +266,29 @@ TEST_F(MenuControllerTest, Submenus) {
   model.AddItem(6, u"three");
 
   MenuControllerCocoa* menu = [[MenuControllerCocoa alloc] initWithModel:&model
-                                                                delegate:nil
-                                                  useWithPopUpButtonCell:NO];
-  EXPECT_EQ(3, [[menu menu] numberOfItems]);
+                                                                delegate:nil];
+  EXPECT_EQ(3, menu.menu.numberOfItems);
 
   // Inspect the submenu to ensure it has correct properties.
-  NSMenuItem* menuItem = [[menu menu] itemAtIndex:1];
-  EXPECT_TRUE([menuItem isEnabled]);
-  NSMenu* submenu = [menuItem submenu];
+  NSMenuItem* menuItem = [menu.menu itemAtIndex:1];
+  EXPECT_TRUE(menuItem.enabled);
+  NSMenu* submenu = menuItem.submenu;
   EXPECT_TRUE(submenu);
-  EXPECT_EQ(3, [submenu numberOfItems]);
+  EXPECT_EQ(3, submenu.numberOfItems);
 
   // Inspect one of the items to make sure it has the correct model as its
   // represented object and the proper tag.
   NSMenuItem* submenuItem = [submenu itemAtIndex:1];
-  NSString* title = [submenuItem title];
+  NSString* title = submenuItem.title;
   EXPECT_EQ(u"sub-two", base::SysNSStringToUTF16(title));
-  EXPECT_EQ(1, [submenuItem tag]);
+  EXPECT_EQ(1, submenuItem.tag);
 
   // Make sure the item after the submenu is correct and its represented
   // object is back to the top model.
-  NSMenuItem* item = [[menu menu] itemAtIndex:2];
-  title = [item title];
+  NSMenuItem* item = [menu.menu itemAtIndex:2];
+  title = item.title;
   EXPECT_EQ(u"three", base::SysNSStringToUTF16(title));
-  EXPECT_EQ(2, [item tag]);
+  EXPECT_EQ(2, item.tag);
 }
 
 TEST_F(MenuControllerTest, EmptySubmenu) {
@@ -303,16 +299,15 @@ TEST_F(MenuControllerTest, EmptySubmenu) {
   model.AddSubMenuWithStringId(2, kTestLabelResourceId, &submodel);
 
   MenuControllerCocoa* menu = [[MenuControllerCocoa alloc] initWithModel:&model
-                                                                delegate:nil
-                                                  useWithPopUpButtonCell:NO];
-  EXPECT_EQ(2, [[menu menu] numberOfItems]);
+                                                                delegate:nil];
+  EXPECT_EQ(2, menu.menu.numberOfItems);
 
   // Inspect the submenu to ensure it has one item labeled "(empty)".
-  NSMenu* submenu = [[[menu menu] itemAtIndex:1] submenu];
+  NSMenu* submenu = [menu.menu itemAtIndex:1].submenu;
   EXPECT_TRUE(submenu);
-  EXPECT_EQ(1, [submenu numberOfItems]);
+  EXPECT_EQ(1, submenu.numberOfItems);
 
-  EXPECT_NSEQ(@"(empty)", [[submenu itemAtIndex:0] title]);
+  EXPECT_NSEQ(@"(empty)", [submenu itemAtIndex:0].title);
 }
 
 // Tests that an empty menu item, "(empty)", is added to a submenu that contains
@@ -330,16 +325,15 @@ TEST_F(MenuControllerTest, EmptySubmenuWhenAllChildItemsAreHidden) {
   model.AddSubMenuWithStringId(4, kTestLabelResourceId, &submodel);
 
   MenuControllerCocoa* menu = [[MenuControllerCocoa alloc] initWithModel:&model
-                                                                delegate:nil
-                                                  useWithPopUpButtonCell:NO];
-  EXPECT_EQ(2, [[menu menu] numberOfItems]);
+                                                                delegate:nil];
+  EXPECT_EQ(2, menu.menu.numberOfItems);
 
   // Inspect the submenu to ensure it has one item labeled "(empty)".
-  NSMenu* submenu = [[[menu menu] itemAtIndex:1] submenu];
+  NSMenu* submenu = [menu.menu itemAtIndex:1].submenu;
   EXPECT_TRUE(submenu);
-  EXPECT_EQ(1, [submenu numberOfItems]);
+  EXPECT_EQ(1, submenu.numberOfItems);
 
-  EXPECT_NSEQ(@"(empty)", [[submenu itemAtIndex:0] title]);
+  EXPECT_NSEQ(@"(empty)", [submenu itemAtIndex:0].title);
 }
 
 // Tests hiding a submenu item. If a submenu item with children is set to
@@ -363,29 +357,27 @@ TEST_F(MenuControllerTest, HiddenSubmenu) {
 
   // Create the controller.
   MenuControllerCocoa* menu_controller =
-      [[MenuControllerCocoa alloc] initWithModel:&model
-                                        delegate:nil
-                          useWithPopUpButtonCell:NO];
-  EXPECT_EQ(2, [[menu_controller menu] numberOfItems]);
-  delegate.menu_to_close_ = [menu_controller menu];
+      [[MenuControllerCocoa alloc] initWithModel:&model delegate:nil];
+  EXPECT_EQ(2, menu_controller.menu.numberOfItems);
+  delegate.menu_to_close_ = menu_controller.menu;
 
   // Show the menu.
-  [NSRunLoop.currentRunLoop
-      performInModes:@[ NSEventTrackingRunLoopMode ]
-               block:^{
-                 EXPECT_TRUE([menu_controller isMenuOpen]);
-                 // Ensure that the submenu is hidden.
-                 NSMenuItem* item = [[menu_controller menu] itemAtIndex:1];
-                 EXPECT_TRUE([item isHidden]);
-               }];
+  [NSRunLoop.currentRunLoop performInModes:@[ NSEventTrackingRunLoopMode ]
+                                     block:^{
+                                       EXPECT_TRUE(menu_controller.menuOpen);
+                                       // Ensure that the submenu is hidden.
+                                       NSMenuItem* item =
+                                           [menu_controller.menu itemAtIndex:1];
+                                       EXPECT_TRUE(item.hidden);
+                                     }];
 
   // Pop open the menu, which will spin an event-tracking run loop.
-  [NSMenu popUpContextMenu:[menu_controller menu]
+  [NSMenu popUpContextMenu:menu_controller.menu
                  withEvent:cocoa_test_event_utils::RightMouseDownAtPoint(
                                NSZeroPoint)
-                   forView:[test_window() contentView]];
+                   forView:test_window().contentView];
 
-  EXPECT_FALSE([menu_controller isMenuOpen]);
+  EXPECT_FALSE(menu_controller.menuOpen);
 
   // Pump the task that notifies the delegate.
   base::RunLoop().RunUntilIdle();
@@ -415,34 +407,33 @@ TEST_F(MenuControllerTest, DisabledSubmenu) {
 
   // Create the controller.
   MenuControllerCocoa* menu_controller =
-      [[MenuControllerCocoa alloc] initWithModel:&model
-                                        delegate:nil
-                          useWithPopUpButtonCell:NO];
-  delegate.menu_to_close_ = [menu_controller menu];
+      [[MenuControllerCocoa alloc] initWithModel:&model delegate:nil];
+  delegate.menu_to_close_ = menu_controller.menu;
 
   // Show the menu.
-  [NSRunLoop.currentRunLoop
-      performInModes:@[ NSEventTrackingRunLoopMode ]
-               block:^{
-                 EXPECT_TRUE([menu_controller isMenuOpen]);
+  [NSRunLoop.currentRunLoop performInModes:@[ NSEventTrackingRunLoopMode ]
+                                     block:^{
+                                       EXPECT_TRUE(menu_controller.menuOpen);
 
-                 // Ensure that the disabled submenu is disabled.
-                 NSMenuItem* disabled_item =
-                     [[menu_controller menu] itemAtIndex:1];
-                 EXPECT_FALSE([disabled_item isEnabled]);
+                                       // Ensure that the disabled submenu is
+                                       // disabled.
+                                       NSMenuItem* disabled_item =
+                                           [menu_controller.menu itemAtIndex:1];
+                                       EXPECT_FALSE(disabled_item.enabled);
 
-                 // Ensure that the enabled submenu is enabled.
-                 NSMenuItem* enabled_item =
-                     [[menu_controller menu] itemAtIndex:2];
-                 EXPECT_TRUE([enabled_item isEnabled]);
-               }];
+                                       // Ensure that the enabled submenu is
+                                       // enabled.
+                                       NSMenuItem* enabled_item =
+                                           [menu_controller.menu itemAtIndex:2];
+                                       EXPECT_TRUE(enabled_item.enabled);
+                                     }];
 
   // Pop open the menu, which will spin an event-tracking run loop.
-  [NSMenu popUpContextMenu:[menu_controller menu]
+  [NSMenu popUpContextMenu:menu_controller.menu
                  withEvent:cocoa_test_event_utils::RightMouseDownAtPoint(
                                NSZeroPoint)
-                   forView:[test_window() contentView]];
-  EXPECT_FALSE([menu_controller isMenuOpen]);
+                   forView:test_window().contentView];
+  EXPECT_FALSE(menu_controller.menuOpen);
 
   // Pump the task that notifies the delegate.
   base::RunLoop().RunUntilIdle();
@@ -450,52 +441,30 @@ TEST_F(MenuControllerTest, DisabledSubmenu) {
   EXPECT_TRUE(delegate.did_close_);
 }
 
-TEST_F(MenuControllerTest, PopUpButton) {
-  Delegate delegate;
-  SimpleMenuModel model(&delegate);
-  model.AddItem(1, u"one");
-  model.AddItem(2, u"two");
-  model.AddItem(3, u"three");
-
-  // Menu should have an extra item inserted at position 0 that has an empty
-  // title.
-  MenuControllerCocoa* menu = [[MenuControllerCocoa alloc] initWithModel:&model
-                                                                delegate:nil
-                                                  useWithPopUpButtonCell:YES];
-  EXPECT_EQ(4, [[menu menu] numberOfItems]);
-  EXPECT_EQ(std::u16string(),
-            base::SysNSStringToUTF16([[[menu menu] itemAtIndex:0] title]));
-
-  // Make sure the tags are still correct (the index no longer matches the tag).
-  NSMenuItem* itemTwo = [[menu menu] itemAtIndex:2];
-  EXPECT_EQ(1, [itemTwo tag]);
-}
-
 TEST_F(MenuControllerTest, Execute) {
   Delegate delegate;
   SimpleMenuModel model(&delegate);
   model.AddItem(1, u"one");
   MenuControllerCocoa* menu = [[MenuControllerCocoa alloc] initWithModel:&model
-                                                                delegate:nil
-                                                  useWithPopUpButtonCell:NO];
-  EXPECT_EQ(1, [[menu menu] numberOfItems]);
+                                                                delegate:nil];
+  EXPECT_EQ(1, menu.menu.numberOfItems);
 
   // Fake selecting the menu item, we expect the delegate to be told to execute
   // a command.
-  NSMenuItem* item = [[menu menu] itemAtIndex:0];
+  NSMenuItem* item = [menu.menu itemAtIndex:0];
 #pragma clang diagnostic push
 #pragma clang diagnostic ignored "-Warc-performSelector-leaks"
-  [[item target] performSelector:[item action] withObject:item];
+  [item.target performSelector:item.action withObject:item];
 #pragma clang diagnostic pop
   EXPECT_EQ(1, delegate.execute_count_);
 }
 
 void Validate(MenuControllerCocoa* controller, NSMenu* menu) {
-  for (int i = 0; i < [menu numberOfItems]; ++i) {
+  for (int i = 0; i < menu.numberOfItems; ++i) {
     NSMenuItem* item = [menu itemAtIndex:i];
     [controller validateUserInterfaceItem:item];
-    if ([item hasSubmenu]) {
-      Validate(controller, [item submenu]);
+    if (item.hasSubmenu) {
+      Validate(controller, item.submenu);
     }
   }
 }
@@ -510,11 +479,10 @@ TEST_F(MenuControllerTest, Validate) {
   model.AddSubMenuWithStringId(3, kTestLabelResourceId, &submodel);
 
   MenuControllerCocoa* menu = [[MenuControllerCocoa alloc] initWithModel:&model
-                                                                delegate:nil
-                                                  useWithPopUpButtonCell:NO];
-  EXPECT_EQ(3, [[menu menu] numberOfItems]);
+                                                                delegate:nil];
+  EXPECT_EQ(3, menu.menu.numberOfItems);
 
-  Validate(menu, [menu menu]);
+  Validate(menu, menu.menu);
 }
 
 // Tests that items which have a font set actually use that font.
@@ -529,34 +497,13 @@ TEST_F(MenuControllerTest, LabelFontList) {
   model.AddItem(2, u"two");
 
   MenuControllerCocoa* menu = [[MenuControllerCocoa alloc] initWithModel:&model
-                                                                delegate:nil
-                                                  useWithPopUpButtonCell:NO];
-  EXPECT_EQ(2, [[menu menu] numberOfItems]);
+                                                                delegate:nil];
+  EXPECT_EQ(2, menu.menu.numberOfItems);
 
-  Validate(menu, [menu menu]);
+  Validate(menu, menu.menu);
 
-  EXPECT_TRUE([[[menu menu] itemAtIndex:0] attributedTitle] != nil);
-  EXPECT_TRUE([[[menu menu] itemAtIndex:1] attributedTitle] == nil);
-}
-
-TEST_F(MenuControllerTest, DefaultInitializer) {
-  Delegate delegate;
-  SimpleMenuModel model(&delegate);
-  model.AddItem(1, u"one");
-  model.AddItem(2, u"two");
-  model.AddItem(3, u"three");
-
-  MenuControllerCocoa* menu = [[MenuControllerCocoa alloc] init];
-  EXPECT_FALSE([menu menu]);
-
-  [menu setModel:&model];
-  [menu setUseWithPopUpButtonCell:NO];
-  EXPECT_TRUE([menu menu]);
-  EXPECT_EQ(3, [[menu menu] numberOfItems]);
-
-  // Check immutability.
-  model.AddItem(4, u"four");
-  EXPECT_EQ(3, [[menu menu] numberOfItems]);
+  EXPECT_TRUE([menu.menu itemAtIndex:0].attributedTitle != nil);
+  EXPECT_TRUE([menu.menu itemAtIndex:1].attributedTitle == nil);
 }
 
 // Test that menus with dynamic labels actually get updated.
@@ -570,16 +517,15 @@ TEST_F(MenuControllerTest, Dynamic) {
   SimpleMenuModel model(&delegate);
   model.AddItem(1, u"foo");
   MenuControllerCocoa* menu = [[MenuControllerCocoa alloc] initWithModel:&model
-                                                                delegate:nil
-                                                  useWithPopUpButtonCell:NO];
-  EXPECT_EQ(1, [[menu menu] numberOfItems]);
+                                                                delegate:nil];
+  EXPECT_EQ(1, menu.menu.numberOfItems);
   // Validate() simulates opening the menu - the item label/icon should be
   // initialized after this so we can validate the menu contents.
-  Validate(menu, [menu menu]);
-  NSMenuItem* item = [[menu menu] itemAtIndex:0];
+  Validate(menu, menu.menu);
+  NSMenuItem* item = [menu.menu itemAtIndex:0];
   // Item should have the "initial" label and no icon.
-  EXPECT_EQ(initial, base::SysNSStringToUTF16([item title]));
-  EXPECT_EQ(nil, [item image]);
+  EXPECT_EQ(initial, base::SysNSStringToUTF16(item.title));
+  EXPECT_EQ(nil, item.image);
 
   // Now update the item to have a label of "second" and an icon.
   std::u16string second = u"second";
@@ -587,15 +533,15 @@ TEST_F(MenuControllerTest, Dynamic) {
   const gfx::Image& icon = gfx::test::CreateImage(32, 32);
   delegate.SetDynamicIcon(icon);
   // Simulate opening the menu and validate that the item label + icon changes.
-  Validate(menu, [menu menu]);
-  EXPECT_EQ(second, base::SysNSStringToUTF16([item title]));
-  EXPECT_TRUE([item image] != nil);
+  Validate(menu, menu.menu);
+  EXPECT_EQ(second, base::SysNSStringToUTF16(item.title));
+  EXPECT_NE(nil, item.image);
 
   // Now get rid of the icon and make sure it goes away.
   delegate.SetDynamicIcon(gfx::Image());
-  Validate(menu, [menu menu]);
-  EXPECT_EQ(second, base::SysNSStringToUTF16([item title]));
-  EXPECT_EQ(nil, [item image]);
+  Validate(menu, menu.menu);
+  EXPECT_EQ(second, base::SysNSStringToUTF16(item.title));
+  EXPECT_EQ(nil, item.image);
 }
 
 TEST_F(MenuControllerTest, OpenClose) {
@@ -612,26 +558,25 @@ TEST_F(MenuControllerTest, OpenClose) {
 
   // Create the controller.
   MenuControllerCocoa* menu = [[MenuControllerCocoa alloc] initWithModel:&model
-                                                                delegate:nil
-                                                  useWithPopUpButtonCell:NO];
-  delegate.menu_to_close_ = [menu menu];
+                                                                delegate:nil];
+  delegate.menu_to_close_ = menu.menu;
 
-  EXPECT_FALSE([menu isMenuOpen]);
+  EXPECT_FALSE(menu.menuOpen);
 
   // In the event tracking run loop mode of the menu, verify that the controller
   // reports the menu as open.
   [NSRunLoop.currentRunLoop performInModes:@[ NSEventTrackingRunLoopMode ]
                                      block:^{
-                                       EXPECT_TRUE([menu isMenuOpen]);
+                                       EXPECT_TRUE(menu.menuOpen);
                                      }];
 
   // Pop open the menu, which will spin an event-tracking run loop.
-  [NSMenu popUpContextMenu:[menu menu]
+  [NSMenu popUpContextMenu:menu.menu
                  withEvent:cocoa_test_event_utils::RightMouseDownAtPoint(
                                NSZeroPoint)
-                   forView:[test_window() contentView]];
+                   forView:test_window().contentView];
 
-  EXPECT_FALSE([menu isMenuOpen]);
+  EXPECT_FALSE(menu.menuOpen);
 
   // When control returns back to here, the menu will have finished running its
   // loop and will have closed itself (see Delegate::OnMenuWillShow).
@@ -669,13 +614,13 @@ TEST_F(MenuControllerTest, OwningDelegate) {
     // Unretained reference to the controller.
     MenuControllerCocoa* controller = delegate->controller();
 
-    item = [[controller menu] itemAtIndex:0];
+    item = [controller.menu itemAtIndex:0];
     EXPECT_TRUE(item);
 
     // Simulate opening the menu and selecting an item. Methods are always
     // invoked by AppKit in the following order.
-    [controller menuWillOpen:[controller menu]];
-    [controller menuDidClose:[controller menu]];
+    [controller menuWillOpen:controller.menu];
+    [controller menuDidClose:controller.menu];
   }
   EXPECT_FALSE(did_dealloc);
   EXPECT_FALSE(did_delete);
@@ -688,28 +633,13 @@ TEST_F(MenuControllerTest, OwningDelegate) {
   @autoreleasepool {
 #pragma clang diagnostic push
 #pragma clang diagnostic ignored "-Warc-performSelector-leaks"
-    [[item target] performSelector:[item action] withObject:item];
+    [item.target performSelector:item.action withObject:item];
 #pragma clang diagnostic pop
   }
   EXPECT_TRUE(did_dealloc);
   EXPECT_TRUE(did_delete);
 }
 
-// Tests to make sure that when |-initWithModel:| is called the menu is
-// constructed.
-TEST_F(MenuControllerTest, InitBuildsMenu) {
-  Delegate delegate;
-  SimpleMenuModel model(&delegate);
-  model.AddItem(1, u"one");
-  model.AddItem(2, u"two");
-  model.AddItem(3, u"three");
-
-  MenuControllerCocoa* menu = [[MenuControllerCocoa alloc] initWithModel:&model
-                                                                delegate:nil
-                                                  useWithPopUpButtonCell:YES];
-  EXPECT_TRUE([menu isMenuBuiltForTesting]);
-}
-
 // Tests that Windows-style ampersand mnemonics are stripped by default, but
 // remain if the `MayHaveMnemonics` is false.
 TEST_F(MenuControllerTest, Ampersands) {
@@ -720,11 +650,10 @@ TEST_F(MenuControllerTest, Ampersands) {
   model.SetMayHaveMnemonicsAt(1, false);
 
   MenuControllerCocoa* menu = [[MenuControllerCocoa alloc] initWithModel:&model
-                                                                delegate:nil
-                                                  useWithPopUpButtonCell:NO];
+                                                                delegate:nil];
 
-  EXPECT_NSEQ([[[menu menu] itemAtIndex:0] title], @"New");
-  EXPECT_NSEQ([[[menu menu] itemAtIndex:1] title], @"Gin & Tonic");
+  EXPECT_NSEQ([menu.menu itemAtIndex:0].title, @"New");
+  EXPECT_NSEQ([menu.menu itemAtIndex:1].title, @"Gin & Tonic");
 }
 
 }  // namespace

ui/views/controls/menu/menu_runner_impl_cocoa.mm

@@ -81,8 +81,7 @@ void MenuRunnerImplCocoa::RunMenuAt(
   menu_delegate_ = [[MenuControllerCocoaDelegateImpl alloc]
       initWithParams:MenuControllerParamsForWidget(parent)];
   menu_controller_ = [[MenuControllerCocoa alloc] initWithModel:menu_model_
-                                                       delegate:menu_delegate_
-                                         useWithPopUpButtonCell:NO];
+                                                       delegate:menu_delegate_];
 
   closing_event_time_ = base::TimeTicks();
   running_ = true;