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

Remove dom_distiller dependency from BraveAds #17366

Merged
merged 3 commits into from
Mar 1, 2023
Merged

Remove dom_distiller dependency from BraveAds #17366

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a6b0a5e
Remove BraveAds dependency from dom_distiller
aseren Feb 24, 2023
27bcce9
Remove unused DistillerPageWebContents patch.
aseren Feb 27, 2023
353ba55
Move dom_distiller_switches include into chrome_main_delegate.cc.
aseren Feb 27, 2023
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
1 change: 0 additions & 1 deletion app/DEPS
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,6 @@ include_rules = [
"+components/sync/base/command_line_switches.h",
"+components/translate/core/browser/translate_prefs.h",
"+components/variations/variations_switches.h",
"+components/dom_distiller/core/dom_distiller_switches.h",
"+google_apis/gaia",
"+gpu/config",
"+media/base/media_switches.h",
Expand Down
1 change: 0 additions & 1 deletion app/brave_main_delegate.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,6 @@
#include "chrome/common/chrome_paths_internal.h"
#include "chrome/common/chrome_switches.h"
#include "components/component_updater/component_updater_switches.h"
#include "components/dom_distiller/core/dom_distiller_switches.h"
#include "components/embedder_support/switches.h"
#include "components/sync/base/command_line_switches.h"
#include "components/variations/variations_switches.h"
Expand Down
2 changes: 0 additions & 2 deletions browser/brave_ads/ads_service_factory.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@
#include "brave/components/brave_federated/brave_federated_service.h"
#include "brave/components/brave_federated/data_store_service.h"
#include "brave/components/brave_federated/notification_ad_task_constants.h"
#include "chrome/browser/dom_distiller/dom_distiller_service_factory.h"
#include "chrome/browser/history/history_service_factory.h"
#include "chrome/browser/notifications/notification_display_service_factory.h"
#include "chrome/browser/profiles/incognito_helpers.h"
Expand Down Expand Up @@ -47,7 +46,6 @@ AdsServiceFactory::AdsServiceFactory()
"AdsService",
BrowserContextDependencyManager::GetInstance()) {
DependsOn(NotificationDisplayServiceFactory::GetInstance());
DependsOn(dom_distiller::DomDistillerServiceFactory::GetInstance());
Copy link
Contributor

Choose a reason for hiding this comment

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

@boocmp fyi

DependsOn(brave_rewards::RewardsServiceFactory::GetInstance());
DependsOn(HistoryServiceFactory::GetInstance());
DependsOn(brave_federated::BraveFederatedServiceFactory::GetInstance());
Expand Down
26 changes: 18 additions & 8 deletions browser/brave_ads/ads_tab_helper.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,7 @@

#include "brave/browser/brave_ads/ads_service_factory.h"
#include "chrome/browser/profiles/profile.h"
#include "components/dom_distiller/content/browser/distiller_javascript_utils.h"
#include "components/dom_distiller/content/browser/distiller_page_web_contents.h"
#include "chrome/common/chrome_isolated_world_ids.h"
#include "components/sessions/content/session_tab_helper.h"
#include "content/public/browser/navigation_handle.h"
#include "content/public/browser/render_frame_host.h"
Expand All @@ -27,6 +26,15 @@

namespace brave_ads {

namespace {

constexpr char16_t kGetDocumentHTMLScript[] =
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

also this appears to be happening even if ads is disabled?

u"new XMLSerializer().serializeToString(document)";

constexpr char16_t kGetInnerTextScript[] = u"document?.body?.innerText";

} // namespace

AdsTabHelper::AdsTabHelper(content::WebContents* web_contents)
: WebContentsObserver(web_contents),
content::WebContentsUserData<AdsTabHelper>(*web_contents),
Expand Down Expand Up @@ -67,15 +75,17 @@ void AdsTabHelper::RunIsolatedJavaScript(
content::RenderFrameHost* render_frame_host) {
DCHECK(render_frame_host);

dom_distiller::RunIsolatedJavaScript(
render_frame_host, "new XMLSerializer().serializeToString(document)",
render_frame_host->ExecuteJavaScriptInIsolatedWorld(
kGetDocumentHTMLScript,
base::BindOnce(&AdsTabHelper::OnJavaScriptHtmlResult,
weak_factory_.GetWeakPtr()));
weak_factory_.GetWeakPtr()),
ISOLATED_WORLD_ID_BRAVE_INTERNAL);

dom_distiller::RunIsolatedJavaScript(
render_frame_host, "document?.body?.innerText",
render_frame_host->ExecuteJavaScriptInIsolatedWorld(
kGetInnerTextScript,
base::BindOnce(&AdsTabHelper::OnJavaScriptTextResult,
weak_factory_.GetWeakPtr()));
weak_factory_.GetWeakPtr()),
ISOLATED_WORLD_ID_BRAVE_INTERNAL);
}

void AdsTabHelper::OnJavaScriptHtmlResult(base::Value value) {
Expand Down
10 changes: 0 additions & 10 deletions browser/brave_ads/ads_tab_helper.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,16 +23,6 @@
class Browser;
class GURL;

namespace dom_distiller {

class DistillerPage;

namespace proto {
class DomDistillerResult;
} // namespace proto

} // namespace dom_distiller

namespace brave_ads {

class AdsService;
Expand Down
1 change: 0 additions & 1 deletion browser/brave_ads/sources.gni
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,6 @@ brave_browser_brave_ads_deps = [
"//chrome/browser/profiles",
"//chrome/browser/profiles:profile",
"//chrome/browser/ui",
"//components/dom_distiller/content/browser",
"//components/history/core/browser",
"//components/keyed_service/content",
"//components/sessions",
Expand Down
1 change: 0 additions & 1 deletion browser/sources.gni
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -197,7 +197,6 @@ brave_chrome_browser_deps = [
"//components/content_settings/browser",
"//components/content_settings/core/browser",
"//components/content_settings/core/common",
"//components/dom_distiller/core",
"//components/embedder_support",
"//components/feed/core/shared_prefs:feed_shared_prefs",
"//components/gcm_driver:gcm_buildflags",
Expand Down
1 change: 1 addition & 0 deletions chromium_src/chrome/app/BUILD.gn
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
group("app") {
deps = [
"//brave/components/update_client:buildflags",
"//components/dom_distiller/content/browser",
"//google_apis",
"//media",
]
Expand Down
1 change: 1 addition & 0 deletions chromium_src/chrome/app/DEPS
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ specific_include_rules = {
"+brave/build/android/jni_headers/BraveQAPreferences_jni.h",
"+brave/components/brave_sync/buildflags.h",
"+brave/components/variations/buildflags.h",
"+components/dom_distiller/core/dom_distiller_switches.h",
"+components/signin/public/base/account_consistency_method.h",
"+components/sync/base/command_line_switches.h",
],
Expand Down
7 changes: 1 addition & 6 deletions chromium_src/chrome/app/chrome_main_delegate.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
#include "brave/components/brave_sync/buildflags.h"
#include "brave/components/variations/buildflags.h"
#include "build/build_config.h"
#include "components/dom_distiller/core/dom_distiller_switches.h"
#include "components/sync/base/command_line_switches.h"

#if BUILDFLAG(IS_ANDROID)
Expand Down Expand Up @@ -52,12 +53,6 @@ void AdjustSyncServiceUrlForAndroid(std::string* brave_sync_service_url) {
// Because of that, upstream tests won't get BraveMainDelegate instantiated and
// therefore we won't get any of the features below disabled/enabled when
// running those browser tests, which is not what we want.
//
// For instance, on DCHECK-enabled builds, not enabling the DOM distiller will
// 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);
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we remove command_line.AppendSwitch(switches::kEnableDomDistiller); ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I tried, but removing this line hides the speed-reader icon in Brave.

Copy link
Contributor

@boocmp boocmp Feb 28, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

thats because we have #define ReaderIconView SpeedreaderIconView :/

Expand Down
14 changes: 0 additions & 14 deletions patches/components-dom_distiller-content-browser-distiller_page_web_contents.cc.patch
View file
Open in desktop

This file was deleted.