Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1258933002:
Make the Response constructor throw when status is a null body status and body is non-null (Closed)
DescriptionMake the Response constructor throw when status is
a null body status (i.e 101, 204, 205, or 304)
and body is non-null
Spec Issue link:
https://fetch.spec.whatwg.org/#dom-response,
https://github.com/whatwg/fetch/issues/86,
https://github.com/whatwg/fetch/issues/94 and
https://github.com/whatwg/fetch/issues/96
BUG=514174
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=199894
Patch Set 1 #
Total comments: 15
Patch Set 2 : #Patch Set 3 : #
Total comments: 3
Patch Set 4 : #Patch Set 5 : #
Total comments: 3
Patch Set 6 : #
Total comments: 4
Patch Set 7 : #Patch Set 8 : #
Total comments: 9
Patch Set 9 : #Patch Set 10 : #
Messages
Total messages: 38 (5 generated)
shiva.jm@samsung.com changed reviewers: + hiroshige@chromium.org, philipj@opera.com, tkent@chromium.org, tyoshino@chromium.org
pls have a look. But for status 101, it will throw range error, as it is covered in step1, so will check with spec author for more clarity on these issue.
https://codereview.chromium.org/1258933002/diff/1/Source/modules/fetch/Respon... File Source/modules/fetch/Response.cpp (right): https://codereview.chromium.org/1258933002/diff/1/Source/modules/fetch/Respon... Source/modules/fetch/Response.cpp:168: exceptionState.throwTypeError("Invalid status"); Let's make this distinguishable with an error in the step 1. How about "body must be null for specified status" or something? https://codereview.chromium.org/1258933002/diff/1/Source/modules/fetch/Respon... Source/modules/fetch/Response.cpp:179: // "3. Let |r| be a new Response object, associated with a new response, please update or remove the step numbers than only changing one at L172.
https://codereview.chromium.org/1258933002/diff/1/LayoutTests/http/tests/fetc... File LayoutTests/http/tests/fetch/script-tests/response.js (right): https://codereview.chromium.org/1258933002/diff/1/LayoutTests/http/tests/fetc... LayoutTests/http/tests/fetch/script-tests/response.js:127: new Response(new Blob(),{status: status}); nit: "new Response(new Blob(), {status: status});" (a space between ',' and '{') https://codereview.chromium.org/1258933002/diff/1/Source/modules/fetch/Respon... File Source/modules/fetch/Response.cpp (right): https://codereview.chromium.org/1258933002/diff/1/Source/modules/fetch/Respon... Source/modules/fetch/Response.cpp:168: exceptionState.throwTypeError("Invalid status"); 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?
https://codereview.chromium.org/1258933002/diff/1/Source/modules/fetch/Respon... File Source/modules/fetch/Response.cpp (left): https://codereview.chromium.org/1258933002/diff/1/Source/modules/fetch/Respon... Source/modules/fetch/Response.cpp:159: exceptionState.throwRangeError("Invalid status"); ExceptionMessages::indexOutsideRange("status", status, 200, ExceptionMessages::InclusiveBound, 599, ExceptionMessages::InclusiveBound) should make this message nicer. https://codereview.chromium.org/1258933002/diff/1/Source/modules/fetch/Respon... File Source/modules/fetch/Response.cpp (right): https://codereview.chromium.org/1258933002/diff/1/Source/modules/fetch/Respon... Source/modules/fetch/Response.cpp:167: if ((status == 101 || status == 204 || status == 205 || status == 304) && body) { 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.
And thanks for following the spec update! Created Issue 514174. Could you refer to it in the CL description?
Updated review comments and tests, pls have a look. https://codereview.chromium.org/1258933002/diff/1/LayoutTests/http/tests/fetc... File LayoutTests/http/tests/fetch/script-tests/response.js (right): https://codereview.chromium.org/1258933002/diff/1/LayoutTests/http/tests/fetc... LayoutTests/http/tests/fetch/script-tests/response.js:127: new Response(new Blob(),{status: status}); On 2015/07/27 11:30:47, hiroshige wrote: > nit: "new Response(new Blob(), {status: status});" > (a space between ',' and '{') Done. https://codereview.chromium.org/1258933002/diff/1/LayoutTests/http/tests/fetc... LayoutTests/http/tests/fetch/script-tests/response.js:127: new Response(new Blob(),{status: status}); On 2015/07/27 11:30:47, hiroshige wrote: > nit: "new Response(new Blob(), {status: status});" > (a space between ',' and '{') Done. https://codereview.chromium.org/1258933002/diff/1/Source/modules/fetch/Respon... File Source/modules/fetch/Response.cpp (right): https://codereview.chromium.org/1258933002/diff/1/Source/modules/fetch/Respon... Source/modules/fetch/Response.cpp:167: if ((status == 101 || status == 204 || status == 205 || status == 304) && body) { On 2015/07/27 11:36:08, philipj wrote: > 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. Done. https://codereview.chromium.org/1258933002/diff/1/Source/modules/fetch/Respon... Source/modules/fetch/Response.cpp:167: if ((status == 101 || status == 204 || status == 205 || status == 304) && body) { On 2015/07/27 11:36:08, philipj wrote: > 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. Done. https://codereview.chromium.org/1258933002/diff/1/Source/modules/fetch/Respon... Source/modules/fetch/Response.cpp:168: exceptionState.throwTypeError("Invalid status"); On 2015/07/27 11:26:52, tyoshino wrote: > Let's make this distinguishable with an error in the step 1. > > How about "body must be null for specified status" or something? Done. https://codereview.chromium.org/1258933002/diff/1/Source/modules/fetch/Respon... Source/modules/fetch/Response.cpp:168: exceptionState.throwTypeError("Invalid status"); On 2015/07/27 11:26:52, tyoshino wrote: > Let's make this distinguishable with an error in the step 1. > > How about "body must be null for specified status" or something? Done. https://codereview.chromium.org/1258933002/diff/1/Source/modules/fetch/Respon... Source/modules/fetch/Response.cpp:168: exceptionState.throwTypeError("Invalid status"); On 2015/07/27 11:30:47, hiroshige wrote: > 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? > Done. https://codereview.chromium.org/1258933002/diff/1/Source/modules/fetch/Respon... Source/modules/fetch/Response.cpp:179: // "3. Let |r| be a new Response object, associated with a new response, On 2015/07/27 11:26:52, tyoshino wrote: > please update or remove the step numbers than only changing one at L172. Done.
On 2015/07/27 11:56:11, shiva.jm wrote: > Updated review comments and tests, pls have a look. Please also see my two comments on PS1.
updated review comments, pls have a look. @philipj: Sorry, i missed the comments at line 159 and 167.
updated issue description, pls have a look. https://codereview.chromium.org/1258933002/diff/1/Source/modules/fetch/Respon... File Source/modules/fetch/Response.cpp (left): https://codereview.chromium.org/1258933002/diff/1/Source/modules/fetch/Respon... Source/modules/fetch/Response.cpp:159: exceptionState.throwRangeError("Invalid status"); On 2015/07/27 11:36:08, philipj wrote: > ExceptionMessages::indexOutsideRange("status", status, 200, > ExceptionMessages::InclusiveBound, 599, ExceptionMessages::InclusiveBound) > should make this message nicer. Done.
https://codereview.chromium.org/1258933002/diff/40001/Source/modules/fetch/Re... File Source/modules/fetch/Response.cpp (right): https://codereview.chromium.org/1258933002/diff/40001/Source/modules/fetch/Re... Source/modules/fetch/Response.cpp:167: // spec link, See https://github.com/whatwg/fetch/issues/86 for details The issue link is already in the description. I meant a static bool isNullBodyStatus(unsigned short status), perhaps documented with https://fetch.spec.whatwg.org/#null-body-status
On 2015/07/28 09:52:51, philipj wrote: > https://codereview.chromium.org/1258933002/diff/40001/Source/modules/fetch/Re... > File Source/modules/fetch/Response.cpp (right): > > https://codereview.chromium.org/1258933002/diff/40001/Source/modules/fetch/Re... > Source/modules/fetch/Response.cpp:167: // spec link, See > https://github.com/whatwg/fetch/issues/86 for details > The issue link is already in the description. I meant a static bool > isNullBodyStatus(unsigned short status), perhaps documented with > https://fetch.spec.whatwg.org/#null-body-status So do we need to add one private function to check null body status i.e like: isNullBodyStatus(unsigned short status) ?. and updated the link to https://fetch.spec.whatwg.org/#null-body-status ?.
On 2015/07/28 11:04:19, shiva.jm wrote: > On 2015/07/28 09:52:51, philipj wrote: > > > https://codereview.chromium.org/1258933002/diff/40001/Source/modules/fetch/Re... > > File Source/modules/fetch/Response.cpp (right): > > > > > https://codereview.chromium.org/1258933002/diff/40001/Source/modules/fetch/Re... > > Source/modules/fetch/Response.cpp:167: // spec link, See > > https://github.com/whatwg/fetch/issues/86 for details > > The issue link is already in the description. I meant a static bool > > isNullBodyStatus(unsigned short status), perhaps documented with > > https://fetch.spec.whatwg.org/#null-body-status > > So do we need to add one private function to check null body status i.e like: > isNullBodyStatus(unsigned short status) ?. > and updated the link to https://fetch.spec.whatwg.org/#null-body-status ?. I'm not an owner of this module, but I would just make it a static function in the implementation file, and make it a member function only if later needed.
updated comments, pls have a look. @tyoshino, hiroshinge, shall we move null body status check to new function like isNullBodyStatus(unsigned short status)?, as per spec, its used only in Main fetch, that will be in different file. https://codereview.chromium.org/1258933002/diff/40001/Source/modules/fetch/Re... File Source/modules/fetch/Response.cpp (right): https://codereview.chromium.org/1258933002/diff/40001/Source/modules/fetch/Re... Source/modules/fetch/Response.cpp:167: // spec link, See https://github.com/whatwg/fetch/issues/86 for details On 2015/07/28 09:52:51, philipj wrote: > The issue link is already in the description. I meant a static bool > isNullBodyStatus(unsigned short status), perhaps documented with > https://fetch.spec.whatwg.org/#null-body-status Done. https://codereview.chromium.org/1258933002/diff/40001/Source/modules/fetch/Re... Source/modules/fetch/Response.cpp:167: // spec link, See https://github.com/whatwg/fetch/issues/86 for details On 2015/07/28 09:52:51, philipj wrote: > The issue link is already in the description. I meant a static bool > isNullBodyStatus(unsigned short status), perhaps documented with > https://fetch.spec.whatwg.org/#null-body-status Done.
On 2015/07/29 04:13:00, shiva.jm wrote: > updated comments, pls have a look. > @tyoshino, hiroshinge, shall we move null body status check to new function like > isNullBodyStatus(unsigned short status)?, > as per spec, its used only in Main fetch, that will be in different file. > > https://codereview.chromium.org/1258933002/diff/40001/Source/modules/fetch/Re... > File Source/modules/fetch/Response.cpp (right): > > https://codereview.chromium.org/1258933002/diff/40001/Source/modules/fetch/Re... > Source/modules/fetch/Response.cpp:167: // spec link, See > https://github.com/whatwg/fetch/issues/86 for details > On 2015/07/28 09:52:51, philipj wrote: > > The issue link is already in the description. I meant a static bool > > isNullBodyStatus(unsigned short status), perhaps documented with > > https://fetch.spec.whatwg.org/#null-body-status > > Done. > > https://codereview.chromium.org/1258933002/diff/40001/Source/modules/fetch/Re... > Source/modules/fetch/Response.cpp:167: // spec link, See > https://github.com/whatwg/fetch/issues/86 for details > On 2015/07/28 09:52:51, philipj wrote: > > The issue link is already in the description. I meant a static bool > > isNullBodyStatus(unsigned short status), perhaps documented with > > https://fetch.spec.whatwg.org/#null-body-status > > Done. Spec has changed again, after pointing out the issue in https://github.com/whatwg/fetch/issues/94, so will update the new patch soon.
Updated the tests and code as per new change in spec, https://github.com/whatwg/fetch/issues/94. pls have a look.
philipj@opera.com changed reviewers: - philipj@opera.com
Removing self as reviewer to leave this to module owners, too many cooks in the kitchen :) https://codereview.chromium.org/1258933002/diff/80001/Source/modules/fetch/Re... File Source/modules/fetch/Response.cpp (right): https://codereview.chromium.org/1258933002/diff/80001/Source/modules/fetch/Re... Source/modules/fetch/Response.cpp:162: exceptionState.throwRangeError(ExceptionMessages::indexOutsideRange<unsigned>("status", status, 200, ExceptionMessages::InclusiveBound, 599, ExceptionMessages::InclusiveBound)); The exception message doesn't make sense with the merged condition.
The spec was changed again: https://github.com/whatwg/fetch/commit/1e0918db0593d3068e4f042225371069b147ed0c > @tyoshino, hiroshinge, shall we move null body status check to new function like > isNullBodyStatus(unsigned short status)?, I think having isNullBodyStatus(unsigned short status) will be better to make code easier to read and to avoid duplicated code. > as per spec, its used only in Main fetch, that will be in different file. We will have to move isNullBodyStatus() to somewhere when we implement the null body status check in main fetch. Perhaps somewhere in core/fetch/, because IIUC the null body check in main fetch affects not only Fetch API but also XHR and other fetching, but not sure currently.
updated the code and test as per latest spec. pls have a look. https://codereview.chromium.org/1258933002/diff/80001/Source/modules/fetch/Re... File Source/modules/fetch/Response.cpp (right): https://codereview.chromium.org/1258933002/diff/80001/Source/modules/fetch/Re... Source/modules/fetch/Response.cpp:162: exceptionState.throwRangeError(ExceptionMessages::indexOutsideRange<unsigned>("status", status, 200, ExceptionMessages::InclusiveBound, 599, ExceptionMessages::InclusiveBound)); On 2015/07/29 09:15:46, philipj wrote: > The exception message doesn't make sense with the merged condition. Done. https://codereview.chromium.org/1258933002/diff/80001/Source/modules/fetch/Re... Source/modules/fetch/Response.cpp:162: exceptionState.throwRangeError(ExceptionMessages::indexOutsideRange<unsigned>("status", status, 200, ExceptionMessages::InclusiveBound, 599, ExceptionMessages::InclusiveBound)); On 2015/07/29 09:15:46, philipj wrote: > The exception message doesn't make sense with the merged condition. Done.
https://codereview.chromium.org/1258933002/diff/100001/Source/modules/fetch/R... File Source/modules/fetch/Response.cpp (right): https://codereview.chromium.org/1258933002/diff/100001/Source/modules/fetch/R... Source/modules/fetch/Response.cpp:160: // spec link, See https://fetch.spec.whatwg.org/#null-body-status for details" According to the latest spec, - null body status check is in Step 7.1, not Step 1. here. - TypeError (not RangeError) must be thrown. https://fetch.spec.whatwg.org/#dom-response https://github.com/whatwg/fetch/commit/1e0918db0593d3068e4f042225371069b147ed0c https://codereview.chromium.org/1258933002/diff/100001/Source/modules/fetch/R... Source/modules/fetch/Response.cpp:161: if (status < 200 || status == 204 || status == 205 || status == 304 || 599 < status) { Please define isNullBodyStatus() in the anonymous name space above and use it.
On 2015/07/29 11:49:11, hiroshige wrote: > https://codereview.chromium.org/1258933002/diff/100001/Source/modules/fetch/R... > File Source/modules/fetch/Response.cpp (right): > > https://codereview.chromium.org/1258933002/diff/100001/Source/modules/fetch/R... > Source/modules/fetch/Response.cpp:160: // spec link, See > https://fetch.spec.whatwg.org/#null-body-status for details" > According to the latest spec, > - null body status check is in Step 7.1, not Step 1. here. > - TypeError (not RangeError) must be thrown. > > https://fetch.spec.whatwg.org/#dom-response > https://github.com/whatwg/fetch/commit/1e0918db0593d3068e4f042225371069b147ed0c > > https://codereview.chromium.org/1258933002/diff/100001/Source/modules/fetch/R... > Source/modules/fetch/Response.cpp:161: if (status < 200 || status == 204 || > status == 205 || status == 304 || 599 < status) { > Please define isNullBodyStatus() in the anonymous name space above and use it. ohh, spec once again got changed just now it seems, ok will update the patch
updated the code and test as per latest spec. pls have a look. https://codereview.chromium.org/1258933002/diff/100001/Source/modules/fetch/R... File Source/modules/fetch/Response.cpp (right): https://codereview.chromium.org/1258933002/diff/100001/Source/modules/fetch/R... Source/modules/fetch/Response.cpp:160: // spec link, See https://fetch.spec.whatwg.org/#null-body-status for details" On 2015/07/29 11:49:11, hiroshige wrote: > According to the latest spec, > - null body status check is in Step 7.1, not Step 1. here. > - TypeError (not RangeError) must be thrown. > > https://fetch.spec.whatwg.org/#dom-response > https://github.com/whatwg/fetch/commit/1e0918db0593d3068e4f042225371069b147ed0c Done. https://codereview.chromium.org/1258933002/diff/100001/Source/modules/fetch/R... Source/modules/fetch/Response.cpp:161: if (status < 200 || status == 204 || status == 205 || status == 304 || 599 < status) { On 2015/07/29 11:49:11, hiroshige wrote: > Please define isNullBodyStatus() in the anonymous name space above and use it. Done.
https://codereview.chromium.org/1258933002/diff/140001/LayoutTests/http/tests... File LayoutTests/http/tests/fetch/script-tests/response.js (right): https://codereview.chromium.org/1258933002/diff/140001/LayoutTests/http/tests... LayoutTests/http/tests/fetch/script-tests/response.js:131: }); Could you add a test for that 101 + body throws RangeError, not TypeError by null body status? https://codereview.chromium.org/1258933002/diff/140001/Source/modules/fetch/R... File Source/modules/fetch/Response.cpp (right): https://codereview.chromium.org/1258933002/diff/140001/Source/modules/fetch/R... Source/modules/fetch/Response.cpp:22: // Check whether |status| is a null body status nit: s/Check/Checks/, and append a period at the last of the statement. http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Function_Comm... > These comments should be descriptive ("Opens the file") rather than imperative ("Open the file") https://codereview.chromium.org/1258933002/diff/140001/Source/modules/fetch/R... Source/modules/fetch/Response.cpp:23: // Spec: https://fetch.spec.whatwg.org/#statuses nit: https://fetch.spec.whatwg.org/#null-body-status is better. https://codereview.chromium.org/1258933002/diff/140001/Source/modules/fetch/R... Source/modules/fetch/Response.cpp:35: Please move isNullBodyStatus() here (or somewhere in "namespace blink { namespace { ..."), rather than exposing it in the global namespace. https://codereview.chromium.org/1258933002/diff/140001/Source/modules/fetch/R... Source/modules/fetch/Response.cpp:171: exceptionState.throwRangeError("Invalid status"); How about applying this? https://codereview.chromium.org/1258933002/diff/1/Source/modules/fetch/Respon... On 2015/07/27 11:36:08, philipj wrote: > ExceptionMessages::indexOutsideRange("status", status, 200, > ExceptionMessages::InclusiveBound, 599, ExceptionMessages::InclusiveBound) > should make this message nicer.
updated tests and comments, pls have a look. https://codereview.chromium.org/1258933002/diff/140001/LayoutTests/http/tests... File LayoutTests/http/tests/fetch/script-tests/response.js (right): https://codereview.chromium.org/1258933002/diff/140001/LayoutTests/http/tests... LayoutTests/http/tests/fetch/script-tests/response.js:131: }); On 2015/07/30 11:20:40, hiroshige wrote: > Could you add a test for that 101 + body throws RangeError, not TypeError by > null body status? Done. https://codereview.chromium.org/1258933002/diff/140001/Source/modules/fetch/R... File Source/modules/fetch/Response.cpp (right): https://codereview.chromium.org/1258933002/diff/140001/Source/modules/fetch/R... Source/modules/fetch/Response.cpp:23: // Spec: https://fetch.spec.whatwg.org/#statuses On 2015/07/30 11:20:40, hiroshige wrote: > nit: https://fetch.spec.whatwg.org/#null-body-status is better. Done. https://codereview.chromium.org/1258933002/diff/140001/Source/modules/fetch/R... Source/modules/fetch/Response.cpp:35: On 2015/07/30 11:20:40, hiroshige wrote: > Please move isNullBodyStatus() here (or somewhere in "namespace blink { > namespace { ..."), rather than exposing it in the global namespace. Done. https://codereview.chromium.org/1258933002/diff/140001/Source/modules/fetch/R... Source/modules/fetch/Response.cpp:171: exceptionState.throwRangeError("Invalid status"); On 2015/07/30 11:20:40, hiroshige wrote: > How about applying this? > https://codereview.chromium.org/1258933002/diff/1/Source/modules/fetch/Respon... > On 2015/07/27 11:36:08, philipj wrote: > > ExceptionMessages::indexOutsideRange("status", status, 200, > > ExceptionMessages::InclusiveBound, 599, ExceptionMessages::InclusiveBound) > > should make this message nicer. Done.
updated now as per spec, pls have a look.
lgtm. thanks! optional: including the word "null body status" in the CL description might be better, e.g. "Make the Response constructor throw when status is a null body status and body is non-null"
The CQ bit was checked by shiva.jm@samsung.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1258933002/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1258933002/160001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: blink_presubmit on tryserver.blink (JOB_FAILED, http://build.chromium.org/p/tryserver.blink/builders/blink_presubmit/builds/3...) linux_chromium_gn_rel on tryserver.blink (JOB_FAILED, http://build.chromium.org/p/tryserver.blink/builders/linux_chromium_gn_rel/bu...) mac_blink_compile_dbg on tryserver.blink (JOB_FAILED, http://build.chromium.org/p/tryserver.blink/builders/mac_blink_compile_dbg/bu...)
Updated patch to latest code, due to merge issue, pls have a look.
On 2015/08/03 10:38:21, shiva.jm wrote: > Updated patch to latest code, due to merge issue, pls have a look. lgtm PS10.
The CQ bit was checked by shiva.jm@samsung.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1258933002/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1258933002/180001
lgtm
Message was sent while issue was closed.
Committed patchset #10 (id:180001) as https://src.chromium.org/viewvc/blink?view=rev&revision=199894 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
