1346313 - Fetch: use ", " as value separator rather than ","

Status: RESOLVED FIXED

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

Description

[Anne (:annevk)]

Somewhat related to bug 1330297. The plan is to make Fetch and XMLHttpRequest as close as possible as they can be and not to have weird small divergences.

Standard change: https://github.com/whatwg/fetch/pull/504.

Tests: https://github.com/w3c/web-platform-tests/pull/5115. (There's many other tests around this too that have already landed. I suspect that any code changes would end up affecting more results than just these.)

Comment 1

[Tom Tung [:tt, :ttung]]

I would like to take this bug.

Comment 2

[Tom Tung [:tt, :ttung]]

Attachment: P1: Modify Fetch's header handler delimiter to 0x2C 0x20 to align with spec.

Simple fix for using ", " rather than just "," to separate combined value. 

I will check the patch again to ensure I don't misunderstand anything.

Comment 3

[Tom Tung [:tt, :ttung]]

Attachment: P2: Update wpt test to verify our implementation algin with spec.

Update the WPT test to verify behavior.

Comment 4

[Tom Tung [:tt, :ttung]]

Attachment: P3: Patch for ensuring reasonalbe HTTP value.

While implementing, I found that we don't ensure there are no any leading or trailing HTTP whitespace for Header's value [1]. 

To achieve this, I check HTTP whitespace on IsReasonableHeaderValue()[2]. But, it is called from lots of places, so I'm not sure whether should we apply this check to those places.

Moreover, I also not sure about whether this patch belongs to this bug.

I'll check above questions next week.

[1] https://fetch.spec.whatwg.org/#dfnReturnLink-0
[2] http://searchfox.org/mozilla-central/source/netwerk/protocol/http/nsHttp.cpp#250-264

Comment 5

[Tom Tung [:tt, :ttung]]

Comment on attachment 8855745 [details] [diff] [review]
P3: Patch for ensuring reasonalbe HTTP value.

I found that I should just strip the HTTP whitespace rather than forbid the HTTP whitespace appears. Moreover, I found there is a bug for stripping HTTP whitespace, which is bug 1330297.

Comment 6

[Tom Tung [:tt, :ttung]]

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ebc27df2e680c9bccd407da361256b6209eaf2cd

Comment 7

[Tom Tung [:tt, :ttung]]

Attachment: P3: Upate mochitests since we change the format for combined value.

Add a patch to modify mochitest which is affected by the change of combined value.

Comment 8

[Tom Tung [:tt, :ttung]]

(In reply to Tom Tung [:tt] from comment #6)
rerun try after applying P3.
try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=351ad60bd64436d1cc4921f47ade07757a13569d

Comment 9

[Tom Tung [:tt, :ttung]]

Attachment: P3: Update mochitests since we change the format for combined value.

Update for the typo of bug commit message.

Comment 10

[Tom Tung [:tt, :ttung]]

Comment on attachment 8855741 [details] [diff] [review]
P1: Modify Fetch's header handler delimiter to 0x2C 0x20 to align with spec.

Hi Andrea,

This bug is to align the behavior for getting header's combined value. We used to used ',' to separate multiple values, and now we want to use ', ' to separate them. 

github issue: https://github.com/whatwg/fetch/pull/504

This patch is the main change for it. Could you help me to review it? Thanks!

Comment 11

[Tom Tung [:tt, :ttung]]

Comment on attachment 8855742 [details] [diff] [review]
P2: Update wpt test to verify our implementation algin with spec.

Patch for updating WPT in github issue to verify the behavior is correct.

Comment 12

[Tom Tung [:tt, :ttung]]

Comment on attachment 8856858 [details] [diff] [review]
P3: Update mochitests since we change the format for combined value.

Patch for fixing mochitest since we change the behavior of Header.

Comment 13

[Tom Tung [:tt, :ttung]]

Attachment: P3: Update mochitests since we change the format for combined value.

Sorry for change patch, I just found there is a useless modification. Update the patch and send review request again... :(

Comment 14

[Andrea Marchesini [:baku]]

Comment on attachment 8855742 [details] [diff] [review]
P2: Update wpt test to verify our implementation algin with spec.

Review of attachment 8855742 [details] [diff] [review]:
-----------------------------------------------------------------

::: testing/web-platform/tests/fetch/api/headers/headers-combine.html
@@ -25,5 @@
>        test(function() {
>          var headers = new Headers(headerSeqCombine);
>          for (name in expectedDict)
> -          assert_equals(headers.get(name), expectedDict[name],
> -              "name: " + name + " has value: " + expectedDict[name]);

why have you removed the message here?

@@ -51,5 @@
>          for (name in expectedDict) {
>            var value = headers.get(name);
>            headers.append(name,"newSingleValue");
> -          assert_equals(headers.get(name), (value + "," + "newSingleValue"),
> -            "name: " + name + " has value: " + headers.get(name));

and here?

Comment 15

[Tom Tung [:tt, :ttung]]

(In reply to Andrea Marchesini [:baku] from comment #14)
Thanks for the reviews!

> ::: testing/web-platform/tests/fetch/api/headers/headers-combine.html
> @@ -25,5 @@
> >        test(function() {
> >          var headers = new Headers(headerSeqCombine);
> >          for (name in expectedDict)
> > -          assert_equals(headers.get(name), expectedDict[name],
> > -              "name: " + name + " has value: " + expectedDict[name]);
> 
> why have you removed the message here?
> 
> @@ -51,5 @@
> >          for (name in expectedDict) {
> >            var value = headers.get(name);
> >            headers.append(name,"newSingleValue");
> > -          assert_equals(headers.get(name), (value + "," + "newSingleValue"),
> > -            "name: " + name + " has value: " + headers.get(name));
> 
> and here?

I didn't have any strong reason for it. I thought it's better to have less differentiate between github and ours, so I applied the commit[2]. I wanted to align it with web-platform-tests in githud[1]. 

I have no much experience about this. Is there any reason that we want to keep it? If so, I will add them back. :)

[1] https://github.com/w3c/web-platform-tests/blob/master/fetch/api/headers/headers-combine.html
[2] https://github.com/w3c/web-platform-tests/commit/c7b0af2363886e40e48b024ccd03eb2b252785c2

Comment 16

[Tom Tung [:tt, :ttung]]

Attachment: [Final] Bug 1346313 - Part 1: Modify Fetch's header handler delimiter to 0x2C 0x20 to align with spec. r=baku

Update commit message since it got r+

Comment 17

[Tom Tung [:tt, :ttung]]

Attachment: [Final] Bug 1346313 - Part 2: Update wpt test to verify our implementation algin with spec. r=baku

Comment 18

[Tom Tung [:tt, :ttung]]

Attachment: [Final] Bug 1346313 - Part 3: Update mochitests since we change the format for combined value. r=baku

Comment 19

[Tom Tung [:tt, :ttung]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=be58a98a00cc81325a7ed10f8412e7f227569a62

Comment 20

[Andrea Marchesini [:baku]]

> I have no much experience about this. Is there any reason that we want to
> keep it? If so, I will add them back. :)

I was asking because those changes were unrelated with the rest of the patch. Good to me.

Comment 21

[Tom Tung [:tt, :ttung]]

(In reply to Andrea Marchesini [:baku] from comment #20)
> > I have no much experience about this. Is there any reason that we want to
> > keep it? If so, I will add them back. :)
> 
> I was asking because those changes were unrelated with the rest of the
> patch. Good to me.

Thanks! I see.

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/68fda8a87a98
Part 1: Modify Fetch's header handler delimiter to 0x2C 0x20 to align with spec. r=baku
https://hg.mozilla.org/integration/mozilla-inbound/rev/825e868da1de
Part 2: Update wpt test to verify our implementation align with spec. r=baku
https://hg.mozilla.org/integration/mozilla-inbound/rev/3217a1c33b4c
Part 3: Update mochitests since we change the format for combined value. r=baku

Comment 23

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/68fda8a87a98
https://hg.mozilla.org/mozilla-central/rev/825e868da1de
https://hg.mozilla.org/mozilla-central/rev/3217a1c33b4c