Closed Bug 1127703 Opened 9 years ago Closed 8 years ago

Support iteration on FormData


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

33 Branch
Not set



Tracking Status
firefox44 --- fixed


(Reporter: nsm, Assigned: baku)



(Keywords: dev-doc-complete)


(1 file, 3 obsolete files)

Attached patch iterable2.patch (obsolete) — Splinter Review
Attachment #8675708 - Flags: review?(bzbarsky)
Attached 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]

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

How about:



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+
Attached patch 268332.diff (obsolete) — Splinter Review
Attachment #8675710 - Attachment is obsolete: true
Keywords: checkin-needed
Attached patch iterable2.patchSplinter Review
Attachment #8675758 - Attachment is obsolete: true
Flags: needinfo?(amarchesini)
Keywords: checkin-needed
Closed: 8 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 <>.  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<>:
Flags: needinfo?(jypenator)
Thank you!  That meta-documentation is pretty awesome, by the way.
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.