Bug 153173

Summary: document.createElement should be able to create a custom element
Product: WebKit Reporter: Ryosuke Niwa <rniwa>
Component: DOMAssignee: Ryosuke Niwa <rniwa>
Status: RESOLVED FIXED    
Severity: Normal CC: buildbot, cdumez, commit-queue, esprehn+autocc, ggaren, gyuyoung.kim, joepeck, kangil.han, keith_miller, koivisto, kondapallykalyan, mark.lam, msaboff, rniwa, sbarati
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 150225    
Attachments:
Description Flags
Patch without change log for EWS
none
Archive of layout-test-results from ews113 for mac-yosemite
none
Archive of layout-test-results from ews102 for mac-yosemite
none
Archive of layout-test-results from ews105 for mac-yosemite-wk2
none
Adds the support to document.createElement
none
Archive of layout-test-results from ews112 for mac-yosemite
none
Fixed builds
none
Updated for ToT
none
Simpler approach
none
Archive of layout-test-results from ews117 for mac-yosemite
none
Address Antti/Chris' comments
none
Archive of layout-test-results from ews115 for mac-yosemite
none
Patch for landing none

Description Ryosuke Niwa 2016-01-15 19:10:43 PST
Add the support for creating custom element via document.createElement.
Comment 1 Ryosuke Niwa 2016-01-15 19:14:11 PST
Created attachment 269137 [details]
Patch without change log for EWS
Comment 2 WebKit Commit Bot 2016-01-15 19:15:28 PST
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.
Comment 3 Joseph Pecoraro 2016-01-15 20:07:35 PST
Comment on attachment 269137 [details]
Patch without change log for EWS

Inspector cleanup looks good, and could event land separately if you want.
Comment 4 Build Bot 2016-01-15 20:21:18 PST
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
Comment 5 Build Bot 2016-01-15 20:21:22 PST
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
Comment 6 Build Bot 2016-01-15 20:21:43 PST
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
Comment 7 Build Bot 2016-01-15 20:21:48 PST
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
Comment 8 Build Bot 2016-01-15 20:26:06 PST
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
Comment 9 Build Bot 2016-01-15 20:26:12 PST
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
Comment 10 Ryosuke Niwa 2016-01-19 20:42:23 PST
Created attachment 269331 [details]
Adds the support to document.createElement
Comment 11 Build Bot 2016-01-19 21:33:58 PST
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.
Comment 12 Build Bot 2016-01-19 21:34:05 PST
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
Comment 13 Darin Adler 2016-01-19 22:39:16 PST
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.
Comment 14 Darin Adler 2016-01-19 22:41:04 PST
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.
Comment 15 Ryosuke Niwa 2016-01-19 22:50:13 PST
Created attachment 269343 [details]
Fixed builds
Comment 16 Ryosuke Niwa 2016-01-21 21:20:47 PST
Created attachment 269549 [details]
Updated for ToT
Comment 17 Antti Koivisto 2016-01-22 10:27:56 PST
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)?
Comment 18 Chris Dumez 2016-01-22 10:34:40 PST
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.
Comment 19 Ryosuke Niwa 2016-01-22 14:54:27 PST
(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.
Comment 20 Ryosuke Niwa 2016-01-22 19:18:17 PST
Created attachment 269632 [details]
Simpler approach
Comment 21 Build Bot 2016-01-22 20:23:50 PST
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
Comment 22 Build Bot 2016-01-22 20:23:56 PST
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
Comment 23 Ryosuke Niwa 2016-01-22 23:41:01 PST
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.
Comment 24 Ryosuke Niwa 2016-01-23 01:15:53 PST
Created attachment 269660 [details]
Address Antti/Chris' comments
Comment 25 Build Bot 2016-01-23 02:21:05 PST
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
Comment 26 Build Bot 2016-01-23 02:21:10 PST
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
Comment 27 Darin Adler 2016-01-24 14:25:35 PST
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"
Comment 28 Ryosuke Niwa 2016-01-25 07:25:26 PST
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.
Comment 29 Ryosuke Niwa 2016-01-25 07:57:25 PST
Created attachment 269750 [details]
Patch for landing
Comment 30 Ryosuke Niwa 2016-01-25 07:57:46 PST
Comment on attachment 269750 [details]
Patch for landing

Wait for EWS.
Comment 31 Ryosuke Niwa 2016-01-25 09:23:49 PST
Committed r195538: <http://trac.webkit.org/changeset/195538>