Bug 114353 – [CoordinatedGraphics] Use ScrollingStateTree to handle fixed elements positioning while scrolling

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Mikhail Pozdnyakov]

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.

[Mikhail Pozdnyakov]

*** Bug 113932 has been marked as a duplicate of this bug. ***

[Mikhail Pozdnyakov]

Created attachment 197251 [details]
WIP

Qt build support is absent as well as change log, however the approach is clear.

[Early Warning System Bot]

Comment on attachment 197251 [details]
WIP

Attachment 197251 [details] did not pass qt-ews (qt):
Output: http://webkit-commit-queue.appspot.com/results/17746002

[EFL EWS Bot]

Comment on attachment 197251 [details]
WIP

Attachment 197251 [details] did not pass efl-ews (efl):
Output: http://webkit-commit-queue.appspot.com/results/17623022

[Early Warning System Bot]

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

[Mikhail Pozdnyakov]

(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 :/

[Mikhail Pozdnyakov]

Created attachment 197254 [details]
WIP 2

Check EWS.

[Mikhail Pozdnyakov]

Created attachment 197255 [details]
WIP 2

Check EWS.

[Early Warning System Bot]

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

[EFL EWS Bot]

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

[Early Warning System Bot]

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

[Mikhail Pozdnyakov]

Created attachment 197260 [details]
WIP 3

Added missing functions definitions, completely removed guards from ScrollingTreeState files.

[Mikhail Pozdnyakov]

Created attachment 197519 [details]
patch

[Jocelyn Turcotte]

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);

[Mikhail Pozdnyakov]

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"

[Mikhail Pozdnyakov]

Created attachment 197581 [details]
patch v2

removed ScrollingCoordinatorCoordinatedGraphics::frameViewRootLayerDidChange

[Noam Rosenthal]

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

[Mikhail Pozdnyakov]

Committed r148197: <http://trac.webkit.org/changeset/148197>

[Jessie Berlin]

(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)

[Ryosuke Niwa]

Lion build fix attempted in http://trac.webkit.org/changeset/148239.

[Simon Fraser (smfr)]

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?

[Anders Carlsson]

(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.

[Ryosuke Niwa]

Another Lion build fix attempted in http://trac.webkit.org/changeset/148252.

[Martin Robinson]

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'-

[Martin Robinson]

(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.

[Mark Salisbury]

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.