Bug 157374

Summary: slotchange event should be fired at the end of microtask
Product: WebKit Reporter: Ryosuke Niwa <rniwa>
Component: DOMAssignee: Ryosuke Niwa <rniwa>
Status: RESOLVED FIXED    
Severity: Normal CC: cdumez, commit-queue, esprehn+autocc, gyuyoung.kim, kangil.han, koivisto, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 148695    
Attachments:
Description Flags
Fixes the bug
none
Updated per spec change
none
Patch for landing none

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.