[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 #9 from jianli at chromium.org  2009-02-20 14:56 PDT -------
All fixed except those commented inline.

(In reply to comment #5)
> (From update of attachment 27822 [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.
> 


-- 
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.