Comment on attachment 8635748[details][diff][review]bug1184550_BodyUsedfix.diff
Review of attachment 8635748[details][diff][review]:
-----------------------------------------------------------------
This is the right idea. Can you take a look at the existing tests in test_request.js:
https://dxr.mozilla.org/mozilla-central/source/dom/tests/mochitest/fetch/test_request.js
And add something to verify this behavior? I think something like this should do it:
var req = new Request(url, {body: 'hello'});
fetch(req);
fetch(req); // this should reject with new changes
Or maybe just:
var req = new Request(url, {body: 'hello'});
var req2 = new Request(req);
var req3 = new Request(req); // this should throw with new changes
You can run these tests with:
./mach mochitest dom/tests/mochitest/fetch/test_request.js
Also, just FYI, I'm on travel this week and may be slow to respond to feedback and review requests.
::: dom/fetch/Request.cpp
@@ +138,5 @@
> + if (inputReq->BodyUsed()) {
> + aRv.ThrowTypeError(MSG_FETCH_BODY_CONSUMED_ERROR);
> + return nullptr;
> + } else {
> + inputReq->SetBodyUsed();
I think the SetBodyUsed() call should just happen inside the if (body) check right after inputRequestHasBody = true statement.

Hi Sir,
Initially, without the added test case, all tests passed successfully.
After Addition of these test cases, there was one case which failed:
failed | uncaught exception - TypeError: HEAD or GET Request cannot have a body. at http://mochi.test:8888/tests/dom/tests/mochitest/fetch/test_request.js:460
Is this the expected behaviour? Am I missing something?
Sorry for the late response, I have a presentation in the coming week which is consuming most of my time.
Thanks :)

Comment on attachment 8637892[details][diff][review]bug1184550_AddedTests.diff
Sorry, the sketch of the test I gave you is not 100% complete.
The error you are running into means you need to specify the POST method in the new Request:
var req = new Request("/index.html", {method: 'POST', body: 'hello'});
Also, the test needs to verify that the promise returned from the second fetch rejects. You can do something like this:
var req = new Request("/index.html", {method: 'POST', body: 'hello'});
fetch(req);
return fetch(req).then(function(resp) {
ok(false, 'second fetch with same request should not succeed');
}).catch(function(err) {
is(err.name, 'TypeError', 'second fetch with same request should fail');
});
And then you have to invoke your new test function in the list of async tests in the promise chain.
Hope that helps. Thanks!

(In reply to Ben Kelly [:bkelly] from comment #19)
> Here is a try build:
>
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=3d0776059803
>
The build seems to be stuck at 97%, but the vast majority of builds are successful and none of the errors look to be in the request module or added test. I haven't found any criteria on the Mozilla wiki or Code Firefox videos for determining if a try build is successful or not. Is it just a value judgement?

There was some sort of glitch earlier today that dropped a few builds on the floor, which is probably where your other 3% are. Basically as long as you don't see any relevant patterns in the test failures, you're good to check in. Your tryserver run looks fine to me.

(In reply to Ben Kelly [:bkelly] from comment #24)
> Yep. Everything looks good to me too. Thanks for working this!
Thank you for mentoring me! You made my introduction to contributing to Mozilla a very positive experience. I will definitely continue to contribute, and I hope I get to work with you on other bugs!