Bug 23636 – Make the async api of ThreadableLoader functional for the worker context.

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[David Levin]

Implement WorkerThreadableContext.

[David Levin]

Created attachment 27173 [details]
Part 1: Expose WorkerMessagingProxy::postTaskToWorkerContext

[Alexey Proskuryakov]

Comment on attachment 27173 [details]
Part 1: Expose WorkerMessagingProxy::postTaskToWorkerContext

Discussed on IRC - we think that it will be better to merge ScriptExecutionContext::Task and WorkerTask, which will eliminate the need for wrappers.

Besides, we don't have a DocumentTask, so why have a WorkerTask?

[David Levin]

Created attachment 27181 [details]
Part 1: Expose WorkerMessagingProxy::postTaskToWorkerContext

Removed WorkerTask.  Hurray for less code!

[David Levin]

Created attachment 27243 [details]
Part 2: Enable the async portion of ThreadableLoader for Workers.

I believe that I have addressed all comments and have refactored the code in an attempt to make it clearer.

[Alexey Proskuryakov]

Comment on attachment 27181 [details]
Part 1: Expose WorkerMessagingProxy::postTaskToWorkerContext

r=me

[Alexey Proskuryakov]

Comment on attachment 27243 [details]
Part 2: Enable the async portion of ThreadableLoader for Workers.

+        Enable ThreadableLoader for workers.  Now, when the xhr bindings are
+        exposed to workers, async xhr should work.

I keep reading this as "async XHR will work after landing this patch", which isn't true. Maybe it isn't that important to say what will happen in the future.

+        static Type copy(const std::auto_ptr<T>& autoPtr)
+        {
+            return std::auto_ptr<T>(*const_cast<std::auto_ptr<T>*>(&autoPtr));
+        }

Both const_cast and the name "copy()" make this look suspicious. I don't have anything to suggest though.

+ * Copyright (c) 2009, Google Inc. All rights reserved.

Looks like you've used an old template for this file.

+            RefPtr<ThreadSafeShared<ThreadableLoaderClientWrapper> > m_workerClientWrapper;

Why not RefPtr<ThreadableLoaderClientWrapper>?

I have some further questions, let's discuss them on IRC.
+            // Maybe used on either thread.
+            WorkerMessagingProxy* m_messagingProxy;

Typo: there should be a space in "May be". You could consider making m_messagingProxy a reference to make it clearer that it is non-null.

+        MainThreadBridge* m_bridge;

The bridge seems to be non-null, too.

[David Levin]

Created attachment 27247 [details]
Part 2: Enable the async portion of ThreadableLoader for Workers.

Addressed comments.

Here's a few that I need a little more explanation.

+        static Type copy(const std::auto_ptr<T>& autoPtr)
+        {
+            return std::auto_ptr<T>(*const_cast<std::auto_ptr<T>*>(&autoPtr));
+        }

> Both const_cast and the name "copy()" make this look suspicious. I don't have
> anything to suggest though.
I agree.  Here's my reasoning, which I think you got because you said you didn't have anything to suggest.
1.  The const_cast.  Unfortunately, the createCallbackTask has to take const T&.  In this case (an auto_ptr), it woud be nice just to take a auto_ptr& or just an auto_ptr, so I casted away the const.  If you're passing in an auto_ptr, you should expect that the function will take it over.

2.  The "copy" method that really transfers ownership.  Well, the intent is to get something that can be used on another thread.  (Maybe Copier/copy wasn't the best choice...)  If you're given an auto_ptr, then taking ownership is the thing to do to match the intent.


+ * Copyright (c) 2009, Google Inc. All rights reserved.
> Looks like you've used an old template for this file.
I fixed this in several other files to get rid of my bad templates.



+            RefPtr<ThreadSafeShared<ThreadableLoaderClientWrapper> >
m_workerClientWrapper;
> Why not RefPtr<ThreadableLoaderClientWrapper>?
I made a CrossThreadCopier template that expects a RefPtr<ThreadSafeShared<T>>.  

It made sense to me to declare it this way rather than put a lot of mechanics everywhere I called createCallbackTask with this member.

[David Levin]

Created attachment 27271 [details]
Part 2: Enable the async portion of ThreadableLoader for Workers.

Fixed bug number in changelog.

[Alexey Proskuryakov]

Comment on attachment 27271 [details]
Part 2: Enable the async portion of ThreadableLoader for Workers.

r=me

I think that in the future, we should think of a way to make the copier work without constructs such as RefPtr<ThreadSafeShared<T>>.

[Alexey Proskuryakov]

Comment on attachment 27181 [details]
Part 1: Expose WorkerMessagingProxy::postTaskToWorkerContext

Part 1 committed revision 40526.

[Alexey Proskuryakov]

Part 2 Committed revision 40527.