Skip to content

Make status message default the empty byte sequence#600

Merged
annevk merged 2 commits intomasterfrom
annevk/status-message-default
Apr 24, 2018
Merged

Make status message default the empty byte sequence#600
annevk merged 2 commits intomasterfrom
annevk/status-message-default

Conversation

@annevk
Copy link
Copy Markdown
Member

@annevk annevk commented Sep 7, 2017

Fixes #599.


Preview | Diff

@annevk
Copy link
Copy Markdown
Member Author

annevk commented Sep 7, 2017

Tests: web-platform-tests/wpt#7274 (note that this is an issue, not actual tests, due to lack of HTTP/2 support).

@annevk annevk force-pushed the annevk/status-message-default branch from ca928c2 to 89f2501 Compare April 11, 2018 17:54
@annevk
Copy link
Copy Markdown
Member Author

annevk commented Apr 11, 2018

@jakearchibald not sure who else to ask for review, but Chrome and Safari already ship this.

@annevk annevk removed the request for review from jakearchibald April 11, 2018 18:02
@annevk
Copy link
Copy Markdown
Member Author

annevk commented Apr 11, 2018

It seems I should investigate #664 first. In particular, what is the status message for synthetic redirects.

annevk added a commit to web-platform-tests/wpt that referenced this pull request Apr 12, 2018
@annevk
Copy link
Copy Markdown
Member Author

annevk commented Apr 12, 2018

Note that the default for new Response() is still OK. It'd be nice to change that too, but should probably be done separately.

@annevk
Copy link
Copy Markdown
Member Author

annevk commented Apr 12, 2018

I need to file a bug against Chrome, Edge, and Firefox for changing statusText for Redirect.response. I think this is once again ready.

@annevk
Copy link
Copy Markdown
Member Author

annevk commented Apr 23, 2018

Additional tests are web-platform-tests/wpt#10442.

Copy link
Copy Markdown
Collaborator

@jakearchibald jakearchibald left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comments are just nits.

fetch.bs Outdated
otherwise it is the empty byte sequence.

<p class=note>This does not have a default value of `<code>OK</code>` as otherwise all HTTP/2
<a for=/>responses</a> would have that value.
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe make it clear that HTTP/2 doesn't support status messages. Something like: "HTTP/2 doesn't support status messages, so this property will be an empty byte sequence for all responses from an HTTP/2 connection"

`<code>text/html;charset=utf-8</code>`, <a for=response>body</a> is the empty byte sequence, and
<a for=response>HTTPS state</a> is <var>request</var>'s <a for=request>client</a>'s
<a for="environment settings object">HTTPS state</a> if <var>request</var>'s
<a for=request>client</a> is non-null.
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given the number of properties, dl/ul would be clearer.

<li><p>Return a <a for=/>response</a> whose <a for=response>status message</a> is
`<code>OK</code>`, whose <a for=response>header list</a> consist of a single <a for=/>header</a>
whose <a for=header>name</a> is `<code>Content-Type</code>` and <a for=header>value</a> is
<var>dataURLStruct</var>'s <a for="data: URL struct">MIME type</a>,
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know it isn't something you changed in this PR, but there's a repetition of "whose" here which is inconsistent with the similar lines above.

Again, dl/ul could be useful here, especially when it comes to the header list.

annevk added a commit to web-platform-tests/wpt that referenced this pull request Apr 24, 2018
@annevk annevk merged commit 0dec453 into master Apr 24, 2018
@annevk annevk deleted the annevk/status-message-default branch April 24, 2018 16:21
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request May 2, 2018
…ions, a=testonly

Automatic update from web-platform-testsFetch: expand Response.redirect() assertions

Needed for whatwg/fetch#664 and whatwg/fetch#600.

--

wpt-commits: 507af0c03617714bfd4134c54da4d534906ee52b
wpt-pr: 10442
gecko-dev-updater pushed a commit to marco-c/gecko-dev-comments-removed that referenced this pull request Oct 2, 2019
…ions, a=testonly

Automatic update from web-platform-testsFetch: expand Response.redirect() assertions

Needed for whatwg/fetch#664 and whatwg/fetch#600.

--

wpt-commits: 507af0c03617714bfd4134c54da4d534906ee52b
wpt-pr: 10442

UltraBlame original commit: d9ac927c0b85220a5183f9e22f3c35affd531a8d
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified that referenced this pull request Oct 3, 2019
…ions, a=testonly

Automatic update from web-platform-testsFetch: expand Response.redirect() assertions

Needed for whatwg/fetch#664 and whatwg/fetch#600.

--

wpt-commits: 507af0c03617714bfd4134c54da4d534906ee52b
wpt-pr: 10442

UltraBlame original commit: d9ac927c0b85220a5183f9e22f3c35affd531a8d
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified-and-comments-removed that referenced this pull request Oct 3, 2019
…ions, a=testonly

Automatic update from web-platform-testsFetch: expand Response.redirect() assertions

Needed for whatwg/fetch#664 and whatwg/fetch#600.

--

wpt-commits: 507af0c03617714bfd4134c54da4d534906ee52b
wpt-pr: 10442

UltraBlame original commit: d9ac927c0b85220a5183f9e22f3c35affd531a8d
jwidar pushed a commit to jwidar/LatencyZeroGithub that referenced this pull request Sep 16, 2025
…ions, a=testonly

Automatic update from web-platform-testsFetch: expand Response.redirect() assertions

Needed for whatwg/fetch#664 and whatwg/fetch#600.

--

wpt-commits: 507af0c03617714bfd4134c54da4d534906ee52b
wpt-pr: 10442
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants