[Webkit-unassigned] [Bug 39083] Refactor FormData and Blob for better support of Blobs synthesized by BlobBuilder
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Tue May 25 09:10:21 PDT 2010
https://bugs.webkit.org/show_bug.cgi?id=39083
--- Comment #40 from Kinuko Yasuda <kinuko at chromium.org> 2010-05-25 09:10:17 PST ---
(In reply to comment #36)
> WebCore/html/Blob.h:36
> + #include "TextEncoding.h"
> This include seems not to be needed.
Removed.
> 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.
Fixed.
> WebCore/html/File.cpp:38
> + int index = name().reverseFind('.');
> Might be better to cache name() first, like:
> const String& name = name();
Fixed.
> WebCore/html/FileStream.cpp:75
> + // FIXME: need to handle multiple items that may include non-file ones
> No need to fold into 2 lines.
Fixed.
> 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.
Makes sense, fixed.
> WebCore/html/FileStream.cpp:98
> + const FileRangeBlobItem* range = file->toFileRangeBlobItem();
> Better to name it as fileRangItem.
Fixed.
> WebCore/html/FormDataList.h:49
> + void appendBlob(const String& key, PassRefPtr<Blob> blob);
> The parameter name 'blob' can be omitted.
Fixed.
> 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);
Looks better, fixed.
> 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?
Right. Did both - renamed the method and added FIXME comment.
> 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.
Fixed.
> 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.
Removed the comment.
> WebCore/platform/network/FormData.cpp:49
> + const FileRangeBlobItem* range = m_item->toFileRangeBlobItem();
> Better to name range as fileRangeItem.
Fixed.
> WebKit/chromium/ChangeLog:8
> + Replace FormDataList::Item list with BlobItemList to get it compiling
> compiling => compiled?
Fixed.
--
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.
More information about the webkit-unassigned
mailing list