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

[Andreas Hartmetz]

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


Again, I'd like to know which testing you did.


kio/kio/tcpslavebase.h
<http://git.reviewboard.kde.org/r/102696/#comment6021>

    "if not a null pointer, *errorString will be set to contain more information about a connect error, or to the empty string if there was no error."



kio/kio/tcpslavebase.cpp
<http://git.reviewboard.kde.org/r/102696/#comment6022>

    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.



kioslave/http/http.cpp
<http://git.reviewboard.kde.org/r/102696/#comment6016>

    int connectError = 0;



kioslave/http/http.cpp
<http://git.reviewboard.kde.org/r/102696/#comment6018>

    proxyType



kioslave/http/http.cpp
<http://git.reviewboard.kde.org/r/102696/#comment6017>

    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.



kioslave/http/http.cpp
<http://git.reviewboard.kde.org/r/102696/#comment6019>

    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.



kioslave/http/http.cpp
<http://git.reviewboard.kde.org/r/102696/#comment6020>

    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.


- Andreas Hartmetz


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/9a5c6513/attachment.htm>