Bug 153173 – document.createElement should be able to create a custom element

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

[Ryosuke Niwa]

Add the support for creating custom element via document.createElement.

[Ryosuke Niwa]

Created attachment 269137 [details]
Patch without change log for EWS

[WebKit Commit Bot]

Attachment 269137 [details] did not pass style-queue:


ERROR: Source/WebCore/dom/ElementFactory.h:39:  Should have a space between // and comment  [whitespace/comments] [4]
Total errors found: 1 in 19 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Joseph Pecoraro]

Comment on attachment 269137 [details]
Patch without change log for EWS

Inspector cleanup looks good, and could event land separately if you want.

[Build Bot]

Comment on attachment 269137 [details]
Patch without change log for EWS

Attachment 269137 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/696809

New failing tests:
fast/custom-elements/Document-defineCustomElement.html
fast/custom-elements/HTMLElement-constructor.html

[Build Bot]

Created attachment 269140 [details]
Archive of layout-test-results from ews113 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews113  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 269137 [details]
Patch without change log for EWS

Attachment 269137 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/696817

New failing tests:
fast/custom-elements/Document-defineCustomElement.html
fast/custom-elements/HTMLElement-constructor.html

[Build Bot]

Created attachment 269141 [details]
Archive of layout-test-results from ews102 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 269137 [details]
Patch without change log for EWS

Attachment 269137 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/696821

New failing tests:
fast/custom-elements/Document-defineCustomElement.html
fast/custom-elements/HTMLElement-constructor.html

[Build Bot]

Created attachment 269143 [details]
Archive of layout-test-results from ews105 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5

[Ryosuke Niwa]

Created attachment 269331 [details]
Adds the support to document.createElement

[Build Bot]

Comment on attachment 269331 [details]
Adds the support to document.createElement

Attachment 269331 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/715389

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 269334 [details]
Archive of layout-test-results from ews112 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews112  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Darin Adler]

Comment on attachment 269331 [details]
Adds the support to document.createElement

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

> Source/WebCore/bindings/js/JSElementCustom.cpp:58
> +    if (element->isCustomElement())
> +        return getCachedWrapper(globalObject->world(), element);

This needs #if ENABLE(CUSTOM_ELEMENTS) around it; that's why the build is failing on GTK and EFL.

[Darin Adler]

Comment on attachment 269331 [details]
Adds the support to document.createElement

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

> Source/WebCore/dom/Document.cpp:1067
> +    ASSERT(element);

This assertion is firing in lots of tests. That’s why the mac-debug EWS bot is failing.

[Ryosuke Niwa]

Created attachment 269343 [details]
Fixed builds

[Ryosuke Niwa]

Created attachment 269549 [details]
Updated for ToT

[Antti Koivisto]

Comment on attachment 269549 [details]
Updated for ToT

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

> Source/WebCore/dom/Node.h:625
> -        // HeyItIsAFreeBit = 1 << 22,
> +        IsCustomElement = 1 << 22,

:|

> Source/WebCore/dom/make_names.pl:1007
> +RefPtr<$parameters{namespace}Element> $parameters{namespace}ElementFactory::createElement(const QualifiedName& name, Document& document$formElementArgumentForDefinition, bool createdByParser, ShouldCreateCustomElement shouldCreateCustomElement)

Did you consider keeping this generated class as-is for buildin element construction and just doing the custom element construction in the clients that want it (currently only one site)?

[Chris Dumez]

Comment on attachment 269549 [details]
Updated for ToT

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

> Source/WebCore/dom/ElementFactory.h:37
> +enum class ShouldCreateCustomElement { Create, DoNotCreate };

I think we usually use things like:
enum class ShouldCreateCustomElement { No, Yes };

>> Source/WebCore/dom/make_names.pl:1007
>> +RefPtr<$parameters{namespace}Element> $parameters{namespace}ElementFactory::createElement(const QualifiedName& name, Document& document$formElementArgumentForDefinition, bool createdByParser, ShouldCreateCustomElement shouldCreateCustomElement)
> 
> Did you consider keeping this generated class as-is for buildin element construction and just doing the custom element construction in the clients that want it (currently only one site)?

I assume this can return null now? This is sad. It makes the call-sites look ugly or unsafe.

[Ryosuke Niwa]

(In reply to comment #18)
> Comment on attachment 269549 [details]
> Updated for ToT
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=269549&action=review
> 
> > Source/WebCore/dom/ElementFactory.h:37
> > +enum class ShouldCreateCustomElement { Create, DoNotCreate };
> 
> I think we usually use things like:
> enum class ShouldCreateCustomElement { No, Yes };

Will do.

> >> Source/WebCore/dom/make_names.pl:1007
> >> +RefPtr<$parameters{namespace}Element> $parameters{namespace}ElementFactory::createElement(const QualifiedName& name, Document& document$formElementArgumentForDefinition, bool createdByParser, ShouldCreateCustomElement shouldCreateCustomElement)
> > 
> > Did you consider keeping this generated class as-is for buildin element construction and just doing the custom element construction in the clients that want it (currently only one site)?

I did. The problem is that we still need to fallback to HTMLUnknownElement when the tag name doesn't match any of builtins and custom elements.

> I assume this can return null now? This is sad. It makes the call-sites look
> ugly or unsafe.

Yes. This was another reason I wanted to split the factory function into two pieces; the one that instantiates only known builtin elements, and another one that falls back to HTMLUnknownElement. The second one can continue to return Ref.

[Ryosuke Niwa]

Created attachment 269632 [details]
Simpler approach

[Build Bot]

Comment on attachment 269632 [details]
Simpler approach

Attachment 269632 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/727762

New failing tests:
imported/w3c/web-platform-tests/streams-api/readable-streams/garbage-collection.html

[Build Bot]

Created attachment 269636 [details]
Archive of layout-test-results from ews117 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews117  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Ryosuke Niwa]

Comment on attachment 269632 [details]
Simpler approach

This simpler approach doesn't quite work. It causes ~1% perf. regression in PerformanceTests/Bindings/create-element.html.

[Ryosuke Niwa]

Created attachment 269660 [details]
Address Antti/Chris' comments

[Build Bot]

Comment on attachment 269660 [details]
Address Antti/Chris' comments

Attachment 269660 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/728662

New failing tests:
imported/w3c/web-platform-tests/streams-api/readable-streams/garbage-collection.html

[Build Bot]

Created attachment 269661 [details]
Archive of layout-test-results from ews115 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews115  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Darin Adler]

Comment on attachment 269660 [details]
Address Antti/Chris' comments

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

Looks like we hit an assertion failure in imported/w3c/web-platform-tests/streams-api/readable-streams/garbage-collection.html

r=me assuming you figure that out

> Source/WebCore/bindings/js/JSCustomElementInterface.h:56
> +#if ENABLE(MATHML)
> +class MathMLElement;
> +#endif

I am not sure we need to wrap forward declarations in #if like this. Is there a big downside to doing it unconditionally instead?

> Source/WebCore/bindings/js/JSMainThreadExecState.h:127
> +    template<typename Type, Type jsType, class DataType>
> +    static InspectorInstrumentationCookie instrumentFunctionInternal(ScriptExecutionContext*, Type, const DataType&);

I think it’s hard to see something like this is a single declaration when it’s on two lines like this. Would you consider changing it to be on one line? Also, please use typename DataType rather than class DataType.

Normally we put the private function members before the private data members. Could you move these functions above so we can spot the data members at the bottom?

> Source/WebCore/inspector/InspectorCSSAgent.cpp:779
> +    RefPtr<Element> styleElement = document.createElement(HTMLNames::styleTag, false);

Should be Ref, not RefPtr. Could use auto.

> Source/WebCore/inspector/InspectorDOMAgent.cpp:718
> +    if (!is<Element>(oldNode.get()))

Surprised that you need get() here. I thought we had the is<> overloaded for the smart pointers.

> Source/WebCore/inspector/InspectorDOMAgent.cpp:722
> +    RefPtr<Element> newElem = oldNode->document().createElementForBindings(tagName, ec);

While touching this code would be nice to expand "newElem" to "newElement"

[Ryosuke Niwa]

Thanks for the review!

(In reply to comment #27)
> Comment on attachment 269660 [details]
> Address Antti/Chris' comments
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=269660&action=review
> 
> Looks like we hit an assertion failure in
> imported/w3c/web-platform-tests/streams-api/readable-streams/garbage-
> collection.html

I don't think this assertion failure is related to my patch.

> > Source/WebCore/bindings/js/JSCustomElementInterface.h:56
> > +#if ENABLE(MATHML)
> > +class MathMLElement;
> > +#endif
> 
> I am not sure we need to wrap forward declarations in #if like this. Is
> there a big downside to doing it unconditionally instead?

Removed the if-def.

> > Source/WebCore/bindings/js/JSMainThreadExecState.h:127
> > +    template<typename Type, Type jsType, class DataType>
> > +    static InspectorInstrumentationCookie instrumentFunctionInternal(ScriptExecutionContext*, Type, const DataType&);
> 
> I think it’s hard to see something like this is a single declaration when
> it’s on two lines like this. Would you consider changing it to be on one
> line? Also, please use typename DataType rather than class DataType.
> 
> Normally we put the private function members before the private data
> members. Could you move these functions above so we can spot the data
> members at the bottom?

Done that.

> > Source/WebCore/inspector/InspectorCSSAgent.cpp:779
> > +    RefPtr<Element> styleElement = document.createElement(HTMLNames::styleTag, false);
> 
> Should be Ref, not RefPtr. Could use auto.

Changed to Ref. Since we need to have a RefPtr/Ref here for security reasons, I'm not gonna use auto.

> > Source/WebCore/inspector/InspectorDOMAgent.cpp:718
> > +    if (!is<Element>(oldNode.get()))
> 
> Surprised that you need get() here. I thought we had the is<> overloaded for
> the smart pointers.

I think we support it only for Ref.

> > Source/WebCore/inspector/InspectorDOMAgent.cpp:722
> > +    RefPtr<Element> newElem = oldNode->document().createElementForBindings(tagName, ec);
> 
> While touching this code would be nice to expand "newElem" to "newElement"

Done that.

[Ryosuke Niwa]

Created attachment 269750 [details]
Patch for landing

[Ryosuke Niwa]

Comment on attachment 269750 [details]
Patch for landing

Wait for EWS.

[Ryosuke Niwa]

Committed r195538: <http://trac.webkit.org/changeset/195538>