Aesthetic changes to the proxy configuration log format.

Also in this change, I stop brodacasting the proxy settings changes to LOG(INFO) --- This is because navigating to "view-net-internal:proxyservice.config" offers a better solution, so no point polluting the log file.

Note that the format change is intended to make the string more succint and human readable. For example, here is a before and after comparison:

------------
New version:
------------

Automatic settings:
  Auto-detect: No
  Custom PAC script: [None]
Manual settings:
  Proxy server: [None]
  Bypass list: [None]
  Bypass local names: No

------------
Old version:
------------

{
  auto_detect: 0
  pac_url: 
  proxy_rules:
  {
    type: TYPE_NO_RULES
    single_proxy: 
    proxy_for_http: 
    proxy_for_https: 
    proxy_for_ftp: 
    socks_proxy: 
  }
  proxy_bypass_local_names: 0
  proxy_bypass_list:
  id: 1
}
 
BUG=http://crbug.com/14478
TEST=ProxyConfigTest.ToString

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

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

3 files changed, 192 insertions, 40 deletions

diff --git a/net/proxy/proxy_config.cc b/net/proxy/proxy_config.cc
index ab719e6..5284327 100644
--- a/net/proxy/proxy_config.cc
+++ b/net/proxy/proxy_config.cc
@@ -175,6 +175,10 @@ std::ostream& operator<<(std::ostream& out,
   return out;
 }
 
+const char* BoolToYesNoString(bool b) {
+  return b ? "Yes" : "No";
+}
+
 }  // namespace
 
 std::ostream& operator<<(std::ostream& out,
@@ -206,26 +210,54 @@ std::ostream& operator<<(std::ostream& out,
 }
 
 std::ostream& operator<<(std::ostream& out, const net::ProxyConfig& config) {
-  out << "{\n"
-      << "  auto_detect: " << config.auto_detect << "\n"
-      << "  pac_url: " << config.pac_url << "\n"
-      << "  proxy_rules:\n" << config.proxy_rules << "\n"
-      << "  proxy_bypass_local_names: " << config.proxy_bypass_local_names
-      << "\n"
-      << "  proxy_bypass_list:\n";
-
-  // Print out the proxy bypass list.
-  if (!config.proxy_bypass.empty()) {
-    out << "  {\n";
+  // "Automatic" settings.
+  out << "Automatic settings:\n";
+  out << "  Auto-detect: " << BoolToYesNoString(config.auto_detect) << "\n";
+  out << "  Custom PAC script: ";
+  if (config.pac_url.is_valid())
+    out << config.pac_url;
+  else
+    out << "[None]";
+  out << "\n";
+
+  // "Manual" settings.
+  out << "Manual settings:\n";
+  out << "  Proxy server: ";
+
+  switch (config.proxy_rules.type) {
+    case net::ProxyConfig::ProxyRules::TYPE_NO_RULES:
+      out << "[None]\n";
+      break;
+    case net::ProxyConfig::ProxyRules::TYPE_SINGLE_PROXY:
+      out << config.proxy_rules.single_proxy;
+      out << "\n";
+      break;
+    case net::ProxyConfig::ProxyRules::TYPE_PROXY_PER_SCHEME:
+      out << "\n";
+      if (config.proxy_rules.proxy_for_http.is_valid())
+        out << "    HTTP: " << config.proxy_rules.proxy_for_http << "\n";
+      if (config.proxy_rules.proxy_for_https.is_valid())
+        out << "    HTTPS: " << config.proxy_rules.proxy_for_https << "\n";
+      if (config.proxy_rules.proxy_for_ftp.is_valid())
+        out << "    FTP: " << config.proxy_rules.proxy_for_ftp << "\n";
+      if (config.proxy_rules.socks_proxy.is_valid())
+        out << "    SOCKS: " << config.proxy_rules.socks_proxy << "\n";
+      break;
+  }
+
+  out << "  Bypass list: ";
+  if (config.proxy_bypass.empty()) {
+    out << "[None]\n";
+  } else {
+    out << "\n";
     std::vector<std::string>::const_iterator it;
     for (it = config.proxy_bypass.begin();
          it != config.proxy_bypass.end(); ++it) {
       out << "    " << *it << "\n";
     }
-    out << "  }\n";
   }
 
-  out << "  id: " << config.id() << "\n"
-      << "}";
+  out << "  Bypass local names: "
+      << BoolToYesNoString(config.proxy_bypass_local_names);
   return out;
 }
diff --git a/net/proxy/proxy_config_unittest.cc b/net/proxy/proxy_config_unittest.cc
index d7ae65b..c72c8c5 100644
--- a/net/proxy/proxy_config_unittest.cc
+++ b/net/proxy/proxy_config_unittest.cc
@@ -2,28 +2,31 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
+#include <ostream>
+
 #include "net/proxy/proxy_config.h"
 #include "net/proxy/proxy_config_service_common_unittest.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
+namespace net {
 namespace {
-static void ExpectProxyServerEquals(const char* expectation,
-                                    const net::ProxyServer& proxy_server) {
+
+void ExpectProxyServerEquals(const char* expectation,
+                             const ProxyServer& proxy_server) {
   if (expectation == NULL) {
     EXPECT_FALSE(proxy_server.is_valid());
   } else {
     EXPECT_EQ(expectation, proxy_server.ToURI());
   }
 }
-}
 
 TEST(ProxyConfigTest, Equals) {
   // Test |ProxyConfig::auto_detect|.
 
-  net::ProxyConfig config1;
+  ProxyConfig config1;
   config1.auto_detect = true;
 
-  net::ProxyConfig config2;
+  ProxyConfig config2;
   config2.auto_detect = false;
 
   EXPECT_FALSE(config1.Equals(config2));
@@ -48,22 +51,22 @@ TEST(ProxyConfigTest, Equals) {
 
   // Test |ProxyConfig::proxy_rules|.
 
-  config2.proxy_rules.type = net::ProxyConfig::ProxyRules::TYPE_SINGLE_PROXY;
+  config2.proxy_rules.type = ProxyConfig::ProxyRules::TYPE_SINGLE_PROXY;
   config2.proxy_rules.single_proxy =
-      net::ProxyServer::FromURI("myproxy:80", net::ProxyServer::SCHEME_HTTP);
+      ProxyServer::FromURI("myproxy:80", ProxyServer::SCHEME_HTTP);
 
   EXPECT_FALSE(config1.Equals(config2));
   EXPECT_FALSE(config2.Equals(config1));
 
-  config1.proxy_rules.type = net::ProxyConfig::ProxyRules::TYPE_SINGLE_PROXY;
+  config1.proxy_rules.type = ProxyConfig::ProxyRules::TYPE_SINGLE_PROXY;
   config1.proxy_rules.single_proxy =
-      net::ProxyServer::FromURI("myproxy:100", net::ProxyServer::SCHEME_HTTP);
+      ProxyServer::FromURI("myproxy:100", ProxyServer::SCHEME_HTTP);
 
   EXPECT_FALSE(config1.Equals(config2));
   EXPECT_FALSE(config2.Equals(config1));
 
   config1.proxy_rules.single_proxy =
-      net::ProxyServer::FromURI("myproxy", net::ProxyServer::SCHEME_HTTP);
+      ProxyServer::FromURI("myproxy", ProxyServer::SCHEME_HTTP);
 
   EXPECT_TRUE(config1.Equals(config2));
   EXPECT_TRUE(config2.Equals(config1));
@@ -97,7 +100,7 @@ TEST(ProxyConfigTest, ParseProxyRules) {
   const struct {
     const char* proxy_rules;
 
-    net::ProxyConfig::ProxyRules::Type type;
+    ProxyConfig::ProxyRules::Type type;
     const char* single_proxy;
     const char* proxy_for_http;
     const char* proxy_for_https;
@@ -108,7 +111,7 @@ TEST(ProxyConfigTest, ParseProxyRules) {
     {
       "myproxy:80",
 
-      net::ProxyConfig::ProxyRules::TYPE_SINGLE_PROXY,
+      ProxyConfig::ProxyRules::TYPE_SINGLE_PROXY,
       "myproxy:80",
       NULL,
       NULL,
@@ -120,7 +123,7 @@ TEST(ProxyConfigTest, ParseProxyRules) {
     {
       "http=myproxy:80",
 
-      net::ProxyConfig::ProxyRules::TYPE_PROXY_PER_SCHEME,
+      ProxyConfig::ProxyRules::TYPE_PROXY_PER_SCHEME,
       NULL,
       "myproxy:80",
       NULL,
@@ -132,7 +135,7 @@ TEST(ProxyConfigTest, ParseProxyRules) {
     {
       "ftp=ftp-proxy ; https=socks4://foopy",
 
-      net::ProxyConfig::ProxyRules::TYPE_PROXY_PER_SCHEME,
+      ProxyConfig::ProxyRules::TYPE_PROXY_PER_SCHEME,
       NULL,
       NULL,
       "socks4://foopy:1080",
@@ -146,7 +149,7 @@ TEST(ProxyConfigTest, ParseProxyRules) {
     {
       "foopy ; ftp=ftp-proxy",
 
-      net::ProxyConfig::ProxyRules::TYPE_SINGLE_PROXY,
+      ProxyConfig::ProxyRules::TYPE_SINGLE_PROXY,
       "foopy:80",
       NULL,
       NULL,
@@ -160,7 +163,7 @@ TEST(ProxyConfigTest, ParseProxyRules) {
     {
       "ftp=ftp-proxy ; foopy",
 
-      net::ProxyConfig::ProxyRules::TYPE_PROXY_PER_SCHEME,
+      ProxyConfig::ProxyRules::TYPE_PROXY_PER_SCHEME,
       NULL,
       NULL,
       NULL,
@@ -172,7 +175,7 @@ TEST(ProxyConfigTest, ParseProxyRules) {
     {
       "ftp=ftp1 ; ftp=ftp2 ; ftp=ftp3",
 
-      net::ProxyConfig::ProxyRules::TYPE_PROXY_PER_SCHEME,
+      ProxyConfig::ProxyRules::TYPE_PROXY_PER_SCHEME,
       NULL,
       NULL,
       NULL,
@@ -184,7 +187,7 @@ TEST(ProxyConfigTest, ParseProxyRules) {
     {
       "socks=foopy",
 
-      net::ProxyConfig::ProxyRules::TYPE_PROXY_PER_SCHEME,
+      ProxyConfig::ProxyRules::TYPE_PROXY_PER_SCHEME,
       NULL,
       NULL,
       NULL,
@@ -196,7 +199,7 @@ TEST(ProxyConfigTest, ParseProxyRules) {
     {
       "http=httpproxy ; https=httpsproxy ; ftp=ftpproxy ; socks=foopy ",
 
-      net::ProxyConfig::ProxyRules::TYPE_PROXY_PER_SCHEME,
+      ProxyConfig::ProxyRules::TYPE_PROXY_PER_SCHEME,
       NULL,
       "httpproxy:80",
       "httpsproxy:80",
@@ -209,7 +212,7 @@ TEST(ProxyConfigTest, ParseProxyRules) {
     {
       "http=httpproxy ; https=httpsproxy ; socks=socks5://foopy ",
 
-      net::ProxyConfig::ProxyRules::TYPE_PROXY_PER_SCHEME,
+      ProxyConfig::ProxyRules::TYPE_PROXY_PER_SCHEME,
       NULL,
       "httpproxy:80",
       "httpsproxy:80",
@@ -221,7 +224,7 @@ TEST(ProxyConfigTest, ParseProxyRules) {
     {
       "crazy=foopy ; foo=bar ; https=myhttpsproxy",
 
-      net::ProxyConfig::ProxyRules::TYPE_PROXY_PER_SCHEME,
+      ProxyConfig::ProxyRules::TYPE_PROXY_PER_SCHEME,
       NULL,
       NULL,
       "myhttpsproxy:80",
@@ -230,7 +233,7 @@ TEST(ProxyConfigTest, ParseProxyRules) {
     },
   };
 
-  net::ProxyConfig config;
+  ProxyConfig config;
 
   for (size_t i = 0; i < ARRAYSIZE_UNSAFE(tests); ++i) {
     config.proxy_rules.ParseFromString(tests[i].proxy_rules);
@@ -273,11 +276,130 @@ TEST(ProxyConfigTest, ParseProxyBypassList) {
     }
   };
 
-  net::ProxyConfig config;
+  ProxyConfig config;
 
   for (size_t i = 0; i < ARRAYSIZE_UNSAFE(tests); ++i) {
     config.ParseNoProxyList(tests[i].proxy_bypass_input);
     EXPECT_EQ(tests[i].flattened_output,
-              net::FlattenProxyBypass(config.proxy_bypass));
+              FlattenProxyBypass(config.proxy_bypass));
+  }
+}
+
+std::string ProxyConfigToString(const ProxyConfig& config) {
+  std::ostringstream stream;
+  stream << config;
+  return stream.str();
+}
+
+TEST(ProxyConfigTest, ToString) {
+  // Manual proxy.
+  {
+    ProxyConfig config;
+    config.auto_detect = false;
+    config.proxy_rules.ParseFromString("http://single-proxy:81");
+
+    EXPECT_EQ("Automatic settings:\n"
+              "  Auto-detect: No\n"
+              "  Custom PAC script: [None]\n"
+              "Manual settings:\n"
+              "  Proxy server: single-proxy:81\n"
+              "  Bypass list: [None]\n"
+              "  Bypass local names: No",
+              ProxyConfigToString(config));
+  }
+
+  // Autodetect + custom PAC + manual proxy.
+  {
+    ProxyConfig config;
+    config.auto_detect = true;
+    config.pac_url = GURL("http://custom/pac.js");
+    config.proxy_rules.ParseFromString("http://single-proxy:81");
+
+    EXPECT_EQ("Automatic settings:\n"
+              "  Auto-detect: Yes\n"
+              "  Custom PAC script: http://custom/pac.js\n"
+              "Manual settings:\n"
+              "  Proxy server: single-proxy:81\n"
+              "  Bypass list: [None]\n"
+              "  Bypass local names: No",
+              ProxyConfigToString(config));
+  }
+
+  // Manual proxy with bypass list + bypass local.
+  {
+    ProxyConfig config;
+    config.auto_detect = false;
+    config.proxy_rules.ParseFromString("http://single-proxy:81");
+    config.proxy_bypass.push_back("google.com");
+    config.proxy_bypass.push_back("bypass2.net:1730");
+    config.proxy_bypass_local_names = true;
+
+    EXPECT_EQ("Automatic settings:\n"
+              "  Auto-detect: No\n"
+              "  Custom PAC script: [None]\n"
+              "Manual settings:\n"
+              "  Proxy server: single-proxy:81\n"
+              "  Bypass list: \n"
+              "    google.com\n"
+              "    bypass2.net:1730\n"
+              "  Bypass local names: Yes",
+              ProxyConfigToString(config));
+  }
+
+  // Proxy-per scheme (HTTP and HTTPS)
+  {
+    ProxyConfig config;
+    config.auto_detect = false;
+    config.proxy_rules.ParseFromString(
+        "http=proxy-for-http:1801; https=proxy-for-https:1802");
+
+    EXPECT_EQ("Automatic settings:\n"
+              "  Auto-detect: No\n"
+              "  Custom PAC script: [None]\n"
+              "Manual settings:\n"
+              "  Proxy server: \n"
+              "    HTTP: proxy-for-http:1801\n"
+              "    HTTPS: proxy-for-https:1802\n"
+              "  Bypass list: [None]\n"
+              "  Bypass local names: No",
+              ProxyConfigToString(config));
+  }
+
+  // Proxy-per scheme (HTTP and SOCKS)
+  {
+    ProxyConfig config;
+    config.auto_detect = false;
+    config.proxy_rules.ParseFromString(
+        "http=http://proxy-for-http:1801; socks=socks-server:6083");
+
+    EXPECT_EQ("Automatic settings:\n"
+              "  Auto-detect: No\n"
+              "  Custom PAC script: [None]\n"
+              "Manual settings:\n"
+              "  Proxy server: \n"
+              "    HTTP: proxy-for-http:1801\n"
+              "    SOCKS: socks4://socks-server:6083\n"
+              "  Bypass list: [None]\n"
+              "  Bypass local names: No",
+              ProxyConfigToString(config));
+  }
+
+  // No proxy.
+  {
+    ProxyConfig config;
+    config.auto_detect = false;
+
+    EXPECT_EQ("Automatic settings:\n"
+              "  Auto-detect: No\n"
+              "  Custom PAC script: [None]\n"
+              "Manual settings:\n"
+              "  Proxy server: [None]\n"
+              "  Bypass list: [None]\n"
+              "  Bypass local names: No",
+              ProxyConfigToString(config));
   }
 }
+
+}  // namespace
+}  // namespace net
+
diff --git a/net/proxy/proxy_service.cc b/net/proxy/proxy_service.cc
index 975b7df..46509c2 100644
--- a/net/proxy/proxy_service.cc
+++ b/net/proxy/proxy_service.cc
@@ -608,8 +608,6 @@ void ProxyService::SetConfig(const ProxyConfig& config) {
   // Increment the ID to reflect that the config has changed.
   config_.set_id(next_config_id_++);
 
-  LOG(INFO) << "New proxy configuration was loaded:\n" << config_;
-
   // Reset state associated with latest config.
   config_is_bad_ = false;
   proxy_retry_info_.clear();