Bug 39083

Created attachment 56030 [details] Patch

The 1st patch for refactoring (some of details about the changes are noted in bug 36903). The patch doesn't include any interface changes made recently in Blob (e.g. type, slice's type argument etc). I've tried to make it smaller but this might be still too large. Once we agree on the general design I think I can split the patch more.

Attachment 56030 [details] did not build on qt: Build output: http://webkit-commit-queue.appspot.com/results/2249045

Attachment 56030 [details] did not build on gtk: Build output: http://webkit-commit-queue.appspot.com/results/2256045

Created attachment 56034 [details] Patch

Comment on attachment 56034 [details] Patch Note that the line number might be somewhat off since what I put here is based on your first patch. WebCore/html/Blob.cpp:43 + return adoptRef(static_cast<Blob*>(File::create(path).get())); I do not understand this. When do we invoke File::create from Blob::create? WebCore/html/Blob.cpp:80 + } else if (static_cast<unsigned long long>(start + length) > totalSize) What if start < 0? WebCore/html/Blob.h:49 + // A special constructor for File blob. Any one is calling this creator? I do not see any usage for this. Also, this creator is very confused with the protected constructor with type string as argument because the creator has path string as argument. WebCore/html/Blob.h:59 + typedef Vector<RefPtr<BlobItem> > ItemList; Could you please add include for Vector.h? WebCore/html/BlobItem.cpp:333 + String m_uniqName; Should it be m_uniqueName? WebCore/html/BlobItem.h:54 + virtual ~BlobItem() { } Why not making it protected? WebCore/html/BlobItem.h:66 + static PassRefPtr<BlobItem> createFileRangeItem(const String& path, long long start, long long length, double snapshotModificationTime); Can we merge these two if there're very similar? If not, it might be better to name RangeBlobItem as DataRangeItem to avoid the confusion since BlobRangeItem sounds more like a super set? WebCore/html/BlobItem.h:73 + virtual const CString cstr() const { return CString(); } Is it possible to move this method to StringBlobItem so that we have cleaner BlobItem interface? Doing so will make us move all the derived classes to the header file, but I think it will make the readability better? WebCore/html/BlobItem.h:76 + virtual const String filePath() const { return String(); } ditto. WebCore/html/BlobItem.h:77 + virtual const String fileOrUniqueName() const { return String(); } ditto. In addition, probably uniqueName() should be enough. WebCore/html/BlobItem.h:80 + bool isFile() const { return !filePath().isEmpty(); } ditto. WebCore/html/BlobItem.h:84 + virtual bool isRange() const { return false; } ditto. WebCore/html/BlobItem.cpp:129 + void StringBlobItem::ensureFinalCString() const What does this function do? Could you please add a comment? WebCore/html/File.cpp:46 + ASSERT(items().size() == 1 && items().at(0)->isFile()); Do we need this ASSERT? The above code will unlikely fail. WebCore/html/File.cpp:51 + ASSERT(items().size() == 1 && items().at(0)->isFile()); Do we need this ASSERT? WebCore/html/FileStream.cpp:84 + if (!getFileModificationTime(file->filePath(), currentModificationTime)) { Why not simply calling blob->path() since it already contains the same logic? You can cache the path if needed.

Comment on attachment 56034 [details] Patch WebCore/platform/network/FormData.h:23 + #include "Blob.h" stuff in platform/ should not depend on stuff in html/

Created attachment 56085 [details] Patch

(In reply to comment #7) > (From update of attachment 56034 [details]) > WebCore/platform/network/FormData.h:23 > + #include "Blob.h" > stuff in platform/ should not depend on stuff in html/ Oh, I see... if that's true we need a bit more restructuring. FormData seems to be already including a few html files: html/Blob.h html/DOMFormData.h" html/File.h (we won't need this if we do this refactoring) Where would be the recommended place to put those header files?

Comment on attachment 56085 [details] Patch I think it might be better to rename BlobItem::createFileItem to BlobItem::createFileBlobItem, and BlobItem::toFileItem to BlobItem::toFileBlobItem, for better understanding. Otherwise, when I see toFileItem, I have to realize that it is to convert to FileBlobItem. The other fix, while keeping createFileItem and asFileItem, is to move all BlobItem stuffs to stay inside the class Blob and then we can remove all Blob pattern from the class names. But this will increase the complexity of Blob.h. FormDataList and FormData seem not to be fully refactored. You still try to keep almost all the old data members for backward compatibility. Do you have plan to refactor these codes to make them better abstracted based on the new BlobItem structures? WebCore/bindings/v8/SerializedScriptValue.cpp:562 + m_writer.writeFile(blob->path()); Why do we need to change this? If we do change it as a temporary hack, please put more detail for FIXME. WebCore/bindings/v8/SerializedScriptValue.cpp:858 + PassRefPtr<File> blob = File::create(path); ditto. WebCore/html/Blob.h:58 + void appendBlobItem(PassRefPtr<BlobItem> item); IMO, append or appendItem sounds more natural. WebCore/html/BlobItem.h:54 + class FileRangeBlobItem; Please sort the above forward declarations. WebCore/html/BlobItem.h:65 + static PassRefPtr<BlobItem> createStringItem(const String& text, EndingType, TextEncoding); I think we can omit the argument name "text". WebCore/html/BlobItem.h:66 + static PassRefPtr<BlobItem> createStringItem(const CString& text); ditto. WebCore/html/BlobItem.h:94 + // BlobItem class for a string. We probably don't need comment here since the class name is straightforward. This also applies to other subclasses. WebCore/html/BlobItem.h:95 + class StringBlobItem: public BlobItem { Please add one space before ':'. This also applies to other subclasses. WebCore/html/BlobItem.h:97 + StringBlobItem(const String& text, EndingType, TextEncoding); I think we can omit text here. WebCore/html/BlobItem.h:98 + StringBlobItem(const CString& text); ditto. WebCore/html/BlobItem.cpp:42 + if (size() != b->size() || data() != b->data() || isValid() != b->isValid()) I think we should only check isValid() here. For example, for FileBlobItem, we do not want to get and check the size. WebCore/html/BlobItem.cpp:88 + ASSERT(start >= 0); Do we also need to ASSERT on length? WebCore/html/BlobItem.cpp:91 + if (item->toFileItem()) { Better to say: FileBlobItem* fileItem = item->toFileItem(); if (fileItem) WebCore/html/BlobItem.cpp:95 + if (static_cast<unsigned long long>(start + length) > item->size()) + length = item->size() - start; Should the above two lines be moved after adjusting start? WebCore/html/BlobItem.cpp:141 + if (m_path != f->m_path || m_fileName != f->m_fileName) I think checking the path should be enough. In addition, it would be simpler to combine the last if check with this if check. WebCore/html/BlobItem.cpp:157 + : (ending == EndingCR) ? "\r" : "\n"; No need to fold into 2 lines. In addition, please use parentheses to wrap the 2nd conditional express for better readability and safety.

Created attachment 56283 [details] Patch

Created attachment 56286 [details] Patch

Attachment 56286 [details] did not build on chromium: Build output: http://webkit-commit-queue.appspot.com/results/2256285

Created attachment 56304 [details] Patch

Created attachment 56305 [details] Patch

Thanks for reviewing. (In reply to comment #10) > (From update of attachment 56085 [details]) > I think it might be better to rename BlobItem::createFileItem to BlobItem::createFileBlobItem, and BlobItem::toFileItem to BlobItem::toFileBlobItem, for better understanding. Otherwise, when I see toFileItem, I have to realize that it is to convert to FileBlobItem. > The other fix, while keeping createFileItem and asFileItem, is to move all BlobItem stuffs to stay inside the class Blob and then we can remove all Blob pattern from the class names. But this will increase the complexity of Blob.h. Agreed, I took the former option, i.e. changed the method names to {to,create}XxxBlobItem(). > FormDataList and FormData seem not to be fully refactored. You still try to keep almost all the old data members for backward compatibility. Do you have plan to refactor these codes to make them better abstracted based on the new BlobItem structures? Right. I'm willing to refactor FormDataList and FormData code further. I thought changing everything in a patch would be a bit scary. (Or I can extend this patch if it would be better.) > WebCore/bindings/v8/SerializedScriptValue.cpp:562 > + m_writer.writeFile(blob->path()); > Why do we need to change this? If we do change it as a temporary hack, please put more detail for FIXME. > > WebCore/bindings/v8/SerializedScriptValue.cpp:858 > + PassRefPtr<File> blob = File::create(path); > ditto. Reverted some changes (calling File methods for read/writeBlob) and added more details to the FIXME comment. > WebCore/html/Blob.h:58 > + void appendBlobItem(PassRefPtr<BlobItem> item); > IMO, append or appendItem sounds more natural. Fixed. > WebCore/html/BlobItem.h:54 > + class FileRangeBlobItem; > Please sort the above forward declarations. Fixed. > WebCore/html/BlobItem.h:65 > + static PassRefPtr<BlobItem> createStringItem(const String& text, EndingType, TextEncoding); > I think we can omit the argument name "text". Fixed. > WebCore/html/BlobItem.h:66 > + static PassRefPtr<BlobItem> createStringItem(const CString& text); > ditto. Fixed. > WebCore/html/BlobItem.h:94 > + // BlobItem class for a string. > We probably don't need comment here since the class name is straightforward. This also applies to other subclasses. Fixed. > WebCore/html/BlobItem.h:95 > + class StringBlobItem: public BlobItem { > Please add one space before ':'. This also applies to other subclasses. Fixed. > WebCore/html/BlobItem.h:97 > + StringBlobItem(const String& text, EndingType, TextEncoding); > I think we can omit text here. Fixed. > WebCore/html/BlobItem.h:98 > + StringBlobItem(const CString& text); > ditto. Fixed. > WebCore/html/BlobItem.cpp:42 > + if (size() != b->size() || data() != b->data() || isValid() != b->isValid()) > I think we should only check isValid() here. For example, for FileBlobItem, we do not want to get and check the size. Right. Instead removed the BlobItem::equals implementation at all. > WebCore/html/BlobItem.cpp:88 > + ASSERT(start >= 0); > Do we also need to ASSERT on length? Added another ASSERT for length. > WebCore/html/BlobItem.cpp:91 > + if (item->toFileItem()) { > Better to say: > FileBlobItem* fileItem = item->toFileItem(); > if (fileItem) Fixed. > WebCore/html/BlobItem.cpp:95 > + if (static_cast<unsigned long long>(start + length) > item->size()) > + length = item->size() - start; > Should the above two lines be moved after adjusting start? For FileRangeBlobItem item->size() returns the length of the sliced range, so it should be ok. > WebCore/html/BlobItem.cpp:141 > + if (m_path != f->m_path || m_fileName != f->m_fileName) > I think checking the path should be enough. In addition, it would be simpler to combine the last if check with this if check. Fixed. > WebCore/html/BlobItem.cpp:157 > + : (ending == EndingCR) ? "\r" : "\n"; > No need to fold into 2 lines. In addition, please use parentheses to wrap the 2nd conditional express for better readability and safety. Fixed.

Comment on attachment 56305 [details] Patch I'll fix the dependency violation issue first.

FYI, here are some comments for your last patch: You might need to add the new file BlobItem.cpp to the new make file: CMakeLists.txt. In addition, I think BlobItem could be put under platform directory so that it can be referred to by both html/Blob and platform/network/FormData. WebCore/bindings/v8/SerializedScriptValue.cpp:562 + // for non-file blobs. No need to fold into 2 lines. WebCore/html/Blob.h:50 + static PassRefPtr<Blob> createFromFile(const String& path, const String type = String()) Should it be "const String type& = ..."? WebCore/html/BlobItem.cpp:68 + static const double invalidModificationTime = -1; Why do we use -1, instead of 0? WebCore/html/BlobItem.h:74 + static PassRefPtr<BlobItem> createFileRangeBlobItem(const String& path, long long start, long long length, double snapshotModificationTime); It would be better to return "PassRefPtr<FileRangeBlobItem>". WebCore/html/BlobItem.h:61 + virtual const char* data() const { return 0; } data() should only make sense for StringBlobItem, BytesArrayBlobItem and DataRangeBlobItem. It is a little bit wield to put it in the base class. Can we move it to the subclass? Since we do not want to add it for each of these subclasses, how about restructure BlobItem classes like the following: BlobItem | --------> DataBlobItem | | | ----------> StringBlobItem | ----------> ByteArrayBlobItem | ----------> DataRangeBlobItem | --------> FileBlobItem | | | ----------> FileRangeBlobItem We can then add data() and releaseBuffer() to DataBlobItem. WebCore/html/File.cpp:  + // We don't use MIMETypeRegistry::getMIMETypeForPath() because it returns "application/octet-stream" upon failure. Can we keep the comment to help people better understand why we call MIMETypeRegistry::getMIMETypeForExtension? WebCore/platform/network/FormData.h:23 + #include "Blob.h" As Darin pointed out, FormData is under platform/network so it should not depend on other non-platform stuff. We could move BlobItem to platform.

Created attachment 56524 [details] Patch

Thanks, I've moved the BlobItem under platform and fixed dependencies. (In reply to comment #18) > You might need to add the new file BlobItem.cpp to the new make file: CMakeLists.txt. Done. > WebCore/bindings/v8/SerializedScriptValue.cpp:562 > WebCore/html/Blob.h:50 > + static PassRefPtr<Blob> createFromFile(const String& path, const String type = String()) In this patch I left it untouched and removed the type parameter from the Blob constructor (as no one is calling Blob::create(type) yet - we'll need it when we add slice(type) or BlobBuilder). > WebCore/html/BlobItem.cpp:68 > + static const double invalidModificationTime = -1; > Why do we use -1, instead of 0? Changed to 0. Back then I wanted to differentiate uninitialized motd (when the item is not a lice) and invalid one (initialization failure) but now we won't need it because the base class doesn't have the field. > WebCore/html/BlobItem.h:74 > + static PassRefPtr<BlobItem> createFileRangeBlobItem(const String& path, long long start, long long length, double snapshotModificationTime); > It would be better to return "PassRefPtr<FileRangeBlobItem>". Hmm, I think it would be easier to return PassRefPtr<BlobItem> for all create methods because in that way we can uniformly assign the return value to RefPtr<BlobItem>. > WebCore/html/BlobItem.h:61 > + virtual const char* data() const { return 0; } > data() should only make sense for StringBlobItem, BytesArrayBlobItem and DataRangeBlobItem. It is a little bit wield to put it in the base class. Can we move it to the subclass? Since we do not want to add it for each of these subclasses, how about restructure BlobItem classes like the following: Sounds great. I made the change. > WebCore/html/File.cpp:  > + // We don't use MIMETypeRegistry::getMIMETypeForPath() because it returns "application/octet-stream" upon failure. > Can we keep the comment to help people better understand why we call MIMETypeRegistry::getMIMETypeForExtension? Done. > WebCore/platform/network/FormData.h:23 > + #include "Blob.h" > As Darin pointed out, FormData is under platform/network so it should not depend on other non-platform stuff. We could move BlobItem to platform. Done.

Attachment 56524 [details] did not build on chromium: Build output: http://webkit-commit-queue.appspot.com/results/2258349

Comment on attachment 56524 [details] Patch Looks much better now. Some more comments. WebCore/platform/BlobItem.h:69 + // | | Nice diagram. Probably better to remove the 1st "|". WebCore/platform/BlobItem.h:70 + // | +---------> FileRangeBlobItem ditto. WebCore/platform/BlobItem.h:78 + static PassRefPtr<BlobItem> createStringBlobItem(const String&, EndingType, TextEncoding); I am thinking if we really need these factory methods. Can we simply call something like StringBlobItem::create() in order to make the whole logic simpler? The only different creator is createRangeBlobItem. Can we make it as slice? WebCore/platform/BlobItem.h:85 + static PassRefPtr<BlobItem> createRangeBlobItem(PassRefPtr<BlobItem> item, long long start, long long length); item can be omitted. WebCore/platform/BlobItem.h:93 + virtual const DataBlobItem* toDataBlobItem() const { return 0; } Do we need to add const modifier? Would "virtual DataBlobItem* toDataBlobItem() { return 0; }" be more straightforward? WebCore/platform/BlobItem.h:94 + virtual const StringBlobItem* toStringBlobItem() const { return 0; } Would it be better to move toStringBlobItem to DataBlobItem? Do we want to also add toByteRangeBlobItem? WebCore/platform/BlobItem.h:115 + virtual const DataBlobItem* toDataBlobItem() const { return this; } Better to add an empty line and a line for "// BlobItem methods." before this line, like what you do for other subclass. WebCore/platform/BlobItem.h:122 + virtual const String& uniqueName() const { return m_fileName; } I do not think file name is a unique name. We should either move it to FileRangeBlobItem or leave it in FormData::append. WebCore/platform/BlobItem.h:158 + virtual unsigned long long size() const { return m_bytesArray.size(); } Better to add an empty line and a line for "// BlobItem methods." before this line. WebCore/platform/BlobItem.h:173 + DataRangeBlobItem(PassRefPtr<BlobItem> item, long long start, long long length); item can be omitted. WebCore/platform/BlobItem.h:185 + RefPtr<BlobItem> m_item; I think m_item should be RefPtr<DataBlobItem>. WebCore/html/FormDataList.h:25 + #include "File.h" Why changing from including Blob.h to File.h? I do not see any reference to File type. WebKit/chromium/ChangeLog:8 + Replace FileItemList::Item list with BlobItemList to get it compiling I think you mean "FormDataList::Item".

Created attachment 56609 [details] Patch

Created attachment 56610 [details] Patch

(In reply to comment #22) > (From update of attachment 56524 [details]) > WebCore/platform/BlobItem.h:69 > + // | | > Nice diagram. Probably better to remove the 1st "|". Fixed. > WebCore/platform/BlobItem.h:78 > + static PassRefPtr<BlobItem> createStringBlobItem(const String&, EndingType, TextEncoding); > I am thinking if we really need these factory methods. Can we simply call something like StringBlobItem::create() in order to make the whole logic simpler? > The only different creator is createRangeBlobItem. Can we make it as slice? Done. I was kind of separated on this since they return BlobItem, but now we have subclass definitions in the header file and probably it would be better to do also for readability. I changed the createRangeBlob to a member method slice. > WebCore/platform/BlobItem.h:85 > + static PassRefPtr<BlobItem> createRangeBlobItem(PassRefPtr<BlobItem> item, long long start, long long length); > item can be omitted. Fixed. > WebCore/platform/BlobItem.h:93 > + virtual const DataBlobItem* toDataBlobItem() const { return 0; } > Do we need to add const modifier? Would "virtual DataBlobItem* toDataBlobItem() { return 0; }" be more straightforward? It might be, but almost all of their methods are const, and Blobs are expected to be immutable... so I'd like to keep them const until it becomes a real problem. Does that make sense to you? > WebCore/platform/BlobItem.h:94 > + virtual const StringBlobItem* toStringBlobItem() const { return 0; } > Would it be better to move toStringBlobItem to DataBlobItem? Do we want to also add toByteRangeBlobItem? It ought to be if we want to make the interface really strict, but I'm afraid it may make code a bit too complicated. If we do that we'd need to call (toDataBlobItem() && toDataBlobItem()->toStringBlobItem()) every time to access its string data. Do you think we want to do that? > WebCore/platform/BlobItem.h:115 > + virtual const DataBlobItem* toDataBlobItem() const { return this; } > Better to add an empty line and a line for "// BlobItem methods." before this line, like what you do for other subclass. Fixed. > WebCore/platform/BlobItem.h:122 > + virtual const String& uniqueName() const { return m_fileName; } > I do not think file name is a unique name. We should either move it to FileRangeBlobItem or leave it in FormData::append. That's true. Can you help me make a few things clear? Do we want to use the same name for the same sliced blobs? If so I think we'd better store it in Blob/BlobItem but otherwise I should rather leave it in FormData::append. Also: will we have different usages for fileName and UUID-based uniqueName? If that applies I'll split into two different methods (e.g. FileBlobItem::fileName(0 and FileRangeBlobItem::uniqueName()) rather than one. (In this patchset I just changed the method name to name(), but I don't have strong opinion on it.) > WebCore/platform/BlobItem.h:158 > + virtual unsigned long long size() const { return m_bytesArray.size(); } Fixed. > WebCore/platform/BlobItem.h:173 > + DataRangeBlobItem(PassRefPtr<BlobItem> item, long long start, long long length); > item can be omitted. Fixed. > WebCore/platform/BlobItem.h:185 > + RefPtr<BlobItem> m_item; > I think m_item should be RefPtr<DataBlobItem>. Done. > WebCore/html/FormDataList.h:25 > + #include "File.h" > Why changing from including Blob.h to File.h? I do not see any reference to File type. Probably by a mistake - thanks for catching it. > WebKit/chromium/ChangeLog:8 > + Replace FileItemList::Item list with BlobItemList to get it compiling > I think you mean "FormDataList::Item". Fixed. Again thanks for catching it.

Comment on attachment 56610 [details] Patch WebCore/ChangeLog:8 + - Introduces a new class BlobItem (platform/BlobData.{h,cpp}) as Should it be "platform/BlobItem.{h,cpp}"? Perhaps we do not even need to mention the file names. WebCore/ChangeLog:22 + The existing tests should be able to be used for regression: There're quite a few more tests to test file and form data. Probably we do not need to mention the test names. WebCore/html/Blob.h:50 + static PassRefPtr<Blob> create(const String& path) Seems that we do not need this creator any more. WebCore/html/Blob.h:58 + const String& path() const; Is this still needed now? I do not see any reference to path() now since we access the underlying items directly. WebCore/html/Blob.h:62 + void append(PassRefPtr<BlobItem> item); item name can be omitted. WebCore/html/Blob.h:73 + BlobItemList m_itemList; Probably it is simpler to call the name as BlobItems and m_items. Not only for the simpler names, but also consistent with items() getter. WebCore/html/Blob.cpp:40 + Blob::Blob(const String& path) I think we do not need this constructor. Instead, we can move the append operation to the File constructor. WebCore/html/FileStream.cpp:93 + const FileRangeBlobItem* range = blob->items().at(0)->toFileRangeBlobItem(); Please add an assert to ensure the blob has only one item. Also a FIXME about needing to handle multiple items when BlobBuilder is introduced. WebCore/html/FormDataList.h:49 + void appendBlob(const String& key, PassRefPtr<Blob> blob); blob can be omitted. WebCore/platform/BlobItem.cpp:63 + start += fileRangeItem->start(); What if the new start value is beyond the old range? It seems that we're only doing partial checks and clamps in BlobItem::slice, which is quite risky. Why not moving the clamp logic in Blob::slice to here? WebCore/platform/BlobItem.cpp:82 + return v.releaseBuffer(); I think this would incur extra overhead for ByteArrayBlobItem that already uses Vector<char> to hold the data. Probably it is better to move the implementation of releaseBuffer to each subclass. You might also choose to override releaseBuffer in ByteArrayBlobItem. WebCore/platform/BlobItem.cpp:122 + if (!fileItem || m_path != fileItem->m_path) It is simpler to say: return fileItem && m_path == fileItem->m_path; WebCore/platform/BlobItem.cpp:140 + StringBlobItem::StringBlobItem(const String& text, EndingType ending, TextEncoding encoding) The constructor body seems to be too cumbersome. Can we move the ending logic to a helper function? WebCore/platform/BlobItem.cpp:243 + : m_item(item) For better readability, can we move setting m_item and m_start values to be else part of the if check in the constructor body? WebCore/platform/BlobItem.cpp:248 + const DataRangeBlobItem* rangeItem = m_item->toDataRangeBlobItem(); This line can be moved before the above if statement. WebCore/platform/BlobItem.cpp:273 + if (!m_item->equals(rangeItem->m_item.get())) It would be simpler to combine all 3 if checks to one return. WebCore/platform/network/FormData.cpp:190 + void FormData::appendFormDataItems(const BlobItemList& list, const TextEncoding& encoding, bool isMultiPartForm, Document* document) The name appendFormDataItems is a bit confusing. How about appendItems? WebCore/platform/network/FormData.h:133 + void appendFormDataItems(const BlobItemList& formDataItems, const TextEncoding&, bool isMultiPartForm, Document* document); We can omit the names of 1st and last arguments.

Created attachment 56706 [details] Patch

Attachment 56706 [details] did not build on qt: Build output: http://webkit-commit-queue.appspot.com/results/2311386

Created attachment 56709 [details] Patch

(In reply to comment #26) > (From update of attachment 56610 [details]) > WebCore/ChangeLog:8 > + - Introduces a new class BlobItem (platform/BlobData.{h,cpp}) as > Should it be "platform/BlobItem.{h,cpp}"? Perhaps we do not even need to mention the file names. I removed the (wrong) file names. > WebCore/ChangeLog:22 > + The existing tests should be able to be used for regression: > There're quite a few more tests to test file and form data. Probably we do not need to mention the test names. Removed the test names too. > WebCore/html/Blob.h:50 > + static PassRefPtr<Blob> create(const String& path) > Seems that we do not need this creator any more. Really? There still seem to be a line calling Blob::create(path) in WebCore/bindings/v8/SerializedScriptValue.cpp. We'll need to fix the usage once we introduce BlobBuilderf but I don't want to put too many changes in this patch. I added 'FIXME: deprecated' comment to the function. > WebCore/html/Blob.h:58 > + const String& path() const; > Is this still needed now? I do not see any reference to path() now since we access the underlying items directly. This is called in FileStream.cpp and again in v8/SerializedScriptValue.cpp. I added the 'FIXME: deprecated' comment here too. > WebCore/html/Blob.h:62 > + void append(PassRefPtr<BlobItem> item); > item name can be omitted. Done. > WebCore/html/Blob.h:73 > + BlobItemList m_itemList; > Probably it is simpler to call the name as BlobItems and m_items. Not only for the simpler names, but also consistent with items() getter. I renamed m_itemList (and other similar variable names) to m_items. As for the type name, doesn't BlobItems sound too similar to BlobItem? > WebCore/html/Blob.cpp:40 > + Blob::Blob(const String& path) > I think we do not need this constructor. Instead, we can move the append operation to the File constructor. Again this is left only for Blob::create(path). I added a FIXME comment. > WebCore/html/FileStream.cpp:93 > + const FileRangeBlobItem* range = blob->items().at(0)->toFileRangeBlobItem(); > Please add an assert to ensure the blob has only one item. Also a FIXME about needing to handle multiple items when BlobBuilder is introduced. Done. > WebCore/html/FormDataList.h:49 > + void appendBlob(const String& key, PassRefPtr<Blob> blob); > blob can be omitted. Done. > WebCore/platform/BlobItem.cpp:63 > + start += fileRangeItem->start(); > What if the new start value is beyond the old range? It seems that we're only doing partial checks and clamps in BlobItem::slice, which is quite risky. Why not moving the clamp logic in Blob::slice to here? Blob::slice() shouldn't call BlobItem::slice() while start is larger than the item's size(), that is the item's valid range. Also it won't make sense if BlobItem::slice() is called with the start > size(). We can return an empty BlobItem (say BytesArrayBlobItem) but it'll only confuse the whole logic assume BlobItem is only used as a collection of items. I added an assert ASSERT(start < size()) in BlobItem::slice(), made BlobItem::slice() private and registered Blob as a friend class of BlobItem. (Rather I thought we may want to put all the logic in BlobItem::slice() into Blob::slice(), but what BlobItem::slice() is doing is deeply related to other BlobItem code.) > WebCore/platform/BlobItem.cpp:82 > + return v.releaseBuffer(); > I think this would incur extra overhead for ByteArrayBlobItem that already uses Vector<char> to hold the data. Vector::releaseBuffer makes the vector's buffer empty by passing its buffer ownership to the caller. BlobItem is ref-counted and should keep its data as far as its ref is >0, so it needs to make a copy. I added a comment here. > WebCore/platform/BlobItem.cpp:122 > + if (!fileItem || m_path != fileItem->m_path) > It is simpler to say: > return fileItem && m_path == fileItem->m_path; Done. > WebCore/platform/BlobItem.cpp:140 > + StringBlobItem::StringBlobItem(const String& text, EndingType ending, TextEncoding encoding) > The constructor body seems to be too cumbersome. Can we move the ending logic to a helper function? Done. > WebCore/platform/BlobItem.cpp:243 > + : m_item(item) > For better readability, can we move setting m_item and m_start values to be else part of the if check in the constructor body? Done. > WebCore/platform/BlobItem.cpp:248 > + const DataRangeBlobItem* rangeItem = m_item->toDataRangeBlobItem(); > This line can be moved before the above if statement. Done. > WebCore/platform/BlobItem.cpp:273 > + if (!m_item->equals(rangeItem->m_item.get())) > It would be simpler to combine all 3 if checks to one return. Done. > WebCore/platform/network/FormData.cpp:190 > + void FormData::appendFormDataItems(const BlobItemList& list, const TextEncoding& encoding, bool isMultiPartForm, Document* document) > The name appendFormDataItems is a bit confusing. How about appendItems? Done. > WebCore/platform/network/FormData.h:133 > + void appendFormDataItems(const BlobItemList& formDataItems, const TextEncoding&, bool isMultiPartForm, Document* document); > We can omit the names of 1st and last arguments. Done.

Attachment 56706 [details] did not build on chromium: Build output: http://webkit-commit-queue.appspot.com/results/2249445

Attachment 56709 [details] did not build on qt: Build output: http://webkit-commit-queue.appspot.com/results/2298396

Attachment 56709 [details] did not build on chromium: Build output: http://webkit-commit-queue.appspot.com/results/2319389

Please check compiler errors in cr-linux and qt.

Created attachment 56861 [details] Patch (fixed cr-linux build)

Comment on attachment 56861 [details] Patch (fixed cr-linux build) Some more comments. It is very close. WebCore/html/Blob.h:36 + #include "TextEncoding.h" This include seems not to be needed. WebCore/html/Blob.h:50 + // FIXME: deprecated method. This is called only from Please capitalize the 1st letter 'd' after FIXME. Please also change other FIXMEs. WebCore/html/File.cpp:38 + int index = name().reverseFind('.'); Might be better to cache name() first, like: const String& name = name(); WebCore/html/FileStream.cpp:75 + // FIXME: need to handle multiple items that may include non-file ones No need to fold into 2 lines. WebCore/html/FileStream.cpp:78 + const FileBlobItem* file = blob->items().at(0)->toFileBlobItem(); Since it will take some time to fix this to support any blob after BlobBuilder is introduced, I think it will be better to return an error instead of falling through and hitting a crash when the user passes a binary blob. Also, I think it will be better to name the variable as fileItem, in order not to be confused with file object. ASSERT(blob->items().size() == 1); const FileBlobItem* fileItem = blob->items().at(0)->toFileBlobItem(); if (!fileItem) { ASSERT(false); m_client->didFail(NOT_READABLE_ERR); return; } WebCore/html/FileStream.cpp:98 + const FileRangeBlobItem* range = file->toFileRangeBlobItem(); Better to name it as fileRangItem. WebCore/html/FormDataList.h:49 + void appendBlob(const String& key, PassRefPtr<Blob> blob); The parameter name 'blob' can be omitted. WebCore/platform/BlobItem.cpp:62 + if (static_cast<unsigned long long>(start + length) > size()) I think this check is also needed for non-range file item. How about moving the logic out, like: if (static_cast<unsigned long long>(start + length) > size()) length = size() - start; start += fileRangeItem->start(); const FileRangeBlobItem* fileRangeItem = toFileRangeBlobItem(); double modificationTime = fileRangeItem ? fileRangeItem->snapshotModificationTime() : getFileSnapshotModificationTime(fileItem->path()); return FileRangeBlobItem::create(fileItem->path(), start, length, modificationTime); WebCore/platform/BlobItem.cpp:79 + char* DataBlobItem::releaseBuffer() const I see what you mean. However, the name 'releaseBuffer' suggests to release the internal buffer while the implementation here does not mean this. Please add a FIXME to address this issue later. Probably consider renaming the method? WebCore/platform/BlobItem.h:97 + friend class Blob; Probably it is not a good idea to add a non-platform class as a friend. You might remove the friend definition and move the slice to public as long as slice does the right check. WebCore/platform/BlobItem.h:104 + // In most cases BlobItem's are handled as a list (e.g. in Blob, FormData). This comment is somewhat confusing. Probably we can remove it. WebCore/platform/network/FormData.cpp:49 + const FileRangeBlobItem* range = m_item->toFileRangeBlobItem(); Better to name range as fileRangeItem. WebKit/chromium/ChangeLog:8 + Replace FormDataList::Item list with BlobItemList to get it compiling compiling => compiled?

Created attachment 57016 [details] Patch

Attachment 57016 [details] did not build on qt: Build output: http://webkit-commit-queue.appspot.com/results/2301604

Created attachment 57018 [details] Patch

(In reply to comment #36) > WebCore/html/Blob.h:36 > + #include "TextEncoding.h" > This include seems not to be needed. Removed. > WebCore/html/Blob.h:50 > + // FIXME: deprecated method. This is called only from > Please capitalize the 1st letter 'd' after FIXME. Please also change other FIXMEs. Fixed. > WebCore/html/File.cpp:38 > + int index = name().reverseFind('.'); > Might be better to cache name() first, like: > const String& name = name(); Fixed. > WebCore/html/FileStream.cpp:75 > + // FIXME: need to handle multiple items that may include non-file ones > No need to fold into 2 lines. Fixed. > WebCore/html/FileStream.cpp:78 > + const FileBlobItem* file = blob->items().at(0)->toFileBlobItem(); > Since it will take some time to fix this to support any blob after BlobBuilder is introduced, I think it will be better to return an error instead of falling through and hitting a crash when the user passes a binary blob. > Also, I think it will be better to name the variable as fileItem, in order not to be confused with file object. Makes sense, fixed. > WebCore/html/FileStream.cpp:98 > + const FileRangeBlobItem* range = file->toFileRangeBlobItem(); > Better to name it as fileRangItem. Fixed. > WebCore/html/FormDataList.h:49 > + void appendBlob(const String& key, PassRefPtr<Blob> blob); > The parameter name 'blob' can be omitted. Fixed. > WebCore/platform/BlobItem.cpp:62 > + if (static_cast<unsigned long long>(start + length) > size()) > I think this check is also needed for non-range file item. How about moving the logic out, like: > if (static_cast<unsigned long long>(start + length) > size()) > length = size() - start; > start += fileRangeItem->start(); > > const FileRangeBlobItem* fileRangeItem = toFileRangeBlobItem(); > double modificationTime = fileRangeItem ? fileRangeItem->snapshotModificationTime() : getFileSnapshotModificationTime(fileItem->path()); > > return FileRangeBlobItem::create(fileItem->path(), start, length, modificationTime); Looks better, fixed. > WebCore/platform/BlobItem.cpp:79 > + char* DataBlobItem::releaseBuffer() const > I see what you mean. However, the name 'releaseBuffer' suggests to release the internal buffer while the implementation here does not mean this. Please add a FIXME to address this issue later. Probably consider renaming the method? Right. Did both - renamed the method and added FIXME comment. > WebCore/platform/BlobItem.h:97 > + friend class Blob; > Probably it is not a good idea to add a non-platform class as a friend. You might remove the friend definition and move the slice to public as long as slice does the right check. Fixed. > WebCore/platform/BlobItem.h:104 > + // In most cases BlobItem's are handled as a list (e.g. in Blob, FormData). > This comment is somewhat confusing. Probably we can remove it. Removed the comment. > WebCore/platform/network/FormData.cpp:49 > + const FileRangeBlobItem* range = m_item->toFileRangeBlobItem(); > Better to name range as fileRangeItem. Fixed. > WebKit/chromium/ChangeLog:8 > + Replace FormDataList::Item list with BlobItemList to get it compiling > compiling => compiled? Fixed.

Comment on attachment 57018 [details] Patch WebCore/platform/BlobItem.cpp:74 + char* DataBlobItem::deepCopyBuffer() const Probably copyBuffer() sounds simpler. WebCore/platform/BlobItem.cpp:111 + bool FileBlobItem::isValid() const This validity check is not necessary because the check is done when we open a file to read. Please also remove isValid() from FileBlobItem class definition. WebCore/platform/BlobItem.cpp:141 + Please remove the extra empty line. WebCore/platform/network/FormData.cpp:171 + void FormData::appendBlobItemList(const BlobItemList& list) This method is very confusing with appendItems because both take BlobItemList and the names sound similar while the semantics are very different. How about naming this method as appendItems (or appendBlobItems) and the other one as appendKeyValuePairItems (or appendBlobItemsInKeyValuePair)?

Created attachment 57069 [details] Patch

(In reply to comment #41) > (From update of attachment 57018 [details]) > WebCore/platform/BlobItem.cpp:74 > + char* DataBlobItem::deepCopyBuffer() const > Probably copyBuffer() sounds simpler. Renamed. > WebCore/platform/BlobItem.cpp:111 > + bool FileBlobItem::isValid() const > This validity check is not necessary because the check is done when we open a file to read. > Please also remove isValid() from FileBlobItem class definition. Removed FileBlobItem::isValid(). > WebCore/platform/BlobItem.cpp:141 > + > Please remove the extra empty line. Done. > WebCore/platform/network/FormData.cpp:171 > + void FormData::appendBlobItemList(const BlobItemList& list) > This method is very confusing with appendItems because both take BlobItemList and the names sound similar while the semantics are very different. > How about naming this method as appendItems (or appendBlobItems) and the other one as appendKeyValuePairItems (or appendBlobItemsInKeyValuePair)? Sounds good, renamed the other private method to appendKeyValuePairItems.

Attachment 57069 [details] did not build on chromium: Build output: http://webkit-commit-queue.appspot.com/results/2509010

Created attachment 57080 [details] Patch

Comment on attachment 57080 [details] Patch It looks good to me. I've asked fishd to take another look, esp on the naming part.

(In reply to comment #46) > (From update of attachment 57080 [details]) > It looks good to me. I've asked fishd to take another look, esp on the naming part. Thanks for your all of reviews Jian. Darin will you have time to take another look at this? Thanks.

Comment on attachment 57080 [details] Patch WebCore/platform/BlobItem.cpp:32 + nit: no new line here. WebCore/platform/BlobItem.h:52 + class BytesArrayBlobItem; nit: BytesArrayBlobItem -> ByteArrayBlobItem, the term "byte array" is more common. WebCore/platform/BlobItem.h:45 + enum EndingType { nit: LineEnding might be a more natural name for this enum. WebCore/platform/network/FormData.h:46 + BlobItemWrapper(const BlobItemWrapper& v) : m_item(v.m_item) { } this copy constructor should be generated by the compiler automatically. WebCore/platform/network/FormData.h:60 + char* releaseBuffer() const the name "releaseBuffer" typically implies an efficient operation that does not require the allocation of memory. it implies that memory is being released from one owner and passed to the caller to own. but this is calling copyBuffer, which is a copy. i see, however, that this is just a temporary solution. WebCore/platform/network/FormData.h:67 + } m_data; nit: it is usually better style to do: struct BlobItemWrapper { ... }; BlobItemWrapper m_data; WebCore/platform/BlobItem.cpp:59 + if (fileItem) { it seems like it would be better to define slice for DataBlobItem and FileBlobItem, and then leave BlobItem::slice pure virtual. the only code you are sharing is the checks at the top of this function.

How do you think of the naming? With this refactoring, BlobItem is the building block for both FormData and Blob. Probably it will be better to use a more generic name. I do not have a good name in mind. Maybe we can call it DataItem that has both BinaryDataItem and FileDataItem.

(In reply to comment #49) > How do you think of the naming? With this refactoring, BlobItem is the building block for both FormData and Blob. Probably it will be better to use a more generic name. I do not have a good name in mind. Maybe we can call it DataItem that has both BinaryDataItem and FileDataItem. I'm still somewhat confused by the desire to make FormData share code with Blob. It seems to me that FormData should just be a list of strings and file paths (plus range and timestamp). What is the advantage of making the internal representation of a Blob share code with FormData?

(In reply to comment #50) > I'm still somewhat confused by the desire to make FormData share code with > Blob. It seems to me that FormData should just be a list of strings and file > paths (plus range and timestamp). What is the advantage of making the internal > representation of a Blob share code with FormData? I should clarify. What I really mean is that it seems like the structure used by the networking stack (which is currently called WebCore::FormData) should really just be a list of strings and file paths (plus range and timestamp). It should just be the input parameters to the network stack. This is why I have previously recommended renaming that structure to be named HTTPBody. Another good name might be ResourceRequestBody. Also, I agree with the idea of sharing code between the BlobBuilder and FormData web platform interfaces. It makes sense for those to have a common backend. Perhaps they should not be building on top of WebCore::FormData, but instead we should generate one of those when it comes time to submit a Blob or a FormData to the networking stack. (It is confusing to talk about this since the web platform API named FormData is not the same thing as WebCore::FormData.) Are you trying to avoid extra copies between a Blob and the network stack?

(In reply to comment #51) > Are you trying to avoid extra copies between a Blob and the network stack? I think we're trying to avoid the similar internal representation and operations that are needed for BlobBuilder and FormData, i.e., append operations. This is what this patch is about. For copying between FormData/Blob and the network stack, it might be more efficient if we can share the same structure for the element list part, that is, FormDataElement vs. WebHTTPBody::Element. What I mean is that the element list is same and can be shared between these two, while each of them has its own specific members. FormData has "identifier" and Blob has "contentType" and "contentDisposition". We could have another patch for this purpose.

(In reply to comment #52) > (In reply to comment #51) > > Are you trying to avoid extra copies between a Blob and the network stack? > > I think we're trying to avoid the similar internal representation and operations that are needed for BlobBuilder and FormData, i.e., append operations. This is what this patch is about. Hmm, I think we can keep the network stack's FormData as a simple list of data or file (or WebHTTPBody::Element like structure) while utilizing BlobItem as one of the interfaces for passing blob-like data to the network stack. At some point we need to convert opaque items to simple data at the network layer, and on the second thought making the network stack's FormData contain BlobItem may not make much sense.

Created attachment 57831 [details] Patch

(In reply to comment #48) > (From update of attachment 57080 [details]) > WebCore/platform/BlobItem.cpp:32 > + > nit: no new line here. Fixed. > WebCore/platform/BlobItem.h:52 > + class BytesArrayBlobItem; > nit: BytesArrayBlobItem -> ByteArrayBlobItem, the term "byte array" is more common. Fixed. > WebCore/platform/BlobItem.h:45 > + enum EndingType { > nit: LineEnding might be a more natural name for this enum. Fixed. > WebCore/platform/network/FormData.h:46 > + BlobItemWrapper(const BlobItemWrapper& v) : m_item(v.m_item) { } > this copy constructor should be generated by the compiler automatically. Removed. > WebCore/platform/network/FormData.h:60 > + char* releaseBuffer() const > the name "releaseBuffer" typically implies an efficient operation that > does not require the allocation of memory. it implies that memory is > being released from one owner and passed to the caller to own. but this > is calling copyBuffer, which is a copy. i see, however, that this is > just a temporary solution. Added a FIXME comment for now. > WebCore/platform/network/FormData.h:67 > + } m_data; > nit: it is usually better style to do: > struct BlobItemWrapper { > ... > }; > BlobItemWrapper m_data; Fixed. > WebCore/platform/BlobItem.cpp:59 > + if (fileItem) { > it seems like it would be better to define slice for DataBlobItem > and FileBlobItem, and then leave BlobItem::slice pure virtual. > the only code you are sharing is the checks at the top of this > function. Indeed, fixed.

(In reply to comment #53) > Hmm, I think we can keep the network stack's FormData as a simple list of data or file (or WebHTTPBody::Element like structure) while utilizing BlobItem as one of the interfaces for passing blob-like data to the network stack. I also uploaded an alternative patch that tries to do this (return back to the simple FormData). http://codereview.chromium.org/1769002/show

Created attachment 57922 [details] Patch

Comment on attachment 57922 [details] Patch Looks generally good. Just several more comments. WebCore/ChangeLog:9 + FormDataList and makes Blob as a collection of BlobItems. The 2nd part after "and" seems to be redundant. WebCore/ChangeLog:49 + (WebCore::FormDataList::appendString): Please also mention here that the line ending fix logic has been moved to StringBlobItem::convertToCString. WebCore/platform/network/FormData.cpp:217 + const RefPtr<BlobItem> value = items[i + 1]; Might be better to use "const BlobItem*" in order to be consistent with previous line. WebCore/platform/network/FormData.h:23 + #include "BlobItem.h" It seems that we do not need to include BlobItem.h. Instead we can add forward reference to BlobItem and BlobItemList. WebKit/chromium/src/WebSearchableFormData.cpp:201 + if (!item) Is there any case that the current item is not StringBlobItem? I think we rather throw ASSERT here.

Created attachment 57930 [details] Patch

(In reply to comment #58) > (From update of attachment 57922 [details]) > WebCore/ChangeLog:9 > + FormDataList and makes Blob as a collection of BlobItems. > The 2nd part after "and" seems to be redundant. Removed the part. > WebCore/ChangeLog:49 > + (WebCore::FormDataList::appendString): > Please also mention here that the line ending fix logic has been moved to StringBlobItem::convertToCString. Added the comment. > WebCore/platform/network/FormData.cpp:217 > + const RefPtr<BlobItem> value = items[i + 1]; > Might be better to use "const BlobItem*" in order to be consistent with previous line. Fixed. > WebCore/platform/network/FormData.h:23 > + #include "BlobItem.h" > It seems that we do not need to include BlobItem.h. Instead we can add forward reference to BlobItem and BlobItemList. Fixed. > WebKit/chromium/src/WebSearchableFormData.cpp:201 > + if (!item) > Is there any case that the current item is not StringBlobItem? I think we rather throw ASSERT here. I suppose there won't be the case. Changed the if to ASSERT.

Comment on attachment 57930 [details] Patch r=me

Committed r60799: <http://trac.webkit.org/changeset/60799>