Last Comment Bug 66057 - Proxy: $http_proxy should influence proxy settings
: Proxy: $http_proxy should influence proxy settings
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Networking (show other bugs)
: Trunk
: x86 Linux
: -- enhancement with 9 votes (vote)
: mozilla1.9beta3
Assigned To: Michael Ventnor
:
Mentors:
: 13470 226816 (view as bug list)
Depends on: 331416 354075 421490 429520
Blocks: 125995 139393 323034
  Show dependency treegraph
 
Reported: 2001-01-20 06:46 PST by robbe
Modified: 2011-08-11 22:47 PDT (History)
41 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (37.85 KB, patch)
2006-01-11 13:33 PST, Robert O'Callahan (:roc) (Exited; email my personal email if necessary)
no flags Details | Diff | Splinter Review
updated patch (37.69 KB, patch)
2006-03-14 13:41 PST, Robert O'Callahan (:roc) (Exited; email my personal email if necessary)
no flags Details | Diff | Splinter Review
UI piece (4.43 KB, patch)
2006-04-11 14:00 PDT, Robert O'Callahan (:roc) (Exited; email my personal email if necessary)
no flags Details | Diff | Splinter Review
provides system proxy service for linux and os x (51.77 KB, patch)
2006-09-24 11:52 PDT, Diane Trout
no flags Details | Diff | Splinter Review
my latest patch (39.96 KB, patch)
2006-09-24 14:28 PDT, Robert O'Callahan (:roc) (Exited; email my personal email if necessary)
no flags Details | Diff | Splinter Review
merged my patch and Robert's patch (57.42 KB, patch)
2006-09-24 23:04 PDT, Diane Trout
no flags Details | Diff | Splinter Review
updated again (57.39 KB, patch)
2006-09-27 16:38 PDT, Robert O'Callahan (:roc) (Exited; email my personal email if necessary)
no flags Details | Diff | Splinter Review
move system proxy into libnecko (63.75 KB, patch)
2006-09-30 23:22 PDT, Diane Trout
no flags Details | Diff | Splinter Review
Now using mozilla/config/static-config.mk (62.02 KB, patch)
2006-10-01 22:33 PDT, Diane Trout
no flags Details | Diff | Splinter Review
Builds as shared & static (62.15 KB, patch)
2006-10-02 10:17 PDT, Diane Trout
no flags Details | Diff | Splinter Review
Merged in Josh's requested updates from bug 125995 (63.61 KB, patch)
2006-10-18 00:28 PDT, Diane Trout
diane: review+
cbiesinger: superreview-
Details | Diff | Splinter Review
Updated Unix patch (39.44 KB, patch)
2007-12-17 20:11 PST, Michael Ventnor
no flags Details | Diff | Splinter Review
Updated Unix patch 1.1 (39.50 KB, patch)
2007-12-18 12:25 PST, Michael Ventnor
cbiesinger: review+
cbiesinger: superreview+
Details | Diff | Splinter Review
String additions for possible future UI (1.43 KB, patch)
2008-01-23 15:40 PST, Michael Ventnor
mbeltzner: ui‑review+
mbeltzner: approval1.9+
Details | Diff | Splinter Review
Nits fixed (39.67 KB, patch)
2008-01-28 10:56 PST, Michael Ventnor
dsicore: approval1.9+
Details | Diff | Splinter Review

Description robbe 2001-01-20 06:46:54 PST
On Unix, the environment variables http_proxy, ftp_proxy, etc. will by
convention often hold URLs pointing to appropriate proxies.

Mozilla should use these settings as the default for its own preferences.

E.g. a setting of http_proxy="http://proxy.example.com:8079/" would result in
default values like this:
user_pref("network.proxy.http", "proxy.example.com");
user_pref("network.proxy.http_port", 8079);
Comment 1 timeless 2001-01-20 20:31:25 PST
is it enough for the installer/first run of a profile to absorb these settings? 
or do you want us to always check them?

personally i would prefer for people to use PAC files [once we have our support 
working] as they are more flexible than an environment variable.
Comment 2 Matthew Paul Thomas 2001-01-20 23:13:15 PST
IMO these environment variables should be treated the same as Internet Config 
settings on Mac OS, or Internet control panel settings on Windows. That is, 
Mozilla reads from (and writes to?) them, but only if the `Use my system 
settings' checkbox (or whatever) is checked in that particular prefs panel.

It's been too long since I used Unix. Where in a user's directory are environment 
variables stored? Is it shell-dependent? If so, is there still an API for writing 
persistent changes to them without having to know where the file is?
Comment 3 timeless 2001-01-20 23:44:22 PST
env vars are relatively transient, a user can set them at will, and your 
average logout script does not persist them.  mozilla has no easy way of 
storing proxy settings for the correct shell(s) and if we tried people would 
yell and scream.

my logic:
as such, if the user has a proxy var setting, and then makes a change in 
mozilla, and then runs mozilla again. suppose we always read from the env, 
we'll dump the user's var and they'll complain.
Comment 4 robbe 2001-01-21 08:32:00 PST
I agree with mpt that a "Use system defaults" option is probably the best. This
should be checked by default. Most users will be comfortable with leaving it at
that.

For me as an admin, this makes it easy to change settings: I just edit
/etc/profile et al to load $http_proxy with a different value, and (hopefully)
all browsers will heed the change (unless the user chose to override it). PAC
files don't give me that until a lot more browsers support them.

That's also why doing it just on profile creation is not enough: sure, it would
help /now/; but as soon as the proxy parameters change, I'd be faced with either
sed'ding through all user's prefs.js, or having someone manually apply these
changes in the UI. Not pretty.
Comment 5 Stephan Niemz 2001-01-22 07:04:09 PST
Confirming (Status NEW) based on the discussion.
Comment 6 Bradley Baetz (:bbaetz) 2001-08-27 21:54:40 PDT
If the "use os settings" option is selected, then mozilla should always read the
settings. Most system administrators set then in global shell config files, such
as /etc/bashrc, for the use of other networking apps. On unix, an external
program can't change the value of an environment variable in another running
program, so these would only hav eto be read on startup, but they should be read
on every startup.

With ns4, if you started mozilla for the first time (ns4 for unix didn't have
profiles) and had those environment variables set, then the default settings
were for manual proxies rather than direct, and the proxies were prefilled for
http, ftp and gopher, as well as no_proxy_for. I don't know if socks was prefilled.

For mozilla, I propose that new profiles always (for all OSs which have this
implemented) default to "use system settings", and prefill the manual proxy
settings if possible, even those this option would not be selected by default.
(Other os's may just have a function which is given a url, and returns proxy
info, in which case this wouldn't be possible. For example, windows can had a
PAC file set up instead of individual manual proxy settings)

Does that sound reasonable? I may consider doing this at some point.
Comment 7 Samir Gehani 2001-09-18 17:23:37 PDT
Networking owns this content.
Comment 8 Gagan 2001-09-18 22:18:04 PDT
no samir. this has not much to do with the fact that the preference is HTTP
specific. I am sure there could be other prefs that could follow this idea. It
really needs someone from prefs area. Over to prefs.
Comment 9 Brian Nesse (gone) 2001-09-19 11:42:33 PDT
Nope, not BE-Prefs either. This sort of thing usually seems to end up in XP
Apps. ->pchen
Comment 10 Bradley Baetz (:bbaetz) 2001-09-19 12:36:39 PDT
So, rather than playing hot potato with this bug, can someone comment on my two
part suggestion? One of those is networking, and I can take the
infrastructure+unix side of that. The other is prefs/xpapps, I guess.

darin, mpt, anyone else?
Comment 11 Darin Fisher 2001-09-19 15:59:08 PDT
i like the idea of picking this up automatically for new profiles, but does
anyone know how to get similar information under windows or under macos?
Comment 12 Bradley Baetz (:bbaetz) 2001-09-19 16:39:03 PDT
open transport on mac (theres a separate bug on that) and windows has an
internet control panel; there must be APIs to get at that data.

I would file a meta networking infrastructure bug, and have this + the OT one +
a new windows one depend on that, I guess.

The pref panel should have a "show me" button for this new option, which would
open the relevent control panels on mac/windows, and show a quick dialog with
the current settings of the environent vars. This will help debugging a _lot_.

The "initial value" thing I mention would depend on the infrastructure bug, if
we do want to go that way.
Comment 13 Gagan 2001-09-19 16:39:35 PDT
I'd imagine NSPR would have a way to access environment variables. AFAIK Mac has
no such concept. 
Comment 14 Bradley Baetz (:bbaetz) 2001-09-19 16:49:52 PDT
Just to clarify - yes, this will be platform specific code:
nsIOSProxy::FindProxyForURL. The code should cope with an instance of that
interface not existing, and disallow the choice of the new option from the ui in
that case.
Comment 15 benc 2001-09-21 09:57:24 PDT
Classic MacOS has "Internet Config", and OS X has a system level proxy pref.

At some point, we should add support for reading the system prefs.

I think most versions of IE cheat, they map their proxy config to the Internet
Config control panel, and they don't support other plats.
Comment 16 Bradley Baetz (:bbaetz) 2001-09-21 10:18:13 PDT
So we have agreement that this should be done.

I'll take this for unix + infrastructure. The profile stuff will be filed as a
separate bug once I'm done.
Comment 17 Patrick C. Beard 2001-09-21 14:12:31 PDT
Mac OS 9 and earlier has no support for environment variables. In Mozilla, 
however, I added a facility which looks for a file called "ENVIRONMENT" in 
the application directory, which contains entries of the form:

NAME=VALUE

Calls to getenv() will return these values. This is really only intended for 
debugging, not for production code.
Comment 18 Bradley Baetz (:bbaetz) 2001-09-21 14:20:20 PDT
beard: But on osX, the official way is to use internet config, right?

I'll see if I have time to crank out some code this weekend for this.
Comment 19 Chris Newman 2001-10-02 11:08:58 PDT
Discussion of Mac-specific implementation issues are in bug 97214.
Comment 20 Bradley Baetz (:bbaetz) 2001-11-20 15:41:59 PST
I have no time. I have a rough design sketched out, but...

->0.9.9, but +helpwanted. I really think that we do want this sooner rather than
later.
Comment 21 Bradley Baetz (:bbaetz) 2002-02-06 03:58:25 PST
No time; -> 1.1
Comment 22 Bradley Baetz (:bbaetz) 2002-05-06 07:44:28 PDT
No time for this, -> 1.1beta, may slip to 1.2
Comment 23 Bradley Baetz (:bbaetz) 2002-08-27 23:44:18 PDT
-> default owner
Comment 24 benc 2002-08-29 09:11:55 PDT
*** Bug 13470 has been marked as a duplicate of this bug. ***
Comment 25 Paul Sorensen 2003-03-24 13:35:52 PST
Two suggestions:

- if an environment variable, for example $http_proxy doesn't exist then mozilla
should act as if there is a direct connection.  For example on my laptop: at
work I have a proxy setting (and I have a script which detects that I'm on the
work network and sets http_proxy), etc - but at home I have no $http_proxy set).

- The text on the preferences dialog should be something like:

  o Honor system environment variables ($http_proxy, $ftp_proxy, etc) if
    they are present, otherwise assume a direct internet connection.

Comment 26 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2005-11-09 13:14:24 PST
I think we could just make "auto-detect proxy settings", which currently uses WPAD, check $http_proxy/$ftp_proxy first on Linux/Unix systems.
Comment 27 Christian :Biesinger (don't email me, ping me on IRC) 2005-11-09 14:52:22 PST
we have some code that asks gconf for the proxy settings... (extensions/system-pref iirc) maybe "autodetect" should ask that too. (it's currently a checkbox in Advanced)
Comment 28 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2005-11-10 17:37:52 PST
I don't want to ask gconf unless use_system_prefs is on.

> (it's currently a checkbox in Advanced)

What checkbox?
Comment 29 Christian :Biesinger (don't email me, ping me on IRC) 2005-11-11 02:54:59 PST
[x] Use System Preferences
Comment 30 Wolfgang Rosenauer [:wolfiR] 2005-12-21 23:54:15 PST
We have some differences in UI between mozilla/seamonkey and firefox.
Firefox 1.0.x has this "Auto-detect proxy settings for this network" but mozilla 1.7 doesn't have it, while mozilla has a checkbox for "Use system preferences".

I'm not sure what the best(TM) behaviour is.
Comment 31 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2005-12-22 01:39:38 PST
I'm going to implement comment #26 (and try to start WPAD resolution earlier in the startup process so it doesn't slow down the time to display the first web page)
Comment 32 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2006-01-11 13:23:46 PST
I have a patch for this which I'll attach in a moment.

Basically it hijacks the "Autodectect proxy settings for this network" preference, which currently just launches WPAD detection (which is just proxy-auto-config with the fixed URL 'http://wpad/wpad.dat'). The new behaviour of this preference setting is as follows:
-- On non-Unix platforms (those not using GTK2 as the toolkit) we try WPAD just as we used to.
-- On Unix platforms, if the environment variable DESKTOP_SESSION is "gnome" then we use the proxy settings from gconf. gconf can specify a PAC URL, in which case we will use it; hence it is possible to get WPAD back by the administrator setting 'http://wpad/wpad.dat' as the PAC URL in gconf.
-- On Unix platforms, if the environment variable DESKTOP_SESSION does not exist or is not is "gnome", then we honour *_PROXY environment variables. If the URL's scheme is XYZ then we look up the XYZ_PROXY environment variable for a proxy, or that does not exist, ALL_PROXY. If neither environment variable exists we fall back to try WPAD. Otherwise we try the indicated proxy, but we honour the NO_PROXY variable and force direct connections for URIs that match it.

Relationship with config.use_system_prefs: when gconf system-prefs is enabled, Mozilla's proxy settings are fully synchronized with the gconf settings --- either the user won't be able to change the proxy settings (if the gconf settings are read-only), or user changes will also update gconf. When gconf system-prefs is disabled, the user has the freedom to choose proxies, and this patch gives the user the ability to select gconf prefs if available.

The implementation introduces a new interface, nsISystemProxySettings. nsProtocolProxyService reinterprets eProxyConfig_WPAD to look for a service implementing this interface. If the service is found, we treat it as a sort of meta-PAC. The service can either return a real PAC URL to use, or otherwise it itself acts as a PAC, with nsProtocolProxyService passing in nsIURIs and getting back PAC-strings describing the proxy(ies) to use. This design accomodates arbitrarily complex system proxy policies.

Right now I only have a nsISystemProxySettings implementation for Unix-like systems, but it would be easy to provide alternative implementations for other platforms. This might be a slightly cleaner way to handle the Mac system proxy settings.
Comment 33 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2006-01-11 13:33:21 PST
Created attachment 208222 [details] [diff] [review]
patch

One question about this patch is where the unixproxy component should go. I've put it in toolkit because it depends on the gconf toolkit component, but possibly it should go in extensions/.
Comment 34 M Lopez-Ibanez 2006-01-13 00:50:09 PST
(In reply to comment #33)
> Created an attachment (id=208222) [edit]
> patch

why are you using *_PROXY variables when the most used by far are $http_proxy and $ftp_proxy (notice the case)? 
Comment 35 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2006-01-13 13:00:01 PST
Er yeah, Wolfgang fixed that but I need to fix it in my version of the patch.
Comment 36 M Lopez-Ibanez 2006-02-12 03:24:24 PST
Just to be sure, will this patch enable the same behaviour in mozilla-thunderbird?
Comment 37 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2006-02-12 15:20:14 PST
yes.
Comment 38 Christian :Biesinger (don't email me, ping me on IRC) 2006-03-11 06:57:39 PST
Comment on attachment 208222 [details] [diff] [review]
patch

netwerk/base/public/nsISystemProxySettings.idl

the contractid should probably live in nsNetCID.h instead
Comment 39 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2006-03-14 13:41:45 PST
Created attachment 215059 [details] [diff] [review]
updated patch

Fixed the environment variable case issue. Also, I made "system proxy settings" be a new preference value instead of overloading _WPAD, add added UI for it, taken from Ron Crocker's patch in bug 66057.

Darin, where do you think unixproxy should be located, given that it depends on toolkit/components/gnome?
Comment 40 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2006-03-22 14:52:56 PST
Sorry, Ron's patch is in bug 125995.
Comment 41 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2006-04-11 14:00:28 PDT
Created attachment 218078 [details] [diff] [review]
UI piece

I left off the UI piece in the previous patch. Here it is.

I still need to sort out the placement of various components before I go for trunk review.
Comment 42 Wolfgang Rosenauer [:wolfiR] 2006-04-21 12:27:06 PDT
Comment on attachment 215059 [details] [diff] [review]
updated patch

>Index: toolkit/components/unixproxy/nsUnixSystemProxySettings.cpp
>+static PRBool
>+IsInNoProxyList(const nsACString& aHost, PRInt32 aPort, const char* noProxyVal)
>+{
>+  NS_ASSERTION(aPort >= 0, "Negative port?");

I'm not sure if that assertion is useful.
During my tests I figured out that it's pretty normal 
that the nsStandardURL has the default port -1
Comment 43 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2006-04-22 13:50:40 PDT
Then I need to add some code to map -1 to the correct default port.
Comment 44 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2006-04-25 23:00:02 PDT
I've got a new patch. I'll attach it later.
Comment 45 Diane Trout 2006-09-24 11:52:59 PDT
Created attachment 239925 [details] [diff] [review]
provides system proxy service for linux and os x

I wasn't sure how to handle a patch that applies to two bugs this one 66057 and  the os x specific version, 125995. So I'm posting it to both. 

This patch updates the previous patch for 66057 to the current CVS Head (I had a couple of problems applying the patch as a few files moved around), and also provides an implementation for retrieving the system settings from OS X.
 
I used a CVS checkout from yesterday for my testing, and verified that it basically works on both OS X and Linux--though under linux I only tested the environment variable proxy handling code.

I renamed "unixproxy" to "systemproxy" and added the OS X implementation. I also broke some of the environment handling code and utilities functions into a seperate file.
Comment 46 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2006-09-24 14:28:02 PDT
Created attachment 239951 [details] [diff] [review]
my latest patch

Here's a copy of my latest patch. I should have attached it before; my humble apologies. It fixes a few issues and you should merge it into yours. (Taking the diff between this patch and my last patch would help you with that.)
Comment 47 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2006-09-24 16:21:14 PDT
Bug 354075 will complete the move of the gnome component to toolkit/system/gnome, and once that's done the patch for this bug should be updated with the new file locations for that component. Also, we'll want to remove mozgnome from the REQUIRES in systemproxy/Makefile.in. And also, I think in some newsgroup discussion we decided to put the new component in netwerk/system/systemproxy (a new directory).

Does that sound OK? Then we should get review from Christian Biesinger (biesi).
Comment 48 Diane Trout 2006-09-24 23:04:46 PDT
Created attachment 239975 [details] [diff] [review]
merged my patch and Robert's patch

Ok I merged the two patches in this bug... What I pulled out from yours was:

In browser/components/prefernces/connection.xul the prefwindow doctype was a bit more complicated.

In netwerk/base/src/nsProtocolProxyService.cpp::Resolve_Internal there was a return NS_OK to replace the line "// Otherwise, allow the WPAD PAC lookup to go ahead"

In toolkit/components/gnome/nsGConfService.cpp there were two more includes: nsSupportsPrimitives.h and nsArray.h

I also moved the REQUIRES mozgnome into the conditional block for the gnome support.

I don't suppose I could talk you into applying this version of the patch and seeing how it works under gnome? My linux boxes run KDE and I've been seduced by the gray-side of OS X and haven't been spending much time running linux on my desktop.

The renaming you suggested in comment #47 seem pretty reasonable. 

Though since this is the first time I've gotten involved with mozilla I don't have any strong opinions about the proper place for modules. 

Also I've got a couple of questions? What newsgroup was it where the suggestion of moving systemproxy to netwerk/system showed up? Also where can I find some documentation on what the magic markup is that makes a hyperlink to a bug? 

Should I go ahead and apply the patch in 354075 and then make an updated version of this patch or wait until the cvs tree updates?
Comment 49 Diane Trout 2006-09-25 01:43:48 PDT
(In reply to comment #48)

Oh yeah I forgot to mention that I'm not sure if this patch will properly handle situations where the proxy exception list contains hosts of the form hostname:80 (or perhaps even when one connects to a host on the exception list with hostname:80). Basically I think the exception list and the base of the URI need to match, either both use the default port or both explicity list the :80.

I'm not sure how common this edge case'll be, but we might want to address it.
Comment 50 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2006-09-27 13:32:43 PDT
(In reply to comment #48)
> Also I've got a couple of questions? What newsgroup was it where the
> suggestion of moving systemproxy to netwerk/system showed up?

mozilla.dev.platform, thread subject "where to put per-platform system-services components"

> Also where can I find some documentation on what the magic markup is that
> makes a hyperlink to a bug?

In Bugzilla? You just say "bug " plus the bug number. Like "bug 354075".

> Should I go ahead and apply the patch in 354075 and then make an updated
> version of this patch or wait until the cvs tree updates?

I've just checked in the build system update from bug 354075. I will update your patch and rebuild, then you can test it on Mac, and if it's OK we'll go for review.

> Oh yeah I forgot to mention that I'm not sure if this patch will properly
> handle situations where the proxy exception list contains hosts of the form
> hostname:80

For Mac only, right? GNOME doesn't support port-based exceptions. I'll take a look at your code and give you some comments.
Comment 51 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2006-09-27 13:34:58 PDT
(In reply to comment #50)
> For Mac only, right? GNOME doesn't support port-based exceptions. I'll take a
> look at your code and give you some comments.

Oh, you mean the envvar stuff. Hmm.
Comment 52 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2006-09-27 13:41:15 PDT
You're right. We should fix this now because the easiest way is to just pass the default port to getProxyForURI as an additional parameter.
Comment 53 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2006-09-27 13:49:34 PDT
Your Mac code looks pretty good. I think we'd prefer it written Mozilla-style instead of Mac-style, e.g. using PRBool instead of "bool", InterCaps instead of underscored_words, and avoiding operator overloading. Biesi might be more lenient :-).

I'd make the little wrapper functions (SCProxySettings::ftp_enabled() etc) inline in the class definition.

+          CFIndex str_len = CFStringGetLength(str);
+          // the +1 is to include space for the null byte
+          char cStr[str_len+1];

This is a GCC extension. I've not seen it used before in Mozilla, I don't know what our policy is on using it in Mac-only code. I think we should probably avoid it if it's not too much pain.

+            // so are you supposed to include the null byte in a dependent 
+            // cstring?

No. What you have is correct.
Comment 54 Diane Trout 2006-09-27 14:18:31 PDT
(In reply to comment #51)
> (In reply to comment #50)
> > For Mac only, right? GNOME doesn't support port-based exceptions. I'll take a
> > look at your code and give you some comments.
> 
> Oh, you mean the envvar stuff. Hmm.
> 

I checked Safari, and it doesn't seem to parse entries with a port number correctly. Though the OS X dialog box certainly lets you enter them.
Comment 55 Diane Trout 2006-09-27 14:42:08 PDT
(In reply to comment #53)
> Your Mac code looks pretty good. I think we'd prefer it written Mozilla-style
> instead of Mac-style, e.g. using PRBool instead of "bool", InterCaps instead of
> underscored_words, and avoiding operator overloading. Biesi might be more
> lenient :-).

Heh, I'd think it's more the C++/Boost style than then Mac style, underscores seem to be falling out of favor. I can go ahead and make the it conform to your suggestions.

> +          CFIndex str_len = CFStringGetLength(str);
> +          // the +1 is to include space for the null byte
> +          char cStr[str_len+1];
> 
> This is a GCC extension. I've not seen it used before in Mozilla, I don't know
> what our policy is on using it in Mac-only code. I think we should probably
> avoid it if it's not too much pain.

I went looking through the variety of mozilla string types, and wasn't able to find a string class that lets you allocate a specific character buffer, is there a prefered idiom for creating temporary buffers? malloc, new, std::string, or something I missed in mozilla?
Comment 56 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2006-09-27 16:33:47 PDT
(In reply to comment #55)
> > This is a GCC extension. I've not seen it used before in Mozilla, I don't
> > know what our policy is on using it in Mac-only code. I think we should
> > probably avoid it if it's not too much pain.
> 
> I went looking through the variety of mozilla string types, and wasn't able
> to find a string class that lets you allocate a specific character buffer, is
> there a prefered idiom for creating temporary buffers? malloc, new,
> std::string, or something I missed in mozilla?

For this, I'd use nsAutoBuffer<char,SOME_CONSTANT>.
http://lxr.mozilla.org/mozilla/search?string=nsautobuffer

We have too many buffer and string types...
Comment 57 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2006-09-27 16:38:06 PDT
Created attachment 240384 [details] [diff] [review]
updated again

Okay, here's my update. It updates to the new file locations, fixes the default port issue, and also updates the GConf GetStringList changes to build without access to private Gecko symbols, which seems to be required now. I also cleaned up a few stylistic issues:
-- We prefer to put most shared functions as static methods of a class, so I did that in nsSystemProxyUtils.
-- license boilerplate goes at the top of files, above #ifdef FOOBAR_H__
-- Don't do "if (blort == PR_TRUE)", just do "if (blort)"
-- some other minor things I forgot

Over to you, Diane
Comment 58 Diane Trout 2006-09-28 02:04:12 PDT
> -- We prefer to put most shared functions as static methods of a class, so I
> did that in nsSystemProxyUtils.

Ok. Though since I haven't seen that done before, I'm curious as to what the advantage is?

> -- Don't do "if (blort == PR_TRUE)", just do "if (blort)"

What about (Pointer != NULL) is there a strong style-guide recommendation? I noticed in your code that you seemed to prefer if (Pointer).


> Over to you, Diane

Ok I'll try to have my updates up sometime tomorrow.
Comment 59 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2006-09-28 02:31:00 PDT
(In reply to comment #58)
> > -- We prefer to put most shared functions as static methods of a class, so I
> > did that in nsSystemProxyUtils.
> 
> Ok. Though since I haven't seen that done before, I'm curious as to what the
> advantage is?

It's just a way to get namespaces that we used when real C++ namespaces were poorly supported in compilers.

> > -- Don't do "if (blort == PR_TRUE)", just do "if (blort)"
> 
> What about (Pointer != NULL) is there a strong style-guide recommendation? I
> noticed in your code that you seemed to prefer if (Pointer).

Yes, I think we generally do prefer "if (ptr)". Some other projects don't though. I can see arguments either way. The argument for "if (myBool)" is stronger, though ... I can't see any reason for == PR_TRUE.

Thanks!
Comment 60 Diane Trout 2006-09-29 15:24:49 PDT
(In reply to comment #57)

> Over to you, Diane
> 

Ok I merged everything in and added the "Use system settings" option back to connection.xul and connection.dtd.

Unfortunately it doesn't work now. What I've traced it to is in:

if (mProxyConfig == eProxyConfig_System) {
    mSystemProxySettings = do_GetService(NS_SYSTEMPROXYSETTINGS_CONTRACTID);
}

As far as I could tell do_GetService is returning NULL. libsystemproxy.a is being built, but when I looked in netwerk/build/libnecko.a the files don't seem to have be added (while "most" other network related compents are). I'm guessing that now that systemproxy is part of netwerk it should be included in libnecko? Any suggestions besides carefully reading the make scripts?
Comment 61 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2006-09-29 21:54:13 PDT
I think we'll have to carefully read make scripts :-).

http://lxr.mozilla.org/mozilla/source/netwerk/build/Makefile.in
I think we need to add the right thing to SHARED_LIBRARY_LIBS.

Also, we should move the build step out of toolkit_tiers and add "system" to the DIRS in netwerk/Makefile.in. netwerk/system will need a stub Makefile.in that adds systemproxy to DIRS if we're on Mac or Linux.
Comment 62 Diane Trout 2006-09-30 01:33:14 PDT
(In reply to comment #61)
> http://lxr.mozilla.org/mozilla/source/netwerk/build/Makefile.in
> I think we need to add the right thing to SHARED_LIBRARY_LIBS.

Oh! When I looked at SHARED_LIBRARY_LIBS I thought about dynamically loaded objects, not this is the list of libraries shared by this component.

When I made that update, nsOSXSystemProxy.o and nsSystemProxyUtils.o ended up in libnecko.

> Also, we should move the build step out of toolkit_tiers and add "system" to
> the DIRS in netwerk/Makefile.in. netwerk/system will need a stub Makefile.in
> that adds systemproxy to DIRS if we're on Mac or Linux.

I'd managed to figure out where I'd needed to add systemproxy to the right Makefile.in DIRS variable. Although I'm still stumped for a good makefile test for the unix proxy. (I've found ifeq ($(OS_ARCH),Darwin) for os x.

Unfortunately firefox still doesn't actually use the proxy. Now my suspicions are the XPCOM configuration isn't quite right. The Init function for nsOSXSystemProxy doesn't seem to be called when I launch firefox. When looking at most of the other modules in netwerk, most of them seem to belong to the netwerk module, and their NS_*_FACTORY_CONSTRUCTOR macros mostly appear to be in netwerk/build/nsNetModule.cpp.

I think I'd like to read up more on XPCOM, and see if I can figure out what's up Saturday.
Comment 63 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2006-09-30 03:02:24 PDT
(In reply to comment #62)
> > Also, we should move the build step out of toolkit_tiers and add "system" to
> > the DIRS in netwerk/Makefile.in. netwerk/system will need a stub Makefile.in
> > that adds systemproxy to DIRS if we're on Mac or Linux.
> 
> I'd managed to figure out where I'd needed to add systemproxy to the right
> Makefile.in DIRS variable. Although I'm still stumped for a good makefile test
> for the unix proxy. (I've found ifeq ($(OS_ARCH),Darwin) for os x.

I think we should just use MOZ_ENABLE_GTK2 for unixproxy.

> Unfortunately firefox still doesn't actually use the proxy. Now my suspicions
> are the XPCOM configuration isn't quite right. The Init function for
> nsOSXSystemProxy doesn't seem to be called when I launch firefox. When looking
> at most of the other modules in netwerk, most of them seem to belong to the
> netwerk module, and their NS_*_FACTORY_CONSTRUCTOR macros mostly appear to be
> in netwerk/build/nsNetModule.cpp.

The XPCOM component is not getting registered. I used to have systemproxy building as a standalone XPCOM component in its own shared library, but I removed the FORCE_SHARED_LIBRARY setting, which broke that I guess (not sure why things worked when I tested my patch). There is actually no reason to be a standalone XPCOM component, we should just link tightly into the rest of necko. That means we need to be added to nsNetModule, because our own module isn't going to be registered. It should be just a matter of moving this code

+#define NS_UNIXSYSTEMPROXYSERVICE_CID  /* 0fa3158c-d5a7-43de-9181-a285e74cf1d4 */\
+     { 0x0fa3158c, 0xd5a7, 0x43de, \
+       {0x91, 0x81, 0xa2, 0x85, 0xe7, 0x4c, 0xf1, 0xd4 } }
+
+NS_GENERIC_FACTORY_CONSTRUCTOR_INIT(nsUnixSystemProxySettings, Init)
+
+static const nsModuleComponentInfo components[] = {
+  { "Unix System Proxy Settings Service",
+    NS_UNIXSYSTEMPROXYSERVICE_CID,
+    NS_SYSTEMPROXYSETTINGS_CONTRACTID,
+    nsUnixSystemProxySettingsConstructor }
+};

to the right place in nsNetModule.
Comment 64 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2006-09-30 03:13:23 PDT
Well, you'll also need to massage header file paths to get the right headers visible in nsNetModule.
Comment 65 Diane Trout 2006-09-30 23:22:56 PDT
Created attachment 240791 [details] [diff] [review]
move system proxy into libnecko

Ok, I have a couple of concerns about this patch.

My biggest concern is that I needed to modify browser/app/Makefile.in in order to add the -framework link option and I'm suspect this would break other applications that depend on libnecko.

I couldn't find a good make variable to set in the netwerk/build/Makefile.in that would actually influence the application build. Also I'm not certain but it seems like frameworks cannot be linked against static libraries. (If needed I can try testing that a bit more carefully).

The second concern is that nsNetModule.cpp looks pretty complex and I didn't want to add a conditional compliation block to include the os x and unix specific proxies, instead I renamed both classes to nsSystemProxySettings and moved the class definition to its own .h file. 

I was also put the NS_SYSTEMPROXYSETTINGS_CLASSNAME and NS_SYSTEMPROXYSETTINGS_CID in as well, which made defining the magic XPCOM blocks in nsNetModule.cpp a bit easier.

The downside is to make the same class header apply to both our classes I need to conditionally include the mGConf private variable. So mostly this modification tried to localize the complexity in netwerk/system/systemproxy to try and simplify its external interface.

It runs correctly on OS X, and I'm having build problems with some svg code on my linux box so wasn't able to test there. (And that was after trying to disable cairo which doesn't seem to link correctly on a debian unstable box).

Diane
Comment 66 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2006-10-01 13:17:53 PDT
(In reply to comment #65)
> I couldn't find a good make variable to set in the netwerk/build/Makefile.in
> that would actually influence the application build. Also I'm not certain but
> it seems like frameworks cannot be linked against static libraries. (If needed
> I can try testing that a bit more carefully).

There are two kinds of builds: static builds, where all the Gecko components are linked into one big shared library, and nonstatic builds where components get their own shared libraries. In the latter case, adding -framework flags to netwerk/build should ensure that the libnecko shared library links correctly and everything will be fine. For the former case, I think things will work if we add to http://lxr.mozilla.org/mozilla/source/config/static-config.mk
so the final libxul shared library links to the frameworks (right, bsmedberg?)

> The downside is to make the same class header apply to both our classes I need
> to conditionally include the mGConf private variable. So mostly this
> modification tried to localize the complexity in netwerk/system/systemproxy to
> try and simplify its external interface.

I think this approach is OK. #ifdef GNOME probably isn't the right thing. You could actually just make mGConf unconditional, it should build on OSX because nsIGConfService is present there, just not implemented by any component. You should delete the associated comment. Also you can delete the commented-out component registration in nsOSXSystemProxySettings/nsUnixSystemProxySettings. Thanks!!!
Comment 67 Diane Trout 2006-10-01 22:33:17 PDT
Created attachment 240886 [details] [diff] [review]
Now using mozilla/config/static-config.mk

Ok adding an ifeq/endif block to static-config.mk to add the -framework did work, and I removed the conditional around the mGConf variable.

I also applied to a clean tree and verified that this patch works on OS X.

I hope we're almost there. 

And I wanted to thank you with your help with the details of the build system.
Diane
Comment 68 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2006-10-01 23:10:10 PDT
Why have you commented these out?

+# FRAMEWORKS += \
+#         -framework SystemConfiguration\
+#         -framework CoreFoundation\
+#         $(NULL)

+# EXTRA_DSO_LDOPTS += \
+#         $(FRAMEWORKS)   

This is needed for non-static builds, isn't it?
Comment 69 Diane Trout 2006-10-01 23:20:40 PDT
(In reply to comment #68)
> Why have you commented these out?

Because I forgot to delete them?

They're left over from when this was being built as a separate component, and I wasn't completely sure that they weren't needed any more. 
Comment 70 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2006-10-01 23:29:00 PDT
I think we need them in netwerk/build/Makefile.in to ensure that the libnecko shared library imports those libraries properly, for non-static builds.
Comment 71 Diane Trout 2006-10-01 23:41:35 PDT
(In reply to comment #70)
> I think we need them in netwerk/build/Makefile.in to ensure that the libnecko
> shared library imports those libraries properly, for non-static builds.
> 

Any suggestions about how I could force a non-static build?
Comment 72 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2006-10-02 00:14:05 PDT
I thought non-static was the default. Maybe you have --enable-static in your mozconfig somewhere?
Comment 73 Diane Trout 2006-10-02 00:39:21 PDT
(In reply to comment #72)
> I thought non-static was the default. Maybe you have --enable-static in your
> mozconfig somewhere?
> 

You're right! I'd just blindly set up my mozconfig from the OS X configuration. Sorry to have been pestering you with so many questions...

I've made the changes you suggested and will try testing tonight or tomorrow morning.
Comment 74 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2006-10-02 00:44:34 PDT
Honestly, the build system is pretty opaque to me too.
Comment 75 Diane Trout 2006-10-02 10:17:13 PDT
Created attachment 240953 [details] [diff] [review]
Builds as shared & static

Ok, new version. This has the framework options back in the shared configuration part of netwerk/build/Makefile.in

I built it as both --disable-shared --enable-static and --enable-static --disable shared. (Though to be fair when retesting this round of the static build I didn't do a make clean after the shared build).

The other possibly questionable thing is I used "FRAMEWORKS +=" instead of "=" or ":=". I don't think that variable was defined in this scope, but I suppose there might be a chance that it's used later. (Though I don't think multiple copies of the same -framework are a problem).
Comment 76 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2006-10-02 15:49:11 PDT
Comment on attachment 240953 [details] [diff] [review]
Builds as shared & static

okay, let's get this going!
Comment 77 Diane Trout 2006-10-02 23:28:31 PDT
(In reply to comment #76)
> (From update of attachment 240953 [details] [diff] [review] [edit])
> okay, let's get this going!
> 

Yay!

I'm excited to have gotten this patch this far.
Comment 78 Diane Trout 2006-10-13 01:55:19 PDT
(In reply to comment #76)
> (From update of attachment 240953 [details] [diff] [review] [edit])
> okay, let's get this going!
> 

Just wanted to mention that Josh Aas was reviewing the old version of this patch that I'd left under bug 125995.

Of potential interest to you, he wanted the copyright headers cleaned up and I guessed how you should be attributed in the files you created, but you might want to review.
Comment 79 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2006-10-15 20:02:47 PDT
Just merge in any changes from Josh's review in bug 125995 and reattach the result here ... thanks
Comment 80 Diane Trout 2006-10-18 00:28:29 PDT
Created attachment 242611 [details] [diff] [review]
Merged in Josh's requested updates from bug 125995

He had me update the copyright notices and return nsresults to indicate failure of a call instead of a PRBool.
Comment 81 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2006-10-18 14:01:49 PDT
Comment on attachment 242611 [details] [diff] [review]
Merged in Josh's requested updates from bug 125995

still needs biesi love
Comment 82 Christian :Biesinger (don't email me, ping me on IRC) 2007-02-02 12:44:40 PST
Comment on attachment 242611 [details] [diff] [review]
Merged in Josh's requested updates from bug 125995

OK, my sincere apologies for taking so long to review this. This "real life"
thing kept me busy the last few months.

(Is there a reason you didn't use cvs diff?)

A Firefox peer will have to review the UI changes here. It would also be nice
if you could file bugs on SeaMonkey and Camino about this new pref value.

Please add the -p diff option next time...


+++ mozilla/netwerk/base/public/nsISystemProxySettings.idl	2006-10-12 23:56:06.000000000 -0700
+#define NS_SYSTEMPROXYSETTINGS_CONTRACTID "@mozilla.org/system-proxy-settings;1"

Don't put contract IDs in IDL files. Instead, put them in nsNetCID.h.

+ * This interface allows the proxy code to use platform-specific proxy
+ * settings when the proxy preference is set to "automatic discovery". If it can
+ * load a service with the above contract ID, it will use it to determine the
+ * PAC file name. If no PAC file is specified then the service itself will behave
+ * like a PAC file.

I'm not sure what the "it" refers to in the second line of this comment. Is
this a description of how the protocol proxy service behaves? If so, why is
this comment here instead of somewhere specific to the protocol proxy service?

+     * See nsIProxyAutoConfig::getProxyForURI; this function behaves exactly
+     * the same way.

So, if it behaves exactly the same way, why does its signature differ?



+++ mozilla/netwerk/base/src/nsProtocolProxyService.cpp	2006-10-13 00:16:26.000000000 -0700
 
+
 //----------------------------------------------------------------------------

Why this change?

+    if (mProxyConfig != eProxyConfig_PAC && mProxyConfig != eProxyConfig_WPAD
+        && mProxyConfig != eProxyConfig_System)

Please put operators at the end of a line instead of at the start of the next
line.

-        ConfigureFromPAC(tempString);

Hm... doesn't this mean that you're no longer reloading the PAC URL when PAC
is configured explicitly and the URL changes?

+    if (mProxyConfig != eProxyConfig_PAC && mProxyConfig != eProxyConfig_WPAD
+                && mProxyConfig != eProxyConfig_System)

Perhaps this should move to a helper function instead? (Otherwise, the same
operator comment, and the second line is incorrectly indented)

     NS_ENSURE_ARG_POINTER(uri);
-
     *usePAC = PR_FALSE;

why this change?

+            // Switch to new PAC file if that setting has changed.

Hm... I'm not sure I like this... Can't the system proxy service broadcast a
message if the PAC URL changes?

Doesn't ReloadPAC need a change too?

+++ mozilla/netwerk/build/Makefile.in	2006-10-12 23:26:13.000000000 -0700
-
+        

Please don't add trailing whitespace

+# It'd be nice if there was a way for testing, GTK and Darwin

There is such a way:
ifneq (,$(filter gtk2 mac cocoa,$(MOZ_WIDGET_TOOLKIT)))

+++ mozilla/netwerk/system/systemproxy/Makefile.in	2006-10-12 23:26:13.000000000 -0700
+REQUIRES = \
+        xpcom \
+        string \
+        necko

Please add a line with $(NULL). That way, if someone adds another dependency
at the end, the diff output will look nicer.

+++ mozilla/netwerk/system/systemproxy/nsOSXSystemProxySettings.cpp	2006-10-13 00:56:13.000000000 -0700
+ * The Initial Developer of the Original Code is
+ * Diane Trout.

Usually this includes an email address... (And usually the original author
isn't also listed as contributor)

+        aResult.Append(pacUrl);

This must be Assign, not Append. There is no guarantee that aResult is already
an empty string.

This happened in a few other places as well.

+  PRInt32 proxyPort=0;
+  PRBool isDirectHost=false;

Usually there are spaces around operators... and is there a reason you are
mixing "false" with a PRBool? That'd usually be PR_FALSE. I'd also move
isDirectHost to the line before it is used. But come to think of it, why not
make InExceptionList return the PRBool and remove this variable?

+    proxy.HTTPHost(proxyHost);
+    proxy.HTTPPort(proxyPort);

Hm... I'd name functions that don't return the value GetHTTPHost, etc. (return
meaning as actual return value)

+nsSystemProxySettings::GetProxyForURI(nsIURI* aURI, PRInt32 aDefaultPort,
+                                         nsACString& aResult)

Incorrect indentation on the second line...

+    return GetProxyForURIFromSystemConfig(proxySettings, scheme, host, port, aResult);
+  } else {

No point in an else after a return.

I didn't review this file in detail, I don't know the Mac APIs. I hope someone
else did.

+++ mozilla/netwerk/system/systemproxy/nsSystemProxySettings.h	2006-10-13 00:10:01.000000000 -0700
+ * The Initial Developer of the Original Code is
+ * Robert O'Callahan.

same comment as above :)


Is it really a good idea to use the same header for this service on all
platfoprms, meaning that OS X can't have a SCProxySettings member and must
have an mGConf member which it has no use for?

+  nsSystemProxySettings() {};  

please remove that semicolon here

+++ mozilla/netwerk/system/systemproxy/nsSystemProxyUtils.cpp	2006-10-13 00:11:32.000000000 -0700
+ * The Initial Developer of the Original Code is
+ * Robert O'Callahan.

as above

    nsCAutoString portStr2(portStr);

is that really needed? :/ i.e. portStr.ToInteger doesn't exist?

+    if (StringEndsWith(aHost, hostStr, nsCaseInsensitiveCStringComparator()))

Why's this a StringEndsWith instead of an equality comparison? The header
definitely needs more comments about what this function does.

BTW, a few lines in this file have trailing whitespace. Please remove that :)

  if (!isHTTP)
    return NS_ERROR_FAILURE;

why not use NS_ERROR_UNKNOWN_PROTOCOL?

Hm, what'd the desired result for a URI without an explicit port be? Your code
would put -1 as the port in the string, I believe. That doesn't seem
desirable.

+++ mozilla/netwerk/system/systemproxy/nsSystemProxyUtils.h	2006-10-13 00:06:58.000000000 -0700

BTW, I wouldn't use HPP in the include guards, the filename is .h not .hpp.

So the comments in this file don't help me much...
+   * Test a specific proxy entry against aHost:aPort. (handles optional port)

What does "handle" mean? I.e., if there is no :port in proxyEntry, what
happens?

+   * Format a proxy result and store it in aResult

Perhaps this should mention how it formats the result (i.e. in the format
expected by PAC), and what possible aType values are

+   * Construct a ProxyURI from environment variables (http_proxy, etc)

This is comment is somewhat misleading. This doesn't construct any URI. It
parses an URI and returns a PAC-like string identifying the proxy.

+  static nsresult GetProxyForURIFromEnvironment(const nsACString& aScheme,

Since this doesn't take a URI, perhaps the ForURI part should be removed from
the name.

Should perhaps mention that the scheme must be lowercase.

#endif /*NSSYSTEMPROXYENVIRONMENT_HPP_*/

please add a space after /* and before */.

+++ mozilla/netwerk/system/systemproxy/nsUnixSystemProxySettings.cpp	2006-10-13 00:20:00.000000000 -0700
+  // was not a GNOME session, using *_PROXY environment variables.

It's *_proxy :)

+  return NS_SUCCEEDED(aGConf->GetString(NS_LITERAL_CSTRING("/system/proxy/mode"), mode)) &&
+      mode.EqualsASCII(aMode);

please indent mode so that it ends up under the NS_ here

+IsProxyMode(nsIGConfService* aGConf, const char* aMode)

If you made this a member function, you wouldn't need the aGConf argument.

  if (StringBeginsWith(aIgnore, NS_LITERAL_CSTRING("*")) &&

how about: |if (aIgnore.First() == '*' &&| ?

Should /system/http_proxy/use_http_proxy be checked as well?

+++ mozilla/toolkit/system/gnome/nsGConfService.cpp	2006-10-12 23:26:13.000000000 -0700

+  virtual ~StringListEnumerator() {
+    g_slist_free(mList);
+  }

The documentation at
http://developer.gnome.org/doc/API/2.0/gconf/gconf-GConfClient.html#gconf-client-get-list
says:
> In the GCONF_VALUE_FLOAT and GCONF_VALUE_STRING cases, you must g_free() each list element.

+++ mozilla/xpcom/system/nsIGConfService.idl	2006-10-12 23:26:13.000000000 -0700

need a new IID
Comment 83 Colin Barrett [:cbarrett] 2007-08-18 06:51:13 PDT
diane, any chance of you updating the patch to address biesi's sr comments?
Comment 84 Michael Ventnor 2007-12-17 20:11:17 PST
Created attachment 293620 [details] [diff] [review]
Updated Unix patch

Roc's old Unix patch carried through over a year of bitrot. I didn't bring along the Mac stuff since I don't understand Mac, and I'll do the UI bits later.
Comment 85 Michael Ventnor 2007-12-17 20:20:47 PST
Just to mention, as Roc suggested I do, the patch above by itself has no UI, instead it uses a hidden preference in about:config by setting network.proxy.type to 5. This patch would be extraordinarily useful for Linux distributors especially for Firefox 3 where proxy integration has sorely lacked, as they can ship that preference as default in their Firefox 3 packages.
Comment 86 Michael Ventnor 2007-12-18 12:25:38 PST
Created attachment 293721 [details] [diff] [review]
Updated Unix patch 1.1

I really must remember the ifdefs since we cater to more than just Linux...
Comment 87 timeless 2008-01-21 13:50:42 PST
Comment on attachment 293721 [details] [diff] [review]
Updated Unix patch 1.1

bad indentation (maybe tabs?):
+            nsresult rv = ConfigureFromPAC(PACURI);
+           if (NS_FAILED(rv))
+                return rv;

proxy_MaskIPv6Addr's indentation doesn't match the file.
Comment 88 Michael Ventnor 2008-01-23 15:40:57 PST
Created attachment 298800 [details] [diff] [review]
String additions for possible future UI

All it would take to make UI for this is to add a radio button in the Connection dialog. This patch gives the strings that will allow for that.
Comment 89 Mike Beltzner [:beltzner, not reading bugmail] 2008-01-24 12:29:31 PST
Comment on attachment 298800 [details] [diff] [review]
String additions for possible future UI

a=beltzner for 1.9
Comment 90 Reed Loden [:reed] (use needinfo?) 2008-01-25 00:24:57 PST
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.8; previous revision: 1.7
done
Comment 91 Christian :Biesinger (don't email me, ping me on IRC) 2008-01-28 00:19:38 PST
Comment on attachment 293721 [details] [diff] [review]
Updated Unix patch 1.1

+    ACString getProxyForURI(in nsIURI aURI);

I think this should better return AUTF8String (so that people can specify an IDN host for the proxy)

+  nsCOMPtr<nsIMutableArray> items (do_CreateInstance(NS_ARRAY_CONTRACTID));

no space after items

+    nsCOMPtr<nsISupportsCString> obj (do_CreateInstance(NS_SUPPORTS_CSTRING_CONTRACTID));

no space after obj

Also, this should be an nsISupportsString, given that gconf stores UTF-8 data and nsISupportsCString only supports Latin1.

+  nsIArray      getStringList(in AUTF8String key);

would be nice to add an comment about the type of the items in the array

+                tempString.AssignLiteral("http://wpad/wpad.dat");

shouldn't that be WPAD_URL?

+            // Switch to new PAC file if that setting has changed. If the setting
+            // hasn't changed, ConfigureFromPAC will exit early.
+            nsresult rv = ConfigureFromPAC(PACURI);
+           if (NS_FAILED(rv))
+                return rv;

indentation is not correct here

+        eProxyConfig_System, // use system proxy settings if available, otherwise WPAD

the behaviour seems to rather be, "otherwise DIRECT", no?

+{
+  if (!mGConf || !IsProxyMode("auto"))
+    return NS_ERROR_FAILURE;

the IDL says to return an empty string in that case, not to throw an exception

+      if (err != 0) {

should be NS_FAILED(err)

+static void SetProxyResult(const char* aType, const nsACString& aHost,
+                               PRInt32 aPort, nsACString& aResult)

incorrect indentation on the second line
Comment 92 Michael Ventnor 2008-01-28 10:56:22 PST
Created attachment 299799 [details] [diff] [review]
Nits fixed

Thanks biesi!

Now please lets get this oft-desired patch into 1.9! The thing is, this is Novell code that Roc wrote and he tells me that Novell has been shipping this for years, so that means it must work well as it has undergone a lot of testing. It'll be about:config only with just this patch, and likely Beta 3, but UI would be trivial to implement since the string has already landed.

This is one of the best things we could ever do for Linux integration.
Comment 93 Damon Sicore (:damons) 2008-01-28 19:53:08 PST
Comment on attachment 299799 [details] [diff] [review]
Nits fixed

a1.9+=damons
Comment 94 Reed Loden [:reed] (use needinfo?) 2008-01-29 07:59:16 PST
RCS file: /cvsroot/mozilla/netwerk/base/public/nsISystemProxySettings.idl,v
done
Checking in netwerk/base/public/nsISystemProxySettings.idl;
/cvsroot/mozilla/netwerk/base/public/nsISystemProxySettings.idl,v  <--  nsISystemProxySettings.idl
initial revision: 1.1
done
Checking in netwerk/base/public/Makefile.in;
/cvsroot/mozilla/netwerk/base/public/Makefile.in,v  <--  Makefile.in
new revision: 1.120; previous revision: 1.119
done
Checking in toolkit/system/gnome/nsGConfService.cpp;
/cvsroot/mozilla/toolkit/system/gnome/nsGConfService.cpp,v  <--  nsGConfService.cpp
new revision: 1.6; previous revision: 1.5
done
Checking in xpcom/system/nsIGConfService.idl;
/cvsroot/mozilla/xpcom/system/nsIGConfService.idl,v  <--  nsIGConfService.idl
new revision: 1.3; previous revision: 1.2
done
Checking in netwerk/base/src/nsPACMan.h;
/cvsroot/mozilla/netwerk/base/src/nsPACMan.h,v  <--  nsPACMan.h
new revision: 1.6; previous revision: 1.5
done
Checking in netwerk/base/src/nsProtocolProxyService.cpp;
/cvsroot/mozilla/netwerk/base/src/nsProtocolProxyService.cpp,v  <--  nsProtocolProxyService.cpp
new revision: 1.72; previous revision: 1.71
done
Checking in netwerk/base/src/nsProtocolProxyService.h;
/cvsroot/mozilla/netwerk/base/src/nsProtocolProxyService.h,v  <--  nsProtocolProxyService.h
new revision: 1.31; previous revision: 1.30
done
Checking in toolkit/toolkit-makefiles.sh;
/cvsroot/mozilla/toolkit/toolkit-makefiles.sh,v  <--  toolkit-makefiles.sh
new revision: 1.10; previous revision: 1.9
done
Checking in toolkit/Makefile.in;
/cvsroot/mozilla/toolkit/Makefile.in,v  <--  Makefile.in
new revision: 1.31; previous revision: 1.30
done
RCS file: /cvsroot/mozilla/toolkit/system/unixproxy/Makefile.in,v
done
Checking in toolkit/system/unixproxy/Makefile.in;
/cvsroot/mozilla/toolkit/system/unixproxy/Makefile.in,v  <--  Makefile.in
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/toolkit/system/unixproxy/nsUnixSystemProxySettings.cpp,v
done
Checking in toolkit/system/unixproxy/nsUnixSystemProxySettings.cpp;
/cvsroot/mozilla/toolkit/system/unixproxy/nsUnixSystemProxySettings.cpp,v  <--  nsUnixSystemProxySettings.cpp
initial revision: 1.1
done
Checking in toolkit/library/libxul-config.mk;
/cvsroot/mozilla/toolkit/library/libxul-config.mk,v  <--  libxul-config.mk
new revision: 1.62; previous revision: 1.61
done
Checking in toolkit/library/nsStaticXULComponents.cpp;
/cvsroot/mozilla/toolkit/library/nsStaticXULComponents.cpp,v  <--  nsStaticXULComponents.cpp
new revision: 1.51; previous revision: 1.50
done
Checking in netwerk/build/nsNetCID.h;
/cvsroot/mozilla/netwerk/build/nsNetCID.h,v  <--  nsNetCID.h
new revision: 1.68; previous revision: 1.67
done
Comment 95 Reed Loden [:reed] (use needinfo?) 2008-01-29 10:04:31 PST
Backed out most of this due to bustage on debug Linux. Most likely we're just missing something simple in toolkit/system/unixproxy/Makefile.in.

http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1201629360.1201629560.21211.gz&fulltext=1#err0

bsmedberg: ideas?
Comment 96 Reed Loden [:reed] (use needinfo?) 2008-01-29 12:30:45 PST
I relanded this. Ventron said he will file a bug on Mac adding support for this.
Comment 97 Stefan [:stefanh] 2008-01-29 13:23:55 PST
(In reply to comment #96)
> I relanded this. Ventron said he will file a bug on Mac adding support for
> this.
> 

The mac part was moved from bug 125995 to here ;-)
Comment 98 Michael Ventnor 2008-01-29 13:26:47 PST
(In reply to comment #96)
> I relanded this. Ventron said he will file a bug on Mac adding support for
> this.
> 

Actually I was going to let a Mac user file that bug, but it seems I don't need to with bug 125995 :)
Comment 99 1ac7b2edaa08e4edd3334c5dc4b966af 2008-02-12 11:17:38 PST
I'm trying to compile thunderbird and it fails since mozilla/toolkit/system/unixproxy does not provide the target "export".
Im working with ubuntu 7.10 and had no problems compiling firefox on 26.1.08.
Is anybody else experiencing this trouble?
Comment 100 Magnus Melin 2008-06-30 01:48:36 PDT
*** Bug 226816 has been marked as a duplicate of this bug. ***

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