WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
78864
Register fixed position layers with ScrollingCoordinator
https://bugs.webkit.org/show_bug.cgi?id=78864
Summary
Register fixed position layers with ScrollingCoordinator
Sami Kyostila
Reported
2012-02-16 17:38:42 PST
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.
Attachments
Patch
(19.48 KB, patch)
2012-04-27 13:54 PDT
,
Sami Kyostila
no flags
Details
Formatted Diff
Diff
Patch
(19.57 KB, patch)
2012-04-27 14:10 PDT
,
Sami Kyostila
no flags
Details
Formatted Diff
Diff
Patch
(18.57 KB, patch)
2012-05-17 11:41 PDT
,
Sami Kyostila
no flags
Details
Formatted Diff
Diff
Patch
(19.63 KB, patch)
2012-05-31 04:29 PDT
,
Sami Kyostila
no flags
Details
Formatted Diff
Diff
Patch
(19.50 KB, patch)
2012-05-31 06:06 PDT
,
Sami Kyostila
no flags
Details
Formatted Diff
Diff
Patch
(20.20 KB, patch)
2012-06-07 08:43 PDT
,
Sami Kyostila
no flags
Details
Formatted Diff
Diff
Patch
(20.60 KB, patch)
2012-06-12 07:27 PDT
,
Sami Kyostila
no flags
Details
Formatted Diff
Diff
Archive of layout-test-results from ec2-cr-linux-04
(641.70 KB, application/zip)
2012-06-12 18:12 PDT
,
WebKit Review Bot
no flags
Details
Patch
(32.87 KB, patch)
2012-06-14 05:06 PDT
,
Sami Kyostila
no flags
Details
Formatted Diff
Diff
Show Obsolete
(7)
View All
Add attachment
proposed patch, testcase, etc.
Simon Fraser (smfr)
Comment 1
2012-02-17 09:49:34 PST
This is already done; see RenderLayerCompositor::requiresCompositingForPosition(). Is there more you plan to do?
Sami Kyostila
Comment 2
2012-02-17 10:30:20 PST
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
Comment 3
2012-02-17 17:57:59 PST
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
Comment 4
2012-04-27 13:54:13 PDT
Created
attachment 139266
[details]
Patch
Gustavo Noronha (kov)
Comment 5
2012-04-27 14:04:44 PDT
Comment on
attachment 139266
[details]
Patch
Attachment 139266
[details]
did not pass gtk-ews (gtk): Output:
http://queues.webkit.org/results/12550335
Sami Kyostila
Comment 6
2012-04-27 14:10:33 PDT
Created
attachment 139273
[details]
Patch Fixed GTK build failure.
Antonio Gomes
Comment 7
2012-04-27 14:34:24 PDT
nice work!
Early Warning System Bot
Comment 8
2012-04-27 14:38:00 PDT
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 9
2012-04-27 14:39:27 PDT
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 10
2012-04-27 14:44:14 PDT
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 11
2012-04-27 14:56:00 PDT
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 12
2012-04-27 16:26:14 PDT
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
Comment 13
2012-04-29 12:28:38 PDT
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
Comment 14
2012-04-30 09:41:42 PDT
The way to accomplish that us to go through the ScrollingCoordinator, no?
Sami Kyostila
Comment 15
2012-05-04 11:10:59 PDT
(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
Comment 16
2012-05-17 11:41:46 PDT
Created
attachment 142514
[details]
Patch
Early Warning System Bot
Comment 17
2012-05-17 11:48:57 PDT
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 18
2012-05-17 11:50:41 PDT
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 19
2012-05-17 11:51:35 PDT
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 20
2012-05-17 11:56:36 PDT
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 21
2012-05-17 12:04:31 PDT
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 22
2012-05-17 13:00:06 PDT
Comment on
attachment 142514
[details]
Patch
Attachment 142514
[details]
did not pass efl-ews (efl): Output:
http://queues.webkit.org/results/12716649
Sami Kyostila
Comment 23
2012-05-31 04:29:51 PDT
Created
attachment 145049
[details]
Patch
Sami Kyostila
Comment 24
2012-05-31 06:06:14 PDT
Created
attachment 145069
[details]
Patch Fixed compilation problems.
Sami Kyostila
Comment 25
2012-06-01 08:58:06 PDT
Anyone up for a review?
Adam Barth
Comment 26
2012-06-01 09:00:39 PDT
Presumably jamesr or enne should review this patch.
James Robinson
Comment 27
2012-06-01 16:32:58 PDT
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
Comment 28
2012-06-04 16:45:46 PDT
@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 29
2012-06-07 08:10:57 PDT
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
Comment 30
2012-06-07 08:43:18 PDT
Created
attachment 146296
[details]
Patch - Added SC::supportsFixedPositionLayers() for gating fast fixed position scrolling
Shawn Singh
Comment 31
2012-06-08 15:15:30 PDT
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
Comment 32
2012-06-08 15:16:28 PDT
(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
Comment 33
2012-06-11 06:25:36 PDT
(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
Comment 34
2012-06-12 07:27:47 PDT
Created
attachment 147081
[details]
Patch - Added plumbing to tie this patch to 70103 now that it has landed.
WebKit Review Bot
Comment 35
2012-06-12 18:12:37 PDT
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
Comment 36
2012-06-12 18:12:43 PDT
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 37
2012-06-13 14:43:34 PDT
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
Comment 38
2012-06-13 16:19:03 PDT
(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
Comment 39
2012-06-13 16:36:39 PDT
Oh, I didn't realize that had landed already.
Sami Kyostila
Comment 40
2012-06-14 04:03:08 PDT
(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
Comment 41
2012-06-14 05:06:54 PDT
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 42
2012-06-14 10:21:11 PDT
Comment on
attachment 147557
[details]
Patch OK! Let's go!
WebKit Review Bot
Comment 43
2012-06-14 10:45:23 PDT
Comment on
attachment 147557
[details]
Patch Clearing flags on attachment: 147557 Committed
r120340
: <
http://trac.webkit.org/changeset/120340
>
WebKit Review Bot
Comment 44
2012-06-14 10:45:34 PDT
All reviewed patches have been landed. Closing bug.
Sam Weinig
Comment 45
2012-06-24 17:26:32 PDT
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
Comment 46
2012-06-24 17:27:25 PDT
Reopening as per the last comment.
James Robinson
Comment 47
2012-06-24 17:56:50 PDT
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
Comment 48
2012-06-26 19:00:31 PDT
(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
Comment 49
2012-06-26 19:40:40 PDT
(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
Comment 50
2012-06-26 20:38:33 PDT
(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)
Comment 51
2012-06-26 22:56:07 PDT
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
Comment 52
2012-06-27 09:40:47 PDT
(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
Comment 53
2012-06-27 10:07:28 PDT
Could you describe what you would like to see, Sam?
Sam Weinig
Comment 54
2012-06-27 11:28:40 PDT
(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
Comment 55
2012-06-27 11:46:10 PDT
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
Comment 56
2012-06-27 17:24:33 PDT
(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
Comment 57
2012-06-27 17:27:59 PDT
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
Comment 58
2012-06-27 17:57:44 PDT
(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
Comment 59
2012-06-27 18:06:25 PDT
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
Comment 60
2012-06-27 18:19:08 PDT
(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
Comment 61
2012-06-27 18:19:52 PDT
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)
Comment 62
2012-06-27 19:05:14 PDT
(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
Comment 63
2012-06-27 19:42:11 PDT
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
Comment 64
2012-06-27 21:25:33 PDT
> 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
Comment 65
2012-07-05 11:44:54 PDT
(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 66
2012-09-10 17:36:55 PDT
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ä
Comment 67
2012-09-12 06:29:42 PDT
(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.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug