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

Status: RESOLVED FIXED

(Core :: DOM: Workers, defect)

Description

[Josh Matthews [:jdm]]

They're currently ignored, and they shouldn't be.

Comment 1

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Attachment: Synth header and status

Comment 2

[Josh Matthews [:jdm]]

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.

Comment 4

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Attachment: Synthesize status and headers from Response returned by ServiceWorker

Comment 5

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

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.

Comment 6

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Attachment: 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 eebcf585c074a1386813bc674d46ce3f1fb275ae

Comment 7

[Josh Matthews [:jdm]]

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.

Comment 8

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

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.

Comment 9

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

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

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

Ship It!

Comment 10

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

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

Comment 11

[Josh Matthews [:jdm]]

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

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

Ship It!

Comment 12

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

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

Comment 13

[Josh Matthews [:jdm]]

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

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

Ship It!

Comment 14

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a67ff547821c
https://hg.mozilla.org/integration/mozilla-inbound/rev/6c0d6d01e922
https://hg.mozilla.org/integration/mozilla-inbound/rev/af96bfcc0108

Comment 15

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

https://hg.mozilla.org/mozilla-central/rev/a67ff547821c
https://hg.mozilla.org/mozilla-central/rev/6c0d6d01e922
https://hg.mozilla.org/mozilla-central/rev/af96bfcc0108

Comment 16

[Daniel Holbert [:dholbert]]

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

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/89c437fe79b5

Comment 19

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Attachment: MozReview Request: Bug 1134462 - Cleanup Promise usage in fetch test SW. r=jdm

Comment 20

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Attachment: MozReview Request: Bug 1134462 - Synthesize status and headers from Response returned by ServiceWorker. r=jdm

Comment 21

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Attachment: MozReview Request: Bug 1134462 - allow null body. r=jdm