Update Fetch API WebIDL and implementation.

RESOLVED FIXED in mozilla38

Status

()

defect
RESOLVED FIXED
5 years ago
5 months ago

People

(Reporter: nsm, Assigned: nsm)

Tracking

({dev-doc-complete})

33 Branch
mozilla38
x86_64
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

Posted patch WebIDL FixesSplinter Review
Add various [SameObject]/[NewObject] annotations.
Adds RequestCache enum.
Ensures that cors-with-forced-preflight is translated to cors in getter.
Reject cors-with-forced-preflight as a valid mode value in Request constructor.
Attachment #8545539 - Flags: review?(bkelly)
Assignee: nobody → nsm.nikhil
Status: NEW → ASSIGNED
Comment on attachment 8545539 [details] [diff] [review]
WebIDL Fixes

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

Do we have a follow-up bug to implement the cache settings in FetchDriver yet?

Flag Andrea for DOM peer sign off.

::: dom/webidl/Request.webidl
@@ +40,1 @@
>  enum RequestMode { "same-origin", "no-cors", "cors", "cors-with-forced-preflight" };

Follow up bug number?  Seems we just need an internal enum that maps to the webidl enum.
Attachment #8545539 - Flags: review?(bkelly)
Attachment #8545539 - Flags: review?(amarchesini)
Attachment #8545539 - Flags: review+
Oh, and the follow-up to remove cors-with-forced-preflight from the webidl should block pref'ing fetch on.
(In reply to Ben Kelly [:bkelly] from comment #2)
> Comment on attachment 8545539 [details] [diff] [review]
> WebIDL Fixes
> 
> Review of attachment 8545539 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Do we have a follow-up bug to implement the cache settings in FetchDriver
> yet?
> 
> Flag Andrea for DOM peer sign off.
> 
> ::: dom/webidl/Request.webidl
> @@ +40,1 @@
> >  enum RequestMode { "same-origin", "no-cors", "cors", "cors-with-forced-preflight" };
> 
> Follow up bug number?  Seems we just need an internal enum that maps to the
> webidl enum.

Actually the comment should have been removed. cors-with-forced-preflight *is* hidden from content in that very patch :) Request::Mode() ensures it never returns it, and Request::Constructor throws an error.
> Actually the comment should have been removed. cors-with-forced-preflight
> *is* hidden from content in that very patch :) Request::Mode() ensures it
> never returns it, and Request::Constructor throws an error.

Hmm.  Based on review feedback I've gotten before I don't think this is ok.  We want the webidl to match as close as possible even if the difference is not exposed to content.

Ehsan, what do you think?
Flags: needinfo?(ehsan)
Do we really need to reflect cors-with-forced-preflight in the IDL?  This seems to be the only place we use this value <https://dxr.mozilla.org/mozilla-central/source/dom/fetch/FetchDriver.cpp#115>, and it looks easy enough to replace with something internal.
Flags: needinfo?(ehsan)
(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #6)
> Do we really need to reflect cors-with-forced-preflight in the IDL?  This
> seems to be the only place we use this value
> <https://dxr.mozilla.org/mozilla-central/source/dom/fetch/FetchDriver.
> cpp#115>, and it looks easy enough to replace with something internal.

We rely on the WebIDL codegen to give us this enum and it's validation for 'free'. At the same time putting it in WebIDL does not affect content since content does not really see a RequestMode type. Is it worth implementing all the validation only for well-formed-ness?
(In reply to Nikhil Marathe [:nsm] (away Dec 27-Jan 1) from comment #7)
> (In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from
> comment #6)
> > Do we really need to reflect cors-with-forced-preflight in the IDL?  This
> > seems to be the only place we use this value
> > <https://dxr.mozilla.org/mozilla-central/source/dom/fetch/FetchDriver.
> > cpp#115>, and it looks easy enough to replace with something internal.
> 
> We rely on the WebIDL codegen to give us this enum and it's validation for
> 'free'. At the same time putting it in WebIDL does not affect content since
> content does not really see a RequestMode type. Is it worth implementing all
> the validation only for well-formed-ness?

Well, as long as you ensure that content will see the exact same error when passing this value as it would when passing "foopityfoo" then I guess you can add some comments in the IDL explaining why you're adding this Mozilla specific member to the enum and that it is not exposed to content.
Attachment #8545539 - Flags: review?(amarchesini) → review+
The patch does indeed display the exact same error. I've added a test in case we ever change it in the future.
https://hg.mozilla.org/mozilla-central/rev/6749eed3e68c
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
Fetch specification docs largely complete, and needing review: https://developer.mozilla.org/en-US/docs/Web/API/Fetch_API. See Bug 1039846 for more details.
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.