[webkit-reviews] review denied: [Bug 109302] Simplify logic for automatically opting into composited scrolling : [Attachment 189137] 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 189137: Patch
https://bugs.webkit.org/attachment.cgi?id=189137&action=review

------- Additional Comments from Julien Chaffraix <jchaffraix at webkit.org>
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 (hartmanng at chromium.org): 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.