[Fetch] Implement Headers iterable<>

RESOLVED FIXED in Firefox 44

Status

()

defect
RESOLVED FIXED
4 years ago
a month ago

People

(Reporter: crimsteam, Assigned: Ehsan)

Tracking

({dev-doc-complete})

36 Branch
mozilla44
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox44 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

4 years ago
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?
(Reporter)

Updated

4 years ago
Component: General → DOM
Status: UNCONFIRMED → NEW
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
Status: NEW → ASSIGNED
(Assignee)

Comment 3

4 years ago
I almost have patches for this!
Assignee: bkelly → ehsan
(Assignee)

Comment 4

4 years ago
Posted 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-
(Assignee)

Comment 6

4 years ago
Attachment #8675849 - Flags: review?(bzbarsky)
(Assignee)

Updated

4 years ago
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:

  [...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!
Attachment #8675849 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 8

4 years ago
(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.
(Assignee)

Comment 9

4 years ago
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.
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.
(Assignee)

Comment 12

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

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

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

Comment 14

4 years ago
Oh right, of course!

Comment 16

4 years ago
bugherdermerge
https://hg.mozilla.org/mozilla-central/rev/9c1b2f524367
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
Component: DOM → DOM: Core & HTML
Product: Core → Core
You need to log in before you can comment on or make changes to this bug.