Bug 155424

Summary: Add slotchange event
Product: WebKit Reporter: Ryosuke Niwa <rniwa>
Component: DOMAssignee: Ryosuke Niwa <rniwa>
Status: RESOLVED FIXED    
Severity: Normal CC: cdumez, cmarcelo, commit-queue, esprehn+autocc, gyuyoung.kim, kangil.han, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 148695    
Attachments:
Description Flags
WIP
none
Implements the feature
none
Updated a change log
none
Patch for landing none

Description Ryosuke Niwa 2016-03-13 23:33:20 PDT 
Add the support for slotchange event as discussed in https://github.com/w3c/webcomponents/issues/288.
Comment 1 Ryosuke Niwa 2016-03-13 23:34:06 PDT 
<rdar://problem/24997534>
Comment 2 Ryosuke Niwa 2016-03-14 00:32:39 PDT 
Created attachment 273933 [details]
WIP
Comment 3 Ryosuke Niwa 2016-03-14 01:56:15 PDT 
Created attachment 273943 [details]
Implements the feature
Comment 4 Ryosuke Niwa 2016-03-14 02:01:16 PDT 
Created attachment 273944 [details]
Updated a change log
Comment 5 Antti Koivisto 2016-03-14 02:24:38 PDT 
Comment on attachment 273944 [details]
Updated a change log

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

> Source/WebCore/dom/ShadowRoot.h:97
> +    void hostChildElementDidChangeSlotAttr(const AtomicString& oldValue, const AtomicString& newValue);

Attr -> Attribute

> Source/WebCore/dom/SlotAssignment.cpp:131
> +static void recursivelyFireSlotChangeEvent(HTMLSlotElement* slotElement)

HTMLSlotElement&

> Source/WebCore/dom/SlotAssignment.cpp:136
> +    if (auto* slotParent = slotElement->parentElement()) {
> +        if (auto* shadowRootOfSlotParent = slotParent->shadowRoot())

I don't understand this. Why not just slot elements shadow root? Real element parent of a slot shouldn't have any special status.

If this is valid please add comment explaining it.

This could use early returns.
Comment 6 Ryosuke Niwa 2016-03-14 03:01:40 PDT 
Comment on attachment 273944 [details]
Updated a change log

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

Thanks for the review!

>> Source/WebCore/dom/ShadowRoot.h:97
>> +    void hostChildElementDidChangeSlotAttr(const AtomicString& oldValue, const AtomicString& newValue);
> 
> Attr -> Attribute

Fixed.

>> Source/WebCore/dom/SlotAssignment.cpp:131
>> +static void recursivelyFireSlotChangeEvent(HTMLSlotElement* slotElement)
> 
> HTMLSlotElement&

Fixed.

>> Source/WebCore/dom/SlotAssignment.cpp:136
>> +        if (auto* shadowRootOfSlotParent = slotParent->shadowRoot())
> 
> I don't understand this. Why not just slot elements shadow root? Real element parent of a slot shouldn't have any special status.
> 
> If this is valid please add comment explaining it.
> 
> This could use early returns.

This is the case in which slotElement's parent has a shadow root
and slotElement is assigned to a slot in that shadow root as we talked on IRC.

Fixed the code to early returns.
Comment 7 Ryosuke Niwa 2016-03-14 03:14:14 PDT 
Created attachment 273950 [details]
Patch for landing
Comment 8 Ryosuke Niwa 2016-03-14 03:14:33 PDT 
Comment on attachment 273950 [details]
Patch for landing

Wait for EWS
Comment 9 Ryosuke Niwa 2016-03-14 03:58:50 PDT 
I don't think the failure of editing/pasteboard/paste-noplugin-xhtml.xhtml is related to this patch.
Comment 10 WebKit Commit Bot 2016-03-14 05:02:09 PDT 
Comment on attachment 273950 [details]
Patch for landing

Clearing flags on attachment: 273950

Committed r198115: <http://trac.webkit.org/changeset/198115>
Comment 11 WebKit Commit Bot 2016-03-14 05:02:14 PDT 
All reviewed patches have been landed.  Closing bug.