[webkit-reviews] review denied: [Bug 71082] [Qt][WK2] Implement favicon support : [Attachment 117976] 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 117976: Patch
https://bugs.webkit.org/attachment.cgi?id=117976&action=review

------- 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
also
> +    // trigger the startup of WebContext's WebIconDatabase. The getter will
return
> +    // 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&
iconURL)
> +{
> +    if (url() != pageURL)
> +	   return;
> +
> +    if (m_iconURL == iconURL)
> +	   return;
> +
> +    QtWebIconDatabase* iconDatabase = m_context->iconDatabase();
> +    QUrl oldPageURL =
QtWebIconDatabase::fromIconURLtoPageURL(m_iconURL.toString());
> +    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.