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

Match exact host in autoplay whitelist before matching eTLD+1 #4741

Merged
merged 1 commit into from
Feb 26, 2020

Conversation

pilgrim-brave
Copy link
Contributor

@pilgrim-brave pilgrim-brave commented Feb 25, 2020

Resolves brave/brave-browser#4621

Submitter Checklist:

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
  • 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.

@@ -32,6 +32,12 @@ AutoplayWhitelistService::~AutoplayWhitelistService() {

bool AutoplayWhitelistService::ShouldAllowAutoplay(const GURL& url) {
DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
// look for exact host match (this is the only way that subdomains
// listed in the autoplay whitelist will match)
if (url.has_host() &&
Copy link
Member

@bbondy bbondy Feb 25, 2020

Choose a reason for hiding this comment

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

looks right to me, but will wait for confirmation that it works. A couple of unit tests would be super too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

simplified the test data file and added multiple tests

@pilgrim-brave pilgrim-brave force-pushed the ap_whitelist_subdomains branch from 70dbf83 to 31feca2 Compare February 25, 2020 21:18
@pilgrim-brave
Copy link
Contributor Author

AFAICT this only failed because of Github's connectivity issues yesterday, but I'll rebase it and run it through again just to be sure.

@pilgrim-brave pilgrim-brave force-pushed the ap_whitelist_subdomains branch from 31feca2 to e7c2198 Compare February 26, 2020 17:57
@pilgrim-brave pilgrim-brave merged commit 9f68fe3 into master Feb 26, 2020
@pilgrim-brave pilgrim-brave deleted the ap_whitelist_subdomains branch February 26, 2020 20:56
@bsclifton bsclifton added this to the 1.7.x - Nightly milestone Feb 27, 2020
@bsclifton
Copy link
Member

Added missing milestone to this PR and brave/brave-browser#4621

@pilgrim-brave
Copy link
Contributor Author

uplifted to release

@kjozwiak
Copy link
Member

kjozwiak commented Mar 3, 2020

Reproduced the original issue on macOS 10.15.3 x64 using the following builds:

Brave | 1.7.21 Chromium: 80.0.3987.122 (Official Build) nightly (64-bit)
--- | ---
Revision | cf72c4c4f7db75bc3da689cd76513962d31c7b52-refs/branch-heads/3987@{#943}
OS | macOS Version 10.15.3 (Build 19D76)
Brave | 1.4.95 Chromium: 80.0.3987.122 (Official Build) (64-bit)
-- | --
Revision | cf72c4c4f7db75bc3da689cd76513962d31c7b52-refs/branch-heads/3987@{#943}
OS | macOS Version 10.15.3 (Build 19D76)

Screen Shot 2020-03-03 at 6 39 13 PM

Verification PASSED on macOS 10.15.3 x64 using the following build:

Brave | 1.7.35 Chromium: 80.0.3987.122 (Official Build) nightly (64-bit)
-- | --
Revision | cf72c4c4f7db75bc3da689cd76513962d31c7b52-refs/branch-heads/3987@{#943}
OS | macOS Version 10.15.3 (Build 19D76)

@kjozwiak
Copy link
Member

kjozwiak commented Mar 3, 2020

Moving this out of https://github.com/brave/brave-core/milestone/56 as this specific PR is on master. #4788 is the PR that belongs in https://github.com/brave/brave-core/milestone/56.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

No audio output in Google Meet (blocked by autoplay)
4 participants