Synthesize response status and headers from the properties of the Response object used in respondWith

RESOLVED FIXED in Firefox 39

Status

()

defect
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: jdm, Assigned: nsm)

Tracking

unspecified
mozilla39
x86
macOS
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox39 fixed)

Details

Attachments

(3 attachments, 3 obsolete attachments)

Reporter

Description

4 years ago
They're currently ignored, and they shouldn't be.
Assignee: nobody → nsm.nikhil
Status: NEW → ASSIGNED
Reporter

Comment 2

4 years ago
Comment on attachment 8568540 [details] [diff] [review]
Synth header and status

Review of attachment 8568540 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/fetch/InternalHeaders.cpp
@@ +44,5 @@
>    if (IsInvalidMutableHeader(lowerName, aValue, aRv)) {
>      return;
>    }
>  
> +  fprintf(stderr, "Appending %s to %p\n", nsAutoCString(aName).get(), this);

Remove.

::: dom/security/nsCORSListenerProxy.cpp
@@ +533,5 @@
>    // Check if the request failed
>    nsresult status;
>    nsresult rv = aRequest->GetStatus(&status);
>    if (NS_FAILED(rv)) {
> +    fprintf(stderr, "Got error %u\n", rv);

Remove.

@@ +538,5 @@
>      LogBlockedRequest(aRequest, "CORSRequestFailed", nullptr);
>      return rv;
>    }
>    if (NS_FAILED(status)) {
> +    fprintf(stderr, "Got status %" PRIu16 " %" PRIu16 "\n", NS_ERROR_GET_MODULE(status), NS_ERROR_GET_CODE(status));

Remove.

::: dom/workers/ServiceWorkerEvents.cpp
@@ +205,5 @@
> +  MOZ_ASSERT(headers);
> +  nsAutoTArray<InternalHeaders::Entry, 5> entries;
> +  headers->GetEntries(entries);
> +  // XXXnsm, how does ParseHeaderLine deal with headers that are actually
> +  // allowed to have multiple entries?

http://mxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/nsHttpHeaderArray.cpp#55 merges them as expected.
Attachment #8568540 - Flags: feedback?(josh) → feedback+
Summary: Synthesize response headers from the properties of the Response object used in respondWith → Synthesize response status and headers from the properties of the Response object used in respondWith
Comment on attachment 8568615 [details] [diff] [review]
Synthesize status and headers from Response returned by ServiceWorker

This isn't ok. Can't touch mInterceptedChannel on the worker thread.
Attachment #8568615 - Attachment is obsolete: true
Attachment #8568615 - Flags: review?(josh)
Posted file MozReview Request: bz://1134462/nsm (obsolete) —
/r/4323 - Bug 1134462 - Synthesize status and headers from Response returned by ServiceWorker. r=jdm

Pull down this commit:

hg pull review -r eebcf585c074a1386813bc674d46ce3f1fb275ae
Attachment #8569434 - Flags: review?(josh)
Reporter

Comment 7

4 years ago
Comment on attachment 8569434 [details]
MozReview Request: bz://1134462/nsm

https://reviewboard.mozilla.org/r/4321/#review3519

::: dom/workers/test/serviceworkers/fetch_event_worker.js
(Diff revision 1)
> +

Let's add a test for a redirect to a real file, too.

Maybe a redirect to a synthesized file as well? I don't recall if redirects are supposed to be intercepted.
Attachment #8569434 - Flags: review?(josh)
https://reviewboard.mozilla.org/r/4321/#review3555

> Let's add a test for a redirect to a real file, too.
> 
> Maybe a redirect to a synthesized file as well? I don't recall if redirects are supposed to be intercepted.

Redirects don't work, but it is a separate bug 1137287. jdm said r+ in person.
Reporter

Updated

4 years ago
Blocks: 1137287
Reporter

Updated

4 years ago
Depends on: 1142124
Comment on attachment 8569434 [details]
MozReview Request: bz://1134462/nsm

/r/4323 - Bug 1134462 - Synthesize status and headers from Response returned by ServiceWorker. r=jdm

Pull down this commit:

hg pull review -r 28d77f0eae0e6dbe5b84dbb17ed7aa41076ec2a6
Attachment #8569434 - Flags: review+ → review?(josh)
Reporter

Updated

4 years ago
No longer depends on: 1134330
Reporter

Updated

4 years ago
Attachment #8569434 - Flags: review?(josh) → review+
Reporter

Comment 11

4 years ago
Comment on attachment 8569434 [details]
MozReview Request: bz://1134462/nsm

https://reviewboard.mozilla.org/r/4321/#review4201

Ship It!
Comment on attachment 8569434 [details]
MozReview Request: bz://1134462/nsm

/r/4323 - Bug 1134462 - Synthesize status and headers from Response returned by ServiceWorker. r=jdm
/r/5399 - Bug 1134462 - allow null body. r=jdm
/r/5401 - Bug 1134462 - Cleanup Promise usage in fetch test SW. r=jdm

Pull down these commits:

hg pull review -r bc2488aa1c10afa88ecb82c6d0383069fca314e6
Reporter

Updated

4 years ago
Attachment #8569434 - Flags: review?(josh) → review+
Reporter

Comment 13

4 years ago
Comment on attachment 8569434 [details]
MozReview Request: bz://1134462/nsm

https://reviewboard.mozilla.org/r/4321/#review4401

Ship It!
https://hg.mozilla.org/mozilla-central/rev/a67ff547821c
https://hg.mozilla.org/mozilla-central/rev/6c0d6d01e922
https://hg.mozilla.org/mozilla-central/rev/af96bfcc0108
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
This was missing some MOZ_OVERRIDE annotations, which caused -Winconsistent-missing-override build warnings in clang 3.6 & newer. (which breaks warnings-as-errors builds w/ that compiler version)

I landed a trivial followup to add this annotation, with the blanket r+ that ehsan granted me for fixes of this sort over on bug 1126447 comment 2:
https://hg.mozilla.org/integration/mozilla-inbound/rev/89c437fe79b5
Attachment #8569434 - Attachment is obsolete: true
Attachment #8619522 - Flags: review+
Attachment #8619523 - Flags: review+
Attachment #8619524 - Flags: review+
You need to log in before you can comment on or make changes to this bug.