Created attachment 192456 [details] Patch

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.

Created attachment 192476 [details] Patch

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.

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).

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.

Comment on attachment 192476 [details] Patch Clearing flags on attachment: 192476 Committed r145494: <http://trac.webkit.org/changeset/145494>

All reviewed patches have been landed. Closing bug.

Re-opened since this is blocked by bug 112117

Created attachment 192685 [details] Patch

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

Comment on attachment 192685 [details] Patch rs=me

Comment on attachment 192685 [details] Patch Clearing flags on attachment: 192685 Committed r145511: <http://trac.webkit.org/changeset/145511>

All reviewed patches have been landed. Closing bug.