Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1695163003:
Support slotchange events (Closed)
|
Created:
4 years, 10 months ago by hayato Modified:
4 years, 10 months ago CC:
blink-reviews, blink-reviews-dom_chromium.org, blink-reviews-html_chromium.org, chromium-reviews, dglazkov+blink, eae+blinkwatch, rwlbuis, sof, webcomponents-bugzilla_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionSupport slotchange events
This is a proof of concept to support *slotchange* events at the end of a microtask.
The spec discussion is here: https://github.com/w3c/webcomponents/issues/288
Because slot elements are guarded by ShadowDOMV1 flag, this new events will be seen only when the flag is enabled.
BUG=531990
Committed: https://crrev.com/f277728808e60a8b6e7fa9d562bfc3c02501de7d
Cr-Commit-Position: refs/heads/master@{#376090}
Patch Set 1 #Patch Set 2 : wip #Patch Set 3 : rebased #Patch Set 4 : rebased #
Total comments: 6
Patch Set 5 : Use PassRefPtr #Patch Set 6 : minor #
Total comments: 6
Messages
Total messages: 45 (16 generated)
The CQ bit was checked by hayato@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1695163003/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1695163003/1
Description was changed from ========== WIP. Support slotchange events. This is a proof of concept to support *slotchange* events at the end of a microtask. The spec discussion is here: https://github.com/w3c/webcomponents/issues/288 BUG=531990 ========== to ========== Support slotchange events. This is a proof of concept to support *slotchange* events at the end of a microtask. The spec discussion is here: https://github.com/w3c/webcomponents/issues/288 Because slot elements are guarded by ShadowDOMV1 flag, this new events will be seen only when the flag is enabled. BUG=531990 ==========
The CQ bit was checked by hayato@chromium.org to run a CQ dry run
wip
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1695163003/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1695163003/20001
The CQ bit was checked by hayato@chromium.org to run a CQ dry run
rebased
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1695163003/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1695163003/40001
The CQ bit was checked by hayato@chromium.org to run a CQ dry run
rebased
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1695163003/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1695163003/60001
hayato@chromium.org changed reviewers: + kochi@chromium.org, kouhei@chromium.org
PTAL kouhei@, could you check the usage of a microtask in this CL? I am not familiar with a microtask.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
kouhei@chromium.org changed reviewers: + yhirano@chromium.org
Sorry I caught flu now. yhirano: Would you take a look at microtask usage?
https://codereview.chromium.org/1695163003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/Node.cpp (right): https://codereview.chromium.org/1695163003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/Node.cpp:697: Microtask::enqueueMicrotask(WTF::bind(&Document::updateDistribution, &document())); WTF::bind stores an off-heap pointer as is. Shouldn't we store PassRefPtrWillBeRawPtr instead? https://codereview.chromium.org/1695163003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/shadow/SlotAssignment.cpp (right): https://codereview.chromium.org/1695163003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/shadow/SlotAssignment.cpp:79: for (auto slot : slots) const auto& https://codereview.chromium.org/1695163003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/HTMLSlotElement.cpp (right): https://codereview.chromium.org/1695163003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLSlotElement.cpp:240: Microtask::enqueueMicrotask(WTF::bind(&HTMLSlotElement::dispatchSlotChangeEvent, this)); ditto
Description was changed from ========== Support slotchange events. This is a proof of concept to support *slotchange* events at the end of a microtask. The spec discussion is here: https://github.com/w3c/webcomponents/issues/288 Because slot elements are guarded by ShadowDOMV1 flag, this new events will be seen only when the flag is enabled. BUG=531990 ========== to ========== Support slotchange events This is a proof of concept to support *slotchange* events at the end of a microtask. The spec discussion is here: https://github.com/w3c/webcomponents/issues/288 Because slot elements are guarded by ShadowDOMV1 flag, this new events will be seen only when the flag is enabled. BUG=531990 ==========
The CQ bit was checked by hayato@chromium.org to run a CQ dry run
Use PassRefPtr
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1695163003/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1695163003/80001
The CQ bit was checked by hayato@chromium.org to run a CQ dry run
minor
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1695163003/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1695163003/100001
Thank you for the review. https://codereview.chromium.org/1695163003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/Node.cpp (right): https://codereview.chromium.org/1695163003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/Node.cpp:697: Microtask::enqueueMicrotask(WTF::bind(&Document::updateDistribution, &document())); On 2016/02/17 05:23:12, yhirano wrote: > WTF::bind stores an off-heap pointer as is. Shouldn't we store > PassRefPtrWillBeRawPtr instead? Good point. Done. Could you check again whether I understand the point correctly? https://codereview.chromium.org/1695163003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/shadow/SlotAssignment.cpp (right): https://codereview.chromium.org/1695163003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/shadow/SlotAssignment.cpp:79: for (auto slot : slots) On 2016/02/17 05:23:12, yhirano wrote: > const auto& Done. https://codereview.chromium.org/1695163003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/HTMLSlotElement.cpp (right): https://codereview.chromium.org/1695163003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLSlotElement.cpp:240: Microtask::enqueueMicrotask(WTF::bind(&HTMLSlotElement::dispatchSlotChangeEvent, this)); On 2016/02/17 05:23:12, yhirano wrote: > ditto Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
microtask related part lgtm (I haven't read the spec discussion).
https://codereview.chromium.org/1695163003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/HTMLSlotElement.cpp (right): https://codereview.chromium.org/1695163003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLSlotElement.cpp:108: m_oldDistributedNodes.swap(m_distributedNodes); Is there any possibility that clearDistribution() is called twice without updateDistribution() in between?
https://codereview.chromium.org/1695163003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/HTMLSlotElement.cpp (right): https://codereview.chromium.org/1695163003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLSlotElement.cpp:108: m_oldDistributedNodes.swap(m_distributedNodes); On 2016/02/18 03:40:56, kochi wrote: > Is there any possibility that clearDistribution() is called twice without > updateDistribution() in between? It could happen in the future. I'm aware the current approach is very weak to the future change. See TODO comment here: https://codereview.chromium.org/1695163003/diff/100001/third_party/WebKit/Sou... I have a plan to use RAAI approach to get rid of m_oldDistributedNodes.
On 2016/02/18 03:51:56, hayato wrote: > https://codereview.chromium.org/1695163003/diff/100001/third_party/WebKit/Sou... > File third_party/WebKit/Source/core/html/HTMLSlotElement.cpp (right): > > https://codereview.chromium.org/1695163003/diff/100001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/html/HTMLSlotElement.cpp:108: > m_oldDistributedNodes.swap(m_distributedNodes); > On 2016/02/18 03:40:56, kochi wrote: > > Is there any possibility that clearDistribution() is called twice without > > updateDistribution() in between? > > It could happen in the future. I'm aware the current approach is very weak to > the future change. > See TODO comment here: > https://codereview.chromium.org/1695163003/diff/100001/third_party/WebKit/Sou... > > I have a plan to use RAAI approach to get rid of m_oldDistributedNodes. I see. Let's land this for now to see how useful this is. lgtm BTW s/RAAI/RAII/
The CQ bit was checked by hayato@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1695163003/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1695163003/100001
Message was sent while issue was closed.
Description was changed from ========== Support slotchange events This is a proof of concept to support *slotchange* events at the end of a microtask. The spec discussion is here: https://github.com/w3c/webcomponents/issues/288 Because slot elements are guarded by ShadowDOMV1 flag, this new events will be seen only when the flag is enabled. BUG=531990 ========== to ========== Support slotchange events This is a proof of concept to support *slotchange* events at the end of a microtask. The spec discussion is here: https://github.com/w3c/webcomponents/issues/288 Because slot elements are guarded by ShadowDOMV1 flag, this new events will be seen only when the flag is enabled. BUG=531990 ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== Support slotchange events This is a proof of concept to support *slotchange* events at the end of a microtask. The spec discussion is here: https://github.com/w3c/webcomponents/issues/288 Because slot elements are guarded by ShadowDOMV1 flag, this new events will be seen only when the flag is enabled. BUG=531990 ========== to ========== Support slotchange events This is a proof of concept to support *slotchange* events at the end of a microtask. The spec discussion is here: https://github.com/w3c/webcomponents/issues/288 Because slot elements are guarded by ShadowDOMV1 flag, this new events will be seen only when the flag is enabled. BUG=531990 Committed: https://crrev.com/f277728808e60a8b6e7fa9d562bfc3c02501de7d Cr-Commit-Position: refs/heads/master@{#376090} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/f277728808e60a8b6e7fa9d562bfc3c02501de7d Cr-Commit-Position: refs/heads/master@{#376090}
Message was sent while issue was closed.
esprehn@chromium.org changed reviewers: + esprehn@chromium.org
Message was sent while issue was closed.
This should be behind a flag, as it stands it's just a performance regression causing us to force distributions after every microtask, and causing microtask floods when you dispatch events inside microtask handlers. Please revert this. https://codereview.chromium.org/1695163003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/Node.cpp (right): https://codereview.chromium.org/1695163003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/Node.cpp:697: Microtask::enqueueMicrotask(WTF::bind(&Document::updateDistribution, PassRefPtrWillBeRawPtr<Document>(&document()))); this means we force a distribution after every DOM mutation even if no one is listening which is just wasted battery. Also this seems pretty bad, it means if you do: 1) micro task that mutates the dom 2) microtask that dispatches an event (causes distribution), then mutates the dom 3) microtask that dispatches another event (causes distribution), then mutates the dom 4) now run *3* updateDistribution microtasks. https://codereview.chromium.org/1695163003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/HTMLSlotElement.cpp (right): https://codereview.chromium.org/1695163003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLSlotElement.cpp:115: return eventTargetData() && eventTargetData()->eventListenerMap.find(EventTypeNames::slotchange); this is not correct per the semantics of events, you should be able to do: mutateDom(); Promise.resolve().then(() => { dom.addEventListener("slotchanged", () => { // this should get called. }); }); so you can't skip the microtask step to fire the event, the event must "always happen", even if no one is listening. this means events are a bad fit, it forces you to burn battery every time there's a mutation even if no one is listening. https://codereview.chromium.org/1695163003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLSlotElement.cpp:240: Microtask::enqueueMicrotask(WTF::bind(&HTMLSlotElement::dispatchSlotChangeEvent, PassRefPtrWillBeRawPtr<HTMLSlotElement>(this))); the de-duplication here is just reinventing what MutationObserver already does :(
Message was sent while issue was closed.
https://codereview.chromium.org/1695163003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/Node.cpp (right): https://codereview.chromium.org/1695163003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/Node.cpp:697: Microtask::enqueueMicrotask(WTF::bind(&Document::updateDistribution, PassRefPtrWillBeRawPtr<Document>(&document()))); This is also adding an extra microtask every time an input element or other thing with a UA shadowroot is inserted since all of those also mark the document for distribution. ex. .appendChild(input); .dispatchEvent(...); .appendChild(input); .dispatchEvent(...); .appendChild(input); .dispatchEvent(...); .appendChild(input); // Now 3 no-op Document::updateDistribution microtasks run. This grows the microtask list unbounded if you interleave creating elements that use shadow roots and doing anything that forces distribution (events, getComputedStyle, layout, ...) That's actually exactly what <iron-list> in polymer does, they appendChild and then measure this means your patch causes an extra 10 microtasks on the chrome downloads page.
Message was sent while issue was closed.
> This should be behind a flag, as it stands it's just a performance regression > causing us to force distributions after every microtask I agree. Let me guard the enqueuing part by the flag. After that, this CL should not increate the number of distribution recalc and microtasks unless the flag is enabled. Does it sound good? > and causing microtask > floods when you dispatch events inside microtask handlers. Can MutationObserver avoid this issue? I'm wondering What's difference between this and MutationObserver. I am aware that this increases the number of microtasks. That's the reason I have expressed a strong concern many times in the spec discussion. See https://github.com/w3c/webcomponents/issues/288 > this means we force a distribution after every DOM mutation even if no one is listening which is just wasted battery. Yeah, that's TODO item as I added a comment in this CL. I'll work on this later. After I finish a TODO item, I'm wondering how we should move this feature request forward. > That's actually exactly what <iron-list> in polymer does, they appendChild and > then measure this means your patch causes an extra 10 microtasks on the chrome > downloads page. I can somehow omit an enqueuing a microtask if I can predict slot's distributed nodes will not change even if the distribution flag is dirty. That's yet another my TODO item. If we can not avoid this, what's your suggestion? Do you think we should not support this feature request? Or we should have yet another approach, instead of using an event? If you have any comment for the feature request itself, I'd appreciate if you comment the spec issue: https://github.com/w3c/webcomponents/issues/288
Message was sent while issue was closed.
As per the spec discussion, let me try a mutation observer approach in another CL. I'd like to compare the performance between them.
Message was sent while issue was closed.
FYI. The patch to make the microtask be behind the flag is here: https://codereview.chromium.org/1715623002/ |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
