Bug 36903

Created attachment 52223 [details] Patch

Created attachment 52227 [details] Patch (rebased)

Comment on attachment 52227 [details] Patch (rebased) > diff --git a/WebCore/html/BlobBuilder.cpp b/WebCore/html/BlobBuilder.cpp > new file mode 100644 > index 0000000..c076180 > --- /dev/null > +++ b/WebCore/html/BlobBuilder.cpp > +typedef HashMap<String, BlobBuilder::EndingsType, CaseFoldingHash> EndingsTypeMap; > +static const EndingsTypeMap* createEndingsTypeMap() > +{ > + EndingsTypeMap* map = new EndingsTypeMap; > + map->add("transparent", BlobBuilder::ENDING_TRANSPARENT); > + map->add("native", BlobBuilder::ENDING_NATIVE); > + map->add("lf", BlobBuilder::ENDING_LF); > + map->add("cr", BlobBuilder::ENDING_CR); > + map->add("crlf", BlobBuilder::ENDING_CRLF); Are all the "endings" strings fixed? If so, we had better use AtomicString instead of String. AtomicString is faster in comparing instances. > +String BlobBuilder::endings() const > +{ > + switch (m_endings) { > + case ENDING_TRANSPARENT: return "transparent"; > + case ENDING_NATIVE: return "native"; > + case ENDING_LF: return "lf"; > + case ENDING_CR: return "cr"; > + case ENDING_CRLF: return "crlf"; > + } > + ASSERT_NOT_REACHED(); > + return "transparent"; ditto. AtomicString is better. > diff --git a/WebCore/html/ByteStore.h b/WebCore/html/ByteStore.h > new file mode 100644 > index 0000000..c1a99cf > --- /dev/null > +++ b/WebCore/html/ByteStore.h > + // Reads the data into |out| at most |maxLength| bytes. > + int64_t read(char* out, size_t offset, size_t maxLength) const; > + > + // Appends raw data |in|. Returns false if the data could not be added. > + bool append(const char* in, size_t length); WebKit doesn't have |parameter| notation.

Comment on attachment 52227 [details] Patch (rebased) 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.

(In reply to comment #4) > (From update of attachment 52227 [details]) > 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: For append(Blob) I was/am going to put them into a list and wasn't going to read them immediately, but I was missing the point that BlobBuilder.endings may be changed after Strings are added. In the spec there's a notion that BlobBuilder.append(string) may throw encoding exceptions, so I had assumed append(string) does some conversion right away. (Actually I think we can drop this part as we now do not have encoding parameters in BlobBuilder.) It makes me wonder when we should really convert the strings; 1. BlobBuilder.append, 2. BlobBuilder.getBlob or 3. when we read Blob. 1. is not efficient as you suggested, but I think we will want some raw data at 2., because Blob.size and Blob.slice seem to be byte-based methods/operations. Does this sound correct? I'm attaching a new patch that do not convert Strings into ByteStore at append() but at getBlob() timing. This time I'm including changes in Blob too to give more overview.

Created attachment 52429 [details] Patch

It is much better now. Thanks for iterating on it! I don't think it can go in in this form, but added code definitely added some clarity. A few points: It seems converting strings at getBlob time is just fine indeed, for the reasons you mentioned. Obviously, we'll not want to do the same for file fragments but it's not in this patch. Looking at the patch, I think we can make what you have as CombinedBlob to be the only type of the blob... A Blob. It would be always a list of BlobItems. A BlobItem would be an abstract class, with implementations like FileBlobItem, StringBlobItem, ByteArrayBlobItem.When File derives from the Blob, such Blob would only have 1 FielBlobitem in the list... StringBlobItem would be the one constructed from string and stored in a mutable list of BlobBuilder. Once BlobBuilder::getBlob() (it probably should be createBlob()) is called, the new list created, with FileBlobItems just copied, and StringBlobItems replaced with ByteArrayBlobItems, with encoding and current endings attached. BTW, why do we need to make all Blobs (or BlobItems, if we'll have them) ThreadSafeShared? I suspect it's because of the FileThread? It would be unfortunate, since it would slow down cases when FileThread is not in the picture (building a blob and sending it via XHR does not require thread safety). Also, and it may be a bigger issue, it is unclear to me what is the story about BlobBuilder and FormData. They feel almost identical objects. It seems we might want to have one of those 'lists of blobs builders'.

Thanks for your comments. Let me update with the current status. (In reply to comment #7) > Looking at the patch, I think we can make what you have as CombinedBlob to be > the only type of the blob... A Blob. It would be always a list of BlobItems. A > BlobItem would be an abstract class, with implementations like FileBlobItem, > StringBlobItem, ByteArrayBlobItem.When File derives from the Blob, such Blob > would only have 1 FielBlobitem in the list... StringBlobItem would be the one > constructed from string and stored in a mutable list of BlobBuilder. Once > BlobBuilder::getBlob() (it probably should be createBlob()) is called, the new > list created, with FileBlobItems just copied, and StringBlobItems replaced with > ByteArrayBlobItems, with encoding and current endings attached. I've started to make those changes with an attempt to refactor BlobBuilder and FormData. In my local tree I'm trying / have tried to: - move most of Blob implementation into BlobItems and make Blob a simple collection of BlobItems. - File would be a special Blob that contains only one FileBlobItem. - BlobBuilder would be a very lightweight class as well. - refactor FormDataList::Item and FormDataElement as a wrapper of BlobItem. This makes appending Blobs to FormData much simpler (it can just pass a list of BlobItems). For now I'm trying to make them keep the same interface as before to avoid refactoring in platform-dependent code. The changes are still rough but I want to get some feedbacks in a design aspect before I go too far. How do they sound to you? Fyi my current patch is uploaded at: http://codereview.chromium.org/1769002 > BTW, why do we need to make all Blobs (or BlobItems, if we'll have them) > ThreadSafeShared? I suspect it's because of the FileThread? No, we won't need it to be ThreadSafeShared. When I wrote the initial patch I wasn't very sure but in the current design it'll be ok without making it ThreadSafeShared (at least for FileReader/Writer usage).

Created attachment 58260 [details] Patch

Uploaded a new patch for BlobBuilder based on BlobItem.

Comment on attachment 58260 [details] Patch WebCore/html/Blob.h:74 + Blob(const String& type, const BlobItemList& items); 'items' can be omitted. WebCore/html/Blob.cpp:94 + return Blob::create(contentType, items); // FIXME: Pass content type when we add it to slice's arguments. return Blob::create(String(), items); WebCore/html/BlobBuilder.cpp:39 + #include "StringHash.h" Is this include needed? WebCore/html/BlobBuilder.cpp:41 + #include <wtf/HashMap.h> Is this include needed? WebCore/html/BlobBuilder.cpp:81 + if (!blob) Probably it is simpler to say: if (blob) for (...) ... return true; WebCore/html/BlobBuilder.h:37 + #include "ExceptionCode.h" Including ExceptionCode.h is not needed. WebCore/html/BlobBuilder.h:51 + bool appendString(const WebCore::String& text, const WebCore::String& ending, ExceptionCode& ec); The namespace WebCore is not necessary. WebCore/html/BlobBuilder.h:54 + PassRefPtr<Blob> getBlob(const WebCore::String& contentType) const; ditto. WebCore/platform/BlobItem.cpp:135 + #else How about CR in mac?

Created attachment 58416 [details] Patch

(In reply to comment #11) > (From update of attachment 58260 [details]) > WebCore/html/Blob.h:74 > + Blob(const String& type, const BlobItemList& items); > 'items' can be omitted. Done. > WebCore/html/Blob.cpp:94 > + return Blob::create(contentType, items); > // FIXME: Pass content type when we add it to slice's arguments. > return Blob::create(String(), items); Done. Accordingly changed the Blob::create's first param const String& -> const String (its implementation is just a RefPtr). > WebCore/html/BlobBuilder.cpp:39 > + #include "StringHash.h" > + #include <wtf/HashMap.h> Removed both includes. > WebCore/html/BlobBuilder.cpp:81 > + if (!blob) > Probably it is simpler to say: > if (blob) > for (...) > ... > return true; Fixed. > WebCore/html/BlobBuilder.h:37 > + #include "ExceptionCode.h" > Including ExceptionCode.h is not needed. Fixed. > WebCore/html/BlobBuilder.h:51 > + bool appendString(const WebCore::String& text, const WebCore::String& ending, ExceptionCode& ec); > The namespace WebCore is not necessary. Fixed. > + PassRefPtr<Blob> getBlob(const WebCore::String& contentType) const; > ditto. Fixed. > WebCore/platform/BlobItem.cpp:135 > + #else > How about CR in mac? Is older Mac supported by WebKit?

Comment on attachment 58416 [details] Patch Some more comments. WebCore/html/Blob.h:44 + static PassRefPtr<Blob> create(const String type, const BlobItemList& items) "const String" in create() is inconsistent with "const String&" in Blob(). WebCore/html/Blob.h:73 + Blob() { } Is this needed? WebCore/html/BlobBuilder.h:40 + #include <wtf/Vector.h> This seems not to be needed. WebCore/html/BlobBuilder.h:57 + BlobBuilder(); We do not really need this default constructor.

Created attachment 58523 [details] Patch

(In reply to comment #14) > (From update of attachment 58416 [details]) > Some more comments. > > WebCore/html/Blob.h:44 > + static PassRefPtr<Blob> create(const String type, const BlobItemList& items) > "const String" in create() is inconsistent with "const String&" in Blob(). Fixed. > WebCore/html/Blob.h:73 > + Blob() { } > WebCore/html/BlobBuilder.h:40 > + #include <wtf/Vector.h> > WebCore/html/BlobBuilder.h:57 > + BlobBuilder(); Removed them.

Comment on attachment 58523 [details] Patch r=me

Committed r61149: <http://trac.webkit.org/changeset/61149>