Display original page URL without a scheme on Reader Mode pages.

Bug: 992073
Change-Id: I6a1639f3e0d727b1a0851eef5765ecf68933d395
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2120264
Commit-Queue: Katie Dektar <katie@chromium.org>
Reviewed-by: Charlie Reis <creis@chromium.org>
Reviewed-by: Tommy Li <tommycli@chromium.org>
Cr-Commit-Position: refs/heads/master@{#754576}

components/dom_distiller/core/url_utils.h

@@ -19,12 +19,20 @@ namespace url_utils {
 const GURL GetDistillerViewUrlFromEntryId(const std::string& scheme,
                                           const std::string& entry_id);
 
-// Returns the URL for viewing distilled content for a URL.
+// Returns the URL for viewing distilled content for |view_url|. This URL should
+// not be displayed to end users (except in DevTools and view-source). Instead,
+// users should always be shown the original page URL minus the http or https
+// scheme in the omnibox (i.e. in LocationBarModel::GetFormattedURL()).
+// A distilled page's true URL, the distiller view URL, should be returned
+// from WebContents::GetLastCommittedURL() and WebContents::GetVisibleURL().
+// This has the chrome-distiller scheme and the form
+// chrome-distiller://<hash>?<params>, where <params> are generated from
+// |view_url| and |start_time_ms|.
 const GURL GetDistillerViewUrlFromUrl(const std::string& scheme,
                                       const GURL& view_url,
                                       int64_t start_time_ms = 0);
 
-// Returns the original URL from the distilled URL.
+// Returns the original article's URL from the distilled URL.
 // If |distilled_url| is not distilled, it is returned as is.
 // If |distilled_url| looks like distilled, but no original URL can be found,
 // an empty, invalid URL is returned.

components/omnibox/browser/BUILD.gn

@@ -225,6 +225,7 @@ jumbo_static_library("browser") {
     ":in_memory_url_index_cache_proto",
     "//components/bookmarks/browser",
     "//components/component_updater",
+    "//components/dom_distiller/core:core",
     "//components/favicon/core",
     "//components/favicon_base",
     "//components/keyed_service/core",
@@ -470,6 +471,7 @@ source_set("unit_tests") {
     "//base/test:test_support",
     "//components/bookmarks/browser",
     "//components/bookmarks/test",
+    "//components/dom_distiller/core:core",
     "//components/favicon/core/test:test_support",
     "//components/history/core/test",
     "//components/open_from_clipboard:test_support",

components/omnibox/browser/DEPS

@@ -2,6 +2,7 @@ include_rules = [
   "+components/bookmarks/browser",
   "+components/bookmarks/test",
   "+components/component_updater",
+  "+components/dom_distiller/core",
   "+components/embedder_support/android/java/src/org/chromium/components/embedder_support/util",
   "+components/favicon/core",
   "+components/favicon_base",

components/omnibox/browser/location_bar_model_impl.cc

@@ -10,6 +10,8 @@
 #include "base/strings/utf_string_conversions.h"
 #include "base/time/time.h"
 #include "build/build_config.h"
+#include "components/dom_distiller/core/url_constants.h"
+#include "components/dom_distiller/core/url_utils.h"
 #include "components/omnibox/browser/autocomplete_classifier.h"
 #include "components/omnibox/browser/autocomplete_match.h"
 #include "components/omnibox/browser/buildflags.h"
@@ -55,6 +57,11 @@ base::string16 LocationBarModelImpl::GetURLForDisplay() const {
     format_types |= url_formatter::kFormatUrlTrimAfterHost;
   }
 
+  // Early exit to prevent elision of URLs when relevant extension is enabled.
+  if (delegate_->ShouldPreventElision()) {
+    return GetFormattedURL(format_types);
+  }
+
 #if defined(OS_IOS)
   format_types |= url_formatter::kFormatUrlTrimAfterHost;
 #endif
@@ -76,14 +83,25 @@ base::string16 LocationBarModelImpl::GetFormattedURL(
   if (!ShouldDisplayURL())
     return base::string16{};
 
-  // Reset |format_types| to prevent elision of URLs when relevant extension or
-  // pref is enabled.
-  if (delegate_->ShouldPreventElision()) {
-    format_types = url_formatter::kFormatUrlOmitDefaults &
-                   ~url_formatter::kFormatUrlOmitHTTP;
+  GURL url(GetURL());
+  // Special handling for dom-distiller:. Instead of showing internal reader
+  // mode URLs, show the original article URL without a http or https scheme.
+  // Note that this does not disallow the user from copy-pasting the reader
+  // mode URL, or from seeing it in the view-source url. Note that this also
+  // impacts GetFormattedFullURL which uses GetFormattedURL as a helper.
+  // Virtual URLs were not a good solution for Reader Mode URLs because some
+  // security UI is based off of the virtual URL rather than the original URL,
+  // and Reader Mode has its own security chip. In addition virtual URLs would
+  // add a lot of complexity around passing necessary URL parameters to the
+  // Reader Mode pages.
+  if (url.SchemeIs(dom_distiller::kDomDistillerScheme)) {
+    // Ensure that HTTPS and HTTP will be removed. Reader mode should not
+    // display a scheme, and should only run on HTTP/HTTPS pages.
+    format_types |= url_formatter::kFormatUrlOmitHTTP;
+    format_types |= url_formatter::kFormatUrlOmitHTTPS;
+    url = dom_distiller::url_utils::GetOriginalUrlFromDistillerUrl(url);
   }
 
-  GURL url(GetURL());
   // Note that we can't unescape spaces here, because if the user copies this
   // and pastes it into another program, that program may think the URL ends at
   // the space.

components/omnibox/browser/location_bar_model_impl_unittest.cc

@@ -8,6 +8,8 @@
 #include "base/test/scoped_feature_list.h"
 #include "base/test/task_environment.h"
 #include "build/build_config.h"
+#include "components/dom_distiller/core/url_constants.h"
+#include "components/dom_distiller/core/url_utils.h"
 #include "components/omnibox/browser/location_bar_model_delegate.h"
 #include "components/omnibox/browser/test_omnibox_client.h"
 #include "components/omnibox/common/omnibox_features.h"
@@ -119,6 +121,43 @@ TEST_F(LocationBarModelImplTest,
             model()->GetURLForDisplay());
 }
 
+TEST_F(LocationBarModelImplTest, FormatsReaderModeUrls) {
+  const GURL http_url("http://www.example.com/article.html");
+  // Get the real article's URL shown to the user.
+  delegate()->SetURL(http_url);
+  base::string16 originalDisplayUrl = model()->GetURLForDisplay();
+  base::string16 originalFormattedFullUrl = model()->GetFormattedFullURL();
+  // We expect that they don't start with "http://." We want the reader mode
+  // URL shown to the user to be the same as this original URL.
+#ifdef OS_IOS
+  EXPECT_EQ(base::ASCIIToUTF16("example.com/TestSuffix"), originalDisplayUrl);
+#else
+  EXPECT_EQ(base::ASCIIToUTF16("example.com/article.html/TestSuffix"),
+            originalDisplayUrl);
+#endif
+  EXPECT_EQ(base::ASCIIToUTF16("www.example.com/article.html/TestSuffix"),
+            originalFormattedFullUrl);
+
+  GURL distilled = dom_distiller::url_utils::GetDistillerViewUrlFromUrl(
+      dom_distiller::kDomDistillerScheme, http_url);
+  // Ensure the test is set up properly by checking the reader mode URL has
+  // the reader mode scheme.
+  EXPECT_EQ(dom_distiller::kDomDistillerScheme, distilled.scheme());
+  delegate()->SetURL(distilled);
+
+  // The user should see the same URL seen for the original article.
+  EXPECT_EQ(originalDisplayUrl, model()->GetURLForDisplay());
+  EXPECT_EQ(originalFormattedFullUrl, model()->GetFormattedFullURL());
+
+  // Similarly, https scheme should also be hidden.
+  const GURL https_url("https://www.example.com/article.html");
+  distilled = dom_distiller::url_utils::GetDistillerViewUrlFromUrl(
+      dom_distiller::kDomDistillerScheme, https_url);
+  delegate()->SetURL(distilled);
+  EXPECT_EQ(originalDisplayUrl, model()->GetURLForDisplay());
+  EXPECT_EQ(originalFormattedFullUrl, model()->GetFormattedFullURL());
+}
+
 // TODO(https://crbug.com/1010418): Fix flakes on linux_chromium_asan_rel_ng and
 // re-enable this test.
 #if defined(OS_LINUX)