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

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Tue Feb 10 01:13:05 PST 2009


------- Comment #2 from levin at chromium.org  2009-02-10 01:13 PDT -------
Here's my comments on this patch.  There's a lot here but it is a big patch.

1. extra blank lines added to the end.  Please remove them.

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

>  void dispatchMessage(const String& message);
1. Omit param name "message" (as explained below for

2. Extra blank lines added at end.  Pls remove.

> #include "GenericWorkerTask.h"
1. I don't think this include is needed anymore.

> void dispatchMessage(const String& message);
1. remove "message"
2. Extra blank lines added at end.  Pls remove.

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

> WorkerDebugMessageTask
1. Why was this added instead of just doing what it did before with

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

> 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

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

> postTaskToWorkerObject(loaderTask);
6. Should be "postTaskToWorkerObject(loaderTask.release());" since the RefPtr
is no longer used after this.


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

> PostDebugMessageToWorkerObject
1. same parameter name comment.

1. Extra blank lines were added to the end.  Pls remove them.

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

  >   if (thisPtr->m_messagingProxy.askedToTerminate()) 
  >        return;      
1. Why was this removed?

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

More information about the webkit-unassigned mailing list