[webkit-reviews] review denied: [Bug 46543] Fix passing blob data with string data item between threads : [Attachment 68794] Proposed Patch

[bugzilla-daemon at webkit.org]

David Levin <levin at chromium.org> has denied Jian Li <jianli at chromium.org>'s
request for review:
Bug 46543: Fix passing blob data with string data item between threads
https://bugs.webkit.org/show_bug.cgi?id=46543

Attachment 68794: Proposed Patch
https://bugs.webkit.org/attachment.cgi?id=68794&action=review

------- Additional Comments from David Levin <levin at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=68794&action=review

There are enough things in here to be addressed at the moment that I'm r-'ing. 
I think once they are fixed some items will be a little clearer and there may
be a few more comments.

> WebCore/platform/network/BlobData.h:36
> +#include <wtf/CrossThreadRefCounted.h>

I'd strongly discourage using CrossThreadRefCounted. It has a very special use
case when the ref counting must live outside of the class (and probably a few
other things that I don't remember right off).

Instead create a class that derives from ThreadSafeShared.  I suspect this
change will make this code much easier to read and may clear up some issues
that others were concerned about.

> WebCore/platform/network/BlobData.h:54
> +	   void makeCrossThread();

makeCrossThread is a terrible name that I think I started.  I like the
suggestion of detachFromCurrentThread (or something like that).

> WebCore/platform/text/LineEnding.cpp:89
> +    virtual void copy(const CString& src)

avoid abbreviations "src"

> WebKit/chromium/public/WebBlobData.h:31
>  #ifndef WebBlobData_h

If you break out this part of the change in to a separate patch, it would be
better as Darin could review just that patch and likely do it quickly.