ScrollingCoordinatorCoordinatedGraphics should implement threaded scrolling interface and re-use threaded scrolling approach (ScrollingStateTree and ViewportConstraints) to handle fixed(sticky) elements positioning while scrolling. The rational: 1) Currently ScrollingCoordinatorCoordinatedGraphics implements Chromium-specific interface which is going to be removed eventually as there is no ScrollingCoordinatorChromium existing.(besides this interface has been modified and fixed elements positioning is broken, see https://bugs.webkit.org/show_bug.cgi?id=113932) 2) Threaded scrolling has a lot of useful functionality for handling both fixed and sticky elements positioning while scrolling that will be needed in coord graphics as well.
*** Bug 113932 has been marked as a duplicate of this bug. ***
Created attachment 197251 [details] WIP Qt build support is absent as well as change log, however the approach is clear.
Comment on attachment 197251 [details] WIP Attachment 197251 [details] did not pass qt-ews (qt): Output: http://webkit-commit-queue.appspot.com/results/17746002
Comment on attachment 197251 [details] WIP Attachment 197251 [details] did not pass efl-ews (efl): Output: http://webkit-commit-queue.appspot.com/results/17623022
Comment on attachment 197251 [details] WIP Attachment 197251 [details] did not pass qt-wk2-ews (qt): Output: http://webkit-commit-queue.appspot.com/results/17692052
(In reply to comment #4) > (From update of attachment 197251 [details]) > Attachment 197251 [details] did not pass efl-ews (efl): > Output: http://webkit-commit-queue.appspot.com/results/17623022 Apparently need to add ScrollingStateScrollingNode::setCounterScrollingLayer definition, strange that it builds for me locally :/
Created attachment 197254 [details] WIP 2 Check EWS.
Created attachment 197255 [details] WIP 2 Check EWS.
Comment on attachment 197255 [details] WIP 2 Attachment 197255 [details] did not pass qt-ews (qt): Output: http://webkit-commit-queue.appspot.com/results/17699059
Comment on attachment 197255 [details] WIP 2 Attachment 197255 [details] did not pass efl-ews (efl): Output: http://webkit-commit-queue.appspot.com/results/17670068
Comment on attachment 197255 [details] WIP 2 Attachment 197255 [details] did not pass qt-wk2-ews (qt): Output: http://webkit-commit-queue.appspot.com/results/17609042
Created attachment 197260 [details] WIP 3 Added missing functions definitions, completely removed guards from ScrollingTreeState files.
Created attachment 197519 [details] patch
Comment on attachment 197519 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=197519&action=review > Source/WebCore/page/scrolling/coordinatedgraphics/ScrollingCoordinatorCoordinatedGraphics.cpp:59 > + ASSERT(frameView->scrollLayerID()); > + attachToStateTree(ScrollingNode, frameView->scrollLayerID(), 0); Do you need this now? This might cause nasty side effects for WKCoordinatedSceneFindScrollableContentsLayerAt. > Source/WebCore/page/scrolling/coordinatedgraphics/ScrollingStateNodeCoordinatedGraphics.cpp:51 > +void ScrollingStateNode::setScrollLayer(GraphicsLayer* graphicsLayer) > +{ > + m_graphicsLayer = graphicsLayer; > +} This might not be needed but Mac has this suspicious code that could be helpful when an existing node has its GraphicsLayer changed: setPropertyChanged(ScrollLayer); m_scrollingStateTree->setHasChangedProperties(true);
Comment on attachment 197519 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=197519&action=review >> Source/WebCore/page/scrolling/coordinatedgraphics/ScrollingCoordinatorCoordinatedGraphics.cpp:59 >> + attachToStateTree(ScrollingNode, frameView->scrollLayerID(), 0); > > Do you need this now? > This might cause nasty side effects for WKCoordinatedSceneFindScrollableContentsLayerAt. agree, this could be obviated as we need scroll state tree just to track constrained elements so far. >> Source/WebCore/page/scrolling/coordinatedgraphics/ScrollingStateNodeCoordinatedGraphics.cpp:51 >> +} > > This might not be needed but Mac has this suspicious code that could be helpful when an existing node has its GraphicsLayer changed: > setPropertyChanged(ScrollLayer); > m_scrollingStateTree->setHasChangedProperties(true); mm.. I do not see how it can be used for us, as we do not use scroll state tree "commits"
Created attachment 197581 [details] patch v2 removed ScrollingCoordinatorCoordinatedGraphics::frameViewRootLayerDidChange
Comment on attachment 197581 [details] patch v2 View in context: https://bugs.webkit.org/attachment.cgi?id=197581&action=review > Source/WebCore/ChangeLog:10 > + fixed(sticky) elements positioning while scrolling. The rationals are below. fixed/sticky > Source/WebCore/ChangeLog:17 > + Threaded scrolling has a lot of useful functionality for handling both fixed and sticky elements > + positioning while scrolling that will be needed in coord graphics as well. Comment unnecessary :) > Source/WebCore/ChangeLog:36 > + Remove line > Source/WebCore/ChangeLog:48 > + Remove line
Committed r148197: <http://trac.webkit.org/changeset/148197>
(In reply to comment #18) > Committed r148197: <http://trac.webkit.org/changeset/148197> This broke the Lion Build http://build.webkit.org/builders/Apple%20Lion%20Debug%20%28Build%29/builds/14913/steps/compile-webkit/logs/stdio Ld /Volumes/Data/slave/lion-debug/build/WebKitBuild/Debug/WebCore.framework/Versions/A/WebCore normal x86_64 cd /Volumes/Data/slave/lion-debug/build/Source/WebCore setenv MACOSX_DEPLOYMENT_TARGET 10.7 /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/clang++ -arch x86_64 -dynamiclib -L/Volumes/Data/slave/lion-debug/build/WebKitBuild/Debug -F/Volumes/Data/slave/lion-debug/build/WebKitBuild/Debug -F/System/Library/Frameworks/Carbon.framework/Frameworks -F/System/Library/Frameworks/ApplicationServices.framework/Frameworks -F/System/Library/Frameworks/CoreServices.framework/Frameworks -filelist /Volumes/Data/slave/lion-debug/build/WebKitBuild/WebCore.build/Debug/WebCore.build/Objects-normal/x86_64/WebCore.LinkFileList -Xlinker --no-demangle -exported_symbols_list /Volumes/Data/slave/lion-debug/build/WebKitBuild/Debug/DerivedSources/WebCore/WebCore.LP64.exp -install_name /System/Library/Frameworks/WebKit.framework/Versions/A/Frameworks/WebCore.framework/Versions/A/WebCore -mmacosx-version-min=10.7 -lsqlite3 -lobjc -lANGLE -allowable_client WebCoreTestSupport -sub_library libobjc -umbrella WebKit -allowable_client WebKit2 -framework IOSurface -stdlib=libc++ -fobjc-link-runtime -framework Accelerate -framework ApplicationServices -framework AudioToolbox -framework AudioUnit -framework Carbon -framework Cocoa -framework CoreAudio -framework IOKit -framework JavaScriptCore -licucore -lobjc -lxml2 -lz -framework OpenGL -framework QuartzCore -framework SystemConfiguration -single_module -compatibility_version 1 -current_version 537.37 -o /Volumes/Data/slave/lion-debug/build/WebKitBuild/Debug/WebCore.framework/Versions/A/WebCore Undefined symbols for architecture x86_64: "__ZN7WebCore18ScrollingStateNode22setScrollPlatformLayerEP7CALayer", referenced from: __ZN7WebCore18ScrollingStateNodeC2ERKS0_ in ScrollingStateNode.o "__ZN7WebCore27ScrollingStateScrollingNode24setCounterScrollingLayerEPNS_13GraphicsLayerE", referenced from: __ZN7WebCore27ScrollingStateScrollingNodeC2ERKS0_ in ScrollingStateScrollingNode.o "__ZNK7WebCore18ScrollingStateNode19platformScrollLayerEv", referenced from: __ZN7WebCore18ScrollingStateNodeC2ERKS0_ in ScrollingStateNode.o ld: symbol(s) not found for architecture x86_64 clang: error: linker command failed with exit code 1 (use -v to see invocation)
Lion build fix attempted in http://trac.webkit.org/changeset/148239.
Comment on attachment 197581 [details] patch v2 So now all platforms that include these files build Scrolling* code that is only needed by two of them?
(In reply to comment #21) > (From update of attachment 197581 [details]) > So now all platforms that include these files build Scrolling* code that is only needed by two of them? I think that’s fine.
Another Lion build fix attempted in http://trac.webkit.org/changeset/148252.
This broke the GTK+ build (the bots are showing it, likely because they haven't done a full build). It's been broken for a day now. Here's the error: ./.libs/libwebkitgtk-3.0.so: undefined reference to `WebCore::ScrollingStateScrollingNode::setCounterScrollingLayer(WebCore::GraphicsLayer*)' ./.libs/libwebkitgtk-3.0.so: undefined reference to `WebCore::ScrollingStateNode::setScrollPlatformLayer(WebCore::TextureMapperPlatformLayer*)' ./.libs/libwebkitgtk-3.0.so: undefined reference to `WebCore::ScrollingStateNode::platformScrollLayer() const' ./.libs/libwebkitgtk-3.0.so: undefined reference to `WebCore::ScrollingStateStickyNode::create(WebCore::ScrollingStateTree*, unsigned long)' collect2: error: ld returned 1 exit status ./.libs/libwebkitgtk-3.0.so: undefined reference to `WebCore::ScrollingStateScrollingNode::setCounterScrollingLayer(WebCore::GraphicsLayer*)' ./.libs/libwebkitgtk-3.0.so: undefined reference to `WebCore::ScrollingStateNode::setScrollPlatformLayer(WebCore::TextureMapperPlatformLayer*)' ./.libs/libwebkitgtk-3.0.so: undefined reference to `WebCore::ScrollingStateNode::platformScrollLayer() const' ./.libs/libwebkitgtk-3.0.so: undefined reference to `WebCore::ScrollingStateStickyNode::create(WebCore::ScrollingStateTree*, unsigned long)' collect2: error: ld returned 1 exit status ./.libs/libwebkitgtk-3.0.so: undefined reference to `WebCore::ScrollingStateScrollingNode::setCounterScrollingLayer(WebCore::GraphicsLayer*)' ./.libs/libwebkitgtk-3.0.so: undefined reference to `WebCoremake[1]: ::ScrollingStateNode::setScrollPlatformLayer(WebCore::TextureMapperPlatformLayer*)' ./.libs/libwebkitgtk-3.0.so: undefined reference to `WebCore::ScrollingStateNode::platformScrollLayer() const' ./.libs/libwebkitgtk-3.0.so: *** [Programs/unittests/testapplicationcache] Error 1undefined reference to `WebCore::ScrollingStateStickyNode::.create(WebCore::make[1]: ScrollingStateTree/**** Waiting for unfinished jobs...., . unsignedlibs long/)make[1]: libwebkitgtk*** [Programs/unittests/testdomdomwindow] Error 1'-
(In reply to comment #24) > This broke the GTK+ build (the bots are showing it, likely because they haven't done a full build). It's been broken for a day now. Here's the error: > > ./.libs/libwebkitgtk-3.0.so: undefined reference to `WebCore::ScrollingStateScrollingNode::setCounterScrollingLayer(WebCore::GraphicsLayer*)' > ./.libs/libwebkitgtk-3.0.so: undefined reference to `WebCore::ScrollingStateNode::setScrollPlatformLayer(WebCore::TextureMapperPlatformLayer*)' > ./.libs/libwebkitgtk-3.0.so: undefined reference to `WebCore::ScrollingStateNode::platformScrollLayer() const' > ./.libs/libwebkitgtk-3.0.so: undefined reference to `WebCore::ScrollingStateStickyNode::create(WebCore::ScrollingStateTree*, unsigned long)' > collect2: error: ld returned 1 exit status > ./.libs/libwebkitgtk-3.0.so: undefined reference to `WebCore::ScrollingStateScrollingNode::setCounterScrollingLayer(WebCore::GraphicsLayer*)' > ./.libs/libwebkitgtk-3.0.so: undefined reference to `WebCore::ScrollingStateNode::setScrollPlatformLayer(WebCore::TextureMapperPlatformLayer*)' > ./.libs/libwebkitgtk-3.0.so: undefined reference to `WebCore::ScrollingStateNode::platformScrollLayer() const' > ./.libs/libwebkitgtk-3.0.so: undefined reference to `WebCore::ScrollingStateStickyNode::create(WebCore::ScrollingStateTree*, unsigned long)' > collect2: error: ld returned 1 exit status > ./.libs/libwebkitgtk-3.0.so: undefined reference to `WebCore::ScrollingStateScrollingNode::setCounterScrollingLayer(WebCore::GraphicsLayer*)' > ./.libs/libwebkitgtk-3.0.so: undefined reference to `WebCoremake[1]: ::ScrollingStateNode::setScrollPlatformLayer(WebCore::TextureMapperPlatformLayer*)' > ./.libs/libwebkitgtk-3.0.so: undefined reference to `WebCore::ScrollingStateNode::platformScrollLayer() const' > ./.libs/libwebkitgtk-3.0.so: *** [Programs/unittests/testapplicationcache] Error 1undefined > reference to `WebCore::ScrollingStateStickyNode::.create(WebCore::make[1]: ScrollingStateTree/**** Waiting for unfinished jobs...., . > unsignedlibs long/)make[1]: libwebkitgtk*** [Programs/unittests/testdomdomwindow] Error 1'- So it seems that we need an empty implementation for these methods or instead or removing the guards they should be #if ENABLE(THREADED_SCROLLING) || ENABLE(COORDINATED_GRAPHICS). I think extending the guards would have been a lot safer than forcing dead code into other ports.
I've created https://bugs.webkit.org/show_bug.cgi?id=114545 and attached a patch to add in #if ENABLE(THREADED_SCROLLING) guards. If someone would review/commit it, I'd be grateful.