Add "?" to query strings if original url has no query string.

BUG=none
TEST=protocol_manager_unittest.cc
Review URL: http://codereview.chromium.org/3244005

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

3 files changed, 26 insertions, 11 deletions

diff --git a/chrome/browser/safe_browsing/protocol_manager.cc b/chrome/browser/safe_browsing/protocol_manager.cc
index 340d0ca..aef07ae 100644
--- a/chrome/browser/safe_browsing/protocol_manager.cc
+++ b/chrome/browser/safe_browsing/protocol_manager.cc
@@ -649,6 +649,8 @@ std::string SafeBrowsingProtocolManager::ComposeUrl(
                                  prefix.c_str(), method.c_str(),
                                  client_name.c_str(), version.c_str());
   if (!additional_query.empty()) {
+    DCHECK(url.find("?") != std::string::npos);
+    url.append("&");
     url.append(additional_query);
   }
   return url;
@@ -695,11 +697,18 @@ GURL SafeBrowsingProtocolManager::NextChunkUrl(const std::string& url) const {
   std::string next_url;
   if (!StartsWithASCII(url, "http://", false) &&
       !StartsWithASCII(url, "https://", false)) {
-    next_url = "http://" + url;
+    next_url.append("http://");
+    next_url.append(url);
   } else {
     next_url = url;
   }
-  if (!additional_query_.empty())
-    next_url += additional_query_;
+  if (!additional_query_.empty()) {
+    if (next_url.find("?") != std::string::npos) {
+      next_url.append("&");
+    } else {
+      next_url.append("?");
+    }
+    next_url.append(additional_query_);
+  }
   return GURL(next_url);
 }
diff --git a/chrome/browser/safe_browsing/protocol_manager.h b/chrome/browser/safe_browsing/protocol_manager.h
index 37f85a2..fcdc438 100644
--- a/chrome/browser/safe_browsing/protocol_manager.h
+++ b/chrome/browser/safe_browsing/protocol_manager.h
@@ -131,14 +131,15 @@ class SafeBrowsingProtocolManager : public URLFetcher::Delegate {
   // so are handled separately.
   enum SafeBrowsingRequestType {
     NO_REQUEST = 0,     // No requests in progress
-    UPDATE_REQUEST,   // Request for redirect URLs
+    UPDATE_REQUEST,     // Request for redirect URLs
     CHUNK_REQUEST,      // Request for a specific chunk
     GETKEY_REQUEST      // Update the client's MAC key
   };
 
   // Composes a URL using |prefix|, |method| (e.g.: gethash, download,
   // newkey, report), |client_name| and |version|. When not empty,
-  // |additional_query| is appended to the URL.
+  // |additional_query| is appended to the URL with an additional "&"
+  // in the front.
   static std::string ComposeUrl(const std::string& prefix,
                                 const std::string& method,
                                 const std::string& client_name,
diff --git a/chrome/browser/safe_browsing/protocol_manager_unittest.cc b/chrome/browser/safe_browsing/protocol_manager_unittest.cc
index 1df9a43..a73db3e 100644
--- a/chrome/browser/safe_browsing/protocol_manager_unittest.cc
+++ b/chrome/browser/safe_browsing/protocol_manager_unittest.cc
@@ -19,7 +19,7 @@ static const char kClientKey[] = "SCg9lcLHd0dfksXgYsacwQ==";
 static const char kWrappedKey[] =
     "AKEgNisjLl7iRYrjWHmpd_XwCiilxrw8nNaYH47tiQ7pDe9cEErjVHGZaPPUau5h61tbXSDqA"
     "BiJZnDFByc_g8B5vTwxkhBf9g==";
-static const char kAdditionalQuery[] = "&additional_query";
+static const char kAdditionalQuery[] = "additional_query";
 
 class SafeBrowsingProtocolManagerTest : public testing::Test {
 };
@@ -162,7 +162,7 @@ TEST_F(SafeBrowsingProtocolManagerTest, TestGetHashUrl) {
             "ErjVHGZaPPUau5h61tbXSDqABiJZnDFByc_g8B5vTwxkhBf9g==",
             pm.GetHashUrl(true).spec());
 
-  pm.set_additional_query("&additional_query");
+  pm.set_additional_query(kAdditionalQuery);
   EXPECT_EQ("http://info.prefix.com/foo/gethash?client=unittest&appver=1.0&"
             "pver=2.2&additional_query",
             pm.GetHashUrl(false).spec());
@@ -184,7 +184,7 @@ TEST_F(SafeBrowsingProtocolManagerTest, TestUpdateUrl) {
             "ErjVHGZaPPUau5h61tbXSDqABiJZnDFByc_g8B5vTwxkhBf9g==",
             pm.UpdateUrl(true).spec());
 
-  pm.set_additional_query("&additional_query");
+  pm.set_additional_query(kAdditionalQuery);
   EXPECT_EQ("http://info.prefix.com/foo/downloads?client=unittest&appver=1.0&"
             "pver=2.2&additional_query", pm.UpdateUrl(false).spec());
   EXPECT_EQ("http://info.prefix.com/foo/downloads?client=unittest&appver=1.0&"
@@ -208,7 +208,7 @@ TEST_F(SafeBrowsingProtocolManagerTest, TestMalwareReportUrl) {
             pm.MalwareReportUrl(malware_url, page_url, referrer_url,
                                 true).spec());
 
-  pm.set_additional_query("&additional_query");
+  pm.set_additional_query(kAdditionalQuery);
   EXPECT_EQ("http://info.prefix.com/foo/report?client=unittest&appver=1.0&"
             "pver=2.2&additional_query&evts=malblhit&"
             "evtd=http%3A%2F%2Fmalware.url.com%2F&"
@@ -226,7 +226,7 @@ TEST_F(SafeBrowsingProtocolManagerTest, TestMacKeyUrl) {
   EXPECT_EQ("https://key.prefix.com/bar/newkey?client=unittest&appver=1.0&"
             "pver=2.2", pm.MacKeyUrl().spec());
 
-  pm.set_additional_query("&additional_query");
+  pm.set_additional_query(kAdditionalQuery);
   EXPECT_EQ("https://key.prefix.com/bar/newkey?client=unittest&appver=1.0&"
             "pver=2.2&additional_query", pm.MacKeyUrl().spec());
 }
@@ -239,6 +239,7 @@ TEST_F(SafeBrowsingProtocolManagerTest, TestNextChunkUrl) {
   std::string url_partial = "localhost:1234/foo/bar?foo";
   std::string url_http_full = "http://localhost:1234/foo/bar?foo";
   std::string url_https_full = "https://localhost:1234/foo/bar?foo";
+  std::string url_https_no_query = "https://localhost:1234/foo/bar";
 
   EXPECT_EQ("http://localhost:1234/foo/bar?foo",
             pm.NextChunkUrl(url_partial).spec());
@@ -246,12 +247,16 @@ TEST_F(SafeBrowsingProtocolManagerTest, TestNextChunkUrl) {
             pm.NextChunkUrl(url_http_full).spec());
   EXPECT_EQ("https://localhost:1234/foo/bar?foo",
             pm.NextChunkUrl(url_https_full).spec());
+  EXPECT_EQ("https://localhost:1234/foo/bar",
+            pm.NextChunkUrl(url_https_no_query).spec());
 
-  pm.set_additional_query("&additional_query");
+  pm.set_additional_query(kAdditionalQuery);
   EXPECT_EQ("http://localhost:1234/foo/bar?foo&additional_query",
             pm.NextChunkUrl(url_partial).spec());
   EXPECT_EQ("http://localhost:1234/foo/bar?foo&additional_query",
             pm.NextChunkUrl(url_http_full).spec());
   EXPECT_EQ("https://localhost:1234/foo/bar?foo&additional_query",
             pm.NextChunkUrl(url_https_full).spec());
+  EXPECT_EQ("https://localhost:1234/foo/bar?additional_query",
+            pm.NextChunkUrl(url_https_no_query).spec());
 }