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




--- Comment #6 from Jian Li <jianli at chromium.org>  2010-05-13 18:02:03 PST ---
(From update of attachment 56034)
Note that the line number might be somewhat off since what I put here is based on your first patch.

WebCore/html/Blob.cpp:43
 +      return adoptRef(static_cast<Blob*>(File::create(path).get()));
I do not understand this. When do we invoke File::create from Blob::create?

WebCore/html/Blob.cpp:80
 +      } else if (static_cast<unsigned long long>(start + length) > totalSize)
What if start < 0?

WebCore/html/Blob.h:49
 +      // A special constructor for File blob.
Any one is calling this creator? I do not see any usage for this. Also, this creator is very confused with the protected constructor with type string as argument because the creator has path string as argument.

WebCore/html/Blob.h:59
 +      typedef Vector<RefPtr<BlobItem> > ItemList;
Could you please add include for Vector.h?

WebCore/html/BlobItem.cpp:333
 +      String m_uniqName;
Should it be m_uniqueName?

WebCore/html/BlobItem.h:54
 +      virtual ~BlobItem() { }
Why not making it protected?

WebCore/html/BlobItem.h:66
 +      static PassRefPtr<BlobItem> createFileRangeItem(const String& path, long long start, long long length, double snapshotModificationTime);
Can we merge these two if there're very similar? If not, it might be better to name RangeBlobItem as DataRangeItem to avoid the confusion since BlobRangeItem sounds more like a super set?

WebCore/html/BlobItem.h:73
 +      virtual const CString cstr() const { return CString(); }
Is it possible to move this method to StringBlobItem so that we have cleaner BlobItem interface? Doing so will make us move all the derived classes to the header file, but I think it will make the readability better?

WebCore/html/BlobItem.h:76
 +      virtual const String filePath() const { return String(); }
ditto.

WebCore/html/BlobItem.h:77
 +      virtual const String fileOrUniqueName() const { return String(); }
ditto. In addition, probably uniqueName() should be enough.

WebCore/html/BlobItem.h:80
 +      bool isFile() const { return !filePath().isEmpty(); }
ditto.

WebCore/html/BlobItem.h:84
 +      virtual bool isRange() const { return false; }
ditto.

WebCore/html/BlobItem.cpp:129
 +  void StringBlobItem::ensureFinalCString() const
What does this function do? Could you please add a comment?

WebCore/html/File.cpp:46
 +      ASSERT(items().size() == 1 && items().at(0)->isFile());
Do we need this ASSERT? The above code will unlikely fail.

WebCore/html/File.cpp:51
 +      ASSERT(items().size() == 1 && items().at(0)->isFile());
Do we need this ASSERT?

WebCore/html/FileStream.cpp:84
 +      if (!getFileModificationTime(file->filePath(), currentModificationTime)) {
Why not simply calling blob->path() since it already contains the same logic? You can cache the path if needed.

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