The bug 100229 limits only HTMLElement as a superclass. Such a limitation should be eventually removed.
*** Bug 111511 has been marked as a duplicate of this bug. ***
Created attachment 192423 [details] Patch
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?
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 ;-)
Created attachment 192615 [details] Patch
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?
Created attachment 193219 [details] Patch
Hi Dimiri, thanks for taking a look! I revised the patch. Haraken, could you give a glance for v8 bits?
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.)
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.
Created attachment 193222 [details] Patch
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?
Created attachment 193230 [details] Patch
(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 :-)
Comment on attachment 193230 [details] Patch Clearing flags on attachment: 193230 Committed r145932: <http://trac.webkit.org/changeset/145932>
All reviewed patches have been landed. Closing bug.