[webkit-reviews] review denied: [Bug 35707] Implementing DOMFormData class : [Attachment 49961] Fix style error

[bugzilla-daemon at webkit.org]

Dmitry Titov <dimich at chromium.org> has denied Jian Li <jianli at chromium.org>'s
request for review:
Bug 35707: Implementing DOMFormData class
https://bugs.webkit.org/show_bug.cgi?id=35707

Attachment 49961: Fix style error
https://bugs.webkit.org/attachment.cgi?id=49961&action=review

------- Additional Comments from Dmitry Titov <dimich at chromium.org>
> diff --git a/WebCore/platform/network/FormData.cpp
b/WebCore/platform/network/FormData.cpp

> diff --git a/WebCore/xml/DOMFormData.cpp b/WebCore/xml/DOMFormData.cpp

> +void DOMFormData::append(const String& name, const String& value)
> +{
> +    if (!name.isEmpty())
> +	   m_list.appendData(name, value);
> +}

Why name can not be empty? Either it should be allowed, or we perhaps want to
throw exception for JS to reprot - a simple noop does not look right.

> +
> +void DOMFormData::append(const String& name, Blob* blob)
> +{
> +    if (!name.isEmpty() && !blob->path().isEmpty()) {

Ditto.

> +	   RefPtr<File> file = static_cast<File*>(blob);

I guess this is a place which will change when Blob.slice() is landed. Add //
FIXME ?

> +	   m_list.appendFile(name, file.release());
> +    }
> +}
> +
> +} // namespace WebCore


> diff --git a/WebCore/xml/DOMFormData.h b/WebCore/xml/DOMFormData.h

Why is it in WebCore/xml? It does not appear to haveanything to do with xml,
except that it can be sent by XHR.
Perhaps a better place for it is WebCore/page.

> +class DOMFormData : public RefCounted<DOMFormData> {
> +public:
> +    static PassRefPtr<DOMFormData> create()
> +    {
> +	   return adoptRef(new DOMFormData());
> +    }

Would be nice to have an empty line here. Also, often these functions are
written in a single line.

> +    void append(const String& name, Blob* blob);

'blob' should be omitted.

> +
> +    const FormDataList& list() const { return m_list; }

Consider: deriving DOMFormData from FormDataList, because DOMFormData "is" a
list, essentially. It can not replace a list for example.
Also, it would provide for simpler domFormData.list().size() kind of calls
rather then ugly domFormData.list().list().size().
Also, then you don't need a create(FormDataList) and a copy constructor on
FormDataList.

r- but it is very close.