[webkit-reviews] review denied: [Bug 26979] Send form data via XMLHttpRequest : [Attachment 49761] Proposed Patch

[bugzilla-daemon at webkit.org]

Dmitry Titov <dimich at chromium.org> has denied Jian Li <jianli at chromium.org>'s
request for review:
Bug 26979: Send form data via XMLHttpRequest
https://bugs.webkit.org/show_bug.cgi?id=26979

Attachment 49761: Proposed Patch
https://bugs.webkit.org/attachment.cgi?id=49761&action=review

------- Additional Comments from Dmitry Titov <dimich at chromium.org>
Woohoo! FormData!

I would recommend to split the patch: 
1. implementation of DOMFormData (all internals, no tests yet)
2. IDL + JSC Bindings + tests
3. V8 Bindings.
#2 and #3 can be landed together but they probably will be reviewed by
different people.

Also:

> diff --git a/WebCore/platform/network/FormData.cpp
b/WebCore/platform/network/FormData.cpp
> +void FormData::appendFormDataListAsMultiPartForm(const FormDataList& list,
FormDataBuilder& formDataBuilder, const TextEncoding& encoding, const CString&
boundary, Document* document) {

Looking at the name and parameters of this method I think we should refactor
this code to make things simpler.

I see what you are doing but I think it can be even cleaner. Since we'll soon
need a method to create a DOMFormData from a From element (for "new
FormData(nyFormElement)"), it may be better to have a method on HTMLFormElement
like: "PassRefPtr<DOMFormData> createDOMFormData()".

Then on FormData, it'd be FormData::create(const DOMFormData&), instead of
append...() method. The appending of all the items could be done inside that
create() method.

This could avoid passing FormBuilder for example - it can be a local variable
of FormData::create. 

Also:
It's possible to do this:
var formData = new FormData(myFormElement);
formData.append("another_file", file);
xhr.send(formData);

I realize this patch does not yet address this, but please consider how this
would affect the shape of this code, to hopefully avoid significant changes in
near future...