-
Notifications
You must be signed in to change notification settings - Fork 128
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
[Ozone] Move ownership of SbWindow to the platform window class #4688
base: experimental/ozone
Are you sure you want to change the base?
Conversation
Move ownership of SbWindow to PlatformWindowStarboard, allowing us to get the proper initialization size and assign it as the platform window in the platform window delegate. b/388348504
@@ -39,6 +38,8 @@ int main(int argc, const char** argv) { | |||
"--disable-features=Vulkan", | |||
// Force some ozone settings. | |||
"--ozone-platform=starboard", "--use-gl=egl", | |||
// Set the default size for the content shell/starboard window. | |||
"--content-shell-host-window-size=1920x1080", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I chose an arbitrary size here for now, but I was thinking we could create a default SbOptions object and get the height and width from that if we wanted to initialize to those. Lmk thoughts on that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Leaving that Q for @y4vor )
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should use the existing SbWindowGetSize
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes LGTM !
Can we add unit tests? A little testing is better than no testing and in time, these small unit tests will enable sanitizer builds etc.
@@ -39,6 +38,8 @@ int main(int argc, const char** argv) { | |||
"--disable-features=Vulkan", | |||
// Force some ozone settings. | |||
"--ozone-platform=starboard", "--use-gl=egl", | |||
// Set the default size for the content shell/starboard window. | |||
"--content-shell-host-window-size=1920x1080", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Leaving that Q for @y4vor )
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes LGTM !
Can we add unit tests? A little testing is better than no testing and in time, these small unit tests will enable sanitizer builds etc.
Move ownership of SbWindow to PlatformWindowStarboard, allowing us to
get the proper initialization size and assign it as the platform window
in the platform window delegate.
b/388348504