Anne (:annevk) Reporter
	Description • 7 years ago

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.)

	Tom Tung [:tt, :ttung] Assignee
	Comment 1 • 7 years ago

I would like to take this bug.

	Tom Tung [:tt, :ttung] Assignee
	Comment 2 • 7 years ago

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

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

	Tom Tung [:tt, :ttung] Assignee
	Comment 3 • 7 years ago

Update the WPT test to verify behavior.

	Tom Tung [:tt, :ttung] Assignee
	Comment 4 • 7 years ago

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

	Tom Tung [:tt, :ttung] Assignee
	Comment 5 • 7 years ago

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.

	Tom Tung [:tt, :ttung] Assignee
	Comment 6 • 7 years ago

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

	Tom Tung [:tt, :ttung] Assignee
	Comment 7 • 7 years ago

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

	Tom Tung [:tt, :ttung] Assignee
	Comment 8 • 7 years ago

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

	Tom Tung [:tt, :ttung] Assignee
	Comment 9 • 7 years ago

Update for the typo of bug commit message.

	Tom Tung [:tt, :ttung] Assignee
	Comment 10 • 7 years ago

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!

	Tom Tung [:tt, :ttung] Assignee
	Comment 11 • 7 years ago

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.

	Tom Tung [:tt, :ttung] Assignee
	Comment 12 • 7 years ago

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.

	Tom Tung [:tt, :ttung] Assignee
	Comment 13 • 7 years ago

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

	Andrea Marchesini [:baku]
	Comment 14 • 7 years ago

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?

	Tom Tung [:tt, :ttung] Assignee
	Comment 15 • 7 years ago

(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

	Tom Tung [:tt, :ttung] Assignee
	Comment 16 • 7 years ago

Update commit message since it got r+

	Tom Tung [:tt, :ttung] Assignee
	Comment 19 • 7 years ago

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

	Andrea Marchesini [:baku]
	Comment 20 • 7 years ago

> 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.

	Tom Tung [:tt, :ttung] Assignee
	Comment 21 • 7 years ago

(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.

	Pulsebot
	Comment 22 • 7 years ago

Pushed by ryanvm@gmail.com:
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

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 23 • 7 years ago
bugherder

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