[webkit-reviews] review granted: [Bug 246216] Invalid counters after contain: style : [Attachment 463096] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Rob Buis <rbuis at igalia.com>'s request
for review:
Bug 246216: Invalid counters after contain: style
https://bugs.webkit.org/show_bug.cgi?id=246216

Attachment 463096: Patch

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




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

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

> Source/WebCore/rendering/RenderCounter.cpp:76
>  // This function processes the renderer tree in the order of the DOM tree
>  // including pseudo elements as defined in CSS 2.1.

This comment does not seem quite accurate any more.

> Source/WebCore/rendering/RenderCounter.cpp:103
> +	   while (previous && !previous->renderer())
> +	       previous = ElementTraversal::previousIncludingPseudo(*previous,
styleContainAncestor);
> +	   if (!previous)
> +	       return nullptr;
> +	   Element* previousStyleContainAncestor =
ancestorStyleContainmentObject(*previous);
> +	   // If the candidate's containment ancestor is the same as elements,
then
> +	   // that's a valid candidate.
> +	   if (previousStyleContainAncestor == styleContainAncestor)
> +	       return previous->renderer();
> +
> +	   // Otherwise, if previous does not have a containment ancestor, it
means
> +	   // that we have already escaped `element`'s containment ancestor, so
return
> +	   // nullptr.
> +	   if (!previousStyleContainAncestor)
> +	       return nullptr;
> +
> +	   // If, however, the candidate does have a containment ancestor, it
could be
> +	   // that we entered a new sub-containment. Try again starting from
the
> +	   // contain ancestor.
> +	   previous = previousStyleContainAncestor;

This can be written more economically:

    while (previous) {
	while (previous && !previous->renderer())
	    previous = ElementTraversal::previousIncludingPseudo(*previous,
styleContainAncestor);
	if (!previous)
	    break;
	auto* previousStyleContainmentAncestor =
ancestorStyleContainmentObject(*previous);
	if (previousStyleContainmentAncestor == styleContainAncestor)
	    return previous->renderer();
	previous = previousStyleContainmentAncestor;
    }
    return nullptr;

Could still include the same comments, but don’t need as much code.