Summary: | [EFL] Repaint throttling API for WebKit-EFL | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Grzegorz Czajkowski <g.czajkowski> | ||||||||||
Component: | WebKit EFL | Assignee: | Nobody <webkit-unassigned> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Enhancement | CC: | antognolli+webkit, gyuyoung.kim, joone.hur, kenneth, leandro, l.slachciak, lucas.de.marchi, ryuan.choi, tonikitoo, webkit.review.bot | ||||||||||
Priority: | P2 | ||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||
Hardware: | PC | ||||||||||||
OS: | Linux | ||||||||||||
Attachments: |
|
Description
Grzegorz Czajkowski
2011-03-11 01:24:38 PST
Created attachment 85446 [details]
patch
I know that we need repaint throttling feature, but could you explain why we should use those level like Qt? Comment on attachment 85446 [details]
patch
No comment of the committer. Once you answered the question you can set the review flag again.
(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. (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 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. (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. (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? > 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?
(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. Created attachment 93086 [details]
updated patch
API has been changed.
Lucas / Rafael,
Could you review my patch again, please?
It may be enough for us. But, does we need to provide getter for others? (In reply to comment #12) > It may be enough for us. > But, does we need to provide getter for others? I think so. (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 :) (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. (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. 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 (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. Created attachment 95860 [details]
updated patch
Proposed values for repaint throttling have been added to the doxygen documentation.
Created attachment 95861 [details]
updated patch
Added missing ChangeLog
Comment on attachment 95861 [details]
updated patch
Looks OK. Informal r+.
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 on attachment 95861 [details] updated patch Clearing flags on attachment: 95861 Committed r88533: <http://trac.webkit.org/changeset/88533> All reviewed patches have been landed. Closing bug. (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 ;) |