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.

Turns out I don't need to rev the id for accesskeys.

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.

Over IRC, we decided not to enable that pref by default and leave it up to distributors instead.

(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.

(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"])

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.

Comment on attachment 304420 [details] [diff] [review] Patch 4 a=beltzner

Oops -- sorry, I misread comment 0 and the patch.

(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

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

Without this patch there will be a javascript error

Comment on attachment 326269 [details] [diff] [review] patch for seamonkey My preference would be for (proxy: system settings).

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....

(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