Simplify logic for automatically opting into composited scrolling

• Component: WebCore Misc.
• Status: RESOLVED WONTFIX

The current method uses an understanding of the stacking order to predict if a promotion will affect it. This is unfortunate for a couple of reasons. Stacking is complex, and it's difficult to guarantee correctness. Also, if the 'real' stacking logic changes the opt-in logic will break, so the current approach is brittle.

A much simpler approach would be to just do a test promotion and check if the stacking order changed. This increases the complexity of the function from O(n) per stacking context (where n is the number of layers visited during RenderLayer::CollectLayers) to O(n*m) (where m is the number of layers in the stacking context that could be potentially promoted), but it would be a huge win in terms of code complexity and maintainability. This increase in complexity may not be a concern in practice, though. We'll have to do some measurements.

I should mention that this is an alternative to https://bugs.webkit.org/show_bug.cgi?id=109236. If this approach works, 109236 will be unnecessary.

[Glenn Hartmann]

Created attachment 188599 [details]
Patch

[WebKit Review Bot]

Comment on attachment 188599 [details]
Patch

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

New failing tests:
platform/chromium/virtual/softwarecompositing/overflow/dynamic-composited-scrolling-status.html
platform/chromium/virtual/gpu/compositedscrolling/overflow/dynamic-composited-scrolling-status.html
platform/chromium/virtual/softwarecompositing/overflow/textarea-scroll-touch.html
compositing/overflow/textarea-scroll-touch.html
compositing/overflow/dynamic-composited-scrolling-status.html
platform/chromium/virtual/gpu/compositedscrolling/overflow/textarea-scroll-touch.html
platform/chromium/virtual/gpu/compositedscrolling/scrollbars/overflow-scrollbar-combinations.html

[WebKit Review Bot]

Comment on attachment 188599 [details]
Patch

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

New failing tests:
platform/chromium/virtual/softwarecompositing/overflow/dynamic-composited-scrolling-status.html
platform/chromium/virtual/gpu/compositedscrolling/overflow/dynamic-composited-scrolling-status.html
platform/chromium/virtual/softwarecompositing/overflow/textarea-scroll-touch.html
compositing/overflow/textarea-scroll-touch.html
compositing/overflow/dynamic-composited-scrolling-status.html
platform/chromium/virtual/gpu/compositedscrolling/overflow/textarea-scroll-touch.html
platform/chromium/virtual/gpu/compositedscrolling/scrollbars/overflow-scrollbar-combinations.html

[Glenn Hartmann]

Created attachment 188637 [details]
Patch

Updating layout test expectations, will rebaseline in https://bugs.webkit.org/show_bug.cgi?id=109965.

[Simon Fraser (smfr)]

Comment on attachment 188637 [details]
Patch

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

> Source/WebCore/rendering/RenderLayer.cpp:5298
> -    bool shouldUpdateDescendantsAreContiguousInStackingOrder = isStackingContext() && (m_zOrderListsDirty || m_normalFlowListDirty);
> +    bool shouldUpdateCanBeStackingContainer = isStackingContext() && (m_zOrderListsDirty || m_normalFlowListDirty);

Only composited scrolling uses the stacking container logic. You need to avoid all the work to update the flag when the layer is not a candidate for composited scrolling, otherwise you'll slow everything down.

[Glenn Hartmann]

Created attachment 189137 [details]
Patch

Added a check for acceleratedCompositingForOverflowScrollEnabled(). I have another bug (https://bugs.webkit.org/show_bug.cgi?id=109966) that I'm working on which will reduce this further to only perform the full computation for overflow-scroll layers. I thought it was enough to make a separate patch out of it. Should I just wait and upload them together?

[Simon Fraser (smfr)]

(In reply to comment #7)
> Created an attachment (id=189137) [details]
> Patch
> 
> Added a check for acceleratedCompositingForOverflowScrollEnabled(). I have another bug (https://bugs.webkit.org/show_bug.cgi?id=109966) that I'm working on which will reduce this further to only perform the full computation for overflow-scroll layers. I thought it was enough to make a separate patch out of it. Should I just wait and upload them together?

I think you should do the other one first.

[Julien Chaffraix]

Comment on attachment 189137 [details]
Patch

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

I have a hard time saying how it's a simplification when the new code end up being *longer* than the original one, seems to double the amount of zorder list we use  and uses a lot of  boolean parameters.

> Source/WebCore/rendering/RenderLayer.cpp:-601
> -//  And we would conclude that C could be promoted.

Removing this comment is not something I am supportive of.

> Source/WebCore/rendering/RenderLayer.cpp:528
> +    return layer->isRootLayer() || layerRenderer->isPositioned() || layer->hasTransform();

Looks like you should be using RenderObject::canContainFixedPositionedObject here as this should handle more cases than that (at least, flow-thread and foreign-object**).

** foreign-object is not actually reachable from this code as SVG objects don't have RenderLayer.

> Source/WebCore/rendering/RenderLayer.cpp:563
> +    // TODO ([e-mail]): We can early-out of this function more

We don't put TODO in WebKit, only FIXMEs without an owner.

> Source/WebCore/rendering/RenderLayer.cpp:581
> +    // We can't use TemporaryChange<> here since m_needsCompositedScrolling and
> +    // m_isNormalFlowOnly are both bitfields, so we have to do it the
> +    // old-fashioned way.
> +    bool oldNeedsCompositedScrolling = m_needsCompositedScrolling;
> +    bool oldIsNormalFlowOnly = m_isNormalFlowOnly;

I don't think I understand your comment, are you saying that because *both* are bitfield you can't use TemporaryChange or just because the type is boolean?

> Source/WebCore/rendering/RenderLayer.cpp:688
> +// call updateCanBeStackingContainer for all descendants of
> +// stacking context "ancestorStackingContext"

Seems like a pretty useless comment.

It also doesn't really match the method name: updateCan*Children*BeStackingContainers, which calls updateCanBeStackingContainer. There is a mismatch between self / children that makes it difficult to read and probably justify the above comment. Suggestion for the naming: updateCanBeStackingContainerRecursively.

> Source/WebCore/rendering/RenderLayer.cpp:1952
> +        updateIsNormalFlowOnly();

I don't understand the need for this. You are concerned about the z-order lists and whether you are a stacking context.

Also this is deeply incorrect: if your normal-flow bit was wrong, you already called updateSelfPaintingLayer() 5 lines above which relies on this information. You will get away when you are promoted as needsCompositedScrolling() force the layer to be self-painting, not so much when you are demoted.

> Source/WebCore/rendering/RenderLayer.h:802
> +    void updateCanChildrenBeStackingContainers(RenderLayer*, bool);

An API taking a bool with *no* explanation about what this boolean is supposed to do.

Please refrain yourself from adding boolean parameters and use enum instead for readability.

(In reply to comment #9)
> (From update of attachment 189137 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=189137&action=review
> 
> I have a hard time saying how it's a simplification when the new code end up being *longer* than the original one, seems to double the amount of zorder list we use  and uses a lot of  boolean parameters.

The new code certainly has some complexity, but comparing to the old code is a bit apples-to-oranges. This patch introduces a few correctness fixes in addition to trying a new approach. The hope was that comparing the pre/post promotion paint order lists would be conceptually simpler than the previous index based approach, but perhaps it isn't. I've updated that approach so that it's correct. PTAL, if you can spare the time: https://bugs.webkit.org/show_bug.cgi?id=109236. The newest patch has the same layout tests as this patch does. Is that approach easier to reason about?

[Glenn Hartmann]

Created attachment 190587 [details]
Patch

(In reply to comment #9)
> (From update of attachment 189137 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=189137&action=review

> > Source/WebCore/rendering/RenderLayer.cpp:-601
> > -//  And we would conclude that C could be promoted.
> 
> Removing this comment is not something I am supportive of.

That comment isn't really relevant anymore. It discusses specific details about the old implementation, which no longer applies to this patch. If you'd like, I can add a more thorough description of the new implementation.


> > Source/WebCore/rendering/RenderLayer.cpp:528
> > +    return layer->isRootLayer() || layerRenderer->isPositioned() || layer->hasTransform();
> 
> Looks like you should be using RenderObject::canContainFixedPositionedObject here as this should handle more cases than that (at least, flow-thread and foreign-object**).
> 
> ** foreign-object is not actually reachable from this code as SVG objects don't have RenderLayer.

I actually didn't write this function, just moved it further up in the file. Not to say that it isn't a valid concern, but maybe it could be addressed in a separate bug?


> > Source/WebCore/rendering/RenderLayer.cpp:563
> > +    // TODO ([e-mail]): We can early-out of this function more
> 
> We don't put TODO in WebKit, only FIXMEs without an owner.

done.


> > Source/WebCore/rendering/RenderLayer.cpp:581
> > +    // We can't use TemporaryChange<> here since m_needsCompositedScrolling and
> > +    // m_isNormalFlowOnly are both bitfields, so we have to do it the
> > +    // old-fashioned way.
> > +    bool oldNeedsCompositedScrolling = m_needsCompositedScrolling;
> > +    bool oldIsNormalFlowOnly = m_isNormalFlowOnly;
> 
> I don't think I understand your comment, are you saying that because *both* are bitfield you can't use TemporaryChange or just because the type is boolean?

TemporaryChange<bool> takes in a reference to a bool. Since m_needsCompositedScrolling and m_isNormalFlowOnly are bitfield values, attempting to pass either of them in to TemporaryChange<bool> as bool& will cause a compile error.


> > Source/WebCore/rendering/RenderLayer.cpp:688
> > +// call updateCanBeStackingContainer for all descendants of
> > +// stacking context "ancestorStackingContext"
> 
> Seems like a pretty useless comment.

removed.


> It also doesn't really match the method name: updateCan*Children*BeStackingContainers, which calls updateCanBeStackingContainer. There is a mismatch between self / children that makes it difficult to read and probably justify the above comment. Suggestion for the naming: updateCanBeStackingContainerRecursively.

done.


> > Source/WebCore/rendering/RenderLayer.cpp:1952
> > +        updateIsNormalFlowOnly();
> 
> I don't understand the need for this. You are concerned about the z-order lists and whether you are a stacking context.

It's true that updateCompositedScrolling() doesn't care about normal flow information, but it could affect the normal-flow-only status of a layer if it decides to opt in (or opt-out) since shouldBeNormalFlowOnly() makes direct use of needsCompositedScrolling(). If we don't update the value here, isNormalFlowOnly() may continue to return stale values.


> Also this is deeply incorrect: if your normal-flow bit was wrong, you already called updateSelfPaintingLayer() 5 lines above which relies on this information. You will get away when you are promoted as needsCompositedScrolling() force the layer to be self-painting, not so much when you are demoted.

I didn't realize that, thanks for pointing it out. I've moved the call to updateIsNormalFlowOnly() to above the updateSelfPaintingLayer() call. This doesn't affect the rest of the behaviour of this patch.


> > Source/WebCore/rendering/RenderLayer.h:802
> > +    void updateCanChildrenBeStackingContainers(RenderLayer*, bool);
> 
> An API taking a bool with *no* explanation about what this boolean is supposed to do.
> 
> Please refrain yourself from adding boolean parameters and use enum instead for readability.

I found a way around having a bool parameter at all.

[WebKit Review Bot]

Comment on attachment 190587 [details]
Patch

Attachment 190587 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://webkit-commit-queue.appspot.com/results/16758240

New failing tests:
html5lib/generated/run-tests16-data.html

[Shawn Singh]

Comment on attachment 190587 [details]
Patch

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

> Source/WebCore/ChangeLog:12
> +        change in stacking order. This should be more intuitive and more

I think it would be helpful to add one more sentence just before the last sentence, explaining what the code does after this comparison.  something like "if the two lists are in fact the same, then we know it is safe to promote the layer to be a stacking container."

> Source/WebCore/ChangeLog:30
> +        (WebCore::RenderLayer::RenderLayer):
> +        (WebCore::isPositionedContainer):
> +        (WebCore):
> +        (WebCore::getStackingOrderElementAt):
> +        (WebCore::RenderLayer::updateCanBeStackingContainer):
> +        (WebCore::RenderLayer::updateCanChildrenBeStackingContainers):
> +        (WebCore::RenderLayer::canBeStackingContainer):
> +        (WebCore::RenderLayer::updateIsNormalFlowOnly):
> +        (WebCore::RenderLayer::updateNeedsCompositedScrolling):
> +        (WebCore::RenderLayer::updateLayerListsIfNeeded):
> +        (WebCore::RenderLayer::updateStackingContextsAfterStyleChange):
> +        (WebCore::RenderLayer::styleChanged):

Especially for a CL of this size (but also in general when not trivial), WebKit folks appreciate having some brief text to explain all the changes associated with each item on this list.  You can look through changelogs to see examples of where patches have done this.  I think that wil also help whoever the final reviewer will be.

> Source/WebCore/rendering/RenderLayer.cpp:155
> +    , m_canBePromotedToStackingcontainer(false)

let's use capital C on "container"

> Source/WebCore/rendering/RenderLayer.cpp:540
> +static const RenderLayer* getStackingOrderElementAt(const Vector<RenderLayer*>* posZOrderList, const Vector<RenderLayer*>* negZOrderList, const bool fromLeft, const size_t index)

This type of bool is definitely a place where enum makes more sense.  On the other hand, this is a minor trivial private helper function, so I don't know what actual reviewers would want.  I would still vote for enum, declared privately immediately above here.

> Source/WebCore/rendering/RenderLayer.cpp:563
> +// two sets of lists.

I don't know where, perhaps this comment is a good place, but I feel like the code needs an example in comments, that illustrates why this heavy-handed robust approach is being taken.  i.e. the example you and Ian have told me 2-3 times now and I keep forgetting that shows how stacking order can break in ways that are tricky and error-prone to catch otherwise, that motivates the approach in this function.

> Source/WebCore/rendering/RenderLayer.cpp:564
> +void RenderLayer::updateCanBeStackingContainer(RenderLayer* ancestorStackingContext)

What if we have an ancestorStackingContainer before an ancestorStackingContext?  Wouldn't we need to use the ancestorStackingContainer instead?

> Source/WebCore/rendering/RenderLayer.cpp:619
> +    // Insert this into the posZOrderistBeforePromote directly after the
> +    // positioned ancestor, if there is one. Otherwise, add it to the very
> +    // beginning.

I think I'm misunderstanding something, why do we need to insert "this" into the before list?  I can take a look again after hearing your explanation =)

> Source/WebCore/rendering/RenderLayer.cpp:648
> +    // Scan over the lists twice - the first time from left-to-right and the
> +    // second from right-to-left - each time stopping when we hit this in
> +    // the after-promote list. We check along the way for any inconsistencies
> +    // between the two lists that could cause a change in paint order if we
> +    // promote.
> +    for (int pass = 0; pass < 2; pass++) {

Are we certain that we don't need to check the middle part of the layer lists here, too?  i.e. the layers that get shifted from the ancestor container to the current layer... do we care/know for certain that the ordering of those layers will not change?

I think the comment here is explaining the implementation, which is OK, but it might help better to explain the concept.  For example something like "The layer lists can conceptually be divided into 3 parts:  (a) before the new stacking container, (b) part of the new stacking container, and (c) after the new stacking container.  We actually only need to check (a) and (c), because <insert reason here>.  To do this, we take two passes over the lists, one from the beginning, the other from the end.

> Source/WebCore/rendering/RenderLayer.cpp:699
> +        updateCanBeStackingContainer(ancestorStackingContext);

See one of my questions above - if we do need to use ancestorStackingContainers instead, I guess it would be a few lines additional code here that potentially changes the RenderLayer that's passed down the recursion.  Does that seem right?

> Source/WebCore/rendering/RenderLayer.cpp:1987
> +        updateIsNormalFlowOnly();

Looking at the code before applying this patch, it looks like something similar to this would have been necessary for correctness in the first place, no?  Otherwise m_isNormalFlowOnly would not have been updated when needsCompositedScrolling changed.  Was the previous code correct, by somehow calling styleChanged()?  If so, what is the new case that requires adding this explicitly?

> Source/WebCore/rendering/RenderLayer.cpp:5391
> +    bool shouldUpdateCanBeStackingContainer = acceleratedCompositingForOverflowScrollEnabled() && isStackingContext() && (m_zOrderListsDirty || m_normalFlowListDirty);

Again, related to my previous question about ancestorStackingContext vs ancestorStackingContainer.   If we did need to use ancestorStackingContainer, we should probably be saying (isStackingContext() || isStackingContainer()) here, right?

[Shawn Singh]

Quick summary of feedback in Comment #13 -  
 a bunch of nits
 2-3 requests for clarification
 one primary question about ancestorStackingContext vs ancestorStackingContainer.  I wouldn't be surprised if you already thought of this and just let us know why it's actually correct this way.

Nice work Glenn =)

[Glenn Hartmann]

Created attachment 190760 [details]
Patch

(In reply to comment #13)
> (From update of attachment 190587 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=190587&action=review
> 
> > Source/WebCore/ChangeLog:12
> > +        change in stacking order. This should be more intuitive and more
> 
> I think it would be helpful to add one more sentence just before the last sentence, explaining what the code does after this comparison.  something like "if the two lists are in fact the same, then we know it is safe to promote the layer to be a stacking container."

done.


> Especially for a CL of this size (but also in general when not trivial), WebKit folks appreciate having some brief text to explain all the changes associated with each item on this list.  You can look through changelogs to see examples of where patches have done this.  I think that wil also help whoever the final reviewer will be.

done.


> > Source/WebCore/rendering/RenderLayer.cpp:155
> > +    , m_canBePromotedToStackingcontainer(false)
> 
> let's use capital C on "container"

done.


> > Source/WebCore/rendering/RenderLayer.cpp:540
> > +static const RenderLayer* getStackingOrderElementAt(const Vector<RenderLayer*>* posZOrderList, const Vector<RenderLayer*>* negZOrderList, const bool fromLeft, const size_t index)
> 
> This type of bool is definitely a place where enum makes more sense.  On the other hand, this is a minor trivial private helper function, so I don't know what actual reviewers would want.  I would still vote for enum, declared privately immediately above here.

done.


> > Source/WebCore/rendering/RenderLayer.cpp:563
> > +// two sets of lists.
> 
> I don't know where, perhaps this comment is a good place, but I feel like the code needs an example in comments, that illustrates why this heavy-handed robust approach is being taken.  i.e. the example you and Ian have told me 2-3 times now and I keep forgetting that shows how stacking order can break in ways that are tricky and error-prone to catch otherwise, that motivates the approach in this function.

done.


> > Source/WebCore/rendering/RenderLayer.cpp:564
> > +void RenderLayer::updateCanBeStackingContainer(RenderLayer* ancestorStackingContext)
> 
> What if we have an ancestorStackingContainer before an ancestorStackingContext?  Wouldn't we need to use the ancestorStackingContainer instead?

Using stacking containers could cause problems here, since we're in the middle of figuring out what layers can (and can't) be stacking containers.

Here's an example Ian gave me of why this could be problematic:
Say we have a stacking context with a child that WAS a stacking container, and a grand child that wasn't. If we use the child as the ancestorStacking* for the grand child and then later it turns out the that child won't actually opt in (or will opt out), then we're in trouble.


> > Source/WebCore/rendering/RenderLayer.cpp:619
> > +    // Insert this into the posZOrderistBeforePromote directly after the
> > +    // positioned ancestor, if there is one. Otherwise, add it to the very
> > +    // beginning.
> 
> I think I'm misunderstanding something, why do we need to insert "this" into the before list?  I can take a look again after hearing your explanation =

If the layer isn't positioned, it won't appear in posZOrderListBeforePromote. Since we want to eventually compare the paint order between the pre-promote list and the post-promote list, it's very convenient to have "this" in posZOrderListBeforePromote in its correct paint-order position. It just simplifies the logic later when we're actually performing the two passes through both lists to ensure paint order hasn't changed. I've added a bit to the comment here, hopefully that helps clarify the reasoning.


> > Source/WebCore/rendering/RenderLayer.cpp:648
> > +    // Scan over the lists twice - the first time from left-to-right and the
> > +    // second from right-to-left - each time stopping when we hit this in
> > +    // the after-promote list. We check along the way for any inconsistencies
> > +    // between the two lists that could cause a change in paint order if we
> > +    // promote.
> > +    for (int pass = 0; pass < 2; pass++) {
> 
> Are we certain that we don't need to check the middle part of the layer lists here, too?  i.e. the layers that get shifted from the ancestor container to the current layer... do we care/know for certain that the ordering of those layers will not change?

Yes, the order of the children should not change with respect to each other because that order is determined by the tree order and z-order, which should be completely unaffected by promotion. The only possible paint-order change is when the children move with respect to their parent who is getting promoted, or if the children move with respect to the other non-children (ie, if the children are non-contiguous).


> I think the comment here is explaining the implementation, which is OK, but it might help better to explain the concept.  For example something like "The layer lists can conceptually be divided into 3 parts:  (a) before the new stacking container, (b) part of the new stacking container, and (c) after the new stacking container.  We actually only need to check (a) and (c), because <insert reason here>.  To do this, we take two passes over the lists, one from the beginning, the other from the end.

done.


> > Source/WebCore/rendering/RenderLayer.cpp:699
> > +        updateCanBeStackingContainer(ancestorStackingContext);
> 
> See one of my questions above - if we do need to use ancestorStackingContainers instead, I guess it would be a few lines additional code here that potentially changes the RenderLayer that's passed down the recursion.  Does that seem right?

Same answer as before. It wouldn't be hard to add in, but it could have correctness implications.


> > Source/WebCore/rendering/RenderLayer.cpp:1987
> > +        updateIsNormalFlowOnly();
> 
> Looking at the code before applying this patch, it looks like something similar to this would have been necessary for correctness in the first place, no?  Otherwise m_isNormalFlowOnly would not have been updated when needsCompositedScrolling changed.  Was the previous code correct, by somehow calling styleChanged()?  If so, what is the new case that requires adding this explicitly?

Yes, this really is necessary for correctness here, with or without the rest of this patch. It's just that we got lucky and never ran into the border cases before working on this patch.


> > Source/WebCore/rendering/RenderLayer.cpp:5391
> > +    bool shouldUpdateCanBeStackingContainer = acceleratedCompositingForOverflowScrollEnabled() && isStackingContext() && (m_zOrderListsDirty || m_normalFlowListDirty);
> 
> Again, related to my previous question about ancestorStackingContext vs ancestorStackingContainer.   If we did need to use ancestorStackingContainer, we should probably be saying (isStackingContext() || isStackingContainer()) here, right?

Same answer as above.

[Glenn Hartmann]

Created attachment 191233 [details]
Patch

[Julien Chaffraix]

Comment on attachment 191233 [details]
Patch

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

> Source/WebCore/rendering/RenderLayer.cpp:542
> +    RenderLayerModelObject* layerRenderer = layer->renderer();
> +    return layer->isRootLayer() || layerRenderer->isPositioned() || layer->hasTransform();

Please add a FIXME about this being not in sync with containingBlock (and yes, it can be handled in a follow-up bug).

> Source/WebCore/rendering/RenderLayer.cpp:545
> +enum StackingOrderDirection {FromRight, FromLeft};

I think we would need spaces before and after '{' and '}'.

> Source/WebCore/rendering/RenderLayer.cpp:549
> +// from left-to-right and right-to-left.

This is not a good use of left / right. You probably think about these directions as you read from left-to-right so you assume Vectors grow towards the "left" but that's arbitrary.

It's also completely unrelated to the actual walking you are doing, which is on the z-order lists so it's more appropriate to talk about background vs foreground walking. If you don't like to use the visual naming, { FromStartOfVector, FromEndOfVector }

> Source/WebCore/rendering/RenderLayer.cpp:609
> +    updateNormalFlowList();

Was the original ASSERT wrong? (AFAICT the rest of the calling code is the same so this shouldn't be needed, unless the ASSERT was indeed wrong).

> Source/WebCore/rendering/RenderLayer.cpp:611
> +    ASSERT(!m_normalFlowListDirty);

This ASSERT is trivially passing as you call updateNormalFlowList() above, did you remove the wrong one?

> Source/WebCore/rendering/RenderLayer.cpp:636
> +    // We can't use TemporaryChange<> here since m_needsCompositedScrolling and
> +    // m_isNormalFlowOnly are both bitfields, so we have to do it the
> +    // old-fashioned way.
> +    bool oldNeedsCompositedScrolling = m_needsCompositedScrolling;
> +    bool oldIsNormalFlowOnly = m_isNormalFlowOnly;
> +
> +    // Set the flag on the current layer, then rebuild ancestor stacking
> +    // context's lists. This way, we can see the exact effects that promoting
> +    // this layer would cause.
> +    m_isNormalFlowOnly = true;
> +    m_needsCompositedScrolling = false;
> +    ancestorStackingContext->rebuildZOrderLists(StopAtStackingContainers, posZOrderListBeforePromote, negZOrderListBeforePromote);
> +
> +    m_isNormalFlowOnly = false;
> +    m_needsCompositedScrolling = true;
> +    ancestorStackingContext->rebuildZOrderLists(StopAtStackingContainers, posZOrderListAfterPromote, negZOrderListAfterPromote);
> +
> +    m_needsCompositedScrolling = oldNeedsCompositedScrolling;
> +    m_isNormalFlowOnly = oldIsNormalFlowOnly;

This should be in a helper function to keep this code short, like collectBeforeAfterPromotionZOrderLists.

> Source/WebCore/rendering/RenderLayer.cpp:643
> +    bool hasBackground = renderer()->hasBackground();

We usually push these booleans when we need them (ie way below).

> Source/WebCore/rendering/RenderLayer.cpp:644
> +    bool weArePositioned = renderer()->isPositioned();

Why don't you just use isPositioned? Or rendererIsPositioned? (a layer isn't strictly positioned, its renderer is)

> Source/WebCore/rendering/RenderLayer.cpp:645
> +

Also I don't know / think the 2 booleans above buy us much as they are exactly used once.

> Source/WebCore/rendering/RenderLayer.cpp:745
> +    for (int pass = 0; pass < 2; pass++) {

The 'for' is super artificial and would probably be more readable if replaced by a helper function below.

> Source/WebCore/rendering/RenderLayer.cpp:794
> +        if (isStackingContext())
> +            return;

You check that this->isStackingContext() is true here and also in updateCanBeStackingContainer. I think we should pick a side and check once: the easiest is here and ASSERT in updateCanBeStackingContainer.

> Source/WebCore/rendering/RenderLayer.cpp:6023
> +        if (isStackingContext || isStackingContainer())

isStackingContext is checked inside isStackingContainer. Thus this should be written:

if (isStackingContainer())

> Source/WebCore/rendering/RenderLayer.cpp:6034
> +        if (isStackingContext || isStackingContainer())

Ditto.

[Glenn Hartmann]

Created attachment 191777 [details]
Patch

(In reply to comment #17)
> (From update of attachment 191233 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=191233&action=review
> 
> > Source/WebCore/rendering/RenderLayer.cpp:542
> > +    RenderLayerModelObject* layerRenderer = layer->renderer();
> > +    return layer->isRootLayer() || layerRenderer->isPositioned() || layer->hasTransform();
> 
> Please add a FIXME about this being not in sync with containingBlock (and yes, it can be handled in a follow-up bug).

done.


> > Source/WebCore/rendering/RenderLayer.cpp:545
> > +enum StackingOrderDirection {FromRight, FromLeft};
> 
> I think we would need spaces before and after '{' and '}'.

done.


> > Source/WebCore/rendering/RenderLayer.cpp:549
> > +// from left-to-right and right-to-left.
> 
> This is not a good use of left / right. You probably think about these directions as you read from left-to-right so you assume Vectors grow towards the "left" but that's arbitrary.
> 
> It's also completely unrelated to the actual walking you are doing, which is on the z-order lists so it's more appropriate to talk about background vs foreground walking. If you don't like to use the visual naming, { FromStartOfVector, FromEndOfVector }

done.


> > Source/WebCore/rendering/RenderLayer.cpp:609
> > +    updateNormalFlowList();
> 
> Was the original ASSERT wrong? (AFAICT the rest of the calling code is the same so this shouldn't be needed, unless the ASSERT was indeed wrong).
> 
> > Source/WebCore/rendering/RenderLayer.cpp:611
> > +    ASSERT(!m_normalFlowListDirty);
> 
> This ASSERT is trivially passing as you call updateNormalFlowList() above, did you remove the wrong one?

Fixed, the original ASSERT was correct, I put it back in and removed the call to updateNormalFlowList().


> > Source/WebCore/rendering/RenderLayer.cpp:636
> > +    // We can't use TemporaryChange<> here since m_needsCompositedScrolling and
> > +    // m_isNormalFlowOnly are both bitfields, so we have to do it the
> > +    // old-fashioned way.
> > +    bool oldNeedsCompositedScrolling = m_needsCompositedScrolling;
> > +    bool oldIsNormalFlowOnly = m_isNormalFlowOnly;
> > +
> > +    // Set the flag on the current layer, then rebuild ancestor stacking
> > +    // context's lists. This way, we can see the exact effects that promoting
> > +    // this layer would cause.
> > +    m_isNormalFlowOnly = true;
> > +    m_needsCompositedScrolling = false;
> > +    ancestorStackingContext->rebuildZOrderLists(StopAtStackingContainers, posZOrderListBeforePromote, negZOrderListBeforePromote);
> > +
> > +    m_isNormalFlowOnly = false;
> > +    m_needsCompositedScrolling = true;
> > +    ancestorStackingContext->rebuildZOrderLists(StopAtStackingContainers, posZOrderListAfterPromote, negZOrderListAfterPromote);
> > +
> > +    m_needsCompositedScrolling = oldNeedsCompositedScrolling;
> > +    m_isNormalFlowOnly = oldIsNormalFlowOnly;
> 
> This should be in a helper function to keep this code short, like collectBeforeAfterPromotionZOrderLists.

done.


> > Source/WebCore/rendering/RenderLayer.cpp:643
> > +    bool hasBackground = renderer()->hasBackground();
> 
> We usually push these booleans when we need them (ie way below).
> 
> > Source/WebCore/rendering/RenderLayer.cpp:644
> > +    bool weArePositioned = renderer()->isPositioned();
> 
> Why don't you just use isPositioned? Or rendererIsPositioned? (a layer isn't strictly positioned, its renderer is)
> 
> > Source/WebCore/rendering/RenderLayer.cpp:645
> > +
> 
> Also I don't know / think the 2 booleans above buy us much as they are exactly used once.

You're right, I took out both bools completely.


> > Source/WebCore/rendering/RenderLayer.cpp:745
> > +    for (int pass = 0; pass < 2; pass++) {
> 
> The 'for' is super artificial and would probably be more readable if replaced by a helper function below.

done.


> > Source/WebCore/rendering/RenderLayer.cpp:794
> > +        if (isStackingContext())
> > +            return;
> 
> You check that this->isStackingContext() is true here and also in updateCanBeStackingContainer. I think we should pick a side and check once: the easiest is here and ASSERT in updateCanBeStackingContainer.

done.


> > Source/WebCore/rendering/RenderLayer.cpp:6023
> > +        if (isStackingContext || isStackingContainer())
> 
> isStackingContext is checked inside isStackingContainer. Thus this should be written:
> 
> if (isStackingContainer())
> 
> > Source/WebCore/rendering/RenderLayer.cpp:6034
> > +        if (isStackingContext || isStackingContainer())
> 
> Ditto.

done both.

[Glenn Hartmann]

Created attachment 192274 [details]
Patch

> > > Source/WebCore/rendering/RenderLayer.cpp:609
> > > +    updateNormalFlowList();
> > 
> > Was the original ASSERT wrong? (AFAICT the rest of the calling code is the same so this shouldn't be needed, unless the ASSERT was indeed wrong).
> > 
> > > Source/WebCore/rendering/RenderLayer.cpp:611
> > > +    ASSERT(!m_normalFlowListDirty);
> > 
> > This ASSERT is trivially passing as you call updateNormalFlowList() above, did you remove the wrong one?
> 
> Fixed, the original ASSERT was correct, I put it back in and removed the call to updateNormalFlowList().

No, I was wrong. The diff there is misleading, as the old function is being called from stacking contexts, whereas the new one is being called from all the stacking contexts' children. The children do not necessarily have clean normal flow lists. However, this call to updateNormalFlowLists() seems to be outdated from a previous version of the patch. I've gone through the function and we don't actually use the normal flow lists for anything, we only need z-order lists. So I took out all the normalFlow stuff from the beginning of that function.

I also found and fixed an indexing bug in getStackingOrderElementAt() that tripped assertions in debug mode.

I updated updateCanBeStackingContainerRecursively, as well, to mirror the behavior of collectLayers().

Finally, I updated the code that builds our pre/post promotion paint order lists to never make use of the 'stacking container' concept (which depends on the promotion decision for other layers).

[Julien Chaffraix]

Comment on attachment 192274 [details]
Patch

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

This change cannot be safely reviewed as it's too massive. I have no clue if the testing is even adequate as I haven't had time to look at it (maybe it is, probably it isn't considering that there were some flaws in the logic in a previous patch). Glenn, I am asking that you split this patch, until then don't expect me to r+ it.

Some ideas: land the test independently (even failing), refactor the RenderLayer code to align it with your vision before adding the final logic.

> Source/WebCore/rendering/RenderLayer.cpp:543
> +    // FIXME: This may not be in sync with containingBlock.

It *is* not, you can check yourself.

> Source/WebCore/rendering/RenderLayer.cpp:560
> +    size_t negZOrderListSize = negZOrderList ? negZOrderList->size() : 0;

Nit: const size_t ...

On top of that, I don't think negZOrderList can be NULL here or empty (size() == 0) or else you would never got inside the for loop below.

> Source/WebCore/rendering/RenderLayer.cpp:569
> +    size_t posZOrderListSize = posZOrderList ? posZOrderList->size() : 0;

Ditto.

> Source/WebCore/rendering/RenderLayer.cpp:574
> +        return posZOrderList->at(posZOrderListSize - index - 1);
> +
> +    return negZOrderList->at(negZOrderListSize - (index - posZOrderListSize) - 1);

The previous comment explains also why these 2 lines are safe to execute (else you could end up passing (size_t) -1 with bad consequences).

> Source/WebCore/rendering/RenderLayer.cpp:800
> +            if (!m_reflection || reflectionLayer() != child)

This is equivalent to just reflectionLayer() != child (if m_reflection is NULL so will be reflectionLayer() and |child| cannot be NULL).

> LayoutTests/compositing/overflow/opt-into-composited-scrolling-positioned-ancestor.html:2
> +<!DOCTYPE html PUBLIC "-//W3C//DTD HTML 4.01 Transitional//EN"
> +   "http://www.w3.org/TR/html4/loose.dtd">

We use HTML doctype for new tests: <!DOCTYPE html>

> LayoutTests/compositing/overflow/opt-into-composited-scrolling-positioned-ancestor.html:4
> +<html lang="en">

No lang attribute.

> LayoutTests/compositing/overflow/opt-into-composited-scrolling-positioned-ancestor.html:7
> +  <meta http-equiv="Content-Type" content="text/html; charset=utf-8"/>
> +  <title>Opting into composited scrolling</title>

These are useless.

> LayoutTests/compositing/overflow/opt-into-composited-scrolling-positioned-ancestor.html:8
> +  <style type="text/css" media="screen">

Same for these attributes.

> LayoutTests/compositing/overflow/opt-into-composited-scrolling-positioned-ancestor.html:91
> +    } // function doTest

Useless comment.