Closed Bug 1136757 Opened 6 years ago Closed 6 years ago

Add direct Response overload for FetchEvent.respondWith()

Categories

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

33 Branch
x86_64
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla39
Tracking Status
firefox39 --- fixed

People

(Reporter: nsm, Assigned: tvaleev, Mentored)

References

Details

(Whiteboard: [lang=c++])

Attachments

(1 file, 3 obsolete files)

Right now we only support Promise<Response>
Depends on: 1065216
This requires adding an overload to FetchEvent.webidl, and adding a C++ overload to ServiceWorkerEvents.cpp that skips the Promise creation and ends up executing the same code as RespondWithHandler::ResolvedCallback currently does.
Mentor: josh
Whiteboard: [lang=c++]
In the draft spec I found that:
4.6
FetchEvent
...
interface FetchEvent : Event {
...
void respondWith((Response or Promise<Response>) r);
};
So respondWith can have two types of arguments, Response or Promise<Response>.
 
Am I right that is needed to add support of the FetchEvent.respondWith() that have Response argument (first type)?
Flags: needinfo?(josh)
That is correct. In the gecko implementation (dom/webidl/FetchEvent.webidl), only Promise is specified. You'll want to change that. Then add a similar function in dom/workers/ServiceWorkerEvents.{h,cpp}, which will just create a Promise with the passed Response and call the existing RespondWith that accepts a Promise.
Flags: needinfo?(josh)
Oh and please assign the bug to yourself if you are working on it. Thanks!
Assignee: nobody → tvaleev
Ok. I added overload RespondWith(Response& aResponse, ErrorResult& aRv) method to ServiceWorkerEvents.{h,cpp} files.
But now I have a question. How to pass aResponse to the new Promise?

I only found one solution:
nsCOMPtr<nsIGlobalObject> global = do_QueryInterface(&aResponse);
nsRefPtr<Promise> promise = Promise::Create(global, aRv);

Is it right? I would appreciate your help.
Flags: needinfo?(nsm.nikhil)
You will want to resolve the promise, passing the response object as an argument. See http://mxr.mozilla.org/mozilla-central/source/dom/workers/ServiceWorkerManager.cpp?force=1#352 for an example of resolving promises from C++.
Flags: needinfo?(nsm.nikhil)
Attached patch 1136757v0.3.diff (obsolete) — Splinter Review
I create draft patch. Hope that I understand correct :)
Attachment #8573954 - Flags: review?(josh)
Looks sensible to me! Best way to find out is to add a test to dom/workers/tests/mochitest/serviceworkers/fetch_event_worker.js and dom/workers/tests/mochitest/serviceworkers/fetch/fetch_tests.js.
Just checking; there's no rush - are you working on adding a test, Timur?
Flags: needinfo?(tvaleev)
Sure. Sorry for the delay.
Comment on attachment 8573954 [details] [diff] [review]
1136757v0.3.diff

Clearing the review until there's a test to show it works :)
Flags: needinfo?(tvaleev)
Attachment #8573954 - Flags: review?(josh) → feedback+
Attached patch 1136757v0.4.diff (obsolete) — Splinter Review
I added some test for respondWith. Am I moving in the right direction?:)
Attachment #8573954 - Attachment is obsolete: true
Attachment #8577313 - Flags: review?(josh)
But when I tried to run the test locally by using:
./mach mochitest-plain dom/workers/test/serviceworkers/test_fetch_event.html
I got:
INFO TEST-UNEXPECTED-FAIL | dom/workers/test/serviceworkers/test_fetch_event.html | uncaught exception - : NS_ERROR_UNEXPECTED:  at http://mochi.test:8888/tests/dom/workers/test/serviceworkers/fetch_event_worker.js:12

Looks like global object from the ServiceWorkerEvents.cpp is not created normally in
254: nsCOMPtr<nsIGlobalObject> global = do_QueryInterface(GetParentObject());

I don't have ideas why it happened :(.
Flags: needinfo?(josh)
Have you stepped through your new method in gdb? Are you sure it's the GetParentObject line that's failing?
Flags: needinfo?(josh)
I wonder if you should call MaybeResolve after calling the other RespondWith first?
Comment on attachment 8577313 [details] [diff] [review]
1136757v0.4.diff

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

The worker scope is a top level DomEventTargetHelper and has no parent object, but Event::SetOwner() tries to use it.
What you want to do is, instead of using GetParentObject(), use the currently running worker's scope like so:

WorkerPrivate* worker = GetCurrentThreadWorkerPrivate();
MOZ_ASSERT(worker);
worker->AssertIsOnWorkerThread();
...
Promise::Create(worker->GlobalScope())

::: dom/workers/test/serviceworkers/fetch/fetch_tests.js
@@ +26,5 @@
>    my_ok(xhr.responseText == "synthesized response body", "load should have synthesized response");
>    finish();
>  });
>  
> +fetch('response.txt', function(xhr) {

Nit: Please call this something more unique, like 'test-respondwith-response'.
Attached patch 1136757v0.5.diff (obsolete) — Splinter Review
I create a new patch that use the currently running worker's scope that describing in comment 16.

But when I try to test it I have:
INFO TEST-UNEXPECTED-FAIL | dom/workers/test/serviceworkers/test_fetch_event.html | uncaught exception - : InvalidStateError: An attempt was made to use an object that is not, or is no longer, usable at http://mochi.test:8888/tests/dom/workers/test/serviceworkers/fetch_event_worker.js:12

So may be I created not correct patch?
Attachment #8577313 - Attachment is obsolete: true
Attachment #8577313 - Flags: review?(josh)
Flags: needinfo?(nsm.nikhil)
Flags: needinfo?(josh)
Attachment #8577632 - Flags: review?(josh)
Timur, in your RespondWith function, you set the mWaitToRespond flag to true, which is then checked by the Promise variant leading to failure. Don't modify mWaitToRespond in the Response overload, let the Promise overload deal with it.
Flags: needinfo?(nsm.nikhil)
Flags: needinfo?(josh)
Comment on attachment 8577632 [details] [diff] [review]
1136757v0.5.diff

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

r=me with comments addressed.
Please upload the changed patch and link to a tryserver build. No need to ask for review again.

::: dom/workers/ServiceWorkerEvents.cpp
@@ +251,5 @@
> +    aRv.Throw(NS_ERROR_DOM_INVALID_STATE_ERR);
> +    return;
> +  }
> +
> +  mWaitToRespond = true;

Remove this otherwise it won't work.

::: dom/workers/test/serviceworkers/fetch/fetch_tests.js
@@ +27,5 @@
>    finish();
>  });
>  
> +fetch('test-respondwith-response.txt', function(xhr) {
> +  my_ok(xhr.status == 200, "load should be successful");

in the test description, please prefix "test-respondwith-response.txt" so that on failures it is easy to figure out which test failed.

@@ +28,5 @@
>  });
>  
> +fetch('test-respondwith-response.txt', function(xhr) {
> +  my_ok(xhr.status == 200, "load should be successful");
> +  my_ok(xhr.responseText == "response body", "load should have response");

same here.

::: dom/workers/test/serviceworkers/fetch_event_worker.js
@@ +7,5 @@
>      ev.respondWith(p);
>    }
>  
> +  else if (ev.request.url.contains("test-respondwith-response.txt")) {
> +    var p = new Response("response body", {});

Nit: call this response instead of p.
Attachment #8577632 - Flags: review?(josh) → review+
Updated patch.
Attachment #8577632 - Attachment is obsolete: true
Timur, thanks for the patch. It is correct. I'm just waiting for inbound to be open to land it.
I created try build. And it has some warnings. But looks like they are not related to the patch. Is it ok? Can I add `checkin-needed` keyword to the bug?

And by the way... could you recommend bugs, in which I can participate? :)
Flags: needinfo?(nsm.nikhil)
Comment on attachment 8579116 [details] [diff] [review]
1136757v0.6.patch

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

::: dom/webidl/FetchEvent.webidl
@@ +17,5 @@
>    readonly attribute Client? client; // The window issuing the request.
>    readonly attribute boolean isReload;
>  
>    [Throws] void respondWith(Promise<Response> r);
> +  [Throws] void respondWith(Response r);

r=me on the IDL change, provided that we fix this later to use a union.
Timur, I will land it for you (right now while the tree is green) so don't bother with checkin-needed. But please file a followup that depends on this as comment 24 says, since the spec has changed while we were on this to use respondWIth(Response or Promise<Response>). And then you could work on that :) if you want. It is a fairly simple fix but lots of spelling changes due to the union change.
Mentor: josh → nsm.nikhil
Flags: needinfo?(nsm.nikhil)
Hi Nikhil! Sorry for misunderstanding...Am I right (as I understand from comment 24) that respondWIth description in the spec will change? I checked latest spec draft (https://slightlyoff.github.io/ServiceWorker/spec/service_worker/#fetch-event-section) and looks it has the same.
Flags: needinfo?(nsm.nikhil)
https://slightlyoff.github.io/ServiceWorker/spec/service_worker/#fetch-event-section
It has become a union (Response or Promise<Response>) instead of the two different functions that this bug implemented. This will lead to some code changes. Specifically codegen will generate only 1 function. On that type you'll have to do something like

if (arg.IsResponse()) {
  nsRefPtr<Response> res = &arg.GetAsResponse(); (i'm just making this syntax up, it may change slightly)
  // what you had done before
} else if (arg.IsPromise()) {
  // Existing Promise handler with minor syntax changes.
}

Is that clear?

See dom/fetch/Fetch.cpp
Flags: needinfo?(nsm.nikhil)
Just change the webidl file to use the union form and then the generated code will make it clearer.
https://hg.mozilla.org/mozilla-central/rev/0f7f9fc972cf
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
Thank you for the explanation, Nikhil! I will try to create new patch that use union in a webidl file.
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.