Refactoring: replace some raw strings with GURL.

Also address a TODO about stripping references from proxy resolve requests.

Review URL: http://codereview.chromium.org/13029

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

10 files changed, 57 insertions, 44 deletions

diff --git a/net/proxy/proxy_resolver_fixed.cc b/net/proxy/proxy_resolver_fixed.cc
index 5deecd6..b064803 100644
--- a/net/proxy/proxy_resolver_fixed.cc
+++ b/net/proxy/proxy_resolver_fixed.cc
@@ -13,8 +13,8 @@ int ProxyResolverFixed::GetProxyConfig(ProxyConfig* config) {
   return OK;
 }
 
-int ProxyResolverFixed::GetProxyForURL(const std::string& query_url,
-                                       const std::string& pac_url,
+int ProxyResolverFixed::GetProxyForURL(const GURL& query_url,
+                                       const GURL& pac_url,
                                        ProxyInfo* results) {
   NOTREACHED() << "Should not be asked to do proxy auto config";
   return ERR_FAILED;
diff --git a/net/proxy/proxy_resolver_fixed.h b/net/proxy/proxy_resolver_fixed.h
index ed92944..2ecd62c 100644
--- a/net/proxy/proxy_resolver_fixed.h
+++ b/net/proxy/proxy_resolver_fixed.h
@@ -16,8 +16,8 @@ class ProxyResolverFixed : public ProxyResolver {
 
   // ProxyResolver methods:
   virtual int GetProxyConfig(ProxyConfig* config);
-  virtual int GetProxyForURL(const std::string& query_url,
-                             const std::string& pac_url,
+  virtual int GetProxyForURL(const GURL& query_url,
+                             const GURL& pac_url,
                              ProxyInfo* results);
 
  private:
diff --git a/net/proxy/proxy_resolver_mac.cc b/net/proxy/proxy_resolver_mac.cc
index ab47528..f58cc85 100644
--- a/net/proxy/proxy_resolver_mac.cc
+++ b/net/proxy/proxy_resolver_mac.cc
@@ -141,7 +141,7 @@ int ProxyResolverMac::GetProxyConfig(ProxyConfig* config) {
             kSCPropNetProxiesProxyAutoConfigURLString,
             CFStringGetTypeID());
     if (pac_url_ref)
-      config->pac_url = base::SysCFStringRefToUTF8(pac_url_ref);
+      config->pac_url = GURL(base::SysCFStringRefToUTF8(pac_url_ref));
   }
   
   // proxies (for now only ftp, http and https)
@@ -222,13 +222,13 @@ int ProxyResolverMac::GetProxyConfig(ProxyConfig* config) {
 
 // Gets the proxy information for a query URL from a PAC. Implementation
 // inspired by http://developer.apple.com/samplecode/CFProxySupportTool/
-int ProxyResolverMac::GetProxyForURL(const std::string& query_url,
-                                     const std::string& pac_url,
+int ProxyResolverMac::GetProxyForURL(const GURL& query_url,
+                                     const GURL& pac_url,
                                      ProxyInfo* results) {
   scoped_cftyperef<CFStringRef> query_ref(
-      base::SysUTF8ToCFStringRef(query_url));
+      base::SysUTF8ToCFStringRef(query_url.spec()));
   scoped_cftyperef<CFStringRef> pac_ref(
-      base::SysUTF8ToCFStringRef(pac_url));
+      base::SysUTF8ToCFStringRef(pac_url.spec()));
   scoped_cftyperef<CFURLRef> query_url_ref(
       CFURLCreateWithString(kCFAllocatorDefault,
                             query_ref.get(),
diff --git a/net/proxy/proxy_resolver_mac.h b/net/proxy/proxy_resolver_mac.h
index 3063866..a3b56fc 100644
--- a/net/proxy/proxy_resolver_mac.h
+++ b/net/proxy/proxy_resolver_mac.h
@@ -15,8 +15,8 @@ class ProxyResolverMac : public ProxyResolver {
  public:
   // ProxyResolver methods:
   virtual int GetProxyConfig(ProxyConfig* config);
-  virtual int GetProxyForURL(const std::string& query_url,
-                             const std::string& pac_url,
+  virtual int GetProxyForURL(const GURL& query_url,
+                             const GURL& pac_url,
                              ProxyInfo* results);
 };
 
diff --git a/net/proxy/proxy_resolver_null.h b/net/proxy/proxy_resolver_null.h
index 17276e8..e1bdcbd 100644
--- a/net/proxy/proxy_resolver_null.h
+++ b/net/proxy/proxy_resolver_null.h
@@ -15,8 +15,8 @@ class ProxyResolverNull : public ProxyResolver {
   virtual int GetProxyConfig(ProxyConfig* config) {
     return ERR_NOT_IMPLEMENTED;
   }
-  virtual int GetProxyForURL(const std::string& query_url,
-                             const std::string& pac_url,
+  virtual int GetProxyForURL(const GURL& query_url,
+                             const GURL& pac_url,
                              ProxyInfo* results) {
     return ERR_NOT_IMPLEMENTED;
   }
diff --git a/net/proxy/proxy_resolver_winhttp.cc b/net/proxy/proxy_resolver_winhttp.cc
index 19dd0b0..382914f 100644
--- a/net/proxy/proxy_resolver_winhttp.cc
+++ b/net/proxy/proxy_resolver_winhttp.cc
@@ -84,14 +84,14 @@ int ProxyResolverWinHttp::GetProxyConfig(ProxyConfig* config) {
     }
   }
   if (ie_config.lpszAutoConfigUrl)
-    config->pac_url = WideToASCII(ie_config.lpszAutoConfigUrl);
+    config->pac_url = GURL(ie_config.lpszAutoConfigUrl);
 
   FreeConfig(&ie_config);
   return OK;
 }
 
-int ProxyResolverWinHttp::GetProxyForURL(const std::string& query_url,
-                                         const std::string& pac_url,
+int ProxyResolverWinHttp::GetProxyForURL(const GURL& query_url,
+                                         const GURL& pac_url,
                                          ProxyInfo* results) {
   // If we don't have a WinHTTP session, then create a new one.
   if (!session_handle_ && !OpenWinHttpSession())
@@ -106,7 +106,7 @@ int ProxyResolverWinHttp::GetProxyForURL(const std::string& query_url,
   WINHTTP_AUTOPROXY_OPTIONS options = {0};
   options.fAutoLogonIfChallenged = FALSE;
   options.dwFlags = WINHTTP_AUTOPROXY_CONFIG_URL;
-  std::wstring pac_url_wide = ASCIIToWide(pac_url);
+  std::wstring pac_url_wide = ASCIIToWide(pac_url.spec());
   options.lpszAutoConfigUrl =
       pac_url_wide.empty() ? L"http://wpad/wpad.dat" : pac_url_wide.c_str();
 
@@ -119,12 +119,13 @@ int ProxyResolverWinHttp::GetProxyForURL(const std::string& query_url,
   // get good performance in the case where WinHTTP uses an out-of-process
   // resolver.  This is important for Vista and Win2k3.
   BOOL ok = CallWinHttpGetProxyForUrl(
-      session_handle_, ASCIIToWide(query_url).c_str(), &options, &info);
+      session_handle_, ASCIIToWide(query_url.spec()).c_str(), &options, &info);
   if (!ok) {
     if (ERROR_WINHTTP_LOGIN_FAILURE == GetLastError()) {
       options.fAutoLogonIfChallenged = TRUE;
       ok = CallWinHttpGetProxyForUrl(
-          session_handle_, ASCIIToWide(query_url).c_str(), &options, &info);
+          session_handle_, ASCIIToWide(query_url.spec()).c_str(),
+          &options, &info);
     }
     if (!ok) {
       DWORD error = GetLastError();
diff --git a/net/proxy/proxy_resolver_winhttp.h b/net/proxy/proxy_resolver_winhttp.h
index 176a87a..ac981686 100644
--- a/net/proxy/proxy_resolver_winhttp.h
+++ b/net/proxy/proxy_resolver_winhttp.h
@@ -20,8 +20,8 @@ class ProxyResolverWinHttp : public ProxyResolver {
 
   // ProxyResolver implementation:
   virtual int GetProxyConfig(ProxyConfig* config);
-  virtual int GetProxyForURL(const std::string& query_url,
-                             const std::string& pac_url,
+  virtual int GetProxyForURL(const GURL& query_url,
+                             const GURL& pac_url,
                              ProxyInfo* results);
 
  private:
diff --git a/net/proxy/proxy_service.cc b/net/proxy/proxy_service.cc
index 394a97f..7cfb9ac 100644
--- a/net/proxy/proxy_service.cc
+++ b/net/proxy/proxy_service.cc
@@ -177,7 +177,7 @@ class ProxyService::PacRequest :
     public base::RefCountedThreadSafe<ProxyService::PacRequest> {
  public:
   PacRequest(ProxyService* service,
-             const std::string& pac_url,
+             const GURL& pac_url,
              CompletionCallback* callback)
       : service_(service),
         callback_(callback),
@@ -190,7 +190,7 @@ class ProxyService::PacRequest :
       origin_loop_ = MessageLoop::current();
   }
 
-  void Query(const std::string& url, ProxyInfo* results) {
+  void Query(const GURL& url, ProxyInfo* results) {
     results_ = results;
     // If we have a valid callback then execute Query asynchronously
     if (callback_) {
@@ -214,8 +214,8 @@ class ProxyService::PacRequest :
  private:
   // Runs on the PAC thread if a valid callback is provided.
   void DoQuery(ProxyResolver* resolver,
-               const std::string& query_url,
-               const std::string& pac_url) {
+               const GURL& query_url,
+               const GURL& pac_url) {
     int rv = resolver->GetProxyForURL(query_url, pac_url, &results_buf_);
     if (origin_loop_) {
       origin_loop_->PostTask(FROM_HERE,
@@ -253,7 +253,7 @@ class ProxyService::PacRequest :
 
   // Usable from within DoQuery on the PAC thread.
   ProxyInfo results_buf_;
-  std::string pac_url_;
+  GURL pac_url_;
   MessageLoop* origin_loop_;
 };
 
@@ -326,7 +326,7 @@ int ProxyService::ResolveProxy(const GURL& url, ProxyInfo* result,
       return OK;
     }
 
-    if (!config_.pac_url.empty() || config_.auto_detect) {
+    if (config_.pac_url.is_valid() || config_.auto_detect) {
       if (callback) {
         // Create PAC thread for asynchronous mode.
         if (!pac_thread_.get()) {
@@ -341,9 +341,20 @@ int ProxyService::ResolveProxy(const GURL& url, ProxyInfo* result,
 
       scoped_refptr<PacRequest> req =
           new PacRequest(this, config_.pac_url, callback);
-      // TODO(darin): We should strip away any reference fragment since it is
-      // not relevant, and moreover it could contain non-ASCII bytes.
-      req->Query(url.spec(), result);
+
+      // Strip away any reference fragments and the username/password, as they
+      // are not relevant to proxy resolution.
+      GURL sanitized_url;
+      { // TODO(eroman): The following duplicates logic from
+        // HttpUtil::SpecForRequest. Should probably live in net_util.h
+        GURL::Replacements replacements;
+        replacements.ClearUsername();
+        replacements.ClearPassword();
+        replacements.ClearRef();
+        sanitized_url = url.ReplaceComponents(replacements);
+      }
+
+      req->Query(sanitized_url, result);
 
       if (callback) {
         if (pac_request)
diff --git a/net/proxy/proxy_service.h b/net/proxy/proxy_service.h
index 8ea3653..ea83509 100644
--- a/net/proxy/proxy_service.h
+++ b/net/proxy/proxy_service.h
@@ -13,6 +13,7 @@
 #include "base/string_util.h"
 #include "base/thread.h"
 #include "base/time.h"
+#include "googleurl/src/gurl.h"
 #include "net/base/completion_callback.h"
 
 #if defined(OS_WIN)
@@ -44,7 +45,7 @@ class ProxyConfig {
   bool auto_detect;
 
   // If non-empty, indicates the URL of the proxy auto-config file to use.
-  std::string pac_url;
+  GURL pac_url;
 
   // If non-empty, indicates the proxy server to use (of the form host:port).
   // If proxies depend on the scheme, a string of the format
@@ -274,8 +275,8 @@ class ProxyResolver {
   // Query the proxy auto-config file (specified by |pac_url|) for the proxy to
   // use to load the given |query_url|.  Returns OK if successful or an error
   // code if otherwise.
-  virtual int GetProxyForURL(const std::string& query_url,
-                             const std::string& pac_url,
+  virtual int GetProxyForURL(const GURL& query_url,
+                             const GURL& pac_url,
                              ProxyInfo* results) = 0;
 };
 
diff --git a/net/proxy/proxy_service_unittest.cc b/net/proxy/proxy_service_unittest.cc
index 8166a42..16a1649 100644
--- a/net/proxy/proxy_service_unittest.cc
+++ b/net/proxy/proxy_service_unittest.cc
@@ -18,8 +18,8 @@ class MockProxyResolver : public net::ProxyResolver {
     *results = *(config.get());
     return net::OK;
   }
-  virtual int GetProxyForURL(const std::string& query_url,
-                             const std::string& pac_url,
+  virtual int GetProxyForURL(const GURL& query_url,
+                             const GURL& pac_url,
                              net::ProxyInfo* results) {
     if (pac_url != config->pac_url)
       return net::ERR_INVALID_ARGUMENT;
@@ -59,7 +59,7 @@ TEST(ProxyServiceTest, Direct) {
 
 TEST(ProxyServiceTest, PAC) {
   MockProxyResolver resolver;
-  resolver.config->pac_url = "http://foopy/proxy.pac";
+  resolver.config->pac_url = GURL("http://foopy/proxy.pac");
   resolver.info.UseNamedProxy("foopy");
   resolver.info_predicate_query_host = "www.google.com";
 
@@ -76,7 +76,7 @@ TEST(ProxyServiceTest, PAC) {
 
 TEST(ProxyServiceTest, PAC_FailoverToDirect) {
   MockProxyResolver resolver;
-  resolver.config->pac_url = "http://foopy/proxy.pac";
+  resolver.config->pac_url = GURL("http://foopy/proxy.pac");
   resolver.info.UseNamedProxy("foopy:8080");
   resolver.info_predicate_query_host = "www.google.com";
 
@@ -100,7 +100,7 @@ TEST(ProxyServiceTest, PAC_FailsToDownload) {
   // Test what happens when we fail to download the PAC URL.
 
   MockProxyResolver resolver;
-  resolver.config->pac_url = "http://foopy/proxy.pac";
+  resolver.config->pac_url = GURL("http://foopy/proxy.pac");
   resolver.info.UseNamedProxy("foopy:8080");
   resolver.info_predicate_query_host = "www.google.com";
   resolver.fail_get_proxy_for_url = true;
@@ -133,7 +133,7 @@ TEST(ProxyServiceTest, ProxyFallback) {
   // are bad.
 
   MockProxyResolver resolver;
-  resolver.config->pac_url = "http://foopy/proxy.pac";
+  resolver.config->pac_url = GURL("http://foopy/proxy.pac");
   resolver.info.UseNamedProxy("foopy1:8080;foopy2:9090");
   resolver.info_predicate_query_host = "www.google.com";
   resolver.fail_get_proxy_for_url = false;
@@ -160,7 +160,7 @@ TEST(ProxyServiceTest, ProxyFallback) {
 
   // Create a new resolver that returns 3 proxies. The second one is already
   // known to be bad.
-  resolver.config->pac_url = "http://foopy/proxy.pac";
+  resolver.config->pac_url = GURL("http://foopy/proxy.pac");
   resolver.info.UseNamedProxy("foopy3:7070;foopy1:8080;foopy2:9090");
   resolver.info_predicate_query_host = "www.google.com";
   resolver.fail_get_proxy_for_url = false;
@@ -191,7 +191,7 @@ TEST(ProxyServiceTest, ProxyFallback_NewSettings) {
   // Test proxy failover when new settings are available.
 
   MockProxyResolver resolver;
-  resolver.config->pac_url = "http://foopy/proxy.pac";
+  resolver.config->pac_url = GURL("http://foopy/proxy.pac");
   resolver.info.UseNamedProxy("foopy1:8080;foopy2:9090");
   resolver.info_predicate_query_host = "www.google.com";
   resolver.fail_get_proxy_for_url = false;
@@ -211,7 +211,7 @@ TEST(ProxyServiceTest, ProxyFallback_NewSettings) {
 
   // Fake an error on the proxy, and also a new configuration on the proxy.
   resolver.config.reset(new net::ProxyConfig);
-  resolver.config->pac_url = "http://foopy-new/proxy.pac";
+  resolver.config->pac_url = GURL("http://foopy-new/proxy.pac");
 
   rv = service.ReconsiderProxyAfterError(url, &info, NULL, NULL);
   EXPECT_EQ(rv, net::OK);
@@ -226,7 +226,7 @@ TEST(ProxyServiceTest, ProxyFallback_NewSettings) {
 
   // We simulate a new configuration.
   resolver.config.reset(new net::ProxyConfig);
-  resolver.config->pac_url = "http://foopy-new2/proxy.pac";
+  resolver.config->pac_url = GURL("http://foopy-new2/proxy.pac");
 
   // We fake anothe error. It should go back to the first proxy.
   rv = service.ReconsiderProxyAfterError(url, &info, NULL, NULL);
@@ -238,7 +238,7 @@ TEST(ProxyServiceTest, ProxyFallback_BadConfig) {
   // Test proxy failover when the configuration is bad.
 
   MockProxyResolver resolver;
-  resolver.config->pac_url = "http://foopy/proxy.pac";
+  resolver.config->pac_url = GURL("http://foopy/proxy.pac");
   resolver.info.UseNamedProxy("foopy1:8080;foopy2:9090");
   resolver.info_predicate_query_host = "www.google.com";
   resolver.fail_get_proxy_for_url = false;