1146365 - Add support of union type for FetchEvent.respondWith

Status: RESOLVED FIXED

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

Description

[Timur Valeev]

By FetchEvent.webidl now we are supporting two overloaded respondWith functions. But by the specification it should be one with union type argument:
interface FetchEvent : Event {
  readonly attribute Request request;
  readonly attribute Client client;
  readonly attribute boolean isReload;

  void respondWith((Response or Promise<Response>) r);
};

Comment 1

[Timur Valeev]

Attachment: bug-1146365-fix-0.1.patch

Comment 2

[Timur Valeev]

I tested this patch by local running of the mochitest test_fetch_event.html and there was no errors.

Comment 3

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

Comment on attachment 8581639 [details] [diff] [review]
bug-1146365-fix-0.1.patch

Review of attachment 8581639 [details] [diff] [review]:
-----------------------------------------------------------------

This looks good. I'd like to review once again after the following changes (and the try build)

::: dom/webidl/FetchEvent.webidl
@@ +16,5 @@
>    // https://github.com/slightlyoff/ServiceWorker/issues/631
>    readonly attribute Client? client; // The window issuing the request.
>    readonly attribute boolean isReload;
>  
> +  [Throws] void respondWith((Response or Promise<Response>) r);

Nit: Could you move the [Throws] to a new line, that way it is easier to diff against the spec's webidl

::: dom/workers/ServiceWorkerEvents.cpp
@@ +302,1 @@
>    }

How about:

nsRefPtr<Promise> promise;

if (IsResponse()) {
  promise = Promise::Create(...);
  resolve with response
} else {
  promise = GetAsPromise()
}

mWaitToRespond = true;
set up handler and append.

::: dom/workers/ServiceWorkerEvents.h
@@ +18,5 @@
>  
>  namespace mozilla {
>  namespace dom {
>    class Request;
> +  class ResponseOrPromise;

I have a feeling a forward declaration won't be enough on some of the platforms. Could you submit a T shaped try build? (i.e. set it to build on all platforms, but run tests only on linux32 or something. You can use http://trychooser.pub.build.mozilla.org/ to generate get the appropriate syntax)

Comment 4

[Timur Valeev]

Attachment: bug-1146365-fix-0.2.patch

Try build: https://treeherder.mozilla.org/#/jobs?repo=try&revision=9597c04c6644

Comment 5

[Timur Valeev]

Looks like the try build have some problem with Linux debug build. Is it ok?

Comment 6

[Josh Matthews [:jdm]]

I've retriggered it to be sure, by pressing the icon next to the pin in the bottom left when the job was highlighted.

Comment 7

[Timur Valeev]

Thank you Josh. Now all builds and tests are green.

Comment 8

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

I was unable to push this due to no DOM peer reviewing the webidl changes.

Comment 9

[Timur Valeev]

Attachment: bug-1146365-fix-0.3.patch

Hi Nikhil. It seems that my patch is obsolete. So I made a new one.
Try build: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c6a5abcaaad2

Comment 10

[Timur Valeev]

As I understand for the patch is needed DOM peer reviewing. Should I add somebody to reviewers?

Comment 11

[Josh Matthews [:jdm]]

Yes, let's have Ehsan review.

Comment 12

[Timur Valeev]

What is "dev-doc-needed"? Is it needed to add some information to MDN?
And additional question :) Could you recommend some task or bug?

Comment 13

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

Comment on attachment 8583672 [details] [diff] [review]
bug-1146365-fix-0.3.patch

Review of attachment 8583672 [details] [diff] [review]:
-----------------------------------------------------------------

Timur, please upload a new patch without the trailing whitespace, then ask :ehsan for review. There is no need for a new try build.
dev-doc-needed is not for you, it is for the documentation team who will document ServiceWorkers.

There is Bug 1115820 if you are interested.

::: dom/workers/ServiceWorkerEvents.cpp
@@ +307,2 @@
>    mWaitToRespond = true;
> +  nsRefPtr<RespondWithHandler> handler = 

Nit: trailing whitespace.

Comment 14

[Timur Valeev]

Attachment: bug-1146365-fix-0.4.patch

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b3abbecbd3c1

Comment 16

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/b3abbecbd3c1

Comment 17

[Jean-Yves Perrier [:teoli]]

Wrt to dev documentation, I wondering if this change the behavior for web devs. If so, what does it change? (If it is a purely technical change, maybe we don't need docs)

Comment 18

[Timur Valeev]

I think it is a technical change. As a result of this change nothing has changed for a web developer.
Am I right Nikhil?

Comment 19

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

(In reply to Timur Valeev from comment #18)
> I think it is a technical change. As a result of this change nothing has
> changed for a web developer.
> Am I right Nikhil?

That is correct.