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

Proposed by Olivier Tilloy on 2015-03-06
Status: Merged
Merged at revision: 1011
Proposed branch: lp:~osomon/oxide/maxCacheSize
Merge into: lp:~oxide-developers/oxide/oxide.trunk
Diff against target: 382 lines (+110/-11)
10 files modified
qt/core/browser/oxide_qt_web_context.cc (+16/-1)
qt/core/browser/oxide_qt_web_context.h (+5/-1)
qt/core/glue/oxide_qt_web_context_adapter.cc (+9/-1)
qt/core/glue/oxide_qt_web_context_adapter.h (+4/-1)
qt/qmlplugin/oxide_qml_plugin.cc (+3/-1)
qt/quick/api/oxideqquickwebcontext.cc (+38/-1)
qt/quick/api/oxideqquickwebcontext_p.h (+8/-1)
qt/tests/qmltests/api/tst_WebContext_semi_construct_only_properties.qml (+2/-1)
shared/browser/oxide_browser_context.cc (+19/-2)
shared/browser/oxide_browser_context.h (+6/-1)
To merge this branch: bzr merge lp:~osomon/oxide/maxCacheSize
Reviewer Review Type Date Requested Status
Chris Coulson 2015-03-06 Approve on 2015-03-10
Review via email: mp+252113@code.launchpad.net

Commit message

Add WebContext.maxCacheSize property.

Description of the change

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.

To post a comment you must log in.
Alexandre Abreu (abreu-alexandre) wrote :

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 updated on 2015-03-06
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) wrote :

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) wrote :

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 updated on 2015-03-10
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) wrote :

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) wrote :

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 updated on 2015-03-11
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) wrote :

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

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'qt/core/browser/oxide_qt_web_context.cc'
2--- qt/core/browser/oxide_qt_web_context.cc 2015-01-16 22:46:17 +0000
3+++ qt/core/browser/oxide_qt_web_context.cc 2015-03-11 15:05:58 +0000
4@@ -1,5 +1,5 @@
5 // vim:expandtab:shiftwidth=2:tabstop=2:
6-// Copyright (C) 2013 Canonical Ltd.
7+// Copyright (C) 2013-2015 Canonical Ltd.
8
9 // This library is free software; you can redistribute it and/or
10 // modify it under the terms of the GNU Lesser General Public
11@@ -78,6 +78,7 @@
12 };
13
14 WebContext::ConstructProperties::ConstructProperties() :
15+ max_cache_size_hint(0),
16 cookie_policy(net::StaticCookiePolicy::ALLOW_ALL_COOKIES),
17 session_cookie_mode(content::CookieStoreConfig::EPHEMERAL_SESSION_COOKIES),
18 popup_blocker_enabled(true),
19@@ -523,6 +524,7 @@
20 oxide::BrowserContext::Params params(
21 construct_props_->data_path,
22 construct_props_->cache_path,
23+ construct_props_->max_cache_size_hint,
24 construct_props_->session_cookie_mode,
25 construct_props_->devtools_enabled,
26 construct_props_->devtools_port,
27@@ -745,5 +747,18 @@
28 construct_props_->host_mapping_rules = rules;
29 }
30
31+int WebContext::GetMaxCacheSizeHint() const {
32+ if (IsInitialized()) {
33+ return context_->GetMaxCacheSizeHint();
34+ }
35+
36+ return construct_props_->max_cache_size_hint;
37+}
38+
39+void WebContext::SetMaxCacheSizeHint(int size) {
40+ DCHECK(!IsInitialized());
41+ construct_props_->max_cache_size_hint = size;
42+}
43+
44 } // namespace qt
45 } // namespace oxide
46
47=== modified file 'qt/core/browser/oxide_qt_web_context.h'
48--- qt/core/browser/oxide_qt_web_context.h 2014-11-13 09:17:40 +0000
49+++ qt/core/browser/oxide_qt_web_context.h 2015-03-11 15:05:58 +0000
50@@ -1,5 +1,5 @@
51 // vim:expandtab:shiftwidth=2:tabstop=2:
52-// Copyright (C) 2013 Canonical Ltd.
53+// Copyright (C) 2013-2015 Canonical Ltd.
54
55 // This library is free software; you can redistribute it and/or
56 // modify it under the terms of the GNU Lesser General Public
57@@ -110,6 +110,9 @@
58 std::vector<std::string> GetHostMappingRules() const;
59 void SetHostMappingRules(const std::vector<std::string>& rules);
60
61+ int GetMaxCacheSizeHint() const;
62+ void SetMaxCacheSizeHint(int size);
63+
64 private:
65 friend class WebContextAdapter;
66
67@@ -120,6 +123,7 @@
68 std::string user_agent;
69 base::FilePath data_path;
70 base::FilePath cache_path;
71+ int max_cache_size_hint;
72 std::string accept_langs;
73 net::StaticCookiePolicy::Type cookie_policy;
74 content::CookieStoreConfig::SessionCookieMode session_cookie_mode;
75
76=== modified file 'qt/core/glue/oxide_qt_web_context_adapter.cc'
77--- qt/core/glue/oxide_qt_web_context_adapter.cc 2015-01-16 22:46:17 +0000
78+++ qt/core/glue/oxide_qt_web_context_adapter.cc 2015-03-11 15:05:58 +0000
79@@ -1,5 +1,5 @@
80 // vim:expandtab:shiftwidth=2:tabstop=2:
81-// Copyright (C) 2013 Canonical Ltd.
82+// Copyright (C) 2013-2015 Canonical Ltd.
83
84 // This library is free software; you can redistribute it and/or
85 // modify it under the terms of the GNU Lesser General Public
86@@ -276,6 +276,14 @@
87 context_->SetAllowedExtraURLSchemes(set);
88 }
89
90+int WebContextAdapter::maxCacheSizeHint() const {
91+ return context_->GetMaxCacheSizeHint();
92+}
93+
94+void WebContextAdapter::setMaxCacheSizeHint(int size) {
95+ context_->SetMaxCacheSizeHint(size);
96+}
97+
98 WebContextAdapter::WebContextAdapter(QObject* q)
99 : AdapterBase(q),
100 context_(WebContext::Create(this)) {
101
102=== modified file 'qt/core/glue/oxide_qt_web_context_adapter.h'
103--- qt/core/glue/oxide_qt_web_context_adapter.h 2014-11-13 09:17:40 +0000
104+++ qt/core/glue/oxide_qt_web_context_adapter.h 2015-03-11 15:05:58 +0000
105@@ -1,5 +1,5 @@
106 // vim:expandtab:shiftwidth=2:tabstop=2:
107-// Copyright (C) 2013 Canonical Ltd.
108+// Copyright (C) 2013-2015 Canonical Ltd.
109
110 // This library is free software; you can redistribute it and/or
111 // modify it under the terms of the GNU Lesser General Public
112@@ -132,6 +132,9 @@
113
114 void setAllowedExtraUrlSchemes(const QStringList& schemes);
115
116+ int maxCacheSizeHint() const;
117+ void setMaxCacheSizeHint(int size);
118+
119 protected:
120 WebContextAdapter(QObject* q);
121
122
123=== modified file 'qt/qmlplugin/oxide_qml_plugin.cc'
124--- qt/qmlplugin/oxide_qml_plugin.cc 2015-01-03 19:27:04 +0000
125+++ qt/qmlplugin/oxide_qml_plugin.cc 2015-03-11 15:05:58 +0000
126@@ -1,5 +1,5 @@
127 // vim:expandtab:shiftwidth=2:tabstop=2:
128-// Copyright (C) 2013 Canonical Ltd.
129+// Copyright (C) 2013-2015 Canonical Ltd.
130
131 // This library is free software; you can redistribute it and/or
132 // modify it under the terms of the GNU Lesser General Public
133@@ -122,6 +122,8 @@
134 qmlRegisterType<OxideQQuickWebView, 2>(uri, 1, 4, "WebView");
135
136 qmlRegisterType<OxideQQuickWebView, 3>(uri, 1, 5, "WebView");
137+
138+ qmlRegisterType<OxideQQuickWebContext, 2>(uri, 1, 6, "WebContext");
139 }
140 };
141
142
143=== modified file 'qt/quick/api/oxideqquickwebcontext.cc'
144--- qt/quick/api/oxideqquickwebcontext.cc 2015-02-19 22:56:12 +0000
145+++ qt/quick/api/oxideqquickwebcontext.cc 2015-03-11 15:05:58 +0000
146@@ -1,5 +1,5 @@
147 // vim:expandtab:shiftwidth=2:tabstop=2:
148-// Copyright (C) 2013 Canonical Ltd.
149+// Copyright (C) 2013-2015 Canonical Ltd.
150
151 // This library is free software; you can redistribute it and/or
152 // modify it under the terms of the GNU Lesser General Public
153@@ -18,6 +18,8 @@
154 #include "oxideqquickwebcontext_p.h"
155 #include "oxideqquickwebcontext_p_p.h"
156
157+#include <limits>
158+
159 #include <QMutex>
160 #include <QMutexLocker>
161 #include <QQmlEngine>
162@@ -896,4 +898,39 @@
163 emit allowedExtraUrlSchemesChanged();
164 }
165
166+int OxideQQuickWebContext::maxCacheSizeHint() const {
167+ Q_D(const OxideQQuickWebContext);
168+
169+ return d->maxCacheSizeHint();
170+}
171+
172+void OxideQQuickWebContext::setMaxCacheSizeHint(int size) {
173+ Q_D(OxideQQuickWebContext);
174+
175+ if (size < 0) {
176+ qWarning() << "WebContext.maxCacheSizeHint cannot have a negative value";
177+ return;
178+ }
179+
180+ static int upper_limit = std::numeric_limits<int>::max() / (1024 * 1024);
181+ if (size > upper_limit) {
182+ // To avoid integer overflow.
183+ qWarning() << "WebContext.maxCacheSizeHint cannot exceed"
184+ << upper_limit << "MB";
185+ return;
186+ }
187+
188+ if (d->isInitialized()) {
189+ qWarning() << "Cannot set WebContext.maxCacheSizeHint once the context is in use";
190+ return;
191+ }
192+
193+ if (d->maxCacheSizeHint() == size) {
194+ return;
195+ }
196+
197+ d->setMaxCacheSizeHint(size);
198+ emit maxCacheSizeHintChanged();
199+}
200+
201 #include "moc_oxideqquickwebcontext_p.cpp"
202
203=== modified file 'qt/quick/api/oxideqquickwebcontext_p.h'
204--- qt/quick/api/oxideqquickwebcontext_p.h 2015-01-16 22:46:17 +0000
205+++ qt/quick/api/oxideqquickwebcontext_p.h 2015-03-11 15:05:58 +0000
206@@ -1,5 +1,5 @@
207 // vim:expandtab:shiftwidth=2:tabstop=2:
208-// Copyright (C) 2013 Canonical Ltd.
209+// Copyright (C) 2013-2015 Canonical Ltd.
210
211 // This library is free software; you can redistribute it and/or
212 // modify it under the terms of the GNU Lesser General Public
213@@ -64,6 +64,9 @@
214
215 Q_PROPERTY(QStringList allowedExtraUrlSchemes READ allowedExtraUrlSchemes WRITE setAllowedExtraUrlSchemes NOTIFY allowedExtraUrlSchemesChanged REVISION 1)
216
217+ // maxCacheSizeHint is a soft limit, expressed in MB
218+ Q_PROPERTY(int maxCacheSizeHint READ maxCacheSizeHint WRITE setMaxCacheSizeHint NOTIFY maxCacheSizeHintChanged REVISION 2)
219+
220 Q_ENUMS(CookiePolicy)
221 Q_ENUMS(SessionCookieMode)
222
223@@ -148,6 +151,9 @@
224 QStringList allowedExtraUrlSchemes() const;
225 void setAllowedExtraUrlSchemes(const QStringList& schemes);
226
227+ int maxCacheSizeHint() const;
228+ void setMaxCacheSizeHint(int size);
229+
230 Q_SIGNALS:
231 void productChanged();
232 void userAgentChanged();
233@@ -166,6 +172,7 @@
234 void devtoolsBindIpChanged();
235 Q_REVISION(1) void hostMappingRulesChanged();
236 Q_REVISION(1) void allowedExtraUrlSchemesChanged();
237+ Q_REVISION(2) void maxCacheSizeHintChanged();
238
239 private:
240 Q_PRIVATE_SLOT(d_func(), void userScriptUpdated());
241
242=== modified file 'qt/tests/qmltests/api/tst_WebContext_semi_construct_only_properties.qml'
243--- qt/tests/qmltests/api/tst_WebContext_semi_construct_only_properties.qml 2014-11-13 19:14:40 +0000
244+++ qt/tests/qmltests/api/tst_WebContext_semi_construct_only_properties.qml 2015-03-11 15:05:58 +0000
245@@ -1,6 +1,6 @@
246 import QtQuick 2.0
247 import QtTest 1.0
248-import com.canonical.Oxide 1.0
249+import com.canonical.Oxide 1.6
250 import com.canonical.Oxide.Testing 1.0
251
252 TestCase {
253@@ -29,6 +29,7 @@
254 var r = [
255 { prop: "dataPath", signal: "dataPathChanged", val: "file:///foo", dataPath: "" },
256 { prop: "cachePath", signal: "cachePathChanged", val: "file:///foo", dataPath: "" },
257+ { prop: "maxCacheSizeHint", signal: "maxCacheSizeHintChanged", val: 1, dataPath: "" },
258 { prop: "sessionCookieMode", signal: "sessionCookieModeChanged", val: WebContext.SessionCookieModeRestored, dataPath: QMLTEST_DATADIR }
259 ];
260
261
262=== modified file 'shared/browser/oxide_browser_context.cc'
263--- shared/browser/oxide_browser_context.cc 2015-03-10 20:34:40 +0000
264+++ shared/browser/oxide_browser_context.cc 2015-03-11 15:05:58 +0000
265@@ -1,5 +1,5 @@
266 // vim:expandtab:shiftwidth=2:tabstop=2:
267-// Copyright (C) 2013 Canonical Ltd.
268+// Copyright (C) 2013-2015 Canonical Ltd.
269
270 // This library is free software; you can redistribute it and/or
271 // modify it under the terms of the GNU Lesser General Public
272@@ -19,6 +19,7 @@
273
274 #include <algorithm>
275 #include <libintl.h>
276+#include <limits>
277 #include <vector>
278
279 #include "base/files/file_enumerator.h"
280@@ -259,6 +260,7 @@
281 BrowserContextSharedIOData(const BrowserContext::Params& params)
282 : path(params.path),
283 cache_path(params.cache_path),
284+ max_cache_size_hint(params.max_cache_size_hint),
285 cookie_policy(net::StaticCookiePolicy::ALLOW_ALL_COOKIES),
286 session_cookie_mode(params.session_cookie_mode),
287 popup_blocker_enabled(true),
288@@ -274,6 +276,7 @@
289
290 base::FilePath path;
291 base::FilePath cache_path;
292+ int max_cache_size_hint;
293
294 std::string user_agent_string;
295 std::string accept_langs;
296@@ -411,6 +414,15 @@
297 return data.cache_path;
298 }
299
300+int BrowserContextIOData::GetMaxCacheSizeHint() const {
301+ int max_cache_size_hint = GetSharedData().max_cache_size_hint;
302+ // max_cache_size_hint is expressed in MB, let’s check that
303+ // converting it to bytes won’t trigger an integer overflow
304+ static int upper_limit = std::numeric_limits<int>::max() / (1024 * 1024);
305+ DCHECK_LE(max_cache_size_hint, upper_limit);
306+ return max_cache_size_hint;
307+}
308+
309 std::string BrowserContextIOData::GetAcceptLangs() const {
310 const BrowserContextSharedIOData& data = GetSharedData();
311 base::AutoLock lock(data.lock);
312@@ -491,7 +503,7 @@
313 net::DISK_CACHE,
314 net::CACHE_BACKEND_DEFAULT,
315 GetCachePath().Append(kCacheDirname),
316- 83886080, // XXX: 80MB - Make this configurable
317+ GetMaxCacheSizeHint() * 1024 * 1024, // MB -> bytes
318 content::BrowserThread::GetMessageLoopProxyForThread(
319 content::BrowserThread::CACHE));
320 }
321@@ -914,6 +926,11 @@
322 return io_data()->GetCachePath();
323 }
324
325+int BrowserContext::GetMaxCacheSizeHint() const {
326+ DCHECK(CalledOnValidThread());
327+ return io_data()->GetMaxCacheSizeHint();
328+}
329+
330 std::string BrowserContext::GetAcceptLangs() const {
331 DCHECK(CalledOnValidThread());
332 return io_data()->GetAcceptLangs();
333
334=== modified file 'shared/browser/oxide_browser_context.h'
335--- shared/browser/oxide_browser_context.h 2014-12-03 07:22:41 +0000
336+++ shared/browser/oxide_browser_context.h 2015-03-11 15:05:58 +0000
337@@ -1,5 +1,5 @@
338 // vim:expandtab:shiftwidth=2:tabstop=2:
339-// Copyright (C) 2013 Canonical Ltd.
340+// Copyright (C) 2013-2015 Canonical Ltd.
341
342 // This library is free software; you can redistribute it and/or
343 // modify it under the terms of the GNU Lesser General Public
344@@ -80,6 +80,7 @@
345
346 base::FilePath GetPath() const;
347 base::FilePath GetCachePath() const;
348+ int GetMaxCacheSizeHint() const;
349
350 std::string GetAcceptLangs() const;
351 std::string GetUserAgent() const;
352@@ -132,12 +133,14 @@
353 struct Params {
354 Params(const base::FilePath& path,
355 const base::FilePath& cache_path,
356+ int max_cache_size_hint,
357 content::CookieStoreConfig::SessionCookieMode session_cookie_mode,
358 bool devtools_enabled,
359 int devtools_port,
360 const std::string& devtools_ip)
361 : path(path),
362 cache_path(cache_path),
363+ max_cache_size_hint(max_cache_size_hint),
364 session_cookie_mode(session_cookie_mode),
365 devtools_enabled(devtools_enabled),
366 devtools_port(devtools_port),
367@@ -145,6 +148,7 @@
368
369 base::FilePath path;
370 base::FilePath cache_path;
371+ int max_cache_size_hint;
372 content::CookieStoreConfig::SessionCookieMode session_cookie_mode;
373 bool devtools_enabled;
374 int devtools_port;
375@@ -183,6 +187,7 @@
376
377 base::FilePath GetPath() const final;
378 base::FilePath GetCachePath() const;
379+ int GetMaxCacheSizeHint() const;
380
381 std::string GetAcceptLangs() const;
382 void SetAcceptLangs(const std::string& langs);

Subscribers

People subscribed via source and target branches