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

Expose desktopCapturer in sandbox mode. #11124

Merged
merged 1 commit into from Nov 16, 2017

Conversation

Projects
None yet
2 participants
@tarruda
Contributor

tarruda commented Nov 15, 2017

nativeImage is a dependency of desktopCapturer, so it has to be exposed as well.

@tarruda tarruda requested a review from electron/reviewers as a code owner Nov 15, 2017

@codebytere

This comment has been minimized.

Member

codebytere commented Nov 15, 2017

This looks good, so i'm curious as to why we wouldn't have exported this before? Also, it might be difficult (i haven't yet attempted to test sandbox apis) but do you think you could look into possibly adding an associated test @tarruda?

@tarruda

This comment has been minimized.

Contributor

tarruda commented Nov 15, 2017

This looks good, so i'm curious as to why we wouldn't have exported this before?

TBH there's not a specific reason. The sandbox mode is still experimental and initially it didn't support any APIs except ipcRenderer, but since remote was exposed in #8939 it became theoretically possible to expose every other module to sandbox. The reason I'm doing this now is because I need to build a WebRTC application that uses sandbox.

Also, it might be difficult (i haven't yet attempted to test sandbox apis) but do you think you could look into possibly adding an associated test @tarruda?

There's already a spec file for desktopCapturer, and even though the tests run in a non-sandboxed renderer, the tested code is basically the same.

@codebytere

This comment has been minimized.

Member

codebytere commented Nov 15, 2017

Hmm ok if you think that the tests cover the change then it looks good to me :) Having done a little more research on sandboxed renderer, i feel comfortable approving this unless others have objections?

@codebytere codebytere merged commit aaae1bb into master Nov 16, 2017

6 of 7 checks passed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
ci/circleci: electron-linux-arm Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-arm64 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/branch This commit looks good
Details

@codebytere codebytere deleted the expose-desktop-capturer branch Nov 16, 2017

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