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/#review45174
-----------------------------------------------------------



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

    Hmm, this has simply moved the problem here.
    
    But now I see: this "identifier" actually comes from FaviconProvider in the first place, so it has full control over that string?
    
    In that case, having the conversion to QUrl should indeed be closer to the conversion from QUrl to QString, i.e. inside Provider, as it was originally.
    
    But I'm missing something, this is a chicken an egg situation. Who creates  this identifier initially, before the Provider is created in the first place?
    
    In other words, who calls sourceRequestEvent?
    Sorry I don't know anything about data engines - but I'm smelling a possible QUrl misuse somewhere here.
    



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

    unrelated, but this should most certainly be .isNull() instead of creating a temporary QImage just for comparison purposes.


- David Faure


On Dec. 5, 2013, 10:48 a.m., Andrea Scarpino wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/114253/
> -----------------------------------------------------------
> 
> (Updated Dec. 5, 2013, 10:48 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/kde-frameworks-devel/attachments/20131205/d825f552/attachment.html>