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

compositing: Remove CompositorThread struct #11569

Merged
merged 1 commit into from Jun 3, 2016
Merged

compositing: Remove CompositorThread struct #11569

merged 1 commit into from Jun 3, 2016
+4 −17

Conversation

@talklittle
Copy link
Contributor

talklittle commented Jun 3, 2016 •
edited by larsbergstrom

Remove CompositorThread struct, since it wraps only IOCompositor. Use IOCompositor directly.

  • ./mach build -d does not report any errors
  • ./mach test-tidy does not report any errors
  • These changes fix #10261 (github issue number if applicable).
  • There are tests for these changes OR
  • These changes do not require tests because refactoring

This change is Reviewable
@highfive
Copy link

highfive commented Jun 3, 2016

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.
@metajack
Copy link
Contributor

metajack commented Jun 3, 2016

This is not a critique of your patch, but I think this existed because we had NullCompositor at one point. I'm curious what happened to it.

cc @pcwalton @glennw
@metajack
Copy link
Contributor

metajack commented Jun 3, 2016

@bors-servo r+

Previously, metajack (Jack Moffitt) wrote…

This is not a critique of your patch, but I think this existed because we had NullCompositor at one point. I'm curious what happened to it.

cc @pcwalton @glennw

Reviewed 3 of 3 files at r1.
Review status: all files reviewed at latest revision, all discussions resolved.

Comments from Reviewable
@bors-servo
Copy link
Contributor

bors-servo commented Jun 3, 2016

📌 Commit d2aad37 has been approved by metajack
@highfive highfive assigned metajack and unassigned larsbergstrom Jun 3, 2016
@bors-servo
Copy link
Contributor

bors-servo commented Jun 3, 2016

⌛ Testing commit d2aad37 with merge 9efc166...
bors-servo added a commit that referenced this pull request Jun 3, 2016
@bors-servo
Auto merge of #11569 - talklittle:remove_struct_10261, r=metajack
9efc166 
compositing: Remove CompositorThread struct

<!-- Please describe your changes on the following line: -->
Remove `CompositorThread` struct, since it wraps only `IOCompositor`. Use `IOCompositor` directly.

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

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

<!-- 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="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/11569)
<!-- Reviewable:end -->
@talklittle
Copy link
Contributor Author

talklittle commented Jun 3, 2016

@metajack The decision to remove NullCompositor was discussed in #8573. tl;dr: maintaining the NullCompositor alongside the IOCompositor was a headache, and easily forgotten, making it easy to introduce bugs. Instead, the normal compositor can be run in headless mode.
@metajack
Copy link
Contributor

metajack commented Jun 3, 2016

Thanks for the answer. Now I remember :)
@bors-servo
Copy link
Contributor

bors-servo commented Jun 3, 2016

💔 Test failed - linux-rel
@highfive
Copy link

highfive commented Jun 3, 2016 

  ▶ FAIL [expected PASS] /css-transforms-1_dev/html/transform-abspos-002.htm
  └   → /css-transforms-1_dev/html/transform-abspos-002.htm 71f0313eedfbfcce0b0fcc5ae55f34b1daa3b8d8
/css-transforms-1_dev/html/reference/transform-abspos-ref.htm 78d197606924062e8dd2a773c977afcecf8940f8
Testing 71f0313eedfbfcce0b0fcc5ae55f34b1daa3b8d8 == 78d197606924062e8dd2a773c977afcecf8940f8
@talklittle
compositing: Remove CompositorThread struct
Loading status checks…
9c8d5d5
@talklittle talklittle force-pushed the talklittle:remove_struct_10261 branch from d2aad37 to 9c8d5d5 Jun 3, 2016
@highfive
Copy link

highfive commented Jun 3, 2016

New code was committed to pull request.
@talklittle
Copy link
Contributor Author

talklittle commented Jun 3, 2016

I rebased and ran locally ./mach test-css --release /css-transforms-1_dev/html/transform-abspos-002.htm

And get success:

Running 1 tests in web-platform-tests

Unsupported test type wdspec for product servo
Ran 1 tests finished in 3.0 seconds.
  • 1 ran as expected. 0 tests skipped.

Is the test failure possibly a heisenbug, or am I missing something?
@Ms2ger
Copy link
Contributor

Ms2ger commented Jun 3, 2016

@bors-servo retry
@Ms2ger
Copy link
Contributor

Ms2ger commented Jun 3, 2016

@bors-servo r=metajack
@bors-servo
Copy link
Contributor

bors-servo commented Jun 3, 2016

📌 Commit 9c8d5d5 has been approved by metajack
@bors-servo
Copy link
Contributor

bors-servo commented Jun 3, 2016

⌛ Testing commit 9c8d5d5 with merge 6c5f5d3...
bors-servo added a commit that referenced this pull request Jun 3, 2016
@bors-servo
Auto merge of #11569 - talklittle:remove_struct_10261, r=metajack
Loading status checks…
6c5f5d3 
compositing: Remove CompositorThread struct

<!-- Please describe your changes on the following line: -->
Remove `CompositorThread` struct, since it wraps only `IOCompositor`. Use `IOCompositor` directly.

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

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

<!-- 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="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/11569)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Jun 3, 2016

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel, mac-dev-unit, mac-rel-css, mac-rel-wpt, windows
@bors-servo bors-servo merged commit 9c8d5d5 into servo:master Jun 3, 2016
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
@talklittle talklittle deleted the talklittle:remove_struct_10261 branch Jun 3, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@metajack metajack

Labels
None yet
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

Remove CompositorThread struct?
6 participants
@talklittle @highfive @metajack @bors-servo @Ms2ger @larsbergstrom
You can’t perform that action at this time.