Remove the forced layout in getComputedStyle for elements in Shadow DOM

Remove isInShadowTree special case in Remove the forced layout in getComputedStyle for elements in Shadow DOM

This came from https://bugs.webkit.org/show_bug.cgi?id=97760 because the patch
walked the parentNode() chain looking for ancestors that need a recalc, which
didn't respect distributions, and the patch also didn't call updateDistribution
so a dirty distribution would cause incorrect results.

In blink the updateLayoutTreeForNodeIfNeeded method does respect shadow dom and
checks the distribution bits so we don't need to unconditionally force a layout.

Removing this exposed a bug in updateLayoutTreeForNodeIfNeeded where it was
using the composed tree looking for ancestor dirty bits, but the composed
tree skips over ShadowRoots, and those too can have dirty bits. To fix this
we extend the ParentTraversalDetails object to have the ShadowRoot that is
crossed when traversing.

[esprehn, 2016-01-29 05:43:17]

Description was changed from

==========
Remove isInShadowTree special case in
CSSComputedStyleDeclaration::getPropertyCSSValue.
==========

to

==========
Remove isInShadowTree special case in
CSSComputedStyleDeclaration::getPropertyCSSValue.

This came from https://bugs.webkit.org/show_bug.cgi?id=97760 but without any
explanation. Looking at the various attempts on that patch, it seems some tests
failed without the shadow dom case once, but now that our system is much saner
we shouldn't need the special case anymore.
==========

[esprehn, 2016-01-29 05:52:41]

Description was changed from

==========
Remove isInShadowTree special case in
CSSComputedStyleDeclaration::getPropertyCSSValue.

This came from https://bugs.webkit.org/show_bug.cgi?id=97760 but without any
explanation. Looking at the various attempts on that patch, it seems some tests
failed without the shadow dom case once, but now that our system is much saner
we shouldn't need the special case anymore.
==========

to

==========
Remove isInShadowTree special case in Remove the forced layout in
getComputedStyle for elements in Shadow DOM

This came from https://bugs.webkit.org/show_bug.cgi?id=97760 because the patch
walked the parentNode() chain looking for ancestors that need a recalc, which
didn't respect distributions, and the patch also didn't call updateDistribution
so a dirty distribution would cause incorrect results.

In blink the updateLayoutTreeForNodeIfNeeded method does respect shadow dom and
checks the distribution bits so we don't need to unconditionally force a layout.
==========

[esprehn, 2016-01-29 05:52:51]

Description was changed from

==========
Remove isInShadowTree special case in Remove the forced layout in
getComputedStyle for elements in Shadow DOM

This came from https://bugs.webkit.org/show_bug.cgi?id=97760 because the patch
walked the parentNode() chain looking for ancestors that need a recalc, which
didn't respect distributions, and the patch also didn't call updateDistribution
so a dirty distribution would cause incorrect results.

In blink the updateLayoutTreeForNodeIfNeeded method does respect shadow dom and
checks the distribution bits so we don't need to unconditionally force a layout.
==========

to

==========
Remove isInShadowTree special case in Remove the forced layout in
getComputedStyle for elements in Shadow DOM

This came from https://bugs.webkit.org/show_bug.cgi?id=97760 because the patch
walked the parentNode() chain looking for ancestors that need a recalc, which
didn't respect distributions, and the patch also didn't call updateDistribution
so a dirty distribution would cause incorrect results.

In blink the updateLayoutTreeForNodeIfNeeded method does respect shadow dom and
checks the distribution bits so we don't need to unconditionally force a layout.
==========

[esprehn, 2016-01-29 05:57:47]

The CQ bit was checked by esprehn@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-01-29 05:58:20]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1649983003/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1649983003/1

[esprehn, 2016-01-29 06:46:40]

unexpected_failures:
fast/dom/shadow/sibling-rules-dynamic-changes.html
fast/dom/shadow/sibling-rules-under-shadow-root.html

Looks like there's some issue with the sibling invalidator?

[esprehn, 2016-01-29 06:50:08]

esprehn@chromium.org changed reviewers:
+ rune@opera.com

[esprehn, 2016-01-29 06:50:09]

Any idea why this would be rune@ ?

[esprehn, 2016-01-29 06:51:40]

On 2016/01/29 at 06:50:09, esprehn wrote:
> Any idea why this would be rune@ ?

Actually I bet we just don't handle the case where the pending sheets are dirty,
but only in a shadow scope?

[commit-bot: I haz the power, 2016-01-29 06:57:36]

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

[commit-bot: I haz the power, 2016-01-29 06:57:37]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[rune, 2016-01-29 10:35:58]

On 2016/01/29 06:51:40, esprehn wrote:
> On 2016/01/29 at 06:50:09, esprehn wrote:
> > Any idea why this would be rune@ ?
> 
> Actually I bet we just don't handle the case where the pending sheets are
dirty,
> but only in a shadow scope?

Looking at one of the failing tests, which is about updating style for sibling
rules when dom nodes are removed, what
ContainerNode::checkForSiblingStyleChanges does should be checked. My gut
feeling is that this also fails without shadow dom, only that we don't have a
layout test for it.

[esprehn, 2016-01-29 23:32:55]

On 2016/01/29 at 10:35:58, rune wrote:
> On 2016/01/29 06:51:40, esprehn wrote:
> > On 2016/01/29 at 06:50:09, esprehn wrote:
> > > Any idea why this would be rune@ ?
> > 
> > Actually I bet we just don't handle the case where the pending sheets are
dirty,
> > but only in a shadow scope?
> 
> Looking at one of the failing tests, which is about updating style for sibling
rules when dom nodes are removed, what
ContainerNode::checkForSiblingStyleChanges does should be checked. My gut
feeling is that this also fails without shadow dom, only that we don't have a
layout test for it.

checkForSiblingStyleChanges is only when nodes are added or removed, how would
that impact the bits being checked inside updateLayoutTreeForNodeIfNeeded?

[esprehn, 2016-01-29 23:35:31]

On 2016/01/29 at 23:32:55, esprehn wrote:
> On 2016/01/29 at 10:35:58, rune wrote:
> > On 2016/01/29 06:51:40, esprehn wrote:
> > > On 2016/01/29 at 06:50:09, esprehn wrote:
> > > > Any idea why this would be rune@ ?
> > > 
> > > Actually I bet we just don't handle the case where the pending sheets are
dirty,
> > > but only in a shadow scope?
> > 
> > Looking at one of the failing tests, which is about updating style for
sibling rules when dom nodes are removed, what
ContainerNode::checkForSiblingStyleChanges does should be checked. My gut
feeling is that this also fails without shadow dom, only that we don't have a
layout test for it.
> 
> checkForSiblingStyleChanges is only when nodes are added or removed, how would
that impact the bits being checked inside updateLayoutTreeForNodeIfNeeded?

Ah this would be the Sibling Invalidator marking a sibling for invalidation,
which would impact this element, but updateLayoutTreeForNodeIfNeeded only walks
up the tree. That's looks broken today with the new sibling invalidator, we just
don't have a test for it.

[esprehn, 2016-01-30 01:38:37]

This bug is specific to Shadow DOM, the problem is that we mark the parent as
needing a subtree style recalc in ContainerNode::checkForSiblingStyleChanges
when there's childrenAffectedByIndirectAdjacentRules, but then inside
updateLayoutTreeForNodeIfNeeded the loop does:

        for (const ContainerNode* ancestor =
LayoutTreeBuilderTraversal::parent(*node); ancestor && !needsRecalc; ancestor =
LayoutTreeBuilderTraversal::parent(*ancestor))
            needsRecalc = ancestor->needsStyleRecalc() ||
ancestor->needsStyleInvalidation() || ancestor->needsAdjacentStyleRecalc();

which is going to skip over ShadowRoots since they're not a part of the composed
tree. This means there's a bunch of obscure bugs today where we use
updateLayoutTreeForNodeIfNeeded and miss doing a recalc because of this case.

To fix this you need to talk the composed ancestors *including* the ShadowRoots
looking for the bits.

[esprehn, 2016-01-30 05:26:43]

The CQ bit was checked by esprehn@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-01-30 05:27:13]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1649983003/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1649983003/20001

[commit-bot: I haz the power, 2016-01-30 06:47:18]

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

[commit-bot: I haz the power, 2016-01-30 06:47:19]

Dry run: This issue passed the CQ dry run.

[esprehn, 2016-01-30 23:12:19]

Description was changed from

==========
Remove isInShadowTree special case in Remove the forced layout in
getComputedStyle for elements in Shadow DOM

This came from https://bugs.webkit.org/show_bug.cgi?id=97760 because the patch
walked the parentNode() chain looking for ancestors that need a recalc, which
didn't respect distributions, and the patch also didn't call updateDistribution
so a dirty distribution would cause incorrect results.

In blink the updateLayoutTreeForNodeIfNeeded method does respect shadow dom and
checks the distribution bits so we don't need to unconditionally force a layout.
==========

to

==========
Remove isInShadowTree special case in Remove the forced layout in
getComputedStyle for elements in Shadow DOM

This came from https://bugs.webkit.org/show_bug.cgi?id=97760 because the patch
walked the parentNode() chain looking for ancestors that need a recalc, which
didn't respect distributions, and the patch also didn't call updateDistribution
so a dirty distribution would cause incorrect results.

In blink the updateLayoutTreeForNodeIfNeeded method does respect shadow dom and
checks the distribution bits so we don't need to unconditionally force a layout.

Removing this exposed a bug in updateLayoutTreeForNodeIfNeeded where it was
using the composed tree looking for ancestor dirty bits, but the composed
tree skips over ShadowRoots, and those too can have dirty bits. To fix this
we extend the ParentTraversalDetails object to have the ShadowRoot that is
crossed when traversing.
==========

[esprehn, 2016-01-30 23:12:19]

esprehn@chromium.org changed reviewers:
+ hayato@chromium.org

[esprehn, 2016-01-30 23:13:00]

Okay I fixed it, we need to check ShadowRoots when walking ancestors. This patch
should also
fix a bunch of edge case bugs in anywhere else in the codebase that used
updateLayoutTreeForNodeIfNeeded

[rune, 2016-02-01 09:24:23]

You're adding no tests. Could you add TEST lines for the failing tests of patch
set 1?

https://codereview.chromium.org/1649983003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/1649983003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Document.cpp:1905: static bool
nodeMayNeedStyleRecalc(const ContainerNode& node)
Should we now use anonymous namespace instead of static functions? I couldn't
find anything about it in the Blink coding standard or in "C++11 use in
Chromium".

https://codereview.chromium.org/1649983003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Document.cpp:1935: needsRecalc = needsRecalc
|| nodeMayNeedStyleRecalc(parentDetails.shadowRoot());
How could that happen? Won't every shadow root have a shadow host parent so that
the when we record a shadow root, the ancestor will be the shadow host?

https://codereview.chromium.org/1649983003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/LayoutTreeBuilderTraversal.h (left):

https://codereview.chromium.org/1649983003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/LayoutTreeBuilderTraversal.h:41:
STACK_ALLOCATED();
Is this also WTF_MAKE_NONCOPYABLE? Not that I think it needs to be, just to
express the current use pattern?

https://codereview.chromium.org/1649983003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/LayoutTreeBuilderTraversal.h:51: bool
operator==(const ParentDetails& other)
Are objects of this class ever being compared? Does it make sense to make
operator== delete?

https://codereview.chromium.org/1649983003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/shadow/ComposedTreeTraversal.h (right):

https://codereview.chromium.org/1649983003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/shadow/ComposedTreeTraversal.h:157: static
ContainerNode* traverseParentOrHost(const Node&, ParentTraversalDetails* = 0);
0 -> nullptr?