Bug 155107

<rdar://problem/25004535>

Created attachment 273170 [details] Patch

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 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 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.

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