Implement webidl stubs for the Fetch specification

RESOLVED FIXED in mozilla35

Status

()

defect
RESOLVED FIXED
5 years ago
3 months ago

People

(Reporter: bkelly, Assigned: nsm)

Tracking

unspecified
mozilla35
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 17 obsolete attachments)

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
> 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.
(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.
Blocks: 995484
Summary: Implement webidl for Request, AbstractResponse, Response interfaces → Implement webidl stubs for the Fetch specification
Assignee: nobody → nsm.nikhil
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
Exposes fetch() to Window and Worker when preffed on.
Attachment #8445342 - Attachment is obsolete: true
Attachment #8445470 - Attachment is obsolete: true
Attachment #8445501 - Attachment is obsolete: true
Attachment #8445547 - Attachment is obsolete: true
It seems we need FormData on workers (bug739173) in order to support FetchBodyStream.asFormData().
Depends on: 739173
Blocks: 1065216
Anne said that we don't strictly need FormData (bug 739173) for early dogfooding (aka "v1").
Ready to land webidl followed by some implementation bits.
Attachment #8490936 - Flags: review?(amarchesini)
Renamed header retrieval function to Headers()_ to avoid ambiguity with Headers interface.
Attachment #8491048 - Flags: review?(amarchesini)
Attachment #8490936 - Attachment is obsolete: true
Attachment #8490936 - Flags: review?(amarchesini)
Please don't create stubs that return values that aren't actually valid for the getter?
Return valid Header object.
Attachment #8491089 - Flags: review?(amarchesini)
Attachment #8491048 - Attachment is obsolete: true
Attachment #8491048 - Flags: review?(amarchesini)
clone() should return non-null too.
Attachment #8491097 - Flags: review?(amarchesini)
Attachment #8491089 - Attachment is obsolete: true
Attachment #8491089 - Flags: review?(amarchesini)
Attachment #8491051 - Attachment is obsolete: true
Attachment #8491051 - Flags: review?(amarchesini)
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 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 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+
> No pref for this interface?

It's NoInterfaceObject, so a [Pref] would make no sense (and codegen disallows it).
> Do we have a bug for this? in case file it and write here the ID.

The spec doesn't allow them, fwiw.
(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.
> 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'.
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.