player: Remove CreateInstance() from some player classes#9244
player: Remove CreateInstance() from some player classes#9244kjyoun merged 6 commits intoyoutube:mainfrom
Conversation
These methods were useful back when thread management was a more manual process and classes like PlayerWorker::CreateInstance() could return nullptr and this had to trickle up the caller hierarchy. This is no longer the case since PRs like youtube#9027 and youtube#8064, and the constructors can be called directly now. Fixed: 477798131
🤖 Gemini Suggested Commit Message💡 Pro Tips for a Better Commit Message:
|
There was a problem hiding this comment.
Code Review
This pull request refactors the player creation logic by removing the static CreateInstance() factory methods from SbPlayerPrivateImpl and PlayerWorker in favor of direct constructor calls. This is a good simplification, as the factory methods are no longer necessary after recent changes to thread management. The changes are well-executed, including the move to initialize worker_ in the member initializer list using std::make_unique and making it a const std::unique_ptr. I have one minor suggestion to improve the clarity of an error message.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request refactors the player creation logic by removing static CreateInstance() factory methods from SbPlayerPrivateImpl and PlayerWorker classes, opting for direct constructor calls to simplify code and improve clarity with modern C++ practices. However, this change introduces a potential null pointer dereference in starboard/android/shared/player_create.cc by removing a crucial validity check before using the newly created player object. Additionally, important error reporting logic for player creation failures has been removed. While the refactoring is well-motivated, these security and robustness concerns must be addressed, and there's also a suggestion to further align the code with modern C++ best practices.
kjyoun
left a comment
There was a problem hiding this comment.
Thanks for making this chage.
This is the one that I wanted to make.
I left some style comments.
|
@kjyoun thanks for reviewing this. are you able to merge this PR as well? |
|
Done. thanks for making this change! |
These methods were useful back when thread management was a more manual process and classes like PlayerWorker::CreateInstance() could return nullptr and this had to trickle up the caller hierarchy.
This is no longer the case since PRs like #9027 and #8064, and the constructors can be called directly now.
Fixed: 477798131