[webkit-reviews] review cancelled: [Bug 23636] Make the async api of ThreadableLoader functional for the worker context. : [Attachment 27243] Part 2: Enable the async portion of ThreadableLoader for Workers.

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Mon Feb 2 08:33:24 PST 2009


David Levin <levin at chromium.org> has cancelled David Levin
<levin at chromium.org>'s request for review:
Bug 23636: Make the async api of ThreadableLoader functional for the worker
context.
https://bugs.webkit.org/show_bug.cgi?id=23636

Attachment 27243: Part 2: Enable the async portion of ThreadableLoader for
Workers.
https://bugs.webkit.org/attachment.cgi?id=27243&action=review

------- Additional Comments from David Levin <levin at chromium.org>
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.


More information about the webkit-reviews mailing list