Bug 153173

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

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 on attachment 269137 [details] Patch without change log for EWS Inspector cleanup looks good, and could event land separately if you want.

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

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

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

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

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

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.

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

Created attachment 269343 [details] Fixed builds

Created attachment 269549 [details] Updated for ToT

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

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

Created attachment 269632 [details] Simpler approach

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

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 on attachment 269632 [details] Simpler approach This simpler approach doesn't quite work. It causes ~1% perf. regression in PerformanceTests/Bindings/create-element.html.

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

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

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

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.

Created attachment 269750 [details] Patch for landing

Comment on attachment 269750 [details] Patch for landing Wait for EWS.

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