[webkit-reviews] review denied: [Bug 39083] Refactor FormData and Blob for better support of Blobs synthesized by BlobBuilder : [Attachment 56610] 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 56610: Patch
https://bugs.webkit.org/attachment.cgi?id=56610&action=review

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