Make NodeFilter a legacy callback interface.

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

Index: third_party/WebKit/Source/bindings/templates/callback_interface.cpp.tmpl
diff --git a/third_party/WebKit/Source/bindings/templates/callback_interface.cpp.tmpl b/third_party/WebKit/Source/bindings/templates/callback_interface.cpp.tmpl
index e3086434b4f8ff97ef1aec9db449c285ec215bdd..39c03fefdf3c4cb911a2184caba990ed31996e64 100644
--- a/third_party/WebKit/Source/bindings/templates/callback_interface.cpp.tmpl
+++ b/third_party/WebKit/Source/bindings/templates/callback_interface.cpp.tmpl
@@ -9,6 +9,64 @@
 
 namespace blink {
 
+{% if is_legacy %}

[bashi, 2017/04/27 08:49:49]

It seems that we should have a new template file
(legacy_callback_interface.{cpp,h}.tmpl).

I think you can modify CodeGeneratorV8.generate_interface_code() to select
appropriate template files. You will need to create Jinja2 context before
selecting templates.

[tkent, 2017/04/27 09:15:12]

IMO, we should not have a separated template. Ideally we should fix
crbug.com/630986, and NodeFilter uses code generated with the normal callback
interface template.

[bashi, 2017/04/27 23:12:32]

How much code you are expecting to have overlaps between legacy callback
interface and non legacy callback interface? Seeing this CL I would suspect
there might be only a few things to share between them and there are large
amount of differences (e.g. non legacy callback doesn't need WrapperTypeInfo).

[tkent, 2017/04/28 00:09:43]

Well, suppose that:
A: Code amount to support non-legacy callback interface
B: Code amount to support legacy callback interface object; WrapperTypeInfo,
DomTemplate, etc.

A in callback_interface.h.tmpl: 20 lines
B in callback_interface.h.tmpl: 7 lines

A in callback_interface.cpp.tmpl: 62 lines
B in callabck_interface.cpp.tmpl: 54 lines

At this moment NodeFilter needs only B, but it should use A+B in the future. I
guess A in *.cpp.tmpl will be increased to fix crbug.com/630986.

[bashi, 2017/04/28 00:59:09]

Still I'd prefer having separate templates. Rationale:
- The number of lines wouldn't be a good metric for code health. Readability
overs writing cost. This CL doesn't share almost all code between legacy and non
legacy callback interface.
- If there are dups we can use Jinja2 macros.
- I'm still not sure how many code we will share between legacy and non legacy
callback interface. If they should use the same template because both hold a v8
value, should we use the same template for callback function as well? Until we
have clear signal I'd prefer keep them separate things.

[tkent, 2017/04/28 06:13:36]

ok, done in Patch Set 2.

+// Suppress warning: global constructors, because struct WrapperTypeInfo is trivial

[bashi, 2017/04/27 08:49:49]

nit: Wrap to 80 columns?

[tkent, 2017/04/28 06:13:36]

Done.

+// and does not depend on another global objects.
+#if defined(COMPONENT_BUILD) && defined(WIN32) && COMPILER(CLANG)
+#pragma clang diagnostic push
+#pragma clang diagnostic ignored "-Wglobal-constructors"
+#endif
+const WrapperTypeInfo {{v8_class}}::wrapperTypeInfo = {
+  gin::kEmbedderBlink,
+  {{v8_class}}::DomTemplate,
+  nullptr,
+  nullptr,
+  nullptr,
+  "{{interface_name}}",
+  nullptr,
+  WrapperTypeInfo::kWrapperTypeNoPrototype,
+  WrapperTypeInfo::kObjectClassId,
+  WrapperTypeInfo::kNotInheritFromActiveScriptWrappable,
+  WrapperTypeInfo::kDependent
+};
+#if defined(COMPONENT_BUILD) && defined(WIN32) && COMPILER(CLANG)
+#pragma clang diagnostic pop
+#endif
+
+static void
+TypeErrorConstructorCallback(const v8::FunctionCallbackInfo<v8::Value>& info) {

[Yuki, 2017/04/27 12:18:29]

Due to the snapshot project, I think that this needs to be
CORE/MODULES_EXPORT'ed.  All callback functions registered to V8 need to be
exported.

peria@?

[peria, 2017/04/27 13:14:49]

Yes, it should.
The project is still under construction, but I'm happy if this callback function
is an EXPORTed static method of V8Class.

[tkent, 2017/04/28 06:13:36]

Done.

+  V8ThrowException::ThrowTypeError(info.GetIsolate(), "Illegal constructor: {{interface_name}}");
+}
+
+{% from 'constants.cpp.tmpl' import install_constants with context %}
+static void Install{{v8_class}}Template(v8::Isolate* isolate, const DOMWrapperWorld& world, v8::Local<v8::FunctionTemplate> interfaceTemplate) {
+  // Legacy callback interface doesn't need prototype.

[Yuki, 2017/04/27 12:18:29]

nit: s/doesn't need prototype/must not have a prototype object/

[tkent, 2017/04/28 06:13:36]

Done.

+  interfaceTemplate->RemovePrototype();
+
+  // Initialize the interface object's template.
+  V8DOMConfiguration::InitializeDOMInterfaceTemplate(isolate, interfaceTemplate,
+      {{v8_class}}::wrapperTypeInfo.interface_name, v8::Local<v8::FunctionTemplate>(),
+      {{v8_class}}::internalFieldCount);
+  interfaceTemplate->SetCallHandler(TypeErrorConstructorCallback);
+  //interfaceTemplate->SetLength({{interface_length}});

[bashi, 2017/04/27 08:49:49]

Remove?

[Yuki, 2017/04/27 12:18:29]

Should be explicitly set to zero because the spec defines so?

[tkent, 2017/04/28 06:13:36]

Done.  Set 0 explicitly.

+
+  v8::Local<v8::Signature> signature = v8::Signature::New(isolate, interfaceTemplate);
+  ALLOW_UNUSED_LOCAL(signature);
+  v8::Local<v8::ObjectTemplate> instanceTemplate = interfaceTemplate->InstanceTemplate();
+  ALLOW_UNUSED_LOCAL(instanceTemplate);
+  v8::Local<v8::ObjectTemplate> prototypeTemplate = interfaceTemplate->PrototypeTemplate();
+  ALLOW_UNUSED_LOCAL(prototypeTemplate);
+
+  // Register DOM constants.
+  {{install_constants() | indent(2)}}
+}
+
+v8::Local<v8::FunctionTemplate> {{v8_class}}::DomTemplate(v8::Isolate* isolate, const DOMWrapperWorld& world) {
+  return V8DOMConfiguration::DomClassTemplate(isolate, world, const_cast<WrapperTypeInfo*>(&wrapperTypeInfo), Install{{v8_class}}Template);
+}
+
+{% else %} {# is_legacy #}
+
 {{v8_class}}::{{v8_class}}(v8::Local<v8::Function> callback, ScriptState* scriptState)
     : m_scriptState(scriptState) {
   m_callback.Set(scriptState->GetIsolate(), callback);
@@ -71,6 +129,9 @@ DEFINE_TRACE({{v8_class}}) {
 }
 
 {% endfor %}
+
+{% endif %} {# is_legacy #}
+
 }  // namespace blink
 
 {% endfilter %}{# format_blink_cpp_source_code #}