Merge lp:~osomon/webbrowser-app/limit-thumbnail-cache-size into lp:webbrowser-app

[Olivier Tilloy]

Delete old thumbnails to ensure the cache never grows bigger than 5MB.

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:282
http://jenkins.qa.ubuntu.com/job/webbrowser-app-ci/276/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-saucy/2032/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/webbrowser-app-saucy-amd64-ci/159/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/webbrowser-app-saucy-armhf-ci/159/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/webbrowser-app-saucy-i386-ci/159/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-saucy/2037/console

Click here to trigger a rebuild:
http://s-jenkins:8080/job/webbrowser-app-ci/276/rebuild

review: Needs Fixing (continuous-integration)

[lp:~osomon/webbrowser-app/limit-thumbnail-cache-size]

283. By Olivier Tilloy on 2013-08-08

Remove unused dependencies.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:283
http://jenkins.qa.ubuntu.com/job/webbrowser-app-ci/277/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-saucy/2035
    SUCCESS: http://jenkins.qa.ubuntu.com/job/webbrowser-app-saucy-amd64-ci/160
    SUCCESS: http://jenkins.qa.ubuntu.com/job/webbrowser-app-saucy-armhf-ci/160
        deb: http://jenkins.qa.ubuntu.com/job/webbrowser-app-saucy-armhf-ci/160/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/webbrowser-app-saucy-i386-ci/160
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-saucy/2040
        deb: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-saucy/2040/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-runner-saucy/1735

Click here to trigger a rebuild:
http://s-jenkins:8080/job/webbrowser-app-ci/277/rebuild

review: Approve (continuous-integration)

[Günter Schwann (schwann)]

45 + m_thumbnailUtilsThread->deleteLater();
I'd call m_thumbnailUtilsThread->wait() and delete the thread directly instead

234 + QMetaObject::invokeMethod(&WebThumbnailUtils::instance(), "cacheThumbnail",
You could add the connection type Qt::QueuedConnection, to highlight (and make sure) that it's asynchronous.

148 + qint64 goal = MAX_CACHE_SIZE_IN_BYTES * 9 / 10;
So the max cache size is 90% of 5MB? Why?

[lp:~osomon/webbrowser-app/limit-thumbnail-cache-size]

284. By Olivier Tilloy on 2013-08-09

Ensure the thread terminates before deleting it.

285. By Olivier Tilloy on 2013-08-09

Make it (more) explicit that the method is called asynchronously.

286. By Olivier Tilloy on 2013-08-09

Do not expire cache entries if the total size doesn’t exceed the limit.

[Olivier Tilloy (osomon)]

> 45 + m_thumbnailUtilsThread->deleteLater();
> I'd call m_thumbnailUtilsThread->wait() and delete the thread directly instead

Done. Thanks for the suggestion.

> 234 + QMetaObject::invokeMethod(&WebThumbnailUtils::instance(),
> "cacheThumbnail",
> You could add the connection type Qt::QueuedConnection, to highlight (and make
> sure) that it's asynchronous.

Although not strictly necessary, done.

> 148 + qint64 goal = MAX_CACHE_SIZE_IN_BYTES * 9 / 10;
> So the max cache size is 90% of 5MB? Why?

That was a mistake on my part. This code is not triggered only if the total cache size exceeds the limit. The goal is 90% of the limit to have some buffer, so that we do not need to expire entries every time a new thumbnail is generated (this is inspired by the implementation of the QNetworkDiskCache class).

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:286
http://jenkins.qa.ubuntu.com/job/webbrowser-app-ci/278/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-saucy/2049
    SUCCESS: http://jenkins.qa.ubuntu.com/job/webbrowser-app-saucy-amd64-ci/161
    SUCCESS: http://jenkins.qa.ubuntu.com/job/webbrowser-app-saucy-armhf-ci/161
        deb: http://jenkins.qa.ubuntu.com/job/webbrowser-app-saucy-armhf-ci/161/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/webbrowser-app-saucy-i386-ci/161
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-saucy/2054
        deb: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-saucy/2054/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-runner-saucy/1747

Click here to trigger a rebuild:
http://s-jenkins:8080/job/webbrowser-app-ci/278/rebuild

review: Approve (continuous-integration)

[Günter Schwann (schwann)]

Looks good

review: Approve

=== modified file 'src/Ubuntu/Components/Extras/Browser/plugin.cpp'
--- src/Ubuntu/Components/Extras/Browser/plugin.cpp	2013-07-30 17:26:49 +0000
+++ src/Ubuntu/Components/Extras/Browser/plugin.cpp	2013-08-09 07:47:22 +0000
@@ -26,11 +26,13 @@
 #include "tabs-model.h"
 #include "bookmarks-model.h"
 #include "webthumbnail-provider.h"
+#include "webthumbnail-utils.h"
 #include "webview-thumbnailer.h"
 
 // Qt
 #include <QtCore/QDir>
 #include <QtCore/QStandardPaths>
+#include <QtCore/QThread>
 #include <QtQml>
 
 void UbuntuBrowserPlugin::initializeEngine(QQmlEngine* engine, const char* uri)
@@ -43,9 +45,18 @@
     QQmlContext* context = engine->rootContext();
     context->setContextProperty("dataLocation", dataLocation.absolutePath());
 
+    // This singleton lives in its own thread to ensure that
+    // disk I/O is not performed in the UI thread.
+    WebThumbnailUtils& utils = WebThumbnailUtils::instance();
+    m_thumbnailUtilsThread = new QThread;
+    utils.moveToThread(m_thumbnailUtilsThread);
+    m_thumbnailUtilsThread->start();
+
     WebThumbnailProvider* thumbnailer = new WebThumbnailProvider;
     engine->addImageProvider(QLatin1String("webthumbnail"), thumbnailer);
     context->setContextProperty("WebThumbnailer", thumbnailer);
+
+    connect(engine, SIGNAL(destroyed()), SLOT(onEngineDestroyed()));
 }
 
 void UbuntuBrowserPlugin::registerTypes(const char* uri)
@@ -61,3 +72,10 @@
     qmlRegisterType<BookmarksModel>(uri, 0, 1, "BookmarksModel");
     qmlRegisterType<WebviewThumbnailer>(uri, 0, 1, "WebviewThumbnailer");
 }
+
+void UbuntuBrowserPlugin::onEngineDestroyed()
+{
+    m_thumbnailUtilsThread->quit();
+    m_thumbnailUtilsThread->wait();
+    delete m_thumbnailUtilsThread;
+}

=== modified file 'src/Ubuntu/Components/Extras/Browser/plugin.h'
--- src/Ubuntu/Components/Extras/Browser/plugin.h	2013-06-11 12:06:15 +0000
+++ src/Ubuntu/Components/Extras/Browser/plugin.h	2013-08-09 07:47:22 +0000
@@ -22,6 +22,8 @@
 // Qt
 #include <QtQml/QQmlExtensionPlugin>
 
+class QThread;
+
 class UbuntuBrowserPlugin : public QQmlExtensionPlugin
 {
     Q_OBJECT
@@ -30,6 +32,12 @@
 public:
     void initializeEngine(QQmlEngine* engine, const char* uri);
     void registerTypes(const char* uri);
+
+private:
+    QThread* m_thumbnailUtilsThread;
+
+private Q_SLOTS:
+    void onEngineDestroyed();
 };
 
 #endif // __PLUGIN_H__

=== modified file 'src/Ubuntu/Components/Extras/Browser/webthumbnail-utils.cpp'
--- src/Ubuntu/Components/Extras/Browser/webthumbnail-utils.cpp	2013-07-10 12:02:00 +0000
+++ src/Ubuntu/Components/Extras/Browser/webthumbnail-utils.cpp	2013-08-09 07:47:22 +0000
@@ -16,11 +16,32 @@
  * along with this program.  If not, see <http://www.gnu.org/licenses/>.
  */
 
+#include "domain-utils.h"
 #include "webthumbnail-utils.h"
 
 // Qt
 #include <QtCore/QCryptographicHash>
+#include <QtCore/QFile>
 #include <QtCore/QStandardPaths>
+#include <QtCore/QUrl>
+#include <QtGui/QImage>
+
+#define MAX_CACHE_SIZE_IN_BYTES 5 * 1024 * 1024 // 5MB
+
+WebThumbnailUtils::WebThumbnailUtils(QObject* parent)
+    : QObject(parent)
+{
+}
+
+WebThumbnailUtils& WebThumbnailUtils::instance()
+{
+    static WebThumbnailUtils utils;
+    return utils;
+}
+
+WebThumbnailUtils::~WebThumbnailUtils()
+{
+}
 
 QDir WebThumbnailUtils::cacheLocation()
 {
@@ -40,3 +61,48 @@
     QString hash(QCryptographicHash::hash(url.toEncoded(), QCryptographicHash::Md5).toHex());
     return cacheLocation().absoluteFilePath(hash + ".png");
 }
+
+void WebThumbnailUtils::cacheThumbnail(const QUrl& url, const QImage& thumbnail) const
+{
+    ensureCacheLocation();
+    QFileInfo file = thumbnailFile(url);
+    bool saved = thumbnail.save(file.absoluteFilePath());
+
+    if (saved) {
+        // Make a link to the thumbnail file for the corresponding domain’s thumbnail.
+        QUrl domain(DomainUtils::extractTopLevelDomainName(url));
+        QString domainThumbnail = WebThumbnailUtils::thumbnailFile(domain).absoluteFilePath();
+        if (QFile::exists(domainThumbnail)) {
+            QFile::remove(domainThumbnail);
+        }
+        QFile::link(file.fileName(), domainThumbnail);
+    }
+
+    expireCache();
+}
+
+void WebThumbnailUtils::expireCache() const
+{
+    QDir cache = cacheLocation();
+    if (!cache.exists()) {
+        return;
+    }
+    QStringList nameFilters = QStringList() << "*.png";
+    QDir::Filters filters = QDir::Files | QDir::NoDotAndDotDot;
+    QDir::SortFlags sort = QDir::Time;
+    QFileInfoList entries = cache.entryInfoList(nameFilters, filters, sort);
+    qint64 currentSize = 0;
+    Q_FOREACH(const QFileInfo& entry, entries) {
+        currentSize += entry.size();
+    }
+    if (currentSize > MAX_CACHE_SIZE_IN_BYTES) {
+        qint64 goal = MAX_CACHE_SIZE_IN_BYTES * 9 / 10;
+        while (!entries.isEmpty() && (currentSize > goal)) {
+            QFileInfo entry = entries.takeLast();
+            qint64 size = entry.size();
+            if (QFile::remove(entry.absoluteFilePath())) {
+                currentSize -= size;
+            }
+        }
+    }
+}

=== modified file 'src/Ubuntu/Components/Extras/Browser/webthumbnail-utils.h'
--- src/Ubuntu/Components/Extras/Browser/webthumbnail-utils.h	2013-07-10 12:02:00 +0000
+++ src/Ubuntu/Components/Extras/Browser/webthumbnail-utils.h	2013-08-09 07:47:22 +0000
@@ -22,14 +22,30 @@
 // Qt
 #include <QtCore/QDir>
 #include <QtCore/QFileInfo>
-#include <QtCore/QUrl>
-
-class WebThumbnailUtils
+#include <QtCore/QObject>
+
+class QImage;
+class QUrl;
+
+class WebThumbnailUtils : public QObject
 {
+    Q_OBJECT
+
 public:
+    static WebThumbnailUtils& instance();
+    ~WebThumbnailUtils();
+
     static QDir cacheLocation();
     static void ensureCacheLocation();
     static QFileInfo thumbnailFile(const QUrl& url);
+
+public Q_SLOTS:
+    void cacheThumbnail(const QUrl& url, const QImage& thumbnail) const;
+
+private:
+    WebThumbnailUtils(QObject* parent=0);
+
+    void expireCache() const;
 };
 
 #endif // __WEBTHUMBNAIL_UTILS_H__

=== modified file 'src/Ubuntu/Components/Extras/Browser/webview-thumbnailer.cpp'
--- src/Ubuntu/Components/Extras/Browser/webview-thumbnailer.cpp	2013-08-06 10:16:28 +0000
+++ src/Ubuntu/Components/Extras/Browser/webview-thumbnailer.cpp	2013-08-09 07:47:22 +0000
@@ -16,12 +16,11 @@
  * along with this program.  If not, see <http://www.gnu.org/licenses/>.
  */
 
-#include "domain-utils.h"
 #include "webthumbnail-utils.h"
 #include "webview-thumbnailer.h"
 
 // Qt
-#include <QtCore/QFile>
+#include <QtCore/QMetaObject>
 #include <QtCore/QTimer>
 #include <QtQuick/private/qsgrenderer_p.h>
 #include <QtWebKit/private/qquickwebpage_p.h>
@@ -140,20 +139,12 @@
 
     fbo.release();
 
+    const QUrl& url = m_webview->url();
     QImage image = fbo.toImage().scaled(m_targetSize, Qt::KeepAspectRatioByExpanding, Qt::SmoothTransformation);
 
-    WebThumbnailUtils::ensureCacheLocation();
-    QUrl url = m_webview->url();
-    QFileInfo thumbnail = WebThumbnailUtils::thumbnailFile(url);
-    bool saved = image.save(thumbnail.absoluteFilePath());
-
-    // Make a link to the thumbnail file for the corresponding domain’s thumbnail.
-    QUrl domain(DomainUtils::extractTopLevelDomainName(url));
-    QString domainThumbnail = WebThumbnailUtils::thumbnailFile(domain).absoluteFilePath();
-    if (QFile::exists(domainThumbnail)) {
-        QFile::remove(domainThumbnail);
-    }
-    QFile::link(thumbnail.fileName(), domainThumbnail);
+    // Invoke the method asynchronously.
+    QMetaObject::invokeMethod(&WebThumbnailUtils::instance(), "cacheThumbnail",
+                              Qt::QueuedConnection, Q_ARG(QUrl, url), Q_ARG(QImage, image));
 
     root.removeChildNode(node);
 
@@ -165,9 +156,7 @@
         }
     }
 
-    if (saved) {
-        Q_EMIT thumbnailRendered(url);
-    }
+    Q_EMIT thumbnailRendered(url);
 
     return oldNode;
 }

=== modified file 'tests/unittests/history-domainlist-chronological-model/CMakeLists.txt'
--- tests/unittests/history-domainlist-chronological-model/CMakeLists.txt	2013-07-29 10:32:13 +0000
+++ tests/unittests/history-domainlist-chronological-model/CMakeLists.txt	2013-08-09 07:47:22 +0000
@@ -6,7 +6,6 @@
     ${webbrowser-plugin_SOURCE_DIR}/history-domainlist-chronological-model.cpp
     ${webbrowser-plugin_SOURCE_DIR}/history-model.cpp
     ${webbrowser-plugin_SOURCE_DIR}/history-timeframe-model.cpp
-    ${webbrowser-plugin_SOURCE_DIR}/webthumbnail-utils.cpp
     tst_HistoryDomainListChronologicalModelTests.cpp
 )
 add_executable(${TEST} ${SOURCES})

=== modified file 'tests/unittests/history-domainlist-model/CMakeLists.txt'
--- tests/unittests/history-domainlist-model/CMakeLists.txt	2013-07-29 10:32:13 +0000
+++ tests/unittests/history-domainlist-model/CMakeLists.txt	2013-08-09 07:47:22 +0000
@@ -5,7 +5,6 @@
     ${webbrowser-plugin_SOURCE_DIR}/history-domainlist-model.cpp
     ${webbrowser-plugin_SOURCE_DIR}/history-model.cpp
     ${webbrowser-plugin_SOURCE_DIR}/history-timeframe-model.cpp
-    ${webbrowser-plugin_SOURCE_DIR}/webthumbnail-utils.cpp
     tst_HistoryDomainListModelTests.cpp
 )
 add_executable(${TEST} ${SOURCES})