[webkit-reviews] review denied: [Bug 23223] Teach ScheduledAction how to execute script in WorkerContext, in addition to a Document. : [Attachment 26586] Proposed patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Sat Jan 10 02:04:55 PST 2009

Alexey Proskuryakov <ap at webkit.org> has denied Dmitry Titov
<dimich at chromium.org>'s request for review:
Bug 23223: Teach ScheduledAction how to execute script in WorkerContext, in
addition to a Document.

Attachment 26586: Proposed patch

------- Additional Comments from Alexey Proskuryakov <ap at webkit.org>
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() ==

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.

More information about the webkit-reviews mailing list