Closed Bug 1085283 Opened 9 years ago Closed 8 years ago

Implement FormData manipulation methods

Categories

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

Product:
Core
Component:
DOM: Core & HTML
Platform:
x86
macOS
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla39
Tracking Flags:
Tracking Status
firefox39 --- fixed

People

(Reporter: annevk, Assigned: nsm)

References

(
URL
)

Details

(Keywords: dev-doc-complete)

Attachments

(7 files, 2 obsolete files)

Patch 1 Implement FormData manipulation methods
21.08 KB, patch
baku
: review+
Details | Diff | Splinter Review
Update web-platform-tests expected data
8.63 KB, patch
jgraham
: review+
Details | Diff | Splinter Review
MozReview Request: Bug 1127150 - Fix FormData File filename
39 bytes, text/x-review-board-request
Details
MozReview Request: Request and Response constructors should set mime type.
39 bytes, text/x-review-board-request
Details
MozReview Request: Bug 1085283 - Implement FormData manipulation methods.
39 bytes, text/x-review-board-request
Details
MozReview Request: Bug 1109751 - FormData in Fetch API.
39 bytes, text/x-review-board-request
Details
MozReview Request: Bug 739173 - Support FormData in workers.
39 bytes, text/x-review-board-request
Details 
FormData is quite a bit more complicated now than when we first implemented it. We should enable the new features.
Keywords: dev-doc-needed
Attached file MozReview Request: bz://1085283/nsm (obsolete) — 
/r/3109 - Bug 1085283 - Implement FormData manipulation methods.

Pull down this commit:

hg pull review -r b0f9f557cfbbdeb39eee90437a88910c2c517eea
Attachment #8556212 - Flags: review?(jonas)
Jonas, should I hide these methods behind a pref?
Assignee: nobody → nsm.nikhil
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
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?
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.
Ok. Like I said, I haven't followed this very closely so I'll defer to others.
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?
Flags: needinfo?(jonas)
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
Attachment #8556212 - Flags: review?(amarchesini)
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
Please do have baku review.
Flags: needinfo?(jonas)
Summary: Implement FormData manipulation and iteration methods → Implement FormData manipulation methods
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
Attachment #8556212 - Flags: review?(jonas)
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
Attachment #8556212 - Flags: review?(jonas)
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 on attachment 8556212 [details]
MozReview Request: bz://1085283/nsm

Sorry about the noise, still figuring reviewboard out.
Attachment #8556212 - Flags: review?(jonas)
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 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
Attachment #8563719 - Flags: review?(amarchesini) → review+
Please take a look at the various DOMBlob->File changes and RemoveAllOthersAndGetFirstFormDataTuple.
Attachment #8566681 - Flags: review?(amarchesini)
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.
Attachment #8566681 - Flags: review?(amarchesini) → review+
Attachment #8567550 - Flags: review?(james) → review+
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!
Flags: needinfo?(nsm.nikhil)
Keywords: checkin-needed
Flags: needinfo?(nsm.nikhil)
Attachment #8556212 - Flags: review?(amarchesini)
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.