[webkit-reviews] review denied: [Bug 39083] Refactor FormData and Blob for better support of Blobs synthesized by BlobBuilder : [Attachment 56524] Patch

[bugzilla-daemon at webkit.org]

Jian Li <jianli at chromium.org> has denied Kinuko Yasuda <kinuko at chromium.org>'s
request for review:
Bug 39083: Refactor FormData and Blob for better support of Blobs synthesized
by BlobBuilder
https://bugs.webkit.org/show_bug.cgi?id=39083

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

------- Additional Comments from Jian Li <jianli at chromium.org>
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".