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.

Sign up for GitHub

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

Jump to bottom

Add Widevine toggle option to settings #7554

Merged
merged 4 commits into from
Jan 20, 2021
Merged

Add Widevine toggle option to settings #7554

merged 4 commits into from
Jan 20, 2021

Conversation

simonhong
Copy link
Member

@simonhong simonhong commented Jan 11, 2021 •
edited
Loading

This toggle option register/unregister widevine cdm components.

Resolves brave/brave-browser#2791

image

Submitter Checklist:

  • There is a ticket for my issue.
  • Used Github auto-closing keywords in the commit message.
  • Wrote a good PR/commit description
  • Added appropriate labels (QA/Yes or QA/No; release-notes/include or release-notes/exclude; OS/...) to the associated issue
  • Checked the PR locally: npm run test -- brave_browser_tests, npm run test -- brave_unit_tests, npm run lint, npm run gn_check, npm run tslint
  • Ran git rebase master (if needed).
  • Requested a security/privacy review as needed.

Reviewer Checklist:

  • New files have MPL-2.0 license header.
  • Adequate test coverage exists to prevent regressions
  • Major classes, functions and non-trivial code blocks are well-commented
  • Changes in component dependencies are properly reflected in gn
  • Code follows the style guide
  • Test plan is specified in PR before merging

After-merge Checklist:

Test Plan:

  1. Launch with clean profile
  2. Load brave://settings/extensions and enable Widevine
  3. Relaunch and check netflix works
  4. Load brave://settings/extensions and disable Widevine
  5. Relaunch and check widevine permisson request popup is displayed with netflix

@simonhong simonhong self-assigned this Jan 11, 2021
@simonhong
Copy link
Member Author

@rebron @karenkliu I just put widevine toggle option below current ask option.
Maybe we need to adjust its postion, desc and etc.. PTAL.

@simonhong simonhong force-pushed the toggle_widevine branch 2 times, most recently from 632c5b9 to 7d76713 Compare January 11, 2021 06:30
@simonhong simonhong requested a review from iefremov January 11, 2021 06:30
@rebron
Copy link
Collaborator

rebron commented Jan 15, 2021

Underneath that setting works.
Proposed text:
Uses Widevine digital rights management (DRM) component to view streaming video and other content in the browser.

@simonhong simonhong marked this pull request as ready for review January 15, 2021 10:29
@simonhong simonhong requested a review from a team as a code owner January 15, 2021 10:29
@simonhong simonhong force-pushed the toggle_widevine branch 2 times, most recently from 32e85cc to 3b38f5c Compare January 15, 2021 12:54
@simonhong
Copy link
Member Author

Kindly ping to reviews :)

iefremov
iefremov reviewed Jan 19, 2021
View reviewed changes
browser/BUILD.gn Outdated Show resolved Hide resolved
@simonhong
Address review comments
d3edbf7 
* Change widevine settings description
* Reverted accidently deleted code
mkarolin
mkarolin approved these changes Jan 20, 2021
View reviewed changes
Copy link
Collaborator

@mkarolin mkarolin left a comment

Choose a reason for hiding this comment

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

chromium_src changes LGTM

@simonhong simonhong merged commit 237b1aa into master Jan 20, 2021
@simonhong simonhong deleted the toggle_widevine branch January 20, 2021 02:51
@simonhong simonhong added this to the 1.21.x - Nightly milestone Jan 20, 2021
karenkliu
karenkliu reviewed Jan 20, 2021
View reviewed changes
Copy link

@karenkliu karenkliu left a comment

Choose a reason for hiding this comment

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

Looks good - use the text that Raf suggested, thanks!

@stephendonner stephendonner mentioned this pull request Feb 8, 2021
Closed
@bsclifton bsclifton mentioned this pull request Feb 12, 2021
Closed
bridiver
bridiver reviewed Sep 1, 2021
View reviewed changes
browser/widevine/widevine_utils.cc

SetWidevineOptedIn(false);
g_brave_browser_process->component_updater()->UnregisterComponent(
BraveDrmTabHelper::kWidevineComponentId);
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't know why the component id would be part of the TabHelper, but this creates a circular dependency and needs to be moved somewhere else.

@bsclifton bsclifton mentioned this pull request Sep 2, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bridiver bridiver bridiver left review comments

@karenkliu karenkliu karenkliu left review comments

@bsclifton bsclifton bsclifton left review comments

@iefremov iefremov iefremov approved these changes

@mkarolin mkarolin mkarolin approved these changes

@rebron rebron Awaiting requested review from rebron

Assignees

@simonhong simonhong

Labels
None yet
Projects
None yet
Milestone
1.21.x - Release
Development

Successfully merging this pull request may close these issues.

Add an option in Settings to install/enable and uninstall/disable Widevine Plugin (Muon parity)
7 participants
@simonhong @rebron @bridiver @karenkliu @iefremov @bsclifton @mkarolin