Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2940183002:
cc: Move ownership of surfaces to SurfaceManager (Closed)
|
Created:
3 years, 6 months ago by Saman Sami Modified:
3 years, 5 months ago CC:
chromium-reviews, cc-bugs_chromium.org, kylechar Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
Descriptioncc: Move ownership of surfaces to SurfaceManager
SurfaceManager::CreateSurface gives up the ownership of the surface
it creates to CompositorFrameSinkSupport by returning a unique_ptr.
CompositorFrameSinkSupport later passes back the unique_ptr to
SurfaceManager::DestroySurface when it wants to destroy it. Eventually
we want SurfaceManager::DestroySurface to go away and give full
authority to SurfaceManager to destroy surfaces when no one is
referencing them without involving CompsositorFrameSinkSupport. To this
end, this CL amends CreateSurface so it returns a raw pointer and
keeps the unique_ptr in SurfaceManager.
BUG=733434
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
Review-Url: https://codereview.chromium.org/2940183002
Cr-Commit-Position: refs/heads/master@{#483165}
Committed: https://chromium.googlesource.com/chromium/src/+/c4d8e3bfc8190090ae22ce114d73d7313135c29c
Patch Set 1 #Patch Set 2 : Rename UnregisterSurface #
Total comments: 4
Patch Set 3 : c #Patch Set 4 : c #Patch Set 5 : return weakptr #Patch Set 6 : c #
Total comments: 8
Patch Set 7 : Remove destroyed #Patch Set 8 : flat set #Patch Set 9 : cleanup #Patch Set 10 : flat map #Patch Set 11 : fix segfault #Patch Set 12 : Remove weakptr #Patch Set 13 : Fix test #
Total comments: 16
Patch Set 14 : Rebase #Patch Set 15 : c #
Total comments: 12
Patch Set 16 : Address comments #
Total comments: 27
Patch Set 17 : Addressed comments #Patch Set 18 : Address comments #
Total comments: 1
Patch Set 19 : Add comment #Patch Set 20 : Fix exo #
Messages
Total messages: 120 (95 generated)
Description was changed from ========== cc: Move ownership of surfaces to SurfaceManager SurfaceManager::CreateSurface gives up the ownership of the surface it creates to CompositorFrameSinkSupport by returning a unique_ptr. CompositorFrameSinkSupport later passes back the unique_ptr to SurfaceManager::DestroySurface when it wants to destroy it. Eventually we want SurfaceManager::DestroySurface to go away and give full authority to SurfaceManager to destroy surfaces when no one is referencing them without involving CompsositorFrameSinkSupport. To this end, this CL amends CreateSurface so it returns a raw pointer and keeps the unique_ptr in SurfaceManager. BUG=733434 ========== to ========== cc: Move ownership of surfaces to SurfaceManager SurfaceManager::CreateSurface gives up the ownership of the surface it creates to CompositorFrameSinkSupport by returning a unique_ptr. CompositorFrameSinkSupport later passes back the unique_ptr to SurfaceManager::DestroySurface when it wants to destroy it. Eventually we want SurfaceManager::DestroySurface to go away and give full authority to SurfaceManager to destroy surfaces when no one is referencing them without involving CompsositorFrameSinkSupport. To this end, this CL amends CreateSurface so it returns a raw pointer and keeps the unique_ptr in SurfaceManager. BUG=733434 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
The CQ bit was checked by samans@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by samans@chromium.org to run a CQ dry run
samans@chromium.org changed reviewers: + fsamuel@chromium.org
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by samans@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by samans@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== cc: Move ownership of surfaces to SurfaceManager SurfaceManager::CreateSurface gives up the ownership of the surface it creates to CompositorFrameSinkSupport by returning a unique_ptr. CompositorFrameSinkSupport later passes back the unique_ptr to SurfaceManager::DestroySurface when it wants to destroy it. Eventually we want SurfaceManager::DestroySurface to go away and give full authority to SurfaceManager to destroy surfaces when no one is referencing them without involving CompsositorFrameSinkSupport. To this end, this CL amends CreateSurface so it returns a raw pointer and keeps the unique_ptr in SurfaceManager. BUG=733434 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== cc: Move ownership of surfaces to SurfaceManager SurfaceManager::CreateSurface gives up the ownership of the surface it creates to CompositorFrameSinkSupport by returning a unique_ptr. CompositorFrameSinkSupport later passes back the unique_ptr to SurfaceManager::DestroySurface when it wants to destroy it. Eventually we want SurfaceManager::DestroySurface to go away and give full authority to SurfaceManager to destroy surfaces when no one is referencing them without involving CompsositorFrameSinkSupport. To this end, this CL amends CreateSurface so it returns a weak pointer and keeps the unique_ptr in SurfaceManager. BUG=733434 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
The CQ bit was checked by samans@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by samans@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
samans@chromium.org changed reviewers: + jbauman@chromium.org
jbauman please review cc
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2940183002/diff/20001/cc/surfaces/surface_man... File cc/surfaces/surface_manager.h (right): https://codereview.chromium.org/2940183002/diff/20001/cc/surfaces/surface_man... cc/surfaces/surface_manager.h:276: std::unordered_map<SurfaceId, std::unique_ptr<Surface>, SurfaceIdHash>; base::flat_map https://codereview.chromium.org/2940183002/diff/20001/cc/surfaces/surface_man... cc/surfaces/surface_manager.h:281: std::set<Surface*> surfaces_to_destroy_; flat_set https://codereview.chromium.org/2940183002/diff/90001/cc/surfaces/surface_man... File cc/surfaces/surface_manager.cc (right): https://codereview.chromium.org/2940183002/diff/90001/cc/surfaces/surface_man... cc/surfaces/surface_manager.cc:96: DCHECK(surface->destroyed()); Can we get rid of the destroy bit?
Some drive by comments. https://codereview.chromium.org/2940183002/diff/90001/cc/surfaces/compositor_... File cc/surfaces/compositor_frame_sink_support.cc (right): https://codereview.chromium.org/2940183002/diff/90001/cc/surfaces/compositor_... cc/surfaces/compositor_frame_sink_support.cc:131: Surface* prev_surface = current_surface_.get(); This is the only place you really use the Surface*. Can you hold onto a SurfaceId and look it up here? That would eliminate all of the added WeakPtr code. https://codereview.chromium.org/2940183002/diff/90001/cc/surfaces/surface_man... File cc/surfaces/surface_manager.h (right): https://codereview.chromium.org/2940183002/diff/90001/cc/surfaces/surface_man... cc/surfaces/surface_manager.h:75: void DestroySurface(Surface* surface); Can this just take a SurfaceId instead of a Surface*? https://codereview.chromium.org/2940183002/diff/90001/cc/surfaces/surface_man... cc/surfaces/surface_manager.h:281: std::set<Surface*> surfaces_to_destroy_; Can you use base::flat_set here? This should almost always be very small. Also you only use the pointer to get a SurfaceId from, you could just have a set of SurfaceIds?
The CQ bit was checked by samans@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by samans@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by samans@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by samans@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by samans@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by samans@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
samans@chromium.org changed reviewers: - jbauman@chromium.org
https://codereview.chromium.org/2940183002/diff/20001/cc/surfaces/surface_man... File cc/surfaces/surface_manager.h (right): https://codereview.chromium.org/2940183002/diff/20001/cc/surfaces/surface_man... cc/surfaces/surface_manager.h:276: std::unordered_map<SurfaceId, std::unique_ptr<Surface>, SurfaceIdHash>; On 2017/06/16 18:56:38, Fady Samuel wrote: > base::flat_map Done. https://codereview.chromium.org/2940183002/diff/20001/cc/surfaces/surface_man... cc/surfaces/surface_manager.h:281: std::set<Surface*> surfaces_to_destroy_; On 2017/06/16 18:56:38, Fady Samuel wrote: > flat_set Done. https://codereview.chromium.org/2940183002/diff/90001/cc/surfaces/compositor_... File cc/surfaces/compositor_frame_sink_support.cc (right): https://codereview.chromium.org/2940183002/diff/90001/cc/surfaces/compositor_... cc/surfaces/compositor_frame_sink_support.cc:131: Surface* prev_surface = current_surface_.get(); On 2017/06/16 19:13:30, kylechar wrote: > This is the only place you really use the Surface*. Can you hold onto a > SurfaceId and look it up here? That would eliminate all of the added WeakPtr > code. Done. https://codereview.chromium.org/2940183002/diff/90001/cc/surfaces/surface_man... File cc/surfaces/surface_manager.cc (right): https://codereview.chromium.org/2940183002/diff/90001/cc/surfaces/surface_man... cc/surfaces/surface_manager.cc:96: DCHECK(surface->destroyed()); On 2017/06/16 18:56:38, Fady Samuel wrote: > Can we get rid of the destroy bit? Done. https://codereview.chromium.org/2940183002/diff/90001/cc/surfaces/surface_man... File cc/surfaces/surface_manager.h (right): https://codereview.chromium.org/2940183002/diff/90001/cc/surfaces/surface_man... cc/surfaces/surface_manager.h:75: void DestroySurface(Surface* surface); On 2017/06/16 19:13:30, kylechar wrote: > Can this just take a SurfaceId instead of a Surface*? Done. https://codereview.chromium.org/2940183002/diff/90001/cc/surfaces/surface_man... cc/surfaces/surface_manager.h:281: std::set<Surface*> surfaces_to_destroy_; On 2017/06/16 19:13:30, kylechar wrote: > Can you use base::flat_set here? This should almost always be very small. Also > you only use the pointer to get a SurfaceId from, you could just have a set of > SurfaceIds? Done.
Description was changed from ========== cc: Move ownership of surfaces to SurfaceManager SurfaceManager::CreateSurface gives up the ownership of the surface it creates to CompositorFrameSinkSupport by returning a unique_ptr. CompositorFrameSinkSupport later passes back the unique_ptr to SurfaceManager::DestroySurface when it wants to destroy it. Eventually we want SurfaceManager::DestroySurface to go away and give full authority to SurfaceManager to destroy surfaces when no one is referencing them without involving CompsositorFrameSinkSupport. To this end, this CL amends CreateSurface so it returns a weak pointer and keeps the unique_ptr in SurfaceManager. BUG=733434 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== cc: Move ownership of surfaces to SurfaceManager SurfaceManager::CreateSurface gives up the ownership of the surface it creates to CompositorFrameSinkSupport by returning a unique_ptr. CompositorFrameSinkSupport later passes back the unique_ptr to SurfaceManager::DestroySurface when it wants to destroy it. Eventually we want SurfaceManager::DestroySurface to go away and give full authority to SurfaceManager to destroy surfaces when no one is referencing them without involving CompsositorFrameSinkSupport. To this end, this CL amends CreateSurface so it returns a raw pointer and keeps the unique_ptr in SurfaceManager. BUG=733434 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
The CQ bit was checked by samans@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by samans@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2940183002/diff/230001/cc/surfaces/compositor... File cc/surfaces/compositor_frame_sink_support.cc (right): https://codereview.chromium.org/2940183002/diff/230001/cc/surfaces/compositor... cc/surfaces/compositor_frame_sink_support.cc:80: if (!GetCurrentSurface()) This is less efficient than it needs to be. Surface* current_surface = GetCurrentSurface(); if (current_surface) surface_manager_->DestroySurface(current_surface->surface_id()); https://codereview.chromium.org/2940183002/diff/230001/cc/surfaces/compositor... cc/surfaces/compositor_frame_sink_support.cc:99: if (GetCurrentSurface()) Surface* current_surface = GetCurrentSurface(); if (current_surface) surface_manager_->SurfaceModified(current_surface->surface_id(), ack); https://codereview.chromium.org/2940183002/diff/230001/cc/surfaces/compositor... cc/surfaces/compositor_frame_sink_support.cc:133: surface_manager()->IsMarkedForDestruction(GetCurrentSurfaceId()) || replace GetCurrentSurface() => current_surface->surface_id() https://codereview.chromium.org/2940183002/diff/230001/cc/surfaces/compositor... cc/surfaces/compositor_frame_sink_support.cc:287: if (GetCurrentSurface()) { Drop the braces: Surface* current_surface = GetCurrentSurface(); if (current_surface) surface_manager_->SurfaceDamageExpected(current_surface->surface_id(), args); https://codereview.chromium.org/2940183002/diff/230001/cc/surfaces/compositor... cc/surfaces/compositor_frame_sink_support.cc:364: SurfaceId CompositorFrameSinkSupport::GetCurrentSurfaceId() { Maybe we don't need this? https://codereview.chromium.org/2940183002/diff/230001/cc/surfaces/compositor... File cc/surfaces/compositor_frame_sink_support.h (right): https://codereview.chromium.org/2940183002/diff/230001/cc/surfaces/compositor... cc/surfaces/compositor_frame_sink_support.h:70: Surface* GetCurrentSurface(); Does this need to be public? https://codereview.chromium.org/2940183002/diff/230001/cc/surfaces/compositor... cc/surfaces/compositor_frame_sink_support.h:71: SurfaceId GetCurrentSurfaceId(); Does this need to be public? https://codereview.chromium.org/2940183002/diff/230001/cc/surfaces/surface_ma... File cc/surfaces/surface_manager.h (right): https://codereview.chromium.org/2940183002/diff/230001/cc/surfaces/surface_ma... cc/surfaces/surface_manager.h:70: Surface* CreateSurface( nit: Comment explaining ownership of Surface.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by samans@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
The CQ bit was checked by samans@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2940183002/diff/230001/cc/surfaces/compositor... File cc/surfaces/compositor_frame_sink_support.cc (right): https://codereview.chromium.org/2940183002/diff/230001/cc/surfaces/compositor... cc/surfaces/compositor_frame_sink_support.cc:80: if (!GetCurrentSurface()) On 2017/06/20 17:29:06, Fady Samuel wrote: > This is less efficient than it needs to be. > > Surface* current_surface = GetCurrentSurface(); > if (current_surface) > surface_manager_->DestroySurface(current_surface->surface_id()); Done. https://codereview.chromium.org/2940183002/diff/230001/cc/surfaces/compositor... cc/surfaces/compositor_frame_sink_support.cc:99: if (GetCurrentSurface()) On 2017/06/20 17:29:06, Fady Samuel wrote: > Surface* current_surface = GetCurrentSurface(); > if (current_surface) > surface_manager_->SurfaceModified(current_surface->surface_id(), ack); Done. https://codereview.chromium.org/2940183002/diff/230001/cc/surfaces/compositor... cc/surfaces/compositor_frame_sink_support.cc:133: surface_manager()->IsMarkedForDestruction(GetCurrentSurfaceId()) || On 2017/06/20 17:29:06, Fady Samuel wrote: > replace GetCurrentSurface() => current_surface->surface_id() Done. https://codereview.chromium.org/2940183002/diff/230001/cc/surfaces/compositor... cc/surfaces/compositor_frame_sink_support.cc:287: if (GetCurrentSurface()) { On 2017/06/20 17:29:06, Fady Samuel wrote: > Drop the braces: > > Surface* current_surface = GetCurrentSurface(); > if (current_surface) > surface_manager_->SurfaceDamageExpected(current_surface->surface_id(), args); Done. https://codereview.chromium.org/2940183002/diff/230001/cc/surfaces/compositor... cc/surfaces/compositor_frame_sink_support.cc:364: SurfaceId CompositorFrameSinkSupport::GetCurrentSurfaceId() { On 2017/06/20 17:29:06, Fady Samuel wrote: > Maybe we don't need this? Done. https://codereview.chromium.org/2940183002/diff/230001/cc/surfaces/compositor... File cc/surfaces/compositor_frame_sink_support.h (right): https://codereview.chromium.org/2940183002/diff/230001/cc/surfaces/compositor... cc/surfaces/compositor_frame_sink_support.h:70: Surface* GetCurrentSurface(); On 2017/06/20 17:29:07, Fady Samuel wrote: > Does this need to be public? It's used in some tests. https://codereview.chromium.org/2940183002/diff/230001/cc/surfaces/compositor... cc/surfaces/compositor_frame_sink_support.h:71: SurfaceId GetCurrentSurfaceId(); On 2017/06/20 17:29:06, Fady Samuel wrote: > Does this need to be public? I just removed it per your suggestion. https://codereview.chromium.org/2940183002/diff/230001/cc/surfaces/surface_ma... File cc/surfaces/surface_manager.h (right): https://codereview.chromium.org/2940183002/diff/230001/cc/surfaces/surface_ma... cc/surfaces/surface_manager.h:70: Surface* CreateSurface( On 2017/06/20 17:29:07, Fady Samuel wrote: > nit: Comment explaining ownership of Surface. Done.
fsamuel@google.com changed reviewers: + fsamuel@google.com
Yaay! LGTM, Please pass this along to jbauman@ :-)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by samans@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
samans@chromium.org changed reviewers: + jbauman@chromium.org
jbauman please review cc
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
samans@chromium.org changed reviewers: + danakj@chromium.org - jbauman@chromium.org
Seems like jbauman is OOO. danakj could you please review cc?
https://codereview.chromium.org/2940183002/diff/270001/cc/surfaces/compositor... File cc/surfaces/compositor_frame_sink_support.cc (right): https://codereview.chromium.org/2940183002/diff/270001/cc/surfaces/compositor... cc/surfaces/compositor_frame_sink_support.cc:80: Surface* current_surface = GetCurrentSurface(); It feels like in general this class is dealing with a Surface* when it should be dealing with surface ids, cuz it is using the Surface* basically to get an ID from it and to test if the surface exists. If you had SurfaceManager::SurfaceExists(SurfaceId) then this class wouldn't need a pointer at all except for RequestCopyOfOutput and QueueFrame? https://codereview.chromium.org/2940183002/diff/270001/cc/surfaces/compositor... cc/surfaces/compositor_frame_sink_support.cc:83: surface_manager_->DestroySurface(current_surface->surface_id()); This could take a surface id and just ignore ids that don't exist. https://codereview.chromium.org/2940183002/diff/270001/cc/surfaces/compositor... cc/surfaces/compositor_frame_sink_support.cc:102: surface_manager_->SurfaceModified(current_surface->surface_id(), ack); This could also early out if the surface didn't exist. The thing is I don't also get why this is checking with SurfaceManager if the surface exists at all. Would the CompositorFrameSinkSupport not have a null current_surface_local_id_ and it could just check that? https://codereview.chromium.org/2940183002/diff/270001/cc/surfaces/compositor... File cc/surfaces/compositor_frame_sink_support.h (right): https://codereview.chromium.org/2940183002/diff/270001/cc/surfaces/compositor... cc/surfaces/compositor_frame_sink_support.h:70: Surface* GetCurrentSurface(); We don't want users of this to go using the Surface*, so we should only expose this to unit tests. Either protected with a subclass in the test that exposes it ("using ParentClass::Method;"), or add a public ForTesting() method that calls the private one. https://codereview.chromium.org/2940183002/diff/270001/cc/surfaces/surface_ma... File cc/surfaces/surface_manager.cc (right): https://codereview.chromium.org/2940183002/diff/270001/cc/surfaces/surface_ma... cc/surfaces/surface_manager.cc:49: surface_map_.clear(); Why do we clear these in the destructor here? Their own destructors do this. Please write comments explaining why we need to do these clear()s explicitly here. https://codereview.chromium.org/2940183002/diff/270001/cc/surfaces/surface_ma... File cc/surfaces/surface_manager.h (right): https://codereview.chromium.org/2940183002/diff/270001/cc/surfaces/surface_ma... cc/surfaces/surface_manager.h:218: bool IsMarkedForDestruction(const SurfaceId& surface_id); Do we want this to be part of the public API for production?
The CQ bit was checked by samans@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2940183002/diff/290001/cc/surfaces/compositor... File cc/surfaces/compositor_frame_sink_support.h (right): https://codereview.chromium.org/2940183002/diff/290001/cc/surfaces/compositor... cc/surfaces/compositor_frame_sink_support.h:124: Surface* current_surface_ = nullptr; Make this a SurfaceId?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by samans@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by samans@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #17 (id:310001) has been deleted
Patchset #16 (id:290001) has been deleted
danakj please take another look https://codereview.chromium.org/2940183002/diff/270001/cc/surfaces/compositor... File cc/surfaces/compositor_frame_sink_support.cc (right): https://codereview.chromium.org/2940183002/diff/270001/cc/surfaces/compositor... cc/surfaces/compositor_frame_sink_support.cc:80: Surface* current_surface = GetCurrentSurface(); On 2017/06/21 21:24:09, danakj wrote: > It feels like in general this class is dealing with a Surface* when it should be > dealing with surface ids, cuz it is using the Surface* basically to get an ID > from it and to test if the surface exists. > > If you had SurfaceManager::SurfaceExists(SurfaceId) then this class wouldn't > need a pointer at all except for RequestCopyOfOutput and QueueFrame? I just keep the current SurfaceId and reset it when we're destroying the surface. https://codereview.chromium.org/2940183002/diff/270001/cc/surfaces/compositor... cc/surfaces/compositor_frame_sink_support.cc:83: surface_manager_->DestroySurface(current_surface->surface_id()); On 2017/06/21 21:24:09, danakj wrote: > This could take a surface id and just ignore ids that don't exist. I'll just check current_surface_id_ is valid. https://codereview.chromium.org/2940183002/diff/270001/cc/surfaces/compositor... cc/surfaces/compositor_frame_sink_support.cc:102: surface_manager_->SurfaceModified(current_surface->surface_id(), ack); On 2017/06/21 21:24:09, danakj wrote: > This could also early out if the surface didn't exist. The thing is I don't also > get why this is checking with SurfaceManager if the surface exists at all. Would > the CompositorFrameSinkSupport not have a null current_surface_local_id_ and it > could just check that? I'll just check current_surface_id_ is valid. https://codereview.chromium.org/2940183002/diff/270001/cc/surfaces/compositor... File cc/surfaces/compositor_frame_sink_support.h (right): https://codereview.chromium.org/2940183002/diff/270001/cc/surfaces/compositor... cc/surfaces/compositor_frame_sink_support.h:70: Surface* GetCurrentSurface(); On 2017/06/21 21:24:09, danakj wrote: > We don't want users of this to go using the Surface*, so we should only expose > this to unit tests. Either protected with a subclass in the test that exposes it > ("using ParentClass::Method;"), or add a public ForTesting() method that calls > the private one. I added a ForTesting suffix. https://codereview.chromium.org/2940183002/diff/270001/cc/surfaces/surface_ma... File cc/surfaces/surface_manager.cc (right): https://codereview.chromium.org/2940183002/diff/270001/cc/surfaces/surface_ma... cc/surfaces/surface_manager.cc:49: surface_map_.clear(); On 2017/06/21 21:24:09, danakj wrote: > Why do we clear these in the destructor here? Their own destructors do this. > Please write comments explaining why we need to do these clear()s explicitly > here. We actually don't need them so I'll remove them. https://codereview.chromium.org/2940183002/diff/270001/cc/surfaces/surface_ma... File cc/surfaces/surface_manager.h (right): https://codereview.chromium.org/2940183002/diff/270001/cc/surfaces/surface_ma... cc/surfaces/surface_manager.h:218: bool IsMarkedForDestruction(const SurfaceId& surface_id); On 2017/06/21 21:24:09, danakj wrote: > Do we want this to be part of the public API for production? I made it private.
https://codereview.chromium.org/2940183002/diff/330001/cc/surfaces/compositor... File cc/surfaces/compositor_frame_sink_support.cc (left): https://codereview.chromium.org/2940183002/diff/330001/cc/surfaces/compositor... cc/surfaces/compositor_frame_sink_support.cc:348: return surface_manager_->CreateSurface(weak_factory_.GetWeakPtr(), Would it be simpler if this was void? https://codereview.chromium.org/2940183002/diff/330001/cc/surfaces/compositor... File cc/surfaces/compositor_frame_sink_support.cc (right): https://codereview.chromium.org/2940183002/diff/330001/cc/surfaces/compositor... cc/surfaces/compositor_frame_sink_support.cc:345: Surface* CompositorFrameSinkSupport::CreateSurface( Would it be simpler if this was void?
https://codereview.chromium.org/2940183002/diff/330001/cc/surfaces/compositor... File cc/surfaces/compositor_frame_sink_support.cc (right): https://codereview.chromium.org/2940183002/diff/330001/cc/surfaces/compositor... cc/surfaces/compositor_frame_sink_support.cc:345: Surface* CompositorFrameSinkSupport::CreateSurface( On 2017/06/26 20:46:43, Fady Samuel wrote: > Would it be simpler if this was void? Then we would have to call GetSurfaceForId after creating the surface. This is slightly more efficient and cleaner.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
still lgtm...please ping Dana for review?
https://codereview.chromium.org/2940183002/diff/330001/cc/surfaces/compositor... File cc/surfaces/compositor_frame_sink_support.cc (right): https://codereview.chromium.org/2940183002/diff/330001/cc/surfaces/compositor... cc/surfaces/compositor_frame_sink_support.cc:82: SurfaceId current_surface_id = current_surface_id_; give this a name that differs from the member var by more than a _. to_destroy_surface_id is one option. https://codereview.chromium.org/2940183002/diff/330001/cc/surfaces/compositor... cc/surfaces/compositor_frame_sink_support.cc:133: Surface* current_surface = give this a name that differs from the member variable by more than a _. replacement_surface is one such option. https://codereview.chromium.org/2940183002/diff/330001/cc/surfaces/compositor... cc/surfaces/compositor_frame_sink_support.cc:134: surface_manager_->GetSurfaceForId(current_surface_id_); can you use if/else to only call this when we would use it? Or maybe do this and store that as previous_surface, then in the if/else you can assign replacement_surface = previous_surface when we're not going to change it? https://codereview.chromium.org/2940183002/diff/330001/cc/surfaces/compositor... cc/surfaces/compositor_frame_sink_support.cc:358: DCHECK(current_surface->compositor_frame_sink_support().get() == this); DCHECK_EQ https://codereview.chromium.org/2940183002/diff/330001/cc/surfaces/surface_ma... File cc/surfaces/surface_manager.cc (right): https://codereview.chromium.org/2940183002/diff/330001/cc/surfaces/surface_ma... cc/surfaces/surface_manager.cc:41: SurfaceManager::~SurfaceManager() = default; should we dcheck that all surfaces are destroyed by now? how are we guarding lifetimes? should this return some RAII object when CreateSurface that you have to hand back with DeleteSurface, and it DCHECKs if its destroyed by anyone other than SurfaceManager? Or how can we feel confident that we're destroying what we create and not leaking surfaces? https://codereview.chromium.org/2940183002/diff/330001/cc/surfaces/surface_ma... cc/surfaces/surface_manager.cc:97: if (IsMarkedForDestruction(surface_id)) We want to be able to destroy a surface twice? https://codereview.chromium.org/2940183002/diff/330001/cc/surfaces/surface_ma... cc/surfaces/surface_manager.cc:508: std::unique_ptr<Surface> doomed = std::move(it->second); do you mean to keep this |doomed| alive until the end of the function? erase() would delete it too, cuz the unique_ptr will. https://codereview.chromium.org/2940183002/diff/330001/cc/surfaces/surface_ma... File cc/surfaces/surface_manager.h (right): https://codereview.chromium.org/2940183002/diff/330001/cc/surfaces/surface_ma... cc/surfaces/surface_manager.h:285: using SurfaceMap = base::flat_map<SurfaceId, std::unique_ptr<Surface>>; nit: drop the type alias and just use the flat_map type to define the variable?
The CQ bit was checked by samans@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2940183002/diff/330001/cc/surfaces/compositor... File cc/surfaces/compositor_frame_sink_support.cc (right): https://codereview.chromium.org/2940183002/diff/330001/cc/surfaces/compositor... cc/surfaces/compositor_frame_sink_support.cc:82: SurfaceId current_surface_id = current_surface_id_; On 2017/06/27 18:21:31, danakj wrote: > give this a name that differs from the member var by more than a _. > to_destroy_surface_id is one option. Done. https://codereview.chromium.org/2940183002/diff/330001/cc/surfaces/compositor... cc/surfaces/compositor_frame_sink_support.cc:133: Surface* current_surface = On 2017/06/27 18:21:31, danakj wrote: > give this a name that differs from the member variable by more than a _. > replacement_surface is one such option. But there is no current_surface_? There is only a current_surface_id_ and I think current_surface is a reasonable name for a surface whose id is current_surface_id_. https://codereview.chromium.org/2940183002/diff/330001/cc/surfaces/compositor... cc/surfaces/compositor_frame_sink_support.cc:134: surface_manager_->GetSurfaceForId(current_surface_id_); On 2017/06/27 18:21:31, danakj wrote: > can you use if/else to only call this when we would use it? Or maybe do this and > store that as previous_surface, then in the if/else you can assign > replacement_surface = previous_surface when we're not going to change it? I don't think I fully understand your suggestion. This variable will be used when we queue a frame, unless you are worried about the invalid SurfaceInfo case? That's an edge case and doesn't really happen. I'm also not sure how assigning this to previous_surface instead of current_surface helps? Can you elaborate? Thanks. https://codereview.chromium.org/2940183002/diff/330001/cc/surfaces/compositor... cc/surfaces/compositor_frame_sink_support.cc:358: DCHECK(current_surface->compositor_frame_sink_support().get() == this); On 2017/06/27 18:21:31, danakj wrote: > DCHECK_EQ Done. https://codereview.chromium.org/2940183002/diff/330001/cc/surfaces/surface_ma... File cc/surfaces/surface_manager.cc (right): https://codereview.chromium.org/2940183002/diff/330001/cc/surfaces/surface_ma... cc/surfaces/surface_manager.cc:41: SurfaceManager::~SurfaceManager() = default; On 2017/06/27 18:21:32, danakj wrote: > should we dcheck that all surfaces are destroyed by now? how are we guarding > lifetimes? should this return some RAII object when CreateSurface that you have > to hand back with DeleteSurface, and it DCHECKs if its destroyed by anyone other > than SurfaceManager? Or how can we feel confident that we're destroying what we > create and not leaking surfaces? All Surfaces are owned by SurfaceManager. When it's destroyed, all Surfaces are destroyed. There is no leak. https://codereview.chromium.org/2940183002/diff/330001/cc/surfaces/surface_ma... cc/surfaces/surface_manager.cc:97: if (IsMarkedForDestruction(surface_id)) On 2017/06/27 18:21:32, danakj wrote: > We want to be able to destroy a surface twice? I think I can get rid of this now. Earlier patch sets needed this. https://codereview.chromium.org/2940183002/diff/330001/cc/surfaces/surface_ma... cc/surfaces/surface_manager.cc:508: std::unique_ptr<Surface> doomed = std::move(it->second); On 2017/06/27 18:21:31, danakj wrote: > do you mean to keep this |doomed| alive until the end of the function? erase() > would delete it too, cuz the unique_ptr will. Yes, I want the surface to be removed from the map before its destructor is run and an ack is sent. https://codereview.chromium.org/2940183002/diff/330001/cc/surfaces/surface_ma... File cc/surfaces/surface_manager.h (right): https://codereview.chromium.org/2940183002/diff/330001/cc/surfaces/surface_ma... cc/surfaces/surface_manager.h:285: using SurfaceMap = base::flat_map<SurfaceId, std::unique_ptr<Surface>>; On 2017/06/27 18:21:32, danakj wrote: > nit: drop the type alias and just use the flat_map type to define the variable? I guess it used to be useful for creating iterators but nowadays you can just use auto.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2940183002/diff/330001/cc/surfaces/compositor... File cc/surfaces/compositor_frame_sink_support.cc (right): https://codereview.chromium.org/2940183002/diff/330001/cc/surfaces/compositor... cc/surfaces/compositor_frame_sink_support.cc:133: Surface* current_surface = On 2017/06/28 14:19:43, Saman Sami wrote: > On 2017/06/27 18:21:31, danakj wrote: > > give this a name that differs from the member variable by more than a _. > > replacement_surface is one such option. > > But there is no current_surface_? There is only a current_surface_id_ and I > think current_surface is a reasonable name for a surface whose id is > current_surface_id_. Ah that's true, I was reading the old code too. https://codereview.chromium.org/2940183002/diff/330001/cc/surfaces/compositor... cc/surfaces/compositor_frame_sink_support.cc:134: surface_manager_->GetSurfaceForId(current_surface_id_); On 2017/06/28 14:19:43, Saman Sami wrote: > On 2017/06/27 18:21:31, danakj wrote: > > can you use if/else to only call this when we would use it? Or maybe do this > and > > store that as previous_surface, then in the if/else you can assign > > replacement_surface = previous_surface when we're not going to change it? > > I don't think I fully understand your suggestion. This variable will be used > when we queue a frame, unless you are worried about the invalid SurfaceInfo > case? That's an edge case and doesn't really happen. I'm also not sure how > assigning this to previous_surface instead of current_surface helps? Can you > elaborate? Thanks. T* t = MakeSomeTea(); if (foo) { t = MakeSomeOtherTea(); } vs T* t; if (!foo) { t = MakeSomeTea(); } else { t = MakeSomeOtherTea(); } Because the re-assignment of current_surface is pretty far from here, it's behaviour is harder to follow in the first way I think. Or here it'd be Surface* prev_surface = GetSurfaceForId(current_surface_id_); Surface* next_surface; if (want to use prev surface still) { next_surface = prev_surface; } else { .. build up a new surface .. next_surface = CreateSurface } This expresses the intent of what's going on and is a small change that improves how it reads I think. https://codereview.chromium.org/2940183002/diff/330001/cc/surfaces/surface_ma... File cc/surfaces/surface_manager.cc (right): https://codereview.chromium.org/2940183002/diff/330001/cc/surfaces/surface_ma... cc/surfaces/surface_manager.cc:41: SurfaceManager::~SurfaceManager() = default; On 2017/06/28 14:19:43, Saman Sami wrote: > On 2017/06/27 18:21:32, danakj wrote: > > should we dcheck that all surfaces are destroyed by now? how are we guarding > > lifetimes? should this return some RAII object when CreateSurface that you > have > > to hand back with DeleteSurface, and it DCHECKs if its destroyed by anyone > other > > than SurfaceManager? Or how can we feel confident that we're destroying what > we > > create and not leaking surfaces? > > All Surfaces are owned by SurfaceManager. When it's destroyed, all Surfaces are > destroyed. There is no leak. What about during the lifetime of the browser? I didn't mean at shutdown (tho we could notice them for sure at shutdown) https://codereview.chromium.org/2940183002/diff/330001/cc/surfaces/surface_ma... cc/surfaces/surface_manager.cc:508: std::unique_ptr<Surface> doomed = std::move(it->second); On 2017/06/28 14:19:44, Saman Sami wrote: > On 2017/06/27 18:21:31, danakj wrote: > > do you mean to keep this |doomed| alive until the end of the function? erase() > > would delete it too, cuz the unique_ptr will. > > Yes, I want the surface to be removed from the map before its destructor is run > and an ack is sent. Can you explain this in a comment? Is there a test that verifies this?
The CQ bit was checked by samans@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2940183002/diff/330001/cc/surfaces/compositor... File cc/surfaces/compositor_frame_sink_support.cc (right): https://codereview.chromium.org/2940183002/diff/330001/cc/surfaces/compositor... cc/surfaces/compositor_frame_sink_support.cc:134: surface_manager_->GetSurfaceForId(current_surface_id_); On 2017/06/28 15:59:45, danakj wrote: > On 2017/06/28 14:19:43, Saman Sami wrote: > > On 2017/06/27 18:21:31, danakj wrote: > > > can you use if/else to only call this when we would use it? Or maybe do this > > and > > > store that as previous_surface, then in the if/else you can assign > > > replacement_surface = previous_surface when we're not going to change it? > > > > I don't think I fully understand your suggestion. This variable will be used > > when we queue a frame, unless you are worried about the invalid SurfaceInfo > > case? That's an edge case and doesn't really happen. I'm also not sure how > > assigning this to previous_surface instead of current_surface helps? Can you > > elaborate? Thanks. > > T* t = MakeSomeTea(); > if (foo) { > t = MakeSomeOtherTea(); > } > > vs > > T* t; > if (!foo) { > t = MakeSomeTea(); > } else { > t = MakeSomeOtherTea(); > } > > Because the re-assignment of current_surface is pretty far from here, it's > behaviour is harder to follow in the first way I think. Or here it'd be > > Surface* prev_surface = GetSurfaceForId(current_surface_id_); > Surface* next_surface; > if (want to use prev surface still) { > next_surface = prev_surface; > } else { > .. build up a new surface .. > next_surface = CreateSurface > } > > This expresses the intent of what's going on and is a small change that improves > how it reads I think. Done. https://codereview.chromium.org/2940183002/diff/330001/cc/surfaces/surface_ma... File cc/surfaces/surface_manager.cc (right): https://codereview.chromium.org/2940183002/diff/330001/cc/surfaces/surface_ma... cc/surfaces/surface_manager.cc:41: SurfaceManager::~SurfaceManager() = default; On 2017/06/28 15:59:45, danakj wrote: > On 2017/06/28 14:19:43, Saman Sami wrote: > > On 2017/06/27 18:21:32, danakj wrote: > > > should we dcheck that all surfaces are destroyed by now? how are we guarding > > > lifetimes? should this return some RAII object when CreateSurface that you > > have > > > to hand back with DeleteSurface, and it DCHECKs if its destroyed by anyone > > other > > > than SurfaceManager? Or how can we feel confident that we're destroying what > > we > > > create and not leaking surfaces? > > > > All Surfaces are owned by SurfaceManager. When it's destroyed, all Surfaces > are > > destroyed. There is no leak. > > What about during the lifetime of the browser? I didn't mean at shutdown (tho we > could notice them for sure at shutdown) We can check if all the remaining surfaces in the surface map are marked for destruction. https://codereview.chromium.org/2940183002/diff/330001/cc/surfaces/surface_ma... cc/surfaces/surface_manager.cc:508: std::unique_ptr<Surface> doomed = std::move(it->second); On 2017/06/28 15:59:45, danakj wrote: > On 2017/06/28 14:19:44, Saman Sami wrote: > > On 2017/06/27 18:21:31, danakj wrote: > > > do you mean to keep this |doomed| alive until the end of the function? > erase() > > > would delete it too, cuz the unique_ptr will. > > > > Yes, I want the surface to be removed from the map before its destructor is > run > > and an ack is sent. > > Can you explain this in a comment? Is there a test that verifies this? Sure. CompositorFrameSinkSupportTest_AddDuringEviction should cover this.
LGTM https://codereview.chromium.org/2940183002/diff/330001/cc/surfaces/surface_ma... File cc/surfaces/surface_manager.cc (right): https://codereview.chromium.org/2940183002/diff/330001/cc/surfaces/surface_ma... cc/surfaces/surface_manager.cc:41: SurfaceManager::~SurfaceManager() = default; On 2017/06/28 18:01:00, Saman Sami wrote: > On 2017/06/28 15:59:45, danakj wrote: > > On 2017/06/28 14:19:43, Saman Sami wrote: > > > On 2017/06/27 18:21:32, danakj wrote: > > > > should we dcheck that all surfaces are destroyed by now? how are we > guarding > > > > lifetimes? should this return some RAII object when CreateSurface that you > > > have > > > > to hand back with DeleteSurface, and it DCHECKs if its destroyed by anyone > > > other > > > > than SurfaceManager? Or how can we feel confident that we're destroying > what > > > we > > > > create and not leaking surfaces? > > > > > > All Surfaces are owned by SurfaceManager. When it's destroyed, all Surfaces > > are > > > destroyed. There is no leak. > > > > What about during the lifetime of the browser? I didn't mean at shutdown (tho > we > > could notice them for sure at shutdown) > > We can check if all the remaining surfaces in the surface map are marked for > destruction. Ok thanks. That's better than nothing, though wouldn't catch anything until shutdown at which point debugging where the leak came from would be very tricky. https://codereview.chromium.org/2940183002/diff/370001/cc/surfaces/surface_ma... File cc/surfaces/surface_manager.cc (right): https://codereview.chromium.org/2940183002/diff/370001/cc/surfaces/surface_ma... cc/surfaces/surface_manager.cc:42: DCHECK_EQ(surfaces_to_destroy_.size(), surface_map_.size()); Can you leave a comment explaining what went wrong if this fires?
The CQ bit was checked by samans@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from fsamuel@google.com, fsamuel@chromium.org, danakj@chromium.org Link to the patchset: https://codereview.chromium.org/2940183002/#ps390001 (title: "Add comment")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by samans@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by samans@chromium.org to run a CQ dry run
The CQ bit was unchecked by samans@chromium.org
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by samans@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from danakj@chromium.org, fsamuel@chromium.org, fsamuel@google.com Link to the patchset: https://codereview.chromium.org/2940183002/#ps410001 (title: "Fix exo")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 410001, "attempt_start_ts": 1498682430959530,
"parent_rev": "e185b897078d1d665d50f8891a867e66b1625d69", "commit_rev":
"c4d8e3bfc8190090ae22ce114d73d7313135c29c"}
Message was sent while issue was closed.
Description was changed from ========== cc: Move ownership of surfaces to SurfaceManager SurfaceManager::CreateSurface gives up the ownership of the surface it creates to CompositorFrameSinkSupport by returning a unique_ptr. CompositorFrameSinkSupport later passes back the unique_ptr to SurfaceManager::DestroySurface when it wants to destroy it. Eventually we want SurfaceManager::DestroySurface to go away and give full authority to SurfaceManager to destroy surfaces when no one is referencing them without involving CompsositorFrameSinkSupport. To this end, this CL amends CreateSurface so it returns a raw pointer and keeps the unique_ptr in SurfaceManager. BUG=733434 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== cc: Move ownership of surfaces to SurfaceManager SurfaceManager::CreateSurface gives up the ownership of the surface it creates to CompositorFrameSinkSupport by returning a unique_ptr. CompositorFrameSinkSupport later passes back the unique_ptr to SurfaceManager::DestroySurface when it wants to destroy it. Eventually we want SurfaceManager::DestroySurface to go away and give full authority to SurfaceManager to destroy surfaces when no one is referencing them without involving CompsositorFrameSinkSupport. To this end, this CL amends CreateSurface so it returns a raw pointer and keeps the unique_ptr in SurfaceManager. BUG=733434 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel Review-Url: https://codereview.chromium.org/2940183002 Cr-Commit-Position: refs/heads/master@{#483165} Committed: https://chromium.googlesource.com/chromium/src/+/c4d8e3bfc8190090ae22ce114d73... ==========
Message was sent while issue was closed.
Committed patchset #20 (id:410001) as https://chromium.googlesource.com/chromium/src/+/c4d8e3bfc8190090ae22ce114d73... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
