Change HostResolver::RequestInfo to take a HostPortPair rather than naked host string + port.

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

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

8 files changed, 65 insertions, 53 deletions

diff --git a/net/base/host_port_pair.cc b/net/base/host_port_pair.cc
index 1e0416b..bb63765 100644
--- a/net/base/host_port_pair.cc
+++ b/net/base/host_port_pair.cc
@@ -4,6 +4,7 @@
 
 #include "net/base/host_port_pair.h"
 #include "base/string_util.h"
+#include "googleurl/src/gurl.h"
 
 namespace net {
 
@@ -11,6 +12,11 @@ HostPortPair::HostPortPair() : port_(0) {}
 HostPortPair::HostPortPair(const std::string& in_host, uint16 in_port)
     : host_(in_host), port_(in_port) {}
 
+// static
+HostPortPair HostPortPair::FromURL(const GURL& url) {
+  return HostPortPair(url.HostNoBrackets(), url.EffectiveIntPort());
+}
+
 std::string HostPortPair::ToString() const {
   return StringPrintf("%s:%u", HostForURL().c_str(), port_);
 }
diff --git a/net/base/host_port_pair.h b/net/base/host_port_pair.h
index 0139b7c..2c7bb9f 100644
--- a/net/base/host_port_pair.h
+++ b/net/base/host_port_pair.h
@@ -9,6 +9,8 @@
 #include <string>
 #include "base/basictypes.h"
 
+class GURL;
+
 namespace net {
 
 class HostPortPair {
@@ -17,6 +19,9 @@ class HostPortPair {
   // If |in_host| represents an IPv6 address, it should not bracket the address.
   HostPortPair(const std::string& in_host, uint16 in_port);
 
+  // Creates a HostPortPair for the origin of |url|.
+  static HostPortPair FromURL(const GURL& url);
+
   // TODO(willchan): Define a functor instead.
   // Comparator function so this can be placed in a std::map.
   bool operator<(const HostPortPair& other) const {
diff --git a/net/base/host_resolver.cc b/net/base/host_resolver.cc
index 596c7df..abef6f5 100644
--- a/net/base/host_resolver.cc
+++ b/net/base/host_resolver.cc
@@ -10,11 +10,10 @@
 
 namespace net {
 
-HostResolver::RequestInfo::RequestInfo(const std::string& hostname, int port)
-    : hostname_(hostname),
+HostResolver::RequestInfo::RequestInfo(const HostPortPair& host_port_pair)
+    : host_port_pair_(host_port_pair),
       address_family_(ADDRESS_FAMILY_UNSPECIFIED),
       host_resolver_flags_(0),
-      port_(port),
       allow_cached_response_(true),
       is_speculative_(false),
       priority_(MEDIUM) {
diff --git a/net/base/host_resolver.h b/net/base/host_resolver.h
index 61fbc3b..58ea8bd 100644
--- a/net/base/host_resolver.h
+++ b/net/base/host_resolver.h
@@ -12,6 +12,7 @@
 #include "googleurl/src/gurl.h"
 #include "net/base/address_family.h"
 #include "net/base/completion_callback.h"
+#include "net/base/host_port_pair.h"
 #include "net/base/request_priority.h"
 
 namespace net {
@@ -32,21 +33,19 @@ class NetLog;
 // goes out of scope).
 class HostResolver : public base::RefCounted<HostResolver> {
  public:
-  // The parameters for doing a Resolve(). |hostname| and |port| are required,
+  // The parameters for doing a Resolve(). A hostname and port are required,
   // the rest are optional (and have reasonable defaults).
   class RequestInfo {
    public:
-    RequestInfo(const std::string& hostname, int port);
+    explicit RequestInfo(const HostPortPair& host_port_pair);
 
-    int port() const { return port_; }
-    void set_port(int port) {
-      port_ = port;
+    const HostPortPair& host_port_pair() const { return host_port_pair_; }
+    void set_host_port_pair(const HostPortPair& host_port_pair) {
+      host_port_pair_ = host_port_pair;
     }
 
-    const std::string& hostname() const { return hostname_; }
-    void set_hostname(const std::string& hostname) {
-      hostname_ = hostname;
-    }
+    int port() const { return host_port_pair_.port(); }
+    const std::string& hostname() const { return host_port_pair_.host(); }
 
     AddressFamily address_family() const { return address_family_; }
     void set_address_family(AddressFamily address_family) {
@@ -73,8 +72,8 @@ class HostResolver : public base::RefCounted<HostResolver> {
     void set_referrer(const GURL& referrer) { referrer_ = referrer; }
 
    private:
-    // The hostname to resolve.
-    std::string hostname_;
+    // The hostname to resolve, and the port to use in resulting sockaddrs.
+    HostPortPair host_port_pair_;
 
     // The address family to restrict results to.
     AddressFamily address_family_;
@@ -82,9 +81,6 @@ class HostResolver : public base::RefCounted<HostResolver> {
     // Flags to use when resolving this request.
     HostResolverFlags host_resolver_flags_;
 
-    // The port number to set in the result's sockaddrs.
-    int port_;
-
     // Whether it is ok to return a result from the host cache.
     bool allow_cached_response_;
 
diff --git a/net/base/host_resolver_impl.cc b/net/base/host_resolver_impl.cc
index 4b4649d..65f0b53 100644
--- a/net/base/host_resolver_impl.cc
+++ b/net/base/host_resolver_impl.cc
@@ -138,8 +138,7 @@ class RequestInfoParameters : public NetLog::EventParameters {
 
   virtual Value* ToValue() const {
     DictionaryValue* dict = new DictionaryValue();
-    dict->SetString("host", HostPortPair(info_.hostname(),
-                                         info_.port()).ToString());
+    dict->SetString("host", info_.host_port_pair().ToString());
     dict->SetInteger("address_family",
                      static_cast<int>(info_.address_family()));
     dict->SetBoolean("allow_cached_response", info_.allow_cached_response());
diff --git a/net/base/host_resolver_impl_unittest.cc b/net/base/host_resolver_impl_unittest.cc
index 78b2087..fb5b5dc 100644
--- a/net/base/host_resolver_impl_unittest.cc
+++ b/net/base/host_resolver_impl_unittest.cc
@@ -46,7 +46,7 @@ HostResolverImpl* CreateHostResolverImpl(HostResolverProc* resolver_proc) {
 HostResolver::RequestInfo CreateResolverRequest(
     const std::string& hostname,
     RequestPriority priority) {
-  HostResolver::RequestInfo info(hostname, 80);
+  HostResolver::RequestInfo info(HostPortPair(hostname, 80));
   info.set_priority(priority);
   return info;
 }
@@ -56,7 +56,7 @@ HostResolver::RequestInfo CreateResolverRequestForAddressFamily(
     const std::string& hostname,
     RequestPriority priority,
     AddressFamily address_family) {
-  HostResolver::RequestInfo info(hostname, 80);
+  HostResolver::RequestInfo info(HostPortPair(hostname, 80));
   info.set_priority(priority);
   info.set_address_family(address_family);
   return info;
@@ -164,7 +164,9 @@ class ResolveRequest {
                  const std::string& hostname,
                  int port,
                  Delegate* delegate)
-      : info_(hostname, port), resolver_(resolver), delegate_(delegate),
+      : info_(HostPortPair(hostname, port)),
+        resolver_(resolver),
+        delegate_(delegate),
         ALLOW_THIS_IN_INITIALIZER_LIST(
             callback_(this, &ResolveRequest::OnLookupFinished)) {
     // Start the request.
@@ -265,7 +267,7 @@ TEST_F(HostResolverImplTest, SynchronousLookup) {
   scoped_refptr<HostResolver> host_resolver(
       CreateHostResolverImpl(resolver_proc));
 
-  HostResolver::RequestInfo info("just.testing", kPortnum);
+  HostResolver::RequestInfo info(HostPortPair("just.testing", kPortnum));
   CapturingBoundNetLog log(CapturingNetLog::kUnbounded);
   int err = host_resolver->Resolve(info, &addrlist, NULL, NULL, log.bound());
   EXPECT_EQ(OK, err);
@@ -297,7 +299,7 @@ TEST_F(HostResolverImplTest, AsynchronousLookup) {
   scoped_refptr<HostResolver> host_resolver(
       CreateHostResolverImpl(resolver_proc));
 
-  HostResolver::RequestInfo info("just.testing", kPortnum);
+  HostResolver::RequestInfo info(HostPortPair("just.testing", kPortnum));
   CapturingBoundNetLog log(CapturingNetLog::kUnbounded);
   int err = host_resolver->Resolve(info, &addrlist, &callback_, NULL,
                                    log.bound());
@@ -341,7 +343,7 @@ TEST_F(HostResolverImplTest, CanceledAsynchronousLookup) {
     AddressList addrlist;
     const int kPortnum = 80;
 
-    HostResolver::RequestInfo info("just.testing", kPortnum);
+    HostResolver::RequestInfo info(HostPortPair("just.testing", kPortnum));
     int err = host_resolver->Resolve(info, &addrlist, &callback_, NULL,
                                      log.bound());
     EXPECT_EQ(ERR_IO_PENDING, err);
@@ -397,7 +399,7 @@ TEST_F(HostResolverImplTest, NumericIPv4Address) {
       CreateHostResolverImpl(resolver_proc));
   AddressList addrlist;
   const int kPortnum = 5555;
-  HostResolver::RequestInfo info("127.1.2.3", kPortnum);
+  HostResolver::RequestInfo info(HostPortPair("127.1.2.3", kPortnum));
   int err = host_resolver->Resolve(info, &addrlist, NULL, NULL, BoundNetLog());
   EXPECT_EQ(OK, err);
 
@@ -422,7 +424,7 @@ TEST_F(HostResolverImplTest, NumericIPv6Address) {
       CreateHostResolverImpl(resolver_proc));
   AddressList addrlist;
   const int kPortnum = 5555;
-  HostResolver::RequestInfo info("2001:db8::1", kPortnum);
+  HostResolver::RequestInfo info(HostPortPair("2001:db8::1", kPortnum));
   int err = host_resolver->Resolve(info, &addrlist, NULL, NULL, BoundNetLog());
   EXPECT_EQ(OK, err);
 
@@ -452,7 +454,7 @@ TEST_F(HostResolverImplTest, EmptyHost) {
       CreateHostResolverImpl(resolver_proc));
   AddressList addrlist;
   const int kPortnum = 5555;
-  HostResolver::RequestInfo info("", kPortnum);
+  HostResolver::RequestInfo info(HostPortPair("", kPortnum));
   int err = host_resolver->Resolve(info, &addrlist, NULL, NULL, BoundNetLog());
   EXPECT_EQ(ERR_NAME_NOT_RESOLVED, err);
 }
@@ -467,7 +469,7 @@ TEST_F(HostResolverImplTest, LongHost) {
   AddressList addrlist;
   const int kPortnum = 5555;
   std::string hostname(4097, 'a');
-  HostResolver::RequestInfo info(hostname, kPortnum);
+  HostResolver::RequestInfo info(HostPortPair(hostname, kPortnum));
   int err = host_resolver->Resolve(info, &addrlist, NULL, NULL, BoundNetLog());
   EXPECT_EQ(ERR_NAME_NOT_RESOLVED, err);
 }
@@ -819,14 +821,14 @@ class BypassCacheVerifier : public ResolveRequest::Delegate {
           reinterpret_cast<CompletionCallback*> (1);
       AddressList addrlist;
 
-      HostResolver::RequestInfo info("a", 70);
+      HostResolver::RequestInfo info(HostPortPair("a", 70));
       int error = resolver->Resolve(info, &addrlist, junk_callback, NULL,
                                     BoundNetLog());
       EXPECT_EQ(OK, error);
 
       // Ok good. Now make sure that if we ask to bypass the cache, it can no
       // longer service the request synchronously.
-      info = HostResolver::RequestInfo("a", 71);
+      info = HostResolver::RequestInfo(HostPortPair("a", 71));
       info.set_allow_cached_response(false);
       final_request_.reset(new ResolveRequest(resolver, info, this));
     } else if (71 == resolve->port()) {
@@ -938,7 +940,7 @@ TEST_F(HostResolverImplTest, Observers) {
   AddressList addrlist;
 
   // Resolve "host1".
-  HostResolver::RequestInfo info1("host1", 70);
+  HostResolver::RequestInfo info1(HostPortPair("host1", 70));
   CapturingBoundNetLog log(CapturingNetLog::kUnbounded);
   int rv = host_resolver->Resolve(info1, &addrlist, NULL, NULL, log.bound());
   EXPECT_EQ(OK, rv);
@@ -972,7 +974,7 @@ TEST_F(HostResolverImplTest, Observers) {
               CapturingObserver::FinishEntry(1, true, info1));
 
   // Resolve "host2", setting referrer to "http://foobar.com"
-  HostResolver::RequestInfo info2("host2", 70);
+  HostResolver::RequestInfo info2(HostPortPair("host2", 70));
   info2.set_referrer(GURL("http://foobar.com"));
   rv = host_resolver->Resolve(info2, &addrlist, NULL, NULL, BoundNetLog());
   EXPECT_EQ(OK, rv);
@@ -989,7 +991,7 @@ TEST_F(HostResolverImplTest, Observers) {
   host_resolver->RemoveObserver(&observer);
 
   // Resolve "host3"
-  HostResolver::RequestInfo info3("host3", 70);
+  HostResolver::RequestInfo info3(HostPortPair("host3", 70));
   host_resolver->Resolve(info3, &addrlist, NULL, NULL, BoundNetLog());
 
   // No effect this time, since observer was removed.
@@ -1017,7 +1019,7 @@ TEST_F(HostResolverImplTest, CancellationObserver) {
     EXPECT_EQ(0U, observer.cancel_log.size());
 
     // Start an async resolve for (host1:70).
-    HostResolver::RequestInfo info1("host1", 70);
+    HostResolver::RequestInfo info1(HostPortPair("host1", 70));
     HostResolver::RequestHandle req = NULL;
     AddressList addrlist;
     int rv = host_resolver->Resolve(info1, &addrlist, &callback, &req,
@@ -1043,7 +1045,7 @@ TEST_F(HostResolverImplTest, CancellationObserver) {
                 CapturingObserver::StartOrCancelEntry(0, info1));
 
     // Start an async request for (host2:60)
-    HostResolver::RequestInfo info2("host2", 60);
+    HostResolver::RequestInfo info2(HostPortPair("host2", 60));
     rv = host_resolver->Resolve(info2, &addrlist, &callback, NULL,
                                 BoundNetLog());
     EXPECT_EQ(ERR_IO_PENDING, rv);
@@ -1067,7 +1069,7 @@ TEST_F(HostResolverImplTest, CancellationObserver) {
   EXPECT_EQ(0U, observer.finish_log.size());
   EXPECT_EQ(2U, observer.cancel_log.size());
 
-  HostResolver::RequestInfo info("host2", 60);
+  HostResolver::RequestInfo info(HostPortPair("host2", 60));
   EXPECT_TRUE(observer.cancel_log[1] ==
               CapturingObserver::StartOrCancelEntry(1, info));
 }
@@ -1080,7 +1082,7 @@ TEST_F(HostResolverImplTest, FlushCacheOnIPAddressChange) {
   AddressList addrlist;
 
   // Resolve "host1".
-  HostResolver::RequestInfo info1("host1", 70);
+  HostResolver::RequestInfo info1(HostPortPair("host1", 70));
   TestCompletionCallback callback;
   int rv = host_resolver->Resolve(info1, &addrlist, &callback, NULL,
                                   BoundNetLog());
@@ -1112,7 +1114,7 @@ TEST_F(HostResolverImplTest, AbortOnIPAddressChanged) {
       new HostResolverImpl(resolver_proc, cache, kMaxJobs, NULL));
 
   // Resolve "host1".
-  HostResolver::RequestInfo info("host1", 70);
+  HostResolver::RequestInfo info(HostPortPair("host1", 70));
   TestCompletionCallback callback;
   AddressList addrlist;
   int rv = host_resolver->Resolve(info, &addrlist, &callback, NULL,
@@ -1171,7 +1173,7 @@ TEST_F(HostResolverImplTest, OnlyAbortExistingRequestsOnIPAddressChange) {
   host_resolver->Reset(resolver_proc);
 
   // Resolve "host1".
-  HostResolver::RequestInfo info("host1", 70);
+  HostResolver::RequestInfo info(HostPortPair("host1", 70));
   ResolveWithinCallback callback(host_resolver, info);
   AddressList addrlist;
   int rv = host_resolver->Resolve(info, &addrlist, &callback, NULL,
diff --git a/net/base/mapped_host_resolver.cc b/net/base/mapped_host_resolver.cc
index 0349232..d81efc6 100644
--- a/net/base/mapped_host_resolver.cc
+++ b/net/base/mapped_host_resolver.cc
@@ -22,11 +22,9 @@ int MappedHostResolver::Resolve(const RequestInfo& info,
                                 const BoundNetLog& net_log) {
   // Modify the request before forwarding it to |impl_|.
   RequestInfo modified_info = info;
-  HostPortPair host_port(info.hostname(), info.port());
-  if (rules_.RewriteHost(&host_port)) {
-    modified_info.set_hostname(host_port.host());
-    modified_info.set_port(host_port.port());
-  }
+  HostPortPair host_port(info.host_port_pair());
+  if (rules_.RewriteHost(&host_port))
+    modified_info.set_host_port_pair(host_port);
   return impl_->Resolve(modified_info, addresses, callback, out_req, net_log);
 }
 
diff --git a/net/base/mapped_host_resolver_unittest.cc b/net/base/mapped_host_resolver_unittest.cc
index 4b7281c..06beeb7 100644
--- a/net/base/mapped_host_resolver_unittest.cc
+++ b/net/base/mapped_host_resolver_unittest.cc
@@ -31,7 +31,8 @@ TEST(MappedHostResolverTest, Inclusion) {
 
   // Try resolving "www.google.com:80". There are no mappings yet, so this
   // hits |resolver_impl| and fails.
-  rv = resolver->Resolve(HostResolver::RequestInfo("www.google.com", 80),
+  rv = resolver->Resolve(HostResolver::RequestInfo(
+                             HostPortPair("www.google.com", 80)),
                          &address_list, NULL, NULL, BoundNetLog());
   EXPECT_EQ(ERR_NAME_NOT_RESOLVED, rv);
 
@@ -39,7 +40,8 @@ TEST(MappedHostResolverTest, Inclusion) {
   EXPECT_TRUE(resolver->AddRuleFromString("map *.google.com baz.com"));
 
   // Try resolving "www.google.com:80". Should be remapped to "baz.com:80".
-  rv = resolver->Resolve(HostResolver::RequestInfo("www.google.com", 80),
+  rv = resolver->Resolve(HostResolver::RequestInfo(
+                             HostPortPair("www.google.com", 80)),
                          &address_list, NULL, NULL, BoundNetLog());
   EXPECT_EQ(OK, rv);
   EXPECT_EQ("192.168.1.5", NetAddressToString(address_list.head()));
@@ -47,7 +49,7 @@ TEST(MappedHostResolverTest, Inclusion) {
 
   // Try resolving "foo.com:77". This will NOT be remapped, so result
   // is "foo.com:77".
-  rv = resolver->Resolve(HostResolver::RequestInfo("foo.com", 77),
+  rv = resolver->Resolve(HostResolver::RequestInfo(HostPortPair("foo.com", 77)),
                          &address_list, NULL, NULL, BoundNetLog());
   EXPECT_EQ(OK, rv);
   EXPECT_EQ("192.168.1.8", NetAddressToString(address_list.head()));
@@ -57,7 +59,8 @@ TEST(MappedHostResolverTest, Inclusion) {
   EXPECT_TRUE(resolver->AddRuleFromString("Map *.org proxy:99"));
 
   // Try resolving "chromium.org:61". Should be remapped to "proxy:99".
-  rv = resolver->Resolve(HostResolver::RequestInfo("chromium.org", 61),
+  rv = resolver->Resolve(HostResolver::RequestInfo
+                             (HostPortPair("chromium.org", 61)),
                          &address_list, NULL, NULL, BoundNetLog());
   EXPECT_EQ(OK, rv);
   EXPECT_EQ("192.168.1.11", NetAddressToString(address_list.head()));
@@ -85,14 +88,16 @@ TEST(MappedHostResolverTest, Exclusion) {
   EXPECT_TRUE(resolver->AddRuleFromString("EXCLUDE *.google.com"));
 
   // Try resolving "www.google.com". Should not be remapped due to exclusion).
-  rv = resolver->Resolve(HostResolver::RequestInfo("www.google.com", 80),
+  rv = resolver->Resolve(HostResolver::RequestInfo(
+                             HostPortPair("www.google.com", 80)),
                          &address_list, NULL, NULL, BoundNetLog());
   EXPECT_EQ(OK, rv);
   EXPECT_EQ("192.168.1.3", NetAddressToString(address_list.head()));
   EXPECT_EQ(80, address_list.GetPort());
 
   // Try resolving "chrome.com:80". Should be remapped to "baz:80".
-  rv = resolver->Resolve(HostResolver::RequestInfo("chrome.com", 80),
+  rv = resolver->Resolve(HostResolver::RequestInfo(
+                             HostPortPair("chrome.com", 80)),
                          &address_list, NULL, NULL, BoundNetLog());
   EXPECT_EQ(OK, rv);
   EXPECT_EQ("192.168.1.5", NetAddressToString(address_list.head()));
@@ -116,14 +121,16 @@ TEST(MappedHostResolverTest, SetRulesFromString) {
   resolver->SetRulesFromString("map *.com baz , map *.net bar:60");
 
   // Try resolving "www.google.com". Should be remapped to "baz".
-  rv = resolver->Resolve(HostResolver::RequestInfo("www.google.com", 80),
+  rv = resolver->Resolve(HostResolver::RequestInfo(
+                             HostPortPair("www.google.com", 80)),
                          &address_list, NULL, NULL, BoundNetLog());
   EXPECT_EQ(OK, rv);
   EXPECT_EQ("192.168.1.7", NetAddressToString(address_list.head()));
   EXPECT_EQ(80, address_list.GetPort());
 
   // Try resolving "chrome.net:80". Should be remapped to "bar:60".
-  rv = resolver->Resolve(HostResolver::RequestInfo("chrome.net", 80),
+  rv = resolver->Resolve(HostResolver::RequestInfo(
+                             HostPortPair("chrome.net", 80)),
                          &address_list, NULL, NULL, BoundNetLog());
   EXPECT_EQ(OK, rv);
   EXPECT_EQ("192.168.1.9", NetAddressToString(address_list.head()));