Speedreader result explicit api

browser/speedreader/speedreader_browsertest.cc

@@ -17,6 +17,10 @@
 #include "chrome/browser/ui/browser.h"
 #include "chrome/browser/ui/browser_commands.h"
 #include "chrome/browser/ui/browser_list.h"
+#include "chrome/browser/ui/page_action/page_action_icon_type.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"
@@ -30,8 +34,19 @@
 #include "net/test/embedded_test_server/embedded_test_server.h"
 
 const char kTestHost[] = "theguardian.com";
+
+// The path is not readable. None of the URL components match the heuristic so
+// the SpeedreaderThrottle is never created.
 const char kTestPageSimple[] = "/simple.html";
+
+// The URL path appears readable and the page has text content. This is used to
+// test distillation in the common case.
 const char kTestPageReadable[] = "/articles/guardian.html";
+
+// The URL path appears readable but the page has no text content. This is used
+// to test the case where Speedreader aborts.
+const char kTestPageFakeReadable[] = "/articles/fake.html";
+
 const base::FilePath::StringPieceType kTestWhitelist =
     FILE_PATH_LITERAL("speedreader_whitelist.json");
 
@@ -116,6 +131,12 @@ class SpeedReaderBrowserTest : public InProcessBrowserTest {
         ui_test_utils::BROWSER_TEST_WAIT_FOR_LOAD_STOP);
   }
 
+  PageActionIconView* SpeedreaderIcon() {
+    return BrowserView::GetBrowserViewForBrowser(browser())
+        ->toolbar_button_provider()
+        ->GetPageActionIconView(PageActionIconType::kReaderMode);
+  }
+
  protected:
   base::test::ScopedFeatureList feature_list_;
   net::EmbeddedTestServer https_server_;
@@ -126,6 +147,7 @@ IN_PROC_BROWSER_TEST_F(SpeedReaderBrowserTest, RestoreSpeedreaderPage) {
   NavigateToPageSynchronously(kTestPageReadable);
   EXPECT_TRUE(
       speedreader::PageStateIsDistilled(tab_helper()->PageDistillState()));
+  EXPECT_TRUE(SpeedreaderIcon()->GetVisible());
 
   Profile* profile = browser()->profile();
 
@@ -139,36 +161,55 @@ IN_PROC_BROWSER_TEST_F(SpeedReaderBrowserTest, RestoreSpeedreaderPage) {
   chrome::OpenWindowWithRestoredTabs(profile);
   EXPECT_EQ(1u, BrowserList::GetInstance()->size());
   SelectFirstBrowser();
+  content::TestNavigationObserver restore_observer(ActiveWebContents());
+  restore_observer.Wait();
   EXPECT_TRUE(
       speedreader::PageStateIsDistilled(tab_helper()->PageDistillState()));
+  EXPECT_TRUE(SpeedreaderIcon()->GetVisible());
 }
 
 IN_PROC_BROWSER_TEST_F(SpeedReaderBrowserTest, NavigationNostickTest) {
   ToggleSpeedreader();
   NavigateToPageSynchronously(kTestPageSimple);
   EXPECT_FALSE(
       speedreader::PageStateIsDistilled(tab_helper()->PageDistillState()));
+  EXPECT_FALSE(SpeedreaderIcon()->GetVisible());
+
   NavigateToPageSynchronously(kTestPageReadable,
                               WindowOpenDisposition::CURRENT_TAB);
   EXPECT_TRUE(
       speedreader::PageStateIsDistilled(tab_helper()->PageDistillState()));
+  EXPECT_TRUE(SpeedreaderIcon()->GetVisible());
 
   // 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()));
+  EXPECT_FALSE(SpeedreaderIcon()->GetVisible());
 }
 
 IN_PROC_BROWSER_TEST_F(SpeedReaderBrowserTest, DisableSiteWorks) {
   ToggleSpeedreader();
   NavigateToPageSynchronously(kTestPageReadable);
   EXPECT_TRUE(
       speedreader::PageStateIsDistilled(tab_helper()->PageDistillState()));
+  EXPECT_TRUE(SpeedreaderIcon()->GetVisible());
+
   tab_helper()->MaybeToggleEnabledForSite(false);
   EXPECT_TRUE(WaitForLoadStop(ActiveWebContents()));
   EXPECT_FALSE(
       speedreader::PageStateIsDistilled(tab_helper()->PageDistillState()));
+  // Disabled sites show the icon
+  EXPECT_TRUE(SpeedreaderIcon()->GetVisible());
+}
+
+IN_PROC_BROWSER_TEST_F(SpeedReaderBrowserTest, TestFakeReadable) {
+  ToggleSpeedreader();
+  NavigateToPageSynchronously(kTestPageFakeReadable);
+  EXPECT_FALSE(
+      speedreader::PageStateIsDistilled(tab_helper()->PageDistillState()));
+  EXPECT_FALSE(SpeedreaderIcon()->GetVisible());
 }
 
 // disabled in https://github.com/brave/brave-browser/issues/11328

browser/speedreader/speedreader_tab_helper.cc

@@ -96,30 +96,6 @@ void SpeedreaderTabHelper::SingleShotSpeedreader() {
   }
 }
 
-bool SpeedreaderTabHelper::MaybeUpdateCachedState(
-    content::NavigationHandle* handle) {
-  auto* entry = handle->GetNavigationEntry();
-  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, speedreader_service);
-  if (state != DistillState::kUnknown) {
-    cached = true;
-    distill_state_ = state;
-  }
-  if (!cached) {
-    SpeedreaderExtendedInfoHandler::ClearPersistedData(entry);
-  }
-  return cached;
-}
-
 void SpeedreaderTabHelper::UpdateActiveState(
     content::NavigationHandle* handle) {
   DCHECK(handle);
@@ -159,18 +135,14 @@ void SpeedreaderTabHelper::SetNextRequestState(DistillState state) {
 void SpeedreaderTabHelper::DidStartNavigation(
     content::NavigationHandle* navigation_handle) {
   if (navigation_handle->IsInMainFrame()) {
-    if (!MaybeUpdateCachedState(navigation_handle)) {
-      UpdateActiveState(navigation_handle);
-    }
+    UpdateActiveState(navigation_handle);
   }
 }
 
 void SpeedreaderTabHelper::DidRedirectNavigation(
     content::NavigationHandle* navigation_handle) {
   if (navigation_handle->IsInMainFrame()) {
-    if (!MaybeUpdateCachedState(navigation_handle)) {
-      UpdateActiveState(navigation_handle);
-    }
+    UpdateActiveState(navigation_handle);
   }
 }
 
@@ -212,16 +184,29 @@ void SpeedreaderTabHelper::HideBubble() {
   }
 }
 
-void SpeedreaderTabHelper::OnDistillComplete() {
-  // Perform a state transition
-  if (distill_state_ == DistillState::kSpeedreaderModePending) {
-    distill_state_ = DistillState::kSpeedreaderMode;
-  } else if (distill_state_ == DistillState::kReaderModePending) {
-    distill_state_ = DistillState::kReaderMode;
-  } else {
-    // We got here via an already cached page.
-    DCHECK(distill_state_ == DistillState::kSpeedreaderMode ||
-           distill_state_ == DistillState::kReaderMode);
+void SpeedreaderTabHelper::OnDistillComplete(DistillStatus status) {
+  if (status == DistillStatus::FAIL) {
+    distill_state_ = DistillState::kNone;
+    return;
+  }
+
+  if (status == DistillStatus::CHECK_CACHE) {
+    auto* entry = web_contents()->GetController().GetLastCommittedEntry();
+    if (entry && SpeedreaderExtendedInfoHandler::IsCached(entry)) {
+      status = DistillStatus::SUCCESS;
+    }
+  }
+
+  if (status == DistillStatus::SUCCESS) {
+    // Perform a state transition
+    if (distill_state_ == DistillState::kSpeedreaderModePending) {
+      distill_state_ = DistillState::kSpeedreaderMode;
+    } else if (distill_state_ == DistillState::kReaderModePending) {
+      distill_state_ = DistillState::kReaderMode;
+    } else {
+      DCHECK(distill_state_ == DistillState::kSpeedreaderMode ||
+             distill_state_ == DistillState::kReaderMode);
+    }
   }
 }
 

browser/speedreader/speedreader_tab_helper.h

@@ -81,7 +81,6 @@ 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);
 
@@ -93,7 +92,7 @@ class SpeedreaderTabHelper
   void DidStopLoading() override;
 
   // SpeedreaderResultDelegate:
-  void OnDistillComplete() override;
+  void OnDistillComplete(DistillStatus status) override;
 
   bool single_shot_next_request_ =
       false;  // run speedreader once on next page load

browser/ui/views/speedreader/speedreader_icon_view.cc

@@ -91,6 +91,9 @@ void SpeedreaderIconView::UpdateImpl() {
           BraveThemeProperties::COLOR_SPEEDREADER_ICON));
     SetVisible(true);
   }
+
+  // Notify the icon to check distillation status again on DidStopLoading()
+  Observe(contents);
 }
 
 const gfx::VectorIcon& SpeedreaderIconView::GetVectorIcon() const {
@@ -149,5 +152,9 @@ DistillState SpeedreaderIconView::GetDistillState() const {
   return state;
 }
 
+void SpeedreaderIconView::DidStopLoading() {
+  Update();
+}
+
 BEGIN_METADATA(SpeedreaderIconView, PageActionIconView)
 END_METADATA

browser/ui/views/speedreader/speedreader_icon_view.h

@@ -8,11 +8,13 @@
 
 #include "brave/browser/speedreader/speedreader_tab_helper.h"
 #include "chrome/browser/ui/views/page_action/page_action_icon_view.h"
+#include "content/public/browser/web_contents_observer.h"
 #include "ui/base/metadata/metadata_header_macros.h"
 
 class PrefService;
 
-class SpeedreaderIconView : public PageActionIconView {
+class SpeedreaderIconView : public PageActionIconView,
+                            public content::WebContentsObserver {
  public:
   METADATA_HEADER(SpeedreaderIconView);
   SpeedreaderIconView(CommandUpdater* command_updater,
@@ -31,6 +33,9 @@ class SpeedreaderIconView : public PageActionIconView {
   std::u16string GetTextForTooltipAndAccessibleName() const override;
   void UpdateImpl() override;
 
+  // content::WebContentsObserver
+  void DidStopLoading() override;
+
  private:
   speedreader::DistillState GetDistillState() const;
 };

components/speedreader/speedreader_extended_info_handler.cc

@@ -47,30 +47,23 @@ void SpeedreaderExtendedInfoHandler::PersistMode(
     case DistillState::kReaderMode:
     case DistillState::kSpeedreaderMode:
       data = std::make_unique<SpeedreaderNavigationData>(kPageSavedDistilled);
+      entry->SetUserData(kSpeedreaderKey, std::move(data));
       break;
     default:
+      SpeedreaderExtendedInfoHandler::ClearPersistedData(entry);
       return;
   }
-
-  entry->SetUserData(kSpeedreaderKey, std::move(data));
 }
 
 // static
-DistillState SpeedreaderExtendedInfoHandler::GetCachedMode(
-    content::NavigationEntry* entry,
-    SpeedreaderService* service) {
+bool SpeedreaderExtendedInfoHandler::IsCached(content::NavigationEntry* entry) {
   DCHECK(entry);
   auto* data = static_cast<SpeedreaderNavigationData*>(
       entry->GetUserData(kSpeedreaderKey));
-  if (!data) {
-    return DistillState::kUnknown;
-  }
-  if (data->value == kPageSavedDistilled) {
-    return service->IsEnabled() ? DistillState::kSpeedreaderMode
-                                : DistillState::kReaderMode;
-  } else {
-    return DistillState::kUnknown;
+  if (!data || data->value.empty()) {
+    return false;
   }
+  return (data->value == kPageSavedDistilled);
 }
 
 // static

components/speedreader/speedreader_extended_info_handler.h

@@ -30,10 +30,7 @@ class SpeedreaderExtendedInfoHandler : public sessions::ExtendedInfoHandler {
   // 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,
-                                    SpeedreaderService* service);
+  static bool IsCached(content::NavigationEntry* entry);
 
   // Clear the NavigationEntry speedreader state
   static void ClearPersistedData(content::NavigationEntry* entry);

components/speedreader/speedreader_result_delegate.h

@@ -8,9 +8,14 @@
 
 // SpeedreaderResultDelegate is an interface for the speedreader component to
 // notify a tab_helper that the distillation has completed successufully.
+enum class DistillStatus {
+  FAIL,
+  SUCCESS,
+  CHECK_CACHE,
+};
 class SpeedreaderResultDelegate {
  public:
-  virtual void OnDistillComplete() = 0;
+  virtual void OnDistillComplete(DistillStatus status) = 0;
 };
 
 #endif  // BRAVE_COMPONENTS_SPEEDREADER_SPEEDREADER_RESULT_DELEGATE_H_

components/speedreader/speedreader_throttle_unittest.cc

@@ -30,7 +30,7 @@ constexpr char kTestProfileName[] = "TestProfile";
 
 class TestSpeedreaderResultDelegate : public SpeedreaderResultDelegate {
  protected:
-  void OnDistillComplete() override {}
+  void OnDistillComplete(DistillStatus status) override {}
 };
 
 }  // anonymous namespace