Closed Bug 1146365 Opened 5 years ago Closed 5 years ago

Add support of union type for FetchEvent.respondWith

Categories

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

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla39
Tracking Status
firefox39 --- fixed

People

(Reporter: tvaleev, Assigned: tvaleev)

Details

(Whiteboard: [lang=c++])

Attachments

(1 file, 3 obsolete files)

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);
};
Attached patch bug-1146365-fix-0.1.patch (obsolete) — Splinter Review
Attachment #8581639 - Flags: review?(nsm.nikhil)
Assignee: nobody → tvaleev
I tested this patch by local running of the mochitest test_fetch_event.html and there was no errors.
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)
Attachment #8581639 - Flags: review?(nsm.nikhil)
Attached patch bug-1146365-fix-0.2.patch (obsolete) — Splinter Review
Try build: https://treeherder.mozilla.org/#/jobs?repo=try&revision=9597c04c6644
Attachment #8581639 - Attachment is obsolete: true
Attachment #8582296 - Flags: review?(nsm.nikhil)
Looks like the try build have some problem with Linux debug build. Is it ok?
Flags: needinfo?(nsm.nikhil)
I've retriggered it to be sure, by pressing the icon next to the pin in the bottom left when the job was highlighted.
Flags: needinfo?(nsm.nikhil)
Thank you Josh. Now all builds and tests are green.
I was unable to push this due to no DOM peer reviewing the webidl changes.
Flags: needinfo?(tvaleev)
Flags: needinfo?(nsm.nikhil)
Keywords: checkin-needed
Attached patch bug-1146365-fix-0.3.patch (obsolete) — Splinter Review
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
Attachment #8582296 - Attachment is obsolete: true
Attachment #8583672 - Flags: review?(nsm.nikhil)
As I understand for the patch is needed DOM peer reviewing. Should I add somebody to reviewers?
Flags: needinfo?(tvaleev)
Yes, let's have Ehsan review.
Flags: needinfo?(nsm.nikhil)
What is "dev-doc-needed"? Is it needed to add some information to MDN?
And additional question :) Could you recommend some task or bug?
Flags: needinfo?(nsm.nikhil)
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.
Attachment #8583672 - Flags: review?(nsm.nikhil) → review+
Attachment #8584361 - Flags: review?(ehsan)
Attachment #8584361 - Flags: review?(ehsan) → review+
Keywords: checkin-needed
Attachment #8583672 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/b3abbecbd3c1
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
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)
Flags: needinfo?(tvaleev)
I think it is a technical change. As a result of this change nothing has changed for a web developer.
Am I right Nikhil?
Flags: needinfo?(tvaleev) → needinfo?(nsm.nikhil)
(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.
Flags: needinfo?(nsm.nikhil)
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.