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 "enable_desktop_capturer" build flag #13133

Merged
merged 2 commits into from Jun 13, 2018

Conversation

Projects
None yet
2 participants
@alexeykuzmin
Contributor

alexeykuzmin commented Jun 1, 2018

Two goals:

  • not everyone needs Desktop Capturer, like Teams
  • we have several issues with it in the Ch66 upgrade branch, I need a nice way to disable it

See #13141 for build results with this feature being disabled.

@alexeykuzmin alexeykuzmin requested a review from electron/reviewers as a code owner Jun 1, 2018

@ckerr

Looks like a good start for making this feature optional.

I don't know the backstory of this feature. What's the use case for disabling this at compile time?

{
name: 'desktopCapturer',
file: 'desktop-capturer',
enabled: features.isDesktopCapturerEnabled()

This comment has been minimized.

@ckerr

ckerr Jun 2, 2018

Member

(minor) since the default of a boolean is false by convention, it would be slightly better to use something like .disabled rather than .enabled to imply that, if the property is missing, the module is not disabled

This comment has been minimized.

@alexeykuzmin

alexeykuzmin Jun 2, 2018

Contributor

Your suggestion looks good on the first sight.
But I'm strongly against any properties/variables/states with a meaning of anything missing or absent.
It's just too unnatural and thus difficult to read code like if this.disabled !== false.
Right, nobody ever should write anything like this, but once such properties are created that kind of code just magically appears out of nowhere =/

This comment has been minimized.

@ckerr

ckerr Jun 3, 2018

Member

I agree that there is a code smell here with the double negative. I'd argue that we're trading one smell for another wrt the three cases:

  • enabled property exists and is true, means enabled
  • enabled property exists and is false, means disabled
  • enabled property doesn't exist, means enabled

...which is also weird.

A cleaner way would be to have the .enabled property everywhere and to not use double negatives

This comment has been minimized.

@alexeykuzmin

alexeykuzmin Jun 3, 2018

Contributor
  • enabled property exists and is true, means enabled
  • enabled property exists and is false, means disabled
  • enabled property doesn't exist, means enabled

...which is also weird.

What exactly do you find weird here? The last clause?
For me absence of the enabled property means that "this module has no conditions to be enabled or disabled, and it must be enabled by default since being unconditionally disabled doesn't make any sense".

This comment has been minimized.

@alexeykuzmin

alexeykuzmin Jun 3, 2018

Contributor

Would it be better if the code processing this list were written like this?:

for (const {name, file, enabled = true, private = false} of moduleList) {
  if (!enabled) {
    continue
  }
  <...>
}

This comment has been minimized.

@alexeykuzmin

alexeykuzmin Jun 4, 2018

Contributor

@ckerr
I don't mind adding enabled: true properties though =)

This comment has been minimized.

@alexeykuzmin

alexeykuzmin Jun 8, 2018

Contributor

@ckerr Which option do you like best?

This comment has been minimized.

@alexeykuzmin

alexeykuzmin Jun 11, 2018

Contributor

@ckerr I just added a new config parsing code here
https://github.com/electron/electron/pull/13133/files#diff-a2cb68582030a995142a60862e3c1d51

and a comment to this file.

This comment has been minimized.

@ckerr

ckerr Jun 12, 2018

Member

To me, the option that would be the least surprising when reading the code for the first time would be if there was an .enabled property for all items, so that we wouldn't have to wonder what it means when it's not there.

If you want to add enabled: true properties to the others for consistency I would approve that change.

This comment has been minimized.

@alexeykuzmin

alexeykuzmin Jun 13, 2018

Contributor

add enabled: true properties to the others for consistency

@ckerr Done.

@alexeykuzmin

This comment has been minimized.

Contributor

alexeykuzmin commented Jun 2, 2018

@ckerr

What's the use case for disabling this at compile time?

Just updated the description.

@alexeykuzmin alexeykuzmin changed the title from [wip] Add "enable_desktop_capturer" build flag to Add "enable_desktop_capturer" build flag Jun 2, 2018

@alexeykuzmin alexeykuzmin requested a review from brenca Jun 4, 2018

name,
file,
enabled = true,
private: isPrivate = false

This comment has been minimized.

@alexeykuzmin

alexeykuzmin Jun 11, 2018

Contributor

private is a reserved keyword, that's why the variable has to have a different name (isPrivate instead of private).

alexeykuzmin added some commits Jun 1, 2018

Put DesktopCapturer API under a build flag
The name is "enable_desktop_capturer".
Enabled by default.
@ckerr

ckerr approved these changes Jun 13, 2018

@ckerr ckerr merged commit dee9aef into master Jun 13, 2018

12 checks passed

VSTS: electron-release-osx-x64 20180613.3 succeeded
Details
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
ci/circleci: electron-mas-x64 Your tests passed on CircleCI!
Details
ci/circleci: electron-osx-x64 Your tests passed on CircleCI!
Details
continuous-integration/appveyor/branch AppVeyor build succeeded
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details

@alexeykuzmin alexeykuzmin deleted the add-enable-desktop-capturer-build-flag branch Jun 13, 2018

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