> On Sept. 29, 2011, 8:08 p.m., Andreas Hartmetz wrote: > > kioslave/http/http.cpp, line 4122 > > <http://git.reviewboard.kde.org/r/102696/diff/5/?file=37372#file37372line4122> > > > > This looks like a hack and is, I think, surprising. It is a part of > > connection setup, where all the other similar variables are set. Is it > > really necessary to move this here? > > Too distributed state changes in too many places are already a big > > problem in this ioslave, which is why I'm very sensitive to such things.
Hmm... I am confused here too. :) I completely removed that line. I did not move it there. Resetting the "isKeepAlive" flag in httpCloseConnection was a very big mistake because it will overwrite the settings we set in resetSessionSettings, especially if the connection happens to have been closed by the server already and a new one has to be established in ::sendQuery again. IOW, isKeepAlive will be set to false in that scenario when it should have been true as already set in ::resetSessionSettings. > On Sept. 29, 2011, 8:08 p.m., Andreas Hartmetz wrote: > > kioslave/http/http.cpp, line 2314 > > <http://git.reviewboard.kde.org/r/102696/diff/5/?file=37372#file37372line2314> > > > > What if no next request is sent and the connection is supposed to get > > closed from our side? > > Waiting for the server to do it will probably work, but it would be > > "nicer" to do it ourself. What does the HTTP spec say? Hmm... Confused. I only moved that piece of code from the bottom part of ::sendQuery to the very top. I did not remove it completely. Did you intend this comment for another line perhaps ? - Dawit ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/102696/#review6928 ----------------------------------------------------------- On Sept. 27, 2011, 9:12 p.m., Dawit Alemayehu wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://git.reviewboard.kde.org/r/102696/ > ----------------------------------------------------------- > > (Updated Sept. 27, 2011, 9:12 p.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 > ------- > > - Tested changes using a real proxy server, privoxy. > > - Tested changes using a poor man's SOCKS proxy setup: > ssh -D <port#> <ssh-server-address> > and the following PAC script: > > function FindProxyForURL( url, host ) > { > var resolved_ip = dnsResolve(host); > > if (isInNet(resolved_ip, "127.0.0.1", "255.255.255.0")) > return "DIRECT"; > > return "SOCKS 127.0.0.1:9999; DIRECT"; > } > > > Thanks, > > Dawit Alemayehu > >
