linux: generalize desktop environment guessing to encompass KDE

BUG=17363

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

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

3 files changed, 53 insertions, 49 deletions

diff --git a/net/proxy/proxy_config_service_linux.cc b/net/proxy/proxy_config_service_linux.cc
index b8a2ce4..3c74c4d 100644
--- a/net/proxy/proxy_config_service_linux.cc
+++ b/net/proxy/proxy_config_service_linux.cc
@@ -504,17 +504,6 @@ ProxyConfigServiceLinux::Delegate::Delegate(
       glib_default_loop_(NULL), io_loop_(NULL) {
 }
 
-bool ProxyConfigServiceLinux::Delegate::ShouldTryGConf() {
-  // I (sdoyon) would have liked to prioritize environment variables
-  // and only fallback to gconf if env vars were unset. But
-  // gnome-terminal "helpfully" sets http_proxy and no_proxy, and it
-  // does so even if the proxy mode is set to auto, which would
-  // mislead us.
-  //
-  // We could introduce a CHROME_PROXY_OBEY_ENV_VARS variable...??
-  return base::UseGnomeForSettings(env_var_getter_.get());
-}
-
 void ProxyConfigServiceLinux::Delegate::SetupAndFetchInitialConfig(
     MessageLoop* glib_default_loop, MessageLoop* io_loop) {
   // We should be running on the default glib main loop thread right
@@ -534,29 +523,47 @@ void ProxyConfigServiceLinux::Delegate::SetupAndFetchInitialConfig(
   // will expect to find it. This is safe to do because we return
   // before this ProxyConfigServiceLinux is passed on to
   // the ProxyService.
+
+  // Note: It would be nice to prioritize environment variables
+  // and only fallback to gconf if env vars were unset. But
+  // gnome-terminal "helpfully" sets http_proxy and no_proxy, and it
+  // does so even if the proxy mode is set to auto, which would
+  // mislead us.
+
   bool got_config = false;
-  if (ShouldTryGConf() &&
-      gconf_getter_->Init() &&
-      (!io_loop || gconf_getter_->SetupNotification(this))) {
-    if (GetConfigFromGConf(&cached_config_)) {
-      cached_config_.set_id(1);  // mark it as valid
-      got_config = true;
-      LOG(INFO) << "Obtained proxy setting from gconf";
-      // If gconf proxy mode is "none", meaning direct, then we take
-      // that to be a valid config and will not check environment
-      // variables.  The alternative would have been to look for a proxy
-      // where ever we can find one.
-      //
-      // Keep a copy of the config for use from this thread for
-      // comparison with updated settings when we get notifications.
-      reference_config_ = cached_config_;
-      reference_config_.set_id(1);  // mark it as valid
-    } else {
-      gconf_getter_->Release();  // Stop notifications
-    }
+  switch (base::GetDesktopEnvironment(env_var_getter_.get())) {
+    case base::DESKTOP_ENVIRONMENT_GNOME:
+      if (gconf_getter_->Init() &&
+          (!io_loop || gconf_getter_->SetupNotification(this))) {
+        if (GetConfigFromGConf(&cached_config_)) {
+          cached_config_.set_id(1);  // mark it as valid
+          got_config = true;
+          LOG(INFO) << "Obtained proxy setting from gconf";
+          // If gconf proxy mode is "none", meaning direct, then we take
+          // that to be a valid config and will not check environment
+          // variables.  The alternative would have been to look for a proxy
+          // where ever we can find one.
+          //
+          // Keep a copy of the config for use from this thread for
+          // comparison with updated settings when we get notifications.
+          reference_config_ = cached_config_;
+          reference_config_.set_id(1);  // mark it as valid
+        } else {
+          gconf_getter_->Release();  // Stop notifications
+        }
+      }
+      break;
+
+    case base::DESKTOP_ENVIRONMENT_KDE:
+      NOTIMPLEMENTED() << "Bug 17363: obey KDE proxy settings.";
+      break;
+
+    case base::DESKTOP_ENVIRONMENT_OTHER:
+      break;
   }
+
   if (!got_config) {
-    // An implementation for KDE settings would be welcome here.
+    // We fall back on environment variables.
     //
     // Consulting environment variables doesn't need to be done from
     // the default glib main loop, but it's a tiny enough amount of
diff --git a/net/proxy/proxy_config_service_linux.h b/net/proxy/proxy_config_service_linux.h
index 00c31ff..a95a288 100644
--- a/net/proxy/proxy_config_service_linux.h
+++ b/net/proxy/proxy_config_service_linux.h
@@ -135,10 +135,6 @@ class ProxyConfigServiceLinux : public ProxyConfigService {
     // and the configuration is valid.
     bool GetConfigFromGConf(ProxyConfig* config);
 
-    // Returns true if environment variables indicate that we are
-    // running GNOME (and therefore we want to use gconf settings).
-    bool ShouldTryGConf();
-
     // This method is posted from the UI thread to the IO thread to
     // carry the new config information.
     void SetNewProxyConfig(const ProxyConfig& new_config);
diff --git a/net/proxy/proxy_config_service_linux_unittest.cc b/net/proxy/proxy_config_service_linux_unittest.cc
index 50c5019..f208a06 100644
--- a/net/proxy/proxy_config_service_linux_unittest.cc
+++ b/net/proxy/proxy_config_service_linux_unittest.cc
@@ -26,7 +26,7 @@ namespace {
 struct EnvVarValues {
   // The strange capitalization is so that the field matches the
   // environment variable name exactly.
-  const char *GNOME_DESKTOP_SESSION_ID, *DESKTOP_SESSION,
+  const char *DESKTOP_SESSION, *GNOME_DESKTOP_SESSION_ID, *KDE_FULL_SESSION,
       *auto_proxy, *all_proxy,
       *http_proxy, *https_proxy, *ftp_proxy,
       *SOCKS_SERVER, *SOCKS_VERSION,
@@ -78,8 +78,9 @@ class MockEnvironmentVariableGetter : public base::EnvironmentVariableGetter {
  public:
   MockEnvironmentVariableGetter() {
 #define ENTRY(x) table.settings[#x] = &values.x
-    ENTRY(GNOME_DESKTOP_SESSION_ID);
     ENTRY(DESKTOP_SESSION);
+    ENTRY(GNOME_DESKTOP_SESSION_ID);
+    ENTRY(KDE_FULL_SESSION);
     ENTRY(auto_proxy);
     ENTRY(all_proxy);
     ENTRY(http_proxy);
@@ -588,7 +589,7 @@ TEST(ProxyConfigServiceLinuxTest, BasicEnvTest) {
     {
       TEST_DESC("No proxying"),
       { // Input.
-        NULL, NULL,  // *DESKTOP*
+        NULL, NULL, NULL,  // *DESKTOP*
         NULL,  // auto_proxy
         NULL,  // all_proxy
         NULL, NULL, NULL,  // per-proto proxies
@@ -607,7 +608,7 @@ TEST(ProxyConfigServiceLinuxTest, BasicEnvTest) {
     {
       TEST_DESC("Auto detect"),
       { // Input.
-        NULL, NULL,  // *DESKTOP*
+        NULL, NULL, NULL,  // *DESKTOP*
         "",  // auto_proxy
         NULL,  // all_proxy
         NULL, NULL, NULL,  // per-proto proxies
@@ -626,7 +627,7 @@ TEST(ProxyConfigServiceLinuxTest, BasicEnvTest) {
     {
       TEST_DESC("Valid PAC url"),
       { // Input.
-        NULL, NULL,  // *DESKTOP*
+        NULL, NULL, NULL,  // *DESKTOP*
         "http://wpad/wpad.dat",  // auto_proxy
         NULL,  // all_proxy
         NULL, NULL, NULL,  // per-proto proxies
@@ -645,7 +646,7 @@ TEST(ProxyConfigServiceLinuxTest, BasicEnvTest) {
     {
       TEST_DESC("Invalid PAC url"),
       { // Input.
-        NULL, NULL,  // *DESKTOP*
+        NULL, NULL, NULL,  // *DESKTOP*
         "wpad.dat",  // auto_proxy
         NULL,  // all_proxy
         NULL, NULL, NULL,  // per-proto proxies
@@ -664,7 +665,7 @@ TEST(ProxyConfigServiceLinuxTest, BasicEnvTest) {
     {
       TEST_DESC("Single-host in proxy list"),
       { // Input.
-        NULL, NULL,  // *DESKTOP*
+        NULL, NULL, NULL,  // *DESKTOP*
         NULL,  // auto_proxy
         "www.google.com",  // all_proxy
         NULL, NULL, NULL,  // per-proto proxies
@@ -683,7 +684,7 @@ TEST(ProxyConfigServiceLinuxTest, BasicEnvTest) {
     {
       TEST_DESC("Single-host, different port"),
       { // Input.
-        NULL, NULL,  // *DESKTOP*
+        NULL, NULL, NULL,  // *DESKTOP*
         NULL,  // auto_proxy
         "www.google.com:99",  // all_proxy
         NULL, NULL, NULL,  // per-proto proxies
@@ -702,7 +703,7 @@ TEST(ProxyConfigServiceLinuxTest, BasicEnvTest) {
     {
       TEST_DESC("Tolerate a scheme"),
       { // Input.
-        NULL, NULL,  // *DESKTOP*
+        NULL, NULL, NULL,  // *DESKTOP*
         NULL,  // auto_proxy
         "http://www.google.com:99",  // all_proxy
         NULL, NULL, NULL,  // per-proto proxies
@@ -721,7 +722,7 @@ TEST(ProxyConfigServiceLinuxTest, BasicEnvTest) {
     {
       TEST_DESC("Per-scheme proxy rules"),
       { // Input.
-        NULL, NULL,  // *DESKTOP*
+        NULL, NULL, NULL,  // *DESKTOP*
         NULL,  // auto_proxy
         NULL,  // all_proxy
         "www.google.com:80", "www.foo.com:110", "ftpfoo.com:121",  // per-proto
@@ -741,7 +742,7 @@ TEST(ProxyConfigServiceLinuxTest, BasicEnvTest) {
     {
       TEST_DESC("socks"),
       { // Input.
-        NULL, NULL,  // *DESKTOP*
+        NULL, NULL, NULL,  // *DESKTOP*
         NULL,  // auto_proxy
         "",  // all_proxy
         NULL, NULL, NULL,  // per-proto proxies
@@ -760,7 +761,7 @@ TEST(ProxyConfigServiceLinuxTest, BasicEnvTest) {
     {
       TEST_DESC("socks5"),
       { // Input.
-        NULL, NULL,  // *DESKTOP*
+        NULL, NULL, NULL,  // *DESKTOP*
         NULL,  // auto_proxy
         "",  // all_proxy
         NULL, NULL, NULL,  // per-proto proxies
@@ -779,7 +780,7 @@ TEST(ProxyConfigServiceLinuxTest, BasicEnvTest) {
     {
       TEST_DESC("socks default port"),
       { // Input.
-        NULL, NULL,  // *DESKTOP*
+        NULL, NULL, NULL,  // *DESKTOP*
         NULL,  // auto_proxy
         "",  // all_proxy
         NULL, NULL, NULL,  // per-proto proxies
@@ -798,7 +799,7 @@ TEST(ProxyConfigServiceLinuxTest, BasicEnvTest) {
     {
       TEST_DESC("bypass"),
       { // Input.
-        NULL, NULL,  // *DESKTOP*
+        NULL, NULL, NULL,  // *DESKTOP*
         NULL,  // auto_proxy
         "www.google.com",  // all_proxy
         NULL, NULL, NULL,  // per-proto