Bug 23223 – Teach ScheduledAction how to execute script in WorkerContext, in addition to a Document.

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

[Dmitry Titov]

For timers in Workers, we need the ScheduledAction to know how to execute the callback in WorkerContext. The patch adds execute(WorkerContext) method that does it.

Since there is no way currently to test the code, I've hacked enough of WorkerContext to be able to run "fake timer" and verify that the code works (separate test patch which is here only for info)

[Dmitry Titov]

Created attachment 26586 [details]
Proposed patch

[Dmitry Titov]

Created attachment 26587 [details]
test case (not for landing)

[Alexey Proskuryakov]

Comment on attachment 26586 [details]
Proposed patch

You need to conditionalize Worker support with ENABLE(WORKERS), or the patch will break ports that don't have workers enabled. r- because of this, some nitpicks below.

> +    
> +void ScheduledAction::executeFunctionInContext(JSGlobalObject* globalObject, JSValuePtr thisValue)

Extra whitespace on empty line (same issue below in this file), please remove

> +    ASSERT(workerContext->thread() && workerContext->thread()->threadID() == currentThread());

It is sub-optimal to have two checks in one ASSERT - if it fails, you won't immediately know which part failed. You could break it in two, or remove the first part completely (depending on how important it is to document this expectation in your opinion).

Maybe a similar assertion should be present in Document case for symmetry. Maybe not.

> +        void executeFunctionInContext(JSC::JSGlobalObject*, JSC::JSValuePtr /* thisValue */);

No need to comment out "thisValue" - this is a method declaration, so compiler won't complain about unused argument.

-        void execute(JSDOMWindowShell*);
+        void executeFunctionInContext(JSC::JSGlobalObject*, JSC::JSValuePtr /* thisValue */);
+        void execute(Document*);
+        void execute(WorkerContext*);

The combination of overloaded functions and manual dispatch for execute() is somewhat inelegant. Sounds like it calls for a virtual function in ScriptExecutionContext, or for ScriptController unification. But it's sufficiently clear and local, so I'm not asking you to do anything about it neither in this bug, nor in near future.

Also, executeFunctionInContext looks like it could mostly go in ScriptController. Again, not something to do now.

[Darin Adler]

Comment on attachment 26586 [details]
Proposed patch

> +        void executeFunctionInContext(JSC::JSGlobalObject*, JSC::JSValuePtr /* thisValue */);

There's no reason to comment out the name thisValue here. That only needs to be done when it's a function *definition* that doesn't use the argument. If it's a pure declaration like this that doesn't define the function you can and should just include the name.

[Dmitry Titov]

Created attachment 26590 [details]
Updated patch

Thanks for review!
Everything fixed in this patch, except making execute() method a virtual of ScriptExecutionContext as Alexey suggested. I really like the idea but feel it'd be better in a separate patch (no functionality change, just code move) so I've created a bug 23229 for that.

[Darin Adler]

Comment on attachment 26590 [details]
Updated patch

> +    if (context->isDocument())
> +        execute(static_cast<Document*>(context));
> +#if ENABLE(WORKERS)
> +    else {
> +        ASSERT(context->isWorkerContext());
> +        execute(static_cast<WorkerContext*>(context));
> +    }
> +#else
>      ASSERT(context->isDocument());
> +#endif

I think the code above is unnecessarily twisted. How about this?

    #if !ENABLE(WORKERS)
        ASSERT(context->isDocument());
        execute(static_cast<Document*>(context));
    #else
        if (context->isDocument())
            execute(static_cast<Document*>(context));
        else {
            ASSERT(context->isWorkerContext());
            execute(static_cast<WorkerContext*>(context));
        }
    #endif

I think Alexey's other original comments are definitely worth considering too. But this patch seems OK to land as-is.

r=me

[Alexey Proskuryakov]

Committed revision 39783.