Support Widevine and 2 component updaters #189

bbondy · 2018-06-22T21:31:38Z

Summary

This PR is easiest to review 1 commit at a time
There are now 2 component updaters, brave server (which is used for all by default), google server (which is used for widevine)
Since the brave component update server is used by default, we don't need the URL rewriting code for now.
There is UI w/ animation in the URL bar when widevine is not installed. This is currently a global setting, but we will probably make it a per origin setting later on. Animation exists for the URL bar icon to say Widevine is not installed.
You can click on the URL bar plugin icon to Install and run Widevine.

What is looks like

This slides in and then back out collapsing to a normal icon:

After clicking it looks like this:

Clicking on the ? loads this page: https://policies.google.com/terms

Clicking on install and run hides the url bar icon and the bubble and installs it. Future starts will always register the widevine plugin so if it failed to install, the next startup would try it.

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).
Ran git rebase -i to squash commits (if needed).
Tagged reviewers and labelled the pull request as needed.
Request a security/privacy review as needed.

Test Plan:

Reviewer Checklist:

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

bbondy · 2018-06-26T18:20:09Z

patches/chrome-browser-ui-content_settings-content_setting_image_model.cc.patch

+-
+#if defined(BRAVE_CHROMIUM_BUILD)
+  BraveGenerateContentSettingImageModels(result);
+#endif


The above function is static so this seemed like the most minimally invasive way to hook the list of content setting image models.

emerick

LGTM, just had a few minor comments.

emerick · 2018-06-26T18:40:42Z

app/brave_generated_resources.grd

+      <message name="IDS_WIDEVINE_NOT_INSTALLED_MESSAGE" desc="Tooltip on the icon when Wideivne is not installed. 'Widevine' is the name of a plugin and should not be translated.">
+        Widevine is not installed.
+      </message>
+      <message name="IDS_INSTALL_AND_RUN_WIDEVINE" desc="Button to install register Widevine with the component updater.  'Widevine' is the name of a plugin and should not be translated.">


Seems like a word is missing in the description "Button to install register Widevine [...]"

yep, "and" thx.

emerick · 2018-06-26T18:49:08Z

patches/chrome-browser-ui-content_settings-content_setting_image_model.cc.patch

+ //   ContentSettingSubresourceFilterImageModel - deceptive content
+ //   ContentSettingFramebustBlockImageModel    - blocked framebust
+
+


I seem to remember comments that we should try to avoid adding/removing whitespace in patch files, but maybe it doesn't matter for simple cases like this? Anyway, just pointing it out!

my bad, will fix.

emerick · 2018-06-26T19:12:49Z

browser/ui/content_settings/brave_widevine_content_setting_bubble_model.h

+                                               content::WebContents* web_contents,
+                                               Profile* profile);
+  BraveBrowserContentSettingBubbleModelDelegate* brave_content_settings_delegate() const {
+    return (BraveBrowserContentSettingBubbleModelDelegate*)


The cast on the return value is necessary here?

seems not anymore, left over from a previous refactoring. Will fix.

emerick

LGTM

bridiver · 2018-06-26T20:56:06Z

patches/chrome-browser-ui-content_settings-content_setting_image_model.cc.patch

+ #include "chrome/app/vector_icons/vector_icons.h"
+ #include "chrome/browser/browser_process.h"
+@@ -737,6 +738,9 @@ ContentSettingImageModel::GenerateContentSettingImageModels() {
+   std::vector<std::unique_ptr<ContentSettingImageModel>> result;


couldn't this be done with an override like RegisterWidevineCdmComponent?

I don't think so because I'd have to patch the header file to add another definition.

This patches to generate Brave content setting image models for the navigation URL bar

…omponent_installer.cc This overrides the RegisterWidevineCdmComponent to first check if Widevine has been opted in before proceeding

The pref is a boolean pref by the name: brave.widevine_opted_in

Because there are 2 different component updatears now, and each has its own configuration.

George9000 · 2018-11-06T01:11:39Z

Using Brave Beta, 0.56.6 Chromium: 70.0.3538.67 on MacOS 10.14.1
The URL prompt to install Widevine should also appear when attempting to play Amazon videos. It does not. Amazon gives an error message about going to chrome://components to install Widevine. There is no such component listed. After reading about the URL bar component for Spotify, I went to Spotify, got the prompt, and installed Widevine. I use neither Spotify nor Netflix. Others who likewise only use Amazon will be stuck unless they find the workaround. Install on Muon Brave was straightforward.

bbondy · 2018-11-06T01:16:09Z

Thanks for the feedback @George9000
We're tracking the better detection mechanism here:
brave/brave-browser#1985

The current URL based method isn't enough.

bbondy force-pushed the widevine branch 4 times, most recently from acd439d to 7e74428 Compare June 25, 2018 02:11

bbondy added the pr/work-in-progress label Jun 25, 2018

bbondy self-assigned this Jun 25, 2018

bbondy force-pushed the widevine branch from 7e74428 to 32d0572 Compare June 26, 2018 14:11

bbondy changed the title ~~WIP: Support Widevine~~ Support Widevine Jun 26, 2018

bbondy force-pushed the widevine branch 4 times, most recently from ed94d9f to 74ab2cb Compare June 26, 2018 15:00

bbondy removed the pr/work-in-progress label Jun 26, 2018

bbondy force-pushed the widevine branch from 74ab2cb to 9c83450 Compare June 26, 2018 18:08

bbondy commented Jun 26, 2018

View reviewed changes

bbondy requested a review from emerick June 26, 2018 18:25

bbondy force-pushed the widevine branch from 9c83450 to 3f04ac7 Compare June 26, 2018 18:38

bbondy changed the title ~~Support Widevine~~ Support Widevine and 2 component updaters Jun 26, 2018

emerick previously approved these changes Jun 26, 2018

View reviewed changes

bbondy dismissed emerick’s stale review via 6f4c533 June 26, 2018 20:21

bbondy force-pushed the widevine branch from 3f04ac7 to 6f4c533 Compare June 26, 2018 20:21

emerick previously approved these changes Jun 26, 2018

View reviewed changes

bridiver reviewed Jun 26, 2018

View reviewed changes

bbondy added 7 commits June 26, 2018 20:23

Add new string resources

af4ccff

Have both a Brave and Google component updater

0484b37

Patch chrome/browser/ui/browser.cc for buble model delegate

0a7ea9a

Patch chrome/browser/ui/content_settings/content_setting_image_model.cc

b95f4f2

This patches to generate Brave content setting image models for the navigation URL bar

Override chromium_src/chrome/browser/component_updater/widevine_cdm_c…

64cd217

…omponent_installer.cc This overrides the RegisterWidevineCdmComponent to first check if Widevine has been opted in before proceeding

Add pref to make sure Widevine was opted into by users

fbc6ebd

The pref is a boolean pref by the name: brave.widevine_opted_in

Remove override for Google component updater URL

c68c84a

Because there are 2 different component updatears now, and each has its own configuration.

bbondy added 2 commits June 26, 2018 20:24

Add content setting icon and bubble models

9cc7479

Add tests for Widevine models, pref and exceptions

e35fcf4

bbondy dismissed emerick’s stale review via e35fcf4 June 27, 2018 00:24

bbondy force-pushed the widevine branch from 6f4c533 to e35fcf4 Compare June 27, 2018 00:24

bbondy merged commit f03caed into master Jun 27, 2018

bsclifton deleted the widevine branch June 27, 2018 16:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support Widevine and 2 component updaters #189

Support Widevine and 2 component updaters #189

bbondy commented Jun 22, 2018 •

edited

Loading

bbondy Jun 26, 2018

emerick left a comment

emerick Jun 26, 2018

bbondy Jun 26, 2018

emerick Jun 26, 2018

bbondy Jun 26, 2018

emerick Jun 26, 2018

bbondy Jun 26, 2018

emerick left a comment

bridiver Jun 26, 2018

bbondy Jun 26, 2018

George9000 commented Nov 6, 2018

bbondy commented Nov 6, 2018

Support Widevine and 2 component updaters #189

Support Widevine and 2 component updaters #189

Conversation

bbondy commented Jun 22, 2018 • edited Loading

Summary

What is looks like

Submitter Checklist:

Test Plan:

Reviewer Checklist:

Choose a reason for hiding this comment

emerick left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emerick left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

George9000 commented Nov 6, 2018

bbondy commented Nov 6, 2018

bbondy commented Jun 22, 2018 •

edited

Loading