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

feat: add media access apis for mojave #15624

Merged
merged 1 commit into from Dec 4, 2018

Conversation

@codebytere
Member

codebytere commented Nov 7, 2018

Description of Change

This PR adds new APIs to control new media privacy and consent rules introduced in MacOS Mojave. Namely:

systemPreferences.getMediaAccessStatus(mediaType)
systemPreferences.askForMediaAccess(mediaType)

Apple Docs:

Checklist

  • PR description included and stakeholders cc'd
  • npm test passes
  • relevant documentation is changed or added
  • PR title follows semantic commit guidelines

Release Notes

Notes: Added media access APIs for macOS Mojave

@codebytere codebytere requested a review from electron/reviewers as a code owner Nov 7, 2018

@codebytere codebytere force-pushed the access-apis branch 2 times, most recently from ad39da6 to e2477bf Nov 7, 2018

@codebytere codebytere changed the title from feat: add media access apis for mojave to [WIP] feat: add media access apis for mojave Nov 7, 2018

@codebytere codebytere requested a review from electron/docs as a code owner Nov 8, 2018

@codebytere codebytere changed the title from [WIP] feat: add media access apis for mojave to feat: add media access apis for mojave Nov 9, 2018

@codebytere codebytere force-pushed the access-apis branch 3 times, most recently from 2d73e0b to fbf4da5 Nov 10, 2018

@groundwater groundwater requested a review from nornagon Nov 13, 2018

@groundwater

This comment has been minimized.

Member

groundwater commented Nov 13, 2018

We're 👍 on porting to 4.x post-merge if:

  • works with MAS
  • Works on older macos versions

However, there is still outstanding debate on the actual API cc @nornagon @codebytere

@miniak

We should return an enum, with all the 4 possible values as returned by the OS API. Being able to distinguish between denied and restricted is quite important to be able to show appropriate UI to the users of the apps.

@miniak

Also I think the API should just be a thin wrapper over the OS APIs. I would definitely not show any message boxes, let the app decide how to present that to the user.

Something like this:

getMediaAccessStatus(mediaType: 'audio' | 'video'): 'not-determined' | 'allowed' | 'denied' | 'restricted'
requestMediaAccess(mediaType: 'audio' | 'video'): Promise<boolean>
@miniak

This comment has been minimized.

Contributor

miniak commented Nov 14, 2018

If you insist on providing a conveniece API, I would expose both the raw API, which provides all the states and just add an additional method to do that, which can be inplemented in JS using the low level APIs. This way the C++ code can stay much simpler and without any business logic

Show resolved Hide resolved atom/browser/api/atom_api_system_preferences_mac.mm Outdated
Show resolved Hide resolved docs/api/system-preferences.md Outdated
Show resolved Hide resolved docs/api/system-preferences.md Outdated
Show resolved Hide resolved docs/api/system-preferences.md Outdated
@rspeyer

This comment has been minimized.

rspeyer commented Nov 14, 2018

+1 on @miniak's feedback - I think this API should do less to transform the data and be treated as more of a passthrough of Apple's data

@codebytere codebytere force-pushed the access-apis branch from aac060f to f656488 Nov 14, 2018

@alexeykuzmin

Questions are up there.

Show resolved Hide resolved atom/browser/ui/cocoa/atom_access_controller.mm Outdated
Show resolved Hide resolved docs/api/system-preferences.md
Show resolved Hide resolved docs/api/system-preferences.md Outdated
@codebytere

This comment has been minimized.

Member

codebytere commented Nov 20, 2018

This should be ready for review again, i've addressed all the above comments and simplified the impl to remove opinionated behaviors 😀

@BinaryMuse

I think this API looks pretty good as it stands now.

@codebytere codebytere force-pushed the access-apis branch from 2ac5456 to 132aeea Nov 26, 2018

@codebytere

This comment has been minimized.

Member

codebytere commented Nov 27, 2018

@miniak @alexeykuzmin are you still requesting changes?

@miniak miniak force-pushed the access-apis branch from 132aeea to bdcedb3 Dec 3, 2018

@miniak

miniak approved these changes Dec 3, 2018

@miniak

This comment has been minimized.

Contributor

miniak commented Dec 3, 2018

@alexeykuzmin, @nornagon can you please check the refactored version?

@codebytere codebytere force-pushed the access-apis branch from bdcedb3 to 5301b91 Dec 4, 2018

@miniak miniak force-pushed the access-apis branch from 5301b91 to f5e9673 Dec 4, 2018

@miniak miniak force-pushed the access-apis branch 2 times, most recently from 2d5132e to 2bc5505 Dec 4, 2018

@miniak

This comment has been minimized.

Contributor

miniak commented Dec 4, 2018

Verified to work properly on macOS 10.13
screen shot 2018-12-04 at 11 19 54 am
cc @groundwater

@miniak

This comment has been minimized.

Contributor

miniak commented Dec 4, 2018

@rspeyer can you sign-off as well?

@miniak miniak force-pushed the access-apis branch from 2bc5505 to 0f9eca3 Dec 4, 2018

@rspeyer

rspeyer approved these changes Dec 4, 2018

@codebytere codebytere merged commit c31629a into master Dec 4, 2018

21 checks passed

Absolute Zero
Semantic Pull Request ready to be squashed
Details
WIP Legacy commit status override — see details
Details
appveyor: win-ia32-debug AppVeyor build succeeded
Details
appveyor: win-ia32-testing AppVeyor build succeeded
Details
appveyor: win-ia32-testing-pr AppVeyor build succeeded
Details
appveyor: win-x64-debug AppVeyor build succeeded
Details
appveyor: win-x64-testing 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
release-notes Release notes found
@release-clerk

This comment has been minimized.

release-clerk bot commented Dec 4, 2018

Release Notes Persisted

Added media access APIs for macOS Mojave

@codebytere codebytere deleted the access-apis branch Dec 4, 2018

@trop

This comment has been minimized.

Contributor

trop bot commented Dec 4, 2018

I have automatically backported this PR to "4-0-x", please check out #15948

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