Summary: | defineElement should upgrade existing unresolved custom elements | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Ryosuke Niwa <rniwa> | ||||
Component: | DOM | Assignee: | 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
Ryosuke Niwa
2016-03-07 03:04:57 PST
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> |