Bug 160731

Created attachment 285716 [details] Updates the implementation

Created attachment 285717 [details] Fix GTK+/EFL builds

Comment on attachment 285717 [details] Fix GTK+/EFL builds Attachment 285717 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/1844564 New failing tests: js/dom/global-constructors-attributes.html

Created attachment 285720 [details] Archive of layout-test-results from ews103 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews103 Port: mac-yosemite Platform: Mac OS X 10.10.5

Comment on attachment 285717 [details] Fix GTK+/EFL builds Attachment 285717 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/1844574 New failing tests: js/dom/global-constructors-attributes.html

Created attachment 285722 [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 on attachment 285717 [details] Fix GTK+/EFL builds Attachment 285717 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/1844572 New failing tests: js/dom/global-constructors-attributes.html

Created attachment 285723 [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 285717 [details] Fix GTK+/EFL builds View in context: https://bugs.webkit.org/attachment.cgi?id=285717&action=review Looks like there is a test failure and that it still does build on EFL / GTK/Win. > Source/WebCore/bindings/js/JSCustomElementsRegistryCustom.cpp:42 > +{ This function should throw if given less than 2 parameters: if (UNLIKELY(state.argumentCount() < 2)) return state.vm().throwException(&state, createNotEnoughArgumentsError(&state)); You can then use uncheckedArgument() below instead of argument(). > Source/WebCore/bindings/js/JSCustomElementsRegistryCustom.cpp:43 > + AtomicString tagName(state.argument(0).toString(&state)->toAtomicString(&state)); Isn't this a local name rather than a qualified name ? If so, I would suggest naming this variable localName or simply name as in the spec. tagName is a bit confusing to me because I think of a qualified name (https://dom.spec.whatwg.org/#dom-element-tagname). > Source/WebCore/bindings/js/JSCustomElementsRegistryCustom.cpp:47 > + JSObject* object = state.argument(1).getObject(); How about we call this 'constructor' rather than 'object' ? > Source/WebCore/bindings/js/JSCustomElementsRegistryCustom.cpp:49 > + if (!object || object->methodTable()->getConstructData(object, callData) == ConstructType::None) Instead of all this, I think we should be able to use JSValue::isFunction(). We should probably pass a JSFunction* to the implementation instead of a JSObject* for clarity. > Source/WebCore/bindings/js/JSCustomElementsRegistryCustom.cpp:51 > + It looks like we may be missing step 2 in the spec? "If constructor is either an interface object or named constructor, whose corresponding interface either is HTMLElement or has HTMLElement in its set of inherited interfaces, throw a TypeError and abort these steps." We should at least add a FIXME. I am not sure how easy it is to detect our interface objects at the moment so we can look into it separately. > Source/WebCore/bindings/js/JSCustomElementsRegistryCustom.cpp:56 > + return throwSyntaxError(&state, "Custom element name cannot be same as one of the builtin elements"); ASCIILiteral()? > Source/WebCore/bindings/js/JSCustomElementsRegistryCustom.cpp:58 > + return throwSyntaxError(&state, "Custom element name must contain a hyphen"); ASCIILiteral()? > Source/WebCore/bindings/js/JSCustomElementsRegistryCustom.cpp:60 > + return throwSyntaxError(&state, "Custom element name cannot contain an upper case letter"); ASCIILiteral()? > Source/WebCore/bindings/js/JSCustomElementsRegistryCustom.cpp:65 > + throwNotSupportedError(state, "Cannot define multiple custom elements with the same tag name"); ASCIILiteral()? > Source/WebCore/bindings/js/JSCustomElementsRegistryCustom.cpp:70 > + throwNotSupportedError(state, "Cannot define multiple custom elements with the same class"); ASCIILiteral()? > Source/WebCore/bindings/js/JSCustomElementsRegistryCustom.cpp:83 > + QualifiedName name(nullAtom, tagName, HTMLNames::xhtmlNamespaceURI); This seems to confirm tagName is a localName, not a qualifiedName. > Source/WebCore/bindings/js/JSHTMLElementCustom.cpp:54 > + return throwVMTypeError(&exec, "new.target is not a valid custom element constructor"); ASCIILiteral()? > Source/WebCore/dom/CustomElementsRegistry.cpp:2 > + * Copyright (C) 2015 Apple Inc. All rights reserved. 2015 on purpose? is it just moved code? > Source/WebCore/dom/CustomElementsRegistry.h:31 > +#pragma once I believe this usually comes before the includes. > Source/WebCore/dom/CustomElementsRegistry.h:33 > +#if ENABLE(CUSTOM_ELEMENTS) Ditto. > Source/WebCore/dom/CustomElementsRegistry.h:48 > + WTF_MAKE_FAST_ALLOCATED; Redundant since this subclasses RefCounted. > Source/WebCore/dom/CustomElementsRegistry.h:53 > + RefPtr<Element> constructElement(const AtomicString&); I cannot find the implementation for this method? In particular, I was curious why this returned a RefPtr<> and not a Ref<>. > Source/WebCore/dom/CustomElementsRegistry.h:66 > + HashMap<AtomicString, Vector<RefPtr<Element>>> m_upgradeCandidatesMap; Could this be a Vector of Ref<Element> ? > Source/WebCore/dom/CustomElementsRegistry.h:67 > + HashMap<AtomicString, RefPtr<JSCustomElementInterface>> m_nameMap; Could this be a Ref<> ? I believe Sam added support for Ref<> values in HashMap a while back. > Source/WebCore/dom/CustomElementsRegistry.idl:28 > + JSGenerateToNativeObject, I don't think we need this one. This is the default since the interface does not have a parent. > Source/WebCore/dom/CustomElementsRegistry.idl:32 > + [InvokesCustomElementLifecycleCallbacks, Custom] void define(DOMString name, Function constructor); nit: I would omit the extra blank lines surrounding this. > Source/WebCore/dom/Document.cpp:883 > +static ALWAYS_INLINE RefPtr<HTMLElement> createUpgradeCandidateElement(Document& document, DOMWindow* window, const QualifiedName& name) Why ALWAYS_INLINE? > Source/WebCore/dom/Document.cpp:891 > + Ref<HTMLElement> element = HTMLElement::create(name, document); auto > Source/WebCore/dom/Document.cpp:897 > static RefPtr<Element> createHTMLElementWithNameValidation(Document& document, const AtomicString& localName, ExceptionCode& ec) Can this return a RefPtr<HTMLElement> ? > Source/WebCore/dom/Document.cpp:923 > return WTFMove(element); No need for the WTFMove() if the function is updated to return a RefPtr<HTMLElement>. > Source/WebCore/dom/Document.cpp:1092 > + Ref<HTMLElement> element = HTMLElement::create(name, document); auto > Source/WebCore/dom/Document.cpp:1101 > + return *element; return element.releaseNonNull(); > Source/WebCore/page/DOMWindow.h:426 > + mutable RefPtr<CustomElementsRegistry> m_customElementsRegistry; Does not look like you need the mutable since your methods are non-const?

Comment on attachment 285717 [details] Fix GTK+/EFL builds View in context: https://bugs.webkit.org/attachment.cgi?id=285717&action=review >> Source/WebCore/bindings/js/JSCustomElementsRegistryCustom.cpp:42 >> +{ > > This function should throw if given less than 2 parameters: > if (UNLIKELY(state.argumentCount() < 2)) > return state.vm().throwException(&state, createNotEnoughArgumentsError(&state)); > > You can then use uncheckedArgument() below instead of argument(). Done. >> Source/WebCore/bindings/js/JSCustomElementsRegistryCustom.cpp:43 >> + AtomicString tagName(state.argument(0).toString(&state)->toAtomicString(&state)); > > Isn't this a local name rather than a qualified name ? If so, I would suggest naming this variable localName or simply name as in the spec. tagName is a bit confusing to me because I think of a qualified name (https://dom.spec.whatwg.org/#dom-element-tagname). Fixed. >> Source/WebCore/bindings/js/JSCustomElementsRegistryCustom.cpp:47 >> + JSObject* object = state.argument(1).getObject(); > > How about we call this 'constructor' rather than 'object' ? Sure. >> Source/WebCore/bindings/js/JSCustomElementsRegistryCustom.cpp:49 >> + if (!object || object->methodTable()->getConstructData(object, callData) == ConstructType::None) > > Instead of all this, I think we should be able to use JSValue::isFunction(). We should probably pass a JSFunction* to the implementation instead of a JSObject* for clarity. object being a function isn't sufficient. We need to use JSValue::isConstructor instead. I'm going to keep JSObject* for now since changing this would require adding a lot more code. >> Source/WebCore/bindings/js/JSCustomElementsRegistryCustom.cpp:51 >> + > > It looks like we may be missing step 2 in the spec? > "If constructor is either an interface object or named constructor, whose corresponding interface either is HTMLElement or has HTMLElement in its set of inherited interfaces, throw a TypeError and abort these steps." > > We should at least add a FIXME. I am not sure how easy it is to detect our interface objects at the moment so we can look into it separately. Added a FIXME. In general, I'm keeping the original behavior we had instead of updating this function to the spec since that would require a lot of code changes. >> Source/WebCore/dom/CustomElementsRegistry.h:48 >> + WTF_MAKE_FAST_ALLOCATED; > > Redundant since this subclasses RefCounted. Removed. >> Source/WebCore/dom/CustomElementsRegistry.h:53 >> + RefPtr<Element> constructElement(const AtomicString&); > > I cannot find the implementation for this method? In particular, I was curious why this returned a RefPtr<> and not a Ref<>. Oops, I forgot to remove this. Done. >> Source/WebCore/dom/CustomElementsRegistry.h:66 >> + HashMap<AtomicString, Vector<RefPtr<Element>>> m_upgradeCandidatesMap; > > Could this be a Vector of Ref<Element> ? No, Vector doesn't support Ref in them. >> Source/WebCore/dom/Document.cpp:883 >> +static ALWAYS_INLINE RefPtr<HTMLElement> createUpgradeCandidateElement(Document& document, DOMWindow* window, const QualifiedName& name) > > Why ALWAYS_INLINE? This is needed for performance. >> Source/WebCore/dom/Document.cpp:897 >> static RefPtr<Element> createHTMLElementWithNameValidation(Document& document, const AtomicString& localName, ExceptionCode& ec) > > Can this return a RefPtr<HTMLElement> ? This requires constructElement returning HTMLElement so I'm gonna do that in a separate patch. >> Source/WebCore/page/DOMWindow.h:426 >> + mutable RefPtr<CustomElementsRegistry> m_customElementsRegistry; > > Does not look like you need the mutable since your methods are non-const? Fixed.

Created attachment 285762 [details] Fixed builds and addressed review comments

Created attachment 285765 [details] Updated for TOT

Created attachment 285769 [details] Another build fix

Comment on attachment 285769 [details] Another build fix View in context: https://bugs.webkit.org/attachment.cgi?id=285769&action=review r=me > Source/WebCore/dom/CustomElementsRegistry.idl:28 > + JSGenerateToNativeObject, I think you missed my previous review comment about this being redundant since this interface has no parent.

(In reply to comment #14) > Comment on attachment 285769 [details] > Another build fix > > View in context: > https://bugs.webkit.org/attachment.cgi?id=285769&action=review > > r=me > > > Source/WebCore/dom/CustomElementsRegistry.idl:28 > > + JSGenerateToNativeObject, > > I think you missed my previous review comment about this being redundant > since this interface has no parent. The thing doesn't build without this.

Comment on attachment 285769 [details] Another build fix View in context: https://bugs.webkit.org/attachment.cgi?id=285769&action=review >>> Source/WebCore/dom/CustomElementsRegistry.idl:28 >>> + JSGenerateToNativeObject, >> >> I think you missed my previous review comment about this being redundant since this interface has no parent. > > The thing doesn't build without this. Very weird... Ok then.

Comment on attachment 285769 [details] Another build fix Clearing flags on attachment: 285769 Committed r204367: <http://trac.webkit.org/changeset/204367>

All reviewed patches have been landed. Closing bug.

<rdar://problem/28090367>