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

Player shutdown #212

Merged
merged 1 commit into from Feb 25, 2019
Merged

Player shutdown #212

merged 1 commit into from Feb 25, 2019

Conversation

@ferjm
Copy link
Member

ferjm commented Feb 25, 2019

This should allow us to fix issues like servo/servo#22931

@ferjm
Copy link
Member Author

ferjm commented Feb 25, 2019

r? @ceyusa

fn shutdown(&self) -> Result<(), PlayerError> {
self.observers.lock().unwrap().clear();
self.renderers.lock().unwrap().clear();
self.stop()

This comment has been minimized.

@ceyusa

ceyusa Feb 25, 2019

Contributor

shouldn't stop be before the observers and renders clearing?

This comment has been minimized.

@ferjm

ferjm Feb 25, 2019

Author Member

Stopping will trigger shutdown related messages (i.e. state change message) that observers may not be able to handle anymore. Since this is an active request to shutdown it is safe to assume that the client already knows about the shutdown related notifications and makes the API easier to use (i.e. the client does not need to keep a shutdown flag to discard shutdown messages).

This comment has been minimized.

@ceyusa

ceyusa Feb 25, 2019

Contributor

ok!

@ceyusa
Copy link
Contributor

ceyusa commented Feb 25, 2019

r+

@ferjm
Copy link
Member Author

ferjm commented Feb 25, 2019

@bors-servo r=ceyusa

@bors-servo
Copy link
Contributor

bors-servo commented Feb 25, 2019

📌 Commit 0149ca5 has been approved by ceyusa

@bors-servo
Copy link
Contributor

bors-servo commented Feb 25, 2019

Testing commit 0149ca5 with merge 698458d...

bors-servo added a commit that referenced this pull request Feb 25, 2019
Player shutdown

This should allow us to fix issues like servo/servo#22931
@bors-servo
Copy link
Contributor

bors-servo commented Feb 25, 2019

☀️ Test successful - checks-travis
Approved by: ceyusa
Pushing 698458d to master...

@bors-servo bors-servo merged commit 0149ca5 into servo:master Feb 25, 2019
2 checks passed
2 checks passed
Travis CI - Pull Request Build Passed
Details
homu Test successful
Details
@ferjm ferjm deleted the ferjm:shutdown branch Feb 26, 2019
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

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