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

Moved WorkerId type to devtools_traits #8862

Merged
merged 1 commit into from Dec 7, 2015
Merged

Conversation

@fstr
Copy link

fstr commented Dec 6, 2015

Fixes #8846.

Review on Reviewable

@highfive
Copy link

highfive commented Dec 6, 2015

Thanks for the pull request, and welcome! The Servo team is excited to review your changes, and you should hear from @larsbergstrom (or someone else) soon.

@Ms2ger
Copy link
Contributor

Ms2ger commented Dec 7, 2015

Thanks for the PR! Unfortunately our "tidy" check caught one small mistake; see below. Let me know when you've made the change?

-S-awaiting-review +S-needs-code-changes


Reviewed 9 of 9 files at r1.
Review status: all files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.


components/script/dom/window.rs, line 7 [r1] (raw file):
Could you merge those two use blocks? Right now, this fails our "tidy" check? Please confirm that ./mach test-tidy reports no errors after fixing that.


Comments from the review on Reviewable.io

@Ms2ger Ms2ger self-assigned this Dec 7, 2015
@fstr
Copy link
Author

fstr commented Dec 7, 2015

components/script/dom/window.rs, line 7 [r1] (raw file):
I tried to find out a pattern for the use statements. Sometimes there's a second use statement for more than 3 imports.

Sometimes, they are all chained in one line.

Interestingly, what works for the tidy check is

use devtools_traits::{ScriptToDevtoolsControlMsg, TimelineMarker, TimelineMarkerType};
use devtools_traits::{WorkerId};

---


*Comments from the [review on Reviewable.io](https://reviewable.io:443/reviews/servo/servo/8862)*
<!-- Sent from Reviewable.io -->
@Ms2ger
Copy link
Contributor

Ms2ger commented Dec 7, 2015

Thank you. Can you please squash the two commits now?

-S-awaiting-review +S-needs-squash


Reviewed 1 of 1 files at r2.
Review status: all files reviewed at latest revision, all discussions resolved.


Comments from the review on Reviewable.io

@fstr fstr force-pushed the fstr:move_workerid branch from 6ddcd24 to b81f712 Dec 7, 2015
@fstr
Copy link
Author

fstr commented Dec 7, 2015

Done!

@Ms2ger
Copy link
Contributor

Ms2ger commented Dec 7, 2015

Thanks again, your help is much appreciated.

@bors-servo r+

@bors-servo
Copy link
Contributor

bors-servo commented Dec 7, 2015

📌 Commit b81f712 has been approved by Ms2ger

@bors-servo
Copy link
Contributor

bors-servo commented Dec 7, 2015

Testing commit b81f712 with merge 02f4be9...

bors-servo added a commit that referenced this pull request Dec 7, 2015
Moved WorkerId type to devtools_traits

Fixes #8846.

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

bors-servo commented Dec 7, 2015

@bors-servo bors-servo merged commit b81f712 into servo:master Dec 7, 2015
3 checks passed
3 checks passed
code-review/reviewable Review complete: all files reviewed, all discussions resolved
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

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