From e84d7c0cdabe376208b7c9903b1306fa0f7125d0 Mon Sep 17 00:00:00 2001 From: Shelley Vohr Date: Mon, 26 Feb 2018 14:47:36 -0800 Subject: [PATCH 1/6] add warning when addTabbedWindow is called on self --- atom/browser/native_window_mac.mm | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/atom/browser/native_window_mac.mm b/atom/browser/native_window_mac.mm index 72dd45728f..30cc66d6f2 100644 --- a/atom/browser/native_window_mac.mm +++ b/atom/browser/native_window_mac.mm @@ -1672,8 +1672,12 @@ void NativeWindowMac::ToggleTabBar() { } void NativeWindowMac::AddTabbedWindow(NativeWindow* window) { - if ([window_ respondsToSelector:@selector(addTabbedWindow:ordered:)]) { - [window_ addTabbedWindow:window->GetNativeWindow() ordered:NSWindowAbove]; + if (window_.get() == window->GetNativeWindow()) { + NSLog(@"Error: AddTabbedWindow cannot be called by a window on itself."); + } else { + if ([window_ respondsToSelector:@selector(addTabbedWindow:ordered:)]) { + [window_ addTabbedWindow:window->GetNativeWindow() ordered:NSWindowAbove]; + } } } From 5336b4a89c33cf5de5ef0b6347111b523c13c62c Mon Sep 17 00:00:00 2001 From: Samuel Attard Date: Tue, 27 Feb 2018 16:11:54 +1100 Subject: [PATCH 2/6] Pass arguments instance through the chain in order to throw error --- atom/browser/api/atom_api_browser_window.cc | 9 +++++++-- atom/browser/api/atom_api_browser_window.h | 2 +- atom/browser/native_window.cc | 2 +- atom/browser/native_window.h | 2 +- atom/browser/native_window_mac.h | 2 +- atom/browser/native_window_mac.mm | 5 +++-- 6 files changed, 14 insertions(+), 8 deletions(-) diff --git a/atom/browser/api/atom_api_browser_window.cc b/atom/browser/api/atom_api_browser_window.cc index 6a6e4f02c4..0812c61f55 100644 --- a/atom/browser/api/atom_api_browser_window.cc +++ b/atom/browser/api/atom_api_browser_window.cc @@ -1082,8 +1082,13 @@ void BrowserWindow::ToggleTabBar() { window_->ToggleTabBar(); } -void BrowserWindow::AddTabbedWindow(NativeWindow* window) { - window_->AddTabbedWindow(window); +void BrowserWindow::AddTabbedWindow(mate::Arguments* args) { + NativeWindow* window; + if (!args->GetNext(&window)) { + args->ThrowError("Insert good error message here"); + return; + } + window_->AddTabbedWindow(window, args); } void BrowserWindow::SetVibrancy(mate::Arguments* args) { diff --git a/atom/browser/api/atom_api_browser_window.h b/atom/browser/api/atom_api_browser_window.h index 65f3a10f7a..55f0edcd07 100644 --- a/atom/browser/api/atom_api_browser_window.h +++ b/atom/browser/api/atom_api_browser_window.h @@ -243,7 +243,7 @@ class BrowserWindow : public mate::TrackableObject, void MergeAllWindows(); void MoveTabToNewWindow(); void ToggleTabBar(); - void AddTabbedWindow(NativeWindow* window); + void AddTabbedWindow(mate::Arguments* args); void SetVibrancy(mate::Arguments* args); void SetTouchBar(const std::vector& items); diff --git a/atom/browser/native_window.cc b/atom/browser/native_window.cc index a96b65424a..8cdb6dd6c7 100644 --- a/atom/browser/native_window.cc +++ b/atom/browser/native_window.cc @@ -336,7 +336,7 @@ void NativeWindow::MoveTabToNewWindow() { void NativeWindow::ToggleTabBar() { } -void NativeWindow::AddTabbedWindow(NativeWindow* window) { +void NativeWindow::AddTabbedWindow(NativeWindow* window, mate::Arguments* args) { } void NativeWindow::SetVibrancy(const std::string& filename) { diff --git a/atom/browser/native_window.h b/atom/browser/native_window.h index 5661f6237d..1a79a9d081 100644 --- a/atom/browser/native_window.h +++ b/atom/browser/native_window.h @@ -192,7 +192,7 @@ class NativeWindow : public base::SupportsUserData, virtual void MergeAllWindows(); virtual void MoveTabToNewWindow(); virtual void ToggleTabBar(); - virtual void AddTabbedWindow(NativeWindow* window); + virtual void AddTabbedWindow(NativeWindow* window, mate::Arguments* args); // Webview APIs. virtual void FocusOnWebView(); diff --git a/atom/browser/native_window_mac.h b/atom/browser/native_window_mac.h index a54e24b466..d7cd363348 100644 --- a/atom/browser/native_window_mac.h +++ b/atom/browser/native_window_mac.h @@ -109,7 +109,7 @@ class NativeWindowMac : public NativeWindow { void MergeAllWindows() override; void MoveTabToNewWindow() override; void ToggleTabBar() override; - void AddTabbedWindow(NativeWindow* window) override; + void AddTabbedWindow(NativeWindow* window, mate::Arguments* args) override; void SetVibrancy(const std::string& type) override; void SetTouchBar( diff --git a/atom/browser/native_window_mac.mm b/atom/browser/native_window_mac.mm index 30cc66d6f2..8a062c4c84 100644 --- a/atom/browser/native_window_mac.mm +++ b/atom/browser/native_window_mac.mm @@ -918,6 +918,7 @@ NativeWindowMac::NativeWindowMac( } if (transparent()) { + NSLog(@"Setting transparent"); // Setting the background color to clear will also hide the shadow. [window_ setBackgroundColor:[NSColor clearColor]]; } @@ -1671,9 +1672,9 @@ void NativeWindowMac::ToggleTabBar() { } } -void NativeWindowMac::AddTabbedWindow(NativeWindow* window) { +void NativeWindowMac::AddTabbedWindow(NativeWindow* window, mate::Arguments* args) { if (window_.get() == window->GetNativeWindow()) { - NSLog(@"Error: AddTabbedWindow cannot be called by a window on itself."); + args->ThrowError("AddTabbedWindow cannot be called by a window on itself"); } else { if ([window_ respondsToSelector:@selector(addTabbedWindow:ordered:)]) { [window_ addTabbedWindow:window->GetNativeWindow() ordered:NSWindowAbove]; From b722150d87c696ee27f30a9c579e95776ad0fe44 Mon Sep 17 00:00:00 2001 From: Samuel Attard Date: Tue, 27 Feb 2018 16:13:17 +1100 Subject: [PATCH 3/6] Don't cast manually for NativeWindow* --- atom/browser/api/atom_api_browser_window.cc | 7 +------ atom/browser/api/atom_api_browser_window.h | 2 +- 2 files changed, 2 insertions(+), 7 deletions(-) diff --git a/atom/browser/api/atom_api_browser_window.cc b/atom/browser/api/atom_api_browser_window.cc index 0812c61f55..8a1d57c2d6 100644 --- a/atom/browser/api/atom_api_browser_window.cc +++ b/atom/browser/api/atom_api_browser_window.cc @@ -1082,12 +1082,7 @@ void BrowserWindow::ToggleTabBar() { window_->ToggleTabBar(); } -void BrowserWindow::AddTabbedWindow(mate::Arguments* args) { - NativeWindow* window; - if (!args->GetNext(&window)) { - args->ThrowError("Insert good error message here"); - return; - } +void BrowserWindow::AddTabbedWindow(NativeWindow* window, mate::Arguments* args) { window_->AddTabbedWindow(window, args); } diff --git a/atom/browser/api/atom_api_browser_window.h b/atom/browser/api/atom_api_browser_window.h index 55f0edcd07..768d975e46 100644 --- a/atom/browser/api/atom_api_browser_window.h +++ b/atom/browser/api/atom_api_browser_window.h @@ -243,7 +243,7 @@ class BrowserWindow : public mate::TrackableObject, void MergeAllWindows(); void MoveTabToNewWindow(); void ToggleTabBar(); - void AddTabbedWindow(mate::Arguments* args); + void AddTabbedWindow(NativeWindow* window, mate::Arguments* args); void SetVibrancy(mate::Arguments* args); void SetTouchBar(const std::vector& items); From 837a2d4bbd12f4e0e720664c3b3cb649822f7e88 Mon Sep 17 00:00:00 2001 From: Shelley Vohr Date: Mon, 26 Feb 2018 22:25:09 -0800 Subject: [PATCH 4/6] appease the linter --- atom/browser/api/atom_api_browser_window.cc | 3 ++- atom/browser/native_window.cc | 3 ++- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/atom/browser/api/atom_api_browser_window.cc b/atom/browser/api/atom_api_browser_window.cc index 8a1d57c2d6..b469503fd1 100644 --- a/atom/browser/api/atom_api_browser_window.cc +++ b/atom/browser/api/atom_api_browser_window.cc @@ -1082,7 +1082,8 @@ void BrowserWindow::ToggleTabBar() { window_->ToggleTabBar(); } -void BrowserWindow::AddTabbedWindow(NativeWindow* window, mate::Arguments* args) { +void BrowserWindow::AddTabbedWindow(NativeWindow* window, + mate::Arguments* args) { window_->AddTabbedWindow(window, args); } diff --git a/atom/browser/native_window.cc b/atom/browser/native_window.cc index 8cdb6dd6c7..78c8ed3e6d 100644 --- a/atom/browser/native_window.cc +++ b/atom/browser/native_window.cc @@ -336,7 +336,8 @@ void NativeWindow::MoveTabToNewWindow() { void NativeWindow::ToggleTabBar() { } -void NativeWindow::AddTabbedWindow(NativeWindow* window, mate::Arguments* args) { +void NativeWindow::AddTabbedWindow(NativeWindow* window, + mate::Arguments* args) { } void NativeWindow::SetVibrancy(const std::string& filename) { From 2abc69780e1bdd62b33250fd291a5c70c02790fa Mon Sep 17 00:00:00 2001 From: Shelley Vohr Date: Tue, 27 Feb 2018 13:00:42 -0800 Subject: [PATCH 5/6] move native-mate back into the api layer --- atom/browser/api/atom_api_browser_window.cc | 4 +++- atom/browser/native_window.cc | 4 ++-- atom/browser/native_window.h | 2 +- atom/browser/native_window_mac.h | 2 +- atom/browser/native_window_mac.mm | 8 ++++---- 5 files changed, 11 insertions(+), 9 deletions(-) diff --git a/atom/browser/api/atom_api_browser_window.cc b/atom/browser/api/atom_api_browser_window.cc index b469503fd1..6e7c146b19 100644 --- a/atom/browser/api/atom_api_browser_window.cc +++ b/atom/browser/api/atom_api_browser_window.cc @@ -1084,7 +1084,9 @@ void BrowserWindow::ToggleTabBar() { void BrowserWindow::AddTabbedWindow(NativeWindow* window, mate::Arguments* args) { - window_->AddTabbedWindow(window, args); + const bool windowAdded = window_->AddTabbedWindow(window); + if (!windowAdded) + args->ThrowError("AddTabbedWindow cannot be called by a window on itself"); } void BrowserWindow::SetVibrancy(mate::Arguments* args) { diff --git a/atom/browser/native_window.cc b/atom/browser/native_window.cc index 78c8ed3e6d..1222151248 100644 --- a/atom/browser/native_window.cc +++ b/atom/browser/native_window.cc @@ -336,8 +336,8 @@ void NativeWindow::MoveTabToNewWindow() { void NativeWindow::ToggleTabBar() { } -void NativeWindow::AddTabbedWindow(NativeWindow* window, - mate::Arguments* args) { +bool NativeWindow::AddTabbedWindow(NativeWindow* window) { + return true; // for non-Mac platforms } void NativeWindow::SetVibrancy(const std::string& filename) { diff --git a/atom/browser/native_window.h b/atom/browser/native_window.h index 1a79a9d081..7f12f5a23b 100644 --- a/atom/browser/native_window.h +++ b/atom/browser/native_window.h @@ -192,7 +192,7 @@ class NativeWindow : public base::SupportsUserData, virtual void MergeAllWindows(); virtual void MoveTabToNewWindow(); virtual void ToggleTabBar(); - virtual void AddTabbedWindow(NativeWindow* window, mate::Arguments* args); + virtual bool AddTabbedWindow(NativeWindow* window); // Webview APIs. virtual void FocusOnWebView(); diff --git a/atom/browser/native_window_mac.h b/atom/browser/native_window_mac.h index d7cd363348..2d856aa9a9 100644 --- a/atom/browser/native_window_mac.h +++ b/atom/browser/native_window_mac.h @@ -109,7 +109,7 @@ class NativeWindowMac : public NativeWindow { void MergeAllWindows() override; void MoveTabToNewWindow() override; void ToggleTabBar() override; - void AddTabbedWindow(NativeWindow* window, mate::Arguments* args) override; + bool AddTabbedWindow(NativeWindow* window) override; void SetVibrancy(const std::string& type) override; void SetTouchBar( diff --git a/atom/browser/native_window_mac.mm b/atom/browser/native_window_mac.mm index 8a062c4c84..ab3ecb3fd8 100644 --- a/atom/browser/native_window_mac.mm +++ b/atom/browser/native_window_mac.mm @@ -1672,14 +1672,14 @@ void NativeWindowMac::ToggleTabBar() { } } -void NativeWindowMac::AddTabbedWindow(NativeWindow* window, mate::Arguments* args) { +bool NativeWindowMac::AddTabbedWindow(NativeWindow* window) { if (window_.get() == window->GetNativeWindow()) { - args->ThrowError("AddTabbedWindow cannot be called by a window on itself"); + return false; } else { - if ([window_ respondsToSelector:@selector(addTabbedWindow:ordered:)]) { + if ([window_ respondsToSelector:@selector(addTabbedWindow:ordered:)]) [window_ addTabbedWindow:window->GetNativeWindow() ordered:NSWindowAbove]; - } } + return true; } void NativeWindowMac::SetRenderWidgetHostOpaque(bool opaque) { From bf491de9fe6fbf8eae74a13ec70f5a64bf81469b Mon Sep 17 00:00:00 2001 From: Shelley Vohr Date: Wed, 28 Feb 2018 00:22:42 -0800 Subject: [PATCH 6/6] fix styling and add spec --- atom/browser/api/atom_api_browser_window.cc | 4 ++-- atom/browser/native_window_mac.mm | 1 - spec/api-browser-window-spec.js | 6 ++++++ 3 files changed, 8 insertions(+), 3 deletions(-) diff --git a/atom/browser/api/atom_api_browser_window.cc b/atom/browser/api/atom_api_browser_window.cc index 6e7c146b19..def0640862 100644 --- a/atom/browser/api/atom_api_browser_window.cc +++ b/atom/browser/api/atom_api_browser_window.cc @@ -1083,10 +1083,10 @@ void BrowserWindow::ToggleTabBar() { } void BrowserWindow::AddTabbedWindow(NativeWindow* window, - mate::Arguments* args) { + mate::Arguments* args) { const bool windowAdded = window_->AddTabbedWindow(window); if (!windowAdded) - args->ThrowError("AddTabbedWindow cannot be called by a window on itself"); + args->ThrowError("AddTabbedWindow cannot be called by a window on itself."); } void BrowserWindow::SetVibrancy(mate::Arguments* args) { diff --git a/atom/browser/native_window_mac.mm b/atom/browser/native_window_mac.mm index ab3ecb3fd8..f4ceaafd12 100644 --- a/atom/browser/native_window_mac.mm +++ b/atom/browser/native_window_mac.mm @@ -918,7 +918,6 @@ NativeWindowMac::NativeWindowMac( } if (transparent()) { - NSLog(@"Setting transparent"); // Setting the background color to clear will also hide the shadow. [window_ setBackgroundColor:[NSColor clearColor]]; } diff --git a/spec/api-browser-window-spec.js b/spec/api-browser-window-spec.js index 3033eb6a1f..9afccc7966 100644 --- a/spec/api-browser-window-spec.js +++ b/spec/api-browser-window-spec.js @@ -733,6 +733,12 @@ describe('BrowserWindow module', () => { done() }) }) + + it('throws when called on itself', () => { + assert.throws(() => { + w.addTabbedWindow(w) + }, /AddTabbedWindow cannot be called by a window on itself./) + }) }) describe('BrowserWindow.setVibrancy(type)', () => {