From c3e335fa56314e09a06c7136a0baffb69714c927 Mon Sep 17 00:00:00 2001 From: Kevin Kuehler Date: Mon, 2 Aug 2021 16:08:45 -0700 Subject: [PATCH 1/7] Bolt up speedreader component to tab_helper Uses the webcontents to deliver the result to the correct tab helper. We might want to follow this up by having the distiller thread return tuple: struct DistillResult { bool success; std::string buffered_body; }; If success is ever false we can abort the loader. --- browser/brave_content_browser_client.cc | 7 ++- browser/speedreader/speedreader_tab_helper.cc | 20 ++++++- browser/speedreader/speedreader_tab_helper.h | 27 +++++++++- components/speedreader/BUILD.gn | 2 + components/speedreader/DEPS | 1 + .../speedreader/speedreader_result_delegate.h | 24 +++++++++ .../speedreader/speedreader_throttle.cc | 54 +++++++++++++++++-- components/speedreader/speedreader_throttle.h | 11 ++++ .../speedreader/speedreader_url_loader.cc | 11 +++- 9 files changed, 147 insertions(+), 10 deletions(-) create mode 100644 components/speedreader/speedreader_result_delegate.h diff --git a/browser/brave_content_browser_client.cc b/browser/brave_content_browser_client.cc index f5c7ff01ba1b..3b70b0833bd7 100644 --- a/browser/brave_content_browser_client.cc +++ b/browser/brave_content_browser_client.cc @@ -508,15 +508,18 @@ BraveContentBrowserClient::CreateURLLoaderThrottles( speedreader::SpeedreaderTabHelper::FromWebContents(contents); if (tab_helper) { const auto state = tab_helper->PageDistillState(); - if (speedreader::SpeedreaderTabHelper::PageStateIsDistilled(state) && + if (speedreader::SpeedreaderTabHelper::PageWantsDistill(state) && request.resource_type == static_cast(blink::mojom::ResourceType::kMainFrame)) { + // Only check for disabled sites if we are in Speedreader mode + const bool check_disabled_sites = + state == DistillState::kSpeedreaderModePending; std::unique_ptr throttle = speedreader::SpeedReaderThrottle::MaybeCreateThrottleFor( g_brave_browser_process->speedreader_rewriter_service(), HostContentSettingsMapFactory::GetForProfile( Profile::FromBrowserContext(browser_context)), - request.url, state == DistillState::kSpeedreaderMode, + contents, tab_helper, request.url, check_disabled_sites, base::ThreadTaskRunnerHandle::Get()); if (throttle) result.push_back(std::move(throttle)); diff --git a/browser/speedreader/speedreader_tab_helper.cc b/browser/speedreader/speedreader_tab_helper.cc index 1b82a1e78747..cbb653363082 100644 --- a/browser/speedreader/speedreader_tab_helper.cc +++ b/browser/speedreader/speedreader_tab_helper.cc @@ -5,6 +5,7 @@ #include "brave/browser/speedreader/speedreader_tab_helper.h" +#include "base/notreached.h" #include "brave/browser/brave_browser_process.h" #include "brave/browser/speedreader/speedreader_service_factory.h" #include "brave/browser/ui/brave_browser_window.h" @@ -91,7 +92,7 @@ void SpeedreaderTabHelper::UpdateActiveState( DCHECK(handle->IsInMainFrame()); if (single_shot_next_request_) { - SetNextRequestState(DistillState::kReaderMode); + SetNextRequestState(DistillState::kReaderModePending); return; } @@ -108,7 +109,7 @@ void SpeedreaderTabHelper::UpdateActiveState( } else if (!IsEnabledForSite(handle->GetURL())) { SetNextRequestState(DistillState::kSpeedreaderOnDisabledPage); } else { - SetNextRequestState(DistillState::kSpeedreaderMode); + SetNextRequestState(DistillState::kSpeedreaderModePending); } return; } @@ -173,6 +174,21 @@ void SpeedreaderTabHelper::HideBubble() { } } +void SpeedreaderTabHelper::OnDistillComplete(bool success) { + DCHECK(SpeedreaderTabHelper::PageWantsDistill(distill_state_)); + if (!success) { + distill_state_ = DistillState::kNone; + return; + } + + // Perform a state transition + if (distill_state_ == DistillState::kSpeedreaderModePending) { + distill_state_ = DistillState::kSpeedreaderMode; + } else { // distill_state_ == DistillState::kReaderModePending + distill_state_ = DistillState::kReaderMode; + } +} + WEB_CONTENTS_USER_DATA_KEY_IMPL(SpeedreaderTabHelper) } // namespace speedreader diff --git a/browser/speedreader/speedreader_tab_helper.h b/browser/speedreader/speedreader_tab_helper.h index 52c1f33b1278..08065ffc5d27 100644 --- a/browser/speedreader/speedreader_tab_helper.h +++ b/browser/speedreader/speedreader_tab_helper.h @@ -6,6 +6,7 @@ #ifndef BRAVE_BROWSER_SPEEDREADER_SPEEDREADER_TAB_HELPER_H_ #define BRAVE_BROWSER_SPEEDREADER_SPEEDREADER_TAB_HELPER_H_ +#include "brave/components/speedreader/speedreader_result_delegate.h" #include "content/public/browser/web_contents_observer.h" #include "content/public/browser/web_contents_user_data.h" @@ -21,7 +22,8 @@ class SpeedreaderBubbleView; // Determines if speedreader should be active for a given top-level navigation. class SpeedreaderTabHelper : public content::WebContentsObserver, - public content::WebContentsUserData { + public content::WebContentsUserData, + public SpeedreaderResultDelegate { public: enum class DistillState { // Used as an initialization state @@ -30,6 +32,12 @@ class SpeedreaderTabHelper // The web contents is not distilled kNone, + // ------------------------------------------------------------------------- + // Pending states. The user requested the page be distilled. + // + // TODO(keur): Should we use bitstrings and make pending a bit both + // speedreader and reader mode can share? + // Reader mode state that can only be reached when Speedreader is disabled // The Speedreader icon will pop up in the address bar, and the user clicks // it. It runs Speedreader is "Single Shot Mode". The Speedreader throttle @@ -38,9 +46,16 @@ class SpeedreaderTabHelper // The first time a user activates reader mode on a page, a bubble drops // down asking them to enable the Speedreader feature for automatic // distillation. - kReaderMode, + kReaderModePending, // Speedreader is enabled and the page was automatically distilled. + kSpeedreaderModePending, + // ------------------------------------------------------------------------- + + // kReaderModePending was ACKed + kReaderMode, + + // kSpeedreaderModePending was ACKed kSpeedreaderMode, // Speedreader is enabled, but the page was blacklisted by the user. @@ -55,6 +70,11 @@ class SpeedreaderTabHelper state == DistillState::kSpeedreaderMode; } + static bool PageWantsDistill(DistillState state) { + return state == DistillState::kReaderModePending || + state == DistillState::kSpeedreaderModePending; + } + ~SpeedreaderTabHelper() override; SpeedreaderTabHelper(const SpeedreaderTabHelper&) = delete; @@ -115,6 +135,9 @@ class SpeedreaderTabHelper void DidRedirectNavigation( content::NavigationHandle* navigation_handle) override; + // SpeedreaderResultDelegate: + void OnDistillComplete(bool success) override; + bool single_shot_next_request_ = false; // run speedreader once on next page load DistillState distill_state_ = DistillState::kNone; diff --git a/components/speedreader/BUILD.gn b/components/speedreader/BUILD.gn index 8aadc77fb36e..aea950240f8a 100644 --- a/components/speedreader/BUILD.gn +++ b/components/speedreader/BUILD.gn @@ -26,6 +26,7 @@ static_library("speedreader") { "speedreader_component.cc", "speedreader_component.h", "speedreader_pref_names.h", + "speedreader_result_delegate.h", "speedreader_rewriter_service.cc", "speedreader_rewriter_service.h", "speedreader_service.cc", @@ -50,6 +51,7 @@ static_library("speedreader") { "//components/content_settings/core/common", "//components/keyed_service/core:core", "//components/prefs:prefs", + "//content/public/browser", "//services/network/public/cpp", "//services/network/public/mojom", "//third_party/blink/public/common", diff --git a/components/speedreader/DEPS b/components/speedreader/DEPS index 9d649be1bd4d..b109bfddb254 100644 --- a/components/speedreader/DEPS +++ b/components/speedreader/DEPS @@ -1,6 +1,7 @@ include_rules = [ "+components/content_settings/core", "+components/prefs", + "+content/public/browser", "+services/network/public", "+services/network/public/cpp", "+services/network/public/mojom", diff --git a/components/speedreader/speedreader_result_delegate.h b/components/speedreader/speedreader_result_delegate.h new file mode 100644 index 000000000000..8a17fad0affa --- /dev/null +++ b/components/speedreader/speedreader_result_delegate.h @@ -0,0 +1,24 @@ +/* Copyright (c) 2021 The Brave Authors. All rights reserved. + * This Source Code Form is subject to the terms of the Mozilla Public + * License, v. 2.0. If a copy of the MPL was not distributed with this file, + * You can obtain one at http://mozilla.org/MPL/2.0/. */ + +#ifndef BRAVE_COMPONENTS_SPEEDREADER_SPEEDREADER_RESULT_DELEGATE_H_ +#define BRAVE_COMPONENTS_SPEEDREADER_SPEEDREADER_RESULT_DELEGATE_H_ + +#include "content/public/browser/web_contents_user_data.h" + +namespace content { +class WebContents; +} // namespace content + +// SpeedreaderResultDelegate is an interface for the speedreader component to +// notify a tab_helper that the distillation has completed. To meet the +// requirements of a tab_helper, the interface is scoped to the lifetime of +// WebContents. +class SpeedreaderResultDelegate { + public: + virtual void OnDistillComplete(bool success) = 0; +}; + +#endif // BRAVE_COMPONENTS_SPEEDREADER_SPEEDREADER_RESULT_DELEGATE_H_ diff --git a/components/speedreader/speedreader_throttle.cc b/components/speedreader/speedreader_throttle.cc index 6e31b8b26765..1b6da2586efb 100644 --- a/components/speedreader/speedreader_throttle.cc +++ b/components/speedreader/speedreader_throttle.cc @@ -7,35 +7,74 @@ #include +#include "brave/components/speedreader/speedreader_result_delegate.h" #include "brave/components/speedreader/speedreader_rewriter_service.h" #include "brave/components/speedreader/speedreader_url_loader.h" #include "brave/components/speedreader/speedreader_util.h" #include "components/content_settings/core/browser/host_content_settings_map.h" #include "components/content_settings/core/common/content_settings.h" +#include "content/public/browser/web_contents_user_data.h" #include "mojo/public/cpp/bindings/pending_receiver.h" #include "mojo/public/cpp/bindings/pending_remote.h" #include "services/network/public/mojom/url_response_head.mojom.h" -namespace speedreader { +namespace { + +// Used to scope the result of distillation to the lifetime of |web_contents| +class SpeedreaderResultWebContentsLifetimeHelper + : public content::WebContentsUserData< + SpeedreaderResultWebContentsLifetimeHelper> { + public: + explicit SpeedreaderResultWebContentsLifetimeHelper( + content::WebContents* contents, + SpeedreaderResultDelegate* delegate) + : delegate_(delegate) {} + + void OnDistillComplete(bool success) { + delegate_->OnDistillComplete(success); + } + + WEB_CONTENTS_USER_DATA_KEY_DECL(); + + private: + friend class content::WebContentsUserData; + + SpeedreaderResultDelegate* delegate_; +}; + +WEB_CONTENTS_USER_DATA_KEY_IMPL(SpeedreaderResultWebContentsLifetimeHelper) +} // anonymous namespace + +namespace speedreader { // static std::unique_ptr SpeedReaderThrottle::MaybeCreateThrottleFor( SpeedreaderRewriterService* rewriter_service, HostContentSettingsMap* content_settings, + content::WebContents* web_contents, + ::SpeedreaderResultDelegate* result_delegate, const GURL& url, bool check_disabled_sites, scoped_refptr task_runner) { if (check_disabled_sites && !IsEnabledForSite(content_settings, url)) return nullptr; - return std::make_unique(rewriter_service, task_runner); + + return std::make_unique(rewriter_service, web_contents, + result_delegate, task_runner); } SpeedReaderThrottle::SpeedReaderThrottle( SpeedreaderRewriterService* rewriter_service, + content::WebContents* web_contents, + ::SpeedreaderResultDelegate* result_delegate, scoped_refptr task_runner) : rewriter_service_(rewriter_service), - task_runner_(std::move(task_runner)) {} + web_contents_(web_contents), + task_runner_(std::move(task_runner)) { + SpeedreaderResultWebContentsLifetimeHelper::CreateForWebContents( + web_contents, result_delegate); +} SpeedReaderThrottle::~SpeedReaderThrottle() = default; @@ -66,4 +105,13 @@ void SpeedReaderThrottle::Resume() { delegate_->Resume(); } +void SpeedReaderThrottle::OnDistillComplete(bool success) { + auto* result_helper = + SpeedreaderResultWebContentsLifetimeHelper::FromWebContents( + web_contents_); + if (result_helper) { + result_helper->OnDistillComplete(success); + } +} + } // namespace speedreader diff --git a/components/speedreader/speedreader_throttle.h b/components/speedreader/speedreader_throttle.h index 48ca1ddf53e7..37b7221b0d12 100644 --- a/components/speedreader/speedreader_throttle.h +++ b/components/speedreader/speedreader_throttle.h @@ -11,11 +11,16 @@ #include "base/memory/scoped_refptr.h" #include "base/memory/weak_ptr.h" #include "base/single_thread_task_runner.h" +#include "brave/components/speedreader/speedreader_result_delegate.h" #include "services/network/public/mojom/url_response_head.mojom-forward.h" #include "third_party/blink/public/common/loader/url_loader_throttle.h" class HostContentSettingsMap; +namespace content { +class WebContents; +} // namespace content + namespace speedreader { class SpeedreaderRewriterService; @@ -31,6 +36,8 @@ class SpeedReaderThrottle : public blink::URLLoaderThrottle { static std::unique_ptr MaybeCreateThrottleFor( SpeedreaderRewriterService* rewriter_service, HostContentSettingsMap* content_settings, + content::WebContents* web_contents, + ::SpeedreaderResultDelegate* result_delegate, const GURL& url, bool check_disabled_sites, scoped_refptr task_runner); @@ -39,6 +46,8 @@ class SpeedReaderThrottle : public blink::URLLoaderThrottle { // IPC in SpeedReaderLoader. |task_runner| is supposed to be bound to the // current sequence. SpeedReaderThrottle(SpeedreaderRewriterService* rewriter_service, + content::WebContents* web_contents, + ::SpeedreaderResultDelegate* result_delegate, scoped_refptr task_runner); ~SpeedReaderThrottle() override; @@ -52,9 +61,11 @@ class SpeedReaderThrottle : public blink::URLLoaderThrottle { // Called from SpeedReaderURLLoader. void Resume(); + void OnDistillComplete(bool success); private: SpeedreaderRewriterService* rewriter_service_; // not owned + content::WebContents* web_contents_; scoped_refptr task_runner_; base::WeakPtrFactory weak_factory_{this}; }; diff --git a/components/speedreader/speedreader_url_loader.cc b/components/speedreader/speedreader_url_loader.cc index 34536cf0b67f..632f8c0827b7 100644 --- a/components/speedreader/speedreader_url_loader.cc +++ b/components/speedreader/speedreader_url_loader.cc @@ -326,8 +326,17 @@ void SpeedReaderURLLoader::CompleteSending() { state_ = State::kCompleted; // Call client's OnComplete() if |this|'s OnComplete() has already been // called. - if (complete_status_.has_value()) + if (complete_status_.has_value()) { destination_url_loader_client_->OnComplete(complete_status_.value()); + if (throttle_) { + // When distillation fails we return the original data. Causes this + // throttle to get deleted. + // + // TODO(iefremov): Is it okay to rely on just this, or should the distill + // thread also return a boolean indicating an error? + throttle_->OnDistillComplete(true); + } + } body_consumer_watcher_.Cancel(); body_producer_watcher_.Cancel(); From 5cfaecde97384c93adb2d35dc03e14b4b345f1f4 Mon Sep 17 00:00:00 2001 From: Kevin Kuehler Date: Wed, 4 Aug 2021 16:14:36 -0700 Subject: [PATCH 2/7] Handle results that are loaded from cached tabs Resolves https://github.com/brave/brave-browser/issues/17355 --- browser/brave_browser_main_parts.cc | 19 ++++ browser/brave_browser_main_parts.h | 1 + browser/speedreader/sources.gni | 2 + .../speedreader_extended_info_handler.cc | 98 +++++++++++++++++++ .../speedreader_extended_info_handler.h | 49 ++++++++++ browser/speedreader/speedreader_tab_helper.cc | 54 +++++++--- browser/speedreader/speedreader_tab_helper.h | 8 +- .../speedreader/speedreader_result_delegate.h | 6 +- .../speedreader/speedreader_throttle.cc | 10 +- components/speedreader/speedreader_throttle.h | 2 +- .../speedreader/speedreader_url_loader.cc | 2 +- 11 files changed, 226 insertions(+), 25 deletions(-) create mode 100644 browser/speedreader/speedreader_extended_info_handler.cc create mode 100644 browser/speedreader/speedreader_extended_info_handler.h diff --git a/browser/brave_browser_main_parts.cc b/browser/brave_browser_main_parts.cc index 2a24b8ae2e10..f5dc2a7b8ebe 100644 --- a/browser/brave_browser_main_parts.cc +++ b/browser/brave_browser_main_parts.cc @@ -13,6 +13,7 @@ #include "brave/common/brave_constants.h" #include "brave/common/pref_names.h" #include "brave/components/brave_sync/features.h" +#include "brave/components/speedreader/buildflags.h" #include "brave/components/tor/buildflags/buildflags.h" #include "chrome/common/chrome_features.h" #include "components/prefs/pref_service.h" @@ -21,6 +22,11 @@ #include "extensions/buildflags/buildflags.h" #include "media/base/media_switches.h" +#if BUILDFLAG(ENABLE_SPEEDREADER) +#include "brave/browser/speedreader/speedreader_extended_info_handler.h" +#include "components/sessions/content/content_serialized_navigation_driver.h" +#endif + #if BUILDFLAG(ENABLE_TOR) #include #include "base/files/file_util.h" @@ -59,6 +65,19 @@ #include "extensions/browser/extension_system.h" #endif +void BraveBrowserMainParts::PreBrowserStart() { +#if BUILDFLAG(ENABLE_SPEEDREADER) + // Register() must be called after the SerializedNavigationDriver is + // initialized, but before any calls to + // ContentSerializedNavigationBuilder::ToNavigationEntries() + // + // TODO(keur): Can we DCHECK the latter condition? + DCHECK(sessions::ContentSerializedNavigationDriver::GetInstance()); + speedreader::SpeedreaderExtendedInfoHandler::Register(); +#endif + ChromeBrowserMainParts::PreBrowserStart(); +} + void BraveBrowserMainParts::PostBrowserStart() { ChromeBrowserMainParts::PostBrowserStart(); diff --git a/browser/brave_browser_main_parts.h b/browser/brave_browser_main_parts.h index 96792509c587..d3ed30c8961c 100644 --- a/browser/brave_browser_main_parts.h +++ b/browser/brave_browser_main_parts.h @@ -14,6 +14,7 @@ class BraveBrowserMainParts : public ChromeBrowserMainParts { using ChromeBrowserMainParts::ChromeBrowserMainParts; ~BraveBrowserMainParts() override = default; + void PreBrowserStart() override; void PostBrowserStart() override; void PreShutdown() override; void PreProfileInit() override; diff --git a/browser/speedreader/sources.gni b/browser/speedreader/sources.gni index b2c7b775f954..4bc7315d4933 100644 --- a/browser/speedreader/sources.gni +++ b/browser/speedreader/sources.gni @@ -10,6 +10,8 @@ brave_browser_speedreader_deps = [] if (enable_speedreader) { brave_browser_speedreader_sources += [ + "//brave/browser/speedreader/speedreader_extended_info_handler.cc", + "//brave/browser/speedreader/speedreader_extended_info_handler.h", "//brave/browser/speedreader/speedreader_service_factory.cc", "//brave/browser/speedreader/speedreader_service_factory.h", "//brave/browser/speedreader/speedreader_tab_helper.cc", diff --git a/browser/speedreader/speedreader_extended_info_handler.cc b/browser/speedreader/speedreader_extended_info_handler.cc new file mode 100644 index 000000000000..c8b98656fffc --- /dev/null +++ b/browser/speedreader/speedreader_extended_info_handler.cc @@ -0,0 +1,98 @@ +/* Copyright 2021 The Brave Authors. All rights reserved. + * This Source Code Form is subject to the terms of the Mozilla Public + * License, v. 2.0. If a copy of the MPL was not distributed with this file, + * You can obtain one at http://mozilla.org/MPL/2.0/. */ + +#include "brave/browser/speedreader/speedreader_extended_info_handler.h" + +#include + +#include "base/memory/ptr_util.h" +#include "base/supports_user_data.h" +#include "components/sessions/content/content_serialized_navigation_driver.h" +#include "content/public/browser/navigation_entry.h" + +namespace { + +// This is the key we register in the extended info map. We also use it for the +// navigation entry user data. +constexpr char kSpeedreaderKey[] = "speedreader"; + +constexpr char kPageSavedReaderMode[] = "reader-mode"; +constexpr char kPageSavedSpeedreaderMode[] = "speedreader-mode"; + +struct SpeedreaderNavigationData : public base::SupportsUserData::Data { + explicit SpeedreaderNavigationData(const std::string& value) : value(value) {} + std::string value; +}; + +} // namespace + +namespace speedreader { + +// static +void SpeedreaderExtendedInfoHandler::Register() { + auto* handler = new SpeedreaderExtendedInfoHandler(); + sessions::ContentSerializedNavigationDriver::GetInstance() + ->RegisterExtendedInfoHandler(kSpeedreaderKey, base::WrapUnique(handler)); +} + +// static +void SpeedreaderExtendedInfoHandler::PersistSpeedreaderMode( + content::NavigationEntry* entry) { + entry->SetUserData( + kSpeedreaderKey, + std::make_unique(kPageSavedSpeedreaderMode)); +} + +// static +void SpeedreaderExtendedInfoHandler::PersistReaderMode( + content::NavigationEntry* entry) { + entry->SetUserData( + kSpeedreaderKey, + std::make_unique(kPageSavedReaderMode)); +} + +// static +void SpeedreaderExtendedInfoHandler::ClearPersistedData( + content::NavigationEntry* entry) { + entry->RemoveUserData(kSpeedreaderKey); +} + +// static +bool SpeedreaderExtendedInfoHandler::IsCachedSpeedreaderMode( + content::NavigationEntry* entry) { + auto* data = static_cast( + entry->GetUserData(kSpeedreaderKey)); + if (!data) { + return false; + } + return data->value == kPageSavedSpeedreaderMode; +} + +// static +bool SpeedreaderExtendedInfoHandler::IsCachedReaderMode( + content::NavigationEntry* entry) { + auto* data = static_cast( + entry->GetUserData(kSpeedreaderKey)); + if (!data) { + return false; + } + return data->value == kPageSavedReaderMode; +} + +std::string SpeedreaderExtendedInfoHandler::GetExtendedInfo( + content::NavigationEntry* entry) const { + auto* data = static_cast( + entry->GetUserData(kSpeedreaderKey)); + return data ? data->value : std::string(); +} + +void SpeedreaderExtendedInfoHandler::RestoreExtendedInfo( + const std::string& info, + content::NavigationEntry* entry) { + entry->SetUserData(kSpeedreaderKey, + std::make_unique(info)); +} + +} // namespace speedreader diff --git a/browser/speedreader/speedreader_extended_info_handler.h b/browser/speedreader/speedreader_extended_info_handler.h new file mode 100644 index 000000000000..f397931469ec --- /dev/null +++ b/browser/speedreader/speedreader_extended_info_handler.h @@ -0,0 +1,49 @@ +/* Copyright 2021 The Brave Authors. All rights reserved. + * This Source Code Form is subject to the terms of the Mozilla Public + * License, v. 2.0. If a copy of the MPL was not distributed with this file, + * You can obtain one at http://mozilla.org/MPL/2.0/. */ + +#ifndef BRAVE_BROWSER_SPEEDREADER_SPEEDREADER_EXTENDED_INFO_HANDLER_H_ +#define BRAVE_BROWSER_SPEEDREADER_SPEEDREADER_EXTENDED_INFO_HANDLER_H_ + +#include + +#include "components/sessions/content/extended_info_handler.h" + +namespace content { +class NavigationEntry; +} // namespace content + +namespace speedreader { + +// This class is meant to persist data to a content::NavigationEntry so that +// distilled pages will be recognized on a restored session. +class SpeedreaderExtendedInfoHandler : public sessions::ExtendedInfoHandler { + public: + // Register the extended info handler. + // Calling this more than once will cause a crash. + static void Register(); + + static void PersistSpeedreaderMode(content::NavigationEntry* entry); + static void PersistReaderMode(content::NavigationEntry* entry); + static void ClearPersistedData(content::NavigationEntry* entry); + + static bool IsCachedSpeedreaderMode(content::NavigationEntry* entry); + static bool IsCachedReaderMode(content::NavigationEntry* entry); + + SpeedreaderExtendedInfoHandler(const SpeedreaderExtendedInfoHandler&) = + delete; + SpeedreaderExtendedInfoHandler& operator=( + const SpeedreaderExtendedInfoHandler&) = delete; + + SpeedreaderExtendedInfoHandler() = default; + ~SpeedreaderExtendedInfoHandler() override = default; + + std::string GetExtendedInfo(content::NavigationEntry* entry) const override; + void RestoreExtendedInfo(const std::string& info, + content::NavigationEntry* entry) override; +}; + +} // namespace speedreader + +#endif // BRAVE_BROWSER_SPEEDREADER_SPEEDREADER_EXTENDED_INFO_HANDLER_H_ diff --git a/browser/speedreader/speedreader_tab_helper.cc b/browser/speedreader/speedreader_tab_helper.cc index cbb653363082..ff09a1823525 100644 --- a/browser/speedreader/speedreader_tab_helper.cc +++ b/browser/speedreader/speedreader_tab_helper.cc @@ -5,8 +5,8 @@ #include "brave/browser/speedreader/speedreader_tab_helper.h" -#include "base/notreached.h" #include "brave/browser/brave_browser_process.h" +#include "brave/browser/speedreader/speedreader_extended_info_handler.h" #include "brave/browser/speedreader/speedreader_service_factory.h" #include "brave/browser/ui/brave_browser_window.h" #include "brave/browser/ui/speedreader/speedreader_bubble_view.h" @@ -18,6 +18,7 @@ #include "chrome/browser/profiles/profile.h" #include "chrome/browser/ui/browser.h" #include "chrome/browser/ui/browser_finder.h" +#include "content/public/browser/navigation_entry.h" #include "content/public/browser/navigation_handle.h" #include "content/public/browser/web_contents.h" @@ -86,6 +87,28 @@ void SpeedreaderTabHelper::SingleShotSpeedreader() { } } +bool SpeedreaderTabHelper::MaybeUpdateCachedState( + content::NavigationHandle* handle) { + auto* entry = handle->GetNavigationEntry(); + if (!entry) { + return false; + } + + bool cached = false; + if (SpeedreaderExtendedInfoHandler::IsCachedReaderMode(entry)) { + distill_state_ = DistillState::kReaderMode; + cached = true; + } else if (SpeedreaderExtendedInfoHandler::IsCachedSpeedreaderMode(entry)) { + distill_state_ = DistillState::kSpeedreaderMode; + cached = true; + } + + if (!cached) { + SpeedreaderExtendedInfoHandler::ClearPersistedData(entry); + } + return cached; +} + void SpeedreaderTabHelper::UpdateActiveState( content::NavigationHandle* handle) { DCHECK(handle); @@ -125,14 +148,18 @@ void SpeedreaderTabHelper::SetNextRequestState(DistillState state) { void SpeedreaderTabHelper::DidStartNavigation( content::NavigationHandle* navigation_handle) { if (navigation_handle->IsInMainFrame()) { - UpdateActiveState(navigation_handle); + if (!MaybeUpdateCachedState(navigation_handle)) { + UpdateActiveState(navigation_handle); + } } } void SpeedreaderTabHelper::DidRedirectNavigation( content::NavigationHandle* navigation_handle) { if (navigation_handle->IsInMainFrame()) { - UpdateActiveState(navigation_handle); + if (!MaybeUpdateCachedState(navigation_handle)) { + UpdateActiveState(navigation_handle); + } } } @@ -174,18 +201,23 @@ void SpeedreaderTabHelper::HideBubble() { } } -void SpeedreaderTabHelper::OnDistillComplete(bool success) { - DCHECK(SpeedreaderTabHelper::PageWantsDistill(distill_state_)); - if (!success) { - distill_state_ = DistillState::kNone; - return; - } - +void SpeedreaderTabHelper::OnDistillComplete() { // Perform a state transition + auto* entry = web_contents()->GetController().GetLastCommittedEntry(); + DCHECK(entry); + if (!entry) { + return; // not possible? + } if (distill_state_ == DistillState::kSpeedreaderModePending) { + SpeedreaderExtendedInfoHandler::PersistSpeedreaderMode(entry); distill_state_ = DistillState::kSpeedreaderMode; - } else { // distill_state_ == DistillState::kReaderModePending + } else if (distill_state_ == DistillState::kReaderModePending) { + SpeedreaderExtendedInfoHandler::PersistReaderMode(entry); distill_state_ = DistillState::kReaderMode; + } else { + // We got here via an already cached page. + DCHECK(distill_state_ == DistillState::kSpeedreaderMode || + distill_state_ == DistillState::kReaderMode); } } diff --git a/browser/speedreader/speedreader_tab_helper.h b/browser/speedreader/speedreader_tab_helper.h index 08065ffc5d27..fccd85f37028 100644 --- a/browser/speedreader/speedreader_tab_helper.h +++ b/browser/speedreader/speedreader_tab_helper.h @@ -11,6 +11,7 @@ #include "content/public/browser/web_contents_user_data.h" namespace content { +class NavigationEntry; class NavigationHandle; class WebContents; } // namespace content @@ -71,7 +72,9 @@ class SpeedreaderTabHelper } static bool PageWantsDistill(DistillState state) { - return state == DistillState::kReaderModePending || + return state == DistillState::kReaderMode || + state == DistillState::kSpeedreaderMode || + state == DistillState::kReaderModePending || state == DistillState::kSpeedreaderModePending; } @@ -126,6 +129,7 @@ class SpeedreaderTabHelper // committed to the WebContents. bool IsEnabledForSite(const GURL& url); + bool MaybeUpdateCachedState(content::NavigationHandle* handle); void UpdateActiveState(content::NavigationHandle* handle); void SetNextRequestState(DistillState state); @@ -136,7 +140,7 @@ class SpeedreaderTabHelper content::NavigationHandle* navigation_handle) override; // SpeedreaderResultDelegate: - void OnDistillComplete(bool success) override; + void OnDistillComplete() override; bool single_shot_next_request_ = false; // run speedreader once on next page load diff --git a/components/speedreader/speedreader_result_delegate.h b/components/speedreader/speedreader_result_delegate.h index 8a17fad0affa..bfc3befe694d 100644 --- a/components/speedreader/speedreader_result_delegate.h +++ b/components/speedreader/speedreader_result_delegate.h @@ -13,12 +13,10 @@ class WebContents; } // namespace content // SpeedreaderResultDelegate is an interface for the speedreader component to -// notify a tab_helper that the distillation has completed. To meet the -// requirements of a tab_helper, the interface is scoped to the lifetime of -// WebContents. +// notify a tab_helper that the distillation has completed successufully. class SpeedreaderResultDelegate { public: - virtual void OnDistillComplete(bool success) = 0; + virtual void OnDistillComplete() = 0; }; #endif // BRAVE_COMPONENTS_SPEEDREADER_SPEEDREADER_RESULT_DELEGATE_H_ diff --git a/components/speedreader/speedreader_throttle.cc b/components/speedreader/speedreader_throttle.cc index 1b6da2586efb..6c67f4ad74e5 100644 --- a/components/speedreader/speedreader_throttle.cc +++ b/components/speedreader/speedreader_throttle.cc @@ -30,9 +30,7 @@ class SpeedreaderResultWebContentsLifetimeHelper SpeedreaderResultDelegate* delegate) : delegate_(delegate) {} - void OnDistillComplete(bool success) { - delegate_->OnDistillComplete(success); - } + void OnDistillComplete() { delegate_->OnDistillComplete(); } WEB_CONTENTS_USER_DATA_KEY_DECL(); @@ -44,7 +42,7 @@ class SpeedreaderResultWebContentsLifetimeHelper WEB_CONTENTS_USER_DATA_KEY_IMPL(SpeedreaderResultWebContentsLifetimeHelper) -} // anonymous namespace +} // namespace namespace speedreader { // static @@ -105,12 +103,12 @@ void SpeedReaderThrottle::Resume() { delegate_->Resume(); } -void SpeedReaderThrottle::OnDistillComplete(bool success) { +void SpeedReaderThrottle::OnDistillComplete() { auto* result_helper = SpeedreaderResultWebContentsLifetimeHelper::FromWebContents( web_contents_); if (result_helper) { - result_helper->OnDistillComplete(success); + result_helper->OnDistillComplete(); } } diff --git a/components/speedreader/speedreader_throttle.h b/components/speedreader/speedreader_throttle.h index 37b7221b0d12..c7c6e80d28f7 100644 --- a/components/speedreader/speedreader_throttle.h +++ b/components/speedreader/speedreader_throttle.h @@ -61,7 +61,7 @@ class SpeedReaderThrottle : public blink::URLLoaderThrottle { // Called from SpeedReaderURLLoader. void Resume(); - void OnDistillComplete(bool success); + void OnDistillComplete(); private: SpeedreaderRewriterService* rewriter_service_; // not owned diff --git a/components/speedreader/speedreader_url_loader.cc b/components/speedreader/speedreader_url_loader.cc index 632f8c0827b7..55b1b298ba25 100644 --- a/components/speedreader/speedreader_url_loader.cc +++ b/components/speedreader/speedreader_url_loader.cc @@ -334,7 +334,7 @@ void SpeedReaderURLLoader::CompleteSending() { // // TODO(iefremov): Is it okay to rely on just this, or should the distill // thread also return a boolean indicating an error? - throttle_->OnDistillComplete(true); + throttle_->OnDistillComplete(); } } From 5e3d893bede1bffa0956e7ddd79b4b72041bf408 Mon Sep 17 00:00:00 2001 From: Kevin Kuehler Date: Fri, 27 Aug 2021 11:44:18 -0700 Subject: [PATCH 3/7] Refactor for review * Use the DistillState enum in SpeedreaderExtendedInfoHandler to de-duplicate logic. * Move DistillState to speedreader component * Move SpeedreaderExtendedInfoHandler to speedreader component * Pass wc_getter to SpeedreaderThrottle instead of creating the lifetime helper --- browser/brave_browser_main_parts.cc | 2 +- browser/brave_content_browser_client.cc | 9 +-- browser/speedreader/sources.gni | 2 - browser/speedreader/speedreader_tab_helper.cc | 21 +++--- browser/speedreader/speedreader_tab_helper.h | 53 +-------------- browser/ui/browser_commands.cc | 2 +- .../speedreader/speedreader_icon_view.cc | 5 +- .../views/speedreader/speedreader_icon_view.h | 3 +- components/speedreader/BUILD.gn | 3 + components/speedreader/DEPS | 1 + .../speedreader_extended_info_handler.cc | 66 ++++++++++--------- .../speedreader_extended_info_handler.h | 26 ++++---- .../speedreader/speedreader_result_delegate.h | 6 -- .../speedreader/speedreader_throttle.cc | 53 ++++----------- components/speedreader/speedreader_throttle.h | 16 ++--- components/speedreader/speedreader_util.cc | 12 ++++ components/speedreader/speedreader_util.h | 47 +++++++++++++ 17 files changed, 152 insertions(+), 175 deletions(-) rename {browser => components}/speedreader/speedreader_extended_info_handler.cc (64%) rename {browser => components}/speedreader/speedreader_extended_info_handler.h (67%) diff --git a/browser/brave_browser_main_parts.cc b/browser/brave_browser_main_parts.cc index f5dc2a7b8ebe..283ac735b7f5 100644 --- a/browser/brave_browser_main_parts.cc +++ b/browser/brave_browser_main_parts.cc @@ -23,7 +23,7 @@ #include "media/base/media_switches.h" #if BUILDFLAG(ENABLE_SPEEDREADER) -#include "brave/browser/speedreader/speedreader_extended_info_handler.h" +#include "brave/components/speedreader/speedreader_extended_info_handler.h" #include "components/sessions/content/content_serialized_navigation_driver.h" #endif diff --git a/browser/brave_content_browser_client.cc b/browser/brave_content_browser_client.cc index 3b70b0833bd7..580b8901126e 100644 --- a/browser/brave_content_browser_client.cc +++ b/browser/brave_content_browser_client.cc @@ -48,6 +48,7 @@ #include "brave/components/gemini/browser/buildflags/buildflags.h" #include "brave/components/ipfs/buildflags/buildflags.h" #include "brave/components/speedreader/buildflags.h" +#include "brave/components/speedreader/speedreader_util.h" #include "brave/components/tor/buildflags/buildflags.h" #include "brave/grit/brave_generated_resources.h" #include "chrome/browser/browser_process.h" @@ -492,14 +493,14 @@ std::vector> BraveContentBrowserClient::CreateURLLoaderThrottles( const network::ResourceRequest& request, content::BrowserContext* browser_context, - const base::RepeatingCallback& wc_getter, + const content::WebContents::Getter& wc_getter, content::NavigationUIData* navigation_ui_data, int frame_tree_node_id) { auto result = ChromeContentBrowserClient::CreateURLLoaderThrottles( request, browser_context, wc_getter, navigation_ui_data, frame_tree_node_id); #if BUILDFLAG(ENABLE_SPEEDREADER) - using DistillState = speedreader::SpeedreaderTabHelper::DistillState; + using DistillState = speedreader::DistillState; content::WebContents* contents = wc_getter.Run(); if (!contents) { return result; @@ -508,7 +509,7 @@ BraveContentBrowserClient::CreateURLLoaderThrottles( speedreader::SpeedreaderTabHelper::FromWebContents(contents); if (tab_helper) { const auto state = tab_helper->PageDistillState(); - if (speedreader::SpeedreaderTabHelper::PageWantsDistill(state) && + if (speedreader::PageWantsDistill(state) && request.resource_type == static_cast(blink::mojom::ResourceType::kMainFrame)) { // Only check for disabled sites if we are in Speedreader mode @@ -519,7 +520,7 @@ BraveContentBrowserClient::CreateURLLoaderThrottles( g_brave_browser_process->speedreader_rewriter_service(), HostContentSettingsMapFactory::GetForProfile( Profile::FromBrowserContext(browser_context)), - contents, tab_helper, request.url, check_disabled_sites, + wc_getter, tab_helper, request.url, check_disabled_sites, base::ThreadTaskRunnerHandle::Get()); if (throttle) result.push_back(std::move(throttle)); diff --git a/browser/speedreader/sources.gni b/browser/speedreader/sources.gni index 4bc7315d4933..b2c7b775f954 100644 --- a/browser/speedreader/sources.gni +++ b/browser/speedreader/sources.gni @@ -10,8 +10,6 @@ brave_browser_speedreader_deps = [] if (enable_speedreader) { brave_browser_speedreader_sources += [ - "//brave/browser/speedreader/speedreader_extended_info_handler.cc", - "//brave/browser/speedreader/speedreader_extended_info_handler.h", "//brave/browser/speedreader/speedreader_service_factory.cc", "//brave/browser/speedreader/speedreader_service_factory.h", "//brave/browser/speedreader/speedreader_tab_helper.cc", diff --git a/browser/speedreader/speedreader_tab_helper.cc b/browser/speedreader/speedreader_tab_helper.cc index ff09a1823525..277deb52707f 100644 --- a/browser/speedreader/speedreader_tab_helper.cc +++ b/browser/speedreader/speedreader_tab_helper.cc @@ -6,10 +6,10 @@ #include "brave/browser/speedreader/speedreader_tab_helper.h" #include "brave/browser/brave_browser_process.h" -#include "brave/browser/speedreader/speedreader_extended_info_handler.h" #include "brave/browser/speedreader/speedreader_service_factory.h" #include "brave/browser/ui/brave_browser_window.h" #include "brave/browser/ui/speedreader/speedreader_bubble_view.h" +#include "brave/components/speedreader/speedreader_extended_info_handler.h" #include "brave/components/speedreader/speedreader_rewriter_service.h" #include "brave/components/speedreader/speedreader_service.h" #include "brave/components/speedreader/speedreader_test_whitelist.h" @@ -95,14 +95,11 @@ bool SpeedreaderTabHelper::MaybeUpdateCachedState( } bool cached = false; - if (SpeedreaderExtendedInfoHandler::IsCachedReaderMode(entry)) { - distill_state_ = DistillState::kReaderMode; - cached = true; - } else if (SpeedreaderExtendedInfoHandler::IsCachedSpeedreaderMode(entry)) { - distill_state_ = DistillState::kSpeedreaderMode; + DistillState state = SpeedreaderExtendedInfoHandler::GetCachedMode(entry); + if (state != DistillState::kUnknown) { cached = true; + distill_state_ = state; } - if (!cached) { SpeedreaderExtendedInfoHandler::ClearPersistedData(entry); } @@ -205,20 +202,20 @@ void SpeedreaderTabHelper::OnDistillComplete() { // Perform a state transition auto* entry = web_contents()->GetController().GetLastCommittedEntry(); DCHECK(entry); - if (!entry) { - return; // not possible? - } + + // Perform a state transition if (distill_state_ == DistillState::kSpeedreaderModePending) { - SpeedreaderExtendedInfoHandler::PersistSpeedreaderMode(entry); distill_state_ = DistillState::kSpeedreaderMode; } else if (distill_state_ == DistillState::kReaderModePending) { - SpeedreaderExtendedInfoHandler::PersistReaderMode(entry); distill_state_ = DistillState::kReaderMode; } else { // We got here via an already cached page. DCHECK(distill_state_ == DistillState::kSpeedreaderMode || distill_state_ == DistillState::kReaderMode); } + + // Save current distill state to navigation entry cache. + SpeedreaderExtendedInfoHandler::PersistMode(entry, distill_state_); } WEB_CONTENTS_USER_DATA_KEY_IMPL(SpeedreaderTabHelper) diff --git a/browser/speedreader/speedreader_tab_helper.h b/browser/speedreader/speedreader_tab_helper.h index fccd85f37028..2d2fc4e38af5 100644 --- a/browser/speedreader/speedreader_tab_helper.h +++ b/browser/speedreader/speedreader_tab_helper.h @@ -7,6 +7,7 @@ #define BRAVE_BROWSER_SPEEDREADER_SPEEDREADER_TAB_HELPER_H_ #include "brave/components/speedreader/speedreader_result_delegate.h" +#include "brave/components/speedreader/speedreader_util.h" #include "content/public/browser/web_contents_observer.h" #include "content/public/browser/web_contents_user_data.h" @@ -26,58 +27,6 @@ class SpeedreaderTabHelper public content::WebContentsUserData, public SpeedreaderResultDelegate { public: - enum class DistillState { - // Used as an initialization state - kUnknown, - - // The web contents is not distilled - kNone, - - // ------------------------------------------------------------------------- - // Pending states. The user requested the page be distilled. - // - // TODO(keur): Should we use bitstrings and make pending a bit both - // speedreader and reader mode can share? - - // Reader mode state that can only be reached when Speedreader is disabled - // The Speedreader icon will pop up in the address bar, and the user clicks - // it. It runs Speedreader is "Single Shot Mode". The Speedreader throttle - // is created for the following request, then deactivated. - // - // The first time a user activates reader mode on a page, a bubble drops - // down asking them to enable the Speedreader feature for automatic - // distillation. - kReaderModePending, - - // Speedreader is enabled and the page was automatically distilled. - kSpeedreaderModePending, - // ------------------------------------------------------------------------- - - // kReaderModePending was ACKed - kReaderMode, - - // kSpeedreaderModePending was ACKed - kSpeedreaderMode, - - // Speedreader is enabled, but the page was blacklisted by the user. - kSpeedreaderOnDisabledPage, - - // Speedreader is disabled, the URL passes the heuristic. - kPageProbablyReadable, - }; - - static bool PageStateIsDistilled(DistillState state) { - return state == DistillState::kReaderMode || - state == DistillState::kSpeedreaderMode; - } - - static bool PageWantsDistill(DistillState state) { - return state == DistillState::kReaderMode || - state == DistillState::kSpeedreaderMode || - state == DistillState::kReaderModePending || - state == DistillState::kSpeedreaderModePending; - } - ~SpeedreaderTabHelper() override; SpeedreaderTabHelper(const SpeedreaderTabHelper&) = delete; diff --git a/browser/ui/browser_commands.cc b/browser/ui/browser_commands.cc index 0e9e828043b2..46daa8b3ea36 100644 --- a/browser/ui/browser_commands.cc +++ b/browser/ui/browser_commands.cc @@ -83,7 +83,7 @@ void OpenGuestProfile() { void MaybeDistillAndShowSpeedreaderBubble(Browser* browser) { #if BUILDFLAG(ENABLE_SPEEDREADER) - using DistillState = speedreader::SpeedreaderTabHelper::DistillState; + using DistillState = speedreader::DistillState; WebContents* contents = browser->tab_strip_model()->GetActiveWebContents(); if (contents) { auto* tab_helper = diff --git a/browser/ui/views/speedreader/speedreader_icon_view.cc b/browser/ui/views/speedreader/speedreader_icon_view.cc index 15b9cf843a53..44b5ce96ef37 100644 --- a/browser/ui/views/speedreader/speedreader_icon_view.cc +++ b/browser/ui/views/speedreader/speedreader_icon_view.cc @@ -28,7 +28,7 @@ #include "ui/views/animation/ink_drop_host_view.h" #include "ui/views/animation/ink_drop_state.h" -using DistillState = speedreader::SpeedreaderTabHelper::DistillState; +using DistillState = speedreader::DistillState; SpeedreaderIconView::SpeedreaderIconView( CommandUpdater* command_updater, @@ -63,8 +63,7 @@ void SpeedreaderIconView::UpdateImpl() { const ui::ThemeProvider* theme_provider = GetThemeProvider(); const DistillState state = GetDistillState(); - const bool is_distilled = - speedreader::SpeedreaderTabHelper::PageStateIsDistilled(state); + const bool is_distilled = speedreader::PageStateIsDistilled(state); if (!is_distilled) { if (state == DistillState::kSpeedreaderOnDisabledPage || diff --git a/browser/ui/views/speedreader/speedreader_icon_view.h b/browser/ui/views/speedreader/speedreader_icon_view.h index db1d041e8a3b..b47b824fe1ad 100644 --- a/browser/ui/views/speedreader/speedreader_icon_view.h +++ b/browser/ui/views/speedreader/speedreader_icon_view.h @@ -30,8 +30,9 @@ class SpeedreaderIconView : public PageActionIconView { views::BubbleDialogDelegate* GetBubble() const override; std::u16string GetTextForTooltipAndAccessibleName() const override; void UpdateImpl() override; + private: - speedreader::SpeedreaderTabHelper::DistillState GetDistillState() const; + speedreader::DistillState GetDistillState() const; }; #endif // BRAVE_BROWSER_UI_VIEWS_SPEEDREADER_SPEEDREADER_ICON_VIEW_H_ diff --git a/components/speedreader/BUILD.gn b/components/speedreader/BUILD.gn index aea950240f8a..e4f1eb6e602a 100644 --- a/components/speedreader/BUILD.gn +++ b/components/speedreader/BUILD.gn @@ -25,6 +25,8 @@ static_library("speedreader") { "features.h", "speedreader_component.cc", "speedreader_component.h", + "speedreader_extended_info_handler.cc", + "speedreader_extended_info_handler.h", "speedreader_pref_names.h", "speedreader_result_delegate.h", "speedreader_rewriter_service.cc", @@ -51,6 +53,7 @@ static_library("speedreader") { "//components/content_settings/core/common", "//components/keyed_service/core:core", "//components/prefs:prefs", + "//components/sessions:sessions", "//content/public/browser", "//services/network/public/cpp", "//services/network/public/mojom", diff --git a/components/speedreader/DEPS b/components/speedreader/DEPS index b109bfddb254..a79e71ba20fa 100644 --- a/components/speedreader/DEPS +++ b/components/speedreader/DEPS @@ -1,6 +1,7 @@ include_rules = [ "+components/content_settings/core", "+components/prefs", + "+components/sessions/content", "+content/public/browser", "+services/network/public", "+services/network/public/cpp", diff --git a/browser/speedreader/speedreader_extended_info_handler.cc b/components/speedreader/speedreader_extended_info_handler.cc similarity index 64% rename from browser/speedreader/speedreader_extended_info_handler.cc rename to components/speedreader/speedreader_extended_info_handler.cc index c8b98656fffc..04a1b94bd137 100644 --- a/browser/speedreader/speedreader_extended_info_handler.cc +++ b/components/speedreader/speedreader_extended_info_handler.cc @@ -3,11 +3,11 @@ * License, v. 2.0. If a copy of the MPL was not distributed with this file, * You can obtain one at http://mozilla.org/MPL/2.0/. */ -#include "brave/browser/speedreader/speedreader_extended_info_handler.h" +#include "brave/components/speedreader/speedreader_extended_info_handler.h" #include +#include -#include "base/memory/ptr_util.h" #include "base/supports_user_data.h" #include "components/sessions/content/content_serialized_navigation_driver.h" #include "content/public/browser/navigation_entry.h" @@ -32,53 +32,55 @@ namespace speedreader { // static void SpeedreaderExtendedInfoHandler::Register() { - auto* handler = new SpeedreaderExtendedInfoHandler(); sessions::ContentSerializedNavigationDriver::GetInstance() - ->RegisterExtendedInfoHandler(kSpeedreaderKey, base::WrapUnique(handler)); + ->RegisterExtendedInfoHandler( + kSpeedreaderKey, std::make_unique()); } // static -void SpeedreaderExtendedInfoHandler::PersistSpeedreaderMode( - content::NavigationEntry* entry) { - entry->SetUserData( - kSpeedreaderKey, - std::make_unique(kPageSavedSpeedreaderMode)); -} - -// static -void SpeedreaderExtendedInfoHandler::PersistReaderMode( - content::NavigationEntry* entry) { - entry->SetUserData( - kSpeedreaderKey, - std::make_unique(kPageSavedReaderMode)); -} +void SpeedreaderExtendedInfoHandler::PersistMode( + content::NavigationEntry* entry, + DistillState state) { + DCHECK(entry); + std::unique_ptr data; + switch (state) { + case DistillState::kReaderMode: + data = std::make_unique(kPageSavedReaderMode); + break; + case DistillState::kSpeedreaderMode: + data = std::make_unique( + kPageSavedSpeedreaderMode); + break; + default: + NOTREACHED(); + return; + } -// static -void SpeedreaderExtendedInfoHandler::ClearPersistedData( - content::NavigationEntry* entry) { - entry->RemoveUserData(kSpeedreaderKey); + entry->SetUserData(kSpeedreaderKey, std::move(data)); } // static -bool SpeedreaderExtendedInfoHandler::IsCachedSpeedreaderMode( +DistillState SpeedreaderExtendedInfoHandler::GetCachedMode( content::NavigationEntry* entry) { + DCHECK(entry); auto* data = static_cast( entry->GetUserData(kSpeedreaderKey)); if (!data) { - return false; + return DistillState::kUnknown; + } + if (data->value == kPageSavedReaderMode) { + return DistillState::kReaderMode; + } else if (data->value == kPageSavedSpeedreaderMode) { + return DistillState::kSpeedreaderMode; + } else { + return DistillState::kUnknown; } - return data->value == kPageSavedSpeedreaderMode; } // static -bool SpeedreaderExtendedInfoHandler::IsCachedReaderMode( +void SpeedreaderExtendedInfoHandler::ClearPersistedData( content::NavigationEntry* entry) { - auto* data = static_cast( - entry->GetUserData(kSpeedreaderKey)); - if (!data) { - return false; - } - return data->value == kPageSavedReaderMode; + entry->RemoveUserData(kSpeedreaderKey); } std::string SpeedreaderExtendedInfoHandler::GetExtendedInfo( diff --git a/browser/speedreader/speedreader_extended_info_handler.h b/components/speedreader/speedreader_extended_info_handler.h similarity index 67% rename from browser/speedreader/speedreader_extended_info_handler.h rename to components/speedreader/speedreader_extended_info_handler.h index f397931469ec..4109f2f51edf 100644 --- a/browser/speedreader/speedreader_extended_info_handler.h +++ b/components/speedreader/speedreader_extended_info_handler.h @@ -3,11 +3,12 @@ * License, v. 2.0. If a copy of the MPL was not distributed with this file, * You can obtain one at http://mozilla.org/MPL/2.0/. */ -#ifndef BRAVE_BROWSER_SPEEDREADER_SPEEDREADER_EXTENDED_INFO_HANDLER_H_ -#define BRAVE_BROWSER_SPEEDREADER_SPEEDREADER_EXTENDED_INFO_HANDLER_H_ +#ifndef BRAVE_COMPONENTS_SPEEDREADER_SPEEDREADER_EXTENDED_INFO_HANDLER_H_ +#define BRAVE_COMPONENTS_SPEEDREADER_SPEEDREADER_EXTENDED_INFO_HANDLER_H_ #include +#include "brave/components/speedreader/speedreader_util.h" #include "components/sessions/content/extended_info_handler.h" namespace content { @@ -24,21 +25,24 @@ class SpeedreaderExtendedInfoHandler : public sessions::ExtendedInfoHandler { // Calling this more than once will cause a crash. static void Register(); - static void PersistSpeedreaderMode(content::NavigationEntry* entry); - static void PersistReaderMode(content::NavigationEntry* entry); - static void ClearPersistedData(content::NavigationEntry* entry); + // Persist the current speedreader state to NavigationEntry + static void PersistMode(content::NavigationEntry* entry, DistillState state); + + // Retrieve cached speedreader state from NavigationEntry. Returns + // DistillState::kUnknown if not cached. + static DistillState GetCachedMode(content::NavigationEntry* entry); - static bool IsCachedSpeedreaderMode(content::NavigationEntry* entry); - static bool IsCachedReaderMode(content::NavigationEntry* entry); + // Clear the NavigationEntry speedreader state + static void ClearPersistedData(content::NavigationEntry* entry); + SpeedreaderExtendedInfoHandler() = default; + ~SpeedreaderExtendedInfoHandler() override = default; SpeedreaderExtendedInfoHandler(const SpeedreaderExtendedInfoHandler&) = delete; SpeedreaderExtendedInfoHandler& operator=( const SpeedreaderExtendedInfoHandler&) = delete; - SpeedreaderExtendedInfoHandler() = default; - ~SpeedreaderExtendedInfoHandler() override = default; - + // sessions::ExtendedInfoHandler: std::string GetExtendedInfo(content::NavigationEntry* entry) const override; void RestoreExtendedInfo(const std::string& info, content::NavigationEntry* entry) override; @@ -46,4 +50,4 @@ class SpeedreaderExtendedInfoHandler : public sessions::ExtendedInfoHandler { } // namespace speedreader -#endif // BRAVE_BROWSER_SPEEDREADER_SPEEDREADER_EXTENDED_INFO_HANDLER_H_ +#endif // BRAVE_COMPONENTS_SPEEDREADER_SPEEDREADER_EXTENDED_INFO_HANDLER_H_ diff --git a/components/speedreader/speedreader_result_delegate.h b/components/speedreader/speedreader_result_delegate.h index bfc3befe694d..c4b56b407bb8 100644 --- a/components/speedreader/speedreader_result_delegate.h +++ b/components/speedreader/speedreader_result_delegate.h @@ -6,12 +6,6 @@ #ifndef BRAVE_COMPONENTS_SPEEDREADER_SPEEDREADER_RESULT_DELEGATE_H_ #define BRAVE_COMPONENTS_SPEEDREADER_SPEEDREADER_RESULT_DELEGATE_H_ -#include "content/public/browser/web_contents_user_data.h" - -namespace content { -class WebContents; -} // namespace content - // SpeedreaderResultDelegate is an interface for the speedreader component to // notify a tab_helper that the distillation has completed successufully. class SpeedreaderResultDelegate { diff --git a/components/speedreader/speedreader_throttle.cc b/components/speedreader/speedreader_throttle.cc index 6c67f4ad74e5..57e09229d737 100644 --- a/components/speedreader/speedreader_throttle.cc +++ b/components/speedreader/speedreader_throttle.cc @@ -13,66 +13,39 @@ #include "brave/components/speedreader/speedreader_util.h" #include "components/content_settings/core/browser/host_content_settings_map.h" #include "components/content_settings/core/common/content_settings.h" +#include "content/public/browser/browser_thread.h" #include "content/public/browser/web_contents_user_data.h" #include "mojo/public/cpp/bindings/pending_receiver.h" #include "mojo/public/cpp/bindings/pending_remote.h" #include "services/network/public/mojom/url_response_head.mojom.h" -namespace { - -// Used to scope the result of distillation to the lifetime of |web_contents| -class SpeedreaderResultWebContentsLifetimeHelper - : public content::WebContentsUserData< - SpeedreaderResultWebContentsLifetimeHelper> { - public: - explicit SpeedreaderResultWebContentsLifetimeHelper( - content::WebContents* contents, - SpeedreaderResultDelegate* delegate) - : delegate_(delegate) {} - - void OnDistillComplete() { delegate_->OnDistillComplete(); } - - WEB_CONTENTS_USER_DATA_KEY_DECL(); - - private: - friend class content::WebContentsUserData; - - SpeedreaderResultDelegate* delegate_; -}; - -WEB_CONTENTS_USER_DATA_KEY_IMPL(SpeedreaderResultWebContentsLifetimeHelper) - -} // namespace - namespace speedreader { // static std::unique_ptr SpeedReaderThrottle::MaybeCreateThrottleFor( SpeedreaderRewriterService* rewriter_service, HostContentSettingsMap* content_settings, - content::WebContents* web_contents, - ::SpeedreaderResultDelegate* result_delegate, + const content::WebContents::Getter& wc_getter, + SpeedreaderResultDelegate* result_delegate, const GURL& url, bool check_disabled_sites, scoped_refptr task_runner) { if (check_disabled_sites && !IsEnabledForSite(content_settings, url)) return nullptr; - return std::make_unique(rewriter_service, web_contents, + return std::make_unique(rewriter_service, wc_getter, result_delegate, task_runner); } SpeedReaderThrottle::SpeedReaderThrottle( SpeedreaderRewriterService* rewriter_service, - content::WebContents* web_contents, - ::SpeedreaderResultDelegate* result_delegate, + const content::WebContents::Getter& wc_getter, + SpeedreaderResultDelegate* result_delegate, scoped_refptr task_runner) : rewriter_service_(rewriter_service), - web_contents_(web_contents), - task_runner_(std::move(task_runner)) { - SpeedreaderResultWebContentsLifetimeHelper::CreateForWebContents( - web_contents, result_delegate); -} + wc_getter_(wc_getter), + result_delegate_(result_delegate), + task_runner_(std::move(task_runner)) {} SpeedReaderThrottle::~SpeedReaderThrottle() = default; @@ -104,11 +77,9 @@ void SpeedReaderThrottle::Resume() { } void SpeedReaderThrottle::OnDistillComplete() { - auto* result_helper = - SpeedreaderResultWebContentsLifetimeHelper::FromWebContents( - web_contents_); - if (result_helper) { - result_helper->OnDistillComplete(); + DCHECK_CURRENTLY_ON(content::BrowserThread::UI); + if (wc_getter_.Run()) { + result_delegate_->OnDistillComplete(); } } diff --git a/components/speedreader/speedreader_throttle.h b/components/speedreader/speedreader_throttle.h index c7c6e80d28f7..518799ee216e 100644 --- a/components/speedreader/speedreader_throttle.h +++ b/components/speedreader/speedreader_throttle.h @@ -12,15 +12,12 @@ #include "base/memory/weak_ptr.h" #include "base/single_thread_task_runner.h" #include "brave/components/speedreader/speedreader_result_delegate.h" +#include "content/public/browser/web_contents.h" #include "services/network/public/mojom/url_response_head.mojom-forward.h" #include "third_party/blink/public/common/loader/url_loader_throttle.h" class HostContentSettingsMap; -namespace content { -class WebContents; -} // namespace content - namespace speedreader { class SpeedreaderRewriterService; @@ -36,8 +33,8 @@ class SpeedReaderThrottle : public blink::URLLoaderThrottle { static std::unique_ptr MaybeCreateThrottleFor( SpeedreaderRewriterService* rewriter_service, HostContentSettingsMap* content_settings, - content::WebContents* web_contents, - ::SpeedreaderResultDelegate* result_delegate, + const content::WebContents::Getter& wc_getter, + SpeedreaderResultDelegate* result_delegate, const GURL& url, bool check_disabled_sites, scoped_refptr task_runner); @@ -46,8 +43,8 @@ class SpeedReaderThrottle : public blink::URLLoaderThrottle { // IPC in SpeedReaderLoader. |task_runner| is supposed to be bound to the // current sequence. SpeedReaderThrottle(SpeedreaderRewriterService* rewriter_service, - content::WebContents* web_contents, - ::SpeedreaderResultDelegate* result_delegate, + const content::WebContents::Getter& wc_getter, + SpeedreaderResultDelegate* result_delegate, scoped_refptr task_runner); ~SpeedReaderThrottle() override; @@ -65,7 +62,8 @@ class SpeedReaderThrottle : public blink::URLLoaderThrottle { private: SpeedreaderRewriterService* rewriter_service_; // not owned - content::WebContents* web_contents_; + content::WebContents::Getter wc_getter_; + SpeedreaderResultDelegate* result_delegate_; scoped_refptr task_runner_; base::WeakPtrFactory weak_factory_{this}; }; diff --git a/components/speedreader/speedreader_util.cc b/components/speedreader/speedreader_util.cc index 3c1d832d1675..4297dbcd1394 100644 --- a/components/speedreader/speedreader_util.cc +++ b/components/speedreader/speedreader_util.cc @@ -54,6 +54,18 @@ bool URLReadableHintExtractor::HasHints(const GURL& url) { return false; } +bool PageStateIsDistilled(DistillState state) { + return state == DistillState::kReaderMode || + state == DistillState::kSpeedreaderMode; +} + +bool PageWantsDistill(DistillState state) { + return state == DistillState::kReaderMode || + state == DistillState::kSpeedreaderMode || + state == DistillState::kReaderModePending || + state == DistillState::kSpeedreaderModePending; +} + void SetEnabledForSite(HostContentSettingsMap* map, const GURL& url, bool enable) { diff --git a/components/speedreader/speedreader_util.h b/components/speedreader/speedreader_util.h index 97f98ae18889..afaaa9d70ad2 100644 --- a/components/speedreader/speedreader_util.h +++ b/components/speedreader/speedreader_util.h @@ -14,6 +14,53 @@ class HostContentSettingsMap; namespace speedreader { +// DistillState is an enum for the current state of a speedreader WebContents +enum class DistillState { + // Used as an initialization state + kUnknown, + + // The web contents is not distilled + kNone, + + // ------------------------------------------------------------------------- + // Pending states. The user requested the page be distilled. + // + // TODO(keur): Should we use bitstrings and make pending a bit both + // speedreader and reader mode can share? + + // Reader mode state that can only be reached when Speedreader is disabled + // The Speedreader icon will pop up in the address bar, and the user clicks + // it. It runs Speedreader is "Single Shot Mode". The Speedreader throttle + // is created for the following request, then deactivated. + // + // The first time a user activates reader mode on a page, a bubble drops + // down asking them to enable the Speedreader feature for automatic + // distillation. + kReaderModePending, + + // Speedreader is enabled and the page was automatically distilled. + kSpeedreaderModePending, + // ------------------------------------------------------------------------- + + // kReaderModePending was ACKed + kReaderMode, + + // kSpeedreaderModePending was ACKed + kSpeedreaderMode, + + // Speedreader is enabled, but the page was blacklisted by the user. + kSpeedreaderOnDisabledPage, + + // Speedreader is disabled, the URL passes the heuristic. + kPageProbablyReadable, +}; + +// Page is in reader mode or speedreader mode. +bool PageStateIsDistilled(DistillState state); + +// Page is in reader mode, speedreader mode, or a pending state. +bool PageWantsDistill(DistillState state); + // Enable or disable Speedreader using a ContentSettingPattern derived from the // url. void SetEnabledForSite(HostContentSettingsMap* map, From 5bf7523d76dc57b9445c35df974a67064f3c0871 Mon Sep 17 00:00:00 2001 From: Kevin Kuehler Date: Wed, 1 Sep 2021 14:06:13 -0700 Subject: [PATCH 4/7] speedreader: Simplify delegate lifetime weak_ptr --- browser/brave_content_browser_client.cc | 2 +- browser/speedreader/speedreader_tab_helper.cc | 4 ++++ browser/speedreader/speedreader_tab_helper.h | 6 ++++++ components/speedreader/speedreader_throttle.cc | 11 ++++------- components/speedreader/speedreader_throttle.h | 10 +++------- 5 files changed, 18 insertions(+), 15 deletions(-) diff --git a/browser/brave_content_browser_client.cc b/browser/brave_content_browser_client.cc index 580b8901126e..f73bd751bc2e 100644 --- a/browser/brave_content_browser_client.cc +++ b/browser/brave_content_browser_client.cc @@ -520,7 +520,7 @@ BraveContentBrowserClient::CreateURLLoaderThrottles( g_brave_browser_process->speedreader_rewriter_service(), HostContentSettingsMapFactory::GetForProfile( Profile::FromBrowserContext(browser_context)), - wc_getter, tab_helper, request.url, check_disabled_sites, + tab_helper->GetWeakPtr(), request.url, check_disabled_sites, base::ThreadTaskRunnerHandle::Get()); if (throttle) result.push_back(std::move(throttle)); diff --git a/browser/speedreader/speedreader_tab_helper.cc b/browser/speedreader/speedreader_tab_helper.cc index 277deb52707f..603a23e8276e 100644 --- a/browser/speedreader/speedreader_tab_helper.cc +++ b/browser/speedreader/speedreader_tab_helper.cc @@ -28,6 +28,10 @@ SpeedreaderTabHelper::~SpeedreaderTabHelper() { HideBubble(); } +base::WeakPtr SpeedreaderTabHelper::GetWeakPtr() { + return weak_factory_.GetWeakPtr(); +} + SpeedreaderTabHelper::SpeedreaderTabHelper(content::WebContents* web_contents) : content::WebContentsObserver(web_contents) {} diff --git a/browser/speedreader/speedreader_tab_helper.h b/browser/speedreader/speedreader_tab_helper.h index 2d2fc4e38af5..6e82e05254f8 100644 --- a/browser/speedreader/speedreader_tab_helper.h +++ b/browser/speedreader/speedreader_tab_helper.h @@ -6,6 +6,7 @@ #ifndef BRAVE_BROWSER_SPEEDREADER_SPEEDREADER_TAB_HELPER_H_ #define BRAVE_BROWSER_SPEEDREADER_SPEEDREADER_TAB_HELPER_H_ +#include "base/memory/weak_ptr.h" #include "brave/components/speedreader/speedreader_result_delegate.h" #include "brave/components/speedreader/speedreader_util.h" #include "content/public/browser/web_contents_observer.h" @@ -32,6 +33,9 @@ class SpeedreaderTabHelper SpeedreaderTabHelper(const SpeedreaderTabHelper&) = delete; SpeedreaderTabHelper& operator=(SpeedreaderTabHelper&) = delete; + // Helper function to return a weak pointer + base::WeakPtr GetWeakPtr(); + // Returns |true| if Speedreader is turned on for all sites. bool IsSpeedreaderEnabled() const; @@ -96,6 +100,8 @@ class SpeedreaderTabHelper DistillState distill_state_ = DistillState::kNone; SpeedreaderBubbleView* speedreader_bubble_ = nullptr; + base::WeakPtrFactory weak_factory_{this}; + WEB_CONTENTS_USER_DATA_KEY_DECL(); }; diff --git a/components/speedreader/speedreader_throttle.cc b/components/speedreader/speedreader_throttle.cc index 57e09229d737..fee60185e6b2 100644 --- a/components/speedreader/speedreader_throttle.cc +++ b/components/speedreader/speedreader_throttle.cc @@ -25,25 +25,22 @@ std::unique_ptr SpeedReaderThrottle::MaybeCreateThrottleFor( SpeedreaderRewriterService* rewriter_service, HostContentSettingsMap* content_settings, - const content::WebContents::Getter& wc_getter, - SpeedreaderResultDelegate* result_delegate, + base::WeakPtr result_delegate, const GURL& url, bool check_disabled_sites, scoped_refptr task_runner) { if (check_disabled_sites && !IsEnabledForSite(content_settings, url)) return nullptr; - return std::make_unique(rewriter_service, wc_getter, + return std::make_unique(rewriter_service, result_delegate, task_runner); } SpeedReaderThrottle::SpeedReaderThrottle( SpeedreaderRewriterService* rewriter_service, - const content::WebContents::Getter& wc_getter, - SpeedreaderResultDelegate* result_delegate, + base::WeakPtr result_delegate, scoped_refptr task_runner) : rewriter_service_(rewriter_service), - wc_getter_(wc_getter), result_delegate_(result_delegate), task_runner_(std::move(task_runner)) {} @@ -78,7 +75,7 @@ void SpeedReaderThrottle::Resume() { void SpeedReaderThrottle::OnDistillComplete() { DCHECK_CURRENTLY_ON(content::BrowserThread::UI); - if (wc_getter_.Run()) { + if (result_delegate_) { result_delegate_->OnDistillComplete(); } } diff --git a/components/speedreader/speedreader_throttle.h b/components/speedreader/speedreader_throttle.h index 518799ee216e..36faad291f13 100644 --- a/components/speedreader/speedreader_throttle.h +++ b/components/speedreader/speedreader_throttle.h @@ -12,7 +12,6 @@ #include "base/memory/weak_ptr.h" #include "base/single_thread_task_runner.h" #include "brave/components/speedreader/speedreader_result_delegate.h" -#include "content/public/browser/web_contents.h" #include "services/network/public/mojom/url_response_head.mojom-forward.h" #include "third_party/blink/public/common/loader/url_loader_throttle.h" @@ -33,8 +32,7 @@ class SpeedReaderThrottle : public blink::URLLoaderThrottle { static std::unique_ptr MaybeCreateThrottleFor( SpeedreaderRewriterService* rewriter_service, HostContentSettingsMap* content_settings, - const content::WebContents::Getter& wc_getter, - SpeedreaderResultDelegate* result_delegate, + base::WeakPtr result_delegate, const GURL& url, bool check_disabled_sites, scoped_refptr task_runner); @@ -43,8 +41,7 @@ class SpeedReaderThrottle : public blink::URLLoaderThrottle { // IPC in SpeedReaderLoader. |task_runner| is supposed to be bound to the // current sequence. SpeedReaderThrottle(SpeedreaderRewriterService* rewriter_service, - const content::WebContents::Getter& wc_getter, - SpeedreaderResultDelegate* result_delegate, + base::WeakPtr result_delegate, scoped_refptr task_runner); ~SpeedReaderThrottle() override; @@ -62,8 +59,7 @@ class SpeedReaderThrottle : public blink::URLLoaderThrottle { private: SpeedreaderRewriterService* rewriter_service_; // not owned - content::WebContents::Getter wc_getter_; - SpeedreaderResultDelegate* result_delegate_; + base::WeakPtr result_delegate_; scoped_refptr task_runner_; base::WeakPtrFactory weak_factory_{this}; }; From 47f68c7677c5f8d247a219390e01653908d392af Mon Sep 17 00:00:00 2001 From: Kevin Kuehler Date: Wed, 1 Sep 2021 22:51:54 -0700 Subject: [PATCH 5/7] Update entry cache in DidFinishNavigation() Previously we were persisting the SpeedreaderExtendedInfoHandler data to the NavigationEntry in OnDistillComplete(). Since OnDistillComplete() is called by the throttle, the navigation entry might not be persisted yet, so the behavior is not defined. On backward navigations we would sometimes persist to the previous navigation entry, causing bugs where pages would incorrectly show up as readable. Since we wait for DidFinishNavigation, we can safely get the last committed navgiation entry. --- browser/speedreader/speedreader_tab_helper.cc | 14 +++++++++----- browser/speedreader/speedreader_tab_helper.h | 2 ++ .../speedreader_extended_info_handler.cc | 1 - 3 files changed, 11 insertions(+), 6 deletions(-) diff --git a/browser/speedreader/speedreader_tab_helper.cc b/browser/speedreader/speedreader_tab_helper.cc index 603a23e8276e..149a80017521 100644 --- a/browser/speedreader/speedreader_tab_helper.cc +++ b/browser/speedreader/speedreader_tab_helper.cc @@ -203,10 +203,6 @@ void SpeedreaderTabHelper::HideBubble() { } void SpeedreaderTabHelper::OnDistillComplete() { - // Perform a state transition - auto* entry = web_contents()->GetController().GetLastCommittedEntry(); - DCHECK(entry); - // Perform a state transition if (distill_state_ == DistillState::kSpeedreaderModePending) { distill_state_ = DistillState::kSpeedreaderMode; @@ -217,8 +213,16 @@ void SpeedreaderTabHelper::OnDistillComplete() { DCHECK(distill_state_ == DistillState::kSpeedreaderMode || distill_state_ == DistillState::kReaderMode); } +} + +void SpeedreaderTabHelper::DidFinishNavigation( + content::NavigationHandle* navigation_handle) { + if (!navigation_handle->HasCommitted()) { + distill_state_ = DistillState::kNone; + } + auto* entry = web_contents()->GetController().GetLastCommittedEntry(); + DCHECK(entry); - // Save current distill state to navigation entry cache. SpeedreaderExtendedInfoHandler::PersistMode(entry, distill_state_); } diff --git a/browser/speedreader/speedreader_tab_helper.h b/browser/speedreader/speedreader_tab_helper.h index 6e82e05254f8..418c02fb1844 100644 --- a/browser/speedreader/speedreader_tab_helper.h +++ b/browser/speedreader/speedreader_tab_helper.h @@ -91,6 +91,8 @@ class SpeedreaderTabHelper content::NavigationHandle* navigation_handle) override; void DidRedirectNavigation( content::NavigationHandle* navigation_handle) override; + void DidFinishNavigation( + content::NavigationHandle* navigation_handle) override; // SpeedreaderResultDelegate: void OnDistillComplete() override; diff --git a/components/speedreader/speedreader_extended_info_handler.cc b/components/speedreader/speedreader_extended_info_handler.cc index 04a1b94bd137..bff49f316599 100644 --- a/components/speedreader/speedreader_extended_info_handler.cc +++ b/components/speedreader/speedreader_extended_info_handler.cc @@ -52,7 +52,6 @@ void SpeedreaderExtendedInfoHandler::PersistMode( kPageSavedSpeedreaderMode); break; default: - NOTREACHED(); return; } From d95845e81774caf6fa59a32f270df3c313bd83e9 Mon Sep 17 00:00:00 2001 From: Kevin Kuehler Date: Wed, 1 Sep 2021 22:55:40 -0700 Subject: [PATCH 6/7] speedreader: Add session logic tests We add two browser test cases for Speedreader. In these tests Speedreader is enabled. (1) Run a readable page through Speedreader. Close the browser and open it back up. The page should restore as readable. (2) Navigate to a non-readable page, then to a readable page, then do a back navigation. The readable state should not "stick". --- .../speedreader/speedreader_browsertest.cc | 96 +++++++++++++++++-- components/speedreader/speedreader_service.cc | 8 ++ components/speedreader/speedreader_service.h | 2 + test/data/{ => articles}/guardian.html | 0 4 files changed, 97 insertions(+), 9 deletions(-) rename test/data/{ => articles}/guardian.html (100%) diff --git a/browser/speedreader/speedreader_browsertest.cc b/browser/speedreader/speedreader_browsertest.cc index 53c2944c707a..6f377940acbc 100644 --- a/browser/speedreader/speedreader_browsertest.cc +++ b/browser/speedreader/speedreader_browsertest.cc @@ -12,17 +12,27 @@ #include "brave/components/speedreader/features.h" #include "brave/components/speedreader/speedreader_service.h" #include "brave/components/speedreader/speedreader_switches.h" +#include "chrome/browser/profiles/profile_keep_alive_types.h" +#include "chrome/browser/profiles/scoped_profile_keep_alive.h" #include "chrome/browser/ui/browser.h" +#include "chrome/browser/ui/browser_commands.h" +#include "chrome/browser/ui/browser_list.h" #include "chrome/test/base/in_process_browser_test.h" #include "chrome/test/base/ui_test_utils.h" +#include "components/keep_alive_registry/keep_alive_types.h" +#include "components/keep_alive_registry/scoped_keep_alive.h" #include "components/network_session_configurator/common/network_switches.h" +#include "content/public/browser/notification_service.h" +#include "content/public/browser/notification_types.h" #include "content/public/test/browser_test.h" #include "content/public/test/browser_test_utils.h" +#include "content/public/test/test_utils.h" #include "net/dns/mock_host_resolver.h" #include "net/test/embedded_test_server/embedded_test_server.h" const char kTestHost[] = "theguardian.com"; -const char kTestPage[] = "/guardian.html"; +const char kTestPageSimple[] = "/simple.html"; +const char kTestPageReadable[] = "/articles/guardian.html"; const base::FilePath::StringPieceType kTestWhitelist = FILE_PATH_LITERAL("speedreader_whitelist.json"); @@ -69,29 +79,99 @@ class SpeedReaderBrowserTest : public InProcessBrowserTest { return browser()->tab_strip_model()->GetActiveWebContents(); } + speedreader::SpeedreaderTabHelper* tab_helper() { + return speedreader::SpeedreaderTabHelper::FromWebContents( + ActiveWebContents()); + } + speedreader::SpeedreaderService* speedreader_service() { return speedreader::SpeedreaderServiceFactory::GetForProfile( browser()->profile()); } void ToggleSpeedreader() { - auto* speedreader_service = - speedreader::SpeedreaderServiceFactory::GetForProfile( - browser()->profile()); - speedreader_service->ToggleSpeedreader(); + speedreader_service()->ToggleSpeedreader(); ActiveWebContents()->GetController().Reload(content::ReloadType::NORMAL, false); } + void EnableSpeedreader() { + speedreader_service()->EnableSpeedreaderForTest(); + } + + void DisableSpeedreader() { + speedreader_service()->DisableSpeedreaderForTest(); + } + + void GoBack(Browser* browser) { + content::WindowedNotificationObserver observer( + content::NOTIFICATION_LOAD_STOP, + content::NotificationService::AllSources()); + chrome::GoBack(browser, WindowOpenDisposition::CURRENT_TAB); + observer.Wait(); + } + + void NavigateToPageSynchronously( + const char* path, + WindowOpenDisposition disposition = + WindowOpenDisposition::NEW_FOREGROUND_TAB) { + const GURL url = https_server_.GetURL(kTestHost, path); + ui_test_utils::NavigateToURLWithDisposition( + browser(), url, disposition, + ui_test_utils::BROWSER_TEST_WAIT_FOR_LOAD_STOP); + } + protected: base::test::ScopedFeatureList feature_list_; net::EmbeddedTestServer https_server_; }; +IN_PROC_BROWSER_TEST_F(SpeedReaderBrowserTest, RestoreSpeedreaderPage) { + EnableSpeedreader(); + const GURL url = https_server_.GetURL(kTestHost, kTestPageReadable); + ui_test_utils::NavigateToURLWithDisposition( + browser(), url, WindowOpenDisposition::NEW_FOREGROUND_TAB, + ui_test_utils::BROWSER_TEST_WAIT_FOR_LOAD_STOP); + EXPECT_TRUE( + speedreader::PageStateIsDistilled(tab_helper()->PageDistillState())); + + Profile* profile = browser()->profile(); + + ScopedKeepAlive test_keep_alive(KeepAliveOrigin::PANEL_VIEW, + KeepAliveRestartOption::DISABLED); + ScopedProfileKeepAlive test_profile_keep_alive( + profile, ProfileKeepAliveOrigin::kBrowserWindow); + CloseBrowserSynchronously(browser()); + + EXPECT_EQ(0u, BrowserList::GetInstance()->size()); + chrome::OpenWindowWithRestoredTabs(profile); + EXPECT_EQ(1u, BrowserList::GetInstance()->size()); + SelectFirstBrowser(); + EXPECT_TRUE( + speedreader::PageStateIsDistilled(tab_helper()->PageDistillState())); +} + +IN_PROC_BROWSER_TEST_F(SpeedReaderBrowserTest, NavigationNostickTest) { + EnableSpeedreader(); + NavigateToPageSynchronously(kTestPageSimple); + EXPECT_FALSE( + speedreader::PageStateIsDistilled(tab_helper()->PageDistillState())); + NavigateToPageSynchronously(kTestPageReadable, + WindowOpenDisposition::CURRENT_TAB); + EXPECT_TRUE( + speedreader::PageStateIsDistilled(tab_helper()->PageDistillState())); + + // Ensure distill state doesn't stick when we back-navigate from a readable + // page to a non-readable one. + GoBack(browser()); + EXPECT_FALSE( + speedreader::PageStateIsDistilled(tab_helper()->PageDistillState())); +} + // disabled in https://github.com/brave/brave-browser/issues/11328 IN_PROC_BROWSER_TEST_F(SpeedReaderBrowserTest, DISABLED_SmokeTest) { ToggleSpeedreader(); - const GURL url = https_server_.GetURL(kTestHost, kTestPage); + const GURL url = https_server_.GetURL(kTestHost, kTestPageReadable); ui_test_utils::NavigateToURL(browser(), url); content::WebContents* contents = ActiveWebContents(); content::RenderFrameHost* rfh = contents->GetMainFrame(); @@ -114,11 +194,9 @@ IN_PROC_BROWSER_TEST_F(SpeedReaderBrowserTest, DISABLED_SmokeTest) { } IN_PROC_BROWSER_TEST_F(SpeedReaderBrowserTest, P3ATest) { + DisableSpeedreader(); base::HistogramTester tester; - const GURL url = https_server_.GetURL(kTestHost, kTestPage); - ui_test_utils::NavigateToURL(browser(), url); - // SpeedReader never enabled EXPECT_FALSE(speedreader_service()->IsEnabled()); tester.ExpectBucketCount(kSpeedreaderEnabledUMAHistogramName, 0, 1); diff --git a/components/speedreader/speedreader_service.cc b/components/speedreader/speedreader_service.cc index ad1829b33b72..d64e1f74d118 100644 --- a/components/speedreader/speedreader_service.cc +++ b/components/speedreader/speedreader_service.cc @@ -95,6 +95,14 @@ void SpeedreaderService::ToggleSpeedreader() { !enabled); // toggling - now enabled } +void SpeedreaderService::EnableSpeedreaderForTest() { + prefs_->SetBoolean(kSpeedreaderPrefEnabled, true); +} + +void SpeedreaderService::DisableSpeedreaderForTest() { + prefs_->SetBoolean(kSpeedreaderPrefEnabled, false); +} + bool SpeedreaderService::IsEnabled() { if (!base::FeatureList::IsEnabled(kSpeedreaderFeature)) { return false; diff --git a/components/speedreader/speedreader_service.h b/components/speedreader/speedreader_service.h index ede26c1e52d4..b0353c8367d5 100644 --- a/components/speedreader/speedreader_service.h +++ b/components/speedreader/speedreader_service.h @@ -23,6 +23,8 @@ class SpeedreaderService : public KeyedService { static void RegisterProfilePrefs(PrefRegistrySimple* registry); void ToggleSpeedreader(); + void EnableSpeedreaderForTest(); + void DisableSpeedreaderForTest(); bool IsEnabled(); bool ShouldPromptUserToEnable() const; void IncrementPromptCount(); diff --git a/test/data/guardian.html b/test/data/articles/guardian.html similarity index 100% rename from test/data/guardian.html rename to test/data/articles/guardian.html From c5677c6be5c26b43f9edd29e46b6fca6880eab22 Mon Sep 17 00:00:00 2001 From: Kevin Kuehler Date: Thu, 2 Sep 2021 11:34:44 -0700 Subject: [PATCH 7/7] speedreader tab helper and test fixes * Use NavigationEntryCommitted() instead of DidNavigationFinish(). We have no guarantees about the next DidNavigationStart() being called first. * Fix throttle unit test * Make several review requests to SpeedreaderBrowserTest --- .../speedreader/speedreader_browsertest.cc | 23 +++------- browser/speedreader/speedreader_tab_helper.cc | 20 +++++---- browser/speedreader/speedreader_tab_helper.h | 4 +- .../speedreader_extended_info_handler.cc | 19 ++++---- .../speedreader_extended_info_handler.h | 5 ++- components/speedreader/speedreader_service.cc | 6 +-- components/speedreader/speedreader_service.h | 1 - .../speedreader/speedreader_throttle.cc | 1 - .../speedreader_throttle_unittest.cc | 45 +++++++++++-------- 9 files changed, 60 insertions(+), 64 deletions(-) diff --git a/browser/speedreader/speedreader_browsertest.cc b/browser/speedreader/speedreader_browsertest.cc index 6f377940acbc..c410a72e139a 100644 --- a/browser/speedreader/speedreader_browsertest.cc +++ b/browser/speedreader/speedreader_browsertest.cc @@ -22,10 +22,9 @@ #include "components/keep_alive_registry/keep_alive_types.h" #include "components/keep_alive_registry/scoped_keep_alive.h" #include "components/network_session_configurator/common/network_switches.h" -#include "content/public/browser/notification_service.h" -#include "content/public/browser/notification_types.h" #include "content/public/test/browser_test.h" #include "content/public/test/browser_test_utils.h" +#include "content/public/test/test_navigation_observer.h" #include "content/public/test/test_utils.h" #include "net/dns/mock_host_resolver.h" #include "net/test/embedded_test_server/embedded_test_server.h" @@ -75,6 +74,8 @@ class SpeedReaderBrowserTest : public InProcessBrowserTest { host_resolver()->AddRule("*", "127.0.0.1"); } + void TearDownOnMainThread() override { DisableSpeedreader(); } + content::WebContents* ActiveWebContents() { return browser()->tab_strip_model()->GetActiveWebContents(); } @@ -95,18 +96,12 @@ class SpeedReaderBrowserTest : public InProcessBrowserTest { false); } - void EnableSpeedreader() { - speedreader_service()->EnableSpeedreaderForTest(); - } - void DisableSpeedreader() { speedreader_service()->DisableSpeedreaderForTest(); } void GoBack(Browser* browser) { - content::WindowedNotificationObserver observer( - content::NOTIFICATION_LOAD_STOP, - content::NotificationService::AllSources()); + content::TestNavigationObserver observer(ActiveWebContents()); chrome::GoBack(browser, WindowOpenDisposition::CURRENT_TAB); observer.Wait(); } @@ -127,11 +122,8 @@ class SpeedReaderBrowserTest : public InProcessBrowserTest { }; IN_PROC_BROWSER_TEST_F(SpeedReaderBrowserTest, RestoreSpeedreaderPage) { - EnableSpeedreader(); - const GURL url = https_server_.GetURL(kTestHost, kTestPageReadable); - ui_test_utils::NavigateToURLWithDisposition( - browser(), url, WindowOpenDisposition::NEW_FOREGROUND_TAB, - ui_test_utils::BROWSER_TEST_WAIT_FOR_LOAD_STOP); + ToggleSpeedreader(); + NavigateToPageSynchronously(kTestPageReadable); EXPECT_TRUE( speedreader::PageStateIsDistilled(tab_helper()->PageDistillState())); @@ -152,7 +144,7 @@ IN_PROC_BROWSER_TEST_F(SpeedReaderBrowserTest, RestoreSpeedreaderPage) { } IN_PROC_BROWSER_TEST_F(SpeedReaderBrowserTest, NavigationNostickTest) { - EnableSpeedreader(); + ToggleSpeedreader(); NavigateToPageSynchronously(kTestPageSimple); EXPECT_FALSE( speedreader::PageStateIsDistilled(tab_helper()->PageDistillState())); @@ -194,7 +186,6 @@ IN_PROC_BROWSER_TEST_F(SpeedReaderBrowserTest, DISABLED_SmokeTest) { } IN_PROC_BROWSER_TEST_F(SpeedReaderBrowserTest, P3ATest) { - DisableSpeedreader(); base::HistogramTester tester; // SpeedReader never enabled diff --git a/browser/speedreader/speedreader_tab_helper.cc b/browser/speedreader/speedreader_tab_helper.cc index 149a80017521..5235d3251b69 100644 --- a/browser/speedreader/speedreader_tab_helper.cc +++ b/browser/speedreader/speedreader_tab_helper.cc @@ -18,6 +18,7 @@ #include "chrome/browser/profiles/profile.h" #include "chrome/browser/ui/browser.h" #include "chrome/browser/ui/browser_finder.h" +#include "content/public/browser/navigation_details.h" #include "content/public/browser/navigation_entry.h" #include "content/public/browser/navigation_handle.h" #include "content/public/browser/web_contents.h" @@ -97,9 +98,14 @@ bool SpeedreaderTabHelper::MaybeUpdateCachedState( if (!entry) { return false; } + Profile* profile = + Profile::FromBrowserContext(web_contents()->GetBrowserContext()); + DCHECK(profile); + auto* speedreader_service = SpeedreaderServiceFactory::GetForProfile(profile); bool cached = false; - DistillState state = SpeedreaderExtendedInfoHandler::GetCachedMode(entry); + DistillState state = + SpeedreaderExtendedInfoHandler::GetCachedMode(entry, speedreader_service); if (state != DistillState::kUnknown) { cached = true; distill_state_ = state; @@ -215,15 +221,11 @@ void SpeedreaderTabHelper::OnDistillComplete() { } } -void SpeedreaderTabHelper::DidFinishNavigation( - content::NavigationHandle* navigation_handle) { - if (!navigation_handle->HasCommitted()) { - distill_state_ = DistillState::kNone; - } +void SpeedreaderTabHelper::DidStopLoading() { auto* entry = web_contents()->GetController().GetLastCommittedEntry(); - DCHECK(entry); - - SpeedreaderExtendedInfoHandler::PersistMode(entry, distill_state_); + if (entry) { + SpeedreaderExtendedInfoHandler::PersistMode(entry, distill_state_); + } } WEB_CONTENTS_USER_DATA_KEY_IMPL(SpeedreaderTabHelper) diff --git a/browser/speedreader/speedreader_tab_helper.h b/browser/speedreader/speedreader_tab_helper.h index 418c02fb1844..66bb71cc3cd1 100644 --- a/browser/speedreader/speedreader_tab_helper.h +++ b/browser/speedreader/speedreader_tab_helper.h @@ -33,7 +33,6 @@ class SpeedreaderTabHelper SpeedreaderTabHelper(const SpeedreaderTabHelper&) = delete; SpeedreaderTabHelper& operator=(SpeedreaderTabHelper&) = delete; - // Helper function to return a weak pointer base::WeakPtr GetWeakPtr(); // Returns |true| if Speedreader is turned on for all sites. @@ -91,8 +90,7 @@ class SpeedreaderTabHelper content::NavigationHandle* navigation_handle) override; void DidRedirectNavigation( content::NavigationHandle* navigation_handle) override; - void DidFinishNavigation( - content::NavigationHandle* navigation_handle) override; + void DidStopLoading() override; // SpeedreaderResultDelegate: void OnDistillComplete() override; diff --git a/components/speedreader/speedreader_extended_info_handler.cc b/components/speedreader/speedreader_extended_info_handler.cc index bff49f316599..d59f22fa07d8 100644 --- a/components/speedreader/speedreader_extended_info_handler.cc +++ b/components/speedreader/speedreader_extended_info_handler.cc @@ -9,6 +9,7 @@ #include #include "base/supports_user_data.h" +#include "brave/components/speedreader/speedreader_service.h" #include "components/sessions/content/content_serialized_navigation_driver.h" #include "content/public/browser/navigation_entry.h" @@ -18,8 +19,7 @@ namespace { // navigation entry user data. constexpr char kSpeedreaderKey[] = "speedreader"; -constexpr char kPageSavedReaderMode[] = "reader-mode"; -constexpr char kPageSavedSpeedreaderMode[] = "speedreader-mode"; +constexpr char kPageSavedDistilled[] = "distilled"; struct SpeedreaderNavigationData : public base::SupportsUserData::Data { explicit SpeedreaderNavigationData(const std::string& value) : value(value) {} @@ -45,11 +45,8 @@ void SpeedreaderExtendedInfoHandler::PersistMode( std::unique_ptr data; switch (state) { case DistillState::kReaderMode: - data = std::make_unique(kPageSavedReaderMode); - break; case DistillState::kSpeedreaderMode: - data = std::make_unique( - kPageSavedSpeedreaderMode); + data = std::make_unique(kPageSavedDistilled); break; default: return; @@ -60,17 +57,17 @@ void SpeedreaderExtendedInfoHandler::PersistMode( // static DistillState SpeedreaderExtendedInfoHandler::GetCachedMode( - content::NavigationEntry* entry) { + content::NavigationEntry* entry, + SpeedreaderService* service) { DCHECK(entry); auto* data = static_cast( entry->GetUserData(kSpeedreaderKey)); if (!data) { return DistillState::kUnknown; } - if (data->value == kPageSavedReaderMode) { - return DistillState::kReaderMode; - } else if (data->value == kPageSavedSpeedreaderMode) { - return DistillState::kSpeedreaderMode; + if (data->value == kPageSavedDistilled) { + return service->IsEnabled() ? DistillState::kSpeedreaderMode + : DistillState::kReaderMode; } else { return DistillState::kUnknown; } diff --git a/components/speedreader/speedreader_extended_info_handler.h b/components/speedreader/speedreader_extended_info_handler.h index 4109f2f51edf..05b03343a85a 100644 --- a/components/speedreader/speedreader_extended_info_handler.h +++ b/components/speedreader/speedreader_extended_info_handler.h @@ -17,6 +17,8 @@ class NavigationEntry; namespace speedreader { +class SpeedreaderService; + // This class is meant to persist data to a content::NavigationEntry so that // distilled pages will be recognized on a restored session. class SpeedreaderExtendedInfoHandler : public sessions::ExtendedInfoHandler { @@ -30,7 +32,8 @@ class SpeedreaderExtendedInfoHandler : public sessions::ExtendedInfoHandler { // Retrieve cached speedreader state from NavigationEntry. Returns // DistillState::kUnknown if not cached. - static DistillState GetCachedMode(content::NavigationEntry* entry); + static DistillState GetCachedMode(content::NavigationEntry* entry, + SpeedreaderService* service); // Clear the NavigationEntry speedreader state static void ClearPersistedData(content::NavigationEntry* entry); diff --git a/components/speedreader/speedreader_service.cc b/components/speedreader/speedreader_service.cc index d64e1f74d118..a30be5b2c1cb 100644 --- a/components/speedreader/speedreader_service.cc +++ b/components/speedreader/speedreader_service.cc @@ -95,12 +95,10 @@ void SpeedreaderService::ToggleSpeedreader() { !enabled); // toggling - now enabled } -void SpeedreaderService::EnableSpeedreaderForTest() { - prefs_->SetBoolean(kSpeedreaderPrefEnabled, true); -} - void SpeedreaderService::DisableSpeedreaderForTest() { prefs_->SetBoolean(kSpeedreaderPrefEnabled, false); + prefs_->SetBoolean(kSpeedreaderPrefEverEnabled, false); + prefs_->SetInteger(kSpeedreaderPrefPromptCount, 0); } bool SpeedreaderService::IsEnabled() { diff --git a/components/speedreader/speedreader_service.h b/components/speedreader/speedreader_service.h index b0353c8367d5..464dcd8efaac 100644 --- a/components/speedreader/speedreader_service.h +++ b/components/speedreader/speedreader_service.h @@ -23,7 +23,6 @@ class SpeedreaderService : public KeyedService { static void RegisterProfilePrefs(PrefRegistrySimple* registry); void ToggleSpeedreader(); - void EnableSpeedreaderForTest(); void DisableSpeedreaderForTest(); bool IsEnabled(); bool ShouldPromptUserToEnable() const; diff --git a/components/speedreader/speedreader_throttle.cc b/components/speedreader/speedreader_throttle.cc index fee60185e6b2..10b2aa4cce65 100644 --- a/components/speedreader/speedreader_throttle.cc +++ b/components/speedreader/speedreader_throttle.cc @@ -14,7 +14,6 @@ #include "components/content_settings/core/browser/host_content_settings_map.h" #include "components/content_settings/core/common/content_settings.h" #include "content/public/browser/browser_thread.h" -#include "content/public/browser/web_contents_user_data.h" #include "mojo/public/cpp/bindings/pending_receiver.h" #include "mojo/public/cpp/bindings/pending_remote.h" #include "services/network/public/mojom/url_response_head.mojom.h" diff --git a/components/speedreader/speedreader_throttle_unittest.cc b/components/speedreader/speedreader_throttle_unittest.cc index 2402d05803f2..2b4956ee84a7 100644 --- a/components/speedreader/speedreader_throttle_unittest.cc +++ b/components/speedreader/speedreader_throttle_unittest.cc @@ -6,6 +6,7 @@ #include #include +#include "brave/components/speedreader/speedreader_result_delegate.h" #include "brave/components/speedreader/speedreader_rewriter_service.h" #include "brave/components/speedreader/speedreader_throttle.h" #include "brave/components/speedreader/speedreader_util.h" @@ -24,7 +25,14 @@ namespace speedreader { namespace { + constexpr char kTestProfileName[] = "TestProfile"; + +class TestSpeedreaderResultDelegate : public SpeedreaderResultDelegate { + protected: + void OnDistillComplete() override {} +}; + } // anonymous namespace class SpeedreaderThrottleTest : public testing::Test { @@ -59,18 +67,26 @@ class SpeedreaderThrottleTest : public testing::Test { return HostContentSettingsMapFactory::GetForProfile(profile()); } + std::unique_ptr speedreader_throttle( + const GURL& url, + bool check_disabled_sites = false) { + auto runner = content::GetUIThreadTaskRunner({}); + return SpeedReaderThrottle::MaybeCreateThrottleFor( + nullptr, content_settings(), + base::WeakPtr(), url, + check_disabled_sites, runner); + } + private: content::BrowserTaskEnvironment task_environment_; std::unique_ptr web_contents_; std::unique_ptr profile_manager_; TestingProfile* profile_; + TestSpeedreaderResultDelegate delegate_; }; TEST_F(SpeedreaderThrottleTest, AllowThrottle) { - auto runner = content::GetUIThreadTaskRunner({}); - std::unique_ptr throttle = - SpeedReaderThrottle::MaybeCreateThrottleFor(nullptr, content_settings(), - url(), false, runner); + auto throttle = speedreader_throttle(url(), false /* check_disabled_sites */); EXPECT_NE(throttle.get(), nullptr); } @@ -79,17 +95,15 @@ TEST_F(SpeedreaderThrottleTest, ToggleThrottle) { std::unique_ptr throttle; speedreader::SetEnabledForSite(content_settings(), url(), false); - throttle = SpeedReaderThrottle::MaybeCreateThrottleFor( - nullptr, content_settings(), url(), true, runner); + throttle = speedreader_throttle(url(), true /* check_disabled_sites */); EXPECT_EQ(throttle.get(), nullptr); // no other domains are affected by the rule. - throttle = SpeedReaderThrottle::MaybeCreateThrottleFor( - nullptr, content_settings(), GURL("kevin.com"), true, runner); + throttle = + speedreader_throttle(GURL("kevin.com"), true /* check_disabled_sites */); EXPECT_NE(throttle.get(), nullptr); speedreader::SetEnabledForSite(content_settings(), url(), true); - throttle = SpeedReaderThrottle::MaybeCreateThrottleFor( - nullptr, content_settings(), url(), true, runner); + throttle = speedreader_throttle(url(), true /* check_disabled_sites */); EXPECT_NE(throttle.get(), nullptr); } @@ -99,14 +113,10 @@ TEST_F(SpeedreaderThrottleTest, ThrottleIgnoreDisabled) { speedreader::SetEnabledForSite(content_settings(), url(), false); - throttle = SpeedReaderThrottle::MaybeCreateThrottleFor( - nullptr, content_settings(), url(), true /* check_disabled_sites */, - runner); + throttle = speedreader_throttle(url(), true /* check_disabled_sites */); EXPECT_EQ(throttle.get(), nullptr); - throttle = SpeedReaderThrottle::MaybeCreateThrottleFor( - nullptr, content_settings(), url(), false /* check_disabled_sites */, - runner); + throttle = speedreader_throttle(url(), false /* check_disabled_sites */); EXPECT_NE(throttle.get(), nullptr); } @@ -118,8 +128,7 @@ TEST_F(SpeedreaderThrottleTest, ThrottleNestedURL) { // to all of brave.com. speedreader::SetEnabledForSite( content_settings(), GURL("https://brave.com/some/nested/page"), false); - throttle = SpeedReaderThrottle::MaybeCreateThrottleFor( - nullptr, content_settings(), url(), true, runner); + throttle = speedreader_throttle(url(), true /* check_disabled_sites */); EXPECT_EQ(throttle.get(), nullptr); }