Bug 56178 – [EFL] Repaint throttling API for WebKit-EFL

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit EFL

[Grzegorz Czajkowski]

The API allows to set the levels for repaint throttling.
It should be ensured displaying a content with many css/gif animations.

[Grzegorz Czajkowski]

Created attachment 85446 [details]
patch

[Ryuan Choi]

I know that we need repaint throttling feature, but could you explain why we should use those level like Qt?

[Dirk Schulze]

Comment on attachment 85446 [details]
patch

No comment of the committer. Once you answered the question you can set the review flag again.

[Grzegorz Czajkowski]

(In reply to comment #2)
> I know that we need repaint throttling feature, but could you explain why we should use those level like Qt?

Yes, these values are taken from WebKit-Qt. The values are well adjusted to slow down page loading and depending on the level they allow to display a content with many css/gif animations.

[Ryuan Choi]

(In reply to comment #4)
> (In reply to comment #2)
> > I know that we need repaint throttling feature, but could you explain why we should use those level like Qt?
> 
> Yes, these values are taken from WebKit-Qt. The values are well adjusted to slow down page loading and depending on the level they allow to display a content with many css/gif animations.

Demarchi,
could you give a comment, if you have time?

IMO, looks fine.

[Lucas De Marchi]

Comment on attachment 85446 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=85446&action=review

Don't you need to ENABLE(REPAINT_THROTTLING)?

> Source/WebKit/efl/ewk/ewk_settings.cpp:67
> +static const struct _repaint_throttling_values {
> +    double deferred_repaint_delay;
> +    double initial_deferred_repaint_delay_during_loading;
> +    double max_deferred_repaint_delay_during_loading;
> +    double deferred_repaint_delay_increment_during_loading;
> +} repaint_throttling_values[] = {
> +            {   0,          0,      0,      0   },
> +            {   0.025,      0,      2.5,    0.5 },
> +            {   0.01,       0,      1,      0.2 },
> +            {   0.025,      1,      5,      0.5 },
> +            {   0.1,        2,      10,      1  }
> +        };
> +

It seems weird for me to expose this API. Wouldn't it be better to let the browser decide which value each field takes? With a quick grep I couldn't find a similar implementation in other ports.

[Grzegorz Czajkowski]

(In reply to comment #6)
> (From update of attachment 85446 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=85446&action=review
> 
> Don't you need to ENABLE(REPAINT_THROTTLING)?

Yes, you are right.

> 
> > Source/WebKit/efl/ewk/ewk_settings.cpp:67
> > +static const struct _repaint_throttling_values {
> > +    double deferred_repaint_delay;
> > +    double initial_deferred_repaint_delay_during_loading;
> > +    double max_deferred_repaint_delay_during_loading;
> > +    double deferred_repaint_delay_increment_during_loading;
> > +} repaint_throttling_values[] = {
> > +            {   0,          0,      0,      0   },
> > +            {   0.025,      0,      2.5,    0.5 },
> > +            {   0.01,       0,      1,      0.2 },
> > +            {   0.025,      1,      5,      0.5 },
> > +            {   0.1,        2,      10,      1  }
> > +        };
> > +
> 
> It seems weird for me to expose this API. Wouldn't it be better to let the browser decide which value each field takes? With a quick grep I couldn't find a similar implementation in other ports.

Methods QWebPagePrivate::dynamicPropertyChangeEvent in http://trac.webkit.org/browser/trunk/Source/WebKit/qt/Api/qwebpage.cpp allows to set repaint throttling based on browser's settings.

[Grzegorz Czajkowski]

(In reply to comment #7)
> (In reply to comment #6)
> > (From update of attachment 85446 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=85446&action=review
> > 
> > Don't you need to ENABLE(REPAINT_THROTTLING)?
> 
> Yes, you are right.
> 
> > 

This macro assigns values for throttling parameters in WebCore only (exactly the legacy level in proposed patch). It doesn't contains any API, variables, methods etc. So in patch for WebKit-EFL we don't need this macro, we are suggesting our levels for repaint throttling.
When the macro is disabled all variables responsible for repaint throttling are initialized on zero. In this case we do not any delay for the repaints, all ones come to update process one by one.

When application has used the values for repaint throttling WebCore accumulates the repaints in vector. On timer the repaints from vector are put to the Efl's queue. The timer calling is calculates on throttling parameters.

Delaying of repaints may help to display some contents on mobile devices.

> 
> > Source/WebKit/efl/ewk/ewk_settings.cpp:67
> > +static const struct _repaint_throttling_values {
> > +    double deferred_repaint_delay;
> > +    double initial_deferred_repaint_delay_during_loading;
> > +    double max_deferred_repaint_delay_during_loading;
> > +    double deferred_repaint_delay_increment_during_loading;
> > +} repaint_throttling_values[] = {
> > +            {   0,          0,      0,      0   },
> > +            {   0.025,      0,      2.5,    0.5 },
> > +            {   0.01,       0,      1,      0.2 },
> > +            {   0.025,      1,      5,      0.5 },
> > +            {   0.1,        2,      10,      1  }
> > +        };
> > +
> 
> It seems weird for me to expose this API. Wouldn't it be better to let the browser decide which value each field takes? With a quick grep I couldn't find a similar implementation in other ports.

I don't know whether browser should assign the values. Of course we can expose API like:
void ewk_setting_defereed_repaint_delay_set(doube )
void ewk_setting_initial_deferred_repaint_delay_during_loading_set(doube )
void ewk_setting_max_deferred_repaint_delay_during_loading_set(doube )
void ewk_setting_deferred_repaint_delay_increment_during_loading_set(doube )

I think it's a good idea to have some dedicated levels for repaints throttling. Please decide which option you prefer. What about to have both ones?

[Lucas De Marchi]

> I don't know whether browser should assign the values. Of course we can expose API like:
> void ewk_setting_defereed_repaint_delay_set(doube )
> void ewk_setting_initial_deferred_repaint_delay_during_loading_set(doube )
> void ewk_setting_max_deferred_repaint_delay_during_loading_set(doube )
> void ewk_setting_deferred_repaint_delay_increment_during_loading_set(doube )

I thought 1 function as below:

ewk_setting_repaint_throttling_set(double, double, double, double)

Leandro / Antognolli, what do you think?

[Rafael Antognolli]

(In reply to comment #9)
> > I don't know whether browser should assign the values. Of course we can expose API like:
> > void ewk_setting_defereed_repaint_delay_set(doube )
> > void ewk_setting_initial_deferred_repaint_delay_during_loading_set(doube )
> > void ewk_setting_max_deferred_repaint_delay_during_loading_set(doube )
> > void ewk_setting_deferred_repaint_delay_increment_during_loading_set(doube )
> 
> I thought 1 function as below:
> 
> ewk_setting_repaint_throttling_set(double, double, double, double)
> 
> Leandro / Antognolli, what do you think?

I agree with Lucas, this simpler API would be better.

If you have some good values for these variables, you could set them as default values, and have this API to allow the browser to change them if necessary.

[Grzegorz Czajkowski]

Created attachment 93086 [details]
updated patch

API has been changed. 

Lucas / Rafael,
Could you review my patch again, please?

[Ryuan Choi]

It may be enough for us.
But, does we need to provide getter for others?

[Leandro Pereira]

(In reply to comment #12)
> It may be enough for us.
> But, does we need to provide getter for others?

I think so.

[Grzegorz Czajkowski]

(In reply to comment #13)
> (In reply to comment #12)
> > It may be enough for us.
> > But, does we need to provide getter for others?
> 
> I think so.

WebCore doesn't provide any public getter methods for this feature. Do you think it's worth to keep all saved values in ewk_setting.cpp?
The previous patch was better for this because we have to remember only level :)

[Leandro Pereira]

(In reply to comment #14)
> (In reply to comment #13)
> > (In reply to comment #12)
> > > It may be enough for us.
> > > But, does we need to provide getter for others?
> > 
> > I think so.
> 
> WebCore doesn't provide any public getter methods for this feature. Do you 
> think it's worth to keep all saved values in ewk_setting.cpp?
>

After thinking for a while, I'm fine without getters for these. You'll probably have these constants on a table on the browser side anyway.

[Grzegorz Czajkowski]

(In reply to comment #15)
> (In reply to comment #14)
> > (In reply to comment #13)
> > > (In reply to comment #12)
> > > > It may be enough for us.
> > > > But, does we need to provide getter for others?
> > > 
> > > I think so.
> > 
> > WebCore doesn't provide any public getter methods for this feature. Do you 
> > think it's worth to keep all saved values in ewk_setting.cpp?
> >
> 
> After thinking for a while, I'm fine without getters for these. You'll probably have these constants on a table on the browser side anyway.

Yes, I also think that getters method for these aren't need.

[Grzegorz Czajkowski]

What do you think to put some recommended values for repaint throttling in doxygen doc? For example,

0,      0,    0,      0    - default values, these do not delay any repaints
0.025,  0,    2.5,    0.5  - recommended values for dynamic content
0.01,   0,    1,      0.2  - minimal level
0.025,  1,    5,      0.5  - medium level
0.1,    2,    10,      1   - heavy level

[Lucas De Marchi]

(In reply to comment #17)
> What do you think to put some recommended values for repaint throttling in doxygen doc? For example,
> 
> 0,      0,    0,      0    - default values, these do not delay any repaints
> 0.025,  0,    2.5,    0.5  - recommended values for dynamic content
> 0.01,   0,    1,      0.2  - minimal level
> 0.025,  1,    5,      0.5  - medium level
> 0.1,    2,    10,      1   - heavy level

Fair enough.

[Grzegorz Czajkowski]

Created attachment 95860 [details]
updated patch

Proposed values for repaint throttling have been added to the doxygen documentation.

[Grzegorz Czajkowski]

Created attachment 95861 [details]
updated patch

Added missing ChangeLog

[Leandro Pereira]

Comment on attachment 95861 [details]
updated patch

Looks OK. Informal r+.

[Kenneth Rohde Christiansen]

Comment on attachment 95861 [details]
updated patch

You might want to check if this actually makes a difference when using tiling with webkit2.

Where did you get those values? Are they invented ? :-) 

API is fine, just making these observations.

[WebKit Review Bot]

Comment on attachment 95861 [details]
updated patch

Clearing flags on attachment: 95861

Committed r88533: <http://trac.webkit.org/changeset/88533>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Grzegorz Czajkowski]

(In reply to comment #22)
> (From update of attachment 95861 [details]) 
> Where did you get those values? Are they invented ? :-) 
> 

They were taken from Qt-port ;)