From 6243dba068dbe3c7d9e4ab6f884b06606ab4bc24 Mon Sep 17 00:00:00 2001 From: Shelley Vohr Date: Wed, 26 Jun 2019 10:18:53 -0700 Subject: [PATCH] chore: use ScopedPumpMessagesInPrivateModes in tray (#18977) * chore: use ScopedPumpMessagesInPrivateModes in tray * revert refcounting of AtomMenuModel * Prefer WeakPtr for posting tasks to handle unexpected destruction --- shell/browser/api/atom_api_menu.h | 2 +- shell/browser/ui/atom_menu_model.h | 7 ++----- shell/browser/ui/tray_icon_cocoa.h | 4 +++- shell/browser/ui/tray_icon_cocoa.mm | 18 ++++++++++-------- 4 files changed, 16 insertions(+), 15 deletions(-) diff --git a/shell/browser/api/atom_api_menu.h b/shell/browser/api/atom_api_menu.h index a473470f5613..351ce2e18557 100644 --- a/shell/browser/api/atom_api_menu.h +++ b/shell/browser/api/atom_api_menu.h @@ -63,7 +63,7 @@ class Menu : public mate::TrackableObject, const base::Closure& callback) = 0; virtual void ClosePopupAt(int32_t window_id) = 0; - scoped_refptr model_; + std::unique_ptr model_; Menu* parent_ = nullptr; // Observable: diff --git a/shell/browser/ui/atom_menu_model.h b/shell/browser/ui/atom_menu_model.h index 773bc8e679d9..f9341d8d9284 100644 --- a/shell/browser/ui/atom_menu_model.h +++ b/shell/browser/ui/atom_menu_model.h @@ -13,8 +13,7 @@ namespace electron { -class AtomMenuModel : public ui::SimpleMenuModel, - public base::RefCounted { +class AtomMenuModel : public ui::SimpleMenuModel { public: class Delegate : public ui::SimpleMenuModel::Delegate { public: @@ -49,6 +48,7 @@ class AtomMenuModel : public ui::SimpleMenuModel, }; explicit AtomMenuModel(Delegate* delegate); + ~AtomMenuModel() override; void AddObserver(Observer* obs) { observers_.AddObserver(obs); } void RemoveObserver(Observer* obs) { observers_.RemoveObserver(obs); } @@ -69,9 +69,6 @@ class AtomMenuModel : public ui::SimpleMenuModel, AtomMenuModel* GetSubmenuModelAt(int index); private: - friend class base::RefCounted; - ~AtomMenuModel() override; - Delegate* delegate_; // weak ref. std::map roles_; // command id -> role diff --git a/shell/browser/ui/tray_icon_cocoa.h b/shell/browser/ui/tray_icon_cocoa.h index 8d8f82174349..a158d796ced8 100644 --- a/shell/browser/ui/tray_icon_cocoa.h +++ b/shell/browser/ui/tray_icon_cocoa.h @@ -30,7 +30,7 @@ class TrayIconCocoa : public TrayIcon, public AtomMenuModel::Observer { void SetHighlightMode(TrayIcon::HighlightMode mode) override; void SetIgnoreDoubleClickEvents(bool ignore) override; bool GetIgnoreDoubleClickEvents() override; - void PopUpOnUI(scoped_refptr menu_model); + void PopUpOnUI(AtomMenuModel* menu_model); void PopUpContextMenu(const gfx::Point& pos, AtomMenuModel* menu_model) override; void SetContextMenu(AtomMenuModel* menu_model) override; @@ -50,6 +50,8 @@ class TrayIconCocoa : public TrayIcon, public AtomMenuModel::Observer { // Used for unregistering observer. AtomMenuModel* menu_model_ = nullptr; // weak ref. + base::WeakPtrFactory weak_factory_; + DISALLOW_COPY_AND_ASSIGN(TrayIconCocoa); }; diff --git a/shell/browser/ui/tray_icon_cocoa.mm b/shell/browser/ui/tray_icon_cocoa.mm index c5d5cafdbb8b..3c116eb7c122 100644 --- a/shell/browser/ui/tray_icon_cocoa.mm +++ b/shell/browser/ui/tray_icon_cocoa.mm @@ -337,7 +337,12 @@ const CGFloat kVerticalTitleMargin = 2; } - (void)popUpContextMenu:(electron::AtomMenuModel*)menu_model { + // Make sure events can be pumped while the menu is up. base::MessageLoopCurrent::ScopedNestableTaskAllower allow; + + // Ensure the UI can update while the menu is fading out. + base::ScopedPumpMessagesInPrivateModes pump_private; + // Show a custom menu. if (menu_model) { base::scoped_nsobject menuController( @@ -346,7 +351,6 @@ const CGFloat kVerticalTitleMargin = 2; forceHighlight_ = YES; // Should highlight when showing menu. [self setNeedsDisplay:YES]; - base::mac::ScopedSendingEvent sendingEventScoper; [statusItem_ popUpStatusItemMenu:[menuController menu]]; forceHighlight_ = NO; [self setNeedsDisplay:YES]; @@ -357,7 +361,6 @@ const CGFloat kVerticalTitleMargin = 2; // Redraw the tray icon to show highlight if it is enabled. [self setNeedsDisplay:YES]; - base::mac::ScopedSendingEvent sendingEventScoper; [statusItem_ popUpStatusItemMenu:[menuController_ menu]]; // The popUpStatusItemMenu returns only after the showing menu is closed. // When it returns, we need to redraw the tray icon to not show highlight. @@ -456,7 +459,7 @@ const CGFloat kVerticalTitleMargin = 2; namespace electron { -TrayIconCocoa::TrayIconCocoa() { +TrayIconCocoa::TrayIconCocoa() : weak_factory_(this) { status_item_view_.reset([[StatusItemView alloc] initWithIcon:this]); } @@ -498,17 +501,16 @@ bool TrayIconCocoa::GetIgnoreDoubleClickEvents() { return [status_item_view_ getIgnoreDoubleClickEvents]; } -void TrayIconCocoa::PopUpOnUI(scoped_refptr menu_model) { - if (auto* model = menu_model.get()) - [status_item_view_ popUpContextMenu:model]; +void TrayIconCocoa::PopUpOnUI(AtomMenuModel* menu_model) { + [status_item_view_ popUpContextMenu:menu_model]; } void TrayIconCocoa::PopUpContextMenu(const gfx::Point& pos, AtomMenuModel* menu_model) { base::PostTaskWithTraits( FROM_HERE, {content::BrowserThread::UI}, - base::BindOnce(&TrayIconCocoa::PopUpOnUI, base::Unretained(this), - base::WrapRefCounted(menu_model))); + base::BindOnce(&TrayIconCocoa::PopUpOnUI, weak_factory_.GetWeakPtr(), + base::Unretained(menu_model))); } void TrayIconCocoa::SetContextMenu(AtomMenuModel* menu_model) {