Node::assignedSlot should call updateDistribution |
|||
Issue descriptionIt's not correct to skip the updateDistribution call inside assignedSlot, that means code is reading the stale slots and getting the wrong answer. updateDistribution is a single branch if distribution is not dirty and should be fine to call here. There's some vague discussion here: https://bugs.chromium.org/p/chromium/issues/detail?id=584596 but it doesn't make sense that there's a penalty to calling updateDistribution(). If it's slow that means distribution was dirty and there was work that must be done to not get the wrong answer.
,
Dec 6 2016
I do not think assignedNode() should call updateDistribution because it might cause the *abuse* of updateDistribution(). Other callers of assignedNodes() does not rely on that. They are calling updateDistribution() at more early stages, I think. I do not have much concern about the performance penalty of updateDistribution(). Rather, my concern is that we should have a clear distribution status management. If we allow calling updateDistribution() at arbitrary places, casually, it is difficult to have a good assumption where distribution is updated. If we can updateDistribution at an earlier stage at a proper timing, we should do it.
,
Dec 6 2016
The function should assert that distribution is clean then? It needs to either assert or call it.
,
Dec 6 2016
Ah, you meant Node::assignedSlot(). I thought you meant HTMLSlotElement::assignedNodes(). Sorry. Please forget comment #2. Regarding Node::assignedSlot (and Node::assignedSlotForBinding), it no longer requires updated *distribution*. I rewrote distribution logic when I introduced slotchange events. A slotname-to-slot map in each treescope is *dynamically* updated on DOM mutation timing. Now we can remove assignedSlotForBling().
,
Dec 9 2016
,
Dec 9 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/5277d3159e5a5ca75fb1306167d6356b99c97e32 commit 5277d3159e5a5ca75fb1306167d6356b99c97e32 Author: hayato <hayato@chromium.org> Date: Fri Dec 09 09:45:02 2016 Do not call updateDistribution() in Node::assingedSlotForBinding() updateDistribution() is no longer required here since slotname-to-slot hashmap is dynamically updated at the timing of DOM mutations. BUG= 671311 Review-Url: https://codereview.chromium.org/2559373003 Cr-Commit-Position: refs/heads/master@{#437505} [modify] https://crrev.com/5277d3159e5a5ca75fb1306167d6356b99c97e32/third_party/WebKit/Source/core/dom/Node.cpp
,
Dec 9 2016
Fixed as "Do not call updateDistribution". |
|||
►
Sign in to add a comment |
|||
Comment 1 by hayato@chromium.org
, Dec 6 2016Status: Assigned (was: Untriaged)