[Qt] QWebSettings::setMaximumPagesInCache(int pages) does not enable the Page Cache

• Component: WebKit Qt
• Status: RESOLVED FIXED

[Andras Becsi]

Created attachment 42772 [details]
proposed patch for further discussion

Calling setMaximumPagesInCache(somePositiveInt) does not enable the Page Cache (http://webkit.org/blog/427/webkit-page-cache-i-the-basics/) which is disabled by default in QtWebKit.
Further more QWebSettings::setMaximumPagesInCache(int pages) is static, so calling d->settings-setUsesPageCache(true) if pages>0 is not an option.
To support page caching in a comparative way and quality as the other ports do, we would need a nice cross platform way to measure the available system memory and calculate the page cache capacity in a similar way as the Mac and Win ports do:

Eg.: WebKit/win/WebView.cpp:502:
        // Page cache capacity (in pages)
        // (Research indicates that value / page drops substantially after 3 pages.)
        if (memSize >= 2048)
            pageCacheCapacity = 5;
        else if (memSize >= 1024)
            pageCacheCapacity = 4;
        else if (memSize >= 512)
            pageCacheCapacity = 3;
        else if (memSize >= 256)
            pageCacheCapacity = 2;
        else
            pageCacheCapacity = 1;

But in embedded devices memory is a precious resource, so there may be situations where caching has to be disabled completely. 
So we would have to guess somehow which setting would be best, or leave the whole decision to the programmer? 

The attached patch tries the second approach and adds methods to qwebsetting and documents the usage and co-relation to setMaximumPagesInCache().
 
It would be nice to get some input on that issue.

[Kenneth Rohde Christiansen]

Well, the heuristics could be implemented by the browser or app using WebKit.
Remember QtWebKit is not just for creating browsers :-), so I think the current
API is OK.

setMaximumPagesInCache was introduced before 4.6 right?

[Andras Becsi]

(In reply to comment #1)
> Well, the heuristics could be implemented by the browser or app using WebKit.
> Remember QtWebKit is not just for creating browsers :-), so I think the current
> API is OK.
> 
> setMaximumPagesInCache was introduced before 4.6 right?
Yes I think so.

[Andras Becsi]

(In reply to comment #2)
> (In reply to comment #1)
> > Well, the heuristics could be implemented by the browser or app using WebKit.
> > Remember QtWebKit is not just for creating browsers :-), so I think the current
> > API is OK.

Further investigation showed that in qwebsettings.cpp:91:QWebSettingsPrivate::apply()) the attributes QHash (which is a member of QWebSettingsPrivate) is completely empty in approx. 5900 layout tests (from 6100 running), and has maximally 1 element in the other ~200, so it seems that updating the whole QWebSettings enum every time apply() is called might be a performance regression.

> > 
> > setMaximumPagesInCache was introduced before 4.6 right?
> Yes I think so.

[Antonio Gomes]

cool.

i'd block qt 4.6 on it, since it make an important feature (pageCache) to actually work.

[Simon Hausmann]

We cannot add new functions to Qt 4.6 anymore.

[Simon Hausmann]

Oops, the last comment was incomplete. But as it said, we cannot add new functions to Qt 4.6 anymore :(

You are right that it seems that we are currently not enabling the page cache in WebCore::Settings. I'm not sure if that was introduced afterwards or if it's been missing since then.

My guts feeling is that it's probably best for now to keep it a global setting, i.e. to enable the page cache in all settings objects if there is a maximum set for pages in the page cache. Similarly we should disable it if the maximum is 0.

That can be done without adding new API and I think it gives us a sensible default. The only thing we're missing is the ability to turn off the page cache _per QWebPage_, but it is debatable whether that makes sense in the first place.

It is however a feature we can also introduce later on.

[Andras Becsi]

(In reply to comment #6)
> Oops, the last comment was incomplete. But as it said, we cannot add new
> functions to Qt 4.6 anymore :(
> 
> You are right that it seems that we are currently not enabling the page cache
> in WebCore::Settings. I'm not sure if that was introduced afterwards or if it's
> been missing since then.
> 
> My guts feeling is that it's probably best for now to keep it a global setting,
> i.e. to enable the page cache in all settings objects if there is a maximum set
> for pages in the page cache. Similarly we should disable it if the maximum is
> 0.
> 
> That can be done without adding new API and I think it gives us a sensible
> default. The only thing we're missing is the ability to turn off the page cache
> _per QWebPage_, but it is debatable whether that makes sense in the first
> place.
> 
> It is however a feature we can also introduce later on.

I understand and that's why I am experimenting with the globalSettings singleton now which can be reached to enable and disable the page cache in setMaximumPagesInCache as needed. This could work out. Shortly I will add a new attachment.

[Andras Becsi]

Created attachment 42961 [details]
proposed patch

Without adding a new QWebSetting enum value it did not enable the global cache, so I needed to add one, but there are no new functions so no API change.
There are two known tests which are skipped because of Page Cache. As implemented they do not time-out and crash any more, but revealed another bug which causes a JS console message:

"CONSOLE MESSAGE: ReferenceError: Trying to access object from destroyed plug-in."

which is a different issue, and tonikitoo will hopefully comfirm that my patch is actually making Page Cache to work.

[Simon Hausmann]

(In reply to comment #8)
> Created an attachment (id=42961) [details]
> proposed patch
> 
> Without adding a new QWebSetting enum value it did not enable the global cache,
> so I needed to add one, but there are no new functions so no API change.

Unfortunately adding a new enum is an API change (although admitadely not on a binary level).

Can't we simply enable the cache if there is a maximum set and disable it if the maximum is zero?

[Antonio Gomes]

> but revealed another bug which causes a JS console message:
> 
> "CONSOLE MESSAGE: ReferenceError: Trying to access object from destroyed
> plug-in."
> 
> which is a different issue, and tonikitoo will hopefully comfirm that my patch
> is actually making Page Cache to work.

I can confirm it made pageCache to work. In a layout test I am doing I was getting "onLoad" called for back/forward navigations. W/ the patch in I am getting it called once and then just pageShow/Hide.

About the CONSOLE MESSAGE, I am able to reproduce it too even w/ JIT off.

[Andras Becsi]

(In reply to comment #9)
> (In reply to comment #8)
> > Created an attachment (id=42961) [details] [details]
> > proposed patch
> > 
> > Without adding a new QWebSetting enum value it did not enable the global cache,
> > so I needed to add one, but there are no new functions so no API change.
> 
> Unfortunately adding a new enum is an API change (although admitadely not on a
> binary level).
> 
> Can't we simply enable the cache if there is a maximum set and disable it if
> the maximum is zero?

Well, I experimented with it for two days now and the only way we can access the platform independent global Settings object now is through QWebSettings::globalSettings()->d which is a QWebSettingsPrivate* and has a WebCore::Settings* member named settings. I tried to set the page cache directly with QWebSettings::globalSettings()->d->settings->setUsesPage(qMax(0, pages)) but this caused DRT to crash on tests which want to override the usage of the cache, because setMaximumPagesInCache is static and the settings member of the singleton didn't get initialized. So I suppose settings was not meant to be called directly but indirectly through d->apply(). This probably will need a refactoring soon since it seems to be much of hiding which might cause performance issues. 
I will try to work around this for Qt 4.6, it will probably be ugly but policy is policy.
After release I will try to refactor this apply in a more elegant way.

[Andras Becsi]

(In reply to comment #10)
> > but revealed another bug which causes a JS console message:
> > 
> > "CONSOLE MESSAGE: ReferenceError: Trying to access object from destroyed
> > plug-in."
> > 
> > which is a different issue, and tonikitoo will hopefully comfirm that my patch
> > is actually making Page Cache to work.
> 
> I can confirm it made pageCache to work. In a layout test I am doing I was
> getting "onLoad" called for back/forward navigations. W/ the patch in I am
> getting it called once and then just pageShow/Hide.
> 
> About the CONSOLE MESSAGE, I am able to reproduce it too even w/ JIT off.

Thank you very much for testing.

[Simon Hausmann]

(In reply to comment #11)
> (In reply to comment #9)
> > (In reply to comment #8)
> > > Created an attachment (id=42961) [details] [details] [details]
> > > proposed patch
> > > 
> > > Without adding a new QWebSetting enum value it did not enable the global cache,
> > > so I needed to add one, but there are no new functions so no API change.
> > 
> > Unfortunately adding a new enum is an API change (although admitadely not on a
> > binary level).
> > 
> > Can't we simply enable the cache if there is a maximum set and disable it if
> > the maximum is zero?
> 
> Well, I experimented with it for two days now and the only way we can access
> the platform independent global Settings object now is through
> QWebSettings::globalSettings()->d which is a QWebSettingsPrivate* and has a
> WebCore::Settings* member named settings. I tried to set the page cache
> directly with QWebSettings::globalSettings()->d->settings->setUsesPage(qMax(0,
> pages)) but this caused DRT to crash on tests which want to override the usage
> of the cache, because setMaximumPagesInCache is static and the settings member
> of the singleton didn't get initialized. So I suppose settings was not meant to
> be called directly but indirectly through d->apply(). This probably will need a
> refactoring soon since it seems to be much of hiding which might cause
> performance issues. 
> I will try to work around this for Qt 4.6, it will probably be ugly but policy
> is policy.
> After release I will try to refactor this apply in a more elegant way.

Thanks Andras!

I agree it's ugly. I would suggest in apply() to simply look at WebCore::pageCache()->capacity() to decide whether to enable or disable the page cache in the WebCore::Settings object.

Then you can call apply() from setMaximumPagesInCache.

[Simon Hausmann]

Comment on attachment 42961 [details]
proposed patch

r- due to the earlier comments. For now we should enable/disable the cache only depending on the maximumPagesInCache value, i.e. the cache capacity.

[Andras Becsi]

Created attachment 42998 [details]
simlified patch

Antonio, could you please test this one if it works in your test? It works on the pagehide tests here, if somebody reviews it you can also land it, or I will set the cq+ tomorrow.
Thanks for the hint, Simon. I had a similar concept but this is simpler and not-so-ugly as I thougth :)

[Antonio Gomes]

(In reply to comment #15)
> Created an attachment (id=42998) [details]
> simlified patch
> 
> Antonio, could you please test this one if it works in your test? 

thx, bbandix. It works for me too.

[Andras Becsi]

(In reply to comment #16)
> (In reply to comment #15)
> > Created an attachment (id=42998) [details] [details]
> > simlified patch
> > 
> > Antonio, could you please test this one if it works in your test? 
> 
> thx, bbandix. It works for me too.

Thanks for testing.

[Simon Hausmann]

Comment on attachment 42998 [details]
simlified patch

The patch looks good to me, except for this piece:

> +    settings()->setMaximumPagesInCache(0); // reset to default
>  }
[...]  
> +    else if (name == "WebKitUsesPageCachePreferenceKey")
> +        settings->setMaximumPagesInCache(value.toInt());
>  }

setMaximumPagesInCache is a class method of QWebSettings, so the code should read

QWebSettings::setMaximumPagesInCache()

It is currently not possible to specify this per settings object after all, only globally.

[Andras Becsi]

Created attachment 43151 [details]
also change DRT to set the cache globally

[WebKit Commit Bot]

Comment on attachment 43151 [details]
also change DRT to set the cache globally

Clearing flags on attachment: 43151

Committed r50938: <http://trac.webkit.org/changeset/50938>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.