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

Miscellaneous fixes to harden the constellation. #10295

Merged
merged 1 commit into from Apr 1, 2016
Merged

Miscellaneous fixes to harden the constellation. #10295

merged 1 commit into from Apr 1, 2016

Conversation

@asajeffrey
Copy link
Member

asajeffrey commented Mar 30, 2016

This change is Reviewable
@asajeffrey
Miscellaneous fixes to harden the constellation.
Loading status checks…
f9f1587
@asajeffrey asajeffrey mentioned this pull request Mar 30, 2016
Closed
@asajeffrey
Copy link
Member Author

asajeffrey commented Mar 30, 2016

Part of #10124 constellation hardening.
@nox
Copy link
Member

nox commented Mar 31, 2016

A few nits, feel free to r=me when you address them.

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

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

components/compositing/constellation.rs, line 714 [r1] (raw file):
Nit: please no unwrap_or_else for side-effects.

components/compositing/constellation.rs, line 1498 [r1] (raw file):
Nit: Please no unwrap_or_else for side-effects.

components/compositing/constellation.rs, line 1574 [r1] (raw file):
Nit: self.frames.remove(&frame_id).is_none()

components/compositing/constellation.rs, line 1575 [r1] (raw file):
I wonder, this could also happen if it receives a nonsensical frame_id value, right? So shouldn't this message just be about not finding a frame?

components/compositing/constellation.rs, line 1606 [r1] (raw file):
Shouldn't this message just be about not finding a pipeline?

Comments from the review on Reviewable.io
@nox nox assigned nox and unassigned larsbergstrom Mar 31, 2016
@asajeffrey
Copy link
Member Author

asajeffrey commented Mar 31, 2016

I'll submit another PR replacing unwrap_or_else by if let.

Review status: all files reviewed at latest revision, 5 unresolved discussions.

components/compositing/constellation.rs, line 1575 [r1] (raw file):
Well... the FrameId is only created by new_frame, which adds the id to the frames hash map, then only removed by close_frame. So the debug message is correct (and if we reword this one, we should probably reword all the other ones too, there's quite a lot of them in other PRs!).

components/compositing/constellation.rs, line 1606 [r1] (raw file):
Ditto.

Comments from the review on Reviewable.io
@asajeffrey
Copy link
Member Author

asajeffrey commented Mar 31, 2016

I should clarify: I can't submit a PR replacing unwrap_or_else by if let until this PR lands.
@asajeffrey
Copy link
Member Author

asajeffrey commented Mar 31, 2016

@bors-servo r=nox

Got the OK on IRC: http://logs.glob.uno/?c=mozilla%23servo#c397095
@bors-servo
Copy link
Contributor

bors-servo commented Mar 31, 2016

📌 Commit f9f1587 has been approved by nox
asajeffrey added a commit to asajeffrey/servo that referenced this pull request Mar 31, 2016
@asajeffrey
Replace side-effecting unwrap_or_else by if let in constellation.
c7e2f0d 
This addresses nox's comments in servo#10295.
@bors-servo
Copy link
Contributor

bors-servo commented Apr 1, 2016

⌛ Testing commit f9f1587 with merge 7f06b46...
bors-servo added a commit that referenced this pull request Apr 1, 2016
@bors-servo
Auto merge of #10295 - asajeffrey:remove-constellation-misc-panic, r=nox
Loading status checks…
7f06b46 
Miscellaneous fixes to harden the constellation.

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

bors-servo commented Apr 1, 2016

☀️ Test successful - android, arm32, linux-dev, linux-rel, mac-dev-unit, mac-rel-css, mac-rel-wpt, status-appveyor
@bors-servo bors-servo merged commit f9f1587 into servo:master Apr 1, 2016
3 checks passed
3 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
asajeffrey added a commit to asajeffrey/servo that referenced this pull request Apr 1, 2016
@asajeffrey
Replace side-effecting unwrap_or_else by if let in constellation.
b152721 
This addresses nox's comments in servo#10295.
@asajeffrey asajeffrey mentioned this pull request Apr 1, 2016
Merged
asajeffrey added a commit to asajeffrey/servo that referenced this pull request Apr 11, 2016
@asajeffrey
Replace side-effecting unwrap_or_else by if let in constellation.
ef84e1f 
This addresses nox's comments in servo#10295.
asajeffrey added a commit to asajeffrey/servo that referenced this pull request Apr 12, 2016
@asajeffrey
Replace side-effecting unwrap_or_else by if let in constellation.
0afb803 
This addresses nox's comments in servo#10295.
asajeffrey added a commit to asajeffrey/servo that referenced this pull request Apr 13, 2016
@asajeffrey
Replace side-effecting unwrap_or_else by if let in constellation.
05f1655 
This addresses nox's comments in servo#10295.
asajeffrey added a commit to asajeffrey/servo that referenced this pull request Apr 14, 2016
@asajeffrey
Replace side-effecting unwrap_or_else by if let in constellation.
0cca4a9 
This addresses nox's comments in servo#10295.
bors-servo added a commit that referenced this pull request Apr 15, 2016
@bors-servo
Auto merge of #10345 - asajeffrey:make-nox-happy, r=nox
Loading status checks…
6c5cfb7 
Replace side-effecting unwrap_or_else by if let in constellation.

This addresses @nox's comments in #10295.

<!-- 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/10345)
<!-- Reviewable:end -->
asajeffrey added a commit to asajeffrey/servo that referenced this pull request Apr 15, 2016
@asajeffrey
Replace side-effecting unwrap_or_else by if let in constellation.
b782d55 
This addresses nox's comments in servo#10295.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@nox nox

Labels
S-awaiting-merge
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
@asajeffrey @nox @bors-servo @larsbergstrom @highfive
You can’t perform that action at this time.