IDL: Remove a FIXME comment for using local variables

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 %})
 {
@@ skipping 30 matching lines @@
     {% if attribute.is_call_with_script_state %}
     ScriptState* scriptState = ScriptState::current(info.GetIsolate());
     {% endif %}
     {% if attribute.is_check_security_for_node or
           attribute.is_getter_raises_exception %}
     ExceptionState exceptionState(ExceptionState::GetterContext, "{{attribute.name}}", "{{interface_name}}", holder, info.GetIsolate());
     {% endif %}
     {% if attribute.is_nullable and not attribute.has_type_checking_nullable %}
     bool isNull = false;
     {% endif %}
     {# FIXME: consider always using a local variable for value #}

[bashi, 2014/06/27 06:20:31]

BTW, what's your take on this comment? There is a comment on getter_context() in
v8_attribute.py for this.

    # Normally we can inline the function call into the return statement to
    # avoid the overhead of using a Ref<> temporary, but for some cases
    # (nullable types, EventHandler, [CachedAttribute], or if there are
    # exceptions), we need to use a local variable.
    # FIXME: check if compilers are smart enough to inline this, and if so,
    # always use a local variable (for readability and CG simplicity).

The behavior of |cpp_value_original| and |cpp_value| is a bit confusing. If we
can always use local vars for |cpp_value|, I think we can remove
|cpp_value_original|. How I can measure the performance impact of this?

[Jens Widell, 2014/06/27 06:31:23]

If we keep the use of a local optional, then we could at least change the
condition below to

  {% if attribute.cpp_value_original %}

to avoid duplicating the actual condition, which must be the same here and in
getter_context() anyway.

[haraken, 2014/06/27 06:35:09]

You can run Dromaeo to see performance (http://dromaeo.com/?dom). If it looks
OK, we can just land the CL and watch perf bots.

I agree that we should remove the confusion about |cpp_value_original| and
|cpp_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)) {
         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}}
@@ skipping 262 matching lines @@
     {% endif %}
     {% if attribute.has_custom_setter %}
     {{v8_class}}::{{attribute.name}}AttributeSetterCustom(v8Value, info);
     {% else %}
     {{cpp_class}}V8Internal::{{attribute.name}}AttributeSetter{{world_suffix}}(v8Value, info);
     {% endif %}
     TRACE_EVENT_SET_SAMPLING_STATE("v8", "V8Execution");
 }
 {% endfilter %}
 {% endmacro %}