[Reading List iOS] Display distilled URL instead of entry URL in omnibox

ios/chrome/browser/reading_list/offline_url_utils_unittest.cc

 // Copyright 2016 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "ios/chrome/browser/reading_list/offline_url_utils.h"
 
 #include <string>
 
 #include "base/files/file_path.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/test/gtest_util.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "url/gurl.h"
 
 // Checks the distilled URL for the page is chrome://offline/MD5/page.html;
 TEST(OfflineURLUtilsTest, DistilledURLForPathTest) {
   base::FilePath page_path("MD5/page.html");
-  GURL distilled_url = reading_list::DistilledURLForPath(page_path, GURL());
+  GURL distilled_url =
+      reading_list::OfflineURLForPath(page_path, GURL(), GURL());
   EXPECT_EQ("chrome://offline/MD5/page.html", distilled_url.spec());
 }
 
 // Checks the distilled URL for the page with an onlineURL is
 // chrome://offline/MD5/page.html?virtualURL=encorded%20URL;

[gambard, 2017/01/20 14:49:34]

Update comment

[Olivier, 2017/01/20 16:09:14]

Done.

-TEST(OfflineURLUtilsTest, DistilledURLForPathWithVirtualURLTest) {
+TEST(OfflineURLUtilsTest, OfflineURLForPathWithEntryURLTest) {

[gambard, 2017/01/20 14:49:34]

s/OfflineURLForPathWithEntryURLTest/OfflineURLForPathWithEntryAndVirtualURLTest

[Olivier, 2017/01/20 16:09:14]

Done.

   base::FilePath page_path("MD5/page.html");
-  GURL online_url = GURL("http://foo.bar");
-  GURL distilled_url = reading_list::DistilledURLForPath(page_path, online_url);
-  EXPECT_EQ("chrome://offline/MD5/page.html?virtualURL=http%3A%2F%2Ffoo.bar%2F",
-            distilled_url.spec());
+  GURL entry_url = GURL("http://foo.bar");
+  GURL virtual_url = GURL("http://foo.bar/virtual");
+  GURL distilled_url =
+      reading_list::OfflineURLForPath(page_path, entry_url, virtual_url);
+  EXPECT_EQ(
+      "chrome://offline/MD5/page.html?"
+      "entryURL=http%3A%2F%2Ffoo.bar%2F&"
+      "virtualURL=http%3A%2F%2Ffoo.bar%2Fvirtual",
+      distilled_url.spec());
 }
 
 // Checks the distilled URL for the page is chrome://offline/MD5/page.html;

[gambard, 2017/01/20 14:49:34]

Update this comment.
You are checking that the entry url is the distilled url if there is no
?entryURL=... or I misunderstood the test?

[Olivier, 2017/01/20 16:09:14]

Done.

-TEST(OfflineURLUtilsTest, VirtualURLForDistilledURLTest) {
+TEST(OfflineURLUtilsTest, ParseOfflineURLTest) {
   GURL distilled_url("chrome://offline/MD5/page.html");
-  GURL virtual_url = reading_list::VirtualURLForDistilledURL(distilled_url);
-  EXPECT_EQ("chrome://offline/MD5/page.html", virtual_url.spec());
+  GURL entry_url = reading_list::EntryURLForOfflineURL(distilled_url);
+  EXPECT_EQ("chrome://offline/MD5/page.html", entry_url.spec());
 }
 
 // Checks the distilled URL for the page with an onlineURL is
-// chrome://offline/MD5/page.html?virtualURL=encorded%20URL;
-TEST(OfflineURLUtilsTest, VirtualURLForDistilledURLWithVirtualURLTest) {
-  GURL distilled_url(
+// chrome://offline/MD5/page.html?entryURL=encorded%20URL

[gambard, 2017/01/20 14:49:34]

WDYT of:
// Checks that the entry URL and the virtual URL of the page are the same if
"?virtualURL=" is not present.
or 
// Checks that the entry URL and the virtual URL of the page are the same if the
virtualURL parameter is not present.

[Olivier, 2017/01/20 16:09:14]

Normalized all comments.

+TEST(OfflineURLUtilsTest, ParseOfflineURLWithEntryURLTest) {
+  GURL offline_url(
+      "chrome://offline/MD5/page.html?entryURL=http%3A%2F%2Ffoo.bar%2F");
+  GURL entry_url = reading_list::EntryURLForOfflineURL(offline_url);
+  EXPECT_EQ("http://foo.bar/", entry_url.spec());
+  GURL virtual_url = reading_list::VirtualURLForOfflineURL(offline_url);
+  EXPECT_EQ("http://foo.bar/", virtual_url.spec());
+}
+
+// Checks the distilled URL for the page with an onlineURL is
+// chrome://offline/MD5/page.html?virtualURL=encorded%20URL

[gambard, 2017/01/20 14:49:34]

WDYT:
// Checks that if the only parameter present is virtualURL, the entry URL is the
offline url and the virtual url is the value of the virtualURL parameter.

[Olivier, 2017/01/20 16:09:14]

Normalized all comments.

+TEST(OfflineURLUtilsTest, ParseOfflineURLWithVirtualURLTest) {
+  GURL offline_url(
       "chrome://offline/MD5/page.html?virtualURL=http%3A%2F%2Ffoo.bar%2F");
-  GURL virtual_url = reading_list::VirtualURLForDistilledURL(distilled_url);
+  GURL entry_url = reading_list::EntryURLForOfflineURL(offline_url);
+  EXPECT_EQ(offline_url, entry_url);
+  GURL virtual_url = reading_list::VirtualURLForOfflineURL(offline_url);
   EXPECT_EQ("http://foo.bar/", virtual_url.spec());
 }
 
+// Checks the distilled URL for the page with an onlineURL is
+// chrome://offline/MD5/page.html?entryURL=...&virtualURL=...

[gambard, 2017/01/20 14:49:34]

WDYT:
// Checks that the entry URL and the virtual URL are equals to the value of
their corresponding parameter.

[Olivier, 2017/01/20 16:09:14]

Normalized all comments.

+TEST(OfflineURLUtilsTest, ParseOfflineURLWithVirtualAndEntryURLTest) {
+  GURL offline_url(
+      "chrome://offline/MD5/"
+      "page.html?virtualURL=http%3A%2F%2Ffoo.bar%2Fvirtual&entryURL=http%3A%2F%"
+      "2Ffoo.bar%2Fentry");
+  GURL entry_url = reading_list::EntryURLForOfflineURL(offline_url);
+  EXPECT_EQ("http://foo.bar/entry", entry_url.spec());
+  GURL virtual_url = reading_list::VirtualURLForOfflineURL(offline_url);
+  EXPECT_EQ("http://foo.bar/virtual", virtual_url.spec());
+}
+
 // Checks the file path for chrome://offline/MD5/page.html is
 // file://profile_path/Offline/MD5/page.html.
 // Checks the resource root for chrome://offline/MD5/page.html is
 // file://profile_path/Offline/MD5
 TEST(OfflineURLUtilsTest, FileURLForDistilledURLTest) {
   base::FilePath offline_path("/profile_path/Offline");
   GURL file_url =
       reading_list::FileURLForDistilledURL(GURL(), offline_path, nullptr);
   EXPECT_FALSE(file_url.is_valid());
 
@@ skipping 19 matching lines @@
   EXPECT_FALSE(reading_list::IsOfflineURL(GURL("chrome://offline-foobar")));
   EXPECT_FALSE(reading_list::IsOfflineURL(GURL("http://offline/")));
   EXPECT_FALSE(reading_list::IsOfflineURL(GURL("http://chrome://offline/")));
   EXPECT_TRUE(reading_list::IsOfflineURL(GURL("chrome://offline")));
   EXPECT_TRUE(reading_list::IsOfflineURL(GURL("chrome://offline/")));
   EXPECT_TRUE(reading_list::IsOfflineURL(GURL("chrome://offline/foobar")));
   EXPECT_TRUE(
       reading_list::IsOfflineURL(GURL("chrome://offline/foobar?foo=bar")));
 }