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

WIP: First rev attempt at removing the deprecated cosmetic filter #5099

Closed
wants to merge 1 commit into from

Conversation

@bsclifton
Copy link
Member

bsclifton commented Mar 30, 2020

Fixes brave/brave-browser#8914

Submitter Checklist:

Test Plan:

  1. Right click a page (any page)
  2. There should no longer be a Brave context menu item

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.
@bsclifton bsclifton self-assigned this Mar 30, 2020
@bsclifton
Copy link
Member Author

bsclifton commented Mar 30, 2020

All JavaScript unit tests pass - built locally and verified nothing is being thrown via stdout. Could you check this out a little more in-depth, @antonok-edm 😄

@bsclifton bsclifton changed the title First rev attempt at removing the deprecated cosmetic filter WIP: First rev attempt at removing the deprecated cosmetic filter Mar 30, 2020
Copy link
Collaborator

antonok-edm left a comment

Just one small modification and this should be good to go.

I'd also raise one more possibility before fully getting rid of this - if users have added any of the old cosmetic filters, I think it should be possible to pull them out of storage, turn them into custom filters on brave://adblock, and clear the storage entirely. In practice though, I'm sure a good number of the rules overlap with ones in the default cosmetic adblock lists, and anyone who used that feature is probably technical enough to update them manually on a site-by-site basis anyways - it might not be worth the extra machinery.

@antonok-edm
Copy link
Collaborator

antonok-edm commented Apr 2, 2020

By the way, most of the testing for the newer system is actually in C++ browser tests. I'd recommend npm run test -- brave_browser_tests --filter='Cosmetic*.*' to verify this hasn't broken anything.

@bsclifton bsclifton force-pushed the bsc-remove-old-cosmetic-filter branch from 44664e1 to 5300c6a Apr 10, 2020
@bsclifton
Copy link
Member Author

bsclifton commented Apr 10, 2020

@antonok-edm thanks for the above feedback 😄 Just addressed those items

As for preserving the existing rules, I'm not sure if this was a well-known enough feature to warrant that kind of migration. I'll check with a few folks and circle back

@pes10k
Copy link
Contributor

pes10k commented Apr 10, 2020

just for my two cents, I second @bsclifton 's idea. Unless its very simple to migrate them, i think its probably not worth the trouble :)

@bsclifton bsclifton added the PR/Blocked label May 1, 2020
@bsclifton
Copy link
Member Author

bsclifton commented May 1, 2020

OK going to go ahead and close this up, after discussion 😄

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

Successfully merging this pull request may close these issues.

3 participants
You can’t perform that action at this time.