Remove reader mode page from navigation stack on desktop.

This means users will not be able to navigate to Reader Mode pages
using "back" or "forward" buttons, matching behavior in Android.

Also improves the ReaderModeIconViewBrowserTest by resolving a TODO.

AX-Relnotes: n/a. Reader mode is still behind a flag.
Bug: 1090588, 1087949
Change-Id: I016a02b95b1a3acbff9299c518e8bf2780379d32
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2235988Reviewed-by: Elly Fong-Jones <ellyjones@chromium.org>
Reviewed-by: Wei-Yin Chen (陳威尹) <wychen@chromium.org>
Commit-Queue: Katie Dektar <katie@chromium.org>
Cr-Commit-Position: refs/heads/master@{#777001}

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

@@ -54,6 +54,8 @@ class ReaderModeIconView : public PageActionIconView,
   void OnResult(const dom_distiller::DistillabilityResult& result) override;
 
  private:
+  friend class ReaderModeIconViewBrowserTest;
+
   PrefService* pref_service_;
 
   DISALLOW_COPY_AND_ASSIGN(ReaderModeIconView);

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

@@ -9,6 +9,7 @@
 #include "base/test/scoped_feature_list.h"
 #include "chrome/browser/interstitials/security_interstitial_page_test_utils.h"
 #include "chrome/browser/ssl/security_state_tab_helper.h"
+#include "chrome/browser/ui/browser_commands.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"
@@ -20,6 +21,7 @@
 #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 "components/dom_distiller/core/url_utils.h"
 #include "components/security_interstitials/content/security_interstitial_page.h"
 #include "components/security_interstitials/content/security_interstitial_tab_helper.h"
 #include "components/security_interstitials/core/controller_client.h"
@@ -58,6 +60,10 @@ class ReaderModeIconViewBrowserTest : public InProcessBrowserTest {
     return https_server_secure_.get();
   }
 
+  bool IsReaderModeIconActive() {
+    return reader_mode_icon_ && reader_mode_icon_->active();
+  }
+
   PageActionIconView* reader_mode_icon_;
   std::unique_ptr<net::EmbeddedTestServer> https_server_secure_;
 
@@ -68,7 +74,6 @@ class ReaderModeIconViewBrowserTest : public InProcessBrowserTest {
 };
 
 // TODO(gilmanmh): Add tests for the following cases:
-//  * Icon is visible on the distilled page.
 //  * Icon is not visible on about://blank, both initially and after navigating
 //    to a distillable page.
 IN_PROC_BROWSER_TEST_F(ReaderModeIconViewBrowserTest,
@@ -100,6 +105,16 @@ IN_PROC_BROWSER_TEST_F(ReaderModeIconViewBrowserTest,
   observer.WaitForResult(expected_result);
   const bool is_visible_on_article = reader_mode_icon_->GetVisible();
   EXPECT_TRUE(is_visible_on_article);
+  EXPECT_FALSE(IsReaderModeIconActive());
+
+  // The icon should be active after going to the distilled page.
+  GURL reader_url = dom_distiller::url_utils::GetDistillerViewUrlFromUrl(
+      "chrome-distiller", https_server_secure()->GetURL(kSimpleArticlePath),
+      kArticleTitle);
+  ui_test_utils::NavigateToURL(browser(), reader_url);
+  const bool is_visible_on_reader = reader_mode_icon_->GetVisible();
+  EXPECT_TRUE(is_visible_on_reader);
+  EXPECT_TRUE(IsReaderModeIconActive());
 
   // Navigating back to a non-distillable page hides the icon again.
   ui_test_utils::NavigateToURL(browser(),
@@ -246,4 +261,58 @@ IN_PROC_BROWSER_TEST_F(ReaderModeIconViewBrowserTest,
   EXPECT_FALSE(reader_mode_icon_->GetVisible());
 }
 
+IN_PROC_BROWSER_TEST_F(ReaderModeIconViewBrowserTest,
+                       CannotEnterWithBackwardOrForward) {
+  content::WebContents* web_contents =
+      browser()->tab_strip_model()->GetActiveWebContents();
+  dom_distiller::TestDistillabilityObserver observer(web_contents);
+  dom_distiller::DistillabilityResult expected_result;
+  expected_result.is_distillable = true;
+  expected_result.is_last = false;
+  expected_result.is_mobile_friendly = false;
+
+  // Load original page.
+  ui_test_utils::NavigateToURL(
+      browser(), https_server_secure()->GetURL(kSimpleArticlePath));
+  observer.WaitForResult(expected_result);
+
+  // Navigate to the reader mode page.
+  GURL reader_url = dom_distiller::url_utils::GetDistillerViewUrlFromUrl(
+      "chrome-distiller", https_server_secure()->GetURL(kSimpleArticlePath),
+      kArticleTitle);
+  ui_test_utils::NavigateToURL(browser(), reader_url);
+
+  // Load some other page.
+  ui_test_utils::NavigateToURL(browser(),
+                               https_server_secure()->GetURL(kNonArticlePath));
+  expected_result.is_distillable = false;
+  observer.WaitForResult(expected_result);
+
+  // "back" should go to the original page, not the reader mode page.
+  chrome::GoBack(browser(), WindowOpenDisposition::CURRENT_TAB);
+  expected_result.is_distillable = true;
+  observer.WaitForResult(expected_result);
+  EXPECT_EQ(https_server_secure()->GetURL(kSimpleArticlePath),
+            web_contents->GetLastCommittedURL());
+
+  // "forward" will also not go to the reader mode page.
+  chrome::GoForward(browser(), WindowOpenDisposition::CURRENT_TAB);
+  expected_result.is_distillable = false;
+  observer.WaitForResult(expected_result);
+  EXPECT_EQ(https_server_secure()->GetURL(kNonArticlePath),
+            web_contents->GetLastCommittedURL());
+  EXPECT_FALSE(chrome::CanGoForward(browser()));
+
+  // Load the article again, then go to Reader Mode, and then back.
+  // We shouldn't be able to go "forward".
+  expected_result.is_distillable = true;
+  ui_test_utils::NavigateToURL(
+      browser(), https_server_secure()->GetURL(kSimpleArticlePath));
+  observer.WaitForResult(expected_result);
+  ui_test_utils::NavigateToURL(browser(), reader_url);
+  chrome::GoBack(browser(), WindowOpenDisposition::CURRENT_TAB);
+  observer.WaitForResult(expected_result);
+  EXPECT_FALSE(chrome::CanGoForward(browser()));
+}
+
 }  // namespace

components/dom_distiller/content/browser/dom_distiller_viewer_source.cc

@@ -30,6 +30,7 @@
 #include "components/dom_distiller/core/viewer.h"
 #include "components/strings/grit/components_strings.h"
 #include "content/public/browser/back_forward_cache.h"
+#include "content/public/browser/navigation_controller.h"
 #include "content/public/browser/navigation_details.h"
 #include "content/public/browser/navigation_entry.h"
 #include "content/public/browser/navigation_handle.h"
@@ -59,6 +60,10 @@ class DomDistillerViewerSource::RequestViewerHandle
   ~RequestViewerHandle() override;
 
   // content::WebContentsObserver implementation:
+#if !defined(OS_ANDROID)
+  void DidStartNavigation(
+      content::NavigationHandle* navigation_handle) override;
+#endif  // !defined(OS_ANDROID)
   void DidFinishNavigation(
       content::NavigationHandle* navigation_handle) override;
   void RenderProcessGone(base::TerminationStatus status) override;
@@ -85,6 +90,11 @@ class DomDistillerViewerSource::RequestViewerHandle
   // Temporary store of pending JavaScript if the page isn't ready to receive
   // data from distillation.
   std::string buffer_;
+
+#if !defined(OS_ANDROID)
+  bool remove_previous_navigation_ = false;
+  int last_distiller_page_index_ = -1;
+#endif  // !defined(OS_ANDROID)
 };
 
 DomDistillerViewerSource::RequestViewerHandle::RequestViewerHandle(
@@ -114,6 +124,29 @@ void DomDistillerViewerSource::RequestViewerHandle::SendJavaScript(
   }
 }
 
+#if !defined(OS_ANDROID)
+void DomDistillerViewerSource::RequestViewerHandle::DidStartNavigation(
+    content::NavigationHandle* navigation_handle) {
+  if (!navigation_handle->IsInMainFrame() ||
+      navigation_handle->IsSameDocument())
+    return;
+
+  // Reader Mode should not pollute the navigation stack. To avoid this, watch
+  // for navigations and prepare to remove any that are "chrome-distiller" URLs.
+  // Note that Android handles this for non-CCT mode in ReaderModeManager.java.
+  // TODO(crbug.com/1090588): Consider combining Android implementation here,
+  // if it doesn't impact CCT mode.
+  int index = web_contents()->GetController().GetLastCommittedEntryIndex();
+  content::NavigationEntry* entry =
+      web_contents()->GetController().GetEntryAtIndex(index);
+
+  if (entry != nullptr && url_utils::IsDistilledPage(entry->GetURL())) {
+    remove_previous_navigation_ = true;
+    last_distiller_page_index_ = index;
+  }
+}
+#endif  // !defined(OS_ANDROID)
+
 void DomDistillerViewerSource::RequestViewerHandle::DidFinishNavigation(
     content::NavigationHandle* navigation_handle) {
   if (!navigation_handle->IsInMainFrame() || !navigation_handle->HasCommitted())
@@ -126,6 +159,17 @@ void DomDistillerViewerSource::RequestViewerHandle::DidFinishNavigation(
     return;
   }
 
+#if !defined(OS_ANDROID)
+  if (remove_previous_navigation_) {
+    remove_previous_navigation_ = false;
+    if (web_contents()->GetController().GetEntryAtIndex(
+            last_distiller_page_index_) != nullptr) {
+      web_contents()->GetController().RemoveEntryAtIndex(
+          last_distiller_page_index_);
+    }
+  }
+#endif  // !defined(OS_ANDROID)
+
   // At the moment we destroy the handle and won't be able
   // to restore the document later, so we prevent the page
   // from being stored in back-forward cache.