Bug 78864 – Register fixed position layers with ScrollingCoordinator

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Sami Kyostila]

Elements with fixed position should be promoted to layers and registered with the ScrollCoordinator. This allows the ScrollCoordinator to keep fixed position layers stationary relative to their containing frame view while scrolling.

[Simon Fraser (smfr)]

This is already done; see RenderLayerCompositor::requiresCompositingForPosition(). Is there more you plan to do?

[Sami Kyostila]

Sorry, the description wasn't very accurate. The layer promotion part is done as you said, but what is missing is making sure ScrollingCoordinator is aware of the fixed position layers and what they are fixed to.

This is a bit preliminary and depends on how 78401 and 78739 turn out.

[James Robinson]

ScrollingCoordinator is informed of the existence of fixed position elements, but see this FIXME in ScrollingCoordinator.h:

    // Should be called whenever the fixed objects counter changes between zero and one.
    // FIXME: This is a temporary workaround so that we'll fall into main thread scrolling mode
    // if a page has fixed elements. The scrolling tree should know about the fixed elements
    // so it can make sure they stay fixed even when we scroll on the scrolling thread.
    void frameViewHasFixedObjectsDidChange(FrameView*);

Resolving that FIXME is what this bug is about.

[Sami Kyostila]

Created attachment 139266 [details]
Patch

[Gustavo Noronha (kov)]

Comment on attachment 139266 [details]
Patch

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

[Sami Kyostila]

Created attachment 139273 [details]
Patch

Fixed GTK build failure.

[Antonio Gomes]

nice work!

[Early Warning System Bot]

Comment on attachment 139273 [details]
Patch

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

[Build Bot]

Comment on attachment 139273 [details]
Patch

Attachment 139273 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12556353

[Early Warning System Bot]

Comment on attachment 139273 [details]
Patch

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

[Build Bot]

Comment on attachment 139273 [details]
Patch

Attachment 139273 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/12548358

[James Robinson]

Comment on attachment 139273 [details]
Patch

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

> Source/WebCore/page/scrolling/ScrollingCoordinator.cpp:335
> +    if (!m_page->settings()->acceleratedCompositingForFixedPositionEnabled())
> +        return frameView->hasFixedObjects();

i don't understand this check. the interface with ScrollingCoordinator should be the same no matter which way this setting is set - WebCore informs ScrollingCoordinator of the state of the world (this GraphicsLayer is fixed, this FrameView has non-composited fixed elements) and then the ScrollingCoordinator implementation figures out what it can do with that. If it can handle fixpos in the compositor, it does so, if not, it slowscrolls

> Source/WebCore/page/scrolling/ScrollingCoordinator.h:116
> +    void setLayerIsContainerForFixedPosition(GraphicsLayer*, bool);

don't you need to know which layer is the container for a given fixpos element?

[Sam Weinig]

My understanding of the design we were going for was that fixed position objects would get a new node in the ScrollingTree.  Anders, can you comment on that?

[James Robinson]

The way to accomplish that us to go through the ScrollingCoordinator, no?

[Sami Kyostila]

(In reply to comment #12)
> (From update of attachment 139273 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=139273&action=review
> 
> > Source/WebCore/page/scrolling/ScrollingCoordinator.cpp:335
> > +    if (!m_page->settings()->acceleratedCompositingForFixedPositionEnabled())
> > +        return frameView->hasFixedObjects();
> 
> i don't understand this check. the interface with ScrollingCoordinator should be the same no matter which way this setting is set - WebCore informs ScrollingCoordinator of the state of the world (this GraphicsLayer is fixed, this FrameView has non-composited fixed elements) and then the ScrollingCoordinator implementation figures out what it can do with that. If it can handle fixpos in the compositor, it does so, if not, it slowscrolls

I guess that depends on what acceleratedCompositingForFixedPositionEnabled really means. If it means the compositor knows how to keep layers fixed, then this check is redundant. OTOH if just means fixed position layers should be composited, then we need a new flag to enable fast scrolling with fixed position layers.

I'd prefer reusing the existing flag, unless some platforms are already using it to mean just the layer promotion.

> > Source/WebCore/page/scrolling/ScrollingCoordinator.h:116
> > +    void setLayerIsContainerForFixedPosition(GraphicsLayer*, bool);
> 
> don't you need to know which layer is the container for a given fixpos element?

The compositor will figure that out by walking up the layer tree. The first ancestor layer that is marked as a fixed position container will be used.

[Sami Kyostila]

Created attachment 142514 [details]
Patch

[Early Warning System Bot]

Comment on attachment 142514 [details]
Patch

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

[Early Warning System Bot]

Comment on attachment 142514 [details]
Patch

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

[Gustavo Noronha (kov)]

Comment on attachment 142514 [details]
Patch

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

[Build Bot]

Comment on attachment 142514 [details]
Patch

Attachment 142514 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/12725183

[Build Bot]

Comment on attachment 142514 [details]
Patch

Attachment 142514 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12723381

[Gyuyoung Kim]

Comment on attachment 142514 [details]
Patch

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

[Sami Kyostila]

Created attachment 145049 [details]
Patch

[Sami Kyostila]

Created attachment 145069 [details]
Patch

Fixed compilation problems.

[Sami Kyostila]

Anyone up for a review?

[Adam Barth]

Presumably jamesr or enne should review this patch.

[James Robinson]

Comment on attachment 145069 [details]
Patch

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

It appears as if this patch will break threaded scrolling on any page with a position:fixed element that happens to be composited (because of a transform, overlap, or anything else) - the ScrollingCoordinator will not correctly mark the view as needing updates on the main thread and the fixpos element will jitter like crazy.  I'm concerned about the level of testing that's gone on if this is the case.

> Source/WebCore/ChangeLog:17
> +        Note that composition of fixed position elements is currently behind a
> +        setting (Settings::acceleratedCompositingForFixedPositionEnabled), so
> +        this functionality is not enabled by default.
> +

This comment is wrong and I'm not sure why it is here at all.  Fixed position elements may or may not be composited completely independently of the acceleratedCompositingForFixedPositionEnabled Setting - that setting simply changes the compositing trigger so that some fixpos elements that otherwise wouldn't be composited are.  The concern for ScrollingCoordinator is whether the presence/absence of fixpos elements changes shouldScrollOnMainThread - that's something that the ScrollingCoordinator implementation needs to figure out in coordination (lol) with the compositor implementation it's talking to.  That isn't something controlled by Settings at all

> Source/WebCore/page/scrolling/ScrollingCoordinator.cpp:348
> +        RenderObject* o = *it;

Don't use one-letter variable names

> Source/WebCore/page/scrolling/ScrollingCoordinator.cpp:352
> +        RenderBox* box = toRenderBox(o);
> +        if (!box->hasLayer() || !box->layer()->backing())

I don't understand this downcast.  RenderBox doesn't expose hasLayer() or layer(), those are on RenderObject.  Perhaps you meant RenderBoxModelObject (since that's the root class of all ROs that could have layers)?  Even that would be weird - why downcast at all?

> Source/WebCore/page/scrolling/ScrollingCoordinator.cpp:367
> +        || hasNonLayerFixedObjects(frameView)

This check is insufficient to know if we should update the scroll layer position on the main thread - we also have to know if this compositor implementation knows how to handle fixpos on a non-main thread.  One way to do this would be to have a function to query this capability that has a default impl that returns false and let people provide other implementations in ScrollingCoordinator*.cpp as they gain that capability.

[Adam Barth]

@skyostil: It appears that the ball is in your court.  Feel free to re-add the AndroidHitList keyword if you're feeling blocked.

[Sami Kyostila]

Comment on attachment 145069 [details]
Patch

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

>It appears as if this patch will break threaded scrolling on any page with a position:fixed element that happens to be composited (because of a transform, overlap, or anything else) - the ScrollingCoordinator will not >correctly mark the view as needing updates on the main thread and the fixpos element will jitter like crazy.  I'm concerned about the level of testing that's gone on if this is the case.

That's actually not the case; the scrolling coordinator knows about all fixed position ROs on the page, and if any of them are not composited it will fall back to main thread scrolling. In other words, threaded scrolling is only enabled if we know for a fact that all fixed elements have been promoted to layers (or there are no fixed elements at all).

>> Source/WebCore/ChangeLog:17
>> +
> 
> This comment is wrong and I'm not sure why it is here at all.  Fixed position elements may or may not be composited completely independently of the acceleratedCompositingForFixedPositionEnabled Setting - that setting simply changes the compositing trigger so that some fixpos elements that otherwise wouldn't be composited are.  The concern for ScrollingCoordinator is whether the presence/absence of fixpos elements changes shouldScrollOnMainThread - that's something that the ScrollingCoordinator implementation needs to figure out in coordination (lol) with the compositor implementation it's talking to.  That isn't something controlled by Settings at all

Yeah, that's not accurate. What I was trying to say is that if you want to make all fixed position elements composited by default, you also need to flip that switch. Like you said, fixed position layers can also become composited via other means, so we may still end up in this code.

Since having all fixed position elements become composited isn't really related to the patch at hand, I'll get rid of this bit.

>> Source/WebCore/page/scrolling/ScrollingCoordinator.cpp:348
>> +        RenderObject* o = *it;
> 
> Don't use one-letter variable names

Done.

>> Source/WebCore/page/scrolling/ScrollingCoordinator.cpp:352
>> +        if (!box->hasLayer() || !box->layer()->backing())
> 
> I don't understand this downcast.  RenderBox doesn't expose hasLayer() or layer(), those are on RenderObject.  Perhaps you meant RenderBoxModelObject (since that's the root class of all ROs that could have layers)?  Even that would be weird - why downcast at all?

I guess I got tripped up by the RO hierarchy. I need to cast to a RenderBoxModelObject to check whether the fixed RO has a backed layer -- AFAIK I can't do that just by looking at RenderObject.

>> Source/WebCore/page/scrolling/ScrollingCoordinator.cpp:367
>> +        || hasNonLayerFixedObjects(frameView)
> 
> This check is insufficient to know if we should update the scroll layer position on the main thread - we also have to know if this compositor implementation knows how to handle fixpos on a non-main thread.  One way to do this would be to have a function to query this capability that has a default impl that returns false and let people provide other implementations in ScrollingCoordinator*.cpp as they gain that capability.

Right, this is the capability bit I was musing about in comment 15. I like your idea about asking this from the ScrollingCoordinator implementation.

[Sami Kyostila]

Created attachment 146296 [details]
Patch

- Added SC::supportsFixedPositionLayers() for gating fast fixed position scrolling

[Shawn Singh]

Comment on attachment 146296 [details]
Patch

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

> Source/WebCore/page/scrolling/chromium/ScrollingCoordinatorChromium.cpp:177
> +void ScrollingCoordinator::setLayerIsContainerForFixedPosition(GraphicsLayer* layer, bool enable)
> +{
> +    // FIXME: Implement!
> +}
> +
> +void ScrollingCoordinator::setLayerIsFixedToContainerLayerVisibleRect(GraphicsLayer* layer, bool enable)
> +{
> +    // FIXME: Implement!
> +}

Would it work if you added the plumbing on this patch, instead of on 70103?  For now I removed it from 70103.  As I understand, this way both patches work together but don't depend on each other explicitly, and nothing breaks if only one patch exists and not the other.

[Shawn Singh]

(In reply to comment #31)
> (From update of attachment 146296 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=146296&action=review
> 
> > Source/WebCore/page/scrolling/chromium/ScrollingCoordinatorChromium.cpp:177
> > +void ScrollingCoordinator::setLayerIsContainerForFixedPosition(GraphicsLayer* layer, bool enable)
> > +{
> > +    // FIXME: Implement!
> > +}
> > +
> > +void ScrollingCoordinator::setLayerIsFixedToContainerLayerVisibleRect(GraphicsLayer* layer, bool enable)
> > +{
> > +    // FIXME: Implement!
> > +}
> 
> Would it work if you added the plumbing on this patch, instead of on 70103?  For now I removed it from 70103.  As I understand, this way both patches work together but don't depend on each other explicitly, and nothing breaks if only one patch exists and not the other.

You know what, nevermind, I was being dumb.  I suppose whoever lands second will add the plumbing, or we can make the final connection in a separate patch =)

[Sami Kyostila]

(In reply to comment #32)
> You know what, nevermind, I was being dumb.  I suppose whoever lands second will add the plumbing, or we can make the final connection in a separate patch =)

:) Let's do a final plumbing patch to tie the two together.

[Sami Kyostila]

Created attachment 147081 [details]
Patch

- Added plumbing to tie this patch to 70103 now that it has landed.

[WebKit Review Bot]

Comment on attachment 147081 [details]
Patch

Attachment 147081 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12945738

New failing tests:
compositing/iframes/iframe-content-flipping.html

[WebKit Review Bot]

Created attachment 147204 [details]
Archive of layout-test-results from ec2-cr-linux-04

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-04  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[James Robinson]

Comment on attachment 147081 [details]
Patch

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

You need to change the value that ScrollingCoordinatorChromium returns for now and I have some naming concerns, but otherwise I think this looks pretty good.  Can you verify whether that layout test failure on cr-ews is really due to this patch?

> Source/WebCore/page/scrolling/ScrollingCoordinator.cpp:352
> +        if (!fixedObject->isBoxModelObject())
> +            return true;
> +        RenderBoxModelObject* fixedBoxModelObject = toRenderBoxModelObject(fixedObject);
> +        if (!fixedBoxModelObject->layer() || !fixedBoxModelObject->layer()->backing())

check hasLayer() before doing the downcast - you can call hasLayer() on a RenderObject - it doesn't even need a virtual call to do it!

> Source/WebCore/page/scrolling/ScrollingCoordinator.h:124
> +    void setLayerIsFixedToContainerLayerVisibleRect(GraphicsLayer*, bool);

I don't think "that ContainerLayerVisibleRect" makes any sense - what is LayerVisibleRect and why does it matter?  Why not just "setLayerIsFixedToContainer()"?

> Source/WebCore/page/scrolling/chromium/ScrollingCoordinatorChromium.cpp:166
> +    return true;

I think you need to say "false" here until we actually land the backend support for this. Otherwise, we'll break scrolling on pages with fixpos elements today.

[Shawn Singh]

(In reply to comment #37)
> 
> I think you need to say "false" here until we actually land the backend support for this. Otherwise, we'll break scrolling on pages with fixpos elements today.


Just curious - what other back-end stuff needs to be landed?   I will try to take that remaining stuff to completion.

Or, were you referring to https://bugs.webkit.org/show_bug.cgi?id=70103 which has already landed?

[James Robinson]

Oh, I didn't realize that had landed already.

[Sami Kyostila]

(In reply to comment #37)
> (From update of attachment 147081 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=147081&action=review
> 
> You need to change the value that ScrollingCoordinatorChromium returns for now and I have some naming concerns, but otherwise I think this looks pretty good.  Can you verify whether that layout test failure on cr-ews is really due to this patch?

The failure was because of the unconditional setNeedsCommit() in LayerChromium::setLayerIsFixedToContainerLayerVisibleRect().

> > Source/WebCore/page/scrolling/ScrollingCoordinator.cpp:352
> > +        if (!fixedObject->isBoxModelObject())
> > +            return true;
> > +        RenderBoxModelObject* fixedBoxModelObject = toRenderBoxModelObject(fixedObject);
> > +        if (!fixedBoxModelObject->layer() || !fixedBoxModelObject->layer()->backing())
> 
> check hasLayer() before doing the downcast - you can call hasLayer() on a RenderObject - it doesn't even need a virtual call to do it!

Ah, good point.
 
> > Source/WebCore/page/scrolling/ScrollingCoordinator.h:124
> > +    void setLayerIsFixedToContainerLayerVisibleRect(GraphicsLayer*, bool);
> 
> I don't think "that ContainerLayerVisibleRect" makes any sense - what is LayerVisibleRect and why does it matter?  Why not just "setLayerIsFixedToContainer()"?

Now that you pointed it out I'm not sure what exactly the name is referring to -- it was inherited from the Android version of this patch. I guess it might have been because fixed position elements generally (but not always) stay fixed relative to the visible layer rect of the containing clip layer's content layer. Because the container layer here is actually the clip layer and not the content layer, I think this is a misnomer.

I'll rename this to setLayerIsFixedToContainerLayer(), because the container still needs to be a layer.

[Sami Kyostila]

Created attachment 147557 [details]
Patch

- Renamed setLayerIsFixedToContainerLayerVisibleRect => setLayerIsFixedToContainerLayer.
- Fixed layout test failure. Also setNeedsCommit() when a layer's container bit changes, but only if there are fixed positioned descendants to avoid breaking the same test.
- Use hasLayer() instead of downcast + layer().

[James Robinson]

Comment on attachment 147557 [details]
Patch

OK! Let's go!

[WebKit Review Bot]

Comment on attachment 147557 [details]
Patch

Clearing flags on attachment: 147557

Committed r120340: <http://trac.webkit.org/changeset/120340>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Sam Weinig]

It seems pretty sucky you guys landed this the week we were really busy with out company wide conference.  This patch went in the wrong direction completely.  Fixed position objects are supposed to be represented by object in ScrollingTree, not as GraphicsLayers. 

Please roll this out ASAP.

[Sam Weinig]

Reopening as per the last comment.

[James Robinson]

The approach was documented here in February and the patch has been up for months, you had plenty of time to leave feedback before landing if you wanted to.

[Sam Weinig]

(In reply to comment #47)
> The approach was documented here in February and the patch has been up for months, you had plenty of time to leave feedback before landing if you wanted to.

I commented on April 29th that fixed position elements should be nodes in the ScrollingTree.  We also talked extensively about this at the contributors meeting.  I'm sorry I have not had time lately to keep an eye on this, but that is no reason to take our infrastructure and change it to suit your own designs.  So, once again, I would please request that you roll this and the associated changes out.

[Vangelis Kokkevis]

(In reply to comment #48)
> (In reply to comment #47)
> > The approach was documented here in February and the patch has been up for months, you had plenty of time to leave feedback before landing if you wanted to.
> 
> I commented on April 29th that fixed position elements should be nodes in the ScrollingTree.  We also talked extensively about this at the contributors meeting.  I'm sorry I have not had time lately to keep an eye on this, but that is no reason to take our infrastructure and change it to suit your own designs.  So, once again, I would please request that you roll this and the associated changes out.

Sam, let's try to fix whatever you think is broken here. Is there something specific in this code that won't work with your implementation? Can you please elaborate on what advantages adding fixed pos elements as nodes to the scrolling tree has over this design? 

We all have our deadlines and need to make forward progress. Before doing a blind revert, let's just figure out how to implement this feature so that it works for everyone.

[Sam Weinig]

(In reply to comment #49)
> (In reply to comment #48)
> > (In reply to comment #47)
> > > The approach was documented here in February and the patch has been up for months, you had plenty of time to leave feedback before landing if you wanted to.
> > 
> > I commented on April 29th that fixed position elements should be nodes in the ScrollingTree.  We also talked extensively about this at the contributors meeting.  I'm sorry I have not had time lately to keep an eye on this, but that is no reason to take our infrastructure and change it to suit your own designs.  So, once again, I would please request that you roll this and the associated changes out.
> 
> Sam, let's try to fix whatever you think is broken here. Is there something specific in this code that won't work with your implementation? Can you please elaborate on what advantages adding fixed pos elements as nodes to the scrolling tree has over this design? 
> 
> We all have our deadlines and need to make forward progress. Before doing a blind revert, let's just figure out how to implement this feature so that it works for everyone.

The motivation of modeling fixed position elements in the scrolling tree is that it allows us to keep the behavioral constructs away from GraphicsLayer, and keep GraphicsLayer sane (we discussed this at some length, I think you will remember, at the contributors meeting).  I think it is disingenuous to act as if I have not brought this up before, and if you disagreed with the approach, pushing your approach in without discussion seems like a hostile move.

[Simon Fraser (smfr)]

The FixedObjectSet on FrameView replicates functionality already present in positionedObjects(), as used by FrameView::scrollContentsFastPath(). However, I think we should change that to use m_fixedObjects (bug 90045).

[Vangelis Kokkevis]

(In reply to comment #50)
> (In reply to comment #49)
> > (In reply to comment #48)
> > > (In reply to comment #47)
> > > > The approach was documented here in February and the patch has been up for months, you had plenty of time to leave feedback before landing if you wanted to.
> > > 
> > > I commented on April 29th that fixed position elements should be nodes in the ScrollingTree.  We also talked extensively about this at the contributors meeting.  I'm sorry I have not had time lately to keep an eye on this, but that is no reason to take our infrastructure and change it to suit your own designs.  So, once again, I would please request that you roll this and the associated changes out.
> > 
> > Sam, let's try to fix whatever you think is broken here. Is there something specific in this code that won't work with your implementation? Can you please elaborate on what advantages adding fixed pos elements as nodes to the scrolling tree has over this design? 
> > 
> > We all have our deadlines and need to make forward progress. Before doing a blind revert, let's just figure out how to implement this feature so that it works for everyone.
> 
> The motivation of modeling fixed position elements in the scrolling tree is that it allows us to keep the behavioral constructs away from GraphicsLayer, and keep GraphicsLayer sane (we discussed this at some length, I think you will remember, at the contributors meeting).  I think it is disingenuous to act as if I have not brought this up before, and if you disagreed with the approach, pushing your approach in without discussion seems like a hostile move.

GraphicsLayer hasn't been modified at all in this patch. All the fixed position knowledge goes through the scrolling coordinator down to the platform layer. This is different from what I was suggesting in the contributors meeting which was to keep flags in the GraphicsLayer itself indicating its fixed or container status.  I do remember having that discussion in the contributor's meeting.

[James Robinson]

Could you describe what you would like to see, Sam?

[Sam Weinig]

(In reply to comment #52)
> (In reply to comment #50)
> > (In reply to comment #49)
> > > (In reply to comment #48)
> > > > (In reply to comment #47)
> > > > > The approach was documented here in February and the patch has been up for months, you had plenty of time to leave feedback before landing if you wanted to.
> > > > 
> > > > I commented on April 29th that fixed position elements should be nodes in the ScrollingTree.  We also talked extensively about this at the contributors meeting.  I'm sorry I have not had time lately to keep an eye on this, but that is no reason to take our infrastructure and change it to suit your own designs.  So, once again, I would please request that you roll this and the associated changes out.
> > > 
> > > Sam, let's try to fix whatever you think is broken here. Is there something specific in this code that won't work with your implementation? Can you please elaborate on what advantages adding fixed pos elements as nodes to the scrolling tree has over this design? 
> > > 
> > > We all have our deadlines and need to make forward progress. Before doing a blind revert, let's just figure out how to implement this feature so that it works for everyone.
> > 
> > The motivation of modeling fixed position elements in the scrolling tree is that it allows us to keep the behavioral constructs away from GraphicsLayer, and keep GraphicsLayer sane (we discussed this at some length, I think you will remember, at the contributors meeting).  I think it is disingenuous to act as if I have not brought this up before, and if you disagreed with the approach, pushing your approach in without discussion seems like a hostile move.
> 
> GraphicsLayer hasn't been modified at all in this patch. All the fixed position knowledge goes through the scrolling coordinator down to the platform layer. This is different from what I was suggesting in the contributors meeting which was to keep flags in the GraphicsLayer itself indicating its fixed or container status.  I do remember having that discussion in the contributor's meeting.

It seems like you effectively have, by modeling the fixed positionedness in your compositor, which is your platform implementation of GraphicsLayer.  I don't think it makes sense for the compositing layer to know about constructs like this, as it makes adding new ones (like position:sticky) exceedingly hard, as one would have to modify your compositor to make it work.  Instead the idea was to build on primitives. GraphicsLayers just being rendering buffers. And the ScrollingTree nodes representing their behaviors with respect to scrolling.  Then each platform could map from ScrollingTree nodes to GraphicsLayers (if they so chose) and get the behavior right.

If this were done right, one platform could implement position: sticky and it would work everywhere.

> Could you describe what you would like to see, Sam?

I would like to see the rendering code being used to drive construction of the ScrollingTree.

[James Robinson]

That's all fine and well, but on Mac (which is the only platform that can use ScrollingTree) you need a way to get the CALayer layers into the ScrollingTree in the first place to be manipulated.  That's done via ScrollingCoordinator calls, exactly as this patch has done them.

[Sam Weinig]

(In reply to comment #55)
> That's all fine and well, but on Mac (which is the only platform that can use ScrollingTree) you need a way to get the CALayer layers into the ScrollingTree in the first place to be manipulated.  That's done via ScrollingCoordinator calls, exactly as this patch has done them.

The intent is for all platforms to adopt the ScrollingTree.

[James Robinson]

As we discussed at the WebKit contributors meeting chromium cannot adopt the ScrollingTree as currently designed.  At least, we all told you that.  I'm not sure if you were listening or just talking.

[Sam Weinig]

(In reply to comment #57)
> As we discussed at the WebKit contributors meeting chromium cannot adopt the ScrollingTree as currently designed.  At least, we all told you that.  I'm not sure if you were listening or just talking.

Have you ever filed a bug about that (if you have, sorry I missed it)?  I remember you expressing concerns, but I didn't think that meant you were just going to give up on working on the problem as a community and go your own way.

[James Robinson]

About what, precisely?  For CoreAnimation, ScrollingTree seems perfectly reasonable.  I probably would use the same thing if I was using CoreAnimation in my port, but I'm not.  For ports that aren't using CoreAnimation (or something nearly identical to it) - i.e. Qt, Chromium, etc - ScrollingTree's design doesn't work.  That's perfectly fine, IMO, we can still collaborate on the integration points between WebCore rendering and graphics code and the compositor implementation (i.e. ScrollingCoordinator) and then hook it up to a different backend.

If you had intended ScrollingTree's design to be something that everyone could you I submit that you should have floated the design out first and asked for opinions rather than landing something and expecting everyone to use it.  I would have been quite happy to explain why it doesn't fit with what Chromium is doing. I spent quite a bit of time on that at the WebKit contributor's meeting and I thought that you may have been listening.

[Noam Rosenthal]

(In reply to comment #59)
> For ports that aren't using CoreAnimation (or something nearly identical to it) - i.e. Qt, Chromium, etc - ScrollingTree's design doesn't work.  

I tend to agree from the Qt perspective. ScrollingCoordinator makes perfect sense to me, but when I go into ScrollingTree it feels really tied to a threaded implementation and more specifically to CA. if I wanted to use it in Qt's cross-process implementation I'd have to bisect it pretty deeply, and create a new layer for proxying it via WebKit2. I'd end up with the same amount of platform specific glue code that I have now, without a clear benefit, and will have to add a fair amount of complexity to ScrollingTree just to support it.

I'd rather have access to all the necessary data (including the binding between a scroll-layer and a GraphicsLayer) in ScrollingCoordinator, or in any other creature that is not tied to a threaded implemenation, and use ScrollingTree for people who want to align with CA's threading model.

[Adam Barth]

At the risk of getting involved in a discussion I don't know much about, it appears that the ScrollingTree was landed in a series of patches with practically zero discussion.  The patches weren't even posted for review for more than a couple minutes each:

https://bugs.webkit.org/show_activity.cgi?id=77695 (4 minutes)
https://bugs.webkit.org/show_activity.cgi?id=77780 (3 minutes)
https://bugs.webkit.org/show_activity.cgi?id=77794 (8 minutes)
https://bugs.webkit.org/show_activity.cgi?id=77818 (6 minutes)
https://bugs.webkit.org/show_activity.cgi?id=77840 (9 minutes)
https://bugs.webkit.org/show_activity.cgi?id=78050 (10 minutes)
https://bugs.webkit.org/show_activity.cgi?id=78056 (3 minutes)
https://bugs.webkit.org/show_activity.cgi?id=78300 (5 minutes)
https://bugs.webkit.org/show_activity.cgi?id=78403 (4 minutes)

[Simon Fraser (smfr)]

(In reply to comment #59)
> About what, precisely?  For CoreAnimation, ScrollingTree seems perfectly reasonable.  I probably would use the same thing if I was using CoreAnimation in my port, but I'm not.  For ports that aren't using CoreAnimation (or something nearly identical to it) - i.e. Qt, Chromium, etc - ScrollingTree's design doesn't work.  That's perfectly fine, IMO, we can still collaborate on the integration points between WebCore rendering and graphics code and the compositor implementation (i.e. ScrollingCoordinator) and then hook it up to a different backend.

I'm curious about why the ScrollingTree implementation isn't a good fit for the Chromium and Qt models. The intent is that it allows you to create a representation of the scrolling behavior of parts of the page which is independent of rendering; this representation can be serialized for shipping off to another process, and/or accessed from other threads.

We don't have a tree yet, but indent to build one up with nodes for things like fixedpos, sticky and overflow scroll areas.

[James Robinson]

First, we don't really need yet another tree structure floating around.  We're maintaining a tree to do compositing and that seems quite sufficient for scrolling.  With CoreAnimation I understand you do not have direct access to the tree structure and thus need to maintain a parallel data structure.

Second, having a WebCore-managed thread handling input events and manipulating composited layers would be very difficult for us to manage.  We don't have a thread-safe public compositor API and don't have any immediate plans to build one (CoreAnimation has such a thing and I'm not surprised you are using it, but it's not trivial).  The input event handling / control flow / etc is quite sensitive and we need to take special care that latency-sensitive scrolling operations work with the rest of the chromium input system including when updates due to scrolling become visible on the WebCore main thread relative to other input events.  We've also found it absolutely essential for good animation performance that updates to the compositing tree be driven by the compositor's schedule and kept tightly in step with the rest of our graphics pipeline.  Lastly, the compositing tree itself will not necessarily always live in the same process as WebCore - Qt already syncs the compositing tree to another process and we've had some early discussions about doing something somewhat similar in Chromium.  We need to make sure that we aren't putting any unnecessary architectural barriers in the way of doing so.

None of this is to meant to criticize ScrollingTree - if you have CoreAnimation as the core of your compositing design, you probably end up with ScrollingTree or something like it.  We just don't have CoreAnimation.

[Noam Rosenthal]

> I'm curious about why the ScrollingTree implementation isn't a good fit for the Chromium and Qt models. The intent is that it allows you to create a representation of the scrolling behavior of parts of the page which is independent of rendering; this representation can be serialized for shipping off to another process, and/or accessed from other threads.
> 
> We don't have a tree yet, but indent to build one up with nodes for things like fixedpos, sticky and overflow scroll areas.

The main issue for the Qt port is that the threading model is baked in to the tree model. If there was some separation between the two, we would gladly join the party, whether the scrolling tree is the same as the GraphicsLayer tree or not, though given the choice I would go for the pragmatic approach of having only one tree.

If to take the GraphicsLayer abstraction as an analogy, ScrollingCoordinator is on the same level as renderLayerCompositor, and ScrollingTree is on the same level as PlatformCALayer, and there is nothing the GraphicsLayer level, where the abstraction is comprehensive enough to have all the features but generic enough to be agnostic of platform-specific detail.

Maybe if we add that in-between layer things would look a bit better?

[Sam Weinig]

(In reply to comment #64)
> > I'm curious about why the ScrollingTree implementation isn't a good fit for the Chromium and Qt models. The intent is that it allows you to create a representation of the scrolling behavior of parts of the page which is independent of rendering; this representation can be serialized for shipping off to another process, and/or accessed from other threads.
> > 
> > We don't have a tree yet, but indent to build one up with nodes for things like fixedpos, sticky and overflow scroll areas.
> 
> The main issue for the Qt port is that the threading model is baked in to the tree model. If there was some separation between the two, we would gladly join the party, whether the scrolling tree is the same as the GraphicsLayer tree or not, though given the choice I would go for the pragmatic approach of having only one tree.
> 
> If to take the GraphicsLayer abstraction as an analogy, ScrollingCoordinator is on the same level as renderLayerCompositor, and ScrollingTree is on the same level as PlatformCALayer, and there is nothing the GraphicsLayer level, where the abstraction is comprehensive enough to have all the features but generic enough to be agnostic of platform-specific detail.
> 
> Maybe if we add that in-between layer things would look a bit better?

In my view, the ScrollingTree should try and operate in terms of GraphicsLayers, and not CALayers. 

That said, for now, lets just try and keep the ScrollingCoordinators API something that we can share.  It is too bad we can't collaborate on a shared scrolling model and everyone will have to implement large aspects themselves.   That is deeply disappointing for the project.

[Shawn Singh]

Comment on attachment 147557 [details]
Patch

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

> Source/WebCore/rendering/RenderLayerBacking.cpp:446
> +                if (m_ancestorClippingLayer)
> +                    scrollingCoordinator->setLayerIsFixedToContainerLayer(m_ancestorClippingLayer.get(), false);
> +                scrollingCoordinator->setLayerIsFixedToContainerLayer(m_graphicsLayer.get(), false);

Hi Sami,   I just noticed this small snippet of code...   wouldnt these first 2 lines of code be a no-op because they'd get overridden by the 3rd line?  Did we mean to put an else statement here?

Looking at the chromium implementation of ScrollingCoordinator::setLayerIsFixedToContainerLayer() there is an additional condition there that may skip setting the container layer, but I wasn't sure how that plays with the logic in these 3 lines, and it really smells like we should have a more clear arrangement of if-statements and logic here?

[Sami Kyöstilä]

(In reply to comment #66)
> Hi Sami,   I just noticed this small snippet of code...   wouldnt these first 2 lines of code be a no-op because they'd get overridden by the 3rd line?  Did we mean to put an else statement here?
> 
> Looking at the chromium implementation of ScrollingCoordinator::setLayerIsFixedToContainerLayer() there is an additional condition there that may skip setting the container layer, but I wasn't sure how that plays with the logic in these 3 lines, and it really smells like we should have a more clear arrangement of if-statements and logic here?

Hey Shawn. I'm not sure I follow; these two calls to setLayerIsFixedToContainerLayer() clear the fixedness bit from both the graphics layer and a possible ancestor clipping layer. At this point in time we don't know whether the bit was originally set for the graphics layer or an ancestor clipping layer, so we clear it from both.

See the code above this that makes the layer returned by childForSuperlayers() fixed positioned -- that function returns either m_graphicsLayer or m_ancestorClippingLayer.