Bug 91329 – Enable using ScrollingCoordinator in a multi-process environment

• Component: WebCore Misc.
• Status: RESOLVED INVALID

[Noam Rosenthal]

Enable using ScrollingCoordinator in a multi-process environment

[Noam Rosenthal]

Created attachment 152440 [details]
Patch

[Noam Rosenthal]

This patch comes following a discussion on IRC with andersca, trying to find a way to separate the threaded aspects of ScrollingTree from the transaction-based ScrollingTreeState which can also be used in WebKit2 or in other multi-process environments.

It's not yet enabled so I'm not foreseeing any changes in behavior, but hopefully the refactor would allow us to reuse ScrollingCoordinator in WebKit2 in the future.

[Noam Rosenthal]

Created attachment 152441 [details]
Patch

[Gyuyoung Kim]

Comment on attachment 152441 [details]
Patch

Attachment 152441 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/13246499

[WebKit Review Bot]

Comment on attachment 152441 [details]
Patch

Attachment 152441 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13248136

[Build Bot]

Comment on attachment 152441 [details]
Patch

Attachment 152441 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13232698

[Early Warning System Bot]

Comment on attachment 152441 [details]
Patch

Attachment 152441 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/13249071

[Noam Rosenthal]

Comment on attachment 152441 [details]
Patch

Seems to be missing some stuff on some (most) platforms. Will check what it is and resubmit.

[Noam Rosenthal]

Created attachment 152442 [details]
Patch

[Gyuyoung Kim]

Comment on attachment 152442 [details]
Patch

Attachment 152442 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/13238556

[Early Warning System Bot]

Comment on attachment 152442 [details]
Patch

Attachment 152442 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/13243568

[Build Bot]

Comment on attachment 152442 [details]
Patch

Attachment 152442 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13242568

[Noam Rosenthal]

Created attachment 152443 [details]
Trying to account for the different #ifdef blends

[Kenneth Rohde Christiansen]

Comment on attachment 152443 [details]
Trying to account for the different #ifdef blends

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

> Source/WebKit2/ChangeLog:8
> +        Added WebKit2-support for ScrollingCoordinator.

So Mac is not using this with WebKit 2 yet?

> Source/WebKit2/ChangeLog:17
> +        Since this code path is still under development, t's guarded in runtime by an enviroment

it's*

> Source/WebCore/page/scrolling/ScrollingCoordinator.cpp:102
> +#if ENABLE(THREADED_SCROLLING)
> +    return m_scrollingTree.get();
> +#else
> +    return m_client;
> +#endif

Would it make sense to turn the scrolling tree into a proper client in the future?

> Source/WebCore/page/scrolling/ScrollingCoordinatorClient.h:42
> +    virtual IntPoint mainFrameScrollPosition() = 0;

Won't we use this for subframes in the future?

> Source/WebCore/page/scrolling/ScrollingTree.h:110
> +
> +

double newline

> Source/WebKit2/Shared/WebCoreArgumentCoders.cpp:671
> +    for (size_t i = 0; i < rects.size(); ++i)
> +        region.unite(Region(rects[i]));

You encode multiple regions but we when decoding you unite them? comment?

> Source/WebKit2/Shared/WebCoreArgumentCoders.cpp:676
> +
> +

double new line

> Source/WebKit2/Shared/WebCoreArgumentCoders.cpp:824
> +    if (changedProperties & ScrollingTreeState::WheelEventHandlerCount)
> +        encoder->encode(state.wheelEventHandlerCount());

I guess a next step would be adding TouchEventHandlerCount :-) or can that be handled by NonFastScrollableRegion?

> Source/WebKit2/UIProcess/WebScrollingCoordinatorProxy.cpp:8
> +    This library is free software; you can redistribute it and/or
> +    modify it under the terms of the GNU Library General Public
> +    License as published by the Free Software Foundation; either
> +    version 2 of the License, or (at your option) any later version.
> +

Any reason to not use BSD license here?

> Source/WebKit2/UIProcess/WebScrollingCoordinatorProxy.cpp:42
> +    // FIXME: Implement.

notImplemented() ?

> Source/WebKit2/WebProcess/WebPage/WebScrollingCoordinator.cpp:44
> +static bool canUseCoordinator()

shouldUseCoordinator>?

[Noam Rosenthal]

(In reply to comment #14)
> So Mac is not using this with WebKit 2 yet?
Not for this... 

> > Source/WebKit2/ChangeLog:17
> > +        Since this code path is still under development, t's guarded in runtime by an enviroment
> 
> it's*
> 
> > Source/WebCore/page/scrolling/ScrollingCoordinator.cpp:102
> > +#if ENABLE(THREADED_SCROLLING)
> > +    return m_scrollingTree.get();
> > +#else
> > +    return m_client;
> > +#endif
> 
> Would it make sense to turn the scrolling tree into a proper client in the future?

It is a proper client. It just isn't saved as an additional member. Couldn't find a cleaner way to do that, suggestions?

> > Source/WebCore/page/scrolling/ScrollingCoordinatorClient.h:42
> > +    virtual IntPoint mainFrameScrollPosition() = 0;
> 
> Won't we use this for subframes in the future?
Probably :) Right now I'm just exposing what scrollingCoordinator needs.

> You encode multiple regions but we when decoding you unite them? comment?
I encode multiple rects and then unite them to a new region.

> Any reason to not use BSD license here?
Copy paste :)

[Kenneth Rohde Christiansen]

> > Would it make sense to turn the scrolling tree into a proper client in the future?
> 
> It is a proper client. It just isn't saved as an additional member. Couldn't find a cleaner way to do that, suggestions?

Then I would have to look better at the code, but I think it is fine.

> > > Source/WebCore/page/scrolling/ScrollingCoordinatorClient.h:42
> > > +    virtual IntPoint mainFrameScrollPosition() = 0;
> > 
> > Won't we use this for subframes in the future?
> Probably :) Right now I'm just exposing what scrollingCoordinator needs.

Yeah, I saw the comment later down in the original code that it doesnt support sub frames yet.
 
> > You encode multiple regions but we when decoding you unite them? comment?
> I encode multiple rects and then unite them to a new region.
> 
> > Any reason to not use BSD license here?
> Copy paste :)

If that is the standard license for WebKit2 UI side code then I am fine with it. I just wondered.

So this current patch compiles (I cannot see the EWs status due to being in China without working VPN)

[Noam Rosenthal]

> So this current patch compiles (I cannot see the EWs status due to being in China without working VPN)
Yes, EWS passes.

[Kenneth Rohde Christiansen]

Comment on attachment 152443 [details]
Trying to account for the different #ifdef blends

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

> Source/WebCore/ChangeLog:8
> +        Moved the threaded behavior out of ScrollingCoordinator and into ScrollingTree.

Should it be called ThreadedScrollingTree?

> Source/WebCore/ChangeLog:13
> +        A new ENABLE(PARALLEL_SCROLLING) guard has been added to include both the existing
> +        THREADED_SCROLLING path and the new cross-process scrolling.

so PARALLEL_SCROLLING turned on means that THREADED_SCROLLING is also turned on, that is not what you wanted to say is it?

> Source/WebCore/page/scrolling/ScrollingCoordinatorClient.h:41
> +class ScrollingCoordinatorClient {
> +public:
> +    virtual void commitScrollingTreeState(const ScrollingTreeState&) = 0;

It is a bit confusing that we are not using the actual ScrollingTree but a client, but we still use things related to ScrollingTree. I think we need to rename ScrollingTree to ScrollingTreeThreaded or so to avoid confusion.

[Noam Rosenthal]

(In reply to comment #18)
> (From update of attachment 152443 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=152443&action=review
> 
> > Source/WebCore/ChangeLog:8
> > +        Moved the threaded behavior out of ScrollingCoordinator and into ScrollingTree.
> 
> Should it be called ThreadedScrollingTree?
Sounds good to me.

> 
> > Source/WebCore/ChangeLog:13
> > +        A new ENABLE(PARALLEL_SCROLLING) guard has been added to include both the existing
> > +        THREADED_SCROLLING path and the new cross-process scrolling.
> 
> so PARALLEL_SCROLLING turned on means that THREADED_SCROLLING is also turned on, that is not what you wanted to say is it?
No. I mean to say that the code included in PARALLEL_SCROLLING includes all the code previously included in THREADED_SCROLLING. 

> It is a bit confusing that we are not using the actual ScrollingTree but a client, but we still use things related to ScrollingTree. I think we need to rename ScrollingTree to ScrollingTreeThreaded or so to avoid confusion.

I agree.

[Noam Rosenthal]

Created attachment 152857 [details]
Patch

[Anders Carlsson]

I hope to be able to take a look at this tomorrow. However, (without looking at the patch) I think you should try to split it up into smaller pieces. It's over 90K.

[Noam Rosenthal]

Created attachment 152864 [details]
Patch 1 - only the WebCore parts

Uploading a patch with only the WebCore parts. The other patch can serve as a reference to how this is going to be used.

[Build Bot]

Comment on attachment 152864 [details]
Patch 1 - only the WebCore parts

Attachment 152864 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13274295

[Build Bot]

Comment on attachment 152864 [details]
Patch 1 - only the WebCore parts

Attachment 152864 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13272298

[WebKit Review Bot]

Comment on attachment 152864 [details]
Patch 1 - only the WebCore parts

Attachment 152864 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13282293

[Gyuyoung Kim]

Comment on attachment 152864 [details]
Patch 1 - only the WebCore parts

Attachment 152864 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/13280301

[Noam Rosenthal]

Created attachment 152881 [details]
Patch

[Kenneth Rohde Christiansen]

Comment on attachment 152881 [details]
Patch

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

Can you get Anders to look at this?

> Source/WebCore/WebCore.xcodeproj/project.pbxproj:14421
> -				1AAADDA114DB409F00AF64B3 /* ScrollingTree.cpp */,
> -				1AAADDA214DB409F00AF64B3 /* ScrollingTree.h */,
> +                                1AAADDA114DB409F00AF64B3 /* ThreadedScrollingTree.cpp */,
> +                                1AAADDA214DB409F00AF64B3 /* ThreadedScrollingTree.h */,

something went wrong with identation?

> Source/WebCore/page/scrolling/ScrollingTreeNode.h:60
> +#if ENABLE(THREADED_SCROLLING)
> +    ScrollingTree* scrollingTree() const { return static_cast<ThreadedScrollingTree*>(m_client); }
> +#endif

maybe a toScrollingTree() would be nicer? 

So you call toScrollingTree(client())-> instead?

[Noam Rosenthal]

(In reply to comment #28)
> (From update of attachment 152881 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=152881&action=review
> 
> Can you get Anders to look at this?
andersca: ping :)

[Noam Rosenthal]

ScrollingCoordinator has evolved since this patch to where it's not the right way to go.