Bug 71082 – [Qt][WK2] Implement favicon support

• Component: WebKit Qt
• Status: RESOLVED FIXED

[Rafael Brandao]

I've been working on a patch to implement basic support for favicons on Qt, and I'll upload it soon. We'd have to discuss how exactly we would like to expose it on QML.

[Rafael Brandao]

Created attachment 113256 [details]
Adds basic implementation for showing favicons on QML and cleaning up the icon database.

Currently a Favicon component needs to target a DesktopWebView so it can fetch the data for its url whenever it changes. The data is stored by WebIconDatabase, and it's using the default icon database path for now. I had to change the visibility of WebContext::iconDatabasePath so I could use it when I'm doing the setup for QtWebIconDatabase. I wonder if there's a way (probably there is) to use it without this change, but I couldn't think of anything more practical right now. Its setup is done when we create the single WebContext. It's destroyed when that WebContext is destroyed. Here's an example of usage on QML:

Favicon {
    id: favicon
    webView: pageWidget.webView
    width: 16
    height: 16
    MouseArea {
        anchors.fill: parent
        onClicked: favicon.removeAllIcons()
    }
}

[WebKit Review Bot]

Attachment 113256 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit/qt/ChangeLog', u'Source/WebK..." exit_code: 1

Source/WebKit2/UIProcess/qt/QtWebPageProxy.cpp:45:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebKit2/UIProcess/API/qt/qfavicon.cpp:87:  An else should appear on the same line as the preceding }  [whitespace/newline] [4]
Source/WebKit2/UIProcess/API/qt/qfavicon.cpp:112:  An else should appear on the same line as the preceding }  [whitespace/newline] [4]
Source/WebKit2/UIProcess/qt/QtWebIconDatabase.cpp:30:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebKit2/UIProcess/qt/QtWebIconDatabase.h:25:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebKit2/UIProcess/qt/QtWebIconDatabase.h:28:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 6 in 14 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Simon Hausmann]

Comment on attachment 113256 [details]
Adds basic implementation for showing favicons on QML and cleaning up the icon database.

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?

[Alexis Menard (darktears)]

Comment on attachment 113256 [details]
Adds basic implementation for showing favicons on QML and cleaning up the icon database.

View in context: https://bugs.webkit.org/attachment.cgi?id=113256&action=review

> Source/WebKit/qt/declarative/plugin.cpp:55
> +        qmlRegisterType<QFavicon>(uri, 3, 0, "Favicon");

The class name should be different in my opinion. QWebFavIcon or similar so we respect the naming convention. The QML name also should be different.

> Source/WebKit2/ChangeLog:6
> +        Handles favicon data with a singleton QtWebIconDatabase

The implementation handles...

> Source/WebKit2/ChangeLog:7
> +        which will be listened by QML components named Favicon.

QML component. FavIcon.

> Source/WebKit2/UIProcess/API/qt/qfavicon.cpp:30
> +QFavicon::QFavicon(QQuickItem* parent)

Whatever name in the future "i" should be upper case.

> Source/WebKit2/UIProcess/API/qt/qfavicon.h:45
> +    QDesktopWebView* webView() const { return m_webView; }

It needs to work for both views.

[Rafael Brandao]

(In reply to comment #3)
> (From update of attachment 113256 [details])
> 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.
> 

I'll change it to support both webviews.

> 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'll give it try.

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

No problem, and thanks for your review. :)

> 
> > 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 :)

The good thing about IconDatabase is that the database itself lies on UIProcess, so this lookup won't send any synchronous message to the WebProcess. Besides the name, IconDatabase works asynchronously: if you ask for an icon when its data is not ready yet, then the page url requesting its icon data will be marked as interested on it and will return an empty image on the "sync" function. When the data is ready we'll receive a signal about that. So we can finally request the actual data to display on UI. WebIconDatabase uses the function imageForPageURL and then it uses IconDatabase's synchronousIconForPageURL. Maybe I should rename that function on QtWebIconDatabase to imageForPageURL instead.
> 
> > 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?

We don't, thanks for pointing that out. :)

[Rafael Brandao]

(In reply to comment #4)
> (From update of attachment 113256 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=113256&action=review
> 
> > Source/WebKit/qt/declarative/plugin.cpp:55
> > +        qmlRegisterType<QFavicon>(uri, 3, 0, "Favicon");
> 
> The class name should be different in my opinion. QWebFavIcon or similar so we respect the naming convention. The QML name also should be different.
> 
> > Source/WebKit2/ChangeLog:6
> > +        Handles favicon data with a singleton QtWebIconDatabase
> 
> The implementation handles...
> 
> > Source/WebKit2/ChangeLog:7
> > +        which will be listened by QML components named Favicon.
> 
> QML component. FavIcon.
> 
> > Source/WebKit2/UIProcess/API/qt/qfavicon.cpp:30
> > +QFavicon::QFavicon(QQuickItem* parent)
> 
> Whatever name in the future "i" should be upper case.
> 
> > Source/WebKit2/UIProcess/API/qt/qfavicon.h:45
> > +    QDesktopWebView* webView() const { return m_webView; }
> 
> It needs to work for both views.

Thanks! I'll give more attention to the names this time. I like your suggestions. :)

[Rafael Brandao]

Created attachment 114991 [details]
Use a QDeclarativeImageProvider to display FavIcons instead of creating a new component

QtWebIconDatabase requests a favicon once the webview changes its url, and then it associates an id for the favicon requested. All pages that has the same url will share the same favicon, so they receive the same favicon id and sets its favicon source url with that id. That url will request the icon from QtWebFavIconImageProvider which will access QtWebIconDatabase's favicon storage to get the QPixmap.

[Caio Marcelo de Oliveira Filho]

Comment on attachment 114991 [details]
Use a QDeclarativeImageProvider to display FavIcons instead of creating a new component

View in context: https://bugs.webkit.org/attachment.cgi?id=114991&action=review

Rafael, there are a bunch of comments. I like that the final solution is smaller than we expected in the beginning, but I feel we need another iteration :-)

I think you could improve the patch ChangeLog by explaining that the icons are actually stored on the UIProcess and how this relates to the IconDatabase in the WebProcess. Understanding this big picture helps understanding the patch.

> Source/WebKit/qt/declarative/plugin.cpp:30
> +#include "qwebfaviconimageprovider.h"

Shouldn't be public API, so either use "qwebfaviconimageprovider_p.h" or move it to non-API directory if possible.

> Source/WebKit/qt/declarative/plugin.cpp:48
> +
> +#endif

Nit pick: swap these two lines.

> Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:358
> +        QObject::connect(q_ptr, SIGNAL(urlChanged(QUrl)), qtWebIconDatabase(), SLOT(requestIconIdForPageURL(QUrl)));

Nit pick: use q instead of q_ptr.

> Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:433
> +void QQuickWebView::requestIconIdForPageURL(const QUrl& pageURL)
> +{
> +    Q_D(const QQuickWebView);
> +    d->pageProxy->requestIconIdForPageURL(pageURL);
> +}

I think this shouldn't be exposed at API level. If other entities in the future want to expose different favicons (History Items), they can do by exposing a favIconSource like the WebView did.

> Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:439
> +void QQuickWebView::cleanupIconDatabase()
> +{
> +    Q_D(const QQuickWebView);
> +    d->pageProxy->cleanupIconDatabase();
> +}

I think this shouldn't be exposed at API level. At least not in QML. Specially considering that cleanupIconDatabase() would clean for all WebViews, which could be a bit surprising. Maybe a candidate to be in a future object that represents the webcontext in C++?

> Source/WebKit2/UIProcess/API/qt/qwebfaviconimageprovider.cpp:38
> +QPixmap QWebFavIconImageProvider::requestPixmap(const QString &id, QSize *size, const QSize &requestedSize)

Style.

> Source/WebKit2/UIProcess/API/qt/qwebfaviconimageprovider.cpp:44
> +        if (size)
> +            *size = m_defaultFavIcon.size();
> +        return m_defaultFavIcon;

What about replacing this with "icon = m_defaultFavIcon;"?

> Source/WebKit2/UIProcess/API/qt/qwebfaviconimageprovider.h:31
> +    QPixmap requestPixmap(const QString & id, QSize * size, const QSize & requestedSize);

Style.

> Source/WebKit2/UIProcess/WebContext.h:194
> +    String iconDatabasePath() const;

Could add comment why you need that? I think iconDatabasePath() is serving as a getter for the property set as well as a way to get the default value for the property, maybe you can improve this. (Relates to previous comment about setter before)

> Source/WebKit2/UIProcess/qt/QtViewInterface.h:88
> +    virtual void didChangeFavIconSource(const QUrl&);

Is this a fit for ViewInterface? I wonder if a SIGNAL in QtWebPageProxy would suffice -- specially because later you ask the QtWebPageProxy in the property getter.

> Source/WebKit2/UIProcess/qt/QtWebIconDatabase.cpp:36
> +QtWebIconDatabase* qtWebIconDatabase()
> +{
> +    return globalQtIconDatabase;
> +}

Not very important, but what about changing it to a static class function and storing the globalQtIconDatabase as a static class member?

> Source/WebKit2/UIProcess/qt/QtWebIconDatabase.cpp:49
> +    // FIXME: user should be able to set its own icon database path
> +    // rather than always using the default path.

You can remove this. This is not a concern right now, we don't support this level of customization for other things as well.

> Source/WebKit2/UIProcess/qt/QtWebIconDatabase.cpp:50
> +    m_context->setIconDatabasePath(m_context->iconDatabasePath());

I think this line deserve some explanation: looks like setting something to its current value. I know that setIconDatabasePath have a side effect that you want to trigger, and the default path is accessible by the "getter". Maybe you can even improve this in WebContext in another patch ;-)

> Source/WebKit2/UIProcess/qt/QtWebIconDatabase.cpp:61
> +    if (!m_context)
> +        return QPixmap();

I think this case will never happen with current code. Same applies for the next functions.

> Source/WebKit2/UIProcess/qt/QtWebIconDatabase.cpp:72
> +    QPixmap* nativeImage = image->nativeImageForCurrentFrame();
> +    if (!nativeImage)
> +        return QPixmap();
> +
> +    return *nativeImage;

When image is destroyed, will this new QPixmap still be valid? Or at this point we know more about the image's lifetime?

> Source/WebKit2/UIProcess/qt/QtWebIconDatabase.cpp:84
> +    m_pixmapForIconId[++m_lastIconId] = icon;

Maybe create a getNextAvailableIconId() function? I'll let Alexis complain about the overflow ;-)

> Source/WebKit2/UIProcess/qt/QtWebIconDatabase.cpp:98
> +    QMap<int, QPixmap>::iterator it = m_pixmapForIconId.find(id);

const_iterator?

> Source/WebKit2/UIProcess/qt/QtWebIconDatabase.cpp:108
> +    if (it != m_retainCountForIconId.constEnd()) {

end() instead of constEnd()?

> Source/WebKit2/UIProcess/qt/QtWebIconDatabase.cpp:109
> +        ++m_retainCountForIconId[id];

You can change the iterator value here, instead of looking up again.

> Source/WebKit2/UIProcess/qt/QtWebIconDatabase.cpp:118
> +    if (it == m_retainCountForIconId.constEnd())

end() instead of constEnd()?

> Source/WebKit2/UIProcess/qt/QtWebIconDatabase.cpp:127
> +    --m_retainCountForIconId[id];

Same as before, use iterator value instead of looking it up again.

> Source/WebKit2/UIProcess/qt/QtWebIconDatabase.h:59
> +    WebContext* m_context;

Maybe just save a reference to the IconDatabase is enough?

> Source/WebKit2/UIProcess/qt/QtWebPageProxy.cpp:88
> +        // FIXME: This is where the icondatabase's client is injected,
> +        // if there's no instance of it by now, then it's created.

I don't think you need this comment.

> Source/WebKit2/UIProcess/qt/QtWebPageProxy.cpp:177
> +            if (qtWebIconDatabase())
> +                delete qtWebIconDatabase();

You don't need the "if" here, just delete.

> Source/WebKit2/UIProcess/qt/QtWebPageProxy.cpp:943
> +void QtWebPageProxy::setFavIconId(int id)

Nit pick: updateFavIconId()? :)

> Source/WebKit2/UIProcess/qt/QtWebPageProxy.cpp:945
> +    if (!qtWebIconDatabase() || m_favIconId == id)

Do we need to check qtWebIconDatabase() here?

> Source/WebKit2/UIProcess/qt/QtWebPageProxy.h:271
> +    int m_favIconId;
> +    QUrl m_favIconSource;

It doesn't feel right that we need to keep two representations of the same data. Can the string be always generated? Usually this property will be queried once each time it changed (or twice if you count the value passed in the signal).

[Rafael Brandao]

Created attachment 115075 [details]
Addressing caio's comments on previous patch.

Thanks Caio for the review. I've had some trouble when I've changed the header/source files to "_p" suffix, so I have to include it differently now on plugin.cpp. I did the changes you proposed, let me know if there's something missing or needing improvement. On MiniBrowser, we only display the favicon for the current page (previously I had a click event there to show how to cleanup the database).

I've tried to explain a bit more on ChangeLog, hope it was clear enough and not too long. :)

[Rafael Brandao]

Created attachment 115088 [details]
Just a small change on ChangeLog.

I'd also want to notice that a few changes on this patch are there just to fix style issues (meaningless names, alphabetical order of includes, and so on).

[Alexis Menard (darktears)]

Comment on attachment 114991 [details]
Use a QDeclarativeImageProvider to display FavIcons instead of creating a new component

View in context: https://bugs.webkit.org/attachment.cgi?id=114991&action=review

> Source/WebKit/qt/declarative/plugin.cpp:45
> +        engine->addImageProvider(QLatin1String("favicon"), new QWebFavIconImageProvider);

The documentation doesn't say anything (which you should probably fix) but what about the ownership of the provider?

> Source/WebKit2/UIProcess/API/qt/qquickwebview.h:58
> +    Q_PROPERTY(QUrl favIconSource READ favIconSource NOTIFY favIconSourceChanged)

FINAL missing we want to get the optimizations of QML and we know that this function is not meant to be overridden by a subclass.

> Source/WebKit2/UIProcess/API/qt/qwebfaviconimageprovider.cpp:30
> +    m_defaultFavIcon.fill(QColor(0, 0, 0, 0));

What about Qt::transparent?

>> Source/WebKit2/UIProcess/qt/QtWebIconDatabase.cpp:108
>> +    if (it != m_retainCountForIconId.constEnd()) {
> 
> end() instead of constEnd()?

and you can move the end() call outside the loop as you don't change the size of the container.

>> Source/WebKit2/UIProcess/qt/QtWebIconDatabase.cpp:127
>> +    --m_retainCountForIconId[id];
> 
> Same as before, use iterator value instead of looking it up again.

I feel this entire block weird code. Is the map the best approach? What you want here is a basic ref counting and when the last ref is released then the thing goes away.

> Source/WebKit2/UIProcess/qt/QtWebIconDatabase.h:62
> +    int m_lastIconId;

This may overflow. Think about embedded system where the system is never restarted. Could be improved in a separate patch but still need to be fixed to me. We have plenty of war stories in Qt where this happened. Not sure about the WebKit code base in general...

[Rafael Brandao]

(In reply to comment #11)
> (From update of attachment 114991 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=114991&action=review
> 
> > Source/WebKit/qt/declarative/plugin.cpp:45
> > +        engine->addImageProvider(QLatin1String("favicon"), new QWebFavIconImageProvider);
> 
> The documentation doesn't say anything (which you should probably fix) but what about the ownership of the provider?

Actually it does: http://doc.qt.nokia.com/5.0-snapshot/qdeclarativeengine.html#addImageProvider. The QDeclarativeEngine takes the ownership.

> 
> > Source/WebKit2/UIProcess/API/qt/qquickwebview.h:58
> > +    Q_PROPERTY(QUrl favIconSource READ favIconSource NOTIFY favIconSourceChanged)
> 
> FINAL missing we want to get the optimizations of QML and we know that this function is not meant to be overridden by a subclass.

Will do that.

> 
> > Source/WebKit2/UIProcess/API/qt/qwebfaviconimageprovider.cpp:30
> > +    m_defaultFavIcon.fill(QColor(0, 0, 0, 0));
> 
> What about Qt::transparent?

Sounds good. :)

> 
> >> Source/WebKit2/UIProcess/qt/QtWebIconDatabase.cpp:108
> >> +    if (it != m_retainCountForIconId.constEnd()) {
> > 
> > end() instead of constEnd()?
> 
> and you can move the end() call outside the loop as you don't change the size of the container.

I don't understand what loop you're talking about.

> 
> >> Source/WebKit2/UIProcess/qt/QtWebIconDatabase.cpp:127
> >> +    --m_retainCountForIconId[id];
> > 
> > Same as before, use iterator value instead of looking it up again.
> 
> I feel this entire block weird code. Is the map the best approach? What you want here is a basic ref counting and when the last ref is released then the thing goes away.

As I'm not reusing icon ids so I may get a long value for it eventually. But on the way there might be many holes (ids that has been discarded). The QMap will abstract this for now, but we could improve it to use direct access in an array once we get the icon ids generated in a better way.

> 
> > Source/WebKit2/UIProcess/qt/QtWebIconDatabase.h:62
> > +    int m_lastIconId;
> 
> This may overflow. Think about embedded system where the system is never restarted. Could be improved in a separate patch but still need to be fixed to me. We have plenty of war stories in Qt where this happened. Not sure about the WebKit code base in general...

I see. This will be done in a following patch.

[Alexis Menard (darktears)]

Comment on attachment 114991 [details]
Use a QDeclarativeImageProvider to display FavIcons instead of creating a new component

View in context: https://bugs.webkit.org/attachment.cgi?id=114991&action=review

>>>> Source/WebKit2/UIProcess/qt/QtWebIconDatabase.cpp:108
>>>> +    if (it != m_retainCountForIconId.constEnd()) {
>>> 
>>> end() instead of constEnd()?
>> 
>> and you can move the end() call outside the loop as you don't change the size of the container.
> 
> I don't understand what loop you're talking about.

Yes I thought the if was a "for". I smoked too much...

[Jesus Sanchez-Palencia]

Comment on attachment 115088 [details]
Just a small change on ChangeLog.

View in context: https://bugs.webkit.org/attachment.cgi?id=115088&action=review

Nitpick mode on :)

> Source/WebKit/qt/declarative/plugin.cpp:23
> +#include <QtDeclarative/qdeclarativeengine.h>

if we are only using this include when HAVE_WEBKIT2 is defined, perhaps we should move this include in there as well?

> Source/WebKit2/ChangeLog:6
> +        QtWebIconDatabase was created to handle FavIcon requests and store

"(...) and stores them (...)" or "(...) and to store them (...)" ?!

> Source/WebKit2/UIProcess/API/qt/qwebfaviconimageprovider_p.cpp:38
> +QPixmap QWebFavIconImageProvider::requestPixmap(const QString& id, QSize* size, const QSize&)

Does our style agree with leaving this last QSize without the var name in a cpp?

> Source/WebKit2/UIProcess/qt/ClientImpl.cpp:181
> +    // FIXME: implement the callback for this event.

Why adding this callback now? You could just leave it for the next patch, right?

> Source/WebKit2/UIProcess/qt/ClientImpl.cpp:382
> +    iconDatabaseClient.didRemoveAllIcons = qt_wk_iconDatabaseDidRemoveAllIcons;

Ditto.

> Source/WebKit2/UIProcess/qt/QtWebIconDatabase.cpp:31
> +QtWebIconDatabase* QtWebIconDatabase::m_instance = 0;

change it to follow the s_ naming pattern, perhaps?



The rest we have talked live, so everything else LGTM!

[Alexis Menard (darktears)]

Comment on attachment 115088 [details]
Just a small change on ChangeLog.

View in context: https://bugs.webkit.org/attachment.cgi?id=115088&action=review

>> Source/WebKit2/UIProcess/API/qt/qwebfaviconimageprovider_p.cpp:38
>> +QPixmap QWebFavIconImageProvider::requestPixmap(const QString& id, QSize* size, const QSize&)
> 
> Does our style agree with leaving this last QSize without the var name in a cpp?

Seems to be a common practice.

[Jesus Sanchez-Palencia]

Comment on attachment 115088 [details]
Just a small change on ChangeLog.

View in context: https://bugs.webkit.org/attachment.cgi?id=115088&action=review

> Source/WebKit2/UIProcess/qt/QtWebIconDatabase.h:62
> +    WebIconDatabase* m_iconDatabase;

I would protect this in a RefPtr and make the init() become create().

> Source/WebKit2/UIProcess/qt/QtWebPageProxy.cpp:173
> +            delete QtWebIconDatabase::instance();

I'm almost sure you will need to ensure that QtWebIconDatabase::m_instance = 0 as well. So, again, maybe you could protect it in a RefPtr and then call clear() here? Whatever you like!

[Rafael Brandao]

Created attachment 115427 [details]
Addressing some of Jesus' concerns and added QML test to check if FavIcon load is done

[Rafael Brandao]

Created attachment 115463 [details]
Patch

[Rafael Brandao]

Could anyone review this? At this point this need a new rebase, but I'm more worried about the concept itself... are we ok about it (the image provider solution)?

I forgot to say, but in the last patch I've added a qmltest for the favicon that checks for the favicon's image size once it changes.

[Simon Hausmann]

Comment on attachment 115463 [details]
Patch

I like the general concept. I think however that the property in WebView should just be called "icon".

The favourite icon is such a basic concept that we probably should keep it straight in the WebView instead of having it in experimental first. Tor Arne, what do you think?

Conceptually the icon urls should continue to work when the WebView is destroyed or showing a different url. This may make the current id based storage unsuitable.

[Simon Hausmann]

Hang on, maybe I'm totally missing something here (it's kind of late and it's been a long week, to bear with me :), but what about this:

The icon database in its C API is all based on the idea that the _key_ to an icon is the page url. So if you'd like to store favourite icons, you need to simply store the urls (and we need to make sure to retain them?). So what again was speaking against having a WebIcon alike element again?

WebIcon { // Is actually a QML Image element
    id: myIcon
    source: myWebView.url;
}

If you'd like to "store" the icon, well, you simply store the page url.

[Rafael Brandao]

(In reply to comment #21)
> Hang on, maybe I'm totally missing something here (it's kind of late and it's been a long week, to bear with me :), but what about this:

Thank you for your time and comments. :)

> 
> The icon database in its C API is all based on the idea that the _key_ to an icon is the page url. So if you'd like to store favourite icons, you need to simply store the urls (and we need to make sure to retain them?). So what again was speaking against having a WebIcon alike element again?
> 
> WebIcon { // Is actually a QML Image element
>     id: myIcon
>     source: myWebView.url;
> }
> 
> If you'd like to "store" the icon, well, you simply store the page url.

Does this mean that we should use private headers from QtDeclarative? I'd rather try to implement this on top of QQuickImage, so I probably could force an update whenever I wanted and could ignore the use of ids for FavIcons (their purpose was just to create something new to have as source for the image that would force it to be updated)... and could even get rid of the QDeclarativeImageProvider.

The only reason I couldn't just use the page urls was this limitation on image's API to request a refresh on it.

[Rafael Brandao]

(In reply to comment #22)
> (In reply to comment #21)
> > Hang on, maybe I'm totally missing something here (it's kind of late and it's been a long week, to bear with me :), but what about this:
> 
> Thank you for your time and comments. :)
> 
> > 
> > The icon database in its C API is all based on the idea that the _key_ to an icon is the page url. So if you'd like to store favourite icons, you need to simply store the urls (and we need to make sure to retain them?). So what again was speaking against having a WebIcon alike element again?
> > 
> > WebIcon { // Is actually a QML Image element
> >     id: myIcon
> >     source: myWebView.url;
> > }
> > 
> > If you'd like to "store" the icon, well, you simply store the page url.
> 
> Does this mean that we should use private headers from QtDeclarative? I'd rather try to implement this on top of QQuickImage, so I probably could force an update whenever I wanted and could ignore the use of ids for FavIcons (their purpose was just to create something new to have as source for the image that would force it to be updated)... and could even get rid of the QDeclarativeImageProvider.
> 
> The only reason I couldn't just use the page urls was this limitation on image's API to request a refresh on it.

Actually the image provider will still be necessary, but there's no problem to keep it. I'm planning to add QtDeclarative private headers to try this idea.

[Tor Arne Vestbø]

(In reply to comment #21)
> Hang on, maybe I'm totally missing something here (it's kind of late and it's been a long week, to bear with me :), but what about this:
> 
> The icon database in its C API is all based on the idea that the _key_ to an icon is the page url. So if you'd like to store favourite icons, you need to simply store the urls (and we need to make sure to retain them?). So what again was speaking against having a WebIcon alike element again?
> 
> WebIcon { // Is actually a QML Image element
>     id: myIcon
>     source: myWebView.url;
> }
> 
> If you'd like to "store" the icon, well, you simply store the page url.

I'll look at this more closely on Monday, but my gut feeling is that we should not need a new QML item type, a regular Image should be able to point its source property to the webview:

Image {
    source: webView.icon
}

Similarly, the navigation history items should expose an icon, so if you're visualizing the navigation history you'll have something like:

ListView {
   model: webView.navigationHistory.backItems
   delegate: Component {
       Image {
           source: model.icon
       }
}

Also, the spec [1] says the UA should choose the most appropriate icon, but we might have to think how we would support icons of other sizes, eg

<link rel=icon href=iphone.png sizes="57x57" type="image/png">
  
[1] http://www.w3.org/TR/html5/links.html#rel-icon

[Rafael Brandao]

(In reply to comment #24)
> (In reply to comment #21)
> > Hang on, maybe I'm totally missing something here (it's kind of late and it's been a long week, to bear with me :), but what about this:
> > 
> > The icon database in its C API is all based on the idea that the _key_ to an icon is the page url. So if you'd like to store favourite icons, you need to simply store the urls (and we need to make sure to retain them?). So what again was speaking against having a WebIcon alike element again?
> > 
> > WebIcon { // Is actually a QML Image element
> >     id: myIcon
> >     source: myWebView.url;
> > }
> > 
> > If you'd like to "store" the icon, well, you simply store the page url.
> 
> I'll look at this more closely on Monday, but my gut feeling is that we should not need a new QML item type, a regular Image should be able to point its source property to the webview:
> 
> Image {
>     source: webView.icon
> }

I like the idea of not creating a new element as well. The only problem is that we need to create unique source urls in order to force everything that relies on it to be updated (imagine a favicon that has changed dynamically for example). This idea was already implemented in the last patch.

In the other hand, if we create an element that derives from QQuickImage, it would probably be possible to take control of this update once we receive the signal that the icon has changed for some page url, but we would need to use private headers of qtdeclarative, and as I didn't see it anywhere on webkit code I think this may not be a good idea.

I would stick with the first option for now, and then we could think of improvements in the future.

> 
> Similarly, the navigation history items should expose an icon, so if you're visualizing the navigation history you'll have something like:
> 
> ListView {
>    model: webView.navigationHistory.backItems
>    delegate: Component {
>        Image {
>            source: model.icon
>        }
> }
> 
> Also, the spec [1] says the UA should choose the most appropriate icon, but we might have to think how we would support icons of other sizes, eg
> 
> <link rel=icon href=iphone.png sizes="57x57" type="image/png">
> 
> [1] http://www.w3.org/TR/html5/links.html#rel-icon

There's currently no support for it on webkit itself, as we only keep one icon for page url for now. Maybe we should use the requested size parameter on image provider's function as entry-point for this, but we wouldn't support it right now.

[Tor Arne Vestbø]

(In reply to comment #25)
> I like the idea of not creating a new element as well. The only problem is that we need to create unique source urls in order to force everything that relies on it to be updated (imagine a favicon that has changed dynamically for example). This idea was already implemented in the last patch.

Aren't the urls of favicons uniqe anwyays? If you do a dynamic favicon it would have different urls, either to static images or data-urls? And if the url of the page's favicon changes, then the webView.icon property will change, and trigger a reload of any Image that has its source property bound to the icon property. Why do we need a derived QQuickImage for this behavior?

[Rafael Brandao]

(In reply to comment #26)
> (In reply to comment #25)
> > I like the idea of not creating a new element as well. The only problem is that we need to create unique source urls in order to force everything that relies on it to be updated (imagine a favicon that has changed dynamically for example). This idea was already implemented in the last patch.
> 
> Aren't the urls of favicons uniqe anwyays? If you do a dynamic favicon it would have different urls, either to static images or data-urls? And if the url of the page's favicon changes, then the webView.icon property will change, and trigger a reload of any Image that has its source property bound to the icon property. Why do we need a derived QQuickImage for this behavior?

It's because we map page urls to favicon urls. We don't use the favicon url itself to get the image from the database. So if a page change its own favicon, its url won't change (only the favicon url), and then the image source url wouldn't change as well, unless we do some kind of trick like generating icon ids.

The other point that is also tricky is that when the image gets its image source with, let`s say, the favicon url and somehow try to get the image for it, the icon database may not have that favicon ready... so it would just return an empty QPixmap. But, at some point later, the QPixmap will be ready for use, we just need to request it again... but in this case, we wouldn't be able to refresh the image, as the favicon url didn't change, so what we could do to trigger a reload to everything that is associated to this favicon url? If we create an element that is actually a QQuickImage, we can do this... we can force it to update when we need.

[Tor Arne Vestbø]

(In reply to comment #27)
> (In reply to comment #26)
> > (In reply to comment #25)
> > > I like the idea of not creating a new element as well. The only problem is that we need to create unique source urls in order to force everything that relies on it to be updated (imagine a favicon that has changed dynamically for example). This idea was already implemented in the last patch.
> > 
> > Aren't the urls of favicons uniqe anwyays? If you do a dynamic favicon it would have different urls, either to static images or data-urls? And if the url of the page's favicon changes, then the webView.icon property will change, and trigger a reload of any Image that has its source property bound to the icon property. Why do we need a derived QQuickImage for this behavior?
> 
> It's because we map page urls to favicon urls. We don't use the favicon url itself to get the image from the database. So if a page change its own favicon, its url won't change (only the favicon url), and then the image source url wouldn't change as well, unless we do some kind of trick like generating icon ids.

The assumption about a page having only one favicon is limiting us in this case. Sounds like we should fix the mapping part then? 

> The other point that is also tricky is that when the image gets its image source with, let`s say, the favicon url and somehow try to get the image for it, the icon database may not have that favicon ready... so it would just return an empty QPixmap. But, at some point later, the QPixmap will be ready for use, we just need to request it again... but in this case, we wouldn't be able to refresh the image, as the favicon url didn't change, so what we could do to trigger a reload to everything that is associated to this favicon url? If we create an element that is actually a QQuickImage, we can do this... we can force it to update when we need.

Our image provider should provide QImages, and block until the image is ready in the database. If the Image needs async loading it can then set asynchronous: true.

Additionally, we can support icons of various sizes, eg touch icons, by respecting the requestedSize argument to the image provider, which maps to the sourceSize property of the image:

Image {
   id: appleTouchIcon
   source: webView.icon
   sourceSize.width: 57
   sourceSize.height: 57
}

[Rafael Brandao]

Created attachment 117261 [details]
Patch

[Tor Arne Vestbø]

Comment on attachment 117261 [details]
Patch

Let's clarify how we want to solve this first.

[Rafael Brandao]

> > It's because we map page urls to favicon urls. We don't use the favicon url itself to get the image from the database. So if a page change its own favicon, its url won't change (only the favicon url), and then the image source url wouldn't change as well, unless we do some kind of trick like generating icon ids.
> 
> The assumption about a page having only one favicon is limiting us in this case. Sounds like we should fix the mapping part then?
>

It is also strange the concept of using the icon url directly. What about bookmarks and history? We conceptually should just keep the page urls for them, not the favicon urls as well. The purpose of the favicons is to be the visual representation of a page and the approach that we already have on WebCore reflects this. What I like about Simon idea is how easily it fits inside the use case present on IconDatabase. WebIcon is basically just a client for it. For all browser-like apps that I have in mind, the usage of WebIcon handles them well.

> > The other point that is also tricky is that when the image gets its image source with, let`s say, the favicon url and somehow try to get the image for it, the icon database may not have that favicon ready... so it would just return an empty QPixmap. But, at some point later, the QPixmap will be ready for use, we just need to request it again... but in this case, we wouldn't be able to refresh the image, as the favicon url didn't change, so what we could do to trigger a reload to everything that is associated to this favicon url? If we create an element that is actually a QQuickImage, we can do this... we can force it to update when we need.
> 
> Our image provider should provide QImages, and block until the image is ready in the database. If the Image needs async loading it can then set asynchronous: true.
> 
> Additionally, we can support icons of various sizes, eg touch icons, by respecting the requestedSize argument to the image provider, which maps to the sourceSize property of the image:
> 
> Image {
>    id: appleTouchIcon
>    source: webView.icon
>    sourceSize.width: 57
>    sourceSize.height: 57
> }

The same trick can be used in WebIcon.

[Tor Arne Vestbø]

Okey, so we had a discussion about this yesterday and came to the following conclusion. Assuming you have the following:

WebView {
    id: webView
}

Image {
    source:  webView.icon
}

We want to support the use-case of dynamic favicons (changing the DOM to change the icon, eg [1]). This means the image item must be notified that the icon changed. The proposed solution of a custom QML element with a side-chain update-signal is not necessary, as QML already has this feature built in: bindings. When the source of the image is bound to the icon property of the webView, any changes to the icon property will update the image. 

This means that when an favicon changes, the webView url needs to change as well. One way of achieving this would be to reflect the real favicon url in the webView.icon url, eg: http://foo.com/icon.png, and the  update it to http://foo.com/icon2.png, etc. This has limitations such as:

 - Not loading the resource through the same QNAM as the web process
 - Not sharing authentication/authorization to resources
 - Not sharing accepted certificates
 - No easy way to expose icons of multiple sized in the api without having multiple properties or a list of icons

We can remedy the first three of these by instead exposing a url to our own image provider:

 image://webicon/______identifier_here_____

Where the identifier is just an encoded version of the icon url. But, that leaves the size variations:

IWebView {
    id: webView
}

Image {
    id: favIcon
    source:  webView.icon
}

Image {
    id: touchIcon
    source:  webView.icon
    sourceSize.width: 57
    sourceSize.height: 57
}

In this case we don't want the webView.icon property to be the encoded icon-url, since the final icon-url could potentially be different for the two images. This points us back to the page url, since that's what's shared between the two.

Incidentally this is also the key to an icon in the icon database (which lives in the UI process), but we're then left with solving the dynamic icon case. We can solve this by appending an incremental id to the encoded page url every time we get an update from the icon database that icons for a page has changed. So the final url is:

 image://webicon/<encoded page url>/<incremental id>

Ideally this should give us:

  - Loading of icons done by the web process (shared certs, etc)
  - Potential to support multiple sizes
  - Allows updates to the images through the basic binding update mechanism of QML (no extra item needed).

The same url would be reflected in the navigationHistory items, if you want to visualize the back-forward history. And if you're a browser and want to serialize the browsing history you'll hook up to loadFinished and record the url and icon url, potentially also fetching the icon data and serializing that too, for faster icon lookups later on.

Does that make sense?


[1] http://ajaxify.com/run/favicon/

[Rafael Brandao]

(In reply to comment #32)
> Okey, so we had a discussion about this yesterday and came to the following conclusion. Assuming you have the following:
> 
> WebView {
>     id: webView
> }
> 
> Image {
>     source:  webView.icon
> }
> 
> We want to support the use-case of dynamic favicons (changing the DOM to change the icon, eg [1]). This means the image item must be notified that the icon changed. The proposed solution of a custom QML element with a side-chain update-signal is not necessary, as QML already has this feature built in: bindings. When the source of the image is bound to the icon property of the webView, any changes to the icon property will update the image. 
> 
> This means that when an favicon changes, the webView url needs to change as well. One way of achieving this would be to reflect the real favicon url in the webView.icon url, eg: http://foo.com/icon.png, and the  update it to http://foo.com/icon2.png, etc. This has limitations such as:
> 
>  - Not loading the resource through the same QNAM as the web process
>  - Not sharing authentication/authorization to resources
>  - Not sharing accepted certificates
>  - No easy way to expose icons of multiple sized in the api without having multiple properties or a list of icons
> 
> We can remedy the first three of these by instead exposing a url to our own image provider:
> 
>  image://webicon/______identifier_here_____
> 
> Where the identifier is just an encoded version of the icon url. But, that leaves the size variations:
> 
> IWebView {
>     id: webView
> }
> 
> Image {
>     id: favIcon
>     source:  webView.icon
> }
> 
> Image {
>     id: touchIcon
>     source:  webView.icon
>     sourceSize.width: 57
>     sourceSize.height: 57
> }
> 
> In this case we don't want the webView.icon property to be the encoded icon-url, since the final icon-url could potentially be different for the two images. This points us back to the page url, since that's what's shared between the two.
> 
> Incidentally this is also the key to an icon in the icon database (which lives in the UI process), but we're then left with solving the dynamic icon case. We can solve this by appending an incremental id to the encoded page url every time we get an update from the icon database that icons for a page has changed. So the final url is:
> 
>  image://webicon/<encoded page url>/<incremental id>
> 
> Ideally this should give us:
> 
>   - Loading of icons done by the web process (shared certs, etc)
>   - Potential to support multiple sizes
>   - Allows updates to the images through the basic binding update mechanism of QML (no extra item needed).
> 
> The same url would be reflected in the navigationHistory items, if you want to visualize the back-forward history. And if you're a browser and want to serialize the browsing history you'll hook up to loadFinished and record the url and icon url, potentially also fetching the icon data and serializing that too, for faster icon lookups later on.
> 
> Does that make sense?
> 
> 
> [1] http://ajaxify.com/run/favicon/

Makes sense. :)

But I'd like to clarify some points:

* Should we store different images for different incremental ids for the same page url? When I've read this, I've imagined that we could just discard the id in the end as we are only interested in the page url itself to access the icon database and fetch the icon (and the requested size). So once we get the full url on the image provider, it would just use the page url and ignore the incremental id. But later when we restart the app, we could simply restart the ids as well. And for bookmark/history, we wouldn't even need to keep the icon url as it could be easily generated later based on its page url.

- I think that if both images points to the same url but one of them has a different requested size, and both images have caching enabled, the image provider will get only one request (so we don't know if it will get the one with the requested size or not), so in the end, wouldn't both images be the same?

- As browsers, aren't we happy enough with the serialization done by the icon database? I mean, do we really need to keep another reference to icons?

I'll try to get earlier this Monday on office (a few hours from now) so we can talk on IRC. Thanks!

[Rafael Brandao]

Created attachment 117976 [details]
Patch

[Rafael Brandao]

(In reply to comment #34)
> Created an attachment (id=117976) [details]
> Patch

Let me know what you think of this version. I've added a favicon on MiniBrowser so you can see how it can be used. My concern about disabling cache was already addressed by the image provider... it make two different requests for it even when the cache is enabled. But if you ask twice for the same id and same requestedSize, then it applies the cache... cool. :)

[Simon Hausmann]

Comment on attachment 117976 [details]
Patch

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.

[Tor Arne Vestbø]

Comment on attachment 117976 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=117976&action=review

> Source/WebKit2/UIProcess/qt/QtWebIconDatabase.cpp:90
> +    QUrl iconURL = QUrl(pattern.arg(url.toString()).arg(nextIconID()));

Should we percent/url-encode the url?

[Rafael Brandao]

(In reply to comment #36)
> (From update of attachment 117976 [details])
> 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.

Good point. I'll fix that.

> 
> > 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?

We are actually using IconDatabase::synchronousIconForPageURL that I know to be thread-safe. The other stuff are just access to pointers or string manipulation. I believe this make it thread-safe as well, right?

> 
> > 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?

Oh I get it. What I actually meant is that it looks weird that I use a set to its get value... it looks wrong. But I actually want it to startup the database right there, and this set is the one who does the trick. :)

> 
> > 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.

Oh, my mistake. Will move this out, sorry.

[Rafael Brandao]

> > > 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?
> 
> We are actually using IconDatabase::synchronousIconForPageURL that I know to be thread-safe. The other stuff are just access to pointers or string manipulation. I believe this make it thread-safe as well, right?
> 

Maybe I have misunderstand the concept of thread-safety, but if you mean you want them all to work _at the same time_, then the answer is no. :-( IconDatabase's synchronousIconForPageURL is protected by mutex lockers and it was meant to allow only one thread per time.

There's also another concern that is commented in the end of that IconDatabase function... it starts with "PARANOID DISCUSSION" and it says that we can't control when the Image we get from IconDatabase will be disposed from memory, then there's a chance that we don't even have a chance to transform it to our client-specific representation (e.g convert to QImage). This was always been the case, but I believe that when we play with many threads this becomes more risky. :-/

Should we stick with a image provider that uses a QPixmap for now until we figure out a good way to do this on IconDatabase, or try it out with a QImage and see how it goes? Or another suggestion?

[Simon Hausmann]

(In reply to comment #39)
> > > > 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?
> > 
> > We are actually using IconDatabase::synchronousIconForPageURL that I know to be thread-safe. The other stuff are just access to pointers or string manipulation. I believe this make it thread-safe as well, right?
> > 
> 
> Maybe I have misunderstand the concept of thread-safety, but if you mean you want them all to work _at the same time_, then the answer is no. :-( IconDatabase's synchronousIconForPageURL is protected by mutex lockers and it was meant to allow only one thread per time.

I don't think the calls have to work at the same time. I just glanced at the code and I think you're right, this code is probably safe to call.
 
> There's also another concern that is commented in the end of that IconDatabase function... it starts with "PARANOID DISCUSSION" and it says that we can't control when the Image we get from IconDatabase will be disposed from memory, then there's a chance that we don't even have a chance to transform it to our client-specific representation (e.g convert to QImage). This was always been the case, but I believe that when we play with many threads this becomes more risky. :-/

I suppose what we could do here to avoid taking any risk is to use the fact that we have only one single caller site for this function (the code you're adding), and there we _could_ add another protecting mutex that is released when we successfully created our QImage.
 
> Should we stick with a image provider that uses a QPixmap for now until we figure out a good way to do this on IconDatabase, or try it out with a QImage and see how it goes? Or another suggestion?

No, I think we should use QImage only. In Qt 5 QPixmap is just a QImage.

[Rafael Brandao]

Created attachment 118136 [details]
Patch

[Rafael Brandao]

The fixes and proposals as we've talked on IRC today. I'd be happy to add tests in a following patch. I already have it in a previous patch, but it has been a pain to keep all of this rebased. In any case, there's MiniBrowser's favicon as example.

It was more complicated than I've thought to avoid many conversions between QUrl <-> QString <-> String, then I've figured out which ones would be more costly if they were not of a certain type and then I've kept them like that. Of course at some points I still have to convert but I believe it's far less than before. I've also decided to convert directly to String in many cases, instead of QUrl -> QString -> String as before.

[Simon Hausmann]

Comment on attachment 118136 [details]
Patch

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

[Tor Arne Vestbø]

Comment on attachment 118136 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=118136&action=review

>> Source/WebKit2/UIProcess/qt/QtWebIconDatabaseClient.cpp:93
>> +    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());

Come to think of it, the id should probably be a hash of the real icon url (you can use String::hash()). Otherwise the QML image cache will treat every change to the url as a new image and cache that, even if you're just switching between two icons (urls). We could also use QImage's key/hash, but that's mor expensive, so the url hash is a compromise.

[Rafael Brandao]

(In reply to comment #44)
> 
> Come to think of it, the id should probably be a hash of the real icon url (you can use String::hash()). Otherwise the QML image cache will treat every change to the url as a new image and cache that, even if you're just switching between two icons (urls). We could also use QImage's key/hash, but that's mor expensive, so the url hash is a compromise.

Excellent idea! :D

[Rafael Brandao]

Created attachment 118664 [details]
Patch

[Rafael Brandao]

(In reply to comment #46)
> Created an attachment (id=118664) [details]
> Patch

I've got some weird ids coming from image provider when I didn't put a '/' preceding the path when I was setting the icon url. In the docs it seems that it should be there, so I had to prepend it. :/

I really loved the idea of the hash, it made things look simpler and of course a lot more efficient. :)

Let me know your thoughts about this one.

[Kenneth Rohde Christiansen]

Comment on attachment 118664 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=118664&action=review

a few comments but this is starting to look really nice. Very easy to use from qml. Could you make the example show how to show a fallback icon if the page has no favicon? a bit like in QtTestBrowser

> Source/WebKit/qt/ChangeLog:7
> +        as source for QQuickImages in order to display it. All images

Images with s?  Don't you want to say QQuickImage

> Source/WebKit/qt/ChangeLog:10
> +        in different times (i.e. dynamically changing favicon).

at* not in

> Source/WebKit/qt/ChangeLog:12
> +        Reviewed by NOBODY (OOPS!).

We normally keep this line just below the bug link

> Source/WebKit2/UIProcess/API/qt/qwebiconimageprovider.cpp:31
> +    , m_defaultFavIcon(1, 1, QImage::Format_ARGB4444_Premultiplied)

Is that a proper default size? or why is it?

> Source/WebKit2/UIProcess/qt/QtWebIconDatabaseClient.cpp:46
> +    // FIXME: a setter using its getter looks wrong but the first actually trigger
> +    // the startup of IconDatabase, this is why we need it. We should separate this setup
> +    // in another function for the sake of clarity.

I think this could be reworded.

// The setter calls the getter here as it triggers the start up of the icon database.
// FIXME: Separate this setup into another method to make this clear.

You could even just drop the last line

[Simon Hausmann]

Comment on attachment 118664 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=118664&action=review

> Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:158
> +    if (q->url() != pageURL)

The call to q->url() expands to q->d->pageProxy->url(), so you can simply write

    if (pageProxy->url() != pageURL)

here.

>> Source/WebKit2/UIProcess/API/qt/qwebiconimageprovider.cpp:31
>> +    , m_defaultFavIcon(1, 1, QImage::Format_ARGB4444_Premultiplied)
> 
> Is that a proper default size? or why is it?

This ties into another aspect of the API that I'm wondering about:

Imagine an application that wants to show a favicon only if there is one and nothing otherwise?

How would we do that? Is it tied to the dimension? like visible: width > 0 && height > 0?

(or wait, zero doesn't work because our default favicon is 1x1 :)

> Source/WebKit2/UIProcess/qt/QtWebIconDatabaseClient.cpp:42
> +    : m_imageLock(new WTF::Mutex)

Any particular reason to allocate the mutex on the heap?

(If yes, then at least use an OwnPtr instead of the manual memory management)

> Source/WebKit2/UIProcess/qt/QtWebIconDatabaseClient.cpp:104
> +    url.setScheme(QStringLiteral("image"));
> +    url.setHost(QStringLiteral("webicon"));
> +    url.setPath(QString::number(iconID).prepend('/'));
> +    url.setEncodedFragment(pageURL.toEncoded());

Any particular reason why you chose the fragment to encode the url instead my suggestion of image://webicon/<id>/url-encoded-as-path ?

I personally find the latter easier to read (for debugging) and the extraction of the encoded url from the path can be done without
creating temporary strings (compared to id.right(id.size() - id.indexOf('#') - 1)).

[Rafael Brandao]

(In reply to comment #48)
> (From update of attachment 118664 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=118664&action=review
> 
> a few comments but this is starting to look really nice. Very easy to use from qml. Could you make the example show how to show a fallback icon if the page has no favicon? a bit like in QtTestBrowser

I will have to handle this somehow. My current idea is to clean the icon url (use an empty url) once we request to load a new page. If the page actually has an icon, then we will update it. Then we could check on the size of the icon to realize whether we have one or not.

> 
> > Source/WebKit/qt/ChangeLog:7
> > +        as source for QQuickImages in order to display it. All images
> 
> Images with s?  Don't you want to say QQuickImage

Sure.

> 
> > Source/WebKit/qt/ChangeLog:10
> > +        in different times (i.e. dynamically changing favicon).
> 
> at* not in

Nice, thanks.

> 
> > Source/WebKit/qt/ChangeLog:12
> > +        Reviewed by NOBODY (OOPS!).
> 
> We normally keep this line just below the bug link

Hm, I don't remember that but I will fix.

> 
> > Source/WebKit2/UIProcess/API/qt/qwebiconimageprovider.cpp:31
> > +    , m_defaultFavIcon(1, 1, QImage::Format_ARGB4444_Premultiplied)
> 
> Is that a proper default size? or why is it?

At that time I just wanted to put a transparent icon there. The size itself is not a problem, but as Simon commented we could use it better, so I will probably discard the default favicon and then let the client handle it.

> 
> > Source/WebKit2/UIProcess/qt/QtWebIconDatabaseClient.cpp:46
> > +    // FIXME: a setter using its getter looks wrong but the first actually trigger
> > +    // the startup of IconDatabase, this is why we need it. We should separate this setup
> > +    // in another function for the sake of clarity.
> 
> I think this could be reworded.
> 
> // The setter calls the getter here as it triggers the start up of the icon database.
> // FIXME: Separate this setup into another method to make this clear.
> 
> You could even just drop the last line

Thanks! I have problems with excess with words... like right now in this sentence. :-)

[Rafael Brandao]

(In reply to comment #49)
> (From update of attachment 118664 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=118664&action=review
> 
> > Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:158
> > +    if (q->url() != pageURL)
> 
> The call to q->url() expands to q->d->pageProxy->url(), so you can simply write
> 
>     if (pageProxy->url() != pageURL)
> 
> here.

I was worried about pageProxy, it may disappear at some point, so I wouldn't want to change my patch because of that... but I can fix this. :-)

> 
> >> Source/WebKit2/UIProcess/API/qt/qwebiconimageprovider.cpp:31
> >> +    , m_defaultFavIcon(1, 1, QImage::Format_ARGB4444_Premultiplied)
> > 
> > Is that a proper default size? or why is it?
> 
> This ties into another aspect of the API that I'm wondering about:
> 
> Imagine an application that wants to show a favicon only if there is one and nothing otherwise?
> 
> How would we do that? Is it tied to the dimension? like visible: width > 0 && height > 0?
> 
> (or wait, zero doesn't work because our default favicon is 1x1 :)
> 

Good point! I didn't think on the case where there's no favicon. I will work on this tomorrow and see if I can get better solution, but my idea right now is to get rid of the default favicon and return an empty QImage.

As we can get a favicon of any size, it's more safe if we specify the size we want to display on the view, so it can make it smaller if we get a bigger one, for example. In this case, I believe we can't rely on the sizes... I've got to give some more thoughts to this, but I'd appreciate ideas. :-)

> > Source/WebKit2/UIProcess/qt/QtWebIconDatabaseClient.cpp:42
> > +    : m_imageLock(new WTF::Mutex)
> 
> Any particular reason to allocate the mutex on the heap?
> 
> (If yes, then at least use an OwnPtr instead of the manual memory management)

I've got some trouble trying to include the mutex headers on QtWebIconDatabaseClient header file. Maybe I can get rid of this pointer, but if not I will fix it as you've suggested.

> 
> > Source/WebKit2/UIProcess/qt/QtWebIconDatabaseClient.cpp:104
> > +    url.setScheme(QStringLiteral("image"));
> > +    url.setHost(QStringLiteral("webicon"));
> > +    url.setPath(QString::number(iconID).prepend('/'));
> > +    url.setEncodedFragment(pageURL.toEncoded());
> 
> Any particular reason why you chose the fragment to encode the url instead my suggestion of image://webicon/<id>/url-encoded-as-path ?
> 
> I personally find the latter easier to read (for debugging) and the extraction of the encoded url from the path can be done without
> creating temporary strings (compared to id.right(id.size() - id.indexOf('#') - 1)).

When we use "<id>/url-encoded-as-path", we are setting this as the path. To accomplish that I would need to concatenate strings earlier just to set it. I've thought it would be more efficient to use the fragment instead.

The use of id.right there is because we get a const QString& as id, so I can't get rid of the prefix as you've suggested (actually I think I could but it would involve taking the constness away and I don't know if the caller of this function will use this id later).

I'm in a hurry to leave, sorry if didn't make sense. Will try to get earlier tomorrow so we can talk if you want.

[Rafael Brandao]

A solution to handle both cases when the user wants to display nothing or a different default favicon for his client is to not provide a default favicon. We cannot rely on the favicon size (width and height) because user may set those on its interface so the favicon will adjust its size to it.

In this case what we can do is to rely on the image source for the favicon. If we provide an empty url when there's no favicon, then we solve our problems. The webview will only change its icon url when we're sure that we have an image ready for it. So when we have a source into that image, then we should have an image to display, otherwise, we display nothing (and the user can use it to display his own default favicon). Here's how we could use on MiniBrowser:

    Item {
        id: favIconArea
        width: 16
        height: 16
        anchors {
            left: parent.left
            leftMargin: 4
            verticalCenter: parent.verticalCenter
        }
        Image {
            id: favIcon
            source: webView.icon
            anchors.fill: parent
            visible: source != ''
        }
        Image {
            id: defaultFavIcon
            source: '../icons/favicon.png'
            anchors.fill: parent
            visible: !favIcon.visible
        }
    }

Are we ok with this approach?

And I'm in a bit of trouble to include WTF libraries into icon database client header, as it is a QObject and it complains when compiling its moc file that "error: 'WTF_EXPORT_PRIVATE' does not name a type". I'll ask the guys here tomorrow if they know a good way to do it, I would love to get rid of that pointer (or use OwnPtr), but right now I don't know how to do it.

[Rafael Brandao]

Created attachment 119700 [details]
Patch

[Alexis Menard (darktears)]

Comment on attachment 119700 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=119700&action=review

> Source/WebKit2/UIProcess/API/qt/qquickwebview_p.h:56
> +    Q_PROPERTY(QUrl icon READ icon NOTIFY iconChanged)

FINAL? I know the other around doesn't have it but they should (an easy fix for you).

> Tools/MiniBrowser/qt/qml/BrowserWindow.qml:219
> +                    source: webView.icon

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

[Simon Hausmann]

(In reply to comment #51)
> > > Source/WebKit2/UIProcess/qt/QtWebIconDatabaseClient.cpp:104
> > > +    url.setScheme(QStringLiteral("image"));
> > > +    url.setHost(QStringLiteral("webicon"));
> > > +    url.setPath(QString::number(iconID).prepend('/'));
> > > +    url.setEncodedFragment(pageURL.toEncoded());
> > 
> > Any particular reason why you chose the fragment to encode the url instead my suggestion of image://webicon/<id>/url-encoded-as-path ?
> > 
> > I personally find the latter easier to read (for debugging) and the extraction of the encoded url from the path can be done without
> > creating temporary strings (compared to id.right(id.size() - id.indexOf('#') - 1)).
> 
> When we use "<id>/url-encoded-as-path", we are setting this as the path. To accomplish that I would need to concatenate strings earlier just to set it. I've thought it would be more efficient to use the fragment instead.

Somebody is going to concatenate anyway, either you or QUrl :)
 
> The use of id.right there is because we get a const QString& as id, so I can't get rid of the prefix as you've suggested (actually I think I could but it would involve taking the constness away and I don't know if the caller of this function will use this id later).

You're going to have to make a copy anyway, but I think it's better to make that explicit in the code instead of
implicit through temporaries.

It boils down to this: (const QString& id)

QString encodedIconUrl = id;

encodedIconUrl.remove(0, encodedIconUrl.indexOf('#') + 1);

QUrl iconUrl = QUrl::fromPercentEncoding(encodedUrl);

[Simon Hausmann]

Comment on attachment 119700 [details]
Patch

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.

[Rafael Brandao]

(In reply to comment #56)
> (From update of attachment 119700 [details])
> 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?

Thanks. I don't see multiple WebIconDatabase (one for each context), but a singleton that every context will point to (or just have a global/static function to get it). This is because we would have many entities dealing with the same database which doesn't make sense (at least for contexts), or different databases but we would never know at the next application start up which one we should use to display favicons. So I think we won't need to know the context the icon url is related as we will just have one place to access. 

But the multi-webprocess issue is still there, for example WebIconDatabase doesn't know where to ask for the resource, so we currently sent the request to all web processes. We will need to figure out how to avoid this in the future, so I think the best thing to do right now is to postpone it right now and think more about it.

> 
> > 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.

Oh yea, we already have it. You got me. :-)

> 
> > 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.

This is why QML is so fantastic, it makes things so simple. Thanks! :-)

[Rafael Brandao]

Created attachment 119954 [details]
Patch

[Simon Hausmann]

(In reply to comment #57)
> (In reply to comment #56)
> > (From update of attachment 119700 [details] [details])
> > 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?
> 
> Thanks. I don't see multiple WebIconDatabase (one for each context), but a singleton that every context will point to (or just have a global/static function to get it). This is because we would have many entities dealing with the same database which doesn't make sense (at least for contexts), or different databases but we would never know at the next application start up which one we should use to display favicons. So I think we won't need to know the context the icon url is related as we will just have one place to access. 

I disagree.

Look at the design:

A document is loaded in the web process and the frame loader's IconController asks the proxy icondatabase for a load decision. The load decision message is sent from WebIconDatabaseProxy::loadDecisionForIconURL like this:

    m_process->connection()->send(Messages::WebIconDatabase::GetLoadDecisionForIconURL(iconURL, id), 0);

That means it ends up in

    void WebContext::didReceiveMessage(CoreIPC::Connection* connection, CoreIPC::MessageID messageID, CoreIPC::ArgumentDecoder* arguments)

where it will be delegated to the context's WebIconDatabase. If the database is not initialized/open, then the icon load decision is negative.

So each context has its own WebIconDatabase, which is responsible for all icon requests from the context (usually == process) it belongs to. If you have two WebContext instances, then you also need two databases if you want favicons to work there. The actual _data_ is loaded from the processes the contexts own and sent to the context specific icon database.


And this seems like a sensible design. If you are using multiple contexts, then you do _not_ want them to share data (from a privacy perspective, and favicons beautifully provide information about websites you've visisted).

My guts feeling is that we need a per-context storage location that can serve as the base directory for things like the icondatabase or web databases.

With your current patch favicons will only work for webviews that belong to the default webcontext.

[Tor Arne Vestbø]

Comment on attachment 119954 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=119954&action=review

In general this looks good, agreed.

> Source/WebKit2/UIProcess/qt/QtWebIconDatabaseClient.cpp:99
> +    url.setHost(QStringLiteral("webicon"));
> +    url.setPath(QString::number(iconID).prepend('/'));

I'm a little confused by this format, why image://webicon/id#page?  A page has an icon, so to me the url would be the reverse, image://webicon/page/id ? or page#id, or page?icon=id

> Source/WebKit2/UIProcess/qt/QtWebPageLoadClient.cpp:174
> +    client->m_webView->d_func()->setIcon(QUrl());

The title does not change until the load commits, so should probably the icon?

> Tools/MiniBrowser/qt/qml/BrowserWindow.qml:210
> +                source: webView.icon != '' ? webView.icon : '../icons/favicon.png'

This is a much nicer short-form, agreed. Though, in a real browser you'd probably actually want two Images, and have the visibility of the default one depend on the state != Image.Ready of the web icon, so that you don't get a blinking effect between setting the source and the image being ready.

[Simon Hausmann]

Comment on attachment 119954 [details]
Patch

Let's get this in. I'll fix up the icon changed notification in a separate change.

[Simon Hausmann]

Committed r103316: <http://trac.webkit.org/changeset/103316>

[Csaba Osztrogonác]

(In reply to comment #62)
> Committed r103316: <http://trac.webkit.org/changeset/103316>

It seems it made many tests crash. Could you check it?

[Csaba Osztrogonác]

.

[Csaba Osztrogonác]

Comment on attachment 119954 [details]
Patch

Remove r+ from landed patch

[Rafael Brandao]

The commit http://trac.webkit.org/changeset/103323 has fixed the test crashes. I'll mark this one as resolved then.