Bug 94573

Summary: [V8] Remove getToStringName() and getToStringTemplate() from V8PerIsolateData
Product: WebKit Reporter: Kentaro Hara <haraken>
Component: WebCore JavaScriptAssignee: Kentaro Hara <haraken>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, dcarney, eric.carlson, feature-media-reviews, japhet, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 93095    
Attachments:
Description Flags
Patch
none
Patch none

Description Kentaro Hara 2012-08-20 22:25:08 PDT
V8PerIsolateData::getToStringName() and V8PerIsolateData::getToStringTemplate() are not used at all. We can remove it and refactor the code around them.
Comment 1 Kentaro Hara 2012-08-20 22:30:41 PDT
Created attachment 159621 [details]
Patch
Comment 2 Adam Barth 2012-08-20 23:18:29 PDT
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.
Comment 3 Kentaro Hara 2012-08-20 23:27:06 PDT
(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.
Comment 4 Kentaro Hara 2012-08-21 03:37:33 PDT
Created attachment 159645 [details]
Patch
Comment 5 Kentaro Hara 2012-08-21 03:47:57 PDT
(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.
Comment 6 Adam Barth 2012-08-21 07:50:22 PDT
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.
Comment 7 Kentaro Hara 2012-08-21 17:32:09 PDT
(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.
Comment 8 Kentaro Hara 2012-08-21 17:32:28 PDT
Committed r126237: <http://trac.webkit.org/changeset/126237>