Bug 94743 – Automatically use composited scrolling

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

https://bugs.webkit.org/show_bug.cgi?id=91117 provides support for composited scrolling. We should automatically opt into this path where possible. That said, automatically opting in must be turned on via a setting.

Created attachment 160000 [details]
Patch

This patch is missing the settings work required to enable this feature as well
as the layout tests. Any feedback, though, would be greatly appreciated.

Created attachment 162782 [details]
Work in progress (still working on layout tests). Not ready for review

Created attachment 163503 [details]
Patch

[Early Warning System Bot]

Comment on attachment 163503 [details]
Patch

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

[Early Warning System Bot]

Comment on attachment 163503 [details]
Patch

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

[kov's GTK+ EWS bot]

Comment on attachment 163503 [details]
Patch

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

[Build Bot]

Comment on attachment 163503 [details]
Patch

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

[Gyuyoung Kim]

Comment on attachment 163503 [details]
Patch

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

[Build Bot]

Comment on attachment 163503 [details]
Patch

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

[WebKit Review Bot]

Comment on attachment 163503 [details]
Patch

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

New failing tests:
compositing/overflow/automatically-opt-into-composited-scrolling.html

Created attachment 163652 [details]
Attempt to make the bots happier

[Gyuyoung Kim]

Comment on attachment 163652 [details]
Attempt to make the bots happier

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

[Build Bot]

Comment on attachment 163652 [details]
Attempt to make the bots happier

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

Created attachment 163657 [details]
Attempt to make the bots happier

[Simon Fraser (smfr)]

Comment on attachment 163657 [details]
Attempt to make the bots happier

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

> Source/WebCore/rendering/RenderLayer.cpp:673
> +static bool isZIndexRespected(const RenderLayer* layer)
> +{
> +    return layer->isStackingContext();
> +}

This function is confusingly named. I would expect it to see whether the element is positioned. It seems that what you're really asking is whether the layer has non-auto z-index, so why not ask style directly?

> Source/WebCore/rendering/RenderLayer.cpp:679
> +    if (!layer)
> +        return 0;

Is this necessary?

> Source/WebCore/rendering/RenderLayer.cpp:696
> +    if (!layer->renderer()->isInline()
> +        && layer->isNormalFlowOnly()
> +        && !layer->renderer()->isPositioned())
> +        return -0.75;
> +
> +    if (!layer->renderer()->isPositioned()
> +        && layer->renderer()->isFloating())
> +        return -0.5;
> +
> +    if (layer->renderer()->isInline()
> +        && layer->isNormalFlowOnly()
> +        && !layer->renderer()->isPositioned())
> +        return -0.25;

A comment explaining these weird magic numbers seems justified.

> Source/WebCore/rendering/RenderLayer.cpp:701
> +static const RenderLayer* firstAppearingDescendant(const RenderLayer* currentLayer, const RenderLayer* firstLayer, const RenderLayer* secondLayer)

I don't know what "appearing" means in this context.

> Source/WebCore/rendering/RenderLayer.cpp:769
> +    double zMin = std::numeric_limits<double>::max();
> +    double zMax = -std::numeric_limits<double>::max();
> +    findMinAndMaxZIndices(this, zMin, zMax);
> +
> +    if (zMin >= zMax)
> +        return true;
> +
> +    return !nonDescendantInThisStackingContextCouldFallBetweenDescendants(stackingContext(), this, zMin, zMax);

This code seems very hard to understand, and I can't easily tell if it's correct.

What you're really asking is whether, by making a layer a stacking context, you affect the traversal order of the z-order tree. It seems like it would be easier to determine that from the z-order tree, rather than crawling around looking at zMin and zMax.

Created attachment 163913 [details]
Patch

> > Source/WebCore/rendering/RenderLayer.cpp:769
> > +    double zMin = std::numeric_limits<double>::max();
> > +    double zMax = -std::numeric_limits<double>::max();
> > +    findMinAndMaxZIndices(this, zMin, zMax);
> > +
> > +    if (zMin >= zMax)
> > +        return true;
> > +
> > +    return !nonDescendantInThisStackingContextCouldFallBetweenDescendants(stackingContext(), this, zMin, zMax);
>
> This code seems very hard to understand, and I can't easily tell if it's correct.
>
> What you're really asking is whether, by making a layer a stacking context, you affect the traversal order of the z-order tree. It seems like it would be easier to determine that from the z-order tree, rather than crawling around looking at zMin and zMax.

This is true. The current patch walks the z-order tree (and I agree that this is much easier to understand), but there's a down side: the z-order lists are usually dirty.
I was expecting this to happen occasionally, and that they'd sort themselves out eventually, but they seem to stay dirty.
Ideally, we could cache the lists when we rebuild them and clear the dirty bits, but ::usesCompositedScrolling
gets called in a deep stack of const functions, so the options I see are

  1) Recompute the lists each time they are needed.
  2) Remove a bunch of const's.
  3) Make the lists mutable.

None of these options seem very nice to me. Do you have a suggestion for how to proceed?

Created attachment 163915 [details]
Patch

Accidentally submitted some debugging code in the last patch.

[Simon Fraser (smfr)]

(In reply to comment #16)
> Created an attachment (id=163913) [details]
> Patch
> 
> > > Source/WebCore/rendering/RenderLayer.cpp:769
> > > +    double zMin = std::numeric_limits<double>::max();
> > > +    double zMax = -std::numeric_limits<double>::max();
> > > +    findMinAndMaxZIndices(this, zMin, zMax);
> > > +
> > > +    if (zMin >= zMax)
> > > +        return true;
> > > +
> > > +    return !nonDescendantInThisStackingContextCouldFallBetweenDescendants(stackingContext(), this, zMin, zMax);
> >
> > This code seems very hard to understand, and I can't easily tell if it's correct.
> >
> > What you're really asking is whether, by making a layer a stacking context, you affect the traversal order of the z-order tree. It seems like it would be easier to determine that from the z-order tree, rather than crawling around looking at zMin and zMax.
> 
> This is true. The current patch walks the z-order tree (and I agree that this is much easier to understand), but there's a down side: the z-order lists are usually dirty.
> I was expecting this to happen occasionally, and that they'd sort themselves out eventually, but they seem to stay dirty.

They are not dirty during compositing evaluation and painting. I'm not sure what makes you think they stay dirty.

> Ideally, we could cache the lists when we rebuild them and clear the dirty bits, but ::usesCompositedScrolling
> gets called in a deep stack of const functions, so the options I see are
> 
>   1) Recompute the lists each time they are needed.
>   2) Remove a bunch of const's.
>   3) Make the lists mutable.
> 
> None of these options seem very nice to me. Do you have a suggestion for how to proceed?

Can you build enough of a z-order subtree to make your determination? You'd have to back up to the stacking context ancestor, build temporary z-order lists with and without your layer being a stacking context, and compare them.

Maybe there is a way to do this without actually building lists, but perhaps it can share logic with the list building.

Created attachment 163925 [details]
Patch

(In reply to comment #18)
> (In reply to comment #16)
> > Created an attachment (id=163913) [details] [details]
> > Patch
> >
> > > > Source/WebCore/rendering/RenderLayer.cpp:769
> > > > +    double zMin = std::numeric_limits<double>::max();
> > > > +    double zMax = -std::numeric_limits<double>::max();
> > > > +    findMinAndMaxZIndices(this, zMin, zMax);
> > > > +
> > > > +    if (zMin >= zMax)
> > > > +        return true;
> > > > +
> > > > +    return !nonDescendantInThisStackingContextCouldFallBetweenDescendants(stackingContext(), this, zMin, zMax);
> > >
> > > This code seems very hard to understand, and I can't easily tell if it's correct.
> > >
> > > What you're really asking is whether, by making a layer a stacking context, you affect the traversal order of the z-order tree. It seems like it would be easier to determine that from the z-order tree, rather than crawling around looking at zMin and zMax.
> >
> > This is true. The current patch walks the z-order tree (and I agree that this is much easier to understand), but there's a down side: the z-order lists are usually dirty.
> > I was expecting this to happen occasionally, and that they'd sort themselves out eventually, but they seem to stay dirty.
>
> They are not dirty during compositing evaluation and painting. I'm not sure what makes you think they stay dirty.

It seems that this new code gets called at a point where these lists are dirty.
I keep checking m_zOrderListsDirty, and it seems to continue to be true.

>
> > Ideally, we could cache the lists when we rebuild them and clear the dirty bits, but ::usesCompositedScrolling
> > gets called in a deep stack of const functions, so the options I see are
> >
> >   1) Recompute the lists each time they are needed.
> >   2) Remove a bunch of const's.
> >   3) Make the lists mutable.
> >
> > None of these options seem very nice to me. Do you have a suggestion for how to proceed?
>
> Can you build enough of a z-order subtree to make your determination? You'd have to back up to the stacking context ancestor, build temporary z-order lists with and without your layer being a stacking context, and compare them.
>
> Maybe there is a way to do this without actually building lists, but perhaps it can share logic with the list building.

Yep, my patch should not build the whole z-order tree, but only those lists for the nodes in the tree that matter.
And I have refactored the list building code so that it is reused by the new code. Perhaps this approach is ok, then?

[Build Bot]

Comment on attachment 163925 [details]
Patch

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

[Antonio Gomes]

Comment on attachment 163925 [details]
Patch

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

> Source/WebCore/page/Settings.h:388
>  
> +        void setAcceleratedCompositingForOverflowScrollEnabled(bool enabled) { m_acceleratedCompositingForOverflowScrollEnabled = enabled; }
> +        bool acceleratedCompositingForOverflowScrollEnabled() const { return m_acceleratedCompositingForOverflowScrollEnabled; }
> +

this code has landed upstream already

[WebKit Review Bot]

Comment on attachment 163925 [details]
Patch

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

[Build Bot]

Comment on attachment 163925 [details]
Patch

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

Created attachment 163954 [details]
Rebasing

[Antonio Gomes]

Comment on attachment 163954 [details]
Rebasing

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

> Source/WebCore/ChangeLog:9
> +        wkb.ug/91117 if the overflow scroll div is styled with

nit: mention 'div' seems too specific. It can get applied to any block element.

> Source/WebCore/ChangeLog:14
> +

I think the most important thing to describe in the changelog here is the heuristic you used.

> Source/WebCore/rendering/RenderLayer.h:881
> +    // Returns true iff z ordering would not change if this layer were to establish a stacking context.

iff*

[Build Bot]

Comment on attachment 163954 [details]
Rebasing

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

Created attachment 164112 [details]
Attempt to make win and gtk bots happy

[Build Bot]

Comment on attachment 164112 [details]
Attempt to make win and gtk bots happy

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

[Build Bot]

Comment on attachment 164112 [details]
Attempt to make win and gtk bots happy

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

[kov's GTK+ EWS bot]

Comment on attachment 164112 [details]
Attempt to make win and gtk bots happy

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

[WebKit Review Bot]

Comment on attachment 164112 [details]
Attempt to make win and gtk bots happy

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

New failing tests:
compositing/overflow/automatically-opt-into-composited-scrolling.html

Created attachment 164385 [details]
Patch

[Build Bot]

Comment on attachment 164385 [details]
Patch

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

[WebKit Review Bot]

Comment on attachment 164385 [details]
Patch

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

New failing tests:
compositing/overflow/automatically-opt-into-composited-scrolling.html

Created attachment 164450 [details]
Patch

(In reply to comment #25)
> (From update of attachment 163954 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=163954&action=review
>
> > Source/WebCore/ChangeLog:9
> > +        wkb.ug/91117 if the overflow scroll div is styled with
>
> nit: mention 'div' seems too specific. It can get applied to any block element.
Fixed.
>
> > Source/WebCore/ChangeLog:14
> > +
>
> I think the most important thing to describe in the changelog here is the heuristic you used.
Done.
>
> > Source/WebCore/rendering/RenderLayer.h:881
> > +    // Returns true iff z ordering would not change if this layer were to establish a stacking context.
>
> iff*
Fixed.

[Build Bot]

Comment on attachment 164450 [details]
Patch

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

Created attachment 164476 [details]
Patch

(In reply to comment #37)
> Created an attachment (id=164476) [details]
> Patch

I'm sorry for all the noise trying to get this patch building on all the ews bots, but I think it's sorted now.

Simon, could you please give this another look when you have a chance?

[Sami Kyöstilä]

Comment on attachment 164476 [details]
Patch

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

> Source/WebCore/rendering/RenderLayer.cpp:678
> +void RenderLayer::countDescendantNonDescandantBoundariesForList(Vector<RenderLayer*>* layerList, const RenderLayer* toBePromoted, int& numBoundaries, bool& lastElementWasDescendant)

s/Descandant/Descendant/ (also elsewhere)

> Source/WebCore/rendering/RenderLayer.cpp:683
> +    for (size_t i = 0; i < layerList->size(); ++i)

Would it make sense to early-out here if numBoundaries > 2 (and call this function something else)?

> Source/WebCore/rendering/RenderLayer.cpp:1658
> +        && canSafelyEstablishAStackingContext();

I'm a bit worried that this function is either called too often, making it pretty expensive to walk the layers all the time (e.g., https://bugs.webkit.org/show_bug.cgi?id=96087 needs to call this during painting), or not often enough so that we miss some crucial style change in the subtree or do the calculation while the tree is in a bad state. I don't have anything concrete to back this up, but still I would sleep better with a push model where descendant layers notify their ancestors about things that affect the opt-in, or perhaps something that's part of the main layer tree construction algorithm.

> Source/WebCore/rendering/RenderLayer.cpp:1821
> +    if (compositor()->inCompositingMode() && usesCompositedScrolling())

Just wondering why we need to check for inCompositingMode(). Is this a bugfix for something else?

> Source/WebCore/rendering/RenderLayer.h:887
> +    // These two methods are used by canSafelyEstablishAStackingContext.

Could these two be static free functions instead of methods, or did I miss something?

> Source/WebCore/testing/Internals.cpp:1103
> +bool Internals::usesCompositedScrolling(Node* node, ExceptionCode& ec)

Instead of adding this helper you could also look at the output of layerTreeAsText() in the test and check that the necessary layers are there. That would have the added benefit of making sure the layer tree is correctly rebuilt after the style changes.

Created attachment 175811 [details]
Patch

Sami, thanks for this great review!

In addition to addressing your comments below, this patch contains one other noteworthy (but minor) change. In RenderLayerBacking, I've adjusted the size of the scrolling clip so that it doesn't contain the scrollbars unless they're overlay scrollbars.

(In reply to comment #39)
> (From update of attachment 164476 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=164476&action=review
>
> > Source/WebCore/rendering/RenderLayer.cpp:678
> > +void RenderLayer::countDescendantNonDescandantBoundariesForList(Vector<RenderLayer*>* layerList, const RenderLayer* toBePromoted, int& numBoundaries, bool& lastElementWasDescendant)
>
> s/Descandant/Descendant/ (also elsewhere)
Done.
>
> > Source/WebCore/rendering/RenderLayer.cpp:683
> > +    for (size_t i = 0; i < layerList->size(); ++i)
>
> Would it make sense to early-out here if numBoundaries > 2 (and call this function something else)?
I've early-out'd as you suggested, but I couldn't think of a better name, so I kept it and added a comment explaining that we can stop counting early. Could that work?
>
> > Source/WebCore/rendering/RenderLayer.cpp:1658
> > +        && canSafelyEstablishAStackingContext();
>
> I'm a bit worried that this function is either called too often, making it pretty expensive to walk the layers all the time (e.g., https://bugs.webkit.org/show_bug.cgi?id=96087 needs to call this during painting), or not often enough so that we miss some crucial style change in the subtree or do the calculation while the tree is in a bad state. I don't have anything concrete to back this up, but still I would sleep better with a push model where descendant layers notify their ancestors about things that affect the opt-in, or perhaps something that's part of the main layer tree construction algorithm.
Good points. I added some tracing to the CL to measure the cost of this check. It turned out we were recomputing the value a *lot*. Tracing showed that the performance was clearly unacceptable -- I was recomputing this value thousands of times a second. I've added some simple caching, and on the pages I was testing, we no longer recompute the value while scrolling.

To address your point about correctness, I now dirty the value (on the appropriate layers) whenever we dirty the zorder or normal flow lists, so it should be fresh when we need it.
>
> > Source/WebCore/rendering/RenderLayer.cpp:1821
> > +    if (compositor()->inCompositingMode() && usesCompositedScrolling())
>
> Just wondering why we need to check for inCompositingMode(). Is this a bugfix for something else?
Yep, it's a bug fix. The current code assumes that if a layer uses composited scrolling, it will actually get a composited scrolling layer. It turns out that this isn't necessarily true if we force compositing mode to be off, so I've added this extra check to be completely sure that we're compositing when we skip this repaint.
>
> > Source/WebCore/rendering/RenderLayer.h:887
> > +    // These two methods are used by canSafelyEstablishAStackingContext.
>
> Could these two be static free functions instead of methods, or did I miss something?
I added an explanatory comment -- the only reason that these are defined in the class is that they access private members on the render layers on which they operate. This seemed preferable to changing the visibility of things in RenderLayer.
>
> > Source/WebCore/testing/Internals.cpp:1103
> > +bool Internals::usesCompositedScrolling(Node* node, ExceptionCode& ec)
>
> Instead of adding this helper you could also look at the output of layerTreeAsText() in the test and check that the necessary layers are there. That would have the added benefit of making sure the layer tree is correctly rebuilt after the style changes.
This is another very good point. After making this change, it revealed that we weren't, in fact, rebuilding correctly after the style changes. The problem was that we needed to reevaluate compositing after layout when the tree is fully built. I've made it so that when we dirty this value, we request a reevaluation.

[Sami Kyöstilä]

Comment on attachment 175811 [details]
Patch

Thanks Ian. The test is looking pretty thorough now.

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

> Source/WebCore/ChangeLog:12
> +        antialiasing, so it is important that automatically opting in is only

Did you mean paint order instead of antialiasing?

> Source/WebCore/rendering/RenderLayer.cpp:127
> +#include "TraceEvent.h"

Not sure if we want to introduce trace events here since there aren't any so far, but I'll defer the decision on that to others. It definitely helps in measuring the overhead of doing this though.

> Source/WebCore/rendering/RenderLayer.cpp:486
> +void RenderLayer::dirtyDescendantsCanSafelyEstablishStackingContext(RenderLayer* layer)

Nit: dirtyDescendantsCanSafelyEstablishStackingContextStatus to match the existing naming.

> Source/WebCore/rendering/RenderLayer.cpp:1727
> +    bool automaticallyOptIn = renderer()->frame()->page()->settings()->acceleratedCompositingForOverflowScrollEnabled()

At least frame() and page() can be null here, so better check for that.

> Source/WebCore/rendering/RenderLayer.cpp:1731
> +    return renderer()->style()->useTouchOverflowScrolling() || automaticallyOptIn;

Nit: this could be the other way around so there's no need to consult the style when we've already opted in.

> Source/WebCore/rendering/RenderLayer.cpp:1901
> +    if (compositor()->inCompositingMode() && usesCompositedScrolling())

usesCompositedScrolling() is also called in a few other places to avoid repaints. Do we need this same inCompositingMode() check there too?

> Source/WebCore/rendering/RenderLayer.h:934
> +    // render layers they operate on.

Ah, I completely missed these were accessing private members before. Thanks for the explanation.

> Source/WebCore/rendering/RenderLayerBacking.cpp:740
> +        FloatSize clipSize = paddingBox.size();

Perhaps this should be a separate patch to keep things simpler? Could use a test too.

[Simon Fraser (smfr)]

Comment on attachment 175811 [details]
Patch

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

As far as I can see, you're only calling canSafelyEstablishAStackingContext() in usesCompositedScrolling() to determine whether it's ok to do so. What I don't see is the actually affecting the computed z-order lists. Am I missing that?

There are other cases where we'd like to be able to treat something like a stacking context when possible (e.g. something with overflow and composited descendants), so it would be good if there same code could be reused there. I think it would also be better if you could run this 'can be a stacking context' logic at z-order-list building time, which would I think allow the isDescendant() logic to avoid running up to the root on failure. Then the z-order lists for the layers would just reflect that this was made a stacking context.

Without pixel or ref tests I don't see how your tests show that the 'can be a stacking context' logic is correct.

>> Source/WebCore/ChangeLog:12
>> +        antialiasing, so it is important that automatically opting in is only
> 
> Did you mean paint order instead of antialiasing?

I presume by antialiasing he really means font smoothing.

> Source/WebCore/rendering/RenderLayer.cpp:492
> +    layer->compositor()->requestReevaluateCompositingAfterLayout();

I don't like it that things outside of RLC can tell it to do compositing-related stuff; this is the first instance of it.

Can you move this logic to inside of RLC somehow?

> Source/WebCore/rendering/RenderLayer.cpp:723
> +static bool isDescendant(const RenderLayer* parent, const RenderLayer* child)
> +{
> +    for (const RenderLayer* current = child; current; current = current->parent())
> +        if (current == parent)
> +            return true;
> +    return false;
> +}

In the worst case, this runs up to the root of the tree (which can be quite deep). Calling this during a tree walk can be O(n^2) on tree depth, which is bad and we try to avoid.

> Source/WebCore/rendering/RenderLayer.cpp:725
> +void RenderLayer::countDescendantNonDescendantBoundariesForList(Vector<RenderLayer*>* layerList, const RenderLayer* toBePromoted, int& numBoundaries, bool& lastElementWasDescendant)

This name is very confusing.

> Source/WebCore/rendering/RenderLayer.cpp:736
> +void RenderLayer::countDescendantNonDescendantBoundariesInZOrderTree(const RenderLayer* currentLayer, const RenderLayer* toBePromoted, int& numBoundaries, bool& lastElementWasDescendant)

Ditto.

> Source/WebCore/rendering/RenderLayer.cpp:785
> +#if PLATFORM(CHROMIUM)
> +    TRACE_EVENT0("webkit", "RenderLayer::canSafelyEstablishAStackingContext");
> +#endif

Why is Chromium the only platform that benefits from this?

>> Source/WebCore/rendering/RenderLayer.cpp:1727
>> +    bool automaticallyOptIn = renderer()->frame()->page()->settings()->acceleratedCompositingForOverflowScrollEnabled()
> 
> At least frame() and page() can be null here, so better check for that.

Rename automaticallyOptIn to alwaysUseCompositedScrolling or something.

> Source/WebCore/rendering/RenderLayer.cpp:4889
> +        for (RenderLayer* child = firstChild(); child; child = child->nextSibling())
> +            dirtyDescendantsCanSafelyEstablishStackingContext(child);

What impact does this have on performance?

> Source/WebCore/rendering/RenderLayer.cpp:4913
> +        for (RenderLayer* child = firstChild(); child; child = child->nextSibling())
> +            dirtyDescendantsCanSafelyEstablishStackingContext(child);

Ditto.

> Source/WebCore/rendering/RenderLayer.h:713
> +    void rebuildZOrderLists(Vector<RenderLayer*>*& posZOrderList, Vector<RenderLayer*>*& negZOrderList) const;

This name is wrong; it's not rebuilding the layer's lists, it's computing new ones.

> Source/WebCore/rendering/RenderLayer.h:717
> +    void rebuildNormalFlowList(Vector<RenderLayer*>*& normalFlowList) const;

Ditto.

>> Source/WebCore/rendering/RenderLayer.h:934
>> +    // render layers they operate on.
> 
> Ah, I completely missed these were accessing private members before. Thanks for the explanation.

Why not make them member functions (on the toBePromoted layer?)

>> Source/WebCore/rendering/RenderLayerBacking.cpp:740
>> +        FloatSize clipSize = paddingBox.size();
> 
> Perhaps this should be a separate patch to keep things simpler? Could use a test too.

Agreed, please separate this out.

> Source/WebCore/rendering/RenderLayerCompositor.h:234
> +    void requestReevaluateCompositingAfterLayout() { m_reevaluateCompositingAfterLayout = true; }

I don't like this name. I think it should be setShouldReevaluateCompositingAfterLayout().

Created attachment 176053 [details]
Patch

(In reply to comment #42)
> > Source/WebCore/rendering/RenderLayer.cpp:1727
> > +    bool automaticallyOptIn = renderer()->frame()->page()->settings()->acceleratedCompositingForOverflowScrollEnabled()
>
> At least frame() and page() can be null here, so better check for that.
I've addded RenderLayer::acceleratedCompositingForOverflowScrollEnabled() since we need to do this check in several places. I do the required nullity checks there.
>
> > Source/WebCore/rendering/RenderLayer.cpp:1731
> > +    return renderer()->style()->useTouchOverflowScrolling() || automaticallyOptIn;
>
> Nit: this could be the other way around so there's no need to consult the style when we've already opted in.
Done.
>
> > Source/WebCore/rendering/RenderLayer.cpp:1901
> > +    if (compositor()->inCompositingMode() && usesCompositedScrolling())
>
> usesCompositedScrolling() is also called in a few other places to avoid repaints. Do we need this same inCompositingMode() check there too?
We do, yes. Rather than checking inCompositingMode() everywhere, I've changed usesCompositedScrolling() to return true if and only iff the layer is actually using composited scrolling, and I've added needsCompositedScrolling() to indicate that the layer would prefer to use composited scrolling.
>
> > Source/WebCore/rendering/RenderLayerBacking.cpp:740
> > +        FloatSize clipSize = paddingBox.size();
>
> Perhaps this should be a separate patch to keep things simpler? Could use a test too.
Done. wkb.ug/103156

(In reply to comment #43)
> (From update of attachment 175811 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=175811&action=review
>
> As far as I can see, you're only calling canSafelyEstablishAStackingContext() in usesCompositedScrolling() to determine whether it's ok to do so. What I don't see is the actually affecting the computed z-order lists. Am I missing that?
I think this comment may no longer be applicable -- I now compute m_canSafelyEstablishAStackingContext right after we update our layer lists.
>
> There are other cases where we'd like to be able to treat something like a stacking context when possible (e.g. something with overflow and composited descendants), so it would be good if there same code could be reused there. I think it would also be better if you could run this 'can be a stacking context' logic at z-order-list building time, which would I think allow the isDescendant() logic to avoid running up to the root on failure. Then the z-order lists for the layers would just reflect that this was made a stacking context.
Done. I think that this approach is also cleaner and more understandable. Although the code is short, I've included a lengthy comment explaining how/why it works. It's also efficient. It's linear the number of nodes in the stacking context -- that is the nodes we visit in collectLayers. We also do this work no more frequently than we call collectLayers.
>
> Without pixel or ref tests I don't see how your tests show that the 'can be a stacking context' logic is correct.
I used to have an internals call that I used to ask the layer if it was really using composited scrolling. I compared this with an array of expected values I made by stepping through the loop and manually checking if it was safe to become a stacking context at that iteration. Sami suggested that rather than using the booleans, I dump the layer trees. So we now either produce no layer tree (if we didn't become a stacking context and promote the layer), or we get the resulting layer tree. This lines up with the boolean test I was doing earlier, but also had the benefit of ensuring that our trees both get generated and look the way we wanted. There are lots of permutations in this test (30) and it seemed overkill to put each one in its own test with a visual expectation.
>
> >> Source/WebCore/ChangeLog:12
> >> +        antialiasing, so it is important that automatically opting in is only
> >
> > Did you mean paint order instead of antialiasing?
>
> I presume by antialiasing he really means font smoothing.
Yes, that's right. I was referring to sub-pixel aa.
>
> > Source/WebCore/rendering/RenderLayer.cpp:492
> > +    layer->compositor()->requestReevaluateCompositingAfterLayout();
>
> I don't like it that things outside of RLC can tell it to do compositing-related stuff; this is the first instance of it.
>
> Can you move this logic to inside of RLC somehow?
I couldn't see a way, but perhaps I'm missing something. It's now the case that changing the z-order or normal flow lists can affect our decision to switch in or out of compositing mode, so I've made a request to reevaluate when these lists get dirtied.
>
> Rename automaticallyOptIn to alwaysUseCompositedScrolling or something.
I've gone with forceUseCompositedScrolling. Is that ok?
>
> >> Source/WebCore/rendering/RenderLayerBacking.cpp:740
> >> +        FloatSize clipSize = paddingBox.size();
> >
> > Perhaps this should be a separate patch to keep things simpler? Could use a test too.
>
> Agreed, please separate this out.
Done.
>
> > Source/WebCore/rendering/RenderLayerCompositor.h:234
> > +    void requestReevaluateCompositingAfterLayout() { m_reevaluateCompositingAfterLayout = true; }
>
> I don't like this name. I think it should be setShouldReevaluateCompositingAfterLayout().
Done.

[Early Warning System Bot]

Comment on attachment 176053 [details]
Patch

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

[Early Warning System Bot]

Comment on attachment 176053 [details]
Patch

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

[EFL EWS Bot]

Comment on attachment 176053 [details]
Patch

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

[Peter Beverloo (cr-android ews)]

Comment on attachment 176053 [details]
Patch

Attachment 176053 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/14987817

Created attachment 176059 [details]
Patch

Fixed the trailing '\'s that are causing the ews bots grief.

[Build Bot]

Comment on attachment 176059 [details]
Patch

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

New failing tests:
compositing/overflow/overflow-scrollbar-layers.html
compositing/overflow/content-gains-scrollbars.html
compositing/overflow/automatically-opt-into-composited-scrolling.html

Created attachment 176316 [details]
Patch

Added missing mac expectation.

Created attachment 176731 [details]
Trivial change: adding a missing line in a comment.

[Sami Kyöstilä]

Comment on attachment 176731 [details]
Trivial change: adding a missing line in a comment.

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

Thanks Ian, I think this looks great. It would be interesting to see what percentage of overflow scrolling elements this applies to. We could add some metrics for that later.

> Source/WebCore/rendering/RenderLayer.cpp:1793
> +    bool forceUseCompositedScrolling = acceleratedCompositingForOverflowScrollEnabled()  && canSafelyEstablishAStackingContext();

Nit: extra space before "&&".

> Source/WebCore/testing/InternalSettings.idl:-72
> -

Was this an intentional change?

Created attachment 177564 [details]
Patch

Some notes about performance. I profiled the behavior of two heavy pages, with and without the flag to enable opting in. I should also mention my profiling methodology. We have scrolling benchmark that loads various pages and simulates scrolls, during which it gathers performance data. Here's what I saw:

With opting in turned on:
  www.gmail.com:
    Avg # layers = 47.4
    Amount of time spent in RenderLayerCompositor::updateCompositingLayers = 1576.1 ms (13.8%)
  www.theverge.com: (It's worth noting that this page did *not* opt into composited scrolling)
    Avg # layers = 3.0
    Amount of time spent in RenderLayerCompositor::updateCompositingLayers = 272.6 ms

With opting in turned off:
  www.gmail.com:
    Avg # layers = 2.0
    Amount of time spent in RenderLayerCompositor::updateCompositingLayers = 196.8 ms
  www.theverge.com:
    Avg # layers = 3.0
    Amount of time spent in RenderLayerCompositor::updateCompositingLayers = 243.7 ms

Interestingly, if I comment out the setShouldReevaluateCompositingAfterLayout calls in dirtyZOrderLists and dirtyNormalFlowList, I measured that we spent 1598.436ms. Also, the time spent in RenderLayer::updateCanSafelyEstablishStackingContext is only 0.83ms! Moreover, if I measure the cost of updateCompositingLayers _before_ the scroll, it's about 144ms. This implies that the cost is almost entirely due to overlap testing during scroll. I certainly want to look at that problem, but it seems clear that it was not caused by this patch, and I don't think it makes sense to squeeze in an overlap testing optimization here, either.


(In reply to comment #53)
> (From update of attachment 176731 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=176731&action=review
>
> Thanks Ian, I think this looks great. It would be interesting to see what percentage of overflow scrolling elements this applies to. We could add some metrics for that later.
For sure, we're working on that already ;)
>
> > Source/WebCore/rendering/RenderLayer.cpp:1793
> > +    bool forceUseCompositedScrolling = acceleratedCompositingForOverflowScrollEnabled()  && canSafelyEstablishAStackingContext();
>
> Nit: extra space before "&&".
Done.
>
> > Source/WebCore/testing/InternalSettings.idl:-72
> > -
>
> Was this an intentional change?
Nope, removed.

[Early Warning System Bot]

Comment on attachment 177564 [details]
Patch

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

[WebKit Review Bot]

Comment on attachment 177564 [details]
Patch

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

[Build Bot]

Comment on attachment 177564 [details]
Patch

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

[Early Warning System Bot]

Comment on attachment 177564 [details]
Patch

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

[Build Bot]

Comment on attachment 177564 [details]
Patch

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

[Peter Beverloo (cr-android ews)]

Comment on attachment 177564 [details]
Patch

Attachment 177564 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/15154135

[EFL EWS Bot]

Comment on attachment 177564 [details]
Patch

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

Created attachment 177758 [details]
Patch

[Adrienne Walker]

Comment on attachment 177758 [details]
Patch

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

I like this change a lot and this feels really close to me.  Thanks for the performance numbers too.  There's definitely more work to be done, but it doesn't seem like it's the fault of this patch.

The only thing that makes me a little wary is that the test is pretty opaque.  It's not clear to me reviewing this patch what the expected output should be, or why it's different on different platforms.

If the text diff changed, it wouldn't be clear to me at all from the output what was broken.  Maybe there's some better way to do it.  More comments about what the tree state is? Parse the output and check if the right layers got promoted and just print PASS/FAIL?

> Source/WebCore/rendering/RenderLayer.cpp:481
> +// If we are a stacking context, then this function will generate a value for

This is an amazing comment.  Thank you!

Created attachment 179166 [details]
Patch

Here is some performance data I gathered on the latest patch. My methodology was
to do a trace while loading each of the pages (I stopped the verge once the page
appeared; I didn't wait for it to pull the ads). I measured what I thought could
be pain points affected by my patch.

With opting in enabled:
    gmail load:
      RenderLayerCompositor::updateCompositingLayers 180.446 ms
      RenderLayer::updateOutOfFlowPositioned 2.279 ms
      RenderLayer::updateDescendantDependentFlags 36.762 ms
      RenderLayer::updateNeedsCompositedScrolling 1.954 ms
      RenderLayerBacking::positionOverflowControlsLayers 0.079 ms
      RenderLayer::dirtyAncestorChainHasOutOfFlowPositionedDescendantStatus 8.064 ms
      RenderLayer::setAncestorChainHasOutOfFlowPositionedDescendant 0.109 ms

   the verge:
      RenderLayerCompositor::updateCompositingLayers 142.694 ms
      RenderLayer::updateOutOfFlowPositioned 3.314 ms
      RenderLayer::updateDescendantDependentFlags 42.278 ms
      RenderLayer::updateNeedsCompositedScrolling 1.979 ms
      RenderLayer::dirtyAncestorChainHasOutOfFlowPositionedDescendantStatus 0.397 ms
      RenderLayer::setAncestorChainHasOutOfFlowPositionedDescendant 0.424 ms

With opting in disabled:
    gmail load:
      RenderLayerCompositor::updateCompositingLayers 57.273 ms
      RenderLayer::updateOutOfFlowPositioned 2.408 ms
      RenderLayer::updateDescendantDependentFlags 64.563 ms
      RenderLayer::updateNeedsCompositedScrolling 1.402 ms
      RenderLayer::dirtyAncestorChainHasOutOfFlowPositionedDescendantStatus 8.091 ms
      RenderLayer::setAncestorChainHasOutOfFlowPositionedDescendant 0.097 ms

   the verge:
      RenderLayerCompositor::updateCompositingLayers 99.741 ms
      RenderLayer::updateOutOfFlowPositioned 2.87 ms
      RenderLayer::updateDescendantDependentFlags 23.787 ms
      RenderLayer::updateNeedsCompositedScrolling 1.473 ms
      RenderLayer::dirtyAncestorChainHasOutOfFlowPositionedDescendantStatus 0.856 ms
      RenderLayer::setAncestorChainHasOutOfFlowPositionedDescendant 0.47 ms

My apologies for the spam, but I should also mention that with this latest patch I've added a second requirement for opting into composited scrolling: we cannot have an out-of-flow positioned descendant whose containing block is not a descendant of ours (because we will clip it after we switch to composited scrolling).

[Adrienne Walker]

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

> LayoutTests/compositing/overflow/automatically-opt-into-composited-scrolling.html:103
> +      var expected_results = [

Oh! This makes way more sense now.  The auto-promoted layer is the only composited layer in the tree.  So if you get a tree, then promotion succeeded.

Sorry to be picky, but can you leave more comments in the test about how you came to these expected results? The quadruple nested for loops using single letter variables is probably efficient to write, but is still hard to decipher.

Created attachment 179183 [details]
Patch

(In reply to comment #66)
> View in context: https://bugs.webkit.org/attachment.cgi?id=179166&action=review
>
> Sorry to be picky, but can you leave more comments in the test about how you came to these expected results? The quadruple nested for loops using single letter variables is probably efficient to write, but is still hard to decipher.

That's a very good point. Rather than using a hard coded table of expected results, the test now dynamically computes the expected value. I've added lots of comments in this passage of code to explain how we arrive at the final value.

Also in this patch is a small bug fix: I've added a missing a call to updateNeedsCompositedScrolling in setAncestorChainHasOutOfFlowPositionedDescendant.

[Adrienne Walker]

Comment on attachment 179183 [details]
Patch

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

The test looks a million times better than two patches ago, thanks!

> Source/WebCore/rendering/RenderLayer.cpp:674
> +void RenderLayer::updateLayerPositionsAfterOverflowScroll(bool newlyPromoted)

This is a bit of a nit, but this function feels a little bit overloaded.  Unless I'm misunderstanding, it's kind of doing both updateLayerPositionsAfterScroll and positionOverflowControlsAfterPromotionToCompositedScrolling.  It might be more clear to separate the two.

> Source/WebCore/rendering/RenderLayer.h:758
> +    void updateIsNormalFlowOnly();

Unused?

Created attachment 179310 [details]
Patch

(In reply to comment #68)
> (From update of attachment 179183 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=179183&action=review
>
> The test looks a million times better than two patches ago, thanks!
>
> > Source/WebCore/rendering/RenderLayer.cpp:674
> > +void RenderLayer::updateLayerPositionsAfterOverflowScroll(bool newlyPromoted)
>
> This is a bit of a nit, but this function feels a little bit overloaded.  Unless I'm misunderstanding, it's kind of doing both updateLayerPositionsAfterScroll and positionOverflowControlsAfterPromotionToCompositedScrolling.  It might be more clear to separate the two.
Good call -- this looks much better.
>
> > Source/WebCore/rendering/RenderLayer.h:758
> > +    void updateIsNormalFlowOnly();
>
> Unused?

Yep, deleted.

[Adrienne Walker]

Comment on attachment 179310 [details]
Patch

R=me.

[WebKit Review Bot]

Comment on attachment 179310 [details]
Patch

Clearing flags on attachment: 179310

Committed r137646: <http://trac.webkit.org/changeset/137646>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Adam Klein]

I'm seeing some rendering crashes in layout tests with this change:

http://test-results.appspot.com/dashboards/flakiness_dashboard.html#tests=fullscreen%2Ffull-screen-fixed-pos-parent.html%2Cfullscreen%2Ffull-screen-iframe-without-allow-attribute-allowed-from-parent.html

Can you please take a look? There are also some plain-old failures.

[Adrienne Walker]

Reverted r137646 for reason:

Breaks some overflow layout tests

Committed r137682: <http://trac.webkit.org/changeset/137682>

[Adrienne Walker]

See http://test-results.appspot.com/dashboards/flakiness_dashboard.html#showExpectations=true&tests=compositing%2Foverflow%2Fscrollbars-with-clipped-owner.html, it looks like scrollbars are being clipped when they shouldn't.

Created attachment 179507 [details]
Run against new virtual test suite

[WebKit Review Bot]

Comment on attachment 179507 [details]
Run against new virtual test suite

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

New failing tests:
platform/chromium/virtual/gpu/compositedscrolling/overflow/scrollbars-with-clipped-owner.html
platform/chromium/virtual/gpu/compositedscrolling/overflow/nested-scrolling.html
platform/chromium/virtual/gpu/compositedscrolling/scrollbars/scrollbars-on-positioned-content.html
platform/chromium/virtual/gpu/compositedscrolling/overflow/overflow-auto-with-touch-toggle.html
platform/chromium/virtual/gpu/compositedscrolling/scrollbars/overflow-scrollbar-combinations.html
platform/chromium/virtual/gpu/compositedscrolling/scrollbars/custom-scrollbar-with-incomplete-style.html

Created attachment 179623 [details]
Patch

Things that have changed in this patch:
 - Added a test expectation (failure is due to wkb.ug/103156).
 - Moved the spot in RLC where we position the newly created overflow scroll controls (its in rebuildCompositingLayerTree now)
 - Renamed this function to positionNewlyCreatedOverflowControls() and removed an early out for when we're not doing composited scrolling.

[Adrienne Walker]

Comment on attachment 179623 [details]
Patch

R=me.  Thanks for following up on the test failures.  :)

[WebKit Review Bot]

Comment on attachment 179623 [details]
Patch

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

New failing tests:
platform/chromium/virtual/gpu/compositedscrolling/overflow/overflow-auto-with-touch-toggle.html
platform/chromium/virtual/gpu/compositedscrolling/scrollbars/overflow-scrollbar-combinations.html
platform/chromium/virtual/gpu/compositedscrolling/overflow/nested-scrolling.html
platform/chromium/virtual/gpu/compositedscrolling/scrollbars/scrollbars-on-positioned-content.html

(In reply to comment #80)
> (From update of attachment 179623 [details])
> Attachment 179623 [details] did not pass chromium-ews (chromium-xvfb):
> Output: http://queues.webkit.org/results/15363241
> 
> New failing tests:
> platform/chromium/virtual/gpu/compositedscrolling/overflow/overflow-auto-with-touch-toggle.html
> platform/chromium/virtual/gpu/compositedscrolling/scrollbars/overflow-scrollbar-combinations.html
> platform/chromium/virtual/gpu/compositedscrolling/overflow/nested-scrolling.html
> platform/chromium/virtual/gpu/compositedscrolling/scrollbars/scrollbars-on-positioned-content.html

I've rerun these tests locally and confirmed that they do pass, they just need to be rebaselined.

[WebKit Review Bot]

Comment on attachment 179623 [details]
Patch

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

New failing tests:
platform/chromium/virtual/gpu/compositedscrolling/overflow/overflow-auto-with-touch-toggle.html
platform/chromium/virtual/gpu/compositedscrolling/scrollbars/overflow-scrollbar-combinations.html
platform/chromium/virtual/gpu/compositedscrolling/overflow/nested-scrolling.html
platform/chromium/virtual/gpu/compositedscrolling/scrollbars/scrollbars-on-positioned-content.html

[James Robinson]

(In reply to comment #81)
> (In reply to comment #80)
> > (From update of attachment 179623 [details] [details])
> > Attachment 179623 [details] [details] did not pass chromium-ews (chromium-xvfb):
> > Output: http://queues.webkit.org/results/15363241
> > 
> > New failing tests:
> > platform/chromium/virtual/gpu/compositedscrolling/overflow/overflow-auto-with-touch-toggle.html
> > platform/chromium/virtual/gpu/compositedscrolling/scrollbars/overflow-scrollbar-combinations.html
> > platform/chromium/virtual/gpu/compositedscrolling/overflow/nested-scrolling.html
> > platform/chromium/virtual/gpu/compositedscrolling/scrollbars/scrollbars-on-positioned-content.html
> 
> I've rerun these tests locally and confirmed that they do pass, they just need to be rebaselined.

Please either rebaseline then in this patch or mark them appropriate in TestExpectations

Commited with - http://trac.webkit.org/changeset/137828

[Chris Dumez]

The new test appears to be failing on WK2 EFL, I'll skip it for now.

[Chris Dumez]

Forgot to provide the link, sorry:
http://build.webkit.org/results/EFL%20Linux%2064-bit%20Debug%20WK2/r137871%20(7193)/compositing/overflow/automatically-opt-into-composited-scrolling-pretty-diff.html

[Antonio Gomes]

Comment on attachment 179310 [details]
Patch

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

I am late here.

A couple of nits below.

> Source/WebCore/rendering/RenderLayer.cpp:485
> +// If we are a stacking context, then this function will determine if our
> +// descendants for a contiguous block in stacking order. This is required in

I feel like there is missing verb here: block *are* in stacking order?

> Source/WebCore/rendering/RenderLayer.cpp:488
> +// order of layers on the page. That can only happen if a non-descendant appear

appears?

> Source/WebCore/rendering/RenderLayer.cpp:499
> +// I've labeled our normal flow descendants A, B, C, and D, our stacking

We usually do not refer to "I" in comments. Whoever look at it, would not know who "I" is:)

same with "our", us, we...

> Source/WebCore/rendering/RenderLayer.cpp:570
> +    // Create a reverse lookup.
> +    HashMap<const RenderLayer*, int> lookup;

name it reverseLookup and remove the comment.

> Source/WebCore/rendering/RenderLayer.cpp:891
> +    if (isStackingContext() || !stackingContext())

I understood this if, but the two cases within it look odd to follow at first. Maybe the methods are mis named?

[Simon Fraser (smfr)]

https://trac.webkit.org/changeset/137828/webkit