[webkit-reviews] review granted: [Bug 226458] [css-contain] Support contain:style : [Attachment 430750] Patch

[bugzilla-daemon at webkit.org]

Antti Koivisto <koivisto at iki.fi> has granted Rob Buis <rbuis at igalia.com>'s
request for review:
Bug 226458: [css-contain] Support contain:style
https://bugs.webkit.org/show_bug.cgi?id=226458

Attachment 430750: Patch

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




--- Comment #9 from Antti Koivisto <koivisto at iki.fi> ---
Comment on attachment 430750
  --> https://bugs.webkit.org/attachment.cgi?id=430750
Patch

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

> Source/WebCore/rendering/RenderCounter.cpp:64
> +    Element* ancestor = is<PseudoElement>(element) ?
downcast<PseudoElement>(element).hostElement() : element.parentElement();
> +    while (ancestor) {
> +	   if (auto* style = ancestor->existingComputedStyle()) {

I suppose this doesn't just traverse the render tree and get the style from
there because you can have style containment in a non-rendered
'display:contents' element? Is that covered by tests?

> Source/WebCore/rendering/RenderCounter.cpp:68
> +	   ancestor = ancestor->parentElement();

This should probably be doing composed tree traversal (parentInComposedTree).
Of course the existing counter code doesn't do that either (so counters don't
work accross shadow boundaries). At least this deserves a comment.

> Source/WebCore/rendering/RenderCounter.cpp:87
> +    Element* styleContainAncestor =
ancestorStyleContainmentObject(*renderer.element());
> +
> +    while (true) {
> +	   while (previous && !previous->renderer())
> +	       previous = ElementTraversal::previousIncludingPseudo(*previous);
> +	   if (!previous)
> +	       return nullptr;
> +	   Element* previousStyleContainAncestor =
ancestorStyleContainmentObject(*previous);
> +	   if (previousStyleContainAncestor == styleContainAncestor)

These traversal functions take optional 'stayWithin' parameter which I think
should here be the styleContainAncestor. This avoids searching	for the
ancestor repeatedly.

> Source/WebCore/rendering/RenderCounter.cpp:578
> +    bool crossedBoundary = false;

Which boundary? This could use a better name.

> Source/WebCore/rendering/RenderCounter.cpp:580
> +	   crossedBoundary |= shouldApplyStyleContainment(*descendant);

using logical operator instead of a bitwise one will avoid running the function
repeatedly

crossedBoundary = crossedBoundary || shouldApplyStyleContainment(*descendant);

> Source/WebCore/rendering/RenderObject.cpp:2520
> +    return renderer.style().containsStyle() && (!renderer.isInline() ||
renderer.isAtomicInlineLevelBox()) && !renderer.isRubyText() &&
(!renderer.isTablePart() || renderer.isTableCaption()) && !renderer.isTable();

if (!renderer.style().containsStyle())
   return false;
if (renderer.isInline() && !renderer.isAtomicInlineLevelBox())
   return false;
...etc organised in some readable manner

> Source/WebCore/rendering/style/RenderStyle.h:531
>      bool containsLayout() const { return
m_rareNonInheritedData->contain.contains(Containment::Layout); }
>      bool containsSize() const { return
m_rareNonInheritedData->contain.contains(Containment::Size); }
> +    bool containsStyle() const { return
m_rareNonInheritedData->contain.contains(Containment::Style); }

Maybe these would read better as

hasLayoutContainment()/hasSizeContainment()/hasStyleContainmen()?