Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(102)

Issue 1098473003: Implement redirect() API for Fetch Response (Closed)

Created:
5 years, 8 months ago by shiva.jm
Modified:
5 years, 7 months ago
CC:
blink-reviews
Base URL:
https://chromium.googlesource.com/chromium/blink.git@master
Target Ref:
refs/heads/master
Project:
blink
Visibility:
Public.

Description

Implement 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
Unified diffs Side-by-side diffs Delta from patch set Stats (+117 lines, -12 lines) Patch
M LayoutTests/http/tests/fetch/resources/fetch-test-helpers.js View 1 2 3 1 chunk +9 lines, -0 lines 0 comments Download
M LayoutTests/http/tests/fetch/script-tests/headers-guard.js View 1 1 chunk +4 lines, -1 line 0 comments Download
M LayoutTests/http/tests/fetch/script-tests/response.js View 1 2 3 3 chunks +80 lines, -10 lines 3 comments Download
M Source/modules/fetch/Response.h View 1 2 3 1 chunk +1 line, -0 lines 0 comments Download
M Source/modules/fetch/Response.cpp View 1 2 3 1 chunk +22 lines, -0 lines 0 comments Download
M Source/modules/fetch/Response.idl View 1 chunk +1 line, -1 line 0 comments Download

Messages

Total messages: 35 (8 generated)
shiva.jm
Please have a look.
5 years, 8 months ago (2015-04-17 06:58:47 UTC) #2
shiva.jm
5 years, 8 months ago (2015-04-17 07:00:32 UTC) #4
shiva.jm
Please have a look.
5 years, 8 months ago (2015-04-20 04:01:47 UTC) #6
tyoshino (SeeGerritForStatus)
https://codereview.chromium.org/1098473003/diff/1/Source/modules/fetch/Response.cpp File Source/modules/fetch/Response.cpp (right): https://codereview.chromium.org/1098473003/diff/1/Source/modules/fetch/Response.cpp#newcode253 Source/modules/fetch/Response.cpp:253: if (status !=301 && status !=302 && status !=303 ...
5 years, 8 months ago (2015-04-20 04:31:45 UTC) #7
horo
https://codereview.chromium.org/1098473003/diff/1/Source/modules/fetch/Response.cpp File Source/modules/fetch/Response.cpp (right): https://codereview.chromium.org/1098473003/diff/1/Source/modules/fetch/Response.cpp#newcode253 Source/modules/fetch/Response.cpp:253: if (status !=301 && status !=302 && status !=303 ...
5 years, 8 months ago (2015-04-20 04:32:12 UTC) #8
horo
Could you please add LayoutTests which check that Response.redirect() is working well with the requests ...
5 years, 8 months ago (2015-04-20 04:43:14 UTC) #9
tyoshino (SeeGerritForStatus)
https://codereview.chromium.org/1098473003/diff/1/LayoutTests/http/tests/fetch/script-tests/response.js File LayoutTests/http/tests/fetch/script-tests/response.js (right): https://codereview.chromium.org/1098473003/diff/1/LayoutTests/http/tests/fetch/script-tests/response.js#newcode129 LayoutTests/http/tests/fetch/script-tests/response.js:129: 'new Response with status code (' + status + ...
5 years, 8 months ago (2015-04-20 04:50:00 UTC) #10
yhirano
https://codereview.chromium.org/1098473003/diff/1/LayoutTests/http/tests/fetch/script-tests/headers-guard.js File LayoutTests/http/tests/fetch/script-tests/headers-guard.js (right): https://codereview.chromium.org/1098473003/diff/1/LayoutTests/http/tests/fetch/script-tests/headers-guard.js#newcode372 LayoutTests/http/tests/fetch/script-tests/headers-guard.js:372: Response.redirect().headers, Response.redirect throws when |url| is not provided, right? ...
5 years, 8 months ago (2015-04-20 04:55:31 UTC) #11
hiroshige
https://chromiumcodereview.appspot.com/1098473003/diff/1/LayoutTests/http/tests/fetch/script-tests/response.js File LayoutTests/http/tests/fetch/script-tests/response.js (right): https://chromiumcodereview.appspot.com/1098473003/diff/1/LayoutTests/http/tests/fetch/script-tests/response.js#newcode135 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/tests/fetch/script-tests/response.js#newcode138 ...
5 years, 8 months ago (2015-04-20 07:53:53 UTC) #12
shiva.jm
Please have a look. Updated the comments and reviews, soory for the delay, was on ...
5 years, 8 months ago (2015-04-27 10:50:55 UTC) #13
hiroshige
https://codereview.chromium.org/1098473003/diff/1/LayoutTests/http/tests/fetch/script-tests/response.js File LayoutTests/http/tests/fetch/script-tests/response.js (right): https://codereview.chromium.org/1098473003/diff/1/LayoutTests/http/tests/fetch/script-tests/response.js#newcode460 LayoutTests/http/tests/fetch/script-tests/response.js:460: 'Location header should be correct absoulte URL'); On 2015/04/27 ...
5 years, 8 months ago (2015-04-27 11:47:25 UTC) #14
shiva.jm
Please have a look. Updated the comments and reviews. https://codereview.chromium.org/1098473003/diff/20001/LayoutTests/http/tests/fetch/resources/fetch-test-helpers.js File LayoutTests/http/tests/fetch/resources/fetch-test-helpers.js (right): https://codereview.chromium.org/1098473003/diff/20001/LayoutTests/http/tests/fetch/resources/fetch-test-helpers.js#newcode191 LayoutTests/http/tests/fetch/resources/fetch-test-helpers.js:191: ...
5 years, 8 months ago (2015-04-28 06:06:48 UTC) #15
yhirano
lgtm
5 years, 8 months ago (2015-04-28 09:14:36 UTC) #16
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1098473003/40001
5 years, 8 months ago (2015-04-28 09:17:07 UTC) #18
hiroshige
https://codereview.chromium.org/1098473003/diff/20001/LayoutTests/http/tests/fetch/resources/fetch-test-helpers.js File LayoutTests/http/tests/fetch/resources/fetch-test-helpers.js (right): https://codereview.chromium.org/1098473003/diff/20001/LayoutTests/http/tests/fetch/resources/fetch-test-helpers.js#newcode191 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 ...
5 years, 8 months ago (2015-04-28 10:48:23 UTC) #19
shiva.jm
Please have a look. Updated the tests.
5 years, 8 months ago (2015-04-28 11:22:12 UTC) #20
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1098473003/60001
5 years, 8 months ago (2015-04-28 11:22:33 UTC) #23
commit-bot: I haz the power
Dry run: This issue passed the CQ dry run.
5 years, 7 months ago (2015-04-28 12:27:35 UTC) #25
hiroshige
lgtm. Thanks!
5 years, 7 months ago (2015-04-28 13:31:03 UTC) #26
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1098473003/60001
5 years, 7 months ago (2015-04-28 14:54:46 UTC) #28
commit-bot: I haz the power
Committed patchset #4 (id:60001) as https://src.chromium.org/viewvc/blink?view=rev&revision=194605
5 years, 7 months ago (2015-04-28 14:55:22 UTC) #29
tyoshino (SeeGerritForStatus)
https://codereview.chromium.org/1098473003/diff/60001/LayoutTests/http/tests/fetch/script-tests/response.js File LayoutTests/http/tests/fetch/script-tests/response.js (left): https://codereview.chromium.org/1098473003/diff/60001/LayoutTests/http/tests/fetch/script-tests/response.js#oldcode322 LayoutTests/http/tests/fetch/script-tests/response.js:322: assert_equals(blob.size, 0); why these assertions have been removed?
5 years, 7 months ago (2015-04-30 06:14:40 UTC) #30
yhirano
https://codereview.chromium.org/1098473003/diff/60001/LayoutTests/http/tests/fetch/script-tests/response.js File LayoutTests/http/tests/fetch/script-tests/response.js (left): https://codereview.chromium.org/1098473003/diff/60001/LayoutTests/http/tests/fetch/script-tests/response.js#oldcode322 LayoutTests/http/tests/fetch/script-tests/response.js:322: assert_equals(blob.size, 0); On 2015/04/30 06:14:40, tyoshino wrote: > why ...
5 years, 7 months ago (2015-04-30 06:21:17 UTC) #31
tyoshino (SeeGerritForStatus)
On 2015/04/30 06:21:17, yhirano wrote: > https://codereview.chromium.org/1098473003/diff/60001/LayoutTests/http/tests/fetch/script-tests/response.js > File LayoutTests/http/tests/fetch/script-tests/response.js (left): > > https://codereview.chromium.org/1098473003/diff/60001/LayoutTests/http/tests/fetch/script-tests/response.js#oldcode322 > ...
5 years, 7 months ago (2015-04-30 06:26:11 UTC) #32
shiva.jm
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/fetch/script-tests/response.js File ...
5 years, 7 months ago (2015-04-30 06:28:59 UTC) #33
tyoshino (SeeGerritForStatus)
On 2015/04/30 06:28:59, shiva.jm wrote: > shall we make new patch to add these deleted ...
5 years, 7 months ago (2015-04-30 06:31:16 UTC) #34
shiva.jm
5 years, 7 months ago (2015-04-30 06:52:11 UTC) #35
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.

Powered by Google App Engine
This is Rietveld 408576698