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

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

Before this change, recalcStyle() reattach()-ed InsertionPoints
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.

- lazyReattach()-es by ContentDistributor was insufficient.
  It didn't reattach insertion point children. Also, it didnt' use lazyReattachIfAttached()
  and marks not-yet-attached nodes attached.
- InsertionPoint should tell its distribution about style change but it didn't.
- Distributed text nodes didn't recalculate style.
- ShadowRoot::recalcStyle() should just attach() when it isn't yet attached().
- There were some missing ensureDistribution() calls.

This change also fixes these problems.

BUG=234801
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=152516

[Hajime Morrita, 2013-06-14 08:18:33]

Could you take a look?

This is another try of https://codereview.chromium.org/15680005/.
The change based on Elliott's idea that suggests to use
InsertionPoint::willRecalcStyle.

The diff on test expectation isn't actually a regression.
Empty anonymous block problem has been there but 
reattaching() insert hid it.

[esprehn, 2013-06-14 08:32:29]

This looks way better! A bunch of random comments, but with those fixes this
should be fine to land.

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

https://codereview.chromium.org/17054002/diff/1/Source/core/dom/Text.cpp#newc...
Source/core/dom/Text.cpp:282: ~ClearNeedsStyleRecalcScope() {
m_node->clearNeedsStyleRecalc(); }
I'd prefer you just put the clearNeedsStyleRecalc() calls in the function
itself.

https://codereview.chromium.org/17054002/diff/1/Source/core/dom/Text.cpp#newc...
Source/core/dom/Text.cpp:287: void Text::recalcTextStyle(StyleChange change)
I don't understand why you changed this function, your new code appears to do
the exact same thing.

https://codereview.chromium.org/17054002/diff/1/Source/core/dom/Text.cpp#newc...
Source/core/dom/Text.cpp:301: return;
Remove this return and make the below if () an else if and then you can put the
clearNeedsStyleRecalc() call as the last line of the function.

https://codereview.chromium.org/17054002/diff/1/Source/core/dom/shadow/Conten...
File Source/core/dom/shadow/ContentDistributor.cpp (right):

https://codereview.chromium.org/17054002/diff/1/Source/core/dom/shadow/Conten...
Source/core/dom/shadow/ContentDistributor.cpp:354: void
ContentDistributor::ensureDistribution(ElementShadow* shadow)
I'd prefer if this was just a method of Element itself.

element->ensureDistribtion(). Internally it can check for shadow() and such. All
the overloads make this confusing.

https://codereview.chromium.org/17054002/diff/1/Source/core/dom/shadow/Conten...
Source/core/dom/shadow/ContentDistributor.cpp:378: Vector<RefPtr<Node> >
nodesNeedingReattach;
These shouldn't need to be RefPtr. I don't think anything below can run script?
If it can then then Node* isn't safe.

https://codereview.chromium.org/17054002/diff/1/Source/core/dom/shadow/Conten...
Source/core/dom/shadow/ContentDistributor.cpp:451:
const_cast<Node*>(i->key)->setNeedsStyleRecalc(SyntheticStyleChange);
Can you just remove the const from the key in the NodeInsertionPointMap type? It
seems unnecessarily restrictive.

Also lets try InlineStyleChange instead of SyntheticStyleChange. I don't think
you need to trigger RenderLayer updates for identical styles.

https://codereview.chromium.org/17054002/diff/1/Source/core/dom/shadow/Insert...
File Source/core/dom/shadow/InsertionPoint.cpp (right):

https://codereview.chromium.org/17054002/diff/1/Source/core/dom/shadow/Insert...
Source/core/dom/shadow/InsertionPoint.cpp:83:
ContentDistributor::ensureDistribution(shadowRoot);
Why not call shadowRoot->host() here and avoid the overload?

[Hajime Morrita, 2013-06-14 09:59:14]

Thanks for quick review, Elliott! The revised one will come shortly.

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

https://codereview.chromium.org/17054002/diff/1/Source/core/dom/Text.cpp#newc...
Source/core/dom/Text.cpp:282: ~ClearNeedsStyleRecalcScope() {
m_node->clearNeedsStyleRecalc(); }
On 2013/06/14 08:32:29, esprehn wrote:
> I'd prefer you just put the clearNeedsStyleRecalc() calls in the function
> itself.

Done.

https://codereview.chromium.org/17054002/diff/1/Source/core/dom/Text.cpp#newc...
Source/core/dom/Text.cpp:287: void Text::recalcTextStyle(StyleChange change)
There is a case there change == NoChange but needsStyleRecalc() is true.
The old code doesn't ask to the resolver in that case. New code does.

https://codereview.chromium.org/17054002/diff/1/Source/core/dom/Text.cpp#newc...
Source/core/dom/Text.cpp:301: return;
On 2013/06/14 08:32:29, esprehn wrote:
> Remove this return and make the below if () an else if and then you can put
the
> clearNeedsStyleRecalc() call as the last line of the function.

Done.

https://codereview.chromium.org/17054002/diff/1/Source/core/dom/shadow/Conten...
File Source/core/dom/shadow/ContentDistributor.cpp (right):

https://codereview.chromium.org/17054002/diff/1/Source/core/dom/shadow/Conten...
Source/core/dom/shadow/ContentDistributor.cpp:354: void
ContentDistributor::ensureDistribution(ElementShadow* shadow)
makes sense. will do.

https://codereview.chromium.org/17054002/diff/1/Source/core/dom/shadow/Conten...
Source/core/dom/shadow/ContentDistributor.cpp:378: Vector<RefPtr<Node> >
nodesNeedingReattach;
On 2013/06/14 08:32:29, esprehn wrote:
> These shouldn't need to be RefPtr. I don't think anything below can run
script?
It shouldn't run any script. WIll turn to raw pointer.

[Hajime Morrita, 2013-06-14 10:02:20]

uploaded.

[esprehn, 2013-06-14 22:22:10]

LGTM. Please remove the unused host argument before landing.

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

https://codereview.chromium.org/17054002/diff/8001/LayoutTests/platform/chrom...
LayoutTests/platform/chromium-win/fast/html/details-add-details-child-2-expected.txt:7:
RenderBlock (anonymous) at (0,0) size 784x0
I think this might not happen anymore now that the other recent patch to clean
up anonymous blocks landed.

https://codereview.chromium.org/17054002/diff/8001/Source/core/dom/shadow/Con...
File Source/core/dom/shadow/ContentDistributor.cpp (right):

https://codereview.chromium.org/17054002/diff/8001/Source/core/dom/shadow/Con...
Source/core/dom/shadow/ContentDistributor.cpp:366: Vector<Node*>
nodesNeedingReattach;
I'd probably give this an inline capacity like the 8> above for elementShadows.

https://codereview.chromium.org/17054002/diff/8001/Source/core/dom/shadow/Con...
Source/core/dom/shadow/ContentDistributor.cpp:435: void
ContentDistributor::setNeedsStyleRecalcIfDistributedTo(Element* host,
InsertionPoint* insertionPoint)
host is unused, please remove.

[Hajime Morrita, 2013-06-17 01:58:23]

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

https://codereview.chromium.org/17054002/diff/8001/LayoutTests/platform/chrom...
LayoutTests/platform/chromium-win/fast/html/details-add-details-child-2-expected.txt:7:
RenderBlock (anonymous) at (0,0) size 784x0
Great! Will update the result.

https://codereview.chromium.org/17054002/diff/8001/Source/core/dom/shadow/Con...
File Source/core/dom/shadow/ContentDistributor.cpp (right):

https://codereview.chromium.org/17054002/diff/8001/Source/core/dom/shadow/Con...
Source/core/dom/shadow/ContentDistributor.cpp:366: Vector<Node*>
nodesNeedingReattach;
On 2013/06/14 22:22:10, esprehn wrote:
> I'd probably give this an inline capacity like the 8> above for
elementShadows.

Done.

https://codereview.chromium.org/17054002/diff/8001/Source/core/dom/shadow/Con...
Source/core/dom/shadow/ContentDistributor.cpp:435: void
ContentDistributor::setNeedsStyleRecalcIfDistributedTo(Element* host,
InsertionPoint* insertionPoint)
On 2013/06/14 22:22:10, esprehn wrote:
> host is unused, please remove.

Done.

[commit-bot: I haz the power, 2013-06-17 01:58:49]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/morrita@chromium.org/17054002/16001

[commit-bot: I haz the power, 2013-06-17 03:30:14]

Change committed as 152516