MacViews: Have only one ui::Compositor per window |
|||||||||
Issue descriptionAt present we have at least 2 ui::Compositors per window - one for the views::BridgedNativeWidget - one for the content::RenderWidgetHostViewMac - sometimes more for, e.g, DevTools These are embedded in each other via a views::NativeViewHostMac, which does the embedding via -[NSView addSubview:]. An alternative scheme is to - continue to embed for input routing via -[NSView addSubView:] - query the NSView for its root ui::Layer - add this layer as a sub-layer of the NativeViewHostMac::layer - make the embedded NSView be transparent (so it only does event routing) The benefits of this scheme is that + we only have to worry about resizing one ui::Compositor at a time + we never stall in RenderWidgetHostImpl::PauseForPendingResizeOrRepaints, which causes jank at tab-switch + we don't have to worry about fixing the two-NSView situation for OOP-D A (super-crashy) prototype of this is at: https://chromium-review.googlesource.com/c/chromium/src/+/1046065
,
May 16 2018
,
May 22 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/200d56fbfb923ba7fb14d7374e198ebea272fbf6 commit 200d56fbfb923ba7fb14d7374e198ebea272fbf6 Author: Christopher Cameron <ccameron@chromium.org> Date: Tue May 22 17:34:00 2018 MacViews: Add support for embedding RWHVMac in browser UI's compositor Add a private NSView method, setParentUILayer. Any NSView that can be drawn using a ui::Compositor (e.g, the WebContentsView) may implement this method to be informed of the parent ui::Layer that it may draw to (instead of drawing into its NSView's CALayers). Prior to this change, BrowserCompositorMac will always allocate its own ui::Compositor (via RecyclableCompositorMac) for drawing. With this change, the -[NSView setParentUILayer] method propagates to the BrowserCompositorMac to allow the BrowserCompositorMac to use the parent ui::Layer's ui::Compositor. Add this as a new entry in BrowserCompositorMac::State, and add the ability to transition between this state and all other states. Of note is that the previous states are still necessary for two use cases. * When a tab is background but is doing tab capture, the current implementation requires that a ui::Compositor be available to draw the content for capture. Allow allocating for this use case. * Popup windows (e.g, date and time picker) are not embedded in a distinct ui::Compositor (they don't even have a parent WebContentsViewCocoa), so they must display through their own allocated ui::Compositor. Use the existing mechanism in this case. Bug: 840173 Change-Id: I68bbf5da8419c7b1b3ad0edd99faec05bbdc5473 Reviewed-on: https://chromium-review.googlesource.com/1063119 Commit-Queue: ccameron <ccameron@chromium.org> Reviewed-by: Sidney San Martín <sdy@chromium.org> Reviewed-by: Trent Apted <tapted@chromium.org> Cr-Commit-Position: refs/heads/master@{#560680} [modify] https://crrev.com/200d56fbfb923ba7fb14d7374e198ebea272fbf6/content/browser/renderer_host/browser_compositor_view_mac.h [modify] https://crrev.com/200d56fbfb923ba7fb14d7374e198ebea272fbf6/content/browser/renderer_host/browser_compositor_view_mac.mm [modify] https://crrev.com/200d56fbfb923ba7fb14d7374e198ebea272fbf6/content/browser/renderer_host/render_widget_host_view_mac.h [modify] https://crrev.com/200d56fbfb923ba7fb14d7374e198ebea272fbf6/content/browser/renderer_host/render_widget_host_view_mac.mm [modify] https://crrev.com/200d56fbfb923ba7fb14d7374e198ebea272fbf6/content/browser/renderer_host/render_widget_host_view_mac_unittest.mm [modify] https://crrev.com/200d56fbfb923ba7fb14d7374e198ebea272fbf6/content/browser/web_contents/web_contents_view_mac.h [modify] https://crrev.com/200d56fbfb923ba7fb14d7374e198ebea272fbf6/content/browser/web_contents/web_contents_view_mac.mm [modify] https://crrev.com/200d56fbfb923ba7fb14d7374e198ebea272fbf6/ui/views/controls/native/native_view_host_mac.mm
,
May 23 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/b0c7ed414b3184e22efa85938ebf15c7271d9ee5 commit b0c7ed414b3184e22efa85938ebf15c7271d9ee5 Author: Christopher Cameron <ccameron@chromium.org> Date: Wed May 23 19:41:05 2018 MacViews: Disable embedding RWHVMac in browser UI's compositor Bug: 845807 , 840173 , 846018 Change-Id: I97768c5c2085f8e1f29371bfedff5aaeebc956fa Reviewed-on: https://chromium-review.googlesource.com/1070464 Commit-Queue: ccameron <ccameron@chromium.org> Reviewed-by: Elly Fong-Jones <ellyjones@chromium.org> Reviewed-by: ccameron <ccameron@chromium.org> Cr-Commit-Position: refs/heads/master@{#561210} [modify] https://crrev.com/b0c7ed414b3184e22efa85938ebf15c7271d9ee5/content/browser/renderer_host/render_widget_host_view_mac.mm
,
May 24 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/4df0ff8ab3aa1317593369a237a5e0fdff817803 commit 4df0ff8ab3aa1317593369a237a5e0fdff817803 Author: Christopher Cameron <ccameron@chromium.org> Date: Thu May 24 05:19:09 2018 MacViews: Fix embedded RWHVMac input routing The method RenderWidgetHostViewMac::GetOffsetFromRootSurface is to be used to indicate where the view is in its root ui::Compositor. It used to be that the two were always aligned, but now they can be offset. Bug: 846018 , 840173 Change-Id: Id3f761d56c75ebd33b75c40c05d46a84b485d162 Reviewed-on: https://chromium-review.googlesource.com/1070883 Reviewed-by: Sidney San Martín <sdy@chromium.org> Commit-Queue: ccameron <ccameron@chromium.org> Cr-Commit-Position: refs/heads/master@{#561389} [modify] https://crrev.com/4df0ff8ab3aa1317593369a237a5e0fdff817803/content/browser/renderer_host/render_widget_host_view_mac.mm
,
May 24 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/1cdbef88a7ade4a852488ea26ee3af26efdedda6 commit 1cdbef88a7ade4a852488ea26ee3af26efdedda6 Author: Christopher Cameron <ccameron@chromium.org> Date: Thu May 24 05:19:23 2018 MacViews: Do not set delegating layer background color Setting the background color on delegated layers can cause artifacts (only non-tiled areas get the background color). This also appears not to have been necessary. Bug: 845968 , 840173 Change-Id: If8eca7d3eb793952a9a606a2d27683b02d2c4b3f Reviewed-on: https://chromium-review.googlesource.com/1071117 Reviewed-by: Sidney San Martín <sdy@chromium.org> Commit-Queue: ccameron <ccameron@chromium.org> Cr-Commit-Position: refs/heads/master@{#561390} [modify] https://crrev.com/1cdbef88a7ade4a852488ea26ee3af26efdedda6/content/browser/renderer_host/browser_compositor_view_mac.mm [modify] https://crrev.com/1cdbef88a7ade4a852488ea26ee3af26efdedda6/content/browser/renderer_host/render_widget_host_view_mac.mm
,
May 24 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/ed350c5af7814a159f5554cd7e3a12e2377037d6 commit ed350c5af7814a159f5554cd7e3a12e2377037d6 Author: Christopher Cameron <ccameron@chromium.org> Date: Thu May 24 05:57:40 2018 MacViews: Use surface sync for resize BrowserCompositorMac has historically rolled its own surface sync via the Suspend/Unsuspend functions, and compositor_suspended_lock_ in RecyclableCompositorMac. This no longer works in MacViews browser, because we no longer use RecyclableCompositorMac. Leave the Suspend/Unsuspend code in place for the non-MacViews case. Split up UpdateSurfaceAndUnsuspend - Make it just update the surface size - Call it whenever the compositor is recycled or the surface resizes - Move the AddRootLayer to TransitionToState Add a GetDeadlinePolicy method - When using surface sync, return a deadline of 8 frames (empirically works well for retina displays) - Otherwise return an immediate deadline. Bug: 840173 Change-Id: I6a1de5698e73d754ee03d9db8f4a97480c71dfb3 Reviewed-on: https://chromium-review.googlesource.com/1069834 Reviewed-by: Fady Samuel <fsamuel@chromium.org> Commit-Queue: ccameron <ccameron@chromium.org> Cr-Commit-Position: refs/heads/master@{#561393} [modify] https://crrev.com/ed350c5af7814a159f5554cd7e3a12e2377037d6/content/browser/renderer_host/browser_compositor_view_mac.h [modify] https://crrev.com/ed350c5af7814a159f5554cd7e3a12e2377037d6/content/browser/renderer_host/browser_compositor_view_mac.mm
,
May 25 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/6f4d713f35161bc425b1032956e2b12fdd5f639f commit 6f4d713f35161bc425b1032956e2b12fdd5f639f Author: Christopher Cameron <ccameron@chromium.org> Date: Fri May 25 01:05:23 2018 MacViews: Re-enable single ui::Compositor view Decide at initialization whether or not a RenderWidgetHostViewMac will be displayed in its NSView or through a ui::Compositor. This fixes a bug whereby some windows (DevTools is a good example) would, through the vagaries of initialization order, draw a single frame to the NSView, which would then never go away, and cover up the ui::Compositor's version of the content (which was then updating correctly, though not visible). Bug: 840173 , 845807 Change-Id: If051d249956b10c9e56450ed1f58b9048d331562 Reviewed-on: https://chromium-review.googlesource.com/1070764 Commit-Queue: ccameron <ccameron@chromium.org> Reviewed-by: Sidney San Martín <sdy@chromium.org> Cr-Commit-Position: refs/heads/master@{#561717} [modify] https://crrev.com/6f4d713f35161bc425b1032956e2b12fdd5f639f/content/browser/renderer_host/render_widget_host_view_mac.h [modify] https://crrev.com/6f4d713f35161bc425b1032956e2b12fdd5f639f/content/browser/renderer_host/render_widget_host_view_mac.mm
,
May 25 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/ad0e8656343a663932a696f128b5bc85df530cac commit ad0e8656343a663932a696f128b5bc85df530cac Author: Fady Samuel <fsamuel@chromium.org> Date: Fri May 25 13:49:14 2018 Revert "MacViews: Re-enable single ui::Compositor view" This reverts commit 6f4d713f35161bc425b1032956e2b12fdd5f639f. Reason for revert: This seems to have caused https://crbug.com/846691 Original change's description: > MacViews: Re-enable single ui::Compositor view > > Decide at initialization whether or not a RenderWidgetHostViewMac will > be displayed in its NSView or through a ui::Compositor. > > This fixes a bug whereby some windows (DevTools is a good example) > would, through the vagaries of initialization order, draw a single > frame to the NSView, which would then never go away, and cover up the > ui::Compositor's version of the content (which was then updating > correctly, though not visible). > > Bug: 840173 , 845807 > Change-Id: If051d249956b10c9e56450ed1f58b9048d331562 > Reviewed-on: https://chromium-review.googlesource.com/1070764 > Commit-Queue: ccameron <ccameron@chromium.org> > Reviewed-by: Sidney San Martín <sdy@chromium.org> > Cr-Commit-Position: refs/heads/master@{#561717} TBR=ccameron@chromium.org,sdy@chromium.org Change-Id: Ic25ee96c567c3531d91ec5431f10d15d34bfee82 No-Presubmit: true No-Tree-Checks: true No-Try: true Bug: 840173 , 845807 Reviewed-on: https://chromium-review.googlesource.com/1073089 Reviewed-by: Fady Samuel <fsamuel@chromium.org> Commit-Queue: Fady Samuel <fsamuel@chromium.org> Cr-Commit-Position: refs/heads/master@{#561850} [modify] https://crrev.com/ad0e8656343a663932a696f128b5bc85df530cac/content/browser/renderer_host/render_widget_host_view_mac.h [modify] https://crrev.com/ad0e8656343a663932a696f128b5bc85df530cac/content/browser/renderer_host/render_widget_host_view_mac.mm
,
May 25 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/e28df26f9442362f169e55d9a82fc7cb59208bd1 commit e28df26f9442362f169e55d9a82fc7cb59208bd1 Author: Christopher Cameron <ccameron@chromium.org> Date: Fri May 25 22:57:51 2018 Fix Cast dialog not appearing on non-MacViews Resize the ui::Compositor's size in BrowserCompositorMac's SynchronizeVisualProperties method. Re-disable using a unified ui::Compositor (it was accidentally enabled by a botched merge). Bug: 840173 Change-Id: I701574e385ae288b991adce81e1ab5efde23e0c2 Reviewed-on: https://chromium-review.googlesource.com/1073682 Reviewed-by: Fady Samuel <fsamuel@chromium.org> Commit-Queue: ccameron <ccameron@chromium.org> Cr-Commit-Position: refs/heads/master@{#562047} [modify] https://crrev.com/e28df26f9442362f169e55d9a82fc7cb59208bd1/content/browser/renderer_host/browser_compositor_view_mac.mm [modify] https://crrev.com/e28df26f9442362f169e55d9a82fc7cb59208bd1/content/browser/renderer_host/render_widget_host_view_mac.mm
,
May 30 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/6be8acd7b09a2378d8c47ca9fa91b9fd9b4839ba commit 6be8acd7b09a2378d8c47ca9fa91b9fd9b4839ba Author: Christopher Cameron <ccameron@chromium.org> Date: Wed May 30 21:40:33 2018 MacViews: Lazily enable using a single ui::Compositor It is not possible to know ahead of time whether or not a WebContentsView will be embedded in an NSView or a views::View, even when MacViewsBrowser is enabled (content_shell, for instance, uses the NSView display path). Lazily determine if a WebContentsView is to display via its parent views::View's compositor. When attaching to a views::View for the first time, clear all content that is being displayed in the NSView (because it will occlude the views::View) and disable displaying through Cocoa for the lifetime of the WebContentsView. When all code that expects to display through Cocoa is updated or deleted, this lazy determination may be removed. Bug: 840173 Change-Id: I36c7054ba7c71e6fe94e496e677a9a9be4fb790a Reviewed-on: https://chromium-review.googlesource.com/1075563 Reviewed-by: Sidney San Martín <sdy@chromium.org> Commit-Queue: ccameron <ccameron@chromium.org> Cr-Commit-Position: refs/heads/master@{#563008} [modify] https://crrev.com/6be8acd7b09a2378d8c47ca9fa91b9fd9b4839ba/content/browser/renderer_host/render_widget_host_ns_view_bridge.h [modify] https://crrev.com/6be8acd7b09a2378d8c47ca9fa91b9fd9b4839ba/content/browser/renderer_host/render_widget_host_ns_view_bridge.mm [modify] https://crrev.com/6be8acd7b09a2378d8c47ca9fa91b9fd9b4839ba/content/browser/renderer_host/render_widget_host_view_mac.mm
,
Jun 2 2018
,
Jun 4 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/e9942645e816b8f50f7f5e11b4194f850a522bd7 commit e9942645e816b8f50f7f5e11b4194f850a522bd7 Author: Christopher Cameron <ccameron@chromium.org> Date: Mon Jun 04 21:10:37 2018 Revert "MacViews: Re-enable single ui::Compositor view" This reverts commit 6f4d713f35161bc425b1032956e2b12fdd5f639f. Reason for revert: This seems to have caused https://crbug.com/846691 Original change's description: > MacViews: Re-enable single ui::Compositor view > > Decide at initialization whether or not a RenderWidgetHostViewMac will > be displayed in its NSView or through a ui::Compositor. > > This fixes a bug whereby some windows (DevTools is a good example) > would, through the vagaries of initialization order, draw a single > frame to the NSView, which would then never go away, and cover up the > ui::Compositor's version of the content (which was then updating > correctly, though not visible). > > Bug: 840173 , 845807 > Change-Id: If051d249956b10c9e56450ed1f58b9048d331562 > Reviewed-on: https://chromium-review.googlesource.com/1070764 > Commit-Queue: ccameron <ccameron@chromium.org> > Reviewed-by: Sidney San Martín <sdy@chromium.org> > Cr-Commit-Position: refs/heads/master@{#561717} TBR=ccameron@chromium.org, fsamuel@chromium.org, sdy@chromium.org (cherry picked from commit ad0e8656343a663932a696f128b5bc85df530cac) Change-Id: Ic25ee96c567c3531d91ec5431f10d15d34bfee82 No-Presubmit: true No-Tree-Checks: true No-Try: true Bug: 840173 , 845807 Reviewed-on: https://chromium-review.googlesource.com/1073089 Reviewed-by: Fady Samuel <fsamuel@chromium.org> Commit-Queue: Fady Samuel <fsamuel@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#561850} Reviewed-on: https://chromium-review.googlesource.com/1085866 Reviewed-by: ccameron <ccameron@chromium.org> Cr-Commit-Position: refs/branch-heads/3440@{#169} Cr-Branched-From: 010ddcfda246975d194964ccf20038ebbdec6084-refs/heads/master@{#561733} [modify] https://crrev.com/e9942645e816b8f50f7f5e11b4194f850a522bd7/content/browser/renderer_host/render_widget_host_view_mac.h [modify] https://crrev.com/e9942645e816b8f50f7f5e11b4194f850a522bd7/content/browser/renderer_host/render_widget_host_view_mac.mm
,
Jun 4 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/4e92c669455bee60be035a0b9b849644cc88fa0a commit 4e92c669455bee60be035a0b9b849644cc88fa0a Author: Christopher Cameron <ccameron@chromium.org> Date: Mon Jun 04 21:16:33 2018 Fix Cast dialog not appearing on non-MacViews Resize the ui::Compositor's size in BrowserCompositorMac's SynchronizeVisualProperties method. Re-disable using a unified ui::Compositor (it was accidentally enabled by a botched merge). Bug: 840173 Change-Id: I701574e385ae288b991adce81e1ab5efde23e0c2 Reviewed-on: https://chromium-review.googlesource.com/1073682 Reviewed-by: Fady Samuel <fsamuel@chromium.org> Commit-Queue: ccameron <ccameron@chromium.org> Cr-Commit-Position: refs/heads/master@{#562047} (cherry picked from commit e28df26f9442362f169e55d9a82fc7cb59208bd1) Merged for TBR=ccameron@chromium.org Bug: 845807 Change-Id: Ic2515fcf91ecdc25de05ab36f7d89bd17fa37bbf Reviewed-on: https://chromium-review.googlesource.com/1086119 Reviewed-by: ccameron <ccameron@chromium.org> Cr-Commit-Position: refs/branch-heads/3440@{#170} Cr-Branched-From: 010ddcfda246975d194964ccf20038ebbdec6084-refs/heads/master@{#561733} [modify] https://crrev.com/4e92c669455bee60be035a0b9b849644cc88fa0a/content/browser/renderer_host/browser_compositor_view_mac.mm [modify] https://crrev.com/4e92c669455bee60be035a0b9b849644cc88fa0a/content/browser/renderer_host/render_widget_host_view_mac.mm
,
Jun 5 2018
Requesting merge for 68 to get this in the first Beta.
,
Jun 5 2018
,
Jun 5 2018
Is the change listed at #14 looking good in canary and safe to merge to M68?
,
Jun 5 2018
The patch I'm planning to merge is #11, which has been in Canary for 4 days and hasn't had any issues that I'm aware of.
,
Jun 5 2018
,
Jun 6 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/83f02728a728a8925d320be23fedadfec719a86e commit 83f02728a728a8925d320be23fedadfec719a86e Author: Christopher Cameron <ccameron@chromium.org> Date: Wed Jun 06 04:35:33 2018 MacViews: Lazily enable using a single ui::Compositor It is not possible to know ahead of time whether or not a WebContentsView will be embedded in an NSView or a views::View, even when MacViewsBrowser is enabled (content_shell, for instance, uses the NSView display path). Lazily determine if a WebContentsView is to display via its parent views::View's compositor. When attaching to a views::View for the first time, clear all content that is being displayed in the NSView (because it will occlude the views::View) and disable displaying through Cocoa for the lifetime of the WebContentsView. When all code that expects to display through Cocoa is updated or deleted, this lazy determination may be removed. TBR=ccameron@chromium.org (cherry picked from commit 6be8acd7b09a2378d8c47ca9fa91b9fd9b4839ba) Bug: 840173 Change-Id: I36c7054ba7c71e6fe94e496e677a9a9be4fb790a Reviewed-on: https://chromium-review.googlesource.com/1075563 Reviewed-by: Sidney San Martín <sdy@chromium.org> Commit-Queue: ccameron <ccameron@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#563008} Reviewed-on: https://chromium-review.googlesource.com/1088314 Reviewed-by: ccameron <ccameron@chromium.org> Cr-Commit-Position: refs/branch-heads/3440@{#204} Cr-Branched-From: 010ddcfda246975d194964ccf20038ebbdec6084-refs/heads/master@{#561733} [modify] https://crrev.com/83f02728a728a8925d320be23fedadfec719a86e/content/browser/renderer_host/render_widget_host_ns_view_bridge.h [modify] https://crrev.com/83f02728a728a8925d320be23fedadfec719a86e/content/browser/renderer_host/render_widget_host_ns_view_bridge.mm [modify] https://crrev.com/83f02728a728a8925d320be23fedadfec719a86e/content/browser/renderer_host/render_widget_host_view_mac.mm
,
Jun 6 2018
|
|||||||||
►
Sign in to add a comment |
|||||||||
Comment 1 by lgrey@chromium.org
, May 10 2018