[webkit-reviews] review granted: [Bug 184961] [GTK][WPE] Enable ASYNC_SCROLLING : [Attachment 338726] Patch

[bugzilla-daemon at webkit.org]

Carlos Garcia Campos <cgarcia at igalia.com> has granted Zan Dobersek
<zan at falconsigh.net>'s request for review:
Bug 184961: [GTK][WPE] Enable ASYNC_SCROLLING
https://bugs.webkit.org/show_bug.cgi?id=184961

Attachment 338726: Patch

https://bugs.webkit.org/attachment.cgi?id=338726&action=review




--- Comment #11 from Carlos Garcia Campos <cgarcia at igalia.com> ---
Comment on attachment 338726
  --> https://bugs.webkit.org/attachment.cgi?id=338726
Patch

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

> Source/WebCore/ChangeLog:3
> +	   [GTK][WPE] Enable ASYNC_SCROLLING

I still find this confusing, please rename the bug title as something like "add
initial implementation of async scrolling for coordinated graphics" or
something similar.

> Source/WebCore/ChangeLog:17
> +	   While the build-time flag is enabled, the feature is still not
usable
> +	   both due to the mentioned pending changes and further enabling in
the
> +	   WebKit layer.

Make it clear here that it's disabled at runtime, please. At least for me "not
usable" doesn't imply it's disabled.

> Source/WebKit/ChangeLog:14
> +	   Despite enabling the code at build-time, the feature (as intended)
is
> +	   not yet used because of the DrawingArea rejection in the WebPage
> +	   constructor.

Ok, it's clear here now, thanks!

>
Source/WebCore/page/scrolling/coordinatedgraphics/ScrollingCoordinatorCoordinat
edGraphics.cpp:45
> +    , m_scrollingStateTreeCommitterTimer(RunLoop::main(), this,
&ScrollingCoordinatorCoordinatedGraphics::commitTreeState)

What thread is this supposed to run in? Should we use current() instead of
main()? If it's the main one, we should probably set a priority to this timer,
I would add at least a FIXME to add it later.