Bug 101917 – [V8] Replace setDOMWrapper() + setJSWrapperForDOMObject() with createDOMWrapper()

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore JavaScript

[Kentaro Hara]

setJSWrapperForDOMObject() is always coupled with setDOMWrapper(). We can replace setDOMWrapper() + setJSWrapperForDOMObject() with createDOMWrapper(). (c.f. CREATE_DOM_WRAPPER() in JSC)

[Kentaro Hara]

Created attachment 173601 [details]
Patch

[WebKit Review Bot]

Comment on attachment 173601 [details]
Patch

Attachment 173601 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14816126

New failing tests:
fast/dom/undetectable-document-all.html

[Kentaro Hara]

Comment on attachment 173601 [details]
Patch

It looks like that special handling is needed for V8HTMLDocument::wrapInShadowObject().

[Adam Barth]

Comment on attachment 173601 [details]
Patch

I really like the idea.

[Kentaro Hara]

Created attachment 174081 [details]
Fixed crashes

[Adam Barth]

Comment on attachment 174081 [details]
Fixed crashes

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

LGTM, assuming the tests pass.

> Source/WebCore/bindings/v8/custom/V8HTMLDocumentCustom.cpp:78
>      shadow->SetPrototype(wrapper);

I looked into this shadow object, by the way.  It's needed to support a corner case in document.all.  If the web site sets document.all to some object, that object is stored in the shadow object so that when the property is deleted, the native document.all can re-appear.

[WebKit Review Bot]

Comment on attachment 174081 [details]
Fixed crashes

Clearing flags on attachment: 174081

Committed r134567: <http://trac.webkit.org/changeset/134567>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Dominik Röttsches (drott)]

If I am not mistaken, this is causing some bindings test failures:

http://build.webkit.org/builders/EFL%20Linux%2064-bit%20Debug%20WK2/builds/5866/steps/bindings-generation-tests/logs/stdio

FAIL: (V8) V8TestOverloadedConstructors.cpp
--- WebCore/bindings/scripts/test/V8/V8TestOverloadedConstructors.cpp	2012-11-13 15:11:00.184439424 -0800
+++ /tmp/tmpRWboHe/V8TestOverloadedConstructors.cpp	2012-11-14 02:03:40.638908011 -0800
@@ -54,8 +54,7 @@
     RefPtr<TestOverloadedConstructors> impl = TestOverloadedConstructors::create(arrayBuffer);
     v8::Handle<v8::Object> wrapper = args.Holder();
 
-    V8DOMWrapper::setDOMWrapper(wrapper, &info, impl.get());
-    V8DOMWrapper::setJSWrapperForDOMObject(impl.release(), wrapper, args.GetIsolate());
+    V8DOMWrapper::createDOMWrapper(impl.release(), &info, wrapper, args.GetIsolate());
     return wrapper;
 }
[...]

[Kentaro Hara]

(In reply to comment #9)
> If I am not mistaken, this is causing some bindings test failures:
> 
> http://build.webkit.org/builders/EFL%20Linux%2064-bit%20Debug%20WK2/builds/5866/steps/bindings-generation-tests/logs/stdio

Sorry, will fix in mins.

[Kentaro Hara]

Fixed in r134582.

[Dominik Röttsches (drott)]

Thanks!