Review Request: favicon local cache

[David Faure]

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


Thanks for this improvement. I got bitten by the konsole/KUriFilter/FavIcon bug too, very annoying.
My attempt at fixing the situation was to make it possible to separate url resolving from getting the icon of the search provider, in KUriFilter, by making iconName() virtual.
Konsole only uses kurifilter to resolve urls, it doesn't care for the icons.
See 8394e75240a1ce57ae68b817a36be1bf642e7abf, but that's KF5 only, and of course kuriikwsfilter.cpp will need to be changed to make use of it, once kde-runtime uses KF5.


kdecore/services/kmimetype.cpp
<http://git.reviewboard.kde.org/r/107358/#comment17099>

    the explicit call to the default constructor doesn't really bring much to the table...



kdecore/services/kmimetype.cpp
<http://git.reviewboard.kde.org/r/107358/#comment17100>

    Double-lookup. Use this instead:
    
    const QString iconFromCache = m_iconNameCache.value(url.url());
    if (!iconFromCache.isEmpty()) {
        return iconFromCache;
    }
    
    Or if an empty value is valid in this cache (which I strongly suspect to be the case), pass a QString containing "NOTFOUND" as 2nd param to value() and compare with that on the line below.
    
    Or if that's considered too ugly, do it properly, with a const_iterator and find() :)



kdecore/services/kmimetype.cpp
<http://git.reviewboard.kde.org/r/107358/#comment17101>

    Of course this opens the whole issue of ... what if the favicon changes?
    E.g. you're a web developer. Or you're talking to a sysadmin who tells you "fixed!"
    
    OK, looking at the kded module, it also caches favicons, for one week, without any way to clean up the cache meanwhile. On the other hand, your QHash will get cleaned up by simply restarting the application (or the whole desktop, more likely, for the case of konqueror or konsole).
    
    So no issue here, OK for the cache.


- David Faure


On Nov. 17, 2012, 10:56 a.m., Jaime Torres Amate wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/107358/
> -----------------------------------------------------------
> 
> (Updated Nov. 17, 2012, 10:56 a.m.)
> 
> 
> Review request for kdelibs.
> 
> 
> Description
> -------
> 
> Does not fix the bugs (I'll not mark them as fixed), but makes them much harder to reproduce.
> Just use a static QHash most of the time instead of a blocking Dbus call.
> 
> 
> This addresses bugs 306338 and 307973.
>     http://bugs.kde.org/show_bug.cgi?id=306338
>     http://bugs.kde.org/show_bug.cgi?id=307973
> 
> 
> Diffs
> -----
> 
>   kdecore/services/kmimetype.cpp 955bf62 
> 
> Diff: http://git.reviewboard.kde.org/r/107358/diff/
> 
> 
> Testing
> -------
> 
> In my tests, there are more than 5 thousands requests for a favicon answered by the local cache vs. less than new 50 favicons inserted.
> 
> 
> Thanks,
> 
> Jaime Torres Amate
> 
>

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