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

Enable widevine on linux #3037

Closed
wants to merge 1 commit into from
Closed

Conversation

simonhong
Copy link
Member

@simonhong simonhong commented Jan 21, 2019

Widevine cdm pkg is downloaded by using version that is spcified in
package.json. Then it is stored to src/third_party/widevine.

widevinew version configuration is added in package.json.
adm-zip and sync-request are newly added into dependencies for
downloading widevine pkg synchronously and unzip.

This PR should be merged with brave/brave-core#1475.

Fix #413

Submitter Checklist:

  • Submitted a ticket for my issue if one did not already exist.
  • Used Github auto-closing keywords in the commit message.
  • Added/updated tests for this change (for new code or code which already has tests).
  • Verified that these changes build without errors on
    • Windows
    • macOS
    • Linux
  • Verified that these changes pass automated tests (npm test brave_unit_tests && npm test brave_browser_tests) on
    • Windows
    • macOS
    • Linux
  • Ran git rebase master (if needed).
  • Ran git rebase -i to squash commits (if needed).
  • Tagged reviewers and labelled the pull request as needed.
  • Requested a security/privacy review as needed.

Test Plan:

  1. Start browser with clean profile
  2. Load https://bitmovin.com/demos/drm
  3. Check No drm
  4. Install widevine via content settings bubble
  5. Check widevine is enabled
  6. Restart browser and check widevine is enabled

Reviewer Checklist:

  • New files have MPL-2.0 license header.
  • Request a security/privacy review as needed.
  • Adequate test coverage exists to prevent regressions.

@simonhong simonhong self-assigned this Jan 21, 2019
@simonhong simonhong changed the title WIP: Enable widevine on all platforms WIP: Enable widevine on linux Jan 21, 2019
@simonhong simonhong force-pushed the support_widevine_in_linux branch 2 times, most recently from eff91d3 to ffb91dc Compare January 28, 2019 06:09
@simonhong simonhong force-pushed the support_widevine_in_linux branch from ffb91dc to 655966f Compare January 28, 2019 06:55
@simonhong simonhong added this to the 0.62.x - Nightly milestone Jan 29, 2019
@simonhong simonhong requested review from bbondy and iefremov January 29, 2019 09:20
@simonhong simonhong changed the title WIP: Enable widevine on linux Enable widevine on linux Jan 29, 2019
Widevine cdm pkg is downloaded by using version that is spcified in
package.json. Then it is stored to src/third_party/widevine.

widevinew version configuration is added in package.json.
adm-zip and sync-request are newly added into dependencies for
downloading widevine pkg synchronously and unzip.
@simonhong simonhong force-pushed the support_widevine_in_linux branch from 655966f to 69b46cc Compare January 31, 2019 05:43

// Synchronously download widevine pkg and extracts into cdm dir.
const widevineZipFileName = `${widevineConfig.configuredVersion}-linux-x64.zip`
const widevineUrl = `https://redirector.gvt1.com/edgedl/widevine-cdm/${widevineZipFileName}`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure if downloading a new binary each time (instead of the manual binary update) is OK from the security point of view, so maybe it is worth to show this to someone from DevOps or security teams.

Copy link
Member Author

@simonhong simonhong Jan 31, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bbondy I think we also need to think about widevine download url could be exposed or not. Also, as far as we use bundling, I think manual binary update seems fine.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please post a security review for this PR entirely in brave/internal

@bbondy bbondy requested a review from diracdeltas February 1, 2019 16:54
@bbondy
Copy link
Member

bbondy commented Feb 1, 2019

Added @diracdeltas to review the new npm dependencies

// because processes except browser process are forked from zygote process not created from
// brave.exe binary. So, cdm should be loaded(not initialized) into zygote process space
// before zygote enters the sandbox.
// This method places WidevineCdm lib and header file into proper place before building.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for delay in reviewing this, but for licensing reasons it's not allowed for us to bundle.
This will need to happen on each client and we should use an npm config variable likebrave_google_api_endpoint

"commander": "^2.9.0",
"fs-extra": "^1.0.0"
"fs-extra": "^1.0.0",
"sync-request": "^6.0.0"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If i'm not mistaken, these can be devDependencies instead of dependencies. (npm install --save-dev)

@bbondy
Copy link
Member

bbondy commented Feb 1, 2019

I'm going to close this PR because I think the approach can't be used to a point that a new PR is needed.

@bbondy bbondy closed this Feb 1, 2019
@simonhong simonhong deleted the support_widevine_in_linux branch June 12, 2019 06:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants