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

Issue 2760433002: SurfaceManager::CreateSurface must set the surface's factory (Closed)

Created:
1 year, 1 month ago by Saman Sami
Modified:
1 year ago
Reviewers:
Fady Samuel, jbauman
CC:
chromium-reviews, cc-bugs_chromium.org
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

SurfaceManager::CreateSurface must set the surface's factory This change is necessary because DelegatedFrameHost can be destroyed and recreated when RendererCompositorFrameSink is alive the whole time and can potentially send the same LocalSurfaceId to both instances of DelegatedFrameHost. BUG=697864 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel Review-Url: https://codereview.chromium.org/2760433002 Cr-Commit-Position: refs/heads/master@{#458138} Committed: https://chromium.googlesource.com/chromium/src/+/9e0fd9a87d69362103af512b8e7b8946604abe71

Patch Set 1 #

Patch Set 2 : Updated #

Total comments: 4

Patch Set 3 : nits #

Total comments: 2

Patch Set 4 : Updated #

Patch Set 5 : unit test nit #

Patch Set 6 : Reset #

Unified diffs Side-by-side diffs Delta from patch set Stats (+81 lines, -17 lines) Patch
M cc/surfaces/compositor_frame_sink_support_unittest.cc View 1 2 3 4 3 chunks +53 lines, -17 lines 0 comments Download
M cc/surfaces/surface.h View 1 2 3 4 5 2 chunks +9 lines, -0 lines 0 comments Download
M cc/surfaces/surface.cc View 1 2 3 4 5 1 chunk +7 lines, -0 lines 0 comments Download
M cc/surfaces/surface_manager.cc View 1 2 3 4 5 1 chunk +12 lines, -0 lines 0 comments Download

Messages

Total messages: 49 (34 generated)
Saman Sami
PTAL
1 year, 1 month ago (2017-03-16 18:29:49 UTC) #8
Fady Samuel
LGTM https://codereview.chromium.org/2760433002/diff/20001/cc/surfaces/compositor_frame_sink_support_unittest.cc File cc/surfaces/compositor_frame_sink_support_unittest.cc (right): https://codereview.chromium.org/2760433002/diff/20001/cc/surfaces/compositor_frame_sink_support_unittest.cc#newcode1067 cc/surfaces/compositor_frame_sink_support_unittest.cc:1067: CompositorFrame()); MakeCompositorFrame(empty_surface_ids()) https://codereview.chromium.org/2760433002/diff/20001/cc/surfaces/compositor_frame_sink_support_unittest.cc#newcode1069 cc/surfaces/compositor_frame_sink_support_unittest.cc:1069: EXPECT_TRUE(surface); ASSERT_TRUE(surface)
1 year, 1 month ago (2017-03-16 23:31:49 UTC) #12
Saman Sami
jbauman@ PTAL https://codereview.chromium.org/2760433002/diff/20001/cc/surfaces/compositor_frame_sink_support_unittest.cc File cc/surfaces/compositor_frame_sink_support_unittest.cc (right): https://codereview.chromium.org/2760433002/diff/20001/cc/surfaces/compositor_frame_sink_support_unittest.cc#newcode1067 cc/surfaces/compositor_frame_sink_support_unittest.cc:1067: CompositorFrame()); On 2017/03/16 23:31:49, Fady Samuel wrote: ...
1 year, 1 month ago (2017-03-16 23:43:53 UTC) #16
Fady Samuel
https://codereview.chromium.org/2760433002/diff/40001/cc/surfaces/compositor_frame_sink_support_unittest.cc File cc/surfaces/compositor_frame_sink_support_unittest.cc (right): https://codereview.chromium.org/2760433002/diff/40001/cc/surfaces/compositor_frame_sink_support_unittest.cc#newcode1085 cc/surfaces/compositor_frame_sink_support_unittest.cc:1085: CompositorFrame()); Oops, here too: MakeCompositorFrame(empty_surface_ids())
1 year, 1 month ago (2017-03-17 01:01:11 UTC) #17
jbauman
So the previous resources will be leaked if the Surface is destroyed before the Surface ...
1 year, 1 month ago (2017-03-17 20:56:55 UTC) #22
Fady Samuel
On 2017/03/17 20:56:55, jbauman wrote: > So the previous resources will be leaked if the ...
1 year, 1 month ago (2017-03-17 21:03:02 UTC) #23
jbauman
lgtm, though I think there are some additional pitfalls you have to worry about. I ...
1 year, 1 month ago (2017-03-17 21:19:58 UTC) #24
Saman Sami
On 2017/03/17 21:19:58, jbauman wrote: > lgtm, though I think there are some additional pitfalls ...
1 year, 1 month ago (2017-03-17 21:52:06 UTC) #25
Saman Sami
Based on the discussion I had with fsamuel@, probably evicting the frames if factory changes ...
1 year ago (2017-03-20 16:21:40 UTC) #29
Saman Sami
https://codereview.chromium.org/2760433002/diff/40001/cc/surfaces/compositor_frame_sink_support_unittest.cc File cc/surfaces/compositor_frame_sink_support_unittest.cc (right): https://codereview.chromium.org/2760433002/diff/40001/cc/surfaces/compositor_frame_sink_support_unittest.cc#newcode1085 cc/surfaces/compositor_frame_sink_support_unittest.cc:1085: CompositorFrame()); On 2017/03/17 01:01:10, Fady Samuel wrote: > Oops, ...
1 year ago (2017-03-20 16:26:09 UTC) #34
Saman Sami
PTAL. Recent changes warrant another look IMO.
1 year ago (2017-03-20 17:37:07 UTC) #39
Fady Samuel
Still LGTM. It's unfortunate we need to do this, but I support it in the ...
1 year ago (2017-03-20 18:37:52 UTC) #42
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/2760433002/120001
1 year ago (2017-03-20 18:40:55 UTC) #45
commit-bot: I haz the power
Committed patchset #6 (id:120001) as https://chromium.googlesource.com/chromium/src/+/9e0fd9a87d69362103af512b8e7b8946604abe71
1 year ago (2017-03-20 18:56:49 UTC) #48
Saman Sami
1 year ago (2017-04-03 17:37:26 UTC) #49
Message was sent while issue was closed.
A revert of this CL (patchset #6 id:120001) has been created in
https://codereview.chromium.org/2794993002/ by samans@chromium.org.

The reason for reverting is: Should not be necessary anymore now that
crbug.com/701988 is fixed..

Powered by Google App Engine
This is Rietveld 408576698