Skip to content

/ servo

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

Use ipc router in webdriver #24007

Merged
merged 1 commit into from Aug 21, 2019
Merged

Use ipc router in webdriver #24007

merged 1 commit into from Aug 21, 2019

Conversation

@gterzian
Copy link
Member

gterzian commented Aug 19, 2019 •
edited by SimonSapin

Fix for #23905 (comment)

  • ./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 ___

This change is Reviewable
@highfive
Copy link

highfive commented Aug 19, 2019

Heads up! This PR modifies the following files:
@gterzian gterzian force-pushed the gterzian:use_router_in_webdriver branch 2 times, most recently from 41c87ba to 3649e82 Aug 19, 2019
@gterzian
Copy link
Member Author

gterzian commented Aug 19, 2019

@asajeffrey r?
@jdm jdm assigned asajeffrey and unassigned Manishearth Aug 19, 2019
@gterzian gterzian mentioned this pull request Aug 19, 2019
Open
@asajeffrey
asajeffrey approved these changes Aug 19, 2019
View changes
Copy link
Member

asajeffrey left a comment

Looks good, the only thing is it could do with some comments, In particular, it would be nice to have some text about why we have one ipc and one crossbeam channel.
@Manishearth
Manishearth approved these changes Aug 19, 2019
View changes
components/webdriver_server/lib.rs
},
select! {
recv(self.load_status_receiver) -> _ => Ok(WebDriverResponse::Void),
recv(after(Duration::from_millis(timeout))) -> _ => Err(

This comment has been minimized.

Sign in to view
@Manishearth

Manishearth Aug 19, 2019

Member

beautiful
@bors-servo
Copy link
Contributor

bors-servo commented Aug 20, 2019

☔ The latest upstream changes (presumably #23943) made this pull request unmergeable. Please resolve the merge conflicts.
@gterzian gterzian force-pushed the gterzian:use_router_in_webdriver branch 2 times, most recently from 186f326 to ceecde7 Aug 20, 2019
@gterzian
Copy link
Member Author

gterzian commented Aug 20, 2019

@asajeffrey Ok I have added comments and rebased...
@CYBAI
CYBAI reviewed Aug 20, 2019
View changes
components/webdriver_server/lib.rs Outdated Show resolved Hide resolved
@gterzian gterzian force-pushed the gterzian:use_router_in_webdriver branch 2 times, most recently from 3f49db4 to 899bec5 Aug 20, 2019
@asajeffrey
Copy link
Member

asajeffrey commented Aug 20, 2019

Might be worth saying that we're doing the forwarding because IPC channels don't support recv_timeout.
@gterzian
use ipc router in webdriver
Loading status checks…
9ddba75
@gterzian gterzian force-pushed the gterzian:use_router_in_webdriver branch from 899bec5 to 9ddba75 Aug 20, 2019
@gterzian
Copy link
Member Author

gterzian commented Aug 20, 2019

Ok, I've added a comment to that effect.
@asajeffrey
Copy link
Member

asajeffrey commented Aug 20, 2019

Thanks! @bors-servo r+
@bors-servo
Copy link
Contributor

bors-servo commented Aug 20, 2019

📌 Commit 9ddba75 has been approved by asajeffrey
@bors-servo
Copy link
Contributor

bors-servo commented Aug 21, 2019

⌛ Testing commit 9ddba75 with merge 47aa1cc...
bors-servo added a commit that referenced this pull request Aug 21, 2019
@bors-servo
Auto merge of #24007 - gterzian:use_router_in_webdriver, r=asajeffrey
Verified
This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.
GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits
Loading status checks…
47aa1cc 
Use ipc router in webdriver

<!-- Please describe your changes on the following line: -->

Fix for #23905 (comment)

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: -->
- [ ] `./mach build -d` does not report any errors
- [ ] `./mach test-tidy` does not report any errors
- [ ] These changes fix #___ (GitHub issue number if applicable)

<!-- Either: -->
- [ ] There are tests for these changes OR
- [ ] These changes do not require tests because ___

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- 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/24007)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Aug 21, 2019

☀️ Test successful - linux-rel-css, linux-rel-wpt, status-taskcluster
Approved by: asajeffrey
Pushing 47aa1cc to master...
@bors-servo bors-servo merged commit 9ddba75 into servo:master Aug 21, 2019
2 of 3 checks passed
2 of 3 checks passed
continuous-integration/appveyor/pr AppVeyor build failed
Details
Taskcluster (pull_request) TaskGroup: success
Details
homu Test successful
Details
@gterzian gterzian deleted the gterzian:use_router_in_webdriver branch Aug 21, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@CYBAI CYBAI

@asajeffrey asajeffrey

@Manishearth Manishearth

Assignees

@asajeffrey asajeffrey

Labels
None yet
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants
@gterzian @highfive @bors-servo @asajeffrey @Manishearth @CYBAI
You can’t perform that action at this time.