Reimplement [PutForwards] per spec

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

 {% from 'utilities.cpp.tmpl' import declare_enum_validation_variable, v8_value_to_local_cpp_value %}
 
 {##############################################################################}
 {% macro attribute_getter(attribute, world_suffix) %}
 static void {{attribute.name}}AttributeGetter{{world_suffix}}(
 {%- if attribute.is_data_type_property %}
 const v8::PropertyCallbackInfo<v8::Value>& info
 {%- else %}
 const v8::FunctionCallbackInfo<v8::Value>& info
 {%- endif %}) {
@@ skipping 281 matching lines @@
   ExceptionState exceptionState(isolate, ExceptionState::kSetterContext, "{{interface_name}}", "{{attribute.name}}");
   {%- endset %}
 
   {% if attribute.is_lenient_this %}
   // [LenientThis]
   // Make sure that info.Holder() really points to an instance if [LenientThis].
   if (!{{v8_class}}::hasInstance(info.Holder(), isolate))
     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 %}
   {{cpp_class}}* uncheckedImpl = {{v8_class}}::toImpl(holder);
   {% else %}
   // Same-origin attributes setters are never exposed via the cross-origin
   // interceptors. Since same-origin access requires a LocalDOMWindow, it is
   // safe to downcast here.
   LocalDOMWindow* impl = ToLocalDOMWindow({{v8_class}}::toImpl(holder));
   {% endif %}{# attribute.is_check_security_for_receiver #}
   {% else %}
   {{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 %}
   if (!BindingSecurity::ShouldAllowAccessTo(CurrentDOMWindow(isolate), uncheckedImpl, exceptionState)) {
   {% else %}
   if (!BindingSecurity::ShouldAllowAccessTo(CurrentDOMWindow(isolate), impl, exceptionState)) {
   {% endif %}{# local_dom_window_only #}
     V8SetReturnValue(info, v8Value);
     return;
   }
   {% if local_dom_window_only %}
   LocalDOMWindow* impl = ToLocalDOMWindow(uncheckedImpl);
   {% endif %}{# local_dom_window_only #}
   {% endif %}
 
   {% if attribute.is_check_security_for_return_value %}
 #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.
   V0CustomElementProcessingStack::CallbackDeliveryScope deliveryScope;
   {% endif %}
 
   {% if attribute.has_setter_exception_state %}
   {{define_exception_state}}
   {% endif %}
 
@@ skipping 45 matching lines @@
   {% endif %}
   {{attribute.cpp_setter}};
 
   {% if attribute.cached_attribute_validation_method %}
   // [CachedAttribute]
   // Invalidate the cached value.
   V8PrivateProperty::GetSymbol(
       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 %}
 
 
 {##############################################################################}
 {% macro attribute_setter_callback(attribute, world_suffix) %}
 void {{v8_class_or_partial}}::{{attribute.name}}AttributeSetterCallback{{world_suffix}}(
 {%- if attribute.is_data_type_property %}
 v8::Local<v8::Name>, v8::Local<v8::Value> v8Value, const v8::PropertyCallbackInfo<void>& info
@@ skipping 97 matching lines @@
 static const V8DOMConfiguration::AccessorConfiguration accessorConfiguration[] = {
   {{attribute_configuration(attribute)}}
 };
 for (const auto& accessorConfig : accessorConfiguration)
   V8DOMConfiguration::InstallAccessor(isolate, world, v8::Local<v8::Object>(), prototypeObject, interfaceObject, signature, accessorConfig);
 {% endfilter %}{# runtime_enabled #}
 {% endfilter %}{# secure_context #}
 {% endfilter %}{# exposed #}
 {% endfor %}
 {% endmacro %}