[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 #18 from Jian Li <jianli at chromium.org>  2010-05-18 17:35:08 PST ---
FYI, here are some comments for your last patch:

You might need to add the new file BlobItem.cpp to the new make file: CMakeLists.txt.

In addition, I think BlobItem could be put under platform directory so that it can be referred to by both html/Blob and platform/network/FormData.

WebCore/bindings/v8/SerializedScriptValue.cpp:562
 +          // for non-file blobs.
No need to fold into 2 lines.

WebCore/html/Blob.h:50
 +      static PassRefPtr<Blob> createFromFile(const String& path, const String type = String())
Should it be "const String type& = ..."?

WebCore/html/BlobItem.cpp:68
 +  static const double invalidModificationTime = -1;
Why do we use -1, instead of 0?

WebCore/html/BlobItem.h:74
 +      static PassRefPtr<BlobItem> createFileRangeBlobItem(const String& path, long long start, long long length, double snapshotModificationTime);
It would be better to return "PassRefPtr<FileRangeBlobItem>".

WebCore/html/BlobItem.h:61
 +      virtual const char* data() const { return 0; }
data() should only make sense for StringBlobItem, BytesArrayBlobItem and DataRangeBlobItem. It is a little bit wield to put it in the base class. Can we move it to the subclass? Since we do not want to add it for each of these subclasses, how about restructure BlobItem classes like the following:
     BlobItem
        |
        --------> DataBlobItem
        |              |
        |              ----------> StringBlobItem
        |              ----------> ByteArrayBlobItem
        |              ----------> DataRangeBlobItem
        |
        --------> FileBlobItem
        |              |
        |              ----------> FileRangeBlobItem

We can then add data() and releaseBuffer() to DataBlobItem.

WebCore/html/File.cpp: 
 +      // We don't use MIMETypeRegistry::getMIMETypeForPath() because it returns "application/octet-stream" upon failure.
Can we keep the comment to help people better understand why we call MIMETypeRegistry::getMIMETypeForExtension?

WebCore/platform/network/FormData.h:23
 +  #include "Blob.h"
As Darin pointed out, FormData is under platform/network so it should not depend on other non-platform stuff. We could move BlobItem to platform.

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