Introduce Node::needsDistributionRecalc and use it in ElementShadow to fix the wrong assertions.

Introduce Node::needsDistributionRecalc and use it in ElementShadow to fix the wrong assertions.

This prevents a possible ASSERTION hit in ElementShadow::finalDestinationInsertionPointFor() (and one more)
if the given node is not in-a-document.

If a node is not in-a-document, the recalc flag of node.document() is not cleared even when node.updateDistribution() is called.
In other words, the following code is *wrong* if a node is not in-a-document.

> node.updateDistribution();
> ASSERT(!node.document().childNeedsDistributionRecalc()).

ElementShadow's finalInsertionPointsFor can be called for a node which is not in-a-document.

BUG=473000
TEST=Existing Layout tests. No behavior change except for assertion.

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

[hayato, 2015-04-02 07:37:02]

hayato@chromium.org changed reviewers:
+ esprehn@chromium.org, kochi@chromium.org, kojii@chromium.org

[hayato, 2015-04-02 07:37:03]

PTAL

[hayato, 2015-04-02 07:45:07]

+cc: sergeyv@, the original crash reporter.

[kochi, 2015-04-02 08:52:16]

if ASSERT is not enabled, does ElementShadow.cpp compile?

[hayato, 2015-04-02 09:14:20]

On 2015/04/02 08:52:16, Takayoshi Kochi wrote:
> if ASSERT is not enabled, does ElementShadow.cpp compile?

I think it compiles.
needsDistributionRecalc() is only used in ASSERT() in ElementShadow.cpp.

[kochi, 2015-04-02 10:27:10]

It would be nice to add a test that hits the wrong assertion.

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

https://codereview.chromium.org/1058763002/diff/20001/Source/core/dom/Node.cp...
Source/core/dom/Node.cpp:671: // Todo(hayato): If we can prove that
this->childNeedsDistributionRecalc()
nit: s/Todo/TODO/

[sergeyv, 2015-04-06 08:14:33]

sergeyv@chromium.org changed reviewers:
+ sergeyv@chromium.org

[sergeyv, 2015-04-06 08:14:33]

lgtm

[kochi, 2015-04-06 08:30:09]

lgtm that this seems to meet sergeyv's expectation.

[hayato, 2015-04-06 08:32:00]

Let me land this.

Yeah, it would be better to have a test, however, let me skip it this time
because this CL only changes the assertions.

I'll have another CL which changes the condition of relevant assertions after I
address the TODO in this CL. Let me defer to write a test until that time.

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

https://codereview.chromium.org/1058763002/diff/20001/Source/core/dom/Node.cp...
Source/core/dom/Node.cpp:671: // Todo(hayato): If we can prove that
this->childNeedsDistributionRecalc()
On 2015/04/02 10:27:10, Takayoshi Kochi wrote:
> nit: s/Todo/TODO/

Done.

[hayato, 2015-04-06 08:32:23]

The CQ bit was checked by hayato@chromium.org

[hayato, 2015-04-06 08:32:23]

The patchset sent to the CQ was uploaded after l-g-t-m from sergeyv@chromium.org
Link to the patchset: https://codereview.chromium.org/1058763002/#ps40001
(title: "Fix Todo in the comment")

[commit-bot: I haz the power, 2015-04-06 08:32:29]

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

[esprehn, 2015-04-06 09:30:18]

This patch isn't correct, you need to walk up the tree from the node to the
subtree root to know if distribution is needed. See how
updateDistributionForNode works.

To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[commit-bot: I haz the power, 2015-04-06 09:44:33]

Committed patchset #3 (id:40001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=193176

[hayato, 2015-04-06 09:53:10]

On 2015/04/06 09:30:18, esprehn wrote:
> This patch isn't correct, you need to walk up the tree from the node to the
> subtree root to know if distribution is needed. See how
> updateDistributionForNode works.

updateDistributionForNode was renamed to Node::updateDistribution().

Yeah, that's exactly what I had in my mind when I was chatting with sergeyv@. I
tried to assert ASSERT(!rootInTreeOfTrees(node).childNeedsDistributionRecalc())
at first.
After that, unfortunately, we don't have such an assertion in our code base at
all. See ComposedTreeTraversal::assertPrecondition for example.

So this CL doesn't use such an assertion yet. I have a plan to investigate
further, as I left a Todo comment in this CL.

Regarding with that, I have a question:
- If the node itself doesn't have a flag set and the root node of the subtree
has a flag set and, does rootInTreeOfTrees(node)::recalcDistribution really
recalculate the node's distribution?
 Looks it skips the recalculation of the node. If I miss something, please
correct me.

[hayato, 2015-04-06 10:06:58]

Let me make a concrete example which I am worrying about, where the distribution
is not re-calculated.

I should have a concrete example to explain that and verify the behavior.

[esprehn, 2015-04-07 04:45:13]

I'm not sure I understand, recalcDistribution() is only called for elements with
ShadowRoot to update the distribution of insertion points in that scope. A node
having that bit set means something totally different from node.treeScope() (the
root in the tree of trees) having it set.

[hayato, 2015-04-08 10:45:48]

On 2015/04/07 04:45:13, esprehn wrote:
> I'm not sure I understand, recalcDistribution() is only called for elements
with
> ShadowRoot to update the distribution of insertion points in that scope. A
node
> having that bit set means something totally different from node.treeScope()
(the
> root in the tree of trees) having it set.

Thank you. I've just uploaded the follow-up patch in
https://codereview.chromium.org/1068193002/.