Enable ScrollingCoordinator code on Chromium
Created attachment 126607 [details] Patch
Created attachment 126612 [details] for EWS
Comment on attachment 126612 [details] for EWS Attachment 126612 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/11496625
Created attachment 126619 [details] for EWS
Created attachment 126661 [details] for EWS
Created attachment 126671 [details] for EWS
Comment on attachment 126671 [details] for EWS Attachment 126671 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/11507434
Created attachment 126683 [details] Patch
Comment on attachment 126683 [details] Patch Attachment 126683 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/11511223
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)
Created attachment 126684 [details] Patch
Comment on attachment 126684 [details] Patch Attachment 126684 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/11511226
Created attachment 126685 [details] trying to make gtk non-accelerated compositing build happy
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 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
Created attachment 126826 [details] gtk buildfix
GTK bot is green, so this is good to go compile-wise.
Comment on attachment 126826 [details] gtk buildfix Clearing review? for now since this patch no longer applies/compiles.
Created attachment 127613 [details] Patch
Comment on attachment 127613 [details] Patch Attachment 127613 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/11543218
Comment on attachment 127613 [details] Patch Attachment 127613 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/11538558
Created attachment 127623 [details] Patch
Created attachment 127625 [details] Patch
Comment on attachment 127625 [details] Patch Attachment 127625 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/11542446
Created attachment 127631 [details] Patch
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 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 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.
Committed r108140: <http://trac.webkit.org/changeset/108140>