Move the offline URL trimming to GetFormattedURL.

Changing directly the URL may have side effects beside hiding the
scheme.
Move the formatting to GetFormattedURL.
Also refactor ToolbarModelIOS to inherit ToolbarModel.

BUG=674986

Review-Url: https://codereview.chromium.org/2606873002
Cr-Commit-Position: refs/heads/master@{#440955}

ios/chrome/browser/reading_list/offline_url_utils.cc

@@ -4,8 +4,11 @@
 
 #include "ios/chrome/browser/reading_list/offline_url_utils.h"
 
+#include "base/logging.h"
 #include "base/md5.h"
+#include "base/strings/string_util.h"
 #include "base/strings/stringprintf.h"
+#include "base/strings/utf_string_conversions.h"
 #include "components/reading_list/ios/offline_url_utils.h"
 #include "ios/chrome/browser/chrome_url_constants.h"
 #include "net/base/url_util.h"
@@ -63,4 +66,27 @@ GURL FileURLForDistilledURL(const GURL& distilled_url,
 bool IsOfflineURL(const GURL& url) {
   return url.SchemeIs(kChromeUIScheme) && url.host() == kChromeUIOfflineHost;
 }
+
+base::string16 StripSchemeFromOnlineURL(const base::string16& online_url,
+                                        size_t* removed_chars) {
+  base::string16 https_scheme = base::UTF8ToUTF16(base::StringPrintf(
+      "%s%s", url::kHttpsScheme, url::kStandardSchemeSeparator));
+  if (base::StartsWith(online_url, https_scheme,
+                       base::CompareCase::SENSITIVE)) {
+    if (removed_chars) {
+      *removed_chars = https_scheme.length();
+    }
+    return online_url.substr(https_scheme.length());
+  }
+  // http:// scheme should already have been trimmed at this point.
+  // DCHECK to detect formatting changes in omnibox.
+  DCHECK(!base::StartsWith(
+      online_url, base::UTF8ToUTF16(base::StringPrintf(
+                      "%s%s", url::kHttpScheme, url::kStandardSchemeSeparator)),
+      base::CompareCase::SENSITIVE));
+  if (removed_chars) {
+    *removed_chars = 0;
+  }
+  return online_url;
+}
 }

ios/chrome/browser/reading_list/offline_url_utils.h

@@ -8,6 +8,7 @@
 #include <string>
 
 #include "base/files/file_path.h"
+#include "base/strings/string16.h"
 #include "url/gurl.h"
 
 namespace reading_list {
@@ -30,6 +31,13 @@ GURL FileURLForDistilledURL(const GURL& distilled_url,
 // Returns whether the URL points to a chrome offline URL.
 bool IsOfflineURL(const GURL& url);
 
+// Strips scheme from the original URL of the offline page. This is meant to be
+// used by UI.
+// If |removed_chars| is non-NULL, it is set to the number of chars that have
+// been removed at the begining of |online_url|.
+base::string16 StripSchemeFromOnlineURL(const base::string16& online_url,
+                                        size_t* removed_chars);
+
 }  // namespace reading_list
 
 #endif  // IOS_CHROME_BROWSER_READING_LIST_OFFLINE_URL_UTILS_H_

ios/chrome/browser/reading_list/offline_url_utils_unittest.cc

@@ -7,6 +7,8 @@
 #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"
 
@@ -68,6 +70,7 @@ TEST(OfflineURLUtilsTest, FileURLForDistilledURLTest) {
   EXPECT_EQ("/profile_path/Offline/MD5/", resource_url.path());
 }
 
+// Checks that the offline URLs are correctly detected by |IsOfflineURL|.
 TEST(OfflineURLUtilsTest, IsOfflineURL) {
   EXPECT_FALSE(reading_list::IsOfflineURL(GURL()));
   EXPECT_FALSE(reading_list::IsOfflineURL(GURL("chrome://")));
@@ -80,3 +83,35 @@ TEST(OfflineURLUtilsTest, IsOfflineURL) {
   EXPECT_TRUE(
       reading_list::IsOfflineURL(GURL("chrome://offline/foobar?foo=bar")));
 }
+
+// Checks that https:// scheme is correctly removed by
+// StripSchemeFromOnlineURLTest.
+TEST(OfflineURLUtilsTest, StripSchemeFromOnlineURLTest) {
+  size_t removed_size;
+  base::string16 empty_url;
+  EXPECT_EQ(reading_list::StripSchemeFromOnlineURL(empty_url, &removed_size),
+            empty_url);
+  EXPECT_EQ(removed_size, 0u);
+
+  base::string16 https_url = base::UTF8ToUTF16("https://www.chromium.org/");
+  base::string16 trimmed_https_url = base::UTF8ToUTF16("www.chromium.org/");
+  EXPECT_EQ(reading_list::StripSchemeFromOnlineURL(https_url, &removed_size),
+            trimmed_https_url);
+  EXPECT_EQ(removed_size, 8u);
+  base::string16 http_url = base::UTF8ToUTF16("http://www.chromium.org/");
+  EXPECT_DCHECK_DEATH(
+      reading_list::StripSchemeFromOnlineURL(http_url, &removed_size));
+
+  base::string16 other_scheme_url =
+      base::UTF8ToUTF16("scheme://www.chromium.org/");
+  EXPECT_EQ(
+      reading_list::StripSchemeFromOnlineURL(other_scheme_url, &removed_size),
+      other_scheme_url);
+  EXPECT_EQ(removed_size, 0u);
+
+  base::string16 no_scheme_url = base::UTF8ToUTF16("www.chromium.org/");
+  EXPECT_EQ(
+      reading_list::StripSchemeFromOnlineURL(no_scheme_url, &removed_size),
+      no_scheme_url);
+  EXPECT_EQ(removed_size, 0u);
+}

ios/chrome/browser/ui/toolbar/BUILD.gn

@@ -167,10 +167,12 @@ source_set("unit_tests") {
     "//ios/chrome/browser",
     "//ios/chrome/browser/bookmarks",
     "//ios/chrome/browser/browser_state:test_support",
+    "//ios/chrome/browser/ssl",
     "//ios/chrome/browser/tabs",
     "//ios/chrome/browser/ui",
     "//ios/chrome/test:test_support",
     "//ios/testing:ocmock_support",
+    "//ios/web",
     "//ios/web:test_support",
     "//testing/gtest",
     "//third_party/ocmock",

ios/chrome/browser/ui/toolbar/test_toolbar_model_ios.h

@@ -18,8 +18,10 @@ class TestToolbarModelIOS : public ToolbarModelIOS {
   TestToolbarModelIOS();
   ~TestToolbarModelIOS() override;
 
+  // Getter to the TestToolbarModel to set its values in tests.
+  TestToolbarModel* GetToolbarModel();
+
   // ToolbarModelIOS implementation:
-  ToolbarModel* GetToolbarModel() override;
   bool IsLoading() override;
   CGFloat GetLoadProgressFraction() override;
   bool CanGoBack() override;
@@ -29,6 +31,15 @@ class TestToolbarModelIOS : public ToolbarModelIOS {
   bool IsCurrentTabBookmarkedByUser() override;
   bool ShouldDisplayHintText() override;
 
+  base::string16 GetFormattedURL(size_t* prefix_end) const override;
+  GURL GetURL() const override;
+  security_state::SecurityLevel GetSecurityLevel(
+      bool ignore_editing) const override;
+  gfx::VectorIconId GetVectorIcon() const override;
+  base::string16 GetSecureVerboseText() const override;
+  base::string16 GetEVCertName() const override;
+  bool ShouldDisplayURL() const override;
+
   void set_is_loading(bool is_loading) { is_loading_ = is_loading; }
   void set_load_progress_fraction(CGFloat load_progress_fraction) {
     load_progress_fraction_ = load_progress_fraction;

ios/chrome/browser/ui/toolbar/test_toolbar_model_ios.mm

@@ -20,7 +20,7 @@ TestToolbarModelIOS::TestToolbarModelIOS()
 
 TestToolbarModelIOS::~TestToolbarModelIOS() {}
 
-ToolbarModel* TestToolbarModelIOS::GetToolbarModel() {
+TestToolbarModel* TestToolbarModelIOS::GetToolbarModel() {
   return test_toolbar_model_.get();
 }
 
@@ -55,3 +55,32 @@ bool TestToolbarModelIOS::IsCurrentTabBookmarkedByUser() {
 bool TestToolbarModelIOS::ShouldDisplayHintText() {
   return false;
 }
+
+base::string16 TestToolbarModelIOS::GetFormattedURL(size_t* prefix_end) const {
+  return test_toolbar_model_->GetFormattedURL(prefix_end);
+}
+
+GURL TestToolbarModelIOS::GetURL() const {
+  return test_toolbar_model_->GetURL();
+}
+
+security_state::SecurityLevel TestToolbarModelIOS::GetSecurityLevel(
+    bool ignore_editing) const {
+  return test_toolbar_model_->GetSecurityLevel(ignore_editing);
+}
+
+gfx::VectorIconId TestToolbarModelIOS::GetVectorIcon() const {
+  return test_toolbar_model_->GetVectorIcon();
+}
+
+base::string16 TestToolbarModelIOS::GetSecureVerboseText() const {
+  return test_toolbar_model_->GetSecureVerboseText();
+}
+
+base::string16 TestToolbarModelIOS::GetEVCertName() const {
+  return test_toolbar_model_->GetEVCertName();
+}
+
+bool TestToolbarModelIOS::ShouldDisplayURL() const {
+  return test_toolbar_model_->ShouldDisplayURL();
+}

ios/chrome/browser/ui/toolbar/toolbar_model_delegate_ios.mm

@@ -13,7 +13,6 @@
 #include "ios/chrome/browser/browser_state/chrome_browser_state.h"
 #include "ios/chrome/browser/chrome_url_constants.h"
 #include "ios/chrome/browser/pref_names.h"
-#include "ios/chrome/browser/reading_list/offline_url_utils.h"
 #include "ios/chrome/browser/ssl/ios_security_state_tab_helper.h"
 #import "ios/chrome/browser/tabs/tab.h"
 #import "ios/chrome/browser/tabs/tab_model.h"
@@ -53,25 +52,7 @@ bool ToolbarModelDelegateIOS::GetURL(GURL* url) const {
   web::NavigationItem* item = GetNavigationItem();
   if (!item)
     return false;
-
-  if (!ShouldDisplayURL()) {
-    *url = GURL();
-    return true;
-  }
-
-  // For security reasons, we shouldn't display the https scheme and secure
-  // padlock when there's no active ssl session.
-  // To hide the scheme we set it http when the loaded url is offline.
-  if (reading_list::IsOfflineURL(item->GetURL()) &&
-      item->GetVirtualURL().SchemeIs(url::kHttpsScheme)) {
-    GURL::Replacements replacements;
-    replacements.SetScheme(url::kHttpScheme,
-                           url::Component(0, strlen(url::kHttpScheme)));
-    *url = item->GetVirtualURL().ReplaceComponents(replacements);
-    return true;
-  }
-
-  *url = item->GetVirtualURL();
+  *url = ShouldDisplayURL() ? item->GetVirtualURL() : GURL::EmptyGURL();
   return true;
 }
 

ios/chrome/browser/ui/toolbar/toolbar_model_impl_ios.h

@@ -18,7 +18,6 @@ class ToolbarModelImplIOS : public ToolbarModelIOS {
   ~ToolbarModelImplIOS() override;
 
   // ToolbarModelIOS implementation:
-  ToolbarModel* GetToolbarModel() override;
   bool IsLoading() override;
   CGFloat GetLoadProgressFraction() override;
   bool CanGoBack() override;
@@ -28,6 +27,17 @@ class ToolbarModelImplIOS : public ToolbarModelIOS {
   bool IsCurrentTabBookmarkedByUser() override;
   bool ShouldDisplayHintText() override;
 
+ private:
+  // ToolbarModel:
+  base::string16 GetFormattedURL(size_t* prefix_end) const override;
+  GURL GetURL() const override;
+  security_state::SecurityLevel GetSecurityLevel(
+      bool ignore_editing) const override;
+  gfx::VectorIconId GetVectorIcon() const override;
+  base::string16 GetSecureVerboseText() const override;
+  base::string16 GetEVCertName() const override;
+  bool ShouldDisplayURL() const override;
+
  private:
   ToolbarModelDelegateIOS* delegate_;
   std::unique_ptr<ToolbarModel> toolbar_model_;

ios/chrome/browser/ui/toolbar/toolbar_model_impl_ios.mm

@@ -9,8 +9,10 @@
 #include "ios/chrome/browser/bookmarks/bookmark_model_factory.h"
 #include "ios/chrome/browser/browser_state/chrome_browser_state.h"
 #include "ios/chrome/browser/chrome_url_constants.h"
+#include "ios/chrome/browser/reading_list/offline_url_utils.h"
 #import "ios/chrome/browser/tabs/tab.h"
 #include "ios/chrome/browser/ui/toolbar/toolbar_model_delegate_ios.h"
+#import "ios/web/public/navigation_item.h"
 #import "ios/web/public/web_state/web_state.h"
 
 namespace {
@@ -35,10 +37,6 @@ ToolbarModelImplIOS::ToolbarModelImplIOS(ToolbarModelDelegateIOS* delegate) {
 
 ToolbarModelImplIOS::~ToolbarModelImplIOS() {}
 
-ToolbarModel* ToolbarModelImplIOS::GetToolbarModel() {
-  return toolbar_model_.get();
-}
-
 bool ToolbarModelImplIOS::IsLoading() {
   // Please note, ToolbarModel's notion of isLoading is slightly different from
   // WebState's IsLoading().
@@ -60,25 +58,68 @@ bool ToolbarModelImplIOS::CanGoForward() {
 }
 
 bool ToolbarModelImplIOS::IsCurrentTabNativePage() {
-  Tab* currentTab = delegate_->GetCurrentTab();
-  return currentTab && currentTab.url.SchemeIs(kChromeUIScheme);
+  Tab* current_tab = delegate_->GetCurrentTab();
+  return current_tab && current_tab.url.SchemeIs(kChromeUIScheme);
 }
 
 bool ToolbarModelImplIOS::IsCurrentTabBookmarked() {
-  Tab* currentTab = delegate_->GetCurrentTab();
-  bookmarks::BookmarkModel* bookmarkModel = GetBookmarkModelForTab(currentTab);
-  return currentTab && bookmarkModel &&
-         bookmarkModel->IsBookmarked(currentTab.url);
+  Tab* current_tab = delegate_->GetCurrentTab();
+  bookmarks::BookmarkModel* bookmarkModel = GetBookmarkModelForTab(current_tab);
+  return current_tab && bookmarkModel &&
+         bookmarkModel->IsBookmarked(current_tab.url);
 }
 
 bool ToolbarModelImplIOS::IsCurrentTabBookmarkedByUser() {
-  Tab* currentTab = delegate_->GetCurrentTab();
-  bookmarks::BookmarkModel* bookmarkModel = GetBookmarkModelForTab(currentTab);
-  return currentTab && bookmarkModel &&
-         bookmarkModel->GetMostRecentlyAddedUserNodeForURL(currentTab.url);
+  Tab* current_tab = delegate_->GetCurrentTab();
+  bookmarks::BookmarkModel* bookmarkModel = GetBookmarkModelForTab(current_tab);
+  return current_tab && bookmarkModel &&
+         bookmarkModel->GetMostRecentlyAddedUserNodeForURL(current_tab.url);
 }
 
 bool ToolbarModelImplIOS::ShouldDisplayHintText() {
-  Tab* currentTab = delegate_->GetCurrentTab();
-  return [currentTab.webController wantsLocationBarHintText];
+  Tab* current_tab = delegate_->GetCurrentTab();
+  return [current_tab.webController wantsLocationBarHintText];
+}
+
+base::string16 ToolbarModelImplIOS::GetFormattedURL(size_t* prefix_end) const {
+  base::string16 formatted_url = toolbar_model_->GetFormattedURL(prefix_end);
+  Tab* current_tab = delegate_->GetCurrentTab();
+  GURL url =
+      current_tab.webState->GetNavigationManager()->GetVisibleItem()->GetURL();
+  if (reading_list::IsOfflineURL(url) &&
+      GetSecurityLevel(true /*ignore_editing*/) ==
+          security_state::SecurityLevel::NONE) {
+    size_t removed = 0;
+    formatted_url =
+        reading_list::StripSchemeFromOnlineURL(formatted_url, &removed);
+    if (prefix_end) {
+      *prefix_end -= removed;
+    }
+  }
+  return formatted_url;
+}
+
+GURL ToolbarModelImplIOS::GetURL() const {
+  return toolbar_model_->GetURL();
+}
+
+security_state::SecurityLevel ToolbarModelImplIOS::GetSecurityLevel(
+    bool ignore_editing) const {
+  return toolbar_model_->GetSecurityLevel(ignore_editing);
+}
+
+gfx::VectorIconId ToolbarModelImplIOS::GetVectorIcon() const {
+  return toolbar_model_->GetVectorIcon();
+}
+
+base::string16 ToolbarModelImplIOS::GetSecureVerboseText() const {
+  return toolbar_model_->GetSecureVerboseText();
+}
+
+base::string16 ToolbarModelImplIOS::GetEVCertName() const {
+  return toolbar_model_->GetEVCertName();
+}
+
+bool ToolbarModelImplIOS::ShouldDisplayURL() const {
+  return toolbar_model_->ShouldDisplayURL();
 }

ios/chrome/browser/ui/toolbar/toolbar_model_impl_ios_unittest.mm

@@ -7,6 +7,7 @@
 #include <memory>
 
 #include "base/mac/scoped_nsobject.h"
+#include "base/memory/ptr_util.h"
 #include "base/message_loop/message_loop.h"
 #include "base/strings/utf_string_conversions.h"
 #include "components/bookmarks/browser/bookmark_model.h"
@@ -14,13 +15,16 @@
 #include "components/toolbar/test_toolbar_model.h"
 #include "ios/chrome/browser/bookmarks/bookmark_model_factory.h"
 #include "ios/chrome/browser/browser_state/test_chrome_browser_state.h"
+#include "ios/chrome/browser/ssl/ios_security_state_tab_helper.h"
 #import "ios/chrome/browser/tabs/tab.h"
 #import "ios/chrome/browser/tabs/tab_model.h"
 #include "ios/chrome/browser/ui/toolbar/toolbar_model_delegate_ios.h"
 #include "ios/chrome/browser/ui/toolbar/toolbar_model_impl_ios.h"
 #import "ios/chrome/browser/xcallback_parameters.h"
 #import "ios/testing/ocmock_complex_type_helper.h"
-#include "ios/web/public/test/test_web_state.h"
+#import "ios/web/public/navigation_item.h"
+#import "ios/web/public/test/test_navigation_manager.h"
+#import "ios/web/public/test/test_web_state.h"
 #include "ios/web/public/test/test_web_thread.h"
 #include "ios/web/public/test/test_web_thread_bundle.h"
 #include "testing/gtest_mac.h"
@@ -217,4 +221,58 @@ TEST_F(ToolbarModelImplIOSTest, TestIsCurrentTabBookmarked) {
   EXPECT_FALSE(toolbarModel_->IsCurrentTabBookmarked());
 }
 
+TEST_F(ToolbarModelImplIOSTest, TestGetFormattedURL) {
+  ToolbarModelImplIOSTestWebState web_state(chrome_browser_state_.get());
+  IOSSecurityStateTabHelper::CreateForWebState(&web_state);
+  auto test_navigation_manager = base::MakeUnique<web::TestNavigationManager>();
+  auto visible_item = web::NavigationItem::Create();
+  test_navigation_manager->SetVisibleItem(visible_item.get());
+  web_state.SetNavigationManager(std::move(test_navigation_manager));
+  id tabMock = [[TMITestTabMock alloc]
+      initWithRepresentedObject:[OCMockObject mockForClass:[Tab class]]];
+  OCMockObject* tabModelMock = static_cast<OCMockObject*>(tabModel_.get());
+  [[[tabModelMock stub] andReturn:tabMock] currentTab];
+  [static_cast<TMITestTabMock*>(tabMock) setWebState:&web_state];
+
+  size_t length = 0;
+  const char no_scheme_url[] = "www.chromium.org";
+  const char http_url[] = "http://www.chromium.org";
+  const char https_url[] = "https://www.chromium.org";
+  const char chrome_url[] = "chrome://www.chromium.org";
+  const char offline_url[] = "chrome://offline/testid/page.html";
+  // Test that only http:// scheme is stripped out for online URL.
+  visible_item->SetURL(GURL(http_url));
+  EXPECT_EQ(toolbarModel_->GetFormattedURL(&length),
+            base::UTF8ToUTF16(no_scheme_url));
+  EXPECT_EQ(length, 0u);
+
+  visible_item->SetURL(GURL(https_url));
+  EXPECT_EQ(toolbarModel_->GetFormattedURL(&length),
+            base::UTF8ToUTF16(https_url));
+  EXPECT_EQ(length, 8u);
+
+  visible_item->SetURL(GURL(chrome_url));
+  EXPECT_EQ(toolbarModel_->GetFormattedURL(&length),
+            base::UTF8ToUTF16(chrome_url));
+  EXPECT_EQ(length, 9u);
+
+  // Test that only http:// and https:// scheme are stripped out for offline
+  // URL.
+  visible_item->SetURL(GURL(offline_url));
+  visible_item->SetVirtualURL(GURL(http_url));
+  EXPECT_EQ(toolbarModel_->GetFormattedURL(&length),
+            base::UTF8ToUTF16(no_scheme_url));
+  EXPECT_EQ(length, 0u);
+
+  visible_item->SetVirtualURL(GURL(https_url));
+  EXPECT_EQ(toolbarModel_->GetFormattedURL(&length),
+            base::UTF8ToUTF16(no_scheme_url));
+  EXPECT_EQ(length, 0u);
+
+  visible_item->SetVirtualURL(GURL(chrome_url));
+  EXPECT_EQ(toolbarModel_->GetFormattedURL(&length),
+            base::UTF8ToUTF16(chrome_url));
+  EXPECT_EQ(length, 9u);
+}
+
 }  // namespace

ios/chrome/browser/ui/toolbar/toolbar_model_ios.h

@@ -9,13 +9,8 @@
 
 #include "components/toolbar/toolbar_model.h"
 
-class ToolbarModelIOS {
+class ToolbarModelIOS : public ToolbarModel {
  public:
-  virtual ~ToolbarModelIOS() {}
-
-  // Returns the |ToolbarModel| contained by this instance.
-  virtual ToolbarModel* GetToolbarModel() = 0;
-
   // Returns true if the current tab is currently loading.
   virtual bool IsLoading() = 0;
 

ios/chrome/browser/ui/toolbar/web_toolbar_controller.mm

@@ -1318,8 +1318,7 @@ CGRect RectShiftedDownAndResizedForStatusBar(CGRect rect) {
 }
 
 - (ToolbarModel*)toolbarModel {
-  ToolbarModelIOS* toolbarModelIOS = [self.delegate toolbarModelIOS];
-  return toolbarModelIOS ? toolbarModelIOS->GetToolbarModel() : nullptr;
+  return [self.delegate toolbarModelIOS];
 }
 
 #pragma mark -

ios/web/public/test/test_navigation_manager.h

@@ -43,10 +43,12 @@ class TestNavigationManager : public web::NavigationManager {
   // Setters for test data.
   void SetLastCommittedItem(NavigationItem* item);
   void SetPendingItem(NavigationItem* item);
+  void SetVisibleItem(NavigationItem* item);
 
  private:
   NavigationItem* pending_item_;
   NavigationItem* last_committed_item_;
+  NavigationItem* visible_item_;
 };
 
 }  // namespace web

ios/web/public/test/test_navigation_manager.mm

@@ -21,8 +21,11 @@ WebState* TestNavigationManager::GetWebState() const {
 }
 
 NavigationItem* TestNavigationManager::GetVisibleItem() const {
-  NOTREACHED();
-  return nullptr;
+  return visible_item_;
+}
+
+void TestNavigationManager::SetVisibleItem(NavigationItem* item) {
+  visible_item_ = item;
 }
 
 NavigationItem* TestNavigationManager::GetLastCommittedItem() const {