Bug 35707 – Implementing DOMFormData class

• Component: WebCore JavaScript
• Status: RESOLVED FIXED

[Jian Li]

This bug only covers the implementation of DOMFormData class for review purpose. The exposing of IDL interface and adding JSC bindings plus layout test will be in separate bug.

[Jian Li]

Created attachment 49960 [details]
Proposed Patch

[WebKit Review Bot]

Attachment 49960 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/html/FormDataList.cpp:36:  One line control clauses should not use braces.  [whitespace/braces] [4]
Total errors found: 1 in 17 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Jian Li]

Created attachment 49961 [details]
Fix style error

[Dmitry Titov]

Comment on attachment 49961 [details]
Fix style error

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

[Jian Li]

Created attachment 50074 [details]
Proposed Patch

[Jian Li]

(In reply to comment #4)
> (From update of attachment 49961 [details])
> > 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.
> 
This is for consistency with current form behavior: the element with empty name, except image, will not be submitted. I will ping the spec writer to check if we want to throw an exception.
> > +
> > +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.

[Sam Weinig]

DOMFormData should probably go in WebCore/html like File and FileList.

[Jian Li]

Created attachment 50249 [details]
Move DOMFormData to html/

[Dmitry Titov]

Comment on attachment 50249 [details]
Move DOMFormData to html/

r=me, with a coupel of nits that can be fixed on landing:

> diff --git a/WebCore/html/DOMFormData.cpp b/WebCore/html/DOMFormData.cpp
> +DOMFormData::DOMFormData(const TextEncoding& encoding) : FormDataList(encoding)

": FormDataList(encoding)" should be on a separate line

> diff --git a/WebCore/html/HTMLFormElement.h b/WebCore/html/HTMLFormElement.h

> +class DOMFormData;

Seems this declaration is not needed.

> diff --git a/WebCore/platform/network/FormData.h b/WebCore/platform/network/FormData.h
> +    static PassRefPtr<FormData> create(const DOMFormData&, bool isMultiPartForm, Document*);

It'd be better for readability if instead of 'bool' this parameter was an enum, something like:
enum  { CreateRegularForm, CreateMultiPartForm };

[Darin Adler]

(In reply to comment #9)
> > +    static PassRefPtr<FormData> create(const DOMFormData&, bool isMultiPartForm, Document*);
> 
> It'd be better for readability if instead of 'bool' this parameter was an enum,
> something like:
> enum  { CreateRegularForm, CreateMultiPartForm };

Two separate creation functions is also a good option in a case like this.

[Jian Li]

Committed as http://trac.webkit.org/changeset/55821.