Closed
Bug 1085283
Opened 10 years ago
Closed 10 years ago
Implement FormData manipulation methods
Categories
(Core :: DOM: Core & HTML, defect)
Tracking
()
RESOLVED
FIXED
mozilla39
Tracking | Status | |
---|---|---|
firefox39 | --- | fixed |
People
(Reporter: annevk, Assigned: nsm)
References
()
Details
(Keywords: dev-doc-complete)
Attachments
(7 files, 2 obsolete files)
21.08 KB,
patch
|
baku
:
review+
|
Details | Diff | Splinter Review |
8.63 KB,
patch
|
jgraham
:
review+
|
Details | Diff | Splinter Review |
39 bytes,
text/x-review-board-request
|
Details | |
39 bytes,
text/x-review-board-request
|
Details | |
39 bytes,
text/x-review-board-request
|
Details | |
39 bytes,
text/x-review-board-request
|
Details | |
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.
Reporter | ||
Updated•10 years ago
|
Keywords: dev-doc-needed
Assignee | ||
Comment 1•10 years ago
|
||
/r/3109 - Bug 1085283 - Implement FormData manipulation methods.
Pull down this commit:
hg pull review -r b0f9f557cfbbdeb39eee90437a88910c2c517eea
Attachment #8556212 -
Flags: review?(jonas)
Assignee | ||
Comment 2•10 years ago
|
||
Jonas, should I hide these methods behind a pref?
Assignee | ||
Updated•10 years ago
|
Assignee: nobody → nsm.nikhil
Assignee | ||
Comment 3•10 years ago
|
||
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?
Reporter | ||
Comment 5•10 years ago
|
||
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.
Assignee | ||
Comment 7•10 years ago
|
||
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)
Assignee | ||
Comment 8•10 years ago
|
||
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)
Assignee | ||
Comment 9•10 years ago
|
||
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)
Assignee | ||
Updated•10 years ago
|
Attachment #8556212 -
Flags: review?(jonas)
Assignee | ||
Updated•10 years ago
|
Summary: Implement FormData manipulation and iteration methods → Implement FormData manipulation methods
Assignee | ||
Comment 11•10 years ago
|
||
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)
Assignee | ||
Updated•10 years ago
|
Attachment #8556212 -
Flags: review?(jonas)
Assignee | ||
Comment 12•10 years ago
|
||
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)
Assignee | ||
Comment 13•10 years ago
|
||
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
Assignee | ||
Comment 14•10 years ago
|
||
Comment on attachment 8556212 [details]
MozReview Request: bz://1085283/nsm
Sorry about the noise, still figuring reviewboard out.
Attachment #8556212 -
Flags: review?(jonas)
Assignee | ||
Comment 15•10 years ago
|
||
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
Assignee | ||
Comment 16•10 years ago
|
||
Attachment #8563719 -
Flags: review?(amarchesini)
Comment 17•10 years ago
|
||
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+
Assignee | ||
Comment 18•10 years ago
|
||
Please take a look at the various DOMBlob->File changes and RemoveAllOthersAndGetFirstFormDataTuple.
Attachment #8566681 -
Flags: review?(amarchesini)
Assignee | ||
Updated•10 years ago
|
Attachment #8563719 -
Attachment is obsolete: true
Comment 19•10 years ago
|
||
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+
Assignee | ||
Comment 20•10 years ago
|
||
Comment 21•10 years ago
|
||
Assignee | ||
Comment 22•10 years ago
|
||
Assignee | ||
Comment 23•10 years ago
|
||
I ran the update script.
Attachment #8567550 -
Flags: review?(james)
Assignee | ||
Updated•10 years ago
|
Attachment #8566681 -
Attachment is obsolete: true
Updated•10 years ago
|
Attachment #8567550 -
Flags: review?(james) → review+
Assignee | ||
Updated•10 years ago
|
Attachment #8566681 -
Attachment is obsolete: false
Assignee | ||
Comment 24•10 years ago
|
||
Assignee | ||
Updated•10 years ago
|
Keywords: checkin-needed
Comment 25•10 years ago
|
||
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
Assignee | ||
Comment 26•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/e243a8f61b70
https://hg.mozilla.org/mozilla-central/rev/1b18b4a58ccd
Status: NEW → RESOLVED
Closed: 10 years ago
status-firefox39:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
Assignee | ||
Updated•10 years ago
|
Flags: needinfo?(nsm.nikhil)
Updated•10 years ago
|
Attachment #8556212 -
Flags: review?(amarchesini)
Comment 28•10 years ago
|
||
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
Keywords: dev-doc-needed → dev-doc-complete
Assignee | ||
Comment 29•10 years ago
|
||
Attachment #8556212 -
Attachment is obsolete: true
Assignee | ||
Comment 30•10 years ago
|
||
Assignee | ||
Comment 31•10 years ago
|
||
Assignee | ||
Comment 32•10 years ago
|
||
Assignee | ||
Comment 33•10 years ago
|
||
Assignee | ||
Comment 34•10 years ago
|
||
Updated•6 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•