Bug 26948 – Need to refactor Worker to derive from AbstractWorker

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore JavaScript

[Andrew Wilson]

There's some duplicate code now in AbstractWorker and Worker - we need to refactor Worker to derive from AbstractWorker and remove the duplicate code.

[Andrew Wilson]

Created attachment 32257 [details]
Diff to aid in discussion of toJS() problem with webkit-dev - do not review

Attached my patch to set GenerateToJS on Worker.idl to work around a problem where constructing a Worker() was really yielding a SharedWorker.

[Andrew Wilson]

Created attachment 32616 [details]
proposed patch

[Andrew Wilson]

Comment on attachment 32616 [details]
proposed patch

Removing "review ?" for now, as it looks like there's a bug in the V8 GC behavior which I saw while working on another refactoring. I'll either upload a new patch or restore this to '?' once I finish my investigation.

[Andrew Wilson]

Created attachment 32892 [details]
proposed patch (fixed problem with previous patch that prevent V8 bindings from compiling)

V8AbstractWorker underwent some bit rot (V8Proxy refactoring moved some of the APIs it depended on, which was masked by the fact that SHARED_WORKERS is disabled by default).

[David Levin]

Comment on attachment 32892 [details]
proposed patch (fixed problem with previous patch that prevent V8 bindings from compiling)

Just a few thing to address (one nit and one bug).

> diff --git a/WebCore/bindings/js/JSWorkerCustom.cpp b/WebCore/bindings/js/JSWorkerCustom.cpp
> @@ -42,40 +40,7 @@ void JSWorker::mark()
>  {
>      DOMObject::mark();

I think this should be JSAbstractWorker::mark(); or even better just "Base::mark();" (and then this would have not have needed to change).



> diff --git a/WebCore/bindings/v8/custom/V8WorkerCustom.cpp b/WebCore/bindings/v8/custom/V8WorkerCustom.cpp
> -    if (args.Length() == 0) {
> +    if (args.Length() == 0)
If you're going to fix the style here

       if (!args.Length())

is what you want.

[Andrew Wilson]

Created attachment 32909 [details]
Patch addressing Levin's comments

[David Levin]

Comment on attachment 32909 [details]
Patch addressing Levin's comments

LayoutTests/fast/workers/worker-constructor.html fails to apply cleanly.  Please update the patch and I'll review/land ot promptly.

[Andrew Wilson]

Created attachment 32959 [details]
Fixed up merge errors with worker-constructor layout test.

[David Levin]

Committed as http://trac.webkit.org/changeset/46048