Review Request: Proxy overhaul Part 4: More proxy changes and fixes for KProtocolManager

[Andreas Hartmetz]

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


I'd actually be interested to hear which testing you did.

The "ResolveHostNamesBeforeProxyCheck" option seems strange. In which situations is this supposed to be set / not set?


kio/kio/kprotocolmanager.h
<http://git.reviewboard.kde.org/r/102691/#comment6014>

    "Returns a comma-separated list of hosts that should be contacted"...



kio/kio/kprotocolmanager.h
<http://git.reviewboard.kde.org/r/102691/#comment6015>

    "overriding other proxy settings"



kio/kio/kprotocolmanager.h
<http://git.reviewboard.kde.org/r/102691/#comment6008>

    What is a partial host name? I know that the wording isn't your fault, but maybe you know what it means.



kio/kio/kprotocolmanager.h
<http://git.reviewboard.kde.org/r/102691/#comment6013>

    Good.



kio/kio/kprotocolmanager.h
<http://git.reviewboard.kde.org/r/102691/#comment6007>

    This is not a method that allocates a resource, for which "request" is the right word. It's a method that answers a question.
    I would suggest something like "Returns the proxy server to use to contact @p url, or "DIRECT" if no proxy should be used."
    
    I don't understand the part about the empty string at all. How is it different from "DIRECT"?



kio/kio/kprotocolmanager.h
<http://git.reviewboard.kde.org/r/102691/#comment6009>

    QString::null usage is frowned upon. Just say "empty string".



kio/kio/kprotocolmanager.h
<http://git.reviewboard.kde.org/r/102691/#comment6010>

    See above. Client programmers don't request a proxy (the meaning of that is closer to "create me one"), they ask for the address of an existing one.



kio/kio/kprotocolmanager.h
<http://git.reviewboard.kde.org/r/102691/#comment6011>

    What's the use case here? Do you expect ioslaves to try all proxies from the list? Is the list ordered in some way?
    
    How is the list of proxy server addresses related to @p url? It's obvious from the name, but it is good practice to state all the important facts in the apidox.



kio/kio/kprotocolmanager.cpp
<http://git.reviewboard.kde.org/r/102691/#comment6012>

    This looks more like
    "if no proxy scheme is given, assume SOCKS"


- Andreas Hartmetz


On Sept. 25, 2011, 2:08 a.m., Dawit Alemayehu wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/102691/
> -----------------------------------------------------------
> 
> (Updated Sept. 25, 2011, 2:08 a.m.)
> 
> 
> Review request for kdelibs.
> 
> 
> Description
> -------
> 
> This patch is the 4th in the serious of patches designed to resolve bugs and missing functionality in KDE's proxy manager. The changes made with this patch are as follows:
> 
>     * Add code that resolves a request url's hostname before attempting to match
>       it against the no proxy for list so long as the "ResolveHostNamesBeforeProxyCheck"
>       option is set.
>     
>     * Allow "DIRECT" as a special keyword in the list of proxy server addresses
>       returned in slaveProtocol(const QString& protocol, QStringList& proxy).
>     
>     * Change KProtocolManager::proxyFor to properly handle the changes in the new
>       proxy management dialog (KDE 4.8) where the proxy server port, in the
>       manual proxy configuration mode, will be saved separated from the address with
>       a white space.
>     
>     * Move the code that accounts for SOCKS proxy from KProtocolManager::proxyFor
>       to KProtocolManager::proxyForUrl where it belongs. The current implementation
>       only works correctly under one circumstance while breaking the previous behavior
>       of the function.
>     
>     * Fix KProtocoManager::proxiesForUrl so that it accounts for the proxy
>       exception list.
>     
>     * Update API documentation to reflect the changes above.
> 
> 
> Diffs
> -----
> 
>   kio/kio/kprotocolmanager.h 11e43fe 
>   kio/kio/kprotocolmanager.cpp 50ebb6e 
> 
> Diff: http://git.reviewboard.kde.org/r/102691/diff/diff
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Dawit Alemayehu
> 
>

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