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

Explicitly cancel blocked requests #6632

Merged
merged 1 commit into from
Nov 2, 2020

Conversation

spinda
Copy link
Contributor

@spinda spinda commented Sep 12, 2020

Resolves brave/brave-browser#10063

Fixes, for example, video playback on https://abcnews.go.com/Politics/video/tennessee-celebrates-100-years-womens-vote-72455124, where returning an empty stub response for https://abcnewsplayer-a.akamaihd.net/player/2.106.5/akamai/amp/nielsen/Nielsen.min.js causes another script to misbehave.

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.

Copy link
Collaborator

@antonok-edm antonok-edm left a comment

Choose a reason for hiding this comment

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

@spinda You'll probably need to update some of the browsertests - there's at least one that uses an $explicitcancel rule in components/brave_shields/browser/ad_block_service_browsertest.cc

browser/net/url_context.h Outdated Show resolved Hide resolved
@iefremov
Copy link
Contributor

also any new tests are welcome, especially browsertests :) The change looks good

@spinda
Copy link
Contributor Author

spinda commented Sep 18, 2020

I ended up removing the explicitcancel test and updating the other tests so that they all expect resources to be blocked explicitly where they previously expected blank responses.

@spinda spinda self-assigned this Sep 18, 2020
@bsclifton
Copy link
Member

cc: @iefremov for re-review 😄

@bsclifton
Copy link
Member

@spinda could you give this a rebase? Sorry that it's taken a while to work through this PR ☹️

@spinda spinda closed this Oct 28, 2020
@spinda spinda deleted the explicitly-cancel-blocked-requests branch October 28, 2020 23:14
@spinda spinda restored the explicitly-cancel-blocked-requests branch October 29, 2020 05:51
@spinda
Copy link
Contributor Author

spinda commented Oct 29, 2020

Oops, accidentally deleted the branch! Rebasing this.

@spinda spinda reopened this Oct 29, 2020
Copy link
Contributor

@pes10k pes10k left a comment

Choose a reason for hiding this comment

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

@spinda it looks like this needs to be rebased again?

@spinda spinda force-pushed the explicitly-cancel-blocked-requests branch from 82df613 to 0f9b911 Compare November 2, 2020 18:34
@spinda
Copy link
Contributor Author

spinda commented Nov 2, 2020

@pes10k Rebased again!

@pes10k
Copy link
Contributor

pes10k commented Nov 2, 2020

Great! Thanks @spinda ! Once its been reviewed by @iefremov i think we're good to merge!

@pes10k pes10k merged commit 2f9630e into brave:master Nov 2, 2020
@pes10k
Copy link
Contributor

pes10k commented Nov 2, 2020

Merged! Thanks again @spinda !

@bsclifton bsclifton added this to the 1.18.x - Nightly milestone Nov 2, 2020
@bsclifton bsclifton mentioned this pull request Nov 3, 2020
33 tasks
bsclifton added a commit that referenced this pull request Nov 3, 2020
…d-requests"

This reverts commit 2f9630e, reversing
changes made to 5da5236.
bsclifton added a commit that referenced this pull request Nov 3, 2020
Revert "Merge pull request #6632 from spinda/explicitly-cancel-blocke…
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants