Bug 136145 – Style invalidation does not work for adjacent node updates

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Benjamin Poulain]

Style invalidation does not work for adjacent node updates

[Benjamin Poulain]

Created attachment 236961 [details]
Patch

[Build Bot]

Comment on attachment 236961 [details]
Patch

Attachment 236961 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/5560241416568832

New failing tests:
fast/table/multiple-captions-crash.xhtml
fast/dom/HTMLImageElement/image-load-cross-document.html
fast/overflow/scroll-div-hide-show.html
fast/parser/move-during-parsing.html
fast/layers/layer-visibility.html
svg/carto.net/tabgroup.svg
fast/table/multiple-captions-crash2.xhtml

[Build Bot]

Created attachment 236965 [details]
Archive of layout-test-results from webkit-ews-11 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-11  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

[Build Bot]

Comment on attachment 236961 [details]
Patch

Attachment 236961 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/5488539085045760

New failing tests:
fast/table/multiple-captions-crash.xhtml
fast/dom/HTMLImageElement/image-load-cross-document.html
fast/overflow/scroll-div-hide-show.html
fast/parser/move-during-parsing.html
fast/layers/layer-visibility.html
svg/carto.net/tabgroup.svg
fast/table/multiple-captions-crash2.xhtml

[Build Bot]

Created attachment 236968 [details]
Archive of layout-test-results from webkit-ews-03 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-03  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[Build Bot]

Comment on attachment 236961 [details]
Patch

Attachment 236961 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/5872496544841728

New failing tests:
fast/table/multiple-captions-crash.xhtml
fast/dom/HTMLImageElement/image-load-cross-document.html
fast/overflow/scroll-div-hide-show.html
fast/parser/move-during-parsing.html
fast/layers/layer-visibility.html
svg/carto.net/tabgroup.svg
fast/table/multiple-captions-crash2.xhtml

[Build Bot]

Created attachment 236970 [details]
Archive of layout-test-results from webkit-ews-08 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-08  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[Darin Adler]

Comment on attachment 236961 [details]
Patch

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.

[Benjamin Poulain]

(In reply to comment #8)
> > 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.

I use TODO for stuff I need to look into before putting the patch for review. I forgot to deal with this one :)

[Benjamin Poulain]

Created attachment 236996 [details]
Patch

[Antti Koivisto]

Comment on attachment 236996 [details]
Patch

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

> Source/WebCore/dom/Document.cpp:6150
> +    for (const ContainerNode* ancestor = node.parentOrShadowHostNode(); ancestor; ancestor = ancestor->parentOrShadowHostNode()) {

Using parentOrShadowHostElement() here would avoid the cast. The only non-Element is the Document which can be handled separately.

[Benjamin Poulain]

Committed r172880: <http://trac.webkit.org/changeset/172880>