New issue
Advanced search Search tips

Issue 834297 link

Starred by 2 users

Issue metadata

Status: Verified
Owner:
Closed: Apr 2018
Components:
EstimatedDays: ----
NextAction: ----
OS: Android
Pri: 1
Type: Bug
Proj-XR
Proj-XR-VR



Sign in to add a comment

VR omnibox rendering is corrupt after handling controller clicks

Project Member Reported by cjgrant@chromium.org, Apr 18 2018

Issue description

Clicks on the omnibox are broken, and selection can't be visibly cleared.

The root cause is that a refactoring CL cleanly split texture measurement and layout, but user input comes between those steps.  Hence, we're redrawing a texture based on old state, rather than new state.
 
Project Member

Comment 1 by bugdroid1@chromium.org, Apr 18 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/779dea58e72c768815fd0db99e9e8262af068d8f

commit 779dea58e72c768815fd0db99e9e8262af068d8f
Author: Christopher Grant <cjgrant@chromium.org>
Date: Wed Apr 18 16:38:41 2018

VR: Fix text updates due to user input

The omnibox is a text element that updates according to user input.
User input happens after element layout, and since the text texture
heavily relies on layout, "dirtying" the text texture really means
laying it out again.

Prior to recent cleanup and refactoring, we mistakenly got away with
this - a text element could resize itself after the layout stage and
appear incorrectly.  Post-cleanup, the re-layout can't happen, but a new
bug was introduced where a post-layout phase could dirty a texture, and
the texture update phase would redraw and "clean" the texture without
doing layout.  The broken layout would then persist indefinitely.

To address the problem, elements may mark themselves as requiring
measurement for texture draw.  These elements will not be redrawn unless
their measurement step has happened.

There was a measured() DCHECK mechanism previously, that was taken out
because most textures no longer measure themselves.  This change
reintroduces that check in it's new form.

BUG= 834297 

Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel;luci.chromium.try:linux_optional_gpu_tests_rel;luci.chromium.try:mac_optional_gpu_tests_rel;luci.chromium.try:win_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_vr
Change-Id: Ib17315a5c053d487edfbf7b858c2e9541924c3a6
Reviewed-on: https://chromium-review.googlesource.com/1016064
Reviewed-by: Ian Vollick <vollick@chromium.org>
Commit-Queue: Christopher Grant <cjgrant@chromium.org>
Cr-Commit-Position: refs/heads/master@{#551714}
[modify] https://crrev.com/779dea58e72c768815fd0db99e9e8262af068d8f/chrome/browser/vr/elements/prompt.cc
[modify] https://crrev.com/779dea58e72c768815fd0db99e9e8262af068d8f/chrome/browser/vr/elements/prompt.h
[modify] https://crrev.com/779dea58e72c768815fd0db99e9e8262af068d8f/chrome/browser/vr/elements/spinner.cc
[modify] https://crrev.com/779dea58e72c768815fd0db99e9e8262af068d8f/chrome/browser/vr/elements/spinner.h
[modify] https://crrev.com/779dea58e72c768815fd0db99e9e8262af068d8f/chrome/browser/vr/elements/text.cc
[modify] https://crrev.com/779dea58e72c768815fd0db99e9e8262af068d8f/chrome/browser/vr/elements/text.h
[modify] https://crrev.com/779dea58e72c768815fd0db99e9e8262af068d8f/chrome/browser/vr/elements/textured_element.cc
[modify] https://crrev.com/779dea58e72c768815fd0db99e9e8262af068d8f/chrome/browser/vr/elements/textured_element.h
[modify] https://crrev.com/779dea58e72c768815fd0db99e9e8262af068d8f/chrome/browser/vr/elements/ui_texture.h
[modify] https://crrev.com/779dea58e72c768815fd0db99e9e8262af068d8f/chrome/browser/vr/elements/vector_icon.cc
[modify] https://crrev.com/779dea58e72c768815fd0db99e9e8262af068d8f/chrome/browser/vr/elements/vector_icon.h

Labels: Merge-Request-67
Project Member

Comment 3 by sheriffbot@chromium.org, Apr 19 2018

Labels: -Merge-Request-67 Merge-Approved-67 Hotlist-Merge-Approved
Your change meets the bar and is auto-approved for M67. Please go ahead and merge the CL to branch 3396 manually. Please contact milestone owner if you have questions.
Owners: cmasso@(Android), cmasso@(iOS), kbleicher@(ChromeOS), govind@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Project Member

Comment 4 by bugdroid1@chromium.org, Apr 19 2018

Labels: -merge-approved-67 merge-merged-3396
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/f6dd6bd127f7ff2a7075d9bf779de9ff20df882b

commit f6dd6bd127f7ff2a7075d9bf779de9ff20df882b
Author: Christopher Grant <cjgrant@chromium.org>
Date: Thu Apr 19 19:05:37 2018

VR: Fix text updates due to user input

The omnibox is a text element that updates according to user input.
User input happens after element layout, and since the text texture
heavily relies on layout, "dirtying" the text texture really means
laying it out again.

Prior to recent cleanup and refactoring, we mistakenly got away with
this - a text element could resize itself after the layout stage and
appear incorrectly.  Post-cleanup, the re-layout can't happen, but a new
bug was introduced where a post-layout phase could dirty a texture, and
the texture update phase would redraw and "clean" the texture without
doing layout.  The broken layout would then persist indefinitely.

To address the problem, elements may mark themselves as requiring
measurement for texture draw.  These elements will not be redrawn unless
their measurement step has happened.

There was a measured() DCHECK mechanism previously, that was taken out
because most textures no longer measure themselves.  This change
reintroduces that check in it's new form.

BUG= 834297 

Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel;luci.chromium.try:linux_optional_gpu_tests_rel;luci.chromium.try:mac_optional_gpu_tests_rel;luci.chromium.try:win_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_vr
Change-Id: Ib17315a5c053d487edfbf7b858c2e9541924c3a6
Reviewed-on: https://chromium-review.googlesource.com/1016064
Reviewed-by: Ian Vollick <vollick@chromium.org>
Commit-Queue: Christopher Grant <cjgrant@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#551714}(cherry picked from commit 779dea58e72c768815fd0db99e9e8262af068d8f)
Reviewed-on: https://chromium-review.googlesource.com/1019964
Reviewed-by: Christopher Grant <cjgrant@chromium.org>
Cr-Commit-Position: refs/branch-heads/3396@{#140}
Cr-Branched-From: 9ef2aa869bc7bc0c089e255d698cca6e47d6b038-refs/heads/master@{#550428}
[modify] https://crrev.com/f6dd6bd127f7ff2a7075d9bf779de9ff20df882b/chrome/browser/vr/elements/prompt.cc
[modify] https://crrev.com/f6dd6bd127f7ff2a7075d9bf779de9ff20df882b/chrome/browser/vr/elements/prompt.h
[modify] https://crrev.com/f6dd6bd127f7ff2a7075d9bf779de9ff20df882b/chrome/browser/vr/elements/spinner.cc
[modify] https://crrev.com/f6dd6bd127f7ff2a7075d9bf779de9ff20df882b/chrome/browser/vr/elements/spinner.h
[modify] https://crrev.com/f6dd6bd127f7ff2a7075d9bf779de9ff20df882b/chrome/browser/vr/elements/text.cc
[modify] https://crrev.com/f6dd6bd127f7ff2a7075d9bf779de9ff20df882b/chrome/browser/vr/elements/text.h
[modify] https://crrev.com/f6dd6bd127f7ff2a7075d9bf779de9ff20df882b/chrome/browser/vr/elements/textured_element.cc
[modify] https://crrev.com/f6dd6bd127f7ff2a7075d9bf779de9ff20df882b/chrome/browser/vr/elements/textured_element.h
[modify] https://crrev.com/f6dd6bd127f7ff2a7075d9bf779de9ff20df882b/chrome/browser/vr/elements/ui_texture.h
[modify] https://crrev.com/f6dd6bd127f7ff2a7075d9bf779de9ff20df882b/chrome/browser/vr/elements/vector_icon.cc
[modify] https://crrev.com/f6dd6bd127f7ff2a7075d9bf779de9ff20df882b/chrome/browser/vr/elements/vector_icon.h

Status: Fixed (was: Started)
Labels: Test-Complete
Status: Verified (was: Fixed)
Fix verified on build 67.0.3396.13 Dev.  Looks good.

Sign in to add a comment