[webkit-reviews] review granted: [Bug 97943] [V8] The concept of forceNewObject is unneeded (dom-traverse gets 3% faster) : [Attachment 166333] Patch

[bugzilla-daemon at webkit.org]

Kentaro Hara <haraken at chromium.org> has granted Adam Barth
<abarth at webkit.org>'s request for review:
Bug 97943: [V8] The concept of forceNewObject is unneeded (dom-traverse gets 3%
faster)
https://bugs.webkit.org/show_bug.cgi?id=97943

Attachment 166333: Patch
https://bugs.webkit.org/attachment.cgi?id=166333&action=review

------- Additional Comments from Kentaro Hara <haraken at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=166333&action=review


> Source/WebCore/ChangeLog:8
> +	   We don't need the concept of forceNewObject. It doen't do anything

Great observation! I think JSC is using a similar concept too. Maybe we can
remove it from JSC too (in another patch)?

Typo: doen't

> Source/WebCore/ChangeLog:11
> +	   * bindings/scripts/CodeGeneratorV8.pm:

Maybe you need to update run-binding-tests?

> Source/WebCore/bindings/v8/custom/V8NodeCustom.cpp:135
> -    if (!forceNewObject) {
> -	   v8::Handle<v8::Value> wrapper =
V8DOMWrapper::getCachedWrapper(impl);
> -	   if (!wrapper.IsEmpty())
> -	       return wrapper;
> -    }
> +    v8::Handle<v8::Value> wrapper = V8DOMWrapper::getCachedWrapper(impl);
> +    if (!wrapper.IsEmpty())
> +	   return wrapper;

Before landing, please just double-check that we are clearing the cached
wrapper correctly. If we kept a stale cached wrapper, we will end up returning
a wrong wrapper object.