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

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

 /*
  * Copyright (C) 2015 Google Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions are
  * met:
  *
  *     * Redistributions of source code must retain the above copyright
  * notice, this list of conditions and the following disclaimer.
  *     * Redistributions in binary form must reproduce the above
@@ skipping 52 matching lines @@
 const HeapVector<Member<Node>>& HTMLSlotElement::AssignedNodes() {
   DCHECK(!NeedsDistributionRecalc());
   DCHECK(IsInShadowTree() || assigned_nodes_.IsEmpty());
   return assigned_nodes_;
 }
 
 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;
   Node* child = NodeTraversal::FirstChild(*this);
   while (child) {
     if (!child->IsSlotable()) {
       child = NodeTraversal::NextSkippingChildren(*child, this);
       continue;
     }
     if (isHTMLSlotElement(child)) {
       child = NodeTraversal::Next(*child, this);
     } else {
       distributed_nodes.push_back(child);
       child = NodeTraversal::NextSkippingChildren(*child, this);
     }
   }
   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_;
 }
 
 void HTMLSlotElement::AppendAssignedNode(Node& host_child) {
   DCHECK(host_child.IsSlotable());
   assigned_nodes_.push_back(&host_child);
 }
 
@@ skipping 11 matching lines @@
 }
 
 void HTMLSlotElement::AppendDistributedNode(Node& node) {
   size_t size = distributed_nodes_.size();
   distributed_nodes_.push_back(&node);
   distributed_indices_.Set(&node, size);
 }
 
 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++);
 }
 
 void HTMLSlotElement::ClearDistribution() {
   // TODO(hayato): Figure out when to call
   // lazyReattachDistributedNodesIfNeeded()
   assigned_nodes_.clear();
   distributed_nodes_.clear();
   distributed_indices_.clear();
 }
@@ skipping 229 matching lines @@
 
 DEFINE_TRACE(HTMLSlotElement) {
   visitor->Trace(assigned_nodes_);
   visitor->Trace(distributed_nodes_);
   visitor->Trace(old_distributed_nodes_);
   visitor->Trace(distributed_indices_);
   HTMLElement::Trace(visitor);
 }
 
 }  // namespace blink