New issue
Advanced search Search tips

Issue 837807 link

Starred by 3 users

Issue metadata

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



Sign in to add a comment

VR Browsing - Content pane returns BLANK after initial search

Project Member Reported by dougman@chromium.org, Apr 27 2018

Issue description

Chrome Version: 67.0.3396.20 Dev
OS: Android 8.1.0 on Pixel 2

What steps will reproduce the problem?
1) Close all Chrome Tabs, and then close Chrome, outside VR.
2) Enter DD Home
3) Open Chrome from DD Home
4) Click the Omnibox to get the Keyboard
5) Search for and open a website

What is the expected result?
Website should be displayed normally.

What happens instead?
Content Pane is just a Black box.
Can't seems to repro without DD Home or having No Chrome Tabs open.

 
Screenshot_20180428-052811.png
251 KB View Download
This a regression? If so should be bisectable.
Labels: -Pri-2 M-67 Hotlist-VRB-MVP Pri-1
Cc: cjgrant@chromium.org
NTP is the initial experience you get, and if you search for one of the sites on the ntp (e.g. for me wikipedia.org), and click on the results in the omnibox, you get black as above.

if you go back to NTP, and click on wikipedia.org, it works fine.
Owner: mthiesse@chromium.org
Status: Started (was: Untriaged)
I'll dig into this.
Well whaddayaknow, another top controls bug.
Project Member

Comment 6 by bugdroid1@chromium.org, May 4 2018

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

commit 0f149108b4dbfaae84eb390718c44a9ddfb727af
Author: Michael Thiessen <mthiesse@chromium.org>
Date: Fri May 04 00:08:15 2018

Ensure initial top controls gets propagated when in VR for new RWHVA.

In some cases the Metadata for a RWHVA is set before VR code calls
SetIsInVr on the RWHVA, so we miss the initial top controls update.

(The only reason we care about VR-ness when updating the frame metadata
is that sending the initial top controls update breaks interstitials)

Bug:  837807 
Change-Id: Ic108fa324725e7c126c9f721c27e4c5f99521af6
Reviewed-on: https://chromium-review.googlesource.com/1042500
Reviewed-by: Bo <boliu@chromium.org>
Reviewed-by: Ted Choc <tedchoc@chromium.org>
Commit-Queue: Michael Thiessen <mthiesse@chromium.org>
Cr-Commit-Position: refs/heads/master@{#555918}
[modify] https://crrev.com/0f149108b4dbfaae84eb390718c44a9ddfb727af/chrome/android/java/src/org/chromium/chrome/browser/fullscreen/ChromeFullscreenManager.java
[modify] https://crrev.com/0f149108b4dbfaae84eb390718c44a9ddfb727af/chrome/android/java/src/org/chromium/chrome/browser/fullscreen/FullscreenManager.java
[modify] https://crrev.com/0f149108b4dbfaae84eb390718c44a9ddfb727af/chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java
[modify] https://crrev.com/0f149108b4dbfaae84eb390718c44a9ddfb727af/chrome/android/java/src/org/chromium/chrome/browser/tab/TabBrowserControlsOffsetHelper.java
[modify] https://crrev.com/0f149108b4dbfaae84eb390718c44a9ddfb727af/content/browser/renderer_host/render_widget_host_view_android.cc
[modify] https://crrev.com/0f149108b4dbfaae84eb390718c44a9ddfb727af/content/browser/renderer_host/render_widget_host_view_android.h

Project Member

Comment 7 by bugdroid1@chromium.org, May 4 2018

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

commit 3208e469223036763c7dcb2b288b8fb0efbae7e7
Author: Michael Thiessen <mthiesse@chromium.org>
Date: Fri May 04 19:55:44 2018

Actually call setPositionsForTab when in VR...

In https://chromium-review.googlesource.com/c/chromium/src/+/1042500 I
missed a commit which added a crucial line :)

TBR=tedchoc@chromium.org

Bug:  837807 
Change-Id: Ibc8f4f69e86ba16cfcff223c72b64481d9289549
Reviewed-on: https://chromium-review.googlesource.com/1044754
Reviewed-by: Michael Thiessen <mthiesse@chromium.org>
Commit-Queue: Michael Thiessen <mthiesse@chromium.org>
Cr-Commit-Position: refs/heads/master@{#556161}
[modify] https://crrev.com/3208e469223036763c7dcb2b288b8fb0efbae7e7/chrome/android/java/src/org/chromium/chrome/browser/tab/TabBrowserControlsOffsetHelper.java

Labels: Merge-Request-67
Verified on Canary.
Project Member

Comment 9 by sheriffbot@chromium.org, May 7 2018

Labels: -Merge-Request-67 Merge-Review-67 Hotlist-Merge-Review
This bug requires manual review: M67 has already been promoted to the beta branch, so this requires manual review
Please contact the 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
How safe is this change for M67?
I'm pretty confident that it only affects VR, so in the worst case the damage should be limited to VR users. I think it's very unlikely the behaviour with this fix will be worse than the previous behaviour while in VR.
Labels: -Hotlist-Merge-Review -Merge-Review-67 Merge-Approved-67
I've found some other related bugs so I'm going to hold off on merging this until I'm sure this CL wasn't the cause.
Cc: tedc...@chromium.org
Turns out when we go to reset the positions for the fullscreenManager after exiting VR, in some cases like the chrome://crash page there's no fullscreenManager associated with the tab, so we fail to reset the fullscreenmanager offsets.

I'll have a fix up shortly, but we shouldn't merge without the fix.
Project Member

Comment 15 by bugdroid1@chromium.org, May 10 2018

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

commit 57552aeb19797ad3871b842224caa2ca5dad2703
Author: Michael Thiessen <mthiesse@chromium.org>
Date: Thu May 10 18:22:28 2018

Always restore FullscreenManager offsets when exiting VR

When exiting VR on chrome://crash (sad tab), the renderer doesn't send
any control offset updates, and resetting the overrides to the control
offsets for VR restores the hidden control values, so we need to tell
the controls to show explicitly.

Also fixes initialization when opening a new tab while in VR...

Bug:  837807 
Change-Id: I14df5479156dc6ac981f845529f8c13aadbd15ac
Reviewed-on: https://chromium-review.googlesource.com/1052892
Reviewed-by: Ted Choc <tedchoc@chromium.org>
Commit-Queue: Michael Thiessen <mthiesse@chromium.org>
Cr-Commit-Position: refs/heads/master@{#557592}
[modify] https://crrev.com/57552aeb19797ad3871b842224caa2ca5dad2703/chrome/android/java/src/org/chromium/chrome/browser/tab/TabBrowserControlsOffsetHelper.java

Okay, going to wait until this hits Canary, make sure all the edge cases are covered, then ping you to confirm this is still okay cmasso@.

Thumbs up from our test team on this fix. I also verified all of the edge cases I could think of.

Comment 18 by cmasso@google.com, May 11 2018

All good!
Project Member

Comment 19 by bugdroid1@chromium.org, May 11 2018

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

commit 6a0b237050569f27ca0015bc2e4f54e29ab0936a
Author: Michael Thiessen <mthiesse@chromium.org>
Date: Fri May 11 20:14:48 2018

Ensure initial top controls gets propagated when in VR for new RWHVA.

In some cases the Metadata for a RWHVA is set before VR code calls
SetIsInVr on the RWHVA, so we miss the initial top controls update.

(The only reason we care about VR-ness when updating the frame metadata
is that sending the initial top controls update breaks interstitials)

TBR=mthiesse@chromium.org

(cherry picked from commit 0f149108b4dbfaae84eb390718c44a9ddfb727af)

Bug:  837807 
Change-Id: Ic108fa324725e7c126c9f721c27e4c5f99521af6
Reviewed-on: https://chromium-review.googlesource.com/1042500
Reviewed-by: Bo <boliu@chromium.org>
Reviewed-by: Ted Choc <tedchoc@chromium.org>
Commit-Queue: Michael Thiessen <mthiesse@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#555918}
Reviewed-on: https://chromium-review.googlesource.com/1055895
Reviewed-by: Michael Thiessen <mthiesse@chromium.org>
Cr-Commit-Position: refs/branch-heads/3396@{#571}
Cr-Branched-From: 9ef2aa869bc7bc0c089e255d698cca6e47d6b038-refs/heads/master@{#550428}
[modify] https://crrev.com/6a0b237050569f27ca0015bc2e4f54e29ab0936a/chrome/android/java/src/org/chromium/chrome/browser/fullscreen/ChromeFullscreenManager.java
[modify] https://crrev.com/6a0b237050569f27ca0015bc2e4f54e29ab0936a/chrome/android/java/src/org/chromium/chrome/browser/fullscreen/FullscreenManager.java
[modify] https://crrev.com/6a0b237050569f27ca0015bc2e4f54e29ab0936a/chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java
[modify] https://crrev.com/6a0b237050569f27ca0015bc2e4f54e29ab0936a/chrome/android/java/src/org/chromium/chrome/browser/tab/TabBrowserControlsOffsetHelper.java
[modify] https://crrev.com/6a0b237050569f27ca0015bc2e4f54e29ab0936a/content/browser/renderer_host/render_widget_host_view_android.cc
[modify] https://crrev.com/6a0b237050569f27ca0015bc2e4f54e29ab0936a/content/browser/renderer_host/render_widget_host_view_android.h

Project Member

Comment 20 by bugdroid1@chromium.org, May 11 2018

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

commit 589e55c87692fd9c3e71e57e3b046172a09828c4
Author: Michael Thiessen <mthiesse@chromium.org>
Date: Fri May 11 20:15:42 2018

Actually call setPositionsForTab when in VR...

In https://chromium-review.googlesource.com/c/chromium/src/+/1042500 I
missed a commit which added a crucial line :)

TBR=mthiesse@chromium.org, tedchoc@chromium.org

(cherry picked from commit 3208e469223036763c7dcb2b288b8fb0efbae7e7)

Bug:  837807 
Change-Id: Ibc8f4f69e86ba16cfcff223c72b64481d9289549
Reviewed-on: https://chromium-review.googlesource.com/1044754
Reviewed-by: Michael Thiessen <mthiesse@chromium.org>
Commit-Queue: Michael Thiessen <mthiesse@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#556161}
Reviewed-on: https://chromium-review.googlesource.com/1055897
Cr-Commit-Position: refs/branch-heads/3396@{#572}
Cr-Branched-From: 9ef2aa869bc7bc0c089e255d698cca6e47d6b038-refs/heads/master@{#550428}
[modify] https://crrev.com/589e55c87692fd9c3e71e57e3b046172a09828c4/chrome/android/java/src/org/chromium/chrome/browser/tab/TabBrowserControlsOffsetHelper.java

Project Member

Comment 21 by bugdroid1@chromium.org, May 11 2018

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

commit b39dfcd3d73df16b21b198f3579647524803364c
Author: Michael Thiessen <mthiesse@chromium.org>
Date: Fri May 11 20:16:46 2018

Always restore FullscreenManager offsets when exiting VR

When exiting VR on chrome://crash (sad tab), the renderer doesn't send
any control offset updates, and resetting the overrides to the control
offsets for VR restores the hidden control values, so we need to tell
the controls to show explicitly.

Also fixes initialization when opening a new tab while in VR...

TBR=mthiesse@chromium.org

(cherry picked from commit 57552aeb19797ad3871b842224caa2ca5dad2703)

Bug:  837807 
Change-Id: I14df5479156dc6ac981f845529f8c13aadbd15ac
Reviewed-on: https://chromium-review.googlesource.com/1052892
Reviewed-by: Ted Choc <tedchoc@chromium.org>
Commit-Queue: Michael Thiessen <mthiesse@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#557592}
Reviewed-on: https://chromium-review.googlesource.com/1055900
Reviewed-by: Michael Thiessen <mthiesse@chromium.org>
Cr-Commit-Position: refs/branch-heads/3396@{#573}
Cr-Branched-From: 9ef2aa869bc7bc0c089e255d698cca6e47d6b038-refs/heads/master@{#550428}
[modify] https://crrev.com/b39dfcd3d73df16b21b198f3579647524803364c/chrome/android/java/src/org/chromium/chrome/browser/tab/TabBrowserControlsOffsetHelper.java

Status: Fixed (was: Started)
Labels: Test-Complete
Status: Verified (was: Fixed)
Verified on build 67.0.3396.46 Beta, this build has not been released to beta yet.  A new build .46+ should be released later this week.  
Thing look good in this build.
Cc: dougman@chromium.org vsupruniuk@google.com
 Issue 840450  has been merged into this issue.

Sign in to add a comment