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