[webkit-reviews] review denied: [Bug 94743] Automatically use composited scrolling : [Attachment 175811] Patch

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has denied vollick at chromium.org's
request for review:
Bug 94743: Automatically use composited scrolling
https://bugs.webkit.org/show_bug.cgi?id=94743

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

------- Additional Comments from Simon Fraser (smfr) <simon.fraser at apple.com>
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()->acceleratedCompositingForOverflowScrol
lEnabled()
> 
> 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().