[webkit-reviews] review denied: [Bug 43216] Script engine agnostic ScriptCallback class : [Attachment 63760] patch 4

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Luiz Agostini <luiz at webkit.org>'s
request for review:
Bug 43216: Script engine agnostic ScriptCallback class
https://bugs.webkit.org/show_bug.cgi?id=43216

Attachment 63760: patch 4
https://bugs.webkit.org/attachment.cgi?id=63760&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context:
https://bugs.webkit.org/attachment.cgi?id=63760&action=prettypatch

Seems like more of this code could be shared between the V8 and non-V8
versions. A lot of the changes are identical to both, which indicates we have
the factoring wrong

> WebCore/bindings/js/ScriptFunctionCall.cpp:201
> +ScriptCallback::ScriptCallback(ScriptState* exec, ScriptValue function)
> +    : ScriptCallArgumentHandler(exec)

I would name this argument “state” rather than “exec”.

> WebCore/bindings/js/ScriptFunctionCall.cpp:208
> +    bool hadException = false;

It does not seem necessary to initialize this boolean.

> WebCore/bindings/js/ScriptFunctionCall.h:52
> +	   ScriptCallArgumentHandler(ScriptState* exec) : m_exec(exec) {}
> +	   virtual ~ScriptCallArgumentHandler() {};

The argument should be named “state”, not “exec”. The braces should have a
single space between them (WebKit codings style).

Not something you introduced, but there is no reason for this class to have a
virtual destructor. It has no other virtual functions, and adding a virtual
destructor will make the object larger and not offer other benefits. I suggest
not declaring the destructor at all.

> WebCore/bindings/js/ScriptFunctionCall.h:76
> +	   virtual ~ScriptFunctionCall() {};

There’s no reason for this class to have a virtual destructor.

> WebCore/bindings/js/ScriptFunctionCall.h:94
> +	   ScriptCallback(ScriptState* exec, ScriptValue function);

There’s no need to name the ScriptState argument at all. Here.

review- because I’d really like to see those needless virtual destructors
removed, but the patch seems otherwise good