Bug 151436 – [GTK] Implement initial support of asynchronous scrolling.

• Status: RESOLVED
• Resolution: DUPLICATE
• Component: WebKitGTK

[ChangSeok Oh]

Mac and iOS port have a feature scrolling asynchronously. It would be good if gtk port does.
Before doing that, we need to investigate if it is really feasible for gtk port.

[ChangSeok Oh]

Created attachment 267430 [details]
Patch

[WebKit Commit Bot]

Attachment 267430 [details] did not pass style-queue:


ERROR: Source/WebCore/page/scrolling/ScrollingThread.cpp:64:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebCore/page/scrolling/ScrollingThread.h:90:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebCore/page/scrolling/gtk/ScrollingThreadGtk.cpp:98:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 3 in 47 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Michael Catanzaro]

Comment on attachment 267430 [details]
Patch

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

> Source/WebCore/ChangeLog:8
> +        This change brings the asyncronous scrolling feature to the gtk port. Basic idea is to handle 

asyncronous -> asynchronous

> Source/cmake/OptionsGTK.cmake:141
> +WEBKIT_OPTION_DEFAULT_PORT_VALUE(ENABLE_ASYNC_SCROLLING PRIVATE OFF)

I would set the default to ON, then do:

WEBKIT_OPTION_DEPEND(ENABLE_ASYNC_SCROLLING ENABLE_THREADED_COMPOSITOR)

Otherwise we might accidentally wind up enabling threaded compositor by default, but forgetting about async scrolling. And it's good to use WEBKIT_OPTION_DEPEND anyway when there is a dependency between options.

[ChangSeok Oh]

Created attachment 270638 [details]
Patch

[WebKit Commit Bot]

Attachment 270638 [details] did not pass style-queue:


ERROR: Source/WebCore/page/scrolling/ScrollingThread.cpp:64:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebCore/page/scrolling/ScrollingThread.h:90:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebCore/page/scrolling/gtk/ScrollingThreadGtk.cpp:98:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 3 in 49 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[ChangSeok Oh]

Comment on attachment 267430 [details]
Patch

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

Thanks for having a look at this, Michael!

>> Source/WebCore/ChangeLog:8
>> +        This change brings the asyncronous scrolling feature to the gtk port. Basic idea is to handle 
> 
> asyncronous -> asynchronous

Oops. Thanks.

>> Source/cmake/OptionsGTK.cmake:141
>> +WEBKIT_OPTION_DEFAULT_PORT_VALUE(ENABLE_ASYNC_SCROLLING PRIVATE OFF)
> 
> I would set the default to ON, then do:
> 
> WEBKIT_OPTION_DEPEND(ENABLE_ASYNC_SCROLLING ENABLE_THREADED_COMPOSITOR)
> 
> Otherwise we might accidentally wind up enabling threaded compositor by default, but forgetting about async scrolling. And it's good to use WEBKIT_OPTION_DEPEND anyway when there is a dependency between options.

Using WEBKIT_OPTION_DEPEND is a good idea. My concern was that this patch did not fully support asyc scrolling yet so it might break current scrolling. But o.k let's do it.

[ChangSeok Oh]

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

Thanks for having a look at this, Michael!

>> Source/WebCore/ChangeLog:8
>> +        This change brings the asyncronous scrolling feature to the gtk port. Basic idea is to handle 
> 
> asyncronous -> asynchronous

Oops. Thanks.

>> Source/cmake/OptionsGTK.cmake:141
>> +WEBKIT_OPTION_DEFAULT_PORT_VALUE(ENABLE_ASYNC_SCROLLING PRIVATE OFF)
> 
> I would set the default to ON, then do:
> 
> WEBKIT_OPTION_DEPEND(ENABLE_ASYNC_SCROLLING ENABLE_THREADED_COMPOSITOR)
> 
> Otherwise we might accidentally wind up enabling threaded compositor by default, but forgetting about async scrolling. And it's good to use WEBKIT_OPTION_DEPEND anyway when there is a dependency between options.

Using WEBKIT_OPTION_DEPEND is a good idea. My concern was that this patch did not fully support asyc scrolling yet so it might break current scrolling. But o.k let's do it.

[ChangSeok Oh]

Created attachment 270641 [details]
Patch

[WebKit Commit Bot]

Attachment 270641 [details] did not pass style-queue:


ERROR: Source/WebCore/page/scrolling/ScrollingThread.cpp:64:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebCore/page/scrolling/ScrollingThread.h:90:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebCore/page/scrolling/gtk/ScrollingThreadGtk.cpp:98:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 3 in 49 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Gwang Yoon Hwang]

Comment on attachment 270641 [details]
Patch

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

It is not easy to find a nice way to get position of layers without multi-threads problems.
What about caching positions of layers at the start of scrolling?
Maybe we can dump the positions of all layers in the compositing thread at the beginning of the scrolling,
instead of asking the position layer by layer.
And ScrollingNode.*Gtk can ask their layers position from the cached values.

> Source/WebKit2/Shared/CoordinatedGraphics/CoordinatedGraphicsScene.cpp:350
> +    return (id != InvalidCoordinatedLayerID) ? layerByID(id) : nullptr;

Nice!

> Source/WebKit2/Shared/CoordinatedGraphics/CoordinatedGraphicsScene.cpp:364
> +

But This change is not needed. )

> Source/WebKit2/Shared/CoordinatedGraphics/CoordinatedGraphicsScene.cpp:379
> +    }

I think this change came from your debugging code, isn't it?

> Source/WebKit2/UIProcess/WebPageProxy.cpp:1913
> +#if ENABLE(ASYNC_SCROLLING) && PLATFORM(IOS)

I think it is dangerous because PLATFORM(IOS) != PLATFORM(COCOA).
Are you sure there are no side effects to the non-IOS platform?

[ChangSeok Oh]

Comment on attachment 270641 [details]
Patch

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

>> Source/WebKit2/Shared/CoordinatedGraphics/CoordinatedGraphicsScene.cpp:364
>> +
> 
> But This change is not needed. )

Removed.

>> Source/WebKit2/Shared/CoordinatedGraphics/CoordinatedGraphicsScene.cpp:379
>> +    }
> 
> I think this change came from your debugging code, isn't it?

Nice catch!

>> Source/WebKit2/UIProcess/WebPageProxy.cpp:1913
>> +#if ENABLE(ASYNC_SCROLLING) && PLATFORM(IOS)
> 
> I think it is dangerous because PLATFORM(IOS) != PLATFORM(COCOA).
> Are you sure there are no side effects to the non-IOS platform?

Yes, I am. Because below touchPoint.location() is defined only for iOS in WebEvent.h. Other platforms do not have location().

[ChangSeok Oh]

(In reply to comment #10)
> Comment on attachment 270641 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=270641&action=review
> 
> It is not easy to find a nice way to get position of layers without
> multi-threads problems.
> What about caching positions of layers at the start of scrolling?
> Maybe we can dump the positions of all layers in the compositing thread at
> the beginning of the scrolling,
> instead of asking the position layer by layer.
> And ScrollingNode.*Gtk can ask their layers position from the cached values.
I think the caching way is worth to try. Thanks.

[ChangSeok Oh]

Created attachment 271050 [details]
Patch

[WebKit Commit Bot]

Attachment 271050 [details] did not pass style-queue:


ERROR: Source/WebCore/page/scrolling/ScrollingThread.cpp:64:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebCore/page/scrolling/ScrollingThread.h:90:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebCore/page/scrolling/gtk/ScrollingThreadGtk.cpp:98:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 3 in 49 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Zan Dobersek]

Comment on attachment 271050 [details]
Patch

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

> Source/WebCore/page/scrolling/ThreadedScrollingTree.cpp:124
>      RunLoop::main().dispatch([scrollingCoordinator, nodeID, horizontal, vertical] {
> +#if PLATFORM(MAC)
>          scrollingCoordinator->setActiveScrollSnapIndices(nodeID, horizontal, vertical);
> +#endif
>      });

Is it required to dispatch an empty task on the main thread?

> Source/WebCore/platform/gtk/ScrollController.cpp:52
> +    if (fabsf(deltaY) >= fabsf(deltaX))

std::abs() should work fine.

[Zan Dobersek]

Is there any dependence on the GTK+ toolkit itself? Or just the threaded compositor?

[ChangSeok Oh]

Created attachment 271530 [details]
Patch

[WebKit Commit Bot]

Attachment 271530 [details] did not pass style-queue:


ERROR: Source/WebCore/page/scrolling/ScrollingThread.cpp:64:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebCore/page/scrolling/ScrollingThread.h:90:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebCore/page/scrolling/gtk/ScrollingThreadGtk.cpp:98:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 3 in 48 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[ChangSeok Oh]

Comment on attachment 271050 [details]
Patch

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

> Is there any dependence on the GTK+ toolkit itself? Or just the threaded compositor?
Just the threaded compositor. No other dependencies reqruied.

>> Source/WebCore/page/scrolling/ThreadedScrollingTree.cpp:124
>>      });
> 
> Is it required to dispatch an empty task on the main thread?

No. it isn't. I thought gtk port might have something to do here but it seems not. currentSnapPointIndicesDidChange is related with RUBBER_BAND which gtk port does not support for now. Let me block the whole implementation of this method with PLATFORM(MAC).

>> Source/WebCore/platform/gtk/ScrollController.cpp:52
>> +    if (fabsf(deltaY) >= fabsf(deltaX))
> 
> std::abs() should work fine.

Done.

[Michael Catanzaro]

Comment on attachment 271530 [details]
Patch

Shame this never got reviewed. It was clearly a lot of work.

Are you still interested in updating this patch and submitting it for review again?

Is anyone more familiar with this code than me willing to review it?

[Zan Dobersek]

*** This bug has been marked as a duplicate of bug 184961 ***