Bindings: use |holder| local variable in attribute getters and setters

Source/bindings/templates/attributes.cpp

 {##############################################################################}
 {% macro attribute_getter(attribute, world_suffix) %}
 {% filter conditional(attribute.conditional_string) %}
 static void {{attribute.name}}AttributeGetter{{world_suffix}}(
 {%- if attribute.is_expose_js_accessors %}
 const v8::FunctionCallbackInfo<v8::Value>& info
 {%- else %}
 const v8::PropertyCallbackInfo<v8::Value>& info
 {%- endif %})
 {
     {% if attribute.is_reflect and not attribute.is_url and
           attribute.idl_type == 'DOMString' and is_node %}
     {% set cpp_class, v8_class = 'Element', 'V8Element' %}
     {% endif %}
-    {# impl #}
-    {# FIXME: use a local variable for holder more often and simplify below #}
-    {% if attribute.is_unforgeable or
-          interface_name == 'Window' and attribute.idl_type == 'EventHandler' %}
-    {% if interface_name == 'Window' %}
-    v8::Handle<v8::Object> holder = info.Holder();
-    {% else %}{# perform lookup first #}
+    {# isolate #}
+    {% if attribute.cached_attribute_validation_method or
+          attribute.is_call_with_execution_context or
+          attribute.is_check_security_for_node or
+          attribute.is_getter_raises_exception or
+          attribute.is_keep_alive_for_gc %}

[haraken, 2014/03/27 04:40:36]

I'm not sure if this improves the IDL compiler. It's ugly to add a complicated
if branch just for avoid using info.GetIsolate().

Avoiding multiple info.GetIsolate()s is just for code beauty. It doesn't matter
for performance because compilers will eliminate the duplicated
info.GetIsolate()s. So we don't need to remove multiple info.GetIsolate()s
unless it improves code beauty.

[Nils Barth (inactive), 2014/03/27 05:19:46]

Got it; revised CL to only have |holder|.

+    v8::Isolate* isolate = info.GetIsolate();
+    {% endif %}
+    {# holder #}
+    {% if attribute.is_unforgeable and interface_name != 'Window' %}
+    {# perform lookup first #}
     {# FIXME: can we remove this lookup? #}
     v8::Handle<v8::Object> holder = {{v8_class}}::findInstanceInPrototypeChain(info.This(), info.GetIsolate());
     if (holder.IsEmpty())
         return;
-    {% endif %}{# Window #}
+    {% elif not attribute.is_static %}
+    v8::Handle<v8::Object> holder = info.Holder();
+    {% endif %}
+    {# impl #}
+    {% if attribute.cached_attribute_validation_method %}
+    v8::Handle<v8::String> propertyName = v8AtomicString(isolate, "{{attribute.name}}");
     {{cpp_class}}* impl = {{v8_class}}::toNative(holder);
-    {% elif attribute.cached_attribute_validation_method %}
-    v8::Handle<v8::String> propertyName = v8AtomicString(info.GetIsolate(), "{{attribute.name}}");
-    {{cpp_class}}* impl = {{v8_class}}::toNative(info.Holder());
     if (!impl->{{attribute.cached_attribute_validation_method}}()) {
-        v8::Handle<v8::Value> jsValue = V8HiddenValue::getHiddenValue(info.GetIsolate(), info.Holder(), propertyName);
+        v8::Handle<v8::Value> jsValue = V8HiddenValue::getHiddenValue(isolate, holder, propertyName);
         if (!jsValue.IsEmpty()) {
             v8SetReturnValue(info, jsValue);
             return;
         }
     }
-    {% elif not (attribute.is_static or attribute.is_unforgeable) %}
-    {{cpp_class}}* impl = {{v8_class}}::toNative(info.Holder());
+    {% elif not attribute.is_static %}
+    {{cpp_class}}* impl = {{v8_class}}::toNative(holder);
     {% endif %}
     {% if attribute.is_implemented_by and not attribute.is_static %}
     ASSERT(impl);
     {% endif %}
     {% if interface_name == 'Window' and attribute.idl_type == 'EventHandler' %}
     if (!impl->document())
         return;
     {% endif %}
     {# Local variables #}
     {% if attribute.is_call_with_execution_context %}
-    ExecutionContext* scriptContext = currentExecutionContext(info.GetIsolate());
+    ExecutionContext* scriptContext = currentExecutionContext(isolate);
     {% endif %}
     {% if attribute.is_check_security_for_node or
           attribute.is_getter_raises_exception %}
-    ExceptionState exceptionState(ExceptionState::GetterContext, "{{attribute.name}}", "{{interface_name}}", info.Holder(), info.GetIsolate());
+    ExceptionState exceptionState(ExceptionState::GetterContext, "{{attribute.name}}", "{{interface_name}}", holder, isolate);
     {% endif %}
     {% if attribute.is_nullable %}
     bool isNull = false;
     {% endif %}
     {# FIXME: consider always using a local variable for value #}
     {% if attribute.cached_attribute_validation_method or
           attribute.is_getter_raises_exception or
           attribute.is_nullable or
           attribute.reflect_only or
           attribute.idl_type == 'EventHandler' %}
     {{attribute.cpp_type}} {{attribute.cpp_value}} = {{attribute.cpp_value_original}};
     {% endif %}
     {# Checks #}
     {% if attribute.is_getter_raises_exception %}
     if (UNLIKELY(exceptionState.throwIfNeeded()))
         return;
     {% endif %}
     {% if attribute.is_check_security_for_node %}
     {# FIXME: use a local variable to not call getter twice #}
-    if (!BindingSecurity::shouldAllowAccessToNode(info.GetIsolate(), {{attribute.cpp_value}}, exceptionState)) {
+    if (!BindingSecurity::shouldAllowAccessToNode(isolate, {{attribute.cpp_value}}, exceptionState)) {
         v8SetReturnValueNull(info);
         exceptionState.throwIfNeeded();
         return;
     }
     {% endif %}
     {% if attribute.reflect_only %}
     {{release_only_check(attribute.reflect_only, attribute.reflect_missing,
                          attribute.reflect_invalid, attribute.reflect_empty)
       | indent}}
     {% endif %}
     {% if attribute.is_nullable %}
     if (isNull) {
         v8SetReturnValueNull(info);
         return;
     }
     {% endif %}
     {% if attribute.cached_attribute_validation_method %}
-    V8HiddenValue::setHiddenValue(info.GetIsolate(), info.Holder(), propertyName, {{attribute.cpp_value}}.v8Value());
+    V8HiddenValue::setHiddenValue(isolate, holder, propertyName, {{attribute.cpp_value}}.v8Value());
     {% endif %}
     {# v8SetReturnValue #}
     {% if attribute.is_keep_alive_for_gc %}
     {# FIXME: merge local variable assignment with above #}
     {{attribute.cpp_type}} result({{attribute.cpp_value}});
     if (result && DOMDataStore::setReturnValueFromWrapper{{world_suffix}}<{{attribute.v8_type}}>(info.GetReturnValue(), result.get()))
         return;
-    v8::Handle<v8::Value> wrapper = toV8(result.get(), info.Holder(), info.GetIsolate());
+    v8::Handle<v8::Value> wrapper = toV8(result.get(), holder, isolate);
     if (!wrapper.IsEmpty()) {
-        V8HiddenValue::setHiddenValue(info.GetIsolate(), info.Holder(), v8AtomicString(info.GetIsolate(), "{{attribute.name}}"), wrapper);
+        V8HiddenValue::setHiddenValue(isolate, holder, v8AtomicString(info.GetIsolate(), "{{attribute.name}}"), wrapper);
         {{attribute.v8_set_return_value}};
     }
     {% elif world_suffix %}
     {{attribute.v8_set_return_value_for_main_world}};
     {% else %}
     {{attribute.v8_set_return_value}};
     {% endif %}
 }
 {% endfilter %}
 {% endmacro %}
@@ skipping 92 matching lines @@
 {%- if attribute.is_expose_js_accessors %}
 v8::Local<v8::Value> jsValue, const v8::FunctionCallbackInfo<v8::Value>& info
 {%- else %}
 v8::Local<v8::Value> jsValue, const v8::PropertyCallbackInfo<void>& info
 {%- endif %})
 {
     {% if attribute.is_reflect and attribute.idl_type == 'DOMString' and
           is_node %}
     {% set cpp_class, v8_class = 'Element', 'V8Element' %}
     {% endif %}
+    {# Local variables #}
+    {% if attribute.cached_attribute_validation_method or
+          attribute.has_setter_exception_state or
+          attribute.has_strict_type_checking or
+          attribute.is_call_with_execution_context or
+          attribute.is_setter_call_with_execution_context or
+          (attribute.idl_type == 'EventHandler' and not is_node) %}

[haraken, 2014/03/27 04:40:36]

Ditto.

+    v8::Isolate* isolate = info.GetIsolate();
+    {% endif %}
+    {% if not attribute.is_static %}
+    v8::Handle<v8::Object> holder = info.Holder();
+    {% endif %}
     {% if attribute.has_setter_exception_state %}
-    ExceptionState exceptionState(ExceptionState::SetterContext, "{{attribute.name}}", "{{interface_name}}", info.Holder(), info.GetIsolate());
+    ExceptionState exceptionState(ExceptionState::SetterContext, "{{attribute.name}}", "{{interface_name}}", holder, isolate);
     {% endif %}
+    {# Type checking #}
     {% if attribute.has_strict_type_checking %}
     {# Type checking for interface types (if interface not implemented, throw
        TypeError), per http://www.w3.org/TR/WebIDL/#es-interface #}
-    if (!isUndefinedOrNull(jsValue) && !V8{{attribute.idl_type}}::hasInstance(jsValue, info.GetIsolate())) {
+    if (!isUndefinedOrNull(jsValue) && !V8{{attribute.idl_type}}::hasInstance(jsValue, isolate)) {
         exceptionState.throwTypeError("The provided value is not of type '{{attribute.idl_type}}'.");
         exceptionState.throwIfNeeded();
         return;
     }
     {% endif %}
     {# impl #}
     {% if attribute.put_forwards %}
-    {{cpp_class}}* proxyImpl = {{v8_class}}::toNative(info.Holder());
+    {{cpp_class}}* proxyImpl = {{v8_class}}::toNative(holder);
     {{attribute.ref_ptr}}<{{attribute.idl_type}}> impl = WTF::getPtr(proxyImpl->{{attribute.name}}());
     if (!impl)
         return;
     {% elif not attribute.is_static %}
-    {{cpp_class}}* impl = {{v8_class}}::toNative(info.Holder());
+    {{cpp_class}}* impl = {{v8_class}}::toNative(holder);
     {% endif %}
     {% if attribute.is_implemented_by and not attribute.is_static %}
     ASSERT(impl);
     {% endif %}
     {% if attribute.idl_type == 'EventHandler' and interface_name == 'Window' %}
     if (!impl->document())
         return;
     {% endif %}
     {# Convert JS value to C++ value #}
     {% if attribute.idl_type != 'EventHandler' %}
     {{attribute.v8_value_to_local_cpp_value}};
     {% elif not is_node %}{# EventHandler hack #}
-    moveEventListenerToNewWrapper(info.Holder(), {{attribute.event_handler_getter_expression}}, jsValue, {{v8_class}}::eventListenerCacheIndex, info.GetIsolate());
+    moveEventListenerToNewWrapper(holder, {{attribute.event_handler_getter_expression}}, jsValue, {{v8_class}}::eventListenerCacheIndex, isolate);
     {% endif %}
     {% if attribute.enum_validation_expression %}
     {# Setter ignores invalid enum values: http://www.w3.org/TR/WebIDL/#idl-enums #}
     String string = cppValue;
     if (!({{attribute.enum_validation_expression}}))
         return;
     {% endif %}
     {# Pre-set context #}
     {% if attribute.is_custom_element_callbacks or
           (attribute.is_reflect and
            not(attribute.idl_type == 'DOMString' and is_node)) %}
     {# Skip on compact node DOMString getters #}
     CustomElementCallbackDispatcher::CallbackDeliveryScope deliveryScope;
     {% endif %}
     {% if attribute.is_call_with_execution_context or
           attribute.is_setter_call_with_execution_context %}
-    ExecutionContext* scriptContext = currentExecutionContext(info.GetIsolate());
+    ExecutionContext* scriptContext = currentExecutionContext(isolate);
     {% endif %}
     {# Set #}
     {{attribute.cpp_setter}};
     {# Post-set #}
     {% if attribute.is_setter_raises_exception %}
     exceptionState.throwIfNeeded();
     {% endif %}
     {% if attribute.cached_attribute_validation_method %}
-    V8HiddenValue::deleteHiddenValue(info.GetIsolate(), info.Holder(), v8AtomicString(info.GetIsolate(), "{{attribute.name}}")); // Invalidate the cached value.
+    V8HiddenValue::deleteHiddenValue(isolate, holder, v8AtomicString(info.GetIsolate(), "{{attribute.name}}")); // Invalidate the cached value.
     {% endif %}
 }
 {% endfilter %}
 {% endmacro %}
 
 
 {##############################################################################}
 {% macro attribute_setter_callback(attribute, world_suffix) %}
 {% filter conditional(attribute.conditional_string) %}
 static void {{attribute.name}}AttributeSetterCallback{{world_suffix}}(
@@ skipping 25 matching lines @@
     {% endif %}
     {% if attribute.has_custom_setter %}
     {{v8_class}}::{{attribute.name}}AttributeSetterCustom(jsValue, info);
     {% else %}
     {{cpp_class}}V8Internal::{{attribute.name}}AttributeSetter{{world_suffix}}(jsValue, info);
     {% endif %}
     TRACE_EVENT_SET_SAMPLING_STATE("V8", "V8Execution");
 }
 {% endfilter %}
 {% endmacro %}