Remove dom_distiller dependency from BraveAds #17366
Conversation
aseren
changed the title
| @@ -47,7 +46,6 @@ AdsServiceFactory::AdsServiceFactory() | |||
| "AdsService", | |||
| BrowserContextDependencyManager::GetInstance()) { | |||
| DependsOn(NotificationDisplayServiceFactory::GetInstance()); | |||
| DependsOn(dom_distiller::DomDistillerServiceFactory::GetInstance()); | |||
|
it seems there are unused |
| // get all browser tests crashing when dom_distiller::RunIsolatedJavaScript() | ||
| // gets called from AdsTabHelper::RunIsolatedJavaScript() (called via the | ||
| // WebContentsObserver machinery), since the JS World ID won't be set yet when | ||
| // dom_distiller::RunIsolatedJavaScript() gets called. | ||
| absl::optional<int> ChromeMainDelegate::BasicStartupComplete() { | ||
| BraveCommandLineHelper command_line(base::CommandLine::ForCurrentProcess()); | ||
| command_line.AppendSwitch(switches::kDisableDomainReliability); |
There was a problem hiding this comment.
Should we remove command_line.AppendSwitch(switches::kEnableDomDistiller); ?
There was a problem hiding this comment.
I tried, but removing this line hides the speed-reader icon in Brave.
There was a problem hiding this comment.
thats because we have #define ReaderIconView SpeedreaderIconView :/
|
also there is a dep in |
These are still needed for |
Fixed, thanks for pointing out these. The patch becomes unused after PR #4649. |
| @@ -27,6 +26,15 @@ | |||
|
|
|||
| namespace brave_ads { | |||
|
|
|||
| namespace { | |||
|
|
|||
| constexpr char16_t kGetDocumentHTMLScript[] = | |||
There was a problem hiding this comment.
We're injecting this into every page? This seems like a probable perf issue, but what exactly are we using this for anyway? If the purpose of this is to get page content, this is definitely not what we should be doing
There was a problem hiding this comment.
also this appears to be happening even if ads is disabled?
Resolves brave/brave-browser#28751
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_testswikinpm run lint,npm run presubmitwiki,npm run gn_check,npm run tslintgit rebase master(if needed)Reviewer Checklist:
gnAfter-merge Checklist:
changes has landed on
Test Plan: