[wrapper-tracing] Bring remoteplayback up to date

third_party/WebKit/Source/bindings/templates/callback_function.h.tmpl

 {% filter format_blink_cpp_source_code %}
 
 {% include 'copyright_block.txt' %}
 
 #ifndef {{cpp_class}}_h
 #define {{cpp_class}}_h
 
 {% for filename in header_includes %}
 #include "{{filename}}"
 {% endfor %}
 
 namespace blink {
 
 class ScriptState;
 {% for forward_declaration in forward_declarations %}
 class {{forward_declaration}};
 {% endfor %}
 
-class {{exported}}{{cpp_class}} final : public GarbageCollectedFinalized<{{cpp_class}}> {
+class {{exported}}{{cpp_class}} final : public GarbageCollectedFinalized<{{cpp_class}}>,
+                                        public TraceWrapperBase {

[Michael Lippautz, 2016/10/19 12:20:21]

If you think that the extra vtable ptr is too much, we would need to have
special handling in RemotePlayback::traceWrappers.

[haraken, 2016/10/19 12:32:38]

I don't think sizeof(callback function) really matters, so I think the vtable
ptr is fine.

However, would you help me understand why the callback function needs to inherit
from TraceWrapperBase? For example, we're tracing ElementRareData without making
ElementRareData TraceWrapperBase.

[Michael Lippautz, 2016/10/19 13:13:14]

Sure; ElementRareData and similar classes are part of the "special" cases in
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/heap/...

This macro generates the appropriate dispatchers. Ideally, all special cases
would just inherit from this TraceWrapperBase and we wouldn't need them.
However, I think for ElementRareData we want to save the vtable ptr. We could
revisit some of the cases and check whether they already have a vtable ptr and
if yes just inherit from TraceWrapperBase.

Additionally, we cannot use the special case macro for the callback in this CL
as the callback is part of modules, which ScriptWrappableVisitor and friends
(platform) cannot  see (linker).

[Marcel Hlopko, 2016/10/19 15:08:17]

To reiterate what Michael is saying, if a class is not inheriting from
ScriptWrappable, we need to tell somehow WrapperVisitor to trace it. Before we
only had WRAPPER_VISITOR_SPECIAL_CLASSES, where you had to add your class.
TraceWrapperBase solves the same problem, it marks the class so WrapperVisitor
traces it, but in a nicer way, because you do the change in your class, not in
some arcane WrapperVisitor.h file.

[haraken, 2016/10/19 15:31:43]

Thanks for the clarification -- yes, TraceWrappersBase definitely looks nicer.

I'm okay with handling ElementRareData (and a couple of classes where the object
size really matters) specially; e.g., keep using
WRAPPER_VISITOR_SPECIAL_CLASSES.

Also making ScriptWrappable inherit from TraceWrappersBase totally makes sense.

[Michael Lippautz, 2016/10/19 15:44:35]

Acknowledged.

 
 public:
     static {{cpp_class}}* create(v8::Isolate* isolate, v8::Local<v8::Function> callback)
     {
         return new {{cpp_class}}(isolate, callback);
     }
 
     ~{{cpp_class}}() = default;
 
     DECLARE_TRACE();
+    DECLARE_TRACE_WRAPPERS();
 
     bool call({{argument_declarations | join(', ')}});
 
     v8::Local<v8::Function> v8Value(v8::Isolate* isolate)
     {
         return m_callback.newLocal(isolate);
     }
 
     void setWrapperReference(v8::Isolate* isolate, const v8::Persistent<v8::Object>& wrapper)
     {
         DCHECK(!m_callback.isEmpty());
         m_callback.setReference(wrapper, isolate);
     }
 
 private:
     {{cpp_class}}(v8::Isolate* isolate, v8::Local<v8::Function>);
     ScopedPersistent<v8::Function> m_callback;
 };
 
 } // namespace blink
 
 #endif // {{cpp_class}}_h
 
 {% endfilter %}{# format_blink_cpp_source_code #}