Persist Reader Mode theme and font family settings on desktop.

This CL expands the existing JavaScript bindings to allow storing the
theme and font family selected via the in-page appearance settings
dialog to the corresponding synced preferences.

The ability to persist the font size/scale will be added in a
follow-up CL. Storing that setting requires some additional work to
convert the font size in pixels used by the desktop UI to the scale
relative to base size expected by the pref.

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

chrome/browser/chrome_browser_interface_binders.cc

@@ -248,7 +248,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

@@ -601,4 +601,61 @@ 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);
+  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());
+
+  // '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);
+  EXPECT_TRUE(queue.WaitForMessage(&result));
+
+  // Verify that the preferences changed in the browser-side DistilledPagePrefs.
+  EXPECT_EQ(mojom::Theme::kDark, distilled_page_prefs->GetTheme());
+  EXPECT_EQ(mojom::FontFamily::kMonospace,
+            distilled_page_prefs->GetFontFamily());
+}
+
 }  // namespace dom_distiller

components/dom_distiller/content/browser/distiller_javascript_service_impl.cc

@@ -4,15 +4,18 @@
 
 #include "components/dom_distiller/content/browser/distiller_javascript_service_impl.h"
 
+#include "base/strings/string_number_conversions.h"
 #include "mojo/public/cpp/bindings/self_owned_receiver.h"
 
 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() {}
+DistillerJavaScriptServiceImpl::~DistillerJavaScriptServiceImpl() = default;
 
 void DistillerJavaScriptServiceImpl::HandleDistillerOpenSettingsCall() {
   if (!distiller_ui_handle_) {
@@ -22,12 +25,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

@@ -7,16 +7,21 @@
 
 #include "base/macros.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:
-  DistillerJavaScriptServiceImpl(DistillerUIHandle* distiller_ui_handle);
+  DistillerJavaScriptServiceImpl(DistillerUIHandle* distiller_ui_handle,
+                                 DistilledPagePrefs* distilled_page_prefs);
   ~DistillerJavaScriptServiceImpl() override;
 
   // Mojo mojom::DistillerJavaScriptService implementation.
@@ -24,8 +29,12 @@ 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;
+
  private:
   DistillerUIHandle* distiller_ui_handle_;
+  DistilledPagePrefs* distilled_page_prefs_;
 
   DISALLOW_COPY_AND_ASSIGN(DistillerJavaScriptServiceImpl);
 };
@@ -33,6 +42,7 @@ class DistillerJavaScriptServiceImpl
 // 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,16 @@
 
 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 updated appearance settings as synced prefs.
+  HandleStoreThemePref(Theme theme);
+  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"
@@ -22,7 +24,21 @@ DistillerNativeJavaScript::DistillerNativeJavaScript(
     content::RenderFrame* render_frame)
     : render_frame_(render_frame) {}
 
-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) {
@@ -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

@@ -35,6 +35,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

@@ -64,30 +64,46 @@ function setTextDirection(direction) {
   document.body.setAttribute('dir', direction);
 }
 
+function getClassFromElement(element, classList) {
+  let foundClass = classList[0];
+  classList.forEach((cls) => {
+    if (element.classList.contains(cls)) {
+      foundClass = cls;
+    }
+  });
+  return foundClass;
+}
+
 // These classes must agree with the font classes in distilledpage.css.
+// The order must also agree with the corresponding enum in
+// distilled_page_prefs.mojom.
 const fontFamilyClasses = ['sans-serif', 'serif', 'monospace'];
 function useFontFamily(fontFamily) {
   fontFamilyClasses.forEach(
       (element) =>
           document.body.classList.toggle(element, element === fontFamily));
+  distiller.storeFontFamilyPref(fontFamilyClasses.indexOf(fontFamily));
 }
 
 // These classes must agree with the theme classes in distilledpage.css.
+// The order must also agree with the corresponding enum in
+// distilled_page_prefs.mojom.
 const themeClasses = ['light', 'dark', 'sepia'];
 function useTheme(theme) {
   themeClasses.forEach(
       (element) => document.body.classList.toggle(element, element === theme));
   updateToolbarColor(theme);
+  distiller.storeThemePref(themeClasses.indexOf(theme));
 }
 
-function getClassFromElement(element, classList) {
-  let foundClass = classList[0];
-  classList.forEach((cls) => {
-    if (element.classList.contains(cls)) {
-      foundClass = cls;
-    }
-  });
-  return foundClass;
+// TODO(https://crbug.com/1027612): Rename this so that the distinction between
+// it and useTheme() is more obvious.
+function updateThemeSelection(theme) {
+  const selectedElem =
+      document.querySelector('.theme-option input[value="' + theme + '"');
+  if (selectedElem) {
+    selectedElem.checked = true;
+  }
 }
 
 function updateToolbarColor(theme) {