[webkit-reviews] review denied: [Bug 136145] Style invalidation does not work for adjacent node updates : [Attachment 236961] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Benjamin Poulain
<benjamin at webkit.org>'s request for review:
Bug 136145: Style invalidation does not work for adjacent node updates
https://bugs.webkit.org/show_bug.cgi?id=136145

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

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=236961&action=review


I can’t say review+ because of the test failures:

[1/7] fast/dom/HTMLImageElement/image-load-cross-document.html failed
unexpectedly (text diff)
[2/7] fast/layers/layer-visibility.html failed unexpectedly (text diff, image
diff)
[3/7] fast/overflow/scroll-div-hide-show.html failed unexpectedly (text diff)
[4/7] fast/parser/move-during-parsing.html failed unexpectedly (text diff)
[5/7] fast/table/multiple-captions-crash.xhtml failed unexpectedly (text diff)
[6/7] fast/table/multiple-captions-crash2.xhtml failed unexpectedly (text diff)

[7/7] svg/carto.net/tabgroup.svg failed unexpectedly (text diff, image diff)

> Source/WebCore/ChangeLog:25
> +	   DirectChildNeedsStyleRecalcFlag to note that one of the child has an
invalid style.

"one of the child has" -> "one of the child nodes has" or "one of the children
has"

> Source/WebCore/dom/Document.cpp:6159
> +    for (const ContainerNode* ancestor = node.parentOrShadowHostNode();
ancestor; ancestor = ancestor->parentOrShadowHostNode()) {
> +	   if (ancestor->needsStyleRecalc())
>	       return true;
> +
> +	   if (ancestor->directChildNeedsStyleRecalc() &&
ancestor->isElementNode()) {
> +	       const Element& ancestorElement = toElement(*ancestor);
> +	       if (ancestorElement.childrenAffectedByDirectAdjacentRules() ||
ancestorElement.childrenAffectedByForwardPositionalRules())
> +		   return true;
> +	   }
>      }

Antti and Andreas would have you write:

    for (auto& ancestor : elementAncestors(node)) {
	...
    }

Or since I know prefer like to avoid auto you could do:

    for (Element& ancestor : elementAncestors(node)) {
	...
    }

There is no need to iterate to a ContainerNode, since the only one besides
Element is the Document, and the document doesn’t have any ancestor elements.
You won’t need to do any type casting, because the iterator handles it for you,
you can use "." instead of "->" and make it so the reader doesn’t have to think
about null.

But I guess you can’t do any of this because elementAncestors uses
parentElement rather than parentOrShadowHostNode.

> Source/WebCore/dom/Element.cpp:1606
> +    // TODO: those should not invalidate the parent anymore.

We use FIXME on this project, not TODO. Also, such comments need to say *why*,
not just state something.

> Source/WebCore/style/StyleResolveTree.cpp:555
> +    bool invalidateDirectChildren = current.directChildNeedsStyleRecalc() &&
(current.childrenAffectedByDirectAdjacentRules() ||
current.childrenAffectedByForwardPositionalRules());

We try to use adjective phrases for predications, so it should be
shouldInvalidateDirectChildren rather than “invalidate direct children”, which
is a verb phrase.