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

VERIFIED FIXED in mozilla1.9.1a1

Status

()

Core
Networking
P2
enhancement
VERIFIED FIXED
17 years ago
4 years ago

People

(Reporter: Tim Gould, Assigned: James Bunton)

Tracking

(Blocks: 1 bug)

Trunk
mozilla1.9.1a1
All
Mac OS X
Points:
---
Dependency tree / graph
Bug Flags:
wanted1.9.1 +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(6 attachments, 10 obsolete attachments)

69.71 KB, image/jpeg
Details
35.85 KB, patch
Details | Diff | Splinter Review
50.00 KB, patch
Details | Diff | Splinter Review
63.61 KB, patch
Diane Trout
: review+
Details | Diff | Splinter Review
16.83 KB, patch
James Bunton
: review+
Details | Diff | Splinter Review
768 bytes, patch
Benjamin Smedberg
: review+
Details | Diff | Splinter Review
(Reporter)

Description

17 years ago
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.

Comment 1

17 years ago
-> Networking for backend support
Assignee: sgehani → new-network-bugs
Component: Preferences → Networking
QA Contact: sairuh → benc

Comment 2

17 years ago
+qawanted - dupeme.
Keywords: qawanted

Comment 3

17 years ago
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!

Comment 4

17 years ago
*** This bug has been confirmed by popular vote. ***
Status: UNCONFIRMED → NEW
Ever confirmed: true

Comment 5

17 years ago
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]

Comment 6

16 years ago
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.
(Reporter)

Comment 7

16 years ago
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...

Comment 8

16 years ago
Removing qawanted since this apparently isn't a duplicate of the Classic proxy
settings bug, bug 97214.
Keywords: qawanted

Comment 9

16 years ago
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.

Comment 10

16 years ago
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.

Comment 11

16 years ago
codewise, mdescalz@mindspring.com pointed out that we should look here:

http://lxr.mozilla.org/mozilla/source/chimera/src/preferences/
PreferenceManager.mm#233 

Comment 12

16 years ago
*** Bug 168321 has been marked as a duplicate of this bug. ***

Comment 13

16 years ago
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...

Comment 14

16 years ago
Pink, Simon, is this being done for Chimera?  
Target Milestone: --- → Future

Comment 15

16 years ago
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]

Comment 16

16 years ago
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

Comment 17

16 years ago
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.

Comment 18

16 years ago
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?


Comment 19

16 years ago
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). 

Comment 20

16 years ago
how often does IE read the system value?

Comment 21

16 years ago
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. ***

Comment 23

14 years ago
*** Bug 268736 has been marked as a duplicate of this bug. ***

Comment 24

14 years ago
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.

Comment 25

14 years ago
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.

Comment 26

14 years ago
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...

Comment 27

14 years ago
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.

Comment 28

14 years ago
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 :-) )

Comment 29

14 years ago
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.

Comment 30

14 years ago
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.

Updated

14 years ago
Blocks: 237584

Comment 31

14 years ago
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.

Comment 32

14 years ago
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?

Comment 33

14 years ago
There's code in Camino to do this, but problems remain (mostly how to map
wildcards in the proxy bypass list).

Comment 34

14 years ago
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.

Comment 35

14 years ago
(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.

Comment 36

14 years ago
(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.

Comment 38

14 years ago
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

Comment 39

14 years ago
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.

Comment 40

14 years ago
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.

Comment 41

14 years ago
*** Bug 278367 has been marked as a duplicate of this bug. ***

Comment 42

14 years ago
Created attachment 171263 [details]
Screenshot

Comment 43

14 years ago
(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.

Comment 44

14 years ago
(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...

Comment 45

14 years ago
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.

Comment 46

14 years ago
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.]

Updated

14 years ago
Assignee: general → joshmoz

Comment 47

13 years ago
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.

Comment 48

13 years ago
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?
(Reporter)

Comment 49

13 years ago
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.

Comment 50

13 years ago
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?

Comment 51

13 years ago
(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

Comment 52

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

Comment 53

13 years ago
(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

Comment 54

13 years ago
Created attachment 201972 [details]
Screenshot of dialog in 1.5RC1
Attachment #171263 - Attachment is obsolete: true

Comment 55

13 years ago
(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

Comment 56

13 years ago
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
> 

Comment 57

13 years ago
(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.

Comment 58

13 years ago
(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

Comment 59

13 years ago
(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.

Comment 60

13 years ago
(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.

Comment 61

13 years ago
How are you implementing the prefs value for the radio button?

Comment 62

13 years ago
(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?

Comment 63

13 years ago
(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.

Comment 64

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

Comment 65

13 years ago
(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?

Comment 66

13 years ago
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.

Comment 67

13 years ago
Created attachment 204539 [details] [diff] [review]
Patch file - needs "extra files" also

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

Comment 68

13 years ago
Created attachment 204540 [details] [diff] [review]
The new files associated with patch1
*** Bug 318443 has been marked as a duplicate of this bug. ***

Comment 70

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

Comment 71

13 years ago
(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 72

13 years ago
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?

Updated

13 years ago
Attachment #204540 - Flags: review?

Comment 73

13 years ago
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.

Comment 76

13 years ago
(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 :-).

Comment 78

13 years ago
(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.

Comment 80

13 years ago
(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?

Comment 83

13 years ago
(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

Updated

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

Updated

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

Comment 85

13 years ago
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.

Updated

13 years ago
Attachment #204539 - Flags: review?(joshmoz)

Updated

13 years ago
Attachment #204540 - Flags: review?(joshmoz)

Comment 87

12 years ago
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?

Comment 88

12 years ago
(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.

Comment 89

12 years ago
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.

Comment 90

12 years ago
(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.

Comment 91

12 years ago
Try this one:

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

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

Comment 92

12 years ago
(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

Comment 93

12 years ago
(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.

Comment 94

12 years ago
(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

Comment 95

12 years ago
> 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?

Comment 96

12 years ago
(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".

Comment 97

12 years ago
Created attachment 239872 [details] [diff] [review]
implements network preference settings retrieval

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)

Comment 98

12 years ago
(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.

Updated

12 years ago
Attachment #239872 - Attachment is obsolete: true
Attachment #239872 - Flags: review?(joshmoz)

Comment 99

12 years ago
Created attachment 239924 [details] [diff] [review]
This patch applies cleanly and implements the retrieval

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)

Comment 100

12 years ago
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.

Comment 101

12 years ago
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 102

12 years ago
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+

Comment 103

12 years ago
 
> +  //! 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.

Comment 104

12 years ago
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

Comment 105

12 years ago
(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). 

Comment 106

12 years ago
Created attachment 242157 [details] [diff] [review]
Made requested updates to patch from bug 66057

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+

Comment 107

12 years ago
(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.

Comment 108

12 years ago
> 
> 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

Comment 109

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

Comment 110

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

Comment 111

12 years ago
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.

Comment 112

12 years ago
(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.

Comment 113

12 years ago
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.

Comment 115

11 years ago
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.

Comment 116

11 years ago
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
(Assignee)

Comment 117

10 years ago
Created attachment 313570 [details] [diff] [review]
Mac OSX implementation of system proxy settings

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
(Assignee)

Comment 118

10 years ago
Created attachment 313572 [details] [diff] [review]
Enables the UI radio button for system proxy settings

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

Comment 119

10 years ago
Created attachment 313576 [details] [diff] [review]
Mac OSX implementation of system proxy settings - #2

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 121

10 years ago
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.

Comment 123

10 years ago
Created attachment 313624 [details] [diff] [review]
Patch v3

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)

Updated

10 years ago
Attachment #313624 - Attachment is patch: true
Attachment #313624 - Attachment mime type: application/octet-stream → text/plain

Comment 124

10 years ago
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.
(Assignee)

Comment 126

10 years ago
Created attachment 313800 [details] [diff] [review]
Mac OSX implementation of system proxy settings - #3

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.

Comment 127

10 years ago
> Could this please be reviewed again? Thanks.

You have to request review then ;-)

Comment 128

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

I'll review it
Attachment #313800 - Flags: review?(hwaara)

Updated

10 years ago
Attachment #313624 - Attachment is obsolete: true
Attachment #313624 - Flags: superreview?(roc)
Attachment #313624 - Flags: review?(roc)

Comment 129

10 years ago
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-

Updated

10 years ago
Assignee: diane → jamesbunton
(Assignee)

Comment 130

10 years ago
Created attachment 313868 [details] [diff] [review]
Mac OSX implementation of system proxy settings - #4

> 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?
(Assignee)

Comment 131

10 years ago
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 133

10 years ago
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

Comment 135

10 years ago
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.
(Assignee)

Comment 136

10 years ago
Created attachment 314053 [details] [diff] [review]
Mac OSX implementation of system proxy settings - #5

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)

Comment 137

10 years ago
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 :-).
(Assignee)

Comment 139

10 years ago
Created attachment 314082 [details] [diff] [review]
Mac OSX implementation of system proxy settings - #6

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?
(Assignee)

Updated

10 years ago
Attachment #314082 - Flags: superreview? → superreview?(roc)
(Assignee)

Updated

10 years ago
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+

Comment 141

10 years ago
Can we get this in for 1.9?
(Assignee)

Comment 143

10 years ago
(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.

Updated

10 years ago
Blocks: 427714

Comment 145

10 years ago
(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.

Comment 146

10 years ago
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.

Comment 148

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

Comment 149

10 years ago
(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

Updated

10 years ago
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
Last Resolved: 10 years ago
Resolution: --- → FIXED
(Reporter)

Comment 155

10 years ago
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

Updated

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

Updated

10 years ago
Keywords: helpwanted
Hardware: Macintosh → All

Comment 157

10 years ago
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...)

Comment 158

10 years ago
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!
(Assignee)

Comment 159

10 years ago
Created attachment 327637 [details] [diff] [review]
Makes the OSX system proxy work in release builds
(Assignee)

Comment 160

10 years ago
(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?
(Assignee)

Comment 163

10 years ago
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.
(Assignee)

Comment 167

10 years ago
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

Updated

10 years ago
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

Updated

9 years ago
Duplicate of this bug: 494563

Comment 174

9 years ago
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.

Comment 175

9 years ago
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/

Comment 176

9 years ago
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.

Comment 179

9 years ago
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").

Comment 180

9 years ago
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.

Comment 182

9 years ago
Done: bug 500983

Updated

4 years ago
Flags: in-litmus?
You need to log in before you can comment on or make changes to this bug.