[webkit-reviews] review denied: [Bug 71082] [Qt][WK2] Implement favicon support : [Attachment 113256] Adds basic implementation for showing favicons on QML and cleaning up the icon database.

[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 113256: Adds basic implementation for showing favicons on QML and
cleaning up the icon database.
https://bugs.webkit.org/attachment.cgi?id=113256&action=review

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


This is just a first rough review, I didn't go much into details. I've added a
few comments on the implementation first. There's one thing that I think we
need to clarify first, design wise:

I like the idea that the favicons have their own "element" in QML and that they
can be associated with webviews (it doesn't have to be a desktop one, btw).
That gives me the impression that they're a much more reusable component.

As such the implementation however should not be a painted item. I'd rather see
a real QQuickItem that uses the C++ scene graph API to create a proper image
node.

I'm giving r- because this will need a few iterations, but I think conceptually
it's a good start (like I said, I really like the idea of a reusable qml
element that is bindable to a webview and updates automatically)

> Source/WebKit2/UIProcess/qt/QtWebIconDatabase.cpp:67
> +QPixmap QtWebIconDatabase::synchronousIconForPageURL(const QUrl& pageURL)
> +{
> +    String url = WebCore::KURL(pageURL).string();
> +    RefPtr<WebCore::Image> image =
m_context->iconDatabase()->imageForPageURL(url);
> +    if (!image)
> +	   return QPixmap();

Ok, we can't do this unfortunately :(. A synchronous icon lookup like this will
send a synchronous message to the web process.

Synchronous messages to the web process are a major PITA and have been a great
source of crashes during the development of the N9 browser, to the extend where
we had to make an effort to avoid them like the plague. So let's not repeat
this mistake :)

> Source/WebKit2/UIProcess/qt/QtWebIconDatabase.h:37
> +class QWEBKIT_EXPORT QtWebIconDatabase : public QObject {

I don't think we need to export this class, do we?