[webkit-reviews] review denied: [Bug 65237] [Qt] Favicons can't be changed dynamically. : [Attachment 103174] Patch

[bugzilla-daemon at webkit.org]

Benjamin Poulain <benjamin at webkit.org> has denied Pierre Rossi
<pierre.rossi at gmail.com>'s request for review:
Bug 65237: [Qt] Favicons can't be changed dynamically.
https://bugs.webkit.org/show_bug.cgi?id=65237

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

------- Additional Comments from Benjamin Poulain <benjamin at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=103174&action=review


Would it be possible to have a fake icon database to get the signal?
A icon database mockup that would just forward the right signals at the right
time?
Of modify the icon database to handle this kind of cases (which could be useful
if the disk is full for example).

> Source/WebKit/qt/Api/qgraphicswebview.cpp:213
> +    \note This signal will not be emitted again after the initial page load
> +    if the icon database is disabled.
> +

This is better, but I think we should do even better :)
This could be read as "you get the signal once for the first page ever being
loaded". While what you really want to communicate is: "the signal is not
emitted again if a loaded page change the icon".

> Source/WebKit/qt/tests/util.h:22
> +#include <QDateTime>
> +#include <QDir>

No need to remove those from tst_qdeclarativewebview.cpp?

> Source/WebKit/qt/tests/util.h:92
> +static QString tmpDirPath()
> +{
> +    static QString tmpd = QDir::tempPath() +
QLatin1String("/tst_qwebframe-")
> +	   +
QDateTime::currentDateTime().toString(QLatin1String("yyyyMMddhhmmss"));
> +    return tmpd;
> +}

Better put the implementation in a new util.cpp (like Jocelyn did for WebKit2
IIRC).
I guess this causes the other tests to have warnings on compilation because
static functions are defined but not used?