Closed
Bug 1108181
Opened 10 years ago
Closed 9 years ago
[Fetch] Implement Headers iterable<>
Categories
(Core :: DOM: Core & HTML, defect)
Tracking
()
RESOLVED
FIXED
mozilla44
Tracking | Status | |
---|---|---|
firefox44 | --- | fixed |
People
(Reporter: crimsteam, Assigned: ehsan.akhgari)
References
Details
(Keywords: dev-doc-complete)
Attachments
(1 file, 1 obsolete file)
6.29 KB,
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
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•10 years ago
|
Component: General → DOM
Status: UNCONFIRMED → NEW
Ever confirmed: true
Updated•10 years ago
|
Blocks: ServiceWorkers-v1
Updated•10 years ago
|
OS: Windows 7 → All
Hardware: x86_64 → All
Updated•10 years ago
|
Updated•10 years ago
|
Keywords: dev-doc-needed
Blocks: 1189673
Blocks: 1189536
Blocks: 1189670
Comment 1•9 years ago
|
||
I think this is meant to be a blocker for the post-v1 SW milestone and not the B2G SW milestone.
Comment 2•9 years ago
|
||
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 4•9 years ago
|
||
Attachment #8675797 -
Flags: review?(bzbarsky)
![]() |
||
Comment 5•9 years ago
|
||
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•9 years ago
|
||
Attachment #8675849 -
Flags: review?(bzbarsky)
Assignee | ||
Updated•9 years ago
|
Attachment #8675797 -
Attachment is obsolete: true
![]() |
||
Comment 7•9 years ago
|
||
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•9 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•9 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.
Comment 10•9 years ago
|
||
Comment 11•9 years ago
|
||
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•9 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)
Comment 13•9 years ago
|
||
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•9 years ago
|
||
Oh right, of course!
Comment 15•9 years ago
|
||
Comment 16•9 years ago
|
||
bugherder merge |
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox44:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
Comment 17•9 years ago
|
||
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
Keywords: dev-doc-needed → dev-doc-complete
Updated•6 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•