Store the most recent DistillabilityResult for each web contents.

The main consumer of DistillabilityResult on desktop, the Reader Mode
icon, updates whenever the user switches tabs. However, the
distillability service usually will not run again when the tabs are
switched, so the icon will not update if it sees the web contents'
distillability only when OnResult() is called. It needs to be retrieve
the distillability of a given web contents in between calls to
OnResult(), as well as keep track of which web contents the result
belonged to.

The simplest solution is to store the result in DistillabilityDriver,
since it is already correctly scoped to a single web contents.

Bug: 952042
Change-Id: I8a8633a52583f04b49298ee5186b24a5eb092fdf
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1788498Reviewed-by: Wei-Yin Chen (陳威尹) <wychen@chromium.org>
Commit-Queue: Aran Gilman <gilmanmh@google.com>
Cr-Commit-Position: refs/heads/master@{#697724}

chrome/browser/dom_distiller/distillable_page_utils_browsertest.cc

@@ -8,6 +8,7 @@
 #include "base/bind.h"
 #include "base/command_line.h"
 #include "base/message_loop/message_loop.h"
+#include "base/optional.h"
 #include "base/run_loop.h"
 #include "base/threading/thread_task_runner_handle.h"
 #include "chrome/browser/ui/browser.h"
@@ -30,6 +31,7 @@ using ::testing::_;
 using ::testing::AllOf;
 using ::testing::Field;
 using ::testing::Not;
+using ::testing::Optional;
 
 // This is essentially an "enum" with human-readable strings (e.g. "adaboost",
 // "none") as values.
@@ -138,6 +140,8 @@ IN_PROC_BROWSER_TEST_F(DistillablePageUtilsBrowserTestAlways,
         .WillOnce(testing::InvokeWithoutArgs(
             this, &DistillablePageUtilsBrowserTestOption::QuitSoon));
     NavigateAndWait(kAllPaths[i], base::TimeDelta());
+    EXPECT_THAT(GetLatestResult(web_contents_),
+                Optional(AllOf(IsDistillable(), IsLast())));
   }
 }
 
@@ -145,6 +149,7 @@ IN_PROC_BROWSER_TEST_F(DistillablePageUtilsBrowserTestAlways,
                        LocalUrlsDoNotCallObserver) {
   EXPECT_CALL(holder_, OnResult(_)).Times(0);
   NavigateAndWait("about:blank", kWaitNoExpectedCall);
+  EXPECT_EQ(GetLatestResult(web_contents_), base::nullopt);
 }
 
 using DistillablePageUtilsBrowserTestNone =
@@ -153,6 +158,7 @@ using DistillablePageUtilsBrowserTestNone =
 IN_PROC_BROWSER_TEST_F(DistillablePageUtilsBrowserTestNone, NeverCallObserver) {
   EXPECT_CALL(holder_, OnResult(_)).Times(0);
   NavigateAndWait(kSimpleArticlePath, kWaitNoExpectedCall);
+  EXPECT_EQ(GetLatestResult(web_contents_), base::nullopt);
 }
 
 using DistillablePageUtilsBrowserTestOGArticle =
@@ -164,6 +170,8 @@ IN_PROC_BROWSER_TEST_F(DistillablePageUtilsBrowserTestOGArticle,
       .WillOnce(testing::InvokeWithoutArgs(
           this, &DistillablePageUtilsBrowserTestOption::QuitSoon));
   NavigateAndWait(kArticlePath, base::TimeDelta());
+  EXPECT_THAT(GetLatestResult(web_contents_),
+              Optional(AllOf(IsDistillable(), IsLast())));
 }
 
 IN_PROC_BROWSER_TEST_F(DistillablePageUtilsBrowserTestOGArticle,
@@ -172,6 +180,8 @@ IN_PROC_BROWSER_TEST_F(DistillablePageUtilsBrowserTestOGArticle,
       .WillOnce(testing::InvokeWithoutArgs(
           this, &DistillablePageUtilsBrowserTestOption::QuitSoon));
   NavigateAndWait(kNonArticlePath, base::TimeDelta());
+  EXPECT_THAT(GetLatestResult(web_contents_),
+              Optional(AllOf(Not(IsDistillable()), IsLast())));
 }
 
 using DistillablePageUtilsBrowserTestAdaboost =
@@ -190,6 +200,10 @@ IN_PROC_BROWSER_TEST_F(DistillablePageUtilsBrowserTestAdaboost,
         .WillOnce(testing::InvokeWithoutArgs(
             this, &DistillablePageUtilsBrowserTestOption::QuitSoon));
     NavigateAndWait(paths[i], base::TimeDelta());
+
+    EXPECT_THAT(
+        GetLatestResult(web_contents_),
+        Optional(AllOf(IsDistillable(), IsLast(), Not(IsMobileFriendly()))));
   }
 }
 
@@ -204,6 +218,9 @@ IN_PROC_BROWSER_TEST_F(DistillablePageUtilsBrowserTestAdaboost,
       .WillOnce(testing::InvokeWithoutArgs(
           this, &DistillablePageUtilsBrowserTestOption::QuitSoon));
   NavigateAndWait(kNonArticlePath, base::TimeDelta());
+  EXPECT_THAT(
+      GetLatestResult(web_contents_),
+      Optional(AllOf(Not(IsDistillable()), IsLast(), Not(IsMobileFriendly()))));
 }
 
 using DistillablePageUtilsBrowserTestAllArticles =
@@ -222,6 +239,9 @@ IN_PROC_BROWSER_TEST_F(DistillablePageUtilsBrowserTestAllArticles,
         .WillOnce(testing::InvokeWithoutArgs(
             this, &DistillablePageUtilsBrowserTestOption::QuitSoon));
     NavigateAndWait(paths[i], base::TimeDelta());
+    EXPECT_THAT(
+        GetLatestResult(web_contents_),
+        Optional(AllOf(IsDistillable(), IsLast(), Not(IsMobileFriendly()))));
   }
 }
 
@@ -236,6 +256,9 @@ IN_PROC_BROWSER_TEST_F(DistillablePageUtilsBrowserTestAllArticles,
       .WillOnce(testing::InvokeWithoutArgs(
           this, &DistillablePageUtilsBrowserTestOption::QuitSoon));
   NavigateAndWait(kNonArticlePath, base::TimeDelta());
+  EXPECT_THAT(
+      GetLatestResult(web_contents_),
+      Optional(AllOf(Not(IsDistillable()), IsLast(), Not(IsMobileFriendly()))));
 }
 
 }  // namespace dom_distiller

components/dom_distiller/content/browser/distillability_driver.cc

@@ -46,7 +46,8 @@ class DistillabilityServiceImpl : public mojom::DistillabilityService {
 };
 
 DistillabilityDriver::DistillabilityDriver(content::WebContents* web_contents)
-    : content::WebContentsObserver(web_contents) {
+    : content::WebContentsObserver(web_contents),
+      latest_result_(base::nullopt) {
   if (!web_contents)
     return;
   frame_interfaces_.AddInterface(
@@ -73,6 +74,7 @@ void DistillabilityDriver::AddObserver(DistillabilityObserver* observer) {
 
 void DistillabilityDriver::OnDistillability(
     const DistillabilityResult& result) {
+  latest_result_ = result;
   for (auto& observer : observers_)
     observer.OnResult(result);
 }

components/dom_distiller/content/browser/distillability_driver.h

@@ -10,6 +10,7 @@
 #include "base/macros.h"
 #include "base/memory/weak_ptr.h"
 #include "base/observer_list.h"
+#include "base/optional.h"
 #include "components/dom_distiller/content/browser/distillable_page_utils.h"
 #include "components/dom_distiller/content/common/mojom/distillability_service.mojom.h"
 #include "content/public/browser/web_contents_observer.h"
@@ -28,6 +29,9 @@ class DistillabilityDriver
   void CreateDistillabilityService(mojom::DistillabilityServiceRequest request);
 
   void AddObserver(DistillabilityObserver* observer);
+  base::Optional<DistillabilityResult> GetLatestResult() const {
+    return latest_result_;
+  }
 
   // content::WebContentsObserver implementation.
   void OnInterfaceRequestFromFrame(
@@ -44,6 +48,12 @@ class DistillabilityDriver
 
   base::ObserverList<DistillabilityObserver> observers_;
 
+  // The most recently received result from the distillability service.
+  //
+  // TODO(https://crbug.com/952042): Set this to nullopt when navigating to a
+  // new page, accounting for same-document navigation.
+  base::Optional<DistillabilityResult> latest_result_;
+
   service_manager::BinderRegistry frame_interfaces_;
 
   base::WeakPtrFactory<DistillabilityDriver> weak_factory_{this};

components/dom_distiller/content/browser/distillable_page_utils.cc

@@ -68,4 +68,15 @@ void AddObserver(content::WebContents* web_contents,
   driver->AddObserver(observer);
 }
 
+base::Optional<DistillabilityResult> GetLatestResult(
+    content::WebContents* web_contents) {
+  CHECK(web_contents);
+  DistillabilityDriver::CreateForWebContents(web_contents);
+
+  DistillabilityDriver* driver =
+      DistillabilityDriver::FromWebContents(web_contents);
+  CHECK(driver);
+  return driver->GetLatestResult();
+}
+
 }  // namespace dom_distiller

components/dom_distiller/content/browser/distillable_page_utils.h

@@ -9,6 +9,7 @@
 
 #include "base/callback.h"
 #include "base/observer_list_types.h"
+#include "base/optional.h"
 
 namespace content {
 class WebContents;
@@ -45,6 +46,9 @@ class DistillabilityObserver : public base::CheckedObserver {
 void AddObserver(content::WebContents* web_contents,
                  DistillabilityObserver* observer);
 
+base::Optional<DistillabilityResult> GetLatestResult(
+    content::WebContents* web_contents);
+
 }  // namespace dom_distiller
 
 #endif  // COMPONENTS_DOM_DISTILLER_CONTENT_BROWSER_DISTILLABLE_PAGE_UTILS_H_