[webkit-reviews] review denied: [Bug 25151] workers with syntax errors not firing error event : [Attachment 32493] Proposed Patch

[bugzilla-daemon at webkit.org]

David Levin <levin at chromium.org> has denied Jian Li <jianli at chromium.org>'s
request for review:
Bug 25151: workers with syntax errors not firing error event
https://bugs.webkit.org/show_bug.cgi?id=25151

Attachment 32493: Proposed Patch
https://bugs.webkit.org/attachment.cgi?id=32493&action=review

------- Additional Comments from David Levin <levin at chromium.org>
Just a few things to address/clarify.  Thanks.

> diff --git a/WebCore/ChangeLog b/WebCore/ChangeLog
> +2009-07-08  Jian Li	<jianli at chromium.org>
> +
> +	   Reviewed by NOBODY (OOPS!).
> +
> +	   Bug 25151 - workers with syntax errors not firing error event.
> +	   https://bugs.webkit.org/show_bug.cgi?id=25151
> +
> +	   This fixes the problem that an error event is not fired when failed

Consider this wording:

  This fixes the problem that an error event is not fired when the worker
script
  fails to load.  Some reasons this may occur are an invalid URL for the worker

  script, a cross-origin redirect, or a syntax error in the script.


Another problem is this changelog doesn't say much about why things were done. 
When I
first looked at the patch I thought another issue was being fixed but some
items were moved
around.  Function level comments in the changelog would be helpful here.

> diff --git a/WebCore/bindings/js/JSWorkerConstructor.cpp
b/WebCore/bindings/js/JSWorkerConstructor.cpp
> -    ExceptionCode ec = 0;
> -    RefPtr<Worker> worker = Worker::create(scriptURL, window->document(),
ec);
> -    setDOMException(exec, ec);
> +    RefPtr<Worker> worker = Worker::create(scriptURL, window->document());

Why is it ok to stop setting the dom exception?


> diff --git a/WebCore/bindings/v8/custom/V8WorkerCustom.cpp
b/WebCore/bindings/v8/custom/V8WorkerCustom.cpp
> -    ExceptionCode ec = 0;
> -    RefPtr<Worker> obj = Worker::create(toWebCoreString(scriptUrl), context,
ec);
> -
> -    if (ec)
> -	   return throwError(ec);
> +    RefPtr<Worker> obj = Worker::create(toWebCoreString(scriptUrl),
context);

Same question as above (but here it is throwError).

> diff --git a/WebCore/workers/WorkerMessagingProxy.cpp
b/WebCore/workers/WorkerMessagingProxy.cpp

>      virtual void performTask(ScriptExecutionContext* context)
>      {
> -	   if (!m_messagingProxy->askedToTerminate())
> -	       context->reportException(m_errorMessage, m_lineNumber,
m_sourceURL);
> +	   Worker* workerObject = m_messagingProxy->workerObject();
> +	   if (!workerObject || m_messagingProxy->askedToTerminate())
> +	       return;
> +
> +	   context->reportException(m_errorMessage, m_lineNumber, m_sourceURL);

> +	   workerObject->dispatchErrorEvent();

This changes the behavior for more than just the case that you're fixing.  Any
code that calls ScriptExecutionContext->reportException will get this behavior.
 Why is that the correct thing to do?

Also, is there a chromium change corresponding to this?

> diff --git a/WebCore/workers/WorkerMessagingProxy.h
b/WebCore/workers/WorkerMessagingProxy.h
> namespace WebCore {
...
> +	   friend class WorkerExceptionTask;

Why is this being added here?

> diff --git a/WebCore/workers/WorkerScriptLoader.cpp
b/WebCore/workers/WorkerScriptLoader.cpp
> +void WorkerScriptLoader::loadSynchronously(ScriptExecutionContext*
scriptExecutionContext, const String& url, URLCompletionPolicy
urlCompletionPolicy, CrossOriginLoadPolicy crossOriginLoadPolicy)
...
> +    CrossOriginRedirectPolicy crossOriginRedirectPolicy =
(crossOriginLoadPolicy == DenyCrossOriginLoad) ? DenyCrossOriginRedirect :
AllowCrossOriginRedirect;
    
> +void WorkerScriptLoader::loadAsynchronously(ScriptExecutionContext*
scriptExecutionContext, const String& url, URLCompletionPolicy
urlCompletionPolicy, CrossOriginLoadPolicy crossOriginLoadPolicy,
WorkerScriptLoaderClient* client)
...
> +    CrossOriginRedirectPolicy crossOriginRedirectPolicy =
(crossOriginLoadPolicy == DenyCrossOriginLoad) ? DenyCrossOriginRedirect :
AllowCrossOriginRedirect;

It would be nice to only have one place that does the conversion of
CrossOriginLoadPolicy to CrossOriginRedirectPolicy.  Right now there are two in
this file.

> +ResourceRequest*
WorkerScriptLoader::createResourceRequest(ScriptExecutionContext*
scriptExecutionContext, const String& url, URLCompletionPolicy
urlCompletionPolicy, CrossOriginLoadPolicy crossOriginLoadPolicy)

This should return a PassOwnPtr<ResourceRequest>