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

Source/bindings/templates/attributes.cpp

Index: Source/bindings/templates/attributes.cpp
diff --git a/Source/bindings/templates/attributes.cpp b/Source/bindings/templates/attributes.cpp
index 78e926b5eec318616c47ebbee81b3bab0cf4247a..31882738ec36c143ebe2e97bb23f2c0ee99d5209 100644
--- a/Source/bindings/templates/attributes.cpp
+++ b/Source/bindings/templates/attributes.cpp
@@ -12,31 +12,37 @@ const v8::PropertyCallbackInfo<v8::Value>& info
           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);
@@ -47,11 +53,11 @@ const v8::PropertyCallbackInfo<v8::Value>& info
     {% 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;
@@ -71,7 +77,7 @@ const v8::PropertyCallbackInfo<v8::Value>& info
     {% 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;
@@ -89,7 +95,7 @@ const v8::PropertyCallbackInfo<v8::Value>& info
     }
     {% 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 %}
@@ -97,9 +103,9 @@ const v8::PropertyCallbackInfo<v8::Value>& info
     {{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 %}
@@ -212,13 +218,26 @@ v8::Local<v8::Value> jsValue, const v8::PropertyCallbackInfo<void>& info
           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;
@@ -226,12 +245,12 @@ v8::Local<v8::Value> jsValue, const v8::PropertyCallbackInfo<void>& info
     {% 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);
@@ -244,7 +263,7 @@ v8::Local<v8::Value> jsValue, const v8::PropertyCallbackInfo<void>& info
     {% 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 #}
@@ -261,7 +280,7 @@ v8::Local<v8::Value> jsValue, const v8::PropertyCallbackInfo<void>& info
     {% 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}};
@@ -270,7 +289,7 @@ v8::Local<v8::Value> jsValue, const v8::PropertyCallbackInfo<void>& info
     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 %}