[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 #16 from jianli at chromium.org  2009-02-11 08:09 PDT -------
(In reply to comment #15)
> (From update of attachment 27542 [review])
> -    , 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.

I will change to do as you suggest.
> 
> -    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.

Yes, we can choose to still load the script in main thread. However, for
multi-process design, I think it would be better to load the script directly in
the worker process, instead of passing the big chunk of data via IPC from one
process to anther. How do you think?
> 
> +    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.
> 
I can change to use another boolean variable. How do you think?
> -       
> 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.
> 
Yes, we can do upcast. I will check if we need to add method to confirm
message. Thanks.


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