Bug 43216 – Script engine agnostic ScriptCallback class

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore JavaScript

[Luiz Agostini]

The objective is to  this kind of code to be script engine agnostic:

void doSomething(ScriptState* exec, ScriptValue function, bool p1, int p2, ...)
{
    ScriptCallback callback(exec, function);
    callback.appendArgument(p1);
    callback.appendArgument(p2);
    callback.call();
}

[Luiz Agostini]

Created attachment 62997 [details]
patch 1

[WebKit Review Bot]

Attachment 62997 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/3578627

[Luiz Agostini]

Created attachment 63001 [details]
patch 2

[WebKit Review Bot]

Attachment 63001 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/3631201

[Luiz Agostini]

Created attachment 63126 [details]
patch 3

[WebKit Review Bot]

Attachment 63126 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/3620319

[Luiz Agostini]

Created attachment 63760 [details]
patch 4

[Luiz Agostini]

Bug 37205 needs a callback class. I could have an abstract class and two concrete classes for javascript, a JS and a V8 classes.

Looking at file ScriptFunctionCall I have seen that it was almost what I needed to have a common class for JS and V8. This patch just does a small refactoring of class ScriptFunctionCall and implements the new callback class using NodeFilterCondition implementation as example (JS and V8).

Any comments?

[Kenneth Rohde Christiansen]

This looks OK with me. Sam, Oliver, can you have a quick look?

[Luiz Agostini]

Comment on attachment 63760 [details]
patch 4

It seems that this patch will not be usefull for bug 37205 as I thought it would at first. I will remove the review flag for now and will close the bug later, together with bug 37205.

[Darin Adler]

Comment on attachment 63760 [details]
patch 4

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

[Luiz Agostini]

Created attachment 67939 [details]
patch

[Luiz Agostini]

this latest patch is equal to previous one except by the virtual destructors that have been removed.

[Luiz Agostini]

Darin, is there anything else you would like to see in this patch?

[Darin Adler]

Comment on attachment 67939 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=67939&action=review

> WebCore/bindings/js/ScriptFunctionCall.cpp:219
> +    ASSERT(callType != CallTypeNone);

What makes this assertion correct? Can we ever get CallTypeNone here? We must only assert something we know will be true for some reason.

> WebCore/bindings/js/ScriptFunctionCall.h:69
> +    protected:
> +        JSC::MarkedArgumentBuffer m_arguments;
> +        ScriptState* m_exec;

Normally we would make data members private, and give derived classes access with protected accessor functions.

[Oliver Hunt]

Comment on attachment 67939 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=67939&action=review

> WebCore/bindings/js/ScriptFunctionCall.h:68
> +        JSC::MarkedArgumentBuffer m_arguments;

MarkedArgumentBuffer can't be heap allocated, so in addition to pulling MarkedArgumentBuffer up to this point you should also pull
// MarkedArgumentBuffer must be stack allocated, so prevent heap
void* operator new(size_t) { ASSERT_NOT_REACHED(); return reinterpret_cast<void*>(0xbadbeef); }
void* operator new[](size_t) { ASSERT_NOT_REACHED(); return reinterpret_cast<void*>(0xbadbeef); }

r-, because this could actually result in real bustage being introduced

[Kenneth Rohde Christiansen]

Comment on attachment 67939 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=67939&action=review

> WebCore/bindings/js/ScriptFunctionCall.h:-75
> -        JSC::MarkedArgumentBuffer m_arguments;

Oliver, it seems that that line was just refactored.

[Oliver Hunt]

(In reply to comment #17)
> (From update of attachment 67939 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=67939&action=review
> 
> > WebCore/bindings/js/ScriptFunctionCall.h:-75
> > -        JSC::MarkedArgumentBuffer m_arguments;
> 
> Oliver, it seems that that line was just refactored.

I know, however the guards to protect the class containing the argument buffer from heap allocation were not refactored with it.

[Luiz Agostini]

Created attachment 68410 [details]
patch

[Oliver Hunt]

Comment on attachment 68410 [details]
patch

r=me

[Luiz Agostini]

(In reply to comment #15)
> (From update of attachment 67939 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=67939&action=review
> 
> > WebCore/bindings/js/ScriptFunctionCall.cpp:219
> > +    ASSERT(callType != CallTypeNone);
> 
> What makes this assertion correct? Can we ever get CallTypeNone here? We must only assert something we know will be true for some reason.

It is possible since a ScriptValue is received in constructor and it may not be callable. Am I wrong?

> 
> > WebCore/bindings/js/ScriptFunctionCall.h:69
> > +    protected:
> > +        JSC::MarkedArgumentBuffer m_arguments;
> > +        ScriptState* m_exec;
> 
> Normally we would make data members private, and give derived classes access with protected accessor functions.

Sure. This time I just kept it as it was before.

[WebKit Commit Bot]

Comment on attachment 68410 [details]
patch

Clearing flags on attachment: 68410

Committed r68074: <http://trac.webkit.org/changeset/68074>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Darin Adler]

(In reply to comment #21)
> (In reply to comment #15)
> > > WebCore/bindings/js/ScriptFunctionCall.cpp:219
> > > +    ASSERT(callType != CallTypeNone);
> > 
> > What makes this assertion correct? Can we ever get CallTypeNone here? We must only assert something we know will be true for some reason.
> 
> It is possible since a ScriptValue is received in constructor and it may not be callable. Am I wrong?

Yes, the assertion is wrong, and thus the code is wrong! We need to handle that case.

Please don’t use this class for anything until you fix that!