Enable granular control of autoplay #222

darkdh · 2018-07-05T22:05:47Z

by following steps:

Add AutoplayPermissionContext
Update PermissionDescriptorToPermissionType
RquestPermission from AutoplayPolicy
Content setting page & blocked status bubble
Auto refresh when clicking allow in permission ask bubble

fix brave/brave-browser#281

Auditors: @bbondy, @bridiver

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:

Automatic:

yarn test brave_unit_tests --filter=AutoplayPermissionContextTests.*
yarn test brave_browser_tests --filter=AutoplayPermissionContextBrowserTest.*
yarn test brave_browser_tests --filter=BraveAutoplayBlockedImageModelTest.*

Manual:
a. Allow autoplay

Go to https://www.cnet.com/ pick a random article
There should be a bubble shows up asking allow/block www.cnet.com to autoplay media
Click allow and the page should auto refresh and video should play
Check Autoplay site setting should be allow
4.a In site detail
4.b In site content setting
4.c In global content setting
Next time you visit cnet.com, the video will be autoplay automatically

b. Block autoplay

Same steps as a.
Except click block will not refresh the page
Notice when video is blocked there will be an indicator on urlbar

c. General behavior

every time you change site content setting, there will be a banner shows up

2. If you close permission bubble, that would be automatically blocked

3. Open any local media files should allow autoplay by default

Reviewer Checklist:

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

darkdh · 2018-07-05T22:07:34Z

patches/third_party-blink-renderer-core-html-BUILD.gn.patch

+     "list_item_ordinal.h",
+     "media/autoplay_policy.cc",
+     "media/autoplay_policy.h",
+    "//third_party/blink/renderer/modules/permissions/permission_utils.cc",


blink/renderer/modules depends on blink/renderer/core so if I add deps here will introduce circular deps

Can you explain more? This seems really strange.

we use functions of third_party/blink/renderer/modules/permissions/permission_utils.h in third_party/blink/renderer/core/html/media/autoplay_policy.cc for requesting permission.
So we need to add dependency for "//third_party/blink/renderer/modules/permissions:permissions"
Unfortunately, it will cause this circular dependency error

ERROR Dependency cycle: //third_party/blink/renderer/core:core -> //third_party/blink/renderer/core/html:html -> //third_party/blink/renderer/modules/permissions:permissions -> //third_party/blink/renderer/core:core

but this will effectively build it twice and we don't want that. What build error do you get without it?

Undefined symbols for architecture x86_64: "blink::ConnectToPermissionService(blink::ExecutionContext*, mojo::InterfaceRequest<blink::mojom::blink::PermissionService>)", referenced from: blink::AutoplayPolicy::IsAutoplayAllowedPerSettings() const in autoplay_policy.o "blink::CreatePermissionDescriptor(blink::mojom::PermissionName)", referenced from: blink::AutoplayPolicy::IsAutoplayAllowedPerSettings() const in autoplay_policy.o

fixed in 2d27548

bbondy · 2018-07-10T16:48:11Z

Some comments for a v2 iteration, not blocking this review, it is still in progress.

Noting that a user might get almost as annoyed with autoplay as with a popup that asks them if they want to allow autoplay. Also when clicking allow on that popup, I know this is just for now, but reloading the page is probably also painful for users.

An alternative to reloading that I think is better is displaying this bar:

But even that might be annoying. URL bar icon only is maybe enough.

bbondy

Didn't have time to make suggestions on everything but we need to try to reduce the patches more. One thing to note is patch and call into src/brave is better than patch and don't. The reason is because maintaining patch files while we have multiple chromium versions is going to be harder for each .patch change. But if we just add in the hooks and then change things in src/brave, those are easy things to pull into different chromium versions that we maintain.

bbondy · 2018-07-10T17:31:26Z

patches/chrome-browser-permissions-permission_manager.cc.patch

+       return CONTENT_SETTINGS_TYPE_CLIPBOARD_WRITE;
+     case PermissionType::PAYMENT_HANDLER:
+       return CONTENT_SETTINGS_TYPE_PAYMENT_HANDLER;
+    case PermissionType::AUTOPLAY:


Maybe go with rename PermissionTypeToContentSetting to PermissionTypeToContentSetting_ChromiumImpl
in a patch and then chromium_src override to provide a proper definition that uses it. In that way we can add in more types later without touching patching from there on.

Alternately call into something like if(BravePermissionTypeToContentSetting(&permission) return permission` and then have the function inside src/brave.

overrides is not gonna work because there are callers of it in the same file. It will end up no one calling this function.
Second idea doesn't work either unless we turn off this compile time checking
error: enumeration value 'AUTOPLAY' not handled in switch [-Werror,-Wswitch]
error: 16 enumeration values not handled in switch: 'MIDI_SYSEX', 'NOTIFICATIONS', 'GEOLOCATION'... [-Werror,-Wswitch]

if we subclass PermissionManager and then override those caller method, it eliminates the patch but that would be too many copy logic. Not sure if it is worthy.

@darkdh it doesn't matter if there are callers in the same file. @bbondy he can redefine PermissionTypeToContentSetting, the problem is actually that he can't reference the original method because it's in an anonymous namespace so he'd have to replace the entire thing

@darkdh Chrome already has content settings for allowing sites to play sound, can't we use those?

actually didn't Chrome briefly turn off autoplay by default? Can't we just turn back on whatever they turned off? Seems odd that we would need our own permission for something that is already in chrome

sound content setting doesn't have capacity to stop video from autoplaying.
Chrome's autoplay is globally defined, users can only change the global autoplay policy

And Chrome also default allows muted video and there is no control of content settings for different sites for users

actually patch one line default: NOTREACHED() should allow us to get around [-Werror,-Wswitch]

fixed in 1154635

bbondy · 2018-07-10T18:49:43Z

patches/chrome-browser-permissions-permission_manager.cc.patch

+   permission_contexts_[CONTENT_SETTINGS_TYPE_PAYMENT_HANDLER] =
+       std::make_unique<payments::PaymentHandlerPermissionContext>(profile);
+  permission_contexts_[CONTENT_SETTINGS_TYPE_AUTOPLAY] =
+      std::make_unique<AutoplayPermissionContext>(profile);


I prefer we just use a subclass of PermissionManager and patch instead to instantiate it.
Allowing less patching in the future has value because we'll need to for example rebase changes that land across multiple Chromium versions.

addressed in ea9e693

bbondy · 2018-07-11T03:05:03Z

patches/content-child-runtime_features.cc.patch

+     WebRuntimeFeatures::EnableWebNfc(true);
+ #endif
+
+#if !defined(BRAVE_CHROMIUM_BUILD)


Instead of patching here can you use chromium_src override and rename away SetRuntimeFeaturesDefaultsAndUpdateFromArgs to SetRuntimeFeaturesDefaultsAndUpdateFromArgs_ChromiumImpl? Then adjust the value of WebRuntimeFeatures::EnableAutoplayMutedVideos as you want after your SetRuntimeFeaturesDefaultsAndUpdateFromArgs calls SetRuntimeFeaturesDefaultsAndUpdateFromArgs_Impl

addressed in e4cc08b

bbondy · 2018-07-11T03:07:27Z

patches/components-content_settings-core-browser-content_settings_registry.cc.patch

+                WebsiteSettingsRegistry::PLATFORM_ANDROID,
+            ContentSettingsInfo::INHERIT_IN_INCOGNITO);
+
+-  Register(CONTENT_SETTINGS_TYPE_AUTOPLAY, "autoplay", CONTENT_SETTING_ALLOW,


I think you can use chromium_src override here and define your own Register function that calls the ChromiumImpl passing on the same args for everything except for when "autoplay" is encountered, it does what you do here.

fixed in a85e50e

bbondy · 2018-07-11T03:42:55Z

patches/media-base-media_switches.cc.patch

+
+   if (base::FeatureList::IsEnabled(media::kUnifiedAutoplay))
+#if defined(BRAVE_CHROMIUM_BUILD)
+    return switches::autoplay::kUserGestureRequiredPolicy;


chromium_src override for this please.

fixed in 2f47e35

bbondy · 2018-07-11T04:08:22Z

patches/chrome-browser-ui-webui-site_settings_helper.cc.patch

+     {CONTENT_SETTINGS_TYPE_SENSORS, "sensors"},
+     {CONTENT_SETTINGS_TYPE_PAYMENT_HANDLER, "payment-handler"},
+     {CONTENT_SETTINGS_TYPE_USB_GUARD, "usb-devices"},
+    {CONTENT_SETTINGS_TYPE_AUTOPLAY, "autoplay"},


I prefer chromium_src override method here for HasRegisteredGroupName, ContentSettingsTypeFromGroupName, ContentSettingsTypeToGroupName here. Create own array built from filtered kContentSettingsTypeGroupNames

fixed in 6557fd8

bbondy · 2018-07-11T04:09:37Z

patches/third_party-blink-renderer-core-html-BUILD.gn.patch

+     "list_item_ordinal.h",
+     "media/autoplay_policy.cc",
+     "media/autoplay_policy.h",
+    "//third_party/blink/renderer/modules/permissions/permission_utils.cc",


Can you explain more? This seems really strange.

bbondy · 2018-07-11T04:12:02Z

patches/chrome-browser-ui-webui-settings-md_settings_localized_strings_provider.cc.patch

+@@ -2080,6 +2081,14 @@ void AddSiteSettingsStrings(content::WebUIDataSource* html_source,
+      IDS_SETTINGS_SITE_SETTINGS_PROTECTED_CONTENT_IDENTIFIERS},
+     {"siteSettingsProtectedContentEnable",
+      IDS_SETTINGS_SITE_SETTINGS_PROTECTED_CONTENT_ENABLE},


Maybe rename in patch file for AddSiteSettingsStrings and chromium_src override to use it and add your own things to it.

already there, just forgot to remove patch

fixed in 2f47e35

updated fix 2ead047

bbondy · 2018-07-11T04:19:16Z

patches/chrome-browser-push_messaging-push_messaging_service_impl.cc.patch

+   // Push does not allow permission requests from iframes.
+   PermissionManager::Get(profile_)->RequestPermission(
+-      CONTENT_SETTINGS_TYPE_NOTIFICATIONS, web_contents->GetMainFrame(),
+      CONTENT_SETTINGS_TYPE_AUTOPLAY, web_contents->GetMainFrame(),


Did you mean to do this in addition?
Maybe subclass for this and patch the creation.

ah, I forgot to remove this

fixed in 2f47e35

bbondy · 2018-07-11T04:22:34Z

patches/chrome-browser-permissions-permission_uma_util.cc.patch

+     case PermissionRequestType::PERMISSION_PAYMENT_HANDLER:
+       return "PaymentHandler";
+    case PermissionRequestType::PERMISSION_AUTOPLAY:
+      return "Autoplay";


It seems like this is only used by RecordEngagementMetric, is that needed? Maybe if PERMISSION_AUTOPLAY was just a separate int which is cast where needed it would simplify needing to have all enum values specified?

NOTREACHED() after this will CHECK failed and crash browser if we don't handle this case.
And I prefer putting it into enum because it can save us from something that we don't handle in compile time when upstream add other usage somewhere.

fixed in 462c5d2

bridiver · 2018-07-31T00:53:25Z

patches/third_party-blink-renderer-core-html-media-autoplay_policy.cc.patch

+     return false;
+
+  // brave(darkdh): default allow local files
+  if (element_->GetDocument().origin() == String("null") || element_->GetDocument().origin() == String("file://"))


I think this can be done with a default content setting

1. Add AutoplayPermissionContext 2. Update PermissionDescriptorToPermissionType 3. RquestPermission from AutoplayPolicy 4. Content setting page & blocked status bubble 5. Auto refresh when clicking allow in permission ask bubble

muted video

…pl::GetIconId overrides

@z

There were link errors on Windows ``` ../../third_party/llvm-build/Release+Asserts/bin\lld-link.exe: error: undefined symbol: ?GetForProfile@PermissionManagerFactory@@SAPEAVPermissionManager@@PEAVProfile@@@z >>> referenced by C:\brave-browser\src\chrome\browser\permissions\permission_manager.cc:264 >>> browser.lib(permission_manager.obj):(?Get@PermissionManager@@SAPEAV1@PEAVProfile@@@z) ../../third_party/llvm-build/Release+Asserts/bin\lld-link.exe: error: undefined symbol: ?GetForProfile@PermissionManagerFactory@@SAPEAVPermissionManager@@PEAVProfile@@@z >>> referenced by C:\brave-browser\src\chrome\browser\budget_service\budget_service_impl.cc:56 >>> browser.lib(budget_service_impl.obj):(?GetBudget@BudgetServiceImpl@@UEAAXV?$OnceCallback@$$A6AXW4BudgetServiceErrorType@mojom@blink@@v?$vector@V?$InlinedStructPtr@VBudgetState@mojom@blink@@@mojo@@v?$allocator@V?$InlinedStructPtr@VBudgetState@mojom@blink@@@mojo@@@std@@@std@@@z@base@@@z) ../../third_party/llvm-build/Release+Asserts/bin\lld-link.exe: error: undefined symbol: ?GetForProfile@PermissionManagerFactory@@SAPEAVPermissionManager@@PEAVProfile@@@z >>> referenced by C:\brave-browser\src\chrome\browser\profiles\profile_impl.cc:1094 >>> browser.lib(profile_impl.obj):(?GetPermissionControllerDelegate@ProfileImpl@@UEAAPEAVPermissionControllerDelegate@content@@xz) ../../third_party/llvm-build/Release+Asserts/bin\lld-link.exe: error: undefined symbol: ?GetForProfile@PermissionManagerFactory@@SAPEAVPermissionManager@@PEAVProfile@@@z >>> referenced by C:\brave-browser\src\chrome\browser\profiles\off_the_record_profile_impl.cc:450 >>> browser.lib(off_the_record_profile_impl.obj):(?GetPermissionControllerDelegate@OffTheRecordProfileImpl@@UEAAPEAVPermissionControllerDelegate@content@@xz) ``` because b4842cc#diff-e9e16c8776e75c58827f5f1af6feac31R1 also changes signature of `PermissionManagerFactory::GetForProfile` but we only need `PermissionManagerFactory::BuildServiceInstanceFor`

darkdh · 2018-08-07T05:05:56Z

build and test pass on all platforms.

bbondy · 2018-08-07T07:41:38Z

browser/autoplay/autoplay_permission_context_unittest.cc

+#include "content/public/test/mock_render_process_host.h"
+#include "content/public/test/web_contents_tester.h"
+#include "testing/gtest/include/gtest/gtest.h"
+


nit: 1 newline

bbondy · 2018-08-07T07:42:01Z

browser/autoplay/autoplay_permission_context_unittest.cc

+                .content_setting);
+}
+
+// There is no way to genearte a request that is automatically accepted in


nit: generate

bbondy · 2018-08-07T07:42:24Z

browser/autoplay/autoplay_permission_context_unittest.cc

+}
+
+// There is no way to genearte a request that is automatically accepted in
+// unittest by RequestPermission so we test reverse cases here


nit: // unittest by RequestPermission, so we test reverse cases here

bbondy · 2018-08-07T07:45:48Z

chromium_src/chrome/browser/permissions/permission_uma_util.cc

+  return GetPermissionRequestString_ChromiumImpl(type);
+}
+
+}


nit:
} // namespace

bbondy · 2018-08-07T07:46:33Z

chromium_src/chrome/browser/permissions/permission_uma_util.cc

+  }
+}
+
+}


nit:
} // namespace

bbondy · 2018-08-07T07:47:07Z

chromium_src/chrome/browser/ui/webui/settings/md_settings_localized_strings_provider.cc

+  BraveAddCommonStrings(html_source, profile);
+}
+
+}


nit:
} // namespace settings

I will stop commenting on those but apply everywhere please.

bbondy · 2018-08-07T07:48:56Z

patches/chrome-browser-content_settings-tab_specific_content_settings.cc.patch

+       content_type != CONTENT_SETTINGS_TYPE_PPAPI_BROKER &&
+       content_type != CONTENT_SETTINGS_TYPE_MIDI_SYSEX &&
+       content_type != CONTENT_SETTINGS_TYPE_CLIPBOARD_READ &&
+      content_type != CONTENT_SETTINGS_TYPE_AUTOPLAY &&


we can probably improve on this patch in a follow up.

brave/brave-browser#699

bbondy

What you mentioned as followups sounds good, but please post a new issue for those and mark this issue as fixed by this PR.

darkdh · 2018-08-07T16:06:27Z

Followup issue link brave/brave-browser#699

darkdh added the pr/work-in-progress label Jul 5, 2018

darkdh self-assigned this Jul 5, 2018

darkdh commented Jul 5, 2018

View reviewed changes

darkdh force-pushed the autoplay-rebase branch 2 times, most recently from c9555b0 to b4ae901 Compare July 10, 2018 01:35

darkdh changed the title ~~WIP: autoplay~~ Enable granular control of autoplay Jul 10, 2018

darkdh requested review from bbondy and bridiver July 10, 2018 02:18

darkdh removed the pr/work-in-progress label Jul 10, 2018

bbondy requested changes Jul 11, 2018

View reviewed changes

darkdh force-pushed the autoplay-rebase branch 2 times, most recently from e9bb454 to 8f07aee Compare July 31, 2018 00:13

bridiver reviewed Jul 31, 2018

View reviewed changes

darkdh added 16 commits August 3, 2018 16:30

Enable granular control of autoplay by following steps

ec8267d

1. Add AutoplayPermissionContext 2. Update PermissionDescriptorToPermissionType 3. RquestPermission from AutoplayPolicy 4. Content setting page & blocked status bubble 5. Auto refresh when clicking allow in permission ask bubble

Default allow autoplay from local file

b084aa6

Add browser test

47e8d03

Add unit test

07678b6

Subclass PermissionManager

b4842cc

EnableAutoplayMutedVideos(false) without patch and add browser test for

faadd75

muted video

GetEffectiveAutoplayPolicy overrides and remove unnecessary patch

f0e40d0

Filter out autoplay from kContentSettingsTypeGroupNames

5c56000

permission_util.cc overrides

cff4a62

Remove blink permission request patch

1b48534

overrides anonymous namespace function

d6832e9

Use BraveInit() to unregister and register the default registry value

8cdd6dc

permission_uma_util patch removal

ce8109a

PermissionRequestImpl::GetMessageTextFragment and PermissionRequestIm…

184f8e3

…pl::GetIconId overrides

BraveAutoplayBlockedImageModel & BraveAutoplayContentSettingBubbleModel

0aaaf51

C69 patch rebase

cdbb29c

darkdh added 2 commits August 4, 2018 23:20

C69 rebase changes

3d3e4b6

Harden AutoplayPermissionContextBrowserTest

34fc976

darkdh force-pushed the autoplay-rebase branch from 82cb8e3 to 34fc976 Compare August 5, 2018 06:23

bbondy reviewed Aug 7, 2018

View reviewed changes

chromium_src/chrome/browser/permissions/permission_uma_util.cc Outdated

}

}

}

Copy link

Member

bbondy Aug 7, 2018

Choose a reason for hiding this comment

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

nit:
} // namespace

bbondy reviewed Aug 7, 2018

View reviewed changes

bbondy previously approved these changes Aug 7, 2018

View reviewed changes

lint fix

09feca1

darkdh dismissed bbondy’s stale review via 09feca1 August 7, 2018 16:04

bbondy approved these changes Aug 7, 2018

View reviewed changes

darkdh merged commit 409515d into master Aug 7, 2018

darkdh deleted the autoplay-rebase branch August 21, 2018 06:06

+                }
+              }
+              }

Enable granular control of autoplay #222

Enable granular control of autoplay #222

Conversation

darkdh commented Jul 5, 2018 • edited Loading

Submitter Checklist:

Test Plan:

Reviewer Checklist:

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

bbondy commented Jul 10, 2018

bbondy 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

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

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

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

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

darkdh commented Aug 7, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bbondy Aug 7, 2018 • edited Loading

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

bbondy left a comment

Choose a reason for hiding this comment

darkdh commented Aug 7, 2018

darkdh commented Jul 5, 2018 •

edited

Loading

bbondy Aug 7, 2018 •

edited

Loading