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

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

Can't Edit
Can't Publish+Mail
Start Review
Created:
2 months, 1 week ago by Saman Sami
Modified:
2 months, 1 week 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: 49 (34 generated)
Saman Sami
PTAL
2 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)
2 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: ...
2 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())
2 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 ...
2 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 ...
2 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 ...
2 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 ...
2 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 ...
2 months, 1 week 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, ...
2 months, 1 week ago (2017-03-20 16:26:09 UTC) #34
Saman Sami
PTAL. Recent changes warrant another look IMO.
2 months, 1 week 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 ...
2 months, 1 week 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
2 months, 1 week 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
2 months, 1 week ago (2017-03-20 18:56:49 UTC) #48
Saman Sami
1 month, 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..
Sign in to reply to this message.

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