Try again: Add proxy config (using gnome-network-preferences)

BUG=11507
TEST=Open options, click change proxy, gnome-network-preferences should launch.  If gnome isn't installed or running, LinuxProxyConfig wiki page should load.

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

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

7 files changed, 142 insertions, 55 deletions

diff --git a/base/linux_util.cc b/base/linux_util.cc
index 604980b..bc99d44 100644
--- a/base/linux_util.cc
+++ b/base/linux_util.cc
@@ -10,6 +10,41 @@
 
 #include "base/command_line.h"
 #include "base/process_util.h"
+#include "base/string_util.h"
+
+namespace {
+
+class EnvironmentVariableGetterImpl
+    : public base::EnvironmentVariableGetter {
+ public:
+  virtual bool Getenv(const char* variable_name, std::string* result) {
+    const char* env_value = ::getenv(variable_name);
+    if (env_value) {
+      // Note that the variable may be defined but empty.
+      *result = env_value;
+      return true;
+    }
+    // Some commonly used variable names are uppercase while others
+    // are lowercase, which is inconsistent. Let's try to be helpful
+    // and look for a variable name with the reverse case.
+    char first_char = variable_name[0];
+    std::string alternate_case_var;
+    if (first_char >= 'a' && first_char <= 'z')
+      alternate_case_var = StringToUpperASCII(std::string(variable_name));
+    else if (first_char >= 'A' && first_char <= 'Z')
+      alternate_case_var = StringToLowerASCII(std::string(variable_name));
+    else
+      return false;
+    env_value = ::getenv(alternate_case_var.c_str());
+    if (env_value) {
+      *result = env_value;
+      return true;
+    }
+    return false;
+  }
+};
+
+} // anonymous namespace
 
 namespace base {
 
@@ -59,4 +94,19 @@ std::string GetLinuxDistro() {
   return linux_distro;
 }
 
+// static
+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");
+}
+
 }  // namespace base
diff --git a/base/linux_util.h b/base/linux_util.h
index 973a2b0..5a46481 100644
--- a/base/linux_util.h
+++ b/base/linux_util.h
@@ -20,6 +20,24 @@ uint8_t* BGRAToRGBA(const uint8_t* pixels, int width, int height, int stride);
 // GetWinVersion() in base/win_util.h.
 std::string GetLinuxDistro();
 
+// These are used to derive mocks for unittests.
+class EnvironmentVariableGetter {
+ public:
+  virtual ~EnvironmentVariableGetter() {}
+  // Gets an environment variable's value and stores it in
+  // result. Returns false if the key is unset.
+  virtual bool Getenv(const char* variable_name, std::string* result) = 0;
+
+  // Create an instance of 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);
+
 }  // namespace base
 
 #endif  // BASE_LINUX_UTIL_H__
diff --git a/chrome/app/resources/locale_settings.grd b/chrome/app/resources/locale_settings.grd
index e3330a8..d5516cb 100644
--- a/chrome/app/resources/locale_settings.grd
+++ b/chrome/app/resources/locale_settings.grd
@@ -548,6 +548,12 @@
       <message name="IDS_LINUX_CERTIFICATES_CONFIG_URL" translateable="false">
         http://code.google.com/p/chromium/wiki/LinuxCertManagement
       </message>
+
+      <!-- The URL for Linux proxy configuration help when not running under -->
+      <!-- a supported desktop environment. -->
+      <message name="IDS_LINUX_PROXY_CONFIG_URL" translateable="false">
+        http://code.google.com/p/chromium/wiki/LinuxProxyConfig
+      </message>
     </messages>
   </release>
 </grit>
diff --git a/chrome/browser/gtk/options/advanced_contents_gtk.cc b/chrome/browser/gtk/options/advanced_contents_gtk.cc
index 8f7a6d5..5cf7141 100644
--- a/chrome/browser/gtk/options/advanced_contents_gtk.cc
+++ b/chrome/browser/gtk/options/advanced_contents_gtk.cc
@@ -4,9 +4,14 @@
 
 #include "chrome/browser/gtk/options/advanced_contents_gtk.h"
 
+#include <sys/types.h>
+#include <sys/wait.h>
+
 #include "app/l10n_util.h"
 #include "base/basictypes.h"
+#include "base/linux_util.h"
 #include "base/path_service.h"
+#include "base/process_util.h"
 #include "chrome/browser/browser_list.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/download/download_manager.h"
@@ -22,6 +27,7 @@
 #include "chrome/common/gtk_util.h"
 #include "chrome/common/pref_member.h"
 #include "chrome/common/pref_names.h"
+#include "chrome/common/process_watcher.h"
 #include "grit/chromium_strings.h"
 #include "grit/generated_resources.h"
 #include "grit/locale_settings.h"
@@ -29,6 +35,9 @@
 
 namespace {
 
+// Command used to configure the gconf proxy settings.
+const char kProxyConfigBinary[] = "gnome-network-preferences";
+
 // The pixel width we wrap labels at.
 // TODO(evanm): make the labels wrap at the appropriate width.
 const int kWrapWidth = 475;
@@ -250,6 +259,10 @@ class NetworkSection : public OptionsPageBase {
   }
 
  private:
+  // The callback functions for invoking the proxy config dialog.
+  static void OnChangeProxiesButtonClicked(GtkButton *button,
+                                           NetworkSection* section);
+
   // The widget containing the options for this section.
   GtkWidget* page_;
 
@@ -259,8 +272,53 @@ class NetworkSection : public OptionsPageBase {
 NetworkSection::NetworkSection(Profile* profile)
     : OptionsPageBase(profile) {
   page_ = gtk_vbox_new(FALSE, gtk_util::kControlSpacing);
-  gtk_box_pack_start(GTK_BOX(page_), gtk_label_new("TODO network options"),
+
+  GtkWidget* proxy_description_label = CreateWrappedLabel(
+      IDS_OPTIONS_PROXIES_LABEL);
+  gtk_misc_set_alignment(GTK_MISC(proxy_description_label), 0, 0);
+  gtk_box_pack_start(GTK_BOX(page_), proxy_description_label,
+                     FALSE, FALSE, 0);
+
+  GtkWidget* change_proxies_button = gtk_button_new_with_label(
+      l10n_util::GetStringUTF8(
+          IDS_OPTIONS_PROXIES_CONFIGURE_BUTTON).c_str());
+  g_signal_connect(change_proxies_button, "clicked",
+                   G_CALLBACK(OnChangeProxiesButtonClicked), this);
+  // Stick it in an hbox so it doesn't expand to the whole width.
+  GtkWidget* button_hbox = gtk_hbox_new(FALSE, 0);
+  gtk_box_pack_start(GTK_BOX(button_hbox),
+                     change_proxies_button,
                      FALSE, FALSE, 0);
+  gtk_box_pack_start(GTK_BOX(page_),
+                     OptionsLayoutBuilderGtk::IndentWidget(button_hbox),
+                     FALSE, FALSE, 0);
+}
+
+// static
+void NetworkSection::OnChangeProxiesButtonClicked(GtkButton *button,
+                                                  NetworkSection* section) {
+  section->UserMetricsRecordAction(L"Options_ChangeProxies", NULL);
+
+  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;
+    }
+    ProcessWatcher::EnsureProcessGetsReaped(handle);
+  } else {
+    BrowserList::GetLastActive()->
+        OpenURL(GURL(l10n_util::GetStringUTF8(IDS_LINUX_PROXY_CONFIG_URL)),
+                GURL(), NEW_FOREGROUND_TAB, PageTransition::LINK);
+  }
 }
 
 ///////////////////////////////////////////////////////////////////////////////
diff --git a/net/proxy/proxy_config_service_linux.cc b/net/proxy/proxy_config_service_linux.cc
index f85e450..b8a2ce4 100644
--- a/net/proxy/proxy_config_service_linux.cc
+++ b/net/proxy/proxy_config_service_linux.cc
@@ -21,36 +21,6 @@ namespace net {
 
 namespace {
 
-class EnvironmentVariableGetterImpl
-    : public ProxyConfigServiceLinux::EnvironmentVariableGetter {
- public:
-  virtual bool Getenv(const char* variable_name, std::string* result) {
-    const char* env_value = ::getenv(variable_name);
-    if (env_value) {
-      // Note that the variable may be defined but empty.
-      *result = env_value;
-      return true;
-    }
-    // Some commonly used variable names are uppercase while others
-    // are lowercase, which is inconsistent. Let's try to be helpful
-    // and look for a variable name with the reverse case.
-    char first_char = variable_name[0];
-    std::string alternate_case_var;
-    if (first_char >= 'a' && first_char <= 'z')
-      alternate_case_var = StringToUpperASCII(std::string(variable_name));
-    else if (first_char >= 'A' && first_char <= 'Z')
-      alternate_case_var = StringToLowerASCII(std::string(variable_name));
-    else
-      return false;
-    env_value = ::getenv(alternate_case_var.c_str());
-    if (env_value) {
-      *result = env_value;
-      return true;
-    }
-    return false;
-  }
-};
-
 // Given a proxy hostname from a setting, returns that hostname with
 // an appropriate proxy server scheme prefix.
 // scheme indicates the desired proxy scheme: usually http, with
@@ -528,20 +498,13 @@ bool ProxyConfigServiceLinux::Delegate::GetConfigFromGConf(
 }
 
 ProxyConfigServiceLinux::Delegate::Delegate(
-    EnvironmentVariableGetter* env_var_getter,
+    base::EnvironmentVariableGetter* env_var_getter,
     GConfSettingGetter* gconf_getter)
     : env_var_getter_(env_var_getter), gconf_getter_(gconf_getter),
       glib_default_loop_(NULL), io_loop_(NULL) {
 }
 
 bool ProxyConfigServiceLinux::Delegate::ShouldTryGConf() {
-  // 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");
   // 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
@@ -549,6 +512,7 @@ bool ProxyConfigServiceLinux::Delegate::ShouldTryGConf() {
   // mislead us.
   //
   // We could introduce a CHROME_PROXY_OBEY_ENV_VARS variable...??
+  return base::UseGnomeForSettings(env_var_getter_.get());
 }
 
 void ProxyConfigServiceLinux::Delegate::SetupAndFetchInitialConfig(
@@ -673,12 +637,12 @@ void ProxyConfigServiceLinux::Delegate::OnDestroy() {
 }
 
 ProxyConfigServiceLinux::ProxyConfigServiceLinux()
-    : delegate_(new Delegate(new EnvironmentVariableGetterImpl(),
+    : delegate_(new Delegate(base::EnvironmentVariableGetter::Create(),
                              new GConfSettingGetterImpl())) {
 }
 
 ProxyConfigServiceLinux::ProxyConfigServiceLinux(
-    EnvironmentVariableGetter* env_var_getter,
+    base::EnvironmentVariableGetter* env_var_getter,
     GConfSettingGetter* gconf_getter)
     : delegate_(new Delegate(env_var_getter, gconf_getter)) {
 }
diff --git a/net/proxy/proxy_config_service_linux.h b/net/proxy/proxy_config_service_linux.h
index 2788731..00c31ff 100644
--- a/net/proxy/proxy_config_service_linux.h
+++ b/net/proxy/proxy_config_service_linux.h
@@ -9,6 +9,7 @@
 #include <vector>
 
 #include "base/basictypes.h"
+#include "base/linux_util.h"
 #include "base/message_loop.h"
 #include "base/ref_counted.h"
 #include "base/scoped_ptr.h"
@@ -23,15 +24,6 @@ namespace net {
 class ProxyConfigServiceLinux : public ProxyConfigService {
  public:
 
-  // These are used to derive mocks for unittests.
-  class EnvironmentVariableGetter {
-   public:
-    virtual ~EnvironmentVariableGetter() {}
-    // Gets an environment variable's value and stores it in
-    // result. Returns false if the key is unset.
-    virtual bool Getenv(const char* variable_name, std::string* result) = 0;
-  };
-
   class GConfSettingGetter {
    public:
     virtual ~GConfSettingGetter() {}
@@ -89,7 +81,7 @@ class ProxyConfigServiceLinux : public ProxyConfigService {
    public:
     // Constructor receives gconf and env var getter implementations
     // to use, and takes ownership of them.
-    Delegate(EnvironmentVariableGetter* env_var_getter,
+    Delegate(base::EnvironmentVariableGetter* env_var_getter,
              GConfSettingGetter* gconf_getter);
     // Synchronously obtains the proxy configuration. If gconf is
     // used, also enables gconf notification for setting
@@ -151,7 +143,7 @@ class ProxyConfigServiceLinux : public ProxyConfigService {
     // carry the new config information.
     void SetNewProxyConfig(const ProxyConfig& new_config);
 
-    scoped_ptr<EnvironmentVariableGetter> env_var_getter_;
+    scoped_ptr<base::EnvironmentVariableGetter> env_var_getter_;
     scoped_ptr<GConfSettingGetter> gconf_getter_;
 
     // Cached proxy configuration, to be returned by
@@ -186,7 +178,7 @@ class ProxyConfigServiceLinux : public ProxyConfigService {
   // Usual constructor
   ProxyConfigServiceLinux();
   // For testing: takes alternate gconf and env var getter implementations.
-  ProxyConfigServiceLinux(EnvironmentVariableGetter* env_var_getter,
+  ProxyConfigServiceLinux(base::EnvironmentVariableGetter* env_var_getter,
                           GConfSettingGetter* gconf_getter);
 
   virtual ~ProxyConfigServiceLinux() {
diff --git a/net/proxy/proxy_config_service_linux_unittest.cc b/net/proxy/proxy_config_service_linux_unittest.cc
index 25697ab..50c5019 100644
--- a/net/proxy/proxy_config_service_linux_unittest.cc
+++ b/net/proxy/proxy_config_service_linux_unittest.cc
@@ -74,8 +74,7 @@ struct SettingsTable {
   map_type settings;
 };
 
-class MockEnvironmentVariableGetter
-    : public ProxyConfigServiceLinux::EnvironmentVariableGetter {
+class MockEnvironmentVariableGetter : public base::EnvironmentVariableGetter {
  public:
   MockEnvironmentVariableGetter() {
 #define ENTRY(x) table.settings[#x] = &values.x