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 devtools open in mixed sandbox #9983

Merged
merged 2 commits into from Jul 14, 2017

Conversation

Projects
None yet
3 participants
@juturu
Contributor

juturu commented Jul 12, 2017

Fixing devtools open in mixed sandbox. Added UTs to validate.

cc: @fasterthanlime @chandrachivukula

@juturu juturu requested a review from kevinsawicki Jul 12, 2017

@fasterthanlime

This comment has been minimized.

Show comment
Hide comment
@fasterthanlime

fasterthanlime Jul 12, 2017

Contributor

I'm not familiar with the mixed sandbox code enough to review the actual code changes - as for the tests, is the devtools-opened event enough? Did it used to fail before the code changes?

My experience with 1.7.4 was that BrowserWindow indicated that the devTools were opened when they actually weren't, so I'm just wary that the event would fire anyway even if the devtools were broken.

Apart from these concerns, lgtm 👍 Thanks for addressing this! 🌟

Contributor

fasterthanlime commented Jul 12, 2017

I'm not familiar with the mixed sandbox code enough to review the actual code changes - as for the tests, is the devtools-opened event enough? Did it used to fail before the code changes?

My experience with 1.7.4 was that BrowserWindow indicated that the devTools were opened when they actually weren't, so I'm just wary that the event would fire anyway even if the devtools were broken.

Apart from these concerns, lgtm 👍 Thanks for addressing this! 🌟

@juturu

This comment has been minimized.

Show comment
Hide comment
@juturu

juturu Jul 12, 2017

Contributor

is the devtools-opened event enough?

Yes. Same event is used to test other devtools tests.

Did it used to fail before the code changes?

Yes. Without the fix, this new test fails.

Contributor

juturu commented Jul 12, 2017

is the devtools-opened event enough?

Yes. Same event is used to test other devtools tests.

Did it used to fail before the code changes?

Yes. Without the fix, this new test fails.

@kevinsawicki

LGTM, thanks for fixing this 👍

@kevinsawicki kevinsawicki merged commit 48f5a66 into master Jul 14, 2017

4 of 6 checks passed

electron-osx-x64 Build #4564 failed in 3 min 2 sec
Details
electron-win-ia32 Build #3544 failed in 14 min
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
electron-mas-x64 Build #4560 succeeded in 12 min
Details
electron-win-x64 Build #3536 succeeded in 10 min
Details

@kevinsawicki kevinsawicki deleted the devtools-mixed-sandbox branch Jul 14, 2017

@juturu

This comment has been minimized.

Show comment
Hide comment
@juturu

juturu Jul 14, 2017

Contributor

Thanks @kevinsawicki 🙇

Contributor

juturu commented Jul 14, 2017

Thanks @kevinsawicki 🙇

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment