Support iteration on FormData

RESOLVED FIXED in Firefox 44

Status

()

defect
RESOLVED FIXED
4 years ago
a month ago

People

(Reporter: nsm, Assigned: baku)

Tracking

({dev-doc-complete})

33 Branch
mozilla44
x86_64
Linux
Points:
---

Firefox Tracking Flags

(firefox44 fixed)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Comment 1

4 years ago
Posted patch iterable2.patch (obsolete) — Splinter Review
Attachment #8675708 - Flags: review?(bzbarsky)
(Assignee)

Comment 2

4 years ago
Posted patch iterable2.patch (obsolete) — Splinter Review
Assignee: nobody → amarchesini
Attachment #8675708 - Attachment is obsolete: true
Attachment #8675708 - Flags: review?(bzbarsky)
Attachment #8675710 - Flags: review?(bzbarsky)
Comment on attachment 8675710 [details] [diff] [review]
iterable2.patch

>+    if (tmp->mFormData[i].value.IsFile()) {
>+      ImplCycleCollectionUnlink(tmp->mFormData[i].value.GetAsFile());

How about:

  ImplCycleCollectionUnlink(tmp->mFormData[i].value)

?

And similar in traverse.  We codegen implementations of ImplCycleCollectionTraverse/ImplCycleCollectionUnlink for owning unions, precisely so you don't have to dig around inside them (especially important when they can have multiple refptr types).

>+      ImplCycleCollectionTraverse(cb,tmp->mFormData[i].value.GetAsFile(),

Missing space before "tmp".

>+    is(key.value, v.toString(), "Correct Key iterator: " + v.toString());
>+    ok(key.done, "key.done is false");

Should be ok(!key.done, "key.done should be false"), no?

>+    ok(value.done, "value.done is false");
>+    ok(entry.done, "entry.done is false");

And here.

>+++ b/xpcom/base/OwningNonNull.h

I guess it doesn't really hurt to add this, but I don't think it's really needed for this patch either...

r=me with the above fixed.
Attachment #8675710 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 4

4 years ago
Posted patch 268332.diff (obsolete) — Splinter Review
Attachment #8675710 - Attachment is obsolete: true
(Assignee)

Updated

4 years ago
Keywords: checkin-needed
(Assignee)

Comment 9

4 years ago
Attachment #8675758 - Attachment is obsolete: true
Flags: needinfo?(amarchesini)
(Assignee)

Updated

4 years ago
Keywords: checkin-needed

Comment 11

4 years ago
backoutbugherdermerge
https://hg.mozilla.org/mozilla-central/rev/512b90a0f35f
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
Jean-Yves, it's not clear to me what the "In most cases, the use of the for...of statement is preferrable to access the key/value pairs." bits in those docs are meant to suggest/imply.

Are they trying to say that it's better to just for..of on the FormData object itself?  Or on the iterators?  Something else?

Similarly, it's not clear what "objects implementing this interface can be used in a for...of structure" means in <https://developer.mozilla.org/en-US/docs/Web/API/FormData>.  Could probably use an example usage.  Just like it might be good to have examples in the values/keys/entries pages of using for-of with the iterators there ("for (var key of formData.keys()) { /* etc */ }").

There will also be a forEach method here once bug 1216751 is fixed, but it's not there yet.
Thx. ni/ myself so I don't forget to come back to this (doing too many things in // right now ;-) )
Flags: needinfo?(jypenator)
So it happens that I didn't understood that I could directly use an iterator in for..of (which is rather obvious now, silly me!).

I fixed the three methods pages removing the sentence (that made no sense) and fixed the example to use for...of.

On FormData, I modified the sentence to explain that 'for (var pair of myFD)' is equivalent to 'for (var pair of myFD.entries())'.

I also added dev-doc-needed and a comment to bug 1216751 so that we don't forget add forEach() when it lands.

I updated our meta-documentation about how to document iterator<>: https://developer.mozilla.org/en-US/docs/MDN/Contribute/Howto/Write_an_API_reference/Information_contained_in_a_WebIDL_file#Pair_iterator
Flags: needinfo?(jypenator)
Thank you!  That meta-documentation is pretty awesome, by the way.
Component: DOM → DOM: Core & HTML
Product: Core → Core
You need to log in before you can comment on or make changes to this bug.