New issue
Advanced search Search tips

Issue 692893 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Jun 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug



Sign in to add a comment

Shadow DOM v1 always does a reattach even when nodes are distributed to the same locations

Project Member Reported by esprehn@chromium.org, Feb 16 2017

Issue description

https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/html/HTMLSlotElement.cpp?l=305

  // TODO(hayato): Figure out an exact condition where reattach is required
  if (m_oldDistributedNodes != m_distributedNodes) {
    for (auto& node : m_oldDistributedNodes)
      node->lazyReattachIfAttached();
    for (auto& node : m_distributedNodes)
      node->lazyReattachIfAttached();
    InspectorInstrumentation::didPerformSlotDistribution(this);
  }
  m_oldDistributedNodes.clear();

we solved this years ago for Shadow DOM v0:

https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/dom/shadow/InsertionPoint.cpp?q=InsertionPoint.cpp+package:%5Echromium$&l=54

This means if you appendChild a new element with the same slot="" we'll reattach all other elements under that element. This is a very large perf regression over shadow dom v0.
 
Description: Show this description

Comment 2 by hayato@chromium.org, Feb 17 2017

Status: Assigned (was: Untriaged)

Comment 3 by hayato@chromium.org, Feb 17 2017

Labels: -Pri-3 Pri-2
Owner: hayato@chromium.org

Comment 4 by hayato@chromium.org, May 10 2017

Status: Started (was: Assigned)

Comment 5 by hayato@chromium.org, May 12 2017

We should do a similar optimization for v1 too. That would be worth trying.
However, it looks the current algorithm for v0 is not excellent.

For example, it reattach all nodes in the following case:

old distributions: a b c d
new distributions: e b c d f

b, c, and d don't have to be reattached in this case.

We might want to apply a better algorithm here, such as a dynamic programming, to minimize the number of reattached nodes.




Comment 6 by hayato@chromium.org, Jun 21 2017

Status: Fixed (was: Started)
This should be fixed by https://chromium-review.googlesource.com/c/535493/.

I forgot to add BUG= line in the CL.

Sign in to add a comment