GNOME Bugzilla – Bug 642928
Soup should take advantage of the new GProxyResolver API
Last modified: 2011-03-21 17:31:23 UTC
Created attachment 181541 [details] [review] first try at implementing this Subject says it all. I discussed this briefly with Dan on IRC. This is a first cut implementation, I still have to do more testing, but it should work.
Comment on attachment 181541 [details] [review] first try at implementing this >+typedef struct { >+ SoupProxyURIResolver *resolver; >+ GCancellable *cancellable; there are scattered tabs-vs-spaces problems throughout >+ gchar **proxy_uris = NULL; no gint or gchar please >+ /* We need to handle direct:// specially, otherwise >+ * SoupSession will try to resolve it as the proxy address. */ >+ if (!g_strcmp0 (proxy_uris[0], "direct://")) { nitpick: put the "*/" on its own line also, this check has to come after the "proxy_uris != NULL" check... it would be much nicer if the return-and-cleanup bit wasn't repeated 3 times in this function. See also bug 644211, and bug 625898. I need to figure out exactly how this is going to work, but I guess maybe SoupProxyResolverGNOME could be the same as SoupProxyResolverDefault, except explicitly requesting the "gnome" GProxyResolver rather than getting the default.
Created attachment 183597 [details] [review] second try What do you think of this one regarding the error handling? Indentation should be fixed as well!
Created attachment 183599 [details] [review] real second try Forgot to add a change to the commit before formatting the patch (doh). Regarding the GNOME resolver, what do you think of having a class method whose responsibility is to obtain the resolver that will be used? In this case the GNOME resolver could be simply inherit the Default resolver and reimplement that class method. It would also make it easier for people to use the default resolver to implement their own custom GProxyResolver-based resolvers, perhaps. Or is that exposing more than you want?
Comment on attachment 183599 [details] [review] real second try > Regarding the GNOME resolver, what do you think of having a class method whose > responsibility is to obtain the resolver that will be used? In this case the > GNOME resolver could be simply inherit the Default resolver and reimplement > that class method. Sounds like a plan Everything else looks good. If you want to fix up the GNOME resolver too then you can attach an updated patch. Otherwise this one is good to commit (and I'll deal with the GNOME one this weekend).
I pushed this myself to get it in before hard code freeze. Thanks for the work!