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

Don't show widevine install prompt when user doesn't want #2980

Merged
merged 1 commit into from
Aug 6, 2019
Merged

Don't show widevine install prompt when user doesn't want #2980

merged 1 commit into from
Aug 6, 2019

Conversation

simonhong
Copy link
Member

@simonhong simonhong commented Jul 23, 2019 •
edited
Loading

User can turn it off from settings page or prompt dialog.

Fix brave/brave-browser#5341

Screen Shot 2019-07-24 at 12 28 32 AM

Screen Shot 2019-07-25 at 9 04 52 AM

Submitter Checklist:

Test Plan:

yarn test --filter=WidevinePermissionRequestBrowserTest.*

  1. Start browser with clean profile
  2. Load https://bitmovin.com/demos/drm
  3. Check widevine install prompt is visible
  4. Check Don't ask again checkbox in the prompt and click Block button
  5. Reload and check prompt is not visible
  6. Open newtab and goto settings
  7. Check Ask Widevine install option is turned off
  8. Turn it on
  9. Reload https://bitmovin.com/demos/drm
  10. Check widevine install prompt is visible

Reviewer Checklist:

  • New files have MPL-2.0 license header.
  • Request a security/privacy review as needed.
  • Adequate test coverage exists to prevent regressions
  • Verify test plan is specified in PR before merging to source

After-merge Checklist:

  • The associated issue milestone is set to the smallest version that the
    changes has landed on.
  • All relevant documentation has been updated.

@simonhong simonhong added this to the 0.69.x - Nightly milestone Jul 23, 2019
@simonhong simonhong requested a review from bridiver as a code owner July 23, 2019 15:34
@simonhong simonhong self-assigned this Jul 23, 2019
@simonhong simonhong force-pushed the support_dont_ask_widevine_install branch from a823dd9 to 663c1dc Compare July 23, 2019 23:19
iefremov
iefremov reviewed Jul 24, 2019
View reviewed changes
browser/widevine/widevine_permission_request_browsertest.cc Outdated
content::RunAllTasksUntilIdle();
EXPECT_FALSE(observer.bubble_added_);

// Check permission bubble is visible when user turns it on.
Copy link
Contributor

Choose a reason for hiding this comment

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

whitespace

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

iefremov
iefremov reviewed Jul 24, 2019
View reviewed changes
browser/widevine/widevine_utils.cc Outdated
profile->GetPrefs()->SetBoolean(kDontAskWidevineInstall, dont_ask);
}

bool ShouldAskWidevineInstall(content::WebContents* web_contents) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd prefer not to add these methods... This one is used only once, and it is even shorter to directly query prefs in that place

Copy link
Member Author

Choose a reason for hiding this comment

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

Yup, deleted.

@iefremov iefremov self-requested a review July 24, 2019 23:30
iefremov
iefremov previously approved these changes Jul 24, 2019
View reviewed changes
@simonhong simonhong force-pushed the support_dont_ask_widevine_install branch from 663c1dc to 1e66b85 Compare July 25, 2019 00:06
@simonhong simonhong requested a review from rebron July 25, 2019 00:07
simonhong
simonhong commented Jul 25, 2019
View reviewed changes
Copy link
Member Author

@simonhong simonhong left a comment

Choose a reason for hiding this comment

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

@iefremov Comments are addressed.
@rebron Also settings are moved from appearance to extensions section.

browser/widevine/widevine_permission_request_browsertest.cc Outdated
content::RunAllTasksUntilIdle();
EXPECT_FALSE(observer.bubble_added_);

// Check permission bubble is visible when user turns it on.
Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

browser/widevine/widevine_utils.cc Outdated
profile->GetPrefs()->SetBoolean(kDontAskWidevineInstall, dont_ask);
}

bool ShouldAskWidevineInstall(content::WebContents* web_contents) {
Copy link
Member Author

Choose a reason for hiding this comment

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

Yup, deleted.

@simonhong simonhong requested a review from iefremov July 25, 2019 00:10
rebron
rebron reviewed Jul 25, 2019
View reviewed changes
app/brave_generated_resources.grd Outdated
Don't ask again
</message>
<message name="IDS_SETTINGS_ASK_WIDEVINE_INSTALL_DESC" desc="Text fragment for asking widevine install or not">
Ask when a site wants to install Widevine on your computer
Copy link
Collaborator

Choose a reason for hiding this comment

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

Minor nit to add a period "." at the end. "Ask when a site wants to install Widevine on your computer."

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

@simonhong simonhong force-pushed the support_dont_ask_widevine_install branch from 1e66b85 to 3d6b639 Compare July 25, 2019 00:55
iefremov
iefremov previously approved these changes Jul 25, 2019
View reviewed changes
@simonhong simonhong force-pushed the support_dont_ask_widevine_install branch from 3d6b639 to da15c9c Compare July 25, 2019 01:18
@simonhong
Copy link
Member Author

Ping to code owner @bridiver

@simonhong
Don't show widevine install prompt when user doesn't want
d3b1624 
User can turn it off from settings page or prompt dialog.
@simonhong simonhong force-pushed the support_dont_ask_widevine_install branch from da15c9c to d3b1624 Compare July 31, 2019 01:56
@simonhong
Copy link
Member Author

kindly ping @bridiver

bsclifton
bsclifton approved these changes Aug 6, 2019
View reviewed changes
Copy link
Member

@bsclifton bsclifton left a comment

Choose a reason for hiding this comment

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

Automated tests pass great; functionality works (ran through test plan). Code changes look good too 👍

@bsclifton bsclifton merged commit 104aa26 into master Aug 6, 2019
@bsclifton bsclifton deleted the support_dont_ask_widevine_install branch August 6, 2019 20:20
@simonhong simonhong mentioned this pull request Aug 6, 2019
Closed
@simonhong simonhong mentioned this pull request Sep 24, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rebron rebron rebron left review comments

@bsclifton bsclifton bsclifton approved these changes

@iefremov iefremov iefremov left review comments

@mkarolin mkarolin Awaiting requested review from mkarolin

@bridiver bridiver Awaiting requested review from bridiver

Assignees

@simonhong simonhong

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

Successfully merging this pull request may close these issues.

Add option for hiding Widevine install prompt
4 participants
@simonhong @iefremov @bsclifton @rebron