[webkit-reviews] review denied: [Bug 36903] Implement BlobBuilder internal class for BlobBuilder support as defined in FileWriter : [Attachment 52227] Patch (rebased)

[bugzilla-daemon at webkit.org]

Dmitry Titov <dimich at chromium.org> has denied Kinuko Yasuda
<kinuko at chromium.org>'s request for review:
Bug 36903: Implement BlobBuilder internal class for BlobBuilder support as
defined in FileWriter
https://bugs.webkit.org/show_bug.cgi?id=36903

Attachment 52227: Patch (rebased)
https://bugs.webkit.org/attachment.cgi?id=52227&action=review

------- Additional Comments from Dmitry Titov <dimich at chromium.org>
General comment: it seems you are taking the path of converting the parts
appended by append() method to ByteStore immediately and synchronously on
append(). This will have 2 undesired effects:

1. Converting strings happen right away, while the result may not even be used
at the end.

2. When you will need to implement append(FileBlob) you will need to fetch the
file data immediately, because it can come between 2 append(String) calls

3. When someone appends an bunch of Strings and then changes Blob.endings,
those strings will have to be re-appended, which is multiple work. I'm not sure
how it will even work with the FileBlobs in the mix.

It seems the Blob should simply contain a list of all appended items, and delay
the actual conversions to the end usage point. this way, perhaps you will never
even need ByteStore object - indeed, the Blob could just implement read(...)
method by walking the list and converting/filling the provided buffer in 'on
demand' fashion.

I mentioned in the webkit-dev discussion and still think that introducing
ByteStore will eventually make Blob.append(FileBlob) a read-file-right-now
operation... And if we don't read/combine anything until absolutely necessary,
we shouldn't have to need ByteStore, at least in this explicit form.

r- because strings can not be converted/merged right in the append(String)
method, due to reasons above.