Closed
Bug 1017613
Opened 10 years ago
Closed 10 years ago
Implement webidl stubs for the Fetch specification
Categories
(Core :: DOM: Core & HTML, defect)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla35
People
(Reporter: bkelly, Assigned: nsm)
References
Details
Attachments
(3 files, 17 obsolete files)
8.20 KB,
patch
|
baku
:
review+
|
Details | Diff | Splinter Review |
15.02 KB,
patch
|
baku
:
review+
|
Details | Diff | Splinter Review |
11.54 KB,
patch
|
baku
:
review+
|
Details | Diff | Splinter Review |
In order to implement the Cache interface from the ServiceWorker spec[1] we also need the Request and AbstractResponse interfaces. These dependent interfaces will also be needed by the Fetch API[2] as well. So we should probably implement these in a location that is not ServiceWorker specific. Also, I noticed that |interface Request| in webidl translates by default into |nsIDOMRequest| in our code. This is really close to |nsIDOMDOMRequest| and |nsIDOMRequestService| which could be confusing. I believe its possible to override the class mapping in the bindings.conf file, so we could name the classes |nsIDOMWebRequest| or something. One of the reasons I opened this bug was to see if people have a strong opinion about the naming of the Request C++ classes. [1] http://slightlyoff.github.io/ServiceWorker/spec/service_worker/index.html [2] http://fetch.spec.whatwg.org/#fetch-api
Comment 1•10 years ago
|
||
> Also, I noticed that |interface Request| in webidl translates by default into
> |nsIDOMRequest| in our code.
If you use a external interface, right? Otherwise it maps to mozilla::dom::Request by default.
Reporter | ||
Comment 2•10 years ago
|
||
(In reply to Boris Zbarsky [:bz] from comment #1) > If you use a external interface, right? Otherwise it maps to > mozilla::dom::Request by default. Ah, I probably just did something wrong when I was stubbing out Cache locally here. Anyway, Nikhil has provided me patches that stub out Request and Response. I'll attach them here shortly.
Reporter | ||
Comment 3•10 years ago
|
||
Reporter | ||
Comment 4•10 years ago
|
||
Assignee | ||
Comment 5•10 years ago
|
||
Attachment #8430840 -
Attachment is obsolete: true
Assignee | ||
Comment 6•10 years ago
|
||
Attachment #8430841 -
Attachment is obsolete: true
Assignee | ||
Updated•10 years ago
|
Attachment #8439383 -
Attachment is obsolete: true
Assignee | ||
Updated•10 years ago
|
Attachment #8439386 -
Attachment is obsolete: true
Assignee | ||
Updated•10 years ago
|
Summary: Implement webidl for Request, AbstractResponse, Response interfaces → Implement webidl stubs for the Fetch specification
Assignee | ||
Comment 7•10 years ago
|
||
Assignee | ||
Updated•10 years ago
|
Assignee: nobody → nsm.nikhil
Assignee | ||
Comment 8•10 years ago
|
||
Forgot to add files.
Assignee | ||
Updated•10 years ago
|
Attachment #8444745 -
Attachment is obsolete: true
Reporter | ||
Comment 9•10 years ago
|
||
Update this patch to account for Headers moving off to bug 1029620. This patch is dependent on the patches in bug 1029620.
Attachment #8444748 -
Attachment is obsolete: true
Assignee | ||
Comment 10•10 years ago
|
||
Moved headers out.
Assignee | ||
Comment 11•10 years ago
|
||
Assignee | ||
Comment 12•10 years ago
|
||
Exposes fetch() to Window and Worker when preffed on.
Reporter | ||
Updated•10 years ago
|
Attachment #8445342 -
Attachment is obsolete: true
Assignee | ||
Updated•10 years ago
|
Blocks: dom-fetch-api
Comment 13•10 years ago
|
||
Attachment #8445470 -
Attachment is obsolete: true
Comment 14•10 years ago
|
||
Attachment #8445501 -
Attachment is obsolete: true
Comment 15•10 years ago
|
||
Attachment #8445547 -
Attachment is obsolete: true
Reporter | ||
Comment 16•10 years ago
|
||
It seems we need FormData on workers (bug739173) in order to support FetchBodyStream.asFormData().
Depends on: 739173
Comment 17•10 years ago
|
||
Anne said that we don't strictly need FormData (bug 739173) for early dogfooding (aka "v1").
Assignee | ||
Comment 18•10 years ago
|
||
Ready to land webidl followed by some implementation bits.
Attachment #8490936 -
Flags: review?(amarchesini)
Assignee | ||
Updated•10 years ago
|
Attachment #8469575 -
Attachment is obsolete: true
Assignee | ||
Updated•10 years ago
|
Attachment #8469576 -
Attachment is obsolete: true
Assignee | ||
Updated•10 years ago
|
Attachment #8469580 -
Attachment is obsolete: true
Assignee | ||
Comment 19•10 years ago
|
||
Renamed header retrieval function to Headers()_ to avoid ambiguity with Headers interface.
Attachment #8491048 -
Flags: review?(amarchesini)
Assignee | ||
Updated•10 years ago
|
Attachment #8490936 -
Attachment is obsolete: true
Attachment #8490936 -
Flags: review?(amarchesini)
Assignee | ||
Comment 20•10 years ago
|
||
Attachment #8491051 -
Flags: review?(amarchesini)
Assignee | ||
Comment 21•10 years ago
|
||
Attachment #8491052 -
Flags: review?(amarchesini)
Comment 22•10 years ago
|
||
Please don't create stubs that return values that aren't actually valid for the getter?
Assignee | ||
Comment 23•10 years ago
|
||
Return valid Header object.
Attachment #8491089 -
Flags: review?(amarchesini)
Assignee | ||
Updated•10 years ago
|
Attachment #8491048 -
Attachment is obsolete: true
Attachment #8491048 -
Flags: review?(amarchesini)
Assignee | ||
Comment 24•10 years ago
|
||
clone() should return non-null too.
Attachment #8491097 -
Flags: review?(amarchesini)
Assignee | ||
Updated•10 years ago
|
Attachment #8491089 -
Attachment is obsolete: true
Attachment #8491089 -
Flags: review?(amarchesini)
Assignee | ||
Comment 25•10 years ago
|
||
Attachment #8491101 -
Flags: review?(amarchesini)
Assignee | ||
Updated•10 years ago
|
Attachment #8491051 -
Attachment is obsolete: true
Attachment #8491051 -
Flags: review?(amarchesini)
Comment 26•10 years ago
|
||
Comment on attachment 8491097 [details] [diff] [review] Part 1 - Fetch Body and Request Review of attachment 8491097 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/bindings/Bindings.conf @@ +874,5 @@ > 'nativeType': 'nsDOMCSSRGBColor', > }, > > +'Request': { > + 'binaryNames': { 'headers': 'Headers_' } Headers_ => headers_ ::: dom/fetch/Request.cpp @@ +4,5 @@ > + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > + > +#include "Request.h" > +#include "nsDOMString.h" > +#include "nsPIDOMWindow.h" alphabetic order ::: dom/fetch/Request.h @@ +34,5 @@ > + aUrl.AsAString() = EmptyString(); > + } > + > + void > + GetMethod(nsCString& aMethod) const @@ +57,5 @@ > + { > + aReferrer.AsAString() = EmptyString(); > + } > + > + already_AddRefed<Headers> Headers_() const; Headers* Headers_() const { return mHeaders; } @@ +66,5 @@ > + > + virtual JSObject* > + WrapObject(JSContext* aCx) > + { > + return mozilla::dom::RequestBinding::Wrap(aCx, this); mozilla::dom:: is not needed. @@ +94,5 @@ > + BodyUsed(); > +private: > + ~Request(); > + > + nsISupports* mOwner; nsCOMPtr<nsISupports> ::: dom/webidl/Fetch.webidl @@ +9,5 @@ > + > +typedef object JSON; > +typedef (ArrayBuffer or ArrayBufferView or Blob or FormData or ScalarValueString or URLSearchParams) BodyInit; > + > +[NoInterfaceObject, Exposed=(Window,Worker)] No pref for this interface? ::: dom/webidl/Request.webidl @@ +26,5 @@ > + > +Request implements Body; > + > +dictionary RequestInit { > + ByteString method; default values for this dictionary attributes. ::: dom/workers/test/fetch/test_interfaces.html @@ +4,5 @@ > +--> > +<!DOCTYPE HTML> > +<html> > +<head> > + <title>Bug XXXXXX - Test fetch API interfaces</title> XXXXXX => 1017613 ::: dom/workers/test/fetch/worker_interfaces.js @@ +3,5 @@ > + postMessage({type: 'status', status: !!a, msg: a + ": " + msg }); > +} > + > +function is(a, b, msg) { > + dump("IS: " + (a===b) + " => " + a + " | " + b + " " + msg + "\n"); is() and isnot() should not be part of this patch. @@ +14,5 @@ > +} > + > +onmessage = function() { > + ok(typeof Headers === "function", "Headers should be defined"); > + ok(typeof Request === "function", "Request should be defined"); I would test the constructors and the attributes too.
Attachment #8491097 -
Flags: review?(amarchesini) → review+
Comment 27•10 years ago
|
||
Comment on attachment 8491101 [details] [diff] [review] Part 2 - Response IDL and stubs Review of attachment 8491101 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/bindings/Bindings.conf @@ +870,5 @@ > 'nativeType': 'nsDOMCSSRect', > }, > > +'Response': { > + 'binaryNames': { 'headers': 'Headers_' }, lower-case H ::: dom/fetch/Response.h @@ +5,5 @@ > + > +#ifndef mozilla_dom_Response_h > +#define mozilla_dom_Response_h > + > +#include "mozilla/dom/ResponseBinding.h" Just because you have a cpp file, move this to the cpp file and move the WrapObject there too. Do the same for the Response object. @@ +52,5 @@ > + aStatusText = EmptyCString(); > + } > + > + already_AddRefed<Headers> > + Headers_() const; Headers* Headers_() const { return mHeaders; } @@ +96,5 @@ > + BodyUsed(); > +private: > + ~Response(); > + > + nsISupports* mOwner; nsCOMPtr ::: dom/webidl/Response.webidl @@ +3,5 @@ > + * License, v. 2.0. If a copy of the MPL was not distributed with this file, > + * You can obtain one at http://mozilla.org/MPL/2.0/. > + * > + * The origin of this IDL file is > + * http://fetch.spec.whatwg.org/ Do we have a better URL pointing to the interface? @@ +27,5 @@ > +Response implements Body; > + > +dictionary ResponseInit { > + unsigned short status = 200; > + // Becase we don't seem to support default values for ByteString. Do we have a bug for this? in case file it and write here the ID.
Attachment #8491101 -
Flags: review?(amarchesini) → review+
Comment 28•10 years ago
|
||
Comment on attachment 8491052 [details] [diff] [review] Part 3 - fetch() IDL and stubs Review of attachment 8491052 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/base/nsGlobalWindow.cpp @@ +6368,5 @@ > return rv.ErrorCode(); > } > > +already_AddRefed<Promise> > +nsGlobalWindow::Fetch(const RequestOrScalarValueString& aInput, const RequestInit& aInit, ErrorResult& aRv) more than 80chars? ::: dom/workers/WorkerScope.cpp @@ +304,5 @@ > } > > +already_AddRefed<Promise> > +WorkerGlobalScope::Fetch(const RequestOrScalarValueString& aInput, const RequestInit& aInit, ErrorResult& aRv) > +{ 80chars
Attachment #8491052 -
Flags: review?(amarchesini) → review+
Comment 29•10 years ago
|
||
> No pref for this interface?
It's NoInterfaceObject, so a [Pref] would make no sense (and codegen disallows it).
Comment 30•10 years ago
|
||
> Do we have a bug for this? in case file it and write here the ID.
The spec doesn't allow them, fwiw.
Assignee | ||
Comment 31•10 years ago
|
||
(In reply to Andrea Marchesini (:baku) from comment #26) > Comment on attachment 8491097 [details] [diff] [review] > Part 1 - Fetch Body and Request > > Review of attachment 8491097 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: dom/bindings/Bindings.conf > @@ +874,5 @@ > > 'nativeType': 'nsDOMCSSRGBColor', > > }, > > > > +'Request': { > > + 'binaryNames': { 'headers': 'Headers_' } > > Headers_ => headers_ codegen requires that the key be the same as the webidl entry, so lowercase is required. > ::: dom/webidl/Request.webidl > @@ +26,5 @@ > > + > > +Request implements Body; > > + > > +dictionary RequestInit { > > + ByteString method; > > default values for this dictionary attributes. There aren't any defined in IDL, and the Request constructor tests each one since there are various requirements. > > ::: dom/workers/test/fetch/worker_interfaces.js > @@ +3,5 @@ > > + postMessage({type: 'status', status: !!a, msg: a + ": " + msg }); > > +} > > + > > +function is(a, b, msg) { > > + dump("IS: " + (a===b) + " => " + a + " | " + b + " " + msg + "\n"); > > is() and isnot() should not be part of this patch. leaving is() in because the implementation test uses it. Removed isnot. > > @@ +14,5 @@ > > +} > > + > > +onmessage = function() { > > + ok(typeof Headers === "function", "Headers should be defined"); > > + ok(typeof Request === "function", "Request should be defined"); > > I would test the constructors and the attributes too. The Request constructor is well tested in the implementation patch.
Comment 32•10 years ago
|
||
> codegen requires that the key be the same as the webidl entry, so lowercase is required.
Andrea was saying that the _value_ should have a lowercase 'h'.
Assignee | ||
Comment 33•10 years ago
|
||
remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/083a2fb884ed remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/f048600cf938 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/77b599edbab4
Assignee | ||
Comment 34•10 years ago
|
||
was backed out due to various trivial compile errors. trying again remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/09e3b826cf7d remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/a223830b97a4 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/9d0634802627
Comment 35•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/09e3b826cf7d https://hg.mozilla.org/mozilla-central/rev/a223830b97a4 https://hg.mozilla.org/mozilla-central/rev/9d0634802627
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
Updated•5 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•