[Webkit-unassigned] [Bug 23859] Change worker code to use different proxy class pointers.

[bugzilla-daemon at webkit.org]

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





------- Comment #5 from jianli at chromium.org  2009-02-10 11:55 PDT -------
All are fixed except those commented inline. Thanks.

(In reply to comment #2)
> Here's my comments on this patch.  There's a lot here but it is a big patch.
> 
> 
> *WebCore/bindings/js/WorkerScriptController.cpp
> 1. extra blank lines added to the end.  Please remove them.
> 
> 
> *WebCore/dom/Worker.cpp
> >  , m_contextProxy(NULL)
> 1. Use 0 instead of NULL.
> 
> 2. Previously, m_messagingProxy would never be 0.  Now m_contextProxy can be.
> It doesn't look like all of this code has been fixed to handle that.  
> One quick example, Worker::~Worker calls terminate() and terminate() does
> this "m_contextProxy->terminateWorkerContext();"
> 
> > // The context is protected by context proxy....
> 3. This statement is confusing as is.  The statement would be clearer if it
> said "worker context proxy" instead of "context proxy".
> 
> 
> *WebCore/dom/Worker.h
> >  void dispatchMessage(const String& message);
> 1. Omit param name "message" (as explained below for
> "postDebugMessageToWorkerObject").
> 
> 2. Extra blank lines added at end.  Pls remove.
> 
> 
> *WebCore/dom/WorkerContext.cpp
> > #include "GenericWorkerTask.h"
> 1. I don't think this include is needed anymore.
> 
> 
> *WebCore/dom/WorkerContext.h
> > void dispatchMessage(const String& message);
> 1. remove "message"
> 2. Extra blank lines added at end.  Pls remove.
> 
> 
> *WebCore/dom/WorkerContextProxy.h
> > static WorkerContextProxy* create(const String&, Worker*);
> 1. Unlike other cases, the param name associated with const String& would add
> information here so it should be included.
> 
> 
> *WebCore/dom/WorkerMessagingProxy.cpp 
> > WorkerDebugMessageTask
> 1. Why was this added instead of just doing what it did before with
> "postTaskToWorkerObject(createCallbackTask"?
> 
> 
> > // The worker context does not exist while loading, so we much ensure that the worker object is not collected, as well as its event listeners.
> 2. I know you didn't add the comment, but it looks like "much" should be
> "must".
> 
> 
> > ASSERT((m_scriptExecutionContext->isDocument() && isMainThread())
>         || (m_scriptExecutionContext->isWorkerContext() && ...
> 3. Why did this assert get removed?
> 
> > WorkerMessagingProxy::notifyFinished
> 4. The position of "m_unconfirmedMessageCount--;" changes where
> hasPendingActivity may become false compared to the previous code.  Is this on
> purpose?
> 
Yes. I added the comment for this.

> > postDebugMessageToWorkerObject
> 5. "debug" sounds like debug code to me.  Can you come up with a better name
> here?

Changed to postConsoleMessageToWorkerObject, as discussed.
> 
> > postTaskToWorkerObject(loaderTask);
> 6. Should be "postTaskToWorkerObject(loaderTask.release());" since the RefPtr
> is no longer used after this.
> 
> 
> 
> *WebCore/dom/WorkerMessagingProxy.h
> 
> > virtual void postDebugMessageToWorkerObject(MessageDestination destination, MessageSource source, MessageLevel level, const String& errorMessage, int lineNumber, const String& sourceURL) = 0;
> 
> 1. Don't add parameter names in declarations when they don't add information. 
> In this case, leave out destination, source, level, so the declaration would
> look like this:
> 
>     virtual void postDebugMessageToWorkerObject(MessageDestination,
> MessageSource, MessageLevel, const String& errorMessage, int lineNumber, const
> String& sourceURL) = 0;
> 
> 
> >  virtual void postTaskToLoader(PassRefPtr<ScriptExecutionContext::Task> task);
> 2. Omit "task"
> 
> 
> 
> *WebCore/dom/WorkerObjectProxy.h
> > PostDebugMessageToWorkerObject
> 1. same parameter name comment.
> 
> 
> *WebCore/dom/WorkerThread.cpp
> 1. Extra blank lines were added to the end.  Pls remove them.
> 
> 
> *WebCore/dom/WorkerThread.h
> > WorkerObjectProxy* m_objectProxy
> 1. The abbreviation "m_objectProxy" is confusing to me when I read it quickly
> it in the code.  Perhaps stay with m_workerObjectProxy;
> 
> 2. Extra blank lines were added to the end.  Pls remove them.
> 
> 
> *WebCore/loader/WorkerThreadableLoader.cpp
>   >   if (thisPtr->m_messagingProxy.askedToTerminate()) 
>   >        return;      
> 1. Why was this removed?
> 
This is because I do not want to expose another method in the proxy interface.
The logic is moved into the LoaderTask::performTask.
> 
> 
> *WebCore/loader/WorkerThreadableLoader.h
>    >  WorkerObjectProxy* m_objectProxy;
> 1. Same comment about the name m_objectProxy.
> 2. This was previously a "T&".  Why did it get changed to a "T*"?
> 


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