Review Request 114253: Port favicons dataengine to KF5

[David Faure]

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



plasma/generic/dataengines/favicons/faviconprovider.h
<http://git.reviewboard.kde.org/r/114253/#comment32263>

    why is this needed now, if it wasn't needed before?



plasma/generic/dataengines/favicons/faviconprovider.h
<http://git.reviewboard.kde.org/r/114253/#comment32267>

    This one is unnecessary for sure.
    class KJob; would be enough.



plasma/generic/dataengines/favicons/faviconprovider.cpp
<http://git.reviewboard.kde.org/r/114253/#comment32264>

    Are you sure that "url" is always a full URL here, and never a local path? KUrl(QString) could deal with local paths, QUrl(QString) can't. It would be much better to change the constructor to take a QUrl argument instead, for type safety.



plasma/generic/dataengines/favicons/faviconprovider.cpp
<http://git.reviewboard.kde.org/r/114253/#comment32265>

    missing '/' before favicons ->  make it "/favicons/"



plasma/generic/dataengines/favicons/faviconprovider.cpp
<http://git.reviewboard.kde.org/r/114253/#comment32266>

    This line is a noop. fromLocalFile is a static method.
    I think setPath was actually correct here, you're changing the path of an existing full URL.


- David Faure


On Dec. 3, 2013, 9:53 a.m., Andrea Scarpino wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/114253/
> -----------------------------------------------------------
> 
> (Updated Dec. 3, 2013, 9:53 a.m.)
> 
> 
> Review request for KDE Frameworks and Plasma.
> 
> 
> Repository: kde-workspace
> 
> 
> Description
> -------
> 
> = subject.
> 
> I replaced KUrl by QUrl and KStandardDirs by QStandardPaths. Please check if I replaced them correctly.
> 
> 
> Diffs
> -----
> 
>   plasma/generic/dataengines/CMakeLists.txt 3e94325 
>   plasma/generic/dataengines/favicons/CMakeLists.txt 4af0f14 
>   plasma/generic/dataengines/favicons/faviconprovider.h 1f370f9 
>   plasma/generic/dataengines/favicons/faviconprovider.cpp bc72a66 
>   plasma/generic/dataengines/favicons/favicons.h 79565bf 
>   plasma/generic/dataengines/favicons/favicons.cpp 2eae673 
> 
> Diff: http://git.reviewboard.kde.org/r/114253/diff/
> 
> 
> Testing
> -------
> 
> Builds.
> 
> 
> Thanks,
> 
> Andrea Scarpino
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/plasma-devel/attachments/20131204/8a41d02d/attachment.html>