Bug 94573 – [V8] Remove getToStringName() and getToStringTemplate() from V8PerIsolateData

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

[Kentaro Hara]

V8PerIsolateData::getToStringName() and V8PerIsolateData::getToStringTemplate() are not used at all. We can remove it and refactor the code around them.

[Kentaro Hara]

Created attachment 159621 [details]
Patch

[Adam Barth]

Comment on attachment 159621 [details]
Patch

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

> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:2964
> -    desc->Set(getToStringName(), getToStringTemplate());
> +    desc->Set(v8::String::NewSymbol("toString"), v8::Persistent<v8::FunctionTemplate>::New(v8::FunctionTemplate::New(constructorToString)));

This doesn't leak the persistent handle?  m_toStringTemplate used to make us reuse the handle.  I guess we only do this when configuration the template, so it's bounded in any case.

[Kentaro Hara]

(In reply to comment #2)
> > +    desc->Set(v8::String::NewSymbol("toString"), v8::Persistent<v8::FunctionTemplate>::New(v8::FunctionTemplate::New(constructorToString)));
> 
> This doesn't leak the persistent handle?  m_toStringTemplate used to make us reuse the handle.  I guess we only do this when configuration the template, so it's bounded in any case.

Good point. Maybe the real problem is that the current code fails to store the created persistent handle to V8PerIsolateData? In the current code (and in the uploaded patch), every time getToStringTemplate() is called, a new persistent handle is created. We should cache the persistent handle to V8PerIsolateData.

[Kentaro Hara]

Created attachment 159645 [details]
Patch

[Kentaro Hara]

(In reply to comment #3)
> Good point. Maybe the real problem is that the current code fails to store the created persistent handle to V8PerIsolateData? In the current code (and in the uploaded patch), every time getToStringTemplate() is called, a new persistent handle is created. We should cache the persistent handle to V8PerIsolateData.

Uploaded the patch to fix the issue.

[Adam Barth]

Comment on attachment 159645 [details]
Patch

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

> Source/WebCore/bindings/v8/V8Binding.cpp:-354
> -    v8::Persistent<v8::FunctionTemplate>& toStringTemplate = V8PerIsolateData::current()->toStringTemplate();
> -    if (toStringTemplate.IsEmpty())
> -        toStringTemplate = v8::Persistent<v8::FunctionTemplate>::New(v8::FunctionTemplate::New(constructorToString));

I mean, this used to store the persistent handle because toStringTemplate is a non-const reference.  However, it's not very clear code!  :)

> Source/WebCore/bindings/v8/V8PerIsolateData.h:55
> +v8::Handle<v8::Value> constructorOfToString(const v8::Arguments&);

Should we make this a static function in V8PerIsolateData?  That way we could make it private.

[Kentaro Hara]

(In reply to comment #6)
> > Source/WebCore/bindings/v8/V8PerIsolateData.h:55
> > +v8::Handle<v8::Value> constructorOfToString(const v8::Arguments&);
> 
> Should we make this a static function in V8PerIsolateData?  That way we could make it private.

Fixed and landed. Thanks.

[Kentaro Hara]

Committed r126237: <http://trac.webkit.org/changeset/126237>