[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 #56610|review?                     |review-
               Flag|                            |




--- Comment #26 from Jian Li <jianli at chromium.org>  2010-05-20 17:35:58 PST ---
(From update of attachment 56610)
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.

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.

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

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.

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

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.

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.

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.

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

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?

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.
Probably it is better to move the implementation of releaseBuffer to each subclass. You might also choose to override releaseBuffer in ByteArrayBlobItem.

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

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?

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?

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

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.

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?

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.

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