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

Pass more parameters to player's constructor #266

Merged
merged 5 commits into from May 28, 2019

Conversation

@ceyusa
Copy link
Contributor

ceyusa commented May 17, 2019

This pull request is mainly about passing more parameters to player's constructor.

Also it includes a patch to make PlayerGLContext's enums as seriable in order to share them through IPC.

Finally, a cosmetic change which removes currently unused external crates.

@ceyusa
Copy link
Contributor Author

ceyusa commented May 17, 2019

@ferjm r?

Copy link
Member

ferjm left a comment

As mentioned on IRC, given that Servo is currently using a single FrameRenderer and a single PlayerEvent observer, I would prefer to keep it simple and remove the list of renderers and observers. We can extend the API in the future if we find any use case where we'd like to have multiple renderers and/or observers.

@@ -69,7 +69,9 @@ pub enum StreamType {
}

pub trait Player: Send {
/// Register an IpcSender that will push PlayerEvents generated by the playerq

This comment has been minimized.

@ferjm

ferjm May 17, 2019

Member

typo: playerq

@ceyusa ceyusa force-pushed the ceyusa:player-constructor branch 2 times, most recently from 6cd30e8 to 1e411dc May 17, 2019
ceyusa added 5 commits May 7, 2019
Marked its enums as clone, debug, deserialize and serialize.

This is required to send the trait trough threads (Servo's
constellation, fo example).
Player cannot work correctly without an event handler, so intead of
registering after the player is instantianted, it is passed as a
constructor parameter.

In the case of the frame renderer, if it non rendered is set, the
video track in the player should be disabled. This patch pass the
frame renderer decorated with an Option, so if it is none, the video
track is disabled, thus removing disable_video() method.

Fixes: #258
Now the player holds a single event handler and a single frame
renderer, simplifying the code.
@ceyusa ceyusa force-pushed the ceyusa:player-constructor branch from 1e411dc to b67121b May 27, 2019
@ceyusa ceyusa mentioned this pull request May 27, 2019
3 of 5 tasks complete
@ferjm
ferjm approved these changes May 28, 2019
Copy link
Member

ferjm left a comment

Sorry for the delay! I missed that you force pushed. In order to avoid future delays, ask for review again after you force push, please. Thanks! :)

@ferjm
Copy link
Member

ferjm commented May 28, 2019

@bors-servo
Copy link
Contributor

bors-servo commented May 28, 2019

📌 Commit b67121b has been approved by ferjm

@bors-servo
Copy link
Contributor

bors-servo commented May 28, 2019

Testing commit b67121b with merge 082496e...

bors-servo added a commit that referenced this pull request May 28, 2019
Pass more parameters to player's constructor

This pull request is mainly about passing more parameters to player's constructor.

Also it includes a patch to make PlayerGLContext's enums as seriable in order to share them through IPC.

Finally, a cosmetic change which removes currently unused external crates.
@bors-servo
Copy link
Contributor

bors-servo commented May 28, 2019

☀️ Test successful - checks-travis
Approved by: ferjm
Pushing 082496e to master...

@bors-servo bors-servo merged commit b67121b into servo:master May 28, 2019
2 checks passed
2 checks passed
Travis CI - Pull Request Build Passed
Details
homu Test successful
Details
@ceyusa
Copy link
Contributor Author

ceyusa commented May 28, 2019

In order to avoid future delays, ask for review again after you force push, please.

Ok! Thanks :)

@ceyusa ceyusa deleted the ceyusa:player-constructor branch May 28, 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.