546528 - Implement FormData

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

This will make it easier to submit multipart/form-data encoded content using XHR. And will allow such submissions to include File objects, without having to read the file contents into memory.

Comment 1

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: Part1: Fixes to XMLHttpRequest.send

This is a patch that I've been wanting to write for a while. Currently nsXMLHttpRequest::Send is very hard to read because it simply does a lot of things.

One big code chunk in particular is the code to convert the passed in parameter to an nsIInputStream that we can then pass to necko.

Breaking out this code into a separate function not only reduces the size of nsXMLHttpRequest::Send, but it also allows us to use early returns instead of ever more nesting if statements.

No functional changes in this patch.

Comment 2

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: Part2: Introduce nsEncodingFormSubmission

This patch moves some of the functionality in nsFormSubmission into a new nsEncodingFormSubmission subclass.

This will allow the next patch to create classes that inherit nsFormSubmission directly, without also getting the functionality that is in nsEncodingFormSubmission.

No functional changes in this patch either.

Comment 3

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: Part3: Implement FormData

This patch finally contains the actual FormData implementation as well as tests for said implementation.

The patch contains the following changes:
* Move the nsFSMultipartFormData class declaration from the .cpp file to the .h
  file to allow nsFormData to instantiate the class directly
* Add two functions to nsFSMultipartFormData to allow getting the content type
  and request body separately so that it can be passed on to XMLHttpRequest
* Introduce nsIXHRSendable interface to avoid having to modify the XHR code
  every time we want to add support for sending more types of objects using XHR.
* nsFormData class implementing FormData object

Comment 4

[Ben Newman (:bnewman) (:benjamn)]

Comment on attachment 427837 [details] [diff] [review]
Part2: Introduce nsEncodingFormSubmission

Looks good.

This comment is more for my education than it is a complaint: why did you move the MOZ_COUNT_* macros into the nsEncodingFormSubmission constructor/destructor, instead of leaving them in the nsFormSubmission ctor/dtor?

Comment 5

[Ben Newman (:bnewman) (:benjamn)]

Comment on attachment 427832 [details] [diff] [review]
Part1: Fixes to XMLHttpRequest.send

r=me with some super-superficial nitpickings regarding nsCOMPtr initialization and indentation:

diff --git a/content/base/src/nsXMLHttpRequest.cpp b/content/base/src/nsXMLHttpRequest.cpp
--- a/content/base/src/nsXMLHttpRequest.cpp
+++ b/content/base/src/nsXMLHttpRequest.cpp
@@ -2221,9 +2221,9 @@ GetRequestBody(nsIVariant* aBody, nsIInp
 
     nsMemory::Free(iid);
 
     // document?
-    nsCOMPtr<nsIDOMDocument> doc = do_QueryInterface(supports);
+    nsCOMPtr<nsIDOMDocument> doc(do_QueryInterface(supports));
     if (doc) {
       aContentType.AssignLiteral("application/xml");
       nsCOMPtr<nsIDOM3Document> dom3doc(do_QueryInterface(doc));
       if (dom3doc) {
@@ -2235,10 +2235,10 @@ GetRequestBody(nsIVariant* aBody, nsIInp
       }
 
       // Serialize to a stream so that the encoding used will
       // match the document's.
-      nsCOMPtr<nsIDOMSerializer> serializer =
-        do_CreateInstance(NS_XMLSERIALIZER_CONTRACTID, &rv);
+      nsCOMPtr<nsIDOMSerializer>
+        serializer(do_CreateInstance(NS_XMLSERIALIZER_CONTRACTID, &rv));
       NS_ENSURE_SUCCESS(rv, rv);
 
       nsCOMPtr<nsIStorageStream> storStream;
       rv = NS_NewStorageStream(4096, PR_UINT32_MAX,
@@ -2268,33 +2268,33 @@ GetRequestBody(nsIVariant* aBody, nsIInp
                                       NS_ConvertUTF16toUTF8(string));
     }
 
     // nsIInputStream?
-    nsCOMPtr<nsIInputStream> stream = do_QueryInterface(supports);
+    nsCOMPtr<nsIInputStream> stream(do_QueryInterface(supports));
     if (stream) {
       *aResult = stream.forget().get();
       aCharset.Truncate();
 
       return NS_OK;
     }
 
     // nsIDOMFile?
-    nsCOMPtr<nsIDOMFileInternal> file = do_QueryInterface(supports);
+    nsCOMPtr<nsIDOMFileInternal> file(do_QueryInterface(supports));
     if (file) {
       aCharset.Truncate();
 
       nsCOMPtr<nsIFile> internalFile;
       rv = file->GetInternalFile(getter_AddRefs(internalFile));
       NS_ENSURE_SUCCESS(rv, rv);
 
       // Get the mimetype
-      nsCOMPtr<nsIMIMEService> mimeService =
-          do_GetService(NS_MIMESERVICE_CONTRACTID, &rv);
+      nsCOMPtr<nsIMIMEService>
+        mimeService(do_GetService(NS_MIMESERVICE_CONTRACTID, &rv));
       NS_ENSURE_SUCCESS(rv, rv);
 
       rv = mimeService->GetTypeFromFile(internalFile, aContentType);
       if (NS_FAILED(rv)) {
-         aContentType.Truncate();
+        aContentType.Truncate();
       }
 
       // Feed local file input stream into our upload channel
       return NS_NewLocalFileInputStream(aResult, internalFile);

Comment 6

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

(In reply to comment #4)
> (From update of attachment 427837 [details] [diff] [review])
> Looks good.
> 
> This comment is more for my education than it is a complaint: why did you move
> the MOZ_COUNT_* macros into the nsEncodingFormSubmission
> constructor/destructor, instead of leaving them in the nsFormSubmission
> ctor/dtor?

Good catch, this was an unintentional sideeffect of the way the location of the ctor moved. I'll move the macro to the nsFormSubmission ctor/dtor again.

(In reply to comment #5)
> (From update of attachment 427832 [details] [diff] [review])
> r=me with some super-superficial nitpickings regarding nsCOMPtr initialization
> and indentation:
> 
> diff --git a/content/base/src/nsXMLHttpRequest.cpp
> b/content/base/src/nsXMLHttpRequest.cpp
> --- a/content/base/src/nsXMLHttpRequest.cpp
> +++ b/content/base/src/nsXMLHttpRequest.cpp
> @@ -2221,9 +2221,9 @@ GetRequestBody(nsIVariant* aBody, nsIInp
> 
>      nsMemory::Free(iid);
> 
>      // document?
> -    nsCOMPtr<nsIDOMDocument> doc = do_QueryInterface(supports);
> +    nsCOMPtr<nsIDOMDocument> doc(do_QueryInterface(supports));

Why? The former is more more readable to me. Note that they by definition mean the same thing. I.e. both call the same ctor and neither calls the assignment operator.

Comment 7

[Ben Newman (:bnewman) (:benjamn)]

(In reply to comment #6)
> Why? The former is more more readable to me. Note that they by definition mean
> the same thing. I.e. both call the same ctor and neither calls the assignment
> operator.

I really don't prefer one style over the other, but both styles are used in GetRequestBody; for instance,

  nsCOMPtr<nsISupportsString> wstr(do_QueryInterface(supports));

uses the constructor-argument style.  I know this variation was inherited from the old code, but, as long as you're cleaning it up, why not pick one style and stick to it?

Comment 8

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Ah, that was my intent, but it appears I missed a few spots. I'll fix the remaining places to use the = syntax.

Comment 9

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

In order to apply the patches in this bug, first land the patches in bug 547165 and bug 544698 (in that order)

Comment 10

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Actually, it's bug 547165, bug 544698 and bug 545059. In that order.

Comment 11

[Ben Newman (:bnewman) (:benjamn)]

Comment on attachment 427839 [details] [diff] [review]
Part3: Implement FormData

>+NS_IMETHODIMP
>+nsFormData::Append(const nsAString& aName, nsIVariant* aValue)
>+{
>+  PRUint16 dataType;
>+  nsresult rv = aValue->GetDataType(&dataType);
>+  NS_ENSURE_SUCCESS(rv, rv);
>+
>+  if (dataType == nsIDataType::VTYPE_INTERFACE ||
>+      dataType == nsIDataType::VTYPE_INTERFACE_IS) {
>+    nsCOMPtr<nsISupports> supports;
>+    nsID *iid;
>+    rv = aValue->GetAsInterface(&iid, getter_AddRefs(supports));
>+    NS_ENSURE_SUCCESS(rv, rv);
>+
>+    nsMemory::Free(iid);
>+
>+    nsCOMPtr<nsIDOMFileInternal> domFile = do_QueryInterface(supports);
>+    if (domFile) {
>+      nsCOMPtr<nsIFile> file;
>+      rv = domFile->GetInternalFile(getter_AddRefs(file));
>+      NS_ENSURE_SUCCESS(rv, rv);
>+
>+      return AddNameFilePair(aName, file);
>+    }
>+  }
>+
>+  PRUnichar* stringData = nsnull;
>+  PRUint32 stringLen = 0;
>+  rv = aValue->GetAsWStringWithSize(&stringLen, &stringData);
>+  NS_ENSURE_SUCCESS(rv, rv);
>+
>+  nsString valAsString;
>+  valAsString.Adopt(stringData, stringLen);
>+
>+  return AddNameValuePair(aName, valAsString);
>+}
>+
>+// -------------------------------------------------------------------------
>+// nsIDXHRSendable
>+
>+NS_IMETHODIMP
>+nsFormData::GetSendInfo(nsIInputStream** aBody, nsACString& aContentType,
>+                        nsACString& aCharset)
>+{
>+  nsFSMultipartFormData fs(NS_LITERAL_CSTRING("UTF-8"));
>+  
>+  for (PRUint32 i = 0; i < mFormData.Length(); ++i) {
>+    if (mFormData[i].valueIsFile) {
>+      fs.AddNameFilePair(mFormData[i].name, mFormData[i].fileValue);
>+    }
>+    else {
>+      fs.AddNameValuePair(mFormData[i].name, mFormData[i].stringValue);
>+    }
>+  }
>+
>+  fs.GetContentType(aContentType);
>+  aCharset.Truncate();
>+  NS_ADDREF(*aBody = fs.GetSubmissionBody());
>+
>+  return NS_OK;
>+}

Since the only way to get the data back out of an nsFormData is to call GetSendInfo, what do you think about moving the nsIVariant-to-* conversion code from Append to GetSendInfo?

Then, it seems, you could just store an nsCOMPtr<nsIVariant> in the FormDataTuple, instead of requiring space for both a string and a nsCOMPtr<nsIFile> for every tuple (while only one of those fields is ever used).  You could also get rid of the PRBool, but I assume the nsIVariant has a similar type-information field, so using an nsIVariant wouldn't save that memory.

>+  struct FormDataTuple
>+  {
>+    nsString name;
>+    nsString stringValue;
>+    nsCOMPtr<nsIFile> fileValue;
>+    PRBool valueIsFile;
>+  };

If that doesn't actually make things simpler, feel free to ignore this advice, since the space saved isn't hugely compelling.

Comment 12

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

We don't always have an nsIVariant. In nsHTMLFormElement::GetFormData we create a FormData object and then iterate all form controls which will call AddNameValuePair/AddNameFilePair directly. This is the reason that nsFormData implement nsFormSubmission.

Comment 13

[Ben Newman (:bnewman) (:benjamn)]

(In reply to comment #12)
> We don't always have an nsIVariant. In nsHTMLFormElement::GetFormData we create
> a FormData object and then iterate all form controls which will call
> AddNameValuePair/AddNameFilePair directly. This is the reason that nsFormData
> implement nsFormSubmission.

Fair enough.  And we're pretty sure that the only types we'll ever have to care about are nsStrings and nsIFiles?

Comment 14

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

(In reply to comment #13)
> And we're pretty sure that the only types we'll ever have to care
> about are nsStrings and nsIFiles?

Nope. In fact, I'm quite certain that we'll have to care about more types down the road once the specs add support for more types. When that happens we'll have to change this code.

Comment 15

[Ben Newman (:bnewman) (:benjamn)]

Comment on attachment 427839 [details] [diff] [review]
Part3: Implement FormData

(In reply to comment #14)
> Nope. In fact, I'm quite certain that we'll have to care about more types down
> the road once the specs add support for more types. When that happens we'll
> have to change this code.

Very well then.  When that happens, we can probably get away with using a union type with simpler types (i.e., types without constructors).

Comment 16

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Checked in, thanks for the reviews

http://hg.mozilla.org/mozilla-central/rev/ffe5fb50ba37
http://hg.mozilla.org/mozilla-central/rev/e3cf67c96fed
http://hg.mozilla.org/mozilla-central/rev/00bc3f167040

Comment 17

[Reed Loden [:reed]]

Comment on attachment 427839 [details] [diff] [review]
Part3: Implement FormData

These need to be corrected:

>+ * The Initial Developer of the Original Code is
>+ * Netscape Communications Corporation.
>+ * Portions created by the Initial Developer are Copyright (C) 2010
>+ * the Initial Developer. All Rights Reserved.

s/Netscape Communications Corporation/Mozilla Foundation/

>+ * The Initial Developer of the Original Code is
>+ * mozilla.org.
>+ * Portions created by the Initial Developer are Copyright (C) 2010
>+ * the Initial Developer. All Rights Reserved.

s/mozilla.org/Mozilla Foundation/

>+ * The Initial Developer of the Original Code is
>+ * mozilla.org.
>+ * Portions created by the Initial Developer are Copyright (C) 2010
>+ * the Initial Developer. All Rights Reserved.

s/mozilla.org/Mozilla Foundation/

Comment 18

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Why is mozilla.org wrong?

Comment 19

[Reed Loden [:reed]]

(In reply to comment #18)
> Why is mozilla.org wrong?

There is no actual "mozilla.org" company that would be able to hold such a copyright. According to the California Secretary of State's Business Entity search (http://kepler.sos.ca.gov/), the only companies in the state of California that include the word "Mozilla" are Mozilla Corporation, Mozilla Foundation, and Mozilla Messaging, Inc. As per bug 507387, "Mozilla Foundation" is to be used in cases where the original author is an employee of the Mozilla Corporation or Mozilla Messaging, Inc. Gerv can better explain this if my description is unclear.

Comment 20

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Checked in license fix

http://hg.mozilla.org/mozilla-central/rev/a29d44f196a6

Comment 21

[Reed Loden [:reed]]

(In reply to comment #20)
> Checked in license fix

Thanks!

Comment 22

[Eric Shepherd [:sheppy]]

Is there a spec or design document I can look at to get an overview of what this is and how it works?

Comment 23

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Yup

http://dev.w3.org/2006/webapi/XMLHttpRequest-2/Overview.html#the-formdata-interface

Thanks for looking into this!

Comment 24

[Eric Shepherd [:sheppy]]

I gather this is simply a way to send a collection of keyword/value paired data via XMLHttpRequest with no formal tie to actual HTTP forms?

Comment 25

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

One thing that hasn't been specced yet, but which I implemented, is the ability to do

myFormElement.getFormData();

This returns a FormData object populated with the data from a <form>. This will populate the FormData with the name/value pairs that would be submitted if the form was submitted. No disabled or unselected controls etc.

So

xhr = new XMLHttpRequest();
xhr.open("POST", "server.php");
xhr.send(myFormElement.getFormData());

will send the same data that
myFormElement.submit();
would send if the form had enctype set to "multipart/form-data".

You can also do things like
fd = myFormElement.getFormData();
fd.append("name", "value");
fd.append("name", myFile);
xhr.send(fd);
to include name/value pairs that aren't in the <form>

Makes sense?

Comment 26

[Eric Shepherd [:sheppy]]

Will that be in the spec? That's wicked cool.

Comment 27

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

That will be in a HTML spec at some point yes. I've proposed it on the whatwg list, and apart from bikeshedding the name, there was agreement that it's useful.

Comment 28

[Eric Shepherd [:sheppy]]

I've written initial documentation for this:

https://developer.mozilla.org/en/XMLHttpRequest/FormData
https://developer.mozilla.org/En/XMLHttpRequest/Using_XMLHttpRequest#Using_FormData_objects

And I've updated the XMLHttpRequest reference's info about send():

https://developer.mozilla.org/en/XMLHttpRequest#send()

I'm working on a sample now.

Comment 29

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

From content/base/public/nsIDOMFormData.idl:

 * The Initial Developer of the Original Code is
 * Netscape Communications Corporation.
 * Portions created by the Initial Developer are Copyright (C) 2010
 * the Initial Developer. All Rights Reserved.

Sounds unlikely... Also, mozilla.org isn't an Initial Developer either, see
<http://weblogs.mozillazine.org/gerv/archives/2010/02/mpl_initial_developer_for_mozilla_employ.html>

Comment 30

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Sorry for the spam, I didn't notice it was fixed already.

Comment 31

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

See comment 20.

Comment 33

[Eric Shepherd [:sheppy]]

I have a sample for this that sends the data but at the moment the server side snippet to receive the data doesn't work due to permissions problems on devmo. I have a ticket filed for that, and hope to have it resolved soonish.

Comment 34

[Eric Shepherd [:sheppy]]

FormData is documented:

https://developer.mozilla.org/en/XMLHttpRequest/FormData

There's additional content with sample code here:

https://developer.mozilla.org/En/XMLHttpRequest/Using_XMLHttpRequest#Using_FormData_objects

Comment 35

[Alfonso Martinez]

The documentation should include a note about how to use it from JS Modules like it does for XMLHttprequest:

	var formData = Components.classes["@mozilla.org/files/formdata;1"]
		.createInstance(Components.interfaces.nsIDOMFormData);

And another note on the File (as that might be one of the main reasons to use FormData) that it will be available as such from Gecko 8 in JS Modules (the current note about Gecko 6 is a little misleading because that doesn't work in a module).