Skip to content
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

Clean up websocket closing code #9358

Merged
merged 4 commits into from Jan 19, 2016

Conversation

@jsanders
Copy link
Contributor

jsanders commented Jan 18, 2016

Fixes #7860.

This also changes quite a bit about how close codes are implemented, I believe bringing them closer in line with the spec. Instead of saving off the close code sent by the client, it uses the code from the server's closing handshake. It also handles NO_STATUS in what I believe is the correct manner. Making this work required a change to the test harness to make the /echo websocket handler echo the code sent by the client and handle NO_STATUS properly, rather than always replying with NORMAL.

Review on Reviewable

@bors-servo
Copy link
Contributor

bors-servo commented Jan 18, 2016

The latest upstream changes (presumably #9368) made this pull request unmergeable. Please resolve the merge conflicts.

@jdm
Copy link
Member

jdm commented Jan 18, 2016

This is so much more readable! 💃 These changes look good to me, but I'll want to wait until upstream agrees with the server change.

@jdm jdm self-assigned this Jan 18, 2016
@jsanders
Copy link
Contributor Author

jsanders commented Jan 18, 2016

@jdm: Will do!

@jsanders
Copy link
Contributor Author

jsanders commented Jan 19, 2016

@jdm: I just submitted web-platform-tests/wpt#2498 with this change.

jsanders added 4 commits Jan 12, 2016
Set code to None if it is STATUS_NO_STATUS_RECEIVED, which should not be
sent across the wire.
@jdm
Copy link
Member

jdm commented Jan 19, 2016

@bors-servo: r+
Thanks!

@bors-servo
Copy link
Contributor

bors-servo commented Jan 19, 2016

📌 Commit 482d23b has been approved by jdm

@bors-servo
Copy link
Contributor

bors-servo commented Jan 19, 2016

Testing commit 482d23b with merge 6663f28...

bors-servo added a commit that referenced this pull request Jan 19, 2016
Clean up websocket closing code

Fixes #7860.

This also changes quite a bit about how close codes are implemented, I believe bringing them closer in line with the spec. Instead of saving off the close code sent by the client, it uses the code from the server's closing handshake. It also handles `NO_STATUS` in what I believe is the correct manner. Making this work required a change to the test harness to make the `/echo` websocket handler echo the code sent by the client and handle `NO_STATUS` properly, rather than always replying with `NORMAL`.

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/9358)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Jan 19, 2016

💔 Test failed - linux-rel

@jdm
Copy link
Member

jdm commented Jan 19, 2016

@bors-servo
Copy link
Contributor

bors-servo commented Jan 19, 2016

Previous build results for android, gonk, linux-dev, mac-dev-unit, mac-rel-css, mac-rel-wpt are reusable. Rebuilding only linux-rel...

@jsanders
Copy link
Contributor Author

jsanders commented Jan 19, 2016

@jdm: Should I go ahead and squash this now?

@jdm
Copy link
Member

jdm commented Jan 19, 2016

No, I think it's fine the way it is. The merging process is already on its way.

@jsanders
Copy link
Contributor Author

jsanders commented Jan 19, 2016

👍 Thanks!

@bors-servo
Copy link
Contributor

bors-servo commented Jan 19, 2016

@bors-servo bors-servo merged commit 482d23b into servo:master Jan 19, 2016
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
@jsanders jsanders mentioned this pull request Jan 20, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

5 participants
You can’t perform that action at this time.