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

Move websocket creation to resource task #8867

Merged
merged 1 commit into from Dec 8, 2015

Conversation

@nfallen
Copy link

nfallen commented Dec 7, 2015

This is a pull request for part of #6638

It includes the following changes:
-The websocket networking code (ie. making a connection, receiving data, and sending data) has been extracted out of components/script/dom/websocket.rs and into the new file components/net/websocket_loader.rs.
-websocket.rs now communicates with the resource task (components/net/resource_task.rs) to instruct it to initiate a new websocket connection

  • websocket_loader.rs now provides an API sent over an IPCChannel that allows websocket.rs to receive feedback about this process and to subsequently send and receive data

Review on Reviewable

@bors-servo
Copy link
Contributor

bors-servo commented Dec 7, 2015

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

@jdm jdm self-assigned this Dec 7, 2015
@nfallen nfallen force-pushed the nfallen:6638-websocket_loader branch from 01957c0 to 93861c9 Dec 7, 2015
@jdm
Copy link
Member

jdm commented Dec 7, 2015

This looks really great! There are a couple changes we can make for efficiency, but I'm really happy with how this PR turned out :D
-S-awaiting-review +S-needs-code-changes


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


components/net/resource_task.rs, line 180 [r1] (raw file):
I'm not clear on what this is asking.


components/net/resource_task.rs, line 359 [r1] (raw file):
No need for mut here.


components/net/websocket_loader.rs, line 45 [r1] (raw file):
Let's call this connect instead.


components/net/websocket_loader.rs, line 57 [r1] (raw file):
Instead of creating these senders and receivers here and immediately sending them, we could create them in the DOM constructor and send the resource_action_receiver and resource_event_sender values as part of the initial connection request.


components/net/websocket_loader.rs, line 120 [r1] (raw file):
Let's use while let Ok(dom_action) = resource_action_receiver.recv() { instead.


components/script/dom/websocket.rs, line 143 [r1] (raw file):
I don't think we need the Arc<Mutex< anymore.


components/script/dom/websocket.rs, line 239 [r1] (raw file):
We should be able to store this in self.sender immediatlely, rather than holding on to it for the ConnectionEstablishedTask later.


components/servo/Cargo.lock, line 1159 [r1] (raw file):
We'll need to run ./mach update-cargo -p net in order to automatically update ports/cef/Cargo.lock and ports/gonk/Cargo.lock with these changes, too.


Comments from the review on Reviewable.io

@nfallen
Copy link
Author

nfallen commented Dec 7, 2015

Made a new commit with the updates so you can see the changes more easily, but I can squash the commits when the PR is ready!

@jdm
Copy link
Member

jdm commented Dec 8, 2015

One last change required - please squash the commits together while doing so, and I'll merge it :)
-S-awaiting-review +S-needs-code-changes +S-needs-squash


Reviewed 6 of 6 files at r2.
Review status: all files reviewed at latest revision, 1 unresolved discussion.


components/net/websocket_loader.rs, line 45 [r1] (raw file):
connect?


Comments from the review on Reviewable.io

@nfallen nfallen force-pushed the nfallen:6638-websocket_loader branch from a248e62 to e8c8277 Dec 8, 2015
@Ms2ger
Copy link
Contributor

Ms2ger commented Dec 8, 2015

@bors-servo r=jdm

@bors-servo
Copy link
Contributor

bors-servo commented Dec 8, 2015

📌 Commit e8c8277 has been approved by jdm

@bors-servo
Copy link
Contributor

bors-servo commented Dec 8, 2015

Testing commit e8c8277 with merge 951ab56...

bors-servo added a commit that referenced this pull request Dec 8, 2015
Move websocket creation to resource task

This is a pull request for part of #6638

It includes the following changes:
-The websocket networking code (ie. making a connection, receiving data, and sending data) has been extracted out of components/script/dom/websocket.rs and into the new file components/net/websocket_loader.rs.
-websocket.rs now communicates with the resource task (components/net/resource_task.rs) to instruct it to initiate a new websocket connection
- websocket_loader.rs now provides an API sent over an IPCChannel that allows websocket.rs to receive feedback about this process and to subsequently send and receive data

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

bors-servo commented Dec 8, 2015

@bors-servo bors-servo merged commit e8c8277 into servo:master Dec 8, 2015
2 of 3 checks passed
2 of 3 checks passed
code-review/reviewable Review in progress: 7 of 10 files reviewed, 1 unresolved discussion
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
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.

None yet

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