Bug 111411 – [Custom Elements][V8] Custom Element doesn't need its own WrapperTypeInfo

• Component: DOM
• Status: RESOLVED FIXED

[Hajime Morrita]

This is preparation for Bug 110436.
In this change, we're going to remove V8HTMLCustomElement::info and
use WrapperTypeInfo of some relevant buit-in element instead.

[Hajime Morrita]

Created attachment 191434 [details]
Patch

[Kentaro Hara]

Comment on attachment 191434 [details]
Patch

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

> Source/WebCore/bindings/v8/V8HTMLCustomElement.cpp:78
> +    v8::Handle<v8::Value> constructorValue = WebCore::toV8(constructor.get(), creationContext, isolate);

Nit: WebCore:: is not needed.

> Source/WebCore/bindings/v8/V8HTMLCustomElement.cpp:82
> +    v8::Handle<v8::Object> prototype = v8::Handle<v8::Object>::Cast(constructorWapper->Get(v8String("prototype", isolate)));

Nit: v8String => v8::String::NewSymbol

> Source/WebCore/bindings/v8/V8PerContextData.cpp:130
> +        if (prototypeObject->InternalFieldCount() == v8PrototypeInternalFieldcount)
> +            prototypeObject->SetAlignedPointerInInternalField(v8PrototypeTypeIndex, type);

It looks a bit fragile. In future there might be other objects whose InternalFieldCount() are 1. Can't you use type->wrapperTypePrototype instead of relying on InternalFieldCount() ?

[Hajime Morrita]

Created attachment 191450 [details]
Patch

[Hajime Morrita]

Comment on attachment 191434 [details]
Patch

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

Thanks for the quick look, haraken! I revised the patch.

>> Source/WebCore/bindings/v8/V8HTMLCustomElement.cpp:78
>> +    v8::Handle<v8::Value> constructorValue = WebCore::toV8(constructor.get(), creationContext, isolate);
> 
> Nit: WebCore:: is not needed.

compiler wants it since there are conflicting V8HTMLCusstomElement::toV8()

>> Source/WebCore/bindings/v8/V8PerContextData.cpp:130
>> +            prototypeObject->SetAlignedPointerInInternalField(v8PrototypeTypeIndex, type);
> 
> It looks a bit fragile. In future there might be other objects whose InternalFieldCount() are 1. Can't you use type->wrapperTypePrototype instead of relying on InternalFieldCount() ?

Changed in that way.

[Kentaro Hara]

Comment on attachment 191450 [details]
Patch

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

LGTM.

> Source/WebCore/bindings/v8/V8HTMLCustomElement.cpp:62
> +        v8::Handle<v8::Value> wrapperValue = toV8(toHTMLUnknownElement(impl.get()), creationContext, isolate);

Hmm? You said a compiler wants WebCore::, but did you remove WebCore:: ?

[WebKit Review Bot]

Comment on attachment 191450 [details]
Patch

Attachment 191450 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://webkit-commit-queue.appspot.com/results/17053076

New failing tests:
editing/inserting/insert-composition-whitespace.html
editing/pasteboard/paste-and-sanitize.html
dom/xhtml/level3/core/documentadoptnode16.xhtml
dom/xhtml/level1/core/hc_documentcreateelementcasesensitive.xhtml
accessibility/canvas-fallback-content.html
http/tests/cache/subresource-failover-to-network.html
editing/execCommand/remove-format-elements.html
editing/execCommand/applyblockelement-visiblepositionforindex-crash.html
dom/xhtml/level3/core/nodeisdefaultnamespace09.xhtml
dom/html/level1/core/hc_elementwrongdocumenterr.html
dom/html/level1/core/hc_attrinsertbefore6.html
editing/selection/extend-by-sentence-001.html
dom/html/level1/core/hc_attrappendchild5.html
editing/deleting/delete-and-cleanup.html
dom/html/level1/core/hc_nodeappendchildnewchilddiffdocument.html
dom/html/level1/core/hc_namednodemapwrongdocumenterr.html
dom/xhtml/level3/core/documentnormalizedocument10.xhtml
editing/editability/ignored-content.html
dom/html/level1/core/hc_attrappendchild2.html
dom/html/level1/core/hc_nodeinsertbeforenewchilddiffdocument.html
editing/undo/replace-text-in-node-preserving-markers-crash.html
editing/pasteboard/paste-without-nesting.html
http/tests/misc/script-defer-after-slow-stylesheet.html
editing/style/remove-format-without-enclosing-block.html
editing/execCommand/ident-crashes-topnode-is-text.html
editing/selection/root-inlinebox-selected-children-crash.html
fast/block/child-not-removed-from-parent-lineboxes-crash.html
dom/xhtml/level1/core/hc_attrappendchild2.xhtml
fast/block/merge-anonymous-block-remove-child-crash2.html
dom/html/level1/core/hc_nodereplacechildnewchilddiffdocument.html

[Hajime Morrita]

Comment on attachment 191450 [details]
Patch

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

>> Source/WebCore/bindings/v8/V8HTMLCustomElement.cpp:62
>> +        v8::Handle<v8::Value> wrapperValue = toV8(toHTMLUnknownElement(impl.get()), creationContext, isolate);
> 
> Hmm? You said a compiler wants WebCore::, but did you remove WebCore:: ?

Oh, this one is OK. Another one below isn't.

[Hajime Morrita]

Created attachment 191622 [details]
Patch for landing

[Hajime Morrita]

(In reply to comment #7)
> > 
> > Hmm? You said a compiler wants WebCore::, but did you remove WebCore:: ?
> 
> Oh, this one is OK. Another one below isn't.

And turned out we need it after all :-/

[WebKit Review Bot]

Comment on attachment 191622 [details]
Patch for landing

Clearing flags on attachment: 191622

Committed r144865: <http://trac.webkit.org/changeset/144865>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.