Bug 157374 - slotchange event should be fired at the end of microtask
Summary: slotchange event should be fired at the end of microtask
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Ryosuke Niwa
URL:
Keywords: InRadar
Depends on:
Blocks: 148695
  Show dependency treegraph
 
Reported: 2016-05-05 09:36 PDT by Ryosuke Niwa
Modified: 2016-06-09 02:08 PDT (History)
7 users (show)

See Also:


Attachments
Fixes the bug (17.08 KB, patch)
2016-06-08 21:03 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Updated per spec change (16.78 KB, patch)
2016-06-09 00:34 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Patch for landing (16.88 KB, patch)
2016-06-09 01:36 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ryosuke Niwa 2016-05-05 09:36:44 PDT
slotchange event as spec'ed is not completely async:
https://github.com/whatwg/dom/pull/229

Match the spec'ed behavior of firing at the end of a micro task.
Comment 1 Radar WebKit Bug Importer 2016-05-06 19:34:08 PDT
<rdar://problem/26154024>
Comment 2 Ryosuke Niwa 2016-06-08 21:03:56 PDT
Created attachment 280880 [details]
Fixes the bug
Comment 3 Ryosuke Niwa 2016-06-09 00:34:04 PDT
Created attachment 280891 [details]
Updated per spec change
Comment 4 Antti Koivisto 2016-06-09 01:31:44 PDT
Comment on attachment 280891 [details]
Updated per spec change

View in context: https://bugs.webkit.org/attachment.cgi?id=280891&action=review

> Source/WebCore/dom/MutationObserver.cpp:189
> +void MutationObserver::enqueueSlotChangeEvent(HTMLSlotElement& slot)
> +{
> +    ASSERT(isMainThread());
> +    ASSERT(!signalSlotList().contains(&slot));
> +    signalSlotList().append(&slot);

I think the code would read better if signalSlotList was called something more descriptive like slotsWithEnqueuedChangeEvents()

> Source/WebCore/html/HTMLSlotElement.h:56
> +    bool m_inSignalSlotList { false };

Similarly m_hasEnqueuedChangeEvent maybe
Comment 5 Ryosuke Niwa 2016-06-09 01:35:19 PDT
(In reply to comment #4)
> Comment on attachment 280891 [details]
> Updated per spec change
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=280891&action=review
> 
> > Source/WebCore/dom/MutationObserver.cpp:189
> > +void MutationObserver::enqueueSlotChangeEvent(HTMLSlotElement& slot)
> > +{
> > +    ASSERT(isMainThread());
> > +    ASSERT(!signalSlotList().contains(&slot));
> > +    signalSlotList().append(&slot);
> 
> I think the code would read better if signalSlotList was called something
> more descriptive like slotsWithEnqueuedChangeEvents()
> 
> > Source/WebCore/html/HTMLSlotElement.h:56
> > +    bool m_inSignalSlotList { false };
> 
> Similarly m_hasEnqueuedChangeEvent maybe

signal slot list is the name spec uses: https://dom.spec.whatwg.org/#signal-slot-list
Will add the URL as comments.
Comment 6 Ryosuke Niwa 2016-06-09 01:36:40 PDT
Created attachment 280898 [details]
Patch for landing
Comment 7 WebKit Commit Bot 2016-06-09 02:08:21 PDT
Comment on attachment 280898 [details]
Patch for landing

Clearing flags on attachment: 280898

Committed r201858: <http://trac.webkit.org/changeset/201858>
Comment 8 WebKit Commit Bot 2016-06-09 02:08:26 PDT
All reviewed patches have been landed.  Closing bug.