Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(1)

Issue 2750223005: Preserve FrameSinkSourceMapping nodes that have path to root (Closed)

Created:
1 year, 1 month ago by Fady Samuel
Modified:
1 year, 1 month ago
Reviewers:
enne (OOO)
CC:
cc-bugs_chromium.org, chromium-reviews
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

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/+/8f0964617b1d3e6abdc4f61e8f1d4b8bf0f6c672

Patch Set 1 #

Patch Set 2 : UnregisterBeginFrameSource, fix debug build of unit test #

Total comments: 3

Patch Set 3 : Added additional unit test #

Unified diffs Side-by-side diffs Delta from patch set Stats (+79 lines, -1 line) Patch
M cc/surfaces/surface_manager.cc View 1 chunk +2 lines, -1 line 0 comments Download
M cc/surfaces/surface_manager_unittest.cc View 1 2 1 chunk +77 lines, -0 lines 0 comments Download

Messages

Total messages: 25 (16 generated)
Fady Samuel
PTAL Enne! Minor change + unit test. Fixes a Mus+Ash bug! Thanks!
1 year, 1 month ago (2017-03-16 22:59:26 UTC) #4
Fady Samuel
PTAL Enne. I fixed a DCHECK by unregistering the BeginFrameSource in the unit test.
1 year, 1 month ago (2017-03-16 23:50:16 UTC) #9
enne (OOO)
https://codereview.chromium.org/2750223005/diff/20001/cc/surfaces/surface_manager.cc File cc/surfaces/surface_manager.cc (right): https://codereview.chromium.org/2750223005/diff/20001/cc/surfaces/surface_manager.cc#newcode585 cc/surfaces/surface_manager.cc:585: if (!iter->second.has_children() && !clients_.count(parent_frame_sink_id) && I'm not sure that ...
1 year, 1 month ago (2017-03-17 17:00:38 UTC) #13
enne (OOO)
Ok, nevermind, convinced myself that this should be ok. The node will get removed, but ...
1 year, 1 month ago (2017-03-17 18:39:41 UTC) #14
Fady Samuel
Thanks Enne. CQ'ing. https://codereview.chromium.org/2750223005/diff/20001/cc/surfaces/surface_manager.cc File cc/surfaces/surface_manager.cc (right): https://codereview.chromium.org/2750223005/diff/20001/cc/surfaces/surface_manager.cc#newcode585 cc/surfaces/surface_manager.cc:585: if (!iter->second.has_children() && !clients_.count(parent_frame_sink_id) && On ...
1 year, 1 month ago (2017-03-17 18:42:35 UTC) #15
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2750223005/40001
1 year, 1 month ago (2017-03-17 21:16:53 UTC) #18
commit-bot: I haz the power
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_rel_ng/builds/386682)
1 year, 1 month ago (2017-03-17 22:56:04 UTC) #20
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2750223005/40001
1 year, 1 month ago (2017-03-18 02:41:42 UTC) #22
commit-bot: I haz the power
1 year, 1 month ago (2017-03-18 03:26:37 UTC) #25
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/8f0964617b1d3e6abdc4f61e8f1d...

Powered by Google App Engine
This is Rietveld 408576698