Chromium Code Reviews
Help | Chromium Project | Sign in
(6)

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

Can't Edit
Can't Publish+Mail
Start Review
Created:
1 week, 5 days ago by Saman Sami
Modified:
1 week, 1 day 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 #

Messages

Total messages: 48 (34 generated)
Saman Sami
PTAL
1 week, 5 days 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 week, 5 days 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 week, 5 days 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 week, 5 days 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 week, 4 days 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 week, 4 days 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 week, 4 days 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 week, 4 days 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 week, 1 day 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 week, 1 day ago (2017-03-20 16:26:09 UTC) #34
Saman Sami
PTAL. Recent changes warrant another look IMO.
1 week, 1 day 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 week, 1 day 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 week, 1 day ago (2017-03-20 18:40:55 UTC) #45
commit-bot: I haz the power
1 week, 1 day ago (2017-03-20 18:56:49 UTC) #48
Message was sent while issue was closed.
Committed patchset #6 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/9e0fd9a87d69362103af512b8e7b...
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld cc6ac46