Bug 160731 – Move document.defineElement to customElements.define

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

[Ryosuke Niwa]

The latest custom elements specification (now merged into HTML spec) moved document.defineElement
to window.customElements.define. Update our implementation to match this change.

[Ryosuke Niwa]

Created attachment 285716 [details]
Updates the implementation

[Ryosuke Niwa]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Chris Dumez]

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?

[Ryosuke Niwa]

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.

[Ryosuke Niwa]

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

[Ryosuke Niwa]

Created attachment 285765 [details]
Updated for TOT

[Ryosuke Niwa]

Created attachment 285769 [details]
Another build fix

[Chris Dumez]

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.

[Ryosuke Niwa]

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

[Chris Dumez]

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.

[WebKit Commit Bot]

Comment on attachment 285769 [details]
Another build fix

Clearing flags on attachment: 285769

Committed r204367: <http://trac.webkit.org/changeset/204367>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Ryosuke Niwa]

<rdar://problem/28090367>