Bug 25934 – [Qt] Add a setting attribute to disable/enable WebCore memory cache.

• Component: WebKit Qt
• Status: RESOLVED WONTFIX

[Yongjun Zhang]

QWebSettings::setObjectCacheCapability(0,0,0) disables memory cache, which stops purging live objects too.  The proper cache behavior should be purging live objects even when cache capacity is set to 0.

[Yongjun Zhang]

Created attachment 30544 [details]
add AlwaysEnableObjectCache setting attribute, to avoid changing the behavior of setObjectCacheCapacities.

[Holger Freyther]

Comment on attachment 30544 [details]
add AlwaysEnableObjectCache setting attribute, to avoid changing the behavior of setObjectCacheCapacities.

r=- because of two/three things:
  1.) Missing documentation. You will need to document the enum
  2.) Look at the name of the enum. OfflineStorageDatabaseEnabled (Enabled at the end) and compare to this attribute. It should be consistent but I suck with names :)
  3.) WebCore::cache() is a singleton, you make it look like something you can set locally (per QWebPage...) but it is not. I think this should be modeled with a static function..


> @@ -568,6 +573,7 @@ int QWebSettings::maximumPagesInCache()
>  void QWebSettings::setObjectCacheCapacities(int cacheMinDeadCapacity, int cacheMaxDead, int totalCapacity)
>  {
>      bool disableCache = cacheMinDeadCapacity == 0 && cacheMaxDead == 0 && totalCapacity == 0;
> +
>      WebCore::cache()->setDisabled(disableCache);
>  
>      WebCore::cache()->setCapacities(qMax(0, cacheMinDeadCapacity),

bogus whitespace.

[Yongjun Zhang]

Created attachment 30673 [details]
change the naming, adding documentations as per zecke's comments.

Thanks for review.  The reason I chose attribute rather than a dedicated enable/disable API is to avoid API changes to QWebSettings.

[Kenneth Rohde Christiansen]

Why the Always in ObjectCacheAlwaysEnabled? That name is pretty confusing, at least according to your documentation:

+    \value ObjectCacheALwaysEnabled Specifies whether the global object cache is
+        enabed or not.

Also, should be "specifies" and "enabled"

Probably we should even explain what the object cache is for normal people?

[Holger Freyther]

(In reply to comment #3)
> Created an attachment (id=30673) [review]
> change the naming, adding documentations as per zecke's comments.
> 
> Thanks for review.  The reason I chose attribute rather than a dedicated
> enable/disable API is to avoid API changes to QWebSettings.

Okay. Let me repeat. WebCore::cache() is a singleton... it is a "global" settings, by hiding it as a value inside the QWebSettings instance you create the impression you can have this setting per QWebPage... but you can't. This is why it screams for a static method... You have basicly two options:
   a) Make it static
   b) Propose a change to make WebCore::cache() operate per Page, or create a WebCore::Settings and make it use.


Do you see the point with making it static? Otherwise I can show you an example where this side effect will produce something one won't like...

[Holger Freyther]

Comment on attachment 30673 [details]
change the naming, adding documentations as per zecke's comments.

As of the previous comment. The side-effect of the WebCore::cache()->setDisabled call is unexpected from an API point of view. The last call to ::apply will determine this...

[Jesus Sanchez-Palencia]

Is this still valid? Long time since the last udpate here...

[Antonio Gomes]

First, page title is ambiguous: when reading it and had an impression it was about page cache (aka bf-cache). After reading the bug comments and patches, I realized that it is about the caching of "dead objects including stylesheets and scripts".

Second, according to the QWebSettings documentation at [1], "Calling setObjectCacheCapacities(0, 0, 0) will disable the cache. Calling it with one non-zero enables it again."

So I think the proposed attribute addition is rather unneeded once it proposed exactly a way to achieve the behavior quoted above through an new Enum.

+        value = attributes.value(QWebSettings::ObjectCacheAlwaysEnabled,
+                                      global->attributes.value(QWebSettings::ObjectCacheAlwaysEnabled));
+        WebCore::cache()->setDisabled(!value);

It is WONTFIX to me.

[Ariya Hidayat]

> It is WONTFIX to me.

I agree with Antonio's analysis.

Closing this bug.