Bug 78401 - Move ScrollingCoordinator out of ENABLE(THREADED_SCROLLING) ifdef and enable on all platforms
Summary: Move ScrollingCoordinator out of ENABLE(THREADED_SCROLLING) ifdef and enable ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: James Robinson
URL:
Keywords:
Depends on:
Blocks: 78864
  Show dependency treegraph
 
Reported: 2012-02-10 17:33 PST by James Robinson
Modified: 2012-02-17 16:23 PST (History)
11 users (show)

See Also:


Attachments
Patch (27.89 KB, patch)
2012-02-10 17:36 PST, James Robinson
no flags Details | Formatted Diff | Diff
for EWS (39.88 KB, patch)
2012-02-10 18:19 PST, James Robinson
no flags Details | Formatted Diff | Diff
for EWS (40.47 KB, patch)
2012-02-10 21:22 PST, James Robinson
no flags Details | Formatted Diff | Diff
for EWS (40.47 KB, patch)
2012-02-11 18:11 PST, James Robinson
no flags Details | Formatted Diff | Diff
for EWS (41.96 KB, patch)
2012-02-11 21:37 PST, James Robinson
no flags Details | Formatted Diff | Diff
Patch (42.25 KB, patch)
2012-02-12 13:53 PST, James Robinson
no flags Details | Formatted Diff | Diff
Patch (42.23 KB, patch)
2012-02-12 14:02 PST, James Robinson
no flags Details | Formatted Diff | Diff
trying to make gtk non-accelerated compositing build happy (43.01 KB, patch)
2012-02-12 14:55 PST, James Robinson
no flags Details | Formatted Diff | Diff
gtk buildfix (43.21 KB, patch)
2012-02-13 13:57 PST, James Robinson
no flags Details | Formatted Diff | Diff
Patch (51.70 KB, patch)
2012-02-17 11:00 PST, James Robinson
no flags Details | Formatted Diff | Diff
Patch (47.61 KB, patch)
2012-02-17 11:44 PST, James Robinson
no flags Details | Formatted Diff | Diff
Patch (47.65 KB, patch)
2012-02-17 11:47 PST, James Robinson
no flags Details | Formatted Diff | Diff
Patch (48.27 KB, patch)
2012-02-17 12:43 PST, James Robinson
abarth: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description James Robinson 2012-02-10 17:33:43 PST
Enable ScrollingCoordinator code on Chromium
Comment 1 James Robinson 2012-02-10 17:36:45 PST
Created attachment 126607 [details]
Patch
Comment 2 James Robinson 2012-02-10 18:19:17 PST
Created attachment 126612 [details]
for EWS
Comment 3 Gyuyoung Kim 2012-02-10 19:32:53 PST
Comment on attachment 126612 [details]
for EWS

Attachment 126612 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/11496625
Comment 4 James Robinson 2012-02-10 21:22:01 PST
Created attachment 126619 [details]
for EWS
Comment 5 James Robinson 2012-02-11 18:11:59 PST
Created attachment 126661 [details]
for EWS
Comment 6 James Robinson 2012-02-11 21:37:20 PST
Created attachment 126671 [details]
for EWS
Comment 7 Philippe Normand 2012-02-12 03:24:06 PST
Comment on attachment 126671 [details]
for EWS

Attachment 126671 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/11507434
Comment 8 James Robinson 2012-02-12 13:53:16 PST
Created attachment 126683 [details]
Patch
Comment 9 Philippe Normand 2012-02-12 13:59:31 PST
Comment on attachment 126683 [details]
Patch

Attachment 126683 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/11511223
Comment 10 James Robinson 2012-02-12 14:01:07 PST
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)
Comment 11 James Robinson 2012-02-12 14:02:14 PST
Created attachment 126684 [details]
Patch
Comment 12 Philippe Normand 2012-02-12 14:11:15 PST
Comment on attachment 126684 [details]
Patch

Attachment 126684 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/11511226
Comment 13 James Robinson 2012-02-12 14:55:48 PST
Created attachment 126685 [details]
trying to make gtk non-accelerated compositing build happy
Comment 14 Philippe Normand 2012-02-12 16:03:28 PST
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
Comment 15 Collabora GTK+ EWS bot 2012-02-12 16:06:12 PST
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
Comment 16 James Robinson 2012-02-13 13:57:16 PST
Created attachment 126826 [details]
gtk buildfix
Comment 17 James Robinson 2012-02-13 14:17:34 PST
GTK bot is green, so this is good to go compile-wise.
Comment 18 James Robinson 2012-02-16 17:05:45 PST
Comment on attachment 126826 [details]
gtk buildfix

Clearing review? for now since this patch no longer applies/compiles.
Comment 19 James Robinson 2012-02-17 11:00:38 PST
Created attachment 127613 [details]
Patch
Comment 20 Philippe Normand 2012-02-17 11:09:44 PST
Comment on attachment 127613 [details]
Patch

Attachment 127613 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/11543218
Comment 21 Early Warning System Bot 2012-02-17 11:10:02 PST
Comment on attachment 127613 [details]
Patch

Attachment 127613 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/11538558
Comment 22 James Robinson 2012-02-17 11:44:21 PST
Created attachment 127623 [details]
Patch
Comment 23 James Robinson 2012-02-17 11:47:18 PST
Created attachment 127625 [details]
Patch
Comment 24 Philippe Normand 2012-02-17 11:57:24 PST
Comment on attachment 127625 [details]
Patch

Attachment 127625 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/11542446
Comment 25 James Robinson 2012-02-17 12:43:37 PST
Created attachment 127631 [details]
Patch
Comment 26 James Robinson 2012-02-17 14:30:38 PST
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.
Comment 27 Adam Barth 2012-02-17 16:10:59 PST
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
Comment 28 James Robinson 2012-02-17 16:22:06 PST
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.
Comment 29 James Robinson 2012-02-17 16:23:30 PST
Committed r108140: <http://trac.webkit.org/changeset/108140>