[webkit-reviews] review denied: [Bug 47423] [chromium] Update WebBlobData to adapt to BlobData change in terms of handling string data item : [Attachment 70268] Proposed Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Sat Oct 9 14:11:33 PDT 2010


Darin Fisher (:fishd, Google) <fishd at chromium.org> has denied Jian Li
<jianli at chromium.org>'s request for review:
Bug 47423: [chromium] Update WebBlobData to adapt to BlobData change in terms
of handling string data item
https://bugs.webkit.org/show_bug.cgi?id=47423

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

------- Additional Comments from Darin Fisher (:fishd, Google)
<fishd at chromium.org>
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?


More information about the webkit-reviews mailing list