Bug 38401 - Make repaint throttling parameters configurable runtime.
Summary: Make repaint throttling parameters configurable runtime.
Status: CLOSED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Enhancement
Assignee: Viatcheslav Ostapenko
URL:
Keywords: Qt, QtTriaged
Depends on:
Blocks: 35784
  Show dependency treegraph
 
Reported: 2010-04-30 13:26 PDT by Viatcheslav Ostapenko
Modified: 2010-06-17 01:28 PDT (History)
7 users (show)

See Also:


Attachments
Make repaint throttling parameters configurable run time. (5.82 KB, patch)
2010-04-30 13:32 PDT, Viatcheslav Ostapenko
kenneth: review-
Details | Formatted Diff | Diff
Updated patch: Make repaint throttling parameters configurable run time. (8.44 KB, patch)
2010-05-19 12:12 PDT, Viatcheslav Ostapenko
no flags Details | Formatted Diff | Diff
Updated patch by anttik comments: Make repaint throttling parameters configurable run time. (10.65 KB, patch)
2010-05-28 10:47 PDT, Viatcheslav Ostapenko
hausmann: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Viatcheslav Ostapenko 2010-04-30 13:26:16 PDT
Different applications on mobile platforms require different amount of repaint throttling in webkit. Now it is compile time flag.
Comment 1 Viatcheslav Ostapenko 2010-04-30 13:32:08 PDT
Created attachment 54823 [details]
Make repaint throttling parameters configurable run time.
Comment 2 Laszlo Gombos 2010-05-03 21:02:23 PDT
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.
Comment 3 Kenneth Rohde Christiansen 2010-05-07 07:41:53 PDT
We might need to consider this as a candidate for a QWebSetting in QWK2.1, but for now I agree with Laszlo, so r-
Comment 4 Simon Hausmann 2010-05-18 14:10:42 PDT
Any progress on this one? It's blocking the release *nudge* :-)
Comment 5 Viatcheslav Ostapenko 2010-05-19 12:12:31 PDT
Created attachment 56510 [details]
Updated patch: Make repaint throttling parameters configurable run time.
Comment 6 Kenneth Rohde Christiansen 2010-05-24 08:15:56 PDT
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?
Comment 7 Kenneth Rohde Christiansen 2010-05-24 08:17:44 PDT
Simon, as this is platform dependent, it might make sense to put this in the platform plugin, what do you think?
Comment 8 Darin Adler 2010-05-25 09:49:29 PDT
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?
Comment 9 Antti Koivisto 2010-05-27 05:39:58 PDT
(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.
Comment 10 Antti Koivisto 2010-05-27 06:13:20 PDT
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.
Comment 11 Antti Koivisto 2010-05-28 07:12:46 PDT
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?
Comment 12 Viatcheslav Ostapenko 2010-05-28 07:19:41 PDT
(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.
Comment 13 Antti Koivisto 2010-05-28 07:39:06 PDT
Comment on attachment 56510 [details]
Updated patch: Make repaint throttling parameters configurable run time.

Seems generally fine so r=me. Please consider the comments.
Comment 14 Viatcheslav Ostapenko 2010-05-28 07:41:35 PDT
(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.
Comment 15 Viatcheslav Ostapenko 2010-05-28 10:47:18 PDT
Created attachment 57344 [details]
Updated patch by anttik comments: Make repaint throttling parameters configurable run time.
Comment 16 Kenneth Rohde Christiansen 2010-05-28 14:24:39 PDT
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.
Comment 17 Simon Hausmann 2010-05-29 01:12:53 PDT
(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?
Comment 18 Kenneth Rohde Christiansen 2010-05-29 06:47:13 PDT
> 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 :-)
Comment 19 Viatcheslav Ostapenko 2010-05-31 18:24:20 PDT
(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.
Comment 20 Kenneth Rohde Christiansen 2010-05-31 18:37:26 PDT
(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.
Comment 21 Eric Seidel (no email) 2010-06-02 02:27:24 PDT
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.
Comment 22 Simon Hausmann 2010-06-16 23:29:38 PDT
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.
Comment 23 Simon Hausmann 2010-06-17 00:53:20 PDT
Committed r61312: <http://trac.webkit.org/changeset/61312>
Comment 24 Simon Hausmann 2010-06-17 01:28:14 PDT
Revision r61312 cherry-picked into qtwebkit-2.0 with commit a469b6af4f4b82a137afc149dcc1257fbe6f96a3