Refactor WorkerContext to move DedicatedWorker-specific APIs into DedicatedWorkerContext

[Andrew Wilson]

There are a set of APIs that are common to both DedicatedWorkerGlobalScope and SharedWorkerGlobalScope. We should keep those in WorkerContext, and refactor the DedicatedWorker-specific APIs (like onmessage/postMessage) into a derived class.

[Andrew Wilson]

Created attachment 33048 [details]
proposed patch

I'd like someone with some experience with the JS bindings internals to take a look at WebCore/bindings/js/WorkerScriptController::initScript where I'm setting up the prototype for the global context. I'll try to find someone on IRC on Monday.

[David Levin]

Comment on attachment 33048 [details]
proposed patch

Just some quick style nits that I noticed when I briefly looked at this.

> diff --git a/WebCore/ChangeLog b/WebCore/ChangeLog
> +        Reviewed by NOBODY (OOPS!).
> +
> +       Refactor WorkerContext to move DedicatedWorker-specific APIs into DedicatedWorkerContext

Off by one space.


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

>  #if ENABLE(WORKERS)
>  #include "JSWorker.h"
> -#include "JSWorkerContext.h"
> +#include "JSDedicatedWorkerContext.h"
>  #include "Worker.h"
> -#include "WorkerContext.h"
> +#include "DedicatedWorkerContext.h"

Out of sort order.


> diff --git a/WebCore/bindings/v8/V8Index.cpp b/WebCore/bindings/v8/V8Index.cpp
> @@ -59,8 +59,8 @@
> -#include "V8CSSVariablesRule.h"
> -#include "V8DataGridColumn.h"
> +#include "V8CSSVariablesRule.h"
> +#include "V8DataGridColumn.h"

> -#if ENABLE(DOM_STORAGE)
> -#include "V8Storage.h"
> -#include "V8StorageEvent.h"
> +#if ENABLE(DOM_STORAGE)
> +#include "V8Storage.h"
> +#include "V8StorageEvent.h"
>  #endif

It would be good to not have these unrelated changes in this patch.

> diff --git a/WebCore/bindings/v8/V8Index.h b/WebCore/bindings/v8/V8Index.h

> @@ -79,8 +80,8 @@ typedef v8::Persistent<v8::FunctionTemplate> (*FunctionTemplateFactory)();
> -    V(COMMENT, Comment)                                                 \
> -    V(DATAGRIDCOLUMN, DataGridColumn)                                   \
> +    V(COMMENT, Comment)                                                 \
> +    V(DATAGRIDCOLUMN, DataGridColumn)                                   \

> -#endif
> -
> -#if ENABLE(DOM_STORAGE)
> -#define DOM_OBJECT_STORAGE_TYPES(V)                                     \
> -    V(STORAGE, Storage)                                                 \
> -    V(STORAGEEVENT, StorageEvent)
> -#else
> -#define DOM_OBJECT_STORAGE_TYPES(V)
> +#endif
> +
> +#if ENABLE(DOM_STORAGE)
> +#define DOM_OBJECT_STORAGE_TYPES(V)                                     \
> +    V(STORAGE, Storage)                                                 \
> +    V(STORAGEEVENT, StorageEvent)
> +#else
> +#define DOM_OBJECT_STORAGE_TYPES(V)
>  #endif

It would be good to not have these unrelated changes in this patch.





> diff --git a/WebCore/dom/EventTarget.h b/WebCore/dom/EventTarget.h
> @@ -48,7 +48,7 @@ namespace WebCore {
>      class ScriptExecutionContext;
>      class SharedWorker;
>      class Worker;
> -    class WorkerContext;
> +    class DedicatedWorkerContext;

Out of sort order.


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

> diff --git a/WebCore/workers/WorkerThread.cpp b/WebCore/workers/WorkerThread.cpp
>  #include "PlatformString.h"
>  #include "ScriptSourceCode.h"
>  #include "ScriptValue.h"
> -#include "WorkerContext.h"
> +#include "DedicatedWorkerContext.h"
>  #include "WorkerObjectProxy.h"

Out of sort order.

[Andrew Wilson]

Created attachment 33068 [details]
Patch addressing levin's comments

Per our IRC conversation, I'm leaving the changes in this patch that remove the extraneous CRs from V8Index.cpp/h

[Dmitry Titov]

A note about v8-related changes. You might want to ask Mads Ager to take a look, since I feel a better v8 expert could be useful here :-)

It doesn't seem that V8ClassIndex::WORKERCONTEXT is removed from everywhere. I think it should be because this is an enum value that is embedded into a v8 wrapper as an id of its type. Lookups and type conversions use that as an enum entry into some switch statements and lookup tables. So if we now have V8ClassIndex::DEDICATEDWORKERCONTEXT, it should replace the old enum value everywhere. Later, you'll also have SHAREDWORKERCONTEXT.

For example, v8 version of WorkerContextExecutionProxy::retrieve() won't work since it won't be able to find a WORKERCONTEXT in a current v8 context... 

It might be easier (maybe) to add a virtual ScriptExecutionContext::IsDedicatedWorkerContext() (in addition to isDocument etc) and have a single type of the v8 wrapper but check and cast in runtime.

[David Levin]

Comment on attachment 33068 [details]
Patch addressing levin's comments

I'm marking this obsolete to move it out of the review queue because I'm fairly certain atwilson is doing some fixes to it.  Drew if I got this wrong please undo this :)

[Andrew Wilson]

Created attachment 33397 [details]
Patch addressing dimich's comments

I fixed up a number of places where we were doing the wrong thing in the V8 bindings.

Note that V8ClassIndex::WORKERCONTEXT can't go away entirely, as it's still used by the generated code in V8WorkerContext.cpp, and also used for generating the template chain for derived classes like V8DedicatedWorkerContext.

Also, it is OK to do lookupDOMWrapper(V8ClassIndex::WORKERCONTEXT, global) in WorkerContextExecutionProxy::retrieve(), because lookupDOMWrapper walks up the prototype chain to determine the class. Likewise for calling convertToNativeObject(WORKERCONTEXT, global) - it still works no matter what type you pass in, derived or base. In fact, convertToNativeObject() mostly just uses the passed-in type for error checking, as it always ends up pulling a pointer to the impl object out of the same place in the wrapper regardless of type anyway.

[Adam Barth]

Comment on attachment 33397 [details]
Patch addressing dimich's comments

Ok.  One infinitesimal nit:

> -    private:
> +    protected:
>          WorkerContext(const KURL&, const String&, WorkerThread*);
> +        bool isClosing() { return m_closing; }
> +    private:
>  
>          virtual void refScriptExecutionContext() { ref(); }
>          virtual void derefScriptExecutionContext() { deref(); }

Please move the private to below the blank line.

[Dmitry Titov]

Assigning to [e-mail] for landing.

[Andrew Wilson]

Created attachment 33459 [details]
patch fixing build error from last night's codegen changes

[Dmitry Titov]

Fixed per abarth's comment and landed: http://trac.webkit.org/changeset/46369