fix: Disable compositor recycling only for attached views #20829

poiru · 2019-10-29T15:48:23Z

In #19873, we completely disabled compositor recycling. This has adverse
effects in our tabbed app where switching tabs (i.e. BrowserViews) now
results in a flicker because we now also switch compositors.

To fix this without regressing the original fix, we now recycle the
compositor when the view is removed from a window or explicitly marked
as hidden. These can only happen with BrowserViews so BrowserWindows
are unaffected.

Notes: Fix flicker when switching between BrowserViews

deepak1556 · 2019-10-29T18:47:38Z

patches/chromium/disable_compositor_recycling.patch

@@ -1,20 +1,25 @@
 From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001
-From: Andy Locascio <andy@slack-corp.com>
-Date: Wed, 21 Aug 2019 12:09:10 -0700
+From: Birunthan Mohanathas <birunthan@mohanathas.com>


Can you comma separate the authors ?

I reverted this bit.

deepak1556 · 2019-10-29T18:47:44Z

/cc @loc

Backport of electron#20829 Notes: Fix flicker when switching between `BrowserView`s

trop · 2019-10-29T19:42:19Z

@poiru has manually backported this PR to "6-1-x", please check out #20834

MarshallOfSound

Blocking this on a manual check the original issue has not resurfaced. We can't afford that that happening.

In electron#19873, we completely disabled compositor recycling. This has adverse effects in our tabbed app where switching tabs (i.e. `BrowserView`s) now results in a flicker because we now also switch compositors. To fix this without regressing the original fix, we now recycle the compositor when the view is removed from a window. This situation can only happen with `BrowserView`s and the common case with `BrowserWindow` is unaffected.

loc · 2019-10-30T00:14:24Z

This regresses the original fix. With the original fix, the RenderWidgetHost is never hidden as far as the compositor is concerned, whereas your patch allows it to be hidden if the NSView is hidden. I think the logic we actually want is more like:

render_widget_host_is_hidden_ = [GetInProcessNSView() window] ? false : hidden;

with hidden being the passed bool.

Edit: Oh I see you just pushed changes. Now it won't regress, but I still think we want to set it to the passed bool for the BrowserView case?
Edit 2: Ah, I was reading the patch wrong. Poiru set me straight in a DM.

poiru · 2019-10-30T00:18:05Z

@loc I actually just fixed that, please refresh the PR 😊

With the updated patch and this logging statement...

 void BrowserCompositorMac::SetRenderWidgetHostIsHidden(bool hidden) {
+  LOG(ERROR) << (hidden ? "COMPOSITOR HIDDEN" : "COMPOSITOR SHOWN");
   render_widget_host_is_hidden_ = hidden;
   UpdateState();
 }

... the results are as follows:

Minimizing and restoring window

COMPOSITOR SHOWN
COMPOSITOR SHOWN

Occluding and unoccluding window

COMPOSITOR SHOWN
COMPOSITOR SHOWN

Removing a BrowserView from a window and adding it back

COMPOSITOR HIDDEN
COMPOSITOR SHOWN

So I think we should be good to go!

Backport of electron#20829 Notes: Fix flicker when switching between `BrowserView`s

trop · 2019-10-30T12:52:34Z

@poiru has manually backported this PR to "7-0-x", please check out #20846

loc

This change looks good. I tested that the 6-1-x backport does not regress.

Upon further investigation though, it looks like we had already regressed on the original bug in Electron 7 and 8 — I'll make an issue and investigate further.

MarshallOfSound

Thanks for checking it out @loc

release-clerk · 2019-10-30T20:47:15Z

Release Notes Persisted

Fix flicker when switching between BrowserViews

…0847) Backport of #20829 Notes: Fix flicker when switching between `BrowserView`s

…0834) Backport of #20829 Notes: Fix flicker when switching between `BrowserView`s

…0846) Backport of #20829 Notes: Fix flicker when switching between `BrowserView`s

In electron#20829, we fixed compositor recycling when switching between BrowserViews, but it turns out that there is one additional case that we need to handle. When we create a completely new BrowserView instance, it starts of as visible (even when it hasn't been added to the window), which means that it will need its own compositor instead of using the recycled compositor. To fix this, lets make BrowserViews hidden by default until they're added to the window. See also electron#19988. This is a potentially breaking change given that the initial value of `document.visibilityState` will now be `hidden`, but given the experimental status of BrowserViews, I think this is a fine change to make. The old behavior can be restored with `webPreferences: { show: true }`. Notes: Fix compositor recycling when creating new BrowserView

In #20829, we fixed compositor recycling when switching between BrowserViews, but it turns out that there is one additional case that we need to handle. When we create a completely new BrowserView instance, it starts of as visible (even when it hasn't been added to the window), which means that it will need its own compositor instead of using the recycled compositor. To fix this, lets make BrowserViews hidden by default until they're added to the window. See also #19988. This is a potentially breaking change given that the initial value of `document.visibilityState` will now be `hidden`, but given the experimental status of BrowserViews, I think this is a fine change to make. The old behavior can be restored with `webPreferences: { show: true }`. Notes: Fix compositor recycling when creating new BrowserView

poiru requested a review from a team as a code owner October 29, 2019 15:48

electron-cation bot added the new-pr 🌱 PR opened in the last 24 hours label Oct 29, 2019

poiru mentioned this pull request Oct 29, 2019

fix: mitigate white screen flash after occlusion by disabling compositor recycling #19873

Merged

6 tasks

poiru force-pushed the recycle-compositor-for-removed-views branch 2 times, most recently from afbae72 to 8a67e74 Compare October 29, 2019 18:08

deepak1556 added target/6-1-x labels Oct 29, 2019

deepak1556 requested review from MarshallOfSound and zcbenz October 29, 2019 18:46

deepak1556 reviewed Oct 29, 2019

View reviewed changes

deepak1556 approved these changes Oct 29, 2019

View reviewed changes

poiru force-pushed the recycle-compositor-for-removed-views branch from 8a67e74 to 8cd600c Compare October 29, 2019 18:56

poiru added a commit to poiru/electron that referenced this pull request Oct 29, 2019

fix: Disable compositor recycling only for attached views (6-1-x)

db442a6

Backport of electron#20829 Notes: Fix flicker when switching between `BrowserView`s

poiru mentioned this pull request Oct 29, 2019

fix: Disable compositor recycling only for attached views (6-1-x) #20834

Merged

trop bot added in-flight/6-1-x and removed target/6-1-x labels Oct 29, 2019

MarshallOfSound requested changes Oct 29, 2019

View reviewed changes

poiru force-pushed the recycle-compositor-for-removed-views branch from 8cd600c to 05f385a Compare October 30, 2019 00:12

poiru added a commit to poiru/electron that referenced this pull request Oct 30, 2019

fix: Disable compositor recycling only for attached views (6-1-x)

9e1ae1d

Backport of electron#20829 Notes: Fix flicker when switching between `BrowserView`s

zcbenz approved these changes Oct 30, 2019

View reviewed changes

poiru added a commit to poiru/electron that referenced this pull request Oct 30, 2019

fix: Disable compositor recycling only for attached views (6-1-x)

f1fe9a1

Backport of electron#20829 Notes: Fix flicker when switching between `BrowserView`s

poiru added a commit to poiru/electron that referenced this pull request Oct 30, 2019

fix: Disable compositor recycling only for attached views (7-0-x)

e4fa79e

Backport of electron#20829 Notes: Fix flicker when switching between `BrowserView`s

poiru mentioned this pull request Oct 30, 2019

fix: Disable compositor recycling only for attached views (7-0-x) #20846

Merged

trop bot added in-flight/8-x-y and removed target/8-x-y labels Oct 30, 2019

electron-cation bot removed the new-pr 🌱 PR opened in the last 24 hours label Oct 30, 2019

loc approved these changes Oct 30, 2019

View reviewed changes

jkleinsc requested a review from MarshallOfSound October 30, 2019 20:44

MarshallOfSound approved these changes Oct 30, 2019

View reviewed changes

MarshallOfSound merged commit b275273 into electron:master Oct 30, 2019

poiru deleted the recycle-compositor-for-removed-views branch October 30, 2019 20:50

jkleinsc pushed a commit that referenced this pull request Oct 30, 2019

fix: Disable compositor recycling only for attached views (8-x-y) (#2…

55201d7

…0847) Backport of #20829 Notes: Fix flicker when switching between `BrowserView`s

jkleinsc pushed a commit that referenced this pull request Oct 30, 2019

fix: Disable compositor recycling only for attached views (6-1-x) (#2…

61fdda3

…0834) Backport of #20829 Notes: Fix flicker when switching between `BrowserView`s

trop bot added merged/6-1-x and removed in-flight/6-1-x labels Oct 30, 2019

jkleinsc pushed a commit that referenced this pull request Oct 30, 2019

fix: Disable compositor recycling only for attached views (7-0-x) (#2…

3841ac9

…0846) Backport of #20829 Notes: Fix flicker when switching between `BrowserView`s

trop bot added merged/7-0-x and removed in-flight/7-0-x labels Oct 30, 2019

codebytere added merged/8-x-y and removed in-flight/8-x-y labels Oct 31, 2019

sofianguy added this to Fixed in 7.0.1 in 7.2.x Nov 4, 2019

sofianguy added this to Fixed in 6.1.3 in 6.1.x Nov 5, 2019

sofianguy added this to Fixed in 8.0.0-beta.2 in 8.2.x Nov 5, 2019

poiru mentioned this pull request Dec 3, 2019

fix: Fix compositor recycling when creating new BrowserView #21372

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Disable compositor recycling only for attached views #20829

fix: Disable compositor recycling only for attached views #20829

poiru commented Oct 29, 2019

deepak1556 Oct 29, 2019

poiru Oct 29, 2019

deepak1556 commented Oct 29, 2019

trop bot commented Oct 29, 2019

MarshallOfSound left a comment

loc commented Oct 30, 2019 •

edited

poiru commented Oct 30, 2019 •

edited

trop bot commented Oct 30, 2019

loc left a comment

MarshallOfSound left a comment

release-clerk bot commented Oct 30, 2019

fix: Disable compositor recycling only for attached views #20829

fix: Disable compositor recycling only for attached views #20829

Conversation

poiru commented Oct 29, 2019

deepak1556 Oct 29, 2019

Choose a reason for hiding this comment

poiru Oct 29, 2019

Choose a reason for hiding this comment

deepak1556 commented Oct 29, 2019

trop bot commented Oct 29, 2019

MarshallOfSound left a comment

Choose a reason for hiding this comment

loc commented Oct 30, 2019 • edited

poiru commented Oct 30, 2019 • edited

trop bot commented Oct 30, 2019

loc left a comment

Choose a reason for hiding this comment

MarshallOfSound left a comment

Choose a reason for hiding this comment

release-clerk bot commented Oct 30, 2019

loc commented Oct 30, 2019 •

edited

poiru commented Oct 30, 2019 •

edited