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

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Tue Dec 6 03:48:59 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 117976: Patch

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

Yeah, this feels like it's the right API. Let's iterate on the implementation

As a general comment, there are a few too many KURL <> QURL <> QString
conversions for my taste. Those can be expensive in terms of unnecessary url
parsing and copying, especially given that we generally do not seem to extract
components from the QUrl but just do the parsing ourselves(!) after convering
the QUrl back to a string.

The WK2 C API expects strings, the QML image provider seems to give us strings.
It's only our API layer that needs a QUrl, right? Perhaps there's a way to
stick with strings all the way until we convert to a QUrl in the "last minute"
when needed.

> Source/WebKit2/UIProcess/API/qt/qwebiconimageprovider.cpp:44
> +QImage QWebIconImageProvider::requestImage(const QString& id, QSize* size,
const QSize& requestedSize)
> +{
> +    RefPtr<QtWebContext> context = QtWebContext::defaultContext();
> +    QtWebIconDatabase* iconDatabase = context->iconDatabase();
> +    QUrl pageURL = QtWebIconDatabase::fromIconURLtoPageURL(id);

Note how the QML documentation says that requestImage can be called by multiple
threads. Is our code thread-safe?

> Source/WebKit2/UIProcess/qt/QtWebIconDatabase.cpp:44
> +    // FIXME: this setter doesn't only set the icon database path, but it
> +    // trigger the startup of WebContext's WebIconDatabase. The getter will
> +    // the default platform path and will start WebIconDatabase with it.

If it's a FIXME, perhaps you should also mention what needs to be fixed ;). Do
you _not_ want it to start up the database thread here?

> Source/WebKit2/UIProcess/qt/QtWebPageProxy.cpp:471
> +void QtWebPageProxy::onIconReadyForPageURL(const QUrl& pageURL, const QUrl&
> +{
> +    if (url() != pageURL)
> +	   return;
> +
> +    if (m_iconURL == iconURL)
> +	   return;
> +
> +    QtWebIconDatabase* iconDatabase = m_context->iconDatabase();
> +    QUrl oldPageURL =
> +    if (!oldPageURL.isEmpty())
> +	   iconDatabase->releaseIconForPageURL(oldPageURL);
> +    m_iconURL = iconURL;
> +    if (!pageURL.isEmpty())
> +	   iconDatabase->retainIconForPageURL(pageURL);
> +
> +    emit m_qmlWebView->iconChanged(iconURL);
> +}

We're trying to get rid of QtWebPageProxy. I'd prefer if we did _not_ add any
code to a class we're trying to get rid of :). QQuickWebViewPrivate and
Q_PRIVATE_SLOT in QQuickWebView should do the trick I think.

More information about the webkit-reviews mailing list