Bug 184961 – [GTK][WPE] Initial ASYNC_SCROLLING support

• Component: New Bugs
• Status: RESOLVED FIXED

[Zan Dobersek]

[GTK][WPE] Enable ASYNC_SCROLLING

[Zan Dobersek]

Created attachment 338715 [details]
Patch

[Carlos Garcia Campos]

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.

[Zan Dobersek]

*** Bug 151436 has been marked as a duplicate of this bug. ***

[Zan Dobersek]

*** Bug 153857 has been marked as a duplicate of this bug. ***

[Zan Dobersek]

*** Bug 153858 has been marked as a duplicate of this bug. ***

[Zan Dobersek]

*** Bug 154279 has been marked as a duplicate of this bug. ***

[Zan Dobersek]

Created attachment 338717 [details]
Patch

[Carlos Garcia Campos]

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?

[Zan Dobersek]

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?

[Zan Dobersek]

Created attachment 338726 [details]
Patch

[Carlos Garcia Campos]

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.

[Zan Dobersek]

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.

[Zan Dobersek]

Created attachment 338863 [details]
Patch for landing

[Zan Dobersek]

Comment on attachment 338863 [details]
Patch for landing

Clearing flags on attachment: 338863

Committed r231043: <https://trac.webkit.org/changeset/231043>

[Zan Dobersek]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/39750623>