Bug 151436 - [GTK] Implement initial support of asynchronous scrolling.
Summary: [GTK] Implement initial support of asynchronous scrolling.
Status: RESOLVED DUPLICATE of bug 184961
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: ChangSeok Oh
URL:
Keywords:
Depends on: 154066
Blocks: 153857 153858 154279
  Show dependency treegraph
 
Reported: 2015-11-18 23:25 PST by ChangSeok Oh
Modified: 2018-04-25 03:11 PDT (History)
11 users (show)

See Also:


Attachments
Patch (104.68 KB, patch)
2015-12-15 20:58 PST, ChangSeok Oh
no flags Details | Formatted Diff | Diff
Patch (113.70 KB, patch)
2016-02-04 00:51 PST, ChangSeok Oh
no flags Details | Formatted Diff | Diff
Patch (114.16 KB, patch)
2016-02-04 01:22 PST, ChangSeok Oh
no flags Details | Formatted Diff | Diff
Patch (112.56 KB, patch)
2016-02-11 00:45 PST, ChangSeok Oh
no flags Details | Formatted Diff | Diff
Patch (112.08 KB, patch)
2016-02-16 21:12 PST, ChangSeok Oh
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description ChangSeok Oh 2015-11-18 23:25:33 PST
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.
Comment 1 ChangSeok Oh 2015-12-15 20:58:46 PST
Created attachment 267430 [details]
Patch
Comment 2 WebKit Commit Bot 2015-12-15 21:01:09 PST
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.
Comment 3 Michael Catanzaro 2015-12-31 08:16:31 PST
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.
Comment 4 ChangSeok Oh 2016-02-04 00:51:05 PST
Created attachment 270638 [details]
Patch
Comment 5 WebKit Commit Bot 2016-02-04 00:52:57 PST
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.
Comment 6 ChangSeok Oh 2016-02-04 01:08:03 PST
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.
Comment 7 ChangSeok Oh 2016-02-04 01:08:32 PST
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.
Comment 8 ChangSeok Oh 2016-02-04 01:22:41 PST
Created attachment 270641 [details]
Patch
Comment 9 WebKit Commit Bot 2016-02-04 01:24:48 PST
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.
Comment 10 Gwang Yoon Hwang 2016-02-04 23:35:00 PST
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?
Comment 11 ChangSeok Oh 2016-02-10 22:17:35 PST
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().
Comment 12 ChangSeok Oh 2016-02-10 22:19:14 PST
(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.
Comment 13 ChangSeok Oh 2016-02-11 00:45:40 PST
Created attachment 271050 [details]
Patch
Comment 14 WebKit Commit Bot 2016-02-11 00:46:45 PST
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.
Comment 15 Zan Dobersek 2016-02-16 04:18:57 PST
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.
Comment 16 Zan Dobersek 2016-02-16 04:20:27 PST
Is there any dependence on the GTK+ toolkit itself? Or just the threaded compositor?
Comment 17 ChangSeok Oh 2016-02-16 21:12:21 PST
Created attachment 271530 [details]
Patch
Comment 18 WebKit Commit Bot 2016-02-16 21:14:58 PST
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.
Comment 19 ChangSeok Oh 2016-02-16 21:22:12 PST
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.
Comment 20 Michael Catanzaro 2017-04-30 17:44:16 PDT
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?
Comment 21 Zan Dobersek 2018-04-25 03:11:53 PDT

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