New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 726564 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
inactive
Closed: May 2017
Cc:
EstimatedDays: ----
NextAction: ----
OS: Android
Pri: 2
Type: Bug



Sign in to add a comment

Place background color behind SurfaceView

Project Member Reported by aelias@chromium.org, May 26 2017

Issue description

According to Android folks, SurfaceView holding off on surfaceRedrawNeeded only locks the system thumbnail placeholder in place in cases where we're early enough, and a thumbnail exists in the first place.  Otherwise we still need to provide a background color as early as possible.
 
Project Member

Comment 1 by bugdroid1@chromium.org, May 26 2017

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

commit f7b1b07daa4fb6337f9806a00e3a299aeb613641
Author: aelias <aelias@chromium.org>
Date: Fri May 26 17:55:02 2017

Make CompositorView be visible and have a background color on creation.

This moves the visibility and background color setting for the
placeholder CompositorView in the constructor (before native is loaded).
This gives the View system the opportunity to draw a white frame earlier
-- in the current codepath we set the SurfaceView and CompositorView
visibility at the same time on native load, it appears that they race
and black is shown when the SurfaceView frame is suppressed by
surfaceRedrawAsyncNeeded.

This patch doesn't change visibility timing of the SurfaceView itself,
because it has the important side effect of suppressing surfaceCreated
and surfaceChanged when the native side is not yet ready to handle them.

BUG= 726564 

Review-Url: https://codereview.chromium.org/2905113003
Cr-Commit-Position: refs/heads/master@{#475056}

[modify] https://crrev.com/f7b1b07daa4fb6337f9806a00e3a299aeb613641/chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorSurfaceManager.java
[modify] https://crrev.com/f7b1b07daa4fb6337f9806a00e3a299aeb613641/chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorView.java
[modify] https://crrev.com/f7b1b07daa4fb6337f9806a00e3a299aeb613641/chrome/android/javatests/src/org/chromium/chrome/browser/compositor/CompositorVisibilityTest.java

Comment 2 by aelias@chromium.org, May 26 2017

Labels: Merge-Request-60
Project Member

Comment 3 by sheriffbot@chromium.org, May 26 2017

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

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

Comment 4 by bugdroid1@chromium.org, May 27 2017

Labels: -merge-approved-60 merge-merged-3112
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/b93ab0e63eb9b449b985a5cb0dba576b31db2f40

commit b93ab0e63eb9b449b985a5cb0dba576b31db2f40
Author: Alexandre Elias <aelias@chromium.org>
Date: Sat May 27 00:20:15 2017

Make CompositorView be visible and have a background color on creation.

This moves the visibility and background color setting for the
placeholder CompositorView in the constructor (before native is loaded).
This gives the View system the opportunity to draw a white frame earlier
-- in the current codepath we set the SurfaceView and CompositorView
visibility at the same time on native load, it appears that they race
and black is shown when the SurfaceView frame is suppressed by
surfaceRedrawAsyncNeeded.

This patch doesn't change visibility timing of the SurfaceView itself,
because it has the important side effect of suppressing surfaceCreated
and surfaceChanged when the native side is not yet ready to handle them.

BUG= 726564 
TBR=aelias@chromium.org

(cherry picked from commit f7b1b07daa4fb6337f9806a00e3a299aeb613641)

Review-Url: https://codereview.chromium.org/2905113003
Cr-Original-Commit-Position: refs/heads/master@{#475056}
Change-Id: Ie82467abc85edd9b42cf9e0ffdd3d2b54ffd0ee5
Reviewed-on: https://chromium-review.googlesource.com/517440
Reviewed-by: Alexandre Elias <aelias@chromium.org>
Cr-Commit-Position: refs/branch-heads/3112@{#9}
Cr-Branched-From: b6460e24cf59f429d69de255538d0fc7a425ccf9-refs/heads/master@{#474897}
[modify] https://crrev.com/b93ab0e63eb9b449b985a5cb0dba576b31db2f40/chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorSurfaceManager.java
[modify] https://crrev.com/b93ab0e63eb9b449b985a5cb0dba576b31db2f40/chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorView.java
[modify] https://crrev.com/b93ab0e63eb9b449b985a5cb0dba576b31db2f40/chrome/android/javatests/src/org/chromium/chrome/browser/compositor/CompositorVisibilityTest.java

Comment 5 by aelias@chromium.org, May 30 2017

Cc: racarr@google.com amineer@chromium.org
Labels: Merge-Request-59
Requesting merge of https://chromium-review.googlesource.com/c/517674/to M59.  This is the change above but adjusted to only apply to Android O to reduce risk.  I tested the 61 build with the change above against News & Weather.

Comment 6 by aelias@chromium.org, May 30 2017

Sorry, the correct link is https://chromium-review.googlesource.com/c/517674/
Project Member

Comment 7 by sheriffbot@chromium.org, May 30 2017

Labels: -Merge-Request-59 Merge-Review-59 Hotlist-Merge-Review
This bug requires manual review: We are only 6 days from stable.
Please contact the milestone owner if you have questions.
Owners: amineer@(Android), cmasso@(iOS), gkihumba@(ChromeOS), Abdul Syed@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Labels: -Merge-Review-59 Merge-Approved-59
Approved for M59 branch 3071.  Please ensure you merge before 5 PM PT today (preferably sooner).
Project Member

Comment 9 by bugdroid1@chromium.org, May 30 2017

Labels: -merge-approved-59 merge-merged-3071
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/2ad6f19334c54cdbbfdf43aa96aa868421610fea

commit 2ad6f19334c54cdbbfdf43aa96aa868421610fea
Author: Alexandre Elias <aelias@chromium.org>
Date: Tue May 30 19:32:32 2017

Cherry-pick white background fix to M59.

This cherry-picks http://crrev.com/475056, adjusted to apply only to
Android O to lower risk (since the bug being fixed only appears there).

BUG= 726564 

Change-Id: I188eeb7132a60ab61120dfb493bf11aab34e0072
Reviewed-on: https://chromium-review.googlesource.com/517674
Reviewed-by: Alexandre Elias <aelias@chromium.org>
Cr-Commit-Position: refs/branch-heads/3071@{#723}
Cr-Branched-From: a106f0abbf69dad349d4aaf4bcc4f5d376dd2377-refs/heads/master@{#464641}
[modify] https://crrev.com/2ad6f19334c54cdbbfdf43aa96aa868421610fea/chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorView.java

Status: Fixed (was: Assigned)

Sign in to add a comment