Bug 47423 – [chromium] Update WebBlobData to adapt to BlobData change in terms of handling string data item

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[Jian Li]

We need to update WebKit implementation WebBlobData to adapt to BlobData change introduced in https://bugs.webkit.org/show_bug.cgi?id=46543.

[Jian Li]

Created attachment 70268 [details]
Proposed Patch

[WebKit Review Bot]

Attachment 70268 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/4192152

[Darin Fisher (:fishd, Google)]

Comment on attachment 70268 [details]
Proposed Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=70268&action=review

> WebKit/chromium/public/WebBlobData.h:48
> +class WebRawData {

please put this in a separate header file.  one class/struct per header please.

i would give this a better name because we already have WebData.  what you have
here it seems is a container for a |const char*| and a length.  that means this
class does not own the data.  WebDependentData or WebDataPiece might be good
names.

> WebKit/chromium/public/WebBlobData.h:56
> +    WEBKIT_API void reset()

note: these do not need WEBKIT_API since they are implemented inline.  you
only need WEBKIT_API on methods that need to be exported from webkit.dll.

> WebKit/chromium/public/WebBlobData.h:86
> +        WebRawData data;

have you considered using WebData here instead?  why do you want this
class to not own the data?

[Jian Li]

Created attachment 70452 [details]
Proposed Patch

[Jian Li]

Created attachment 70453 [details]
Proposed Patch

[Jian Li]

(In reply to comment #3)
> (From update of attachment 70268 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=70268&action=review
> 
> > WebKit/chromium/public/WebBlobData.h:48
> > +class WebRawData {
> 
> please put this in a separate header file.  one class/struct per header please.
> 
> i would give this a better name because we already have WebData.  what you have
> here it seems is a container for a |const char*| and a length.  that means this
> class does not own the data.  WebDependentData or WebDataPiece might be good
> names.

Renamed to WebDependentData and moved to a separate file.

> 
> > WebKit/chromium/public/WebBlobData.h:56
> > +    WEBKIT_API void reset()
> 
> note: these do not need WEBKIT_API since they are implemented inline.  you
> only need WEBKIT_API on methods that need to be exported from webkit.dll.

Done.

> 
> > WebKit/chromium/public/WebBlobData.h:86
> > +        WebRawData data;
> 
> have you considered using WebData here instead?  why do you want this
> class to not own the data?

Yes, I have looked into WebData, but decided not to use it because we want to avoid copying the string data. Since the lifetime of the string data is controlled by the BlobData and thus WebBlobData, we do not need to use WebData to copy it over.

[WebKit Review Bot]

Attachment 70453 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/4278027

[Darin Fisher (:fishd, Google)]

Comment on attachment 70453 [details]
Proposed Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=70453&action=review

> WebKit/chromium/public/WebBlobData.h:51
> +        WebDependentData data;

This scares me because you are putting a memory address into a data structure,
and relying on the guy who created the WebBlobData to keep the memory address
valid.  It would be much better to use WebData.

Why don't you change WebCore::BlobDataItem::data to be of type RefPtr<SharedBuffer>
instead of CString?  Then, the conversion from WebData to RefPtr<SharedBuffer>
would be super cheap.

[Jian Li]

(In reply to comment #8)
> (From update of attachment 70453 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=70453&action=review
> 
> > WebKit/chromium/public/WebBlobData.h:51
> > +        WebDependentData data;
> 
> This scares me because you are putting a memory address into a data structure,
> and relying on the guy who created the WebBlobData to keep the memory address
> valid.  It would be much better to use WebData.
> 
> Why don't you change WebCore::BlobDataItem::data to be of type RefPtr<SharedBuffer>
> instead of CString?  Then, the conversion from WebData to RefPtr<SharedBuffer>
> would be super cheap.

I can't use SharedBuffer since it is not thread-safe. I have to make the representation of string data be thread-safe so that the blob data can be passed from worker thread to main thread.

[Jian Li]

Created attachment 70544 [details]
Proposed Patch

[Jian Li]

> Why don't you change WebCore::BlobDataItem::data to be of type RefPtr<SharedBuffer>
> instead of CString?  Then, the conversion from WebData to RefPtr<SharedBuffer>
> would be super cheap.

Changed to do something similar to WebData by introducing WebThreadSafeData to wrap RawData.

[Darin Fisher (:fishd, Google)]

Comment on attachment 70544 [details]
Proposed Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=70544&action=review

> WebKit/chromium/public/WebThreadSafeData.h:37
> +namespace WebCore { class RawData; }

where is RawData defined?  i cannot find it in the source tree.

> WebKit/chromium/public/WebThreadSafeData.h:84
> +    WebThreadSafeDataPrivate* m_private;

please take advantage of WebPrivatePtr<T>.  that is the new way of wrapping
a reference counted WebCore type.  see WebNode.h for example.

[Jian Li]

Created attachment 70558 [details]
Proposed Patch

[Jian Li]

(In reply to comment #12)
> (From update of attachment 70544 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=70544&action=review
> 
> > WebKit/chromium/public/WebThreadSafeData.h:37
> > +namespace WebCore { class RawData; }
> 
> where is RawData defined?  i cannot find it in the source tree.

It is defined in BlobData.h in the patch for bug 46543 (https://bugs.webkit.org/show_bug.cgi?id=46543).
Note that I am going to update this patch to move RawData out of "struct BlobDataItem" to allow forward declaration.

> 
> > WebKit/chromium/public/WebThreadSafeData.h:84
> > +    WebThreadSafeDataPrivate* m_private;
> 
> please take advantage of WebPrivatePtr<T>.  that is the new way of wrapping
> a reference counted WebCore type.  see WebNode.h for example.

Done.

[WebKit Review Bot]

Attachment 70558 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/4373012

[WebKit Review Bot]

http://trac.webkit.org/changeset/69611 might have broken Chromium Win Release

[Jian Li]

Committed as http://trac.webkit.org/changeset/69611.