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

Fix widevine install prompt is shown after using new profiles #3959

Merged
merged 4 commits into from
Nov 19, 2019

Conversation

simonhong
Copy link
Member

@simonhong simonhong commented Nov 11, 2019

Fix brave/brave-browser#6747

Install prompt is displayed if widevine is opt opted in by user and user
doesn't choose don't ask option of it.

Widevine cdm lib is loaded into cdm utility process after it installed.
And it is browser widely used not just for specific profile.
However, Brave has prompted widevine permission prompt when user loads
contents site(ex, netflix) in newly created profile.
If Widevine cdm lib is already installed by any profile, Brave should not
display it again because cdm lib is already used.

To fix this, two kWidevineOptedIn/kWidevineInstalledVersion are migrated
from profile prefs to local state.
When migration is needed, firstly active profile is used to get existing
pref values.

There is one more Widevine related pref. It's kAskWidevineInstall.
It would be good to migrate kAskWidevineInstall to local prefs also.
But left as-is in profile prefs also seems fine.
After Widevine is installed that prefs doesn't have much meaning and if not just click once to dont ask is sufficient. Also, it is convinient to implement settings option with profile prefs.

Submitter Checklist:

Test Plan:

yarn test brave_browser_tests --filter=*Widevine*
yarn test brave_unit_tests --filter=*Widevine*

Migration test steps

  1. Launch previous browser(that doesn't include this fix)
  2. Install Widevine and exit
  3. Launch latest browser that includes this fix
  4. Check Widevine install prompt is not displayed in default profile and any newly created profiles

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 force-pushed the widevine_prefs_migration branch 3 times, most recently from 6b1fa2f to 84d531f Compare November 11, 2019 10:06
@simonhong simonhong self-assigned this Nov 11, 2019
@simonhong simonhong force-pushed the widevine_prefs_migration branch 7 times, most recently from 72d9fef to 1dc70e5 Compare November 11, 2019 21:40
@simonhong simonhong changed the title Migrate widevine prefs from profile to local state Fix widevine install prompt is shown after using new profiles Nov 11, 2019
@simonhong simonhong added this to the 0.74.x - Nightly milestone Nov 11, 2019
@simonhong simonhong force-pushed the widevine_prefs_migration branch 2 times, most recently from 2be1eae to 5ddc33c Compare November 12, 2019 02:46
@simonhong simonhong marked this pull request as ready for review November 12, 2019 03:09
@simonhong simonhong added CI/skip Do not run CI builds (except noplatform) and removed CI/skip Do not run CI builds (except noplatform) labels Nov 12, 2019
@simonhong simonhong added CI/skip Do not run CI builds (except noplatform) and removed CI/skip Do not run CI builds (except noplatform) labels Nov 12, 2019
@simonhong simonhong added the CI/skip Do not run CI builds (except noplatform) label Nov 12, 2019
@simonhong
Copy link
Member Author

CI looks good with current commit. So, added skip tag.
After reviewing, will remove skip tag.

@@ -31,6 +32,10 @@
#include "brave/components/p3a/p3a_core_metrics.h"
#endif // !defined(OS_ANDROID)

#if BUILDFLAG(ENABLE_WIDEVINE_CDM_COMPONENT) || BUILDFLAG(BUNDLE_WIDEVINE_CDM)
Copy link
Collaborator

Choose a reason for hiding this comment

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

this should just be ENABLE_WIDEVINE

Install prompt is displayed if widevine is opt opted in by user and user
doesn't choose don't ask option of it.

Widevine cdm lib is loaded into cdm utility process after it installed.
And it is browser widely used not just for specific profile.
However, Brave has prompted widevine permission prompt when user loads
contents site(ex, netflix) in newly created profile.
If Widevine cdm lib is already installed by any profile, Brave should not
display it again because cdm lib is already used.

To fix this, two kWidevineOptedIn/kWidevineInstalledVersion are migrated
from profile prefs to local state.
When migration is needed, firstly active profile is used to get existing
pref values.

There is one more Widevine related pref. It's kAskWidevineInstall.
It would be good to migrate to local prefs also. but left as-is in profile
prefs also seems fine. After Widevine is installed that prefs doesn't have
much meaning and if not just click once to dont ask is sufficient. Also, it
is convinient to implement settings option with profile prefs.
@simonhong simonhong force-pushed the widevine_prefs_migration branch 2 times, most recently from cba6c6e to 28eff92 Compare November 18, 2019 23:34
Add WidevinePrefsMigrationTest.PrefMigrationTest for prefs migration
test.
@simonhong simonhong added the CI/skip Do not run CI builds (except noplatform) label Nov 19, 2019
@simonhong
Copy link
Member Author

Last CI build was success and pushed one more commit for just adding comments.


using WidevinePrefsMigrationTest = InProcessBrowserTest;

IN_PROC_BROWSER_TEST_F(WidevinePrefsMigrationTest, PrefMigrationTest) {
Copy link
Member

Choose a reason for hiding this comment

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

Is it possible to add a PRE_PrefMigrationTest and set local_state to default value and also set profile value for kWidevineOptedIn and kWidevineInstalledVersion to verify the migrated values?

Copy link
Member Author

Choose a reason for hiding this comment

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

I tried that scenario at first.
But, local_state is not the default value at the second launching.(Of course, cleared local state in the first launching).
Not sure why it happens. I'm trying again.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm, local state seems not consistent during the spanning tests.
In the second launching, it's state is not the last state of first browser launching.
profile prefs shows consistent state but not local state during the spanning test.

Copy link
Member Author

Choose a reason for hiding this comment

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

local state is persisted but not persisted after clearing prefs(ClearPref()).
At second test, local state has the value before the clearing. I expected it has default value because ClearePref() is called but it's not default value at second test.
So, testing is done by explicitly calling MigrateWidevinePrefs() instead of spanning test.

@simonhong simonhong added CI/skip-linux CI/skip-macos-x64 Do not run CI builds for macOS x64 CI/skip-android Do not run CI builds for Android CI/skip-ios Do not run CI builds for iOS and removed CI/skip Do not run CI builds (except noplatform) CI/skip-linux CI/skip-macos-x64 Do not run CI builds for macOS x64 labels Nov 19, 2019
local state is persisted but not persisted after clearing prefs in the
spanning test.
At second test, local state has the value before the clearing.
I expected it has default value because ClearePref() is called
but it's not default value at second test.
So, testing is done by explicitly calling MigrateWidevinePrefs() instead of
spanning test.
@simonhong simonhong merged commit ce5c12c into master Nov 19, 2019
@simonhong simonhong deleted the widevine_prefs_migration branch November 19, 2019 12:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI/skip-android Do not run CI builds for Android CI/skip-ios Do not run CI builds for iOS
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Migrate widevine all prefs from profile to local_state
4 participants