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-7] fix issue when destroy a not fully initialized player #66

Conversation

jparez
Copy link
Contributor

@jparez jparez commented Mar 20, 2024

Description

When calling setupRender and destroy() in a loop destroy() destroy try to delete object not already load

see https://genymobile.atlassian.net/browse/PLAYER-7

Type of change

  • Bug fix (non-breaking change which fixes an issue)

@jparez jparez added the bug Something isn't working label Mar 20, 2024
@jparez jparez requested a review from pgivel March 20, 2024 13:49
@jparez jparez changed the base branch from main to integration/player-7-fix-issue-when-destroy-a-not-fully-initialized-player March 20, 2024 13:50
@jparez jparez force-pushed the dev/player-7-fix-issue-when-destroy-a-not-fully-initialized-player branch 2 times, most recently from 6395bc0 to cc6a7f7 Compare March 20, 2024 14:16
Copy link
Contributor

@pgivel pgivel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM as is, but a couple suggestions for improvment.

Comment on lines 879 to 883
/**
* if the websocket is in connecting state,
* we can't destroy the instance cause object is not fully initialized
*/
if (this.webRTCWebsocket.readyState !== 0) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it could be a little bit cleaner to reverse the condition:

Suggested change
/**
* if the websocket is in connecting state,
* we can't destroy the instance cause object is not fully initialized
*/
if (this.webRTCWebsocket.readyState !== 0) {
/**
* if the websocket is in connecting state,
* we can't destroy the instance cause object is not fully initialized
*/
if (this.webRTCWebsocket.readyState === 0) {
return;
}

I don't know if we could also add an event listener for when the socket is ready ? Maybe it's useless since the page would get closed anyway? I was thinking of something like this:

Suggested change
/**
* if the websocket is in connecting state,
* we can't destroy the instance cause object is not fully initialized
*/
if (this.webRTCWebsocket.readyState !== 0) {
/**
* if the websocket is in connecting state,
* we can't destroy the instance cause object is not fully initialized
*/
if (this.webRTCWebsocket.readyState === 0) {
this.addListener(this.webRTCWebsocket, 'open', this.destroy.bind(this), {once: true});
return;
}

@jparez jparez force-pushed the dev/player-7-fix-issue-when-destroy-a-not-fully-initialized-player branch from cc6a7f7 to cfcdfd4 Compare March 20, 2024 15:17
@jparez jparez force-pushed the dev/player-7-fix-issue-when-destroy-a-not-fully-initialized-player branch from cfcdfd4 to abecc80 Compare March 20, 2024 15:18
@jparez jparez merged commit aeba690 into integration/player-7-fix-issue-when-destroy-a-not-fully-initialized-player Mar 20, 2024
1 check passed
@jparez jparez deleted the dev/player-7-fix-issue-when-destroy-a-not-fully-initialized-player branch March 20, 2024 15:20
jparez added a commit that referenced this pull request Mar 21, 2024
…stroy-a-not-fully-initialized-player

[PLAYER-7] fix issue when destroy a not fully initialized player
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants