If a distilled URL doesn't correspond to an article, show original URL.

In the omnibox we should show the original article's URL for distilled
pages, but if the distilled URL is invalid then we should continue
to show the dom-distiller:// URL rather than nothing.

This might happen if a user types in a URL beginning with
dom-distiller://. We do not want users to try to navigate directly to
dom-distiller pages, but we should handle the omnibox well when they
do.

Bug: 1066552
Change-Id: I7e569b74c49f9c5557647635f53815f724c8ce33
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2129900
Commit-Queue: Katie Dektar <katie@chromium.org>
Reviewed-by: Wei-Yin Chen (陳威尹) <wychen@chromium.org>
Reviewed-by: Tommy Li <tommycli@chromium.org>
Cr-Commit-Position: refs/heads/master@{#757603}

components/dom_distiller/core/url_utils.cc

@@ -54,7 +54,7 @@ const GURL GetDistillerViewUrlFromUrl(const std::string& scheme,
 }
 
 const GURL GetOriginalUrlFromDistillerUrl(const GURL& url) {
-  if (!IsDistilledPage(url))
+  if (!IsUrlDistilledFormat(url))
     return url;
 
   std::string original_url_str;
@@ -127,6 +127,11 @@ bool IsUrlDistillable(const GURL& url) {
 }
 
 bool IsDistilledPage(const GURL& url) {
+  return IsUrlDistilledFormat(url) &&
+         GetOriginalUrlFromDistillerUrl(url).is_valid();
+}
+
+bool IsUrlDistilledFormat(const GURL& url) {
   return url.is_valid() && url.scheme() == kDomDistillerScheme;
 }
 

components/dom_distiller/core/url_utils.h

@@ -61,9 +61,15 @@ std::string GetValueForKeyInUrlPathQuery(const std::string& path,
 // Returns whether it should be possible to distill the given |url|.
 bool IsUrlDistillable(const GURL& url);
 
-// Returns whether the given |url| is for a distilled page.
+// Returns whether the given |url| is for a distilled page. This means the
+// format of the URL is proper for a distilled page and that it encodes a
+// valid article URL.
 bool IsDistilledPage(const GURL& url);
 
+// Returns whether the given |url| is formatted as if it were for a distilled
+// page, i.e. it is valid and has a chrome-distiller:// scheme.
+bool IsUrlDistilledFormat(const GURL& url);
+
 }  // namespace url_utils
 
 }  // namespace dom_distiller

components/dom_distiller/core/url_utils_unittest.cc

@@ -134,6 +134,28 @@ TEST(DomDistillerUrlUtilsTest, TestRejectInvalidURLs) {
       net::AppendOrReplaceQueryParameter(view_url, kUrlKey, url2);
   EXPECT_EQ(GURL(), GetOriginalUrlFromDistillerUrl(bad_view_url));
 }
+
+TEST(DomDistillerUrlUtilsTest, TestRejectInvalidDistilledURLs) {
+  EXPECT_FALSE(IsDistilledPage(GURL("chrome-distiller://any")));
+  EXPECT_FALSE(IsDistilledPage(GURL("chrome-distiller://any/invalid")));
+  EXPECT_FALSE(
+      IsDistilledPage(GURL("chrome-distiller://any/?time=123&url=abc")));
+
+  EXPECT_FALSE(IsDistilledPage(GetDistillerViewUrlFromUrl(
+      "not-distiller", GURL("http://example.com/"), "title")));
+  EXPECT_FALSE(IsDistilledPage(GetDistillerViewUrlFromUrl(
+      kDomDistillerScheme, GURL("not-http://example.com/"), "title")));
+
+  EXPECT_TRUE(IsDistilledPage(GetDistillerViewUrlFromUrl(
+      kDomDistillerScheme, GURL("http://example.com/"), "title")));
+  EXPECT_TRUE(IsDistilledPage(GetDistillerViewUrlFromUrl(
+      kDomDistillerScheme, GURL("http://www.example.com/page.html"), "title")));
+  EXPECT_TRUE(IsDistilledPage(GetDistillerViewUrlFromUrl(
+      kDomDistillerScheme,
+      GURL("http://www.example.com/page.html?cats=1&dogs=2"), "title")));
+  EXPECT_TRUE(IsDistilledPage(GetDistillerViewUrlFromUrl(
+      kDomDistillerScheme, GURL("https://example.com/?params=any"), "title")));
+}
 }  // namespace url_utils
 
 }  // namespace dom_distiller

components/omnibox/browser/location_bar_model_impl.cc

@@ -96,7 +96,9 @@ base::string16 LocationBarModelImpl::GetFormattedURL(
   // 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)) {
+  // Note: if the URL begins with dom-distiller:// but is invalid we display it
+  // as-is because it cannot be transformed into an article URL.
+  if (dom_distiller::url_utils::IsDistilledPage(url)) {
     // 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;

components/omnibox/browser/location_bar_model_impl_unittest.cc

@@ -129,12 +129,12 @@ TEST_F(LocationBarModelImplTest, FormatsReaderModeUrls) {
   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
+#if defined(OS_IOS)
   EXPECT_EQ(base::ASCIIToUTF16("example.com/TestSuffix"), originalDisplayUrl);
-#else
+#else   // #!defined(OS_IOS)
   EXPECT_EQ(base::ASCIIToUTF16("example.com/article.html/TestSuffix"),
             originalDisplayUrl);
-#endif
+#endif  // #defined (OS_IOS)
   EXPECT_EQ(base::ASCIIToUTF16("www.example.com/article.html/TestSuffix"),
             originalFormattedFullUrl);
 
@@ -156,6 +156,21 @@ TEST_F(LocationBarModelImplTest, FormatsReaderModeUrls) {
   delegate()->SetURL(distilled);
   EXPECT_EQ(originalDisplayUrl, model()->GetURLForDisplay());
   EXPECT_EQ(originalFormattedFullUrl, model()->GetFormattedFullURL());
+
+  // Invalid dom-distiller:// URLs should be shown, because they do not
+  // correspond to any article.
+  delegate()->SetURL(GURL(("chrome-distiller://abc/?url=invalid")));
+#if defined(OS_IOS)
+  EXPECT_EQ(base::ASCIIToUTF16("chrome-distiller://abc/TestSuffix"),
+            model()->GetURLForDisplay());
+#else   // #!defined(OS_IOS)
+  EXPECT_EQ(
+      base::ASCIIToUTF16("chrome-distiller://abc/?url=invalid/TestSuffix"),
+      model()->GetURLForDisplay());
+#endif  // #defined (OS_IOS)
+  EXPECT_EQ(
+      base::ASCIIToUTF16("chrome-distiller://abc/?url=invalid/TestSuffix"),
+      model()->GetFormattedFullURL());
 }
 
 // TODO(https://crbug.com/1010418): Fix flakes on linux_chromium_asan_rel_ng and

components/omnibox/browser/omnibox_edit_model.cc

@@ -390,10 +390,10 @@ void OmniboxEditModel::AdjustTextForCopy(int sel_min,
     // If the omnibox is displaying a URL, set the hyperlink text to the URL's
     // spec. This undoes any URL elisions.
     if (!controller()->GetLocationBarModel()->GetDisplaySearchTerms(nullptr)) {
-      // Don't let users copy Reader Mode ("chrome-distiller://") URLs.
+      // Don't let users copy Reader Mode page URLs.
       // We display the original article's URL in the omnibox, so users will
       // expect that to be what is copied to the clipboard.
-      if (url_from_text->SchemeIs(dom_distiller::kDomDistillerScheme)) {
+      if (dom_distiller::url_utils::IsDistilledPage(*url_from_text)) {
         *url_from_text =
             dom_distiller::url_utils::GetOriginalUrlFromDistillerUrl(
                 *url_from_text);