[webkit-reviews] review denied: [Bug 68733] REGRESSION (r95239): [chromium] Twitter.com now extremely slow from border-radius clips. : [Attachment 111064] Patch

[bugzilla-daemon at webkit.org]

Julien Chaffraix <jchaffraix at webkit.org> has denied Tom Hudson
<tomhudson at google.com>'s request for review:
Bug 68733: REGRESSION (r95239): [chromium] Twitter.com now extremely slow from
border-radius clips.
https://bugs.webkit.org/show_bug.cgi?id=68733

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

------- Additional Comments from Julien Chaffraix <jchaffraix at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=111064&action=review


r- as the patch does not pass the tests and contains several errors in the
logic. It would also be really nice to add a performance test for this bug (if
that's possible).

> Source/WebCore/rendering/RenderLayer.cpp:2642
> +    if (paintFlags & RenderLayer::PaintLayerAppliedTransform)

This can be replaced with a boolean at the call site (paintFlags &
PaintLayerAppliedTransform). Now if you take the paintsWithTransform() check
out and transform() out of shouldApplyHitTestTransform, your functions are
equivalent. Unless the ordering is important for performance (you need a
comment about that then) that I missed here.

> Source/WebCore/rendering/RenderLayer.cpp:4219
> +	   && oldStyle && oldStyle->position() == FixedPosition) {

Those 2 if's are not the negation of one another. This line should be:

|| (oldStyle && oldStyle->position() == FixedPosition) {

To make the code more readable, please use some boolean variables. For example,
this would be a lot easier to read:

bool isFixedPositionedLayer = renderer()->style()->position() == FixedPosition;

bool wasFixedPositionedLayer = oldStyle && oldStyle->position() ==
FixedPosition;

if (isFixedPosition != wasFixedPosition) { ... }

>> Source/WebCore/rendering/RenderLayer.cpp:4347
>> +	    layer->m_fixedPositionedObjectsInSubtree -= delta;
> 
> If the delta is -1, won't this increment the count? If not, or it that's the
intention, then the code needs more comments!

It looks like the logic is backwards here. You would like your delta to be
added to your count not to be subtracted.

> Source/WebCore/rendering/RenderLayer.h:806
> +    int m_fixedPositionedObjectsInSubtree;

I really wonder if using a boolean would not make the whole logic better (also
make us use less memory). You just need to know if you have *one* fixed
descendant, not really the exact number. It should be renamed to
m_hasFixedPositionedDescendant in this case (or an equivalent name).

If that's not possible, the field should be unsigned as we don't expect it to
be negative. A better name would be m_fixedPositionedDescendantCount.