[webkit-reviews] review denied: [Bug 109302] Simplify logic for automatically opting into composited scrolling : [Attachment 191233] Patch

[bugzilla-daemon at webkit.org]

Julien Chaffraix <jchaffraix at webkit.org> has denied Glenn Hartmann
<hartmanng at chromium.org>'s request for review:
Bug 109302: Simplify logic for automatically opting into composited scrolling
https://bugs.webkit.org/show_bug.cgi?id=109302

Attachment 191233: Patch
https://bugs.webkit.org/attachment.cgi?id=191233&action=review

------- Additional Comments from Julien Chaffraix <jchaffraix at webkit.org>
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.