[Webkit-unassigned] [Bug 23223] Teach ScheduledAction how to execute script in WorkerContext, in addition to a Document.

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=23223


ap at webkit.org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #26586|review?                     |review-
               Flag|                            |




------- Comment #3 from ap at webkit.org  2009-01-10 02:04 PDT -------
(From update of attachment 26586)
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.


-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.