[Qt][WK2] Layer QtWebIconDatabaseClient on the C API
Created attachment 187837 [details] Patch
(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.
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.
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.
Created attachment 191746 [details] Patch Use the existing implicit conversion from WTF::String in WKURLCopyQString
Comment on attachment 191746 [details] Patch LGTM :)
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.
(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.
Comment on attachment 191746 [details] Patch Remember to add sign-off line :)
Committed r145518: <http://trac.webkit.org/changeset/145518>