Bug 38401 – Make repaint throttling parameters configurable runtime.

• Status: CLOSED
• Resolution: FIXED
• Component: Layout and Rendering

[Viatcheslav Ostapenko]

Different applications on mobile platforms require different amount of repaint throttling in webkit. Now it is compile time flag.

[Viatcheslav Ostapenko]

Created attachment 54823 [details]
Make repaint throttling parameters configurable run time.

[Laszlo Gombos]

Comment on attachment 54823 [details]
Make repaint throttling parameters configurable run time.

> +void FrameView::setRepaintThrottlingParams(
> +           double deferredRepaintDelayP,
> +           double initialDeferredRepaintDelayDuringLoadingP,
> +           double maxDeferredRepaintDelayDuringLoadingP,
> +           double deferredRepaintDelayIncrementDuringLoadingP

Maybe you should spell out "Param" or "Parameter" instead of just "P".

> Index: WebKit/qt/Api/qwebsettings.cpp
> ===================================================================
> --- WebKit/qt/Api/qwebsettings.cpp	(revision 58319)
> +++ WebKit/qt/Api/qwebsettings.cpp	(working copy)
> @@ -28,6 +28,7 @@
>  #include "CrossOriginPreflightResultCache.h"
>  #include "Database.h"
>  #include "FontCache.h"
> +#include "FrameView.h"
>  #include "Page.h"
>  #include "PageCache.h"
>  #include "Settings.h"
> @@ -59,6 +60,17 @@ void QWEBKIT_EXPORT qt_networkAccessAllo
>  #endif
>  }
>  
> +void  qt_setRepaintThrottlingParams(
> +            qreal deferredRepaintDelayP,                        // Normal delay
> +            qreal initialDeferredRepaintDelayDuringLoadingP,    // Negative value would mean that first few
> +            qreal maxDeferredRepaintDelayDuringLoadingP,        // The delay grows on each repaint to this
> +            qreal deferredRepaintDelayIncrementDuringLoadingP   // On each repaint the delay increses by th
> +        )
> +{
> +    WebCore::FrameView::setRepaintThrottlingParams(deferredRepaintDelayP, initialDeferredRepaintDelayDuringLoadingP,
> +                            maxDeferredRepaintDelayDuringLoadingP, deferredRepaintDelayIncrementDuringLoadingP);
> +}
> +

We could should use dynamic properties to set these paramaters, without introducing a new symbol. See bug 37023 for an example on how to do that.

[Kenneth Rohde Christiansen]

We might need to consider this as a candidate for a QWebSetting in QWK2.1, but for now I agree with Laszlo, so r-

[Simon Hausmann]

Any progress on this one? It's blocking the release *nudge* :-)

[Viatcheslav Ostapenko]

Created attachment 56510 [details]
Updated patch: Make repaint throttling parameters configurable run time.

[Kenneth Rohde Christiansen]

Maybe it makes sense to split up the patch into two. One Qt specific and one for the WebCore change.

Darin, are you OK with the WebCore part?

[Kenneth Rohde Christiansen]

Simon, as this is platform dependent, it might make sense to put this in the platform plugin, what do you think?

[Darin Adler]

My concern is performance. The repaint throttling code is in super-hot parsing code, and changing this from a compile time constant to global variables could have an effect on performance.

Hyatt, what do you think about this change?

[Antti Koivisto]

(In reply to comment #8)
> My concern is performance. The repaint throttling code is in super-hot parsing code, and changing this from a compile time constant to global variables could have an effect on performance.
> 
> Hyatt, what do you think about this change?

Repaint throttling is not in parsing code. I don't think there are major performance concerns here.

[Antti Koivisto]

Looks basically good to me. It would be somewhat better to make the static variables that are no longer constants class members (with s_ prefix). It would be good to add a comment that REPAINT_THROTTLING define now simply controls the default values and should eventually be removed.

Someone else has to comment on Qt API.

[Antti Koivisto]

I think having both individual settings and _q_RepaintThrottlingPresets are unnecessary duplication in the API. Could you leave just one of these mechanisms and remove the other?

[Viatcheslav Ostapenko]

(In reply to comment #11)
> I think having both individual settings and _q_RepaintThrottlingPresets are unnecessary duplication in the API. Could you leave just one of these mechanisms and remove the other?

I know that it is duplicate API, but most of people wouldn't know, what set of values would work. And in most cases presets would be fine, but some people would like to have their own sets of parameters. Presets are just for convenience.

[Antti Koivisto]

Comment on attachment 56510 [details]
Updated patch: Make repaint throttling parameters configurable run time.

Seems generally fine so r=me. Please consider the comments.

[Viatcheslav Ostapenko]

(In reply to comment #13)
> (From update of attachment 56510 [details])
> Seems generally fine so r=me. Please consider the comments.

I'm making changes right now. Was busy with s60 issues.

[Viatcheslav Ostapenko]

Created attachment 57344 [details]
Updated patch by anttik comments: Make repaint throttling parameters configurable run time.

[Kenneth Rohde Christiansen]

Comment on attachment 57344 [details]
Updated patch by anttik comments: Make repaint throttling parameters configurable run time.


> +typedef struct {
> +    const char* name;
> +    double deferredRepaintDelay;
> +    double initialDeferredRepaintDelayDuringLoading;
> +    double maxDeferredRepaintDelayDuringLoading;
> +    double deferredRepaintDelayIncrementDuringLoading;
> +} QRepaintThrottlingPreset;

Thsi doesn't really seem that private, plus the name is not very Qt'ish.

Simon, should this be part of the platform plugin instead? That way each platform can handle this one place and handle it just like they want.

[Simon Hausmann]

(In reply to comment #16)
> (From update of attachment 57344 [details])
> 
> > +typedef struct {
> > +    const char* name;
> > +    double deferredRepaintDelay;
> > +    double initialDeferredRepaintDelayDuringLoading;
> > +    double maxDeferredRepaintDelayDuringLoading;
> > +    double deferredRepaintDelayIncrementDuringLoading;
> > +} QRepaintThrottlingPreset;
> 
> Thsi doesn't really seem that private, plus the name is not very Qt'ish.
> 
> Simon, should this be part of the platform plugin instead? That way each platform can handle this one place and handle it just like they want.

The idea of platform plugins is to reduce link-time dependencies to libraries that conceptually define the operating platform from a UI perspective but are built after QtWebKit in the build systems.

I don't think browser specific tuned values belong into the platform plugin.

An alternate idea for making these values configurable is to read them using QSettings.

What are the correct values for the platforms and why can't we define them inside of WebCore? It seems to me that the current defaults are perhaps tuned for desktop setups. If we want mobile - or even maemo or symbian specific - values, then why can't we simply put them there, next to the other defaults?

From the little I understand about this, I would think that there could be mobile specific values. I cannot think of why the value would be different between say symbian and maemo.

What do other ports like Android uses for these values?

[Kenneth Rohde Christiansen]

> The idea of platform plugins is to reduce link-time dependencies to libraries that conceptually define the operating platform from a UI perspective but are built after QtWebKit in the build systems.
> 
> I don't think browser specific tuned values belong into the platform plugin.
> 
> An alternate idea for making these values configurable is to read them using QSettings.
> 
> What are the correct values for the platforms and why can't we define them inside of WebCore? It seems to me that the current defaults are perhaps tuned for desktop setups. If we want mobile - or even maemo or symbian specific - values, then why can't we simply put them there, next to the other defaults?
> 
> From the little I understand about this, I would think that there could be mobile specific values. I cannot think of why the value would be different between say symbian and maemo.
> 
> What do other ports like Android uses for these values?

Well, if this is something that just differs from mobile, desktop etc, I agree with you. But on the other hand if this is really something that can be tuned per platform/device, then I do not want to do so per app using WebKit, thus my suggestion on adding that to the platform/device plugin :-)

[Viatcheslav Ostapenko]

(In reply to comment #18)
> I do not want to do so per app using WebKit, 
> thus my suggestion on adding that to the platform/device plugin :-)

Why application shouldn't be able to tune those parameters? I think, that browser and wrt could have different requirements for repaint latency.
We discussed even per view tuning (chrome and webview), but decided, that difference will be really minimal.
Per device tuning is quite important. For example, N97 and N8 are very different.

[Kenneth Rohde Christiansen]

(In reply to comment #19)
> (In reply to comment #18)
> > I do not want to do so per app using WebKit, 
> > thus my suggestion on adding that to the platform/device plugin :-)
> 
> Why application shouldn't be able to tune those parameters? I think, that browser and wrt could have different requirements for repaint latency.
> We discussed even per view tuning (chrome and webview), but decided, that difference will be really minimal.
> Per device tuning is quite important. For example, N97 and N8 are very different.

Then you will easily end up in the situation that you want different tuning variables per web runtime widget, (per device!) and I would really like avoiding us getting there. Per device seems very fine with me.

Anyway, I just want good - out of the box - values.

[Eric Seidel (no email)]

Comment on attachment 56510 [details]
Updated patch: Make repaint throttling parameters configurable run time.

Cleared Antti Koivisto's review+ from obsolete attachment 56510 [details] so that this bug does not appear in http://webkit.org/pending-commit.

[Simon Hausmann]

Comment on attachment 57344 [details]
Updated patch by anttik comments: Make repaint throttling parameters configurable run time.

r=me

The loop is missing a break, so that we don't continue looking for more presets, but otherwise this looks okay. I'll fix it when landing.

[Simon Hausmann]

Committed r61312: <http://trac.webkit.org/changeset/61312>

[Simon Hausmann]

Revision r61312 cherry-picked into qtwebkit-2.0 with commit a469b6af4f4b82a137afc149dcc1257fbe6f96a3