adding initial support for websocket subprotocol negotation #8825

jmr0 · 2015-12-04T17:29:09Z

Addresses #8177

I also noticed some bugs/gaps (and at least one of my TODO's can be an E-Easy)

frewsxcv · 2015-12-04T17:37:31Z

components/script/dom/websocket.rs, line 192 [r1] (raw file):
I don't think the !protocols.is_empty() is necessary here since calling .iter() on an empty collection is a no-op

Comments from the review on Reviewable.io

frewsxcv · 2015-12-04T17:37:34Z

components/script/dom/websocket.rs, line 208 [r1] (raw file):
Alternatively, I think something like this would also work:

if let Some(ref p) = protocol_list.get(0) {
  return Some(p);
}

Comments from the review on Reviewable.io

jmr0 · 2015-12-04T18:26:54Z

Review status: 0 of 10 files reviewed at latest revision, 2 unresolved discussions.

components/script/dom/websocket.rs, line 192 [r1] (raw file):
The idea here was to shortcut out if we were dealing with an empty vector -- this Err should not be triggered if protocols is empty, and if iter().any() returns false on an empty vector, Err would be incorrectly triggered I think

Comments from the review on Reviewable.io

frewsxcv · 2015-12-05T13:06:18Z

components/script/dom/websocket.rs, line 192 [r1] (raw file):
Yeah, that sounds right. I think I overlooked the ! after the &&

Comments from the review on Reviewable.io

bors-servo · 2015-12-07T03:52:32Z

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

bors-servo · 2015-12-08T16:33:59Z

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

bors-servo · 2015-12-11T17:23:18Z

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

jdm · 2015-12-16T21:29:17Z

Great work! I've made a couple comments on ways to clarify the code, but I was able to compare this against the specification text without difficulty.
-S-awaiting-review +S-needs-code-changes

Reviewed 10 of 10 files at r1.
Review status: 9 of 12 files reviewed at latest revision, 4 unresolved discussions.

components/script/dom/websocket.rs, line 204 [r1] (raw file):
nit: add a space after ///

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

wsp.and_then(|protocol_list| protocol_list.get(0).map(|protocol| &*protocol))

components/script/dom/websocket.rs, line 539 [r1] (raw file):
This could move into the related if block.

components/script/dom/websocket.rs, line 540 [r1] (raw file):
I don't think this change is necessary?

Comments from the review on Reviewable.io

jdm · 2015-12-16T21:31:52Z

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

components/net_traits/lib.rs, line 434 [r2] (raw file):
nit: space after ///

components/net_traits/lib.rs, line 436 [r2] (raw file):

wsp.and_then(|protocol_list| protocol_list.get(0).map(|protocol| &*protocol))

Comments from the review on Reviewable.io

jmr0 · 2015-12-16T22:45:45Z

Review status: all files reviewed at latest revision, 2 unresolved discussions.

components/net_traits/lib.rs, line 436 [r2] (raw file):
oddly enough, the &* syntax here leads to expected core::option::Option<&str> foundcore::option::Option<&collections::string::String>` but using as_ref() works

Comments from the review on Reviewable.io

jdm · 2015-12-17T01:23:54Z

Go ahead an squash this into a single commit, then it will be ready for merging. Thanks!
-S-awaiting-review +S-needs-squash

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

Comments from the review on Reviewable.io

jmr0 · 2015-12-17T01:31:32Z

All set. Thanks again.

jdm · 2015-12-17T01:35:57Z

@bors-servo: r+

bors-servo · 2015-12-17T01:42:05Z

⌛ Testing commit 01ff7c2 with merge bf5fb09...

@jdm

adding initial support for websocket subprotocol negotation Addresses #8177 I also noticed some bugs/gaps (and at least one of my TODO's can be an E-Easy) cc @jdm  [<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/8825)

bors-servo · 2015-12-17T02:42:29Z

💔 Test failed - mac-rel-wpt

jdm · 2015-12-17T13:24:22Z

Tests with unexpected results:
  ▶ Unexpected subtest result in /html/dom/interfaces.html:
  └ PASS [expected FAIL] WebSocket interface: attribute protocol

One more test result change required.

jdm · 2015-12-17T14:25:06Z

@bors-servo: r+

bors-servo · 2015-12-17T14:25:06Z

📌 Commit 7d0bede has been approved by jdm

@jdm

adding initial support for websocket subprotocol negotation Addresses #8177 I also noticed some bugs/gaps (and at least one of my TODO's can be an E-Easy) cc @jdm  [<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/8825)

bors-servo · 2015-12-17T18:57:07Z

⌛ Testing commit 7d0bede with merge 5395a7a...

bors-servo · 2015-12-17T19:37:48Z

💔 Test failed - mac-rel-wpt

KiChjang · 2015-12-17T19:38:46Z


Ran 3735 tests finished in 869.0 seconds.
  • 3734 ran as expected. 708 tests skipped.
  • 1 tests timed out unexpectedly

Tests with unexpected results:
  ▶ TIMEOUT [expected PASS] /_mozilla/css/iframe/hide_layers2.html
  │ 
  │ thread 'Constellation' panicked at 'unable to find pipeline - this is a bug', ../src/libcore/option.rs:335
  │ stack backtrace:
  │    1:        0x110180638 - sys::backtrace::tracing::imp::write::hca66179add43b2e5lLt
  │    2:        0x1101826ff - panicking::log_panic::_<closure>::closure.40842
  │    3:        0x1101821a2 - panicking::log_panic::h7a04edac7e1f2797HEx
  │    4:        0x11016dfb6 - sys_common::unwind::begin_unwind_inner::h39a94087039ae0efnOs
  │    5:        0x11016e39e - sys_common::unwind::begin_unwind_fmt::h7be0824d83765cd5tNs
  │    6:        0x11017fc57 - rust_begin_unwind
  │    7:        0x1101a4d60 - panicking::panic_fmt::h871c0ef100fda9e9LFK
  │    8:        0x10eb8fa7d - constellation::_<impl>::handle_request::handle_request::h3729023280372224693
  │    9:        0x10eb717d7 - sys_common::unwind::try::try_fn::try_fn::h8847135229958083224
  │   10:        0x11017fa78 - __rust_try
  │   11:        0x11017cb7e - sys_common::unwind::try::inner_try::h819d2da8e3dc4516VKs
  │   12:        0x10eb73cba - boxed::_<impl>::call_box::call_box::h8173981309006571422
  │   13:        0x11018198d - sys::thread::_<impl>::new::thread_start::h328957c252b95d3cOYw
  │   14:     0x7fff8416a059 - _pthread_body
  └   15:     0x7fff84169fd6 - _pthread_start

jdm · 2015-12-17T19:44:44Z

@bors-servo: retry

Intermittent timeout in /_mozilla/css/iframe/hide_layers2.html (Constellation panic) #8769

bors-servo · 2015-12-17T23:53:15Z

⌛ Testing commit 7d0bede with merge 63923bc...

@jdm

adding initial support for websocket subprotocol negotation Addresses #8177 I also noticed some bugs/gaps (and at least one of my TODO's can be an E-Easy) cc @jdm  [<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/8825)

bors-servo · 2015-12-18T00:25:22Z

☀️ Test successful - android, gonk, linux-dev, linux-rel, mac-dev-ref-unit, mac-rel-css, mac-rel-wpt

highfive added the S-awaiting-review There is new code that needs to be reviewed. label Dec 4, 2015

highfive added S-needs-rebase There are merge conflict errors. and removed S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. labels Dec 7, 2015

jdm removed the S-needs-rebase There are merge conflict errors. label Dec 7, 2015

jdm self-assigned this Dec 7, 2015

highfive added S-needs-rebase There are merge conflict errors. and removed S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. S-needs-rebase There are merge conflict errors. labels Dec 8, 2015

highfive added the S-needs-rebase There are merge conflict errors. label Dec 11, 2015

jdm removed the S-needs-rebase There are merge conflict errors. label Dec 12, 2015

jdm added S-needs-code-changes Changes have not yet been made that were requested by a reviewer. and removed S-awaiting-review There is new code that needs to be reviewed. labels Dec 16, 2015

highfive added S-awaiting-review There is new code that needs to be reviewed. and removed S-needs-code-changes Changes have not yet been made that were requested by a reviewer. labels Dec 17, 2015

jdm added S-needs-squash Some (or all) of the commits in the PR should be combined. and removed S-awaiting-review There is new code that needs to be reviewed. labels Dec 17, 2015

highfive added the S-awaiting-review There is new code that needs to be reviewed. label Dec 17, 2015

highfive added S-tests-failed The changes caused existing tests to fail. and removed S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. labels Dec 17, 2015

adding initial support for websocket subprotocol negotation

7d0bede

highfive added S-awaiting-review There is new code that needs to be reviewed. and removed S-tests-failed The changes caused existing tests to fail. labels Dec 17, 2015

highfive added S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. and removed S-awaiting-review There is new code that needs to be reviewed. labels Dec 17, 2015

highfive added S-tests-failed The changes caused existing tests to fail. and removed S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. labels Dec 17, 2015

highfive added S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. and removed S-tests-failed The changes caused existing tests to fail. labels Dec 17, 2015

bors-servo mentioned this pull request Dec 18, 2015

Handle toDataURL with no context #8725

Merged

bors-servo merged commit 7d0bede into servo:master Dec 18, 2015

This was referenced Dec 18, 2015

Websocket errors #8868

Closed

Fix prototypes of interface objects (fixes #2665) #8954

Merged

jdm mentioned this pull request Jan 12, 2016

Implement websocket subprotocol negotiation #8177

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

adding initial support for websocket subprotocol negotation #8825

adding initial support for websocket subprotocol negotation #8825

jmr0 commented Dec 4, 2015

frewsxcv commented Dec 4, 2015

frewsxcv commented Dec 4, 2015

jmr0 commented Dec 4, 2015

frewsxcv commented Dec 5, 2015

bors-servo commented Dec 7, 2015

bors-servo commented Dec 8, 2015

bors-servo commented Dec 11, 2015

jdm commented Dec 16, 2015

jdm commented Dec 16, 2015

jmr0 commented Dec 16, 2015

jdm commented Dec 17, 2015

jmr0 commented Dec 17, 2015

jdm commented Dec 17, 2015

bors-servo commented Dec 17, 2015

bors-servo commented Dec 17, 2015

jdm commented Dec 17, 2015

jdm commented Dec 17, 2015

bors-servo commented Dec 17, 2015

bors-servo commented Dec 17, 2015

bors-servo commented Dec 17, 2015

KiChjang commented Dec 17, 2015

jdm commented Dec 17, 2015

bors-servo commented Dec 17, 2015

bors-servo commented Dec 18, 2015

adding initial support for websocket subprotocol negotation #8825

adding initial support for websocket subprotocol negotation #8825

Conversation

jmr0 commented Dec 4, 2015

frewsxcv commented Dec 4, 2015

frewsxcv commented Dec 4, 2015

jmr0 commented Dec 4, 2015

frewsxcv commented Dec 5, 2015

bors-servo commented Dec 7, 2015

bors-servo commented Dec 8, 2015

bors-servo commented Dec 11, 2015

jdm commented Dec 16, 2015

jdm commented Dec 16, 2015

jmr0 commented Dec 16, 2015

jdm commented Dec 17, 2015

jmr0 commented Dec 17, 2015

jdm commented Dec 17, 2015

bors-servo commented Dec 17, 2015

bors-servo commented Dec 17, 2015

jdm commented Dec 17, 2015

jdm commented Dec 17, 2015

bors-servo commented Dec 17, 2015

bors-servo commented Dec 17, 2015

bors-servo commented Dec 17, 2015

KiChjang commented Dec 17, 2015

jdm commented Dec 17, 2015

bors-servo commented Dec 17, 2015

bors-servo commented Dec 18, 2015