From bfb93881916578f73bf48c929ceb73a1b3b95710 Mon Sep 17 00:00:00 2001 From: Kevin Sawicki Date: Thu, 6 Apr 2017 15:02:32 -0700 Subject: [PATCH 1/5] Add failing spec for app.exit with >2 windows --- spec/api-app-spec.js | 10 ++++++++++ .../api/exit-closes-all-windows-app/main.js | 19 +++++++++++++++++++ .../exit-closes-all-windows-app/package.json | 4 ++++ 3 files changed, 33 insertions(+) create mode 100644 spec/fixtures/api/exit-closes-all-windows-app/main.js create mode 100644 spec/fixtures/api/exit-closes-all-windows-app/package.json diff --git a/spec/api-app-spec.js b/spec/api-app-spec.js index 301d6ae4bf47..9a9b4334bceb 100644 --- a/spec/api-app-spec.js +++ b/spec/api-app-spec.js @@ -137,6 +137,16 @@ describe('app module', function () { done() }) }) + + it('closes all windows', function (done) { + var appPath = path.join(__dirname, 'fixtures', 'api', 'exit-closes-all-windows-app') + var electronPath = remote.getGlobal('process').execPath + appProcess = ChildProcess.spawn(electronPath, [appPath]) + appProcess.on('close', function (code) { + assert.equal(code, 123) + done() + }) + }) }) describe('app.relaunch', function () { diff --git a/spec/fixtures/api/exit-closes-all-windows-app/main.js b/spec/fixtures/api/exit-closes-all-windows-app/main.js new file mode 100644 index 000000000000..c97d8d1f195b --- /dev/null +++ b/spec/fixtures/api/exit-closes-all-windows-app/main.js @@ -0,0 +1,19 @@ +const {app, BrowserWindow} = require('electron') + +const windows = [] + +function createWindow (id) { + const window = new BrowserWindow({show: false}) + window.loadURL(`data:,window${id}`) + windows.push(window) +} + +app.once('ready', () => { + for (let i = 1; i <= 5; i++) { + createWindow(i) + } + + setImmediate(function () { + app.exit(123) + }) +}) diff --git a/spec/fixtures/api/exit-closes-all-windows-app/package.json b/spec/fixtures/api/exit-closes-all-windows-app/package.json new file mode 100644 index 000000000000..ae52532315a3 --- /dev/null +++ b/spec/fixtures/api/exit-closes-all-windows-app/package.json @@ -0,0 +1,4 @@ +{ + "name": "electron-exit-closes-all-windows", + "main": "main.js" +} From 8311aa667c0bc637e9cf7ba72b1a8d32ee706991 Mon Sep 17 00:00:00 2001 From: Kevin Sawicki Date: Thu, 6 Apr 2017 14:25:22 -0700 Subject: [PATCH 2/5] Add DestroyAllWindows helper that uses vector copy --- atom/browser/browser.cc | 3 +-- atom/browser/window_list.cc | 7 +++++++ atom/browser/window_list.h | 3 +++ 3 files changed, 11 insertions(+), 2 deletions(-) diff --git a/atom/browser/browser.cc b/atom/browser/browser.cc index b18ab7b70fd7..1df1f316c1b8 100644 --- a/atom/browser/browser.cc +++ b/atom/browser/browser.cc @@ -71,8 +71,7 @@ void Browser::Exit(mate::Arguments* args) { } else { // Unlike Quit(), we do not ask to close window, but destroy the window // without asking. - for (NativeWindow* window : *window_list) - window->CloseContents(nullptr); // e.g. Destroy() + atom::WindowList::DestroyAllWindows(); } } } diff --git a/atom/browser/window_list.cc b/atom/browser/window_list.cc index d627f6d1206b..bcf4a9f1e2c0 100644 --- a/atom/browser/window_list.cc +++ b/atom/browser/window_list.cc @@ -76,6 +76,13 @@ void WindowList::CloseAllWindows() { window->Close(); } +// static +void WindowList::DestroyAllWindows() { + WindowVector windows = GetInstance()->windows_; + for (const auto& window : windows) + window->CloseContents(nullptr); // e.g. Destroy() +} + WindowList::WindowList() { } diff --git a/atom/browser/window_list.h b/atom/browser/window_list.h index d9b307352e90..557a7969e59a 100644 --- a/atom/browser/window_list.h +++ b/atom/browser/window_list.h @@ -51,6 +51,9 @@ class WindowList { // Closes all windows. static void CloseAllWindows(); + // Destroy all windows. + static void DestroyAllWindows(); + private: WindowList(); ~WindowList(); From 0883a9f96606cb54fd29693eab5d1ec3c573f7f6 Mon Sep 17 00:00:00 2001 From: Kevin Sawicki Date: Thu, 6 Apr 2017 14:37:39 -0700 Subject: [PATCH 3/5] Use CloseAllWindows helper --- atom/browser/api/atom_api_auto_updater.cc | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/atom/browser/api/atom_api_auto_updater.cc b/atom/browser/api/atom_api_auto_updater.cc index 276d889006d1..60bbcbda044f 100644 --- a/atom/browser/api/atom_api_auto_updater.cc +++ b/atom/browser/api/atom_api_auto_updater.cc @@ -95,8 +95,7 @@ void AutoUpdater::QuitAndInstall() { // Otherwise do the restart after all windows have been closed. window_list->AddObserver(this); - for (NativeWindow* window : *window_list) - window->Close(); + WindowList::CloseAllWindows(); } // static From da5d7d72b034c4bd7f7b1dbdd2d4f6f4c8cd0fb6 Mon Sep 17 00:00:00 2001 From: Kevin Sawicki Date: Thu, 6 Apr 2017 14:43:37 -0700 Subject: [PATCH 4/5] Add GetWindows helper that returns a vector --- atom/browser/browser_linux.cc | 4 +--- atom/browser/browser_mac.mm | 5 ++--- atom/browser/native_window.cc | 3 +-- atom/browser/window_list.cc | 5 +++++ atom/browser/window_list.h | 15 ++------------- 5 files changed, 11 insertions(+), 21 deletions(-) diff --git a/atom/browser/browser_linux.cc b/atom/browser/browser_linux.cc index 4dd799dfaf02..f569040a2189 100644 --- a/atom/browser/browser_linux.cc +++ b/atom/browser/browser_linux.cc @@ -16,9 +16,7 @@ namespace atom { void Browser::Focus() { // Focus on the first visible window. - WindowList* list = WindowList::GetInstance(); - for (WindowList::iterator iter = list->begin(); iter != list->end(); ++iter) { - NativeWindow* window = *iter; + for (const auto& window : WindowList::GetWindows()) { if (window->IsVisible()) { window->Focus(true); break; diff --git a/atom/browser/browser_mac.mm b/atom/browser/browser_mac.mm index 51604e377cd6..38a0a003d968 100644 --- a/atom/browser/browser_mac.mm +++ b/atom/browser/browser_mac.mm @@ -204,9 +204,8 @@ std::string Browser::DockGetBadgeText() { } void Browser::DockHide() { - WindowList* list = WindowList::GetInstance(); - for (WindowList::iterator it = list->begin(); it != list->end(); ++it) - [(*it)->GetNativeWindow() setCanHide:NO]; + for (const auto& window : WindowList::GetWindows()) + [window->GetNativeWindow() setCanHide:NO]; ProcessSerialNumber psn = { 0, kCurrentProcess }; TransformProcessType(&psn, kProcessTransformToUIElementApplication); diff --git a/atom/browser/native_window.cc b/atom/browser/native_window.cc index af32a2e8a85a..316cc8dc2e3d 100644 --- a/atom/browser/native_window.cc +++ b/atom/browser/native_window.cc @@ -104,8 +104,7 @@ NativeWindow::~NativeWindow() { // static NativeWindow* NativeWindow::FromWebContents( content::WebContents* web_contents) { - WindowList& window_list = *WindowList::GetInstance(); - for (NativeWindow* window : window_list) { + for (const auto& window : WindowList::GetWindows()) { if (window->web_contents() == web_contents) return window; } diff --git a/atom/browser/window_list.cc b/atom/browser/window_list.cc index bcf4a9f1e2c0..8fa2ed67438e 100644 --- a/atom/browser/window_list.cc +++ b/atom/browser/window_list.cc @@ -26,6 +26,11 @@ WindowList* WindowList::GetInstance() { return instance_; } +// static +WindowList::WindowVector WindowList::GetWindows() { + return GetInstance()->windows_; +} + // static void WindowList::AddWindow(NativeWindow* window) { DCHECK(window); diff --git a/atom/browser/window_list.h b/atom/browser/window_list.h index 557a7969e59a..81ff5de10573 100644 --- a/atom/browser/window_list.h +++ b/atom/browser/window_list.h @@ -19,24 +19,13 @@ class WindowListObserver; class WindowList { public: typedef std::vector WindowVector; - typedef WindowVector::iterator iterator; - typedef WindowVector::const_iterator const_iterator; - - // Windows are added to the list before they have constructed windows, - // so the |window()| member function may return NULL. - const_iterator begin() const { return windows_.begin(); } - const_iterator end() const { return windows_.end(); } - - iterator begin() { return windows_.begin(); } - iterator end() { return windows_.end(); } bool empty() const { return windows_.empty(); } - size_t size() const { return windows_.size(); } - - NativeWindow* get(size_t index) const { return windows_[index]; } static WindowList* GetInstance(); + static WindowVector GetWindows(); + // Adds or removes |window| from the list it is associated with. static void AddWindow(NativeWindow* window); static void RemoveWindow(NativeWindow* window); From e7b679ead64045846aa457a6af8bd251ffe239e3 Mon Sep 17 00:00:00 2001 From: Kevin Sawicki Date: Thu, 6 Apr 2017 14:48:58 -0700 Subject: [PATCH 5/5] Add IsEmpty helper and remove GetInstance public usage --- atom/browser/api/atom_api_auto_updater.cc | 5 ++--- atom/browser/browser.cc | 10 ++++------ atom/browser/window_list.cc | 5 +++++ atom/browser/window_list.h | 7 +++---- 4 files changed, 14 insertions(+), 13 deletions(-) diff --git a/atom/browser/api/atom_api_auto_updater.cc b/atom/browser/api/atom_api_auto_updater.cc index 60bbcbda044f..ea3024191e97 100644 --- a/atom/browser/api/atom_api_auto_updater.cc +++ b/atom/browser/api/atom_api_auto_updater.cc @@ -87,14 +87,13 @@ void AutoUpdater::SetFeedURL(const std::string& url, mate::Arguments* args) { void AutoUpdater::QuitAndInstall() { // If we don't have any window then quitAndInstall immediately. - WindowList* window_list = WindowList::GetInstance(); - if (window_list->empty()) { + if (WindowList::IsEmpty()) { auto_updater::AutoUpdater::QuitAndInstall(); return; } // Otherwise do the restart after all windows have been closed. - window_list->AddObserver(this); + WindowList::AddObserver(this); WindowList::CloseAllWindows(); } diff --git a/atom/browser/browser.cc b/atom/browser/browser.cc index 1df1f316c1b8..b2900a326ff3 100644 --- a/atom/browser/browser.cc +++ b/atom/browser/browser.cc @@ -43,11 +43,10 @@ void Browser::Quit() { if (!is_quiting_) return; - atom::WindowList* window_list = atom::WindowList::GetInstance(); - if (window_list->empty()) + if (atom::WindowList::IsEmpty()) NotifyAndShutdown(); - - window_list->CloseAllWindows(); + else + atom::WindowList::CloseAllWindows(); } void Browser::Exit(mate::Arguments* args) { @@ -65,8 +64,7 @@ void Browser::Exit(mate::Arguments* args) { is_exiting_ = true; // Must destroy windows before quitting, otherwise bad things can happen. - atom::WindowList* window_list = atom::WindowList::GetInstance(); - if (window_list->empty()) { + if (atom::WindowList::IsEmpty()) { Shutdown(); } else { // Unlike Quit(), we do not ask to close window, but destroy the window diff --git a/atom/browser/window_list.cc b/atom/browser/window_list.cc index 8fa2ed67438e..374389e0a786 100644 --- a/atom/browser/window_list.cc +++ b/atom/browser/window_list.cc @@ -31,6 +31,11 @@ WindowList::WindowVector WindowList::GetWindows() { return GetInstance()->windows_; } +// static +bool WindowList::IsEmpty() { + return GetInstance()->windows_.empty(); +} + // static void WindowList::AddWindow(NativeWindow* window) { DCHECK(window); diff --git a/atom/browser/window_list.h b/atom/browser/window_list.h index 81ff5de10573..e336c8073dc8 100644 --- a/atom/browser/window_list.h +++ b/atom/browser/window_list.h @@ -20,11 +20,8 @@ class WindowList { public: typedef std::vector WindowVector; - bool empty() const { return windows_.empty(); } - - static WindowList* GetInstance(); - static WindowVector GetWindows(); + static bool IsEmpty(); // Adds or removes |window| from the list it is associated with. static void AddWindow(NativeWindow* window); @@ -44,6 +41,8 @@ class WindowList { static void DestroyAllWindows(); private: + static WindowList* GetInstance(); + WindowList(); ~WindowList();