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

Add "Manage custom filters" context menu item #8394

Merged
merged 3 commits into from
Apr 4, 2021

Conversation

antonok-edm
Copy link
Collaborator

@antonok-edm antonok-edm commented Mar 30, 2021

Resolves brave/brave-browser#14978

I've also added 2 commits here to clean up minor details from #8156 - one function no longer needs to be async and there are a bunch of locale strings that can now be removed safely.

Submitter Checklist:

  • I confirm that no security/privacy review is needed, or that I have requested one
  • There is a ticket for my issue
  • Used Github auto-closing keywords in the PR description above
  • Wrote a good PR/commit description
  • Added appropriate labels (QA/Yes or QA/No; release-notes/include or release-notes/exclude; OS/...) to the associated issue
  • Checked the PR locally: npm run test -- brave_browser_tests, npm run test -- brave_unit_tests, npm run lint, npm run gn_check, npm run tslint
  • Ran git rebase master (if needed)

Reviewer Checklist:

  • A security review is not needed, or a link to one is included in the PR description
  • New files have MPL-2.0 license header
  • Adequate test coverage exists to prevent regressions
  • Major classes, functions and non-trivial code blocks are well-commented
  • Changes in component dependencies are properly reflected in gn
  • Code follows the style guide
  • Test plan is specified in PR before merging

After-merge Checklist:

Test Plan:

From any page, right click on the page and check that the Brave > Manage custom filters context menu item exists and navigates to a new brave://adblock tab, or an existing one if present.

@antonok-edm antonok-edm self-assigned this Mar 30, 2021
@antonok-edm antonok-edm changed the title Add "filter management" context menu item Add "Manage custom filters" context menu item Mar 31, 2021
Copy link
Member

@bsclifton bsclifton left a comment

Choose a reason for hiding this comment

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

Functionality works great! Just had a question about the async keyword removal on addSiteCosmeticFilter. The other changes look good! 😄

@antonok-edm
Copy link
Collaborator Author

@bsclifton after discussion with @pes10k it looks like the async change shouldn't affect anything - onSelectorReturned is only used as the callback in chrome.tabs.sendMessage(tabs[0].id, { type: 'getTargetSelector' }, onSelectorReturned), and the return value of that callback is ignored as per https://developer.chrome.com/docs/extensions/reference/tabs/#method-sendMessage

Copy link
Member

@bsclifton bsclifton left a comment

Choose a reason for hiding this comment

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

@antonok-edm perfect - thanks for confirming. Sorry I didn't get to dig in there more after raising the comment. LGTM! 😄👍

@bbondy
Copy link
Member

bbondy commented Apr 4, 2021

@antonok-edm
Copy link
Collaborator Author

antonok-edm commented Apr 5, 2021

Yep, my fault - sorry about that! 😬

I just opened #8429 to update the tests accordingly.

@bsclifton
Copy link
Member

ah crap - my bad on that 😱 I saw the pre-init failure and (unfortunately) assumed it was an npm vulnerability. Great catch @bbondy 😄

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

Successfully merging this pull request may close these issues.

Add a a button to go to brave://adblock/ in the right click menu and maybe change 'Brave' name.
3 participants