[webkit-reviews] review granted: [Bug 234018] nullptr deref in ComputeFloatOffsetForLineLayoutAdapter<FloatingObject::FloatLeft>::updateOffsetIfNeeded : [Attachment 449117] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Gabriel Nava Marino
<gnavamarino at apple.com>'s request for review:
Bug 234018: nullptr deref in
ComputeFloatOffsetForLineLayoutAdapter<FloatingObject::FloatLeft>::updateOffset
IfNeeded
https://bugs.webkit.org/show_bug.cgi?id=234018

Attachment 449117: Patch

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




--- Comment #28 from Darin Adler <darin at apple.com> ---
Comment on attachment 449117
  --> https://bugs.webkit.org/attachment.cgi?id=449117
Patch

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

> Source/WebCore/ChangeLog:9
> +	   Implement InclusiveRenderDescendantIteratorAdapter and use
inclusiveDescendantsOfType in
> +	   RenderBlockFlow::subtreeContainsFloat and
RenderBlockFlow::subtreeContainsFloats.

I suspect there is a way to implement this sharing more code with the
non-inclusive case, since non-inclusive is just like inclusive, but just
skipping the thing itself, but it’s more important how we *use*
inclusiveDescendantsOfType than how smooth the implementation is. That can
always be refined later.

I would suggest, though, that we address the const-ness issue eventually. Maybe
even at the same time.

Ideally the descendantsOfType on a const reference would iterate const
references, and non-const would iterate non-const. Ideally without duplicating
all the code twice, so maybe using some const_cast in the implementation.

> Source/WebCore/rendering/RenderBlockFlow.cpp:1957
> +    for (auto& block :
inclusiveDescendantsOfType<RenderBlock>(const_cast<RenderBlockFlow&>(*this))) {
> +	   if (is<RenderBlockFlow>(block) &&
downcast<RenderBlockFlow>(block).containsFloat(renderer))
>	       return true;
>      }

Hmm, can we write this?

    for (auto& block : inclusiveDescendantsOfType<RenderBlockFlow>(*this)) {
	if (block.containsFloat(renderer))
	    return true;
    }

That’s the whole point of having the xxxOfType feature (and assumes the const).