First implementation of lazily cached accessor for DOM attributes.

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..24ad3f989f0b2c3cfe065b6a31155e632aecdf34 100644
--- a/third_party/WebKit/Source/bindings/templates/attributes.cpp.tmpl
+++ b/third_party/WebKit/Source/bindings/templates/attributes.cpp.tmpl
@@ -257,6 +257,16 @@ v8::Local<v8::Private> {{v8_class_or_partial}}::{{attribute.name}}CachedProperty
 }
 {% endmacro %}
 
+{##############################################################################}
+{% macro attribute_cache_init_callback(attribute) %}
+void {{attribute.name}}CacheInitCallback(v8::Isolate* isolate, v8::Local<v8::Object> holder)
+{
+    auto prop = V8PrivateProperty::Get{{attribute.cached_accessor_name}}(isolate);

[Yuki, 2017/05/09 06:06:31]

We may want to use V8PrivateProperty::GetSymbol(isolate,
"{{interface_name}}#{{attribute.cached_accessor_name}}").  This is slower than
Get{{attribute.cached_accessor_name}}(), but CacheInitCallback should be rarely
called and it should be okay, I think.

+    {{cpp_class}}* impl = {{v8_class}}::toImpl(holder);
+    prop.Set(holder, ToV8({{attribute.cpp_value}}, holder, isolate));
+    //LOG(ERROR) << "{{attribute.name}}CacheInitCallback";

[Yuki, 2017/05/09 06:06:31]

nit: Remove this debugging code.

[nverne, 2017/05/10 02:11:49]

Done

+}
+{% endmacro %}
 
 {##############################################################################}
 {% macro constructor_getter_callback(attribute, world_suffix) %}
@@ -408,6 +418,12 @@ v8::Local<v8::Value> v8Value, const v8::FunctionCallbackInfo<v8::Value>& info
       isolate, "{{cpp_class}}#{{attribute.name.capitalize()}}")
       .DeleteProperty(holder, v8::Undefined(isolate));
   {% endif %}
+
+  {% if attribute.is_lazy_cached_accessor %}
+  // Call cache init callback to store the correct private property.
+  {{attribute.name}}CacheInitCallback(info.GetIsolate(), holder);
+  {% endif %}
+

[Yuki, 2017/05/09 06:06:31]

nit: Remove this empty line.

[nverne, 2017/05/10 02:11:49]

Done

 }
 {% endfilter %}{# format_remove_duplicates #}
 {% endmacro %}
@@ -469,7 +485,7 @@ const v8::FunctionCallbackInfo<v8::Value>& info
 
 
 {##############################################################################}
-{% macro attribute_configuration(attribute) %}
+{% macro attribute_accessor_configuration(attribute, is_attribute) %}

[Yuki, 2017/05/09 06:06:30]

nit: s/is_attribute/is_data_property/

IDL attributes are defined to be ES accessor properties in Web IDL spec, but
Blink is using ES data properties for some limited cases.

"attribute" shouldn't mean "data property", so let's call it "data property".

[nverne, 2017/05/10 02:11:49]

This macro builds either an AccessorConfiguration or an AttributeConfiguration
from a python attribute object.  The meaning we want is now captured in the two
macros attribute_configuration() and accessor_configuration() calling this one.
Previously we only had the former macro and it forced us to include the same
fields in the underlying C++ structs, but this change allows them to safely
diverge. 

How about if I rename the macro build_configuration_from_attribute() and have
the change the second parameter to a string specifying the type of config?

 {% from 'utilities.cpp.tmpl' import property_location %}
 {% if attribute.constructor_type %}
   {% if attribute.needs_constructor_getter_callback %}
@@ -491,10 +507,15 @@ const v8::FunctionCallbackInfo<v8::Value>& info
        if attribute.constructor_type else 'nullptr' %}
 {% set property_attribute = 'static_cast<v8::PropertyAttribute>(%s)' %
                             ' | '.join(attribute.property_attributes) %}
-{% set cached_accessor_callback =
+{% set cached_property_key =
        '%s::%sCachedPropertyKey' % (v8_class_or_partial, attribute.name)
        if attribute.is_cached_accessor else
        'nullptr' %}
+{% set cache_init_callback = 
+      '%sV8Internal::%sCacheInitCallback' % 
+          (cpp_class_or_partial, attribute.name)
+       if attribute.is_lazy_cached_accessor else
+       'nullptr' %}
 {% set holder_check = 'V8DOMConfiguration::kDoNotCheckHolder'
        if attribute.is_lenient_this else 'V8DOMConfiguration::kCheckHolder' %}
 {% if attribute.is_per_world_bindings %}
@@ -502,12 +523,62 @@ const v8::FunctionCallbackInfo<v8::Value>& info
 {% set setter_callback_for_main_world =
        '%sForMainWorld' % setter_callback
        if attribute.has_setter else 'nullptr' %}
-{"{{attribute.name}}", {{getter_callback_for_main_world}}, {{setter_callback_for_main_world}}, {{cached_accessor_callback}}, {{wrapper_type_info}}, {{property_attribute}}, {{property_location(attribute)}}, {{holder_check}}, V8DOMConfiguration::kMainWorld},
-{"{{attribute.name}}", {{getter_callback}}, {{setter_callback}}, {{cached_accessor_callback}}, {{wrapper_type_info}}, {{property_attribute}}, {{property_location(attribute)}}, {{holder_check}}, V8DOMConfiguration::kNonMainWorlds}
-{%- else %}
-{"{{attribute.name}}", {{getter_callback}}, {{setter_callback}}, {{cached_accessor_callback}}, {{wrapper_type_info}}, {{property_attribute}}, {{property_location(attribute)}}, {{holder_check}}, V8DOMConfiguration::kAllWorlds}
-{%- endif %}
-{%- endmacro %}
+{% endif %}
+
+
+{% set config_pre = {

[Yuki, 2017/05/09 06:06:31]

nit: I'd prefer naming this config_world_dependent or world_dependent_config.

+  "main" : [
+         '"%s"' % attribute.name,
+         getter_callback_for_main_world,
+         setter_callback_for_main_world,
+       ],
+  "other" : [       

[Yuki, 2017/05/09 06:06:31]

nit: "non-main" or something like that?
"non-main" is more specific and clearer than "other", I think.

+         '"%s"' % attribute.name,
+         getter_callback,
+         setter_callback,
+       ],
+} %}
+
+{% set accessor_only_fields = [] if is_attribute else [       

[Yuki, 2017/05/09 06:06:31]

nit: Can we fold this into config_post and rename it to
config_world_independent?

+      cached_property_key,
+      cache_init_callback,
+] %}
+
+{% set config_post = [       
+       wrapper_type_info,
+       property_attribute,
+       property_location(attribute),
+       holder_check,
+] %}
+
+{% if attribute.is_per_world_bindings %}

[Yuki, 2017/05/09 06:06:31]

Great improvement!  Can we go farther?

I'm wondering if we can define a constexpr function that takes world-dependent
config first and world-independent config last (or vice-versa) and return the
struct that we want, in order to make this code cleaner.

I vaguely think whether we can do something like the following.

  constexpr V8DOMConfiguration::AccessorConfiguration
  V8DOMConfiguration::BuildAccessorConfigration(...);

  V8DOMConfiguration::AccessorConfiguration accessor_config[] = {
    V8DOMConfiguration::BuildAccessorConfiguration(
      *{{world_dependent_config}}, *{{world_independent_config}}),
    ...,
  };

+  {% set main_config_list = config_pre["main"] + accessor_only_fields + 
+      config_post + ['V8DOMConfiguration::kMainWorld'] %}
+  {% set non_main_config_list = config_pre["other"] + accessor_only_fields + 
+      config_post + ['V8DOMConfiguration::kNonMainWorlds'] %}
+  {# Emit for main world then non-main.#}
+  {{'{'}}{{main_config_list | join(', ')}}{{'}'}}
+  {{','}}
+  {{'{'}}{{non_main_config_list | join(', ')}}{{'}'}}
+{% else %}
+  {% set all_worlds_config_list = config_pre["other"] + accessor_only_fields + 
+      config_post + ['V8DOMConfiguration::kAllWorlds'] %}
+  {# Emit only for all worlds #}
+  {{'{'}}{{all_worlds_config_list | join(', ')}}{{'}'}}
+{% endif %}
+{% endmacro %}
+
+{##############################################################################}
+{% macro attribute_configuration(attribute) %}
+{% set is_attribute = true %}

[Yuki, 2017/05/09 06:06:31]

nit: s/is_attribute/is_data_property/

+{{attribute_accessor_configuration(attribute, is_attribute)}}
+{% endmacro %}
+
+{##############################################################################}
+{% macro accessor_configuration(attribute) %}
+{% set is_attribute = false %}

[Yuki, 2017/05/09 06:06:31]

ditto

+{{attribute_accessor_configuration(attribute, is_attribute)}}
+{% endmacro %}
 
 {##############################################################################}
 {% macro install_conditionally_enabled_attributes_on_prototype() %}
@@ -516,7 +587,7 @@ const v8::FunctionCallbackInfo<v8::Value>& info
 {% filter secure_context(attribute.secure_context_test) %}
 {% filter runtime_enabled(attribute.runtime_enabled_feature_name) %}
 static const V8DOMConfiguration::AccessorConfiguration accessorConfiguration[] = {
-  {{attribute_configuration(attribute)}}
+  {{accessor_configuration(attribute)}}
 };
 for (const auto& accessorConfig : accessorConfiguration)
   V8DOMConfiguration::InstallAccessor(isolate, world, v8::Local<v8::Object>(), prototypeObject, interfaceObject, signature, accessorConfig);