Add proxy config (using gnome-network-preferences)

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, LinuxProxyConfig wiki page should load.

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=21023

[Evan Stade, 2009-07-17 03:00:59]

overall looks ok to me, even if it does make my stomach a little urpy. Adding
Evan1 because it involves some importantish policy decisions.

Also adding Lei because we should sort out a way to get rid of these zombie
processes (I would be ok with an extra fork() call as the overhead doesn't seem
that bad--what do I know--although in this case I guess we can't just fire and
forget if we want to know if the process has launched)

http://codereview.chromium.org/149785/diff/1/2
File chrome/browser/gtk/options/advanced_contents_gtk.cc (right):

http://codereview.chromium.org/149785/diff/1/2#newcode37
Line 37: const char kProxyConfigBinary[] = "/usr/bin/gnome-network-preferences";
can this just be gnome-network-preferences?

http://codereview.chromium.org/149785/diff/1/2#newcode42
Line 42: "http://code.google.com/p/chromium/wiki/LinuxProxyConfig";
seems like this should be a string resource (kind of like
IDS_CHROMIUM_PROJECT_URL) although this is fine for now.

http://codereview.chromium.org/149785/diff/1/2#newcode265
Line 265: class CheckProxyConfigPidStatusTask : public Task {
won't this leave a zombie process laying around? (we have this problem with
xdg-open as well)

http://codereview.chromium.org/149785/diff/1/2#newcode312
Line 312: LOG(INFO) << "OpenProxyConfigDialogTask pid=" << pid;
this line is unnecessary (I think LOG(INFO) should not show very frequently, or
at all, unless it is code that is still under development)

[mattm, 2009-07-17 19:34:25]

http://codereview.chromium.org/149785/diff/1/2
File chrome/browser/gtk/options/advanced_contents_gtk.cc (right):

http://codereview.chromium.org/149785/diff/1/2#newcode37
Line 37: const char kProxyConfigBinary[] = "/usr/bin/gnome-network-preferences";
On 2009/07/17 03:00:59, Evan Stade wrote:
> can this just be gnome-network-preferences?

Yeah, the comment for LaunchApp was out of date (which I have a CL to fix), so
I'll change that

http://codereview.chromium.org/149785/diff/1/2#newcode265
Line 265: class CheckProxyConfigPidStatusTask : public Task {
On 2009/07/17 03:00:59, Evan Stade wrote:
> won't this leave a zombie process laying around? (we have this problem with
> xdg-open as well)

yeah :/

I guess a cleaner approach overall might be to do the PATH search ourselves and
if the binary exists just run it with the double fork, and if it doesn't, then
load the wiki url.  Thoughts?

[Evan Stade, 2009-07-17 20:37:49]

On 2009/07/17 19:34:25, mattm wrote:
> http://codereview.chromium.org/149785/diff/1/2
> File chrome/browser/gtk/options/advanced_contents_gtk.cc (right):
> 
> http://codereview.chromium.org/149785/diff/1/2#newcode37
> Line 37: const char kProxyConfigBinary[] =
"/usr/bin/gnome-network-preferences";
> On 2009/07/17 03:00:59, Evan Stade wrote:
> > can this just be gnome-network-preferences?
> 
> Yeah, the comment for LaunchApp was out of date (which I have a CL to fix), so
> I'll change that
> 
> http://codereview.chromium.org/149785/diff/1/2#newcode265
> Line 265: class CheckProxyConfigPidStatusTask : public Task {
> On 2009/07/17 03:00:59, Evan Stade wrote:
> > won't this leave a zombie process laying around? (we have this problem with
> > xdg-open as well)
> 
> yeah :/
> 
> I guess a cleaner approach overall might be to do the PATH search ourselves
and
> if the binary exists just run it with the double fork, and if it doesn't, then
> load the wiki url.  Thoughts?

Lei and I talked and it seems hard or impossible to know if
gnome-network-preferences is the right thing to launch. Even if it exists it may
not be the thing the user actually wants. Really the whole thing is a
convenience but it seems like a lot of work to get right for such a little task.
Perhaps we should just have a wiki page?

[Evan Martin, 2009-07-17 20:40:10]

On 2009/07/17 20:37:49, Evan Stade wrote:
> Lei and I talked and it seems hard or impossible to know if
> gnome-network-preferences is the right thing to launch. Even if it exists it
may
> not be the thing the user actually wants. Really the whole thing is a
> convenience but it seems like a lot of work to get right for such a little
task.
> Perhaps we should just have a wiki page?

We should only start gnome-network-prefs if we're using those prefs for our
proxy config.  The proxy code has some logic to "guess" whether we're in gnome. 
We should pull that code out into base, e.g. base::UseGnomeForSettings(), and
share it between the proxy code and this code.

[mattm, 2009-07-17 21:58:04]

On 2009/07/17 20:40:10, Evan Martin wrote:
> On 2009/07/17 20:37:49, Evan Stade wrote:
> > Lei and I talked and it seems hard or impossible to know if
> > gnome-network-preferences is the right thing to launch. Even if it exists it
> may
> > not be the thing the user actually wants. Really the whole thing is a
> > convenience but it seems like a lot of work to get right for such a little
> task.
> > Perhaps we should just have a wiki page?
> 
> We should only start gnome-network-prefs if we're using those prefs for our
> proxy config.  The proxy code has some logic to "guess" whether we're in
gnome. 
> We should pull that code out into base, e.g. base::UseGnomeForSettings(), and
> share it between the proxy code and this code.

Uploaded a new version which does this (and addresses other comments, except for
still leaking zombies)

[mattm, 2009-07-17 22:00:54]

On 2009/07/17 21:58:04, mattm wrote:
> On 2009/07/17 20:40:10, Evan Martin wrote:
> > On 2009/07/17 20:37:49, Evan Stade wrote:
> > > Lei and I talked and it seems hard or impossible to know if
> > > gnome-network-preferences is the right thing to launch. Even if it exists
it
> > may
> > > not be the thing the user actually wants. Really the whole thing is a
> > > convenience but it seems like a lot of work to get right for such a little
> > task.
> > > Perhaps we should just have a wiki page?
> > 
> > We should only start gnome-network-prefs if we're using those prefs for our
> > proxy config.  The proxy code has some logic to "guess" whether we're in
> gnome. 
> > We should pull that code out into base, e.g. base::UseGnomeForSettings(),
and
> > share it between the proxy code and this code.
> 
> Uploaded a new version which does this (and addresses other comments, except
for
> still leaking zombies)

(Which I think should be addressed by making LaunchApp double fork when wait ==
false && process_handle == NULL, looking into that separately.)

[Evan Stade, 2009-07-17 22:06:53]

ok sounds good

http://codereview.chromium.org/149785/diff/1005/1009
File chrome/browser/gtk/options/advanced_contents_gtk.cc (right):

http://codereview.chromium.org/149785/diff/1005/1009#newcode13
Line 13: #include "base/linux_util.h"
nit:alphabetical

http://codereview.chromium.org/149785/diff/1005/1009#newcode327
Line 327: FROM_HERE, new OpenProxyConfigDialogTask());
do we still need this to be posted on another thread? Can't we just fire and
forget from the ui thread?

[mattm, 2009-07-17 22:59:29]

http://codereview.chromium.org/149785/diff/1005/1009
File chrome/browser/gtk/options/advanced_contents_gtk.cc (right):

http://codereview.chromium.org/149785/diff/1005/1009#newcode13
Line 13: #include "base/linux_util.h"
On 2009/07/17 22:06:54, Evan Stade wrote:
> nit:alphabetical

Done.

http://codereview.chromium.org/149785/diff/1005/1009#newcode327
Line 327: FROM_HERE, new OpenProxyConfigDialogTask());
On 2009/07/17 22:06:54, Evan Stade wrote:
> do we still need this to be posted on another thread? Can't we just fire and
> forget from the ui thread?

I guess so, fork should be fast.

[Evan Stade, 2009-07-17 23:05:03]

lgtm

http://codereview.chromium.org/149785/diff/2009/1015
File chrome/browser/gtk/options/advanced_contents_gtk.cc (right):

http://codereview.chromium.org/149785/diff/2009/1015#newcode311
Line 311: GURL(), NEW_WINDOW, PageTransition::LINK);
new foreground tab probably better (like when a user clicks on history or show
all downloads)

[Evan Martin, 2009-07-18 00:18:34]

the proxy test has started failling on the buildbots, maybe this change?

[mattm, 2009-07-18 00:31:29]

hm, yeah.  reverting..

On Fri, Jul 17, 2009 at 17:18, <evan@chromium.org> wrote:
> the proxy test has started failling on the buildbots, maybe this change?
>
>
> http://codereview.chromium.org/149785
>