Adds a setting to change reader mode discoverability.

When the enable-reader-mode flag is set, users by default see
the reader mode icon in the omnibox for all articles which can
be distilled by reader mode. This adds a feature variable which
allows users to decide in chrome://settings/appearance whether
they want to be shown the reader mode icon in the omnibox
when available.

Bug: 1041589
Change-Id: I6205323ff4eccf47f8c776829525f34adda0f899
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2039336
Commit-Queue: Katie Dektar <katie@chromium.org>
Reviewed-by: Vasilii Sukhanov <vasilii@chromium.org>
Reviewed-by: Elly Fong-Jones <ellyjones@chromium.org>
Reviewed-by: Demetrios Papadopoulos <dpapad@chromium.org>
Reviewed-by: Dominic Mazzoni <dmazzoni@chromium.org>
Reviewed-by: Wei-Yin Chen (陳威尹) <wychen@chromium.org>
Cr-Commit-Position: refs/heads/master@{#742697}

chrome/app/settings_strings.grdp

@@ -508,6 +508,12 @@
       Show warning before quitting with ⌘Q
     </message>
   </if>
+  <message name="IDS_SETTINGS_READER_MODE" desc="Label for a setting to enable/disable offers to show articles in reader mode">
+    Reader mode for web pages
+  </message>
+  <message name="IDS_SETTINGS_READER_MODE_DESCRIPTION" desc="Description for a setting to enable/disable offers to show articles in reader mode, which is a simplified view of the original page">
+    Offer to show articles in reader mode, when supported
+  </message>
 
   <!-- Common -->
   <message name="IDS_SETTINGS_ADVANCED" desc="Name of the settings page which displays advanced preferences.">

chrome/app/settings_strings_grdp/IDS_SETTINGS_READER_MODE.png.sha1

+711ce3273fe49f1b6d0bea700039c9bf0de61b25
\ No newline at end of file

chrome/app/settings_strings_grdp/IDS_SETTINGS_READER_MODE_DESCRIPTION.png.sha1

+711ce3273fe49f1b6d0bea700039c9bf0de61b25
\ No newline at end of file

chrome/browser/about_flags.cc

@@ -426,7 +426,14 @@ const FeatureEntry::FeatureVariation kCCTModuleCacheVariations[] = {
     {"30 minutes", kCCTModuleCache_ThirtyMinutes,
      base::size(kCCTModuleCache_ThirtyMinutes), nullptr},
 };
-
+#else  // !defined(OS_ANDROID)
+const FeatureEntry::FeatureParam kReaderModeOfferInSettings[] = {
+    {switches::kReaderModeDiscoverabilityParamName,
+     switches::kReaderModeOfferInSettings}};
+
+const FeatureEntry::FeatureVariation kReaderModeDiscoverabilityVariations[] = {
+    {"available in settings", kReaderModeOfferInSettings,
+     base::size(kReaderModeOfferInSettings), nullptr}};
 #endif  // OS_ANDROID
 
 #if !defined(OS_CHROMEOS)
@@ -1597,7 +1604,9 @@ const FeatureEntry kFeatureEntries[] = {
 #if !defined(OS_ANDROID)
     {"enable-reader-mode", flag_descriptions::kEnableReaderModeName,
      flag_descriptions::kEnableReaderModeDescription, kOsDesktop,
-     FEATURE_VALUE_TYPE(dom_distiller::kReaderMode)},
+     FEATURE_WITH_PARAMS_VALUE_TYPE(dom_distiller::kReaderMode,
+                                    kReaderModeDiscoverabilityVariations,
+                                    "ReaderMode")},
 #endif  // !defined(OS_ANDROID)
 #if defined(WEBRTC_USE_PIPEWIRE)
     {"enable-webrtc-pipewire-capturer",

chrome/browser/extensions/api/settings_private/prefs_util.cc

@@ -21,6 +21,7 @@
 #include "components/browsing_data/core/pref_names.h"
 #include "components/component_updater/pref_names.h"
 #include "components/content_settings/core/common/pref_names.h"
+#include "components/dom_distiller/core/pref_names.h"
 #include "components/drive/drive_pref_names.h"
 #include "components/embedder_support/pref_names.h"
 #include "components/language/core/browser/pref_names.h"
@@ -199,6 +200,8 @@ const PrefsUtil::TypedPrefMap& PrefsUtil::GetWhitelistedKeys() {
   (*s_whitelist)[::prefs::kConfirmToQuitEnabled] =
       settings_api::PrefType::PREF_TYPE_BOOLEAN;
 #endif
+  (*s_whitelist)[dom_distiller::prefs::kOfferReaderMode] =
+      settings_api::PrefType::PREF_TYPE_BOOLEAN;
 
   // On startup.
   (*s_whitelist)[::prefs::kRestoreOnStartup] =

chrome/browser/prefs/browser_prefs.cc

@@ -93,6 +93,8 @@
 #include "components/content_settings/core/browser/host_content_settings_map.h"
 #include "components/data_reduction_proxy/core/browser/data_reduction_proxy_prefs.h"
 #include "components/dom_distiller/core/distilled_page_prefs.h"
+#include "components/dom_distiller/core/dom_distiller_features.h"
+#include "components/dom_distiller/core/pref_names.h"
 #include "components/feature_engagement/buildflags.h"
 #include "components/flags_ui/pref_service_flags_storage.h"
 #include "components/image_fetcher/core/cache/image_cache.h"
@@ -834,8 +836,9 @@ void RegisterProfilePrefs(user_prefs::PrefRegistrySyncable* registry,
   chrome_browser_net::NetErrorTabHelper::RegisterProfilePrefs(registry);
   chrome_browser_net::RegisterPredictionOptionsProfilePrefs(registry);
   chrome_prefs::RegisterProfilePrefs(registry);
-  dom_distiller::DistilledPagePrefs::RegisterProfilePrefs(registry);
   DocumentProvider::RegisterProfilePrefs(registry);
+  dom_distiller::DistilledPagePrefs::RegisterProfilePrefs(registry);
+  dom_distiller::RegisterProfilePrefs(registry);
   DownloadPrefs::RegisterProfilePrefs(registry);
   HostContentSettingsMap::RegisterProfilePrefs(registry);
   image_fetcher::ImageCache::RegisterProfilePrefs(registry);

chrome/browser/resources/settings/appearance_page/appearance_page.html

@@ -158,6 +158,13 @@
             </template>
           </select>
         </div>
+        <template is="dom-if" if="[[showReaderModeOption_]]">
+          <settings-toggle-button
+              pref="{{prefs.dom_distiller.offer_reader_mode}}"
+              label="$i18n{readerMode}"
+              sub-label="$i18n{readerModeDescription}">
+          </settings-toggle-button>
+        </template>
 <if expr="is_macosx">
         <settings-toggle-button pref="{{prefs.webkit.webprefs.tabs_to_links}}"
             label="$i18n{tabsToLinks}">

chrome/browser/resources/settings/appearance_page/appearance_page.js

@@ -94,6 +94,14 @@ Polymer({
         return map;
       },
     },
+
+    /** @private */
+    showReaderModeOption_: {
+      type: Boolean,
+      value() {
+        return loadTimeData.getBoolean('showReaderModeOption');
+      },
+    },
   },
 
   /** @private {?settings.AppearanceBrowserProxy} */

chrome/browser/ui/toolbar/app_menu_model.cc

@@ -802,7 +802,8 @@ void AppMenuModel::Build() {
                 ->GetLastCommittedURL())) {
       // Show the menu option if we are on a distilled page.
       AddItemWithStringId(IDC_DISTILL_PAGE, IDS_DISTILL_PAGE);
-    } else {
+    } else if (dom_distiller::ShowReaderModeOption(
+                   browser_->profile()->GetPrefs())) {
       // Show the menu option if the page is distillable.
       base::Optional<dom_distiller::DistillabilityResult> distillability =
           dom_distiller::GetLatestResult(

chrome/browser/ui/views/page_action/page_action_icon_controller.cc

@@ -116,7 +116,8 @@ void PageActionIconController::Init(const PageActionIconParams& params,
         DCHECK(params.command_updater);
         reader_mode_icon_ = new ReaderModeIconView(
             params.command_updater, params.icon_label_bubble_delegate,
-            params.page_action_icon_delegate);
+            params.page_action_icon_delegate,
+            params.browser->profile()->GetPrefs());
         page_action_icons_.push_back(reader_mode_icon_);
         break;
       case PageActionIconType::kSaveCard:

chrome/browser/ui/views/reader_mode/reader_mode_icon_view.cc

@@ -7,7 +7,9 @@
 #include "chrome/app/chrome_command_ids.h"
 #include "chrome/app/vector_icons/vector_icons.h"
 #include "chrome/grit/generated_resources.h"
+#include "components/dom_distiller/core/dom_distiller_features.h"
 #include "components/dom_distiller/core/url_utils.h"
+#include "components/prefs/pref_service.h"
 #include "content/public/browser/navigation_handle.h"
 #include "content/public/browser/web_contents.h"
 #include "ui/base/l10n/l10n_util.h"
@@ -17,11 +19,13 @@ using dom_distiller::url_utils::IsDistilledPage;
 ReaderModeIconView::ReaderModeIconView(
     CommandUpdater* command_updater,
     IconLabelBubbleView::Delegate* icon_label_bubble_delegate,
-    PageActionIconView::Delegate* page_action_icon_delegate)
+    PageActionIconView::Delegate* page_action_icon_delegate,
+    PrefService* pref_service)
     : PageActionIconView(command_updater,
                          IDC_DISTILL_PAGE,
                          icon_label_bubble_delegate,
-                         page_action_icon_delegate) {}
+                         page_action_icon_delegate),
+      pref_service_(pref_service) {}
 
 ReaderModeIconView::~ReaderModeIconView() {
   content::WebContents* contents = web_contents();
@@ -47,6 +51,14 @@ void ReaderModeIconView::UpdateImpl() {
     SetVisible(true);
     SetActive(true);
   } else {
+    // If the reader mode option shouldn't be shown to the user per their pref
+    // in appearance settings, simply hide the icon.
+    // TODO(katie): In this case, we should not even check if a page is
+    // distillable.
+    if (!dom_distiller::ShowReaderModeOption(pref_service_)) {
+      SetVisible(false);
+      return;
+    }
     // If the currently active web contents has changed since last time, stop
     // observing the old web contents and start observing the new one.
     // (WebContentsObserver::web_contents() is not updated until the call to

chrome/browser/ui/views/reader_mode/reader_mode_icon_view.h

@@ -19,6 +19,7 @@ namespace views {
 class BubbleDialogDelegateView;
 }
 class CommandUpdater;
+class PrefService;
 
 // A location bar icon that toggles Reader Mode for the current page.
 class ReaderModeIconView : public PageActionIconView,
@@ -27,7 +28,8 @@ class ReaderModeIconView : public PageActionIconView,
  public:
   ReaderModeIconView(CommandUpdater* command_updater,
                      IconLabelBubbleView::Delegate* icon_label_bubble_delegate,
-                     PageActionIconView::Delegate* page_action_icon_delegate);
+                     PageActionIconView::Delegate* page_action_icon_delegate,
+                     PrefService* pref_service);
   ~ReaderModeIconView() override;
 
  protected:
@@ -48,6 +50,9 @@ class ReaderModeIconView : public PageActionIconView,
 
   void OnResult(const dom_distiller::DistillabilityResult& result) override;
 
+ private:
+  PrefService* pref_service_;
+
   DISALLOW_COPY_AND_ASSIGN(ReaderModeIconView);
 };
 

chrome/browser/ui/views/reader_mode/reader_mode_icon_view_browsertest.cc

@@ -14,6 +14,8 @@
 #include "components/dom_distiller/content/browser/distillable_page_utils.h"
 #include "components/dom_distiller/core/distilled_page_prefs.h"
 #include "components/dom_distiller/core/dom_distiller_features.h"
+#include "components/dom_distiller/core/dom_distiller_switches.h"
+#include "components/dom_distiller/core/pref_names.h"
 #include "content/public/browser/web_contents.h"
 #include "content/public/test/browser_test.h"
 #include "testing/gtest/include/gtest/gtest.h"
@@ -107,6 +109,8 @@ class ReaderModeIconViewBrowserTest : public InProcessBrowserTest {
 
  private:
   base::test::ScopedFeatureList feature_list_;
+
+  DISALLOW_COPY_AND_ASSIGN(ReaderModeIconViewBrowserTest);
 };
 
 // TODO(gilmanmh): Add tests for the following cases:
@@ -153,4 +157,73 @@ IN_PROC_BROWSER_TEST_F(ReaderModeIconViewBrowserTest,
   EXPECT_FALSE(is_visible_after_navigation_back_to_non_distillable_page);
 }
 
+class ReaderModeIconViewBrowserTestWithSettings
+    : public ReaderModeIconViewBrowserTest {
+ protected:
+  ReaderModeIconViewBrowserTestWithSettings() {
+    feature_list_.InitAndEnableFeatureWithParameters(
+        dom_distiller::kReaderMode,
+        {{switches::kReaderModeDiscoverabilityParamName,
+          switches::kReaderModeOfferInSettings}});
+  }
+
+  void SetOfferReaderModeSetting(bool value) {
+    browser()->profile()->GetPrefs()->SetBoolean(
+        dom_distiller::prefs::kOfferReaderMode, value);
+  }
+
+ private:
+  base::test::ScopedFeatureList feature_list_;
+
+  DISALLOW_COPY_AND_ASSIGN(ReaderModeIconViewBrowserTestWithSettings);
+};
+
+IN_PROC_BROWSER_TEST_F(ReaderModeIconViewBrowserTestWithSettings,
+                       IconVisibilityDependsOnSettingIfExperimentEnabled) {
+  SetOfferReaderModeSetting(false);
+
+  TestDistillabilityObserver observer(
+      browser()->tab_strip_model()->GetActiveWebContents());
+  dom_distiller::DistillabilityResult expected_result;
+  expected_result.is_distillable = true;
+  expected_result.is_last = false;
+  expected_result.is_mobile_friendly = false;
+
+  // The icon should not appear after navigating to a distillable article,
+  // because the setting to offer reader mode is disabled.
+  ui_test_utils::NavigateToURL(
+      browser(), embedded_test_server()->GetURL(kSimpleArticlePath));
+  observer.WaitForResult(expected_result);
+  bool is_visible_on_article = reader_mode_icon_->GetVisible();
+  EXPECT_FALSE(is_visible_on_article);
+
+  // It continues to not show up when navigating to a non-distillable page.
+  ui_test_utils::NavigateToURL(browser(),
+                               embedded_test_server()->GetURL(kNonArticlePath));
+  expected_result.is_distillable = false;
+  observer.WaitForResult(expected_result);
+  bool is_visible_after_navigation_back_to_non_distillable_page =
+      reader_mode_icon_->GetVisible();
+  EXPECT_FALSE(is_visible_after_navigation_back_to_non_distillable_page);
+
+  // If we turn on the setting, the icon should start to show up on a
+  // distillable page.
+  SetOfferReaderModeSetting(true);
+  ui_test_utils::NavigateToURL(
+      browser(), embedded_test_server()->GetURL(kSimpleArticlePath));
+  expected_result.is_distillable = true;
+  observer.WaitForResult(expected_result);
+  is_visible_on_article = reader_mode_icon_->GetVisible();
+  EXPECT_TRUE(is_visible_on_article);
+
+  // But it still turns off when navigating to a non-distillable page.
+  ui_test_utils::NavigateToURL(browser(),
+                               embedded_test_server()->GetURL(kNonArticlePath));
+  expected_result.is_distillable = false;
+  observer.WaitForResult(expected_result);
+  is_visible_after_navigation_back_to_non_distillable_page =
+      reader_mode_icon_->GetVisible();
+  EXPECT_FALSE(is_visible_after_navigation_back_to_non_distillable_page);
+}
+
 }  // namespace

chrome/browser/ui/webui/settings/settings_localized_strings_provider.cc

@@ -50,6 +50,7 @@
 #include "components/autofill/core/common/autofill_payments_features.h"
 #include "components/browsing_data/core/features.h"
 #include "components/content_settings/core/common/features.h"
+#include "components/dom_distiller/core/dom_distiller_features.h"
 #include "components/google/core/common/google_util.h"
 #include "components/omnibox/common/omnibox_features.h"
 #include "components/password_manager/core/browser/manage_passwords_referrer.h"
@@ -294,11 +295,15 @@ void AddAppearanceStrings(content::WebUIDataSource* html_source,
     {"tabsToLinks", IDS_SETTINGS_TABS_TO_LINKS_PREF},
     {"warnBeforeQuitting", IDS_SETTINGS_WARN_BEFORE_QUITTING_PREF},
 #endif
+    {"readerMode", IDS_SETTINGS_READER_MODE},
+    {"readerModeDescription", IDS_SETTINGS_READER_MODE_DESCRIPTION},
   };
   AddLocalizedStringsBulk(html_source, kLocalizedStrings);
 
   html_source->AddString("presetZoomFactors",
                          zoom::GetPresetZoomFactorsAsJSON());
+  html_source->AddBoolean("showReaderModeOption",
+                          dom_distiller::OfferReaderModeInSettings());
 }
 
 void AddChangePasswordStrings(content::WebUIDataSource* html_source) {

components/dom_distiller/core/dom_distiller_features.cc

@@ -7,19 +7,46 @@
 #include <string>
 
 #include "base/command_line.h"
+#include "base/metrics/field_trial_params.h"
 #include "components/dom_distiller/core/dom_distiller_switches.h"
+#include "components/dom_distiller/core/pref_names.h"
+#include "components/pref_registry/pref_registry_syncable.h"
+#include "components/prefs/pref_service.h"
 
 namespace dom_distiller {
 
 const base::Feature kReaderMode{"ReaderMode",
                                 base::FEATURE_DISABLED_BY_DEFAULT};
 
+void RegisterProfilePrefs(user_prefs::PrefRegistrySyncable* registry) {
+  // Whether the reader mode option should be shown on distillable pages.
+  registry->RegisterBooleanPref(
+      dom_distiller::prefs::kOfferReaderMode, false,
+      user_prefs::PrefRegistrySyncable::SYNCABLE_PREF);
+}
+
 bool IsDomDistillerEnabled() {
   return base::CommandLine::ForCurrentProcess()->HasSwitch(
              switches::kEnableDomDistiller) ||
          base::FeatureList::IsEnabled(kReaderMode);
 }
 
+bool OfferReaderModeInSettings() {
+  if (!base::FeatureList::IsEnabled(kReaderMode))
+    return false;
+
+  // Check if the settings parameter is set.
+  std::string parameter = base::GetFieldTrialParamValueByFeature(
+      kReaderMode, switches::kReaderModeDiscoverabilityParamName);
+  return parameter == switches::kReaderModeOfferInSettings;
+}
+
+bool ShowReaderModeOption(PrefService* pref_service) {
+  if (OfferReaderModeInSettings())
+    return pref_service->GetBoolean(prefs::kOfferReaderMode);
+  return IsDomDistillerEnabled();
+}
+
 bool ShouldStartDistillabilityService() {
   return base::CommandLine::ForCurrentProcess()->HasSwitch(
              switches::kEnableDistillabilityService) ||

components/dom_distiller/core/dom_distiller_features.h

@@ -7,13 +7,32 @@
 
 #include "base/feature_list.h"
 
+class PrefService;
+
+namespace user_prefs {
+class PrefRegistrySyncable;
+}
+
 namespace dom_distiller {
 
 extern const base::Feature kReaderMode;
 
-// Returns true when flag enable-dom-distiller is set or enabled from Finch.
+void RegisterProfilePrefs(user_prefs::PrefRegistrySyncable* registry);
+
+// Returns true when flag enable-dom-distiller is set or reader mode is enabled
+// from flags or Finch.
 bool IsDomDistillerEnabled();
 
+// Returns true when reader mode flag is enabled and the flag parameter to add
+// "offer reader mode" in chrome://settings is set.
+bool OfferReaderModeInSettings();
+
+// Returns true if a user should be shown the option to view pages in reader
+// mode, when available. This happens when either:
+// A. OfferReaderModeInSettings is true and kOfferReaderMode pref is enabled,
+// B. or OfferReaderModeInSettings is false, but IsDomDistillerEnabled is true.
+bool ShowReaderModeOption(PrefService* pref_service);
+
 bool ShouldStartDistillabilityService();
 
 }  // namespace dom_distiller

components/dom_distiller/core/dom_distiller_switches.cc

@@ -19,4 +19,7 @@ const char kAlwaysTrue[] = "alwaystrue";
 const char kNone[] = "none";
 }  // namespace reader_mode_heuristics
 
+const char kReaderModeDiscoverabilityParamName[] = "discoverability";
+const char kReaderModeOfferInSettings[] = "offer-in-settings";
+
 }  // namespace switches

components/dom_distiller/core/dom_distiller_switches.h

@@ -30,6 +30,9 @@ extern const char kAlwaysTrue[];
 extern const char kNone[];
 }  // namespace reader_mode_heuristics
 
+extern const char kReaderModeDiscoverabilityParamName[];
+extern const char kReaderModeOfferInSettings[];
+
 }  // namespace switches
 
 #endif  // COMPONENTS_DOM_DISTILLER_CORE_DOM_DISTILLER_SWITCHES_H_

components/dom_distiller/core/pref_names.cc

@@ -15,6 +15,9 @@ const char kTheme[] = "dom_distiller.theme";
 const char kFontScale[] = "dom_distiller.font_scale";
 // Path to the boolean whether Reader Mode for Accessibility option is enabled.
 const char kReaderForAccessibility[] = "dom_distiller.reader_for_accessibility";
+// A boolean pref set to true if the option to use reader mode should be visible
+// on articles, when available.
+const char kOfferReaderMode[] = "dom_distiller.offer_reader_mode";
 
 }  // namespace prefs
 }  // namespace dom_distiller

components/dom_distiller/core/pref_names.h

@@ -12,6 +12,7 @@ extern const char kFont[];
 extern const char kTheme[];
 extern const char kFontScale[];
 extern const char kReaderForAccessibility[];
+extern const char kOfferReaderMode[];
 
 }  // namespace prefs
 }  // namespace dom_distiller