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

Remove CollectMemoryReports from compositor (fixes #13735) #14087

Merged
merged 1 commit into from Nov 6, 2016

Conversation

@MichaelKohler
Copy link
Contributor

MichaelKohler commented Nov 5, 2016

Remove all of the code in compositor.rs related to:

  • sending the RegisterReporter message
  • sending the UnregisterReporter message
  • the mem_profiler_chan member of IOCompositor
  • move the code that sends the ProfilerMsg::Exit message into Constellation::handle_exit instead

  • ./mach build -d does not report any errors

  • ./mach test-tidy does not report any errors

  • These changes fix #13735 (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 Nov 5, 2016

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

@highfive
Copy link

highfive commented Nov 5, 2016

Heads up! This PR modifies the following files:

  • @asajeffrey: components/constellation/constellation.rs
match reporter_request.to::<ReporterRequest>() {
Err(e) => error!("Cast to ReporterRequest failed ({}).", e),
Ok(reporter_request) => {
let msg = Msg::CollectMemoryReports(reporter_request.reports_channel);

This comment has been minimized.

Copy link
@jdm

jdm Nov 5, 2016

Member

I suspect the code that receives and processes this message can be removed as well. Probably the message variant from the enum, too.

@MichaelKohler MichaelKohler force-pushed the MichaelKohler:issue13735 branch from ed2e404 to 1985ad6 Nov 6, 2016
@MichaelKohler
Copy link
Contributor Author

MichaelKohler commented Nov 6, 2016

@jdm thanks for the review, I've updated this PR

@jdm
Copy link
Member

jdm commented Nov 6, 2016

@bors-servo: r+
Thanks!

@bors-servo
Copy link
Contributor

bors-servo commented Nov 6, 2016

📌 Commit 1985ad6 has been approved by jdm

@highfive highfive assigned jdm and unassigned emilio Nov 6, 2016
@bors-servo
Copy link
Contributor

bors-servo commented Nov 6, 2016

Testing commit 1985ad6 with merge 41e0b6a...

bors-servo added a commit that referenced this pull request Nov 6, 2016
Remove CollectMemoryReports from compositor (fixes #13735)

Remove all of the code in compositor.rs related to:

* sending the RegisterReporter message
* sending the UnregisterReporter message
* the mem_profiler_chan member of IOCompositor
* move the code that sends the ProfilerMsg::Exit message into Constellation::handle_exit instead

---
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] These changes fix #13735 (github issue number if applicable).

- [ ] There are tests for these changes OR
- [x] These changes do not require tests because _____

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

bors-servo commented Nov 6, 2016

💔 Test failed - mac-rel-wpt1

@highfive
Copy link

highfive commented Nov 6, 2016

  ▶ Unexpected subtest result in /XMLHttpRequest/responsexml-document-properties.htm:
  │ FAIL [expected PASS] lastModified set to time of response if no HTTP header provided
  │   → assert_less_than_equal: expected a number less than or equal to 1478411709 but got 1478415309
  │ 
  │ @http://web-platform.test:8000/XMLHttpRequest/responsexml-document-properties.htm:39:9
  │ Test.prototype.step@http://web-platform.test:8000/resources/testharness.js:1401:20
  │ test@http://web-platform.test:8000/resources/testharness.js:501:9
  └ @http://web-platform.test:8000/XMLHttpRequest/responsexml-document-properties.htm:35:7

  ▶ Unexpected subtest result in /html/dom/documents/resource-metadata-management/document-lastModified-01.html:
  │ FAIL [expected PASS] Date returned by lastModified is current at page load
  │   → assert_approx_equals: expected 1478411913 +/- 2.5 but got 1478415513
  │ 
  │ @http://web-platform.test:8000/html/dom/documents/resource-metadata-management/document-lastModified-01.html:8:5
  │ Test.prototype.step@http://web-platform.test:8000/resources/testharness.js:1401:20
  │ test@http://web-platform.test:8000/resources/testharness.js:501:9
  └ @http://web-platform.test:8000/html/dom/documents/resource-metadata-management/document-lastModified-01.html:7:3

  ▶ Unexpected subtest result in /html/dom/documents/resource-metadata-management/document-lastModified-01.html:
  │ FAIL [expected PASS] Date returned by lastModified is current after timeout.
  │   → assert_approx_equals: (initial value was 1478415513) expected 1478411917 +/- 2.5 but got 1478415517
  │ 
  │ @http://web-platform.test:8000/html/dom/documents/resource-metadata-management/document-lastModified-01.html:41:7
  │ Test.prototype.step@http://web-platform.test:8000/resources/testharness.js:1401:20
  └ @http://web-platform.test:8000/html/dom/documents/resource-metadata-management/document-lastModified-01.html:38:5
@jdm
Copy link
Member

jdm commented Nov 6, 2016

@bors-servo
Copy link
Contributor

bors-servo commented Nov 6, 2016

Testing commit 1985ad6 with merge 72981db...

bors-servo added a commit that referenced this pull request Nov 6, 2016
Remove CollectMemoryReports from compositor (fixes #13735)

Remove all of the code in compositor.rs related to:

* sending the RegisterReporter message
* sending the UnregisterReporter message
* the mem_profiler_chan member of IOCompositor
* move the code that sends the ProfilerMsg::Exit message into Constellation::handle_exit instead

---
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] These changes fix #13735 (github issue number if applicable).

- [ ] There are tests for these changes OR
- [x] These changes do not require tests because _____

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

bors-servo commented Nov 6, 2016

💔 Test failed - linux-rel-wpt

@jdm
Copy link
Member

jdm commented Nov 6, 2016

@bors-servo
Copy link
Contributor

bors-servo commented Nov 6, 2016

Testing commit 1985ad6 with merge b5cf4db...

bors-servo added a commit that referenced this pull request Nov 6, 2016
Remove CollectMemoryReports from compositor (fixes #13735)

Remove all of the code in compositor.rs related to:

* sending the RegisterReporter message
* sending the UnregisterReporter message
* the mem_profiler_chan member of IOCompositor
* move the code that sends the ProfilerMsg::Exit message into Constellation::handle_exit instead

---
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] These changes fix #13735 (github issue number if applicable).

- [ ] There are tests for these changes OR
- [x] These changes do not require tests because _____

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

bors-servo commented Nov 6, 2016

@bors-servo bors-servo merged commit 1985ad6 into servo:master Nov 6, 2016
2 of 3 checks passed
2 of 3 checks passed
continuous-integration/appveyor/pr AppVeyor build failed
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.

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