Closed
Bug 1119044
Opened 10 years ago
Closed 10 years ago
Update Fetch API WebIDL and implementation.
Categories
(Core :: DOM: Core & HTML, defect)
Tracking
()
RESOLVED
FIXED
mozilla38
People
(Reporter: nsm, Assigned: nsm)
References
Details
(Keywords: dev-doc-complete)
Attachments
(1 file)
(deleted),
patch
|
bkelly
:
review+
baku
:
review+
|
Details | Diff | Splinter Review |
No description provided.
Assignee | ||
Comment 1•10 years ago
|
||
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 | ||
Updated•10 years ago
|
Assignee: nobody → nsm.nikhil
Status: NEW → ASSIGNED
Comment 2•10 years ago
|
||
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+
Comment 3•10 years ago
|
||
Oh, and the follow-up to remove cors-with-forced-preflight from the webidl should block pref'ing fetch on.
Assignee | ||
Comment 4•10 years ago
|
||
(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.
Comment 5•10 years ago
|
||
> 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)
Comment 6•10 years ago
|
||
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)
Assignee | ||
Comment 7•10 years ago
|
||
(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?
Comment 8•10 years ago
|
||
(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.
Updated•10 years ago
|
Attachment #8545539 -
Flags: review?(amarchesini) → review+
Assignee | ||
Comment 9•10 years ago
|
||
The patch does indeed display the exact same error. I've added a test in case we ever change it in the future.
Assignee | ||
Comment 10•10 years ago
|
||
Comment 11•10 years ago
|
||
Assignee | ||
Comment 12•10 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
Updated•10 years ago
|
Keywords: dev-doc-needed
Comment 14•10 years ago
|
||
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.
Keywords: dev-doc-needed → dev-doc-complete
Comment 15•10 years ago
|
||
Also https://developer.mozilla.org/en-US/Firefox/Releases/38#Interfaces.2FAPIs.2FDOM has a mention.
Updated•6 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•