[webkit-reviews] review granted: [Bug 203445] RenderTreeNeedsLayoutChecker asserts with css-position/position-absolute* tests : [Attachment 393394] Patch

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has granted zalan
<zalan at apple.com>'s request for review:
Bug 203445: RenderTreeNeedsLayoutChecker asserts with
css-position/position-absolute* tests
https://bugs.webkit.org/show_bug.cgi?id=203445

Attachment 393394: Patch

https://bugs.webkit.org/attachment.cgi?id=393394&action=review




--- Comment #3 from Simon Fraser (smfr) <simon.fraser at apple.com> ---
Comment on attachment 393394
  --> https://bugs.webkit.org/attachment.cgi?id=393394
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=393394&action=review

> Source/WebCore/rendering/RenderBlock.cpp:380
> +    auto outOfFlowDescendantsHaveNewContainingBlock = (hadTransform &&
!willHaveTransform) || (newStyle.position() == PositionType::Static &&
!willHaveTransform);

auto -> bool otherwise at a glance I might think this is a "containing block"

It's crazy that we don't have an "is containing block for out of flow
descendants" helper.

Why does this code not look at the oldStyle.position()?

> Source/WebCore/rendering/RenderBlock.cpp:1783
> +	   while (parent && !is<RenderBlock>(parent))
>	       parent = parent->parent();

Weird that there's no helper for this.

> Source/WebCore/rendering/RenderBlock.cpp:1787
> +	   if (renderer->isFixedPositioned())
> +	       view().setNeedsLayout();

Blank line above. Is this special-cased for fixed necessary or just an
optimization to avoid calling containingBlock() below?

> Source/WebCore/rendering/RenderBlock.cpp:1788
> +	   else if (auto* newContainingBlock = this->containingBlock()) {

Don't need this->