Update DOM headers `append` and `delete` #13004

jeenalee · 2016-08-23T23:07:17Z

Two changes are included in this PR:

A resolved TODO comment in delete is removed.
append method adds a space after a comma when combining header values. Expected wpt results are updated with this change.

./mach build -d does not report any errors
./mach test-tidy does not report any errors
These changes fix #__ (github issue number if applicable).

There are tests for these changes OR
These changes do not require tests because _____

This change is


        Remove TODO from step 4 of `delete`

The confusion around step 4 of `delete` method has been resolved through whatwg/fetch#372.

highfive · 2016-08-23T23:07:21Z

Heads up! This PR modifies the following files:

@KiChjang: components/script/dom/headers.rs

jeenalee · 2016-08-23T23:07:34Z

r? @jdm

Ms2ger · 2016-08-24T07:14:39Z

components/script/dom/headers.rs

@@ -86,6 +86,7 @@ impl HeadersMethods for Headers {
        if let Some(v) = self.header_list.borrow().get_raw(&valid_name) {
            combined_value = v[0].clone();
            combined_value.push(b","[0]);
+            combined_value.push(b" "[0]);


b' ' with single quotes would work; perhaps you want .extend_from_slice(b", "), though.

@jeenalee Are you sure this is what should be done here? According to the spec, "append" is not the same as "combine".

The underlying hyper::header::Headers's HashMap insert method overwrites entries instead of appending. As a result of this, for a given header name there is at most one value in the inner "header list"/HashMap. I recall that we were debating whether to re-write hyper::header::Headers or re-write our Append function. We ended up going with the latter. Therefore we ended up combining all the values for a given name together. (I changed the append function to reflect that in this PR).

@nox I'll add a comment so that it doesn't cause confusion in the future.

@Ms2ger That's a really good point! I was wondering how we can compress those two lines into one.

I think each element in the raw vector corresponds to a single header value, so instead of doing v[0].clone() it seems to me like a new vector should be pushed in v, unfortunately it's just a slice and not the underlaying vector. Cc @seanmonstar.

@nox @seanmonstar Here is a snippet of the irc conversation in regards to append and combine.

I wasn't aware that you were working with the raw bytes of the Header. In that case, it's likely more according to the spec to add a new 'line'. It seems that there is missing method to do this, get_raw_mut, but it would otherwise look like this:

match headers.get_raw_mut("foo") { Some(mut raw) => raw.push(value), None => headers.set_raw("foo", vec![value]) }

jdm · 2016-08-24T20:36:20Z

Commit 2 is interesting, since the specification does not mention adding a space. We should either modify the specification or modify the tests before proceeding.

jeenalee · 2016-08-24T20:58:26Z

Relevant spec links:

KiChjang · 2016-08-24T20:58:52Z

components/script/dom/headers.rs

@@ -85,7 +85,7 @@ impl HeadersMethods for Headers {
        let mut combined_value: Vec<u8> = vec![];
        if let Some(v) = self.header_list.borrow().get_raw(&valid_name) {


I highly suspect that we can do something fancy here, such as filter_map on the header_list iterator.

KiChjang · 2016-08-24T21:05:38Z

I recall having this conversation with @seanmonstar before, on whether OWS should be present or not when converting Hyper types into string representation (Hyper does not "remember" OWS). The conclusion IIRC was to do exactly what this PR is doing - by stringizing every Header and appending commas and OWS as we see fit, in order to pass our WPT tests.

Also see web-platform-tests/wpt#2807 for prior discussions in modifying WPT expected values to match normalized formats.

jdm · 2016-08-24T21:06:37Z

I don't believe OWS is an issue here. The Fetch specification says that combined header values should be separated by a , (no space); that's unrelated to the actual header content.

jdm · 2016-08-24T21:08:00Z

Oh, and to be clear on what I'm looking for here - let's take a survey of the various browsers that implement the Headers API (Firefox and Chrome, at minimum) and see what their behaviour is here, then we can decide whether we should be filing a spec issue to resolve this.

jdm · 2016-09-01T22:57:48Z

@jeenalee Have you investigated what other browsers do with respect to returning combined header values and the separating character?

jeenalee · 2016-09-02T00:31:28Z

@jdm Malisa and I just spent some time figuring this out.

I'm confused because MDN's definition of Headers.get() is different from the Fetch spec link MDN lists in the bottom of the page. According to MDN, the get() method returns the first value of a given header, while Fetch states that get() returns the combined value. MDN writes that Headers.getAll would return an array containing all values.

Chrome and Firefox seem to follow MDN's definition. I tried out one of the failing headers/headers-combine.html wpt subtests:

Firefox:

> var headerSeqCombine = [["single", "singleValue"],
                          ["double", "doubleValue1"],
                          ["double", "doubleValue2"],
                          ["triple", "tripleValue1"],
                          ["triple", "tripleValue2"],
                          ["triple", "tripleValue3"]
                         ];
undefined
> var headers = new Headers(headerSeqCombine);
undefined
> headers.get("double")
"doubleValue1"
> headers.getAll("double")
Array [ "doubleValue1", "doubleValue2" ]

Chrome:

> var headerSeqCombine = [["single", "singleValue"],
                          ["double", "doubleValue1"],
                          ["double", "doubleValue2"],
                          ["triple", "tripleValue1"],
                          ["triple", "tripleValue2"],
                          ["triple", "tripleValue3"]
                         ];
undefined
> var headers = new Headers(headerSeqCombine);
undefined
> headers.get("double")
"doubleValue1"
> headers.getAll("double")
["doubleValue1", "doubleValue2"]

Also, this line in another failing headers-combine.html subtest caught my attention. When (value + ", " + "newSingleValue") is changed to (value + "," + "newSingleValue"), the subtest passes.

Trying the failing subtest on Firefox and Chrome makes me think they did not implement Headers that conform to the Fetch spec. If they were to run the headers-combine.html tests, they would fail because get() will return only one value, and no combining will happen. And the fact that Firefox and Chrome's getAll() return an array of multiple elements makes us believe they aren't combining values at all, which is different from the Fetch spec.

The confusion around get and getAll aside, I can think of two options:

Add a space after comma when appending values (in Servo). This will make tests pass, and the spec will have to be changed.
Tests are modified (no more space), and the spec stays the same. It's difficult to compare our current implementation to Firefox and Chrome's so I'm not sure if this will make Servo's Headers behave more or less similar to theirs.

Finally, it seems like Firefox and Chrome might in fact internally store header values as a vector of u8-vectors, in which each header value is its own vector. So we might want to implement the get_raw_mut function @seanmonstar suggested.

jdm · 2016-09-02T02:46:46Z

The Fetch spec changed since get/getAll were implemented in Firefox, so https://bugzilla.mozilla.org/show_bug.cgi?id=1278275 will be very relevant. Fun times!

jdm · 2016-09-06T17:34:37Z

Since #3646 merged, you can revert the change to add spaces to the combined header values. The tests will start passing after we update our local copy of the upstream repository.

seanmonstar · 2016-09-06T17:36:28Z

@jdm is that the correct issue linked?


        Refactor combine method, and update expected wpt results

Instead of creating an array with length of 1 of `b','`, then pushing the first element of that array to `combined_value`, push a `b','` directly to `combined_value`. The web platform test for combining headers has been updated to reflect the Fetch spec (web-platform-tests/wpt#3646). The expected web platform test results that will be affected by this change are updated.

jeenalee · 2016-09-07T17:31:23Z

@seanmonstar It is wpt's 3646: web-platform-tests/wpt#3646

jdm · 2016-09-12T13:53:53Z

@bors-servo: r+

bors-servo · 2016-09-12T13:53:55Z

📌 Commit 8946a65 has been approved by jdm

bors-servo · 2016-09-12T14:21:53Z

⌛ Testing commit 8946a65 with merge 7264592...


        Auto merge of #13004 - jeenalee:combine-headers, r=jdm

Update DOM headers `append` and `delete`  Two changes are included in this PR: 1. A resolved TODO comment in `delete` is removed. 2. `append` method adds a space after a comma when combining header values. Expected wpt results are updated with this change. ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [ ] These changes fix #__ (github issue number if applicable).  - [X] There are tests for these changes OR - [ ] These changes do not require tests because _____   --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/13004)

bors-servo · 2016-09-12T15:19:15Z

☀️ Test successful - arm32, arm64, linux-dev, linux-rel, mac-dev-unit, mac-rel-css, mac-rel-wpt, windows-dev

Remove TODO from step 4 of `delete`

2c4b268

The confusion around step 4 of `delete` method has been resolved through whatwg/fetch#372.

highfive assigned SimonSapin Aug 23, 2016

highfive added the S-awaiting-review label Aug 23, 2016

highfive assigned jdm and unassigned SimonSapin Aug 23, 2016

Ms2ger reviewed Aug 24, 2016
View changes

jdm added S-blocked-on-external and removed S-awaiting-review labels Aug 24, 2016

KiChjang reviewed Aug 24, 2016
View changes

jeenalee mentioned this pull request Sep 1, 2016

Implement JS fetch API #11894

Closed

jdm mentioned this pull request Sep 6, 2016

Combine headers without extra space, per spec web-platform-tests/wpt#3646

Merged

jdm added S-needs-code-changes and removed S-blocked-on-external labels Sep 6, 2016

jeenalee force-pushed the jeenalee:combine-headers branch from 7fc4d85 to 8946a65 Sep 6, 2016

highfive added S-awaiting-review and removed S-needs-code-changes labels Sep 6, 2016

highfive added S-awaiting-merge and removed S-awaiting-review labels Sep 12, 2016

bors-servo merged commit 8946a65 into servo:master Sep 12, 2016
3 checks passed

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details

continuous-integration/travis-ci/pr The Travis CI build passed
Details

homu Test successful
Details

highfive removed the S-awaiting-merge label Sep 12, 2016

servo / servo

Update DOM headers `append` and `delete` #13004

Update DOM headers `append` and `delete` #13004

jeenalee commented Aug 23, 2016 •

edited by larsbergstrom

highfive commented Aug 23, 2016

jeenalee commented Aug 23, 2016

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

jdm commented Aug 24, 2016

jeenalee commented Aug 24, 2016

This comment has been minimized.

KiChjang commented Aug 24, 2016

jdm commented Aug 24, 2016

jdm commented Aug 24, 2016 •

edited

jdm commented Sep 1, 2016

jeenalee commented Sep 2, 2016 •

edited

jdm commented Sep 2, 2016

jdm commented Sep 6, 2016

seanmonstar commented Sep 6, 2016

jeenalee commented Sep 7, 2016

jdm commented Sep 12, 2016

bors-servo commented Sep 12, 2016

bors-servo commented Sep 12, 2016

bors-servo commented Sep 12, 2016

		@@ -85,7 +85,7 @@ impl HeadersMethods for Headers {
		let mut combined_value: Vec<u8> = vec![];
		if let Some(v) = self.header_list.borrow().get_raw(&valid_name) {

servo / servo

Join GitHub today

Update DOM headers `append` and `delete` #13004

Update DOM headers `append` and `delete` #13004

Conversation

jeenalee commented Aug 23, 2016 • edited by larsbergstrom

highfive commented Aug 23, 2016

jeenalee commented Aug 23, 2016

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

jdm commented Aug 24, 2016

jeenalee commented Aug 24, 2016

This comment has been minimized.

KiChjang commented Aug 24, 2016

jdm commented Aug 24, 2016

jdm commented Aug 24, 2016 • edited

jdm commented Sep 1, 2016

jeenalee commented Sep 2, 2016 • edited

jdm commented Sep 2, 2016

jdm commented Sep 6, 2016

seanmonstar commented Sep 6, 2016

jeenalee commented Sep 7, 2016

jdm commented Sep 12, 2016

bors-servo commented Sep 12, 2016

bors-servo commented Sep 12, 2016

bors-servo commented Sep 12, 2016

jeenalee commented Aug 23, 2016 •

edited by larsbergstrom

jdm commented Aug 24, 2016 •

edited

jeenalee commented Sep 2, 2016 •

edited