Bug 155107

Summary: defineElement should upgrade existing unresolved custom elements
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, kling, koivisto, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 154907, 155108    
Attachments:
Description Flags
Patch darin: review+

Description Ryosuke Niwa 2016-03-07 03:04:57 PST
We should queue up the existing unresolved elements and upgrade them when defineElement is called asynchronously.
Comment 1 Radar WebKit Bug Importer 2016-03-07 03:06:14 PST
<rdar://problem/25004535>
Comment 2 Ryosuke Niwa 2016-03-07 03:23:27 PST
Created attachment 273170 [details]
Patch
Comment 3 Ryosuke Niwa 2016-03-07 03:32:46 PST
Comment on attachment 273170 [details]
Patch

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

> Source/WebCore/ChangeLog:16
> +        exclusive with isUnresolvedCustomElement().

This Node flag will be used in the bug 155108 when I implement :unresolved.
Comment 4 Darin Adler 2016-03-08 09:38:07 PST
Comment on attachment 273170 [details]
Patch

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

> Source/WebCore/dom/CustomElementDefinitions.cpp:84
> +    Vector<RefPtr<Element>> list;
> +    list.swap(candidateList->value);

I think we now prefer to use WTFMove instead of swapping for the “take” idiom in cases like this.

> Source/WebCore/dom/CustomElementDefinitions.cpp:94
> +    ASSERT(m_upgradeCandidatesMap.find(localName) == m_upgradeCandidatesMap.end());

More elegant to use "contains" here.

> Source/WebCore/dom/CustomElementDefinitions.cpp:99
> +    auto addResult = m_upgradeCandidatesMap.add(candidate.localName(), Vector<RefPtr<Element>>());

May be more efficient to use ensure instead of add here.

> Source/WebCore/dom/Document.cpp:1099
> +    // FIXME: Should we check the non-existance of prefix?

Type "existence" is the correct spelling.

Would be nicer for the comment to be a little more explicit about what this is. Is it a performance optimization comment? A correctness comment?

> Source/WebCore/dom/Node.h:277
> +    void setCustomElementIsResolved()
> +    {
> +        clearFlag(IsEditingTextOrUnresolvedCustomElementFlag);
> +        setFlag(IsCustomElement);
> +    }

Unpleasant to have these multi-line functions in the class definition. Makes it harder and harder to read it and get the overview. I prefer to put the inline function bodies outside the class definition to make the class easier to read.

> Source/WebCore/html/parser/HTMLConstructionSite.cpp:676
> +        auto* definitions = ownerDocument.customElementDefinitions();

I suggest moving this inside the if, after checking customElementInterface for null.
Comment 5 Ryosuke Niwa 2016-03-09 16:58:39 PST
Comment on attachment 273170 [details]
Patch

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

>> Source/WebCore/dom/CustomElementDefinitions.cpp:84
>> +    list.swap(candidateList->value);
> 
> I think we now prefer to use WTFMove instead of swapping for the “take” idiom in cases like this.

Fixed.

>> Source/WebCore/dom/CustomElementDefinitions.cpp:94
>> +    ASSERT(m_upgradeCandidatesMap.find(localName) == m_upgradeCandidatesMap.end());
> 
> More elegant to use "contains" here.

Fixed.

>> Source/WebCore/dom/CustomElementDefinitions.cpp:99
>> +    auto addResult = m_upgradeCandidatesMap.add(candidate.localName(), Vector<RefPtr<Element>>());
> 
> May be more efficient to use ensure instead of add here.

Fixed.

>> Source/WebCore/dom/Document.cpp:1099
>> +    // FIXME: Should we check the non-existance of prefix?
> 
> Type "existence" is the correct spelling.
> 
> Would be nicer for the comment to be a little more explicit about what this is. Is it a performance optimization comment? A correctness comment?

Revised the comment to: "should we also check the equality of prefix between the custom element and name?"

>> Source/WebCore/dom/Node.h:277
>> +    }
> 
> Unpleasant to have these multi-line functions in the class definition. Makes it harder and harder to read it and get the overview. I prefer to put the inline function bodies outside the class definition to make the class easier to read.

Moved to the end of the file.

>> Source/WebCore/html/parser/HTMLConstructionSite.cpp:676
>> +        auto* definitions = ownerDocument.customElementDefinitions();
> 
> I suggest moving this inside the if, after checking customElementInterface for null.

Fixed.
Comment 6 Ryosuke Niwa 2016-03-09 18:32:59 PST
Committed r197917: <http://trac.webkit.org/changeset/197917>