Bug 23636 - Make the async api of ThreadableLoader functional for the worker context.
Summary: Make the async api of ThreadableLoader functional for the worker context.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: David Levin
URL:
Keywords:
Depends on: 23618
Blocks: 22720
  Show dependency treegraph
 
Reported: 2009-01-29 22:09 PST by David Levin
Modified: 2009-02-02 23:43 PST (History)
1 user (show)

See Also:


Attachments
Part 1: Expose WorkerMessagingProxy::postTaskToWorkerContext (8.72 KB, patch)
2009-01-29 23:45 PST, David Levin
ap: review-
Details | Formatted Diff | Diff
Part 1: Expose WorkerMessagingProxy::postTaskToWorkerContext (19.54 KB, patch)
2009-01-30 08:56 PST, David Levin
no flags Details | Formatted Diff | Diff
Part 2: Enable the async portion of ThreadableLoader for Workers. (34.52 KB, patch)
2009-02-01 22:55 PST, David Levin
no flags Details | Formatted Diff | Diff
Part 2: Enable the async portion of ThreadableLoader for Workers. (36.40 KB, patch)
2009-02-02 08:33 PST, David Levin
no flags Details | Formatted Diff | Diff
Part 2: Enable the async portion of ThreadableLoader for Workers. (36.44 KB, patch)
2009-02-02 18:01 PST, David Levin
ap: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description David Levin 2009-01-29 22:09:06 PST
Implement WorkerThreadableContext.
Comment 1 David Levin 2009-01-29 23:45:51 PST
Created attachment 27173 [details]
Part 1: Expose WorkerMessagingProxy::postTaskToWorkerContext
Comment 2 Alexey Proskuryakov 2009-01-30 00:16:16 PST
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?
Comment 3 David Levin 2009-01-30 08:56:28 PST
Created attachment 27181 [details]
Part 1: Expose WorkerMessagingProxy::postTaskToWorkerContext

Removed WorkerTask.  Hurray for less code!
Comment 4 David Levin 2009-02-01 22:55:09 PST
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.
Comment 5 Alexey Proskuryakov 2009-02-02 02:05:24 PST
Comment on attachment 27181 [details]
Part 1: Expose WorkerMessagingProxy::postTaskToWorkerContext

r=me
Comment 6 Alexey Proskuryakov 2009-02-02 03:41:51 PST
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.
Comment 7 David Levin 2009-02-02 08:33:24 PST
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.
Comment 8 David Levin 2009-02-02 18:01:43 PST
Created attachment 27271 [details]
Part 2: Enable the async portion of ThreadableLoader for Workers.

Fixed bug number in changelog.
Comment 9 Alexey Proskuryakov 2009-02-02 23:30:12 PST
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>>.
Comment 10 Alexey Proskuryakov 2009-02-02 23:39:21 PST
Comment on attachment 27181 [details]
Part 1: Expose WorkerMessagingProxy::postTaskToWorkerContext

Part 1 committed revision 40526.
Comment 11 Alexey Proskuryakov 2009-02-02 23:43:02 PST
Part 2 Committed revision 40527.