cobalt: Use activity Window surface for UI rendering#11538
Conversation
🤖 Gemini Suggested Commit Message💡 Pro Tips for a Better Commit Message:
|
abfea0c to
c37b117
Compare
There was a problem hiding this comment.
Code Review
This pull request refactors ContentViewRenderView to support rendering directly to the Activity's Window surface via a new WindowSurfaceBridge strategy, alongside the default child SurfaceView rendering path (SurfaceViewBridge). An abstract SurfaceBridge class is introduced to encapsulate these strategies. The feedback highlights a potential NullPointerException in WindowSurfaceBridge.connect() where windowAndroid.getActivity() is dereferenced without a null check.
Introduces support for rendering UI directly into the Activity's Window surface via Window.takeSurface(), as an alternative to an embedded child SurfaceView. This behavior is abstracted behind WindowSurfaceBridge and is controlled by the `--use-window-surface-for-ui` command-line flag (disabled by default). Issue: 494590075
c37b117 to
0e66bb7
Compare
jasonzhangxx
left a comment
There was a problem hiding this comment.
The change overall looks good to me, but we need @johnxwork for a final review.
|
|
||
| protected SurfaceBridge createSurfaceBridge() { | ||
| return new SurfaceBridge(); | ||
| if (CommandLine.getInstance().hasSwitch("use-window-surface-for-ui")) { |
There was a problem hiding this comment.
The function is called very early in CobaltActivity.createContent() right after CommandLine is initialized. My understanding is we have to use kimono gcl to run the experiments. And please double check if the change can be properly enabled via the experiments.
| if (!(surfaceCallback instanceof SurfaceHolder.Callback2)) { | ||
| return; | ||
| } | ||
| ((SurfaceHolder.Callback2) surfaceCallback).surfaceRedrawNeeded(holder); |
There was a problem hiding this comment.
The input callback is defined as SurfaceHolder.Callback, but takeSurface requires a SurfaceHolder.Callback2. While casting the surfaceCallback to SurfaceHolder.Callback2 is necessary to implement the redraw notification, please ensure that the cast here is intentional and safe.
| /** | ||
| * @return whether the surface view is initialized and ready to render. | ||
| */ | ||
| public boolean isInitialized() { |
There was a problem hiding this comment.
nit: this function was removed, but it appears that it was unused anyway.
|
|
||
| @Override | ||
| protected void connect( | ||
| SurfaceHolder.Callback surfaceCallback, WindowAndroid windowAndroid) { |
There was a problem hiding this comment.
I think adding a new method ContentViewRenderView.getWindow() in ContentViewRenderView and then retrieving the mWindow during the initialize() would be a cleaner, more consistent approach that aligns better with the SurfaceViewBridge initialization pattern.
| */ | ||
| @JNINamespace("cobalt") | ||
| public class ContentViewRenderView extends FrameLayout { | ||
| private static final String TAG = "cobalt"; |
There was a problem hiding this comment.
nit: use import static dev.cobalt.util.Log.TAG;
| mWindowSurfaceHolder = holder; | ||
| if (mSurfaceFormat != null) { | ||
| Log.i(TAG, "ContentViewRenderView: Applying pending format"); | ||
| mWindowSurfaceHolder.setFormat(mSurfaceFormat); |
There was a problem hiding this comment.
Should mSurfaceFormat be cleaned up once it is applied?
|
nit: title should be |
| new CobaltA11yHelper(this, mShellManager.getContentViewRenderView().getSurfaceView()); | ||
|
|
||
| maybeRegisterNetworkRecoveryObserver(); | ||
| new CobaltA11yHelper(this, mShellManager.getContentViewRenderView().getAnchorView()); |
There was a problem hiding this comment.
We should check if this breaks a11y.
Introduces support for rendering UI directly into the Activity's Window surface via Window.takeSurface(), as an alternative to an embedded child SurfaceView.
This behavior is abstracted behind WindowSurfaceBridge and is controlled by the
--use-window-surface-for-uicommand-line flag (disabled by default).Issue: 494590075