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

Fri May 21 07:55:09 PDT 2010

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

--- Comment #30 from Kinuko Yasuda <kinuko at chromium.org>  2010-05-21 07:55:07 PST ---
(In reply to comment #26)
> (From update of attachment 56610 [details])
> WebCore/ChangeLog:8
>  +          - Introduces a new class BlobItem (platform/BlobData.{h,cpp}) as
> Should it be "platform/BlobItem.{h,cpp}"? Perhaps we do not even need to mention the file names.

I removed the (wrong) file names.

> WebCore/ChangeLog:22
>  +          The existing tests should be able to be used for regression:
> There're quite a few more tests to test file and form data. Probably we do not need to mention the test names.

Removed the test names too.

> WebCore/html/Blob.h:50
>  +      static PassRefPtr<Blob> create(const String& path)
> Seems that we do not need this creator any more.

Really?  There still seem to be a line calling Blob::create(path) in WebCore/bindings/v8/SerializedScriptValue.cpp.  We'll need to fix the usage once we introduce BlobBuilderf but I don't want to put too many changes in this patch.
I added 'FIXME: deprecated' comment to the function.

> WebCore/html/Blob.h:58
>  +      const String& path() const;
> Is this still needed now? I do not see any reference to path() now since we access the underlying items directly.

This is called in FileStream.cpp and again in v8/SerializedScriptValue.cpp.
I added the 'FIXME: deprecated' comment here too.

> WebCore/html/Blob.h:62
>  +      void append(PassRefPtr<BlobItem> item);
> item name can be omitted.

Done.

> WebCore/html/Blob.h:73
>  +      BlobItemList m_itemList;
> Probably it is simpler to call the name as BlobItems and m_items. Not only for the simpler names, but also consistent with items() getter.

I renamed m_itemList (and other similar variable names) to m_items.  As for the type name, doesn't BlobItems sound too similar to BlobItem?

> WebCore/html/Blob.cpp:40
>  +  Blob::Blob(const String& path)
> I think we do not need this constructor. Instead, we can move the append operation to the File constructor.

Again this is left only for Blob::create(path).  I added a FIXME comment.

> WebCore/html/FileStream.cpp:93
>  +      const FileRangeBlobItem* range = blob->items().at(0)->toFileRangeBlobItem();
> Please add an assert to ensure the blob has only one item. Also a FIXME about needing to handle multiple items when BlobBuilder is introduced.

Done.

> WebCore/html/FormDataList.h:49
>  +      void appendBlob(const String& key, PassRefPtr<Blob> blob);
> blob can be omitted.

Done.

> WebCore/platform/BlobItem.cpp:63
>  +              start += fileRangeItem->start();
> What if the new start value is beyond the old range? It seems that we're only doing partial checks and clamps in BlobItem::slice, which is quite risky. Why not moving the clamp logic in Blob::slice to here?

Blob::slice() shouldn't call BlobItem::slice() while start is larger than the item's size(), that is the item's valid range.  Also it won't make sense if BlobItem::slice() is called with the start > size().  We can return an empty BlobItem (say BytesArrayBlobItem) but it'll only confuse the whole logic assume BlobItem is only used as a collection of items.

I added an assert ASSERT(start < size()) in BlobItem::slice(), made BlobItem::slice() private and registered Blob as a friend class of BlobItem.
(Rather I thought we may want to put all the logic in BlobItem::slice() into Blob::slice(), but what BlobItem::slice() is doing is deeply related to other BlobItem code.)

> WebCore/platform/BlobItem.cpp:82
>  +      return v.releaseBuffer();
> I think this would incur extra overhead for ByteArrayBlobItem that already uses Vector<char> to hold the data.

Vector::releaseBuffer makes the vector's buffer empty by passing its buffer ownership to the caller.  BlobItem is ref-counted and should keep its data as far as its ref is >0, so it needs to make a copy.  I added a comment here.

> WebCore/platform/BlobItem.cpp:122
>  +      if (!fileItem || m_path != fileItem->m_path)
> It is simpler to say:
>     return fileItem && m_path == fileItem->m_path;

Done.

> WebCore/platform/BlobItem.cpp:140
>  +  StringBlobItem::StringBlobItem(const String& text, EndingType ending, TextEncoding encoding)
> The constructor body seems to be too cumbersome. Can we move the ending logic to a helper function?

Done.

> WebCore/platform/BlobItem.cpp:243
>  +      : m_item(item)
> For better readability, can we move setting m_item and m_start values to be else part of the if check in the constructor body?

Done.

> WebCore/platform/BlobItem.cpp:248
>  +          const DataRangeBlobItem* rangeItem = m_item->toDataRangeBlobItem();
> This line can be moved before the above if statement.

Done.

> WebCore/platform/BlobItem.cpp:273
>  +      if (!m_item->equals(rangeItem->m_item.get()))
> It would be simpler to combine all 3 if checks to one return.

Done.

> WebCore/platform/network/FormData.cpp:190
>  +  void FormData::appendFormDataItems(const BlobItemList& list, const TextEncoding& encoding, bool isMultiPartForm, Document* document)
> The name appendFormDataItems is a bit confusing. How about appendItems?

Done.

> WebCore/platform/network/FormData.h:133
>  +      void appendFormDataItems(const BlobItemList& formDataItems, const TextEncoding&, bool isMultiPartForm, Document* document);
> We can omit the names of 1st and last arguments.

Done.

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