Bug 23223 - Teach ScheduledAction how to execute script in WorkerContext, in addition to a Document.
Summary: Teach ScheduledAction how to execute script in WorkerContext, in addition to ...
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore JavaScript (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
Depends on:
Blocks: 22718
  Show dependency treegraph
Reported: 2009-01-09 18:18 PST by Dmitry Titov
Modified: 2009-01-11 00:07 PST (History)
1 user (show)

See Also:

Proposed patch (5.76 KB, patch)
2009-01-09 18:25 PST, Dmitry Titov
ap: review-
Details | Formatted Diff | Diff
test case (not for landing) (2.86 KB, patch)
2009-01-09 18:30 PST, Dmitry Titov
no flags Details | Formatted Diff | Diff
Updated patch (6.35 KB, patch)
2009-01-10 14:30 PST, Dmitry Titov
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dmitry Titov 2009-01-09 18:18:37 PST
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)
Comment 1 Dmitry Titov 2009-01-09 18:25:45 PST
Created attachment 26586 [details]
Proposed patch
Comment 2 Dmitry Titov 2009-01-09 18:30:21 PST
Created attachment 26587 [details]
test case (not for landing)
Comment 3 Alexey Proskuryakov 2009-01-10 02:04:55 PST
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.
Comment 4 Darin Adler 2009-01-10 10:25:39 PST
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.
Comment 5 Dmitry Titov 2009-01-10 14:30:54 PST
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.
Comment 6 Darin Adler 2009-01-10 14:40:36 PST
Comment on attachment 26590 [details]
Updated patch

> +    if (context->isDocument())
> +        execute(static_cast<Document*>(context));
> +    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 (context->isDocument())
        else {

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

Comment 7 Alexey Proskuryakov 2009-01-11 00:07:49 PST
Committed revision 39783.