Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(92)

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

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.

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!
3 years, 9 months 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.
3 years, 9 months 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 ...
3 years, 9 months 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 ...
3 years, 9 months 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 ...
3 years, 9 months 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
3 years, 9 months 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)
3 years, 9 months 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
3 years, 9 months ago (2017-03-18 02:41:42 UTC) #22
commit-bot: I haz the power
3 years, 9 months 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