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

Issue 1288263003: Normalize and update the header value checks to RFC 7230 for Fetch

Created:
5 years, 4 months ago by shiva.jm
Modified:
4 years, 2 months ago
CC:
Habib Virji
Base URL:
https://chromium.googlesource.com/chromium/blink.git@master
Target Ref:
refs/heads/master
Project:
blink
Visibility:
Public.

Description

Normalize and update the header value checks to RFC 7230 for Fetch. Normalize the Header value for append() and set() api. To normalize a value, remove any leading and trailing HTTP whitespace bytes from it. HTTP whitespace bytes are 0x09, 0x0A, 0x0D, and 0x20. Intent to update header is: https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/K5jd8Y5l6pI Spec link: https://fetch.spec.whatwg.org/#concept-header-value-normalize BUG=455099

Patch Set 1 #

Patch Set 2 : #

Patch Set 3 : #

Patch Set 4 : #

Patch Set 5 : #

Patch Set 6 : #

Total comments: 16

Patch Set 7 : #

Patch Set 8 : #

Total comments: 4

Patch Set 9 : #

Patch Set 10 : #

Unified diffs Side-by-side diffs Delta from patch set Stats (+66 lines, -26 lines) Patch
M LayoutTests/http/tests/fetch/resources/fetch-test-helpers.js View 1 2 3 4 5 6 7 8 9 1 chunk +6 lines, -3 lines 0 comments Download
M LayoutTests/http/tests/fetch/script-tests/headers.js View 1 2 3 4 5 6 7 1 chunk +31 lines, -0 lines 0 comments Download
M Source/modules/fetch/FetchHeaderList.h View 1 2 3 4 5 6 7 1 chunk +1 line, -1 line 0 comments Download
M Source/modules/fetch/FetchHeaderList.cpp View 1 2 3 4 5 6 7 1 chunk +6 lines, -4 lines 0 comments Download
M Source/modules/fetch/Headers.cpp View 1 2 3 4 5 6 7 8 2 chunks +22 lines, -18 lines 0 comments Download

Messages

Total messages: 28 (9 generated)
shiva.jm
pls have a look.
5 years, 3 months ago (2015-09-08 14:18:16 UTC) #2
yhirano
IIUC this CL also changes the way to check header values, right? If so, please ...
5 years, 3 months ago (2015-09-09 04:41:50 UTC) #3
hiroshige
How about mentioning in CL title/description that this CL also updates the header value check ...
5 years, 3 months ago (2015-09-09 07:01:48 UTC) #4
shiva.jm
updated patch with comments, pls have a look. https://codereview.chromium.org/1288263003/diff/100001/LayoutTests/http/tests/fetch/resources/fetch-test-helpers.js File LayoutTests/http/tests/fetch/resources/fetch-test-helpers.js (right): https://codereview.chromium.org/1288263003/diff/100001/LayoutTests/http/tests/fetch/resources/fetch-test-helpers.js#newcode82 LayoutTests/http/tests/fetch/resources/fetch-test-helpers.js:82: 'test\r\n ...
5 years, 3 months ago (2015-09-10 10:10:27 UTC) #5
shiva.jm
5 years, 3 months ago (2015-09-10 10:11:45 UTC) #7
shiva.jm
updated the description with intent link.
5 years, 3 months ago (2015-09-10 14:12:12 UTC) #8
yhirano
LGTM, but please wait for the intent-to-deprecate-to-remove to be approved. https://codereview.chromium.org/1288263003/diff/140001/Source/modules/fetch/Headers.cpp File Source/modules/fetch/Headers.cpp (right): https://codereview.chromium.org/1288263003/diff/140001/Source/modules/fetch/Headers.cpp#newcode109 ...
5 years, 3 months ago (2015-09-11 04:22:25 UTC) #9
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1288263003/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1288263003/140001
5 years, 3 months ago (2015-09-11 04:34:19 UTC) #11
commit-bot: I haz the power
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_ng/builds/105779)
5 years, 3 months ago (2015-09-11 07:26:25 UTC) #13
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1288263003/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1288263003/140001
5 years, 3 months ago (2015-09-11 07:43:08 UTC) #15
commit-bot: I haz the power
Dry run: This issue passed the CQ dry run.
5 years, 3 months ago (2015-09-11 08:31:57 UTC) #17
shiva.jm
Updated the patch with review comments. From the intent discussion, it looks like we have ...
5 years, 3 months ago (2015-09-14 03:41:48 UTC) #18
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1288263003/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1288263003/160001
5 years, 3 months ago (2015-09-14 03:42:41 UTC) #20
commit-bot: I haz the power
Dry run: This issue passed the CQ dry run.
5 years, 3 months ago (2015-09-14 04:51:28 UTC) #22
hiroshige
On 2015/09/14 04:51:28, commit-bot: I haz the power wrote: > Dry run: This issue passed ...
5 years, 3 months ago (2015-09-14 09:15:29 UTC) #23
shiva.jm
On 2015/09/14 09:15:29, hiroshige wrote: > On 2015/09/14 04:51:28, commit-bot: I haz the power wrote: ...
5 years, 3 months ago (2015-09-14 11:21:24 UTC) #24
hiroshige
> So, if i get correctly, we want to split the header value normalization > ...
5 years, 3 months ago (2015-09-15 05:21:23 UTC) #25
shiva.jm
updated the patch to split into 2 CL, pls have a look.
5 years, 3 months ago (2015-09-15 10:52:47 UTC) #26
yhirano
4 years, 2 months ago (2016-09-27 01:19:27 UTC) #28
(removing myself from the reviewer list)

Powered by Google App Engine
This is Rietveld 408576698