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


*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?

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

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



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