Strip the trailing slash from URLs like "http://google.com/". This especially helps when the scheme has also been stripped, as it makes the hostname look less unbalanced. We're careful to avoid stripping the slash when doing so would confuse the omnibox.

This also moves to more aggressive stripping and/or unescaping in several places.  In general, it seems like we should be as aggressive as is feasible.

BUG=43587
TEST=Visit google.com.  There should be no slash in the address bar.
Review URL: http://codereview.chromium.org/2389002

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@48691 0039d316-1c4b-4281-b951-d872f2087c98

16 files changed, 100 insertions, 60 deletions

diff --git a/chrome/browser/autocomplete/autocomplete.cc b/chrome/browser/autocomplete/autocomplete.cc
index 035a1ea..d92677e 100644
--- a/chrome/browser/autocomplete/autocomplete.cc
+++ b/chrome/browser/autocomplete/autocomplete.cc
@@ -90,8 +90,6 @@ AutocompleteInput::Type AutocompleteInput::Parse(
     const std::wstring& desired_tld,
     url_parse::Parsed* parts,
     std::wstring* scheme) {
-  DCHECK(parts);
-
   const size_t first_non_white = text.find_first_not_of(kWhitespaceWide, 0);
   if (first_non_white == std::wstring::npos)
     return INVALID;  // All whitespace.
@@ -106,6 +104,9 @@ AutocompleteInput::Type AutocompleteInput::Parse(
   // use the URLFixerUpper here because we want to be smart about what we
   // consider a scheme.  For example, we shouldn't consider www.google.com:80
   // to have a scheme.
+  url_parse::Parsed local_parts;
+  if (!parts)
+    parts = &local_parts;
   const std::wstring parsed_scheme(URLFixerUpper::SegmentURL(text, parts));
   if (scheme)
     *scheme = parsed_scheme;
@@ -344,6 +345,19 @@ void AutocompleteInput::ParseForEmphasizeComponents(
   }
 }
 
+// static
+std::wstring AutocompleteInput::FormattedStringWithEquivalentMeaning(
+    const GURL& url,
+    const std::wstring& formatted_url) {
+  if (!net::CanStripTrailingSlash(url))
+    return formatted_url;
+  const std::wstring url_with_path(formatted_url + L"/");
+  return (AutocompleteInput::Parse(formatted_url, std::wstring(), NULL, NULL) ==
+          AutocompleteInput::Parse(url_with_path, std::wstring(), NULL, NULL)) ?
+      formatted_url : url_with_path;
+}
+
+
 bool AutocompleteInput::Equals(const AutocompleteInput& other) const {
   return (text_ == other.text_) &&
          (type_ == other.type_) &&
@@ -591,16 +605,13 @@ void AutocompleteProvider::UpdateStarredStateOfMatches() {
     i->starred = bookmark_model->IsBookmarked(GURL(i->destination_url));
 }
 
-std::wstring AutocompleteProvider::StringForURLDisplay(
-    const GURL& url,
-    bool check_accept_lang,
-    bool trim_http) const {
-  std::wstring languages = (check_accept_lang && profile_) ?
-      profile_->GetPrefs()->GetString(prefs::kAcceptLanguages) : std::wstring();
-  const net::FormatUrlTypes format_types = trim_http ?
-      net::kFormatUrlOmitAll : net::kFormatUrlOmitUsernamePassword;
-  return net::FormatUrl(url, languages, format_types, UnescapeRule::SPACES,
-                        NULL, NULL, NULL);
+std::wstring AutocompleteProvider::StringForURLDisplay(const GURL& url,
+                                                       bool check_accept_lang,
+                                                       bool trim_http) const {
+  return net::FormatUrl(url, (check_accept_lang && profile_) ?
+      profile_->GetPrefs()->GetString(prefs::kAcceptLanguages) : std::wstring(),
+      net::kFormatUrlOmitAll & ~(trim_http ? 0 : net::kFormatUrlOmitHTTP),
+      UnescapeRule::SPACES, NULL, NULL, NULL);
 }
 
 // AutocompleteResult ---------------------------------------------------------
diff --git a/chrome/browser/autocomplete/autocomplete.h b/chrome/browser/autocomplete/autocomplete.h
index e7766dd..4f85cf4 100644
--- a/chrome/browser/autocomplete/autocomplete.h
+++ b/chrome/browser/autocomplete/autocomplete.h
@@ -190,7 +190,8 @@ class AutocompleteInput {
   static std::string TypeToString(Type type);
 
   // Parses |text| and returns the type of input this will be interpreted as.
-  // The components of the input are stored in the output parameter |parts|.
+  // The components of the input are stored in the output parameter |parts|, if
+  // it is non-NULL.
   static Type Parse(const std::wstring& text,
                     const std::wstring& desired_tld,
                     url_parse::Parsed* parts,
@@ -205,6 +206,16 @@ class AutocompleteInput {
                                           url_parse::Component* scheme,
                                           url_parse::Component* host);
 
+  // Code that wants to format URLs with a format flag including
+  // net::kFormatUrlOmitTrailingSlashOnBareHostname risk changing the meaning if
+  // the result is then parsed as AutocompleteInput.  Such code can call this
+  // function with the URL and its formatted string, and it will return a
+  // formatted string with the same meaning as the original URL (i.e. it will
+  // re-append a slash if necessary).
+  static std::wstring FormattedStringWithEquivalentMeaning(
+      const GURL& url,
+      const std::wstring& formatted_url);
+
   // User-provided text to be completed.
   const std::wstring& text() const { return text_; }
 
diff --git a/chrome/browser/autocomplete/autocomplete_edit.cc b/chrome/browser/autocomplete/autocomplete_edit.cc
index de74891..976e2e0 100644
--- a/chrome/browser/autocomplete/autocomplete_edit.cc
+++ b/chrome/browser/autocomplete/autocomplete_edit.cc
@@ -158,9 +158,8 @@ AutocompleteMatch::Type AutocompleteEditModel::CurrentTextType() const {
 
 bool AutocompleteEditModel::GetURLForText(const std::wstring& text,
                                           GURL* url) const {
-  url_parse::Parsed parts;
   const AutocompleteInput::Type type = AutocompleteInput::Parse(
-      UserTextFromDisplayText(text), std::wstring(), &parts, NULL);
+      UserTextFromDisplayText(text), std::wstring(), NULL, NULL);
   if (type != AutocompleteInput::URL)
     return false;
 
diff --git a/chrome/browser/autocomplete/history_contents_provider.cc b/chrome/browser/autocomplete/history_contents_provider.cc
index 5ad20c5..deb35d2 100644
--- a/chrome/browser/autocomplete/history_contents_provider.cc
+++ b/chrome/browser/autocomplete/history_contents_provider.cc
@@ -207,9 +207,11 @@ AutocompleteMatch HistoryContentsProvider::ResultToMatch(
   // Also show star in popup.
   AutocompleteMatch match(this, score, false, MatchInTitle(result) ?
       AutocompleteMatch::HISTORY_TITLE : AutocompleteMatch::HISTORY_BODY);
-  match.fill_into_edit = StringForURLDisplay(result.url(), true, trim_http_);
+  match.contents = StringForURLDisplay(result.url(), true, trim_http_);
+  match.fill_into_edit =
+      AutocompleteInput::FormattedStringWithEquivalentMeaning(result.url(),
+                                                              match.contents);
   match.destination_url = result.url();
-  match.contents = match.fill_into_edit;
   match.contents_class.push_back(
       ACMatchClassification(0, ACMatchClassification::URL));
   match.description = result.title();
diff --git a/chrome/browser/autocomplete/history_url_provider.cc b/chrome/browser/autocomplete/history_url_provider.cc
index 8880a12..703b909 100644
--- a/chrome/browser/autocomplete/history_url_provider.cc
+++ b/chrome/browser/autocomplete/history_url_provider.cc
@@ -256,21 +256,26 @@ AutocompleteMatch HistoryURLProvider::SuggestExactInput(
   const GURL& url = input.canonicalized_url();
   if (url.is_valid()) {
     match.destination_url = url;
-    match.fill_into_edit = StringForURLDisplay(url, false, false);
+
+    // Trim off "http://" if the user didn't type it.
+    // NOTE: We use TrimHttpPrefix() here rather than StringForURLDisplay() to
+    // strip the scheme as we need to know the offset so we can adjust the
+    // |match_location| below.  StringForURLDisplay() and TrimHttpPrefix() have
+    // slightly different behavior as well (the latter will strip even without
+    // two slashes after the scheme).
+    std::wstring display_string(StringForURLDisplay(url, false, false));
+    const size_t offset = trim_http ? TrimHttpPrefix(&display_string) : 0;
+    match.fill_into_edit =
+        AutocompleteInput::FormattedStringWithEquivalentMeaning(url,
+                                                                display_string);
     // NOTE: Don't set match.input_location (to allow inline autocompletion)
     // here, it's surprising and annoying.
-    // Trim off "http://" if the user didn't type it.
-    // Double NOTE: we use TrimHttpPrefix here rather than StringForURLDisplay
-    // to strip the http as we need to know the offset so we can adjust the
-    // match_location below. StringForURLDisplay and TrimHttpPrefix have
-    // slightly different behavior when stripping http as well.
-    const size_t offset = trim_http ? TrimHttpPrefix(&match.fill_into_edit) : 0;
 
     // Try to highlight "innermost" match location.  If we fix up "w" into
     // "www.w.com", we want to highlight the fifth character, not the first.
     // This relies on match.destination_url being the non-prefix-trimmed version
     // of match.contents.
-    match.contents = match.fill_into_edit;
+    match.contents = display_string;
     const Prefix* best_prefix = BestPrefix(match.destination_url, input.text());
     // Because of the vagaries of GURL, it's possible for match.destination_url
     // to not contain the user's input at all.  In this case don't mark anything
@@ -832,13 +837,14 @@ AutocompleteMatch HistoryURLProvider::HistoryMatchToACMatch(
   DCHECK(match.destination_url.is_valid());
   size_t inline_autocomplete_offset =
       history_match.input_location + params->input.text().length();
-  const net::FormatUrlTypes format_types =
-      (params->trim_http && !history_match.match_in_scheme) ?
-      net::kFormatUrlOmitAll : net::kFormatUrlOmitUsernamePassword;
-  match.fill_into_edit = net::FormatUrl(info.url(),
-      match_type == WHAT_YOU_TYPED ? std::wstring() : params->languages,
-      format_types, UnescapeRule::SPACES, NULL, NULL,
-      &inline_autocomplete_offset);
+  const net::FormatUrlTypes format_types = net::kFormatUrlOmitAll &
+      ~((params->trim_http && !history_match.match_in_scheme) ?
+          0 : net::kFormatUrlOmitHTTP);
+  match.fill_into_edit =
+      AutocompleteInput::FormattedStringWithEquivalentMeaning(info.url(),
+      net::FormatUrl(info.url(), match_type == WHAT_YOU_TYPED ?
+      std::wstring() : params->languages, format_types, UnescapeRule::SPACES,
+      NULL, NULL, &inline_autocomplete_offset));
   if (!params->input.prevent_inline_autocomplete())
     match.inline_autocomplete_offset = inline_autocomplete_offset;
   DCHECK((match.inline_autocomplete_offset == std::wstring::npos) ||
diff --git a/chrome/browser/autocomplete/search_provider.cc b/chrome/browser/autocomplete/search_provider.cc
index beda1f3..ba550a6 100644
--- a/chrome/browser/autocomplete/search_provider.cc
+++ b/chrome/browser/autocomplete/search_provider.cc
@@ -754,7 +754,9 @@ AutocompleteMatch SearchProvider::NavigationToMatch(
   // suggestion, non-Search results will suddenly appear.
   if (input_.type() == AutocompleteInput::FORCED_QUERY)
     match.fill_into_edit.assign(L"?");
-  match.fill_into_edit.append(match.contents);
+  match.fill_into_edit.append(
+      AutocompleteInput::FormattedStringWithEquivalentMeaning(navigation.url,
+                                                              match.contents));
   // TODO(pkasting): http://b/1112879 These should perhaps be
   // inline-autocompletable?
 
diff --git a/chrome/browser/cocoa/status_bubble_mac_unittest.mm b/chrome/browser/cocoa/status_bubble_mac_unittest.mm
index 6f4dab4..6a15f8b 100644
--- a/chrome/browser/cocoa/status_bubble_mac_unittest.mm
+++ b/chrome/browser/cocoa/status_bubble_mac_unittest.mm
@@ -1,4 +1,4 @@
-// Copyright (c) 2009 The Chromium Authors. All rights reserved.
+// Copyright (c) 2010 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.
 
@@ -141,7 +141,7 @@ TEST_F(StatusBubbleMacTest, SetURL) {
   EXPECT_TRUE([GetURLText() isEqualToString:@"foopy://"]);
   bubble_->SetURL(GURL("http://www.cnn.com"), L"");
   EXPECT_TRUE(IsVisible());
-  EXPECT_TRUE([GetURLText() isEqualToString:@"www.cnn.com/"]);
+  EXPECT_TRUE([GetURLText() isEqualToString:@"www.cnn.com"]);
 }
 
 // Test hiding bubble that's already hidden.
@@ -160,23 +160,23 @@ TEST_F(StatusBubbleMacTest, SetStatusAndURL) {
   bubble_->SetStatus(L"Status");
   EXPECT_TRUE(IsVisible());
   EXPECT_TRUE([GetBubbleViewText() isEqualToString:@"Status"]);
-  bubble_->SetURL(GURL("http://www.nytimes.com/"), L"");
+  bubble_->SetURL(GURL("http://www.nytimes.com"), L"");
   EXPECT_TRUE(IsVisible());
-  EXPECT_TRUE([GetBubbleViewText() isEqualToString:@"www.nytimes.com/"]);
+  EXPECT_TRUE([GetBubbleViewText() isEqualToString:@"www.nytimes.com"]);
   bubble_->SetURL(GURL(), L"");
   EXPECT_TRUE(IsVisible());
   EXPECT_TRUE([GetBubbleViewText() isEqualToString:@"Status"]);
   bubble_->SetStatus(L"");
   EXPECT_FALSE(IsVisible());
-  bubble_->SetURL(GURL("http://www.nytimes.com/"), L"");
+  bubble_->SetURL(GURL("http://www.nytimes.com"), L"");
   EXPECT_TRUE(IsVisible());
-  EXPECT_TRUE([GetBubbleViewText() isEqualToString:@"www.nytimes.com/"]);
+  EXPECT_TRUE([GetBubbleViewText() isEqualToString:@"www.nytimes.com"]);
   bubble_->SetStatus(L"Status");
   EXPECT_TRUE(IsVisible());
   EXPECT_TRUE([GetBubbleViewText() isEqualToString:@"Status"]);
   bubble_->SetStatus(L"");
   EXPECT_TRUE(IsVisible());
-  EXPECT_TRUE([GetBubbleViewText() isEqualToString:@"www.nytimes.com/"]);
+  EXPECT_TRUE([GetBubbleViewText() isEqualToString:@"www.nytimes.com"]);
   bubble_->SetURL(GURL(), L"");
   EXPECT_FALSE(IsVisible());
 }
diff --git a/chrome/browser/gtk/options/passwords_exceptions_page_gtk.cc b/chrome/browser/gtk/options/passwords_exceptions_page_gtk.cc
index 9405fe1..a5bf522 100644
--- a/chrome/browser/gtk/options/passwords_exceptions_page_gtk.cc
+++ b/chrome/browser/gtk/options/passwords_exceptions_page_gtk.cc
@@ -115,8 +115,7 @@ void PasswordsExceptionsPageGtk::SetExceptionList(
   exception_list_.resize(result.size());
   for (size_t i = 0; i < result.size(); ++i) {
     exception_list_[i] = *result[i];
-    std::wstring formatted = net::FormatUrl(result[i]->origin, languages,
-        net::kFormatUrlOmitAll, UnescapeRule::NONE, NULL, NULL, NULL);
+    std::wstring formatted = net::FormatUrl(result[i]->origin, languages);
     std::string site = WideToUTF8(formatted);
     GtkTreeIter iter;
     gtk_list_store_insert_with_values(exception_list_store_, &iter, (gint) i,
diff --git a/chrome/browser/gtk/options/passwords_page_gtk.cc b/chrome/browser/gtk/options/passwords_page_gtk.cc
index 9ecf89f..90cd7b6 100644
--- a/chrome/browser/gtk/options/passwords_page_gtk.cc
+++ b/chrome/browser/gtk/options/passwords_page_gtk.cc
@@ -159,8 +159,7 @@ void PasswordsPageGtk::SetPasswordList(
   password_list_.resize(result.size());
   for (size_t i = 0; i < result.size(); ++i) {
     password_list_[i] = *result[i];
-    std::wstring formatted = net::FormatUrl(result[i]->origin, languages,
-        net::kFormatUrlOmitAll, UnescapeRule::NONE, NULL, NULL, NULL);
+    std::wstring formatted = net::FormatUrl(result[i]->origin, languages);
     std::string site = WideToUTF8(formatted);
     std::string user = UTF16ToUTF8(result[i]->username_value);
     GtkTreeIter iter;
diff --git a/chrome/browser/gtk/options/url_picker_dialog_gtk.cc b/chrome/browser/gtk/options/url_picker_dialog_gtk.cc
index 860a33f..dd51798 100644
--- a/chrome/browser/gtk/options/url_picker_dialog_gtk.cc
+++ b/chrome/browser/gtk/options/url_picker_dialog_gtk.cc
@@ -198,11 +198,12 @@ std::string UrlPickerDialogGtk::GetURLForPath(GtkTreePath* path) const {
   }
   std::wstring languages =
       profile_->GetPrefs()->GetString(prefs::kAcceptLanguages);
-  // Because the url_field_ is user-editable, we set the URL with
-  // username:password and escaped path and query.
+  // Because this gets parsed by FixupURL(), it's safe to omit the scheme or
+  // trailing slash, and unescape most characters, but we need to not drop any
+  // username/password, or unescape anything that changes the meaning.
   std::wstring formatted = net::FormatUrl(url_table_model_->GetURL(row),
-      languages, net::kFormatUrlOmitNothing, UnescapeRule::NONE, NULL, NULL,
-      NULL);
+      languages, net::kFormatUrlOmitAll & ~net::kFormatUrlOmitUsernamePassword,
+      UnescapeRule::SPACES, NULL, NULL, NULL);
   return WideToUTF8(formatted);
 }
 
diff --git a/chrome/browser/tab_contents/navigation_controller_unittest.cc b/chrome/browser/tab_contents/navigation_controller_unittest.cc
index 658cbee..b60db13 100644
--- a/chrome/browser/tab_contents/navigation_controller_unittest.cc
+++ b/chrome/browser/tab_contents/navigation_controller_unittest.cc
@@ -1623,8 +1623,8 @@ TEST_F(NavigationControllerTest, SameSubframe) {
 // Test view source redirection is reflected in title bar.
 TEST_F(NavigationControllerTest, ViewSourceRedirect) {
   const char kUrl[] = "view-source:http://redirect.to/google.com";
-  const char kResult[] = "http://google.com/";
-  const char kExpected[] = "view-source:http://google.com/";
+  const char kResult[] = "http://google.com";
+  const char kExpected[] = "view-source:http://google.com";
   const GURL url(kUrl);
   const GURL result_url(kResult);
 
diff --git a/chrome/browser/tab_contents/navigation_entry_unittest.cc b/chrome/browser/tab_contents/navigation_entry_unittest.cc
index d742d3e..4ecd1df 100644
--- a/chrome/browser/tab_contents/navigation_entry_unittest.cc
+++ b/chrome/browser/tab_contents/navigation_entry_unittest.cc
@@ -57,8 +57,8 @@ TEST_F(NavigationEntryTest, NavigationEntryURLs) {
   // Setting URL affects virtual_url and GetTitleForDisplay
   entry1_.get()->set_url(GURL("http://www.google.com"));
   EXPECT_EQ(GURL("http://www.google.com"), entry1_.get()->url());
-  EXPECT_EQ(GURL("http://www.google.com/"), entry1_.get()->virtual_url());
-  EXPECT_EQ(ASCIIToUTF16("http://www.google.com/"),
+  EXPECT_EQ(GURL("http://www.google.com"), entry1_.get()->virtual_url());
+  EXPECT_EQ(ASCIIToUTF16("www.google.com"),
             entry1_.get()->GetTitleForDisplay(NULL));
 
   // Title affects GetTitleForDisplay
diff --git a/chrome/browser/tab_contents/web_contents_unittest.cc b/chrome/browser/tab_contents/web_contents_unittest.cc
index 9e95563..682aac6 100644
--- a/chrome/browser/tab_contents/web_contents_unittest.cc
+++ b/chrome/browser/tab_contents/web_contents_unittest.cc
@@ -233,7 +233,7 @@ TEST_F(TabContentsTest, UpdateTitle) {
 
 // Test view source mode for the new tabs page.
 TEST_F(TabContentsTest, NTPViewSource) {
-  const char kUrl[] = "view-source:chrome://newtab/";
+  const char kUrl[] = "view-source:chrome://newtab";
   const GURL kGURL(kUrl);
 
   process()->sink().ClearMessages();
diff --git a/chrome/browser/toolbar_model.cc b/chrome/browser/toolbar_model.cc
index a591e5c..740bc3e9 100644
--- a/chrome/browser/toolbar_model.cc
+++ b/chrome/browser/toolbar_model.cc
@@ -53,8 +53,12 @@ std::wstring ToolbarModel::GetText() const {
       url = GURL(url.scheme() + ":");
     }
   }
-  return net::FormatUrl(url, languages, net::kFormatUrlOmitAll,
-                        UnescapeRule::NORMAL, NULL, NULL, NULL);
+  // 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.
+  return AutocompleteInput::FormattedStringWithEquivalentMeaning(url,
+      net::FormatUrl(url, languages, net::kFormatUrlOmitAll,
+                     UnescapeRule::NORMAL, NULL, NULL, NULL));
 }
 
 ToolbarModel::SecurityLevel ToolbarModel::GetSecurityLevel() const {
diff --git a/chrome/browser/views/bookmark_editor_view.cc b/chrome/browser/views/bookmark_editor_view.cc
index 616000b..f0aa7e9 100644
--- a/chrome/browser/views/bookmark_editor_view.cc
+++ b/chrome/browser/views/bookmark_editor_view.cc
@@ -279,9 +279,12 @@ void BookmarkEditorView::Init() {
     std::wstring languages = profile_
         ? profile_->GetPrefs()->GetString(prefs::kAcceptLanguages)
         : std::wstring();
-    // The following URL is user-editable, so we don't strip anything from it.
+    // Because this gets parsed by FixupURL(), it's safe to omit the scheme or
+    // trailing slash, and unescape most characters, but we need to not drop any
+    // username/password, or unescape anything that changes the meaning.
     url_text = net::FormatUrl(details_.existing_node->GetURL(), languages,
-        net::kFormatUrlOmitNothing, UnescapeRule::NONE, NULL, NULL, NULL);
+        net::kFormatUrlOmitAll & ~net::kFormatUrlOmitUsernamePassword,
+        UnescapeRule::SPACES, NULL, NULL, NULL);
   }
   url_tf_.SetText(url_text);
   url_tf_.SetController(this);
diff --git a/chrome/browser/views/url_picker.cc b/chrome/browser/views/url_picker.cc
index 3d275d4..116075f 100644
--- a/chrome/browser/views/url_picker.cc
+++ b/chrome/browser/views/url_picker.cc
@@ -221,10 +221,13 @@ void UrlPicker::OnSelectionChanged() {
   if (selection >= 0 && selection < url_table_model_->RowCount()) {
     std::wstring languages =
         profile_->GetPrefs()->GetString(prefs::kAcceptLanguages);
-    // Because the url_field_ is user-editable, we don't strip anything.
+    // Because this gets parsed by FixupURL(), it's safe to omit the scheme or
+    // trailing slash, and unescape most characters, but we need to not drop any
+    // username/password, or unescape anything that changes the meaning.
     std::wstring formatted = net::FormatUrl(url_table_model_->GetURL(selection),
-        languages, net::kFormatUrlOmitNothing, UnescapeRule::NONE, NULL, NULL,
-        NULL);
+        languages,
+        net::kFormatUrlOmitAll & ~net::kFormatUrlOmitUsernamePassword,
+        UnescapeRule::SPACES, NULL, NULL, NULL);
     url_field_->SetText(formatted);
     GetDialogClientView()->UpdateDialogButtons();
   }