Bug 155107 – defineElement should upgrade existing unresolved custom elements

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Ryosuke Niwa]

We should queue up the existing unresolved elements and upgrade them when defineElement is called asynchronously.

[Radar WebKit Bug Importer]

<rdar://problem/25004535>

[Ryosuke Niwa]

Created attachment 273170 [details]
Patch

[Ryosuke Niwa]

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.

[Darin Adler]

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.

[Ryosuke Niwa]

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.

[Ryosuke Niwa]

Committed r197917: <http://trac.webkit.org/changeset/197917>