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

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