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

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

Created:
11 months, 1 week ago by Saman Sami
Modified:
11 months 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
11 months, 1 week 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)
11 months, 1 week 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: ...
11 months, 1 week 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())
11 months, 1 week 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 ...
11 months, 1 week 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 ...
11 months, 1 week 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 ...
11 months, 1 week 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 ...
11 months, 1 week 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 ...
11 months 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, ...
11 months ago (2017-03-20 16:26:09 UTC) #34
Saman Sami
PTAL. Recent changes warrant another look IMO.
11 months 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 ...
11 months 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
11 months 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
11 months ago (2017-03-20 18:56:49 UTC) #48
Saman Sami
10 months, 3 weeks 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