[webkit-reviews] review granted: [Bug 22310] Worker exceptions should be printed to console : [Attachment 25215] proposed patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Alexey Proskuryakov <ap at webkit.org>'s
request for review:
Bug 22310: Worker exceptions should be printed to console
https://bugs.webkit.org/show_bug.cgi?id=22310

Attachment 25215: proposed patch
https://bugs.webkit.org/attachment.cgi?id=25215&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
> +	       UString errorMessage = exception->toString(exec);
> +	       JSObject* exceptionObject = exception->toObject(exec);
> +	       int lineNumber = exceptionObject->get(exec, Identifier(exec,
"line"))->toInt32(exec);
> +	       UString exceptionSourceURL = exceptionObject->get(exec,
Identifier(exec, "sourceURL"))->toString(exec);
> +
> +	       scriptExecutionContext->reportException(errorMessage,
lineNumber, exceptionSourceURL);

This seems like too much code to have here in line. I'd like to see a helper
function that does more of this work.

> +	       if (exec->hadException())
> +		   exec->clearException();

This if seems unnecessary. There's no reason to check for null before setting
the value to null.

Also, I think it's OK for new code to get and clear exceptions in the
JSGlobalObject without involving ExecState. The API to do it through ExecState
is the "old way" in my opinion.

> +	   UString errorMessage = exception->toString(exec);
> +	   JSObject* exceptionObject = exception->toObject(exec);
> +	   int lineNumber = exceptionObject->get(exec, Identifier(exec,
"line"))->toInt32(exec);
> +	   UString exceptionSourceURL = exceptionObject->get(exec,
Identifier(exec, "sourceURL"))->toString(exec);
> +
> +	  
m_workerContext->thread()->messagingProxy()->postWorkerException(errorMessage,
lineNumber, exceptionSourceURL);
> +	   exec->clearException();

Here's the same code repeated again. I think this validates my
request/suggestion that this be in a helper function.

> +	   virtual void reportException(const String& errorMessage, int
lineNumber, const String& sourceURL);

Instead of having a default that does nothing, perhaps it would be better to
have this be a pure virtual function. The default empty version is only handy
if we expect to derive classes that want to leave this out.

r=me as is -- please consider my suggestions.