Reimplement [PutForwards] per spec

third_party/WebKit/Source/bindings/templates/attributes.cpp.tmpl

Index: third_party/WebKit/Source/bindings/templates/attributes.cpp.tmpl
diff --git a/third_party/WebKit/Source/bindings/templates/attributes.cpp.tmpl b/third_party/WebKit/Source/bindings/templates/attributes.cpp.tmpl
index 93dd7c8e93b4aa240283c6c2d70bc6a8232ac12d..41892f8c99c75cd7c9a4047fb18e2e9544ba756f 100644
--- a/third_party/WebKit/Source/bindings/templates/attributes.cpp.tmpl
+++ b/third_party/WebKit/Source/bindings/templates/attributes.cpp.tmpl
@@ -299,14 +299,11 @@ v8::Local<v8::Value> v8Value, const v8::FunctionCallbackInfo<v8::Value>& info
     return; // Return silently because of [LenientThis].
   {% endif %}
 
-  {% if not attribute.is_static and not attribute.is_replaceable %}
+  {% set check_security_for_receiver = attribute.is_check_security_for_receiver and not attribute.is_data_type_property %}

[Yuki, 2017/05/08 09:31:19]

nit: If this is not a big deal, I'd prefer not aliasing, especially when it's
used only once.

|attribute.is_check_security_for_receiver|
should be good enough to understand, and |is_data_type_property| should be gone
(in future).

+
+  {% if not attribute.is_static and not attribute.is_replaceable and
+        (not attribute.is_put_forwards or check_security_for_receiver) %}
   v8::Local<v8::Object> holder = info.Holder();

[Yuki, 2017/05/08 09:31:19]

Personally, I'd prefer the previous code rather than the new one.

* holder = info.Holder()
This is just an alias.  Why we didn't do this aliasing for the case of |not
attribute.is_static| is just because static attributes never use |holder|
(because it's static).
Probably |attribute.is_replaceable| wouldn't use |holder|, too.
We could do this aliasing unconditionally with using ALLOW_UNUSED.
Anyway, my preference is that I don't want to make this condition more complex.

-  {% if attribute.is_put_forwards %}
-  {{cpp_class}}* proxyImpl = {{v8_class}}::toImpl(holder);
-  {{attribute.cpp_type}} impl = WTF::GetPtr(proxyImpl->{{attribute.name}}());
-  if (!impl)
-    return;
-  {% else %}
   {% set local_dom_window_only = interface_name == 'Window' and not attribute.has_cross_origin_setter %}
   {% if local_dom_window_only %}
   {% if attribute.is_check_security_for_receiver %}
@@ -321,9 +318,8 @@ v8::Local<v8::Value> v8Value, const v8::FunctionCallbackInfo<v8::Value>& info
   {{cpp_class}}* impl = {{v8_class}}::toImpl(holder);
   {% endif %}{# local_dom_window_only #}
   {% endif %}
-  {% endif %}
 
-  {% if attribute.is_check_security_for_receiver and not attribute.is_data_type_property %}
+  {% if check_security_for_receiver %}
   // Perform a security check for the receiver object.
   {{define_exception_state}}
   {% if local_dom_window_only %}
@@ -343,6 +339,17 @@ v8::Local<v8::Value> v8Value, const v8::FunctionCallbackInfo<v8::Value>& info
 #error Attribute setter with the security check for the return value is not supported.  Since the return value is the given value to be set, it\'s meaningless to perform the security check for the return value.
   {% endif %}
 
+  {% if attribute.is_put_forwards %}
+  {{define_exception_state}}
+  v8::Local<v8::Value> target;
+  if (!info.Holder()->Get(info.GetIsolate()->GetCurrentContext(), V8String(info.GetIsolate(), "{{attribute.name}}")).ToLocal(&target))

[Yuki, 2017/05/08 09:31:19]

nit: You could use |holder| instead of |info.GetHolder()|.

+    return;
+  if (!target->IsObject()) {
+    exceptionState.ThrowTypeError("The attribute value is not an object");
+    return;
+  }
+  target.As<v8::Object>()->Set(info.GetIsolate()->GetCurrentContext(), V8String(info.GetIsolate(), "{{attribute.target_attribute_name}}"), v8Value).IsNothing();

[Yuki, 2017/05/08 09:31:19]

IsNothing() doesn't make sense here.

IIUC, you should do:

  bool result;
  if (!target.Set(...).To(&result))
    return;
  if (!result) {
    // In this case, no exception should be thrown, but
    // somehow we failed to set the value.  Then, shall
    // we throw something?
    exceptionState->ThrowSomething(...);
    return;
  }

Or, if we believe that it never fails, DCHECK is okay.

+  {% else %}

[Yuki, 2017/05/08 09:31:19]

nit: {% else %}{# attribute.is_put_forwards %}

   {% 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.
@@ -408,6 +415,7 @@ v8::Local<v8::Value> v8Value, const v8::FunctionCallbackInfo<v8::Value>& info
       isolate, "{{cpp_class}}#{{attribute.name.capitalize()}}")
       .DeleteProperty(holder, v8::Undefined(isolate));
   {% endif %}
+  {% endif %}{# is_put_forwards #}

[Yuki, 2017/05/08 09:31:19]

nit: {# attribute.is_put_forwards #}

 }
 {% endfilter %}{# format_remove_duplicates #}
 {% endmacro %}