Avoid flash during cross-process navigation on Mac |
||||||||
Issue descriptioncrrev.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.
,
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
,
Apr 23 2018
Issue 818350 has been merged into this issue.
,
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
,
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
,
Apr 26 2018
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?
,
Apr 26 2018
Certainly. Adding merge request.
,
Apr 26 2018
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
,
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.
,
Apr 27 2018
I feel comfortable with merging this to 67 -- it feels safe to me and makes a big difference in user experience.
,
Apr 27 2018
Approving merge to M67 branch 3396 based on comment #10. Please merge ASAP. Thank you.
,
Apr 27 2018
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
,
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
,
Apr 27 2018
Merged, closing.
,
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.)
,
Apr 27 2018
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 |
||||||||
►
Sign in to add a comment |
||||||||
Comment 1 by samans@chromium.org
, Apr 6 2018Owner: ccameron@chromium.org
Status: Assigned (was: Untriaged)