document.createElementNS doesn't construct a custom element

[Ryosuke Niwa]

document.createElementNS doesn't create a custom element :(

[Ryosuke Niwa]

Created attachment 294659 [details]
Fixes the bug

[Ryosuke Niwa]

Created attachment 294660 [details]
Added a change log description

[Radar WebKit Bug Importer]

<rdar://problem/29237299>

[Radar WebKit Bug Importer]

<rdar://problem/29237304>

[Ryosuke Niwa]

Created attachment 294661 [details]
Updated for ToT

[Darin Adler]

Comment on attachment 294661 [details]
Updated for ToT

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

> Source/WebCore/bindings/js/JSCustomElementInterface.cpp:85
> +    return element.get();

This churns the reference count. Should do WTFMove(element) instead to avoid that.

> Source/WebCore/dom/DOMImplementation.cpp:118
> +        ASSERT(!document->domWindow()); // createElemenNS should not synchronously construct a custom element here.

Typo, missing "t" in createElementNS.

Also completely unclear what domWindow has to do with createElementNS.

> Source/WebCore/dom/Document.cpp:881
> +static bool isValidHTMLElementName(const AtomicString& localName) { return Document::isValidName(localName); }
> +static bool isValidHTMLElementName(const QualifiedName& name) { return Document::isValidName(name.localName()); }

Can we write these out normally instead of as one-liners? Should they be marked inline?

> Source/WebCore/dom/Document.cpp:883
> +template <class NameType>

Should be template<typename NameType>; we prefer typename or class and no space after "template".

[Ryosuke Niwa]

Thanks for the review.

(In reply to comment #6)
> Comment on attachment 294661 [details]
> Updated for ToT
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=294661&action=review
> 
> > Source/WebCore/bindings/js/JSCustomElementInterface.cpp:85
> > +    return element.get();
> 
> This churns the reference count. Should do WTFMove(element) instead to avoid
> that.

Fixed.

> > Source/WebCore/dom/DOMImplementation.cpp:118
> > +        ASSERT(!document->domWindow()); // createElemenNS should not synchronously construct a custom element here.
> 
> Typo, missing "t" in createElementNS.
> 
> Also completely unclear what domWindow has to do with createElementNS.

Revised to: If domWindow is not null, createElementNS could find CustomElementRegistry and arbitrary scripts.

> > Source/WebCore/dom/Document.cpp:881
> > +static bool isValidHTMLElementName(const AtomicString& localName) { return Document::isValidName(localName); }
> > +static bool isValidHTMLElementName(const QualifiedName& name) { return Document::isValidName(name.localName()); }
> 
> Can we write these out normally instead of as one-liners? Should they be
> marked inline?

Fixed, and added inline keyword.

> > Source/WebCore/dom/Document.cpp:883
> > +template <class NameType>
> 
> Should be template<typename NameType>; we prefer typename or class and no
> space after "template".

Fixed.

[Ryosuke Niwa]

Created attachment 294767 [details]
Patch for landing

[Ryosuke Niwa]

Created attachment 294768 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 294768 [details]
Patch for landing

Clearing flags on attachment: 294768

Committed r208716: <http://trac.webkit.org/changeset/208716>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.