Closed Bug 1108181 Opened 9 years ago Closed 8 years ago

[Fetch] Implement Headers iterable<>


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

36 Branch
Not set



Tracking Status
firefox44 --- fixed


(Reporter: crimsteam, Assigned: ehsan.akhgari)



(Keywords: dev-doc-complete)


(1 file, 1 obsolete file)

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:34.0) Gecko/20100101 Firefox/34.0
Build ID: 20141125180439

Steps to reproduce:

Probably forgot about this after close this bug:

Is there any way to iterate over Headers at this moment?
Component: General → DOM
Depends on: 1085293
Ever confirmed: true
OS: Windows 7 → All
Hardware: x86_64 → All
I think this is meant to be a blocker for the post-v1 SW milestone and not the B2G SW milestone.
Blocks: ServiceWorkers-postv1
No longer blocks: ServiceWorkers-B2G
I know this is not a v1 blocker, but I want to get it in so we pass more wpt tests.  It should be easy since Kyle did all the heavy lifting.
Assignee: nobody → bkelly
I almost have patches for this!
Assignee: bkelly → ehsan
Attached patch Make Headers iterable (obsolete) — Splinter Review
Attachment #8675797 - Flags: review?(bzbarsky)
Comment on attachment 8675797 [details] [diff] [review]
Make Headers iterable

You need some non-ASCII tests, which would quickly show that this implementation is wrong.

>+  return ToJSValue(aCx, NS_ConvertUTF8toUTF16(aArgument), aValue);

This is why we don't have a ToJSValue overload for nsACString: it's not clear whether to do NS_ConvertUTF8toUTF16 or NS_ConvertASCIItoUTF16.  For Headers you need the latter.

I think we have two main options:

1)  Come up with some class that is known to represent a ByteString (e.g. a subclass of nsCString, or something that can be inited with a const nsACString reference, or something), implement ToJSValue for it and use it here.  This would work because the caller in IterableIterator.h never examines the return type of GetKeyAtIndex/GetValueAtIndex.

2)  Do the conversion to char16_t strings in your GetKeyAtIndex/GetValueAtIndex nsString or some such.  Then you fully control the conversion.

And please do add tests that would have caught this.
Attachment #8675797 - Flags: review?(bzbarsky) → review-
Attachment #8675849 - Flags: review?(bzbarsky)
Attachment #8675797 - Attachment is obsolete: true
Comment on attachment 8675849 [details] [diff] [review]
Make Headers iterable

>+  const nsString GetKeyAtIndex(unsigned aIndex) const
>+    return NS_ConvertASCIItoUTF16(mList[aIndex].mName);

It's possible that you could avoid a string copy and allocation here if you made the return type NS_ConvertASCIItoUTF16, due to return value optimization.  Might be worth doing.

>+function iterate(iter) {
>+function iterateForOf(iter) {

In theory, these should both be equivalent, as long as the for-of impl is not broken, and both should be equivalent to:


but I'm not sure to what extend you want to depend on this.

>+  arrayEquals(iterate(value_iter), ["bar", ehsanInflated, "baz2"], "Correct key iterator");

"Correct values iterator".

r=me.  Thanks for beefing up the tests!
Attachment #8675849 - Flags: review?(bzbarsky) → review+
(In reply to Boris Zbarsky [:bz] from comment #7)
> >+function iterate(iter) {
> >+function iterateForOf(iter) {
> In theory, these should both be equivalent, as long as the for-of impl is
> not broken, and both should be equivalent to:
>   [...iter]
> but I'm not sure to what extend you want to depend on this.

Yeah, I realize this is really an EcmaScript test.  :-)  But I prefer to keep it anyway.
This also fixes up a bunch of Cache web-platform tests that were previously failing because of <>.  I'll update the expectation files.
Backed out in - it certainly looks like you updated the wpt expectations, but was there some other step you had to take to make those updates actually have an effect? Maybe running some script, or maybe fixing the build system so that updating expectations doesn't require a clobber? and friends, they sure don't look like the update took.
Hmm, James, do you know what causes this?  As far as I can tell, I've done everything I was supposed to do...

Flags: needinfo?(james)
It looks like you updated the files for /service-workers/cache-storage/serviceworker/* but not /service-workers/cache-storage/window/* or /service-workers/cache-storage/worker/* ? has the metadata updated based on the results of that Windows job.
Flags: needinfo?(james)
Oh right, of course!
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.