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

Reply via email to