Make getComputedStyle optimization work for adjacent combinators.

Make getComputedStyle optimization work for adjacent combinators.

Recalc style was avoided if none of the ascendants needed style recalc or
style invalidation. That did not catch the cases where SubtreeStyleChange
is propagated to siblings when the node or some of its siblings have tried
to match selectors with adjacent combinators (propagation happens in
ContainerNode::checkForChildrenAdjacentRuleChanges()).

We change the condition for recalc to include the case where we have an
ascendant which has children that needs recalc, and at the same time has
children that tried to match some selector with an adjacent combinator.
For that case a SubtreeStyleChange may be propagated to a sibling which
is an ascendant of the element we are calling getComputedStyle on.

Likewise for ascendants which have children that needs style invalidation
and at the same time have children that tried to match selectors with
adjacent combinators.

R=esprehn@chromium.org
BUG=436064
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=186599

[rune, 2014-12-05 08:51:17]

PTAL

[esprehn, 2014-12-05 09:52:36]

lgtm, it would be nice to phrase the code differently, but the logic is fine.
Good catch!

https://codereview.chromium.org/768533004/diff/1/Source/core/dom/Document.cpp
File Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/768533004/diff/1/Source/core/dom/Document.cpp...
Source/core/dom/Document.cpp:1901: bool childrenAffectedByAdjacent = node !=
ancestor
Can we hoist this out of the loop?

if (node->needsStyleRecalc() || node->needsStyleInvalidation())
  needsRecalc = true;

then do the loop starting from a real ancestor.

for (const ContainerNode* ancestor = NodeRenderingTraversal::parent(*node);
ancestor && !needsRecalc; ancestor = NodeRenderingTraversal::parent(*ancestor))
{
  ...

https://codereview.chromium.org/768533004/diff/1/Source/core/dom/Document.cpp...
Source/core/dom/Document.cpp:1902: && ancestor->isContainerNode()
this statement doesn't make sense, an ancestor must be a container node. (I get
what the code does, but it's confusing with these names)

https://codereview.chromium.org/768533004/diff/1/Source/core/dom/Document.cpp...
Source/core/dom/Document.cpp:1908: || (childrenAffectedByAdjacent &&
(ancestor->childNeedsStyleRecalc() || ancestor->childNeedsStyleInvalidation()));
It seems like you can combine this with the above bit checks into a single
method like ancestor->needsAdjacentStyleRecalc() that checks

bool Node::needsAdjacentStyleRecalc() const {
  if (!childrenAffectedByDirectAdjacentRules() &&
!childrenAffectedByIndirectAdjacentRules())
    return false;
  return childNeedsStyleRecalc() || childNeedsStyleInvalidation();
}

then the loop is nicely

needsRecalc = ancestor->needsStyleRecalc() || ancestor->needsStyleInvalidation()
|| ancestor->needsAdjacentStyleRecalc();

[rune, 2014-12-05 11:07:23]

On 2014/12/05 at 09:52:36, esprehn wrote:
> lgtm, it would be nice to phrase the code differently, but the logic is fine.
Good catch!

Code re-arranged.

[rune, 2014-12-05 11:07:40]

The CQ bit was checked by rune@opera.com

[commit-bot: I haz the power, 2014-12-05 11:08:11]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/768533004/20001

[commit-bot: I haz the power, 2014-12-05 11:08:18]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-12-05 11:08:19]

Failed to apply patch for Source/core/dom/Document.cpp:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file Source/core/dom/Document.cpp
  Hunk #1 FAILED at 1895.
  1 out of 1 hunk FAILED -- saving rejects to file
Source/core/dom/Document.cpp.rej

Patch:       Source/core/dom/Document.cpp
Index: Source/core/dom/Document.cpp
diff --git a/Source/core/dom/Document.cpp b/Source/core/dom/Document.cpp
index
63cb1e518c441db49e3ef02308e29485cf4d1bf2..09e742415d9f151fada455284b63dfd1576ab905
100644
--- a/Source/core/dom/Document.cpp
+++ b/Source/core/dom/Document.cpp
@@ -1895,10 +1895,11 @@ void Document::updateStyle(StyleRecalcChange change)
 
 void Document::updateRenderTreeForNodeIfNeeded(Node* node)
 {
-    bool needsRecalc = needsFullRenderTreeUpdate();
+    ASSERT(node);
+    bool needsRecalc = needsFullRenderTreeUpdate() || node->needsStyleRecalc()
|| node->needsStyleInvalidation();
 
-    for (const Node* ancestor = node; ancestor && !needsRecalc; ancestor =
NodeRenderingTraversal::parent(*ancestor))
-        needsRecalc = ancestor->needsStyleRecalc() ||
ancestor->needsStyleInvalidation();
+    for (const ContainerNode* ancestor = NodeRenderingTraversal::parent(*node);
ancestor && !needsRecalc; ancestor = NodeRenderingTraversal::parent(*ancestor))
+        needsRecalc = ancestor->needsStyleRecalc() ||
ancestor->needsStyleInvalidation() || ancestor->needsAdjacentStyleRecalc();
 
     if (needsRecalc)
         updateRenderTreeIfNeeded();

[rune, 2014-12-05 11:30:42]

The CQ bit was checked by rune@opera.com

[commit-bot: I haz the power, 2014-12-05 11:31:21]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/768533004/40001

[commit-bot: I haz the power, 2014-12-05 12:39:03]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-12-05 12:39:03]

Try jobs failed on following builders:
  linux_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/3...)

[rune, 2014-12-05 13:38:40]

The CQ bit was checked by rune@opera.com

[commit-bot: I haz the power, 2014-12-05 13:39:22]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/768533004/60001

[commit-bot: I haz the power, 2014-12-05 16:15:29]

Committed patchset #4 (id:60001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=186599