[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 #16 from Kinuko Yasuda <kinuko at chromium.org>  2010-05-17 19:15:29 PST ---
Thanks for reviewing.

(In reply to comment #10)
> (From update of attachment 56085 [details])
> 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.

Agreed, I took the former option, i.e. changed the method names to {to,create}XxxBlobItem().

> 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?

Right.  I'm willing to refactor FormDataList and FormData code further.  I thought changing everything in a patch would be a bit scary.  (Or I can extend this patch if it would be better.)

> 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.

Reverted some changes (calling File methods for read/writeBlob) and added more details to the FIXME comment.

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

Fixed.

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

Fixed.

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

Fixed.

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

Fixed.

> 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.

Fixed.

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

Fixed.

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

Fixed.

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

Fixed.

> 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.

Right.  Instead removed the BlobItem::equals implementation at all.

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

Added another ASSERT for length.

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

Fixed.

> 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?

For FileRangeBlobItem item->size() returns the length of the sliced range, so it should be ok.

> 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.

Fixed.

> 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.

Fixed.

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