From 7977845b1cda9624cf684d2a443d3602fb4115f0 Mon Sep 17 00:00:00 2001 From: Pavel Beloborodov Date: Thu, 17 Mar 2022 15:39:30 +0700 Subject: [PATCH 01/10] Fixed speedreader & reader page actions. --- browser/speedreader/speedreader_tab_helper.cc | 250 ++++++++++++------ browser/speedreader/speedreader_tab_helper.h | 31 ++- browser/ui/browser_commands.cc | 29 +- .../speedreader/speedreader_icon_view.cc | 4 +- components/speedreader/speedreader_service.cc | 12 +- components/speedreader/speedreader_service.h | 5 +- 6 files changed, 210 insertions(+), 121 deletions(-) diff --git a/browser/speedreader/speedreader_tab_helper.cc b/browser/speedreader/speedreader_tab_helper.cc index 62f28815ec69..f65122f998a4 100644 --- a/browser/speedreader/speedreader_tab_helper.cc +++ b/browser/speedreader/speedreader_tab_helper.cc @@ -5,11 +5,15 @@ #include "brave/browser/speedreader/speedreader_tab_helper.h" +#include + +#include "base/bind.h" #include "brave/browser/brave_browser_process.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_pref_names.h" #include "brave/components/speedreader/speedreader_rewriter_service.h" #include "brave/components/speedreader/speedreader_service.h" #include "brave/components/speedreader/speedreader_util.h" @@ -17,6 +21,8 @@ #include "chrome/browser/profiles/profile.h" #include "chrome/browser/ui/browser.h" #include "chrome/browser/ui/browser_finder.h" +#include "components/content_settings/core/browser/host_content_settings_map.h" +#include "components/prefs/pref_change_registrar.h" #include "content/public/browser/navigation_details.h" #include "content/public/browser/navigation_entry.h" #include "content/public/browser/navigation_handle.h" @@ -24,23 +30,31 @@ namespace speedreader { +SpeedreaderTabHelper::SpeedreaderTabHelper(content::WebContents* web_contents) + : content::WebContentsObserver(web_contents), + content::WebContentsUserData(*web_contents) { + pref_change_registrar_ = std::make_unique(); + pref_change_registrar_->Init(GetProfile()->GetPrefs()); + pref_change_registrar_->Add( + kSpeedreaderPrefEnabled, + base::BindRepeating(&SpeedreaderTabHelper::OnPrefChanged, + weak_factory_.GetWeakPtr())); + content_rules_ = HostContentSettingsMapFactory::GetForProfile( + web_contents->GetBrowserContext()); +} + SpeedreaderTabHelper::~SpeedreaderTabHelper() { - HideBubble(); + DCHECK(!pref_change_registrar_); + DCHECK(!speedreader_bubble_); + DCHECK(!content_rules_); } base::WeakPtr SpeedreaderTabHelper::GetWeakPtr() { return weak_factory_.GetWeakPtr(); } -SpeedreaderTabHelper::SpeedreaderTabHelper(content::WebContents* web_contents) - : content::WebContentsObserver(web_contents), - content::WebContentsUserData(*web_contents) {} - bool SpeedreaderTabHelper::IsSpeedreaderEnabled() const { - Profile* profile = - Profile::FromBrowserContext(web_contents()->GetBrowserContext()); - DCHECK(profile); - return SpeedreaderServiceFactory::GetForProfile(profile)->IsEnabled(); + return SpeedreaderServiceFactory::GetForProfile(GetProfile())->IsEnabled(); } bool SpeedreaderTabHelper::IsEnabledForSite() { @@ -50,52 +64,62 @@ bool SpeedreaderTabHelper::IsEnabledForSite() { bool SpeedreaderTabHelper::IsEnabledForSite(const GURL& url) { if (!IsSpeedreaderEnabled()) return false; + return speedreader::IsEnabledForSite(content_rules_, url); +} - Profile* profile = - Profile::FromBrowserContext(web_contents()->GetBrowserContext()); - auto* content_rules = HostContentSettingsMapFactory::GetForProfile(profile); - return speedreader::IsEnabledForSite(content_rules, url); +void SpeedreaderTabHelper::Distill() { + switch (distill_state_) { + case DistillState::kSpeedreaderMode: + case DistillState::kSpeedreaderOnDisabledPage: + ShowSpeedreaderBubble(); + break; + case DistillState::kReaderMode: + SetNextRequestState(DistillState::kPageProbablyReadable); + ClearCache(); + ReloadContents(); + break; + case DistillState::kPageProbablyReadable: + SingleShotSpeedreader(); + break; + default: + NOTREACHED(); + } } void SpeedreaderTabHelper::MaybeToggleEnabledForSite(bool on) { if (!IsSpeedreaderEnabled()) return; - if (auto* entry = web_contents()->GetController().GetLastCommittedEntry()) { - SpeedreaderExtendedInfoHandler::ClearPersistedData(entry); - } + const bool enabled = speedreader::IsEnabledForSite( + content_rules_, web_contents()->GetLastCommittedURL()); + if (enabled == on) + return; - Profile* profile = - Profile::FromBrowserContext(web_contents()->GetBrowserContext()); - auto* content_rules = HostContentSettingsMapFactory::GetForProfile(profile); - bool enabled = speedreader::IsEnabledForSite( - content_rules, web_contents()->GetLastCommittedURL()); - if (enabled != on) { - speedreader::SetEnabledForSite(content_rules, - web_contents()->GetLastCommittedURL(), on); - web_contents()->GetController().Reload(content::ReloadType::NORMAL, false); - } + speedreader::SetEnabledForSite(content_rules_, + web_contents()->GetLastCommittedURL(), on); + ClearCache(); + ReloadContents(); } void SpeedreaderTabHelper::SingleShotSpeedreader() { single_shot_next_request_ = true; // Refresh the page so it runs through the speedreader throttle - auto* contents = web_contents(); - if (contents) - contents->GetController().Reload(content::ReloadType::NORMAL, false); + ReloadContents(); // Determine if bubble should be shown automatically - Profile* profile = - Profile::FromBrowserContext(web_contents()->GetBrowserContext()); - DCHECK(profile); - auto* speedreader_service = SpeedreaderServiceFactory::GetForProfile(profile); + auto* speedreader_service = + SpeedreaderServiceFactory::GetForProfile(GetProfile()); if (speedreader_service->ShouldPromptUserToEnable()) { ShowReaderModeBubble(); speedreader_service->IncrementPromptCount(); } } +SpeedreaderBubbleView* SpeedreaderTabHelper::speedreader_bubble_view() const { + return speedreader_bubble_; +} + bool SpeedreaderTabHelper::MaybeUpdateCachedState( content::NavigationHandle* handle) { auto* entry = handle->GetNavigationEntry(); @@ -107,39 +131,33 @@ bool SpeedreaderTabHelper::MaybeUpdateCachedState( DCHECK(profile); auto* speedreader_service = SpeedreaderServiceFactory::GetForProfile(profile); - bool cached = false; - DistillState state = + const DistillState state = SpeedreaderExtendedInfoHandler::GetCachedMode(entry, speedreader_service); - if (state != DistillState::kUnknown) { - cached = true; + const bool cached = state != DistillState::kUnknown; + if (cached) { distill_state_ = state; - } - if (!cached) { + } else { SpeedreaderExtendedInfoHandler::ClearPersistedData(entry); } return cached; } -void SpeedreaderTabHelper::UpdateActiveState( - content::NavigationHandle* handle) { - DCHECK(handle); - DCHECK(handle->IsInMainFrame()); - +void SpeedreaderTabHelper::UpdateActiveState(const GURL& url) { if (single_shot_next_request_) { SetNextRequestState(DistillState::kReaderModePending); return; } // Work only with casual main frame navigations. - if (handle->GetURL().SchemeIsHTTPOrHTTPS()) { + if (url.SchemeIsHTTPOrHTTPS()) { auto* rewriter_service = g_brave_browser_process->speedreader_rewriter_service(); - if (rewriter_service->URLLooksReadable(handle->GetURL())) { + if (rewriter_service->URLLooksReadable(url)) { VLOG(2) << __func__ - << "URL passed speedreader heuristic: " << handle->GetURL(); + << "URL passed speedreader heuristic: " << url; if (!IsSpeedreaderEnabled()) { SetNextRequestState(DistillState::kPageProbablyReadable); - } else if (!IsEnabledForSite(handle->GetURL())) { + } else if (!IsEnabledForSite(url)) { SetNextRequestState(DistillState::kSpeedreaderOnDisabledPage); } else { SetNextRequestState(DistillState::kSpeedreaderModePending); @@ -155,34 +173,9 @@ void SpeedreaderTabHelper::SetNextRequestState(DistillState state) { single_shot_next_request_ = false; } -void SpeedreaderTabHelper::DidStartNavigation( - content::NavigationHandle* navigation_handle) { - if (navigation_handle->IsInMainFrame()) { - if (!MaybeUpdateCachedState(navigation_handle)) { - UpdateActiveState(navigation_handle); - } - } -} - -void SpeedreaderTabHelper::DidRedirectNavigation( - content::NavigationHandle* navigation_handle) { - if (navigation_handle->IsInMainFrame()) { - if (!MaybeUpdateCachedState(navigation_handle)) { - UpdateActiveState(navigation_handle); - } - } -} - -SpeedreaderBubbleView* SpeedreaderTabHelper::speedreader_bubble_view() const { - return speedreader_bubble_; -} - void SpeedreaderTabHelper::OnBubbleClosed() { speedreader_bubble_ = nullptr; - auto* contents = web_contents(); - Browser* browser = chrome::FindBrowserWithWebContents(contents); - DCHECK(browser); - browser->window()->UpdatePageActionIcon(PageActionIconType::kReaderMode); + UpdateButtonIfNeeded(); } void SpeedreaderTabHelper::ShowSpeedreaderBubble() { @@ -193,6 +186,12 @@ void SpeedreaderTabHelper::ShowReaderModeBubble() { ShowBubble(false); } +Profile* SpeedreaderTabHelper::GetProfile() const { + auto *profile = Profile::FromBrowserContext(web_contents()->GetBrowserContext()); + DCHECK(profile); + return profile; +} + void SpeedreaderTabHelper::ShowBubble(bool is_bubble_speedreader) { auto* contents = web_contents(); Browser* browser = chrome::FindBrowserWithWebContents(contents); @@ -200,10 +199,8 @@ void SpeedreaderTabHelper::ShowBubble(bool is_bubble_speedreader) { speedreader_bubble_ = static_cast(browser->window()) ->ShowSpeedreaderBubble(this, is_bubble_speedreader); - browser->window()->UpdatePageActionIcon(PageActionIconType::kReaderMode); } -// Hides speedreader information void SpeedreaderTabHelper::HideBubble() { if (speedreader_bubble_) { speedreader_bubble_->Hide(); @@ -211,6 +208,100 @@ void SpeedreaderTabHelper::HideBubble() { } } +void SpeedreaderTabHelper::ClearCache() { + if (auto* entry = web_contents()->GetController().GetLastCommittedEntry()) { + SpeedreaderExtendedInfoHandler::ClearPersistedData(entry); + } +} + +void SpeedreaderTabHelper::ReloadContents() { + web_contents()->GetController().Reload(content::ReloadType::NORMAL, false); +} + +void SpeedreaderTabHelper::ProcessNavigation( + content::NavigationHandle* navigation_handle) { + if (!navigation_handle->IsInMainFrame() || + MaybeUpdateCachedState(navigation_handle)) { + return; + } + + UpdateActiveState(navigation_handle->GetURL()); + UpdateButtonIfNeeded(); +} + +void SpeedreaderTabHelper::OnPrefChanged() { + const bool is_speedreader_enabled = IsSpeedreaderEnabled(); + + switch (distill_state_) { + case DistillState::kUnknown: + case DistillState::kNone: + break; // Nothing to do. + case DistillState::kPageProbablyReadable: + if (is_speedreader_enabled) { + distill_state_ = DistillState::kSpeedreaderOnDisabledPage; + } + break; + case DistillState::kSpeedreaderMode: + if (!is_speedreader_enabled) { + distill_state_ = DistillState::kReaderMode; + } + break; + case DistillState::kReaderMode: + if (is_speedreader_enabled) { + distill_state_ = DistillState::kSpeedreaderMode; + } else { + distill_state_ = DistillState::kSpeedreaderOnDisabledPage; + } + break; + case DistillState::kSpeedreaderOnDisabledPage: { + if (!is_speedreader_enabled) { + distill_state_ = DistillState::kPageProbablyReadable; + } + break; + } + default: + break; + } + + UpdateButtonIfNeeded(); +} + +void SpeedreaderTabHelper::UpdateButtonIfNeeded() { + if (!is_active_) + return; + if (const auto* browser = + chrome::FindBrowserWithWebContents(web_contents())) { + browser->window()->UpdatePageActionIcon(PageActionIconType::kReaderMode); + } +} + +void SpeedreaderTabHelper::DidStartNavigation( + content::NavigationHandle* navigation_handle) { + ProcessNavigation(navigation_handle); +} + +void SpeedreaderTabHelper::DidRedirectNavigation( + content::NavigationHandle* navigation_handle) { + ProcessNavigation(navigation_handle); +} + +void SpeedreaderTabHelper::DidStopLoading() { + auto* entry = web_contents()->GetController().GetLastCommittedEntry(); + if (entry) { + SpeedreaderExtendedInfoHandler::PersistMode(entry, distill_state_); + } +} + +void SpeedreaderTabHelper::OnVisibilityChanged(content::Visibility visibility) { + is_active_ = visibility != content::Visibility::HIDDEN; +} + +void SpeedreaderTabHelper::WebContentsDestroyed() { + pref_change_registrar_.reset(); + content_rules_ = nullptr; + HideBubble(); +} + void SpeedreaderTabHelper::OnDistillComplete() { // Perform a state transition if (distill_state_ == DistillState::kSpeedreaderModePending) { @@ -222,13 +313,8 @@ void SpeedreaderTabHelper::OnDistillComplete() { DCHECK(distill_state_ == DistillState::kSpeedreaderMode || distill_state_ == DistillState::kReaderMode); } -} -void SpeedreaderTabHelper::DidStopLoading() { - auto* entry = web_contents()->GetController().GetLastCommittedEntry(); - if (entry) { - SpeedreaderExtendedInfoHandler::PersistMode(entry, distill_state_); - } + UpdateButtonIfNeeded(); } WEB_CONTENTS_USER_DATA_KEY_IMPL(SpeedreaderTabHelper); diff --git a/browser/speedreader/speedreader_tab_helper.h b/browser/speedreader/speedreader_tab_helper.h index 66bb71cc3cd1..e81a808ec3f5 100644 --- a/browser/speedreader/speedreader_tab_helper.h +++ b/browser/speedreader/speedreader_tab_helper.h @@ -6,18 +6,20 @@ #ifndef BRAVE_BROWSER_SPEEDREADER_SPEEDREADER_TAB_HELPER_H_ #define BRAVE_BROWSER_SPEEDREADER_SPEEDREADER_TAB_HELPER_H_ +#include + #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" #include "content/public/browser/web_contents_user_data.h" +class PrefChangeRegistrar; namespace content { class NavigationEntry; class NavigationHandle; class WebContents; } // namespace content - namespace speedreader { class SpeedreaderBubbleView; @@ -42,6 +44,8 @@ class SpeedreaderTabHelper // active web contents is blacklisted. bool IsEnabledForSite(); + void Distill(); + DistillState PageDistillState() const { return distill_state_; } // Allow or deny a site from being run through speedreader if |on| toggles @@ -71,6 +75,8 @@ class SpeedreaderTabHelper friend class content::WebContentsUserData; explicit SpeedreaderTabHelper(content::WebContents* web_contents); + Profile* GetProfile() const; + // Called by ShowSpeedreaderBubble and ShowReaderModeBubble. // |is_bubble_speedreader| will show a bubble for pages in Speedreader if set // to true, otherwise pages in reader mode. @@ -82,23 +88,38 @@ class SpeedreaderTabHelper bool IsEnabledForSite(const GURL& url); bool MaybeUpdateCachedState(content::NavigationHandle* handle); - void UpdateActiveState(content::NavigationHandle* handle); + void UpdateActiveState(const GURL& url); void SetNextRequestState(DistillState state); - // content::WebContentsObserver + void ClearCache(); + void ReloadContents(); + void ProcessNavigation(content::NavigationHandle* navigation_handle); + + void OnPrefChanged(); + + void UpdateButtonIfNeeded(); + + // content::WebContentsObserver: void DidStartNavigation( content::NavigationHandle* navigation_handle) override; void DidRedirectNavigation( content::NavigationHandle* navigation_handle) override; void DidStopLoading() override; + void OnVisibilityChanged(content::Visibility visibility) override; + void WebContentsDestroyed() override; // SpeedreaderResultDelegate: - void OnDistillComplete() override; + void OnDistillComplete() override; + + std::unique_ptr pref_change_registrar_; + + bool is_active_ = false; bool single_shot_next_request_ = false; // run speedreader once on next page load DistillState distill_state_ = DistillState::kNone; - SpeedreaderBubbleView* speedreader_bubble_ = nullptr; + raw_ptr speedreader_bubble_ = nullptr; + raw_ptr content_rules_ = nullptr; base::WeakPtrFactory weak_factory_{this}; diff --git a/browser/ui/browser_commands.cc b/browser/ui/browser_commands.cc index 33eaca2ed7cf..5fbd693d7d65 100644 --- a/browser/ui/browser_commands.cc +++ b/browser/ui/browser_commands.cc @@ -99,31 +99,14 @@ void OpenGuestProfile() { void MaybeDistillAndShowSpeedreaderBubble(Browser* browser) { #if BUILDFLAG(ENABLE_SPEEDREADER) - using DistillState = speedreader::DistillState; WebContents* contents = browser->tab_strip_model()->GetActiveWebContents(); - if (contents) { - auto* tab_helper = - speedreader::SpeedreaderTabHelper::FromWebContents(contents); - if (!tab_helper) - return; - - const DistillState state = tab_helper->PageDistillState(); - switch (state) { - case DistillState::kSpeedreaderMode: - case DistillState::kSpeedreaderOnDisabledPage: - tab_helper->ShowSpeedreaderBubble(); - break; - case DistillState::kReaderMode: - // Refresh the page (toggles off Speedreader) - contents->GetController().Reload(content::ReloadType::NORMAL, false); - break; - case DistillState::kPageProbablyReadable: - tab_helper->SingleShotSpeedreader(); - break; - default: - NOTREACHED(); - } + if (!contents) + return; + if (auto* tab_helper = + speedreader::SpeedreaderTabHelper::FromWebContents(contents)) { + tab_helper->Distill(); } + #endif // BUILDFLAG(ENABLE_SPEEDREADER) } diff --git a/browser/ui/views/speedreader/speedreader_icon_view.cc b/browser/ui/views/speedreader/speedreader_icon_view.cc index 44b5ce96ef37..af2542b62be0 100644 --- a/browser/ui/views/speedreader/speedreader_icon_view.cc +++ b/browser/ui/views/speedreader/speedreader_icon_view.cc @@ -124,11 +124,11 @@ void SpeedreaderIconView::OnExecuting( PageActionIconView::ExecuteSource execute_source) {} views::BubbleDialogDelegate* SpeedreaderIconView::GetBubble() const { - auto* web_contents = GetWebContents(); + const auto* web_contents = GetWebContents(); if (!web_contents) return nullptr; - auto* tab_helper = + const auto* tab_helper = speedreader::SpeedreaderTabHelper::FromWebContents(web_contents); if (!tab_helper) return nullptr; diff --git a/components/speedreader/speedreader_service.cc b/components/speedreader/speedreader_service.cc index a30be5b2c1cb..926cf2364366 100644 --- a/components/speedreader/speedreader_service.cc +++ b/components/speedreader/speedreader_service.cc @@ -68,7 +68,7 @@ void RecordHistograms(PrefService* prefs, bool toggled, bool enabled_now) { } else if (prefs->GetBoolean(kSpeedreaderPrefEverEnabled)) { status = EnabledStatus::kEverEnabled; } - + UMA_HISTOGRAM_ENUMERATION(kSpeedreaderEnabledUMAHistogramName, status); } @@ -76,7 +76,7 @@ void RecordHistograms(PrefService* prefs, bool toggled, bool enabled_now) { SpeedreaderService::SpeedreaderService(PrefService* prefs) : prefs_(prefs) {} -SpeedreaderService::~SpeedreaderService() {} +SpeedreaderService::~SpeedreaderService() = default; // static void SpeedreaderService::RegisterProfilePrefs(PrefRegistrySimple* registry) { @@ -87,12 +87,12 @@ void SpeedreaderService::RegisterProfilePrefs(PrefRegistrySimple* registry) { } void SpeedreaderService::ToggleSpeedreader() { - const bool enabled = prefs_->GetBoolean(kSpeedreaderPrefEnabled); - prefs_->SetBoolean(kSpeedreaderPrefEnabled, !enabled); - if (!enabled) + const bool enabled = !prefs_->GetBoolean(kSpeedreaderPrefEnabled); + prefs_->SetBoolean(kSpeedreaderPrefEnabled, enabled); + if (enabled) prefs_->SetBoolean(kSpeedreaderPrefEverEnabled, true); RecordHistograms(prefs_, true, - !enabled); // toggling - now enabled + enabled); // toggling - now enabled } void SpeedreaderService::DisableSpeedreaderForTest() { diff --git a/components/speedreader/speedreader_service.h b/components/speedreader/speedreader_service.h index 464dcd8efaac..c5602f02df46 100644 --- a/components/speedreader/speedreader_service.h +++ b/components/speedreader/speedreader_service.h @@ -6,8 +6,7 @@ #ifndef BRAVE_COMPONENTS_SPEEDREADER_SPEEDREADER_SERVICE_H_ #define BRAVE_COMPONENTS_SPEEDREADER_SPEEDREADER_SERVICE_H_ -#include - +#include "base/memory/raw_ptr.h" #include "components/keyed_service/core/keyed_service.h" class PrefRegistrySimple; @@ -32,7 +31,7 @@ class SpeedreaderService : public KeyedService { SpeedreaderService& operator=(const SpeedreaderService&) = delete; private: - PrefService* prefs_ = nullptr; + raw_ptr prefs_ = nullptr; }; } // namespace speedreader From 22c2050b036b26c201d052ac222362bc457eec45 Mon Sep 17 00:00:00 2001 From: boocmp Date: Thu, 17 Mar 2022 15:41:16 +0700 Subject: [PATCH 02/10] Tests was fixed & new were added. --- .../speedreader/speedreader_browsertest.cc | 119 ++++++++++++++++-- 1 file changed, 106 insertions(+), 13 deletions(-) diff --git a/browser/speedreader/speedreader_browsertest.cc b/browser/speedreader/speedreader_browsertest.cc index 58561daa4702..71e554a31c4d 100644 --- a/browser/speedreader/speedreader_browsertest.cc +++ b/browser/speedreader/speedreader_browsertest.cc @@ -5,17 +5,23 @@ #include "base/bind.h" #include "base/path_service.h" +#include "base/run_loop.h" #include "brave/app/brave_command_ids.h" #include "brave/browser/speedreader/speedreader_service_factory.h" #include "brave/browser/speedreader/speedreader_tab_helper.h" #include "brave/common/brave_paths.h" #include "brave/components/speedreader/features.h" #include "brave/components/speedreader/speedreader_service.h" +#include "brave/components/speedreader/speedreader_util.h" #include "chrome/browser/profiles/keep_alive/profile_keep_alive_types.h" #include "chrome/browser/profiles/keep_alive/scoped_profile_keep_alive.h" #include "chrome/browser/ui/browser.h" +#include "chrome/browser/ui/browser_command_controller.h" #include "chrome/browser/ui/browser_commands.h" #include "chrome/browser/ui/browser_list.h" +#include "chrome/browser/ui/views/frame/browser_view.h" +#include "chrome/browser/ui/views/frame/toolbar_button_provider.h" +#include "chrome/browser/ui/views/page_action/page_action_icon_view.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" @@ -76,10 +82,20 @@ class SpeedReaderBrowserTest : public InProcessBrowserTest { browser()->profile()); } + PageActionIconView* GetReaderButton() { + return BrowserView::GetBrowserViewForBrowser(browser()) + ->toolbar_button_provider() + ->GetPageActionIconView(PageActionIconType::kReaderMode); + } + + void ClickReaderButton() { + browser()->command_controller()->ExecuteCommand( + IDC_SPEEDREADER_ICON_ONCLICK); + content::WaitForLoadStop(ActiveWebContents()); + } + void ToggleSpeedreader() { speedreader_service()->ToggleSpeedreader(); - ActiveWebContents()->GetController().Reload(content::ReloadType::NORMAL, - false); } void DisableSpeedreader() { @@ -157,29 +173,33 @@ IN_PROC_BROWSER_TEST_F(SpeedReaderBrowserTest, DisableSiteWorks) { speedreader::PageStateIsDistilled(tab_helper()->PageDistillState())); } -// disabled in https://github.com/brave/brave-browser/issues/11328 -IN_PROC_BROWSER_TEST_F(SpeedReaderBrowserTest, DISABLED_SmokeTest) { +IN_PROC_BROWSER_TEST_F(SpeedReaderBrowserTest, SmokeTest) { ToggleSpeedreader(); const GURL url = https_server_.GetURL(kTestHost, kTestPageReadable); ASSERT_TRUE(ui_test_utils::NavigateToURL(browser(), url)); content::WebContents* contents = ActiveWebContents(); - content::RenderFrameHost* rfh = contents->GetMainFrame(); - const char kGetStyleLength[] = - "document.getElementById(\"brave_speedreader_style\").innerHTML.length"; + const std::string kGetStyleLength = + "document.getElementById('brave_speedreader_style').innerHTML.length"; - const char kGetContentLength[] = "document.body.innerHTML.length"; + const std::string kGetContentLength = "document.body.innerHTML.length"; + + const auto eval_js = [&](const std::string& script) { + int out; + EXPECT_TRUE(content::ExecuteScriptAndExtractInt( + contents, "window.domAutomationController.send(" + script + ")", &out)); + return out; + }; // Check that the document became much smaller and that non-empty speedreader // style is injected. - EXPECT_LT(0, content::EvalJs(rfh, kGetStyleLength)); - EXPECT_GT(17750 + 1, content::EvalJs(rfh, kGetContentLength)); + EXPECT_LT(0, eval_js(kGetStyleLength)); + EXPECT_GT(17750 + 1, eval_js(kGetContentLength)); // Check that disabled speedreader doesn't affect the page. ToggleSpeedreader(); ASSERT_TRUE(ui_test_utils::NavigateToURL(browser(), url)); - rfh = contents->GetMainFrame(); - EXPECT_LT(106000, content::EvalJs(rfh, kGetContentLength)); + EXPECT_LT(106000, eval_js(kGetContentLength)); } IN_PROC_BROWSER_TEST_F(SpeedReaderBrowserTest, P3ATest) { @@ -192,7 +212,80 @@ IN_PROC_BROWSER_TEST_F(SpeedReaderBrowserTest, P3ATest) { // SpeedReader recently enabled, toggled once ToggleSpeedreader(); - tester.ExpectBucketCount(kSpeedreaderEnabledUMAHistogramName, 2, 1); + tester.ExpectBucketCount(kSpeedreaderEnabledUMAHistogramName, 2, 2); tester.ExpectBucketCount(kSpeedreaderToggleUMAHistogramName, 1, 1); tester.ExpectBucketCount(kSpeedreaderToggleUMAHistogramName, 2, 0); } + +IN_PROC_BROWSER_TEST_F(SpeedReaderBrowserTest, ClickingOnReaderButton) { + EXPECT_FALSE(speedreader_service()->IsEnabled()); + + NavigateToPageSynchronously(kTestPageReadable); + EXPECT_TRUE(GetReaderButton()->GetVisible()); + + EXPECT_EQ(speedreader::DistillState::kPageProbablyReadable, + tab_helper()->PageDistillState()); + ClickReaderButton(); + EXPECT_TRUE(GetReaderButton()->GetVisible()); + EXPECT_EQ(speedreader::DistillState::kReaderMode, + tab_helper()->PageDistillState()); + EXPECT_TRUE(GetReaderButton()->GetVisible()); + + ClickReaderButton(); + EXPECT_TRUE(GetReaderButton()->GetVisible()); + EXPECT_EQ(speedreader::DistillState::kPageProbablyReadable, + tab_helper()->PageDistillState()); + + EXPECT_FALSE(speedreader_service()->IsEnabled()); +} + +IN_PROC_BROWSER_TEST_F(SpeedReaderBrowserTest, EnableDisableSpeedreader) { + EXPECT_FALSE(speedreader_service()->IsEnabled()); + NavigateToPageSynchronously(kTestPageReadable); + + EXPECT_TRUE(GetReaderButton()->GetVisible()); + EXPECT_EQ(speedreader::DistillState::kPageProbablyReadable, + tab_helper()->PageDistillState()); + ToggleSpeedreader(); + EXPECT_TRUE(GetReaderButton()->GetVisible()); + EXPECT_EQ(speedreader::DistillState::kSpeedreaderOnDisabledPage, + tab_helper()->PageDistillState()); + DisableSpeedreader(); + EXPECT_TRUE(GetReaderButton()->GetVisible()); + EXPECT_EQ(speedreader::DistillState::kPageProbablyReadable, + tab_helper()->PageDistillState()); + + ClickReaderButton(); + EXPECT_TRUE(GetReaderButton()->GetVisible()); + EXPECT_EQ(speedreader::DistillState::kReaderMode, + tab_helper()->PageDistillState()); + ToggleSpeedreader(); + EXPECT_TRUE(GetReaderButton()->GetVisible()); + EXPECT_EQ(speedreader::DistillState::kSpeedreaderMode, + tab_helper()->PageDistillState()); + DisableSpeedreader(); + EXPECT_TRUE(GetReaderButton()->GetVisible()); + EXPECT_EQ(speedreader::DistillState::kReaderMode, + tab_helper()->PageDistillState()); +} + +IN_PROC_BROWSER_TEST_F(SpeedReaderBrowserTest, TogglingSiteSpeedreader) { + ToggleSpeedreader(); + NavigateToPageSynchronously(kTestPageReadable); + + for (int i = 0; i < 2; ++i) { + EXPECT_TRUE(WaitForLoadStop(ActiveWebContents())); + EXPECT_EQ(speedreader::DistillState::kSpeedreaderMode, + tab_helper()->PageDistillState()); + EXPECT_TRUE(GetReaderButton()->GetVisible()); + + tab_helper()->MaybeToggleEnabledForSite(false); + EXPECT_TRUE(WaitForLoadStop(ActiveWebContents())); + EXPECT_EQ(speedreader::DistillState::kSpeedreaderOnDisabledPage, + tab_helper()->PageDistillState()); + EXPECT_TRUE(GetReaderButton()->GetVisible()); + + tab_helper()->MaybeToggleEnabledForSite(true); + EXPECT_TRUE(WaitForLoadStop(ActiveWebContents())); + } +} \ No newline at end of file From 4020ea0a5b28b722553c4096d9d21b914decefbe Mon Sep 17 00:00:00 2001 From: boocmp Date: Thu, 17 Mar 2022 19:22:57 +0700 Subject: [PATCH 03/10] No button if feature is disabled. --- browser/brave_tab_helpers.cc | 2 +- browser/speedreader/speedreader_tab_helper.cc | 10 +++++ browser/speedreader/speedreader_tab_helper.h | 2 + .../speedreader/speedreader_icon_view.cc | 41 ++++++++----------- components/speedreader/speedreader_util.cc | 9 ++++ components/speedreader/speedreader_util.h | 6 +++ 6 files changed, 44 insertions(+), 26 deletions(-) diff --git a/browser/brave_tab_helpers.cc b/browser/brave_tab_helpers.cc index 9b2dc529bc81..5861157befe7 100644 --- a/browser/brave_tab_helpers.cc +++ b/browser/brave_tab_helpers.cc @@ -110,7 +110,7 @@ void AttachTabHelpers(content::WebContents* web_contents) { #endif #if BUILDFLAG(ENABLE_SPEEDREADER) - speedreader::SpeedreaderTabHelper::CreateForWebContents(web_contents); + speedreader::SpeedreaderTabHelper::MaybeCreateForWebContents(web_contents); #endif #if BUILDFLAG(ENABLE_TOR) diff --git a/browser/speedreader/speedreader_tab_helper.cc b/browser/speedreader/speedreader_tab_helper.cc index f65122f998a4..d6c6027e307d 100644 --- a/browser/speedreader/speedreader_tab_helper.cc +++ b/browser/speedreader/speedreader_tab_helper.cc @@ -8,10 +8,12 @@ #include #include "base/bind.h" +#include "base/feature_list.h" #include "brave/browser/brave_browser_process.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/features.h" #include "brave/components/speedreader/speedreader_extended_info_handler.h" #include "brave/components/speedreader/speedreader_pref_names.h" #include "brave/components/speedreader/speedreader_rewriter_service.h" @@ -49,6 +51,14 @@ SpeedreaderTabHelper::~SpeedreaderTabHelper() { DCHECK(!content_rules_); } +// static +void SpeedreaderTabHelper::MaybeCreateForWebContents( + content::WebContents* contents) { + if (base::FeatureList::IsEnabled(speedreader::kSpeedreaderFeature)) { + SpeedreaderTabHelper::CreateForWebContents(contents); + } +} + base::WeakPtr SpeedreaderTabHelper::GetWeakPtr() { return weak_factory_.GetWeakPtr(); } diff --git a/browser/speedreader/speedreader_tab_helper.h b/browser/speedreader/speedreader_tab_helper.h index e81a808ec3f5..a6f6a549d6a2 100644 --- a/browser/speedreader/speedreader_tab_helper.h +++ b/browser/speedreader/speedreader_tab_helper.h @@ -35,6 +35,8 @@ class SpeedreaderTabHelper SpeedreaderTabHelper(const SpeedreaderTabHelper&) = delete; SpeedreaderTabHelper& operator=(SpeedreaderTabHelper&) = delete; + static void MaybeCreateForWebContents(content::WebContents* contents); + base::WeakPtr GetWeakPtr(); // Returns |true| if Speedreader is turned on for all sites. diff --git a/browser/ui/views/speedreader/speedreader_icon_view.cc b/browser/ui/views/speedreader/speedreader_icon_view.cc index af2542b62be0..0917e372de16 100644 --- a/browser/ui/views/speedreader/speedreader_icon_view.cc +++ b/browser/ui/views/speedreader/speedreader_icon_view.cc @@ -21,6 +21,7 @@ #include "chrome/browser/ui/views/location_bar/location_bar_bubble_delegate_view.h" #include "chrome/browser/ui/views/page_action/page_action_icon_view.h" #include "chrome/grit/generated_resources.h" +#include "include/core/SkColor.h" #include "ui/base/l10n/l10n_util.h" #include "ui/base/metadata/metadata_impl_macros.h" #include "ui/base/theme_provider.h" @@ -45,13 +46,8 @@ SpeedreaderIconView::SpeedreaderIconView( SpeedreaderIconView::~SpeedreaderIconView() = default; void SpeedreaderIconView::UpdateImpl() { - if (!base::FeatureList::IsEnabled(speedreader::kSpeedreaderFeature)) { - SetVisible(false); - return; - } - - auto* contents = GetWebContents(); - if (!contents || !contents->GetLastCommittedURL().SchemeIsHTTPOrHTTPS()) { + const DistillState state = GetDistillState(); + if (speedreader::PageDoesntSupportDistillation(state)) { SetVisible(false); return; } @@ -62,35 +58,30 @@ void SpeedreaderIconView::UpdateImpl() { } const ui::ThemeProvider* theme_provider = GetThemeProvider(); - const DistillState state = GetDistillState(); const bool is_distilled = speedreader::PageStateIsDistilled(state); - if (!is_distilled) { - if (state == DistillState::kSpeedreaderOnDisabledPage || - state == DistillState::kPageProbablyReadable) { - SetVisible(true); - } else { - SetVisible(false); + if (is_distilled) { + UpdateIconImage(); + if (theme_provider) { + const SkColor icon_color_active = theme_provider->GetColor( + BraveThemeProperties::COLOR_SPEEDREADER_ICON); + SetIconColor(icon_color_active); } - - if (GetVisible()) { + SetVisible(true); + } else { + if (speedreader::PageSupportsDistilation(state)) { // Reset the icon color if (theme_provider) { - SkColor icon_color_default = + const SkColor icon_color_default = GetOmniboxColor(theme_provider, OmniboxPart::RESULTS_ICON); SetIconColor(icon_color_default); } UpdateIconImage(); + SetVisible(true); + } else { + SetVisible(false); } } - - if (is_distilled) { - UpdateIconImage(); - if (theme_provider) - SetIconColor(theme_provider->GetColor( - BraveThemeProperties::COLOR_SPEEDREADER_ICON)); - SetVisible(true); - } } const gfx::VectorIcon& SpeedreaderIconView::GetVectorIcon() const { diff --git a/components/speedreader/speedreader_util.cc b/components/speedreader/speedreader_util.cc index 4297dbcd1394..c95ab710fd8b 100644 --- a/components/speedreader/speedreader_util.cc +++ b/components/speedreader/speedreader_util.cc @@ -54,6 +54,15 @@ bool URLReadableHintExtractor::HasHints(const GURL& url) { return false; } +bool PageDoesntSupportDistillation(DistillState state) { + return state == DistillState::kNone || state == DistillState::kUnknown; +} + +bool PageSupportsDistilation(DistillState state) { + return state == DistillState::kSpeedreaderOnDisabledPage || + state == DistillState::kPageProbablyReadable; +} + bool PageStateIsDistilled(DistillState state) { return state == DistillState::kReaderMode || state == DistillState::kSpeedreaderMode; diff --git a/components/speedreader/speedreader_util.h b/components/speedreader/speedreader_util.h index afaaa9d70ad2..9c889bf663fc 100644 --- a/components/speedreader/speedreader_util.h +++ b/components/speedreader/speedreader_util.h @@ -58,6 +58,12 @@ enum class DistillState { // Page is in reader mode or speedreader mode. bool PageStateIsDistilled(DistillState state); +// Page can't be distilled. +bool PageDoesntSupportDistillation(DistillState state); + +// Page can be destilled. +bool PageSupportsDistilation(DistillState state); + // Page is in reader mode, speedreader mode, or a pending state. bool PageWantsDistill(DistillState state); From 5e52915855cb1628f22a362076f621162c5cdf68 Mon Sep 17 00:00:00 2001 From: boocmp Date: Mon, 21 Mar 2022 13:21:34 +0700 Subject: [PATCH 04/10] LINT. --- browser/speedreader/speedreader_browsertest.cc | 4 +--- browser/speedreader/speedreader_tab_helper.cc | 14 +++++++------- browser/speedreader/speedreader_tab_helper.h | 4 ++-- components/speedreader/speedreader_service.cc | 2 +- 4 files changed, 11 insertions(+), 13 deletions(-) diff --git a/browser/speedreader/speedreader_browsertest.cc b/browser/speedreader/speedreader_browsertest.cc index 71e554a31c4d..378bbad4d68a 100644 --- a/browser/speedreader/speedreader_browsertest.cc +++ b/browser/speedreader/speedreader_browsertest.cc @@ -94,9 +94,7 @@ class SpeedReaderBrowserTest : public InProcessBrowserTest { content::WaitForLoadStop(ActiveWebContents()); } - void ToggleSpeedreader() { - speedreader_service()->ToggleSpeedreader(); - } + void ToggleSpeedreader() { speedreader_service()->ToggleSpeedreader(); } void DisableSpeedreader() { speedreader_service()->DisableSpeedreaderForTest(); diff --git a/browser/speedreader/speedreader_tab_helper.cc b/browser/speedreader/speedreader_tab_helper.cc index d6c6027e307d..5ab894a9d293 100644 --- a/browser/speedreader/speedreader_tab_helper.cc +++ b/browser/speedreader/speedreader_tab_helper.cc @@ -102,7 +102,7 @@ void SpeedreaderTabHelper::MaybeToggleEnabledForSite(bool on) { const bool enabled = speedreader::IsEnabledForSite( content_rules_, web_contents()->GetLastCommittedURL()); - if (enabled == on) + if (enabled == on) return; speedreader::SetEnabledForSite(content_rules_, @@ -163,8 +163,7 @@ void SpeedreaderTabHelper::UpdateActiveState(const GURL& url) { auto* rewriter_service = g_brave_browser_process->speedreader_rewriter_service(); if (rewriter_service->URLLooksReadable(url)) { - VLOG(2) << __func__ - << "URL passed speedreader heuristic: " << url; + VLOG(2) << __func__ << "URL passed speedreader heuristic: " << url; if (!IsSpeedreaderEnabled()) { SetNextRequestState(DistillState::kPageProbablyReadable); } else if (!IsEnabledForSite(url)) { @@ -197,7 +196,8 @@ void SpeedreaderTabHelper::ShowReaderModeBubble() { } Profile* SpeedreaderTabHelper::GetProfile() const { - auto *profile = Profile::FromBrowserContext(web_contents()->GetBrowserContext()); + auto* profile = + Profile::FromBrowserContext(web_contents()->GetBrowserContext()); DCHECK(profile); return profile; } @@ -245,12 +245,12 @@ void SpeedreaderTabHelper::OnPrefChanged() { switch (distill_state_) { case DistillState::kUnknown: case DistillState::kNone: - break; // Nothing to do. + break; // Nothing to do. case DistillState::kPageProbablyReadable: if (is_speedreader_enabled) { distill_state_ = DistillState::kSpeedreaderOnDisabledPage; } - break; + break; case DistillState::kSpeedreaderMode: if (!is_speedreader_enabled) { distill_state_ = DistillState::kReaderMode; @@ -270,7 +270,7 @@ void SpeedreaderTabHelper::OnPrefChanged() { break; } default: - break; + break; } UpdateButtonIfNeeded(); diff --git a/browser/speedreader/speedreader_tab_helper.h b/browser/speedreader/speedreader_tab_helper.h index a6f6a549d6a2..541c277ad642 100644 --- a/browser/speedreader/speedreader_tab_helper.h +++ b/browser/speedreader/speedreader_tab_helper.h @@ -111,11 +111,11 @@ class SpeedreaderTabHelper void WebContentsDestroyed() override; // SpeedreaderResultDelegate: - void OnDistillComplete() override; + void OnDistillComplete() override; std::unique_ptr pref_change_registrar_; - bool is_active_ = false; + bool is_active_ = false; bool single_shot_next_request_ = false; // run speedreader once on next page load diff --git a/components/speedreader/speedreader_service.cc b/components/speedreader/speedreader_service.cc index 926cf2364366..70df78367aac 100644 --- a/components/speedreader/speedreader_service.cc +++ b/components/speedreader/speedreader_service.cc @@ -68,7 +68,7 @@ void RecordHistograms(PrefService* prefs, bool toggled, bool enabled_now) { } else if (prefs->GetBoolean(kSpeedreaderPrefEverEnabled)) { status = EnabledStatus::kEverEnabled; } - + UMA_HISTOGRAM_ENUMERATION(kSpeedreaderEnabledUMAHistogramName, status); } From 87dff8016cfbe3da15f2e0ea9c04018372ede4c2 Mon Sep 17 00:00:00 2001 From: boocmp Date: Mon, 21 Mar 2022 13:32:34 +0700 Subject: [PATCH 05/10] Cleanup. --- browser/speedreader/speedreader_browsertest.cc | 2 +- browser/ui/views/speedreader/speedreader_icon_view.cc | 3 +-- components/speedreader/speedreader_util.cc | 2 +- components/speedreader/speedreader_util.h | 2 +- 4 files changed, 4 insertions(+), 5 deletions(-) diff --git a/browser/speedreader/speedreader_browsertest.cc b/browser/speedreader/speedreader_browsertest.cc index 378bbad4d68a..342695996043 100644 --- a/browser/speedreader/speedreader_browsertest.cc +++ b/browser/speedreader/speedreader_browsertest.cc @@ -286,4 +286,4 @@ IN_PROC_BROWSER_TEST_F(SpeedReaderBrowserTest, TogglingSiteSpeedreader) { tab_helper()->MaybeToggleEnabledForSite(true); EXPECT_TRUE(WaitForLoadStop(ActiveWebContents())); } -} \ No newline at end of file +} diff --git a/browser/ui/views/speedreader/speedreader_icon_view.cc b/browser/ui/views/speedreader/speedreader_icon_view.cc index 0917e372de16..3212fa9d347f 100644 --- a/browser/ui/views/speedreader/speedreader_icon_view.cc +++ b/browser/ui/views/speedreader/speedreader_icon_view.cc @@ -21,7 +21,6 @@ #include "chrome/browser/ui/views/location_bar/location_bar_bubble_delegate_view.h" #include "chrome/browser/ui/views/page_action/page_action_icon_view.h" #include "chrome/grit/generated_resources.h" -#include "include/core/SkColor.h" #include "ui/base/l10n/l10n_util.h" #include "ui/base/metadata/metadata_impl_macros.h" #include "ui/base/theme_provider.h" @@ -69,7 +68,7 @@ void SpeedreaderIconView::UpdateImpl() { } SetVisible(true); } else { - if (speedreader::PageSupportsDistilation(state)) { + if (speedreader::PageSupportsDistillation(state)) { // Reset the icon color if (theme_provider) { const SkColor icon_color_default = diff --git a/components/speedreader/speedreader_util.cc b/components/speedreader/speedreader_util.cc index c95ab710fd8b..12a82a8fceac 100644 --- a/components/speedreader/speedreader_util.cc +++ b/components/speedreader/speedreader_util.cc @@ -58,7 +58,7 @@ bool PageDoesntSupportDistillation(DistillState state) { return state == DistillState::kNone || state == DistillState::kUnknown; } -bool PageSupportsDistilation(DistillState state) { +bool PageSupportsDistillation(DistillState state) { return state == DistillState::kSpeedreaderOnDisabledPage || state == DistillState::kPageProbablyReadable; } diff --git a/components/speedreader/speedreader_util.h b/components/speedreader/speedreader_util.h index 9c889bf663fc..b71afc5049c2 100644 --- a/components/speedreader/speedreader_util.h +++ b/components/speedreader/speedreader_util.h @@ -62,7 +62,7 @@ bool PageStateIsDistilled(DistillState state); bool PageDoesntSupportDistillation(DistillState state); // Page can be destilled. -bool PageSupportsDistilation(DistillState state); +bool PageSupportsDistillation(DistillState state); // Page is in reader mode, speedreader mode, or a pending state. bool PageWantsDistill(DistillState state); From 8eff59a6ca5fd367d3573ef01a2dfcf063022b88 Mon Sep 17 00:00:00 2001 From: boocmp Date: Mon, 21 Mar 2022 17:52:02 +0700 Subject: [PATCH 06/10] Fixed speedreaderurl loader after refactoring. --- .../speedreader/speedreader_browsertest.cc | 10 ++++----- .../body_sniffer/body_sniffer_url_loader.h | 5 ++--- components/de_amp/browser/de_amp_url_loader.h | 1 + .../speedreader/speedreader_url_loader.cc | 21 ++++++++++++------- .../speedreader/speedreader_url_loader.h | 4 +++- 5 files changed, 23 insertions(+), 18 deletions(-) diff --git a/browser/speedreader/speedreader_browsertest.cc b/browser/speedreader/speedreader_browsertest.cc index 342695996043..269cb7b767f6 100644 --- a/browser/speedreader/speedreader_browsertest.cc +++ b/browser/speedreader/speedreader_browsertest.cc @@ -173,10 +173,7 @@ IN_PROC_BROWSER_TEST_F(SpeedReaderBrowserTest, DisableSiteWorks) { IN_PROC_BROWSER_TEST_F(SpeedReaderBrowserTest, SmokeTest) { ToggleSpeedreader(); - const GURL url = https_server_.GetURL(kTestHost, kTestPageReadable); - ASSERT_TRUE(ui_test_utils::NavigateToURL(browser(), url)); - content::WebContents* contents = ActiveWebContents(); - + NavigateToPageSynchronously(kTestPageReadable); const std::string kGetStyleLength = "document.getElementById('brave_speedreader_style').innerHTML.length"; @@ -185,7 +182,8 @@ IN_PROC_BROWSER_TEST_F(SpeedReaderBrowserTest, SmokeTest) { const auto eval_js = [&](const std::string& script) { int out; EXPECT_TRUE(content::ExecuteScriptAndExtractInt( - contents, "window.domAutomationController.send(" + script + ")", &out)); + ActiveWebContents(), + "window.domAutomationController.send(" + script + ")", &out)); return out; }; @@ -196,7 +194,7 @@ IN_PROC_BROWSER_TEST_F(SpeedReaderBrowserTest, SmokeTest) { // Check that disabled speedreader doesn't affect the page. ToggleSpeedreader(); - ASSERT_TRUE(ui_test_utils::NavigateToURL(browser(), url)); + NavigateToPageSynchronously(kTestPageReadable); EXPECT_LT(106000, eval_js(kGetContentLength)); } diff --git a/components/body_sniffer/body_sniffer_url_loader.h b/components/body_sniffer/body_sniffer_url_loader.h index aba1fac5f17c..b1146b8ce15b 100644 --- a/components/body_sniffer/body_sniffer_url_loader.h +++ b/components/body_sniffer/body_sniffer_url_loader.h @@ -95,7 +95,7 @@ class BodySnifferURLLoader : public network::mojom::URLLoaderClient, virtual void OnBodyReadable(MojoResult) = 0; virtual void OnBodyWritable(MojoResult) = 0; - void CompleteLoading(std::string body); + virtual void CompleteLoading(std::string body); void CompleteSending(); virtual void OnCompleteSending(); void SendReceivedBodyToClient(); @@ -125,10 +125,9 @@ class BodySnifferURLLoader : public network::mojom::URLLoaderClient, mojo::SimpleWatcher body_consumer_watcher_; mojo::SimpleWatcher body_producer_watcher_; - base::WeakPtrFactory weak_factory_{this}; - private: void CancelAndResetHandles(); + base::WeakPtrFactory weak_factory_{this}; }; } // namespace body_sniffer diff --git a/components/de_amp/browser/de_amp_url_loader.h b/components/de_amp/browser/de_amp_url_loader.h index 7abb063e4902..f390ad6c82dc 100644 --- a/components/de_amp/browser/de_amp_url_loader.h +++ b/components/de_amp/browser/de_amp_url_loader.h @@ -51,6 +51,7 @@ class DeAmpURLLoader : public body_sniffer::BodySnifferURLLoader { void ForwardBodyToClient(); base::WeakPtr de_amp_throttle_; + base::WeakPtrFactory weak_factory_{this}; }; } // namespace de_amp diff --git a/components/speedreader/speedreader_url_loader.cc b/components/speedreader/speedreader_url_loader.cc index 61929af48f51..8b8fdf2b8364 100644 --- a/components/speedreader/speedreader_url_loader.cc +++ b/components/speedreader/speedreader_url_loader.cc @@ -11,6 +11,7 @@ #include "base/bind.h" #include "base/check.h" +#include "base/memory/weak_ptr.h" #include "base/metrics/histogram_macros.h" #include "base/task/task_traits.h" #include "base/task/thread_pool.h" @@ -97,15 +98,15 @@ void SpeedReaderURLLoader::OnBodyWritable(MojoResult r) { } } -void SpeedReaderURLLoader::MaybeLaunchSpeedreader() { +void SpeedReaderURLLoader::CompleteLoading(std::string body) { DCHECK_EQ(State::kLoading, state_); if (!throttle_ || !rewriter_service_) { Abort(); return; } - VLOG(2) << __func__ << " buffered body size = " << buffered_body_.size(); - bytes_remaining_in_buffer_ = buffered_body_.size(); + VLOG(2) << __func__ << " buffered body size = " << body.size(); + bytes_remaining_in_buffer_ = body.size(); if (bytes_remaining_in_buffer_ > 0) { // Offload heavy distilling to another thread. @@ -133,14 +134,18 @@ void SpeedReaderURLLoader::MaybeLaunchSpeedreader() { return stylesheet + transformed; }, - std::move(buffered_body_), - rewriter_service_->MakeRewriter(response_url_), + std::move(body), rewriter_service_->MakeRewriter(response_url_), rewriter_service_->GetContentStylesheet()), - base::BindOnce(&SpeedReaderURLLoader::CompleteLoading, - weak_factory_.GetWeakPtr())); + base::BindOnce( + [](base::WeakPtr self, std::string result) { + if (self) { + self->BodySnifferURLLoader::CompleteLoading(result); + } + }, + weak_factory_.GetWeakPtr())); return; } - CompleteLoading(std::move(buffered_body_)); + BodySnifferURLLoader::CompleteLoading(std::move(body)); } void SpeedReaderURLLoader::OnCompleteSending() { diff --git a/components/speedreader/speedreader_url_loader.h b/components/speedreader/speedreader_url_loader.h index e31acac7aa71..9154949319dd 100644 --- a/components/speedreader/speedreader_url_loader.h +++ b/components/speedreader/speedreader_url_loader.h @@ -78,13 +78,15 @@ class SpeedReaderURLLoader : public body_sniffer::BodySnifferURLLoader { void OnBodyReadable(MojoResult) override; void OnBodyWritable(MojoResult) override; - void MaybeLaunchSpeedreader(); + void CompleteLoading(std::string body) override; void OnCompleteSending() override; base::WeakPtr delegate_; // Not Owned raw_ptr rewriter_service_ = nullptr; + + base::WeakPtrFactory weak_factory_{this}; }; } // namespace speedreader From 89ee6c7955f20fa27c7c82d6683927743d7734eb Mon Sep 17 00:00:00 2001 From: boocmp Date: Mon, 21 Mar 2022 18:11:59 +0700 Subject: [PATCH 07/10] Review issues are addressed. --- browser/speedreader/speedreader_tab_helper.cc | 1 + components/speedreader/speedreader_service.cc | 15 ++++++++------- components/speedreader/speedreader_url_loader.h | 3 ++- 3 files changed, 11 insertions(+), 8 deletions(-) diff --git a/browser/speedreader/speedreader_tab_helper.cc b/browser/speedreader/speedreader_tab_helper.cc index 5ab894a9d293..2c74af536f99 100644 --- a/browser/speedreader/speedreader_tab_helper.cc +++ b/browser/speedreader/speedreader_tab_helper.cc @@ -231,6 +231,7 @@ void SpeedreaderTabHelper::ReloadContents() { void SpeedreaderTabHelper::ProcessNavigation( content::NavigationHandle* navigation_handle) { if (!navigation_handle->IsInMainFrame() || + navigation_handle->IsSameDocument() || MaybeUpdateCachedState(navigation_handle)) { return; } diff --git a/components/speedreader/speedreader_service.cc b/components/speedreader/speedreader_service.cc index 70df78367aac..95b0dcfa3023 100644 --- a/components/speedreader/speedreader_service.cc +++ b/components/speedreader/speedreader_service.cc @@ -87,12 +87,11 @@ void SpeedreaderService::RegisterProfilePrefs(PrefRegistrySimple* registry) { } void SpeedreaderService::ToggleSpeedreader() { - const bool enabled = !prefs_->GetBoolean(kSpeedreaderPrefEnabled); - prefs_->SetBoolean(kSpeedreaderPrefEnabled, enabled); - if (enabled) + const bool toggled_value = !prefs_->GetBoolean(kSpeedreaderPrefEnabled); + prefs_->SetBoolean(kSpeedreaderPrefEnabled, toggled_value); + if (toggled_value) prefs_->SetBoolean(kSpeedreaderPrefEverEnabled, true); - RecordHistograms(prefs_, true, - enabled); // toggling - now enabled + RecordHistograms(prefs_, true, toggled_value); } void SpeedreaderService::DisableSpeedreaderForTest() { @@ -115,13 +114,15 @@ bool SpeedreaderService::ShouldPromptUserToEnable() const { constexpr int max_speedreader_prompts = 2; if (prefs_->GetBoolean(kSpeedreaderPrefEverEnabled) || - prefs_->GetInteger(kSpeedreaderPrefPromptCount) > max_speedreader_prompts) + prefs_->GetInteger(kSpeedreaderPrefPromptCount) > + max_speedreader_prompts) { return false; + } return true; } void SpeedreaderService::IncrementPromptCount() { - int count = prefs_->GetInteger(kSpeedreaderPrefPromptCount); + const int count = prefs_->GetInteger(kSpeedreaderPrefPromptCount); prefs_->SetInteger(kSpeedreaderPrefPromptCount, count + 1); } diff --git a/components/speedreader/speedreader_url_loader.h b/components/speedreader/speedreader_url_loader.h index 9154949319dd..669110ede0a0 100644 --- a/components/speedreader/speedreader_url_loader.h +++ b/components/speedreader/speedreader_url_loader.h @@ -6,6 +6,7 @@ #ifndef BRAVE_COMPONENTS_SPEEDREADER_SPEEDREADER_URL_LOADER_H_ #define BRAVE_COMPONENTS_SPEEDREADER_SPEEDREADER_URL_LOADER_H_ +#include #include #include "base/memory/raw_ptr.h" @@ -79,7 +80,7 @@ class SpeedReaderURLLoader : public body_sniffer::BodySnifferURLLoader { void OnBodyReadable(MojoResult) override; void OnBodyWritable(MojoResult) override; - void CompleteLoading(std::string body) override; + void CompleteLoading(std::string body) override; void OnCompleteSending() override; base::WeakPtr delegate_; From 891c901161fa381e1ab5be428790b8940997b6d9 Mon Sep 17 00:00:00 2001 From: boocmp Date: Mon, 21 Mar 2022 21:00:16 +0700 Subject: [PATCH 08/10] Missed std::move. --- browser/speedreader/speedreader_tab_helper.h | 1 + components/body_sniffer/body_sniffer_url_loader.h | 1 + components/speedreader/speedreader_url_loader.cc | 2 +- 3 files changed, 3 insertions(+), 1 deletion(-) diff --git a/browser/speedreader/speedreader_tab_helper.h b/browser/speedreader/speedreader_tab_helper.h index 541c277ad642..751cbecff3bf 100644 --- a/browser/speedreader/speedreader_tab_helper.h +++ b/browser/speedreader/speedreader_tab_helper.h @@ -20,6 +20,7 @@ class NavigationEntry; class NavigationHandle; class WebContents; } // namespace content + namespace speedreader { class SpeedreaderBubbleView; diff --git a/components/body_sniffer/body_sniffer_url_loader.h b/components/body_sniffer/body_sniffer_url_loader.h index b1146b8ce15b..fcf7311fa38d 100644 --- a/components/body_sniffer/body_sniffer_url_loader.h +++ b/components/body_sniffer/body_sniffer_url_loader.h @@ -127,6 +127,7 @@ class BodySnifferURLLoader : public network::mojom::URLLoaderClient, private: void CancelAndResetHandles(); + base::WeakPtrFactory weak_factory_{this}; }; diff --git a/components/speedreader/speedreader_url_loader.cc b/components/speedreader/speedreader_url_loader.cc index 8b8fdf2b8364..ef3679824ea3 100644 --- a/components/speedreader/speedreader_url_loader.cc +++ b/components/speedreader/speedreader_url_loader.cc @@ -139,7 +139,7 @@ void SpeedReaderURLLoader::CompleteLoading(std::string body) { base::BindOnce( [](base::WeakPtr self, std::string result) { if (self) { - self->BodySnifferURLLoader::CompleteLoading(result); + self->BodySnifferURLLoader::CompleteLoading(std::move(result)); } }, weak_factory_.GetWeakPtr())); From 6a266e94591abee07ce03885601b25d8a360cf67 Mon Sep 17 00:00:00 2001 From: boocmp Date: Tue, 22 Mar 2022 21:40:14 +0700 Subject: [PATCH 09/10] Review issues are addressed. --- browser/speedreader/speedreader_tab_helper.cc | 12 +++---- browser/speedreader/speedreader_tab_helper.h | 11 +++++-- browser/ui/browser_commands.cc | 3 +- .../speedreader/speedreader_icon_view.cc | 31 +++++++------------ components/de_amp/browser/de_amp_url_loader.h | 1 - components/speedreader/speedreader_util.cc | 4 --- components/speedreader/speedreader_util.h | 5 +-- 7 files changed, 27 insertions(+), 40 deletions(-) diff --git a/browser/speedreader/speedreader_tab_helper.cc b/browser/speedreader/speedreader_tab_helper.cc index 2c74af536f99..5493f81447b7 100644 --- a/browser/speedreader/speedreader_tab_helper.cc +++ b/browser/speedreader/speedreader_tab_helper.cc @@ -77,7 +77,7 @@ bool SpeedreaderTabHelper::IsEnabledForSite(const GURL& url) { return speedreader::IsEnabledForSite(content_rules_, url); } -void SpeedreaderTabHelper::Distill() { +void SpeedreaderTabHelper::ProcessIconClick() { switch (distill_state_) { case DistillState::kSpeedreaderMode: case DistillState::kSpeedreaderOnDisabledPage: @@ -85,7 +85,7 @@ void SpeedreaderTabHelper::Distill() { break; case DistillState::kReaderMode: SetNextRequestState(DistillState::kPageProbablyReadable); - ClearCache(); + ClearPersistedData(); ReloadContents(); break; case DistillState::kPageProbablyReadable: @@ -107,7 +107,7 @@ void SpeedreaderTabHelper::MaybeToggleEnabledForSite(bool on) { speedreader::SetEnabledForSite(content_rules_, web_contents()->GetLastCommittedURL(), on); - ClearCache(); + ClearPersistedData(); ReloadContents(); } @@ -218,7 +218,7 @@ void SpeedreaderTabHelper::HideBubble() { } } -void SpeedreaderTabHelper::ClearCache() { +void SpeedreaderTabHelper::ClearPersistedData() { if (auto* entry = web_contents()->GetController().GetLastCommittedEntry()) { SpeedreaderExtendedInfoHandler::ClearPersistedData(entry); } @@ -278,7 +278,7 @@ void SpeedreaderTabHelper::OnPrefChanged() { } void SpeedreaderTabHelper::UpdateButtonIfNeeded() { - if (!is_active_) + if (!is_visible_) return; if (const auto* browser = chrome::FindBrowserWithWebContents(web_contents())) { @@ -304,7 +304,7 @@ void SpeedreaderTabHelper::DidStopLoading() { } void SpeedreaderTabHelper::OnVisibilityChanged(content::Visibility visibility) { - is_active_ = visibility != content::Visibility::HIDDEN; + is_visible_ = visibility != content::Visibility::HIDDEN; } void SpeedreaderTabHelper::WebContentsDestroyed() { diff --git a/browser/speedreader/speedreader_tab_helper.h b/browser/speedreader/speedreader_tab_helper.h index 751cbecff3bf..e00686fa5282 100644 --- a/browser/speedreader/speedreader_tab_helper.h +++ b/browser/speedreader/speedreader_tab_helper.h @@ -47,7 +47,8 @@ class SpeedreaderTabHelper // active web contents is blacklisted. bool IsEnabledForSite(); - void Distill(); + // In Speedreader mode shows bubble. In Reader mode toggles state. + void ProcessIconClick(); DistillState PageDistillState() const { return distill_state_; } @@ -94,12 +95,16 @@ class SpeedreaderTabHelper void UpdateActiveState(const GURL& url); void SetNextRequestState(DistillState state); - void ClearCache(); + void ClearPersistedData(); void ReloadContents(); + + // Applies the distill state & updates UI for the navigation. void ProcessNavigation(content::NavigationHandle* navigation_handle); + // Updates the distill state when the global speedreader state is changed. void OnPrefChanged(); + // Updates UI if the tab is visible. void UpdateButtonIfNeeded(); // content::WebContentsObserver: @@ -116,7 +121,7 @@ class SpeedreaderTabHelper std::unique_ptr pref_change_registrar_; - bool is_active_ = false; + bool is_visible_ = false; bool single_shot_next_request_ = false; // run speedreader once on next page load diff --git a/browser/ui/browser_commands.cc b/browser/ui/browser_commands.cc index 5fbd693d7d65..94d6ecb8071d 100644 --- a/browser/ui/browser_commands.cc +++ b/browser/ui/browser_commands.cc @@ -104,9 +104,8 @@ void MaybeDistillAndShowSpeedreaderBubble(Browser* browser) { return; if (auto* tab_helper = speedreader::SpeedreaderTabHelper::FromWebContents(contents)) { - tab_helper->Distill(); + tab_helper->ProcessIconClick(); } - #endif // BUILDFLAG(ENABLE_SPEEDREADER) } diff --git a/browser/ui/views/speedreader/speedreader_icon_view.cc b/browser/ui/views/speedreader/speedreader_icon_view.cc index 3212fa9d347f..3c669f98c22d 100644 --- a/browser/ui/views/speedreader/speedreader_icon_view.cc +++ b/browser/ui/views/speedreader/speedreader_icon_view.cc @@ -46,7 +46,8 @@ SpeedreaderIconView::~SpeedreaderIconView() = default; void SpeedreaderIconView::UpdateImpl() { const DistillState state = GetDistillState(); - if (speedreader::PageDoesntSupportDistillation(state)) { + if (!speedreader::PageStateIsDistilled(state) && + !speedreader::PageSupportsDistillation(state)) { SetVisible(false); return; } @@ -56,31 +57,21 @@ void SpeedreaderIconView::UpdateImpl() { nullptr); } - const ui::ThemeProvider* theme_provider = GetThemeProvider(); - const bool is_distilled = speedreader::PageStateIsDistilled(state); - - if (is_distilled) { - UpdateIconImage(); - if (theme_provider) { + if (const ui::ThemeProvider* theme_provider = GetThemeProvider()) { + if (speedreader::PageStateIsDistilled(state)) { const SkColor icon_color_active = theme_provider->GetColor( BraveThemeProperties::COLOR_SPEEDREADER_ICON); SetIconColor(icon_color_active); - } - SetVisible(true); - } else { - if (speedreader::PageSupportsDistillation(state)) { + } else if (speedreader::PageSupportsDistillation(state)) { // Reset the icon color - if (theme_provider) { - const SkColor icon_color_default = - GetOmniboxColor(theme_provider, OmniboxPart::RESULTS_ICON); - SetIconColor(icon_color_default); - } - UpdateIconImage(); - SetVisible(true); - } else { - SetVisible(false); + const SkColor icon_color_default = + GetOmniboxColor(theme_provider, OmniboxPart::RESULTS_ICON); + SetIconColor(icon_color_default); } } + + UpdateIconImage(); + SetVisible(true); } const gfx::VectorIcon& SpeedreaderIconView::GetVectorIcon() const { diff --git a/components/de_amp/browser/de_amp_url_loader.h b/components/de_amp/browser/de_amp_url_loader.h index f390ad6c82dc..7abb063e4902 100644 --- a/components/de_amp/browser/de_amp_url_loader.h +++ b/components/de_amp/browser/de_amp_url_loader.h @@ -51,7 +51,6 @@ class DeAmpURLLoader : public body_sniffer::BodySnifferURLLoader { void ForwardBodyToClient(); base::WeakPtr de_amp_throttle_; - base::WeakPtrFactory weak_factory_{this}; }; } // namespace de_amp diff --git a/components/speedreader/speedreader_util.cc b/components/speedreader/speedreader_util.cc index 12a82a8fceac..56d7da77fdd0 100644 --- a/components/speedreader/speedreader_util.cc +++ b/components/speedreader/speedreader_util.cc @@ -54,10 +54,6 @@ bool URLReadableHintExtractor::HasHints(const GURL& url) { return false; } -bool PageDoesntSupportDistillation(DistillState state) { - return state == DistillState::kNone || state == DistillState::kUnknown; -} - bool PageSupportsDistillation(DistillState state) { return state == DistillState::kSpeedreaderOnDisabledPage || state == DistillState::kPageProbablyReadable; diff --git a/components/speedreader/speedreader_util.h b/components/speedreader/speedreader_util.h index b71afc5049c2..b1bef66fa0b2 100644 --- a/components/speedreader/speedreader_util.h +++ b/components/speedreader/speedreader_util.h @@ -58,10 +58,7 @@ enum class DistillState { // Page is in reader mode or speedreader mode. bool PageStateIsDistilled(DistillState state); -// Page can't be distilled. -bool PageDoesntSupportDistillation(DistillState state); - -// Page can be destilled. +// Page can be distilled. bool PageSupportsDistillation(DistillState state); // Page is in reader mode, speedreader mode, or a pending state. From 24b495a61e9dcd8e320c93f3013a3fd92bb35d41 Mon Sep 17 00:00:00 2001 From: boocmp Date: Wed, 23 Mar 2022 12:42:38 +0700 Subject: [PATCH 10/10] Fixed speedreader state after content reloading. --- .../speedreader/speedreader_browsertest.cc | 34 +++++++++++++++++++ browser/speedreader/speedreader_tab_helper.cc | 9 +++-- 2 files changed, 38 insertions(+), 5 deletions(-) diff --git a/browser/speedreader/speedreader_browsertest.cc b/browser/speedreader/speedreader_browsertest.cc index 269cb7b767f6..31244043df2c 100644 --- a/browser/speedreader/speedreader_browsertest.cc +++ b/browser/speedreader/speedreader_browsertest.cc @@ -27,6 +27,7 @@ #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/reload_type.h" #include "content/public/test/browser_test.h" #include "content/public/test/browser_test_utils.h" #include "content/public/test/test_navigation_observer.h" @@ -285,3 +286,36 @@ IN_PROC_BROWSER_TEST_F(SpeedReaderBrowserTest, TogglingSiteSpeedreader) { EXPECT_TRUE(WaitForLoadStop(ActiveWebContents())); } } + +IN_PROC_BROWSER_TEST_F(SpeedReaderBrowserTest, ReloadContent) { + ToggleSpeedreader(); + NavigateToPageSynchronously(kTestPageReadable); + auto* contents_1 = ActiveWebContents(); + NavigateToPageSynchronously(kTestPageReadable); + auto* contents_2 = ActiveWebContents(); + + auto* tab_helper_1 = + speedreader::SpeedreaderTabHelper::FromWebContents(contents_1); + auto* tab_helper_2 = + speedreader::SpeedreaderTabHelper::FromWebContents(contents_2); + + EXPECT_EQ(speedreader::DistillState::kSpeedreaderMode, + tab_helper_1->PageDistillState()); + EXPECT_EQ(speedreader::DistillState::kSpeedreaderMode, + tab_helper_2->PageDistillState()); + + tab_helper_1->MaybeToggleEnabledForSite(false); + content::WaitForLoadStop(contents_1); + EXPECT_EQ(speedreader::DistillState::kSpeedreaderOnDisabledPage, + tab_helper_1->PageDistillState()); + EXPECT_EQ(speedreader::DistillState::kSpeedreaderMode, + tab_helper_2->PageDistillState()); + + contents_2->GetController().Reload(content::ReloadType::NORMAL, false); + content::WaitForLoadStop(contents_2); + + EXPECT_EQ(speedreader::DistillState::kSpeedreaderOnDisabledPage, + tab_helper_1->PageDistillState()); + EXPECT_EQ(speedreader::DistillState::kSpeedreaderOnDisabledPage, + tab_helper_2->PageDistillState()); +} diff --git a/browser/speedreader/speedreader_tab_helper.cc b/browser/speedreader/speedreader_tab_helper.cc index 5493f81447b7..2fb0bccc3256 100644 --- a/browser/speedreader/speedreader_tab_helper.cc +++ b/browser/speedreader/speedreader_tab_helper.cc @@ -28,6 +28,7 @@ #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/reload_type.h" #include "content/public/browser/web_contents.h" namespace speedreader { @@ -133,13 +134,11 @@ SpeedreaderBubbleView* SpeedreaderTabHelper::speedreader_bubble_view() const { bool SpeedreaderTabHelper::MaybeUpdateCachedState( content::NavigationHandle* handle) { auto* entry = handle->GetNavigationEntry(); - if (!entry) { + if (!entry || handle->GetRestoreType() != content::RestoreType::kRestored) { return false; } - Profile* profile = - Profile::FromBrowserContext(web_contents()->GetBrowserContext()); - DCHECK(profile); - auto* speedreader_service = SpeedreaderServiceFactory::GetForProfile(profile); + auto* speedreader_service = + SpeedreaderServiceFactory::GetForProfile(GetProfile()); const DistillState state = SpeedreaderExtendedInfoHandler::GetCachedMode(entry, speedreader_service);