[XR] Extend XrSceneCoreSessionManager to allow creating spatial panels and sufraces [chromium/src : main]
Nick Mondello (Gerrit)
Nick Mondello added 3 comments![]( "Open in Gerrit")
Clean CL! Nice to see the DFM structure working out.
if (mIsDisposed) return;Is there a reason that assertDisposed() isn't called here similar to the other setter methods?
void setKeyEntity(XrEntityHolder entityHolder);nit: I noticed some of the functions add @param through all these interfaces, can you keep it uniform across all interfaces? Unless you had some reasoning
Related details
- Gurmeet Kalra
- Oleh Desiatyrikov (xWF)
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Gurmeet Kalra (Gerrit)
Gurmeet Kalra added 5 comments![]( "Open in Gerrit")
This cls focuses on adding just new APIs. A change of this size adding tests is crucial to ensure correctness and prevent future regressions. I would recommend adding unit tests for the new implementation classes in chrome/android/modules/xr/internal/java/src/org/chromium/chrome/browser/xr/scenecore/.
List<MovableComponent> components = mEntity.getComponentsOfType(MovableComponent.class);The `detachFromEntity()` method is called right before this, which handles the removal of the component managed by this class. This loop is redundant and could unintentionally remove `MovableComponent` instances added by other parts of the code. It's better to only manage the component owned by this class. Recommend remove this loop.
List<ResizableComponent> components = mEntity.getComponentsOfType(ResizableComponent.class);AI suggested: `detachFromEntity()` is already called before this, which should handle removing the component managed by this class. This loop seems redundant and could remove `ResizableComponent`s added by other parts of the application, leading to unexpected behavior. Please remove it.
if (mIsDisposed) return;For consistency with other methods in this class and its superclass, any reason why `assertDisposed()` not called here? This ensures that use-after-dispose errors are caught early.
super.dispose();The `mMovableComponent` and `mResizableComponent` are not being disposed of here, which could lead to resource leaks. Please add `mMovableComponent.dispose()` and `mResizableComponent.dispose()` before the call to `super.dispose()`.
Related details
- Oleh Desiatyrikov (xWF)
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Nick Mondello (Gerrit)
Nick Mondello added 1 comment![]( "Open in Gerrit")
This cls focuses on adding just new APIs. A change of this size adding tests is crucial to ensure correctness and prevent future regressions. I would recommend adding unit tests for the new implementation classes in chrome/android/modules/xr/internal/java/src/org/chromium/chrome/browser/xr/scenecore/.
How testable is scenecore? I know with GTV there were many blockers that prevented compose unit testing other than basic checks to see if the proper functions were called
Oleh Desiatyrikov (xWF) (Gerrit)
Oleh Desiatyrikov (xWF) added 6 comments![]( "Open in Gerrit")
List<MovableComponent> components = mEntity.getComponentsOfType(MovableComponent.class);The `detachFromEntity()` method is called right before this, which handles the removal of the component managed by this class. This loop is redundant and could unintentionally remove `MovableComponent` instances added by other parts of the code. It's better to only manage the component owned by this class. Recommend remove this loop.
Done
List<ResizableComponent> components = mEntity.getComponentsOfType(ResizableComponent.class);AI suggested: `detachFromEntity()` is already called before this, which should handle removing the component managed by this class. This loop seems redundant and could remove `ResizableComponent`s added by other parts of the application, leading to unexpected behavior. Please remove it.
Done
Is there a reason that assertDisposed() isn't called here similar to the other setter methods?
Done
For consistency with other methods in this class and its superclass, any reason why `assertDisposed()` not called here? This ensures that use-after-dispose errors are caught early.
Done
The `mMovableComponent` and `mResizableComponent` are not being disposed of here, which could lead to resource leaks. Please add `mMovableComponent.dispose()` and `mResizableComponent.dispose()` before the call to `super.dispose()`.
Done
nit: I noticed some of the functions add @param through all these interfaces, can you keep it uniform across all interfaces? Unless you had some reasoning
- Gurmeet Kalra
- Nick Mondello
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Oleh Desiatyrikov (xWF) (Gerrit)
Oleh Desiatyrikov (xWF) added 1 comment![]( "Open in Gerrit")
Nick MondelloThis cls focuses on adding just new APIs. A change of this size adding tests is crucial to ensure correctness and prevent future regressions. I would recommend adding unit tests for the new implementation classes in chrome/android/modules/xr/internal/java/src/org/chromium/chrome/browser/xr/scenecore/.
How testable is scenecore? I know with GTV there were many blockers that prevented compose unit testing other than basic checks to see if the proper functions were called
I see there is a androidx.xr.scenecore:scenecore-testing package. [link](https://cs.android.com/androidx/platform/frameworks/support/+/androidx-main:xr/scenecore/scenecore-testing/src/main/kotlin/androidx/xr/scenecore/testing/)
I'll try to add some basic unit tests to cover the API but this will probably land as a separate CL.
Nick Mondello (Gerrit)
Nick Mondello voted Code-Review+1![]( "Open in Gerrit")
- Gurmeet Kalra
- Oleh Desiatyrikov (xWF)
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Gurmeet Kalra (Gerrit)
Gurmeet Kalra voted and added 2 comments![]( "Open in Gerrit")
Votes added by Gurmeet Kalra
| Code-Review | +1 |
Nick MondelloThis cls focuses on adding just new APIs. A change of this size adding tests is crucial to ensure correctness and prevent future regressions. I would recommend adding unit tests for the new implementation classes in chrome/android/modules/xr/internal/java/src/org/chromium/chrome/browser/xr/scenecore/.
Oleh Desiatyrikov (xWF)How testable is scenecore? I know with GTV there were many blockers that prevented compose unit testing other than basic checks to see if the proper functions were called
I see there is a androidx.xr.scenecore:scenecore-testing package. [link](https://cs.android.com/androidx/platform/frameworks/support/+/androidx-main:xr/scenecore/scenecore-testing/src/main/kotlin/androidx/xr/scenecore/testing/)
I'll try to add some basic unit tests to cover the API but this will probably land as a separate CL.
Please create a P1 bug and add a TODO in each of the Impl classed so that the tests are added.
Related details
- Oleh Desiatyrikov (xWF)
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Oleh Desiatyrikov (xWF) (Gerrit)
Oleh Desiatyrikov (xWF) added 1 comment![]( "Open in Gerrit")
Nick MondelloThis cls focuses on adding just new APIs. A change of this size adding tests is crucial to ensure correctness and prevent future regressions. I would recommend adding unit tests for the new implementation classes in chrome/android/modules/xr/internal/java/src/org/chromium/chrome/browser/xr/scenecore/.
Oleh Desiatyrikov (xWF)How testable is scenecore? I know with GTV there were many blockers that prevented compose unit testing other than basic checks to see if the proper functions were called
Gurmeet KalraI see there is a androidx.xr.scenecore:scenecore-testing package. [link](https://cs.android.com/androidx/platform/frameworks/support/+/androidx-main:xr/scenecore/scenecore-testing/src/main/kotlin/androidx/xr/scenecore/testing/)
I'll try to add some basic unit tests to cover the API but this will probably land as a separate CL.
Please create a P1 bug and add a TODO in each of the Impl classed so that the tests are added.
- Gurmeet Kalra
- Nick Mondello
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |