-
Notifications
You must be signed in to change notification settings - Fork 900
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
Support widevine in linux #1606
Conversation
7edb43d
to
1fbba6e
Compare
0331f22
to
678c528
Compare
I think implementation is ready to review. |
180718c
to
b3cef89
Compare
DVLOG(1) << __func__ << ": Widevine install completed w/o error"; | ||
// TODO(simonhong): This is very aggresive. | ||
// Showing relaunch dialog would be better. | ||
chrome::AttemptRelaunch(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bbondy I think there are many ways to make user restart.
- just restart after install is success like I did
- change content settings bubble string after install. In this case, blocked image is still visible to user and bubble has different text like
To use widevine, please restart browser
. - show relaunch dialog
Which one is best UX? Or please let me know if our UX team has different option.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@simonhong #2 please. That messaging only for Linux.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
d757642
to
f1c7dd1
Compare
0ab6e1f
to
6ef8f86
Compare
Widevine install is only downloaded when user accepts the use of widevine from google service via content bubble dialog. Then, it can be used after browser is restarted due to the use of zygote process in linux. Background update is only triggered when brave has different version of widevine with installed version. That means widevine_cdm_version.h included updated widevine version. This new version is also in effect after brwoser restart. We can update version string by changing widevine version in package.json. Of course, we could implement install and update w/o depending on version string. Instead it can be done by fetching latest version from google server. However, this could introduce many upstream file change because upstream code assumes version is not changed in runtime. BraveWidevineBundleManager manages widevine bundle's install state and it uses BraveWidevineBundleUnzipper to unzip downloaded zipped bundle. When installed, contents settings bubble text is changed to "restart".
6ef8f86
to
bf07b8c
Compare
Widevine install is only downloaded when user accepts the use of
widevine from google service via content bubble dialog. Then, it
can be used after browser is restarted due to the use of zygote
process in linux.
Background update is only triggered when brave has different
version of widevine with installed version. That means
widevine_cdm_version.h included updated widevine version.
This new version is also in effect after brwoser restart.
We can update version string by changing widevine version in
package.json.
Of course, we could implement install and update w/o depending on
version string. Instead it can be done by fetching latest version
from google server. However, this could introduce many upstream file
change because upstream code assumes version is not changed in runtime.
BraveWidevineBundleManager manages widevine bundle's install state and
it uses BraveWidevineBundleUnzipper to unzip downloaded zipped bundle.
Fix brave/brave-browser#413
Submitter Checklist:
npm test brave_unit_tests && npm test brave_browser_tests
) ongit rebase master
(if needed).git rebase -i
to squash commits (if needed).Test Plan:
yarn test brave_unit_tests --filter=BraveWidevineBundleManagerTest.*
For manual test, please refer to Test Plan in brave/brave-browser#3300
Reviewer Checklist: