Make the Response constructor throw when status is a null body status and body is non-null

Source/modules/fetch/Response.cpp

 // Copyright 2014 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "config.h"
 #include "modules/fetch/Response.h"
 
 #include "bindings/core/v8/Dictionary.h"
 #include "bindings/core/v8/ExceptionState.h"
 #include "core/dom/DOMArrayBuffer.h"
@@ skipping 135 matching lines @@
         const long long length = blobData->length();
         Blob* blob = Blob::create(BlobDataHandle::create(blobData.release(), length));
         return create(context, blob, ResponseInit(responseInit, exceptionState), exceptionState);
     }
     ASSERT_NOT_REACHED();
     return nullptr;
 }
 
 Response* Response::create(ExecutionContext* context, Blob* body, const ResponseInit& responseInit, ExceptionState& exceptionState)
 {
+    unsigned short status = responseInit.status;
+
     // "1. If |init|'s status member is not in the range 200 to 599, throw a
     // RangeError."
-    if (responseInit.status < 200 || 599 < responseInit.status) {
+    if (status < 200 || 599 < status) {
         exceptionState.throwRangeError("Invalid status");

[philipj_slow, 2015/07/27 11:36:08]

ExceptionMessages::indexOutsideRange("status", status, 200,
ExceptionMessages::InclusiveBound, 599, ExceptionMessages::InclusiveBound)
should make this message nicer.

[shiva.jm, 2015/07/28 09:17:27]

Done.

         return 0;
     }
 
-    // "2. If |init|'s statusText member does not match the Reason-Phrase
+    // "2. If init's status member is a null body status and body is non-null,
+    // throw a TypeError.
+    if ((status == 101 || status == 204 || status == 205 || status == 304) && body) {

[philipj_slow, 2015/07/27 11:36:08]

A helper for this would make it more obvious, as one cannot follow the link to
"null body status" here. It's also used in two places in the spec, so presumably
there's another place that could use the same helper.

[shiva.jm, 2015/07/27 11:56:11]

Done.

[shiva.jm, 2015/07/27 11:56:11]

Done.

+        exceptionState.throwTypeError("Invalid status");

[tyoshino (SeeGerritForStatus), 2015/07/27 11:26:52]

Let's make this distinguishable with an error in the step 1.

How about "body must be null for specified status" or something?

[hiroshige, 2015/07/27 11:30:47]

I'd like to make the message here more informative and different from the one in
L161.
How about "Response with null body status cannot have body" or so?

[shiva.jm, 2015/07/27 11:56:11]

Done.

[shiva.jm, 2015/07/27 11:56:11]

Done.

[shiva.jm, 2015/07/27 11:56:11]

Done.

+        return 0;
+    }
+
+    // "3. If |init|'s statusText member does not match the Reason-Phrase
     // token production, throw a TypeError."
     if (!isValidReasonPhrase(responseInit.statusText)) {
         exceptionState.throwTypeError("Invalid statusText");
         return 0;
     }
 
     // "3. Let |r| be a new Response object, associated with a new response,

[tyoshino (SeeGerritForStatus), 2015/07/27 11:26:52]

please update or remove the step numbers than only changing one at L172.

[shiva.jm, 2015/07/27 11:56:11]

Done.

     // Headers object, and Body object."
     Response* r = new Response(context);
     r->suspendIfNeeded();
 
     // "4. Set |r|'s response's status to |init|'s status member."
     r->m_response->setStatus(responseInit.status);
 
     // "5. Set |r|'s response's status message to |init|'s statusText member."
     r->m_response->setStatusMessage(AtomicString(responseInit.statusText));
 
@@ skipping 253 matching lines @@
 }
 
 DEFINE_TRACE(Response)
 {
     Body::trace(visitor);
     visitor->trace(m_response);
     visitor->trace(m_headers);
 }
 
 } // namespace blink