[webkit-reviews] review denied: [Bug 111971] [V8] Get rid of function-level static FunctionTemplates in generated bindings code : [Attachment 192456] Patch

[bugzilla-daemon at webkit.org]

Kentaro Hara <haraken at chromium.org> has denied Marja Hölttä
<marja at chromium.org>'s request for review:
Bug 111971: [V8] Get rid of function-level static FunctionTemplates in
generated bindings code
https://bugs.webkit.org/show_bug.cgi?id=111971

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

------- Additional Comments from Kentaro Hara <haraken at chromium.org>
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.