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

Issue 829523 link

Starred by 3 users

Issue metadata

Status: Fixed
Owner:
Closed: Apr 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Mac
Pri: 1
Type: Bug



Sign in to add a comment

Avoid flash during cross-process navigation on Mac

Project Member Reported by samans@chromium.org, Apr 5 2018

Issue description

crrev.com/c/995915 tries to avoid a flash during cross-process navigation by copying fallback surface id from the old RenderWidgetHostView to the new one. However, when I try the same approach on Mac I still see a flash. It's worth investigating what can be done on Mac to avoid such flashes.
 
Cc: samans@chromium.org
Owner: ccameron@chromium.org
Status: Assigned (was: Untriaged)
ccameron has started looking into this.
Project Member

Comment 2 by bugdroid1@chromium.org, Apr 6 2018

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

commit 0de9902ad3217bd525cd45ac92356be66b4fb1a4
Author: Christopher Cameron <ccameron@chromium.org>
Date: Fri Apr 06 21:20:06 2018

mac: Allow recycling the same compositor on navigation

Make the compositor recycling queue be LIFO.

Don't allocate a compositor until the widget is made visible. On
navigation, this allows us to delay the allocation until the previous
page's compositor has been recycled (so we can pick it up immediately).

Bug:  829523 
Change-Id: Ie949e7e73df17a3bfe06d146a1d0c9987ba0dba5
Reviewed-on: https://chromium-review.googlesource.com/999957
Reviewed-by: Saman Sami <samans@chromium.org>
Reviewed-by: ccameron <ccameron@chromium.org>
Commit-Queue: ccameron <ccameron@chromium.org>
Cr-Commit-Position: refs/heads/master@{#548931}
[modify] https://crrev.com/0de9902ad3217bd525cd45ac92356be66b4fb1a4/content/browser/renderer_host/browser_compositor_view_mac.mm

Cc: fsam...@chromium.org chrishtr@chromium.org ccameron@chromium.org
 Issue 818350  has been merged into this issue.
Project Member

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

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

commit e84d14b44632effa54f0bd52bbebe64fee877e9b
Author: Christopher Cameron <ccameron@chromium.org>
Date: Tue Apr 24 01:56:01 2018

mac: Take previous RWHVMac contents at navigation

When navigating, make the old RHWVMac dump its ui::Compositor in
TakeFallbackContentFrom, and have the new RWHVMac pick it up.

Ensure that the ui::AcceleratedWidgetMac preserve its contents through
this transition.

This is not enough to completely eliminate all flashes, because the
NSViews in play aren't being atomically swapped.

Bug:  829523 
Change-Id: Ica6eab8dcea7a7987e8b7f83dbfd24acb2c6f7c4
Reviewed-on: https://chromium-review.googlesource.com/1023653
Commit-Queue: ccameron <ccameron@chromium.org>
Reviewed-by: Saman Sami <samans@chromium.org>
Cr-Commit-Position: refs/heads/master@{#552954}
[modify] https://crrev.com/e84d14b44632effa54f0bd52bbebe64fee877e9b/content/browser/renderer_host/browser_compositor_view_mac.h
[modify] https://crrev.com/e84d14b44632effa54f0bd52bbebe64fee877e9b/content/browser/renderer_host/browser_compositor_view_mac.mm
[modify] https://crrev.com/e84d14b44632effa54f0bd52bbebe64fee877e9b/content/browser/renderer_host/render_widget_host_ns_view_client.h
[modify] https://crrev.com/e84d14b44632effa54f0bd52bbebe64fee877e9b/content/browser/renderer_host/render_widget_host_view_mac.mm
[modify] https://crrev.com/e84d14b44632effa54f0bd52bbebe64fee877e9b/ui/accelerated_widget_mac/accelerated_widget_mac.h
[modify] https://crrev.com/e84d14b44632effa54f0bd52bbebe64fee877e9b/ui/accelerated_widget_mac/accelerated_widget_mac.mm

Project Member

Comment 5 by bugdroid1@chromium.org, Apr 25 2018

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

commit faae3909d597f2c4fb0b8721ce43875b64322a5f
Author: Christopher Cameron <ccameron@chromium.org>
Date: Wed Apr 25 17:34:48 2018

mac: Eliminate remaining flashes during navigation

During navigation, in RenderFrameHostManager::CommitPending, when the
navigation results in creation of a new RenderWidgetHostView, the
sequence of events in that function is that
- the old RenderWidgetHostView is hidden
- the new RenderWidgetHostView takes content from the old one
- the new RenderWidgetHostView is shown

On macOS, this can result in a one-frame flicker where there is no
content present (between the hide and the show). Fix this by adding
a ScopedCocoaDisableScreenUpdates for the duration of CommitPending.
This will make all Cocoa changes in the function be atomic.

In order to include ScopedCocoaDisableScreenUpdates in a non-objective-C
file, move its ScopedCocoaDisableScreenUpdates implementation out of
the header file.

Bug:  829523 
Change-Id: If541ef8985cf7990d63a79435b179f84339a9372
Reviewed-on: https://chromium-review.googlesource.com/1024968
Reviewed-by: ccameron <ccameron@chromium.org>
Reviewed-by: Saman Sami <samans@chromium.org>
Reviewed-by: Antoine Labour <piman@chromium.org>
Commit-Queue: ccameron <ccameron@chromium.org>
Cr-Commit-Position: refs/heads/master@{#553628}
[modify] https://crrev.com/faae3909d597f2c4fb0b8721ce43875b64322a5f/content/browser/frame_host/render_frame_host_manager.cc
[modify] https://crrev.com/faae3909d597f2c4fb0b8721ce43875b64322a5f/ui/gfx/BUILD.gn
[modify] https://crrev.com/faae3909d597f2c4fb0b8721ce43875b64322a5f/ui/gfx/mac/scoped_cocoa_disable_screen_updates.h
[add] https://crrev.com/faae3909d597f2c4fb0b8721ce43875b64322a5f/ui/gfx/mac/scoped_cocoa_disable_screen_updates.mm

Comment 6 by creis@chromium.org, Apr 26 2018

Cc: creis@chromium.org
Components: UI>Browser>Navigation Internals>Sandbox>SiteIsolation
Labels: -Pri-3 M-67 OS-Mac Pri-1
Status: Started (was: Assigned)
Thanks for picking this up, and sorry for not seeing it until now!  Is this resolved as of r553628?

Note that there will be a significantly higher number of cross-process navigations in M67 when Site Isolation gets turned on.  Would it be possible to merge these changes to M67?
Labels: Merge-Request-67
Certainly. Adding merge request.
Project Member

Comment 8 by sheriffbot@chromium.org, Apr 26 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

Comment 9 by gov...@chromium.org, Apr 26 2018

Before we approve merge to M67, please answer followings:

* Are the changes listed at #4 & #5 well baked/verified in Canary, having enough automation tests coverage and safe to merge to M67?

Please note M67 is already in Beta, so merge bar is very high. Thank you.
I feel comfortable with merging this to 67 -- it feels safe to me and makes a big difference in user experience.
Labels: -Merge-Review-67 Merge-Approved-67
Approving merge to M67 branch 3396 based on comment #10. Please merge ASAP. Thank you.
Project Member

Comment 12 by bugdroid1@chromium.org, Apr 27 2018

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

commit d59fe1ba157dcbe29001e53be196075467ff1395
Author: Christopher Cameron <ccameron@chromium.org>
Date: Fri Apr 27 17:19:55 2018

mac: Take previous RWHVMac contents at navigation

When navigating, make the old RHWVMac dump its ui::Compositor in
TakeFallbackContentFrom, and have the new RWHVMac pick it up.

Ensure that the ui::AcceleratedWidgetMac preserve its contents through
this transition.

This is not enough to completely eliminate all flashes, because the
NSViews in play aren't being atomically swapped.

TBR=ccameron@chromium.org

(cherry picked from commit e84d14b44632effa54f0bd52bbebe64fee877e9b)

Bug:  829523 
Change-Id: Ica6eab8dcea7a7987e8b7f83dbfd24acb2c6f7c4
Reviewed-on: https://chromium-review.googlesource.com/1023653
Commit-Queue: ccameron <ccameron@chromium.org>
Reviewed-by: Saman Sami <samans@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#552954}
Reviewed-on: https://chromium-review.googlesource.com/1032937
Reviewed-by: ccameron <ccameron@chromium.org>
Cr-Commit-Position: refs/branch-heads/3396@{#347}
Cr-Branched-From: 9ef2aa869bc7bc0c089e255d698cca6e47d6b038-refs/heads/master@{#550428}
[modify] https://crrev.com/d59fe1ba157dcbe29001e53be196075467ff1395/content/browser/renderer_host/browser_compositor_view_mac.h
[modify] https://crrev.com/d59fe1ba157dcbe29001e53be196075467ff1395/content/browser/renderer_host/browser_compositor_view_mac.mm
[modify] https://crrev.com/d59fe1ba157dcbe29001e53be196075467ff1395/content/browser/renderer_host/render_widget_host_ns_view_client.h
[modify] https://crrev.com/d59fe1ba157dcbe29001e53be196075467ff1395/content/browser/renderer_host/render_widget_host_view_mac.mm
[modify] https://crrev.com/d59fe1ba157dcbe29001e53be196075467ff1395/ui/accelerated_widget_mac/accelerated_widget_mac.h
[modify] https://crrev.com/d59fe1ba157dcbe29001e53be196075467ff1395/ui/accelerated_widget_mac/accelerated_widget_mac.mm

Project Member

Comment 13 by bugdroid1@chromium.org, Apr 27 2018

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

commit e7bf99f6772fa04114058c529752bc1818b6c97d
Author: Christopher Cameron <ccameron@chromium.org>
Date: Fri Apr 27 17:27:09 2018

mac: Eliminate remaining flashes during navigation

During navigation, in RenderFrameHostManager::CommitPending, when the
navigation results in creation of a new RenderWidgetHostView, the
sequence of events in that function is that
- the old RenderWidgetHostView is hidden
- the new RenderWidgetHostView takes content from the old one
- the new RenderWidgetHostView is shown

On macOS, this can result in a one-frame flicker where there is no
content present (between the hide and the show). Fix this by adding
a ScopedCocoaDisableScreenUpdates for the duration of CommitPending.
This will make all Cocoa changes in the function be atomic.

In order to include ScopedCocoaDisableScreenUpdates in a non-objective-C
file, move its ScopedCocoaDisableScreenUpdates implementation out of
the header file.

TBR=ccameron@chromium.org

(cherry picked from commit faae3909d597f2c4fb0b8721ce43875b64322a5f)

Bug:  829523 
Change-Id: If541ef8985cf7990d63a79435b179f84339a9372
Reviewed-on: https://chromium-review.googlesource.com/1024968
Reviewed-by: ccameron <ccameron@chromium.org>
Reviewed-by: Saman Sami <samans@chromium.org>
Reviewed-by: Antoine Labour <piman@chromium.org>
Commit-Queue: ccameron <ccameron@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#553628}
Reviewed-on: https://chromium-review.googlesource.com/1032938
Cr-Commit-Position: refs/branch-heads/3396@{#348}
Cr-Branched-From: 9ef2aa869bc7bc0c089e255d698cca6e47d6b038-refs/heads/master@{#550428}
[modify] https://crrev.com/e7bf99f6772fa04114058c529752bc1818b6c97d/content/browser/frame_host/render_frame_host_manager.cc
[modify] https://crrev.com/e7bf99f6772fa04114058c529752bc1818b6c97d/ui/gfx/BUILD.gn
[modify] https://crrev.com/e7bf99f6772fa04114058c529752bc1818b6c97d/ui/gfx/mac/scoped_cocoa_disable_screen_updates.h
[add] https://crrev.com/e7bf99f6772fa04114058c529752bc1818b6c97d/ui/gfx/mac/scoped_cocoa_disable_screen_updates.mm

Status: Fixed (was: Started)
Merged, closing.

Comment 15 by creis@chromium.org, Apr 27 2018

Many thanks!  Just to confirm, is it ok that r552954 from comment 2 wasn't merged as well?  (I'm not familiar with whether that was necessary or not.)
r552954 is indeed needed -- it landed before the branch point (this was brought back up on my radar just before branch)

https://storage.googleapis.com/chromium-find-releases-static/0de.html#0de9902ad3217bd525cd45ac92356be66b4fb1a4

Comment 17 by creis@chromium.org, Apr 27 2018

Ah, sorry, I copied the wrong revision number, didn't I?  :)  Comment 2 was r548931 (67.0.3392.0), and r552954 (68.0.3405.0) was from comment 4 and got merged to M67 in comment 12.

So yes, agreed that we're all set!  Thanks again.

Sign in to add a comment