From cfed9fa4b9d2476075e9af11afeb9ae02d30d5d3 Mon Sep 17 00:00:00 2001 From: Cheng Zhao Date: Tue, 17 Apr 2018 16:18:55 +0900 Subject: [PATCH 1/5] Make sure we append parent->child_windows after InitWith --- atom/browser/api/atom_api_top_level_window.cc | 22 +++++++++++++------ atom/browser/api/atom_api_top_level_window.h | 3 +++ 2 files changed, 18 insertions(+), 7 deletions(-) diff --git a/atom/browser/api/atom_api_top_level_window.cc b/atom/browser/api/atom_api_top_level_window.cc index 735835c87c..dd0c06af2c 100644 --- a/atom/browser/api/atom_api_top_level_window.cc +++ b/atom/browser/api/atom_api_top_level_window.cc @@ -99,13 +99,6 @@ TopLevelWindow::TopLevelWindow(v8::Isolate* isolate, if (options.Get(options::kIcon, &icon) && !icon.IsEmpty()) SetIcon(icon); #endif - - AttachAsUserData(window_.get()); - - // We can only append this window to parent window's child windows after this - // window's JS wrapper gets initialized. - if (!parent.IsEmpty()) - parent->child_windows_.Set(isolate, weak_map_id(), wrapper); } TopLevelWindow::~TopLevelWindow() { @@ -117,6 +110,21 @@ TopLevelWindow::~TopLevelWindow() { base::ThreadTaskRunnerHandle::Get()->DeleteSoon(FROM_HERE, window_.release()); } +void TopLevelWindow::InitWith(v8::Isolate* isolate, + v8::Local wrapper) { + AttachAsUserData(window_.get()); + mate::TrackableObject::InitWith(isolate, wrapper); + + // We can only append this window to parent window's child windows after this + // window's JS wrapper gets initialized. + if (!parent_window_.IsEmpty()) { + mate::Handle parent; + mate::ConvertFromV8(isolate, GetParentWindow(), &parent); + DCHECK(!parent.IsEmpty()); + parent->child_windows_.Set(isolate, weak_map_id(), wrapper); + } +} + void TopLevelWindow::WillCloseWindow(bool* prevent_default) { *prevent_default = Emit("close"); } diff --git a/atom/browser/api/atom_api_top_level_window.h b/atom/browser/api/atom_api_top_level_window.h index ef140f6e8b..6082735a8c 100644 --- a/atom/browser/api/atom_api_top_level_window.h +++ b/atom/browser/api/atom_api_top_level_window.h @@ -40,6 +40,9 @@ class TopLevelWindow : public mate::TrackableObject, const mate::Dictionary& options); ~TopLevelWindow() override; + // TrackableObject: + void InitWith(v8::Isolate* isolate, v8::Local wrapper) override; + // NativeWindowObserver: void WillCloseWindow(bool* prevent_default) override; void OnWindowClosed() override; From 71ebd99dfa0400879509b9bef75c0d9de1a0cf23 Mon Sep 17 00:00:00 2001 From: Cheng Zhao Date: Tue, 17 Apr 2018 16:29:16 +0900 Subject: [PATCH 2/5] Expose TopLevelWindow to JavaScript --- atom/browser/api/atom_api_browser_window.cc | 2 +- atom/browser/api/atom_api_top_level_window.cc | 10 +++++++++- atom/browser/api/atom_api_top_level_window.h | 3 +++ filenames.gypi | 1 + lib/browser/api/module-list.js | 1 + lib/browser/api/top-level-window.js | 9 +++++++++ 6 files changed, 24 insertions(+), 2 deletions(-) create mode 100644 lib/browser/api/top-level-window.js diff --git a/atom/browser/api/atom_api_browser_window.cc b/atom/browser/api/atom_api_browser_window.cc index b137ec092e..e662b487d9 100644 --- a/atom/browser/api/atom_api_browser_window.cc +++ b/atom/browser/api/atom_api_browser_window.cc @@ -29,7 +29,7 @@ namespace api { BrowserWindow::BrowserWindow(v8::Isolate* isolate, v8::Local wrapper, const mate::Dictionary& options) - : TopLevelWindow(isolate, wrapper, options), weak_factory_(this) { + : TopLevelWindow(isolate, options), weak_factory_(this) { mate::Handle web_contents; // Use options.webPreferences in WebContents. diff --git a/atom/browser/api/atom_api_top_level_window.cc b/atom/browser/api/atom_api_top_level_window.cc index dd0c06af2c..0ee8c228ed 100644 --- a/atom/browser/api/atom_api_top_level_window.cc +++ b/atom/browser/api/atom_api_top_level_window.cc @@ -70,7 +70,6 @@ v8::Local ToBuffer(v8::Isolate* isolate, void* val, int size) { } // namespace TopLevelWindow::TopLevelWindow(v8::Isolate* isolate, - v8::Local wrapper, const mate::Dictionary& options) : weak_factory_(this) { // The parent window. @@ -101,6 +100,15 @@ TopLevelWindow::TopLevelWindow(v8::Isolate* isolate, #endif } +TopLevelWindow::TopLevelWindow(v8::Isolate* isolate, + v8::Local wrapper, + const mate::Dictionary& options) + : TopLevelWindow(isolate, options) { + InitWith(isolate, wrapper); + // Init window after everything has been setup. + window()->InitFromOptions(options); +} + TopLevelWindow::~TopLevelWindow() { if (!window_->IsClosed()) window_->CloseImmediately(); diff --git a/atom/browser/api/atom_api_top_level_window.h b/atom/browser/api/atom_api_top_level_window.h index 6082735a8c..2022bca628 100644 --- a/atom/browser/api/atom_api_top_level_window.h +++ b/atom/browser/api/atom_api_top_level_window.h @@ -35,6 +35,9 @@ class TopLevelWindow : public mate::TrackableObject, NativeWindow* window() const { return window_.get(); } protected: + // Common constructor. + TopLevelWindow(v8::Isolate* isolate, const mate::Dictionary& options); + // Creating independent TopLevelWindow instance. TopLevelWindow(v8::Isolate* isolate, v8::Local wrapper, const mate::Dictionary& options); diff --git a/filenames.gypi b/filenames.gypi index 8b8dd7690b..3e0cb381fb 100644 --- a/filenames.gypi +++ b/filenames.gypi @@ -34,6 +34,7 @@ 'lib/browser/api/screen.js', 'lib/browser/api/session.js', 'lib/browser/api/system-preferences.js', + 'lib/browser/api/top-level-window.js', 'lib/browser/api/touch-bar.js', 'lib/browser/api/tray.js', 'lib/browser/api/web-contents.js', diff --git a/lib/browser/api/module-list.js b/lib/browser/api/module-list.js index e6f398b47d..68f41b491b 100644 --- a/lib/browser/api/module-list.js +++ b/lib/browser/api/module-list.js @@ -19,6 +19,7 @@ module.exports = [ {name: 'screen', file: 'screen'}, {name: 'session', file: 'session'}, {name: 'systemPreferences', file: 'system-preferences'}, + {name: 'TopLevelWindow', file: 'top-level-window'}, {name: 'TouchBar', file: 'touch-bar'}, {name: 'Tray', file: 'tray'}, {name: 'webContents', file: 'web-contents'}, diff --git a/lib/browser/api/top-level-window.js b/lib/browser/api/top-level-window.js new file mode 100644 index 0000000000..d913c3585c --- /dev/null +++ b/lib/browser/api/top-level-window.js @@ -0,0 +1,9 @@ +'use strict' + +const {EventEmitter} = require('events') +const {TopLevelWindow} = process.atomBinding('top_level_window') +const v8Util = process.atomBinding('v8_util') + +Object.setPrototypeOf(TopLevelWindow.prototype, EventEmitter.prototype) + +module.exports = TopLevelWindow From e38f511737e491eedc59e41eed83ebfebac61a42 Mon Sep 17 00:00:00 2001 From: Cheng Zhao Date: Tue, 17 Apr 2018 16:46:32 +0900 Subject: [PATCH 3/5] Make BrowserWindow inheirt TopLevelWindow in JS --- atom/browser/api/atom_api_browser_window.cc | 1 - lib/browser/api/browser-window.js | 16 ++++++---------- lib/browser/api/top-level-window.js | 13 ++++++++++++- 3 files changed, 18 insertions(+), 12 deletions(-) diff --git a/atom/browser/api/atom_api_browser_window.cc b/atom/browser/api/atom_api_browser_window.cc index e662b487d9..b82c050970 100644 --- a/atom/browser/api/atom_api_browser_window.cc +++ b/atom/browser/api/atom_api_browser_window.cc @@ -397,7 +397,6 @@ mate::WrappableBase* BrowserWindow::New(mate::Arguments* args) { // static void BrowserWindow::BuildPrototype(v8::Isolate* isolate, v8::Local prototype) { - TopLevelWindow::BuildPrototype(isolate, prototype); prototype->SetClassName(mate::StringToV8(isolate, "BrowserWindow")); mate::ObjectTemplateBuilder(isolate, prototype->PrototypeTemplate()) .SetMethod("focusOnWebView", &BrowserWindow::FocusOnWebView) diff --git a/lib/browser/api/browser-window.js b/lib/browser/api/browser-window.js index 6b14eb84d6..cbf5ab857c 100644 --- a/lib/browser/api/browser-window.js +++ b/lib/browser/api/browser-window.js @@ -1,22 +1,18 @@ 'use strict' const electron = require('electron') -const {ipcMain} = electron -const {EventEmitter} = require('events') +const {ipcMain, TopLevelWindow} = electron const {BrowserWindow} = process.atomBinding('window') const v8Util = process.atomBinding('v8_util') -Object.setPrototypeOf(BrowserWindow.prototype, EventEmitter.prototype) +Object.setPrototypeOf(BrowserWindow.prototype, TopLevelWindow.prototype) BrowserWindow.prototype._init = function () { - // Avoid recursive require. - const {app} = require('electron') + // Call parent class's _init. + TopLevelWindow.prototype._init.call(this) - // Simulate the application menu on platforms other than macOS. - if (process.platform !== 'darwin') { - const menu = app.getApplicationMenu() - if (menu) this.setMenu(menu) - } + // Avoid recursive require. + const {app} = electron // Make new windows requested by links behave like "window.open" this.webContents.on('-new-window', (event, url, frameName, disposition, diff --git a/lib/browser/api/top-level-window.js b/lib/browser/api/top-level-window.js index d913c3585c..9745fec9b7 100644 --- a/lib/browser/api/top-level-window.js +++ b/lib/browser/api/top-level-window.js @@ -1,9 +1,20 @@ 'use strict' +const electron = require('electron') const {EventEmitter} = require('events') const {TopLevelWindow} = process.atomBinding('top_level_window') -const v8Util = process.atomBinding('v8_util') Object.setPrototypeOf(TopLevelWindow.prototype, EventEmitter.prototype) +TopLevelWindow.prototype._init = function () { + // Avoid recursive require. + const {app} = electron + + // Simulate the application menu on platforms other than macOS. + if (process.platform !== 'darwin') { + const menu = app.getApplicationMenu() + if (menu) this.setMenu(menu) + } +} + module.exports = TopLevelWindow From 1340b174242c82197d6435b71204b5438db9a11d Mon Sep 17 00:00:00 2001 From: Cheng Zhao Date: Tue, 17 Apr 2018 16:53:55 +0900 Subject: [PATCH 4/5] Do not return TopLevelWindow in BrowserWindow.getAllWindows --- atom/browser/api/atom_api_browser_window.cc | 8 +------- lib/browser/api/browser-window.js | 13 +++++++++++++ 2 files changed, 14 insertions(+), 7 deletions(-) diff --git a/atom/browser/api/atom_api_browser_window.cc b/atom/browser/api/atom_api_browser_window.cc index b82c050970..bb8247ec96 100644 --- a/atom/browser/api/atom_api_browser_window.cc +++ b/atom/browser/api/atom_api_browser_window.cc @@ -438,14 +438,8 @@ void Initialize(v8::Local exports, templ->InstanceTemplate()->SetInternalFieldCount(1); BrowserWindow::BuildPrototype(isolate, templ); - mate::Dictionary browser_window(isolate, templ->GetFunction()); - browser_window.SetMethod( - "fromId", &mate::TrackableObject::FromWeakMapID); - browser_window.SetMethod("getAllWindows", - &mate::TrackableObject::GetAll); - mate::Dictionary dict(isolate, exports); - dict.Set("BrowserWindow", browser_window); + dict.Set("BrowserWindow", templ->GetFunction()); } } // namespace diff --git a/lib/browser/api/browser-window.js b/lib/browser/api/browser-window.js index cbf5ab857c..924a99c4be 100644 --- a/lib/browser/api/browser-window.js +++ b/lib/browser/api/browser-window.js @@ -128,6 +128,19 @@ BrowserWindow.prototype._init = function () { }) } +const isBrowserWindow = (win) => { + return win && win.constructor.name === 'BrowserWindow' +} + +BrowserWindow.fromId = (id) => { + const win = TopLevelWindow.fromId(id) + return isBrowserWindow(win) ? win : null +} + +BrowserWindow.getAllWindows = () => { + return TopLevelWindow.getAllWindows().filter(isBrowserWindow) +} + BrowserWindow.getFocusedWindow = () => { for (let window of BrowserWindow.getAllWindows()) { if (window.isFocused() || window.isDevToolsFocused()) return window From 7473b612c5602bc580c04cc14100117874f12f76 Mon Sep 17 00:00:00 2001 From: Cheng Zhao Date: Tue, 17 Apr 2018 17:14:49 +0900 Subject: [PATCH 5/5] Make Menu API accept TopLevelWindow --- atom/browser/api/atom_api_browser_window.h | 23 ------------------- atom/browser/api/atom_api_menu.h | 4 ++-- atom/browser/api/atom_api_menu_mac.h | 2 +- atom/browser/api/atom_api_menu_mac.mm | 4 +--- atom/browser/api/atom_api_menu_views.cc | 4 +--- atom/browser/api/atom_api_menu_views.h | 2 +- atom/browser/api/atom_api_top_level_window.cc | 1 + atom/browser/api/atom_api_top_level_window.h | 23 +++++++++++++++++++ lib/browser/api/menu.js | 14 +++++------ lib/browser/api/top-level-window.js | 4 ++++ 10 files changed, 41 insertions(+), 40 deletions(-) diff --git a/atom/browser/api/atom_api_browser_window.h b/atom/browser/api/atom_api_browser_window.h index 7b1307922c..5f9610f3a3 100644 --- a/atom/browser/api/atom_api_browser_window.h +++ b/atom/browser/api/atom_api_browser_window.h @@ -118,27 +118,4 @@ class BrowserWindow : public TopLevelWindow, } // namespace atom -namespace mate { - -template <> -struct Converter { - static bool FromV8(v8::Isolate* isolate, - v8::Local val, - atom::NativeWindow** out) { - // null would be tranfered to NULL. - if (val->IsNull()) { - *out = NULL; - return true; - } - - atom::api::BrowserWindow* window; - if (!Converter::FromV8(isolate, val, &window)) - return false; - *out = window->window(); - return true; - } -}; - -} // namespace mate - #endif // ATOM_BROWSER_API_ATOM_API_BROWSER_WINDOW_H_ diff --git a/atom/browser/api/atom_api_menu.h b/atom/browser/api/atom_api_menu.h index 2770742ce2..53c7927689 100644 --- a/atom/browser/api/atom_api_menu.h +++ b/atom/browser/api/atom_api_menu.h @@ -8,7 +8,7 @@ #include #include -#include "atom/browser/api/atom_api_browser_window.h" +#include "atom/browser/api/atom_api_top_level_window.h" #include "atom/browser/api/trackable_object.h" #include "atom/browser/ui/atom_menu_model.h" #include "base/callback.h" @@ -54,7 +54,7 @@ class Menu : public mate::TrackableObject, void ExecuteCommand(int command_id, int event_flags) override; void MenuWillShow(ui::SimpleMenuModel* source) override; - virtual void PopupAt(BrowserWindow* window, + virtual void PopupAt(TopLevelWindow* window, int x, int y, int positioning_item, diff --git a/atom/browser/api/atom_api_menu_mac.h b/atom/browser/api/atom_api_menu_mac.h index 5b4816ea84..fb575cfcba 100644 --- a/atom/browser/api/atom_api_menu_mac.h +++ b/atom/browser/api/atom_api_menu_mac.h @@ -22,7 +22,7 @@ class MenuMac : public Menu { protected: MenuMac(v8::Isolate* isolate, v8::Local wrapper); - void PopupAt(BrowserWindow* window, + void PopupAt(TopLevelWindow* window, int x, int y, int positioning_item, diff --git a/atom/browser/api/atom_api_menu_mac.mm b/atom/browser/api/atom_api_menu_mac.mm index cc08fa5315..fe077e3476 100644 --- a/atom/browser/api/atom_api_menu_mac.mm +++ b/atom/browser/api/atom_api_menu_mac.mm @@ -9,8 +9,6 @@ #include "base/mac/scoped_sending_event.h" #include "base/message_loop/message_loop.h" #include "base/strings/sys_string_conversions.h" -#include "brightray/browser/inspectable_web_contents.h" -#include "brightray/browser/inspectable_web_contents_view.h" #include "content/public/browser/browser_thread.h" #include "content/public/browser/web_contents.h" @@ -27,7 +25,7 @@ MenuMac::MenuMac(v8::Isolate* isolate, v8::Local wrapper) weak_factory_(this) { } -void MenuMac::PopupAt(BrowserWindow* window, +void MenuMac::PopupAt(TopLevelWindow* window, int x, int y, int positioning_item, const base::Closure& callback) { NativeWindow* native_window = window->window(); diff --git a/atom/browser/api/atom_api_menu_views.cc b/atom/browser/api/atom_api_menu_views.cc index 52d2fb726c..1060964a29 100644 --- a/atom/browser/api/atom_api_menu_views.cc +++ b/atom/browser/api/atom_api_menu_views.cc @@ -6,8 +6,6 @@ #include "atom/browser/native_window_views.h" #include "atom/browser/unresponsive_suppressor.h" -#include "brightray/browser/inspectable_web_contents.h" -#include "brightray/browser/inspectable_web_contents_view.h" #include "ui/display/screen.h" using views::MenuRunner; @@ -19,7 +17,7 @@ namespace api { MenuViews::MenuViews(v8::Isolate* isolate, v8::Local wrapper) : Menu(isolate, wrapper), weak_factory_(this) {} -void MenuViews::PopupAt(BrowserWindow* window, +void MenuViews::PopupAt(TopLevelWindow* window, int x, int y, int positioning_item, diff --git a/atom/browser/api/atom_api_menu_views.h b/atom/browser/api/atom_api_menu_views.h index 31d7d48db8..e5b93a3cfa 100644 --- a/atom/browser/api/atom_api_menu_views.h +++ b/atom/browser/api/atom_api_menu_views.h @@ -21,7 +21,7 @@ class MenuViews : public Menu { MenuViews(v8::Isolate* isolate, v8::Local wrapper); protected: - void PopupAt(BrowserWindow* window, + void PopupAt(TopLevelWindow* window, int x, int y, int positioning_item, diff --git a/atom/browser/api/atom_api_top_level_window.cc b/atom/browser/api/atom_api_top_level_window.cc index 0ee8c228ed..87cc03540e 100644 --- a/atom/browser/api/atom_api_top_level_window.cc +++ b/atom/browser/api/atom_api_top_level_window.cc @@ -9,6 +9,7 @@ #include "atom/browser/api/atom_api_browser_view.h" #include "atom/browser/api/atom_api_menu.h" +#include "atom/browser/api/atom_api_web_contents.h" #include "atom/common/color_util.h" #include "atom/common/native_mate_converters/callback.h" #include "atom/common/native_mate_converters/file_path_converter.h" diff --git a/atom/browser/api/atom_api_top_level_window.h b/atom/browser/api/atom_api_top_level_window.h index 2022bca628..caba53c0e0 100644 --- a/atom/browser/api/atom_api_top_level_window.h +++ b/atom/browser/api/atom_api_top_level_window.h @@ -229,4 +229,27 @@ class TopLevelWindow : public mate::TrackableObject, } // namespace atom +namespace mate { + +template<> +struct Converter { + static bool FromV8(v8::Isolate* isolate, + v8::Local val, + atom::NativeWindow** out) { + // null would be tranfered to NULL. + if (val->IsNull()) { + *out = NULL; + return true; + } + + atom::api::TopLevelWindow* window; + if (!Converter::FromV8(isolate, val, &window)) + return false; + *out = window->window(); + return true; + } +}; + +} // namespace mate + #endif // ATOM_BROWSER_API_ATOM_API_TOP_LEVEL_WINDOW_H_ diff --git a/lib/browser/api/menu.js b/lib/browser/api/menu.js index eab8184efe..ddce950290 100644 --- a/lib/browser/api/menu.js +++ b/lib/browser/api/menu.js @@ -1,6 +1,6 @@ 'use strict' -const {BrowserWindow, MenuItem, webContents} = require('electron') +const {TopLevelWindow, MenuItem, webContents} = require('electron') const EventEmitter = require('events').EventEmitter const v8Util = process.atomBinding('v8_util') const bindings = process.atomBinding('menu') @@ -26,7 +26,7 @@ const delegate = { executeCommand: (menu, event, id) => { const command = menu.commandsMap[id] if (!command) return - command.click(event, BrowserWindow.getFocusedWindow(), webContents.getFocusedWebContents()) + command.click(event, TopLevelWindow.getFocusedWindow(), webContents.getFocusedWebContents()) }, menuWillShow: (menu) => { // Ensure radio groups have at least one menu item seleted @@ -61,14 +61,14 @@ Menu.prototype.popup = function (options) { if (typeof positioningItem !== 'number') positioningItem = -1 // find which window to use - const wins = BrowserWindow.getAllWindows() + const wins = TopLevelWindow.getAllWindows() if (!wins || wins.indexOf(window) === -1) { - window = BrowserWindow.getFocusedWindow() + window = TopLevelWindow.getFocusedWindow() if (!window && wins && wins.length > 0) { window = wins[0] } if (!window) { - throw new Error(`Cannot open Menu without a BrowserWindow present`) + throw new Error(`Cannot open Menu without a TopLevelWindow present`) } } @@ -77,7 +77,7 @@ Menu.prototype.popup = function (options) { } Menu.prototype.closePopup = function (window) { - if (window && window.constructor !== BrowserWindow) { + if (window instanceof TopLevelWindow) { this.closePopupAt(window.id) } else { // Passing -1 (invalid) would make closePopupAt close the all menu runners @@ -148,7 +148,7 @@ Menu.setApplicationMenu = function (menu) { menu._callMenuWillShow() bindings.setApplicationMenu(menu) } else { - const windows = BrowserWindow.getAllWindows() + const windows = TopLevelWindow.getAllWindows() return windows.map(w => w.setMenu(menu)) } } diff --git a/lib/browser/api/top-level-window.js b/lib/browser/api/top-level-window.js index 9745fec9b7..d798314e96 100644 --- a/lib/browser/api/top-level-window.js +++ b/lib/browser/api/top-level-window.js @@ -17,4 +17,8 @@ TopLevelWindow.prototype._init = function () { } } +TopLevelWindow.getFocusedWindow = () => { + return TopLevelWindow.getAllWindows().find((win) => win.isFocused()) +} + module.exports = TopLevelWindow