Oxide

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

  1. maxCacheSize
  2. Merge into oxide.trunk
Proposed by Olivier Tilloy
Status: Merged
Merged at revision: 1011
Proposed branch: lp://staging/~osomon/oxide/maxCacheSize
Merge into: lp://staging/~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://staging/~osomon/oxide/maxCacheSize
Reviewer Review Type Date Requested Status
Chris Coulson Approve
Review via email: mp+252113@code.staging.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.
Revision history for this message
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://staging/~osomon/oxide/maxCacheSize updated
1003. By Olivier Tilloy

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

1004. By Olivier Tilloy

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

1005. By Olivier Tilloy

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

Revision history for this message
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).

Revision history for this message
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://staging/~osomon/oxide/maxCacheSize updated
1006. By Olivier Tilloy

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

Enforce an upper limit to avoid integer overflow.

Revision history for this message
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?

Revision history for this message
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://staging/~osomon/oxide/maxCacheSize updated
1008. By Olivier Tilloy

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.

Revision history for this message
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
The diff is not available at this time. You can reload the page or download it.

Subscribers

People subscribed via source and target branches