[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 #15 from ap at webkit.org  2009-02-11 02:06 PDT -------
(From update of attachment 27542)
-    , m_messagingProxy(new WorkerMessagingProxy(doc, this))
+    , m_contextProxy(0)
...
-    ASSERT(scriptExecutionContext()); // The context is protected by messaging
proxy, so it cannot be destroyed while a Worker exists.
-    m_messagingProxy->workerObjectDestroyed();
+    ASSERT(scriptExecutionContext()); // The context is protected by worker
context proxy, so it cannot be destroyed while a Worker exists.
+    if (m_contextProxy)
+        m_contextProxy->workerObjectDestroyed();

What's the reason for these changes? The proxy used to be non-null, as long as
the worker object existed. Adding a null check in destructor makes this design
less obvious, complicating the life of anyone trying to understand this code.

If the only reason is to pass scriptURL to the proxy, I suggest doing that
after construction as a separate setScriptURL() call.

-    m_cachedScript = doc->docLoader()->requestScript(m_scriptURL,
document()->charset());
-    if (!m_cachedScript) {
-        dispatchErrorEvent();
-        return;
-    }

What's the reason for moving this into proxy class? I don't think that fetching
the script has anything to do with messaging proxy responsibilities.

+    m_unconfirmedMessageCount++;  // The worker context does not exist while
loading, so we must ensure that the worker object is not collected, as well as
its event listeners.

I think that this is an abuse of this variable. It counts unconfirmed messages,
which has nothing to do with keeping the object alive while loading script
source. Even though this technically works, it is misleading and will cause
mistakes later.

-       
context->thread()->messagingProxy()->confirmWorkerThreadMessage(context->hasPendingActivity());
+       
m_messagingProxy->confirmWorkerThreadMessage(context->hasPendingActivity());

Why do you need to store m_messagingProxy? Knowing that we have a threaded
implementation, I think you can always upcast
context->thread()->workerObjectProxy(). But also, I expect that you'll
ultimately need confirmWorkerThreadMessage() in multi-process design, too.


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