From dc62e51ba49b5d3b181e508778d71845085113ab Mon Sep 17 00:00:00 2001 From: Cheng Zhao Date: Wed, 21 Feb 2018 01:11:35 +0900 Subject: [PATCH] Fix the cyclic reference in menu delegate (#11967) * Fix the cyclic reference in menu delegate * Fix menu tests due to delegate change --- atom/browser/api/atom_api_menu.cc | 21 +++++++++--- atom/browser/api/atom_api_menu.h | 14 ++++---- atom/browser/api/event_emitter.h | 4 ++- lib/browser/api/menu.js | 53 ++++++++++++++++--------------- spec/api-menu-spec.js | 10 +++--- vendor/native_mate | 2 +- 6 files changed, 61 insertions(+), 43 deletions(-) diff --git a/atom/browser/api/atom_api_menu.cc b/atom/browser/api/atom_api_menu.cc index 651e2067e185..b27ee2ba7898 100644 --- a/atom/browser/api/atom_api_menu.cc +++ b/atom/browser/api/atom_api_menu.cc @@ -47,15 +47,21 @@ void Menu::AfterInit(v8::Isolate* isolate) { } bool Menu::IsCommandIdChecked(int command_id) const { - return is_checked_.Run(command_id); + v8::Locker locker(isolate()); + v8::HandleScope handle_scope(isolate()); + return is_checked_.Run(GetWrapper(), command_id); } bool Menu::IsCommandIdEnabled(int command_id) const { - return is_enabled_.Run(command_id); + v8::Locker locker(isolate()); + v8::HandleScope handle_scope(isolate()); + return is_enabled_.Run(GetWrapper(), command_id); } bool Menu::IsCommandIdVisible(int command_id) const { - return is_visible_.Run(command_id); + v8::Locker locker(isolate()); + v8::HandleScope handle_scope(isolate()); + return is_visible_.Run(GetWrapper(), command_id); } bool Menu::GetAcceleratorForCommandIdWithParams( @@ -65,18 +71,23 @@ bool Menu::GetAcceleratorForCommandIdWithParams( v8::Locker locker(isolate()); v8::HandleScope handle_scope(isolate()); v8::Local val = get_accelerator_.Run( - command_id, use_default_accelerator); + GetWrapper(), command_id, use_default_accelerator); return mate::ConvertFromV8(isolate(), val, accelerator); } void Menu::ExecuteCommand(int command_id, int flags) { + v8::Locker locker(isolate()); + v8::HandleScope handle_scope(isolate()); execute_command_.Run( + GetWrapper(), mate::internal::CreateEventFromFlags(isolate(), flags), command_id); } void Menu::MenuWillShow(ui::SimpleMenuModel* source) { - menu_will_show_.Run(); + v8::Locker locker(isolate()); + v8::HandleScope handle_scope(isolate()); + menu_will_show_.Run(GetWrapper()); } void Menu::InsertItemAt( diff --git a/atom/browser/api/atom_api_menu.h b/atom/browser/api/atom_api_menu.h index aafd8f893967..c7c114e12113 100644 --- a/atom/browser/api/atom_api_menu.h +++ b/atom/browser/api/atom_api_menu.h @@ -93,12 +93,14 @@ class Menu : public mate::TrackableObject, bool IsVisibleAt(int index) const; // Stored delegate methods. - base::Callback is_checked_; - base::Callback is_enabled_; - base::Callback is_visible_; - base::Callback(int, bool)> get_accelerator_; - base::Callback, int)> execute_command_; - base::Callback menu_will_show_; + base::Callback, int)> is_checked_; + base::Callback, int)> is_enabled_; + base::Callback, int)> is_visible_; + base::Callback(v8::Local, int, bool)> + get_accelerator_; + base::Callback, v8::Local, int)> + execute_command_; + base::Callback)> menu_will_show_; DISALLOW_COPY_AND_ASSIGN(Menu); }; diff --git a/atom/browser/api/event_emitter.h b/atom/browser/api/event_emitter.h index f5a8025e860e..8d9d2960df35 100644 --- a/atom/browser/api/event_emitter.h +++ b/atom/browser/api/event_emitter.h @@ -42,8 +42,10 @@ class EventEmitter : public Wrappable { // Make the convinient methods visible: // https://isocpp.org/wiki/faq/templates#nondependent-name-lookup-members - v8::Local GetWrapper() { return Wrappable::GetWrapper(); } v8::Isolate* isolate() const { return Wrappable::isolate(); } + v8::Local GetWrapper() const { + return Wrappable::GetWrapper(); + } // this.emit(name, event, args...); template diff --git a/lib/browser/api/menu.js b/lib/browser/api/menu.js index e599cfb3c06c..438a8bd587ba 100644 --- a/lib/browser/api/menu.js +++ b/lib/browser/api/menu.js @@ -11,36 +11,39 @@ let groupIdIndex = 0 Object.setPrototypeOf(Menu.prototype, EventEmitter.prototype) +// Menu Delegate. +// This object should hold no reference to |Menu| to avoid cyclic reference. +const delegate = { + isCommandIdChecked: (menu, id) => menu.commandsMap[id] ? menu.commandsMap[id].checked : undefined, + isCommandIdEnabled: (menu, id) => menu.commandsMap[id] ? menu.commandsMap[id].enabled : undefined, + isCommandIdVisible: (menu, id) => menu.commandsMap[id] ? menu.commandsMap[id].visible : undefined, + getAcceleratorForCommandId: (menu, id, useDefaultAccelerator) => { + const command = menu.commandsMap[id] + if (!command) return + if (command.accelerator) return command.accelerator + if (useDefaultAccelerator) return command.getDefaultRoleAccelerator() + }, + executeCommand: (menu, event, id) => { + const command = menu.commandsMap[id] + if (!command) return + command.click(event, BrowserWindow.getFocusedWindow(), webContents.getFocusedWebContents()) + }, + menuWillShow: (menu) => { + // Ensure radio groups have at least one menu item seleted + for (const id in menu.groupsMap) { + const found = menu.groupsMap[id].find(item => item.checked) || null + if (!found) v8Util.setHiddenValue(menu.groupsMap[id][0], 'checked', true) + } + } +} + /* Instance Methods */ Menu.prototype._init = function () { this.commandsMap = {} this.groupsMap = {} this.items = [] - this.delegate = { - isCommandIdChecked: id => this.commandsMap[id] ? this.commandsMap[id].checked : undefined, - isCommandIdEnabled: id => this.commandsMap[id] ? this.commandsMap[id].enabled : undefined, - isCommandIdVisible: id => this.commandsMap[id] ? this.commandsMap[id].visible : undefined, - getAcceleratorForCommandId: (id, useDefaultAccelerator) => { - const command = this.commandsMap[id] - if (!command) return - if (command.accelerator) return command.accelerator - if (useDefaultAccelerator) return command.getDefaultRoleAccelerator() - }, - getIconForCommandId: id => this.commandsMap[id] ? this.commandsMap[id].icon : undefined, - executeCommand: (event, id) => { - const command = this.commandsMap[id] - if (!command) return - command.click(event, BrowserWindow.getFocusedWindow(), webContents.getFocusedWebContents()) - }, - menuWillShow: () => { - // Ensure radio groups have at least one menu item seleted - for (const id in this.groupsMap) { - const found = this.groupsMap[id].find(item => item.checked) || null - if (!found) v8Util.setHiddenValue(this.groupsMap[id][0], 'checked', true) - } - } - } + this.delegate = delegate } Menu.prototype.popup = function (window, x, y, positioningItem) { @@ -150,7 +153,7 @@ Menu.prototype.insert = function (pos, item) { } Menu.prototype._callMenuWillShow = function () { - if (this.delegate) this.delegate.menuWillShow() + if (this.delegate) this.delegate.menuWillShow(this) this.items.forEach(item => { if (item.submenu) item.submenu._callMenuWillShow() }) diff --git a/spec/api-menu-spec.js b/spec/api-menu-spec.js index 985f00bcf666..b34c35d5668c 100644 --- a/spec/api-menu-spec.js +++ b/spec/api-menu-spec.js @@ -430,7 +430,7 @@ describe('Menu module', () => { } } ]) - menu.delegate.executeCommand({}, menu.items[0].commandId) + menu.delegate.executeCommand(menu, {}, menu.items[0].commandId) }) }) @@ -443,7 +443,7 @@ describe('Menu module', () => { } ]) assert.equal(menu.items[0].checked, false) - menu.delegate.executeCommand({}, menu.items[0].commandId) + menu.delegate.executeCommand(menu, {}, menu.items[0].commandId) assert.equal(menu.items[0].checked, true) }) @@ -454,9 +454,9 @@ describe('Menu module', () => { type: 'radio' } ]) - menu.delegate.executeCommand({}, menu.items[0].commandId) + menu.delegate.executeCommand(menu, {}, menu.items[0].commandId) assert.equal(menu.items[0].checked, true) - menu.delegate.executeCommand({}, menu.items[0].commandId) + menu.delegate.executeCommand(menu, {}, menu.items[0].commandId) assert.equal(menu.items[0].checked, true) }) @@ -476,7 +476,7 @@ describe('Menu module', () => { }) } const menu = Menu.buildFromTemplate(template) - menu.delegate.menuWillShow() + menu.delegate.menuWillShow(menu) assert.equal(menu.items[0].checked, true) assert.equal(menu.items[12].checked, true) }) diff --git a/vendor/native_mate b/vendor/native_mate index a38fb5df41be..99d9e262eb36 160000 --- a/vendor/native_mate +++ b/vendor/native_mate @@ -1 +1 @@ -Subproject commit a38fb5df41be48fa98330f54033ff14f1558bac5 +Subproject commit 99d9e262eb36cb9dc8e83f61e026d2a7ad1e96ab