Bug 109564 – [Qt][WK2] Layer QtWebIconDatabaseClient on the C API

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Jocelyn Turcotte]

[Qt][WK2] Layer QtWebIconDatabaseClient on the C API

[Jocelyn Turcotte]

Created attachment 187837 [details]
Patch

[Jocelyn Turcotte]

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

There are two things that feel dirty about this patch:

- Going from WTF::String then to QUrl then back to WTF::String through WKUrlRef to keep the key to the icon for our pageUrl seems brittle to me. I feel that this could break at some point if QUrl isn't encoding to a string the same way that it was received. But I think that, at least, the code looks cleaner that way if we only look at the interface given by the WK2 C API.

- A change to updateID() happens on each icon update/creation. Any QML code fetching the icon() property of any of the view would then get a different URL, but only the matching pageUrl will have its iconChanged signal emitted. I don't see what this could break, but this stretches the contract that a property value change should be followed by a changed signal. In this case we ignore it since we know that this URL is only used to eventually fetch a QImage from the image provider, which we know didn't change.
Ideally it seems like the proper solution would be to ask people to set cache:false on the Image displaying icons. Even better would be to have a way from C++ to invalidate a QQuickPixmap without having to change the key URL pointing to it.

So also please tell me if you see any issue with those.

[Jocelyn Turcotte]

Created attachment 188350 [details]
Patch

Bug 109468 has been updated to use QString for the page URL. Use this and allow converting it to a WKUrlRef with WKURLCreateWithQString and WKURLCopyQString.
This addresses the first dirty point mentioned above.

[Simon Hausmann]

Comment on attachment 188350 [details]
Patch

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

> Source/WebKit2/UIProcess/API/cpp/qt/WKURLQt.cpp:41
> +    return QString(reinterpret_cast<const QChar*>(string.characters()), string.length());

You can just return string, WTF::String has a conversion operator to QString that is _very_ fast if the WTF::String was previously created from a QString.

[Jocelyn Turcotte]

Created attachment 191746 [details]
Patch

Use the existing implicit conversion from WTF::String in WKURLCopyQString

[Simon Hausmann]

Comment on attachment 191746 [details]
Patch

LGTM :)

[Benjamin Poulain]

Comment on attachment 191746 [details]
Patch

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

I don't know shit about the IconDatabase, I can't really review this :(
This is all using stable APIs so it is a positive change anyway.

I sign off on this, but make sure you get a real review :)

> Source/WebKit2/ChangeLog:21
> +          - The WKUrlRef behind the scene is a WTF::String and encoding it back and forth to
> +            a QUrl can slightly change its string representation. Allow converting a WKUrlRef
> +            to and from a QString to ensure this.

You should not rely on WKUrlRef being a String. This is an implementation detail and will hopefully change in the future.

> Source/WebKit2/UIProcess/API/qt/qwebiconimageprovider.cpp:52
> +    url.setPath(QStringLiteral("/%1").arg(QtWebIconDatabaseClient::updateID()));

Wouldn't QString::setNum(QtWebIconDatabaseClient::updateID()) be better?

> Source/WebKit2/UIProcess/qt/QtWebIconDatabaseClient.cpp:56
> +    WKContextSetIconDatabasePath(context, adoptWK(WKStringCreateWithQString(QtWebContext::preparedStoragePath(QtWebContext::IconDatabaseStorage))).get());

I'd split this to have 'adoptWK(WKStringCreateWithQString(QtWebContext::preparedStoragePath(QtWebContext::IconDatabaseStorage)))' on its own line.

[Jocelyn Turcotte]

(In reply to comment #7)
> > Source/WebKit2/ChangeLog:21
> > +          - The WKUrlRef behind the scene is a WTF::String and encoding it back and forth to
> > +            a QUrl can slightly change its string representation. Allow converting a WKUrlRef
> > +            to and from a QString to ensure this.
> 
> You should not rely on WKUrlRef being a String. This is an implementation detail and will hopefully change in the future.

The issue for now is that the URL is used as a key to access the icon database, so we need our representation to be equal char-by-char to the internal one.
If we need to handle this in a more complex way in the future we'll do so, but for now I think that we get a reliability benefit from relying on this implementation detail.

> > Source/WebKit2/UIProcess/API/qt/qwebiconimageprovider.cpp:52
> > +    url.setPath(QStringLiteral("/%1").arg(QtWebIconDatabaseClient::updateID()));
> 
> Wouldn't QString::setNum(QtWebIconDatabaseClient::updateID()) be better?

I used arg because of the prefixing slash (maybe you didn't see it).

> > Source/WebKit2/UIProcess/qt/QtWebIconDatabaseClient.cpp:56
> > +    WKContextSetIconDatabasePath(context, adoptWK(WKStringCreateWithQString(QtWebContext::preparedStoragePath(QtWebContext::IconDatabaseStorage))).get());
> 
> I'd split this to have 'adoptWK(WKStringCreateWithQString(QtWebContext::preparedStoragePath(QtWebContext::IconDatabaseStorage)))' on its own line.

Ok, done in bug #111435.

[Simon Hausmann]

Comment on attachment 191746 [details]
Patch

Remember to add sign-off line :)

[Jocelyn Turcotte]

Committed r145518: <http://trac.webkit.org/changeset/145518>