Bug 78401 – Move ScrollingCoordinator out of ENABLE(THREADED_SCROLLING) ifdef and enable on all platforms

• Component: New Bugs
• Status: RESOLVED FIXED

[James Robinson]

Enable ScrollingCoordinator code on Chromium

[James Robinson]

Created attachment 126607 [details]
Patch

[James Robinson]

Created attachment 126612 [details]
for EWS

[Gyuyoung Kim]

Comment on attachment 126612 [details]
for EWS

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

[James Robinson]

Created attachment 126619 [details]
for EWS

[James Robinson]

Created attachment 126661 [details]
for EWS

[James Robinson]

Created attachment 126671 [details]
for EWS

[Philippe Normand]

Comment on attachment 126671 [details]
for EWS

Attachment 126671 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/11507434

[James Robinson]

Created attachment 126683 [details]
Patch

[Philippe Normand]

Comment on attachment 126683 [details]
Patch

Attachment 126683 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/11511223

[James Robinson]

Anders, would you mind reviewing this?  This patch moves ScrollingCoordinator and ScrollingTreeState out of the ENABLE(THREADED_SCROLLING) #ifdef and adds the relevant build files to every build system.  In Chrmium we plan to use ScrollingCoordinator and the tree state management, but I think our compositor architecture requires that we can't reuse the THREADED_SCROLLING code directly (at least not at first).

Summary of changes:

*) stuff outside of Source/WebCore/page/scrolling - moving all ScrollingCoordinator out of #ifs.  This logic is all guarded by runtime checks so it won't impact the behavior of ports where the ScrollingCoordinator is 0x0.

*) build systems - adding all code moved out of the ENABLE()

*) Source/WebCore/page/scrolling/ScrollingCoordinator.h/cpp - separated code inherently tied to ENABLE(THREADED_SCROLLING) under this ifdef, but moved most code out

*) Source/WebCore/page/scrolling/ScrollingTreeState.h/cpp - no change except for removing ENABLE(THREADED_SCROLLING) and adding USE(ACCELERATED_COMPOSITING) guards since this file does not compile without that being set (as the gtk EWS bot proved)

[James Robinson]

Created attachment 126684 [details]
Patch

[Philippe Normand]

Comment on attachment 126684 [details]
Patch

Attachment 126684 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/11511226

[James Robinson]

Created attachment 126685 [details]
trying to make gtk non-accelerated compositing build happy

[Philippe Normand]

Comment on attachment 126685 [details]
trying to make gtk non-accelerated compositing build happy

Attachment 126685 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/11507594

[Collabora GTK+ EWS bot]

Comment on attachment 126685 [details]
trying to make gtk non-accelerated compositing build happy

Attachment 126685 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/11509521

[James Robinson]

Created attachment 126826 [details]
gtk buildfix

[James Robinson]

GTK bot is green, so this is good to go compile-wise.

[James Robinson]

Comment on attachment 126826 [details]
gtk buildfix

Clearing review? for now since this patch no longer applies/compiles.

[James Robinson]

Created attachment 127613 [details]
Patch

[Philippe Normand]

Comment on attachment 127613 [details]
Patch

Attachment 127613 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/11543218

[Early Warning System Bot]

Comment on attachment 127613 [details]
Patch

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

[James Robinson]

Created attachment 127623 [details]
Patch

[James Robinson]

Created attachment 127625 [details]
Patch

[Philippe Normand]

Comment on attachment 127625 [details]
Patch

Attachment 127625 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/11542446

[James Robinson]

Created attachment 127631 [details]
Patch

[James Robinson]

The approach here is to move ScrollingCoordinator completely out of #ifdefs and move the THREADED_SCROLLING bits of the implementation under the #ifdef, leaving stubs for other platforms.  ScrollingCoordinator is the only class that's completely outside of guards, everything else (ScrollingTree, ScrollingTreeState, etc) are still #if ENABLE(THREADED_SCROLLING) and chromium will probably not use them.

This approach does result in leaving stubs in ScrollingCoordinator.  A slightly more idomatic way to do this would be to have ScrollingCoordinator defer implementation to virtual functions and have a ThreadedScrollingCoordinator (or ScrollingCoordinatorThreaded) subclass that did the THREADED_SCROLLING-specific implementation, if you'd prefer virtuals to the stubs.  I'm happy either way.

Another less attractive approach for Chromium would be to simply provide an alternate ScrollingCoordinator.h and use include path trickery to use our implementation instead.  I think this would be less preferable since the two headers would have to stay in sync, but it would be less intrusive to the current THREADED_SCROLLING implementation at least in the short term.

[Adam Barth]

Comment on attachment 127631 [details]
Patch

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

Ok.  Generally, this approach is better than having two headers with the same name, although we do use that approach elsewhere (e.g., ResourceHandle).

> Source/WebCore/page/scrolling/ScrollingCoordinator.cpp:219
> +#if ENABLE(THREADED_SCROLLING)

btw, THREADED_SCROLLING should be a USE not an ENABLE.

> Source/WebCore/page/scrolling/ScrollingCoordinator.cpp:294
> +#if !PLATFORM(MAC) && !PLATFORM(CHROMIUM)

These ifdefs are slightly unfortunate.  It would be better if there was a way to capture this set without using PLATFORM names.

> Source/WebCore/page/scrolling/ScrollingTreeState.cpp:27
> +

nit: this change looks spurious

[James Robinson]

Comment on attachment 127631 [details]
Patch

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

>> Source/WebCore/page/scrolling/ScrollingCoordinator.cpp:294
>> +#if !PLATFORM(MAC) && !PLATFORM(CHROMIUM)
> 
> These ifdefs are slightly unfortunate.  It would be better if there was a way to capture this set without using PLATFORM names.

I'll collapse this into the #ifs below - !ENABLE(THREADED_SCROLLING) && !PLATFORM(CHROMIUM), but I'm not sure how to allow platforms to replace some of these functions while still linking successfully on platforms that don't want to bother with any of this.

Maybe I could have a ScrollingCoordinatorNone.cpp to define all of these symbols and put the platform guards there instead, to keep this file smaller?  I'm not sure what the ideal pattern is.  I'll go with this for now and if we have a good replacement go ahead and do that.

[James Robinson]

Committed r108140: <http://trac.webkit.org/changeset/108140>