1108181 - [Fetch] Implement Headers iterable<>

Status: RESOLVED FIXED

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

Description

[Arkadiusz Michalski (Spirit)]

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:
https://bugzilla.mozilla.org/show_bug.cgi?id=1029620#c14

Is there any way to iterate over Headers at this moment?

Comment 1

[Andrew Overholt [:overholt]]

I think this is meant to be a blocker for the post-v1 SW milestone and not the B2G SW milestone.

Comment 2

[Ben Kelly [:bkelly, not reviewing]]

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.

Comment 3

[(no longer active)]

I almost have patches for this!

Comment 4

[(no longer active)]

Attachment: Make Headers iterable

Comment 5

[Boris Zbarsky [: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.

Comment 6

[(no longer active)]

Attachment: Make Headers iterable

Comment 7

[Boris Zbarsky [:bzbarsky]]

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:

  [...iter]

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!

Comment 8

[(no longer active)]

(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.

Comment 9

[(no longer active)]

This also fixes up a bunch of Cache web-platform tests that were previously failing because of <http://mxr.mozilla.org/mozilla-central/source/testing/web-platform/tests/service-workers/cache-storage/resources/testharness-helpers.js#40>.  I'll update the expectation files.

Comment 10

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/069effcd9de6

Comment 11

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/d630beb006c9 - 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? https://treeherder.mozilla.org/logviewer.html#?job_id=15915635&repo=mozilla-inbound and friends, they sure don't look like the update took.

Comment 12

[(no longer active)]

Hmm, James, do you know what causes this?  As far as I can tell, I've done everything I was supposed to do...

Thanks!

Comment 13

[James Graham [:jgraham]]

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/* ?

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d62986f26f00 has the metadata updated based on the results of that Windows job.

Comment 14

[(no longer active)]

Oh right, of course!

Comment 15

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9c1b2f524367

Comment 16

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/9c1b2f524367

Comment 17

[Jean-Yves Perrier [:teoli]]

Created:
https://developer.mozilla.org/en-US/docs/Web/API/Headers/entries
https://developer.mozilla.org/en-US/docs/Web/API/Headers/keys
https://developer.mozilla.org/en-US/docs/Web/API/Headers/values

Updated:
https://developer.mozilla.org/en-US/docs/Web/API/Headers
and
https://developer.mozilla.org/en-US/Firefox/Releases/44#Service_Workers