Bug 184961 - [GTK][WPE] Initial ASYNC_SCROLLING support
Summary: [GTK][WPE] Initial ASYNC_SCROLLING support
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Zan Dobersek
URL:
Keywords: InRadar
: 151436 153857 153858 154279 (view as bug list)
Depends on:
Blocks:
 
Reported: 2018-04-25 02:03 PDT by Zan Dobersek
Modified: 2018-04-26 00:55 PDT (History)
4 users (show)

See Also:


Attachments
Patch (51.65 KB, patch)
2018-04-25 02:18 PDT, Zan Dobersek
no flags Details | Formatted Diff | Diff
Patch (52.14 KB, patch)
2018-04-25 03:39 PDT, Zan Dobersek
no flags Details | Formatted Diff | Diff
Patch (52.37 KB, patch)
2018-04-25 06:47 PDT, Zan Dobersek
no flags Details | Formatted Diff | Diff
Patch for landing (52.61 KB, patch)
2018-04-26 00:36 PDT, Zan Dobersek
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Zan Dobersek 2018-04-25 02:03:45 PDT
[GTK][WPE] Enable ASYNC_SCROLLING
Comment 1 Zan Dobersek 2018-04-25 02:18:58 PDT
Created attachment 338715 [details]
Patch
Comment 2 Carlos Garcia Campos 2018-04-25 02:51:22 PDT
Is this the same as bug #151436? If this patch makes the one attached there obsolete, let's mark one of them as duplicate, please.
Comment 3 Zan Dobersek 2018-04-25 03:11:53 PDT
*** Bug 151436 has been marked as a duplicate of this bug. ***
Comment 4 Zan Dobersek 2018-04-25 03:25:26 PDT
*** Bug 153857 has been marked as a duplicate of this bug. ***
Comment 5 Zan Dobersek 2018-04-25 03:25:30 PDT
*** Bug 153858 has been marked as a duplicate of this bug. ***
Comment 6 Zan Dobersek 2018-04-25 03:25:33 PDT
*** Bug 154279 has been marked as a duplicate of this bug. ***
Comment 7 Zan Dobersek 2018-04-25 03:39:06 PDT
Created attachment 338717 [details]
Patch
Comment 8 Carlos Garcia Campos 2018-04-25 05:46:51 PDT
Comment on attachment 338717 [details]
Patch

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

> Source/WebCore/page/scrolling/coordinatedgraphics/ScrollingCoordinatorCoordinatedGraphics.h:36
> +class ScrollingCoordinatorCoordinatedGraphics : public AsyncScrollingCoordinator {

final?

> Source/WebCore/page/scrolling/coordinatedgraphics/ScrollingCoordinatorCoordinatedGraphics.h:52
> +    Timer m_scrollingStateTreeCommitterTimer;

Any reason to use a WebCore timer here? Could we use a RunLoop::Timer instead that we can also set a priority?

> Source/WebCore/page/scrolling/coordinatedgraphics/ScrollingTreeCoordinatedGraphics.cpp:65
> +    ASSERT_NOT_REACHED();
> +    return ScrollingTreeFixedNode::create(*this, nodeID);

If we use RELEASE_ASSERT_NOT_REACHED we don't need to add a fake return here.

> Source/WebCore/page/scrolling/coordinatedgraphics/ScrollingTreeFixedNode.h:36
> +class ScrollingTreeFixedNode : public ScrollingTreeNode {

final?

> Source/WebCore/page/scrolling/coordinatedgraphics/ScrollingTreeFrameScrollingNodeCoordinatedGraphics.h:36
> +class ScrollingTreeFrameScrollingNodeCoordinatedGraphics : public ScrollingTreeFrameScrollingNode {

final?

> Source/WebCore/page/scrolling/coordinatedgraphics/ScrollingTreeStickyNode.h:36
> +class ScrollingTreeStickyNode : public ScrollingTreeNode {

final?
Comment 9 Zan Dobersek 2018-04-25 06:34:55 PDT
Comment on attachment 338717 [details]
Patch

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

>> Source/WebCore/page/scrolling/coordinatedgraphics/ScrollingCoordinatorCoordinatedGraphics.h:36
>> +class ScrollingCoordinatorCoordinatedGraphics : public AsyncScrollingCoordinator {
> 
> final?

OK.

>> Source/WebCore/page/scrolling/coordinatedgraphics/ScrollingCoordinatorCoordinatedGraphics.h:52
>> +    Timer m_scrollingStateTreeCommitterTimer;
> 
> Any reason to use a WebCore timer here? Could we use a RunLoop::Timer instead that we can also set a priority?

OK.

>> Source/WebCore/page/scrolling/coordinatedgraphics/ScrollingTreeCoordinatedGraphics.cpp:65
>> +    return ScrollingTreeFixedNode::create(*this, nodeID);
> 
> If we use RELEASE_ASSERT_NOT_REACHED we don't need to add a fake return here.

We have to return something because of Ref<> being the return value, or am I missing something?
Comment 10 Zan Dobersek 2018-04-25 06:47:53 PDT
Created attachment 338726 [details]
Patch
Comment 11 Carlos Garcia Campos 2018-04-25 23:48:58 PDT
Comment on attachment 338726 [details]
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/ScrollingCoordinatorCoordinatedGraphics.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.
Comment 12 Zan Dobersek 2018-04-26 00:32:17 PDT
Comment on attachment 338726 [details]
Patch

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

>> Source/WebCore/page/scrolling/coordinatedgraphics/ScrollingCoordinatorCoordinatedGraphics.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.

The main one. It's not clear yet what the best priority for it should be, so I'll add a FIXME.
Comment 13 Zan Dobersek 2018-04-26 00:36:11 PDT
Created attachment 338863 [details]
Patch for landing
Comment 14 Zan Dobersek 2018-04-26 00:54:22 PDT
Comment on attachment 338863 [details]
Patch for landing

Clearing flags on attachment: 338863

Committed r231043: <https://trac.webkit.org/changeset/231043>
Comment 15 Zan Dobersek 2018-04-26 00:54:26 PDT
All reviewed patches have been landed.  Closing bug.
Comment 16 Radar WebKit Bug Importer 2018-04-26 00:55:22 PDT
<rdar://problem/39750623>