Closed Bug 1330297 Opened 8 years ago Closed 7 years ago

Share code between Headers and setRequestHeader

Categories

(Core :: DOM: Core & HTML, defect, P3)

defect

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: annevk, Assigned: tt)

References

(Blocks 1 open bug)

Details

Attachments

(3 files, 7 obsolete files)

6.98 KB, patch
Details | Diff | Splinter Review
1.28 KB, patch
Details | Diff | Splinter Review
2.76 KB, patch
Details | Diff | Splinter Review
Headers fails on values 0x00, 0x0A, and 0x0D. setRequestHeader() only fails on 0x00. I suggest we make the latter fail on the same as Headers at the very least.

See ongoing discussion at https://github.com/whatwg/fetch/issues/332.
Those tests don't actually check 0x0A or 0x0D behavior, right?  In that the pull request there removes those tests?
It removes tests for \b and \x7F throwing, as those should no longer throw. It does not change \r and \n tests and adds a whole bunch of tests for both XMLHttpRequest and fetch(). Maybe you missed those? Note that https://github.com/w3c/web-platform-tests/pull/4583/files changes the added tests to perform less network fetches.
Er, yeah, I just misread the diff. :(
Priority: -- → P3
I found this problem when I investigated bug 1346313, so I would like to work on this bug. :p
Assignee: nobody → ttung
Current status: I just upload two patches to make sure they share the codes for triming HTTP whitespace. 

I'll take a look at the other part of SetRequestHeader() in xhr and Headers in fetch to see if there are any other things that can be shared.
Hi Anne,

After reading comment[1] in github issue and spec[2], I have a question about stripping trailing/leading HTTP whitespace.
I list my question as following:

var headers = new Headers();
headers.set("foo", "b\r\nar"); // throw
headers.set("foo", "\r\n");    // ok
headers.set("foo", "\rbar\n"); // ok? but I'm not sure about it

Does it mean we can have 0x0A/0x0D in the begining or ending of header's value since we strip the trailing/leading HTTP whitespace?

[1] https://github.com/whatwg/fetch/issues/332#issuecomment-271879535
[2] https://fetch.spec.whatwg.org/#concept-header-value
Flags: needinfo?(annevk)
Yes, that is what it means. Any API that deals with header values will first run https://fetch.spec.whatwg.org/#concept-header-value-normalize and only then check it meets the other constraints before throwing.
Flags: needinfo?(annevk)
(In reply to Anne (:annevk) from comment #10)
> Yes, that is what it means. Any API that deals with header values will first
> run https://fetch.spec.whatwg.org/#concept-header-value-normalize and only
> then check it meets the other constraints before throwing.

Got it! Thanks for answering my question!
Attachment #8856387 - Attachment is obsolete: true
Sorry, previous one is wrong patch...
Attachment #8857853 - Attachment is obsolete: true
Comment on attachment 8857848 [details] [diff] [review]
P1: Strip leading or trailing HTTP whitespace for Header value to follow the spec and share code bewteen fetch and XHR.

Hi Andrea,

As comment 1, this bug is mainly to make fetch be able to trim leading/trailing HTTP whitespace and share the code with XHR. 

And, this patch is the implementation part.

Could you help me to review this patch? Thanks!
Attachment #8857848 - Flags: review?(amarchesini)
Comment on attachment 8857852 [details] [diff] [review]
P2: Remove ini files to verify we can pass the test.

This patch is for removing WPT tests.
Attachment #8857852 - Flags: review?(amarchesini)
Comment on attachment 8857854 [details] [diff] [review]
P3: Update mochitests to aligin with spec since we normalize value before checking and throwing.

This patch is for modifying mochitests since the behavior is changed.
Attachment #8857854 - Flags: review?(amarchesini)
(In reply to Tom Tung [:tt] from comment #17)
> This patch is for removing WPT tests.

s/removing/enabling/
Attachment #8857854 - Flags: review?(amarchesini) → review+
Attachment #8857852 - Flags: review?(amarchesini) → review+
Attachment #8857848 - Flags: review?(amarchesini) → review+
Thanks for the reviews!
Attachment #8859420 - Attachment description: Bug 1330297 - Part 1: Strip leading or trailing HTTP whitespace for Header value to follow the spec and share code bewteen fetch and XHR. r=baku → [Final] Bug 1330297 - Part 1: Strip leading or trailing HTTP whitespace for Header value to follow the spec and share code bewteen fetch and XHR. r=baku
Attachment #8859421 - Attachment description: Bug 1330297 - Part 2: Remove ini files to verify we can pass the test. r=baku → [Final] Bug 1330297 - Part 2: Remove ini files to verify we can pass the test. r=baku
Attachment #8859423 - Attachment description: Bug 1330297 - Part 3: Update mochitests to aligin with spec since we normalize value before checking and throwing. r=baku → [Final] Bug 1330297 - Part 3: Update mochitests to aligin with spec since we normalize value before checking and throwing. r=baku
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c72f2f5feaae
Part 1: Strip leading or trailing HTTP whitespace for Header value to follow the spec and share code bewteen fetch and XHR. r=baku
https://hg.mozilla.org/integration/mozilla-inbound/rev/e230df9fbec5
Part 2: Remove ini files to verify we can pass the test. r=baku
https://hg.mozilla.org/integration/mozilla-inbound/rev/51df9eb41148
Part 3: Update mochitests to align with spec since we normalize value before checking and throwing. r=baku
Keywords: checkin-needed
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: