[Webkit-unassigned] [Bug 24054] Implement two separate proxy classes in order to split WorkerMessagingProxy.

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=24054





------- Comment #5 from levin at chromium.org  2009-02-20 00:45 PDT -------
(From update of attachment 27822)
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.


-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.