Skip to content
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

fix: handle window.open events in BrowserView #12686

Merged
merged 3 commits into from Nov 27, 2018

Conversation

Projects
None yet
8 participants
@Anrock
Copy link
Contributor

commented Apr 22, 2018

Fixes #10707

Not sure if it's a proper fix, but Work For Me™
Basically it's just a mindless copypaste of -new-window, -web-contents-created, -add-new-contents handlers from lib/browser/api/browser-window.js _init along with relevant unit-tests.

notes: Fixed child windows invisible if opened with window.open from BrowserView with nativeWindowOpen enabled

@Anrock Anrock requested a review from as a code owner Apr 22, 2018

@Anrock

This comment has been minimized.

Copy link
Contributor Author

commented Apr 22, 2018

Also, personally i need this backported to 1.7.x and 1.8.x branches but i haven't found the docs on how to do this: from pull request guides it seems only permitted target branch is master. Is there any docs on backporting process or could someone explain it to me?

@MarshallOfSound MarshallOfSound requested a review from Apr 22, 2018


Object.setPrototypeOf(BrowserView.prototype, EventEmitter.prototype)
BrowserView.prototype._init = function () {
// Make new windows requested by links behave like "window.open"
this.webContents.on('-new-window', (event, url, frameName, disposition,

This comment has been minimized.

Copy link
@poiru

poiru Apr 23, 2018

Member

Maybe we could just do this in lib/browser/api/web-contents.js to share the impl between BrowserWindow/BrowserView? cc @zcbenz

This comment has been minimized.

Copy link
@Anrock

Anrock Apr 23, 2018

Author Contributor

Yeah, that seems like a nice fix of "mindless copypaste".

This comment has been minimized.

Copy link
@Anrock

Anrock Apr 23, 2018

Author Contributor

@poiru how do i handle commits with fixes from review? Is it ok to fixup it all and push -f or is it better to lay on top of existing commits?

This comment has been minimized.

Copy link
@poiru

poiru Apr 23, 2018

Member

@Anrock Either way is fine with me. Feel free to just make new commits. We can squash it all when merging.

@Anrock

This comment has been minimized.

Copy link
Contributor Author

commented Apr 24, 2018

@poiru i've added second commit that moves common handlers from BrowserView and BrowserWindow to WebContents. Also rebased on fresh master.

@poiru

poiru approved these changes Apr 24, 2018

Copy link
Member

left a comment

LGTM, thank you!

@Anrock

This comment has been minimized.

Copy link
Contributor Author

commented Apr 24, 2018

@poiru thanks. Can it be also ported to 1.7? AFAIK there were some conflicts - i can make separate PR with adapted version.

@MarshallOfSound

This comment has been minimized.

Copy link
Member

commented Apr 24, 2018

This is something that can't be backported right? It's semver/feature 🤔

@Anrock

This comment has been minimized.

Copy link
Contributor Author

commented Apr 24, 2018

@MarshallOfSound i'd say it's a bugfix. At least i expect, from user perspective, that BrowserView should behave identically irregardless if nativeWindowOpen is true or false. Just like BrowserWindow does.

@MarshallOfSound

This comment has been minimized.

Copy link
Member

commented Apr 24, 2018

@Anrock There's a line between "adding a feature that probably should have been there in the first place" and "fixing something that wasn't functioning correctly". I think this is quite nicely fitting in the first category

@Anrock

This comment has been minimized.

Copy link
Contributor Author

commented Apr 24, 2018

@MarshallOfSound ok.
Only thing that bothers me now is what should i do to get this changes to 1.7 and 1.8 branches, please advice.

@poiru

This comment has been minimized.

Copy link
Member

commented Apr 26, 2018

@ckerr Can you merge?

@Anrock

This comment has been minimized.

Copy link
Contributor Author

commented May 11, 2018

@poiru @ckerr any eta on merge? Two weeks and no movement.

@MarshallOfSound

This comment has been minimized.

Copy link
Member

commented May 11, 2018

The failing tests are blocking this

@poiru

This comment has been minimized.

Copy link
Member

commented May 12, 2018

@Anrock The test failures might be flakes. Can you rebase on top of master and force push?

@alexeykuzmin

This comment has been minimized.

Copy link
Contributor

commented Aug 31, 2018

@Anrock
Can you please rebase your branch onto the latest master and resolve merge conflicts?

@Anrock Anrock changed the title handle window.open events in BrowserView fix: handle window.open events in BrowserView Sep 3, 2018

@Anrock

This comment has been minimized.

Copy link
Contributor Author

commented Sep 3, 2018

@alexeykuzmin in progress. However i suspect that test failures are not flakes actually.

@Anrock

This comment has been minimized.

Copy link
Contributor Author

commented Sep 3, 2018

@alexeykuzmin done. But i don't have time to investigate test failures.

@Akshat-Jain
Copy link

left a comment

Please resolve the conflicts. Thanks :-)

@Anrock

This comment has been minimized.

Copy link
Contributor Author

commented Sep 21, 2018

@Akshat-Jain done. I think it's time to scrap this PR and rewrite from scratch. Looks like handlers have changed multiple times and given non-trivial conflicts i suspect something is missed in action.

@Anrock Anrock closed this Oct 4, 2018

@Anrock Anrock reopened this Oct 4, 2018

@Anrock

This comment has been minimized.

Copy link
Contributor Author

commented Oct 4, 2018

Can someone help with broken tests?
linux-arm-debug, linux-arm-testing, linux-ia32-testing: failed build with sscache error
inux-x64-testing-tests, linux-x64-testing-verify-ffmpeg: failed tests with cannot open display
osx-testing: marked as failed, but circleci build step is not even finished yet

@Anrock

This comment has been minimized.

Copy link
Contributor Author

commented Oct 5, 2018

@Akshat-Jain i've rewritten this from scratch, so PR needs re-review.

@Anrock

This comment has been minimized.

Copy link
Contributor Author

commented Oct 5, 2018

Looks like #14984 should fix part of failing linux tests.

@Anrock

This comment has been minimized.

Copy link
Contributor Author

commented Oct 8, 2018

@alexeykuzmin regarding #15016, this PR rebased on master and in branch log i see that branch includes 918488a commit that should fix build breakage, however build is still broken.
Anything else i'm missing?

@Anrock

This comment has been minimized.

Copy link
Contributor Author

commented Oct 11, 2018

@codebytere

This comment has been minimized.

Copy link
Member

commented Oct 11, 2018

hi @Anrock, @akshat-jain isn't involved in this project at all; you can see his review wasn't a valid one as it was a grey check. You can ping anyone in @electron/reviewers for future review!

@Anrock

This comment has been minimized.

Copy link
Contributor Author

commented Oct 11, 2018

@codebytere thank you! How do i find member list of @electron/reviewers?

@codebytere

This comment has been minimized.

Copy link
Member

commented Oct 18, 2018

ah i mean you can literally tag @electron/reviewers!

@Anrock

This comment has been minimized.

Copy link
Contributor Author

commented Oct 18, 2018

@codebytere for some reason it doesn't appear in autocompletion list and not marked as mention after i manually type it.

@MarshallOfSound

This comment has been minimized.

Copy link
Member

commented Oct 18, 2018

@Anrock All the test failures seem related to your code changes. For instance:

BrowserWindow module "webPreferences" option "sandbox" option should inherit the sandbox setting in opened windows - should inherit the sandbox setting in opened windows

Without green builds we can't look at merging this, you can run the tests locally with npm run test to verify before pushing to our CI.

@codebytere

This comment has been minimized.

Copy link
Member

commented Nov 8, 2018

@Anrock are you still interested in moving forward with this?

@Anrock

This comment has been minimized.

Copy link
Contributor Author

commented Nov 8, 2018

@codebytere yes, but i really don't have time to do it - busy fixing window.opener in my app. And i guess code is gone stale again. Should i close PR then or there is someone else interested doing it?

@zcbenz

This comment has been minimized.

Copy link
Member

commented Nov 27, 2018

I have rebased the branch and added some fixes and tests.

@zcbenz

zcbenz approved these changes Nov 27, 2018

@zcbenz zcbenz merged commit aafbd86 into electron:master Nov 27, 2018

22 of 23 checks passed

ci/circleci: mas-testing-tests Your tests failed on CircleCI
Details
Absolute Zero
Artifact Comparison No Changes
Details
Semantic Pull Request ready to be squashed
Details
WIP ready for review
Details
appveyor: win-ia32-testing-pr AppVeyor build succeeded
Details
appveyor: win-x64-testing-pr AppVeyor build succeeded
Details
ci/circleci: linux-arm-debug Your tests passed on CircleCI!
Details
ci/circleci: linux-arm-testing Your tests passed on CircleCI!
Details
ci/circleci: linux-arm64-debug Your tests passed on CircleCI!
Details
ci/circleci: linux-arm64-testing Your tests passed on CircleCI!
Details
ci/circleci: linux-checkout Your tests passed on CircleCI!
Details
ci/circleci: linux-ia32-debug Your tests passed on CircleCI!
Details
ci/circleci: linux-ia32-testing Your tests passed on CircleCI!
Details
ci/circleci: linux-ia32-testing-tests Your tests passed on CircleCI!
Details
ci/circleci: linux-x64-debug Your tests passed on CircleCI!
Details
ci/circleci: linux-x64-testing Your tests passed on CircleCI!
Details
ci/circleci: linux-x64-testing-tests Your tests passed on CircleCI!
Details
ci/circleci: mas-testing Your tests passed on CircleCI!
Details
ci/circleci: osx-testing Your tests passed on CircleCI!
Details
ci/circleci: osx-testing-tests Your tests passed on CircleCI!
Details
electron-lint Build #20181127.2 succeeded
Details
release-notes Release notes found
@release-clerk

This comment has been minimized.

Copy link

commented Nov 27, 2018

Release Notes Persisted

Fixed child windows invisible if opened with window.open from BrowserView with nativeWindowOpen enabled

@welcome

This comment has been minimized.

Copy link

commented Nov 27, 2018

Congrats on merging your first pull request! 🎉🎉🎉

@Anrock Anrock deleted the Anrock:browserview-handle-window-open branch Dec 4, 2018

@BinaryMuse

This comment has been minimized.

Copy link
Member

commented Dec 13, 2018

/trop run backport-to 4-0-x

BinaryMuse added a commit that referenced this pull request Dec 13, 2018

Merge pull request #12686 from Anrock/browserview-handle-window-open
fix: handle window.open events in BrowserView
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.