|
|
Created:
3 years, 9 months ago by Fady Samuel Modified:
3 years, 9 months ago Reviewers:
enne (OOO) CC:
cc-bugs_chromium.org, chromium-reviews Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionPreserve FrameSinkSourceMapping nodes that have path to root
In the Mus window server, every ServerWindow has a corresponding unique
FrameSinkId whether or not it has a CompositorFrameSink. Some (most)
ServerWindows do not submit CompositorFrames because they are local windows
within some ancestor that does submit a CompositorFrame.
We keep the FrameSinkSourceMapping in sync with the ServerWindow hierarchy
to simplify reparenting of windows across displays.
However, prior to this patch, if a FrameSinkSourceMapping loses all its
children and does not have a CompositorFrameSink (a SurfaceFactoryClient)
registered, then the node will be removed. When the window server adds
a child to the ServerWindow corresponding to the deleted FrameSinkSourceMapping
that child becomes orphaned and an appropriate BeginFrameSource does
not propagate to it. This manifests when all windows are closed in Mus+Ash
and a new window is opened. That window will not show any content because
it will not receive BeginFrames.
This CL fixes this issue by preserving the FrameSinkSourceMapping as long
as it has a BeginFrameSource.
BUG=701626
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
Review-Url: https://codereview.chromium.org/2750223005
Cr-Commit-Position: refs/heads/master@{#457948}
Committed: https://chromium.googlesource.com/chromium/src/+/8f0964617b1d3e6abdc4f61e8f1d4b8bf0f6c672
Patch Set 1 #Patch Set 2 : UnregisterBeginFrameSource, fix debug build of unit test #
Total comments: 3
Patch Set 3 : Added additional unit test #
Messages
Total messages: 25 (16 generated)
Description was changed from ========== Preserve FrameSinkSourceMapping nodes that have path to root In the Mus window server, every ServerWindow has a corresponding unique FrameSinkId whether or not it has a CompositorFrameSink. Some (most) ServerWindows do not submit CompositorFrames because they are local windows within some ancestor that does submit a CompositorFrame. We keep the FrameSinkSourceMapping in sync with the ServerWindow hierarchy to simplify reparenting of windows across displays. However, prior to this patch, if a FrameSinkSourceMapping loses all its children and does not have a CompositorFrameSink (a SurfaceFactoryClient) registered, then the node will be removed. When the window server adds a child to the ServerWindow corresponding to the deleted FrameSinkSourceMapping that child becomes orphaned and an appropriate BeginFrameSource does not propagate to it. This manifests when all windows are closed in Mus+Ash and a new window is opened. That window will not show any content because it will not receive BeginFrames. This CL fixes this issue by preserving the FrameSinkSourceMapping as long as it has a BeginFrameSource. BUG=701626 ========== to ========== Preserve FrameSinkSourceMapping nodes that have path to root In the Mus window server, every ServerWindow has a corresponding unique FrameSinkId whether or not it has a CompositorFrameSink. Some (most) ServerWindows do not submit CompositorFrames because they are local windows within some ancestor that does submit a CompositorFrame. We keep the FrameSinkSourceMapping in sync with the ServerWindow hierarchy to simplify reparenting of windows across displays. However, prior to this patch, if a FrameSinkSourceMapping loses all its children and does not have a CompositorFrameSink (a SurfaceFactoryClient) registered, then the node will be removed. When the window server adds a child to the ServerWindow corresponding to the deleted FrameSinkSourceMapping that child becomes orphaned and an appropriate BeginFrameSource does not propagate to it. This manifests when all windows are closed in Mus+Ash and a new window is opened. That window will not show any content because it will not receive BeginFrames. This CL fixes this issue by preserving the FrameSinkSourceMapping as long as it has a BeginFrameSource. BUG=701626 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
The CQ bit was checked by fsamuel@chromium.org to run a CQ dry run
fsamuel@chromium.org changed reviewers: + enne@chromium.org
PTAL Enne! Minor change + unit test. Fixes a Mus+Ash bug! Thanks!
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
The CQ bit was checked by fsamuel@chromium.org to run a CQ dry run
PTAL Enne. I fixed a DCHECK by unregistering the BeginFrameSource in the unit test.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2750223005/diff/20001/cc/surfaces/surface_man... File cc/surfaces/surface_manager.cc (right): https://codereview.chromium.org/2750223005/diff/20001/cc/surfaces/surface_man... cc/surfaces/surface_manager.cc:585: if (!iter->second.has_children() && !clients_.count(parent_frame_sink_id) && I'm not sure that this is enough. The additional check is that it has a source here, but what if you didn't register the begin frame source in your unit test until after you attached C? It seems like C would not get a source at that point. If so would it make more sense for UnregisterFrameSinkHierarchy to remove the *child* if it had no children or clients instead of the parent? Or, maybe there's some other solution here. https://codereview.chromium.org/2750223005/diff/20001/cc/surfaces/surface_man... cc/surfaces/surface_manager.cc:585: if (!iter->second.has_children() && !clients_.count(parent_frame_sink_id) && I'm not sure that this is enough. The additional check is that it has a source here, but what if you didn't register the begin frame source in your unit test until after you attached C? It seems like C would not get a source at that point. If so would it make more sense for UnregisterFrameSinkHierarchy to remove the *child* if it had no children or clients instead of the parent? Or, maybe there's some other solution here.
Ok, nevermind, convinced myself that this should be ok. The node will get removed, but it'll get added back. lgtm
Thanks Enne. CQ'ing. https://codereview.chromium.org/2750223005/diff/20001/cc/surfaces/surface_man... File cc/surfaces/surface_manager.cc (right): https://codereview.chromium.org/2750223005/diff/20001/cc/surfaces/surface_man... cc/surfaces/surface_manager.cc:585: if (!iter->second.has_children() && !clients_.count(parent_frame_sink_id) && On 2017/03/17 17:00:38, enne wrote: > I'm not sure that this is enough. The additional check is that it has a source > here, but what if you didn't register the begin frame source in your unit test > until after you attached C? It seems like C would not get a source at that > point. > > If so would it make more sense for UnregisterFrameSinkHierarchy to remove the > *child* if it had no children or clients instead of the parent? Or, maybe > there's some other solution here. As discussed offline, this isn't an issue. I've added an additional unit test to verify this.
The CQ bit was checked by fsamuel@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from enne@chromium.org Link to the patchset: https://codereview.chromium.org/2750223005/#ps40001 (title: "Added additional unit test")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by fsamuel@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch. Bot data: {"patchset_id": 40001, "attempt_start_ts": 1489804858600960, "parent_rev": "ee07b33227f7c3ff717f7845b281780ff1428264", "commit_rev": "8f0964617b1d3e6abdc4f61e8f1d4b8bf0f6c672"}
Message was sent while issue was closed.
Description was changed from ========== Preserve FrameSinkSourceMapping nodes that have path to root In the Mus window server, every ServerWindow has a corresponding unique FrameSinkId whether or not it has a CompositorFrameSink. Some (most) ServerWindows do not submit CompositorFrames because they are local windows within some ancestor that does submit a CompositorFrame. We keep the FrameSinkSourceMapping in sync with the ServerWindow hierarchy to simplify reparenting of windows across displays. However, prior to this patch, if a FrameSinkSourceMapping loses all its children and does not have a CompositorFrameSink (a SurfaceFactoryClient) registered, then the node will be removed. When the window server adds a child to the ServerWindow corresponding to the deleted FrameSinkSourceMapping that child becomes orphaned and an appropriate BeginFrameSource does not propagate to it. This manifests when all windows are closed in Mus+Ash and a new window is opened. That window will not show any content because it will not receive BeginFrames. This CL fixes this issue by preserving the FrameSinkSourceMapping as long as it has a BeginFrameSource. BUG=701626 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== Preserve FrameSinkSourceMapping nodes that have path to root In the Mus window server, every ServerWindow has a corresponding unique FrameSinkId whether or not it has a CompositorFrameSink. Some (most) ServerWindows do not submit CompositorFrames because they are local windows within some ancestor that does submit a CompositorFrame. We keep the FrameSinkSourceMapping in sync with the ServerWindow hierarchy to simplify reparenting of windows across displays. However, prior to this patch, if a FrameSinkSourceMapping loses all its children and does not have a CompositorFrameSink (a SurfaceFactoryClient) registered, then the node will be removed. When the window server adds a child to the ServerWindow corresponding to the deleted FrameSinkSourceMapping that child becomes orphaned and an appropriate BeginFrameSource does not propagate to it. This manifests when all windows are closed in Mus+Ash and a new window is opened. That window will not show any content because it will not receive BeginFrames. This CL fixes this issue by preserving the FrameSinkSourceMapping as long as it has a BeginFrameSource. BUG=701626 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel Review-Url: https://codereview.chromium.org/2750223005 Cr-Commit-Position: refs/heads/master@{#457948} Committed: https://chromium.googlesource.com/chromium/src/+/8f0964617b1d3e6abdc4f61e8f1d... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/8f0964617b1d3e6abdc4f61e8f1d... |