1085283 - Implement FormData manipulation methods

Status: RESOLVED FIXED

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

Description

[Anne (:annevk)]

FormData is quite a bit more complicated now than when we first implemented it. We should enable the new features.

Comment 1

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Attachment: MozReview Request: bz://1085283/nsm

/r/3109 - Bug 1085283 - Implement FormData manipulation methods.

Pull down this commit:

hg pull review -r b0f9f557cfbbdeb39eee90437a88910c2c517eea

Comment 2

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Jonas, should I hide these methods behind a pref?

Comment 3

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Comment on attachment 8556212 [details]
MozReview Request: bz://1085283/nsm

/r/3109 - Bug 1085283 - Implement FormData manipulation methods.
/r/3121 - Bug 1127150 - Fix FormData File filename

Pull down these commits:

hg pull review -r 9eb24b9cfabf58c2ea1e6a0045b7045b387174c2

Comment 4

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

I don't have an opinion on FormData really. I haven't been following it lately.

That said, it seems to me that FormData is growing into a generic holder for a list of name-value pairs. Which is something that JS already has in the form of plain JS Objects/Arrays.

Would it be possible to come up with a syntax for fetch() and onfetch that doesn't requre FormData objects at all and that instead uses some other mechanism to indicate that a given JS Array should be multipart/form-data encoded when it's sent to the network?

Comment 5

[Anne (:annevk)]

We had that discussion before. JavaScript doesn't really have anything like a multimap. And FormData already has the integration with HTML forms and XMLHttpRequest.

Comment 6

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

Ok. Like I said, I haven't followed this very closely so I'll defer to others.

Comment 7

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Jonas are you still going to review this considering you were the original author? Otherwise I'll pass it on to baku since he has done most of the File work. Pref or no-pref?

Comment 8

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Comment on attachment 8556212 [details]
MozReview Request: bz://1085283/nsm

/r/3109 - Bug 1085283 - Implement FormData manipulation methods.
/r/3121 - Bug 1127150 - Fix FormData File filename
/r/3135 - Bug 739173 - Support FormData in workers.

Pull down these commits:

hg pull review -r 207ddd21438ee94173aff7f4dfd8fa64f5d03953

Comment 9

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Comment on attachment 8556212 [details]
MozReview Request: bz://1085283/nsm

/r/3109 - Bug 1085283 - Implement FormData manipulation methods.
/r/3121 - Bug 1127150 - Fix FormData File filename
/r/3135 - Bug 739173 - Support FormData in workers.

Pull down these commits:

hg pull review -r 1ca20fe40fc694bc297c613234b8ad0eecee438f

Comment 10

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

Please do have baku review.

Comment 11

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Comment on attachment 8556212 [details]
MozReview Request: bz://1085283/nsm

/r/3109 - Bug 1085283 - Implement FormData manipulation methods.
/r/3121 - Bug 1127150 - Fix FormData File filename
/r/3135 - Bug 739173 - Support FormData in workers.

Pull down these commits:

hg pull review -r f5f371d421ae790537d02deca243125d87675518

Comment 12

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Comment on attachment 8556212 [details]
MozReview Request: bz://1085283/nsm

/r/3109 - Bug 1085283 - Implement FormData manipulation methods.
/r/3121 - Bug 1127150 - Fix FormData File filename
/r/3135 - Bug 739173 - Support FormData in workers.

Pull down these commits:

hg pull review -r f5f371d421ae790537d02deca243125d87675518

Comment 13

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Comment on attachment 8556212 [details]
MozReview Request: bz://1085283/nsm

/r/3109 - Bug 1085283 - Implement FormData manipulation methods.
/r/3121 - Bug 1127150 - Fix FormData File filename
/r/3135 - Bug 739173 - Support FormData in workers.
/r/3275 - Bug 1109751 - FormData in Fetch API.

Pull down these commits:

hg pull review -r 9172b6235e6189c256f9257ed3eaa8da19ad7623

Comment 14

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Comment on attachment 8556212 [details]
MozReview Request: bz://1085283/nsm

Sorry about the noise, still figuring reviewboard out.

Comment 15

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Comment on attachment 8556212 [details]
MozReview Request: bz://1085283/nsm

/r/3109 - Bug 1085283 - Implement FormData manipulation methods.
/r/3121 - Bug 1127150 - Fix FormData File filename
/r/3135 - Bug 739173 - Support FormData in workers.
/r/3275 - Bug 1109751 - FormData in Fetch API.
/r/3291 - Request and Response constructors should set mime type.

Pull down these commits:

hg pull review -r 285049f6f3eba7e5b7269c40c6fb718082065a56

Comment 16

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Attachment: Patch 1 Implement FormData manipulation methods

Comment 17

[Andrea Marchesini [:baku]]

Comment on attachment 8563719 [details] [diff] [review]
Patch 1 Implement FormData manipulation methods

Review of attachment 8563719 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/base/nsFormData.cpp
@@ +98,5 @@
> +nsFormData::ExtractValue(const FormDataTuple& aTuple,
> +                         OwningFileOrUSVString* aOutValue)
> +{
> +  if (aTuple.valueIsFile) {
> +    nsRefPtr<File> file = static_cast<File*>(aTuple.fileValue.get());

if you use File everywhere you don't need this static_Cast

@@ +144,5 @@
> +  return false;
> +}
> +
> +int32_t
> +nsFormData::RemoveAllExceptFirstAndGetFirstIndex(const nsAString& aName)

I don't like this int32_t. What about this (I didn't test this code):

FormDataTuple*
nsFormData::RemoveAllExceptFirstAndGetFromDataTuple(const nsAString& aName) // invent a better name please
{
  FormDataTuple* data = nullptr;
  for (uint32_t i = 0, len = mFormData.Length(); i < len;) {
    if (!aName.Equals(mFormData[i].name)) {
      ++i;
      continue;
    }

    if (!data) {
      data = &mFormData[i];
      ++i;
      continue;
    }

    mFormData[i].RemoveElement(i);
    --len;
  }

  return data;
}

@@ +165,5 @@
> +void
> +nsFormData::Set(const nsAString& aName, File& aBlob,
> +                const Optional<nsAString>& aFilename)
> +{
> +  int32_t index = RemoveAllExceptFirstAndGetFirstIndex(aName);

Then, here you can do:

FormDataTule* data = RemoveAllExceptFirstAndFoobar(aName);
if (data) {e
  ...
} else {
  Append()
}

::: dom/base/nsFormData.h
@@ +36,5 @@
> +  struct FormDataTuple
> +  {
> +    nsString name;
> +    nsString stringValue;
> +    nsCOMPtr<nsIDOMBlob> fileValue;

nsRefPtr<File>
and somewhere typedef mozilla::dom::File File;

@@ +58,5 @@
> +  }
> +
> +  void SetNameFilePair(FormDataTuple* aData,
> +                       const nsAString& aName,
> +                       nsIDOMBlob* aBlob,

File*

@@ +100,5 @@
> +  void Delete(const nsAString& aName);
> +  void Get(const nsAString& aName, mozilla::dom::Nullable<mozilla::dom::OwningFileOrUSVString>& aOutValue);
> +  void GetAll(const nsAString& aName, nsTArray<mozilla::dom::OwningFileOrUSVString>& aValues);
> +  bool Has(const nsAString& aName);
> +  void Set(const nsAString& aName, mozilla::dom::File& aBlob,

then remove mozilla::dom::

::: dom/webidl/FormData.webidl
@@ +18,5 @@
> +  sequence<FormDataEntryValue> getAll(USVString name);
> +  boolean has(USVString name);
> +  void set(USVString name, Blob value, optional USVString filename);
> +  void set(USVString name, USVString value);
> +  // iterable<USVString, FormDataEntryValue>; - Bug 1085284

bug 1085293

Comment 18

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Attachment: Patch 1 Implement FormData manipulation methods

Please take a look at the various DOMBlob->File changes and RemoveAllOthersAndGetFirstFormDataTuple.

Comment 19

[Andrea Marchesini [:baku]]

Comment on attachment 8566681 [details] [diff] [review]
Patch 1 Implement FormData manipulation methods

Review of attachment 8566681 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/base/nsFormData.cpp
@@ +98,5 @@
> +nsFormData::ExtractValue(const FormDataTuple& aTuple,
> +                         OwningFileOrUSVString* aOutValue)
> +{
> +  if (aTuple.valueIsFile) {
> +    aOutValue->SetAsFile() = aTuple.fileValue.get();

do you need this .get() ?

@@ +147,5 @@
> +nsFormData::RemoveAllOthersAndGetFirstFormDataTuple(const nsAString& aName)
> +{
> +  FormDataTuple* lastFoundTuple = nullptr;
> +  uint32_t lastFoundIndex = mFormData.Length();
> +  for (int32_t i = mFormData.Length() - 1; i >= 0; --i) {

uint32_t

::: dom/html/nsFormSubmission.h
@@ +60,2 @@
>                                     const nsString& aFilename) = 0;
>    

remove this extra space.

Comment 20

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ef51eb31fa09

Comment 21

[Ryan VanderMeulen [:RyanVM]]

Backed out for w-p-t failures.
https://hg.mozilla.org/integration/mozilla-inbound/rev/717e92977f9a

https://treeherder.mozilla.org/logviewer.html#?job_id=6849757&repo=mozilla-inbound

Comment 22

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4aaefbebaca5

Comment 23

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Attachment: Update web-platform-tests expected data

I ran the update script.

Comment 24

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ca629bc05862

Comment 25

[Carsten Book [:Tomcat]]

Hi, the wpt part failes to apply:

applying bug1085283-1
patching file testing/web-platform/meta/media-source/mediasource-config-change-mp4-a-bitrate.html.ini
Hunk #1 FAILED at 0
1 out of 1 hunks FAILED -- saving rejects to file testing/web-platform/meta/media-source/mediasource-config-change-mp4-a-bitrate.html.ini.rej
patch failed, unable to continue (try -v)

could you take a look, thanks!

Comment 26

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e243a8f61b70
https://hg.mozilla.org/integration/mozilla-inbound/rev/1b18b4a58ccd

Comment 27

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/e243a8f61b70
https://hg.mozilla.org/mozilla-central/rev/1b18b4a58ccd

Comment 28

[Chris Mills [:cmills]]

I believe this is documented sufficiently with the recent changes we made to the docs:

https://developer.mozilla.org/en-US/docs/Web/API/FormData
https://developer.mozilla.org/en-US/docs/Web/API/Worker/Functions_and_classes_available_to_workers
https://developer.mozilla.org/en-US/docs/Web/API/Body/formData

Comment 30

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Attachment: MozReview Request: Bug 1127150 - Fix FormData File filename

Comment 31

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Attachment: MozReview Request: Request and Response constructors should set mime type.

Comment 32

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Attachment: MozReview Request: Bug 1085283 - Implement FormData manipulation methods.

Comment 33

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Attachment: MozReview Request: Bug 1109751 - FormData in Fetch API.

Comment 34

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Attachment: MozReview Request: Bug 739173 - Support FormData in workers.