Reland "Reland "Persist Reader Mode theme and font family settings on desktop.""

This reverts commit eb95adf5.

Reason for revert: Flaky test fixed.

Original change's description:
> Revert "Reland "Persist Reader Mode theme and font family settings on desktop.""
> 
> This reverts commit d1173dbd.
> 
> Reason for revert: DomDistillerViewerSourceBrowserTest.UISetsPrefs flaky
> 
> Original change's description:
> > Reland "Persist Reader Mode theme and font family settings on desktop."
> > 
> > This reverts commit 23979013, which
> > reverted an earlier implementation of this CL in commit
> > a18fd063 due to
> > https://crbug.com/1065341.
> > 
> > The main functional difference between this and the original version is
> > where the JavaScript code calls the pref-storing functions. They were
> > previously called inside useTheme() and useFontFamily() instead of the
> > UI callbacks. Changes to the synced prefs result in a series of
> > observers being notified, which ultimately led to the useTheme() and
> > useFontFamily() methods being called again, potentially leading to an
> > infinite loop and the behavior seen in the bug.
> > 
> > The updateThemeSelection() method from the original CL was excluded in
> > this reimplementation because it was unintentionally never called in the
> > original CL. Omitting it helps reduce the scope of the CL and highlight
> > the important changes.
> > 
> > Bug: 1016615, 1065341
> > Change-Id: Ifd0e71027afd196267419ed81f7bdc0d78978a98
> > Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2152901
> > Reviewed-by: Emily Stark <estark@chromium.org>
> > Reviewed-by: Wei-Yin Chen (陳威尹) <wychen@chromium.org>
> > Reviewed-by: Dominic Mazzoni <dmazzoni@chromium.org>
> > Commit-Queue: Aran Gilman <gilmanmh@google.com>
> > Cr-Commit-Position: refs/heads/master@{#762524}
> 
> TBR=dmazzoni@chromium.org,estark@chromium.org,wychen@chromium.org,gilmanmh@google.com
> 
> # Not skipping CQ checks because original CL landed > 1 day ago.
> 
> Bug: 1016615, 1065341, 1075439
> Change-Id: I222e95940848b68fa8158fd64aaca76048ed96db
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2167751
> Reviewed-by: Alexander Hendrich <hendrich@chromium.org>
> Commit-Queue: Alexander Hendrich <hendrich@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#762756}

TBR=dmazzoni@chromium.org,estark@chromium.org,wychen@chromium.org,hendrich@chromium.org,gilmanmh@google.com

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug: 1016615, 1065341, 1075439
Change-Id: Id1033620710d17d45c2df85a126b44a1f226e8b7
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2170422Reviewed-by: Aran Gilman <gilmanmh@google.com>
Reviewed-by: Emily Stark <estark@chromium.org>
Reviewed-by: Wei-Yin Chen (陳威尹) <wychen@chromium.org>
Reviewed-by: Dominic Mazzoni <dmazzoni@chromium.org>
Commit-Queue: Aran Gilman <gilmanmh@google.com>
Cr-Commit-Position: refs/heads/master@{#768954}

chrome/browser/chrome_browser_interface_binders.cc

@@ -257,7 +257,9 @@ void BindDistillerJavaScriptService(
       distiller_ui_handle)
       ->set_render_frame_host(frame_host);
 #endif
-  CreateDistillerJavaScriptService(distiller_ui_handle, std::move(receiver));
+  auto* distilled_page_prefs = dom_distiller_service->GetDistilledPagePrefs();
+  CreateDistillerJavaScriptService(distiller_ui_handle, distilled_page_prefs,
+                                   std::move(receiver));
 }
 
 void BindPrerenderCanceler(

chrome/browser/dom_distiller/dom_distiller_viewer_source_browsertest.cc

@@ -53,6 +53,8 @@ using test::FakeDistiller;
 using test::MockDistillerFactory;
 using test::MockDistillerPage;
 using test::MockDistillerPageFactory;
+using testing::Eq;
+using testing::ExplainMatchResult;
 using testing::HasSubstr;
 using testing::Not;
 
@@ -94,6 +96,26 @@ void ExpectBodyHasThemeAndFont(content::WebContents* contents,
   EXPECT_THAT(result, HasSubstr(expected_font));
 }
 
+class PrefChangeObserver : public DistilledPagePrefs::Observer {
+ public:
+  void WaitForChange(DistilledPagePrefs* prefs) {
+    prefs->AddObserver(this);
+    base::RunLoop run_loop;
+    callback_ = run_loop.QuitClosure();
+    run_loop.Run();
+    prefs->RemoveObserver(this);
+  }
+
+  void OnChangeFontFamily(mojom::FontFamily font_family) override {
+    callback_.Run();
+  }
+  void OnChangeTheme(mojom::Theme theme) override { callback_.Run(); }
+  void OnChangeFontScaling(float scaling) override { callback_.Run(); }
+
+ private:
+  base::RepeatingClosure callback_;
+};
+
 }  // namespace
 
 class DomDistillerViewerSourceBrowserTest : public InProcessBrowserTest {
@@ -602,4 +624,70 @@ IN_PROC_BROWSER_TEST_F(DomDistillerViewerSourceBrowserTest, PrefPersist) {
   ASSERT_FLOAT_EQ(kScale, fontSize / oldFontSize);
 }
 
+IN_PROC_BROWSER_TEST_F(DomDistillerViewerSourceBrowserTest, UISetsPrefs) {
+  content::WebContents* contents =
+      browser()->tab_strip_model()->GetActiveWebContents();
+
+  // Load the distilled page
+  GURL view_url;
+  expect_distillation_ = true;
+  expect_distiller_page_ = true;
+  GURL original_url("http://www.example.com/1");
+  view_url = url_utils::GetDistillerViewUrlFromUrl(kDomDistillerScheme,
+                                                   original_url, "Title");
+  ViewSingleDistilledPage(view_url, "text/html");
+  content::WaitForLoadStop(contents);
+
+  DistilledPagePrefs* distilled_page_prefs =
+      DomDistillerServiceFactory::GetForBrowserContext(browser()->profile())
+          ->GetDistilledPagePrefs();
+
+  // Verify that the initial preferences aren't the same as those set below.
+  ExpectBodyHasThemeAndFont(contents, "light", "sans-serif");
+  EXPECT_NE(mojom::Theme::kDark, distilled_page_prefs->GetTheme());
+  EXPECT_NE(mojom::FontFamily::kMonospace,
+            distilled_page_prefs->GetFontFamily());
+
+  // Wait for all currently executing scripts to finish. Otherwise, the
+  // distiller object used to send the prefs to the browser from the JavaScript
+  // may not exist, causing test flakiness.
+  base::RunLoop().RunUntilIdle();
+
+  // 'Click' the associated UI elements for changing each preference.
+  const std::string script = R"(
+      (() => {
+        const observer = new MutationObserver((mutationsList, observer) => {
+          const classes = document.body.classList;
+          if (classes.contains('dark') && classes.contains('monospace')) {
+            observer.disconnect();
+            window.domAutomationController.send(document.body.className);
+          }
+        });
+
+        observer.observe(document.body, {
+          attributes: true,
+          attributeFilter: [ 'class' ]
+        });
+
+        document.querySelector('.theme-option .dark')
+          .dispatchEvent(new MouseEvent('click'));
+        document.querySelector(
+            '#font-family-selection option[value="monospace"]')
+          .dispatchEvent(new Event("change", { bubbles: true }));
+      })();)";
+  content::DOMMessageQueue queue(contents);
+  std::string result;
+  content::ExecuteScriptAsync(contents, script);
+  ASSERT_TRUE(queue.WaitForMessage(&result));
+
+  // Verify that the preferences changed in the browser-side
+  // DistilledPagePrefs.
+  PrefChangeObserver observer;
+  while (distilled_page_prefs->GetTheme() != mojom::Theme::kDark ||
+         mojom::FontFamily::kMonospace !=
+             distilled_page_prefs->GetFontFamily()) {
+    observer.WaitForChange(distilled_page_prefs);
+  }
+}
+
 }  // namespace dom_distiller

components/dom_distiller/content/browser/distiller_javascript_service_impl.cc

@@ -9,8 +9,10 @@
 namespace dom_distiller {
 
 DistillerJavaScriptServiceImpl::DistillerJavaScriptServiceImpl(
-    DistillerUIHandle* distiller_ui_handle)
-    : distiller_ui_handle_(distiller_ui_handle) {}
+    DistillerUIHandle* distiller_ui_handle,
+    DistilledPagePrefs* distilled_page_prefs)
+    : distiller_ui_handle_(distiller_ui_handle),
+      distilled_page_prefs_(distilled_page_prefs) {}
 
 DistillerJavaScriptServiceImpl::~DistillerJavaScriptServiceImpl() = default;
 
@@ -22,12 +24,22 @@ void DistillerJavaScriptServiceImpl::HandleDistillerOpenSettingsCall() {
   distiller_ui_handle_->OpenSettings();
 }
 
+void DistillerJavaScriptServiceImpl::HandleStoreThemePref(mojom::Theme theme) {
+  distilled_page_prefs_->SetTheme(theme);
+}
+
+void DistillerJavaScriptServiceImpl::HandleStoreFontFamilyPref(
+    mojom::FontFamily font_family) {
+  distilled_page_prefs_->SetFontFamily(font_family);
+}
+
 void CreateDistillerJavaScriptService(
     DistillerUIHandle* distiller_ui_handle,
+    DistilledPagePrefs* distilled_page_prefs,
     mojo::PendingReceiver<mojom::DistillerJavaScriptService> receiver) {
-  mojo::MakeSelfOwnedReceiver(
-      std::make_unique<DistillerJavaScriptServiceImpl>(distiller_ui_handle),
-      std::move(receiver));
+  mojo::MakeSelfOwnedReceiver(std::make_unique<DistillerJavaScriptServiceImpl>(
+                                  distiller_ui_handle, distilled_page_prefs),
+                              std::move(receiver));
 }
 
 }  // namespace dom_distiller

components/dom_distiller/content/browser/distiller_javascript_service_impl.h

@@ -6,17 +6,21 @@
 #define COMPONENTS_DOM_DISTILLER_CONTENT_BROWSER_DISTILLER_JAVASCRIPT_SERVICE_IMPL_H_
 
 #include "components/dom_distiller/content/common/mojom/distiller_javascript_service.mojom.h"
+#include "components/dom_distiller/core/distilled_page_prefs.h"
 #include "components/dom_distiller/core/distiller_ui_handle.h"
+#include "components/dom_distiller/core/mojom/distilled_page_prefs.mojom.h"
 #include "mojo/public/cpp/bindings/pending_receiver.h"
 
 namespace dom_distiller {
 
+class DistilledPagePrefs;
+
 // This is the receiving end of "distiller" JavaScript object calls.
 class DistillerJavaScriptServiceImpl
     : public mojom::DistillerJavaScriptService {
  public:
-  explicit DistillerJavaScriptServiceImpl(
-      DistillerUIHandle* distiller_ui_handle);
+  DistillerJavaScriptServiceImpl(DistillerUIHandle* distiller_ui_handle,
+                                 DistilledPagePrefs* distilled_page_prefs);
   ~DistillerJavaScriptServiceImpl() override;
 
   // Mojo mojom::DistillerJavaScriptService implementation.
@@ -24,6 +28,9 @@ class DistillerJavaScriptServiceImpl
   // Show the Android view containing Reader Mode settings.
   void HandleDistillerOpenSettingsCall() override;
 
+  void HandleStoreThemePref(mojom::Theme theme) override;
+  void HandleStoreFontFamilyPref(mojom::FontFamily font_family) override;
+
   DistillerJavaScriptServiceImpl(const DistillerJavaScriptServiceImpl&) =
       delete;
   DistillerJavaScriptServiceImpl& operator=(
@@ -31,11 +38,13 @@ class DistillerJavaScriptServiceImpl
 
  private:
   DistillerUIHandle* distiller_ui_handle_;
+  DistilledPagePrefs* distilled_page_prefs_;
 };
 
 // static
 void CreateDistillerJavaScriptService(
     DistillerUIHandle* distiller_ui_handle,
+    DistilledPagePrefs* distilled_page_prefs,
     mojo::PendingReceiver<mojom::DistillerJavaScriptService> receiver);
 
 }  // namespace dom_distiller

components/dom_distiller/content/common/mojom/BUILD.gn

@@ -10,4 +10,5 @@ mojom("mojom") {
     "distiller_javascript_service.mojom",
     "distiller_page_notifier_service.mojom",
   ]
+  deps = [ "//components/dom_distiller/core/mojom" ]
 }

components/dom_distiller/content/common/mojom/distiller_javascript_service.mojom

@@ -4,10 +4,17 @@
 
 module dom_distiller.mojom;
 
+import "components/dom_distiller/core/mojom/distilled_page_prefs.mojom";
+
 // This service is implemented by the browser process and is used by the
 // renderer when a distiller JavaScript function is called.
 interface DistillerJavaScriptService {
   // Open the Android view containing settings for Reader Mode; the
   // "distiller.openSettings" function.
   HandleDistillerOpenSettingsCall();
+
+  // Store selected theme as a synced preference.
+  HandleStoreThemePref(Theme theme);
+  // Store selected font family as a synced preference.
+  HandleStoreFontFamilyPref(FontFamily font_family);
 };

components/dom_distiller/content/renderer/BUILD.gn

@@ -19,6 +19,7 @@ static_library("renderer") {
     "//base",
     "//components/dom_distiller/content/common/mojom",
     "//components/dom_distiller/core",
+    "//components/dom_distiller/core/mojom",
     "//content/public/common",
     "//content/public/renderer",
     "//gin",

components/dom_distiller/content/renderer/distiller_native_javascript.cc

@@ -9,6 +9,8 @@
 #include "base/bind.h"
 #include "base/strings/utf_string_conversions.h"
 #include "components/dom_distiller/content/common/mojom/distiller_javascript_service.mojom.h"
+#include "components/dom_distiller/core/distilled_page_prefs.h"
+#include "components/dom_distiller/core/mojom/distilled_page_prefs.mojom.h"
 #include "content/public/renderer/render_frame.h"
 #include "gin/arguments.h"
 #include "gin/function_template.h"
@@ -24,6 +26,20 @@ DistillerNativeJavaScript::DistillerNativeJavaScript(
 
 DistillerNativeJavaScript::~DistillerNativeJavaScript() = default;
 
+void DistillerNativeJavaScript::StoreIntTheme(int int_theme) {
+  auto theme = static_cast<mojom::Theme>(int_theme);
+  if (!mojom::IsKnownEnumValue(theme))
+    return;
+  distiller_js_service_->HandleStoreThemePref(theme);
+}
+
+void DistillerNativeJavaScript::StoreIntFontFamily(int int_font_family) {
+  auto font_family = static_cast<mojom::FontFamily>(int_font_family);
+  if (!mojom::IsKnownEnumValue(font_family))
+    return;
+  distiller_js_service_->HandleStoreFontFamilyPref(font_family);
+}
+
 void DistillerNativeJavaScript::AddJavaScriptObjectToFrame(
     v8::Local<v8::Context> context) {
   v8::Isolate* isolate = blink::MainThreadIsolate();
@@ -46,6 +62,15 @@ void DistillerNativeJavaScript::AddJavaScriptObjectToFrame(
       base::BindRepeating(
           &mojom::DistillerJavaScriptService::HandleDistillerOpenSettingsCall,
           base::Unretained(distiller_js_service_.get())));
+
+  BindFunctionToObject(isolate, distiller_obj, "storeThemePref",
+                       base::Bind(&DistillerNativeJavaScript::StoreIntTheme,
+                                  base::Unretained(this)));
+
+  BindFunctionToObject(
+      isolate, distiller_obj, "storeFontFamilyPref",
+      base::Bind(&DistillerNativeJavaScript::StoreIntFontFamily,
+                 base::Unretained(this)));
 }
 
 template <typename Sig>

components/dom_distiller/content/renderer/distiller_native_javascript.h

@@ -36,6 +36,11 @@ class DistillerNativeJavaScript {
   // Make sure the mojo service is connected.
   void EnsureServiceConnected();
 
+  // Wrappers to convert integer representations of the pref enums, then send
+  // the enum values to the browser process.
+  void StoreIntTheme(int theme);
+  void StoreIntFontFamily(int font_family);
+
   content::RenderFrame* render_frame_;
   mojo::Remote<mojom::DistillerJavaScriptService> distiller_js_service_;
 };

components/dom_distiller/core/javascript/dom_distiller_viewer.js

@@ -449,11 +449,15 @@ class SettingsDialog {
     $('close-settings-button').addEventListener('click', this.close.bind(this));
 
     $('theme-selection').addEventListener('change', (e) => {
-      useTheme(e.target.value);
+      const newTheme = e.target.value;
+      useTheme(newTheme);
+      distiller.storeThemePref(themeClasses.indexOf(newTheme));
     });
 
     $('font-family-selection').addEventListener('change', (e) => {
-      useFontFamily(e.target.value);
+      const newFontFamily = e.target.value;
+      useFontFamily(newFontFamily);
+      distiller.storeFontFamilyPref(fontFamilyClasses.indexOf(newFontFamily));
     });
   }