New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add warning when addTabbedWindow is called on self #12059

Merged
merged 6 commits into from Feb 28, 2018

Conversation

Projects
None yet
4 participants
@codebytere
Member

codebytere commented Feb 26, 2018

Closes #12046.

Per documentation, you can't add a tabbed window with the same window; you need to create a new one. When you add a new window, you need to pass in a value for NSWindowOrderingMode, which can be only NSWindowAbove, NSWindowBelow, or NSWindowOut. As you can't set a window above or below itself, this will crash it.

This PR catches attempts to do the above and instead outputs an error explaining that it must be called on a new window.

/cc @bpasero @MarshallOfSound

@codebytere codebytere requested a review from electron/reviewers as a code owner Feb 26, 2018

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.");

This comment has been minimized.

@MarshallOfSound

MarshallOfSound Feb 27, 2018

Member

This doesn't go over JS logs, can we do ThrowError instead (pass in mate::arguments or something)

MarshallOfSound and others added some commits Feb 27, 2018

window_->AddTabbedWindow(window);
void BrowserWindow::AddTabbedWindow(NativeWindow* window,
mate::Arguments* args) {
window_->AddTabbedWindow(window, args);

This comment has been minimized.

@deepak1556

deepak1556 Feb 27, 2018

Member

We shouldn't be processing/using native-mate outside the api layer, can you instead change the signature of NativeWindow::AddTabbedWindow to have a return value and throw error in api::BrowserWindow ? Thanks!

This comment has been minimized.

@codebytere

@jkleinsc jkleinsc closed this Feb 27, 2018

@jkleinsc jkleinsc reopened this Feb 27, 2018

@deepak1556

LGTM, with minor changes. Thanks!

@@ -918,6 +918,7 @@ static bool FromV8(v8::Isolate* isolate, v8::Handle<v8::Value> val,
}
if (transparent()) {
NSLog(@"Setting transparent");

This comment has been minimized.

@deepak1556

deepak1556 Feb 28, 2018

Member

I believe this is not related to the PR.

This comment has been minimized.

@MarshallOfSound

MarshallOfSound Feb 28, 2018

Member

Woops, had that in my staged env 🏃

void BrowserWindow::AddTabbedWindow(NativeWindow* window) {
window_->AddTabbedWindow(window);
void BrowserWindow::AddTabbedWindow(NativeWindow* window,
mate::Arguments* args) {

This comment has been minimized.

@deepak1556

deepak1556 Feb 28, 2018

Member

Can you run clang-format to fix style issue.

@codebytere codebytere merged commit dfa1dc4 into master Feb 28, 2018

9 checks passed

WIP ready for review
Details
ci/circleci: electron-linux-arm Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-arm-test Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-arm64 Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-arm64-test Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-ia32 Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-x64 Your tests passed on CircleCI!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/jenkins/pr-head This commit looks good
Details

@codebytere codebytere deleted the add-tabbedwindow-warning branch Feb 28, 2018

alexeykuzmin added a commit that referenced this pull request Mar 8, 2018

Merge pull request #12059 from electron/add-tabbedwindow-warning
add warning when addTabbedWindow is called on self

(cherry picked from commit dfa1dc4)

alexeykuzmin added a commit that referenced this pull request Mar 8, 2018

Merge pull request #12059 from electron/add-tabbedwindow-warning
add warning when addTabbedWindow is called on self

(cherry picked from commit dfa1dc4)

alexeykuzmin added a commit that referenced this pull request Mar 8, 2018

Merge pull request #12174 from electron/cp-2-0-x/12059
Merge pull request #12059 from electron/add-tabbedwindow-warning
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment