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.

Sign up for GitHub

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

Jump to bottom

Decouple adblock engines from component loading methods #10994

Merged
merged 18 commits into from
Feb 18, 2022
Merged

Decouple adblock engines from component loading methods #10994

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
3153432
add httpse component installer
bridiver Nov 2, 2021
c9a05e6
delete on correct task runner
bridiver Nov 2, 2021
345c566
create engine to handle task runner vs UI thread
bridiver Nov 3, 2021
2578955
initialize engine right away
bridiver Nov 3, 2021
7648cba
fix tests
antonok-edm Nov 10, 2021
2718626
rename things according to review feedback
antonok-edm Dec 2, 2021
d8a2360
remove dead code
antonok-edm Dec 10, 2021
8ae094f
add regional catalog provider interface
antonok-edm Dec 13, 2021
331afde
call LoadRegionalCatalog in AdBlockRegionalServiceManager::Init
antonok-edm Jan 8, 2022
6ceedb7
fix thread issues with new classes
bridiver Feb 11, 2022
9ad25d7
use the new engine
bridiver Feb 11, 2022
e4a1acc
add deleter for all adblock services
bridiver Feb 11, 2022
1ed764d
fix destruction of providers
bridiver Feb 13, 2022
bf82da8
review feedback
antonok-edm Feb 14, 2022
64d01d6
final fixes
antonok-edm Feb 15, 2022
391d032
fix sequence checking in tests
antonok-edm Feb 16, 2022
a60b50d
remove outdated WaitForBraveExtensionShieldsDataReady method
antonok-edm Feb 17, 2022
70c6a33
use a PrefChangeRegistrar for DefaultCookieListFlagEnabledTest
antonok-edm Feb 18, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 0 additions & 6 deletions browser/brave_browser_process.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,8 +32,6 @@ class LocalDataFilesService;

namespace brave_shields {
class AdBlockService;
class AdBlockCustomFiltersService;
class AdBlockRegionalServiceManager;
class HTTPSEverywhereService;
} // namespace brave_shields

Expand Down Expand Up @@ -77,10 +75,6 @@ class BraveBrowserProcess {
virtual ~BraveBrowserProcess();
virtual void StartBraveServices() = 0;
virtual brave_shields::AdBlockService* ad_block_service() = 0;
virtual brave_shields::AdBlockCustomFiltersService*
ad_block_custom_filters_service() = 0;
virtual brave_shields::AdBlockRegionalServiceManager*
ad_block_regional_service_manager() = 0;
#if BUILDFLAG(ENABLE_EXTENSIONS)
virtual brave_component_updater::ExtensionWhitelistService*
extension_whitelist_service() = 0;
Expand Down
26 changes: 11 additions & 15 deletions browser/brave_browser_process_impl.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
#include "base/bind.h"
#include "base/path_service.h"
#include "base/task/post_task.h"
#include "base/task/thread_pool.h"
#include "brave/browser/brave_shields/ad_block_subscription_download_manager_getter.h"
#include "brave/browser/brave_stats/brave_stats_updater.h"
#include "brave/browser/component_updater/brave_component_updater_configurator.h"
Expand All @@ -23,7 +24,6 @@
#include "brave/components/brave_component_updater/browser/brave_on_demand_updater.h"
#include "brave/components/brave_component_updater/browser/local_data_files_service.h"
#include "brave/components/brave_referrals/buildflags/buildflags.h"
#include "brave/components/brave_shields/browser/ad_block_custom_filters_service.h"
#include "brave/components/brave_shields/browser/ad_block_regional_service_manager.h"
#include "brave/components/brave_shields/browser/ad_block_service.h"
#include "brave/components/brave_shields/browser/ad_block_subscription_service_manager.h"
Expand Down Expand Up @@ -192,27 +192,21 @@ void BraveBrowserProcessImpl::StartBraveServices() {

brave_shields::AdBlockService* BraveBrowserProcessImpl::ad_block_service() {
if (!ad_block_service_) {
scoped_refptr<base::SequencedTaskRunner> task_runner(
base::ThreadPool::CreateSequencedTaskRunner(
{base::MayBlock(), base::TaskPriority::USER_BLOCKING,
base::TaskShutdownBehavior::SKIP_ON_SHUTDOWN}));
ad_block_service_ = std::make_unique<brave_shields::AdBlockService>(
brave_component_updater_delegate(),
local_state(), GetApplicationLocale(), component_updater(), task_runner,
std::make_unique<brave_shields::AdBlockSubscriptionServiceManager>(
brave_component_updater_delegate(),
local_state(), task_runner,
AdBlockSubscriptionDownloadManagerGetter(),
profile_manager()->user_data_dir().Append(
profile_manager()->GetInitialProfileDir())));
}
return ad_block_service_.get();
}

brave_shields::AdBlockCustomFiltersService*
BraveBrowserProcessImpl::ad_block_custom_filters_service() {
return ad_block_service()->custom_filters_service();
}

brave_shields::AdBlockRegionalServiceManager*
BraveBrowserProcessImpl::ad_block_regional_service_manager() {
return ad_block_service()->regional_service_manager();
}

NTPBackgroundImagesService*
BraveBrowserProcessImpl::ntp_background_images_service() {
if (!ntp_background_images_service_) {
Expand Down Expand Up @@ -262,9 +256,11 @@ BraveBrowserProcessImpl::debounce_component_installer() {

brave_shields::HTTPSEverywhereService*
BraveBrowserProcessImpl::https_everywhere_service() {
if (!https_everywhere_service_)
if (!created_https_everywhere_service_) {
Copy link
Contributor

Choose a reason for hiding this comment

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

but why?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@bridiver I believe this was your change

Copy link
Collaborator

Choose a reason for hiding this comment

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

this is pretty standard for lazy init and you can find many examples in browser_process_impl.cc. I just changed it to match upstream conventions

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm pretty sure those four upstream occurences of bool in browser_process_impl.h are leftovers from ancient times (mb scoped_ptr was not convertible to bool?) You can see that all new additions to browser_process_impl simply check unique_ptrs, no additional flags are really needed.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't remember why I bothered doing it in the first place, maybe some test issue?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I feel like there was some reason I did this, but it was so long ago I can't remember. If it doesn't cause any problems I'm fine with undoing it

https_everywhere_service_ = brave_shields::HTTPSEverywhereServiceFactory(
brave_component_updater_delegate());
brave_component_updater_delegate()->GetTaskRunner());
created_https_everywhere_service_ = true;
}
return https_everywhere_service_.get();
}

Expand Down
7 changes: 1 addition & 6 deletions browser/brave_browser_process_impl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,8 +34,6 @@ class LocalDataFilesService;

namespace brave_shields {
class AdBlockService;
class AdBlockCustomFiltersService;
class AdBlockRegionalServiceManager;
class HTTPSEverywhereService;
} // namespace brave_shields

Expand Down Expand Up @@ -90,10 +88,6 @@ class BraveBrowserProcessImpl : public BraveBrowserProcess,

void StartBraveServices() override;
brave_shields::AdBlockService* ad_block_service() override;
brave_shields::AdBlockCustomFiltersService* ad_block_custom_filters_service()
override;
brave_shields::AdBlockRegionalServiceManager*
ad_block_regional_service_manager() override;
#if BUILDFLAG(ENABLE_EXTENSIONS)
brave_component_updater::ExtensionWhitelistService*
extension_whitelist_service() override;
Expand Down Expand Up @@ -155,6 +149,7 @@ class BraveBrowserProcessImpl : public BraveBrowserProcess,
#endif
std::unique_ptr<debounce::DebounceComponentInstaller>
debounce_component_installer_;
bool created_https_everywhere_service_ = false;
Copy link
Contributor

Choose a reason for hiding this comment

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

not sure why this is needed

Copy link
Collaborator

Choose a reason for hiding this comment

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

as explained above this was changed to match upstream conventions

std::unique_ptr<brave_shields::HTTPSEverywhereService>
https_everywhere_service_;
std::unique_ptr<brave_stats::BraveStatsUpdater> brave_stats_updater_;
Expand Down
3 changes: 2 additions & 1 deletion browser/brave_content_browser_client.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -876,7 +876,8 @@ BraveContentBrowserClient::CreateThrottlesForNavigation(
content::NavigationThrottle> domain_block_navigation_throttle =
brave_shields::DomainBlockNavigationThrottle::MaybeCreateThrottleFor(
handle, g_brave_browser_process->ad_block_service(),
g_brave_browser_process->ad_block_custom_filters_service(),
g_brave_browser_process->ad_block_service()
->custom_filters_provider(),
EphemeralStorageServiceFactory::GetForContext(context),
HostContentSettingsMapFactory::GetForProfile(
Profile::FromBrowserContext(context)),
Expand Down
Loading