The search terms are escaped using + or %20 for space depending on whether replacement is in query part of the URL or not.

Removed duplicate EscapeQueryParamValue functions without |use_plus| argument.

BUG=24571
TEST=Verify that space is escaped as stated in description; see bug description for example with search on Wikipedia.
Review URL: http://codereview.chromium.org/543077

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

11 files changed, 82 insertions, 42 deletions

diff --git a/chrome/browser/dom_ui/history_ui.cc b/chrome/browser/dom_ui/history_ui.cc
index 0d78ddd..df4ef27 100644
--- a/chrome/browser/dom_ui/history_ui.cc
+++ b/chrome/browser/dom_ui/history_ui.cc
@@ -377,7 +377,7 @@ HistoryUI::HistoryUI(TabContents* contents) : DOMUI(contents) {
 // static
 const GURL HistoryUI::GetHistoryURLWithSearchText(const std::wstring& text) {
   return GURL(std::string(chrome::kChromeUIHistoryURL) + "#q=" +
-              EscapeQueryParamValue(WideToUTF8(text)));
+              EscapeQueryParamValue(WideToUTF8(text), true));
 }
 
 // static
diff --git a/chrome/browser/extensions/extension_updater.cc b/chrome/browser/extensions/extension_updater.cc
index 9228905..da55735 100644
--- a/chrome/browser/extensions/extension_updater.cc
+++ b/chrome/browser/extensions/extension_updater.cc
@@ -472,7 +472,7 @@ void AppendExtensionInfo(std::string* str, const Extension& extension) {
     parts.push_back("v=" + version->GetString());
     parts.push_back("uc");
 
-    str->append("x=" + EscapeQueryParamValue(JoinString(parts, '&')));
+    str->append("x=" + EscapeQueryParamValue(JoinString(parts, '&'), true));
 }
 
 // Creates a blacklist update url.
@@ -480,7 +480,7 @@ GURL ExtensionUpdater::GetBlacklistUpdateUrl(const std::wstring& version) {
   std::string blklist_info = StringPrintf("id=%s&v=%s&uc", kBlacklistAppID,
       WideToASCII(version).c_str());
   return GURL(StringPrintf("%s?x=%s", kBlacklistUpdateUrl,
-                           EscapeQueryParamValue(blklist_info).c_str()));
+                           EscapeQueryParamValue(blklist_info, true).c_str()));
 }
 
 void ExtensionUpdater::ScheduleNextCheck(const TimeDelta& target_delay) {
@@ -575,7 +575,7 @@ void ExtensionUpdater::CheckNow() {
       std::string uid = uid_provider_->GetUidString();
       if (uid.length() > 0 && uid.length() <= UidProvider::maxUidLength) {
         full_url_string.append("&" + std::string(ExtensionUpdater::kUidKey) +
-                               "=" + EscapeQueryParamValue(uid));
+                               "=" + EscapeQueryParamValue(uid, true));
       }
     }
 
diff --git a/chrome/browser/safe_browsing/protocol_manager.cc b/chrome/browser/safe_browsing/protocol_manager.cc
index 42c6158..d47d4d1 100644
--- a/chrome/browser/safe_browsing/protocol_manager.cc
+++ b/chrome/browser/safe_browsing/protocol_manager.cc
@@ -619,9 +619,9 @@ void SafeBrowsingProtocolManager::ReportMalware(const GURL& malware_url,
                                                 const GURL& referrer_url) {
   std::string report_str = StringPrintf(
       kSbMalwareReportUrl,
-      EscapeQueryParamValue(malware_url.spec()).c_str(),
-      EscapeQueryParamValue(page_url.spec()).c_str(),
-      EscapeQueryParamValue(referrer_url.spec()).c_str(),
+      EscapeQueryParamValue(malware_url.spec(), true).c_str(),
+      EscapeQueryParamValue(page_url.spec(), true).c_str(),
+      EscapeQueryParamValue(referrer_url.spec(), true).c_str(),
       client_name_.c_str(),
       version_.c_str());
   GURL report_url(report_str);
diff --git a/chrome/browser/safe_browsing/safe_browsing_blocking_page.cc b/chrome/browser/safe_browsing/safe_browsing_blocking_page.cc
index 04aa1b1..114473c 100644
--- a/chrome/browser/safe_browsing/safe_browsing_blocking_page.cc
+++ b/chrome/browser/safe_browsing/safe_browsing_blocking_page.cc
@@ -375,7 +375,7 @@ void SafeBrowsingBlockingPage::CommandReceived(const std::string& cmd) {
     // We're going to take the user to Google's SafeBrowsing diagnostic page.
     std::string diagnostic =
         StringPrintf(kSbDiagnosticUrl,
-                     EscapeQueryParamValue(bad_url_spec).c_str());
+                     EscapeQueryParamValue(bad_url_spec, true).c_str());
     GURL diagnostic_url(diagnostic);
     diagnostic_url = google_util::AppendGoogleLocaleParam(diagnostic_url);
     DCHECK(unsafe_resources_[element_index].threat_type ==
diff --git a/chrome/browser/safe_browsing/safe_browsing_util.cc b/chrome/browser/safe_browsing/safe_browsing_util.cc
index f036646..2d6cd9c 100644
--- a/chrome/browser/safe_browsing/safe_browsing_util.cc
+++ b/chrome/browser/safe_browsing/safe_browsing_util.cc
@@ -596,8 +596,8 @@ GURL GeneratePhishingReportUrl(const std::string& report_page,
   if (!lang)
     lang = "en";  // fallback
   const std::string continue_esc =
-      EscapeQueryParamValue(StringPrintf(kContinueUrlFormat, lang));
-  const std::string current_esc = EscapeQueryParamValue(url_to_report);
+      EscapeQueryParamValue(StringPrintf(kContinueUrlFormat, lang), true);
+  const std::string current_esc = EscapeQueryParamValue(url_to_report, true);
 
 #if defined(OS_WIN)
   BrowserDistribution* dist = BrowserDistribution::GetDistribution();
diff --git a/chrome/browser/search_engines/template_url.cc b/chrome/browser/search_engines/template_url.cc
index 82bec3b..26da6fa 100644
--- a/chrome/browser/search_engines/template_url.cc
+++ b/chrome/browser/search_engines/template_url.cc
@@ -246,6 +246,19 @@ std::wstring TemplateURLRef::ReplaceSearchTerms(
   if (replacements_.empty())
     return parsed_url_;
 
+  // Determine if the search terms are in the query or before. We're escaping
+  // space as '+' in the former case and as '%20' in the latter case.
+  bool use_plus = true;
+  for (Replacements::iterator i = replacements_.begin();
+       i != replacements_.end(); ++i) {
+    if (i->type == SEARCH_TERMS) {
+      std::wstring::size_type query_start = parsed_url_.find(L'?');
+      use_plus = query_start != std::wstring::npos &&
+          (static_cast<std::wstring::size_type>(i->index) > query_start);
+      break;
+    }
+  }
+
   // Encode the search terms so that we know the encoding.
   const std::vector<std::string>& encodings = host.input_encodings();
   string16 encoded_terms;
@@ -253,21 +266,23 @@ std::wstring TemplateURLRef::ReplaceSearchTerms(
   std::wstring input_encoding;
   for (size_t i = 0; i < encodings.size(); ++i) {
     if (EscapeQueryParamValue(WideToUTF16Hack(terms),
-                              encodings[i].c_str(), &encoded_terms)) {
+                              encodings[i].c_str(), use_plus, &encoded_terms)) {
       if (!original_query_for_suggestion.empty()) {
         EscapeQueryParamValue(WideToUTF16Hack(original_query_for_suggestion),
-                              encodings[i].c_str(), &encoded_original_query);
+                              encodings[i].c_str(),
+                              true,
+                              &encoded_original_query);
       }
       input_encoding = ASCIIToWide(encodings[i]);
       break;
     }
   }
   if (input_encoding.empty()) {
-    encoded_terms = WideToUTF16Hack(EscapeQueryParamValueUTF8(terms));
+    encoded_terms = WideToUTF16Hack(EscapeQueryParamValueUTF8(terms, use_plus));
     if (!original_query_for_suggestion.empty()) {
       encoded_original_query =
           WideToUTF16Hack(
-              EscapeQueryParamValueUTF8(original_query_for_suggestion));
+              EscapeQueryParamValueUTF8(original_query_for_suggestion, true));
     }
     input_encoding = L"UTF-8";
   }
diff --git a/chrome/browser/search_engines/template_url_unittest.cc b/chrome/browser/search_engines/template_url_unittest.cc
index f15d6fc3..261adf0 100644
--- a/chrome/browser/search_engines/template_url_unittest.cc
+++ b/chrome/browser/search_engines/template_url_unittest.cc
@@ -38,15 +38,29 @@ TEST_F(TemplateURLTest, TestValidWithComplete) {
 }
 
 TEST_F(TemplateURLTest, URLRefTestSearchTerms) {
-  TemplateURL t_url;
-  TemplateURLRef ref(L"http://foo{searchTerms}", 0, 0);
-  ASSERT_TRUE(ref.IsValid());
-
-  ASSERT_TRUE(ref.SupportsReplacement());
-  GURL result = GURL(WideToUTF8(ref.ReplaceSearchTerms(t_url, L"search",
-      TemplateURLRef::NO_SUGGESTIONS_AVAILABLE, std::wstring())));
-  ASSERT_TRUE(result.is_valid());
-  ASSERT_EQ("http://foosearch/", result.spec());
+  struct SearchTermsCase {
+    const wchar_t* url;
+    const wchar_t* terms;
+    const char* output;
+  } search_term_cases[] = {
+    { L"http://foo{searchTerms}", L"sea rch", "http://foosea%20rch/" },
+    { L"http://foo{searchTerms}?boo=abc", L"sea rch",
+      "http://foosea%20rch/?boo=abc" },
+    { L"http://foo/?boo={searchTerms}", L"sea rch",
+      "http://foo/?boo=sea+rch" }
+  };
+  for (size_t i = 0; i < ARRAYSIZE_UNSAFE(search_term_cases); ++i) {
+    const SearchTermsCase& value = search_term_cases[i];
+    TemplateURL t_url;
+    TemplateURLRef ref(value.url, 0, 0);
+    ASSERT_TRUE(ref.IsValid());
+
+    ASSERT_TRUE(ref.SupportsReplacement());
+    GURL result = GURL(WideToUTF8(ref.ReplaceSearchTerms(t_url, value.terms,
+        TemplateURLRef::NO_SUGGESTIONS_AVAILABLE, std::wstring())));
+    ASSERT_TRUE(result.is_valid());
+    ASSERT_EQ(value.output, result.spec());
+  }
 }
 
 TEST_F(TemplateURLTest, URLRefTestCount) {
diff --git a/chrome/renderer/render_view.cc b/chrome/renderer/render_view.cc
index 92b42a7..649542f 100644
--- a/chrome/renderer/render_view.cc
+++ b/chrome/renderer/render_view.cc
@@ -2935,7 +2935,7 @@ GURL RenderView::GetAlternateErrorPageURL(const GURL& failed_url,
   // Construct the query params to send to link doctor.
   std::string params(alternate_error_page_url_.query());
   params.append("&url=");
-  params.append(EscapeQueryParamValue(url_to_send.spec()));
+  params.append(EscapeQueryParamValue(url_to_send.spec(), true));
   params.append("&sourceid=chrome");
   params.append("&error=");
   switch (error_type) {
diff --git a/net/base/escape.cc b/net/base/escape.cc
index 184e93c..5a00b07 100644
--- a/net/base/escape.cc
+++ b/net/base/escape.cc
@@ -190,14 +190,11 @@ std::string EscapeQueryParamValue(const std::string& text, bool use_plus) {
   return Escape(text, kQueryCharmap, use_plus);
 }
 
-std::string EscapeQueryParamValue(const std::string& text) {
-  return Escape(text, kQueryCharmap, true);
-}
-
 // Convert the string to a sequence of bytes and then % escape anything
 // except alphanumerics and !'()*-._~
-std::wstring EscapeQueryParamValueUTF8(const std::wstring& text) {
-  return UTF8ToWide(Escape(WideToUTF8(text), kQueryCharmap, true));
+std::wstring EscapeQueryParamValueUTF8(const std::wstring& text,
+                                       bool use_plus) {
+  return UTF8ToWide(Escape(WideToUTF8(text), kQueryCharmap, use_plus));
 }
 
 // non-printable, non-7bit, and (including space)  "#%:<>?[\]^`{|}
@@ -239,7 +236,7 @@ std::string EscapeExternalHandlerValue(const std::string& text) {
 }
 
 bool EscapeQueryParamValue(const string16& text, const char* codepage,
-                           string16* escaped) {
+                           bool use_plus, string16* escaped) {
   // TODO(brettw) bug 1201094: this function should be removed, this "SKIP"
   // behavior is wrong when the character can't be encoded properly.
   std::string encoded;
@@ -247,7 +244,7 @@ bool EscapeQueryParamValue(const string16& text, const char* codepage,
                              base::OnStringConversionError::SKIP, &encoded))
     return false;
 
-  escaped->assign(UTF8ToUTF16(Escape(encoded, kQueryCharmap, true)));
+  escaped->assign(UTF8ToUTF16(Escape(encoded, kQueryCharmap, use_plus)));
   return true;
 }
 
diff --git a/net/base/escape.h b/net/base/escape.h
index 679e613..67ccc5f 100644
--- a/net/base/escape.h
+++ b/net/base/escape.h
@@ -2,8 +2,8 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-#ifndef NET_BASE_ESCAPE_H__
-#define NET_BASE_ESCAPE_H__
+#ifndef NET_BASE_ESCAPE_H_
+#define NET_BASE_ESCAPE_H_
 
 #include <string>
 
@@ -117,16 +117,15 @@ string16 UnescapeAndDecodeUTF8URLComponent(const std::string& text,
 //
 // TODO(brettw) bug 1201094: This function should be removed. See the bug for
 // why and what callers should do instead.
-std::string EscapeQueryParamValue(const std::string& text);
 std::string EscapeQueryParamValue(const std::string& text, bool use_plus);
 bool EscapeQueryParamValue(const string16& text, const char* codepage,
-                           string16* escaped);
+                           bool use_plus, string16* escaped);
 
 // A specialized version of EscapeQueryParamValue for wide strings that
 // assumes the codepage is UTF8.  This is provided as a convenience.
 //
 // TODO(brettw) bug 1201094: This function should be removed. See the bug for
 // why and what callers should do instead.
-std::wstring EscapeQueryParamValueUTF8(const std::wstring& text);
+std::wstring EscapeQueryParamValueUTF8(const std::wstring& text, bool use_plus);
 
-#endif  // #ifndef NET_BASE_ESCAPE_H__
+#endif  // NET_BASE_ESCAPE_H_
diff --git a/net/base/escape_unittest.cc b/net/base/escape_unittest.cc
index f350117f..c93024c 100644
--- a/net/base/escape_unittest.cc
+++ b/net/base/escape_unittest.cc
@@ -58,7 +58,17 @@ TEST(EscapeTest, EscapeTextForFormSubmission) {
   };
   for (size_t i = 0; i < arraysize(escape_cases); ++i) {
     EscapeCase value = escape_cases[i];
-    EXPECT_EQ(value.output, EscapeQueryParamValueUTF8(value.input));
+    EXPECT_EQ(value.output, EscapeQueryParamValueUTF8(value.input, true));
+  }
+
+  const EscapeCase escape_cases_no_plus[] = {
+    {L"foo", L"foo"},
+    {L"foo bar", L"foo%20bar"},
+    {L"foo++", L"foo%2B%2B"}
+  };
+  for (size_t i = 0; i < arraysize(escape_cases_no_plus); ++i) {
+    EscapeCase value = escape_cases_no_plus[i];
+    EXPECT_EQ(value.output, EscapeQueryParamValueUTF8(value.input, false));
   }
 
   // Test all the values in we're supposed to be escaping.
@@ -70,7 +80,7 @@ TEST(EscapeTest, EscapeTextForFormSubmission) {
   for (int i = 0; i < 256; ++i) {
     std::string in;
     in.push_back(i);
-    std::string out = EscapeQueryParamValue(in);
+    std::string out = EscapeQueryParamValue(in, true);
     if (0 == i) {
       EXPECT_EQ(out, std::string("%00"));
     } else if (32 == i) {
@@ -94,9 +104,14 @@ TEST(EscapeTest, EscapeTextForFormSubmission) {
     test_str.push_back(i);
   }
   string16 wide;
-  EXPECT_TRUE(EscapeQueryParamValue(test_str, base::kCodepageUTF8, &wide));
+  EXPECT_TRUE(EscapeQueryParamValue(test_str, base::kCodepageUTF8, true,
+                                    &wide));
+  EXPECT_EQ(UTF16ToWideHack(wide),
+            EscapeQueryParamValueUTF8(UTF16ToWideHack(test_str), true));
+  EXPECT_TRUE(EscapeQueryParamValue(test_str, base::kCodepageUTF8, false,
+                                    &wide));
   EXPECT_EQ(UTF16ToWideHack(wide),
-            EscapeQueryParamValueUTF8(UTF16ToWideHack(test_str)));
+            EscapeQueryParamValueUTF8(UTF16ToWideHack(test_str), false));
 }
 
 TEST(EscapeTest, EscapePath) {