Summary: | Style invalidation does not work for adjacent node updates | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Benjamin Poulain <benjamin> | ||||||||||||
Component: | New Bugs | Assignee: | Benjamin Poulain <benjamin> | ||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||
Severity: | Normal | CC: | buildbot, kling, koivisto, rniwa | ||||||||||||
Priority: | P2 | ||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||
Hardware: | Unspecified | ||||||||||||||
OS: | Unspecified | ||||||||||||||
Attachments: |
|
Description
Benjamin Poulain
2014-08-21 20:35:26 PDT
Created attachment 236961 [details]
Patch
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 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 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 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 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 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 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. (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 :) Created attachment 236996 [details]
Patch
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. Committed r172880: <http://trac.webkit.org/changeset/172880> |