[V8] Get rid of function-level static FunctionTemplates in generated bindings code

• Component: New Bugs
• Status: RESOLVED FIXED

[Marja Hölttä]

When we create and store function templates for main world and non-main worlds separately (see bug 111724), having function templates as static variables inside functions breaks it. (We cannot make it initialize with some world type when the execution runs through that line of code for the first time, and use it later for a different world type when the function is called again.)

The patch (which will be attached soon) gets rid of these function-level statics and stores the functiontemplates inside V8PerIsolateData instead.

[Marja Hölttä]

Created attachment 192456 [details]
Patch

[Kentaro Hara]

Comment on attachment 192456 [details]
Patch

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

> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:807
> +    // This is only for getting a unique pointer which we can pass to privateTemplateMap.
> +    static String privateTemplateUniqueKey = "${funcName}PrivateTemplate";
> +    WrapperWorldType currentWorldType = worldType(info.GetIsolate());
> +    V8PerIsolateData::PrivateTemplateMap& templateMap = V8PerIsolateData::from(info.GetIsolate())->privateTemplateMap(currentWorldType);
> +    v8::Persistent<v8::FunctionTemplate> privateTemplate;
> +    V8PerIsolateData::PrivateTemplateMap::iterator result = templateMap.find(&privateTemplateUniqueKey);
> +    if (result == templateMap.end()) {
> +        privateTemplate = v8::Persistent<v8::FunctionTemplate>::New(info.GetIsolate(), $newTemplateString);
> +        templateMap.add(&privateTemplateUniqueKey, privateTemplate);
> +    } else
> +        privateTemplate = result->value;
> +
> +    v8::Handle<v8::Object> holder = info.This()->FindInstanceInPrototypeChain(${v8InterfaceName}::GetTemplate(info.GetIsolate(), currentWorldType));

You are duplicating the code to multiple places in V8LocationCustom.cpp. Shall we create a helper method that does the work in V8PerIsolateData.cpp ?

Just to confirm: This change won't affect any performance-sensitive DOM attributes, right?

> Source/WebCore/bindings/v8/V8PerIsolateData.h:73
> +    typedef HashMap<void*, v8::Persistent<v8::FunctionTemplate> > PrivateTemplateMap;

Maybe PerWorldTemplateMap is a better name?

> Source/WebCore/bindings/v8/V8PerIsolateData.h:75
> +    PrivateTemplateMap& privateTemplateMap(WrapperWorldType worldType)

perWorldTemplateMap ?

> Source/WebCore/bindings/v8/V8PerIsolateData.h:147
> +    PrivateTemplateMap m_privateTemplatesForMainWorld;

m_perWorldTemplatesForMainWorld ?

> Source/WebCore/bindings/v8/V8PerIsolateData.h:148
> +    PrivateTemplateMap m_privateTemplatesForNonMainWorld;

m_perWorldTemplatesForNonMainWorld ?

Let's add a comment about what "NonMainWorld" means, i.e. let's say that this template map can be used for both an isolated world and a worker world.

> Source/WebCore/bindings/v8/custom/V8LocationCustom.cpp:198
> +        static String sharedTemplateUniqueKey = "$replaceSharedTemplate";

"$replaceSharedTemplate" => "replaceSharedTemplate"

A helper method will resolve your typo:)

> Source/WebCore/bindings/v8/custom/V8LocationCustom.cpp:234
> +        static String sharedTemplateUniqueKey = "${funcName}SharedTemplate";

Ditto.

[Marja Hölttä]

Created attachment 192476 [details]
Patch

[Marja Hölttä]

Comment on attachment 192456 [details]
Patch

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

>> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:807
>> +    v8::Handle<v8::Object> holder = info.This()->FindInstanceInPrototypeChain(${v8InterfaceName}::GetTemplate(info.GetIsolate(), currentWorldType));
> 
> You are duplicating the code to multiple places in V8LocationCustom.cpp. Shall we create a helper method that does the work in V8PerIsolateData.cpp ?
> 
> Just to confirm: This change won't affect any performance-sensitive DOM attributes, right?

Helper method created.

>> Source/WebCore/bindings/v8/V8PerIsolateData.h:73
>> +    typedef HashMap<void*, v8::Persistent<v8::FunctionTemplate> > PrivateTemplateMap;
> 
> Maybe PerWorldTemplateMap is a better name?

This no longer exists, everything is stored in the same map, as discussed.

>> Source/WebCore/bindings/v8/V8PerIsolateData.h:75
>> +    PrivateTemplateMap& privateTemplateMap(WrapperWorldType worldType)
> 
> perWorldTemplateMap ?

The same here.

>> Source/WebCore/bindings/v8/V8PerIsolateData.h:147
>> +    PrivateTemplateMap m_privateTemplatesForMainWorld;
> 
> m_perWorldTemplatesForMainWorld ?

This no longer exists.

>> Source/WebCore/bindings/v8/V8PerIsolateData.h:148
>> +    PrivateTemplateMap m_privateTemplatesForNonMainWorld;
> 
> m_perWorldTemplatesForNonMainWorld ?
> 
> Let's add a comment about what "NonMainWorld" means, i.e. let's say that this template map can be used for both an isolated world and a worker world.

This no longer exists.

>> Source/WebCore/bindings/v8/custom/V8LocationCustom.cpp:198
>> +        static String sharedTemplateUniqueKey = "$replaceSharedTemplate";
> 
> "$replaceSharedTemplate" => "replaceSharedTemplate"
> 
> A helper method will resolve your typo:)

Fixed

>> Source/WebCore/bindings/v8/custom/V8LocationCustom.cpp:234
>> +        static String sharedTemplateUniqueKey = "${funcName}SharedTemplate";
> 
> Ditto.

Fixed.

[Kentaro Hara]

Comment on attachment 192476 [details]
Patch

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

> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:809
> +        static String sharedTemplateUniqueKey = "${funcName}SharedTemplate";
> +        v8::Persistent<v8::FunctionTemplate> sharedTemplate = V8PerIsolateData::from(info.GetIsolate())->privateTemplate(currentWorldType, &sharedTemplateUniqueKey, $newTemplateParams);

I understand that we need to use a different template from 'privateTemplate', but still I don't understand why it is called 'sharedTemplate' (What does 'shared' mean?). We might want a more descriptive name.

Either way you can fix it in a follow-up patch.

> Source/WebCore/bindings/v8/V8PerIsolateData.h:68
> +    typedef HashMap<void*, v8::Persistent<v8::FunctionTemplate> > TemplateMap;

Do you have a plan to use any key whose type is not a string? (Although we discussed it offline, I forgot where we reached:-)

Otherwise, you can use a string key for safety. void* is hacky (and might be vulnerable).

[Marja Hölttä]

Comment on attachment 192476 [details]
Patch

Offline discussion: void* needs to be void*, since WrapperTypeInfo* pointers are also used as a key. Renaming private & shared left out of this patch.

[WebKit Review Bot]

Comment on attachment 192476 [details]
Patch

Clearing flags on attachment: 192476

Committed r145494: <http://trac.webkit.org/changeset/145494>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

Re-opened since this is blocked by bug 112117

[Marja Hölttä]

Created attachment 192685 [details]
Patch

[Marja Hölttä]

The new patch changes static String -> static const char*; no other changes.

Comment on attachment 192685 [details]
Patch

rs=me

[WebKit Review Bot]

Comment on attachment 192685 [details]
Patch

Clearing flags on attachment: 192685

Committed r145511: <http://trac.webkit.org/changeset/145511>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.