217664 - Ability to use same proxy for all protocols

Status: RESOLVED FIXED

(Firefox :: Settings UI, enhancement, P3)

Description

[Mikel Ward]

The Connection preferences dialog allows the user to select manual proxy
configuration and enter the host and port name for five protocols
(HTTP, HTTPS, FTP, Gopher, SOCKS).  If the user has the same proxy for all
protocols (as is common), he must type the same information five times.
It would be very useful if there was a way to specify the proxy host and
port once then reuse this information for the other protocols.

Comment 1

[Mike Connor [:mconnor]]

-> taking

usability win

Comment 2

[Mike Connor [:mconnor]]

Attachment: provisional patch

similar to IE's implementation.  this isn't 100% final since my head's all
messed right now, should be good

Comment 3

[Mike Connor [:mconnor]]

Comment on attachment 131478 [details] [diff] [review]
provisional patch

built from a clean CVS tree with this patch, looks good after a night's sleep

Comment 4

[Olivier Cahagne]

is patch ready to land ? :)

Comment 5

[Ben Goodger (use ben at mozilla dot org for email)]

Can you provide a screenshot?

Comment 6

[Mike Connor [:mconnor]]

Attachment: checked

Comment 7

[Mike Connor [:mconnor]]

Attachment: checked (yes, its a PNG, not a patch)

Comment 8

[Mike Connor [:mconnor]]

Attachment: unchecked

Comment 9

[Mike Connor [:mconnor]]

Comment on attachment 131478 [details] [diff] [review]
provisional patch

small regression (bitrot?) new patch once I update my tree

Comment 10

[Mike Connor [:mconnor]]

Attachment: patch

Comment 11

[Ben Goodger (use ben at mozilla dot org for email)]

-> 0.9

Comment 12

[Mike Connor [:mconnor]]

pushing to 1.0beta as this needs to be looked at post-0.9 when the new prefpanel
lands.

Comment 13

[Ben Goodger (use ben at mozilla dot org for email)]

What does this UI look like?

Use consistent ' vs. " too ;-) 

Comment 14

[Ben Goodger (use ben at mozilla dot org for email)]

- 1.0 since there are localization changes I'll review this week see if you can
get it in asap. 

Comment 15

[Mike Connor [:mconnor]]

Attachment: unbitrotted patch

Comment 16

[Ben Goodger (use ben at mozilla dot org for email)]

Comment on attachment 154156 [details] [diff] [review]
unbitrotted patch

Pretty much only nits. 

>+  var proxycheckbox = document.getElementById("proxycheckbox");

Can we get something slightly more descriptive here, "shareAllProxies"
(intercaps)

>+  var manual2 = [ http, httpPort, noProxy, proxycheckbox];

nit: leading whitespace inside array.

>   switch ( radiogroup.value ) {

nix the wacky whitespace

>@@ -66,15 +68,21 @@ function DoEnabling()
>         auto[i].setAttribute( "disabled", "true" );

kill extra whitespace here, and all subsequent cases.

>+  if (!document.getElementById('proxycheckbox').checked)
>+    return;

The rest of the quotes are double quotes, don't use single here. 

>+            <hbox>
>+              <checkbox id="proxycheckbox" oncommand="toggleProxySettings();" label="&shareproxy.label;"
>+                        accesskey="&shareproxy.accesskey;"
>+                        preftype="bool" prefstring="network.proxy.share_proxy_settings"/>

Formatting: bring label down onto the same line as accesskey

r=[e-mail]

Comment 17

[Mike Connor [:mconnor]]

Attachment: patch checked in on aviary

fixed on aviary branch.  Thanks Ben!

Comment 18

[Mike Shaver (:shaver emeritus)]

Do we want to fix the IE migrator to set this pref if IE is set in this mode as
well?  We already detect it, but expand the settings out explicitly.  Not
critical, but might be a nice polish item.

Comment 19

[Michaël Arnauts]

i don't know if this is intended behavior, but the socks-field isn't filled in
when clicking the checkbox...

you can also see that on the screenshot
http://bugzilla.mozilla.org/attachment.cgi?id=135436&action=view

Comment 20

[Mike Connor [:mconnor]]

yes, its intentional.  SOCKS is something a little different, and if you do this
in IE, it doesn't fill in the SOCKS entry.

I spun off shaver's idea in Bug 253008.

Comment 21

[Mike Connor [:mconnor]]

*** Bug 255384 has been marked as a duplicate of this bug. ***

Comment 22

[Stefan Borggraefe]

(In reply to comment #20)
> yes, its intentional.  SOCKS is something a little different, and if you do this
> in IE, it doesn't fill in the SOCKS entry.

But why are the SOCKS UI elements disabled when the checkbox is selected? I
don't think this makes sense, because:

1. As far as I know the SOCKS proxy is used as a fallback value when the other
configured proxy isn't reachable or can't handle a certain application level
protocol.
2. You can still change the SOCKS Proxy when you deselect the checkbox, change
the SOCKS config and select the checkbox again. When this works, why do I have
to deal with the checkbox at all?
3. The SOCKS proxy can still be a different proxy when the checkbox is selected.
So the label and the behaviour of the checkbox is misleading currently.

Comment 23

[benc]

So the pref (network.proxy.share_proxy_settings) was first added to the branch,
and then introduced to the trunk later, in the WPAD rewrite?

Comment 24

[Mike Connor [:mconnor]]

sorry for bugspam, long-overdue mass reassign of ancient QA contact bugs,
filter on "beltznerLovesGoats" to get rid of this mass change

Comment 25

[benc]

I finally had some time to look at this, in FF2.

This is what I found:

The checkbox controls the values for all other proxy values. In other words, we have a UI mode which is a single proxy host:port.

It affects all fields, including SOCKS.

After playing with the settings for a while, I have to admin, I like the all-in-one implementation BETTER than the all-application proxys (but not SOCKS) implementation, for two reasons:

1- "SSL" is also a circuit-level proxy, we just never properly enabled its usage for historical Netscape reasons.

2- If the goal is to simplify life for the user, the "all-in-one" user is much more common than the SOCKS + all-the-rest-in-one user.

One last question:

I was reading the nsProtocolProxyService.cpp code, and I noticed that it listens to network.proxy.*.

Is there any benefit/concern that we added these new prefs:

network.proxy.backup.*
network.proxy.share_proxy_settings

Does that bloat the overhead of the prefs observer? Just curious.

I'll summarize some test results and attach them, so it doesn't clutter up search by comment results.