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 61 matching lines @@
   if (options.hasFlatten() && options.flatten())
     return GetDistributedNodesForBinding();
   return assigned_nodes_;
 }
 
 const HeapVector<Member<Node>>
 HTMLSlotElement::GetDistributedNodesForBinding() {
   DCHECK(!NeedsDistributionRecalc());
   if (SupportsDistribution())
     return distributed_nodes_;
+  return GetDistributedNodesInNonShadowTree();
+}
 
-  // If a slot does not support distribution, its m_distributedNodes should not
+const HeapVector<Member<Node>>
+HTMLSlotElement::GetDistributedNodesInNonShadowTree() const {
+  // 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)) {
@@ skipping 31 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 =

[hayato, 2017/05/16 04:22:32]

Avoid this temporary vector being allocated. It looks inefficient.

[hayato, 2017/05/16 04:26:44]

Ah, it is a reference, and being used in L145. Ignore this comment.

[kochi, 2017/05/16 06:06:21]

Acknowledged.

+      other.SupportsDistribution() ? other.distributed_nodes_
+                                   : other.GetDistributedNodesInNonShadowTree();

[hayato, 2017/05/16 04:22:32]

As I mentioned before, I prefer more logical name for
GetDistributedNodesInNonShadowTree().

- SupportsDistribution is a logical name, which doesn't leak an internal
condition.
- GetDistributedNodesInNonShadowTree() is not a logical name, which leaks an
internal condition.

Could you rename GetDistributedNodesInNonShadowTree so that it is consistent
with the naming of SupportsDistribution()?

[kochi, 2017/05/16 06:06:21]

I thought "InNonShadowTree()" is logical in that it can be either true or false.
Though I understand that you want more consistency with
"SupportsDistribution()".  

I renamed this to "CollectDistributedNodesFromSlotWithoutDistribution()",
but it went away with the other change.

+  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 217 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