1017613 - Implement webidl stubs for the Fetch specification

Status: RESOLVED FIXED

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

Description

[Ben Kelly [:bkelly, not reviewing]]

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

[Boris Zbarsky [:bzbarsky]]

> 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.

Comment 2

[Ben Kelly [:bkelly, not reviewing]]

(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.

Comment 3

[Ben Kelly [:bkelly, not reviewing]]

Attachment: Part 1 - Request IDL and stubs

Comment 4

[Ben Kelly [:bkelly, not reviewing]]

Attachment: Part 2 - Response IDL and stubs.

Comment 5

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Attachment: Part 1 - Request IDL and stubs

Comment 6

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Attachment: Part 2 - Response IDL and stubs.

Comment 7

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Attachment: Part 1 - FetchBodyStream, Headers and Request.

Comment 8

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Attachment: Part 1 - FetchBodyStream, Headers and Request.

Forgot to add files.

Comment 9

[Ben Kelly [:bkelly, not reviewing]]

Attachment: Part 1 - FetchBodyStream, Headers and Request.

Update this patch to account for Headers moving off to bug 1029620.  This patch is dependent on the patches in bug 1029620.

Comment 10

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Attachment: Part 1 - FetchBodyStream and Request.

Moved headers out.

Comment 11

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Attachment: Part 2 - Response IDL and stubs.

Comment 12

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Attachment: Part 2 - fetch() IDL and stubs.

Exposes fetch() to Window and Worker when preffed on.

Comment 13

[Cătălin Badea (:catalinb)]

Attachment: Part 1 - FetchBodyStream and Request.(v2)

Comment 14

[Cătălin Badea (:catalinb)]

Attachment: Part 2 - Response IDL and stubs.(v2)

Comment 15

[Cătălin Badea (:catalinb)]

Attachment: Part 3 - fetch() IDL and stubs.(v2)

Comment 16

[Ben Kelly [:bkelly, not reviewing]]

It seems we need FormData on workers (bug739173) in order to support FetchBodyStream.asFormData().

Comment 17

[Andrew Overholt [:overholt]]

Anne said that we don't strictly need FormData (bug 739173) for early dogfooding (aka "v1").

Comment 18

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Attachment: Part 1 - Fetch Body and Request

Ready to land webidl followed by some implementation bits.

Comment 19

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Attachment: Part 1 - Fetch Body and Request

Renamed header retrieval function to Headers()_ to avoid ambiguity with Headers interface.

Comment 20

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Attachment: Part 2 - Response IDL and stubs

Comment 21

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Attachment: Part 3 - fetch() IDL and stubs

Comment 22

[Boris Zbarsky [:bzbarsky]]

Please don't create stubs that return values that aren't actually valid for the getter?

Comment 23

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Attachment: Part 1 - Fetch Body and Request

Return valid Header object.

Comment 24

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Attachment: Part 1 - Fetch Body and Request

clone() should return non-null too.

Comment 25

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Attachment: Part 2 - Response IDL and stubs

Comment 26

[Andrea Marchesini [:baku]]

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.

Comment 27

[Andrea Marchesini [:baku]]

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.

Comment 28

[Andrea Marchesini [:baku]]

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

Comment 29

[Boris Zbarsky [:bzbarsky]]

> No pref for this interface?

It's NoInterfaceObject, so a [Pref] would make no sense (and codegen disallows it).

Comment 30

[Boris Zbarsky [:bzbarsky]]

> Do we have a bug for this? in case file it and write here the ID.

The spec doesn't allow them, fwiw.

Comment 31

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

(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

[Boris Zbarsky [:bzbarsky]]

> 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'.

Comment 33

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

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

Comment 34

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

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

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/09e3b826cf7d
https://hg.mozilla.org/mozilla-central/rev/a223830b97a4
https://hg.mozilla.org/mozilla-central/rev/9d0634802627