Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2005013002:
mac: ensure ui::Compositor exists for visible RWHVs (Closed)
|
Created:
4 years, 7 months ago by enne (OOO) Modified:
4 years, 6 months ago CC:
chromium-reviews, darin-cc_chromium.org, jam, nona+watch_chromium.org, shuchen+watch_chromium.org, James Su, yusukes+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptionmac: ensure ui::Compositor exists for visible RWHVs
RenderWidgetHostViewMac is careful about creating ui::Compositor and so
currently only does this in Show and SwapCompositorFrame. However, if
it the RenderWidgetHostView is created while the RenderWidgetHost is
visible, then Show will early out and never create a ui::Compositor.
This causes problems with begin frame scheduling, which wants to have
the Display (which is created as part of creating the ui::Compositor's
output surface currently) own the real begin frame source that drives
everything. In that world, if no ui::Compositor exists, no frames ever
are sent or swapped. So, the ui::Compositor needs to be created first.
To avoid issues where there's inconsistencies between host impl hidden
state and browser compositor existence, always ensure that the browser
compositor exists in ::Show.
R=asvitkine@chromium.org,ccameron@chromium.org
Committed: https://crrev.com/a46e810069effce565efe42ccddfd13f74c5bc93
Cr-Commit-Position: refs/heads/master@{#401133}
Patch Set 1 #Patch Set 2 : Also clean up swap buffers #Patch Set 3 : Also clean up swap buffers #
Total comments: 2
Patch Set 4 : OOPS #Patch Set 5 : OOPS #Patch Set 6 : More unit test fixes #
Total comments: 3
Patch Set 7 : Add shutdown method #Patch Set 8 : Rebase #Patch Set 9 : Don't register display reconfig callback in tests #Patch Set 10 : Fix gn analyze #Patch Set 11 : More gn fixes #Patch Set 12 : Fix for gyp, remove display config callback #Patch Set 13 : Fixed hangs, more dchecks #Patch Set 14 : Move dcheck #
Total comments: 1
Patch Set 15 : Add speculative WillOccluded check like Hide #Patch Set 16 : Add speculative WillOccluded check like Hide #Patch Set 17 : Ensure in ctor and always in show #Messages
Total messages: 51 (21 generated)
lgtm
Description was changed from ========== mac: ensure ui::Compositor exists for visible RWHVs RenderWidgetHostViewMac is careful about creating ui::Compositor and so currently only does this in Show and SwapCompositorFrame. However, if it the RenderWidgetHostView is created while the RenderWidgetHost is visible, then Show will early out and never create a ui::Compositor. This causes problems with begin frame scheduling, which wants to have the Display (which is created as part of creating the ui::Compositor's output surface currently) own the real begin frame source that drives everything. In that world, if no ui::Compositor exists, no frames ever are sent or swapped. So, the ui::Compositor needs to be created first. R=asvitkine@chromium.org,ccameron@chromium.org ========== to ========== mac: ensure ui::Compositor exists for visible RWHVs RenderWidgetHostViewMac is careful about creating ui::Compositor and so currently only does this in Show and SwapCompositorFrame. However, if it the RenderWidgetHostView is created while the RenderWidgetHost is visible, then Show will early out and never create a ui::Compositor. This causes problems with begin frame scheduling, which wants to have the Display (which is created as part of creating the ui::Compositor's output surface currently) own the real begin frame source that drives everything. In that world, if no ui::Compositor exists, no frames ever are sent or swapped. So, the ui::Compositor needs to be created first. Also, if the ui::Compositor is correctly created during Show (or the constructor), then there's no need to ensure that it exists during swap compositor frame, and so that can be safely removed. R=asvitkine@chromium.org,ccameron@chromium.org ==========
Also went ahead and cleaned up swap buffers in this patch, as the two changes are tied together.
danakj@chromium.org changed reviewers: + danakj@chromium.org
https://codereview.chromium.org/2005013002/diff/40001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_mac.mm (right): https://codereview.chromium.org/2005013002/diff/40001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_mac.mm:1483: if (!browser_compositor->compositor()) { Is this inverted? if (!a) a->Foo() is a crash
https://codereview.chromium.org/2005013002/diff/40001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_mac.mm (right): https://codereview.chromium.org/2005013002/diff/40001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_mac.mm:1483: if (!browser_compositor->compositor()) { On 2016/05/24 00:34:40, danakj wrote: > Is this inverted? > > if (!a) > a->Foo() > > is a crash Just needs "if (browser_compositor)"
o7 "This change is too small to bother copying a patch file over. I'll just type it in directly." <_<
lgtm
Would also encourage to find a way to write a unit test for this, if possible.
Thanks. On 2016/05/24 at 17:32:46, asvitkine wrote: > Would also encourage to find a way to write a unit test for this, if possible. With https://codereview.chromium.org/1939253002 applied, this causes a large number of content browser tests to time out. It's a pretty fundamental thing and I think would be quite obvious if it ever regressed.
The CQ bit was checked by enne@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from ccameron@chromium.org, asvitkine@chromium.org Link to the patchset: https://codereview.chromium.org/2005013002/#ps100001 (title: "More unit test fixes")
The CQ bit was unchecked by enne@chromium.org
Ok, there's a few more tests that are failing that I needed to adjust. Sorry for the noise. https://codereview.chromium.org/2005013002/diff/100001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_mac.mm (right): https://codereview.chromium.org/2005013002/diff/100001/content/browser/render... content/browser/renderer_host/render_widget_host_view_mac.mm:1502: if (browser_compositor_ && browser_compositor_->compositor()) { I forgot to check if browser compositor also existed here, oops. https://codereview.chromium.org/2005013002/diff/100001/content/public/test/te... File content/public/test/test_renderer_host.cc (right): https://codereview.chromium.org/2005013002/diff/100001/content/public/test/te... content/public/test/test_renderer_host.cc:220: if (!ui::WindowResizeHelperMac::Get()->task_runner()) { I'd love some feedback on what I should do here. Here's the problem. content_unittests now creates a ui::Compositor where it didn't before. In Mac land, there's a dcheck that the WindowResizeHelperMac has a task runner initialized on it. This is a little bit weirdly global. I think maybe the right thing to do here is have a Shutdown() that clears it, so that I can always call Init/Shutdown between tests when there aren't any compositors using this task runner? Thoughts?
https://codereview.chromium.org/2005013002/diff/100001/content/public/test/te... File content/public/test/test_renderer_host.cc (right): https://codereview.chromium.org/2005013002/diff/100001/content/public/test/te... content/public/test/test_renderer_host.cc:220: if (!ui::WindowResizeHelperMac::Get()->task_runner()) { On 2016/05/26 00:07:06, enne wrote: > I'd love some feedback on what I should do here. Here's the problem. > content_unittests now creates a ui::Compositor where it didn't before. In Mac > land, there's a dcheck that the WindowResizeHelperMac has a task runner > initialized on it. > > This is a little bit weirdly global. I think maybe the right thing to do here > is have a Shutdown() that clears it, so that I can always call Init/Shutdown > between tests when there aren't any compositors using this task runner? > > Thoughts? Adding a Shutdown that tests use to clear this sounds reasonable.
enne@chromium.org changed reviewers: + phajdan.jr@chromium.org
phajdan.jr: content/public/test and content/test OWNERS
LGTM
The CQ bit was checked by enne@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from asvitkine@chromium.org, ccameron@chromium.org Link to the patchset: https://codereview.chromium.org/2005013002/#ps220001 (title: "Fix for gyp, remove display config callback")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2005013002/220001
Message was sent while issue was closed.
Committed patchset #12 (id:220001)
Message was sent while issue was closed.
Description was changed from ========== mac: ensure ui::Compositor exists for visible RWHVs RenderWidgetHostViewMac is careful about creating ui::Compositor and so currently only does this in Show and SwapCompositorFrame. However, if it the RenderWidgetHostView is created while the RenderWidgetHost is visible, then Show will early out and never create a ui::Compositor. This causes problems with begin frame scheduling, which wants to have the Display (which is created as part of creating the ui::Compositor's output surface currently) own the real begin frame source that drives everything. In that world, if no ui::Compositor exists, no frames ever are sent or swapped. So, the ui::Compositor needs to be created first. Also, if the ui::Compositor is correctly created during Show (or the constructor), then there's no need to ensure that it exists during swap compositor frame, and so that can be safely removed. R=asvitkine@chromium.org,ccameron@chromium.org ========== to ========== mac: ensure ui::Compositor exists for visible RWHVs RenderWidgetHostViewMac is careful about creating ui::Compositor and so currently only does this in Show and SwapCompositorFrame. However, if it the RenderWidgetHostView is created while the RenderWidgetHost is visible, then Show will early out and never create a ui::Compositor. This causes problems with begin frame scheduling, which wants to have the Display (which is created as part of creating the ui::Compositor's output surface currently) own the real begin frame source that drives everything. In that world, if no ui::Compositor exists, no frames ever are sent or swapped. So, the ui::Compositor needs to be created first. Also, if the ui::Compositor is correctly created during Show (or the constructor), then there's no need to ensure that it exists during swap compositor frame, and so that can be safely removed. R=asvitkine@chromium.org,ccameron@chromium.org Committed: https://crrev.com/9af6c23da7070f9423d5cf25856ed27f7d5d8e5e Cr-Commit-Position: refs/heads/master@{#397755} ==========
Message was sent while issue was closed.
Patchset 12 (id:??) landed as https://crrev.com/9af6c23da7070f9423d5cf25856ed27f7d5d8e5e Cr-Commit-Position: refs/heads/master@{#397755}
Message was sent while issue was closed.
A revert of this CL (patchset #12 id:220001) has been created in https://codereview.chromium.org/2040013002/ by enne@chromium.org. The reason for reverting is: Causes blank tabs, tab switching perf tests BUG=617497,617427.
Ok, this is finally ready to reland. ccameron, did you want to take another look at rwhvmac and see if you're ok with the changes I made there and the extra dchecks? https://codereview.chromium.org/2005013002/diff/260001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_mac.mm (right): https://codereview.chromium.org/2005013002/diff/260001/content/browser/render... content/browser/renderer_host/render_widget_host_view_mac.mm:927: // Re-check hidden flag, as the thumbnail generation could have called This was the cause of the white tabs. I added a bunch more dchecks to try to avoid this. Show now makes sure that if it early outs, that the browser is already created. Creating and destroying the browser compositor also verify that the RWH hidden state is as expected. This also fixes the webrtc regression. The tab switching test was extremely noisy when run locally but it looked slightly better with this change, so will reland and see what the bots look like.
On 2016/06/15 22:07:44, enne wrote: > Ok, this is finally ready to reland. ccameron, did you want to take another > look at rwhvmac and see if you're ok with the changes I made there and the extra > dchecks? This lgtm, except insofar as RWHVMac is getting to a be a very complicated piece of machinery ... we'll probably want to see if there's a way to clean it up at some point. This business of recycling ui::Compositors that I added is causing lots of problems. > > https://codereview.chromium.org/2005013002/diff/260001/content/browser/render... > File content/browser/renderer_host/render_widget_host_view_mac.mm (right): > > https://codereview.chromium.org/2005013002/diff/260001/content/browser/render... > content/browser/renderer_host/render_widget_host_view_mac.mm:927: // Re-check > hidden flag, as the thumbnail generation could have called > This was the cause of the white tabs. I added a bunch more dchecks to try to > avoid this. Show now makes sure that if it early outs, that the browser is > already created. Creating and destroying the browser compositor also verify > that the RWH hidden state is as expected. > > This also fixes the webrtc regression. The tab switching test was extremely > noisy when run locally but it looked slightly better with this change, so will > reland and see what the bots look like.
Description was changed from ========== mac: ensure ui::Compositor exists for visible RWHVs RenderWidgetHostViewMac is careful about creating ui::Compositor and so currently only does this in Show and SwapCompositorFrame. However, if it the RenderWidgetHostView is created while the RenderWidgetHost is visible, then Show will early out and never create a ui::Compositor. This causes problems with begin frame scheduling, which wants to have the Display (which is created as part of creating the ui::Compositor's output surface currently) own the real begin frame source that drives everything. In that world, if no ui::Compositor exists, no frames ever are sent or swapped. So, the ui::Compositor needs to be created first. Also, if the ui::Compositor is correctly created during Show (or the constructor), then there's no need to ensure that it exists during swap compositor frame, and so that can be safely removed. R=asvitkine@chromium.org,ccameron@chromium.org Committed: https://crrev.com/9af6c23da7070f9423d5cf25856ed27f7d5d8e5e Cr-Commit-Position: refs/heads/master@{#397755} ========== to ========== mac: ensure ui::Compositor exists for visible RWHVs RenderWidgetHostViewMac is careful about creating ui::Compositor and so currently only does this in Show and SwapCompositorFrame. However, if it the RenderWidgetHostView is created while the RenderWidgetHost is visible, then Show will early out and never create a ui::Compositor. This causes problems with begin frame scheduling, which wants to have the Display (which is created as part of creating the ui::Compositor's output surface currently) own the real begin frame source that drives everything. In that world, if no ui::Compositor exists, no frames ever are sent or swapped. So, the ui::Compositor needs to be created first. Also, if the ui::Compositor is correctly created during Show (or the constructor), then there's no need to ensure that it exists during swap compositor frame, and so that can be safely removed. R=asvitkine@chromium.org,ccameron@chromium.org ==========
Description was changed from ========== mac: ensure ui::Compositor exists for visible RWHVs RenderWidgetHostViewMac is careful about creating ui::Compositor and so currently only does this in Show and SwapCompositorFrame. However, if it the RenderWidgetHostView is created while the RenderWidgetHost is visible, then Show will early out and never create a ui::Compositor. This causes problems with begin frame scheduling, which wants to have the Display (which is created as part of creating the ui::Compositor's output surface currently) own the real begin frame source that drives everything. In that world, if no ui::Compositor exists, no frames ever are sent or swapped. So, the ui::Compositor needs to be created first. Also, if the ui::Compositor is correctly created during Show (or the constructor), then there's no need to ensure that it exists during swap compositor frame, and so that can be safely removed. R=asvitkine@chromium.org,ccameron@chromium.org Committed: https://crrev.com/9af6c23da7070f9423d5cf25856ed27f7d5d8e5e Cr-Commit-Position: refs/heads/master@{#397755} ========== to ========== mac: ensure ui::Compositor exists for visible RWHVs RenderWidgetHostViewMac is careful about creating ui::Compositor and so currently only does this in Show and SwapCompositorFrame. However, if it the RenderWidgetHostView is created while the RenderWidgetHost is visible, then Show will early out and never create a ui::Compositor. This causes problems with begin frame scheduling, which wants to have the Display (which is created as part of creating the ui::Compositor's output surface currently) own the real begin frame source that drives everything. In that world, if no ui::Compositor exists, no frames ever are sent or swapped. So, the ui::Compositor needs to be created first. Also, if the ui::Compositor is correctly created during Show (or the constructor), then there's no need to ensure that it exists during swap compositor frame, and so that can be safely removed. R=asvitkine@chromium.org,ccameron@chromium.org ==========
The CQ bit was checked by enne@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from asvitkine@chromium.org, phajdan.jr@chromium.org Link to the patchset: https://codereview.chromium.org/2005013002/#ps260001 (title: "Move dcheck")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2005013002/260001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_android_rel_ng on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by enne@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2005013002/260001
Message was sent while issue was closed.
Committed patchset #14 (id:260001)
Message was sent while issue was closed.
Description was changed from ========== mac: ensure ui::Compositor exists for visible RWHVs RenderWidgetHostViewMac is careful about creating ui::Compositor and so currently only does this in Show and SwapCompositorFrame. However, if it the RenderWidgetHostView is created while the RenderWidgetHost is visible, then Show will early out and never create a ui::Compositor. This causes problems with begin frame scheduling, which wants to have the Display (which is created as part of creating the ui::Compositor's output surface currently) own the real begin frame source that drives everything. In that world, if no ui::Compositor exists, no frames ever are sent or swapped. So, the ui::Compositor needs to be created first. Also, if the ui::Compositor is correctly created during Show (or the constructor), then there's no need to ensure that it exists during swap compositor frame, and so that can be safely removed. R=asvitkine@chromium.org,ccameron@chromium.org ========== to ========== mac: ensure ui::Compositor exists for visible RWHVs RenderWidgetHostViewMac is careful about creating ui::Compositor and so currently only does this in Show and SwapCompositorFrame. However, if it the RenderWidgetHostView is created while the RenderWidgetHost is visible, then Show will early out and never create a ui::Compositor. This causes problems with begin frame scheduling, which wants to have the Display (which is created as part of creating the ui::Compositor's output surface currently) own the real begin frame source that drives everything. In that world, if no ui::Compositor exists, no frames ever are sent or swapped. So, the ui::Compositor needs to be created first. Also, if the ui::Compositor is correctly created during Show (or the constructor), then there's no need to ensure that it exists during swap compositor frame, and so that can be safely removed. R=asvitkine@chromium.org,ccameron@chromium.org Committed: https://crrev.com/b08fb143ac17b3cc42def859e3b0a9c93bda06f3 Cr-Commit-Position: refs/heads/master@{#400324} ==========
Message was sent while issue was closed.
Patchset 14 (id:??) landed as https://crrev.com/b08fb143ac17b3cc42def859e3b0a9c93bda06f3 Cr-Commit-Position: refs/heads/master@{#400324}
Message was sent while issue was closed.
A revert of this CL (patchset #14 id:260001) has been created in https://codereview.chromium.org/2079023002/ by enne@chromium.org. The reason for reverting is: Causes windows to stay white if minimized for >5 min.
Description was changed from ========== mac: ensure ui::Compositor exists for visible RWHVs RenderWidgetHostViewMac is careful about creating ui::Compositor and so currently only does this in Show and SwapCompositorFrame. However, if it the RenderWidgetHostView is created while the RenderWidgetHost is visible, then Show will early out and never create a ui::Compositor. This causes problems with begin frame scheduling, which wants to have the Display (which is created as part of creating the ui::Compositor's output surface currently) own the real begin frame source that drives everything. In that world, if no ui::Compositor exists, no frames ever are sent or swapped. So, the ui::Compositor needs to be created first. Also, if the ui::Compositor is correctly created during Show (or the constructor), then there's no need to ensure that it exists during swap compositor frame, and so that can be safely removed. R=asvitkine@chromium.org,ccameron@chromium.org Committed: https://crrev.com/b08fb143ac17b3cc42def859e3b0a9c93bda06f3 Cr-Commit-Position: refs/heads/master@{#400324} ========== to ========== mac: ensure ui::Compositor exists for visible RWHVs RenderWidgetHostViewMac is careful about creating ui::Compositor and so currently only does this in Show and SwapCompositorFrame. However, if it the RenderWidgetHostView is created while the RenderWidgetHost is visible, then Show will early out and never create a ui::Compositor. This causes problems with begin frame scheduling, which wants to have the Display (which is created as part of creating the ui::Compositor's output surface currently) own the real begin frame source that drives everything. In that world, if no ui::Compositor exists, no frames ever are sent or swapped. So, the ui::Compositor needs to be created first. Also, if the ui::Compositor is correctly created during Show (or the constructor), then there's no need to ensure that it exists during swap compositor frame, and so that can be safely removed. R=asvitkine@chromium.org,ccameron@chromium.org ==========
Description was changed from ========== mac: ensure ui::Compositor exists for visible RWHVs RenderWidgetHostViewMac is careful about creating ui::Compositor and so currently only does this in Show and SwapCompositorFrame. However, if it the RenderWidgetHostView is created while the RenderWidgetHost is visible, then Show will early out and never create a ui::Compositor. This causes problems with begin frame scheduling, which wants to have the Display (which is created as part of creating the ui::Compositor's output surface currently) own the real begin frame source that drives everything. In that world, if no ui::Compositor exists, no frames ever are sent or swapped. So, the ui::Compositor needs to be created first. Also, if the ui::Compositor is correctly created during Show (or the constructor), then there's no need to ensure that it exists during swap compositor frame, and so that can be safely removed. R=asvitkine@chromium.org,ccameron@chromium.org ========== to ========== mac: ensure ui::Compositor exists for visible RWHVs RenderWidgetHostViewMac is careful about creating ui::Compositor and so currently only does this in Show and SwapCompositorFrame. However, if it the RenderWidgetHostView is created while the RenderWidgetHost is visible, then Show will early out and never create a ui::Compositor. This causes problems with begin frame scheduling, which wants to have the Display (which is created as part of creating the ui::Compositor's output surface currently) own the real begin frame source that drives everything. In that world, if no ui::Compositor exists, no frames ever are sent or swapped. So, the ui::Compositor needs to be created first. To avoid issues where there's inconsistencies between host impl hidden state and browser compositor existence, always ensure that the browser compositor exists in ::Show. R=asvitkine@chromium.org,ccameron@chromium.org ==========
lgtm
I wasn't able to repro the bug that caused the last revert, but this patch has much bigger hammers to ensure state is correct than previous patches which tried to do things carefully. ccameron says he wants to rewrite this, so am going to land this as-is to unblock unified begin frame.
The CQ bit was checked by enne@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from asvitkine@chromium.org, phajdan.jr@chromium.org Link to the patchset: https://codereview.chromium.org/2005013002/#ps320001 (title: "Ensure in ctor and always in show")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2005013002/320001
Message was sent while issue was closed.
Committed patchset #17 (id:320001)
Message was sent while issue was closed.
Description was changed from ========== mac: ensure ui::Compositor exists for visible RWHVs RenderWidgetHostViewMac is careful about creating ui::Compositor and so currently only does this in Show and SwapCompositorFrame. However, if it the RenderWidgetHostView is created while the RenderWidgetHost is visible, then Show will early out and never create a ui::Compositor. This causes problems with begin frame scheduling, which wants to have the Display (which is created as part of creating the ui::Compositor's output surface currently) own the real begin frame source that drives everything. In that world, if no ui::Compositor exists, no frames ever are sent or swapped. So, the ui::Compositor needs to be created first. To avoid issues where there's inconsistencies between host impl hidden state and browser compositor existence, always ensure that the browser compositor exists in ::Show. R=asvitkine@chromium.org,ccameron@chromium.org ========== to ========== mac: ensure ui::Compositor exists for visible RWHVs RenderWidgetHostViewMac is careful about creating ui::Compositor and so currently only does this in Show and SwapCompositorFrame. However, if it the RenderWidgetHostView is created while the RenderWidgetHost is visible, then Show will early out and never create a ui::Compositor. This causes problems with begin frame scheduling, which wants to have the Display (which is created as part of creating the ui::Compositor's output surface currently) own the real begin frame source that drives everything. In that world, if no ui::Compositor exists, no frames ever are sent or swapped. So, the ui::Compositor needs to be created first. To avoid issues where there's inconsistencies between host impl hidden state and browser compositor existence, always ensure that the browser compositor exists in ::Show. R=asvitkine@chromium.org,ccameron@chromium.org Committed: https://crrev.com/a46e810069effce565efe42ccddfd13f74c5bc93 Cr-Commit-Position: refs/heads/master@{#401133} ==========
Message was sent while issue was closed.
Patchset 17 (id:??) landed as https://crrev.com/a46e810069effce565efe42ccddfd13f74c5bc93 Cr-Commit-Position: refs/heads/master@{#401133} |
