User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:34.0) Gecko/20100101 Firefox/34.0 Build ID: 20141125180439 Steps to reproduce: Algorithm from spec: https://fetch.spec.whatwg.org/#dom-response-redirect Actually when invoke redirect("someURL", status) we can see that: - "someURL" argument isn't parse, passing wrong URL not throw - status argument is not taken into account - default value for response's status is 200 (but should be 302) - "someURL" argument isn't set in headers list as value of `Location' header Looks like that now redirect() method wokrs the same as Response() constructor.

Comment on attachment 8541714 [details] [diff] [review] Implement Response.redirect Review of attachment 8541714 [details] [diff] [review]: ----------------------------------------------------------------- Overall this looks good and the test nicely shows that it does what the spec says. Just a few small issues. r=me with these addressed. Also flagging Andrea to look it over as I'm not a peer. ::: dom/fetch/Response.cpp @@ +56,2 @@ > { > + nsString parsedURL; nsAutoString @@ +57,5 @@ > + nsString parsedURL; > + > + if (NS_IsMainThread()) { > + nsCOMPtr<nsPIDOMWindow> window = do_QueryInterface(aGlobal.GetAsSupports()); > + MOZ_ASSERT(window); This assert is unneeded as nsCOMPtr<> de-ref will crash on null. @@ +59,5 @@ > + if (NS_IsMainThread()) { > + nsCOMPtr<nsPIDOMWindow> window = do_QueryInterface(aGlobal.GetAsSupports()); > + MOZ_ASSERT(window); > + nsCOMPtr<nsIURI> docURI = window->GetDocumentURI(); > + nsCString spec; nsAutoCString @@ +74,5 @@ > + > + url->Stringify(parsedURL, aRv); > + if (aRv.Failed()) { > + return nullptr; > + } You could move this last failure check from here and the else condition to just after the else block. This would remove a little duplicate code. @@ +80,5 @@ > + workers::WorkerPrivate* worker = workers::GetCurrentThreadWorkerPrivate(); > + MOZ_ASSERT(worker); > + worker->AssertIsOnWorkerThread(); > + > + nsString baseURL = NS_ConvertUTF8toUTF16(worker->GetLocationInfo().mHref); Does this create an extra string ref count? You can avoid the issue by just making the local have NS_ConvertUTF8toUTF16 directly: NS_ConvertUTF8toUTF16 baseURL(worker->GetLocationInfo().mHref); @@ +96,5 @@ > + > + if (aStatus != 301 && aStatus != 302 && aStatus != 303 && aStatus != 307 && aStatus != 308) { > + aRv.Throw(NS_ERROR_RANGE_ERR); > + return nullptr; > + } It would be nice if the spec allowed us to do this first so we didn't have to waste time parsing the URL. @@ +98,5 @@ > + aRv.Throw(NS_ERROR_RANGE_ERR); > + return nullptr; > + } > + > + Optional<ArrayBufferOrArrayBufferViewOrBlobOrUSVStringOrURLSearchParams> body; Holy type name, batman! ::: dom/webidl/Response.webidl @@ +11,5 @@ > Exposed=(Window,Worker), > Func="mozilla::dom::Headers::PrefEnabled"] > interface Response { > [NewObject] static Response error(); > + [NewObject, Throws] static Response redirect(USVString url, optional unsigned short status = 302); The feedback I recently received on webidl was to put moz-specific things like Throws on their own line. The goal is to make the webidl as close to the spec as possible so copy/paste comparisons can be mode. So maybe this instead: [Throws, NewObject] static Response redirect(USVString url, optional unsigned short status = 302);

Comment on attachment 8541714 [details] [diff] [review] Implement Response.redirect Review of attachment 8541714 [details] [diff] [review]: ----------------------------------------------------------------- Nothing more to add to bkelly's comments.

https://hg.mozilla.org/mozilla-central/rev/9932fb743bf8 https://hg.mozilla.org/mozilla-central/rev/ed5d8f2baead