Bug 111411 - [Custom Elements][V8] Custom Element doesn't need its own WrapperTypeInfo
Summary: [Custom Elements][V8] Custom Element doesn't need its own WrapperTypeInfo
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Hajime Morrita
URL:
Keywords:
Depends on:
Blocks: 110436
  Show dependency treegraph
 
Reported: 2013-03-05 00:13 PST by Hajime Morrita
Modified: 2013-03-05 18:54 PST (History)
6 users (show)

See Also:


Attachments
Patch (10.01 KB, patch)
2013-03-05 01:29 PST, Hajime Morrita
no flags Details | Formatted Diff | Diff
Patch (10.57 KB, patch)
2013-03-05 03:00 PST, Hajime Morrita
no flags Details | Formatted Diff | Diff
Patch for landing (10.11 KB, patch)
2013-03-05 17:55 PST, Hajime Morrita
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Hajime Morrita 2013-03-05 00:13:05 PST
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.
Comment 1 Hajime Morrita 2013-03-05 01:29:59 PST
Created attachment 191434 [details]
Patch
Comment 2 Kentaro Hara 2013-03-05 01:47:26 PST
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() ?
Comment 3 Hajime Morrita 2013-03-05 03:00:29 PST
Created attachment 191450 [details]
Patch
Comment 4 Hajime Morrita 2013-03-05 03:01:01 PST
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.
Comment 5 Kentaro Hara 2013-03-05 03:18:52 PST
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:: ?
Comment 6 WebKit Review Bot 2013-03-05 03:54:08 PST
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
Comment 7 Hajime Morrita 2013-03-05 16:36:55 PST
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.
Comment 8 Hajime Morrita 2013-03-05 17:55:41 PST
Created attachment 191622 [details]
Patch for landing
Comment 9 Hajime Morrita 2013-03-05 17:56:13 PST
(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 :-/
Comment 10 WebKit Review Bot 2013-03-05 18:54:38 PST
Comment on attachment 191622 [details]
Patch for landing

Clearing flags on attachment: 191622

Committed r144865: <http://trac.webkit.org/changeset/144865>
Comment 11 WebKit Review Bot 2013-03-05 18:54:42 PST
All reviewed patches have been landed.  Closing bug.