Plumb Xr Status into InputTransferHandler [chromium/src : main]
Alexander Cooper (Gerrit)
Alexander Cooper added 1 comment![]( "Open in Gerrit")
Bo PTAL InputTransferHandler and transfer_input_to_viz_result.h
Matt PTAL CompositorView
Michael PTAL enums.xml
Brian PTAL WebXrInputTransferTest
Aman Please review all.
Related details
- Aman Verma
- Bo Liu
- Brian Sheedy
- Matthew Jones
- Michael Thiessen
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Brian Sheedy (Gerrit)
Brian Sheedy added 6 comments![]( "Open in Gerrit")
CriteriaHelper.pollInstrumentationThread(
() -> {
Criteria.checkThat(
mScrollOffsetY, Matchers.greaterThan(initialScrollOffset));
});
CriteriaHelper.pollInstrumentationThread(
() -> {
Criteria.checkThat(
mLastSeenEventAction, Matchers.equalTo(MotionEvent.ACTION_CANCEL));
});Is it safe to assume that once we get the `ACTION_CANCEL` event action, the scroll offset should have updated? If so, we could probably swap the order and made the scroll check a single assert instead of a poll?
CriteriaHelper.pollInstrumentationThread(
() -> {
int numFrames =
Integer.parseInt(
mWebXrArTestFramework.runJavaScriptOrFail(
"window.numFramesWithInput",
WebXrArTestFramework.POLL_TIMEOUT_SHORT_MS));
Criteria.checkThat(numFrames, Matchers.greaterThanOrEqualTo(1));
});Nit: Could be shorter by using `pollJavaScriptBooleanOrFail()` and checking `window.numFramesWithInput >= 1`
CriteriaHelper.pollInstrumentationThread(
() -> {
int numSelects =
Integer.parseInt(
mWebXrArTestFramework.runJavaScriptOrFail(
"window.numSelectEvents",
WebXrArTestFramework.POLL_TIMEOUT_SHORT_MS));
Criteria.checkThat(numSelects, Matchers.equalTo(1));
});Ditto.
CriteriaHelper.pollInstrumentationThread(I don't think polling does anything here since we're checking that something didn't happen instead of waiting until something happens.
CriteriaHelper.pollInstrumentationThread(Ditto.
performScrollAndCheckTransfer(true);
// 2. During session: input should NOT be transferred to Viz.
mWebXrArTestFramework.enterSessionWithUserGestureOrFail();
performScrollAndCheckTransfer(false);
// 3. After session: input should be transferred to Viz.
mWebXrArTestFramework.endSession();
performScrollAndCheckTransfer(true);Is there a need to ensure that input has settled between these three steps?
Related details
- Alexander Cooper
- Aman Verma
- Bo Liu
- Matthew Jones
- Michael Thiessen
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. |
Aman Verma (Gerrit)
Message from Aman Verma![]( "Open in Gerrit")
Overall, the logic for blocking input transfer to Viz during WebXR/AR sessions looks solid and well-integrated. The new test provides good coverage for the expected behavior.
I have a few observations and questions:
- *CompositorView.java**
- The distinction between AR and VR behavior seems a bit inconsistent.
- `setOverlayImmersiveArMode` explicitly calls `handler.setIsInXr(enabled)`.
- `setOverlayVrMode` updates `mIsInXr = enabled` but does NOT update the existing handler.
- However, `surfaceChanged` calls `handler.setIsInXr(mIsInXr)` for any newly created handler. Since `mIsInXr` is true for both AR and VR, this means if a surface change occurs during a VR session, the input transfer WILL be blocked.
- The commit message mentions that VR input \"should remain unaffected.\" If this means VR input should still be transferred to Viz, then using the shared `mIsInXr` flag in `surfaceChanged` might inadvertently change VR's behavior. If \"unaffected\" means we don't care if it's blocked, it would be good to clarify.
- *WebXrInputTransferTest.java**
- I agree with the nits and suggestions from bsheedy@. Specifically:
- Using `pollJavaScriptBooleanOrFail()` on L157 and L166 would make the JS checks cleaner.
- The suggestion to swap the order on L145 and use a single assert if `ACTION_CANCEL` implies a scroll update is a nice cleanup.
**InputTransferHandler.java**
The check in `canTransferInputToViz` is straightforward and correctly uses the new `mIsInXr` state.
**Metrics**
The addition of `XrIsActive` to `TransferInputToVizResult` and `enums.xml` is correct.
Aman Verma (Gerrit)
Aman Verma added 6 comments![]( "Open in Gerrit")
Thanks Alex, conceptually SGTM, added a couple of small things.
assert (handler != null);Nit: parentheses here are redundant, `assert handler != null;`
public void setOverlayVrMode(boolean enabled) {Do we also want to update `InputTransferHandler` in this case?
if (expectTransfer) {Can we check if WebXR did not get any input frames here? Maybe reset the JS counters on line 110 irrespective of `!expectTransfer` and assert numFrames==0 here?
performScrollAndCheckTransfer(true);nit: `/*expectTransfer=*/ true`, same at other places
kXrIsActive = 17,Can you please add a TODO here about investigating InputEventObserver potentially, maybe file a bug?
Michael Thiessen (Gerrit)
Michael Thiessen voted and added 1 comment![]( "Open in Gerrit")
Votes added by Michael Thiessen
| Code-Review | +1 |
1 comment
histograms lgtm
Related details
- Alexander Cooper
- Aman Verma
- Bo Liu
- Matthew Jones
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. |
Bo Liu (Gerrit)
Bo Liu added 3 comments![]( "Open in Gerrit")
Side question.. was this AI generated review?
https://chromium-review.googlesource.com/c/chromium/src/+/7552845/3#message-dbd4a064dffb85b867840244f73d9d896c0f37ea
Weird interface that it's not a comment and no way to reply to it or anything..
public void setOverlayVrMode(boolean enabled) {Do we also want to update `InputTransferHandler` in this case?
+1
public class InputTransferHandler implements WindowAndroid.SelectionHandlesObserver {hmm... this is a lot of implementation in the content public API
Related details
- Alexander Cooper
- Aman Verma
- Matthew Jones
Bo Liu (Gerrit)
Bo Liu voted and added 1 comment![]( "Open in Gerrit")
Votes added by Bo Liu
| Code-Review | +1 |
1 comment
oh and content bits lgtm, I guess
Related details
- Alexander Cooper
- Aman Verma
- Matthew Jones
Code-Coverage
Code-Owners
Matthew Jones (Gerrit)
Matthew Jones voted and added 1 comment![]( "Open in Gerrit")
Votes added by Matthew Jones
| Code-Review | +1 |
1 comment
CompositorView lgtm
Related details
- Alexander Cooper
- Aman Verma
Aman Verma (Gerrit)
Aman Verma added 1 comment![]( "Open in Gerrit")
Side question.. was this AI generated review?
https://chromium-review.googlesource.com/c/chromium/src/+/7552845/3#message-dbd4a064dffb85b867840244f73d9d896c0f37eaWeird interface that it's not a comment and no way to reply to it or anything..
Yes, this was gemini-cli generated comment, was trying to see how good it is. Was running it in YOLO mode, lesson learned :P
Alexander Cooper (Gerrit)
Alexander Cooper voted and added 11 comments![]( "Open in Gerrit")
Votes added by Alexander Cooper
| Commit-Queue | +1 |
11 comments
Nit: parentheses here are redundant, `assert handler != null;`
Done
public void setOverlayVrMode(boolean enabled) {Bo LiuDo we also want to update `InputTransferHandler` in this case?
+1
I don't think VR needs it because it's input needs are different, but I think given the need to put the logic in `surfaceChanged` it's probably cleaner to do so.
I've created a helper method to minimize the duplication between the two methods, but I'm not enamored of the name if you have a better suggestion.
Can we check if WebXR did not get any input frames here? Maybe reset the JS counters on line 110 irrespective of `!expectTransfer` and assert numFrames==0 here?
I don't think that's necessary. We do other checks outside of this that should be sufficient to validate if we are in a WebXR session or not.
CriteriaHelper.pollInstrumentationThread(
() -> {
Criteria.checkThat(
mScrollOffsetY, Matchers.greaterThan(initialScrollOffset));
});
CriteriaHelper.pollInstrumentationThread(
() -> {
Criteria.checkThat(
mLastSeenEventAction, Matchers.equalTo(MotionEvent.ACTION_CANCEL));
});Is it safe to assume that once we get the `ACTION_CANCEL` event action, the scroll offset should have updated? If so, we could probably swap the order and made the scroll check a single assert instead of a poll?
Done
CriteriaHelper.pollInstrumentationThread(
() -> {
int numFrames =
Integer.parseInt(
mWebXrArTestFramework.runJavaScriptOrFail(
"window.numFramesWithInput",
WebXrArTestFramework.POLL_TIMEOUT_SHORT_MS));
Criteria.checkThat(numFrames, Matchers.greaterThanOrEqualTo(1));
});Nit: Could be shorter by using `pollJavaScriptBooleanOrFail()` and checking `window.numFramesWithInput >= 1`
Done
CriteriaHelper.pollInstrumentationThread(
() -> {
int numSelects =
Integer.parseInt(
mWebXrArTestFramework.runJavaScriptOrFail(
"window.numSelectEvents",
WebXrArTestFramework.POLL_TIMEOUT_SHORT_MS));
Criteria.checkThat(numSelects, Matchers.equalTo(1));
});Alexander CooperDitto.
Done
I don't think polling does anything here since we're checking that something didn't happen instead of waiting until something happens.
Done
CriteriaHelper.pollInstrumentationThread(Alexander CooperDitto.
Done
nit: `/*expectTransfer=*/ true`, same at other places
Done
performScrollAndCheckTransfer(true);
// 2. During session: input should NOT be transferred to Viz.
mWebXrArTestFramework.enterSessionWithUserGestureOrFail();
performScrollAndCheckTransfer(false);
// 3. After session: input should be transferred to Viz.
mWebXrArTestFramework.endSession();
performScrollAndCheckTransfer(true);Is there a need to ensure that input has settled between these three steps?
I don't think so. I think once the Transfer check is done it's okay to exit, plus we wait on the change in Xr state anyways, which should be sufficient to ensure things have settled down? LMK if you disagree though.
Can you please add a TODO here about investigating InputEventObserver potentially, maybe file a bug?
Done: crbug.com/483269973
I'll note though that I'm not sure we'll see similar input improvements in Xr mode though to make it worth it, up to you how time intensive it will be to investigate.
Related details
- Aman Verma
- Aman Verma
- Bo Liu
- Brian Sheedy
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. |
Aman Verma (Gerrit)
Aman Verma voted and added 2 comments![]( "Open in Gerrit")
Votes added by Aman Verma
| Code-Review | +1 |
2 comments
public void setOverlayVrMode(boolean enabled) {Bo LiuDo we also want to update `InputTransferHandler` in this case?
Alexander Cooper+1
I don't think VR needs it because it's input needs are different, but I think given the need to put the logic in `surfaceChanged` it's probably cleaner to do so.
I've created a helper method to minimize the duplication between the two methods, but I'm not enamored of the name if you have a better suggestion.
- Alexander Cooper
- Aman Verma
- Bo Liu
- Brian Sheedy
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Brian Sheedy (Gerrit)
Brian Sheedy voted and added 1 comment![]( "Open in Gerrit")
Votes added by Brian Sheedy
| Code-Review | +1 |
1 comment
Test code LGTM
Related details
- Alexander Cooper
- Aman Verma
- Bo Liu
Alexander Cooper (Gerrit)
Alexander Cooper voted and added 1 comment![]( "Open in Gerrit")
Votes added by Alexander Cooper
| Commit-Queue | +2 |
1 comment
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
Plumb Xr Status into InputTransferHandler
Configures the InputTransferHandler to be aware of WebXR sessions, which
should not have their input transferred to viz, because doing so blocks
any processing within the WebXR session.
- M chrome/android/BUILD.gn
- M chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorView.java
- A chrome/android/javatests/src/org/chromium/chrome/browser/vr/WebXrInputTransferTest.java
- A chrome/test/data/xr/e2e_test_files/html/test_webxr_input_transfer.html
- M content/public/android/java/src/org/chromium/content_public/browser/InputTransferHandler.java
- M content/public/browser/android/transfer_input_to_viz_result.h
- M tools/metrics/histograms/metadata/android/enums.xml
Code-Review: +1 by Matthew Jones, +1 by Brian Sheedy, +1 by Bo Liu, +1 by Aman Verma, +1 by Michael Thiessen
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |