Merge lp:~osomon/oxide/maxCacheSize into lp:~oxide-developers/oxide/oxide.trunk

[Olivier Tilloy]

Add WebContext.maxCacheSize property.

Note to the reviewer: bug #1277659 also mentions lowering the default value of 80MB. In this changeset I left it unchanged, let’s lower it in a separate branch after we agree on what a good default value would be.

[Alexandre Abreu (abreu-alexandre)]

would it make sense to reflect in the name of the prop that the cache size limit is a soft limit? A few comments inline,

[lp:~osomon/oxide/maxCacheSize]

1003. By Olivier Tilloy on 2015-03-06

Default to 0 for the max cache size: chromium will pick a size based on available disk space.

1004. By Olivier Tilloy on 2015-03-06

Rename maxCacheSize to maxCacheSizeHint to reflect the fact that the limit is a soft one.

1005. By Olivier Tilloy on 2015-03-06

Do not allow setting the max cache size hint to a negative value.

[Olivier Tilloy (osomon)]

I renamed maxCacheSize to maxCacheSizeHint, changed the default value to 0 (which lets chromium choose a sensible value based on the available disk space), and enforced a positive value (cannot make the property a uint because net::HttpCache::DefaultBackend’s constructor takes an int).

[Chris Coulson (chrisccoulson)]

This looks mostly ok - I just have one small comment on the API. Could we make the unit for WebContext.maxCacheSizeHint MB instead of bytes? The initial cache size is about 1MB when it's created anyway, and given that this is not a hard limit I don't think it makes sense to create an illusion that applications can control the cache size to the nearest byte.

Other than that, it looks fine.

review: Approve

[lp:~osomon/oxide/maxCacheSize]

1006. By Olivier Tilloy on 2015-03-10

Make the unit for maxCacheSizeHint MB instead of bytes.
The initial cache size is about 1MB when it's created anyway, and given that this is not a hard limit it doesn’t make sense to create an illusion that applications can control the cache size to the nearest byte.

1007. By Olivier Tilloy on 2015-03-10

Enforce an upper limit to avoid integer overflow.

[Olivier Tilloy (osomon)]

Your suggestion makes sense, I changed the unit to MB instead of bytes.
To prevent integer overflow I enforced an upper limit. I did that in the QML API, do you think it would make sense to have the check in the shared layer?

[Chris Coulson (chrisccoulson)]

I think the current approach is ok - it's preferable for argument validation to go near to their API's in qt/ because we can be consistent with the logging mechanism we use (although, this could probably actually go in WebContextAdapter). I'd be tempted to just DCHECK that the passed in size isn't too big in BrowserContextIOData though, to make it clear to anyone who wanted to write a new port that they need to limit the size.

[lp:~osomon/oxide/maxCacheSize]

1008. By Olivier Tilloy on 2015-03-11

Add a DCHECK for the passed in size in BrowserContextIOData,
to make it clear to anyone who writes a new port that they need to limit the size.

[Olivier Tilloy (osomon)]

I added the DCHECK in BrowserContextIOData as suggested.

I see two problems with moving the Qt port check to WebContextAdapter though:

 - (minor) the warning message cannot mention "WebContext.maxCacheSizeHint" as we’re not guaranteed the class and property would be called like this in a QtWidgets port

 - maxCacheSizeHintChanged() would still be emitted from the QML API even if an integer overflow (or a value < 0) was detected and the size hint was not actually changed, unless we change the signature of WebContextAdapter::setMaxCacheSizeHint(…) to return a boolean indicating success

=== modified file 'qt/core/browser/oxide_qt_web_context.cc'
--- qt/core/browser/oxide_qt_web_context.cc	2015-01-16 22:46:17 +0000
+++ qt/core/browser/oxide_qt_web_context.cc	2015-03-11 15:05:58 +0000
@@ -1,5 +1,5 @@
 // vim:expandtab:shiftwidth=2:tabstop=2:
-// Copyright (C) 2013 Canonical Ltd.
+// Copyright (C) 2013-2015 Canonical Ltd.
 
 // This library is free software; you can redistribute it and/or
 // modify it under the terms of the GNU Lesser General Public
@@ -78,6 +78,7 @@
 };
 
 WebContext::ConstructProperties::ConstructProperties() :
+    max_cache_size_hint(0),
     cookie_policy(net::StaticCookiePolicy::ALLOW_ALL_COOKIES),
     session_cookie_mode(content::CookieStoreConfig::EPHEMERAL_SESSION_COOKIES),
     popup_blocker_enabled(true),
@@ -523,6 +524,7 @@
   oxide::BrowserContext::Params params(
       construct_props_->data_path,
       construct_props_->cache_path,
+      construct_props_->max_cache_size_hint,
       construct_props_->session_cookie_mode,
       construct_props_->devtools_enabled,
       construct_props_->devtools_port,
@@ -745,5 +747,18 @@
   construct_props_->host_mapping_rules = rules;
 }
 
+int WebContext::GetMaxCacheSizeHint() const {
+  if (IsInitialized()) {
+    return context_->GetMaxCacheSizeHint();
+  }
+
+  return construct_props_->max_cache_size_hint;
+}
+
+void WebContext::SetMaxCacheSizeHint(int size) {
+  DCHECK(!IsInitialized());
+  construct_props_->max_cache_size_hint = size;
+}
+
 } // namespace qt
 } // namespace oxide
 
=== modified file 'qt/core/browser/oxide_qt_web_context.h'
--- qt/core/browser/oxide_qt_web_context.h	2014-11-13 09:17:40 +0000
+++ qt/core/browser/oxide_qt_web_context.h	2015-03-11 15:05:58 +0000
@@ -1,5 +1,5 @@
 // vim:expandtab:shiftwidth=2:tabstop=2:
-// Copyright (C) 2013 Canonical Ltd.
+// Copyright (C) 2013-2015 Canonical Ltd.
 
 // This library is free software; you can redistribute it and/or
 // modify it under the terms of the GNU Lesser General Public
@@ -110,6 +110,9 @@
   std::vector<std::string> GetHostMappingRules() const;
   void SetHostMappingRules(const std::vector<std::string>& rules);
 
+  int GetMaxCacheSizeHint() const;
+  void SetMaxCacheSizeHint(int size);
+
  private:
   friend class WebContextAdapter;
 
@@ -120,6 +123,7 @@
     std::string user_agent;
     base::FilePath data_path;
     base::FilePath cache_path;
+    int max_cache_size_hint;
     std::string accept_langs;
     net::StaticCookiePolicy::Type cookie_policy;
     content::CookieStoreConfig::SessionCookieMode session_cookie_mode;
 
=== modified file 'qt/core/glue/oxide_qt_web_context_adapter.cc'
--- qt/core/glue/oxide_qt_web_context_adapter.cc	2015-01-16 22:46:17 +0000
+++ qt/core/glue/oxide_qt_web_context_adapter.cc	2015-03-11 15:05:58 +0000
@@ -1,5 +1,5 @@
 // vim:expandtab:shiftwidth=2:tabstop=2:
-// Copyright (C) 2013 Canonical Ltd.
+// Copyright (C) 2013-2015 Canonical Ltd.
 
 // This library is free software; you can redistribute it and/or
 // modify it under the terms of the GNU Lesser General Public
@@ -276,6 +276,14 @@
   context_->SetAllowedExtraURLSchemes(set);
 }
 
+int WebContextAdapter::maxCacheSizeHint() const {
+  return context_->GetMaxCacheSizeHint();
+}
+
+void WebContextAdapter::setMaxCacheSizeHint(int size) {
+  context_->SetMaxCacheSizeHint(size);
+}
+
 WebContextAdapter::WebContextAdapter(QObject* q)
     : AdapterBase(q),
       context_(WebContext::Create(this)) {
 
=== modified file 'qt/core/glue/oxide_qt_web_context_adapter.h'
--- qt/core/glue/oxide_qt_web_context_adapter.h	2014-11-13 09:17:40 +0000
+++ qt/core/glue/oxide_qt_web_context_adapter.h	2015-03-11 15:05:58 +0000
@@ -1,5 +1,5 @@
 // vim:expandtab:shiftwidth=2:tabstop=2:
-// Copyright (C) 2013 Canonical Ltd.
+// Copyright (C) 2013-2015 Canonical Ltd.
 
 // This library is free software; you can redistribute it and/or
 // modify it under the terms of the GNU Lesser General Public
@@ -132,6 +132,9 @@
 
   void setAllowedExtraUrlSchemes(const QStringList& schemes);
 
+  int maxCacheSizeHint() const;
+  void setMaxCacheSizeHint(int size);
+
  protected:
   WebContextAdapter(QObject* q);
 
 
=== modified file 'qt/qmlplugin/oxide_qml_plugin.cc'
--- qt/qmlplugin/oxide_qml_plugin.cc	2015-01-03 19:27:04 +0000
+++ qt/qmlplugin/oxide_qml_plugin.cc	2015-03-11 15:05:58 +0000
@@ -1,5 +1,5 @@
 // vim:expandtab:shiftwidth=2:tabstop=2:
-// Copyright (C) 2013 Canonical Ltd.
+// Copyright (C) 2013-2015 Canonical Ltd.
 
 // This library is free software; you can redistribute it and/or
 // modify it under the terms of the GNU Lesser General Public
@@ -122,6 +122,8 @@
     qmlRegisterType<OxideQQuickWebView, 2>(uri, 1, 4, "WebView");
 
     qmlRegisterType<OxideQQuickWebView, 3>(uri, 1, 5, "WebView");
+
+    qmlRegisterType<OxideQQuickWebContext, 2>(uri, 1, 6, "WebContext");
   }
 };
 
 
=== modified file 'qt/quick/api/oxideqquickwebcontext.cc'
--- qt/quick/api/oxideqquickwebcontext.cc	2015-02-19 22:56:12 +0000
+++ qt/quick/api/oxideqquickwebcontext.cc	2015-03-11 15:05:58 +0000
@@ -1,5 +1,5 @@
 // vim:expandtab:shiftwidth=2:tabstop=2:
-// Copyright (C) 2013 Canonical Ltd.
+// Copyright (C) 2013-2015 Canonical Ltd.
 
 // This library is free software; you can redistribute it and/or
 // modify it under the terms of the GNU Lesser General Public
@@ -18,6 +18,8 @@
 #include "oxideqquickwebcontext_p.h"
 #include "oxideqquickwebcontext_p_p.h"
 
+#include <limits>
+
 #include <QMutex>
 #include <QMutexLocker>
 #include <QQmlEngine>
@@ -896,4 +898,39 @@
   emit allowedExtraUrlSchemesChanged();
 }
 
+int OxideQQuickWebContext::maxCacheSizeHint() const {
+  Q_D(const OxideQQuickWebContext);
+
+  return d->maxCacheSizeHint();
+}
+
+void OxideQQuickWebContext::setMaxCacheSizeHint(int size) {
+  Q_D(OxideQQuickWebContext);
+
+  if (size < 0) {
+    qWarning() << "WebContext.maxCacheSizeHint cannot have a negative value";
+    return;
+  }
+
+  static int upper_limit = std::numeric_limits<int>::max() / (1024 * 1024);
+  if (size > upper_limit) {
+    // To avoid integer overflow.
+    qWarning() << "WebContext.maxCacheSizeHint cannot exceed"
+               << upper_limit << "MB";
+    return;
+  }
+
+  if (d->isInitialized()) {
+    qWarning() << "Cannot set WebContext.maxCacheSizeHint once the context is in use";
+    return;
+  }
+
+  if (d->maxCacheSizeHint() == size) {
+    return;
+  }
+
+  d->setMaxCacheSizeHint(size);
+  emit maxCacheSizeHintChanged();
+}
+
 #include "moc_oxideqquickwebcontext_p.cpp"
 
=== modified file 'qt/quick/api/oxideqquickwebcontext_p.h'
--- qt/quick/api/oxideqquickwebcontext_p.h	2015-01-16 22:46:17 +0000
+++ qt/quick/api/oxideqquickwebcontext_p.h	2015-03-11 15:05:58 +0000
@@ -1,5 +1,5 @@
 // vim:expandtab:shiftwidth=2:tabstop=2:
-// Copyright (C) 2013 Canonical Ltd.
+// Copyright (C) 2013-2015 Canonical Ltd.
 
 // This library is free software; you can redistribute it and/or
 // modify it under the terms of the GNU Lesser General Public
@@ -64,6 +64,9 @@
 
   Q_PROPERTY(QStringList allowedExtraUrlSchemes READ allowedExtraUrlSchemes WRITE setAllowedExtraUrlSchemes NOTIFY allowedExtraUrlSchemesChanged REVISION 1)
 
+  // maxCacheSizeHint is a soft limit, expressed in MB
+  Q_PROPERTY(int maxCacheSizeHint READ maxCacheSizeHint WRITE setMaxCacheSizeHint NOTIFY maxCacheSizeHintChanged REVISION 2)
+
   Q_ENUMS(CookiePolicy)
   Q_ENUMS(SessionCookieMode)
 
@@ -148,6 +151,9 @@
   QStringList allowedExtraUrlSchemes() const;
   void setAllowedExtraUrlSchemes(const QStringList& schemes);
 
+  int maxCacheSizeHint() const;
+  void setMaxCacheSizeHint(int size);
+
  Q_SIGNALS:
   void productChanged();
   void userAgentChanged();
@@ -166,6 +172,7 @@
   void devtoolsBindIpChanged();
   Q_REVISION(1) void hostMappingRulesChanged();
   Q_REVISION(1) void allowedExtraUrlSchemesChanged();
+  Q_REVISION(2) void maxCacheSizeHintChanged();
 
  private:
   Q_PRIVATE_SLOT(d_func(), void userScriptUpdated());
 
=== modified file 'qt/tests/qmltests/api/tst_WebContext_semi_construct_only_properties.qml'
--- qt/tests/qmltests/api/tst_WebContext_semi_construct_only_properties.qml	2014-11-13 19:14:40 +0000
+++ qt/tests/qmltests/api/tst_WebContext_semi_construct_only_properties.qml	2015-03-11 15:05:58 +0000
@@ -1,6 +1,6 @@
 import QtQuick 2.0
 import QtTest 1.0
-import com.canonical.Oxide 1.0
+import com.canonical.Oxide 1.6
 import com.canonical.Oxide.Testing 1.0
 
 TestCase {
@@ -29,6 +29,7 @@
     var r = [
       { prop: "dataPath", signal: "dataPathChanged", val: "file:///foo", dataPath: "" },
       { prop: "cachePath", signal: "cachePathChanged", val: "file:///foo", dataPath: "" },
+      { prop: "maxCacheSizeHint", signal: "maxCacheSizeHintChanged", val: 1, dataPath: "" },
       { prop: "sessionCookieMode", signal: "sessionCookieModeChanged", val: WebContext.SessionCookieModeRestored, dataPath: QMLTEST_DATADIR }
     ];
 
 
=== modified file 'shared/browser/oxide_browser_context.cc'
--- shared/browser/oxide_browser_context.cc	2015-03-10 20:34:40 +0000
+++ shared/browser/oxide_browser_context.cc	2015-03-11 15:05:58 +0000
@@ -1,5 +1,5 @@
 // vim:expandtab:shiftwidth=2:tabstop=2:
-// Copyright (C) 2013 Canonical Ltd.
+// Copyright (C) 2013-2015 Canonical Ltd.
 
 // This library is free software; you can redistribute it and/or
 // modify it under the terms of the GNU Lesser General Public
@@ -19,6 +19,7 @@
 
 #include <algorithm>
 #include <libintl.h>
+#include <limits>
 #include <vector>
 
 #include "base/files/file_enumerator.h"
@@ -259,6 +260,7 @@
   BrowserContextSharedIOData(const BrowserContext::Params& params)
       : path(params.path),
         cache_path(params.cache_path),
+        max_cache_size_hint(params.max_cache_size_hint),
         cookie_policy(net::StaticCookiePolicy::ALLOW_ALL_COOKIES),
         session_cookie_mode(params.session_cookie_mode),
         popup_blocker_enabled(true),
@@ -274,6 +276,7 @@
 
   base::FilePath path;
   base::FilePath cache_path;
+  int max_cache_size_hint;
 
   std::string user_agent_string;
   std::string accept_langs;
@@ -411,6 +414,15 @@
   return data.cache_path;
 }
 
+int BrowserContextIOData::GetMaxCacheSizeHint() const {
+  int max_cache_size_hint = GetSharedData().max_cache_size_hint;
+  // max_cache_size_hint is expressed in MB, let’s check that
+  // converting it to bytes won’t trigger an integer overflow
+  static int upper_limit = std::numeric_limits<int>::max() / (1024 * 1024);
+  DCHECK_LE(max_cache_size_hint, upper_limit);
+  return max_cache_size_hint;
+}
+
 std::string BrowserContextIOData::GetAcceptLangs() const {
   const BrowserContextSharedIOData& data = GetSharedData();
   base::AutoLock lock(data.lock);
@@ -491,7 +503,7 @@
           net::DISK_CACHE,
           net::CACHE_BACKEND_DEFAULT,
           GetCachePath().Append(kCacheDirname),
-          83886080, // XXX: 80MB - Make this configurable
+          GetMaxCacheSizeHint() * 1024 * 1024, // MB -> bytes
           content::BrowserThread::GetMessageLoopProxyForThread(
               content::BrowserThread::CACHE));
   }
@@ -914,6 +926,11 @@
   return io_data()->GetCachePath();
 }
 
+int BrowserContext::GetMaxCacheSizeHint() const {
+  DCHECK(CalledOnValidThread());
+  return io_data()->GetMaxCacheSizeHint();
+}
+
 std::string BrowserContext::GetAcceptLangs() const {
   DCHECK(CalledOnValidThread());
   return io_data()->GetAcceptLangs();
 
=== modified file 'shared/browser/oxide_browser_context.h'
--- shared/browser/oxide_browser_context.h	2014-12-03 07:22:41 +0000
+++ shared/browser/oxide_browser_context.h	2015-03-11 15:05:58 +0000
@@ -1,5 +1,5 @@
 // vim:expandtab:shiftwidth=2:tabstop=2:
-// Copyright (C) 2013 Canonical Ltd.
+// Copyright (C) 2013-2015 Canonical Ltd.
 
 // This library is free software; you can redistribute it and/or
 // modify it under the terms of the GNU Lesser General Public
@@ -80,6 +80,7 @@
 
   base::FilePath GetPath() const;
   base::FilePath GetCachePath() const;
+  int GetMaxCacheSizeHint() const;
 
   std::string GetAcceptLangs() const;
   std::string GetUserAgent() const;
@@ -132,12 +133,14 @@
   struct Params {
     Params(const base::FilePath& path,
            const base::FilePath& cache_path,
+           int max_cache_size_hint,
            content::CookieStoreConfig::SessionCookieMode session_cookie_mode,
            bool devtools_enabled,
            int devtools_port,
            const std::string& devtools_ip)
         : path(path),
           cache_path(cache_path),
+          max_cache_size_hint(max_cache_size_hint),
           session_cookie_mode(session_cookie_mode),
           devtools_enabled(devtools_enabled),
           devtools_port(devtools_port),
@@ -145,6 +148,7 @@
 
     base::FilePath path;
     base::FilePath cache_path;
+    int max_cache_size_hint;
     content::CookieStoreConfig::SessionCookieMode session_cookie_mode;
     bool devtools_enabled;
     int devtools_port;
@@ -183,6 +187,7 @@
 
   base::FilePath GetPath() const final;
   base::FilePath GetCachePath() const;
+  int GetMaxCacheSizeHint() const;
 
   std::string GetAcceptLangs() const;
   void SetAcceptLangs(const std::string& langs);