[Webkit-unassigned] [Bug 39083] Refactor FormData and Blob for better support of Blobs synthesized by BlobBuilder

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=39083


Jian Li <jianli at chromium.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #56524|review?                     |review-
               Flag|                            |




--- Comment #22 from Jian Li <jianli at chromium.org>  2010-05-19 18:22:54 PST ---
(From update of attachment 56524)
Looks much better now. Some more comments.

WebCore/platform/BlobItem.h:69
 +  //       |              |
Nice diagram. Probably better to remove the 1st "|".

WebCore/platform/BlobItem.h:70
 +  //       |              +---------> FileRangeBlobItem
ditto.

WebCore/platform/BlobItem.h:78
 +      static PassRefPtr<BlobItem> createStringBlobItem(const String&, EndingType, TextEncoding);
I am thinking if we really need these factory methods. Can we simply call something like StringBlobItem::create() in order to make the whole logic simpler?
The only different creator is createRangeBlobItem. Can we make it as slice?

WebCore/platform/BlobItem.h:85
 +      static PassRefPtr<BlobItem> createRangeBlobItem(PassRefPtr<BlobItem> item, long long start, long long length);
item can be omitted.

WebCore/platform/BlobItem.h:93
 +      virtual const DataBlobItem* toDataBlobItem() const { return 0; }
Do we need to add const modifier? Would "virtual DataBlobItem* toDataBlobItem() { return 0; }" be more straightforward?

WebCore/platform/BlobItem.h:94
 +      virtual const StringBlobItem* toStringBlobItem() const { return 0; }
Would it be better to move toStringBlobItem to DataBlobItem? Do we want to also add toByteRangeBlobItem?

WebCore/platform/BlobItem.h:115
 +      virtual const DataBlobItem* toDataBlobItem() const { return this; }
Better to add an empty line and a line for "// BlobItem methods." before this line, like what you do for other subclass.

WebCore/platform/BlobItem.h:122
 +      virtual const String& uniqueName() const { return m_fileName; }
I do not think file name is a unique name. We should either move it to FileRangeBlobItem or leave it in FormData::append.

WebCore/platform/BlobItem.h:158
 +      virtual unsigned long long size() const { return m_bytesArray.size(); }
Better to add an empty line and a line for "// BlobItem methods." before this line.

WebCore/platform/BlobItem.h:173
 +      DataRangeBlobItem(PassRefPtr<BlobItem> item, long long start, long long length);
item can be omitted.

WebCore/platform/BlobItem.h:185
 +      RefPtr<BlobItem> m_item;
I think m_item should be RefPtr<DataBlobItem>.

WebCore/html/FormDataList.h:25
 +  #include "File.h"
Why changing from including Blob.h to File.h? I do not see any reference to File type.

WebKit/chromium/ChangeLog:8
 +          Replace FileItemList::Item list with BlobItemList to get it compiling
I think you mean "FormDataList::Item".

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.