[webkit-reviews] review granted: [Bug 83093] [V8] Add a per context data store and use that for caching boiler plates as well as constructor functions : [Attachment 135695] Patch

[bugzilla-daemon at webkit.org]

Adam Barth <abarth at webkit.org> has granted Erik Arvidsson <arv at chromium.org>'s
request for review:
Bug 83093: [V8] Add a per context data store and use that for caching boiler
plates as well as constructor functions
https://bugs.webkit.org/show_bug.cgi?id=83093

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

------- Additional Comments from Adam Barth <abarth at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=135695&action=review


> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:746
> -    return V8DOMWrapper::getConstructor(type,
V8DOMWindow::toNative(info.Holder()));
> +    return V8DOMWrapper::constructorForType(type,
V8DOMWindow::toNative(info.Holder()));

Make sure to run-bindings-tests --reset-results before landing.

> Source/WebCore/bindings/v8/V8BindingPerContextData.h:75
> +	 : m_context(context)

Four-space indent.

> Source/WebCore/bindings/v8/V8BindingPerContextData.h:76
> +    { }

nit: Technically each of { and } should be on its own lines.

> Source/WebCore/bindings/v8/V8BindingPerContextData.h:91
> +    v8::Handle<v8::Context> m_context;

I'm unclear who is responsible for keeping this handle alive.  This needs to be
a v8::Persistent handle because this object outlives the current HandleScope. 
Is there a reason not to use SharedPersistent ?

> Source/WebCore/bindings/v8/V8DOMWindowShell.h:104
>      v8::Persistent<v8::Context> m_context;

I see.	This is where we have the strong reference to the context.  Hum...  So,
I think this is ok, but not ideal.  It's like having two raw pointers to one
piece of memory and having one of the folks be in charge of deleting it.  We
can take this approach in this patch and add more automated memory management
in a later patch.