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

[bugzilla-daemon at webkit.org]

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
https://bugs.webkit.org/show_bug.cgi?id=71082

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

------- 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.setScheme(QStringLiteral("image"));
iconURL.setHost(QStringLiteral("webicon"));
iconURL.setEncodedPath(url.toEncoded());  // Note how we avoid an unecessary
url.toString() here!
iconURL.setFragment(QString::number(nextIconID());

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

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:

image://webicon/<id>/url-encoded-as-path

Then you could search for the slash:

QString imageId = ...;

ASSERT(imageId.startsWith("image://webicon/"));

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());