[webkit-reviews] review denied: [Bug 101779] Accumulate using a LayoutSize in mapLocalToContainer when possible : [Attachment 178596] Patch

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has denied Levi Weintraub
<leviw at chromium.org>'s request for review:
Bug 101779: Accumulate using a LayoutSize in mapLocalToContainer when possible
https://bugs.webkit.org/show_bug.cgi?id=101779

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

------- Additional Comments from Simon Fraser (smfr) <simon.fraser at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=178596&action=review


Why aren't the pixel results using mock scrollbars?

It's very hard for me to tell whether this patch is correct. I'd feel more
confident if you had a bunch of tests that hit assertions with the wrong
version of the patch, but no longer do so. r- for the apparently incorrect
flattening.

> Source/WebCore/ChangeLog:42
> +	   (WebCore::RenderObject::mapLocalToContainer): Broke
mapLocalToContainer into this public function and
> +	   mapLocalToContainerInternal. The internal flavors track an
accumulated offset which is used to recreate the pixel
> +	   snapping that's performed during paint. This public function calls
the internal one, then applies any leftover
> +	   accumulated offset before returning the TransformState to the
caller.

mapLocalToContainer was already the "internal" method. I agree with Julian that
some renaming might help.

> Source/WebCore/rendering/RenderBox.cpp:1425
> +	   applyAccumulatedOffset(transformState, accumulatedOffset, mode);

applyAccumulatedOffset() is flattening here, since the default last param is
'false'. How can that be correct?

> Source/WebCore/rendering/RenderBox.cpp:1429
> -	   transformState.move(containerOffset.width(),
containerOffset.height(), preserve3D ? TransformState::AccumulateTransform :
TransformState::FlattenTransform);
> +	   accumulatedOffset.expand(containerOffset);

So it seems like you're carrying along all of the non-transform offsets in
accumulatedOffset, which kinda defeats the purpose of TransformState. This
seems like a cop-out.