[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]

Marja Hölttä <marja at chromium.org> has denied  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 Marja Hölttä <marja at chromium.org>
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.