Bug 110436 – [Custom Elements] Any HTMLElement subclass should become a superclass of custom element

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

[Hajime Morrita]

The bug 100229 limits only HTMLElement as a superclass. Such a limitation should be eventually removed.

[Hajime Morrita]

*** Bug 111511 has been marked as a duplicate of this bug. ***

[Hajime Morrita]

Created attachment 192423 [details]
Patch

[Dimitri Glazkov (Google)]

Comment on attachment 192423 [details]
Patch

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

> Source/WebCore/bindings/v8/CustomElementHelpers.cpp:67
> +static bool hasValidPrototypeChain(v8::Handle<v8::Object> requiredAscendant, v8::Handle<v8::Value> chain)

Ascendant -> Ancestor? Or is that too close to ancestor in hierarchical sense?

> Source/WebCore/html/HTMLAttributeNames.in:129
> +is

Should this be ifdef'd somehow?

[Hajime Morrita]

Comment on attachment 192423 [details]
Patch

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

>> Source/WebCore/bindings/v8/CustomElementHelpers.cpp:67
>> +static bool hasValidPrototypeChain(v8::Handle<v8::Object> requiredAscendant, v8::Handle<v8::Value> chain)
> 
> Ascendant -> Ancestor? Or is that too close to ancestor in hierarchical sense?

Ouch. Just confused. Should be ancestor.

>> Source/WebCore/html/HTMLAttributeNames.in:129
>> +is
> 
> Should this be ifdef'd somehow?

Unfortunately, there is no such mechanism for attribute names.
Fortunately, this difference isn't observable from the Web ;-)

[Hajime Morrita]

Created attachment 192615 [details]
Patch

[Dimitri Glazkov (Google)]

Comment on attachment 192615 [details]
Patch

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

The non-V8 things look good. Some comments inline:

> Source/WebCore/ChangeLog:30
> +          this change, the C++ backend of custom elements were alwasy

alwasy->always.

> Source/WebCore/dom/CustomElementConstructor.cpp:57
> +    , m_elementName(elementName)

should we call this customElementName? Maybe I should rename it to something like "type name" rather than "custom element name" in the spec.

> Source/WebCore/dom/CustomElementConstructor.cpp:75
> +PassRefPtr<Element> markAsExtendedCustomElement(PassRefPtr<Element> element, const AtomicString& typeExtension)

setTypeExtension?

[Hajime Morrita]

Created attachment 193219 [details]
Patch

[Hajime Morrita]

Hi Dimiri, thanks for taking a look! I revised the patch.
Haraken, could you give a glance for v8 bits?

[Kentaro Hara]

Comment on attachment 193219 [details]
Patch

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

> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:521
> +template<>
> +class WrapperTypeTraits<${nativeType} > {
> +public:
> +    static WrapperTypeInfo* info() { return &${v8InterfaceName}::info; }
> +};

You need to update run-bindings-tests.

> Source/WebCore/bindings/v8/CustomElementHelpers.cpp:73
>      while (!chain.IsEmpty()) {
> -        if (chain == htmlPrototype)
> +        if (chain == requiredAncestor)
>              return true;
>          if (!chain->IsObject())
>              return false;

Nit: I'd prefer:

  while (!chain.IsEmpty() && chain->IsObject()) {
    if (chain == requiredAncestor)
      return true;
    chain = v8::Handle<v8::Object>::Cast(chain)->GetPrototype();
  }
  return true;

> Source/WebCore/bindings/v8/V8CustomElement.cpp:48
> +    RefPtr<CustomElementConstructor> protectedConstructor(constructor);

Why does this need to be protected again? PassRefPtr<CustomElementConstructor> already keeps +1, doesn't it?

> Source/WebCore/bindings/v8/V8CustomElement.h:-63
> -inline v8::Handle<v8::Value> V8CustomElement::toV8(Element* impl, v8::Handle<v8::Object> creationContext, v8::Isolate* isolate)
> -{
> -    if (UNLIKELY(!impl))
> -        return v8NullWithCheck(isolate);
> -    v8::Handle<v8::Object> wrapper = DOMDataStore::getWrapper(impl, isolate);
> -    if (!wrapper.IsEmpty())
> -        return wrapper;
> -    return V8CustomElement::wrap(impl, creationContext, isolate);
> -}

Why can you remove this? Is it because there is no caller by chance, or because toV8() is auto-generated?

> Source/WebCore/bindings/v8/custom/V8CustomElementConstructorCustom.cpp:51
> -    return V8CustomElement::toV8(element.get(), args.Holder(), args.GetIsolate());
> +    return V8CustomElement::wrap(element.get(), args.Holder(), impl, args.GetIsolate());

What's the rationale for changing toV8() to wrap() ? (Actually I don't fully understand how toV8() and wrap() are related to each other in V8CustomElement.)

[Hajime Morrita]

Comment on attachment 193219 [details]
Patch

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

Hi Haraken, thanks for taking a look! I'm revising the patch.

>> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:521
>> +};
> 
> You need to update run-bindings-tests.

Will do.

>> Source/WebCore/bindings/v8/CustomElementHelpers.cpp:73
>>              return false;
> 
> Nit: I'd prefer:
> 
>   while (!chain.IsEmpty() && chain->IsObject()) {
>     if (chain == requiredAncestor)
>       return true;
>     chain = v8::Handle<v8::Object>::Cast(chain)->GetPrototype();
>   }
>   return true;

OK, will do.

>> Source/WebCore/bindings/v8/V8CustomElement.cpp:48
>> +    RefPtr<CustomElementConstructor> protectedConstructor(constructor);
> 
> Why does this need to be protected again? PassRefPtr<CustomElementConstructor> already keeps +1, doesn't it?

Right. Will remove protectedConstructor variable.

>> Source/WebCore/bindings/v8/V8CustomElement.h:-63
>> -}
> 
> Why can you remove this? Is it because there is no caller by chance, or because toV8() is auto-generated?

We can remove this since this one cannot be called.
The point here is that there is no CustomElement class. V8CustomElement class is just a set of helper function
which is used mainly from generated code. The client can just use toV8(Node*), and V8CustomElement::wrap() is called inside the generated code
if necessary.

>> Source/WebCore/bindings/v8/custom/V8CustomElementConstructorCustom.cpp:51
>> +    return V8CustomElement::wrap(element.get(), args.Holder(), impl, args.GetIsolate());
> 
> What's the rationale for changing toV8() to wrap() ? (Actually I don't fully understand how toV8() and wrap() are related to each other in V8CustomElement.)

wrap() is used here because there is no longer toV8().
This is safe since |element| is just created in this function through impl->createElement() call above.

[Hajime Morrita]

Created attachment 193222 [details]
Patch

[Kentaro Hara]

LGTM on the V8 side change.

(In reply to comment #10)
> >> Source/WebCore/bindings/v8/V8CustomElement.h:-63
> >> -}
> > 
> > Why can you remove this? Is it because there is no caller by chance, or because toV8() is auto-generated?
> 
> We can remove this since this one cannot be called.
> The point here is that there is no CustomElement class. V8CustomElement class is just a set of helper function
> which is used mainly from generated code. The client can just use toV8(Node*), and V8CustomElement::wrap() is called inside the generated code
> if necessary.

Makes sense to me. Would you add a comment about it?

[Hajime Morrita]

Created attachment 193230 [details]
Patch

[Hajime Morrita]

(In reply to comment #12)
> LGTM on the V8 side change.
> 
>> (snip)
> 
> Makes sense to me. Would you add a comment about it?

Added a comment, waiting for rubberstamping from Dimitri :-)

[WebKit Review Bot]

Comment on attachment 193230 [details]
Patch

Clearing flags on attachment: 193230

Committed r145932: <http://trac.webkit.org/changeset/145932>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.