New issue
Advanced search Search tips

Issue 747590 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Feb 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Android
Pri: 2
Type: Bug
Proj-VR
Proj-XR
Proj-XR-VR



Sign in to add a comment

VR offline chip overlaps with URL if offline state comes after URL change

Project Member Reported by cjgrant@chromium.org, Jul 21 2017

Issue description

ToT today: We cache rendered URL text, but not text position.  The URL moves if the offline chip appears/disappears.  We need to cache the rendered URL position.


 
Project Member

Comment 1 by bugdroid1@chromium.org, Jul 22 2017

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

commit d4de1c23d68363b5b9e78d31bb37b4ebbf881726
Author: Christopher Grant <cjgrant@chromium.org>
Date: Sat Jul 22 19:09:47 2017

VR: Ensure URL and offline security text do not overlap.

BUG= 747590 

Change-Id: I88129d8b84ae9852ae63d30898bd190c5c028381
Reviewed-on: https://chromium-review.googlesource.com/582083
Reviewed-by: Ian Vollick <vollick@chromium.org>
Commit-Queue: Christopher Grant <cjgrant@chromium.org>
Cr-Commit-Position: refs/heads/master@{#488863}
[modify] https://crrev.com/d4de1c23d68363b5b9e78d31bb37b4ebbf881726/chrome/browser/vr/elements/url_bar_texture.cc
[modify] https://crrev.com/d4de1c23d68363b5b9e78d31bb37b4ebbf881726/chrome/browser/vr/elements/url_bar_texture.h

Labels: Merge-Request-61
Project Member

Comment 3 by bugdroid1@chromium.org, Jul 24 2017

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

commit d4a941c61b23cf4a59c7e23655249155aea2d6e7
Author: Christopher Grant <cjgrant@chromium.org>
Date: Mon Jul 24 17:43:52 2017

VR: Add offline mode URL rendering unit test.

Check that:
- The security verbose text is present in offline mode oly.
- The URL moves when security verbose text is present.

BUG=

VR: Ensure URL and offline security text do not overlap.

BUG= 747590 

Change-Id: I2220ba6a2657a78a7fab1cb03fd23e6b02024bf9
Reviewed-on: https://chromium-review.googlesource.com/582934
Reviewed-by: Ian Vollick <vollick@chromium.org>
Commit-Queue: Christopher Grant <cjgrant@chromium.org>
Cr-Commit-Position: refs/heads/master@{#489020}
[modify] https://crrev.com/d4a941c61b23cf4a59c7e23655249155aea2d6e7/chrome/browser/vr/elements/url_bar_texture.cc
[modify] https://crrev.com/d4a941c61b23cf4a59c7e23655249155aea2d6e7/chrome/browser/vr/elements/url_bar_texture.h
[modify] https://crrev.com/d4a941c61b23cf4a59c7e23655249155aea2d6e7/chrome/browser/vr/elements/url_bar_texture_unittest.cc

Project Member

Comment 4 by sheriffbot@chromium.org, Jul 25 2017

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

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

Comment 5 by bugdroid1@chromium.org, Jul 25 2017

Labels: -merge-approved-61 merge-merged-3163
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/b1232cfddc43dae1b83621f6413d7d654e13c3dc

commit b1232cfddc43dae1b83621f6413d7d654e13c3dc
Author: Christopher Grant <cjgrant@chromium.org>
Date: Tue Jul 25 14:40:25 2017

VR: Ensure URL and security text do not overlap

BUG= 747590 
TBR=cjgrant@chromium.org

(cherry picked from commit d4de1c23d68363b5b9e78d31bb37b4ebbf881726)

Change-Id: I88129d8b84ae9852ae63d30898bd190c5c028381
Reviewed-on: https://chromium-review.googlesource.com/582083
Reviewed-by: Ian Vollick <vollick@chromium.org>
Commit-Queue: Christopher Grant <cjgrant@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#488863}
Reviewed-on: https://chromium-review.googlesource.com/584933
Reviewed-by: Christopher Grant <cjgrant@chromium.org>
Cr-Commit-Position: refs/branch-heads/3163@{#24}
Cr-Branched-From: ff259bab28b35d242e10186cd63af7ed404fae0d-refs/heads/master@{#488528}
[modify] https://crrev.com/b1232cfddc43dae1b83621f6413d7d654e13c3dc/chrome/browser/vr/elements/url_bar_texture.cc
[modify] https://crrev.com/b1232cfddc43dae1b83621f6413d7d654e13c3dc/chrome/browser/vr/elements/url_bar_texture.h

Status: Fixed (was: Started)
Hi Christopher, I'd like to manually verify this but I'm unsure how to go from an online page to an offline page while in Chrome VR. I was able to go from an offline page to an online page by hitting the back button. Do you have specific steps I can take to verify this manually? Thanks!
Status: Assigned (was: Fixed)
I don't know of a way, no.  IIRC, this bug showed when entering VR on an offline page.  The issue was that different parts of state appear asynchronously, and if they arrived in the wrong order, we'd incorrectly render the UI.

A great way to test this would be the UI testapp.  We can amend it to ratchet through a set of URL/offline/security states on demand.  This would give better test coverage, but wouldn't be a true "live on device" test.  What do you think?

I have https://chromium-review.googlesource.com/c/chromium/src/+/848254 up for review to force the UI to cycle through several page origins, including offline state. 
Owner: dbbrooks@chromium.org
This landed.  One can now use the testapp to view various origin configurations, and essentially verify this fix.
(let me know if you'd like help firing up the testapp!)
Labels: -Pri-1 Pri-2
Cc: dbbrooks@chromium.org
Status: Fixed (was: Assigned)
@dbbrooks, with Yash's priority change, I noticed this bug had its Fixed status removed during the previous discussion.  The bug was actually fixed a long time ago, and is now just waiting verification (although I don't even think that makes sense anymore, as the code has changed since).

Please feel free to *not* manually verify this one.

If you are looking to learn how to use the testapp for exercising odd UI cases, as always, let me now.
Owner: cjgrant@chromium.org
Labels: Test-Manual

Sign in to add a comment