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

6 files changed, 109 insertions, 78 deletions

diff --git a/base/linux_util.cc b/base/linux_util.cc
index bc99d44..259899f 100644
--- a/base/linux_util.cc
+++ b/base/linux_util.cc
@@ -99,14 +99,25 @@ EnvironmentVariableGetter* EnvironmentVariableGetter::Create() {
   return new EnvironmentVariableGetterImpl();
 }
 
-bool UseGnomeForSettings(EnvironmentVariableGetter* env_var_getter) {
-  // GNOME_DESKTOP_SESSION_ID being defined is a good indication that
-  // we are probably running under GNOME.
-  // Note: KDE_FULL_SESSION is a corresponding env var to recognize KDE.
-  std::string dummy, desktop_session;
-  return env_var_getter->Getenv("GNOME_DESKTOP_SESSION_ID", &dummy)
-      || (env_var_getter->Getenv("DESKTOP_SESSION", &desktop_session)
-          && desktop_session == "gnome");
+DesktopEnvironment GetDesktopEnvironment(EnvironmentVariableGetter* env) {
+  std::string desktop_session;
+  if (env->Getenv("DESKTOP_SESSION", &desktop_session)) {
+    if (desktop_session == "gnome")
+      return DESKTOP_ENVIRONMENT_GNOME;
+    else if (desktop_session.substr(3) == "kde")  // kde3 or kde4
+      return DESKTOP_ENVIRONMENT_KDE;
+  }
+
+  // Fall back on some older environment variables.
+  // Useful particularly in the DESKTOP_SESSION=default case.
+  std::string dummy;
+  if (env->Getenv("GNOME_DESKTOP_SESSION_ID", &dummy)) {
+    return DESKTOP_ENVIRONMENT_GNOME;
+  } else if (env->Getenv("KDE_FULL_SESSION", &dummy)) {
+    return DESKTOP_ENVIRONMENT_KDE;
+  }
+
+  return DESKTOP_ENVIRONMENT_OTHER;
 }
 
 }  // namespace base
diff --git a/base/linux_util.h b/base/linux_util.h
index 5a46481..693377c 100644
--- a/base/linux_util.h
+++ b/base/linux_util.h
@@ -32,11 +32,17 @@ class EnvironmentVariableGetter {
   static EnvironmentVariableGetter* Create();
 };
 
-// Return true if we appear to be running under Gnome and should attempt to use
-// some prefrences from the desktop environment (eg proxy settings),
-// If someone adds support for other environments, this function could be
-// replaced with one that returns an enum so we an specify Gnome, KDE, etc.
-bool UseGnomeForSettings(EnvironmentVariableGetter* env_var_getter);
+enum DesktopEnvironment {
+  DESKTOP_ENVIRONMENT_OTHER,
+  DESKTOP_ENVIRONMENT_GNOME,
+  DESKTOP_ENVIRONMENT_KDE,
+};
+
+// Return an entry from the DesktopEnvironment enum with a best guess
+// of which desktop environment we're using.  We use this to know when
+// to attempt to use preferences from the desktop environment --
+// proxy settings, password manager, etc.
+DesktopEnvironment GetDesktopEnvironment(EnvironmentVariableGetter* env);
 
 }  // namespace base
 
diff --git a/chrome/browser/gtk/options/advanced_contents_gtk.cc b/chrome/browser/gtk/options/advanced_contents_gtk.cc
index 7aa21b28..fdae7b7 100644
--- a/chrome/browser/gtk/options/advanced_contents_gtk.cc
+++ b/chrome/browser/gtk/options/advanced_contents_gtk.cc
@@ -302,23 +302,33 @@ void NetworkSection::OnChangeProxiesButtonClicked(GtkButton *button,
 
   scoped_ptr<base::EnvironmentVariableGetter> env_getter(
       base::EnvironmentVariableGetter::Create());
-  if (base::UseGnomeForSettings(env_getter.get())) {
-    std::vector<std::string> argv;
-    argv.push_back(kProxyConfigBinary);
-    base::file_handle_mapping_vector no_files;
-    base::environment_vector env;
-    base::ProcessHandle handle;
-    env.push_back(std::make_pair("GTK_PATH",
-                                 getenv("CHROMIUM_SAVED_GTK_PATH")));
-    if (!base::LaunchApp(argv, env, no_files, false, &handle)) {
-      LOG(ERROR) << "OpenProxyConfigDialogTask failed";
-      return;
+
+  switch (base::GetDesktopEnvironment(env_getter.get())) {
+    case base::DESKTOP_ENVIRONMENT_GNOME: {
+      std::vector<std::string> argv;
+      argv.push_back(kProxyConfigBinary);
+      base::file_handle_mapping_vector no_files;
+      base::environment_vector env;
+      base::ProcessHandle handle;
+      env.push_back(std::make_pair("GTK_PATH",
+                                   getenv("CHROMIUM_SAVED_GTK_PATH")));
+      if (!base::LaunchApp(argv, env, no_files, false, &handle)) {
+        LOG(ERROR) << "OpenProxyConfigDialogTask failed";
+        return;
+      }
+      ProcessWatcher::EnsureProcessGetsReaped(handle);
+      break;
     }
-    ProcessWatcher::EnsureProcessGetsReaped(handle);
-  } else {
-    BrowserList::GetLastActive()->
-        OpenURL(GURL(l10n_util::GetStringUTF8(IDS_LINUX_PROXY_CONFIG_URL)),
-                GURL(), NEW_FOREGROUND_TAB, PageTransition::LINK);
+
+    case base::DESKTOP_ENVIRONMENT_KDE:
+      NOTIMPLEMENTED() << "Bug 17363: obey KDE proxy settings.";
+      // Fall through to default behavior for now.
+
+    case base::DESKTOP_ENVIRONMENT_OTHER:
+      BrowserList::GetLastActive()->
+          OpenURL(GURL(l10n_util::GetStringUTF8(IDS_LINUX_PROXY_CONFIG_URL)),
+                  GURL(), NEW_FOREGROUND_TAB, PageTransition::LINK);
+      break;
   }
 }
 
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