Closed Bug 546528 Opened 10 years ago Closed 10 years ago

Implement FormData

Categories

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

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: sicking, Assigned: sicking)

References

Details

(Keywords: dev-doc-complete, Whiteboard: [evang-wanted-3.7])

Attachments

(3 files)

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.
Whiteboard: [evang-wanted-3.7]
Assignee: nobody → jonas
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.
Attachment #427832 - Flags: review?(bnewman)
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.
Attachment #427837 - Flags: review?(bnewman)
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
Attachment #427839 - Flags: review?(bnewman)
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?
Attachment #427837 - Flags: review?(bnewman) → review+
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);
Attachment #427832 - Flags: review?(bnewman) → review+
(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.
(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?
Ah, that was my intent, but it appears I missed a few spots. I'll fix the remaining places to use the = syntax.
In order to apply the patches in this bug, first land the patches in bug 547165 and bug 544698 (in that order)
Actually, it's bug 547165, bug 544698 and bug 545059. In that order.
Depends on: 547165
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.
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.
(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?
(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 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).
Attachment #427839 - Flags: review?(bnewman) → review+
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/
Why is mozilla.org wrong?
(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.
(In reply to comment #20)
> Checked in license fix

Thanks!
Keywords: dev-doc-needed
Is there a spec or design document I can look at to get an overview of what this is and how it works?
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?
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?
Will that be in the spec? That's wicked cool.
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.
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.
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>
Sorry for the spam, I didn't notice it was fixed already.
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.
Depends on: 607217
No longer depends on: 607217
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).
Component: HTML: Form Submission → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.