Review Request: Make NetworkAccessManagerProxy::getData honor redirects

[Martin Blumenstingl]

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



src/covermanager/CoverFetcher.cpp
<http://git.reviewboard.kde.org/r/100285/#comment501>

    This is basically untested (unfortunately).



src/network/NetworkAccessManagerProxy.cpp
<http://git.reviewboard.kde.org/r/100285/#comment502>

    Redirects to the initial URL are handled here.
    However, infinite (redirection) loops are not correctly handled yet, but do we need that?



src/scriptengine/AmarokNetworkScript.cpp
<http://git.reviewboard.kde.org/r/100285/#comment503>

    This is needed as the slot which gets the result looks up some data in a QHash.
    The key of the QHash is the URL, but since the URL changed (due to the redirect) we have to update the URL (= the key) in the QHash.


- Martin


On 2011-01-02 20:25:45, Martin Blumenstingl wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/100285/
> -----------------------------------------------------------
> 
> (Updated 2011-01-02 20:25:45)
> 
> 
> Review request for Amarok and Rick W. Chen.
> 
> 
> Summary
> -------
> 
> Currently NetworkAccessManagerProxy::getData() ignores redirects.
> Since redirects are required for some URLs (see the linked bug) it would be nice if Amarok's NetworkAccessManagerProxy would also support them.
> 
> Please have a look at my comments below, I'm trying to explain some of the issues there.
> 
> Here's a quick overview of my changes:
> -I implemented redirection support
> -Infinite loops are currently not handled properly, if page A redirects to itself my code will not do the redirect.
>  But my code does not handle "if page A redirects to page B which redirects to page A"-loops.
> -I tried to test the changes in CoverFetcher but I couldn't find anything that uses redirects there.
>  Thus those changes are basically "untested".
> 
> 
> This addresses bug 261839.
>     https://bugs.kde.org/show_bug.cgi?id=261839
> 
> 
> Diffs
> -----
> 
>   src/covermanager/CoverFetcher.h 9dc2f2e 
>   src/covermanager/CoverFetcher.cpp b21dd87 
>   src/network/NetworkAccessManagerProxy.h 045ded3 
>   src/network/NetworkAccessManagerProxy.cpp 9a0e763 
>   src/scriptengine/AmarokNetworkScript.h 342cc2a 
>   src/scriptengine/AmarokNetworkScript.cpp 1742f41 
> 
> Diff: http://git.reviewboard.kde.org/r/100285/diff
> 
> 
> Testing
> -------
> 
> I tested Sven's "Free Music Charts" script: it works fine now.
> Fetching lyrics through the "Ultimate Lyrics" script is also still working.
> 
> 
> Thanks,
> 
> Martin
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.kde.org/pipermail/amarok-devel/attachments/20110102/97bd4b75/attachment.htm