Bug 23776 – Introduce 2 base classes to split WorkerMessaingProxy.

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Jian Li]

In order to support running Worker in other process, i.e. Chrome's worker process, we need to split WorkerMessagingProxy into two pieces.

[Jian Li]

Created attachment 27374 [details]
Proposed Patch

This patch only adds these two base classes. The next patch will change WorkerMessagingProxy to derive from these two base classes.

[David Levin]

ChangeLog:
  WorkerMessaingProxy sp

   // A proxy to talk to the worker context.
I'm not sure that this comment adds much info.

  // Creator.
This comment should be omitted.

   static WorkerContextProxyBase* create(const String& scriptURL, Worker* worker);
The param "worker" doesn't add any info so it should be omited.

Also I don't think this method is implemented yet, so it probably shouldn't be here.

Lastly, it bring up lifetime issues.  Is this class deleted by who ever creates it.  I suspect the lifetime will be more like WorkerMessagingProxy and this class will need a method like "void workerObjectDestroyed();"  (something to signal the class that the WorkerObject is done with it).


The per method comments throughout seem to repeat the method name and should just be omitted.

The vcproj file references .cpp files that aren't part of the patch.

I think GNUMakefile.am needs to have the header files added to it.  

[David Levin]

I was thinking about the Base suffix.  I don't think it is needed here because the classes are intended to pure virtual so there will not be a WorkerContextProxy that adds non virtual methods.

It is usually used when you want a class that is non-platform specific but files need to refer to the non base version.  For example, you could have "class FooBase" in platform but files in WebCore/dom would refer to class Foo and #include "foo.h" but foo.h would be in a platform specific directory like platform/win/foo.h.
   
Lastly, if implementation of these classes is going to be platform specific, then dom isn't the correct directory for them.  You'd want WebCore/platform.

[David Levin]

Actually, I take back the comment about moving them to platform because there is a dom dependency.

It is fine where it is, but it will need to be moved before implementing platform specific items.

[Jian Li]

Created attachment 27413 [details]
Proposed Patch

[David Levin]

Comment on attachment 27374 [details]
Proposed Patch

Marking as obsolete due to new patch.

[David Levin]

These issues appear unaddressed:

ChangeLog:
  WorkerMessaingProxy sp

   // A proxy to talk to the worker context.
I'm not sure that this comment adds much info.

  // Creator.
This comment should be omitted.

   static WorkerContextProxyBase* create(const String& scriptURL, Worker*
worker);
The param "worker" doesn't add any info so it should be omited.

Also I don't think this method is implemented yet, so it probably shouldn't be
here.

Lastly, it bring up lifetime issues.  Is this class deleted by who ever creates
it.  I suspect the lifetime will be more like WorkerMessagingProxy and this
class will need a method like "void workerObjectDestroyed();"  (something to
signal the class that the WorkerObject is done with it).


The per method comments throughout seem to repeat the method name and should
just be omitted.


I think GNUMakefile.am needs to have the header files added to it.  

[Jian Li]

Created attachment 27441 [details]
Proposed Patch

I fixed all the problems excepted those as commented below.

I still keep the comment for the whole class since it clarifies what the class is intended for.

I do not add workerObjectDestroyed to WorkerConextProxy class since I think we can merge it into terminateWorkerContext.

[David Levin]

Comment on attachment 27413 [details]
Proposed Patch

New patch makes this one obsolete.

[David Levin]

Looks good to me.

As suggested by dimich, after this check in, I think we'll need to move this and some other worker classes to a WebCore/workers dir (and then we'll have platform specific directories under that).

[Alexey Proskuryakov]

Comment on attachment 27441 [details]
Proposed Patch

> +namespace WebCore {
> +    class String;

We usually add an empty line here.

+        virtual void postMessageToWorkerContext(const String& message) = 0;
+        virtual void postMessageToWorkerObject(const String& message) = 0;

The argument name can be omitted in these functions.

r=me

[Alexey Proskuryakov]

Committed revision 40780.

When creating patches, please make sure that the current directory is WebKit root - that will make them easier to apply. To make directory scanning faster, you can pass a list to svn-create-patch, e.g.:

svn-create-patch WebCore LayoutTests/ChangeLog LayoutTests/fast/workers