Review Request: Proxy overhaul Part 5: Add support for trying multiple proxies to KIO HTTP

[Dawit Alemayehu]

> On Sept. 25, 2011, 2:55 p.m., Andreas Hartmetz wrote:
> > Again, I'd like to know which testing you did.

See my response to the same question you asked in https://git.reviewboard.kde.org/r/102691.


> On Sept. 25, 2011, 2:55 p.m., Andreas Hartmetz wrote:
> > kioslave/http/http.cpp, line 2166
> > <http://git.reviewboard.kde.org/r/102696/diff/1/?file=37191#file37191line2166>
> >
> >     AFAIU the proxy list can't contain a DIRECT entry somewhere; if it contains DIRECT it contains only DIRECT.
> >     Does it even make sense to put the DIRECT case into the loop? It looks like a special case to me that could and should be handled outside the loop.

Yes, it can. See my test sample I already provided. A proxy returned by a PAC file can contain "DIRECT" as a last resort. See http://en.wikipedia.org/wiki/Proxy_auto-config for details.


> On Sept. 25, 2011, 2:55 p.m., Andreas Hartmetz wrote:
> > kioslave/http/http.cpp, line 2168
> > <http://git.reviewboard.kde.org/r/102696/diff/1/?file=37191#file37191line2168>
> >
> >     Not sure if that would be correct, but I'd prefer something like:
> >     if (isDirectConnect) {
> >         Q_ASSERT(proxyType == QNetworProxy::NoProxy);
> >         (...)
> >     
> >     because you have two variables here that should make sense together, and it's better to check that they do instead of risking confusion later when something goes wrong.

Ahhh... I do not see the point because there is no way "if (isDirectConnect)" will ever be called unless proxyType == QNetworkProxy::NoProxy. IOW, it is impossible for this to happen.


> On Sept. 25, 2011, 2:55 p.m., Andreas Hartmetz wrote:
> > kioslave/http/http.cpp, line 2195
> > <http://git.reviewboard.kde.org/r/102696/diff/1/?file=37191#file37191line2195>
> >
> >     Why is this here and not in a more "general" place? The path between the place where the variable is set and the place where it is used is unclear to me. If there is too much in between, it is hard to guarantee that the variable has the correct value where it is used.

You are right. I will find a more proper place for it. Probably in "sendQuery" since we would know if we are using a proxy or not by then.


> On Sept. 25, 2011, 2:55 p.m., Andreas Hartmetz wrote:
> > kio/kio/tcpslavebase.cpp, line 321
> > <http://git.reviewboard.kde.org/r/102696/diff/1/?file=37189#file37189line321>
> >
> >     Please always clear *errorString on no error. That makes the API nicer to use because the value of *errorString will depend strictly on the result of connectToHost, not on its previous value if any.

Shouldn't that be the responsiblity the caller ? But I will change it as you suggest to be on the safe side, especially since I did make the exact same thing in KProtocolManager::slaveProtocol. :)


> On Sept. 25, 2011, 2:55 p.m., Andreas Hartmetz wrote:
> > kioslave/http/http.cpp, line 2139
> > <http://git.reviewboard.kde.org/r/102696/diff/1/?file=37191#file37191line2139>
> >
> >     int connectError = 0;


- Dawit


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/102696/#review6797
-----------------------------------------------------------


On Sept. 25, 2011, 2:28 a.m., Dawit Alemayehu wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/102696/
> -----------------------------------------------------------
> 
> (Updated Sept. 25, 2011, 2:28 a.m.)
> 
> 
> Review request for kdelibs.
> 
> 
> Description
> -------
> 
> This 5th patch in a serious of patches meant to improve proxy support in KDE deals with support at the kioslave level. More specifically the http ioslave. The patch is necessary to provide proper support for PAC script based proxy configuration. Namely allowing aleternate proxy servers to be specified and used as necessary. Here are the change this patch makes:
> 
>      * Add a new function in TCPSlaveBase to connect to the a remote server without 
>         automatically sending error notitification to the client. [NEW API]
>     
>      * Move the proxy related code from 'resetSessionSettings' to 'httpOpenConnection'.
>        Proxy information will now be only set from 'setHost' and reset from 
>        'reparseConfiguration' as it should have been from the beginning. No need to 
>        reparse proxy related information on every request.
>     
>      * Added a new variable, proxyUrls, to HTTPRequest to store the multiple proxy URLs
>         obtained from the "ProxyUrls" meta-data.
>     
>      * Modified 'httpShouldCloseConnection' to account for multiple proxy addresses.
>     
>      * Modified 'sendQuery' to connect to the remote server before formatting the HTTP
>        headers so that the headers can properly reflect the correct proxy settings.
> 
> 
> Diffs
> -----
> 
>   kio/kio/tcpslavebase.h 3f87ea8 
>   kio/kio/tcpslavebase.cpp ec70559 
>   kioslave/http/http.h d8c47c7 
>   kioslave/http/http.cpp 6d41a13 
> 
> Diff: http://git.reviewboard.kde.org/r/102696/diff/diff
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Dawit Alemayehu
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-core-devel/attachments/20110925/41bd5661/attachment.htm>