Bug 153092

Summary: Add document.defineCustomElement
Product: WebKit Reporter: Ryosuke Niwa <rniwa>
Component: DOMAssignee: Ryosuke Niwa <rniwa>
Status: RESOLVED FIXED    
Severity: Normal CC: buildbot, cdumez, ggaren, keith_miller, koivisto, rniwa, sam
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 150225    
Attachments:
Description Flags
Adds the feature
none
Added the missing expected results
none
Archive of layout-test-results from ews102 for mac-yosemite
none
Archive of layout-test-results from ews107 for mac-yosemite-wk2
none
Fixed a failing test cdumez: review+

Description Ryosuke Niwa 2016-01-13 23:39:39 PST
Add a method to define a custom element and allow subclassing of HTMLElement.
Comment 1 Ryosuke Niwa 2016-01-14 00:11:31 PST
Created attachment 268944 [details]
Adds the feature
Comment 2 Ryosuke Niwa 2016-01-14 00:13:55 PST
Created attachment 268945 [details]
Added the missing expected results
Comment 3 Ryosuke Niwa 2016-01-14 00:15:33 PST
This patch doesn't add support for creating a custom element in document.createElement, HTML parser, etc... as well as lifecycle callbacks. They're coming in separate patches.
Comment 4 Build Bot 2016-01-14 01:12:44 PST
Comment on attachment 268945 [details]
Added the missing expected results

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

New failing tests:
imported/w3c/web-platform-tests/html/dom/interfaces.html
Comment 5 Build Bot 2016-01-14 01:12:46 PST
Created attachment 268948 [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 6 Build Bot 2016-01-14 01:16:07 PST
Comment on attachment 268945 [details]
Added the missing expected results

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

New failing tests:
imported/w3c/web-platform-tests/html/dom/interfaces.html
Comment 7 Build Bot 2016-01-14 01:16:11 PST
Created attachment 268949 [details]
Archive of layout-test-results from ews107 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5
Comment 8 Ryosuke Niwa 2016-01-14 14:04:24 PST
Created attachment 269001 [details]
Fixed a failing test
Comment 9 Chris Dumez 2016-01-14 16:07:44 PST
Comment on attachment 269001 [details]
Fixed a failing test

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

r=me with comments.

> Source/WebCore/bindings/js/JSDocumentCustom.cpp:161
> +        setDOMException(&state, NOT_SUPPORTED_ERR);

We could probably provide a meaningful error message here.

> Source/WebCore/bindings/js/JSHTMLElementCustom.cpp:42
> +    DOMConstructorObject* jsConstructor = jsCast<DOMConstructorObject*>(state->callee());

auto*

> Source/WebCore/bindings/js/JSHTMLElementCustom.cpp:44
> +    ScriptExecutionContext* context = jsConstructor->scriptExecutionContext();

auto*

> Source/WebCore/bindings/js/JSHTMLElementCustom.cpp:45
> +    if (!context || !context->isDocument())

if (!is<Document>(context))

> Source/WebCore/bindings/js/JSHTMLElementCustom.cpp:58
> +        if (state->argumentCount() < 1)

UNLIKELY()

> Source/WebCore/bindings/js/JSHTMLElementCustom.cpp:80
> +    if (state->hadException())

UNLIKELY()

> Source/WebCore/bindings/js/JSHTMLElementCustom.cpp:84
> +    auto* jsElement = JSHTMLElement::create(newElementStructure, globalObject, element.get());

I don't think you need the .get()

> Source/WebCore/dom/CustomElementDefinitions.cpp:53
> +    // FIXME: We should be taking the advantage of QualifiedNames in SVG and MathML.

This does not sound like a lot of work to do in this patch and would be a lot more efficient as we would just compare AtomicStrings.

> Source/WebCore/dom/CustomElementDefinitions.cpp:69
> +    auto* constructor = interface->constructor();

I would move this after the assertion.

> Source/WebCore/dom/CustomElementDefinitions.cpp:71
> +    ASSERT(m_nameMap.find(fullName.localName()) == m_nameMap.end());

I would just use !contains() to make this shorter.

> Source/WebCore/dom/CustomElementDefinitions.cpp:74
> +    auto it = m_constructorMap.add(constructor, fullName);

We usually call this one addResult rather than "it" I believe? The iterator is a member of the AddResult.

> Source/WebCore/dom/CustomElementDefinitions.h:48
> +class CustomElementDefinitions {

WTF_MAKE_FAST_ALLOCATED

> LayoutTests/fast/custom-elements/Document-defineCustomElement.html:29
> +    assert_throws({'name': 'SyntaxError'}, function () { document.defineCustomElement('aBc', MyCustomElement); },

I would add a '-' in there so it is otherwise valid.

> LayoutTests/fast/custom-elements/HTMLElement-constructor.html:43
> +    class CustomElmeentWithInferredTagName extends HTMLElement {};

Typo: Elmeent
Comment 10 Ryosuke Niwa 2016-01-14 18:59:15 PST
Committed r195087: <http://trac.webkit.org/changeset/195087>