Bug 78875 - [v8] v8 doesn't assume to do 'new WebKitShadowRoot(host)'.
Summary: [v8] v8 doesn't assume to do 'new WebKitShadowRoot(host)'.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Shinya Kawanaka
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-02-16 21:50 PST by Shinya Kawanaka
Modified: 2012-02-16 22:42 PST (History)
3 users (show)

See Also:


Attachments
Test (1.05 KB, patch)
2012-02-16 22:05 PST, Shinya Kawanaka
no flags Details | Formatted Diff | Diff
Patch (7.69 KB, patch)
2012-02-16 22:19 PST, Shinya Kawanaka
no flags Details | Formatted Diff | Diff
Patch (7.50 KB, patch)
2012-02-16 22:22 PST, Shinya Kawanaka
haraken: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Shinya Kawanaka 2012-02-16 21:50:11 PST
new WebKitShadowRoot(host) and internals.ensureShadowRoot(host) returns a different object now.
These should be the same, however they return different objects, which contain the same ShadowRoot object though.
Comment 1 Shinya Kawanaka 2012-02-16 22:05:36 PST
Created attachment 127521 [details]
Test
Comment 2 Kentaro Hara 2012-02-16 22:12:43 PST
Comment on attachment 127521 [details]
Test

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

> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:1687
> +    # A DOMObject that is an ActiveDOMObject and also a DOMNode should be treated as an DOMNode here.
> +    # setJSWrapperForDOMNode() will look if node is active and choose correct map to add node to.

The change looks OK, but the comment is confusing to me. Maybe you can just remove the comment.

Why "should" an ActiveDOMObject-and-DOMNode object be treated as DOMNode? In my understanding,
- There is no ActiveDOMObject-and-DOMNode object for now.
- If an ActiveDOMObject-and-DOMNode appeared in the future, we need another logic, like "setJSWrapperForActiveDOMNodeObject()".
Comment 3 Shinya Kawanaka 2012-02-16 22:19:45 PST
Created attachment 127524 [details]
Patch
Comment 4 Shinya Kawanaka 2012-02-16 22:22:11 PST
(In reply to comment #2)
> (From update of attachment 127521 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=127521&action=review
> 
> > Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:1687
> > +    # A DOMObject that is an ActiveDOMObject and also a DOMNode should be treated as an DOMNode here.
> > +    # setJSWrapperForDOMNode() will look if node is active and choose correct map to add node to.
> 
> The change looks OK, but the comment is confusing to me. Maybe you can just remove the comment.
> 
> Why "should" an ActiveDOMObject-and-DOMNode object be treated as DOMNode? In my understanding,
> - There is no ActiveDOMObject-and-DOMNode object for now.
> - If an ActiveDOMObject-and-DOMNode appeared in the future, we need another logic, like "setJSWrapperForActiveDOMNodeObject()".

Thanks. I've removed the comment.
Comment 5 Shinya Kawanaka 2012-02-16 22:22:14 PST
Created attachment 127525 [details]
Patch
Comment 6 Kentaro Hara 2012-02-16 22:25:15 PST
Comment on attachment 127525 [details]
Patch

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

> LayoutTests/ChangeLog:17
> +        * platform/wincairo/Skipped:

platform/wk2/Skipped might be needed...? (I am not sure.)

> LayoutTests/fast/dom/shadow/shadow-root-new.html:9
> +<div id="container"></div>

Nit: This line is not necessary.
Comment 7 Shinya Kawanaka 2012-02-16 22:31:18 PST
(In reply to comment #6)
> (From update of attachment 127525 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=127525&action=review
> 
> > LayoutTests/ChangeLog:17
> > +        * platform/wincairo/Skipped:
> 
> platform/wk2/Skipped might be needed...? (I am not sure.)

No one has added Skipped line for SHADOW_DOM there, but let's add this time for defensive style :-)

> 
> > LayoutTests/fast/dom/shadow/shadow-root-new.html:9
> > +<div id="container"></div>
> 
> Nit: This line is not necessary.

Thanks.

After fixing them, I'll land this.
Comment 8 Shinya Kawanaka 2012-02-16 22:42:41 PST
Committed r108035: <http://trac.webkit.org/changeset/108035>