Closed Bug 1119044 Opened 10 years ago Closed 10 years ago

Update Fetch API WebIDL and implementation.

Categories

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

33 Branch
x86_64
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla38

People

(Reporter: nsm, Assigned: nsm)

References

Details

(Keywords: dev-doc-complete)

Attachments

(1 file)

Attached patch WebIDL Fixes (deleted) — Splinter 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.
Status: ASSIGNED → RESOLVED
Closed: 10 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.

Attachment

General

Created:
Updated:
Size: