[webkit-reviews] review granted: [Bug 82346] [GTK] Purge unused favicons from IconDatabase after 30 days : [Attachment 134095] Patch

[bugzilla-daemon at webkit.org]

Xan Lopez <xan.lopez at gmail.com> has granted Sergio Villar Senin
<svillar at igalia.com>'s request for review:
Bug 82346: [GTK] Purge unused favicons from IconDatabase after 30 days
https://bugs.webkit.org/show_bug.cgi?id=82346

Attachment 134095: Patch
https://bugs.webkit.org/attachment.cgi?id=134095&action=review

------- Additional Comments from Xan Lopez <xan.lopez at gmail.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=134095&action=review


This looks OK to me. If you want to move the 30day pruning to another
transaction feel free, but I don't think it's mandatory.

> Source/WebCore/loader/icon/IconDatabase.cpp:77
> +// them if they have not been used after that time even if they are

Nitpick, but I think this sentence is a bit confusing. I'd just write "We are
not interested in icons that have been unused for more than 30 days, delete
them even if they have not been explicitly released".

> Source/WebCore/loader/icon/IconDatabase.cpp:1724
> +    // Delete icons not used in the last 30 days

I'd add "even if they are not retained", and missing dot at the end.

> Source/WebCore/loader/icon/IconDatabase.cpp:1725
> +    String deleteQuery = String::format("DELETE FROM PageURL WHERE iconID IN
(SELECT iconID FROM IconInfo WHERE stamp <= %.0f AND stamp > 0);",
floor(currentTime() - notUsedIconExpirationTime));

So this just deletes the PageURL stuff and the actual icon data will be pruned
later in this same method. I think it would be worth making that clear, since
the comment says "Delete icons".