Closed Bug 416274 Opened 12 years ago Closed 12 years ago

Add UI for System Proxy Settings and use the system proxy by default in Linux

Categories

(Firefox :: Preferences, defect)

x86
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 3 beta4

People

(Reporter: ventnor.bugzilla, Assigned: ventnor.bugzilla)

References

(Blocks 1 open bug)

Details

(Keywords: late-l10n)

Attachments

(2 files, 3 obsolete files)

Attached patch Patch (obsolete) — Splinter Review
Bug 66057 added support for use of the system-wide proxy settings for non-OSX UNIXes. We should add some UI for this, and even turn it on by default.

The option is smart and will only show itself if the system proxy service is available.

This is only adding a single radio button to the pref dialog. In addition, I needed to solve a conflict between accesskeys on the dialog.
Attachment #302059 - Flags: review?(gavin.sharp)
Attached patch Patch 2 (obsolete) — Splinter Review
Turns out I don't need to rev the id for accesskeys.
Attachment #302059 - Attachment is obsolete: true
Attachment #302067 - Flags: review?(gavin.sharp)
Attachment #302059 - Flags: review?(gavin.sharp)
Comment on attachment 302059 [details] [diff] [review]
Patch

>+#ifndef XP_MACOSX
>+#ifdef XP_UNIX
>+pref("network.proxy.type",                  5);
>+#endif
>+#endif

use #ifdef UNIX_BUT_NOT_MAC instead.
Attachment #302059 - Attachment is obsolete: false
Attached patch Patch 3 (obsolete) — Splinter Review
Over IRC, we decided not to enable that pref by default and leave it up to distributors instead.
Attachment #302059 - Attachment is obsolete: true
Attachment #302067 - Attachment is obsolete: true
Attachment #302069 - Flags: review?(gavin.sharp)
Attachment #302067 - Flags: review?(gavin.sharp)
Attachment #302069 - Flags: review?(gavin.sharp) → review+
(In reply to comment #3)
> Created an attachment (id=302069) [details]
> Patch 3
> 
> Over IRC, we decided not to enable that pref by default and leave it up to
> distributors instead.
> 

...And because Gavin can't make a call like that.
Attachment #302069 - Flags: ui-review?(beltzner)
(In reply to comment #2)
> (From update of attachment 302059 [details] [diff] [review])
> >+#ifndef XP_MACOSX
> >+#ifdef XP_UNIX
> >+pref("network.proxy.type",                  5);
> >+#endif
> >+#endif
> 
> use #ifdef UNIX_BUT_NOT_MAC instead.
> 

That's only defined in firefox.js...
(In reply to comment #5)
> > use #ifdef UNIX_BUT_NOT_MAC instead.
>
> That's only defined in firefox.js...

How lame. Somebody should clean all.js up then...
Comment on attachment 302069 [details] [diff] [review]
Patch 3

>+    var sysProxy = Components.classes["@mozilla.org/system-proxy-settings;1"];
>+    if (sysProxy)

Nit: This will lead to the strict warning

> Warning: reference to undefined property Components.classes['@mozilla.org/system-proxy-settings;1']

on systems where that component isn't available. Checking for it directly in the if condition won't:

>+    if (Components.classes["@mozilla.org/system-proxy-settings;1"])
Attachment #302069 - Attachment is obsolete: true
Attachment #302069 - Flags: ui-review?(beltzner)
Attached patch Patch 4Splinter Review
mconnor said over IRC that he's fine with turning it on by default as long as there is no significant Ts hit. If there is a Ts hit just back out the all.js part of the patch, the rest won't affect it.
Attachment #304420 - Flags: approval1.9?
Comment on attachment 304420 [details] [diff] [review]
Patch 4

a=beltzner
Attachment #304420 - Flags: approval1.9? → approval1.9+
Keywords: checkin-needed
OS: Linux → All
Hardware: PC → All
Summary: Add UI for System Proxy Settings → Add UI for System Proxy Settings and use the system proxy by default on Linux
Oops -- sorry, I misread comment 0 and the patch.
OS: All → Linux
Hardware: All → PC
Summary: Add UI for System Proxy Settings and use the system proxy by default on Linux → Add UI for System Proxy Settings and use the system proxy by default
(In reply to comment #7)
> (From update of attachment 302069 [details] [diff] [review])
> >+    var sysProxy = Components.classes["@mozilla.org/system-proxy-settings;1"];
> >+    if (sysProxy)
> 
> Nit: This will lead to the strict warning
> 
> > Warning: reference to undefined property Components.classes['@mozilla.org/system-proxy-settings;1']
> 
> on systems where that component isn't available. Checking for it directly in
> the if condition won't:
> 
> >+    if (Components.classes["@mozilla.org/system-proxy-settings;1"])
> 

this would probably be better:

if ("@mozilla.org/system-proxy-settings;1" in Components.classes)
Committed with Dão's nit addressed.

Checking in browser/components/preferences/connection.js;
/cvsroot/mozilla/browser/components/preferences/connection.js,v  <--  connection.js
new revision: 1.9; previous revision: 1.8
done
Checking in browser/components/preferences/connection.xul;
/cvsroot/mozilla/browser/components/preferences/connection.xul,v  <--  connection.xul
new revision: 1.12; previous revision: 1.11
done
Checking in browser/locales/en-US/chrome/browser/preferences/connection.dtd;
/cvsroot/mozilla/browser/locales/en-US/chrome/browser/preferences/connection.dtd,v  <--  connection.dtd
new revision: 1.9; previous revision: 1.8
done
Checking in modules/libpref/src/init/all.js;
/cvsroot/mozilla/modules/libpref/src/init/all.js,v  <--  all.js
new revision: 3.730; previous revision: 3.729
done
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 3 beta4
I backed out the all.js change to see if this is the cause of the perf regression that occurred around when I landed this patch.
Relanded the all.js change since that didn't affect Tp2.

Checking in modules/libpref/src/init/all.js;
/cvsroot/mozilla/modules/libpref/src/init/all.js,v  <--  all.js
new revision: 3.733; previous revision: 3.732
done
Depends on: 421490
Without this patch there will be a javascript error
Attachment #326269 - Flags: review?(neil)
Comment on attachment 326269 [details] [diff] [review]
patch for seamonkey

My preference would be for (proxy: system settings).
Attachment #326269 - Flags: review?(neil) → review+
When you say turning on by default, you mean turning it on by default for Linux-only?

This is a pretty significant departure from the previous implementations of proxy config. 

if I understand this, a platform ifdef was used. Is there any reason why the platform.js file was not used?
Checked in the SeaMonkey patch on comm-central: http://hg.mozilla.org/comm-central/index.cgi/rev/0ce42125210f
Also, I don't want to sound too old-school, but if you are going to checkin the same functionality into two products, you really should create a second bug for the second product....
Summary: Add UI for System Proxy Settings and use the system proxy by default → Add UI for System Proxy Settings and use the system proxy by default in Linux
Blocks: 444157
Blocks: 428456
(In reply to comment #19)
> Also, I don't want to sound too old-school, but if you are going to checkin the
> same functionality into two products, you really should create a second bug for
> the second product....

Yup, the patch already should have gone onto bug 428456, which is for SeaMonkey.
I don't know where the right place to right this is, but I think I found a bug in the Linux system proxy settings.

I'm on ubuntu 11.04 with firefox 4.0.1, and while the system proxy settings are applied, the "ignored hosts" list is ignored, and it tries to use the proxy. If I put the exceptions into firefox's proxy settings, it works great. (This problem doesn't exist with google chrome)

thanks
You need to log in before you can comment on or make changes to this bug.