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




--- Comment #10 from Jian Li <jianli at chromium.org>  2010-05-17 10:36:08 PST ---
(From update of attachment 56085)
I think it might be better to rename BlobItem::createFileItem to BlobItem::createFileBlobItem, and BlobItem::toFileItem to BlobItem::toFileBlobItem, for better understanding. Otherwise, when I see toFileItem, I have to realize that it is to convert to FileBlobItem.
The other fix, while keeping createFileItem and asFileItem, is to move all BlobItem stuffs to stay inside the class Blob and then we can remove all Blob pattern from the class names. But this will increase the complexity of Blob.h.

FormDataList and FormData seem not to be fully refactored. You still try to keep almost all the old data members for backward compatibility. Do you have plan to refactor these codes to make them better abstracted based on the new BlobItem structures?


WebCore/bindings/v8/SerializedScriptValue.cpp:562
 +          m_writer.writeFile(blob->path());
Why do we need to change this? If we do change it as a temporary hack, please put more detail for FIXME.

WebCore/bindings/v8/SerializedScriptValue.cpp:858
 +          PassRefPtr<File> blob = File::create(path);
ditto.

WebCore/html/Blob.h:58
 +      void appendBlobItem(PassRefPtr<BlobItem> item);
IMO, append or appendItem sounds more natural.

WebCore/html/BlobItem.h:54
 +  class FileRangeBlobItem;
Please sort the above forward declarations.

WebCore/html/BlobItem.h:65
 +      static PassRefPtr<BlobItem> createStringItem(const String& text, EndingType, TextEncoding);
I think we can omit the argument name "text".

WebCore/html/BlobItem.h:66
 +      static PassRefPtr<BlobItem> createStringItem(const CString& text);
ditto.

WebCore/html/BlobItem.h:94
 +  // BlobItem class for a string.
We probably don't need comment here since the class name is straightforward. This also applies to other subclasses.

WebCore/html/BlobItem.h:95
 +  class StringBlobItem: public BlobItem {
Please add one space before ':'. This also applies to other subclasses.

WebCore/html/BlobItem.h:97
 +      StringBlobItem(const String& text, EndingType, TextEncoding);
I think we can omit text here.

WebCore/html/BlobItem.h:98
 +      StringBlobItem(const CString& text);
ditto.

WebCore/html/BlobItem.cpp:42
 +      if (size() != b->size() || data() != b->data() || isValid() != b->isValid())
I think we should only check isValid() here. For example, for FileBlobItem, we do not want to get and check the size.

WebCore/html/BlobItem.cpp:88
 +      ASSERT(start >= 0);
Do we also need to ASSERT on length?

WebCore/html/BlobItem.cpp:91
 +      if (item->toFileItem()) {
Better to say:
    FileBlobItem* fileItem = item->toFileItem();
    if (fileItem)

WebCore/html/BlobItem.cpp:95
 +              if (static_cast<unsigned long long>(start + length) > item->size())
 +                  length = item->size() - start;
Should the above two lines be moved after adjusting start?

WebCore/html/BlobItem.cpp:141
 +      if (m_path != f->m_path || m_fileName != f->m_fileName)
I think checking the path should be enough. In addition, it would be simpler to combine the last if check with this if check.

WebCore/html/BlobItem.cpp:157
 +          : (ending == EndingCR) ? "\r" : "\n";
No need to fold into 2 lines. In addition, please use parentheses to wrap the 2nd conditional express for better readability and safety.

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