Bug 56178 - [EFL] Repaint throttling API for WebKit-EFL
Summary: [EFL] Repaint throttling API for WebKit-EFL
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit EFL (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Linux
: P2 Enhancement
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-03-11 01:24 PST by Grzegorz Czajkowski
Modified: 2011-06-10 02:49 PDT (History)
10 users (show)

See Also:


Attachments
patch (4.44 KB, patch)
2011-03-11 01:31 PST, Grzegorz Czajkowski
no flags Details | Formatted Diff | Diff
updated patch (3.19 KB, patch)
2011-05-11 01:21 PDT, Grzegorz Czajkowski
no flags Details | Formatted Diff | Diff
updated patch (2.82 KB, patch)
2011-06-02 23:51 PDT, Grzegorz Czajkowski
no flags Details | Formatted Diff | Diff
updated patch (3.55 KB, patch)
2011-06-02 23:56 PDT, Grzegorz Czajkowski
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Grzegorz Czajkowski 2011-03-11 01:24:38 PST
The API allows to set the levels for repaint throttling.
It should be ensured displaying a content with many css/gif animations.
Comment 1 Grzegorz Czajkowski 2011-03-11 01:31:37 PST
Created attachment 85446 [details]
patch
Comment 2 Ryuan Choi 2011-03-17 21:25:31 PDT
I know that we need repaint throttling feature, but could you explain why we should use those level like Qt?
Comment 3 Dirk Schulze 2011-04-26 15:59:16 PDT
Comment on attachment 85446 [details]
patch

No comment of the committer. Once you answered the question you can set the review flag again.
Comment 4 Grzegorz Czajkowski 2011-04-27 05:51:35 PDT
(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.
Comment 5 Ryuan Choi 2011-05-09 00:48:31 PDT
(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.
Comment 6 Lucas De Marchi 2011-05-09 05:12:05 PDT
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.
Comment 7 Grzegorz Czajkowski 2011-05-09 05:29:28 PDT
(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.
Comment 8 Grzegorz Czajkowski 2011-05-09 23:51:19 PDT
(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?
Comment 9 Lucas De Marchi 2011-05-10 10:43:44 PDT
> 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?
Comment 10 Rafael Antognolli 2011-05-10 11:34:16 PDT
(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.
Comment 11 Grzegorz Czajkowski 2011-05-11 01:21:58 PDT
Created attachment 93086 [details]
updated patch

API has been changed. 

Lucas / Rafael,
Could you review my patch again, please?
Comment 12 Ryuan Choi 2011-05-30 14:24:00 PDT
It may be enough for us.
But, does we need to provide getter for others?
Comment 13 Leandro Pereira 2011-05-30 14:26:52 PDT
(In reply to comment #12)
> It may be enough for us.
> But, does we need to provide getter for others?

I think so.
Comment 14 Grzegorz Czajkowski 2011-05-30 23:56:19 PDT
(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 :)
Comment 15 Leandro Pereira 2011-05-31 09:43:25 PDT
(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.
Comment 16 Grzegorz Czajkowski 2011-06-01 07:10:38 PDT
(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.
Comment 17 Grzegorz Czajkowski 2011-06-01 23:34:08 PDT
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
Comment 18 Lucas De Marchi 2011-06-02 05:19:06 PDT
(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.
Comment 19 Grzegorz Czajkowski 2011-06-02 23:51:10 PDT
Created attachment 95860 [details]
updated patch

Proposed values for repaint throttling have been added to the doxygen documentation.
Comment 20 Grzegorz Czajkowski 2011-06-02 23:56:25 PDT
Created attachment 95861 [details]
updated patch

Added missing ChangeLog
Comment 21 Leandro Pereira 2011-06-03 10:06:46 PDT
Comment on attachment 95861 [details]
updated patch

Looks OK. Informal r+.
Comment 22 Kenneth Rohde Christiansen 2011-06-10 01:31:56 PDT
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.
Comment 23 WebKit Review Bot 2011-06-10 02:09:56 PDT
Comment on attachment 95861 [details]
updated patch

Clearing flags on attachment: 95861

Committed r88533: <http://trac.webkit.org/changeset/88533>
Comment 24 WebKit Review Bot 2011-06-10 02:10:02 PDT
All reviewed patches have been landed.  Closing bug.
Comment 25 Grzegorz Czajkowski 2011-06-10 02:49:16 PDT
(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 ;)