[webkit-reviews] review denied: [Bug 71082] [Qt][WK2] Implement favicon support : [Attachment 118136] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Thu Dec 8 06:07:06 PST 2011

Simon Hausmann <hausmann at webkit.org> has denied Rafael Brandao
<rafael.lobo at openbossa.org>'s request for review:
Bug 71082: [Qt][WK2] Implement favicon support

Attachment 118136: Patch

------- Additional Comments from Simon Hausmann <hausmann at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=118136&action=review

> Source/WebKit2/UIProcess/qt/QtWebIconDatabaseClient.cpp:93
> +    QString iconUrlString = url.toString();
> +    QImage icon = iconImageForPageURL(iconUrlString);
> +    if (icon.isNull())
> +	   return;
> +
> +    static QString pattern = QLatin1String("image://webicon/%1/%2");
> +    QUrl iconURL(pattern.arg(iconUrlString).arg(nextIconID()));

This way of encoding the url into the icon url is wrong. "iconUrlString" may
contain characters that are not permitted as the path, such as ? or #.

I believe one correct way of doing that would be something along these lines:

QUrl iconURL;
iconURL.setEncodedPath(url.toEncoded());  // Note how we avoid an unecessary
url.toString() here!

Note that using this pattern and the QUrl API, you can avoid your own parsing

QUrl iconURL = ...
ASSERT(iconURL.scheme() == QStringLiteral("image"));
ASSERT(iconURL.host() == QStringLiteral("webicon"));

QUrl pageUrl = QUrl::fromEncoded(iconURL.encodedPath());

However this format isn't quiite as easy to decode if all you have is a unicode
string, as we get it from the image provider and would want to avoid
throwing QUrl's parser at it (avoid QUrl-from-QString creation as much as
possible). So perhaps a format like this would be better:


Then you could search for the slash:

QString imageId = ...;


imageId.remove(0, 16); // Remove leading scheme and host.
imageId.remove(0, id.indexOf('/') + 1); // Ignore & remove icon id.

QString pageURL = QUrl::fromPercentEncoding(id.toUtf8());

More information about the webkit-reviews mailing list