Bug 25934 - [Qt] Add a setting attribute to disable/enable WebCore memory cache.
Summary: [Qt] Add a setting attribute to disable/enable WebCore memory cache.
Status: RESOLVED WONTFIX
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Qt (show other bugs)
Version: 528+ (Nightly build)
Hardware: All Linux
: P2 Enhancement
Assignee: QtWebKit Unassigned
URL:
Keywords: Qt
Depends on:
Blocks:
 
Reported: 2009-05-21 08:14 PDT by Yongjun Zhang
Modified: 2010-08-22 02:19 PDT (History)
6 users (show)

See Also:


Attachments
add AlwaysEnableObjectCache setting attribute, to avoid changing the behavior of setObjectCacheCapacities. (3.02 KB, patch)
2009-05-21 08:23 PDT, Yongjun Zhang
zecke: review-
Details | Formatted Diff | Diff
change the naming, adding documentations as per zecke's comments. (3.31 KB, patch)
2009-05-26 11:08 PDT, Yongjun Zhang
zecke: review-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Yongjun Zhang 2009-05-21 08:14:00 PDT
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.
Comment 1 Yongjun Zhang 2009-05-21 08:23:17 PDT
Created attachment 30544 [details]
add AlwaysEnableObjectCache setting attribute, to avoid changing the behavior of setObjectCacheCapacities.
Comment 2 Holger Freyther 2009-05-22 21:36:42 PDT
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.
Comment 3 Yongjun Zhang 2009-05-26 11:08:00 PDT
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.
Comment 4 Kenneth Rohde Christiansen 2009-06-04 05:50:54 PDT
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?
Comment 5 Holger Freyther 2009-06-07 06:05:26 PDT
(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...

Comment 6 Holger Freyther 2009-06-07 06:13:52 PDT
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...
Comment 7 Jesus Sanchez-Palencia 2010-05-13 14:16:03 PDT
Is this still valid? Long time since the last udpate here...
Comment 8 Antonio Gomes 2010-05-14 06:31:02 PDT
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.
Comment 9 Ariya Hidayat 2010-08-22 02:19:37 PDT
> It is WONTFIX to me.

I agree with Antonio's analysis.

Closing this bug.