Bug 26948 - Need to refactor Worker to derive from AbstractWorker
Summary: Need to refactor Worker to derive from AbstractWorker
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore JavaScript (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Andrew Wilson
URL:
Keywords:
Depends on: shared-workers
Blocks:
  Show dependency treegraph
 
Reported: 2009-07-02 21:57 PDT by Andrew Wilson
Modified: 2009-07-17 14:20 PDT (History)
2 users (show)

See Also:


Attachments
Diff to aid in discussion of toJS() problem with webkit-dev - do not review (23.37 KB, patch)
2009-07-04 11:51 PDT, Andrew Wilson
no flags Details | Formatted Diff | Diff
proposed patch (24.61 KB, patch)
2009-07-11 11:09 PDT, Andrew Wilson
no flags Details | Formatted Diff | Diff
proposed patch (fixed problem with previous patch that prevent V8 bindings from compiling) (28.11 KB, patch)
2009-07-16 14:15 PDT, Andrew Wilson
levin: review-
Details | Formatted Diff | Diff
Patch addressing Levin's comments (28.15 KB, patch)
2009-07-16 18:14 PDT, Andrew Wilson
levin: review-
Details | Formatted Diff | Diff
Fixed up merge errors with worker-constructor layout test. (28.35 KB, patch)
2009-07-17 11:08 PDT, Andrew Wilson
levin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Andrew Wilson 2009-07-02 21:57:45 PDT
There's some duplicate code now in AbstractWorker and Worker - we need to refactor Worker to derive from AbstractWorker and remove the duplicate code.
Comment 1 Andrew Wilson 2009-07-04 11:51:01 PDT
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.
Comment 2 Andrew Wilson 2009-07-11 11:09:24 PDT
Created attachment 32616 [details]
proposed patch
Comment 3 Andrew Wilson 2009-07-12 10:32:56 PDT
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.
Comment 4 Andrew Wilson 2009-07-16 14:15:53 PDT
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).
Comment 5 David Levin 2009-07-16 17:15:42 PDT
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.
Comment 6 Andrew Wilson 2009-07-16 18:14:51 PDT
Created attachment 32909 [details]
Patch addressing Levin's comments
Comment 7 David Levin 2009-07-17 00:05:16 PDT
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.
Comment 8 Andrew Wilson 2009-07-17 11:08:24 PDT
Created attachment 32959 [details]
Fixed up merge errors with worker-constructor layout test.
Comment 9 David Levin 2009-07-17 14:20:09 PDT
Committed as http://trac.webkit.org/changeset/46048