Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2921023002:
[VrShell] FullscreenActivity is not supported when in VR. (Closed)
Description[VrShell] FullscreenActivity is not supported when in VR.
BUG=729068
Review-Url: https://codereview.chromium.org/2921023002
Cr-Commit-Position: refs/heads/master@{#477132}
Committed: https://chromium.googlesource.com/chromium/src/+/1a721f49cac31e4294a8d1898aaf612042f5f18c
Patch Set 1 #
Messages
Total messages: 16 (9 generated)
amp@chromium.org changed reviewers: + peconn@chromium.org
This fixes fullscreen in VR by disabling the full screen activity support. peconn@, do you see any problems with this approach? Also this should allow all existing tests to function regardless of the feature state.
On 2017/06/02 18:06:09, amp wrote: > This fixes fullscreen in VR by disabling the full screen activity support. > > peconn@, do you see any problems with this approach? > > Also this should allow all existing tests to function regardless of the feature > state. I don't like the idea of choosing between FullscreenActivity and the legacy Activity at runtime - the two should never interact. However I do plan on making FullscreenActivity and VR work, so this would be a temporary solution. Are the VR tests broken, or is VR itself broken when we use FullscreenActivity? If it's only the tests I'd prefer to disable FullscreenActivity for the tests (and then fix the tests later), however if all of VR is broken then LGTM.
On 2017/06/05 15:24:27, PEConn wrote: > On 2017/06/02 18:06:09, amp wrote: > > This fixes fullscreen in VR by disabling the full screen activity support. > > > > peconn@, do you see any problems with this approach? > > > > Also this should allow all existing tests to function regardless of the > feature > > state. > > I don't like the idea of choosing between FullscreenActivity and the legacy > Activity at runtime - the two should never interact. However I do plan on making > FullscreenActivity and VR work, so this would be a temporary solution. > > Are the VR tests broken, or is VR itself broken when we use FullscreenActivity? > If it's only the tests I'd prefer to disable FullscreenActivity for the tests > (and then fix the tests later), however if all of VR is broken then LGTM. Yes this completely breaks fullscreen in VR, it appears to visually kick the user of out VR but Chrome still thinks it's in VR so controls don't show up. I still need approval from a CTA owner, but I wanted to make sure this was ok as a mitigation for the time being. Is the plan to get rid of the legacy fullscreen entirely? Feel free to ping me offline to discuss what the best way for VR to handle this should be. As long as the underlying web contents gets the signal and we don't draw an overlay on the screen then I think VR should be ok with the new implementation.
The CQ bit was checked by amp@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
amp@chromium.org changed reviewers: + tedchoc@chromium.org
+tedchoc, for OWNERS review of CTA.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
The CQ bit was checked by amp@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 1, "attempt_start_ts": 1496708694450800, "parent_rev":
"b386a80386b9e1208af15c2a902c9d97bc6f214c", "commit_rev":
"1a721f49cac31e4294a8d1898aaf612042f5f18c"}
Message was sent while issue was closed.
Description was changed from ========== [VrShell] FullscreenActivity is not supported when in VR. BUG=729068 ========== to ========== [VrShell] FullscreenActivity is not supported when in VR. BUG=729068 Review-Url: https://codereview.chromium.org/2921023002 Cr-Commit-Position: refs/heads/master@{#477132} Committed: https://chromium.googlesource.com/chromium/src/+/1a721f49cac31e4294a8d1898aaf... ==========
Message was sent while issue was closed.
Committed patchset #1 (id:1) as https://chromium.googlesource.com/chromium/src/+/1a721f49cac31e4294a8d1898aaf... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
