[webkit-reviews] review denied: [Bug 26932] Add ENABLE(SHARED_WORKERS) flag and define SharedWorker APIs : [Attachment 32199] proposed patch

[bugzilla-daemon at webkit.org]

David Levin <levin at chromium.org> has denied Andrew Wilson
<atwilson at google.com>'s request for review:
Bug 26932: Add ENABLE(SHARED_WORKERS) flag and define SharedWorker APIs
https://bugs.webkit.org/show_bug.cgi?id=26932

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

------- Additional Comments from David Levin <levin at chromium.org>
This looks good in general.  There are a few things to address at the moment,
but it is looking good.


> diff --git a/LayoutTests/fast/workers/shared-worker-constructor.html-disabled
b/LayoutTests/fast/workers/shared-worker-constructor.html-disabled
> +try {
> +    new SharedWorker({toString:function(){throw "exception"}}, "name")
> +    log("FAIL: toString exception not propagated.");

According to
http://www.whatwg.org/specs/web-workers/current-work/#runtime-script-errors,

"Whenever a runtime script error occurs in one of the worker's scripts, if the
error did not occur while handling a previous script error, the user agent must
report the error using the WorkerGlobalScope object's onerror attribute.

For shared workers, if the error is still not handled afterwards, or if the
error occurred while handling a previous script error, the error should be
reported to the user."

So it sounds this the exception should not be propogated.


> +try {
> +    var foo = {toString:function(){new Worker(foo)}}
> +    new SharedWorker(foo, name);
> +    log("FAIL: no exception when trying to create workers recursively");

Same comment as above.


> diff --git a/WebCore/DerivedSources.make b/WebCore/DerivedSources.make
> +webcore_built_sources += \
> +	WebCore/bindings/js/JSAbstractWorkerCustom.cpp \
> +	WebCore/bindings/js/JSSharedWorkerConstructor.cpp \
> +	WebCore/bindings/js/JSSharedWorkerCustom.cpp \
> +	WebCore/workers/AbstractWorker.cpp \
> +	WebCore/workers/AbstractWorker.h \
> +	WebCore/workers/SharedWorker.cpp \
> +	WebCore/workers/SharedWorker.h

You're missing JSSharedWorkerConstructor.h in the above list.

> diff --git a/WebCore/WebCore.pro b/WebCore/WebCore.pro

This seems to be missing a line like this one:
    !contains(DEFINES, ENABLE_WORKERS=.): DEFINES += ENABLE_WORKERS=1

> +    SOURCES += \
> +	   bindings/js/JSAbstractWorkerCustom.cpp \
> +	   bindings/js/JSSharedWorkerConstructor.cpp \
> +	   bindings/js/JSSharedWorkerCustom.cpp \
> +	   workers/AbstractWorker.cpp \
> +	   workers/AbstractWorker.h \
> +	   workers/SharedWorker.cpp \
> +	   workers/SharedWorker.h

As you noted (well) before I did, the *.h need to be removed for this file.

> +
> +

It looks like there are typically not blank lines at this point in other
sections like this.
> +}


> diff --git a/WebCore/WebCore.xcodeproj/project.pbxproj
b/WebCore/WebCore.xcodeproj/project.pbxproj
>			isa = PBXGroup;
>			children = (
> +				2E4346350F546A8200B0F1BA /* Worker.idl */,

"Worker.idl" is already in this list, so this addition seems like a mistake.

>				2E4346330F546A8200B0F1BA /* Worker.cpp */,
>				2E4346340F546A8200B0F1BA /* Worker.h */,
>				2E4346350F546A8200B0F1BA /* Worker.idl */,


> diff --git a/WebCore/bindings/js/JSAbstractWorkerCustom.cpp
b/WebCore/bindings/js/JSAbstractWorkerCustom.cpp
> +#include "JSDOMGlobalObject.h"
> +#include "JSEventListener.h"
> +#include "AbstractWorker.h"

Sort these alphabetically.

Of course this file looks exceedingly similar to
bindings/js/JSWorkerCustom.cpp, but it sounds like that is on your roadmap to
clean that up.

Why not file a bug now about this so it is tracked?


> diff --git a/WebCore/bindings/js/JSEventTarget.cpp
b/WebCore/bindings/js/JSEventTarget.cpp

> +#if ENABLE(SHARED_WORKERS)
> +    if (AbstractWorker* abstractWorker = target->toAbstractWorker())
> +	   return toJS(exec, abstractWorker);
> +#endif
...
> +#if ENABLE(SHARED_WORKERS)
> +    CONVERT_TO_EVENT_TARGET(AbstractWorker)
> +#endif

Why aren't these SharedWorker instead of AbstractWorker?


> diff --git a/WebCore/bindings/js/JSSharedWorkerConstructor.cpp
b/WebCore/bindings/js/JSSharedWorkerConstructor.cpp
> +#include "JSDOMWindowCustom.h"
> +#include "JSSharedWorker.h"
> +#include "SharedWorker.h"

Sort these headers.

> +JSSharedWorkerConstructor::JSSharedWorkerConstructor(ExecState* exec,
JSDOMGlobalObject* globalObject)
> +    :
DOMObject(JSSharedWorkerConstructor::createStructure(exec->lexicalGlobalObject(
)->objectPrototype()))
> +{
> +    putDirect(exec->propertyNames().prototype,
JSSharedWorkerPrototype::self(exec, globalObject), None);

This file looks ok, but I'm weak on JSC bindings, so it would be great to get
this file glanced at by someone who knows it better (for example ap, olliej,
weinig, ...).

That being said the JSWorkerConstructor has the following line which you
omitted:

    putDirect(exec->propertyNames().length, jsNumber(exec, 1),
ReadOnly|DontDelete|DontEnum);

Why?  (To be honest I don't know why this line is necessary in that other file)



> diff --git a/WebCore/dom/EventTarget.h b/WebCore/dom/EventTarget.h
> +    class SharedWorker;

Sorting is done case sensitive so SharedWorker comes after
ScriptExecutionContext. (ScriptExecutionContext comes after SVGElementInstance
because ascii('c') > ascii('V')).

>      class SVGElementInstance;
>      class ScriptExecutionContext;


> diff --git a/WebCore/workers/AbstractWorker.cpp
b/WebCore/workers/AbstractWorker.cpp
> +void AbstractWorker::dispatchScriptErrorEvent(const String&, const String&,
int)
> +{
> +    //FIXME(atwilson): Generate an ErrorEvent instead of a simple event

FIXME's in WebKit don't have names in them.  If people want to know who added
it, they can use the revision history.


> diff --git a/WebCore/workers/SharedWorker.h b/WebCore/workers/SharedWorker.h
> +	   SharedWorker(const String&, const String&, ScriptExecutionContext*,
ExceptionCode&);

I would keep the parameter names for the two strings here.