Bug 136145 - Style invalidation does not work for adjacent node updates
Summary: Style invalidation does not work for adjacent node updates
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Benjamin Poulain
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-08-21 20:35 PDT by Benjamin Poulain
Modified: 2014-08-22 21:30 PDT (History)
4 users (show)

See Also:


Attachments
Patch (75.07 KB, patch)
2014-08-21 20:59 PDT, Benjamin Poulain
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-11 for mac-mountainlion-wk2 (705.07 KB, application/zip)
2014-08-21 22:19 PDT, Build Bot
no flags Details
Archive of layout-test-results from webkit-ews-03 for mac-mountainlion (726.02 KB, application/zip)
2014-08-21 22:54 PDT, Build Bot
no flags Details
Archive of layout-test-results from webkit-ews-08 for mac-mountainlion (723.50 KB, application/zip)
2014-08-21 23:47 PDT, Build Bot
no flags Details
Patch (74.28 KB, patch)
2014-08-22 14:12 PDT, Benjamin Poulain
koivisto: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Benjamin Poulain 2014-08-21 20:35:26 PDT
Style invalidation does not work for adjacent node updates
Comment 1 Benjamin Poulain 2014-08-21 20:59:00 PDT
Created attachment 236961 [details]
Patch
Comment 2 Build Bot 2014-08-21 22:19:20 PDT
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
Comment 3 Build Bot 2014-08-21 22:19:21 PDT
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
Comment 4 Build Bot 2014-08-21 22:54:03 PDT
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
Comment 5 Build Bot 2014-08-21 22:54:05 PDT
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
Comment 6 Build Bot 2014-08-21 23:47:56 PDT
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
Comment 7 Build Bot 2014-08-21 23:47:58 PDT
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
Comment 8 Darin Adler 2014-08-22 09:17:40 PDT
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.
Comment 9 Benjamin Poulain 2014-08-22 13:00:05 PDT
(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 :)
Comment 10 Benjamin Poulain 2014-08-22 14:12:37 PDT
Created attachment 236996 [details]
Patch
Comment 11 Antti Koivisto 2014-08-22 20:54:23 PDT
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.
Comment 12 Benjamin Poulain 2014-08-22 21:30:51 PDT
Committed r172880: <http://trac.webkit.org/changeset/172880>