Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1098473003:
Implement redirect() API for Fetch Response (Closed)
DescriptionImplement redirect() API for Fetch Response
BUG=455114
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=194605
Patch Set 1 #
Total comments: 25
Patch Set 2 : #
Total comments: 9
Patch Set 3 : #Patch Set 4 : #
Total comments: 3
Messages
Total messages: 35 (8 generated)
shiva.jm@samsung.com changed reviewers: + a.cavalcanti@samsung.com, horo@chromium.org, jsbell@chromium.org, tyoshino@chromium.org
Please have a look.
shiva.jm@samsung.com changed reviewers: + hiroshige@chromium.org
shiva.jm@samsung.com changed reviewers: + yhirano@chromium.org
Please have a look.
https://codereview.chromium.org/1098473003/diff/1/Source/modules/fetch/Respon... File Source/modules/fetch/Response.cpp (right): https://codereview.chromium.org/1098473003/diff/1/Source/modules/fetch/Respon... Source/modules/fetch/Response.cpp:253: if (status !=301 && status !=302 && status !=303 && status !=307 && status !=308) { put a space between != and the second operand
https://codereview.chromium.org/1098473003/diff/1/Source/modules/fetch/Respon... File Source/modules/fetch/Response.cpp (right): https://codereview.chromium.org/1098473003/diff/1/Source/modules/fetch/Respon... Source/modules/fetch/Response.cpp:253: if (status !=301 && status !=302 && status !=303 && status !=307 && status !=308) { spaces after "!=" if (status != 301 && status != 302 && status != 303 && status != 307 && status != 308) {
Could you please add LayoutTests which check that Response.redirect() is working well with the requests from iframe, XHR, fetch API and <script> tag?
https://codereview.chromium.org/1098473003/diff/1/LayoutTests/http/tests/fetc... File LayoutTests/http/tests/fetch/script-tests/response.js (right): https://codereview.chromium.org/1098473003/diff/1/LayoutTests/http/tests/fetc... LayoutTests/http/tests/fetch/script-tests/response.js:129: 'new Response with status code (' + status + use the same text as L120 or update the line, too. https://codereview.chromium.org/1098473003/diff/1/Source/modules/fetch/Respon... File Source/modules/fetch/Response.cpp (right): https://codereview.chromium.org/1098473003/diff/1/Source/modules/fetch/Respon... Source/modules/fetch/Response.cpp:254: exceptionState.throwRangeError("Invalid statusCode"); statusCode -> status, since the argument name in the spec is "status". Or "statusCode" -> "status code" if you want to emphasize it's status code.
https://codereview.chromium.org/1098473003/diff/1/LayoutTests/http/tests/fetc... File LayoutTests/http/tests/fetch/script-tests/headers-guard.js (right): https://codereview.chromium.org/1098473003/diff/1/LayoutTests/http/tests/fetc... LayoutTests/http/tests/fetch/script-tests/headers-guard.js:372: Response.redirect().headers, Response.redirect throws when |url| is not provided, right? https://codereview.chromium.org/1098473003/diff/1/Source/modules/fetch/Respon... File Source/modules/fetch/Response.cpp (right): https://codereview.chromium.org/1098473003/diff/1/Source/modules/fetch/Respon... Source/modules/fetch/Response.cpp:260: r->suspendIfNeeded(); Please call |suspendIfNeeded| right after creation.
https://chromiumcodereview.appspot.com/1098473003/diff/1/LayoutTests/http/tes... File LayoutTests/http/tests/fetch/script-tests/response.js (right): https://chromiumcodereview.appspot.com/1098473003/diff/1/LayoutTests/http/tes... LayoutTests/http/tests/fetch/script-tests/response.js:135: function() { -1 indent (L135, L136, L142, L143). https://chromiumcodereview.appspot.com/1098473003/diff/1/LayoutTests/http/tes... LayoutTests/http/tests/fetch/script-tests/response.js:138: 'new Response with invalid URL http:// should throw'); The message (http://) and the test code (https://) don't match. s/new Response/Response.redirect()/g? (here, L129, and L145). Also, the assert message here (L138) and in L145 are the same. Could you make these different? (to distinguish assert failures and to describe what the tests do more precisely). https://chromiumcodereview.appspot.com/1098473003/diff/1/LayoutTests/http/tes... LayoutTests/http/tests/fetch/script-tests/response.js:139: Could you add a test with invalid URL and invalid status? (i.e. a copy of L133-L138, with e.g. 300) This is to test that redirect() checks the URL first and throws TypeError (not RangeError) if both URL and status are invalid. https://chromiumcodereview.appspot.com/1098473003/diff/1/LayoutTests/http/tes... LayoutTests/http/tests/fetch/script-tests/response.js:145: 'new Response with invalid URL http:// should throw'); Could you add tests of invalid URLs that contains \x00, \x0a, or \x0d? I think URL parser https://url.spec.whatwg.org/#concept-url-parser returns failure for such URLs but not sure...but anyway we have to reject such URLs to avoid to set a Location header with invalid characters. https://chromiumcodereview.appspot.com/1098473003/diff/1/LayoutTests/http/tes... LayoutTests/http/tests/fetch/script-tests/response.js:460: 'Location header should be correct absoulte URL'); Could you add assertion that response.headers has only one header? https://chromiumcodereview.appspot.com/1098473003/diff/1/LayoutTests/http/tes... LayoutTests/http/tests/fetch/script-tests/response.js:474: }, 'Response.redirect()'); optional: How about 'Response.redirect() with 301'?
Please have a look. Updated the comments and reviews, soory for the delay, was on leave for couple of days. https://codereview.chromium.org/1098473003/diff/1/LayoutTests/http/tests/fetc... File LayoutTests/http/tests/fetch/script-tests/headers-guard.js (right): https://codereview.chromium.org/1098473003/diff/1/LayoutTests/http/tests/fetc... LayoutTests/http/tests/fetch/script-tests/headers-guard.js:372: Response.redirect().headers, On 2015/04/20 04:55:30, yhirano wrote: > Response.redirect throws when |url| is not provided, right? Done. added valid URL to correct tests. https://codereview.chromium.org/1098473003/diff/1/LayoutTests/http/tests/fetc... File LayoutTests/http/tests/fetch/script-tests/response.js (right): https://codereview.chromium.org/1098473003/diff/1/LayoutTests/http/tests/fetc... LayoutTests/http/tests/fetch/script-tests/response.js:129: 'new Response with status code (' + status + On 2015/04/20 04:50:00, tyoshino wrote: > use the same text as L120 or update the line, too. Done. changed both line to wrap to 80 columns. https://codereview.chromium.org/1098473003/diff/1/LayoutTests/http/tests/fetc... LayoutTests/http/tests/fetch/script-tests/response.js:135: function() { On 2015/04/20 07:53:53, hiroshige wrote: > -1 indent (L135, L136, L142, L143). Done. https://codereview.chromium.org/1098473003/diff/1/LayoutTests/http/tests/fetc... LayoutTests/http/tests/fetch/script-tests/response.js:138: 'new Response with invalid URL http:// should throw'); On 2015/04/20 07:53:52, hiroshige wrote: > The message (http://) and the test code (https://) don't match. > s/new Response/Response.redirect()/g? (here, L129, and L145). > > Also, the assert message here (L138) and in L145 are the same. Could you make > these different? (to distinguish assert failures and to describe what the tests > do more precisely). Done. https://codereview.chromium.org/1098473003/diff/1/LayoutTests/http/tests/fetc... LayoutTests/http/tests/fetch/script-tests/response.js:139: On 2015/04/20 07:53:52, hiroshige wrote: > Could you add a test with invalid URL and invalid status? > (i.e. a copy of L133-L138, with e.g. 300) > This is to test that redirect() checks the URL first and throws TypeError (not > RangeError) if both URL and status are invalid. Done. https://codereview.chromium.org/1098473003/diff/1/LayoutTests/http/tests/fetc... LayoutTests/http/tests/fetch/script-tests/response.js:145: 'new Response with invalid URL http:// should throw'); On 2015/04/20 07:53:53, hiroshige wrote: > Could you add tests of invalid URLs that contains \x00, \x0a, or \x0d? > I think URL parser https://url.spec.whatwg.org/#concept-url-parser returns > failure for such URLs but not sure...but anyway we have to reject such URLs to > avoid to set a Location header with invalid characters. Done. https://codereview.chromium.org/1098473003/diff/1/LayoutTests/http/tests/fetc... LayoutTests/http/tests/fetch/script-tests/response.js:460: 'Location header should be correct absoulte URL'); On 2015/04/20 07:53:53, hiroshige wrote: > Could you add assertion that response.headers has only one header? Done. https://codereview.chromium.org/1098473003/diff/1/LayoutTests/http/tests/fetc... LayoutTests/http/tests/fetch/script-tests/response.js:474: }, 'Response.redirect()'); On 2015/04/20 07:53:52, hiroshige wrote: > optional: How about 'Response.redirect() with 301'? Done. https://codereview.chromium.org/1098473003/diff/1/Source/modules/fetch/Respon... File Source/modules/fetch/Response.cpp (right): https://codereview.chromium.org/1098473003/diff/1/Source/modules/fetch/Respon... Source/modules/fetch/Response.cpp:253: if (status !=301 && status !=302 && status !=303 && status !=307 && status !=308) { On 2015/04/20 04:32:12, horo wrote: > spaces after "!=" > > if (status != 301 && status != 302 && status != 303 && status != 307 && status > != 308) { Done. https://codereview.chromium.org/1098473003/diff/1/Source/modules/fetch/Respon... Source/modules/fetch/Response.cpp:253: if (status !=301 && status !=302 && status !=303 && status !=307 && status !=308) { On 2015/04/20 04:31:44, tyoshino wrote: > put a space between != and the second operand Done. https://codereview.chromium.org/1098473003/diff/1/Source/modules/fetch/Respon... Source/modules/fetch/Response.cpp:254: exceptionState.throwRangeError("Invalid statusCode"); On 2015/04/20 04:50:00, tyoshino wrote: > statusCode -> status, since the argument name in the spec is "status". > > Or "statusCode" -> "status code" if you want to emphasize it's status code. Done. changed to "status code" https://codereview.chromium.org/1098473003/diff/1/Source/modules/fetch/Respon... Source/modules/fetch/Response.cpp:260: r->suspendIfNeeded(); On 2015/04/20 04:55:30, yhirano wrote: > Please call |suspendIfNeeded| right after creation. Done.
https://codereview.chromium.org/1098473003/diff/1/LayoutTests/http/tests/fetc... File LayoutTests/http/tests/fetch/script-tests/response.js (right): https://codereview.chromium.org/1098473003/diff/1/LayoutTests/http/tests/fetc... LayoutTests/http/tests/fetch/script-tests/response.js:460: 'Location header should be correct absoulte URL'); On 2015/04/27 10:50:54, shiva.jm wrote: > On 2015/04/20 07:53:53, hiroshige wrote: > > Could you add assertion that response.headers has only one header? > > Done. Not added? e.g. assert_equals(size(response.headers), 1, 'Respons.redirect().headers must contain ' + 'a Location header only'); https://codereview.chromium.org/1098473003/diff/20001/LayoutTests/http/tests/... File LayoutTests/http/tests/fetch/resources/fetch-test-helpers.js (right): https://codereview.chromium.org/1098473003/diff/20001/LayoutTests/http/tests/... LayoutTests/http/tests/fetch/resources/fetch-test-helpers.js:191: 'http://ex%08ample.com']; Could you also add the following? (I'd like to test the invalid characters directly embedded in the strings) 'http://ex\x00ample.com' 'http://ex\x0dample.com' 'http://ex\x0aample.com' https://codereview.chromium.org/1098473003/diff/20001/LayoutTests/http/tests/... File LayoutTests/http/tests/fetch/script-tests/response.js (right): https://codereview.chromium.org/1098473003/diff/20001/LayoutTests/http/tests/... LayoutTests/http/tests/fetch/script-tests/response.js:140: 'status as 301 should throw'); nit: ' and status 301 should throw'. - space after https:// - inserting "and" https://codereview.chromium.org/1098473003/diff/20001/LayoutTests/http/tests/... LayoutTests/http/tests/fetch/script-tests/response.js:148: 'Response.redirect() with invalid URL' + url + nit: 'Response.redirect() with invalid URL ' + url + ' and default status value should throw'. - spaces before/after url - inserting "and" https://codereview.chromium.org/1098473003/diff/20001/LayoutTests/http/tests/... LayoutTests/http/tests/fetch/script-tests/response.js:158: 'invalid status 300 should throw'); nit: ' and invalid status 300 should throw TypeError' - spaces before/after url - inserting "and" - inserting "TypeError" (because this shouldn't throw RangeError)
Please have a look. Updated the comments and reviews. https://codereview.chromium.org/1098473003/diff/20001/LayoutTests/http/tests/... File LayoutTests/http/tests/fetch/resources/fetch-test-helpers.js (right): https://codereview.chromium.org/1098473003/diff/20001/LayoutTests/http/tests/... LayoutTests/http/tests/fetch/resources/fetch-test-helpers.js:191: 'http://ex%08ample.com']; On 2015/04/27 11:47:25, hiroshige wrote: > Could you also add the following? (I'd like to test the invalid characters > directly embedded in the strings) > 'http://ex\x00ample.com' > 'http://ex\x0dample.com' > 'http://ex\x0aample.com' > when tried with 'http://ex\x0dample.com', it did not throw the error. But with 'http://ex\0xdample.com', it give error. https://codereview.chromium.org/1098473003/diff/20001/LayoutTests/http/tests/... File LayoutTests/http/tests/fetch/script-tests/response.js (right): https://codereview.chromium.org/1098473003/diff/20001/LayoutTests/http/tests/... LayoutTests/http/tests/fetch/script-tests/response.js:140: 'status as 301 should throw'); On 2015/04/27 11:47:25, hiroshige wrote: > nit: ' and status 301 should throw'. > - space after https:// > - inserting "and" Done. https://codereview.chromium.org/1098473003/diff/20001/LayoutTests/http/tests/... LayoutTests/http/tests/fetch/script-tests/response.js:148: 'Response.redirect() with invalid URL' + url + On 2015/04/27 11:47:25, hiroshige wrote: > nit: 'Response.redirect() with invalid URL ' + url + > ' and default status value should throw'. > - spaces before/after url > - inserting "and" Done. https://codereview.chromium.org/1098473003/diff/20001/LayoutTests/http/tests/... LayoutTests/http/tests/fetch/script-tests/response.js:158: 'invalid status 300 should throw'); On 2015/04/27 11:47:25, hiroshige wrote: > nit: ' and invalid status 300 should throw TypeError' > - spaces before/after url > - inserting "and" > - inserting "TypeError" (because this shouldn't throw RangeError) Done.
lgtm
The CQ bit was checked by shiva.jm@samsung.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1098473003/40001
https://codereview.chromium.org/1098473003/diff/20001/LayoutTests/http/tests/... File LayoutTests/http/tests/fetch/resources/fetch-test-helpers.js (right): https://codereview.chromium.org/1098473003/diff/20001/LayoutTests/http/tests/... LayoutTests/http/tests/fetch/resources/fetch-test-helpers.js:191: 'http://ex%08ample.com']; On 2015/04/28 06:06:48, shiva.jm wrote: > On 2015/04/27 11:47:25, hiroshige wrote: > > Could you also add the following? (I'd like to test the invalid characters > > directly embedded in the strings) > > 'http://ex\x00ample.com' > > 'http://ex\x0dample.com' > > 'http://ex\x0aample.com' > > > > when tried with 'http://ex\x0dample.com', it did not throw the error. But with > 'http://ex\0xdample.com', it give error. Hmm. 'http://ex\x00ample.com' throws TypeError. 'http://ex\x0dample.com' and 'http://ex\x0aample.com' don't throw exceptions, because they are normalized to 'http://example.com' (by DoRemoveURLWhitespace()?) https://code.google.com/p/chromium/codesearch#chromium/src/url/url_canon_etc.... Could you add 'http://ex\x00ample.com' to INVALID_URLS, and tests for another two in response.js, like: test(function() { ['http://ex\x0aample.com', 'http://ex\x0dample.com'].forEach(function(url) { assert_equals(Response.redirect(url).headers.get('Location'), 'http://example.com/', 'Location header value must not contain CR or LF'); }); }, 'Response.redirect() with URLs with CR or LF'); I'm not sure what to do for those URLs in the spec, but checking header values do not contain CR/LF/null byte will be useful for us.
Please have a look. Updated the tests.
The CQ bit was checked by shiva.jm@samsung.com to run a CQ dry run
The patchset sent to the CQ was uploaded after l-g-t-m from yhirano@chromium.org Link to the patchset: https://codereview.chromium.org/1098473003/#ps60001 (title: " ")
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1098473003/60001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm. Thanks!
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/1098473003/60001
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://src.chromium.org/viewvc/blink?view=rev&revision=194605
Message was sent while issue was closed.
https://codereview.chromium.org/1098473003/diff/60001/LayoutTests/http/tests/... File LayoutTests/http/tests/fetch/script-tests/response.js (left): https://codereview.chromium.org/1098473003/diff/60001/LayoutTests/http/tests/... LayoutTests/http/tests/fetch/script-tests/response.js:322: assert_equals(blob.size, 0); why these assertions have been removed?
Message was sent while issue was closed.
https://codereview.chromium.org/1098473003/diff/60001/LayoutTests/http/tests/... File LayoutTests/http/tests/fetch/script-tests/response.js (left): https://codereview.chromium.org/1098473003/diff/60001/LayoutTests/http/tests/... LayoutTests/http/tests/fetch/script-tests/response.js:322: assert_equals(blob.size, 0); On 2015/04/30 06:14:40, tyoshino wrote: > why these assertions have been removed? Maybe it's a rebase failure. I added the test at https://codereview.chromium.org/1068783005/.
Message was sent while issue was closed.
On 2015/04/30 06:21:17, yhirano wrote: > https://codereview.chromium.org/1098473003/diff/60001/LayoutTests/http/tests/... > File LayoutTests/http/tests/fetch/script-tests/response.js (left): > > https://codereview.chromium.org/1098473003/diff/60001/LayoutTests/http/tests/... > LayoutTests/http/tests/fetch/script-tests/response.js:322: > assert_equals(blob.size, 0); > On 2015/04/30 06:14:40, tyoshino wrote: > > why these assertions have been removed? > > Maybe it's a rebase failure. I added the test at > https://codereview.chromium.org/1068783005/. OK. I'll revive it.
Message was sent while issue was closed.
shall we make new patch to add these deleted changes from https://codereview.chromium.org/1068783005 ?. https://codereview.chromium.org/1098473003/diff/60001/LayoutTests/http/tests/... File LayoutTests/http/tests/fetch/script-tests/response.js (left): https://codereview.chromium.org/1098473003/diff/60001/LayoutTests/http/tests/... LayoutTests/http/tests/fetch/script-tests/response.js:322: assert_equals(blob.size, 0); On 2015/04/30 06:21:17, yhirano wrote: > On 2015/04/30 06:14:40, tyoshino wrote: > > why these assertions have been removed? > > Maybe it's a rebase failure. I added the test at > https://codereview.chromium.org/1068783005/. shall we make new patch to add these deleted changes from https://codereview.chromium.org/1068783005 ?.
Message was sent while issue was closed.
On 2015/04/30 06:28:59, shiva.jm wrote: > shall we make new patch to add these deleted changes from > https://codereview.chromium.org/1068783005 ?. > > https://codereview.chromium.org/1098473003/diff/60001/LayoutTests/http/tests/... > File LayoutTests/http/tests/fetch/script-tests/response.js (left): > > https://codereview.chromium.org/1098473003/diff/60001/LayoutTests/http/tests/... > LayoutTests/http/tests/fetch/script-tests/response.js:322: > assert_equals(blob.size, 0); > On 2015/04/30 06:21:17, yhirano wrote: > > On 2015/04/30 06:14:40, tyoshino wrote: > > > why these assertions have been removed? > > > > Maybe it's a rebase failure. I added the test at > > https://codereview.chromium.org/1068783005/. > > shall we make new patch to add these deleted changes from > https://codereview.chromium.org/1068783005 ?. Yes. Just re-adding should be fine. Created https://codereview.chromium.org/1109423005/
Message was sent while issue was closed.
On 2015/04/30 06:31:16, tyoshino wrote: > On 2015/04/30 06:28:59, shiva.jm wrote: > > shall we make new patch to add these deleted changes from > > https://codereview.chromium.org/1068783005 ?. > > > > > https://codereview.chromium.org/1098473003/diff/60001/LayoutTests/http/tests/... > > File LayoutTests/http/tests/fetch/script-tests/response.js (left): > > > > > https://codereview.chromium.org/1098473003/diff/60001/LayoutTests/http/tests/... > > LayoutTests/http/tests/fetch/script-tests/response.js:322: > > assert_equals(blob.size, 0); > > On 2015/04/30 06:21:17, yhirano wrote: > > > On 2015/04/30 06:14:40, tyoshino wrote: > > > > why these assertions have been removed? > > > > > > Maybe it's a rebase failure. I added the test at > > > https://codereview.chromium.org/1068783005/. > > > > shall we make new patch to add these deleted changes from > > https://codereview.chromium.org/1068783005 ?. > > Yes. Just re-adding should be fine. > > Created https://codereview.chromium.org/1109423005/ @tyoshino, thanks, will check for rebase failures for future patches. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
