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

[bugzilla-daemon at webkit.org]

Simon Hausmann <hausmann at webkit.org> has granted 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 119700: Patch
https://bugs.webkit.org/attachment.cgi?id=119700&action=review

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


I'm going to say r+, this is rather close.

A few comments below that can be fixed in follow-up patches, or also in this
one if you want.

There is however one bigger design issue (that can also be solved later if you
want): We are breaking the association of WebViews with a context by always
using QWebContext::defaultContext(). In almost all of the cases in this patch
the use of defaultContext() is not entirely correct and the view specific
context should be used instead (which right now _happens_ to be the default
context, but it will change in the future).

I think we may have to keep track of the web contexts in a vector, encode the
index in the icon url and use that as a mechanism to get back the correct
context when the image provider needs the image for the icon url.

Am I missing something perhaps?

> Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:521
> +    String newPageURL = WebCore::KURL(q->url()).string();

This is a completely unnecessary url conversion. We call q->url(), which will
end up converting the main frame's url (string) to a QUrl. Then upon returning
here it'll be converted to a KURL and then back to a string. Each conversion
involves parsing the url, no reason. It would be better to directly access the
main frame's url string here.

> Source/WebKit2/UIProcess/API/qt/qwebiconimageprovider.cpp:34
> +

Stray newline?

> Source/WebKit2/UIProcess/API/qt/qwebiconimageprovider.cpp:39
> +

Stray newline?

>> Tools/MiniBrowser/qt/qml/BrowserWindow.qml:219

> 
> Can you just do -> source : webview.icon != '' ? webView.icon :
"../icons/favicon.png" so you have only one Image element?

I agree with Alexis here, using one image element is much better/easier.