Element::recalcStyle() overly reattach()-es InsertionPoints.

Element::recalcStyle() overly reattach()-es InsertionPoints.

Before this change, recalcStyle() reattach()-ed InsertinoPoints
even if only inheriting styles were changed. This misbehavior was caused by the fact
that InsertionPoint never have renderers. This change fixes it by telling
recalcStyle() such a special case.

The fix reveals some hidden flaws, which are also fixed:

- ContentDistributor::invalidate()
   It didn't reattach() its children. These children, as well as ones of the host,
   need to be refreshed on invalidation.
- ShadowRoot::attach()
   It should attach() itself if it isn't.
- Element::setNeedsRecalcStyle()
   It should know about node distribution.

TEST=input-color-in-content.html
BUG=234801

[Hajime Morrita, 2013-05-27 10:12:01]

Hi Dimitri could you take a look?
I understand ContentDistributor::invalidate() needs to be less messy.
But I'd like to do the cleanup in separate CLs.

[Hajime Morrita, 2013-05-27 10:18:45]

Adding a glory reviewer

[Hajime Morrita, 2013-05-28 10:01:32]

seems broken :-(
looking...

[dglazkov, 2013-05-28 19:25:04]

Attack of the overly attached recalcStyle! Looking forward to the next patch :)

[dglazkov, 2013-05-28 19:25:36]

+esprehn, who loves overly attached things.

[Hajime Morrita, 2013-05-29 07:21:53]

Fixed broken parts. This should pass LayoutTests.
Actually, the problem is on the intersection of Shadow DOM and Elliott's
territory.

[esprehn, 2013-05-29 08:31:53]

I'd prefer you didn't add these bool vars, checking if you have custom
callbacks is a bit field. There's no reason to cache the value. You also
only check the insertion point once.

On Wednesday, May 29, 2013, wrote:

> Fixed broken parts. This should pass LayoutTests.
> Actually, the problem is on the intersection of Shadow DOM and Elliott's
> territory.
>
>
https://codereview.chromium.**org/15680005/<https://codereview.chromium.org/1...
>

[esprehn, 2013-05-29 08:38:01]

Actually not lgtm. That setAttached is on purpose, it's not fishy. Attached
means everHadStyleRecalc and I'm going to rename it soon. Also we shouldn't
be doing recalc style out of order as in this patch. That breaks certain
selectors. I think we may have already done some of that, but it was wrong
too.

I'm not a fan of adding this big chunk of code to recalcStyle for insertion
points either.

On Wednesday, May 29, 2013, Elliott Sprehn wrote:

> I'd prefer you didn't add these bool vars, checking if you have custom
> callbacks is a bit field. There's no reason to cache the value. You also
> only check the insertion point once.
>
> On Wednesday, May 29, 2013, wrote:
>
>> Fixed broken parts. This should pass LayoutTests.
>> Actually, the problem is on the intersection of Shadow DOM and Elliott's
>> territory.
>>
>>
https://codereview.chromium.**org/15680005/<https://codereview.chromium.org/1...
>>
>

[Hajime Morrita, 2013-05-29 08:44:37]

OK, let me search other approach.

[esprehn, 2013-05-29 16:32:32]

On 2013/05/29 08:44:37, morrita1 wrote:
> OK, let me search other approach.

Btw the reason for the setAttached() call is that we don't run the reattach call
in the above if statement if there is no parent style. This happens if you
appendChild an element to a display none parent (or a descendant of one) that is
already attached. Lazy attach marks the element as needing a recalc and we go
through here but theres no parent style so we fall to the bottom and need to
mark ourself as attached (which now means "had a style recalc". Before my recent
patch lazyAttach set elements as being attached even though they had never been
through attach. When I removed that we now need to set attached here so
descendants of display none things are marked as attached.

[Hajime Morrita, 2013-05-29 23:24:09]

Thanks for explanation, Elliott!
I'm getting the point.

I hope we could eventually kill the attached( or ever styled whatever) flag
by moving guts in overriden attach() into insertedInto() and devirtualize
attach()/detach(),
but it will take long.

[Hajime Morrita, 2013-06-13 00:26:20]

ping?

[esprehn, 2013-06-13 00:50:42]

I'm not sure what more you wanted me to do here?

[Hajime Morrita, 2013-06-13 01:06:44]

What do you think this new approach?

[esprehn, 2013-06-13 02:25:37]

This is too big a hammer, it's going to make changing styles in ShadowRoot's
crazy expensive.

doing div.style.border = '1px' will now cause a forced style recalc on every
element in the containing ShadowRoot, and all other shadow roots all the way up
the tree. In the case of toolkit where the app is in <x-app> I think you might
cause a Force recalcStyle on the entire app. :(

https://codereview.chromium.org/15680005/diff/28001/LayoutTests/platform/chro...
File
LayoutTests/platform/chromium-win/fast/html/details-add-details-child-2-expected.txt
(right):

https://codereview.chromium.org/15680005/diff/28001/LayoutTests/platform/chro...
LayoutTests/platform/chromium-win/fast/html/details-add-details-child-2-expected.txt:7:
RenderBlock (anonymous) at (0,0) size 784x0
Why do we create all these empty anonymous blocks now? That doesn't seem right.

https://codereview.chromium.org/15680005/diff/28001/Source/core/dom/Node.cpp
File Source/core/dom/Node.cpp (right):

https://codereview.chromium.org/15680005/diff/28001/Source/core/dom/Node.cpp#...
Source/core/dom/Node.cpp:844: if (!distribution ||
!distribution->hasInsertionPoint(root))
hasInsertionPoint is static, this doesn't make sense.

https://codereview.chromium.org/15680005/diff/28001/Source/core/dom/Node.cpp#...
Source/core/dom/Node.cpp:849: insertionPoints[i]->setNeedsStyleRecalc();
This is much too aggressive. Now if we setNeedsStyleRecalc a <div> in a
ShadowRoot (ex. an animation is running) we're going to cause Force style
recalcs on every ShadowRoot and all their descendants all the way down the tree.

setNeedsStyleRecalc means Force for myself and all descendants. You don't even
need this setNeedsStyleRecalc for the insertionPoints because of how big a
hammer this is.

[esprehn, 2013-06-13 02:45:40]

https://codereview.chromium.org/15680005/diff/28001/LayoutTests/fast/dom/shad...
File LayoutTests/fast/dom/shadow/input-color-in-content.html (right):

https://codereview.chromium.org/15680005/diff/28001/LayoutTests/fast/dom/shad...
LayoutTests/fast/dom/shadow/input-color-in-content.html:26: }, 0);
Btw, these nested setTimeout calls will be flaky. I don't think you need all the
dragMode or setWindowIsKey stuff either. Nothing ensures that after two set
timeouts the chooser will be shown.

What's the purpose of this test? The bug referenced is about the value of the
color picker, not if the picker is shown. This test passes without your patch.

[Hajime Morrita, 2013-06-13 03:03:36]

> What's the purpose of this test? The bug referenced is about the value of the
> color picker, not if the picker is shown. This test passes without your patch.

Hmm. that's bad. It failed, at least on linux.
The point is that without this patch, the distributed <input> is re-attached
while the style recalculation triggered by window-focus change, which let the
chooser close.

Chooser open/close happens asynchronously, so I need setTimeout there.
Probably i should have retries there.

[Hajime Morrita, 2013-06-14 08:19:02]

Let me have another shot at https://codereview.chromium.org/17054002/