[webkit-reviews] review denied: [Bug 39083] Refactor FormData and Blob for better support of Blobs synthesized by BlobBuilder : [Attachment 56861] Patch (fixed cr-linux build)

[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 56861: Patch (fixed cr-linux build)
https://bugs.webkit.org/attachment.cgi?id=56861&action=review

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