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

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