Remove cosmetic filtering extension version #9995
Conversation
antonok-edm
force-pushed
the
remove-cosmetic-filtering-extension-version
branch
from
bf5dde9 to
efd2e4b
Compare
antonok-edm
force-pushed
the
remove-cosmetic-filtering-extension-version
branch
from
efd2e4b to
ff223a5
Compare
| if (custom_selectors && custom_selectors->is_list()) | ||
| hide_selectors->Append(std::move(*custom_selectors)); | ||
| if (custom_selectors && custom_selectors->is_list()) { | ||
| for (auto i = custom_selectors->GetList().begin(); |
There was a problem hiding this comment.
Maybe for (auto i : custom_selectors->GetList()) ?
There was a problem hiding this comment.
Sadly I don't think this is an option because of base::Value - auto i and auto* i aren't allowed, and auto& i prevents me from std::moveing the value inside the loop.
| ExtensionFunction::ResponseAction | ||
| BraveShieldsShouldDoCosmeticFilteringFunction::Run() { | ||
| #if !defined(OS_ANDROID) && !defined(CHROME_OS) | ||
| if (base::FeatureList::IsEnabled( |
There was a problem hiding this comment.
pls double-check unused headers, e.g. #include "base/feature_list.h" or #include "brave/components/brave_shields/common/features.h", maybe anything else
There was a problem hiding this comment.
#include "brave/browser/brave_browser_process.h"
There was a problem hiding this comment.
We still have e.g.
auto* custom_filters_service =
g_brave_browser_process->ad_block_custom_filters_service();so I can't remove brave_browser_process.h, but I removed the others
|
|
||
| private: | ||
| std::unique_ptr<base::ListValue> GetHiddenClassIdSelectorsOnTaskRunner( | ||
| const std::vector<std::string>& classes, |
| if (!hide_selectors || !hide_selectors->is_list()) | ||
| hide_selectors = base::ListValue(); | ||
|
|
||
| if (custom_selectors && custom_selectors->is_list()) | ||
| hide_selectors->Append(std::move(*custom_selectors)); |
There was a problem hiding this comment.
why did we used to append a list?..
There was a problem hiding this comment.
great question 😅
I don't think it was correct. We had a couple of changes there when adding support for both backends in parallel, and I think something broke in the meantime. I think brave/brave-browser#17013 was caused by this, but I've tried manually and it's fixed with these changes. I'll look into getting test coverage for it.
Resolves brave/brave-browser#17918
Submitter Checklist:
QA/YesorQA/No;release-notes/includeorrelease-notes/exclude;OS/...) to the associated issuenpm run test -- brave_browser_tests,npm run test -- brave_unit_tests,npm run lint,npm run gn_check,npm run tslintgit rebase master(if needed)Reviewer Checklist:
gnAfter-merge Checklist:
changes has landed on
Test Plan:
This change has already been enabled by default using Griffin for quite some time, so we should be fine without any additional testing.