Closed Bug 125995 Opened 23 years ago Closed 16 years ago

Proxy: Take settings from Network Preferences [Mac OS X]

Categories

(Core :: Networking, enhancement, P2)

Product:
Core
Component:
Networking
Platform:
All
macOS
Type:
enhancement

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla1.9.1a1

People

(Reporter: bugzilla, Assigned: jamesbunton)

References

(Blocks 1 open bug)

Details

Attachments

(6 files, 10 obsolete files)

Screenshot of dialog in 1.5RC1
69.71 KB, image/jpeg
Details
Patch file - needs "extra files" also
35.85 KB, patch
Details | Diff | Splinter Review
The new files associated with patch1
50.00 KB, patch
Details | Diff | Splinter Review
Made requested updates to patch from bug 66057
63.61 KB, patch
diane
: review+
Details | Diff | Splinter Review
Mac OSX implementation of system proxy settings - #6
16.83 KB, patch
jamesbunton
: review+
roc
: superreview+
Details | Diff | Splinter Review
Makes the OSX system proxy work in release builds
768 bytes, patch
benjamin
: review+
Details | Diff | Splinter Review 
Other browsers are (slowly) taking their proxy settings from the Network
Preferences settings area. This allows users to change "Location" and quickly
have proxy settings change.
At this stage, users need to access Preferences each time location changes.
-> Networking for backend support
Assignee: sgehani → new-network-bugs
Component: Preferences → Networking
QA Contact: sairuh → benc
+qawanted - dupeme.
Keywords: qawanted
The "Location" settings on Mac OS X are great and I'd love to use Mozilla at
home and at work. For the moment I'll stick with IE as it supports "Location"
and only use Mozilla at home.

In my opinion this is a very important one on the Mac platform!
*** This bug has been confirmed by popular vote. ***
Status: UNCONFIRMED → NEW
Ever confirmed: true
changed summary for consistency and searchability.

+helpwanted, please find some Mac experts for this bug.
Blocks: 139393
Keywords: helpwanted
Summary: Take proxy settings from Network Preferences on Mac OS X → [RFE] Proxy: Take settings from Network Preferences [Mac OS X]
I agree that implementing a fix for this bug is very important.

For laptops especially, this is crucial. One of the main advantages in using OSX
is the fact that you can switch network 'locations' without having to restart
applications (let alone rebooting, which is required on a lot of other
platforms). I use this at home to switch between 'direct to internet' and 'vpn
tunnel to work' setting several times a day, and then I take my laptop to work
and other people's houses (with WAPs). As it is, I need to open Mozilla's
preferences panel and switch between direct, automatic, and manual, depending on
where I am.

Max.
I've noticed that Chimera (chimera.mozdev.org) does this quite happily as of
0.2.6, and also gets the OS homepage settings.

Perhaps that is a good place to look for some idea on how it's done...
Removing qawanted since this apparently isn't a duplicate of the Classic proxy
settings bug, bug 97214.
Keywords: qawanted
OK, this bug is causing me enough pain to make me move to Internet Explorer, so
I won't be using Mozilla any more (until this is fixed).

Max.
If you are a very "pure" Mac OS X user, you should consider loading Chimera,
which as someone mentioned to me, does read these prefs.

I have not had time to re-write and productize my proxy test to Chimera, but
nobody has filed a bug that complains it behavees badly.
codewise, mdescalz@mindspring.com pointed out that we should look here:

http://lxr.mozilla.org/mozilla/source/chimera/src/preferences/
PreferenceManager.mm#233 
*** Bug 168321 has been marked as a duplicate of this bug. ***
We also need to decide if parts of bug 66057 affect Mac OS X, because Mac OS X
=> NeXTSTEP => BSD UNIX :)

There might already be a relationship between the Networking Prefs and the shell
variables at the OS level...
Pink, Simon, is this being done for Chimera?
Target Milestone: --- → Future
Chimera does read proxy settings from Internet Config at startup. I'm not sure
what else we have to do.
Summary: [RFE] Proxy: Take settings from Network Preferences [Mac OS X] → Proxy: Take settings from Network Preferences [Mac OS X]
for what it is worth, MSIE 5.2.2 does not understand this setting too well
either -- changing locations forces the application to be restarted in order for
it to take the new proxy. again, this is crucial for multihomed laptops.

also, i cant seem to find any setting in the environment for the "network
settings"... i think that takes that solution off the map.

alex
Yes. I change my 'location' frequently (several times a day), and have to switch
proxy settings (between direct and manual settings) each time, even though the
settings are change for me in the networking preferences.

It would be a real time saver if mozilla were to change them for me. Is there no
way that the system can tell mozilla when the location has changed, which will
trigger mozilla to fetch the new settings?

Max.
I'm in a similar situation to the laptop users who made comments earlier.

Even an arrangement like that used by MSIE 5.2.2 would be better than the
current daily proxy prefs edit by hand?



I think that being able to select a preference that allows a user to use the
locations proxy settings for Mozilla would benefit those of us that move between
2 or more locations that may or may not have a proxy server. I too find it a
pain to change my settings 3 times a day. For some reason I still prefer Mozilla
over Chimera or Camino I guess is what its being called now. 

Please excuse my ignorance in the dev process or logic, but would it be possible
to have Mozilla keep somewhat of a session where after x specified of time
passes, Mozilla will check OS X for new proxy settings. This may keep from
having to quit and restart Mozilla. I personall don't mind having to quit the
browser and restart it if I move to a location that changes my proxy settings. 

Another thought is to specify the amount of time in whioch you would like
Mozilla to check for new settings, this would be somewhat like how History can
be kept for x number of days. Unless of course this too is dependent on the
browser being quit and restarted. If it is not dependent on this, maybe the
settings could range from x number of hours (for those who only need a proxy
server at work) to x number of days (for those on a business trip that may need
the settings for 5 days). 
how often does IE read the system value?
We can register to get notified when any proxy settings change. Chimera does
this. We don't have to poll.
*** Bug 256145 has been marked as a duplicate of this bug. ***
*** Bug 268736 has been marked as a duplicate of this bug. ***
Camino does this now, so I think there is some code that could be ported to
Firefox/Mac.

I'm not sure we want this to work transparently in mozilla.
From comment 24:

> I'm not sure we want this to work transparently in mozilla.

Why not? This is really needed for users with laptops in different environments.
I can't see why this should be implemented in Camino and Fx, but not in Seamonkey.
It is really useful to be able to make settings independantly of the OS X
settings, so I'd love to have the capability to override the OS settings...
Max: Since you can manage several environments with OS X, I can't see, where
this is useful, except having different proxies for different browsers. But I
can't see the point in having different settings in different browsers.
Perhaps the best UI would be an additional radio button "Use Control Panel Settings".  This would 
preserve the existing configuration capability (including support for proxyconfig urls) but add location 
support (which I could really use :-) )
Paul: I wouldn't mind if there were this possibility, but I would consider it
bad design to reflect it in the UI. IMHO, a hidden pref would be better.

Anyway, since system-wide proxy autoconfiguration is possible since OS X 10.3.2,
I also can't see any advantage in having browser-specific options.
On occasion (mainly for testing proxy servers and access to different web
sites), I have found it useful to have different browsers set up to use
different proxies, or no proxies. We have a company proxy server, but we can
access the internet directly too, and, of course, we can use an anonymous proxy.

Using the system settings would not be satisfactory for this use.

Having a hidden option, or an advanced tab, would be fine for my purposes.

It really isn't a big issue for me, though I suspect it might be for some.
Blocks: macmeta
This missing feature is the sole reason why I still don't use this browser.

I can't believe that there are "religious" wars whether to support this
important feature (at least for me it is). If I would at least be given a
choice, even by setting a hidden option in a config file. I've given up hope.

PS: Just noticed my first comment on this dates back to february 2002. Pity.
I want to try to fix this bug, because it just honks me off that it doesn't work correctly. Reading through 
the comments makes me think it's reasonably straight-forward, so I may be able to do it during the 
Christmas/New Year's break. 

What I'm looking for is some insight into how we (this community) would like to see it come out. First, I 
think the default value should be to use the proxy setting of the System setting, but I don't see any 
reason to prevent someone from changing the settings to something else if they want to. That means, 
to me, that there's another radio button in the Preferences>Advanced>Proxies page, perhaps *before* 
the "Direct connection to the Internet" option that says "Use settings from System Preferences." Looking 
at something similar in Safari, there's a button that takes you there. I'm not sure I want that in here 
(what do you think). I'd change the default to select this option. This is a MacOS-only thing, I think - 
are there other systems that have similar "system-wide" proxy settings?
There's code in Camino to do this, but problems remain (mostly how to map
wildcards in the proxy bypass list).
Bruno: This isn't a relgious war, this is a design problem. Mozilla inherits its
design from Communicator, which is cross platform. As with most x-plat
appliactions, the common functionality was developed first, and the platform
specific adaptions came second (as it should be). Each platform adoption takes
time, and is relatively low cost-beneficial vs. fixing problems that affect all
platforms.

Also, rushing this process is not a good idea. If Simon says it doesn't work
great in Camino, it can't possibly work much better in Mozilla, because they are
Mac-focused. In fact, some comments above say that IE doesn't do this well either.

When people dump OS-specific features into mozilla, they often don't do it
correctly, and end up creating more problems than they solve. Look at the bugs
related to network.autodial-helper.enabled if you want an example.
(In reply to comment #34)
> Bruno: This isn't a relgious war, this is a design problem. Mozilla inherits its
> design from Communicator, which is cross platform. As with most x-plat
> appliactions, the common functionality was developed first, and the platform
> specific adaptions came second (as it should be). Each platform adoption takes
> time, and is relatively low cost-beneficial vs. fixing problems that affect all
> platforms.

I've looked through the code, and have found a good place (the right place?) to fix this - it's a 
collaboration between the "netwerk" library implementation of finding the right proxy for a given URL 
and the preferences manager in, for example, browser/components/prefwindow. 

The solution I'm coding up right now changes two parts of the mozilla code, the netwerk subsystem 
and the preferences panel in the browser subsystem. The netwerk change effects how the netwerk 
code, specifically nsProtocolProxyService, is notified of changes in proxy settings - a default 
implementation that uses the existing mechanism (coupled to the browser preference), and a MacOS 
specific notifier that uses callbacks from the SystemPreferences subsystem (and is NOT coupled to the 
browser preferences). The other side of the netwerk code, that is the part that determines the proxy for 
a given URL, is unchanged - the changes impact only how the proxy configurations themselves are 
updated. Finally, this fix includes changes in the preferences panels to remove any means of locally 
configuring proxies (local to Firefox, that is) - what I can't seem to figure out is how to launch the 
SystemPreferences application at the right page for proxy settings (for example the way Safari does), 
but that doesn't bother me nearly as much as having local settings that are different than those of the 
system.

One brief note about this solution - it would seem that this impacts ALL of the users of the netwerk 
library, including not only Firefox but Thunderbird as well. Specifically for Thunderbird, the netwerk 
change is sufficient to implement the proxy management, but a similar change in Thunderbird 
preferences is probably the right thing to do to avoid user confusion. What I'm trying to say is that if 
the preferences panels of Thunderbird aren't changed, the only bad thing that happens is that users 
may be confused. I'm more than willing to change that panel as well - it's a simple change and similar 
to the change for Firefox.

The "design problem" to which you refer is essentially that no platform other than MacOS (that's a 
strong statement, so if I'm wrong tell me) has a centralized location for proxy settings. Given this, the 
common demoninator solution seems correct - specify local proxy settings in the application. Further, I 
can't see how to make this change WITHOUT impacting the UI, in particular the preferences panel - the 
design problem is coupled to its implementation in the preferences panel, and hence the solution is 
similarly coupled to this same panel.
 
> Also, rushing this process is not a good idea. If Simon says it doesn't work
> great in Camino, it can't possibly work much better in Mozilla, because they are
> Mac-focused. In fact, some comments above say that IE doesn't do this well either.

Re: rushing - we wouldn't want to rush something, but this bug has been sitting around for almost 3 
years :(

> When people dump OS-specific features into mozilla, they often don't do it
> correctly, and end up creating more problems than they solve. Look at the bugs
> related to network.autodial-helper.enabled if you want an example.

Before I comment on your assumption that I'm an idiot, I'd like to give you something concrete to 
complain about. I should have something working by the end of next week, perhaps earlier if my real 
work cooperates, that we can all poke on to see if it's in the right direction or not.

And to be totally honest, I'm not particularly clear on what I should be doing to share this solution with 
you - getting the code was simple, but posting my changes I'm more than a little confused about what 
procedure I should follow, who to notify, etc. Feel free to email me directly with recommendations.

(In reply to comment #35)
> Finally, this fix includes changes in the preferences panels to remove any 
means of locally 
> configuring proxies (local to Firefox, that is)

Not that it is as important as the changes you have made, but consider adding 
hidden prefs or (preferably to me) an 'advanced' button, to make local 
settings - if you have time, etc etc, grovel grovel.

Thanks.

Max.
while simon says there are a few problems, for the most part it works great.
most people never notice.
Those issues in camino that I alluded to above have been partiallly fixed (bug
48718) but at least one issue remains (bug 185649).

FYI, the camino code is here:
http://lxr.mozilla.org/mozilla1.7/source/camino/src/preferences/PreferenceManager.mm#424
Ron: (as a side note, you can see via bug 139393, that there are linux-UI and
UNIX level prefs for proxy.).

I'm pretty sure you were not offended in advance by my comment, I was
(admittedly) being defensive towards Bruno's comments, which I thought lacked a
balanced perspective on proxy and mozilla (I see here this is the only bug he
has ever been in). People who come into bugs and make comments about how old a
bug is, w/o talking about design (much less trying to help w/ code), create a
lot of negative spam.

Comments like the one you made are exactly what keeps mozilla working,
especially since you have some understanding of our proxy code and Mac OS X.

So, back to the original point, my next question for you (and Simon) is:

assuming the backend prototype code works, should it be added to Caminio
(branch) or Mozilla (trunk) first? Using a hidden pref, set off by default,
would probably be the right approach, then the code could be verified,
documented, and pushed out to people that really want it. Sometime afterwards,
the UI problem could be fixed. Since necko uses a listener for proxy pref
changes, we don't need to muck w/ the necko end much, but we also should talk to
Darin once a patch is available.
Mac users who run VPN software to login to a corporate network that requires a
proxy to the Internet have two different behavior patterns: 1. Change the
location to one with proxy settings before running the VPN and use a browser
that supports this feature already (Safari, Camino).  2. Do not change the
location, instead use two different browsers: one that has the proxies set when
on he VPN (typically Mozilla/Firefox), and one that uses the OS settings when
not on the VPN.

Users in the latter community will insist on preserving the ability for
Mozilla/Firefox to have separate proxy settings from MacOS.  So consider this
advance warning that that community exists and will generate flames unless there
is an advanced option to preserve the current behavior.  As a member of the
former community, I very much support this change and do not want to see it
backed out because of flames from the latter community.
*** Bug 278367 has been marked as a duplicate of this bug. ***
Attached image Screenshot (obsolete) — 

    
    

    
  
(In reply to comment #40)
> Users in the latter community will insist on preserving the ability for
> Mozilla/Firefox to have separate proxy settings from MacOS.  So consider this
> advance warning that that community exists and will generate flames unless there
> is an advanced option to preserve the current behavior.  As a member of the
> former community, I very much support this change and do not want to see it
> backed out because of flames from the latter community.

Let me update everyone on the state of what I'm implementing. After reading some of the other 
comments in this bug (e.g., 28 and 39), I've decided to add a radio button on the preferences page that 
says "Use System Preferences settings" (see atttached screenshot). I invite comments on the wording of 
the item (it's very Mac-centric, I feel), but the intent is to allow the proxy settings to be acquired from 
the OS-specific appropriate place. I do feel this is different than the auto-detect option, as that loads a 
URL and is more like auto-config than using system preferences.

So, here's the summary of what I've done:
a) Added new preference option to use the system preferences, allowing manual config, etc., to remain.
b) Restructured some of the preference-handling code in necko to provide a "configuration" setting and 
an "in use" setting. The configuration setting indicates where the settings come from, the "in use" 
setting indicates how the settings behave. This is necessary for MacOS because you can configure an 
auto-config URL, for example, and change it in the system preferences. I want this implementation to 
pick up those changes as well, but the operation of an auto-config URL is independent of its source of 
configuration.
c) Added a listener (now moved to modules/libpref) for system configuration changes. This makes 
sense for MacOS where the changes can be rather active, but less so for UNIX implementations where 
the "system preferences" are environment variables. Nonetheless, the implementation is rational for 
both cases - when the SystemConfigListener is first contacted, it will reply with the current state and 
subsequently monitor for state changes. This interface is the Observer interface already familiar in 
mozilla implementations. These SystemConfigListeners are different per OS, but their interface and 
behavior is intended to be the same.

The "good news" right now is that I can get this all to build and run. The bad news is that it's not 
working completely yet. It should be working some time this weekend.

NOTE: I can't test autoconfig URLs because I'm not at all familar with how they work and I don't know of 
a URL (or file content) to test against. If someone can post an autoconfig URL that I can try, I'll gladly 
make sure that still works. I can host the file but I don't know what it should look like. I don't know 
about auto-detect either, but that's more of a regression test - you can't configure an "autodetect" 
option in MacOS.

    
    

    
  
(In reply to comment #43)

> NOTE: I can't test autoconfig URLs because I'm not at all familar with how
they work and I don't know of 
> a URL (or file content) to test against. If someone can post an autoconfig URL
that I can try, I'll gladly 
> make sure that still works. I can host the file but I don't know what it
should look like. I don't know 
> about auto-detect either, but that's more of a regression test - you can't
configure an "autodetect" 
> option in MacOS.

Autoconfig scripts aren't too complicated.


Simply adding something like below into a file such as proxy.pac shared by a
http server is all that's needed.

function FindProxyForURL(url, host)
{
        if (isPlainHostName(host) ||
            dnsDomainIs(host, ".example.com") ||
            dnsDomainIs(host, "localhost") ||
            dnsDomainIs(host, "127.0.0.1") ||
            isInNet(host, "192.168.0.0", "255.255.255.0"))
            return "DIRECT";
return "PROXY proxy.example.com:12345; DIRECT";
}

Adjusting appropriately...


    
    

    
  
Ron:
I recommend making the UI use a checkbox, not a radio button.

The preference should be something like:
network.proxy.usesystemsettings: true | false

If the checkbox is checked, then the normal UI would be disabled.

As for PAC, do you have a proxy server? I have some old guidelines I can send
you for testing.

    
    

    
  
Ok, I'm finished and I need some other suckers\h\h\h\h\h\h\htesters to try it out. I'm not sure exactly 
*HOW* to do this, so right now I'm just asking you to email me and I'll send you a ~2MB compressed 
.dmg file.

To use the new capabilities, just open preferences, go to Proxies, and set "Use System Preferences" It 
should just work. If it doesn't, or it does something *weird*, let me know.

Open Questions:
1) I had to create a new thread to listen for the system updates. I did this using the various 
pthread_xxx() functions. Is this the right way to do it in mozilla-ville? If not, is there an example 
someone can point to that does it the right way?

2) There was mention before of "exceptions" not working correctly. The only thing I can see here that 
might not work well is the particular specification of sites through the MacOS proxy settings. There was 
one formatting change that I had to do, to change the list of sites from an array of strings into a single 
comma-separated list (which was trivial). Any other SPECIFIC comments on this? It does seem to work, 
but my admittedly simple list of exceptions is not necessarily typical or representative...

> I recommend making the UI use a checkbox, not a radio button.

3) Oh yeah, radio button vs. check box. It's a radio button, and here's my argument as to why: The 
proxy settings are mutually-exclusive - you have manually configured them, you use a direct 
connection, you get them from an Autoconfig URL, you get them by autodetecting them. I've added a 
new choice, system preferences, which is similar to all the existing ones and hence should be a peer. 
Since it's a peer, it's simply a new value (5) for the existing preference network.proxy.type. One of the 
things you should note about this is that if you set the radio button to manual configuration and enter 
stuff there, when you set it back to system preferences it will NOT wipe out those settings. [... and just 
so we're all clear on this, I will change it to a checkbox and use the new recommended setting, but I 
don't really like it - the radio button thing seems cleaner to me, that's all...]

To Do's:
a) Re-integrate into code base - HOW DO I DO THIS? I'm looking for some hints on the next steps of 
this part of the process. I've updated my copy of the code using "cvs update" and it builds again. I need 
to send somebody something, but it's not clear to me what I'm sending where. Email suggestions 
please.

b) Make similar changes to all the other application types. This change covers "application=browser" 
only. [The reason it only covers application=browser isn't that the changes in necko are 
browser-specific, rather it's because the particular preference page only exists in the browser directory. 
The change it to put this radio-button on all the other application pages. Right now, if you build these 
changes into thunderbird, you'll have to manually set your saved "network.proxy.type" to 5.]

    
  
Assignee: general → joshmoz

    
    

    
  
Ron, could you attach a patch?  I'm interested in testing this, but I'd rather
build it out myself in lieu of being sent a binary.

    
    

    
  
Hi Ron and others, has there been any progress on this feature? Are testers
still wanted? I tried to e-mail Ron but didn't hear anything back. Posts on
mozillazine.org suggest that there is some interest in getting this working and
deployed - what needs to be done to move it along?

    
    

    
  
It worked flawlessly for me for several months, before I upgraded to Tiger where
it wasn't so happy.
I still have the .dmg he pointed me to, but that was off-list so I'll send you
the link direct rather than posting it here if you want it. It hasn't been
updated past Firefox 1.0, either.
My last conversation with him basically concluded that he had no idea how to get
the code properly inserted into mainline, something I couldn't him help with.
I'm still very keen to see it progressed though, as I miss it twice a day when I
have to manually switch.

    
    

    
  
What a shame to have someone with a contribution ready to go and then get
discouraged by the difficulty of having it accepted. I'd like to have a look at
the build you have - could you e-mail me a link privately?

    
    

    
  
(In reply to comment #50)
> What a shame to have someone with a contribution ready to go and then get
> discouraged by the difficulty of having it accepted. I'd like to have a look at
> the build you have - could you e-mail me a link privately?

Here's the story: I upgraded my Powerbook to Tiger, and now I can't build mozilla, my changes 
notwithstanding - I can't get the basic downloaded source to build (or at least not the last time).

I will post an update when I know something further.

Ron

    
    

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

    
    

    
  
(In reply to comment #51)
> (In reply to comment #50)
> > What a shame to have someone with a contribution ready to go and then get
> > discouraged by the difficulty of having it accepted. I'd like to have a look at
> > the build you have - could you e-mail me a link privately?
> 
> Here's the story: I upgraded my Powerbook to Tiger, and now I can't build mozilla, my changes 
> notwithstanding - I can't get the basic downloaded source to build (or at least not the last time).
> 
> I will post an update when I know something further.
> 
> Ron

Well, good news, I think. I'm actually able to build 1.5RC1, so I'm working on porting this fix over to that version. It should be done in a couple of weeks or less, depending on my real job. I'm still **** that 10.4 broke everything, but I'll get over it. I'll post new screen shots when I get that part going.

And yes, it's still a radio button...

Ron

    
    

    
  


  

        Attachment #171263 -
        Attachment is obsolete: true

    
    

    
  
(In reply to comment #53)
> And yes, it's still a radio button...

here's the screenshot of the new dialog. as always, comments are welcome.

Ron


    
    

    
  
Very nice! Is that already working? I still don't get what is "Auto-detect proxy settings for this network".
I can't want for definitively switch to firefox.
Thanks

(In reply to comment #55)
> (In reply to comment #53)
> > And yes, it's still a radio button...
> 
> here's the screenshot of the new dialog. as always, comments are welcome.
> 
> Ron
> 



    
    

    
  
(In reply to comment #56)
> Very nice! Is that already working? I still don't get what is "Auto-detect
> proxy settings for this network".

Well, the preference item is working :)

Of course, that's just a little bit of xul and js, not the hard part. That part I'm porting from the old version to the new version. I can get it to build but it doesn't work quite yet. Once it works I'll post a link to a DMG file to let people poke on it a bit to make sure I didn't break something else.

Presuming that I can actually make it work, I'd like some guidance on the next step - how do I get this change into the distribution? I'll have added a handful of files, and made changes to another handful.

    
    

    
  
(In reply to comment #57)
> (In reply to comment #56)
> > Very nice! Is that already working? I still don't get what is "Auto-detect
> > proxy settings for this network".

It *seems* to work, so here's a mountable file to give it a spin. Let me know if there are problems. Given that enough of the beta testers are happy, I'll turn it into a patch.

Ron

    
    

    
  
(In reply to comment #58)
> It *seems* to work, so here's a mountable file to give it a spin. Let me know
> if there are problems. Given that enough of the beta testers are happy, I'll
> turn it into a patch.

Ok, I lied - the file is too big to attach here, so I'll post it at http://www.roncrocker.com/mozilla/ later today - once I get out from behind my corporate firewall; check back after 7PM CST (about 4.5 hours from now)

It'll be called something like "DP-1.5RC1.1.dmg" as in DeerPark version 1.5 Release Candidate 1, with "dot 1" to indicate this fix is added to it.

It's quite possible that something hideous will occur when you run this - I built it on my Powerbook running 10.4.3, but I built it against the 10.3.9 SDK. If something weird does happen, let me know and I'll try to fix it.


    
    

    
  
(In reply to comment #59)
> (In reply to comment #58)
> > It *seems* to work, so here's a mountable file to give it a spin. Let me know
> > if there are problems. Given that enough of the beta testers are happy, I'll
> > turn it into a patch.
> 
> Ok, I lied - the file is too big to attach here, so I'll post it at
> http://www.roncrocker.com/mozilla/ later today - once I get out from behind my
> corporate firewall; check back after 7PM CST (about 4.5 hours from now)
> 
> It'll be called something like "DP-1.5RC1.1.dmg" as in DeerPark version 1.5
> Release Candidate 1, with "dot 1" to indicate this fix is added to it.
> 
> It's quite possible that something hideous will occur when you run this - I
> built it on my Powerbook running 10.4.3, but I built it against the 10.3.9 SDK.
> If something weird does happen, let me know and I'll try to fix it.
> 

Sorry but it doesn't work for me. The application crashes when I try to launch it. I'm running mac os x 10.4.3 on a powerbook G4.

    
    

    
  
How are you implementing the prefs value for the radio button?

    
    

    
  
(In reply to comment #61)
> How are you implementing the prefs value for the radio button?

Right now it's simply the next value - 5 - for the existing network.proxy.type preference item.

As I responded last time, I think of this method as a peer to the other existing options; given that, the radio button model (choose exactly one) is appropriate. 

I have posted an installable version, and I still have the same problem I had before - what's the next step?


    
    

    
  
(In reply to comment #62)

> I have posted an installable version, and I still have the same problem I had
> before - what's the next step?

Post a patch. Do a cvs diff -u8 on your tree (or just the dirs you touched), and attach it to this bug.

    
    

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

    
    

    
  
(In reply to comment #63)
> (In reply to comment #62)
> 
> > I have posted an installable version, and I still have the same problem I had
> > before - what's the next step?
> 
> Post a patch. Do a cvs diff -u8 on your tree (or just the dirs you touched),
> and attach it to this bug.

I hate to be such a newb, but if the shoe fits...

I did this cvs diff -u8 and I /can/ post it, but I think it's broken - I've added some files and their contents don't show up in the listing. Is there a good way to include them?

    
    

    
  
TO get new files in the patch, cvs add them, then cvs diff -u8 -N
(cvs add is a local operation, so this don't affect the repository).
Or just make a .zip of the new files and attach them along with the patch.

    
    

    
  


  
I'm attaching 2 files: the patch itself and a tar file containing the new files. I tried to "cvs add" them but cvs complained:

$ cvs -t add nsSystemConfigListener.cpp 
 -> main loop with CVSROOT=:pserver:anonymous@cvs-mirror.mozilla.org:/cvsroot
 -> Connecting to cvs-mirror.mozilla.org(207.126.111.214):2401
cvs [server aborted]: "add" requires write access to the repository
 -> Lock_Cleanup()

    
    

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

    
    

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

    
    

    
  
(In reply to comment #68)
> Created an attachment (id=204540) [edit]
> The new files associated with patch1

It's gotten pretty quiet - has anyone had the nerve to give this patch a spin?

I'm still not quite sure what I should do next? Should I alert someone that is a committer for this code that they should review this patch? Your help is appreciated.

Ron

    
    

    
  
Comment on attachment 204539 [details] [diff] [review]
Patch file - needs "extra files" also

For starters, we need to flag your patches for review.

        Attachment #204539 -
        Flags: review?

    
  

        Attachment #204540 -
        Flags: review?

    
    

    
  
Ok, so after skimming the patch, it looks like the new files are in libpref (which I'm not sure who owns these days).  Then there's code in necko to hook that code up, I guess.

Not sure about the best reviewers here.  But CC darin/biesi for necko changes. Perhaps smfr or pink can have a look at the libpref changes since they seem to be mac-specific?

    
    

    
  
OK, so...

what about the stuff in extensions/system-pref? and then there's the patch in bug 66057...

    
    

    
  
I would go with the approach in bug 66057. Take that patch, and add a Mac implementation of nsISystemProxySettings.idl. Then set the default proxy preference to "autodiscover" to get the system proxy settings.

    
    

    
  
(In reply to comment #75)
> I would go with the approach in bug 66057. 

There's two things I don't like about the approach in bug 66057 (though I haven't looked at the code, just the discussion):

a) it hijacks (their word - see the text in the bug) a setting that already has a well defined meaning - autodetect means use the autodetect URL, not poke around looking for interesting system variables, ...

b) It only happens once, at application startup. The difference between an environment variable and what happens on the Mac is that the proxy settings can (and will) change while the application is running. Changing the location can change the proxy settings, and this is something I will do several times during the day. Last time I checked it was pretty difficult to change environment variables of a running process in Unix systems.

There are some arguments above (you can see the dialog) about checkbox vs. radio button, and I think my arguments for a new radio button are cogent.

So this implementation does two things (and which is why it's in two parts of the system) - it changes the user interface to add the radio button, and it implements a thread to listen for changes to the proxy settings. 

We can argue about the first part (although we already have argued it here), but the latter part sure seems a necessary component of the implementation - you can't get the proxy settings used inside mozilla to change upon a location change without listening for the location change, and the system doesn't tell you about the change unless you register a listener for the change.

    
    

    
  
(In reply to comment #76)
> There's two things I don't like about the approach in bug 66057 (though I
> haven't looked at the code, just the discussion):
> 
> a) it hijacks (their word - see the text in the bug)

Which text/bug?

> a setting that already
> has a well defined meaning - autodetect means use the autodetect URL, not
> poke around looking for interesting system variables, ...

I think using system settings is well within the spirit of "autodetect". I very much doubt users who have this set to get WPAD would be affected, since presumably the system itself can be or already is configured to use WPAD too.

> b) It only happens once, at application startup.

This isn't so. First of all, the environment variable stuff is a platform specific implementation for Unix systems which use those variables. For Mac, a completely different implementation of the interface is appropriate. Dynamic changes work perfectly well; the patch in bug 66057 includes an implementation that updates settings dynamically from GConf on GNOME systems. Dynamic changes work because when a nsISystemProxySettings component is available, we call it for every single load ... it works just like a PAC script. You might need a background thread on Mac to catch changes (hopefully not, because that would consume significant resources) but that's an implementation detail.

> There are some arguments above (you can see the dialog) about checkbox vs.
> radio button, and I think my arguments for a new radio button are cogent.

This one?
> I do feel this is different than the auto-detect option, as that loads a
> URL and is more like auto-config than using system preferences.

Personally I feel that four radio buttons is enough for one preference :-).

> We can argue about the first part (although we already have argued it here),

I think what's been argued here is that there should be a way to manually override the Mac system proxy settings. I agree with that.

> but the latter part sure seems a necessary component of the implementation -
> you can't get the proxy settings used inside mozilla to change upon a 
> location change without listening for the location change, and the system
> doesn't tell you about the change unless you register a listener for the
> change.

Yeah. I think you could integrate the non-UI parts of your patch with the interfaces in the patch in bug 66057. Then we have a common system proxy settings solution for every platform except Windows :-).

    
    

    
  
(In reply to comment #77)
> (In reply to comment #76)
> > a) it hijacks (their word - see the text in the bug)
> 
> Which text/bug?

bug 66057, comment #32 From Robert O'Callahan (Novell) 2006-01-11 13:23 PST, says
% Basically it hijacks the "Autodectect proxy settings for this network"
% preference

> I think using system settings is well within the spirit of "autodetect". I very
> much doubt users who have this set to get WPAD would be affected, since
> presumably the system itself can be or already is configured to use WPAD too.

Until today, autodetect had a well-defined meaning - use the autodetect URL to load proxy settings. Now suddenly it has new meanings and a spirit. Autodetect is a network setting - the network operator can define what that means; system preferences is a [local] system setting. I'm not getting the spirit thing.

> > b) It only happens once, at application startup.
> .... Dynamic
> changes work perfectly well; the patch in bug 66057 includes an implementation
> that updates settings dynamically from GConf on GNOME systems. Dynamic changes
> work because when a nsISystemProxySettings component is available, we call it
> for every single load ... it works just like a PAC script. 

Ok, I stand corrected, but...

Yuck - everytime I load a page I have to call out to something to find out that it hasn't changed to catch that one time that it will change? I guess that's due to not having a method of telling apps when system-level settings change.

> You might need a
> background thread on Mac to catch changes (hopefully not, because that would
> consume significant resources).

In what universe is the gazillion times you read in the not-changed-proxy-configuration more efficient and consuming less resources than an operating system thread that does nothing until it's woken up by the system callback? (Yes, doing nothing but waiting, using a thread's worth of stack and 0 cpu time. I just checked that using Thread Viewer (a MacOS developer tool that tells you what various threads are doing) and the system config listener thread is "waiting in run loop" - blocked waiting on some event)

> > There are some arguments above (you can see the dialog) about checkbox vs.
> > radio button, and I think my arguments for a new radio button are cogent.
> 
> This one?

Uh, no, how about comment #46 and others.

> > I do feel this is different than the auto-detect option, as that loads a
> > URL and is more like auto-config than using system preferences.
> 
> Personally I feel that four radio buttons is enough for one preference :-).

Then get rid of autodetect and inform users to use the WPAD URL as the autoconfig url.

> > We can argue about the first part (although we already have argued it here),
> I think what's been argued here is that there should be a way to manually
> override the Mac system proxy settings. I agree with that.

What I was trying to argue is that the approach I built is better than the approach in bug 66057.

> Yeah. I think you could integrate the non-UI parts of your patch with the
> interfaces in the patch in bug 66057. Then we have a common system proxy
> settings solution for every platform except Windows :-).

... or you can integrate your stuff into my approach, which was here before yours.

This is my first try at contributing to an open-source software project, and it may be my last; I've been pretty disappointed by the experience. I've had a working solution to this problem for over a year (see comment #46 and comment #49). I've been begging for some help on getting it into the load (see, for example, comment #46 and comment #50); "post a patch" was nonsense to me until someone told me HOW to post a patch. The baseline load changed out from under me in that time and I ported the solution to that version too (see comment #58). Finally someone told me what to do and I posted a patch. That was 3+ months ago. Until I mentioned it yesterday, who knows how long it would have been sitting...

I'm sorry I needed a little hand holding, and that this little thread has turned into a **** contest - maybe there's some social order here that I don't really get or I didn't follow some unwritten rule. None of that really matters; all I really wanted to do was fix some problem that I had with using mozilla that kept me from wanting to use it full time on MacOS X. Reading some of the comments above (e.g., comment #9 (Max), comment #31 (Bruno), and others) I'm not alone in that feeling.

    
    

    
  
(In reply to comment #78)
> bug 66057, comment #32 From Robert O'Callahan (Novell) 2006-01-11 13:23 PST,
> says
> % Basically it hijacks the "Autodectect proxy settings for this network"
> % preference

Sorry, you confused me by referring to me in the third person :-).

> Until today, autodetect had a well-defined meaning - use the autodetect URL
> to load proxy settings. Now suddenly it has new meanings and a spirit.
> Autodetect is a network setting - the network operator can define what that
> means; system preferences is a [local] system setting. I'm not getting the
> spirit thing.

Alright, I'll adjust my patch to use a new proxy preference and allow for the UI to expose a fifth radio button. Whether the Firefox UI people actually agree that they want to show a fifth radio button is up to them, not us.

> Yuck - everytime I load a page I have to call out to something to find out
> that it hasn't changed to catch that one time that it will change? I guess
> that's due to not having a method of telling apps when system-level settings
> change.

It's not a lot of work. It's no more work than currently happens on every load when a PAC URL is configured ... considerably less actually because we don't have to go through Javascript to get the PAC string.

> > You might need a
> > background thread on Mac to catch changes (hopefully not, because that
> > would consume significant resources).
> 
> In what universe is the gazillion times you read in the
> not-changed-proxy-configuration more efficient and consuming less resources
> than an operating system thread that does nothing until it's woken up by the
> system callback?

A native thread typically consumes a significant amount of virtual memory reservation for its stack, and some real memory. It consumes kernel resources too.

> Uh, no, how about comment #46 and others.

There's an argument there about radio vs checkbox but that doesn't seem relevant anymore. Anyway, I can live with a fifth checkbox.

> Then get rid of autodetect and inform users to use the WPAD URL as the
> autoconfig url.

That would break things for a lot more users.

> > Yeah. I think you could integrate the non-UI parts of your patch with the
> > interfaces in the patch in bug 66057. Then we have a common system proxy
> > settings solution for every platform except Windows :-).
> 
> ... or you can integrate your stuff into my approach, which was here before
> yours.

Your patch adds a lot of Mac-specific code into nsProtocolProxyService, protected by #ifdefs. I don't think this is the way to go; it would get a lot worse if we tried to add my Linux code too. My patch keeps all platform-specific code separated, behind an interface designed specifically for accessing system proxy settings, and makes minimal changes to core netwerk code.

Another important difference is that my nsISystemProxySettings interface allows the system proxy configuration to be more complicated than can be expressed using Mozilla's proxy preferences, because it lets the system decide how to treat each individual URL. For example, connections on different ports can get different proxies. This is useful for ensuring that the system proxy policy is followed regardless of how Mozilla's proxy preferences behave.

I'm sorry you're not having a good experience, and all I can say is that I didn't know about this bug until now. Believe me when I say that I want to get this fixed for me and for you.

    
    

    
  
(In reply to comment #79)
> Your patch adds a lot of Mac-specific code into nsProtocolProxyService,
> protected by #ifdefs. I don't think this is the way to go; it would get a lot
> worse if we tried to add my Linux code too. My patch keeps all
> platform-specific code separated, behind an interface designed specifically for
> accessing system proxy settings, and makes minimal changes to core netwerk
> code.

Sigh. Well I definitely agree about that. I thought I got all of that Mac-specific stuff out and into the one place that adapted the MacOS information (like the list of exceptions) to what this code wanted. I probably just rushed the refactoring; I'll take another look at the code in nsProtocolProxyService to make sure it really belongs there (or more likely not).

> Another important difference is that my nsISystemProxySettings interface allows
> the system proxy configuration to be more complicated than can be expressed
> using Mozilla's proxy preferences, because it lets the system decide how to
> treat each individual URL. For example, connections on different ports can get
> different proxies. This is useful for ensuring that the system proxy policy is
> followed regardless of how Mozilla's proxy preferences behave.

Ok, I'll take a deeper look at your code to see how we can merge this to get to a happier place. 

<noob>Of course, one question I have is how I should do that exactly? Should I download your patch and apply it to my version of the code? And how exactly would I "apply it"?</noob>

> I'm sorry you're not having a good experience, and all I can say is that I
> didn't know about this bug until now. Believe me when I say that I want to get
> this fixed for me and for you.

That's more of the attitude I would have expected - we'll get this resolved...

    
    

    
  
(In reply to comment #80) 
> <noob>Of course, one question I have is how I should do that exactly? Should I
> download your patch and apply it to my version of the code? And how exactly
> would I "apply it"?</noob>

Download my patch and apply it to a clean tree. You do this with the "patch" tool. cd to your mozilla/ directory, then run "patch -p0 < patchfile". You should then be able to build Mozilla, although you won't get any behavior change because my patch only adds a Linux system proxy component. (I guess you could build Mac-GTK2 in theory but that would be nuts.)

The next step is to clone my Linux unixproxy component, call it macproxy or something. This just means copying directories and files, changing my UUIDs to fresh uuids (use uuidgen), changing unixproxy to macproxy in the right places, and building your component by adding something to mozilla/Makefile.in like I did for unixproxy. After this you should be able to build and set proxies using environment variables (for now choosing "Autodetect" in the browser prefs), but of course that's not what you really want, so...

Modify nsMacSystemProxySettings to use your code. In ::Init you'll want to start your thread etc. The core two functions you need to implement are GetPACURI and GetProxyForURI. The former can return the URL of a PAC file to load and use; return something non-empty from here if a PAC URL is configured in the system. The latter returns a PAC-format proxy string given a URI. If the Mac system offers a convenient function to get the proxy for a URI, you could just use that, otherwise you need to work it out yourself using the URI and the Mac system settings. Don't parse the URI by hand, use our URI methods GetHost, GetPort, SchemeIs etc, otherwise it's very easy to screw up. Feel free to copy and paste my code; if you do, we'll find some way to share it.

Keep in mind that my patch hasn't landed yet and will probably need some changes before it goes into the tree. Certainly I need to change the way it interacts with preferences as I just promised to do :-). You may run into problems that require additional changes so I'm sure you'll let me know :-). But having two platforms sharing a common interface and infrastructure will certainly help prove the interface and get it landed.

    
    

    
  
maybe all the system proxy components should live in a single directory, with nsGConfSystemProxy.cpp nsMacSystemProxy.cpp etc?

    
    

    
  
(In reply to comment #82)
> maybe all the system proxy components should live in a single directory, with
> nsGConfSystemProxy.cpp nsMacSystemProxy.cpp etc?
> 

I can put that on the list of fixes to make, but first I'm going to make it work :)

After some networking issues Friday, I've downloaded a fresh version 1.5.0.1, built that successfully, applied the patch from bug 66057 (as it is - while I do (and will continue to) argue strongly for the distinct preference item, it's not interesting enough to stop progress on integrating the meaty part of these implementations) and am building that now. 

One note - the bug 66057 patch "failed' in a strict sense as it didn't work on two of the files: Makefile.in and netwerk/base/public/Makefile.in - both of these were one-line additions that I easily integrated by hand.

Once this builds, I'll integrate the MacOS-specific parts into that structure and make that work. I won't really be able to tell if I broke the linux stuff or not, but I don't plan on a lot of restructuring right now. If there are any significant changes to the interface, I'll work that out offline with  
if I broke the linux stuff, but we'll get there soon enough.

    
    

    
  
I've updated my patch in bug 66057 to use a new preference value. I took your UI changes directly. Hopefully this suits you.
Depends on: 66057

    
  

        Attachment #204539 -
        Flags: review? → review?(joshmoz)

    
  

        Attachment #204540 -
        Flags: review? → review?(joshmoz)

    
    

    
  
I've requested review from myself so I can look at this in the future. If there should be some other reviewer (roc? biesi?) feel free to reassing. Reviews should target a particular person, not just left untargeted.

    
    

    
  
This patch isn't ready yet, it needs to be updated to use the interfaces from bug 66057. I guess we're waiting for Ron to do that, although you could do it yourself if you feel like it.

    
  

        Attachment #204539 -
        Flags: review?(joshmoz)

    
  

        Attachment #204540 -
        Flags: review?(joshmoz)

    
    

    
  
This is exactly what I had hoped for. Pardon my ignorance of the whole process, but when will this patch be available in a release?

    
    

    
  
(In reply to comment #87)
> This is exactly what I had hoped for. Pardon my ignorance of the whole process,
> but when will this patch be available in a release?
> 

First it's waiting on me to get some time to do this right. What I have works; what Robert has works. What I'm trying to find time to do is glue them together. I understand what Robert's code is trying to do, I understand what my code is trying to do - now i'm trying to get time to do the work to get them working together. The principal distinction is that the Mac approach can be "passive," waiting for the system to tell it when something changes, while the linux approach is "active," checking the setting on every page load. They're both appropriate for their platforms, it's just a matter of getting an interface that's appropriate for both. 

Right now my thoughts are that enabling this mode of operation does two things - calls a "enabler" method (that turns on the listener thread on the Mac and does nothing for linux) and installing an "interposition function" to do the per-page lookup. For Linux, this does the heavy-lifting; for the Mac this does nothing.

If you're a Mac user and impatient :), there's a DMG with a working version at http://www.roncrocker.com/mozilla/firefox-v2.dmg. Give it a spin and let us know what you think. The trick there is to use the configuration panel to set the "Use System Preferences settings" and then you'll get it automatically picking up location changes.

    
    

    
  
From an email from ron crocker:

If you're a Mac user and impatient :), there's a DMG with a working version at
http://www.roncrocker.com/mozilla/firefox-v2.dmg. Give it a spin and let us
know what you think. The trick there is to use the configuration panel to set
the "Use System Preferences settings" and then you'll get it automatically
picking up location changes.

Tried it. The app crashes on opening.  Tried multiple times... This is using OS X 10.4.6
Let me know if there is additional info that will help.

    
    

    
  
(In reply to comment #89)
> From an email from ron crocker:
> 
> If you're a Mac user and impatient :), there's a DMG with a working version at
> http://www.roncrocker.com/mozilla/firefox-v2.dmg. Give it a spin and let us
> know what you think. The trick there is to use the configuration panel to set
> the "Use System Preferences settings" and then you'll get it automatically
> picking up location changes.
> 
> Tried it. The app crashes on opening.  Tried multiple times... This is using OS
> X 10.4.6
> Let me know if there is additional info that will help.

Not surprising, unfortunately - when I build these they tend to be very sensitive to OS release values. Let me remake one (i'm on 10.4.6 too) and publish that.



    
    

    
  
Try this one:

http://www.afonsoarriaga.net/backup/firefox_osx_proxy.dmg.zip

It works for me...
Credits go to Ron Crocker.

    
    

    
  
(In reply to comment #91)
> Try this one:
> 
> http://www.afonsoarriaga.net/backup/firefox_osx_proxy.dmg.zip
> 
> It works for me...
> Credits go to Ron Crocker.
> 


It complains about not finding Firefox Modern in a suitable version. No idea what the fallout will be from that...

but it seems to work. And it picks up changes in the Internet Connection setup without missing a beat!

thanks


    
    

    
  
(In reply to comment #90)
> Not surprising, unfortunately - when I build these they tend to be very
> sensitive to OS release values. Let me remake one (i'm on 10.4.6 too) and
> publish that.

well, it turns out that I pointed you to the wrong file. I've deleted it (it's just bad); use the other files in the directory http://www.roncrocker.com/mozilla - the one with the bug number in it is freshly made and works with 10.4.6.

    
    

    
  
(In reply to comment #93)

I have to say that the fix does not work for me.  My network preferences have the proxy settings configured + a list of local proxy bypass addresses.  Safari works flawlessly with this.

If I use the above Firefox build  with the option "Manual proxy configuration" then everything works as expected, however if I change it to "Use system preference settings" I can access the local resources but as soon as I try to get onto the internet nothing happens.  I don't get a username and password prompt for the proxy, and eventaully the page times out.

Hmmm


    
    

    
  
> http://www.roncrocker.com/mozilla - the one with the bug number in it is
> freshly made and works with 10.4.6.
> 

Discovered a strange behavior in this release. I was looking at the following URL:
     http://www.isecureonline.com/Reports/IL/WILVG418/
and when I clicked on the tab for Firefox Help (linked to http://texturizer.net/firefox/)
I end up with a pop-up ad instead:  http://www.isecureonline.com/Reports/IL/WILVG418/popup.cfm
How does this occur?


    
    

    
  
(In reply to comment #93)
> (In reply to comment #90)
> > Not surprising, unfortunately - when I build these they tend to be very
> > sensitive to OS release values. Let me remake one (i'm on 10.4.6 too) and
> > publish that.
> 
> well, it turns out that I pointed you to the wrong file. I've deleted it (it's
> just bad); use the other files in the directory
> http://www.roncrocker.com/mozilla - the one with the bug number in it is
> freshly made and works with 10.4.6.
> 

Hi Ron,

Thanks for taking the time to try and address one of my biggest gripes with Mozilla/Firefox.  I downloaded firefox-1.5.en-US-bug125995.dmg last night and set its Preference to "Use System Preferences settings" but using this option does not allow me to connect to the Internet using the Proxy server that I have configured (manual HTTP and HTTPS proxy settings for MS Office applications and Automatic Proxy Configuration for PAC-aware applications).  At the moment I am getting the message "Unable to connect".

    
    

    
  


  
This is based on the unix specific version from bug 66057 as the comments about the patches in 125995 seemed suggest that the large number of files patched was a bad thing. There are a few hooks into the current PAC handling code in netwerk/base/src/nsProtocolProxyService and the UI, but most of the logic is located in a new toolkit component called "systemproxy".

I used a fairly recent CVS checkout (within the last day or two (2006 Sept 21)) and tested it on both OS X and Linux.

        Attachment #239872 -
        Flags: review?(joshmoz)

    
    

    
  
(In reply to comment #97)
> Created an attachment (id=239872) [edit]
> implements network preference settings retrieval

I forgot a file in my patch (nsISystemProxySettings.idl). I'll post the full and complete patch tomorrow after another round of checkout a pristine copy, apply the patch, wait for firefox to build, and make sure everything is working.

    
  

        Attachment #239872 -
        Attachment is obsolete: true

        Attachment #239872 -
        Flags: review?(joshmoz)

    
    

    
  


  
This version does apply cleanly to a pristine cvs tree in both OS X and Linux and includes all of the bits necessary for this to work.. 

It's based on the patch in bug 66057. It hooks into the current proxy PAC processing and on OS X retrives a system configuration dictionary for each connection. (Which allows it to respond to changing system configurations). The advantage of this over the previous implementation is that most of the implementation is in a seperate module.

OS X does have a place to store a PAC URL, but that api call is only available in 10.4 and I'm not sure if firefox should require that. (Currently that code path is #ifdef'ed out).

The other problem is that a proxy exception of "hostname:80" doesn't work.

Does -1 mean the default port 80?

        Attachment #239924 -
        Flags: review?(joshmoz)

    
    

    
  
Just reading over the patch quickly, here are two nits, no need to regenerate the patch for these.

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

You're using tab characters here. There shouldn't be any tab characters in .cpp source files.

Please generate patches using something like "diff -U 8", the -U is a unified diff format used by most Mozilla developers, the number 8 specifies that there be 8 lines of context for changes (default is 3, too small to easily read patches).

Looks good so far, I'll finish my review soon.

    
    

    
  
diane - also, looking at how your patches are generated you might want to investigate the cvs diff command - like "cvs diff -U 8". It'll diff your changes against cvs so you don't need a "pristine" tree.

    
    

    
  
Comment on attachment 239924 [details] [diff] [review]
This patch applies cleanly and implements the retrieval

+};
\ No newline at end of file

Files should have a newline at the end.

+		if (mPACMan->IsPACURI(pacURI))
+				return NS_OK;

Again, a lot of tab characters in the patch that shouldn't be there.

+  for (GSList* l = list; l; l = l->next) {

That is a confusing iteration variable name because "l" looks like the number "1" and thus it looks like your loop is an infinite one :)

+  //! do we have an https proxy?

Why the exclamation mark? Looks strange and unnecessary.

+  bool ftp_host(nsCAutoString&) const;

In some cases you're returning a bool that is the value you're asking for, in other cases you're returning a bool to indicate the success of the call. This is a bit confusing. Using "bool" at all is something that should probably be done only when you're dealing with Carbon/CF functions directly, and in some cases you are. In cases where bool reflects the success of the call that could be made more clear by using nsresult - it make the intent of the return value clear to readers.

+++ mozilla/toolkit/components/systemproxy/nsOSXSystemProxySettings.cpp
...
+ * The Initial Developer of the Original Code is
+ * Netscape Communications Corporation.
+ * Portions created by the Initial Developer are Copyright (C) 1998

This is in a file it looks like you wrote - I'm guessing you don't work for Netscape Communications Corporation and you didn't write it in 1998 :) I do that all the time myself. It looks like there is the same problem with some of the stuff roc wrote.

+proxy_MaskIPv6Addr(PRIPv6Addr &addr, PRUint16 mask_len)
+{
+    if (mask_len == 128)

Why all of the sudden is there 4-space indentation? Should be 2 space indentation, like the sutff above it.

Fix that stuff and r+. Thanks!

        Attachment #239924 -
        Flags: review?(joshmoz) → review+

    
    

    
  
 
> +  //! do we have an https proxy?
> 
> Why the exclamation mark? Looks strange and unnecessary.

The sequence "//!" is one of several flags to doxygen to indicate that the comment is documentation. Grepping through the source tree suggests that it's only the javascript that has anything looking like a doxygen/javadoc comment block. I don't suppose there's any interest in starting to use a code documentation tool? ;)

> +  bool ftp_host(nsCAutoString&) const;
> 
> In some cases you're returning a bool that is the value you're asking for, in
> other cases you're returning a bool to indicate the success of the call. This
> is a bit confusing. Using "bool" at all is something that should probably be
> done only when you're dealing with Carbon/CF functions directly, and in some
> cases you are. In cases where bool reflects the success of the call that could
> be made more clear by using nsresult - it make the intent of the return value
> clear to readers.

Ok, I was a bit unclear about the usage difference between nsresult and bool. I'll go ahead and fix it. 

> +++ mozilla/toolkit/components/systemproxy/nsOSXSystemProxySettings.cpp
> ...
> + * The Initial Developer of the Original Code is
> + * Netscape Communications Corporation.
> + * Portions created by the Initial Developer are Copyright (C) 1998
> 
> This is in a file it looks like you wrote - I'm guessing you don't work for
> Netscape Communications Corporation and you didn't write it in 1998 :) I do
> that all the time myself. It looks like there is the same problem with some of
> the stuff roc wrote.

Heh, the dangers of cut-n-pasting. Is there some other recommended header for new code? Or just modify this one by updating the date and killing the inappropriate company references?

> Fix that stuff and r+. Thanks!

I should have time to work on this tonight.

    
    

    
  
AFAIK, the chances of anyone starting to use doxygen on that code are pretty slim. Probably best to just do what we do elsewhere in the mozilla code.

As for the license header, you should find an example that fits your situation. Check out this page, but check it against some examples from our source tree:

http://www.mozilla.org/MPL/boilerplate-1.1/index.html

    
    

    
  
(In reply to comment #101)
> diane - also, looking at how your patches are generated you might want to
> investigate the cvs diff command - like "cvs diff -U 8". It'll diff your
> changes against cvs so you don't need a "pristine" tree.
> 

Yes but the problem with the cvs command is that some of this patch involves creating a new directory which is a bit difficult to deal without cvs commit access. I glanced at cvsdo but there was some reason it didn't seem like it'd work right. (Not that I remember why). 

    
    

    
  


  
I did a bunch of work with Robert O'Callahan in bug 66057 on this patch. So this new patch is based on that code. 

I think the biggest change with the new version is the systemproxy stuff was moved into netwerk/system (which took more build system fiddling to get it to compile correctly). You might want to check how I had to add SystemConfiguration and CoreFramework to the build system.

I merged in some of Roberts updates to the gnome stuff as well. Though I didn't paied much attention to it.

I updated the copyright notices as requested, I assumed that all of Robert's code was written in 2006, since his first patch for bug 66057 was in 2006. Since most of my original code started from his patch I included him as a contributer on the files I added. 

I took your suggestion and changed the bool return value from Roberts suggestion of PRBool to nsresult, except for the *Enabled() functions as it was too convienent to be able to do a simple test to see if the proxy was enabled.

There are fewer tabs (hopefully I got them all), and the spacing is more consistent as well.

        Attachment #239924 -
        Attachment is obsolete: true

        Attachment #242157 -
        Flags: review+

    
    

    
  
(In reply to comment #106)
> Created an attachment (id=242157) [edit]
> Made requested updates to patch from bug 66057

Oh yes, I forgot, I did compile and test this version of the patch. Though I didn't do the more stringent apply to a fresh checkout and build.

    
    

    
  
> 
> Fix that stuff and r+. Thanks!
> 

I forgot one other detail, a slightly earlier version of the patch I just generated is up under bug 66057 for review and super review by cbiesinger

    
    

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

    
    

    
  
What is the status here? What are we waiting for?
Assignee: joshmoz → diane

    
    

    
  
I, for one, am not waiting for anything and have switched to Camino, which I found via this bug report. It's not quite as nice as firefox (yet), but it's good enough for gmail and spares me the switching frustration.

    
    

    
  
(In reply to comment #110)
> What is the status here? What are we waiting for?
> 

I was waiting for review? I might not have set the flags properly to indicate that. I can try to make sure the patch still applies to HEAD in the next day or two if you'd be willing to review it.



    
    

    
  
diane - if you could make sure the patch is up to date and attach a new one if necessary that would be great. I have already given you r+, you should request sr from roc. If you have trouble doing that, just send me an email to my bugzilla address and I'll help you. 'Twould be a shame to let all this hard work sit here!

    
    

    
  
If I understand correctly, the latest and greatest version of this patch is in bug 66057 and waiting for biesi's review. Can we all focus over there? I've already chased biesi once, now it's Josh's turn.

    
    

    
  
Man! A 5-year-old bug :) I subscribed to it when I bought my Powerbook, but I've *long* since sold it ... Good luck with it though.

    
    

    
  
For two years now I am waiting for the bug to become part of the official firefox release! The network environment on mac os x are a very great feature - only firefox does not support it for no reason!

The solution describe above works great in the 1.5 patch build. But this is far too old to use anymore. 

Can anyone PLEASE, PLEASE review the patch and make it available in the official ff branch?

Regards,
Timmo


    
    

    
  


  
The attached patch applies to Mercurial revision 389998e32107. It makes network.proxy.type = 5 (use system settings) work correctly under Mac OSX.

The patch is based on some code from attachment 242611 [details] [diff] [review] in bug #66057. I've also taken into account the review feedback given by Christian Biesinger in #66057 comment 82.

Whenever the settings are updated in SystemConfiguration or the location is changed Firefox will notice the new settings.

Could somebody please review this and let me know how well I did? Thanks :)

Oh, and thanks Robert O'Callahan for pointing me in the right direction. I'm still alive too! :P

    
    

    
  


  
This is needed otherwise most users will never be able to use the system proxies feature.

    
    

    
  


  
Removed some stray printf()s :$

        Attachment #313570 -
        Attachment is obsolete: true

        Attachment #313572 -
        Attachment is patch: true

        Attachment #313572 -
        Attachment mime type: application/octet-stream → text/plain

    
    

    
  
Comment on attachment 313576 [details] [diff] [review]
Mac OSX implementation of system proxy settings - #2

I'll review this

        Attachment #313576 -
        Flags: superreview?(roc)

        Attachment #313576 -
        Flags: review?(roc)

    
    

    
  
Comment on attachment 313576 [details] [diff] [review]
Mac OSX implementation of system proxy settings - #2

>diff -r 5fd2908b2e99 toolkit/library/libxul-config.mk
>--- a/toolkit/library/libxul-config.mk	Fri Apr 04 21:01:36 2008 +1100
>+++ b/toolkit/library/libxul-config.mk	Fri Apr 04 21:42:33 2008 +1100
>@@ -150,9 +150,15 @@ ifdef MOZ_XUL
> ifdef MOZ_XUL
> ifdef MOZ_ENABLE_GTK2
> COMPONENT_LIBS += \
>-        unixproxy \
>-        $(NULL)
>+	unixproxy \
>+	$(NULL)
> endif
>+endif
>+
>+ifeq ($(OS_ARCH),Darwin)
>+COMPONENT_LIBS += \
>+	osxproxy \
>+	$(NULL)
> endif

You probably want to check for Cocoa here instead of Darwin?

Other places do something like this:

ifneq (,$(filter mac cocoa,$(MOZ_WIDGET_TOOLKIT)))


>+///////////// Manage Access to the OS X System Configuration ////////////
>+class SCProxySettings

What's with the SC prefix? I've never seen us use that elsewhere.

>+{
>+public:
>+  SCProxySettings();
>+  ~SCProxySettings();
>+
>+  void proxyHasChanged();
>+
>+  // return true if we were able to retrieve the proxy settings
>+  PRBool valid() const { return (proxyDict != NULL); }
>+
>+  // is there a PAC url specified in the system configuration
>+  inline PRBool IsAutoconfigEnabled() const;
>+  // retrieve the pac url
>+  inline nsresult GetAutoconfigURL(nsCAutoString&) const;
>+
>+  // do we have an FTP proxy?
>+  inline PRBool IsFTPEnabled() const;
>+  // do we have an http proxy?
>+  inline PRBool IsHTTPEnabled() const;
>+  // do we have an https proxy?
>+  inline PRBool IsHTTPSEnabled() const;
>+  // do we have a socks proxy?
>+  inline PRBool IsSOCKSSEnabled() const;

How about bool IsProtocolEnabled(string protocol) ? 

(which exakt |string| datatype to use here depends on whether you prefer CF/Cocoa strings or XPCOM strings in this context, hence the generic type I use in the pseudo-code)

>+
>+  // get the host name for the ftp proxy
>+  inline nsresult GetFTPHost(nsCAutoString&) const;
>+  // get the host name for the http proxy
>+  inline nsresult GetHTTPHost(nsCAutoString&) const;
>+  // get the host name for the https proxy
>+  inline nsresult GetHTTPSHost(nsCAutoString&) const;
>+  // get the host name for the socks proxy
>+  inline nsresult GetSOCKSHost(nsCAutoString&) const;

In the same vein, string GetProtocolHost(string protocol)

>+
>+  // get the port for the ftp proxy
>+  inline nsresult GetFTPPort(PRInt32&) const;
>+  // get the port for the http proxy
>+  inline nsresult GetHTTPPort(PRInt32&) const;
>+  // get the port for the https proxy
>+  inline nsresult GetHTTPSPort(PRInt32&) const;
>+  // get the port for the socks proxy
>+  inline nsresult GetSOCKSPort(PRInt32&) const;

int GetProtocolPort(string protocol)

>+
>+  // get a boolean state for the provided SCSchemaDefinition
>+  nsresult GetBool(CFStringRef proxy_name, PRBool &) const;
>+  // get a string for the provided SCSchemaDefinition
>+  nsresult GetString(CFStringRef proxy_name, nsCAutoString&) const;
>+  // get an integer for the provided SCSchemaDefinition
>+  nsresult GetInt(CFStringRef proxy_name, PRInt32&) const;

About the Mac-specific code in this file. 

1. You're gonna need the new "protect for sudden Objective-C exceptions being thrown from Apple frameworks" protection around every method that calls into a system API. See /xpcom/base/nsObjCExceptions.h and how it's used elsewhere in the tree.

2. I think there would be less, and more readable code, if it was renamed to a ".mm" file, linked to Cocoa, and done using Objective-C instead of C-style Core Foundation. This conversion should be pretty easy, see my examples below.  Of course this is probably roc's decision in the end, but I think it would simplify some of the code.

>+SCProxySettings::SCProxySettings() {
>+  // Register for notification of proxy setting changes
>+  // See: http://developer.apple.com/documentation/Networking/Conceptual/CFNetwork/CFStreamTasks/chapter_4_section_5.html
>+
>+  context.version = 0;
>+  context.info = this;
>+  context.retain = NULL;
>+  context.release = NULL;
>+  context.copyDescription = NULL;
>+  systemDynamicStore = SCDynamicStoreCreate(NULL, CFSTR("Mozilla"),
>+                                            proxyHasChangedWrapper, &context);
>+
>+  // Set up the store to monitor any changes to the proxies
>+  CFStringRef proxiesKey = SCDynamicStoreKeyCreateProxies(NULL);
>+  CFArrayRef keyArray = CFArrayCreate(NULL, (const void **)(&proxiesKey),
>+                                      1, &kCFTypeArrayCallBacks);

For example, the above would just be 

NSString *proxiesKey = SCDynamicStoreKeyCreateProxies(NULL);
NSArray *keyArray = [[NSArray alloc] initWithObject:proxiesKey];

NSArray can be used where-ever CFArrayRef is used in Core Foundation, same with CFStringRef <-> NSString, etc etc. In general, most data types in Core Foundation are "bridged" to Cocoa types. Whether a CF type is bridged is noted in the documentation for each data types, but for "primitives" like these, you can mostly assume they are.

>+  SCDynamicStoreSetNotificationKeys(systemDynamicStore, keyArray, NULL);
>+  CFRelease(keyArray);
>+  CFRelease(proxiesKey);
>+
>+  // Add the dynamic store to the run loop
>+  CFRunLoopSourceRef storeRLSource =
>+      SCDynamicStoreCreateRunLoopSource(NULL, systemDynamicStore, 0);
>+  CFRunLoopAddSource(CFRunLoopGetCurrent(), storeRLSource, kCFRunLoopCommonModes);
>+  CFRelease(storeRLSource);
>+

CFRunLoopSourceRef <-> NSRunLoop

etc.  I won't note more bridged types, I think you get the idea.

>+nsresult SCProxySettings::GetBool(CFStringRef proxy_name, PRBool &result) const
>+{
>+  int value;
>+  CFNumberRef boolNum;
>+
>+  if (proxyDict) {
>+    boolNum = (CFNumberRef)CFDictionaryGetValue(proxyDict, proxy_name);
>+
>+    if (boolNum && (CFGetTypeID(boolNum) == CFNumberGetTypeID())) {
>+       if (CFNumberGetValue(boolNum, kCFNumberIntType, &value)) {
>+         result = (value != 0);
>+         return NS_OK;
>+       }
>+     }
>+  }
>+  return NS_ERROR_FAILURE;
>+}

The above could be 

[(NSNumber *)[proxyDict objectForKey:proxy_name] boolValue]

And you wouldn't even need a helper method for that, would you? :-)

>+
>+nsresult
>+SCProxySettings::GetString(CFStringRef proxyName, nsCAutoString& resultValue)
>+const
>+{
>+  CFStringRef strValue;
>+
>+  if (proxyDict) {
>+    strValue = (CFStringRef) CFDictionaryGetValue(proxyDict, proxyName);
>+
>+    if ((strValue) && (CFGetTypeID(strValue) == CFStringGetTypeID())) {
>+      CFIndex length = CFStringGetLength(strValue);
>+      if (length != 0) {
>+        char buf[length+1];
>+        if(CFStringGetCString(strValue, buf, length+1, kCFStringEncodingASCII)) {
>+          resultValue.Assign(buf);
>+          return NS_OK;
>+        }
>+      }
>+    }
>+  }
>+  return NS_ERROR_FAILURE;
>+}

Just for reference, it's a pain that we have all these duplicated nsA[C]String <-> CFSStringRef/NSString conversion methods all over the tree. We really need shared helper methods for that. See bug 346345

>+
>+nsresult SCProxySettings::GetInt(CFStringRef proxy_name, PRInt32& resultValue) const
>+{
>+  CFNumberRef intValue;
>+
>+  if (proxyDict) {
>+    intValue = (CFNumberRef) CFDictionaryGetValue(proxyDict, proxy_name);
>+
>+    if ((intValue) && (CFGetTypeID(intValue) == CFNumberGetTypeID())) {
>+      if (CFNumberGetValue(intValue, kCFNumberIntType, &resultValue)) {
>+        return NS_OK;
>+      }
>+    }
>+  }
>+  return NS_ERROR_FAILURE;
>+}

[(NSNumber *)[proxyDict objectForValue:proxy_name] intValue]

>+
>+  CFIndex length = CFArrayGetCount(exceptionList);
>+  for (CFIndex i = 0; i != length; ++i) {
>+    CFStringRef strRef = (CFStringRef)CFArrayGetValueAtIndex(exceptionList, i);
>+    if (strRef && (CFGetTypeID(strRef) == CFStringGetTypeID())) {
>+      if (IsHostProxyEntry(aHost, aPort, strRef)) {
>+        inList = true;

Use PR_TRUE since it's a PRBool

>+  if (NS_OK == proxy.IsInExceptionList(aHost, aPort, isDirectHost) && isDirectHost) {
>+    aResult.AssignASCII("DIRECT");
>+  } else {
>+    aResult.AssignASCII("PROXY");
>+    aResult.Append(' ');
>+    aResult.Append(proxyHost);
>+    aResult.Append(':');
>+    aResult.Append(nsPrintfCString("%d", proxyPort));

You can probably merge these into one call to Assign, something like this:

aResult.Assign(NS_LITERAL_CSTRING("PROXY ") + proxyHost + nsPrintfCString(":%d", proxyPort));

    
    

    
  
I'm not really sure about the Cocoa vs CF issue, but Hakan's probably right.

I think we should just get rid of SCProxySettings. It's only used here, might as well just fold it into nsOSXSystemProxySettings. That will actually reduce the number of lines of code quite a bit.

Get rid of all the methods like GetFTPPort that are one or two-liners and are only called once, just move the code directly into the caller.

There's no point in GetBool/GetInt/GetString returning error codes if the callers don't check them. Probably we should check them.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Patch v3 (obsolete)
        — 
          — Splinter Review
      

    


  
Here's a much cleaned up version. I've sanitized the CF stuff, generalized methods, and other things, so this patch is a good bit lighter now. 

Depending on how strongly roc feels about moving the SCProxySettings code into nsOSXSystemProxySettings class, this may be ready for review (again).

However, it needs testing. It compiles, but I have no proxy, and no way to test, so please do that.

        Attachment #313576 -
        Attachment is obsolete: true

        Attachment #313624 -
        Flags: superreview?(roc)

        Attachment #313624 -
        Flags: review?(roc)

        Attachment #313576 -
        Flags: superreview?(roc)

        Attachment #313576 -
        Flags: review?(roc)

    
  

        Attachment #313624 -
        Attachment is patch: true

        Attachment #313624 -
        Attachment mime type: application/octet-stream → text/plain

    
    

    
  
Oh, one simple thing that I forgot to do was use nsObjCExceptions.h as described in comment 121.

    
    

    
  
+  if (protocol.Equals(NS_LITERAL_CSTRING("ftp")))

All these can be simplified to use EqualsLiteral.

I think it would be simpler still to fold SCProxySettings into nsOSXSystemProxySettings, please do that unless there's a good reason not to.

    
    

    
  


  
Thanks for the feedback and updates. I don't know why I didn't think of using ObjC++ :)

The macros from nsObjCExceptions.h have been added to the beginning of all the functions.
SCProxySettings has been merged into nsOSXSystemProxySettings.
I also made the recommended changes to the build system.

Could this please be reviewed again? Thanks.

    
    

    
  
> Could this please be reviewed again? Thanks.

You have to request review then ;-)



    
    

    
  
Comment on attachment 313800 [details] [diff] [review]
Mac OSX implementation of system proxy settings - #3

I'll review it

        Attachment #313800 -
        Flags: review?(hwaara)

    
  

        Attachment #313624 -
        Attachment is obsolete: true

        Attachment #313624 -
        Flags: superreview?(roc)

        Attachment #313624 -
        Flags: review?(roc)

    
    

    
  
Comment on attachment 313800 [details] [diff] [review]
Mac OSX implementation of system proxy settings - #3

Good work James, this patch keeps getting smaller and smaller! :-)

Just some more minor changes below, and this is good for super-review.

>+class nsOSXSystemProxySettings : public nsISystemProxySettings {
>+public:
>+  NS_DECL_ISUPPORTS
>+  NS_DECL_NSISYSTEMPROXYSETTINGS
>+
>+  nsOSXSystemProxySettings();
>+  nsresult Init();
>+
>+  // called by OSX when the proxy settings have changed
>+  void proxyHasChanged();
>+
>+  // is there a PAC url specified in the system configuration
>+  inline PRBool IsAutoconfigEnabled() const;
>+  // retrieve the pac url
>+  inline nsresult GetAutoconfigURL(nsCAutoString&) const;
>+
>+  // is the specified protocol enabled?
>+  inline PRBool IsProtocolEnabled(const nsACString& protocol) const;
>+  // get the host name for the specified protocol's proxy
>+  inline nsresult GetProtocolHost(nsCString& host, const nsACString& protocol) const;
>+  // get the port for the specified protocol's proxy
>+  inline PRInt32 GetProtocolPort(const nsACString& protocol) const;
>+
>+  // is host:port on the proxy exception list?
>+  PRBool IsInExceptionList(const nsACString& host, PRInt32 port) const;

I don't know if we want all of these to be inline; are we sure that's not gonna add a lot of code size? Please do whatever the other systems' proxy settings implementations do.

>+nsresult
>+nsOSXSystemProxySettings::Init()
>+{
>+  if (proxyDict == NULL) {
>+    return NS_OK;
>+  }
>+  else {
>+    return NS_ERROR_FAILURE;
>+  }
>+}

I don't know when this class is usually instantiated ; if it is created before it is used (maybe even before we know it's gonna be used) I think we should defer some of the initialization that is currently done in the constructor, and add it here instead.

>+
>+  NSNumber* value = [proxyDict objectForKey:(NSString*)protocolProxySetting];
>+  return ([value intValue] != 0);

Just for symmetry and safety, do a check that what we get here is really a NSNumber, like we already do in GetProtocolHost(). Also add that check to the other methods that miss it, so we're doing the same thing everywhere.

>+nsresult
>+nsOSXSystemProxySettings::GetPACURI(nsACString& aResult)
>+{
>+  NS_OBJC_BEGIN_TRY_ABORT_BLOCK_NSRESULT;
>+
>+  nsCAutoString pacUrl;
>+
>+  if (IsAutoconfigEnabled() && NS_OK == GetAutoconfigURL(pacUrl)) {

Use NS_SUCCEEDED(GetAutoconfigURL(pacURL)) instead

>+    aResult.Assign(pacUrl);
>+    return NS_OK;
>+  }
>+
>+  NS_OBJC_END_TRY_ABORT_BLOCK_NSRESULT;

Missing to return an error here (maybe NS_OBJC_... does it for you, but please don't rely on whatever that does). Same in other methods.

Other than that, I'm happy with it.

        Attachment #313800 -
        Flags: review?(hwaara) → review-

    
  
Assignee: diane → jamesbunton

    
    

    
  


  
> Good work James, this patch keeps getting smaller and smaller! :-)
Thanks :)

Changes:
 * There are no inline functions
 * All the initialisation has been moved to the Init() function.
 * Checks for ObjC object types have been added where appropriate.
 * NS_SUCCEEDED(x) is used instead of NS_OK == x
 * NS_ERROR_FAILURE returned explicitly instead of relying on the nsObjC macro.
 * Use NS_ENSURE_TRUE for assertions
 * Fixed GetProtocolPort() so it works correctly

        Attachment #313800 -
        Attachment is obsolete: true

        Attachment #313868 -
        Flags: review?

    
    

    
  
I can't figure out how to get this to build in release mode. It fails in the linking step because -framework SystemConfiguration isn't being added.

Any ideas?

    
    

    
  
You probably just need to add to toolkit/library/Makefile.in.

BTW

+PRBool
+IsHostProxyEntry(const nsACString& aHost, PRInt32 aPort, NSString* str)

Should be 'static'.

I'll have some more nitpicky comments soon but basically this looks great.

    
    

    
  
Comment on attachment 313868 [details] [diff] [review]
Mac OSX implementation of system proxy settings - #4

Good work

        Attachment #313868 -
        Flags: superreview?(roc)

        Attachment #313868 -
        Flags: review?

        Attachment #313868 -
        Flags: review+

    
    

    
  
+ifneq (,$(filter mac cocoa,$(MOZ_WIDGET_TOOLKIT)))

Drop 'mac', the old Carbon port is dead.

Some really minor style nits for consistency with Mozilla style guidelines. I don't necessarily like them either but consistency is good.

+  void proxyHasChanged();

*P*roxyHasChanged

+  SCDynamicStoreContext context;
+  SCDynamicStoreRef systemDynamicStore;
+  NSDictionary* proxyDict;

mContext, mSystemDynamicStore (or just mDynamicStore), mProxyDict

+  nsresult GetAutoconfigURL(nsCAutoString&) const;

Give a parameter name

+  PRBool IsProtocolEnabled(const nsACString& protocol) const;
+  // get the host name for the specified protocol's proxy
+  nsresult GetProtocolHost(nsCString& host, const nsACString& protocol) const;
+  // get the port for the specified protocol's proxy
+  PRInt32 GetProtocolPort(const nsACString& protocol) const;
+
+  // is host:port on the proxy exception list?
+  PRBool IsInExceptionList(const nsACString& host, PRInt32 port) const;

Parameter names should start with 'a', e.g. 'aFoo'

+static void proxyHasChangedWrapper(SCDynamicStoreRef store, CFArrayRef changedKeys, void* info);

Might as well put the definition right here so we don't have to repeat the prototype. And call it "ProxyHasChangedWrapper".

+  systemDynamicStore = SCDynamicStoreCreate(NULL, CFSTR("Mozilla"),
+                                            proxyHasChangedWrapper, &context);

Should we be checking for failure here?

Is CFRelease really unsafe to call with a null parameter? If so, that's lame.

+  CFRelease(rls);
+  CFRelease(systemDynamicStore);

Maybe need null checks here in case getting these failed in Init?

+  NS_ENSURE_TRUE(value == NULL || [value isKindOfClass:[NSNumber class]], PR_FALSE);
+  return ([value intValue] != 0);

Hang on, you're allowing value to be NULL but then if it is, we'll crash in [value intValue], won't we?

+  NS_ENSURE_TRUE(value == NULL || [value isKindOfClass:[NSString class]], NS_ERROR_FAILURE);
+  resultValue.Assign([value UTF8String]);

Similar

+  }
+  else if (protocol.EqualsLiteral("http")) {

I prefer "} else if (...) {", would save a few lines here.

But actually we could simplify all this a little bit more :-). Just define a static, const array of structs, one for each protocol, each struct with four fields:
  char mScheme[6];
  CFStringRef mSCKeyEnable, mSCKeyProxy, mSCKeyPort;
and then define a function that takes the URI as a parameter and loops through the array to find the struct for that scheme. You can use nsIURI::SchemeIs. Then you can call that function once and pass that struct around so other functions don't have to check the scheme.

+    aResult.AssignASCII("DIRECT");

AssignLiteral

    
    

    
  
Two things about nullness:

* CFRelease on NULL will crash (yes - this *is* lame): http://developer.apple.com/documentation/CoreFoundation/Reference/CFTypeRef/Reference/reference.html#//apple_ref/c/func/CFRelease

* Sending a message to NULL (or nil) in objc is harmless and will not crash. It will do nothing and just return 0 -- this means that often in objc code you can omit null-checking in many cases where C/C++ code would do it just to avoid crashing.

    
    

    
  


  
Thanks Roc. Consistency is indeed important :)

Changes:
 * Made suggested changes to the build system
 * Improved safety of Init() method
 * Added explicit checks for NULL with UTF8String so we don't Assign(NULL)
 * Loop over a static array of structs instead of if/else if
 * Adapted to use the style aParameter, mMember and UpperCaseFunctionNames
 * Use short style else statements

        Attachment #313868 -
        Attachment is obsolete: true

        Attachment #314053 -
        Flags: review?

        Attachment #313868 -
        Flags: superreview?(roc)

    
    

    
  
James, when you request review on a patch, you always need to specify from whom. Review from the wind just isn't gonna work.

Since I already gave this patch r, you can go ahead and review+ it and carry forward the super-review request to roc.

    
    

    
  
+          if (mProxyDict) {
+            return NS_OK;

Shouldn't we release proxiesKey here?

In general we try to not nest too much. So instead of 

  if (ok) {
    if (ok) {
      if (ok) {
do
  if (!ok)
    cleanup and return;
  if (!ok)
    cleanup and return;
  if (!ok)
    cleanup and return

--- as long as that's not significantly more complicated.

SchemeMappingList should start with a lowercase letter, probably gSchemeMappingList. It should be declared const too.

+    if ([enabled intValue] == 0) {
+      continue;
+    }

This might as well be "break", right? Since each scheme occurs just once in the list. Also, single 'continue', 'break' or 'return' statements can omit the surrounding braces.

+    if (host == NULL) {
+      continue;
+    }

Ditto

Everything else looks great. We've nearly converged here :-).

    
    

    
  


  
Sorry, I obviously didn't "get" the flags :)

Changes:
 * Flattened out Init()
 * mContext needs to be around for the life of mSystemDynamicStore
 * break instead of continue
 * Use short if() forms with break, continue, return statements

Nearly there :)

        Attachment #314053 -
        Attachment is obsolete: true

        Attachment #314082 -
        Flags: superreview?

        Attachment #314082 -
        Flags: review+

        Attachment #314053 -
        Flags: review?

    
  

        Attachment #314082 -
        Flags: superreview? → superreview?(roc)

    
  

        Attachment #313800 -
        Attachment description: Mac OSX implementation of system proxy settings - #2 → Mac OSX implementation of system proxy settings - #3

    
    

    
  
Comment on attachment 314082 [details] [diff] [review]
Mac OSX implementation of system proxy settings - #6

excellent!

        Attachment #314082 -
        Flags: superreview?(roc) → superreview+

    
    

    
  
Can we get this in for 1.9?

    
    

    
  
(In reply to comment #140)
> (From update of attachment 314082 [details] [diff] [review])
> excellent!

Thanks for your help guys :)

So, where do we go from here? Anything else I can do to help?

    
    

    
  
It would be great if there was a way to test this automatically, but I don't think there is.

I think we'll just have to land this on trunk as soon as it reopens after Firefox 3.

    
  
Blocks: 427714

    
    

    
  
(In reply to comment #144)
> It would be great if there was a way to test this automatically, but I don't
> think there is.

Maybe we could let some tinderboxes use proxies, as a simple way to test this code?  filed bug 427714 for this issue, anyhow.

    
    

    
  
I sure hope this can be implemented. Changing 'Locations' on my Mac doesn't do anything to FF. It's a pain having to go into the preferences to change the proxies there are well.

Good luck guys with the coding....I can't help :(

    
    

    
  
The coding is done. This will be in the next major release of Firefox.

    
    

    
  
Any chance of a new screenshot to whet our appetite :) ...or is the 'current' one still applicable?

    
    

    
  
(In reply to comment #148)
> Any chance of a new screenshot to whet our appetite :) ...or is the 'current'
> one still applicable?

The one from 2005 is still pretty much accurate. The radio button label says "Use system proxy settings" and is underneath "Autodetect proxy settings". It is not enabled by default.


    
    

    
  
(In reply to comment #147)
> The coding is done. This will be in the next major release of Firefox.
> 

By next major version, you mean the one after FF3, I suppose, not in a point version... Or will it be backported to the FF3 branch as well?

    
    

    
  
There will be a Gecko 1.9.1 release, and we should be able to take this patch for that.

    
    

    
  
marking wanted1.9.1? to get this in the triage queue. 
Flags: wanted1.9.1?
Priority: -- → P2

    
  
Flags: wanted1.9.1? → wanted1.9.1+

    
    

    
  
I'll land this as soon as I get a chance, but if someone else gets to it first that's fine.
Keywords: checkin-needed

    
    

    
  
Checked in. Thanks everyone!!!
Status: NEW → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED

    
    

    
  
As the original requestor (6yrs 2mths ago!) many thanks to the efforts of those involved. 
I look forward to actually being able to use it in a release sometime soon :)

    
    

    
  
(In reply to comment #154)
> Checked in. Thanks everyone!!!
> 
Based on this, removing checkin-needed keyword. This appears to be in hg now and no other approvals granted/requested.
Keywords: checkin-needed

    
  
Flags: in-litmus?
Target Milestone: Future → mozilla1.9.1

    
  
Keywords: helpwanted
Hardware: Macintosh → All

    
    

    
  
Yes, can we get this in FF3?
(I'm a recent switcher thanks to the use of Cocoa and the general niceness, but I'm also about to ditch FF3 again because it doesn't support my central proxy configuration via Network Locations...)

    
    

    
  
Is it really true that all the effort to find and fix a bug or undesirable behavior gets flushed down the toilet with the release of the next major version?  Tell me it's not true!

    
    

    
  
(In reply to comment #158)
> Is it really true that all the effort to find and fix a bug or undesirable
> behavior gets flushed down the toilet with the release of the next major
> version?  Tell me it's not true!

No, of course not. This patch was developed far too late in the Firefox 3 release cycle (just before rc1) to be included then.

It has been checked in to the repository and will appear in Firefox 3.1. It's compiled into the current nightly builds, however due to a small bug it isn't enabled yet.

The patches in attachments 313572,327637 and  need to be applied for this feature to be fully functional, so I'm marking this bug as INCOMPLETE for now.

Resolution: FIXED → INCOMPLETE

    
    

    
  
(In reply to comment #160)
> The patches in attachments 313572,327637 and  need to be applied for this
> feature to be fully functional, so I'm marking this bug as INCOMPLETE for now.

This bug should remain FIXED. Please file follow up bugs for any other work that needs to get done.
Resolution: INCOMPLETE → FIXED

    
    

    
  
Samuel, shouldn't we try to get both remaining fixes into 1.9.1 under the hood of this bug? Both patches are attached here. So it looks like they were forgotten. Is it really worth filing a new bug for that?

    
    

    
  
Please see bug 443083.

    
    

    
  
For attachment 313572 [details] [diff] [review], this will show the button on Windows where we don't actually have support. We probably shouldn't do that. Gavin, what's the best way to hide it on Windows?

    
    

    
  
Does the Mac service not use the same contract ID as the Linux one?

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/browser/components/preferences/connection.js&rev=1.9#74 should be taking care of unhiding it.

    
    

    
  
Indeed. In fact, in my debug builds the option *does* show up, so I think attachment 313572 [details] [diff] [review] is not needed. We just need attachment 327637 [details] [diff] [review] so the component shows up in opt builds. Thanks for pointing that out.

    
    

    
  
Comment on attachment 313572 [details] [diff] [review]
Enables the UI radio button for system proxy settings

Confirmed. Attachment 313572 [details] [diff] is not needed.

Thanks.

        Attachment #313572 -
        Attachment is obsolete: true

    
  

        Attachment #327637 -
        Flags: review?(benjamin) → review+

    
    

    
  
I still have a problem to verify this patch. How can I set the system proxy with a current 1.9.1a1 nightly build? 

    
    

    
  
Does attachment 327637 [details] [diff] [review] need sr or can it be checked-in?

(In reply to comment #168)
> I still have a problem to verify this patch. How can I set the system proxy
> with a current 1.9.1a1 nightly build? 

Looks like that the above fix is needed to check with a nightly build.
Target Milestone: mozilla1.9.1 → mozilla1.9.1a1

    
    

    
  
It needs checkin, no sr needed for build changes.

    
    

    
  
Comment on attachment 327637 [details] [diff] [review]
Makes the OSX system proxy work in release builds

Pushed in 15865:e0b988b411cd.

    
  

        Attachment #327637 -
        Flags: approval1.9.0.2?

    
  

        Attachment #327637 -
        Flags: approval1.9.0.2?

    
    

    
  
Verified with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1a1pre) Gecko/2008071302 Minefield/3.1a1pre ID:2008071302

I run tests for the different protocols and everything looks fine. The system settings are used as specified.
Status: RESOLVED → VERIFIED

    
    

    
  
There appears to be a flaw with this code (at least as it exists in FF3.5 RC2).  I've posted my findings to a more appropriate bug (457377), but thought I would make mention here so that everyone who was involved with this code is exposed.

    
    

    
  
Hi,

for the last 4 years I'm following this ticket. 

In the meanwhile THIS TICKET IS OBSOLETE because the litle Firefox EXTENSION "SYSTEM PROXY" does the job excellent!

http://systemproxy.mozdev.org/

    
    

    
  
Re comment #175:  this suggestion is just not appropriate.  

The bug refers to the core behavior of the browser. It should not be "fixed" via extensions to Firefox, which break frequently as new versions of firefox are introduced.

    
    

    
  
Can you please tell us for which protocol the settings aren't taken from the system settings? Thanks.

    
    

    
  
(In reply to comment #177)
> Can you please tell us for which protocol the settings aren't taken from the
> system settings? Thanks.

I suspect the previous commenters are using 3.0.x release builds, where this bug isn't fixed. It is fixed in 3.5, which will be released shortly.

    
    

    
  
It's almost fixed in 3.5: Fx 3.5 can take system proxy settings, if it is told so. However, I think, using the system settings should be the standard setting (current standard is "No proxy").

    
    

    
  
Suppose we should open a separate bug for using system settings as default?  This one's kind of dated, and regardless it's more of a UI discussion.

    
    

    
  
Andrew, yes please do so. Mention the bug number here so we are aware of it. Thanks.
Flags: in-litmus?

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






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: