1126815 - implement Fetch Response finalURL attribute

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Ben Kelly [:bkelly, not reviewing]]

The fetch Response object recently added a FinalURL attribute:

  https://github.com/whatwg/fetch/commit/1ed10e81fe2301eea9974d8f8c671aae36bdd487

Comment 1

[surabhi anand]

Hello, i am not getting idea what needs to be done. Any help?

Comment 2

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

(In reply to surabhi anand from comment #1)
> Hello, i am not getting idea what needs to be done. Any help?

The fetch spec added a boolean flag finalURL https://fetch.spec.whatwg.org/#concept-response-final-url-flag
To implement it in Gecko, you'll have to add the same to WebIDL at dom/webidl/Response.webidl.
You will also have to add a boolean member to dom/fetch/InternalResponse.cpp, and a getter+setter for it, along with a getter and setter in dom/fetch/Response.cpp that just forwards to the internal response. Then, based on how the spec defines the usage of it, behaviour will have to be implemented.

Comment 3

[surabhi anand]

(In reply to Nikhil Marathe [:nsm] (needinfo? please) from comment #2)
Please correct me -


> The fetch spec added a boolean flag finalURL
> https://fetch.spec.whatwg.org/#concept-response-final-url-flag
> To implement it in Gecko, you'll have to add the same to WebIDL at
> dom/webidl/Response.webidl.

1-What i have understood so far by reading fetch spec that this finalURL attribute has to return true if the response's final url flag is set ,and this response is the URL of the requested thing whatever is being fetched ?

> You will also have to add a boolean member to
> dom/fetch/InternalResponse.cpp, and a getter+setter for it, along with a
> getter and setter in dom/fetch/Response.cpp that just forwards to the
> internal response.

2-This finalURL is a boolean flag but what is this final url flag ,i mean i have to set or unset it but by what(probably URL) ?

> Then, based on how the spec defines the usage of it, behaviour will have to be implemented.

3-How do i have to implement it,first in WebIDL ?.And by behaviour do you mean the way this flag returns true or false according to the state of final url flag ?

Comment 4

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

The fetch spec's lower case request/response concepts map to dom/fetch/Internal{Request,Response}, while the title-case, DOM-exposed Request/Response map to dom/fetch/{Request,Response}. So you will need a corresponding final url flag on InternalResponse. 

For 3, yes I mean what you stated. Adding a webidl attribute will only generate some automatic code that will call Response::FinalURL() and Response::SetFinalURL(bool) methods, which you'll have to define. Those methods will just get/set the value of the underlying InternalResponse (Response has mInternalResponse). In dom/fetch/FetchDriver.cpp, you'll have to implement the behaviour specified in Fetch step 9.

Comment 5

[surabhi anand]

Sorry Nikhil i cant work on this bug right now.Thank you:)

Comment 6

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

Attachment: Implement Response.finalURL

Comment 7

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8565543 [details] [diff] [review]
Implement Response.finalURL

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

LGTM.  Review to baku for DOM peer sign off.

Thanks!

::: dom/webidl/Response.webidl
@@ +19,5 @@
>  
>    readonly attribute USVString url;
> +  [Throws]
> +           attribute boolean finalURL;
> +

Nit: remove extra line to minimize diff to spec

::: dom/workers/test/fetch/worker_test_response.js
@@ +66,5 @@
>    ok(!r4.ok, "Response with status 302 should have ok false");
>  }
>  
> +// It is not possible to test setting finalURL until we have ServiceWorker
> +// interception. This is because synthetic Responses do not have a url, the url

Can we file a bug or reference an existing bug to test that finalURL is set correctly in the future?

Comment 8

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/4b55612b43f5

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/4b55612b43f5