New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Headers API constructor and methods #12634

Merged
merged 1 commit into from Jul 30, 2016

Conversation

Projects
None yet
6 participants
@malisas
Contributor

malisas commented Jul 28, 2016

This PR fills out the constructor and the delete, get, has, and set methods for the Headers API. Addresses issue #11897 .

The PR also rewrites the append method to support hyper::header::Headers's HashMap insert method, which 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. Multiple values for the same name are comma-delimited.

There are still a few TODOs:

  • Support OpenEndedDictionary<ByteString> as a possible HeadersInit value. OpenEndedDictionary is a future IDL construct.
  • Support iterable<ByteString, ByteString>. Related issue: #12628
  • Values are comma-delimited, except for values with the name set-cookie, which are newline-delimited. This is because values for set-cookie are allowed to contain inner commas. This violates the spec.
  • The TODOs from PR #12467 regarding value parsing also still need to be resolved.

  • ./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 tests for the Headers API already exists, but this commit does not implement the interface fully. The tests will fail.

This change is Reviewable

@highfive

This comment has been minimized.

highfive commented Jul 28, 2016

Heads up! This PR modifies the following files:

  • @KiChjang: components/script/dom/webidls/Headers.webidl, components/script/dom/headers.rs
@highfive

This comment has been minimized.

highfive commented Jul 28, 2016

warning Warning warning

  • These commits modify script code, but no tests are modified. Please consider adding a test!
@malisas

This comment has been minimized.

Contributor

malisas commented Jul 28, 2016

r? @jdm

@highfive highfive assigned jdm and unassigned larsbergstrom Jul 28, 2016

@fitzgen

This comment has been minimized.

Member

fitzgen commented Jul 28, 2016

cc myself

@jdm jdm referenced this pull request Jul 28, 2016

Merged

Enable Headers API tests. #12636

3 of 3 tasks complete
@jdm

This comment has been minimized.

Member

jdm commented Jul 28, 2016

Great work! I've also submitted #12636 which will allow us to verify at least some of these changes against automated tests.
-S-awaiting-review +S-needs-code-changes


Reviewed 2 of 2 files at r1.
Review status: all files reviewed at latest revision, 12 unresolved discussions.


components/script/dom/headers.rs, line 46 [r1] (raw file):

    // https://fetch.spec.whatwg.org/#concept-headers-fill
    pub fn new(global: GlobalRef, init: Option<HeadersBinding::HeadersInit>)
               -> Result<Root<Headers>, Error> {

This can be Fallible<Root<Header>> instead.


components/script/dom/headers.rs, line 47 [r1] (raw file):

    pub fn new(global: GlobalRef, init: Option<HeadersBinding::HeadersInit>)
               -> Result<Root<Headers>, Error> {
        let domHeadersNew = reflect_dom_object(box Headers::new_inherited(), global, HeadersBinding::Wrap);

Let's call this dom_headers_new.


components/script/dom/headers.rs, line 52 [r1] (raw file):

            Some(HeadersOrByteStringSequenceSequence::Headers(h)) => {
                // headerListCopy has type hyper::header::Headers
                let headerListCopy = h.header_list.clone();

Traditional Servo code style is to name this header_list_copy instead.


components/script/dom/headers.rs, line 53 [r1] (raw file):

                // headerListCopy has type hyper::header::Headers
                let headerListCopy = h.header_list.clone();
                for header in headerListCopy.borrow().iter() {

for header in &headerListCopy.borrow() {


components/script/dom/headers.rs, line 70 [r1] (raw file):

                    } else {
                        return Err(Error::Type(
                            "Each inner Vec<ByteString> in HeadersInit must have length 2"

format!("Each header object must be a sequence of length 2 - found one with length {}", seq.len())


components/script/dom/headers.rs, line 122 [r1] (raw file):

        // "set-cookie" values allow commas, so we use a different delimiter.
        if &*valid_name == "set-cookie" {
            combined_value.push(b"\n"[0]);

This change isn't part of the fetch specification, so this isn't compatible with the Headers implementation in other browsers. We should remove the special case here and file an issue against the Fetch specification.


components/script/dom/headers.rs, line 132 [r1] (raw file):

    // https://fetch.spec.whatwg.org/#dom-headers-delete
    fn Delete(&self, name: ByteString) -> Result<(), Error> {

This can return ErrorResult instead.


components/script/dom/headers.rs, line 157 [r1] (raw file):

    // https://fetch.spec.whatwg.org/#dom-headers-get
    fn Get(&self, name: ByteString) -> Result<Option<ByteString>, Error> {

This can use Fallible.


components/script/dom/headers.rs, line 165 [r1] (raw file):

        // TODO header fields with a name of "set-cookie" will be
        // returned as newline-delimited values. This
        // goes against the spec, which requires commas:

We should remove this note when we remove the spec-incompatible behaviour.


components/script/dom/headers.rs, line 167 [r1] (raw file):

        // goes against the spec, which requires commas:
        // https://fetch.spec.whatwg.org/#concept-header-value-combined
        match self.header_list.borrow_mut().get_raw(&valid_name) {
Ok(self.header_list.borrow().get_raw(&valid_name).map(|v| {
    ByteString::new(v[0].clone())
}))

components/script/dom/headers.rs, line 174 [r1] (raw file):

    // https://fetch.spec.whatwg.org/#dom-headers-has
    fn Has(&self, name: ByteString) -> Result<bool, Error> {

Fallible.


components/script/dom/headers.rs, line 183 [r1] (raw file):

    // https://fetch.spec.whatwg.org/#dom-headers-set
    fn Set(&self, name: ByteString, value: ByteString) -> Result<(), Error> {

Fallible.


Comments from Reviewable

bors-servo added a commit that referenced this pull request Jul 28, 2016

Auto merge of #12636 - jdm:headerstests, r=pcwalton
Enable Headers API tests.

- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] There are tests for these changes

This will be useful for #12634.

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/12636)
<!-- Reviewable:end -->
@jdm

This comment has been minimized.

Member

jdm commented Jul 28, 2016

@bors-servo

This comment has been minimized.

Contributor

bors-servo commented Jul 28, 2016

⌛️ Trying commit 51cfea3 with merge 8533327...

bors-servo added a commit that referenced this pull request Jul 28, 2016

Auto merge of #12634 - malisas:malisa-headersAPI, r=<try>
Headers API constructor and methods

<!-- Please describe your changes on the following line: -->
This PR fills out the constructor and the delete, get, has, and set methods for the Headers API. Addresses issue #11897 .

The PR also rewrites the append method to support `hyper::header::Headers`'s HashMap `insert` method, which 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. Multiple values for the same name are comma-delimited.

There are still a few TODOs:
- Support `OpenEndedDictionary<ByteString>`  as a possible `HeadersInit` value. [OpenEndedDictionary<T> is a future IDL construct.](https://fetch.spec.whatwg.org/#headers-class)
- Support `iterable<ByteString, ByteString>`. Related issue: #12628
- Values are comma-delimited, except for values with the name `set-cookie`, which are newline-delimited. This is because values for `set-cookie` are [allowed to contain](https://tools.ietf.org/html/rfc7230#section-3.2.2) inner commas. This violates the [spec](https://fetch.spec.whatwg.org/#concept-header-value-combined).
- The TODOs from PR #12467 regarding value parsing also still need to be resolved.

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [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).

<!-- Either: -->
- [ ] There are tests for these changes OR
- [X] These changes do not require tests because tests for the Headers API already exists, but this commit does not implement the interface fully. The tests will fail.

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/12634)
<!-- Reviewable:end -->
@bors-servo

This comment has been minimized.

Contributor

bors-servo commented Jul 28, 2016

💔 Test failed - linux-rel

@highfive

This comment has been minimized.

highfive commented Jul 28, 2016

  ▶ Unexpected subtest result in /fetch/api/headers/headers-basic.html:
  └ PASS [expected FAIL] Create headers from no parameter

  ▶ Unexpected subtest result in /fetch/api/headers/headers-basic.html:
  └ PASS [expected FAIL] Create headers from undefined parameter

  ▶ Unexpected subtest result in /fetch/api/headers/headers-errors.html:
  └ PASS [expected FAIL] Check headers get with an invalid name invalid\u0100

  ▶ Unexpected subtest result in /fetch/api/headers/headers-errors.html:
  └ PASS [expected FAIL] Check headers get with an invalid name [object Object]

  ▶ Unexpected subtest result in /fetch/api/headers/headers-errors.html:
  └ PASS [expected FAIL] Check headers delete with an invalid name invalid\u0100

  ▶ Unexpected subtest result in /fetch/api/headers/headers-errors.html:
  └ PASS [expected FAIL] Check headers delete with an invalid name [object Object]

  ▶ Unexpected subtest result in /fetch/api/headers/headers-errors.html:
  └ PASS [expected FAIL] Check headers has with an invalid name invalid\u0100

  ▶ Unexpected subtest result in /fetch/api/headers/headers-errors.html:
  └ PASS [expected FAIL] Check headers has with an invalid name [object Object]

  ▶ Unexpected subtest result in /fetch/api/headers/headers-errors.html:
  └ PASS [expected FAIL] Check headers set with an invalid name invalid\u0100

  ▶ Unexpected subtest result in /fetch/api/headers/headers-errors.html:
  └ PASS [expected FAIL] Check headers set with an invalid name [object Object]

  ▶ Unexpected subtest result in /fetch/api/headers/headers-errors.html:
  └ PASS [expected FAIL] Check headers set with an invalid value invalid\u0100

  ▶ Unexpected subtest result in /fetch/api/headers/headers-errors.html:
  └ PASS [expected FAIL] Check headers append with an invalid name invalid\u0100

  ▶ Unexpected subtest result in /fetch/api/headers/headers-errors.html:
  └ PASS [expected FAIL] Check headers append with an invalid name [object Object]

  ▶ Unexpected subtest result in /fetch/api/headers/headers-errors.html:
  └ PASS [expected FAIL] Check headers append with an invalid value invalid\u0100

  ▶ OK [expected TIMEOUT] /fetch/api/headers/headers-structure.html

  ▶ Unexpected s</span><span class="stdout">ubtest result in /fetch/api/headers/headers-structure.html:
  │ FAIL [expected PASS] Headers has entries method
  │   → assert_true: headers has entries method expected true got false
  │ FAIL [expected PASS] Headers has keys method
  │   → assert_true: headers has keys method expected true got false
  │ FAIL [expected PASS] Headers has values method
  │   → assert_true: headers has values method expected true got false
  │ 
  │ @http://web-platform.test:8000/fetch/api/headers/headers-structure.html:14:13
  │ Test.prototype.step@http://web-platform.test:8000/resources/testharness.js:1402:20
  │ test@http://web-platform.test:8000/resources/testharness.js:500:9
  └ @http://web-platform.test:8000/fetch/api/headers/headers-structure.html:13:11

@malisas malisas force-pushed the malisas:malisa-headersAPI branch from 51cfea3 to 8b3a526 Jul 29, 2016

@malisas

This comment has been minimized.

Contributor

malisas commented Jul 29, 2016

Thanks @jdm for the feedback!
I made the changes -- still need to file the Fetch spec issue though.

@malisas malisas force-pushed the malisas:malisa-headersAPI branch from 8b3a526 to f91c196 Jul 29, 2016

@malisas

This comment has been minimized.

Contributor

malisas commented Jul 29, 2016

(and here is the filed issue about the Set-Cookie headers exception)

@jdm

This comment has been minimized.

Member

jdm commented Jul 29, 2016

The only remaining change is to update the expected test results and squash the commits together.
-S-awaiting-review +S-needs-code-changes +S-needs-squash


Reviewed 1 of 1 files at r2.
Review status: 1 of 2 files reviewed at latest revision, all discussions resolved.


Comments from Reviewable

@malisas malisas force-pushed the malisas:malisa-headersAPI branch from f91c196 to 3f5b617 Jul 29, 2016

@malisas malisas force-pushed the malisas:malisa-headersAPI branch from 3f5b617 to 431b697 Jul 30, 2016

Headers API constructor and methods
- Reworked the append method to support the inner `hyper::header::Headers`'s HashMap `insert` method, which overwrites entries instead of appending.
- Filled out constructor as well as delete, get, has, and set methods.
- Updated relevant test expectations

@malisas malisas force-pushed the malisas:malisa-headersAPI branch from 431b697 to e631d3a Jul 30, 2016

@jdm

This comment has been minimized.

Member

jdm commented Jul 30, 2016

@bors-servo

This comment has been minimized.

Contributor

bors-servo commented Jul 30, 2016

📌 Commit e631d3a has been approved by jdm

@bors-servo

This comment has been minimized.

Contributor

bors-servo commented Jul 30, 2016

⌛️ Testing commit e631d3a with merge 1ce4be8...

bors-servo added a commit that referenced this pull request Jul 30, 2016

Auto merge of #12634 - malisas:malisa-headersAPI, r=jdm
Headers API constructor and methods

<!-- Please describe your changes on the following line: -->
This PR fills out the constructor and the delete, get, has, and set methods for the Headers API. Addresses issue #11897 .

The PR also rewrites the append method to support `hyper::header::Headers`'s HashMap `insert` method, which 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. Multiple values for the same name are comma-delimited.

There are still a few TODOs:
- Support `OpenEndedDictionary<ByteString>`  as a possible `HeadersInit` value. [OpenEndedDictionary<T> is a future IDL construct.](https://fetch.spec.whatwg.org/#headers-class)
- Support `iterable<ByteString, ByteString>`. Related issue: #12628
- Values are comma-delimited, except for values with the name `set-cookie`, which are newline-delimited. This is because values for `set-cookie` are [allowed to contain](https://tools.ietf.org/html/rfc7230#section-3.2.2) inner commas. This violates the [spec](https://fetch.spec.whatwg.org/#concept-header-value-combined).
- The TODOs from PR #12467 regarding value parsing also still need to be resolved.

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [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).

<!-- Either: -->
- [ ] There are tests for these changes OR
- [X] These changes do not require tests because tests for the Headers API already exists, but this commit does not implement the interface fully. The tests will fail.

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/12634)
<!-- Reviewable:end -->
@bors-servo

This comment has been minimized.

Contributor

bors-servo commented Jul 30, 2016

@bors-servo bors-servo merged commit e631d3a into servo:master Jul 30, 2016

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

@malisas malisas referenced this pull request Aug 24, 2016

Merged

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

3 of 5 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment