Bug 24054 - Implement two separate proxy classes in order to split WorkerMessagingProxy.
Summary: Implement two separate proxy classes in order to split WorkerMessagingProxy.
Status: RESOLVED LATER
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 24055
  Show dependency treegraph
 
Reported: 2009-02-19 20:49 PST by Jian Li
Modified: 2009-02-25 02:23 PST (History)
4 users (show)

See Also:


Attachments
Proposed Patch (24.38 KB, patch)
2009-02-19 20:52 PST, Jian Li
no flags Details | Formatted Diff | Diff
Proposed Patch (23.05 KB, patch)
2009-02-20 14:46 PST, Jian Li
no flags Details | Formatted Diff | Diff
Proposed Patch (23.39 KB, patch)
2009-02-20 15:59 PST, Jian Li
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jian Li 2009-02-19 20:49:05 PST
This is the first part of work in order to split WorkerMessagingProxy into two separate proxies. We need to add WorkerContextCrossThreadProxy and WorkerObjectCrossThreadProxy to do cross-thread messaging in WebKit.
Comment 1 Jian Li 2009-02-19 20:52:27 PST
Created attachment 27822 [details]
Proposed Patch
Comment 2 Alexey Proskuryakov 2009-02-19 23:36:20 PST
Is this patch intended for review? Please mark it as such by clicking Edit button to the right and choosing review? flag.
Comment 3 Dmitry Titov 2009-02-19 23:45:40 PST
Small naming comment: looking at this patch, I think WorkerObjectProxy and WorkerContextProxy have some names unnecessarily repeated. 

WorkerObjectCrossThreadProxy has some methods that are named xxxxxToWorkerObject and
WorkerContextCrossThreadProxy has some methods that are named xxxxxToWorkerContext.

It seems the 'ToWorkerObject' and 'ToWorkerContext' are unnecessary in method names since this is already implied in the naming of proxies themselves. It probably deserves a separate patch though.
Comment 4 Dmitry Titov 2009-02-20 00:25:06 PST
It seems that WorkerContextProxy::create(Worker*) is a bit unclear. It doesn't really create a WorkerContextProxy since the latter is an abstract interface.

It could be better to have the following initialization sequence:
1. Worker constructor always first creates a local proxy to itself, WorkerObjectCrossThreadProxy, passing itself as a parameter.
2. Then it can cast WorkerObjectCrossThreadProxy to base type WorkerObjectProxy and pass that into a global function createWorkerContextProxy(WorkerObjectProxy*), which in case of WebKit would create a WorkerContextCrossThreadProxy, and in case of Chromium - an IPC pipe and return something that implements WorkerContextProxy.

This also allows to get rid of WorkerContextCrossThreadProxy::setWorkerObjectProxy() sinceit'll be passed to constructor. 

Also, WorkerContextCrossThreadProxy does not seem to need m_parentScriptExecutionContext, and so does not need this constructor parameter. In Chromium case, it is also impossible to provide it in the worker process. Was it intended for postTaskToLoader() implementation?
Comment 5 David Levin 2009-02-20 00:45:59 PST
Comment on attachment 27822 [details]
Proposed Patch

I really like the direction here and think this has made a lot of progress in the right direction.

I have some comments/questions below -- some of which may be easiest to discuss tomorrow.


WebCore/dom/WorkerContextCrossThreadProxy.cpp
>  PassRefPtr<WorkerContextCrossThreadProxy> WorkerContextCrossThreadProxy::create(ScriptExecutionContext* parentScriptExecutionContext)
>  {
>      return adoptRef(new WorkerContextCrossThreadProxy(parentScriptExecutionContext));
>  }
What do you think about parentContext instead of parentScriptExecutionContext?

Also it looks like this is never used, which is just as it should be since WorkerContextCrossThreadProxy could be in a different process from the parent context.

Should it be removed? (Or does it serve some purpose like helping xhr in webkit until I get that fixed up :)?)

   
> +WorkerContextCrossThreadProxy::WorkerContextCrossThreadProxy(ScriptExecutionContext* parentScriptExecutionContext)
> +    : m_parentScriptExecutionContext(parentScriptExecutionContext)
> +    , m_workerObjectProxy(0)
> +    , m_askedToTerminate(false)
> +{
> +    ref();

I don't understand why there is this ref/deref thing.  Sure workerObject has to call workerObjectDestroyed, but since it has a pointer to this class, why not have it be a RefPtr<> and get rid of this internal ref/deref.


59 WorkerContextCrossThreadProxy::~WorkerContextCrossThreadProxy()
 60 {
 61     m_workerThread = 0;
 62 }

Why is m_workerThread set to zero here?


> +void WorkerContextCrossThreadProxy::postTaskToWorkerContext(PassRefPtr<ScriptExecutionContext::Task> task)
> +{
...
> +    if (m_workerThread) {
> +        m_workerThread->runLoop().postTask(task);
> +    } else

No {} here (single line statement).


> +void WorkerContextCrossThreadProxy::workerObjectDestroyed()
> +{
> +    terminateWorkerContext();
> +    m_workerObjectProxy->terminateWorkerObject();
> +    m_workerObjectProxy = 0;
It looks very odd that it calls terminateWorkerObject when it is told that the worker object was already destroyed. 


> Index: WebCore/dom/WorkerContextCrossThreadProxy.h

> +#include <wtf/Noncopyable.h>
Appears unused.

> +    class WorkerContextCrossThreadProxy : public WorkerContextProxy {
...
> +        virtual void startWorkerContext(const KURL& scriptURL, const String& userAgent, const String& sourceCode);
> +        virtual void terminateWorkerContext();
> +        virtual void postMessageToWorkerContext(const String&);
> +        void postTaskToWorkerContext(PassRefPtr<ScriptExecutionContext::Task>);
It would be nice if "[To]WorkerContext" were removed from all of these.


> +        RefPtr<ScriptExecutionContext> m_parentScriptExecutionContext;
As mentioned before, this appears unused.


> Index: WebCore/dom/WorkerMessagingWebKit.cpp
> ===================================================================
> --- WebCore/dom/WorkerMessagingWebKit.cpp	(revision 0)
> +++ WebCore/dom/WorkerMessagingWebKit.cpp	(revision 0)
> @@ -0,0 +1,53 @@
> +/*
> + * Copyright (C) 2008 Apple Inc. All Rights Reserved.
Why is there an Apple copyright here?


It would be great if all of this were done in Worker directory off of WebCore, since you are essentially defining a platform specific file now.


> +WorkerContextProxy* WorkerContextProxy::create(Worker* worker)
> +{
> +    RefPtr<WorkerContextCrossThreadProxy> workerContextProxy = WorkerContextCrossThreadProxy::create(worker->scriptExecutionContext());
> +    RefPtr<WorkerObjectCrossThreadProxy> workerObjectProxy = WorkerObjectCrossThreadProxy::create(worker);
> +    workerContextProxy->setWorkerObjectProxy(workerObjectProxy);
workerObjectProxy.release() ?

> +    workerObjectProxy->setWorkerContextProxy(workerContextProxy);
workerContextProxy.release() ?


> Index: WebCore/dom/WorkerObjectCrossThreadProxy.cpp
>
> +    
> +WorkerObjectCrossThreadProxy::WorkerObjectCrossThreadProxy(Worker* workerObject)
> +    : m_scriptExecutionContext(workerObject->scriptExecutionContext())
> +    , m_workerObject(workerObject)
> +    , m_askedToTerminate(false)
> +    , m_workerThreadHadPendingActivity(false)
> +{
> +    ASSERT(m_workerObject);
> +    ASSERT((m_scriptExecutionContext->isDocument() && isMainThread())
> +        || (m_scriptExecutionContext->isWorkerContext() && currentThread() == static_cast<WorkerContext*>(m_scriptExecutionContext.get())->thread()->threadID()));
> +
> +    ref();
I see you manage all the ref counts on this object on the WorkerObjectThread, but this feel fragile.  Exposing a ref count on an object that is used cross thread but only one thread is suppose to do ref counting.  Is there a way to avoid having this object be ref counted?


> +static void postMessageToWorkerObjectTask(ScriptExecutionContext*, WorkerObjectCrossThreadProxy* proxy, const String& message)
> +{
> +    if (proxy->askedToTerminate())
> +        return;
How do you know that proxy is still alive at this point?


> +void WorkerObjectCrossThreadProxy::postMessageToWorkerObject(const String& message)
> +{
> +    if (m_askedToTerminate)
> +        return;
I don't think if is the right place to check this, but we should discuss.


> Index: WebCore/dom/WorkerObjectCrossThreadProxy.h

> +    class WorkerObjectCrossThreadProxy : public WorkerObjectProxy {
...
> +        RefPtr<WorkerContextProxy> m_workerContextProxy;
Is this ever used?


> +        bool m_workerThreadHadPendingActivity; // The latest confirmation from worker thread reported that it was still active.

This appears never to be set.
Comment 6 Jian Li 2009-02-20 11:26:08 PST
The reason I do double ref-counting between WorkerContextCrossThreadProxy and WorkerObjectCrossThreadProxy is to manage the lifetime correctly. When the worker page is unloaded, the following destruction steps are executed:
1) Worker::terminate() is called and then it calls WorkerContextProxy::terminateWorkerContext(). Then WorkerThread::stop() is called to stop the worker thread.
2) GC kicks in and destructs Worker object. As the result, Worker::~Worker() calls WorkerContextProxy::workerObjectDestroyed().
3) After WorkerThread leaves its run loop, WorkerContext is destructed. As the result, WorkerContext::~WorkerContext calls WorkerObjectProxy::workerContextDestroyed.
Step 2 and 3 might be swapped depending when GC kicks in on different platform.

WorkerContextCrossThreadProxy contains WorkerThread object. The WorkerThread object should not be deleted before WorkerThread leaves its run loop.

When destructions are done in step 1, 2, and 3. WorkerContextCrossThreadProxy ::workerObjectDestroyed() could delete itself if there is not other reference being held. This will cause WorkerThread being destroyed before step 3 and crash could happen when some code is still executed.

So we let WorkerContextCrossThreadProxy and WorkerObjectCrossThreadProxy hold the reference to each time. Only after both Worker (step 2) and WorkerContext (step 3) are destroyed, we then delete both proxies.



(In reply to comment #5)
> (From update of attachment 27822 [details] [review])
> I really like the direction here and think this has made a lot of progress in
> the right direction.
> 
> I have some comments/questions below -- some of which may be easiest to discuss
> tomorrow.
> 
> 
> WebCore/dom/WorkerContextCrossThreadProxy.cpp
> >  PassRefPtr<WorkerContextCrossThreadProxy> WorkerContextCrossThreadProxy::create(ScriptExecutionContext* parentScriptExecutionContext)
> >  {
> >      return adoptRef(new WorkerContextCrossThreadProxy(parentScriptExecutionContext));
> >  }
> What do you think about parentContext instead of parentScriptExecutionContext?
> 
> Also it looks like this is never used, which is just as it should be since
> WorkerContextCrossThreadProxy could be in a different process from the parent
> context.
> 
> Should it be removed? (Or does it serve some purpose like helping xhr in webkit
> until I get that fixed up :)?)
> 
> 
> > +WorkerContextCrossThreadProxy::WorkerContextCrossThreadProxy(ScriptExecutionContext* parentScriptExecutionContext)
> > +    : m_parentScriptExecutionContext(parentScriptExecutionContext)
> > +    , m_workerObjectProxy(0)
> > +    , m_askedToTerminate(false)
> > +{
> > +    ref();
> 
> I don't understand why there is this ref/deref thing.  Sure workerObject has to
> call workerObjectDestroyed, but since it has a pointer to this class, why not
> have it be a RefPtr<> and get rid of this internal ref/deref.
> 
> 
> 59 WorkerContextCrossThreadProxy::~WorkerContextCrossThreadProxy()
>  60 {
>  61     m_workerThread = 0;
>  62 }
> 
> Why is m_workerThread set to zero here?
> 
> 
> > +void WorkerContextCrossThreadProxy::postTaskToWorkerContext(PassRefPtr<ScriptExecutionContext::Task> task)
> > +{
> ...
> > +    if (m_workerThread) {
> > +        m_workerThread->runLoop().postTask(task);
> > +    } else
> 
> No {} here (single line statement).
> 
> 
> > +void WorkerContextCrossThreadProxy::workerObjectDestroyed()
> > +{
> > +    terminateWorkerContext();
> > +    m_workerObjectProxy->terminateWorkerObject();
> > +    m_workerObjectProxy = 0;
> It looks very odd that it calls terminateWorkerObject when it is told that the
> worker object was already destroyed. 
> 
> 
> > Index: WebCore/dom/WorkerContextCrossThreadProxy.h
> 
> > +#include <wtf/Noncopyable.h>
> Appears unused.
> 
> > +    class WorkerContextCrossThreadProxy : public WorkerContextProxy {
> ...
> > +        virtual void startWorkerContext(const KURL& scriptURL, const String& userAgent, const String& sourceCode);
> > +        virtual void terminateWorkerContext();
> > +        virtual void postMessageToWorkerContext(const String&);
> > +        void postTaskToWorkerContext(PassRefPtr<ScriptExecutionContext::Task>);
> It would be nice if "[To]WorkerContext" were removed from all of these.
> 
> 
> > +        RefPtr<ScriptExecutionContext> m_parentScriptExecutionContext;
> As mentioned before, this appears unused.
> 
> 
> > Index: WebCore/dom/WorkerMessagingWebKit.cpp
> > ===================================================================
> > --- WebCore/dom/WorkerMessagingWebKit.cpp	(revision 0)
> > +++ WebCore/dom/WorkerMessagingWebKit.cpp	(revision 0)
> > @@ -0,0 +1,53 @@
> > +/*
> > + * Copyright (C) 2008 Apple Inc. All Rights Reserved.
> Why is there an Apple copyright here?
> 
> 
> It would be great if all of this were done in Worker directory off of WebCore,
> since you are essentially defining a platform specific file now.
> 
> 
> > +WorkerContextProxy* WorkerContextProxy::create(Worker* worker)
> > +{
> > +    RefPtr<WorkerContextCrossThreadProxy> workerContextProxy = WorkerContextCrossThreadProxy::create(worker->scriptExecutionContext());
> > +    RefPtr<WorkerObjectCrossThreadProxy> workerObjectProxy = WorkerObjectCrossThreadProxy::create(worker);
> > +    workerContextProxy->setWorkerObjectProxy(workerObjectProxy);
> workerObjectProxy.release() ?
> 
> > +    workerObjectProxy->setWorkerContextProxy(workerContextProxy);
> workerContextProxy.release() ?
> 
> 
> > Index: WebCore/dom/WorkerObjectCrossThreadProxy.cpp
> >
> > +    
> > +WorkerObjectCrossThreadProxy::WorkerObjectCrossThreadProxy(Worker* workerObject)
> > +    : m_scriptExecutionContext(workerObject->scriptExecutionContext())
> > +    , m_workerObject(workerObject)
> > +    , m_askedToTerminate(false)
> > +    , m_workerThreadHadPendingActivity(false)
> > +{
> > +    ASSERT(m_workerObject);
> > +    ASSERT((m_scriptExecutionContext->isDocument() && isMainThread())
> > +        || (m_scriptExecutionContext->isWorkerContext() && currentThread() == static_cast<WorkerContext*>(m_scriptExecutionContext.get())->thread()->threadID()));
> > +
> > +    ref();
> I see you manage all the ref counts on this object on the WorkerObjectThread,
> but this feel fragile.  Exposing a ref count on an object that is used cross
> thread but only one thread is suppose to do ref counting.  Is there a way to
> avoid having this object be ref counted?
> 
> 
> > +static void postMessageToWorkerObjectTask(ScriptExecutionContext*, WorkerObjectCrossThreadProxy* proxy, const String& message)
> > +{
> > +    if (proxy->askedToTerminate())
> > +        return;
> How do you know that proxy is still alive at this point?
> 
> 
> > +void WorkerObjectCrossThreadProxy::postMessageToWorkerObject(const String& message)
> > +{
> > +    if (m_askedToTerminate)
> > +        return;
> I don't think if is the right place to check this, but we should discuss.
> 
> 
> > Index: WebCore/dom/WorkerObjectCrossThreadProxy.h
> 
> > +    class WorkerObjectCrossThreadProxy : public WorkerObjectProxy {
> ...
> > +        RefPtr<WorkerContextProxy> m_workerContextProxy;
> Is this ever used?
> 
> 
> > +        bool m_workerThreadHadPendingActivity; // The latest confirmation from worker thread reported that it was still active.
> 
> This appears never to be set.
> 

Comment 7 David Levin 2009-02-20 14:32:54 PST
OK, after staring at this code for a while (and talking to you), I think I understand the lifetime issues and they all seem fine.

We all need to spend some time trying to figure out something simpler though after this as discussed.  (Lifetime issues are a place where the WorkerThreadableLoader code got complex as well, so it feels like there is a core issue here).  For this code, it was especially tricky that WorkerContextCrossThreadProxy is the way to keep the WorkerThread alive but I totally understand why you do this.
Comment 8 Jian Li 2009-02-20 14:46:21 PST
Created attachment 27839 [details]
Proposed Patch
Comment 9 Jian Li 2009-02-20 14:56:00 PST
All fixed except those commented inline.

(In reply to comment #5)
> (From update of attachment 27822 [details] [review])
> I really like the direction here and think this has made a lot of progress in
> the right direction.
> 
> I have some comments/questions below -- some of which may be easiest to discuss
> tomorrow.
> 
> 
> WebCore/dom/WorkerContextCrossThreadProxy.cpp
> >  PassRefPtr<WorkerContextCrossThreadProxy> WorkerContextCrossThreadProxy::create(ScriptExecutionContext* parentScriptExecutionContext)
> >  {
> >      return adoptRef(new WorkerContextCrossThreadProxy(parentScriptExecutionContext));
> >  }
> What do you think about parentContext instead of parentScriptExecutionContext?
> 
> Also it looks like this is never used, which is just as it should be since
> WorkerContextCrossThreadProxy could be in a different process from the parent
> context.
> 
> Should it be removed? (Or does it serve some purpose like helping xhr in webkit
> until I get that fixed up :)?)
> 
> 
> > +WorkerContextCrossThreadProxy::WorkerContextCrossThreadProxy(ScriptExecutionContext* parentScriptExecutionContext)
> > +    : m_parentScriptExecutionContext(parentScriptExecutionContext)
> > +    , m_workerObjectProxy(0)
> > +    , m_askedToTerminate(false)
> > +{
> > +    ref();
> 
> I don't understand why there is this ref/deref thing.  Sure workerObject has to
> call workerObjectDestroyed, but since it has a pointer to this class, why not
> have it be a RefPtr<> and get rid of this internal ref/deref.
> 
Currently I just keep as it was before. I could change to let Worker object keep a RefPtr for WorkerContextProxy. However, extra ref count for WorkerObjectProxy is still held by itself since WorkerContext could not hold the ref count to WorkerObjectProxy per current design. I keep this in order to make ref/deref logic consistent for both proxy classes.

> 
> 59 WorkerContextCrossThreadProxy::~WorkerContextCrossThreadProxy()
>  60 {
>  61     m_workerThread = 0;
>  62 }
> 
> Why is m_workerThread set to zero here?
> 
> 
> > +void WorkerContextCrossThreadProxy::postTaskToWorkerContext(PassRefPtr<ScriptExecutionContext::Task> task)
> > +{
> ...
> > +    if (m_workerThread) {
> > +        m_workerThread->runLoop().postTask(task);
> > +    } else
> 
> No {} here (single line statement).
> 
> 
> > +void WorkerContextCrossThreadProxy::workerObjectDestroyed()
> > +{
> > +    terminateWorkerContext();
> > +    m_workerObjectProxy->terminateWorkerObject();
> > +    m_workerObjectProxy = 0;
> It looks very odd that it calls terminateWorkerObject when it is told that the
> worker object was already destroyed. 
> 
Yes. I moved the logic to WorkerContextCrossThreadProxy::terminate().
> 
> > Index: WebCore/dom/WorkerContextCrossThreadProxy.h
> 
> > +#include <wtf/Noncopyable.h>
> Appears unused.
> 
> > +    class WorkerContextCrossThreadProxy : public WorkerContextProxy {
> ...
> > +        virtual void startWorkerContext(const KURL& scriptURL, const String& userAgent, const String& sourceCode);
> > +        virtual void terminateWorkerContext();
> > +        virtual void postMessageToWorkerContext(const String&);
> > +        void postTaskToWorkerContext(PassRefPtr<ScriptExecutionContext::Task>);
> It would be nice if "[To]WorkerContext" were removed from all of these.
> 
> 
> > +        RefPtr<ScriptExecutionContext> m_parentScriptExecutionContext;
> As mentioned before, this appears unused.
> 
> 
> > Index: WebCore/dom/WorkerMessagingWebKit.cpp
> > ===================================================================
> > --- WebCore/dom/WorkerMessagingWebKit.cpp	(revision 0)
> > +++ WebCore/dom/WorkerMessagingWebKit.cpp	(revision 0)
> > @@ -0,0 +1,53 @@
> > +/*
> > + * Copyright (C) 2008 Apple Inc. All Rights Reserved.
> Why is there an Apple copyright here?
> 
> 
> It would be great if all of this were done in Worker directory off of WebCore,
> since you are essentially defining a platform specific file now.
> 
Yes. However, I would rather do it in next patch.
> 
> > +WorkerContextProxy* WorkerContextProxy::create(Worker* worker)
> > +{
> > +    RefPtr<WorkerContextCrossThreadProxy> workerContextProxy = WorkerContextCrossThreadProxy::create(worker->scriptExecutionContext());
> > +    RefPtr<WorkerObjectCrossThreadProxy> workerObjectProxy = WorkerObjectCrossThreadProxy::create(worker);
> > +    workerContextProxy->setWorkerObjectProxy(workerObjectProxy);
> workerObjectProxy.release() ?
> 
> > +    workerObjectProxy->setWorkerContextProxy(workerContextProxy);
> workerContextProxy.release() ?
> 
Can not use release() since it returns pointer.
> 
> > Index: WebCore/dom/WorkerObjectCrossThreadProxy.cpp
> >
> > +    
> > +WorkerObjectCrossThreadProxy::WorkerObjectCrossThreadProxy(Worker* workerObject)
> > +    : m_scriptExecutionContext(workerObject->scriptExecutionContext())
> > +    , m_workerObject(workerObject)
> > +    , m_askedToTerminate(false)
> > +    , m_workerThreadHadPendingActivity(false)
> > +{
> > +    ASSERT(m_workerObject);
> > +    ASSERT((m_scriptExecutionContext->isDocument() && isMainThread())
> > +        || (m_scriptExecutionContext->isWorkerContext() && currentThread() == static_cast<WorkerContext*>(m_scriptExecutionContext.get())->thread()->threadID()));
> > +
> > +    ref();
> I see you manage all the ref counts on this object on the WorkerObjectThread,
> but this feel fragile.  Exposing a ref count on an object that is used cross
> thread but only one thread is suppose to do ref counting.  Is there a way to
> avoid having this object be ref counted?
> 
Yes, as discussed.
> 
> > +static void postMessageToWorkerObjectTask(ScriptExecutionContext*, WorkerObjectCrossThreadProxy* proxy, const String& message)
> > +{
> > +    if (proxy->askedToTerminate())
> > +        return;
> How do you know that proxy is still alive at this point?
> 
> 
> > +void WorkerObjectCrossThreadProxy::postMessageToWorkerObject(const String& message)
> > +{
> > +    if (m_askedToTerminate)
> > +        return;
> I don't think if is the right place to check this, but we should discuss.
> 
> 
> > Index: WebCore/dom/WorkerObjectCrossThreadProxy.h
> 
> > +    class WorkerObjectCrossThreadProxy : public WorkerObjectProxy {
> ...
> > +        RefPtr<WorkerContextProxy> m_workerContextProxy;
> Is this ever used?
> 
> 
> > +        bool m_workerThreadHadPendingActivity; // The latest confirmation from worker thread reported that it was still active.
> 
> This appears never to be set.
> 

Comment 10 David Levin 2009-02-20 15:01:40 PST
Thx! This looks good to me.

Two caveats to be done *after this* (imo):
1. We need to move some (all?) of the worker stuff to its own directory (so we can put some platform specific directories there).
2. We all need to brainstorm some ways to simplify the lifetime model here.
Comment 11 Jian Li 2009-02-20 15:59:40 PST
Created attachment 27842 [details]
Proposed Patch
Comment 12 Alexey Proskuryakov 2009-02-23 07:04:53 PST
Why do we need to split WorkerMessagingProxy? The bug description says it's to do cross-thread messaging in WebKit, but this works already, doesn't it?
Comment 13 Jian Li 2009-02-23 10:09:29 PST
(In reply to comment #12)
> Why do we need to split WorkerMessagingProxy? The bug description says it's to
> do cross-thread messaging in WebKit, but this works already, doesn't it?
> 

This is because we want to reuse what's implemented for cross-thread messaging in WebKit towards other platform, like Chromium. Without splitting WorkerMessagingProxy, we will have to duplicate a lot of existing logic as in WorkerMessagingProxy when implementing WorkerContextProxy and WorkerObjectProxy in Chromium. To share as much code as possible and improve the consistency, we propose splitting WorkerMessagingProxy into two halves. 

For WebKit (cross-thread):
 Worker -> WorkerContextCrossThreadProxy -> WorkerContext
        <- WorkerObjectCrossThreadProxy  <-

For Chromium (cross-process):
 Worker -> WorkerContextCrossProcessProxy <...> WorkerObjectCrossProcessProxy <- WorkerContext
                     |                                      |
                     v                                      v
        <- WorkerObjectCrossThreadProxy         WorkerContextCrossThreadProxy -> 

Comment 14 Alexey Proskuryakov 2009-02-23 12:42:42 PST
I'm not very sympathetic towards the idea of making the threaded implementation so much more complicated. Could you please tell more about what logic will have to be duplicated if we don't do this?
Comment 15 Jian Li 2009-02-23 15:58:09 PST
(In reply to comment #14)
> I'm not very sympathetic towards the idea of making the threaded implementation
> so much more complicated. Could you please tell more about what logic will have
> to be duplicated if we don't do this?
> 

The logic is to share as much code as possible for different platforms so that it will ease the pain of the future integration and merge of WebKit code into other platform, like Chromium.

Yes, this brings the complexity to the cross-thread implementation for WebKit. So probably it is not a good idea to do it now. We will not do the splitting now. Instead, we will implement both proxies by doing similar things as in WorkerMessagingProxy plus cross-process IPC stuffs.
Comment 16 Alexey Proskuryakov 2009-02-25 02:23:32 PST
Comment on attachment 27842 [details]
Proposed Patch

Clearing review flag to get this out of queue.