Fix assignedNodes({flatten:true}) to return fallback content in document tree

third_party/WebKit/Source/core/html/HTMLSlotElement.cpp

Index: third_party/WebKit/Source/core/html/HTMLSlotElement.cpp
diff --git a/third_party/WebKit/Source/core/html/HTMLSlotElement.cpp b/third_party/WebKit/Source/core/html/HTMLSlotElement.cpp
index 12eec94cb5b617d39bc22179f0aaa73672cff0c4..68aa37076fe74a3706707f3f5cf2b098ed8f92ab 100644
--- a/third_party/WebKit/Source/core/html/HTMLSlotElement.cpp
+++ b/third_party/WebKit/Source/core/html/HTMLSlotElement.cpp
@@ -70,17 +70,14 @@ const HeapVector<Member<Node>> HTMLSlotElement::assignedNodesForBinding(
     const AssignedNodesOptions& options) {
   UpdateDistribution();
   if (options.hasFlatten() && options.flatten())
-    return GetDistributedNodesForBinding();
+    return GetDistributedNodes();
   return assigned_nodes_;
 }
 
-const HeapVector<Member<Node>>
-HTMLSlotElement::GetDistributedNodesForBinding() {
-  DCHECK(!NeedsDistributionRecalc());

[hayato, 2017/05/18 06:58:25]

Don't remove DCHECK(). DCHECK() is important and shouldn't be removed without a
reason.

If there is some code path which doesn't meet this condition, please fix the
caller, instead of removing DCHECK().

At least, I suggest to explain what can justify removing DCHECK(), so that we
can know the reason why DCHECK() was removed.

[kochi, 2017/05/18 07:21:49]

The only user of GetDistributedNodesForBinding() was from
assignedNodesForBinding() previously, though the interface
was public so something could call it, so DCHECK() made sense.

Now this interface became private and called from two places:
one from the same place, assignedNodesForBinding() as before,
just after UpdateDistribution(). The other from
AppendDistributedNodesFrom(), which are supposed to be called
*during* distribution update, so DCHECK(!NeedsDistribution())
will fail. 

I deliberately removed DCHECK() here because the existing
call path already guarantee that distribution is updated,
and it should not DHCECK() for the new call path.

[hayato, 2017/05/19 04:05:36]

Okay. Since it is now private, it can be acceptable.
Though I still prefer DCHECK() as much as possible, with some kind of
refactoring, we can tackle this issue as another issue.

+const HeapVector<Member<Node>> HTMLSlotElement::GetDistributedNodes() const {
   if (SupportsDistribution())
     return distributed_nodes_;
-
-  // If a slot does not support distribution, its m_distributedNodes should not
+  // If a slot does not support distribution, its distributed_nodes_ should not
   // be used.  Instead, calculate distribution manually here. This happens only
   // in a slot in non-shadow trees, so its assigned nodes are always empty.
   HeapVector<Member<Node>> distributed_nodes;
@@ -100,7 +97,8 @@ HTMLSlotElement::GetDistributedNodesForBinding() {
   return distributed_nodes;
 }
 
-const HeapVector<Member<Node>>& HTMLSlotElement::GetDistributedNodes() {
+const HeapVector<Member<Node>>&
+HTMLSlotElement::GetDistributedNodesExcludingFallback() {

[hayato, 2017/05/18 06:58:25]

GetDistributedNodesExcludingFallback doesn't sound a good name to me.
It is likely to exclude some elements from the current distributed nodes.  e.g
set difference.
Here, we are returning distributed_nodes_ itself, not excluding any element from
there.

[kochi, 2017/05/18 07:21:49]

Hmm, then |distributed_nodes_| is a misnomer.

I'm not really sure the benefit of not adding fallback content
to distributed_nodes_ and recalculate on demand. I thought it was
done for some optimization - do you remember why? It isn't obvious
to me.

[hayato, 2017/05/19 04:05:36]

We are adding fallback content to distributed_nodes_. See
UpdateDistributedNodesWithFallback

[kochi, 2017/05/19 04:14:21]

Ah, sorry. I was misunderstanding.
Let me rework.

   DCHECK(!NeedsDistributionRecalc());
   DCHECK(SupportsDistribution() || distributed_nodes_.IsEmpty());
   return distributed_nodes_;
@@ -132,8 +130,12 @@ void HTMLSlotElement::AppendDistributedNode(Node& node) {
 
 void HTMLSlotElement::AppendDistributedNodesFrom(const HTMLSlotElement& other) {
   size_t index = distributed_nodes_.size();
-  distributed_nodes_.AppendVector(other.distributed_nodes_);
-  for (const auto& node : other.distributed_nodes_)
+
+  const HeapVector<Member<Node>>& other_distributed_nodes =
+      other.GetDistributedNodes();
+  distributed_nodes_.AppendVector(other_distributed_nodes);
+
+  for (const auto& node : other_distributed_nodes)
     distributed_indices_.Set(node.Get(), index++);
 }