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

[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 102763: changelog fixup
https://bugs.webkit.org/attachment.cgi?id=102763&action=review

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


> Source/WebKit/qt/Api/qgraphicswebview.cpp:212
> +    \note This signal may not be emitted if the icon database is disabled.
> +

_may_ not be emitted?
I do not like fuzzy documentation because it is a bit scary for the reader. One
might wonder if the signal is ever emitted, randomly emitted, etc. if the icon
database is not set.

> Source/WebKit/qt/tests/qwebframe/tst_qwebframe.cpp:587
> +	   if (entries[i].isDir())
> +	       removeRecursive(entries[i].filePath());
> +	   else
> +	       dir.remove(entries[i].fileName());

Good practice: entires[i] -> entries.at(i) to avoid copying stuff.

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

You do not use QLatin1String elsewhere, no need to put it here.

Please do not put this method inline. This should be a static function and not
a method. A method that does not use any attribute and use a static variable ->
something evil is going on :)

> Source/WebKit/qt/tests/qwebframe/tst_qwebframe.cpp:3717
> +    QImage oldIcon  = frame->icon().pixmap(iconSize).toImage();

2 space before the equal sign.

> Source/WebKit/qt/tests/qwebframe/tst_qwebframe.cpp:3718
> +    QImage image =
QImage(":/favicon1.png").convertToFormat(QImage::Format_ARGB32_Premultiplied);

Why do you convert the image to ARGB32_PM?

> Source/WebKit/qt/tests/qwebframe/tst_qwebframe.cpp:3723
> +    QString js =
QLatin1String("document.getElementById('favicon').setAttribute(\"href\",
\"favicon2.png\")");

Sometime you use QLatin1String, sometime not, better decide :)

> Source/WebKit/qt/tests/qwebframe/tst_qwebframe.cpp:3734
> +

Space