Autoplay: Add Chrome policy to whitelist origins to autoplay [chromium/src : master]

Becca Hughes (Gerrit)

unread,

Mar 15, 2018, 11:19:44 AM3/15/18

to asvitki...@chromium.org, feature-me...@chromium.org, Mounir Lamouri, Commit Bot, chromium...@chromium.org

Patch Set 2:
(2 comments)
In addition of the comments below, to make we are on the same page with regards to what's going to happen: if the top frame is whitelisted, it will be allowed to autoplay and all its iframes will also be allowed to autoplay, it basically disables the policy for the entire tab. Does that match your expectations? I wonder if we could test this.

Yes the autoplay bypass should be inherited by iframes. I have added tests to cover this.

View Change

2 comments:

File chrome/browser/chrome_content_browser_client.cc:
- Patch Set #2, Line 609: ContentSettingsPattern pattern =
  I see some places that use the whitelist as ContentSettingsPattern, like https://cs.chromium. […]
  Done
File chrome/browser/policy/policy_browsertest.cc:
- Patch Set #2, Line 5093: return embedded_test_server2_.get();
  EXPECT_EQ(expected, actual) (same below) ... […]
  Done

To view, visit change 960666. To unsubscribe, or for help writing mail filters, visit settings.

Mounir Lamouri (Gerrit)

unread,

Mar 16, 2018, 8:15:05 AM3/16/18

to Becca Hughes, asvitki...@chromium.org, feature-me...@chromium.org, Commit Bot, chromium...@chromium.org

lgtm -- I would recommend renaming the pref but up to you.

Patch set 3:Code-Review +1

View Change

2 comments:

File chrome/browser/chrome_content_browser_client.cc:
- Patch Set #3, Line 601: IsOriginWhitelisted
  nit: IsURLWhitelisted
File chrome/common/pref_names.cc:
- Patch Set #3, Line 2569: const char kAutoplayOriginWhitelist[] = "media.autoplay_origin_whitelist";
  nit: maybe drop "origin" from the name?

To view, visit change 960666. To unsubscribe, or for help writing mail filters, visit settings.

Becca Hughes (Gerrit)

unread,

Mar 16, 2018, 9:40:41 AM3/16/18

to asvitki...@chromium.org, feature-me...@chromium.org, Maksim Ivanov, Mounir Lamouri, Commit Bot, chromium...@chromium.org

This change is ready for review.

Patch set 4:Commit-Queue +1

View Change

2 comments:

File chrome/browser/chrome_content_browser_client.cc:
- Patch Set #3, Line 601: co
  nit: IsURLWhitelisted
  Done
File chrome/common/pref_names.cc:
- Patch Set #3, Line 2569: const char kAutoplayWhitelist[] = "media.autoplay_whitelist";

- nit: maybe drop "origin" from the name?

- Done

To view, visit change 960666. To unsubscribe, or for help writing mail filters, visit settings.

François Beaufort (Gerrit)

unread,

Mar 19, 2018, 3:48:34 AM3/19/18

to Becca Hughes, asvitki...@chromium.org, feature-me...@chromium.org, Maksim Ivanov, Mounir Lamouri, Commit Bot, chromium...@chromium.org

View Change

1 comment:

File components/policy/resources/policy_templates.json:
- Patch Set #4, Line 11521: 'schema': { 'type': 'boolean' },
  I've seen some pattern like below for `RestoreOnStartupURLs`. WDYT?
```
      'type': 'list',
      'schema': {
        'type': 'array',
        'items': { 'type': 'string' },
      },
```

To view, visit change 960666. To unsubscribe, or for help writing mail filters, visit settings.

Becca Hughes (Gerrit)

unread,

Mar 19, 2018, 5:52:52 AM3/19/18

to asvitki...@chromium.org, feature-me...@chromium.org, François Beaufort, Maksim Ivanov, Mounir Lamouri, Commit Bot, chromium...@chromium.org

View Change

1 comment:

File components/policy/resources/policy_templates.json:

- Patch Set #4, Line 11521: 'schema': {

- I've seen some pattern like below for `RestoreOnStartupURLs`. WDYT? […]
  Nice catch!

To view, visit change 960666. To unsubscribe, or for help writing mail filters, visit settings.

Maksim Ivanov (Gerrit)

unread,

Mar 19, 2018, 5:13:46 PM3/19/18

to Becca Hughes, asvitki...@chromium.org, feature-me...@chromium.org, François Beaufort, Mounir Lamouri, Commit Bot, chromium...@chromium.org

Have you considered superseding the old policy AutoplayAllowed with this new whitelist policy? For example, "AutoplayAllowed==True" would match to "AutoplayWhitelist=['*']", "AutoplayAllowed==False" would match to "AutoplayWhitelist=[]". The benefit - this allows to not spend time on taking care of interaction between the two policies.
The drawback is that, once that merging is done, it becomes hard to invert the default Chrome behavior (because there'll be no way to explicitly configure "autoplay is always disallowed").

View Change

11 comments:

File chrome/browser/policy/policy_browsertest.cc:
- Patch Set #5, Line 5130:
```
embedded_test_server2_ = std::make_unique<net::EmbeddedTestServer>();
    embedded_test_server2()->AddDefaultHandlers(
        base::FilePath(FILE_PATH_LITERAL("chrome/test/data")));
    ASSERT_TRUE(embedded_test_server()->Start());
    ASSERT_TRUE(embedded_test_server2()->Start());
```
  nit: Does it make sense to move part or all of it into the fixture constructor?
  The usual recommendation is to use SetUp() only when it's required:
  https://github.com/google/googletest/blob/master/googletest/docs/FAQ.md#should-i-use-the-constructordestructor-of-the-test-fixture-or-the-set-uptear-down-function
- Patch Set #5, Line 5181: Embedded test server, cheap to create, started on demand
  nit: This comment needs some adjustment - this server is always started in this test. Also it's good to highlight that this is a second instance of test server, in addition to the BrowserTestBase's one.
- Patch Set #5, Line 5236: whitelist.push_back(base::Value("https://www.example.com"));
  The added tests seem to only cover the case of exact match, while the implementation actually uses pattern matching. Therefore please add at least one test that makes use of a non-trivial pattern.
- Patch Set #5, Line 5249:
  Please add tests that verifies the autoplay functionality when *both* autoplay policies are set. There are several configurations possible here.
File components/policy/resources/policy_templates.json:
- Patch Set #5, Line 11512: If the policy is set to False, <ph name="PRODUCT_NAME">$1<ex>Google Chrome</ex></ph> is not allowed to autoplay media.
  Seems like this description needs to be adjusted to consider the AutoplayWhitelist policy.
- Patch Set #5, Line 11513: By default, <ph name="PRODUCT_NAME">$1<ex>Google Chrome</ex></ph> is not allowed to autoplay media.
  Ditto for taking AutoplayWhitelist into account here.
- Patch Set #5, Line 11525: 'chrome.win:66-', 'chrome.linux:66-', 'chrome.mac:66-'
  nit: Replace "win"/"linux"/"mac" items with just "chrome.*".
- Patch Set #5, Line 11534: Allows you to control if videos can play automatically (without user consent) with audio content in <ph name="PRODUCT_NAME">$1<ex>Google Chrome</ex></ph> on a whitelist of urls.
  This sentence is a bit vague: the policy is not controlling "if autoplay is on for the whitelisted urls", but is actually controlling the whitelist of URLs, for which the autoplay is always enabled. So please restructure this sentence.
- Patch Set #5, Line 11535:
  Please document the format of the policy. According to the code, the items are perceived as patterns. Does the page [1] contain relevant information? Then please refer to it in the description.
  [1] https://www.chromium.org/administrators/url-blacklist-filter-format
- Patch Set #5, Line 11537: ''',
  Please also document how this new policy interacts with the AutoplayAllowed policy.
- Patch Set #5, Line 11537:
  nit: Better to remove these extra spaces and the line break, as this makes translators' job harder.

To view, visit change 960666. To unsubscribe, or for help writing mail filters, visit settings.

Becca Hughes (Gerrit)

unread,

Mar 20, 2018, 6:25:07 AM3/20/18

to asvitki...@chromium.org, feature-me...@chromium.org, Maksim Ivanov, François Beaufort, Mounir Lamouri, Commit Bot, chromium...@chromium.org

I have unified the logic for AutoplayAllowed and AutoplayWhitelist to take this into account. Disallowing autoplay is the new default in Chrome so all a user would have to do is not defined this policy.

Patch set 5:Commit-Queue +1

View Change

11 comments:

File chrome/browser/policy/policy_browsertest.cc:

Patch Set #5, Line 5130:

embedded_test_server2_ = std::make_unique<net::EmbeddedTestServer>();
    embedded_test_server2()->AddDefaultHandlers(
        base::FilePath(FILE_PATH_LITERAL("chrome/test/data")));
    ASSERT_TRUE(embedded_test_server()->Start());
    ASSERT_TRUE(embedded_test_server2()->Start());

- nit: Does it make sense to move part or all of it into the fixture constructor? […]
  Done

- Patch Set #5, Line 5181: Embedded test server, cheap to create, started on demand

- nit: This comment needs some adjustment - this server is always started in this test. […]
  Done

- Patch Set #5, Line 5236: whitelist.push_back(base::Value("https://www.example.com"));

- The added tests seem to only cover the case of exact match, while the implementation actually uses p […]
  Done

- Patch Set #5, Line 5249:

- Please add tests that verifies the autoplay functionality when *both* autoplay policies are set. […]
  Done
File components/policy/resources/policy_templates.json:

- Patch Set #5, Line 11512: If the policy is set to False, <ph name="PRODUCT_NAME">$1<ex>Google Chrome</ex></ph> is not allowed to autoplay media.
  Seems like this description needs to be adjusted to consider the AutoplayWhitelist policy.

- Done

- Patch Set #5, Line 11513: By default, <ph name="PRODUCT_NAME">$1<ex>Google Chrome</ex></ph> is not allowed to autoplay media.
  Ditto for taking AutoplayWhitelist into account here.

- Done

- Patch Set #5, Line 11525: 'chrome.win:66-', 'chrome.linux:66-', 'chrome.mac:66-'
  nit: Replace "win"/"linux"/"mac" items with just "chrome.*".

- Done

- Patch Set #5, Line 11534: Allows you to control if videos can play automatically (without user consent) with audio content in <ph name="PRODUCT_NAME">$1<ex>Google Chrome</ex></ph> on a whitelist of urls.

- This sentence is a bit vague: the policy is not controlling "if autoplay is on for the whitelisted u […]
  Done

- Patch Set #5, Line 11535:

- Please document the format of the policy. […]
  Done

- Patch Set #5, Line 11537: ''',
  Please also document how this new policy interacts with the AutoplayAllowed policy.

- Done

- Patch Set #5, Line 11537:
  nit: Better to remove these extra spaces and the line break, as this makes translators' job harder.

- Done

To view, visit change 960666. To unsubscribe, or for help writing mail filters, visit settings.

Mounir Lamouri (Gerrit)

unread,

Mar 20, 2018, 8:54:53 AM3/20/18

to Becca Hughes, asvitki...@chromium.org, feature-me...@chromium.org, Maksim Ivanov, François Beaufort, Commit Bot, chromium...@chromium.org

View Change

1 comment:

File chrome/common/pref_names.cc:
- Patch Set #6, Line 2568: origins
  nit: not origins. maybe use the same terms as other whitelist entrieS?

To view, visit change 960666. To unsubscribe, or for help writing mail filters, visit settings.

Becca Hughes (Gerrit)

unread,

Mar 20, 2018, 9:39:20 AM3/20/18

to asvitki...@chromium.org, feature-me...@chromium.org, Mounir Lamouri, Maksim Ivanov, François Beaufort, Commit Bot, chromium...@chromium.org

View Change

1 comment:

File chrome/common/pref_names.cc:
- Patch Set #6, Line 2568: L patte
  nit: not origins. […]
  Done

To view, visit change 960666. To unsubscribe, or for help writing mail filters, visit settings.

Maksim Ivanov (Gerrit)

unread,

Mar 20, 2018, 1:01:27 PM3/20/18

to Becca Hughes, asvitki...@chromium.org, feature-me...@chromium.org, Mounir Lamouri, François Beaufort, Commit Bot, chromium...@chromium.org

Thanks for adding elaborate tests.
I think you could merge some of them together, so that the policies setting is interleaved with testing, allowing to cover the same set of combinations with fewer test cases. But that's up to you if you want to have a separate test case for each combination.

View Change

6 comments:

File chrome/browser/chrome_content_browser_client.cc:
- Patch Set #7, Line 2848: autoplay_allowed_by_policy
  nit: Reusing the same variable for two different values seems a bit confusing. Maybe extract all this logic into a separate function? Then it'll be possible to write it in a more linear fashion, like that:
```
bool IsAutoplayAllowedByPolicy(...) {
  if (prefs->GetBoolean(prefs::kAutoplayAllowed) && ...)
    return true;
  if (!contents)
    return false;
  const base::ListValue* autoplay_whitelist = ...
  return autoplay_whitelist && prefs->IsManagedPreference(...) && IsURLWhitelisted(...);
}
```
File chrome/browser/policy/policy_browsertest.cc:
- Patch Set #7, Line 5127: : PolicyTest()
  nit: Just omit this.
- Patch Set #7, Line 5180: std::unique_ptr<
  nit: Now that this is initialized in constructor, there's no more need in unique_ptr.
- Patch Set #7, Line 5278: allow
  nit: s/allow/forbid/
  Ditto for the copy-pasted blocks in the following tests.
- Patch Set #7, Line 5284: allowed by policy
  s/allowed by policy/not allowed/
  Ditto for the copy-pasted blocks in the following tests.
File components/policy/resources/policy_templates.json:
- Patch Set #7, Line 11542: If the AutoplayaAllowed policy is set to False then any URL patterns set in this policy will still be allowed to play.
  nit: s/AutoplayaAllowed/AutoplayAllowed/

To view, visit change 960666. To unsubscribe, or for help writing mail filters, visit settings.

Becca Hughes (Gerrit)

unread,

Mar 20, 2018, 1:26:28 PM3/20/18

to asvitki...@chromium.org, feature-me...@chromium.org, Maksim Ivanov, Mounir Lamouri, François Beaufort, Commit Bot, chromium...@chromium.org

View Change

6 comments:

File chrome/browser/chrome_content_browser_client.cc:

- Patch Set #7, Line 2848: autoplay_allowed_by_policy

- nit: Reusing the same variable for two different values seems a bit confusing. […]
  Done
File chrome/browser/policy/policy_browsertest.cc:

- Patch Set #7, Line 5127: : PolicyTest()
  nit: Just omit this.

- Done

- Patch Set #7, Line 5180: std::unique_ptr<
  nit: Now that this is initialized in constructor, there's no more need in unique_ptr.

- Done

- Patch Set #7, Line 5278: allow

- nit: s/allow/forbid/ […]
  Done

- Patch Set #7, Line 5284: allowed by policy

- s/allowed by policy/not allowed/ […]
  Done
File components/policy/resources/policy_templates.json:

- Patch Set #7, Line 11542: If the AutoplayaAllowed policy is set to False then any URL patterns set in this policy will still be allowed to play.
  nit: s/AutoplayaAllowed/AutoplayAllowed/

- Done

To view, visit change 960666. To unsubscribe, or for help writing mail filters, visit settings.

Maksim Ivanov (Gerrit)

unread,

Mar 20, 2018, 2:11:24 PM3/20/18

to Becca Hughes, asvitki...@chromium.org, feature-me...@chromium.org, Mounir Lamouri, François Beaufort, Commit Bot, chromium...@chromium.org

Patch set 8:Code-Review +1

View Change

To view, visit change 960666. To unsubscribe, or for help writing mail filters, visit settings.

Becca Hughes (Gerrit)

unread,

Mar 21, 2018, 5:37:45 AM3/21/18

to asvitki...@chromium.org, feature-me...@chromium.org, Maksim Ivanov, Mounir Lamouri, François Beaufort, Commit Bot, chromium...@chromium.org

Patch set 8:Commit-Queue +2

View Change

To view, visit change 960666. To unsubscribe, or for help writing mail filters, visit settings.

Commit Bot (Gerrit)

unread,

Mar 21, 2018, 7:39:05 AM3/21/18

to Becca Hughes, asvitki...@chromium.org, feature-me...@chromium.org, Maksim Ivanov, Mounir Lamouri, François Beaufort, chromium...@chromium.org

Try jobs failed on following builders:
  win7_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win7_chromium_rel_ng/builds/126914)

View Change

To view, visit change 960666. To unsubscribe, or for help writing mail filters, visit settings.

Becca Hughes (Gerrit)

unread,

Mar 21, 2018, 7:47:18 AM3/21/18

to asvitki...@chromium.org, feature-me...@chromium.org, Maksim Ivanov, Mounir Lamouri, François Beaufort, Commit Bot, chromium...@chromium.org

Patch set 8:Commit-Queue +2

View Change

To view, visit change 960666. To unsubscribe, or for help writing mail filters, visit settings.

Commit Bot (Gerrit)

unread,

Mar 21, 2018, 9:37:03 AM3/21/18

to Becca Hughes, asvitki...@chromium.org, feature-me...@chromium.org, Maksim Ivanov, Mounir Lamouri, François Beaufort, chromium...@chromium.org

Commit Bot merged this change.

View Change

Approvals:
  Mounir Lamouri: Looks good to me
  Maksim Ivanov: Looks good to me
  Becca Hughes: Commit

Autoplay: Add Chrome policy to whitelist origins to autoplay

Add a 'AutoplayOriginsWhitelist' managed policy setting that allows
autoplay on these origins (e.g. for internal enterprise websites).

BUG=821379

Change-Id: I7bde18ef1c68741af5645f184c910b779002fdf7
Reviewed-on: https://chromium-review.googlesource.com/960666
Reviewed-by: Maksim Ivanov <em...@chromium.org>
Reviewed-by: Mounir Lamouri <mlam...@chromium.org>
Commit-Queue: Becca Hughes <becca...@chromium.org>
Cr-Commit-Position: refs/heads/master@{#544682}
---
M chrome/browser/chrome_content_browser_client.cc
M chrome/browser/policy/configuration_policy_handler_list_factory.cc
M chrome/browser/policy/policy_browsertest.cc
M chrome/common/pref_names.cc
M chrome/common/pref_names.h
A chrome/test/data/media/autoplay_iframe.html
M chrome/test/data/policy/policy_test_cases.json
M components/policy/resources/policy_templates.json
M tools/metrics/histograms/enums.xml
9 files changed, 354 insertions(+), 16 deletions(-)

To view, visit change 960666. To unsubscribe, or for help writing mail filters, visit settings.

Reply all

Reply to author

Forward