Re: Approval for no proxy patches

[Malcolm Tredinnick <malcolm commsecure com au>]

On Thu, 2003-08-07 at 07:45, Jonathan Blandford wrote:
> mpeseng tin it writes:
> > I'd like to request permission to check in
> > 
> > http://bugzilla.gnome.org/show_bug.cgi?id=112979
> > Teach gnome-vfs HTTP method to not use the proxy for some hosts
> > (and add a gconf key to specify it)
> > 
> > http://bugzilla.gnome.org/show_bug.cgi?id=113222
> > Implement the key in epiphany
> > 
> > Alex would like the gnome vfs patch to be checked in too. It adds a string
> > but should not bother translators becuase gnome-vfs schemas are not translated.
> > The feature is quite necessary for a lot of people. Unfortunately there
> > is still not an user interface for it, I guess that will have to wait post
> > 2.4.
> 
> Reading through both patches, I have a couple comments:
> 
>  * We should definitely commit the schemas change whether or not any of
>    the code changes happens.  It seems clear that we need the key in
>    GNOME, and epiphany having its own temporary key is a bad idea.
> 
>  * I'm a tiny little concerned that you don't actually check any of the
>    strings you get from the key before passing them to mozilla in the
>    epiphany patch.  Also, it doesn't look like you handle the empty set
>    case at all, and are counting on mozilla handling what you pass it.
>    In theory, I'm happy with a patch to epiphany to handle the GConf
>    key.

As the guy who wrote the Epiphany patch and the Galeon patch and the
gnome-vfs patch, I can probably comment here. Mozilla does a lot of
error checking on the string it gets for the proxies (including allowing
for it to be empty) <insert unpleasant memories of reading the Moz
source code here/>. So there is no real problem there, hence redundant
error checking is left out.

>  * The patch to gnome-vfs is more scary, and I'm less thrilled with
>    it. I'd really like to hear from Alex and Teuf on whether or not it's
>    worth pushing this in.

I think it should wait for GNOME 2.6 so that we can fix the control
centre widget to permit the setting to be correctly adjusted. I started
to write this code, but ran out of time before feature-freeze, so I've
moved onto other things since then.

With respect to this patch, could you possibly elaborate on why you are
"less than thrilled with it". It's been through a couple of rounds of
review (see the bug report) and is pretty well tested (including with
IPv6) on my systems; I am using this patch every day now, since things
are pretty unusable without it as I move between networks. I am happy to
mess around with the patch as required to meet whatever minor complaints
people may have -- the coding style and variable naming schemes, etc, in
gnome-vfs is hardly a model of consistency. If you have major design
issues, then you need to explain them to me in small words and short
sentences, since I cannot read minds.

Cheers,
Malcolm