[Qt] Sync up favicon-implementation with WebView url changes in r118158

https://bugs.webkit.org/show_bug.cgi?id=87133 We now base64-encode the page url in the image-provider url, so that any normalization done by QUrl will not mess up the page-url. The logic of creating and parsing the provider-url has been moved into the image provider, to keep it in one place. We were also releasing icons (even ones we hadn't retained), which we can't do since we don't know when the icon url is no longer in use. Reviewed-by Simon Hausmann. Canonical link: https://commits.webkit.org/105504@main git-svn-id: https://svn.webkit.org/repository/webkit/trunk@118762 268f45cc-cd09-0410-ab3c-d52691b4dbfc
WebKit · May 29, 2012 · 908e0f8 · 908e0f8
1 parent c07c784
commit 908e0f8
Show file tree

Hide file tree

Showing 15 changed files with 194 additions and 92 deletions.
diff --git a/Source/WebKit/qt/ChangeLog b/Source/WebKit/qt/ChangeLog
@@ -1,3 +1,22 @@
+2012-05-29  Tor Arne Vestbø  <tor.arne.vestbo@nokia.com>
+
+        [Qt] Sync up favicon-implementation with WebView url changes in r118158
+
+        https://bugs.webkit.org/show_bug.cgi?id=87133
+
+        We now base64-encode the page url in the image-provider url, so that any
+        normalization done by QUrl will not mess up the page-url. The logic of
+        creating and parsing the provider-url has been moved into the image
+        provider, to keep it in one place.
+
+        We were also releasing icons (even ones we hadn't retained), which we can't
+        do since we don't know when the icon url is no longer in use.
+
+        Reviewed-by Simon Hausmann.
+
+        * declarative/plugin.cpp:
+        * declarative/public.pri:
+
 2012-05-25  Jesus Sanchez-Palencia  <jesus.palencia@openbossa.org>
 
         WebKitTestRunner needs to support layoutTestController.setJavaScriptProfilingEnabled

diff --git a/Source/WebKit/qt/declarative/plugin.cpp b/Source/WebKit/qt/declarative/plugin.cpp
@@ -17,6 +17,8 @@
     Boston, MA 02110-1301, USA.
 */
 
+#include "config.h"
+
 #include "qglobal.h"
 
 #if defined(HAVE_QQUICK1)

diff --git a/Source/WebKit/qt/declarative/public.pri b/Source/WebKit/qt/declarative/public.pri
@@ -34,6 +34,8 @@ contains(DEFINES, HAVE_QQUICK1=1) {
     HEADERS += qdeclarativewebview_p.h
 }
 
+WEBKIT += wtf
+
 DESTDIR = $${ROOT_BUILD_DIR}/imports/$${TARGET.module_name}
 
 CONFIG += rpath

diff --git a/Source/WebKit2/ChangeLog b/Source/WebKit2/ChangeLog
@@ -1,3 +1,43 @@
+2012-05-29  Tor Arne Vestbø  <tor.arne.vestbo@nokia.com>
+
+        [Qt] Sync up favicon-implementation with WebView url changes in r118158
+
+        https://bugs.webkit.org/show_bug.cgi?id=87133
+
+        We now base64-encode the page url in the image-provider url, so that any
+        normalization done by QUrl will not mess up the page-url. The logic of
+        creating and parsing the provider-url has been moved into the image
+        provider, to keep it in one place.
+
+        We were also releasing icons (even ones we hadn't retained), which we can't
+        do since we don't know when the icon url is no longer in use.
+
+        Reviewed-by Simon Hausmann.
+
+        * UIProcess/API/qt/qquickwebview.cpp:
+        (QQuickWebViewPrivate::initialize):
+        (QQuickWebViewPrivate::loadProgressDidChange):
+        (QQuickWebViewPrivate::_q_onUrlChanged):
+        (QQuickWebViewPrivate::_q_onIconChangedForPageURL):
+        (QQuickWebViewPrivate::updateIcon):
+        (QQuickWebView::icon):
+        * UIProcess/API/qt/qquickwebview_p.h:
+        * UIProcess/API/qt/qquickwebview_p_p.h:
+        (QQuickWebViewPrivate):
+        * UIProcess/API/qt/qwebiconimageprovider.cpp:
+        (QWebIconImageProvider::iconURLForPageURLInContext):
+        (QWebIconImageProvider::requestImage):
+        * UIProcess/API/qt/qwebiconimageprovider_p.h:
+        (WebKit):
+        * UIProcess/API/qt/tests/qmltests/WebView/tst_favIconLoad.qml:
+        * UIProcess/qt/QtWebIconDatabaseClient.cpp:
+        (WebKit::QtWebIconDatabaseClient::didChangeIconForPageURL):
+        (WebKit::QtWebIconDatabaseClient::iconForPageURL):
+        (WebKit):
+        (WebKit::QtWebIconDatabaseClient::iconImageForPageURL):
+        * UIProcess/qt/QtWebIconDatabaseClient.h:
+        (QtWebIconDatabaseClient):
+
 2012-05-29  Kenneth Rohde Christiansen  <kenneth@webkit.org>
 
         [Qt][WK2] Fix failing qmltests::FitToView::test_basic()

diff --git a/Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp b/Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp
@@ -44,6 +44,7 @@
 #include "qquickwebpage_p_p.h"
 #include "qquickwebview_p_p.h"
 #include "qwebdownloaditem_p_p.h"
+#include "qwebiconimageprovider_p.h"
 #include "qwebkittest_p.h"
 #include "qwebloadrequest_p.h"
 #include "qwebnavigationhistory_p.h"
@@ -56,6 +57,7 @@
 #include <QDateTime>
 #include <QtCore/QFile>
 #include <QtQml/QJSValue>
+#include <QtQuick/QQuickView>
 #include <WKOpenPanelResultListener.h>
 #include <WKSerializedScriptValue.h>
 #include <WebCore/IntPoint.h>
@@ -305,7 +307,7 @@ void QQuickWebViewPrivate::initialize(WKContextRef contextRef, WKPageGroupRef pa
     navigationHistory = adoptPtr(QWebNavigationHistoryPrivate::createHistory(toAPI(webPageProxy.get())));
 
     QtWebIconDatabaseClient* iconDatabase = context->iconDatabase();
-    QObject::connect(iconDatabase, SIGNAL(iconChangedForPageURL(QUrl, QUrl)), q_ptr, SLOT(_q_onIconChangedForPageURL(QUrl, QUrl)));
+    QObject::connect(iconDatabase, SIGNAL(iconChangedForPageURL(QString)), q_ptr, SLOT(_q_onIconChangedForPageURL(QString)));
 
     // Any page setting should preferrable be set before creating the page.
     webPageProxy->pageGroup()->preferences()->setAcceleratedCompositingEnabled(true);
@@ -375,9 +377,6 @@ void QQuickWebViewPrivate::loadProgressDidChange(int loadProgress)
 {
     Q_Q(QQuickWebView);
 
-    if (!loadProgress)
-        setIcon(QUrl());
-
     m_loadProgress = loadProgress;
 
     emit q->loadProgressChanged();
@@ -420,16 +419,6 @@ void QQuickWebViewPrivate::setNeedsDisplay()
     q->page()->update();
 }
 
-void QQuickWebViewPrivate::_q_onIconChangedForPageURL(const QUrl& pageURL, const QUrl& iconURL)
-{
-    Q_Q(QQuickWebView);
-
-    if (q->url() != pageURL)
-        return;
-
-    setIcon(iconURL);
-}
-
 void QQuickWebViewPrivate::processDidCrash()
 {
     Q_Q(QQuickWebView);
@@ -482,9 +471,39 @@ void QQuickWebViewPrivate::_q_onVisibleChanged()
 }
 
 void QQuickWebViewPrivate::_q_onUrlChanged()
+{
+    updateIcon();
+}
+
+void QQuickWebViewPrivate::_q_onIconChangedForPageURL(const QString& pageUrl)
+{
+    if (pageUrl != QString(m_currentUrl))
+        return;
+
+    updateIcon();
+}
+
+/* Called either when the url changes, or when the icon for the current page changes */
+void QQuickWebViewPrivate::updateIcon()
 {
     Q_Q(QQuickWebView);
-    context->iconDatabase()->requestIconForPageURL(q->url());
+
+    QQuickView* view = qobject_cast<QQuickView*>(q->canvas());
+    if (!view)
+        return;
+
+    QWebIconImageProvider* provider = static_cast<QWebIconImageProvider*>(
+                view->engine()->imageProvider(QWebIconImageProvider::identifier()));
+    if (!provider)
+        return;
+
+    WTF::String iconUrl = provider->iconURLForPageURLInContext(m_currentUrl, context.get());
+
+    if (iconUrl == m_iconUrl)
+        return;
+
+    m_iconUrl = iconUrl;
+    emit q->iconChanged();
 }
 
 void QQuickWebViewPrivate::_q_onReceivedResponseFromDownload(QWebDownloadItem* downloadItem)
@@ -644,31 +663,6 @@ void QQuickWebViewPrivate::addAttachedPropertyTo(QObject* object)
     attached->setView(q);
 }
 
-void QQuickWebViewPrivate::setIcon(const QUrl& iconURL)
-{
-    Q_Q(QQuickWebView);
-    if (m_iconURL == iconURL)
-        return;
-
-    if (!webPageProxy->mainFrame())
-        return;
-
-    String oldPageURL = QUrl::fromPercentEncoding(m_iconURL.encodedFragment());
-    String newPageURL = webPageProxy->mainFrame()->url();
-
-    if (oldPageURL != newPageURL) {
-        QtWebIconDatabaseClient* iconDatabase = context->iconDatabase();
-        if (!oldPageURL.isEmpty())
-            iconDatabase->releaseIconForPageURL(oldPageURL);
-
-        if (!newPageURL.isEmpty())
-            iconDatabase->retainIconForPageURL(newPageURL);
-    }
-
-    m_iconURL = iconURL;
-    emit q->iconChanged();
-}
-
 bool QQuickWebViewPrivate::navigatorQtObjectEnabled() const
 {
     return m_navigatorQtObjectEnabled;
@@ -1561,7 +1555,7 @@ void QQuickWebView::emitUrlChangeIfNeeded()
 QUrl QQuickWebView::icon() const
 {
     Q_D(const QQuickWebView);
-    return d->m_iconURL;
+    return QUrl(d->m_iconUrl);
 }
 
 /*!

diff --git a/Source/WebKit2/UIProcess/API/qt/qquickwebview_p.h b/Source/WebKit2/UIProcess/API/qt/qquickwebview_p.h
@@ -212,7 +212,7 @@ public Q_SLOTS:
     Q_PRIVATE_SLOT(d_func(), void _q_onVisibleChanged());
     Q_PRIVATE_SLOT(d_func(), void _q_onUrlChanged());
     Q_PRIVATE_SLOT(d_func(), void _q_onReceivedResponseFromDownload(QWebDownloadItem*));
-    Q_PRIVATE_SLOT(d_func(), void _q_onIconChangedForPageURL(const QUrl&, const QUrl&));
+    Q_PRIVATE_SLOT(d_func(), void _q_onIconChangedForPageURL(const QString&));
     // Hides QObject::d_ptr allowing us to use the convenience macros.
     QScopedPointer<QQuickWebViewPrivate> d_ptr;
     QQuickWebViewExperimental* m_experimental;

diff --git a/Source/WebKit2/UIProcess/API/qt/qquickwebview_p_p.h b/Source/WebKit2/UIProcess/API/qt/qquickwebview_p_p.h
@@ -103,7 +103,7 @@ class QQuickWebViewPrivate {
     void _q_onVisibleChanged();
     void _q_onUrlChanged();
     void _q_onReceivedResponseFromDownload(QWebDownloadItem*);
-    void _q_onIconChangedForPageURL(const QUrl& pageURL, const QUrl& iconURLString);
+    void _q_onIconChangedForPageURL(const QString&);
 
     void chooseFiles(WKOpenPanelResultListenerRef, const QStringList& selectedFileNames, WebKit::QtWebPageUIClient::FileChooserType);
     quint64 exceededDatabaseQuota(const QString& databaseName, const QString& displayName, WKSecurityOriginRef securityOrigin, quint64 currentQuota, quint64 currentOriginUsage, quint64 currentDatabaseUsage, quint64 expectedUsage);
@@ -118,7 +118,6 @@ class QQuickWebViewPrivate {
     void setRenderToOffscreenBuffer(bool enable) { m_renderToOffscreenBuffer = enable; }
     void setTransparentBackground(bool);
     void addAttachedPropertyTo(QObject*);
-    void setIcon(const QUrl&);
 
     bool navigatorQtObjectEnabled() const;
     bool renderToOffscreenBuffer() const { return m_renderToOffscreenBuffer; }
@@ -133,6 +132,8 @@ class QQuickWebViewPrivate {
 
     void setDialogActive(bool active) { m_dialogActive = active; }
 
+    void updateIcon();
+
     // PageClient.
     WebCore::IntSize viewSize() const;
     void didReceiveMessageFromNavigatorQtObject(const String& message);
@@ -200,7 +201,7 @@ class QQuickWebViewPrivate {
     bool m_renderToOffscreenBuffer;
     bool m_dialogActive;
     bool m_allowAnyHTTPSCertificateForLocalHost;
-    QUrl m_iconURL;
+    WTF::String m_iconUrl;
     int m_loadProgress;
     WTF::String m_currentUrl;
 };

diff --git a/Source/WebKit2/UIProcess/API/qt/qwebiconimageprovider.cpp b/Source/WebKit2/UIProcess/API/qt/qwebiconimageprovider.cpp
@@ -24,6 +24,7 @@
 #include "QtWebIconDatabaseClient.h"
 #include <QtCore/QUrl>
 #include <QtGui/QImage>
+#include <wtf/text/StringHash.h>
 #include <wtf/text/WTFString.h>
 
 using namespace WebKit;
@@ -37,24 +38,52 @@ QWebIconImageProvider::~QWebIconImageProvider()
 {
 }
 
-QImage QWebIconImageProvider::requestImage(const QString& id, QSize* size, const QSize& requestedSize)
+WTF::String QWebIconImageProvider::iconURLForPageURLInContext(const WTF::String &pageURL, QtWebContext* context)
 {
-    QString decodedIconUrl = id;
-    decodedIconUrl.remove(0, decodedIconUrl.indexOf('#') + 1);
-    String pageURL = QString::fromUtf8(QUrl(decodedIconUrl).toEncoded());
+    QtWebIconDatabaseClient* iconDatabase = context->iconDatabase();
+    WTF::String iconURL = iconDatabase->iconForPageURL(pageURL);
+
+    if (iconURL.isEmpty())
+        return String();
+
+    QUrl url;
+    url.setScheme(QStringLiteral("image"));
+    url.setHost(QWebIconImageProvider::identifier());
+
+    QString path;
+    path.append(QLatin1Char('/'));
+    path.append(QString::number(context->contextID()));
+    path.append(QLatin1Char('/'));
+    path.append(QString::number(WTF::StringHash::hash(iconURL)));
+    url.setPath(path);
+
+    // FIXME: Use QUrl::DecodedMode when landed in Qt
+    url.setFragment(QString::fromLatin1(QByteArray(QString(pageURL).toUtf8()).toBase64()));
+
+    // FIXME: We can't know when the icon url is no longer in use,
+    // so we never release these icons. At some point we might want
+    // to introduce expiry of icons to elevate this issue.
+    iconDatabase->retainIconForPageURL(pageURL);
 
+    return url.toString(QUrl::FullyEncoded);
+}
+
+QImage QWebIconImageProvider::requestImage(const QString& id, QSize* size, const QSize& requestedSize)
+{
     // The string identifier has the leading image://webicon/ already stripped, so we just
     // need to truncate from the first slash to get the context id.
-    QString contextIDAsString = id;
-    contextIDAsString.truncate(contextIDAsString.indexOf(QLatin1Char('/')));
+    QString contextIDString = id.left(id.indexOf(QLatin1Char('/')));
     bool ok = false;
-    uint64_t contextId = contextIDAsString.toUInt(&ok);
+    uint64_t contextId = contextIDString.toUInt(&ok);
     if (!ok)
         return QImage();
+
     QtWebContext* context = QtWebContext::contextByID(contextId);
     if (!context)
         return QImage();
 
+    QString pageURL = QString::fromUtf8(QByteArray::fromBase64(id.midRef(id.indexOf('#') + 1).toLatin1()));
+
     QtWebIconDatabaseClient* iconDatabase = context->iconDatabase();
     QImage icon = requestedSize.isValid() ? iconDatabase->iconImageForPageURL(pageURL, requestedSize) : iconDatabase->iconImageForPageURL(pageURL);
     ASSERT(!icon.isNull());

diff --git a/Source/WebKit2/UIProcess/API/qt/qwebiconimageprovider_p.h b/Source/WebKit2/UIProcess/API/qt/qwebiconimageprovider_p.h
@@ -21,13 +21,23 @@
 #define qwebiconimageprovider_p_h
 
 #include "qwebkitglobal.h"
+#include <QString>
 #include <QtQuick/QQuickImageProvider>
+#include <wtf/text/WTFString.h>
+
+namespace WebKit {
+    class QtWebContext;
+}
 
 class QWEBKIT_EXPORT QWebIconImageProvider : public QQuickImageProvider {
 public:
     QWebIconImageProvider();
     ~QWebIconImageProvider();
-    QImage requestImage(const QString& id, QSize* size, const QSize& requestedSize);
+
+    static QString identifier() { return QStringLiteral("webicon"); }
+
+    WTF::String iconURLForPageURLInContext(const WTF::String& pageURL, WebKit::QtWebContext* context);
+    virtual QImage requestImage(const QString& id, QSize* size, const QSize& requestedSize);
 };
 
 #endif
diff --git a/Source/WebKit2/UIProcess/API/qt/tests/qmltests/WebView/tst_favIconLoad.qml b/Source/WebKit2/UIProcess/API/qt/tests/qmltests/WebView/tst_favIconLoad.qml
@@ -20,6 +20,7 @@ TestWebView {
     TestCase {
         id: test
         name: "WebViewLoadFavIcon"
+        when: windowShown
 
         function init() {
             if (webView.icon != '') {
@@ -36,7 +37,6 @@ TestWebView {
             var url = Qt.resolvedUrl("../common/favicon.html")
             webView.url = url
             verify(webView.waitForLoadSucceeded())
-            expectFail("", "https://bugs.webkit.org/show_bug.cgi?id=87133")
             compare(spy.count, 1)
             compare(favicon.width, 48)
             compare(favicon.height, 48)
@@ -47,7 +47,6 @@ TestWebView {
             var url = Qt.resolvedUrl("../common/favicon2.html?favicon=load should work with#whitespace!")
             webView.url = url
             verify(webView.waitForLoadSucceeded())
-            expectFail("", "https://bugs.webkit.org/show_bug.cgi?id=87133")
             compare(spy.count, 1)
             compare(favicon.width, 16)
             compare(favicon.height, 16)