[webkit-reviews] review granted: [Bug 222739] Cache cross-origin methods / accessors of Window and Location per lexical global object : [Attachment 422286] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Alexey Shvayka
<shvaikalesh at gmail.com>'s request for review:
Bug 222739: Cache cross-origin methods / accessors of Window and Location per
lexical global object
https://bugs.webkit.org/show_bug.cgi?id=222739

Attachment 422286: Patch

https://bugs.webkit.org/attachment.cgi?id=422286&action=review




--- Comment #4 from Darin Adler <darin at apple.com> ---
Comment on attachment 422286
  --> https://bugs.webkit.org/attachment.cgi?id=422286
Patch

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

> Source/WebCore/ChangeLog:18
> +	   because generated JSLocation can't be easily extended.

Is this a long term decision to never fix for location, or just a short term
decision to not do it at this time?

> Source/WebCore/bindings/js/JSDOMGlobalObject.cpp:314
> +    JSFunction* jsFunction = m_crossOriginFunctionMap.get(key);
> +    if (!jsFunction) {
> +	   jsFunction = JSFunction::create(lexicalGlobalObject->vm(),
lexicalGlobalObject, length, propertyName.publicName(), nativeFunction);
> +	   m_crossOriginFunctionMap.set(key, jsFunction);
> +    }
> +    return jsFunction;

This is the less-efficient, double hashing, idiom. The more efficient idiom
uses either add or ensure. The ensure version is something like this:

    return m_crossOriginFunctionMap.ensure(key, [&] {
	return JSFunction::create(lexicalGlobalObject->vm(),
lexicalGlobalObject, length, propertyName.publicName(), nativeFunction);
    }).iterator->value;

I am not sure why WeakGCMap implements set, but neither add nor ensure, so
you’d need to deal with that first.

It also seems that WeakGCMap’s set function has a peculiar combination of a
non-reference type (I would have expected an rvalue reference as in HashMap
itself) but use of WTFMove.

> Source/WebCore/bindings/js/JSDOMGlobalObject.cpp:319
> +    CrossOriginMapKey key = std::make_pair(lexicalGlobalObject, getter ?
reinterpret_cast<void*>(getter) : reinterpret_cast<void*>(setter));

I think for clarity we should add this assertion:

    ASSERT(getter || setter);

> Source/WebCore/bindings/js/JSDOMGlobalObject.cpp:328
> +    GetterSetter* getterSetter = m_crossOriginGetterSetterMap.get(key);
> +    if (!getterSetter) {
> +	   auto& vm = lexicalGlobalObject->vm();
> +	   getterSetter = GetterSetter::create(vm, lexicalGlobalObject,
> +	       getter ? JSCustomGetterFunction::create(vm, lexicalGlobalObject,
propertyName, getter) : nullptr,
> +	       setter ? JSCustomSetterFunction::create(vm, lexicalGlobalObject,
propertyName, setter) : nullptr);
> +	   m_crossOriginGetterSetterMap.set(key, getterSetter);
> +    }
> +    return getterSetter;

Same as above:

    m_crossOriginFunctionMap.ensure(key, [&] {
	auto& vm = lexicalGlobalObject->vm();
	return GetterSetter::create(vm, lexicalGlobalObject,
	    getter ? JSCustomGetterFunction::create(vm, lexicalGlobalObject,
propertyName, getter) : nullptr,
	    JSCustomSetterFunction::create(vm, lexicalGlobalObject,
propertyName, setter) : nullptr);;
    }).iterator->value;

> Source/WebCore/bindings/js/JSDOMGlobalObject.h:88
> +    JSC::JSFunction* getCrossOriginFunction(JSC::JSGlobalObject*,
JSC::PropertyName, JSC::NativeFunction, unsigned length);
> +    JSC::GetterSetter* getCrossOriginGetterSetter(JSC::JSGlobalObject*,
JSC::PropertyName, JSC::GetValueFunc, JSC::PutValueFunc);

WebKit coding style discourages the use of the word "get" in the names of
functions like these.

> Source/WebCore/bindings/js/JSDOMWindowCustom.cpp:206
> +	       && slot.isValue() &&
isHostFunction(slot.getValue(lexicalGlobalObject, propertyName),
JSDOMWindow::info()->staticPropHashTable->entry(propertyName)->function());

Rationale for looking this up at runtime instead of link time? Just so we can
avoid the need for ForwardDeclareInHeader?