[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





--- Comment #25 from Kinuko Yasuda <kinuko at chromium.org>  2010-05-20 11:37:30 PST ---
(In reply to comment #22)
> (From update of attachment 56524 [details])
> WebCore/platform/BlobItem.h:69
>  +  //       |              |
> Nice diagram. Probably better to remove the 1st "|".

Fixed.

> 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?

Done.
I was kind of separated on this since they return BlobItem, but now we have subclass definitions in the header file and probably it would be better to do also for readability.
I changed the createRangeBlob to a member method slice.

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

Fixed.

> 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?

It might be, but almost all of their methods are const, and Blobs are expected to be immutable... so I'd like to keep them const until it becomes a real problem.  Does that make sense to you?

> 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?

It ought to be if we want to make the interface really strict, but I'm afraid it may make code a bit too complicated.  If we do that we'd need to call (toDataBlobItem() && toDataBlobItem()->toStringBlobItem()) every time to access its string data.  Do you think we want to do that?

> 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.

Fixed.

> 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.

That's true.  Can you help me make a few things clear?
Do we want to use the same name for the same sliced blobs?  If so I think we'd better store it in Blob/BlobItem but otherwise I should rather leave it in FormData::append.
Also: will we have different usages for fileName and UUID-based uniqueName?  If that applies I'll split into two different methods (e.g. FileBlobItem::fileName(0 and FileRangeBlobItem::uniqueName()) rather than one.

(In this patchset I just changed the method name to name(), but I don't have strong opinion on it.)

> WebCore/platform/BlobItem.h:158
>  +      virtual unsigned long long size() const { return m_bytesArray.size(); }

Fixed.

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

Fixed.

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

Done.

> 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.

Probably by a mistake - thanks for catching it.

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

Fixed.  Again thanks for catching it.

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