Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2493223002:
Change exo::SurfaceFactoryOwner to exo::ExoCompositorFrameSink (Closed)
DescriptionMoved exo::SurfaceFactoryOwner to its own file and renamed it to exo::CompositorFrameSink.
CompositorFrameSink implements cc::mojom::MojoCompositorFrameSink.
CompositorFrameSink is no longer a friend class of exo::Surface.
Added exo::CompositorFrameSinkHolder class that implements cc::mojom::MojoCompositorFrameSinkClient.
BUG=659601
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
Review-Url: https://codereview.chromium.org/2493223002
Committed: https://crrev.com/d8f4f642d72fc5fdb428f5a90ca207c2143a6f79
Cr-Commit-Position: refs/heads/master@{#437780}
Patch Set 1 #Patch Set 2 : Renamed SurfaceFactoryOwner to ExoCompositorFrameSink and moved it to its own file #Patch Set 3 : Moved ExoCompositorFrameSink into its own file #
Total comments: 10
Patch Set 4 : rebase #
Total comments: 2
Patch Set 5 : Added ExoCompositorFrameSink::SubmitCompositorFrame(). #
Total comments: 15
Patch Set 6 : Nit changes based on comments #
Total comments: 20
Patch Set 7 : Added LocalFrameId as the second parameter of ExoCompositorFrameSink::SubmitCompositorFrame(); Addr… #Patch Set 8 : Calling SurfaceManager::UnregisterSurfaceFactoryClient() in exo::Surface::~Surface() to fix test cr… #
Total comments: 10
Patch Set 9 : Reordered parameters of SubmitCompositorFrame() #Patch Set 10 : Added CompositorFrameSinkHolder class and addressed comments" #
Total comments: 23
Patch Set 11 : CompositorFrameSinkHolder implements MojoCompositorFrameSinkClient #Patch Set 12 : Added base::MessageLoop to ExoTestBase #Patch Set 13 : Add mojo initialization in exo/test/run_all_unittests.cc and updated include rules accordingly" #Patch Set 14 : Updated DEPS rules to address errors in trybots' analyze step #
Total comments: 2
Patch Set 15 : Modified DEPS rules; Added WillDrawSurface() to MojoCompositorFrameSinkClient #Patch Set 16 : Fixed include rules #Patch Set 17 : Addressed some comments and rebased #
Total comments: 32
Patch Set 18 : Changed type of CompositorFrameSinkHolder::surface_ to WeakPtr<Surface> #Patch Set 19 : Addressed comments #
Total comments: 4
Patch Set 20 : Addressed comments #Patch Set 21 : Added an ExternalBeginFrameSource to CompositorFrameSinkHolder #
Total comments: 8
Patch Set 22 : Added RunAllPendingInMessageLoop() to the end of SurfaceTest.Attach #Patch Set 23 : Surface::~Surface() calls UnregisterSurfaceFactoryClient() #Patch Set 24 : Revert "Surface::~Surface() calls UnregisterSurfaceFactoryClient()" #Patch Set 25 : NOT FOR COMMIT: Added MessageLoopRunner and printfs for debugging #Patch Set 26 : NOT FOR COMMIT: Fixed GamepadTest.OnStageChange with TestReleaseCallbackManager. #Patch Set 27 : Added RunAllPendingInMessageLoop() to AshTestHelper::TearDown() to ensure exo::CompositorFrameSinkH… #Patch Set 28 : Thoroughly removed TestCallbackManager #Patch Set 29 : Rebase #Patch Set 30 : Added EvictFrame() to MojoCompositorFrameSink and its implementations #Patch Set 31 : Added EvictFrame() to OffscreenCanvasCompositorFrameSink to comply with the interface change #Patch Set 32 : Modified ~ExoCompositorFrameSink() so it looks like ~GpuCompositorFrameSink() #Patch Set 33 : Removed another printf #Patch Set 34 : Undid a change on an if condition in exo::Surface::CommitSurfaceHierarchy() #
Total comments: 24
Patch Set 35 : Renamed ExoCompositorFrameSink to CompositorFrameSink; removed unnecessary includes #Patch Set 36 : Renamed files #Patch Set 37 : Addressed comments #Patch Set 38 : Changed exo::CompositorFrameSinkHolder::compositor_frame_sink_ from std::unique_ptr<CompositorFrame… #Patch Set 39 : NOT FOR COMMIT: Failing SurfaceTest.OverlayCandidate with GL ERROR: GL_INVALID_ENUM : glBindTexture… #Patch Set 40 : Code clean up #
Total comments: 12
Patch Set 41 : Added MojoCompositorFrameSink::Require() and MojoCompositorFrameSink::Satisfy() #
Total comments: 4
Patch Set 42 : Moved CompositorFrameSink implementation details in to CompositorFrameSinkSupport; addressed commen… #
Total comments: 2
Patch Set 43 : exo::Surface uses CompositorFrameSink accessor from CompositorFrameSinkHolder #
Total comments: 84
Patch Set 44 : Addressed comments: #
Total comments: 4
Patch Set 45 : Fixed a typo in mojo_compositor_frame_sink.mojom #
Total comments: 2
Patch Set 46 : Call UpdateNeedsBeginFrame() at the end of CompositorFrameSinkHolder::ActivateFrameCallbacks() #
Total comments: 1
Patch Set 47 : Addressed comments; removed unnecessary includes #Patch Set 48 : Removed unnecessary includes #Patch Set 49 : Added dependency //mojo/edk/embedder:headers to exo_unittests #
Total comments: 18
Patch Set 50 : Replaced SatisfyCallback() and RequireCallback() with CompositorFrameSinkHolder::Satisfy() and Comp… #
Total comments: 4
Patch Set 51 : Removed unused MojoCompositorFrameSinkPtr #Patch Set 52 : Moving release callbacks to a local variable before running in CompositorFrameSinkHolder #Patch Set 53 : Added list_.reset() at the end of DesktopMediaListAshTest.Screen to stop refreshing #Patch Set 54 : Added comments to explain the reset() #
Total comments: 2
Patch Set 55 : Override DesktopMediaListAshTest::TearDown() to reset list_ #Messages
Total messages: 149 (67 generated)
The CQ bit was checked by staraz@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-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
fsamuel@chromium.org changed reviewers: + fsamuel@chromium.org
fsamuel@chromium.org changed reviewers: + fsamuel@chromium.org
https://codereview.chromium.org/2493223002/diff/40001/components/exo/exo_comp... File components/exo/exo_compositor_frame_sink.cc (right): https://codereview.chromium.org/2493223002/diff/40001/components/exo/exo_comp... components/exo/exo_compositor_frame_sink.cc:17: aura::Env::GetInstance()->context_factory()->AllocateFrameSinkId()) { Don't allocate the FrameSinkId in here. Instead, the CompositorFrameSink should take in a FrameSinkId.
https://codereview.chromium.org/2493223002/diff/40001/components/exo/surface.cc File components/exo/surface.cc (right): https://codereview.chromium.org/2493223002/diff/40001/components/exo/surface.... components/exo/surface.cc:180: surface_manager_->RegisterFrameSinkId( Move this into ExoCompositorFrameSink's constructor. https://codereview.chromium.org/2493223002/diff/40001/components/exo/surface.... components/exo/surface.cc:183: compositor_frame_sink_->frame_sink_id_, compositor_frame_sink_.get()); Move this into ExoCompositorFrameSink's constructor. https://codereview.chromium.org/2493223002/diff/40001/components/exo/surface.... components/exo/surface.cc:184: compositor_frame_sink_->Init(surface_manager_); Get rid of init. https://codereview.chromium.org/2493223002/diff/40001/components/exo/surface.... components/exo/surface.cc:207: surface_manager_->UnregisterSurfaceFactoryClient(surface_id_.frame_sink_id()); This should be in ExoCompositorFrameSink's destructor.
The CQ bit was checked by staraz@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/2493223002/diff/40001/components/exo/exo_comp... File components/exo/exo_compositor_frame_sink.cc (right): https://codereview.chromium.org/2493223002/diff/40001/components/exo/exo_comp... components/exo/exo_compositor_frame_sink.cc:17: aura::Env::GetInstance()->context_factory()->AllocateFrameSinkId()) { On 2016/11/14 23:29:41, Fady Samuel wrote: > Don't allocate the FrameSinkId in here. Instead, the CompositorFrameSink should > take in a FrameSinkId. Done.
https://codereview.chromium.org/2493223002/diff/60001/components/exo/exo_comp... File components/exo/exo_compositor_frame_sink.cc (right): https://codereview.chromium.org/2493223002/diff/60001/components/exo/exo_comp... components/exo/exo_compositor_frame_sink.cc:43: return std::move(cc::SurfaceId(frame_sink_id_, local_frame_id)); This isn't safe. You're returning a reference to a temporary. Could you please avoid UnregisterSurface and CommitToNewSurface and instead implement a SubmitCompositorFrame? Look at what GpuCompositorFrameSink is doing and try to reproduce that.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_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 staraz@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/2493223002/diff/40001/components/exo/surface.cc File components/exo/surface.cc (right): https://codereview.chromium.org/2493223002/diff/40001/components/exo/surface.... components/exo/surface.cc:180: surface_manager_->RegisterFrameSinkId( On 2016/11/15 15:42:40, Fady Samuel wrote: > Move this into ExoCompositorFrameSink's constructor. Done. https://codereview.chromium.org/2493223002/diff/40001/components/exo/surface.... components/exo/surface.cc:183: compositor_frame_sink_->frame_sink_id_, compositor_frame_sink_.get()); On 2016/11/15 15:42:40, Fady Samuel wrote: > Move this into ExoCompositorFrameSink's constructor. Done. https://codereview.chromium.org/2493223002/diff/40001/components/exo/surface.... components/exo/surface.cc:184: compositor_frame_sink_->Init(surface_manager_); On 2016/11/15 15:42:40, Fady Samuel wrote: > Get rid of init. Done. https://codereview.chromium.org/2493223002/diff/40001/components/exo/surface.... components/exo/surface.cc:207: surface_manager_->UnregisterSurfaceFactoryClient(surface_id_.frame_sink_id()); On 2016/11/15 15:42:40, Fady Samuel wrote: > This should be in ExoCompositorFrameSink's destructor. Done. https://codereview.chromium.org/2493223002/diff/60001/components/exo/exo_comp... File components/exo/exo_compositor_frame_sink.cc (right): https://codereview.chromium.org/2493223002/diff/60001/components/exo/exo_comp... components/exo/exo_compositor_frame_sink.cc:43: return std::move(cc::SurfaceId(frame_sink_id_, local_frame_id)); On 2016/11/15 16:01:32, Fady Samuel wrote: > This isn't safe. You're returning a reference to a temporary. > > Could you please avoid UnregisterSurface and CommitToNewSurface and instead > implement a SubmitCompositorFrame? Look at what GpuCompositorFrameSink is doing > and try to reproduce that. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
+reveman@: release_callbacks are confusing. They seem to differ from the way resources are returned elsewhere in Chrome. Could you please explain what's going on there? Thanks! https://codereview.chromium.org/2493223002/diff/80001/components/exo/exo_comp... File components/exo/exo_compositor_frame_sink.cc (right): https://codereview.chromium.org/2493223002/diff/80001/components/exo/exo_comp... components/exo/exo_compositor_frame_sink.cc:56: if (!local_frame_id_.is_valid() /* || needs_commit_to_new_surface_*/) { Remove commented out code. https://codereview.chromium.org/2493223002/diff/80001/components/exo/exo_comp... components/exo/exo_compositor_frame_sink.cc:60: // UpdateSurface(true); // surface_factory.SubmitCompositorFrame happens here Remove commented out code https://codereview.chromium.org/2493223002/diff/80001/components/exo/exo_comp... File components/exo/exo_compositor_frame_sink.h (right): https://codereview.chromium.org/2493223002/diff/80001/components/exo/exo_comp... components/exo/exo_compositor_frame_sink.h:60: const cc::FrameSinkId& frame_sink_id() { return frame_sink_id_; } nit: do you need this? https://codereview.chromium.org/2493223002/diff/80001/components/exo/exo_comp... components/exo/exo_compositor_frame_sink.h:61: cc::SurfaceManager* surface_manager() { return surface_manager_; } nit: do you need this? https://codereview.chromium.org/2493223002/diff/80001/components/exo/exo_comp... components/exo/exo_compositor_frame_sink.h:74: release_callbacks_; What is this for? https://codereview.chromium.org/2493223002/diff/80001/components/exo/exo_comp... components/exo/exo_compositor_frame_sink.h:75: cc::FrameSinkId frame_sink_id_; Mark as const. https://codereview.chromium.org/2493223002/diff/80001/components/exo/surface.cc File components/exo/surface.cc (right): https://codereview.chromium.org/2493223002/diff/80001/components/exo/surface.... components/exo/surface.cc:694: compositor_frame_sink_->release_callbacks_[resource.id] = std::make_pair( These release_callbacks are weird. I would investigate more what they're doing. Please speak to reveman@
fsamuel@chromium.org changed reviewers: + reveman@chromium.org
+reveman@ for real this time.
It's clear from the description what the goals are but it's not clear to me why we want to achieve those goals. Can you explain a bit more? https://codereview.chromium.org/2493223002/diff/80001/components/exo/exo_comp... File components/exo/exo_compositor_frame_sink.h (right): https://codereview.chromium.org/2493223002/diff/80001/components/exo/exo_comp... components/exo/exo_compositor_frame_sink.h:1: // Copyright 2016 The Chromium Authors. All rights reserved. Why does this need to be in a new file when it's only used by exo::Surface? If it contains enough logic to justify a new file then I think it should also be unit tested by adding a compositor_frame_sink_unittest.cc file. https://codereview.chromium.org/2493223002/diff/80001/components/exo/exo_comp... components/exo/exo_compositor_frame_sink.h:39: class ExoCompositorFrameSink : public base::RefCounted<ExoCompositorFrameSink>, Exo is typically not used as a prefix inside the exo:: namespace. Can this be CompositorFrameSink or SurfaceCompositorFrameSink?
On 2016/11/16 03:48:38, reveman wrote: > It's clear from the description what the goals are but it's not clear to me why > we want to achieve those goals. Can you explain a bit more? > > https://codereview.chromium.org/2493223002/diff/80001/components/exo/exo_comp... > File components/exo/exo_compositor_frame_sink.h (right): > > https://codereview.chromium.org/2493223002/diff/80001/components/exo/exo_comp... > components/exo/exo_compositor_frame_sink.h:1: // Copyright 2016 The Chromium > Authors. All rights reserved. > Why does this need to be in a new file when it's only used by exo::Surface? If > it contains enough logic to justify a new file then I think it should also be > unit tested by adding a compositor_frame_sink_unittest.cc file. > > https://codereview.chromium.org/2493223002/diff/80001/components/exo/exo_comp... > components/exo/exo_compositor_frame_sink.h:39: class ExoCompositorFrameSink : > public base::RefCounted<ExoCompositorFrameSink>, > Exo is typically not used as a prefix inside the exo:: namespace. Can this be > CompositorFrameSink or SurfaceCompositorFrameSink? I added more details in the bug: Background for exo folks: We would like to move the display compositor to the gpu process for Poliwog and then later Salamander to support Vulkan, centralized scheduling, centralized animation, scrolling, etc. Thus, SurfaceFactory, SurfaceManager and related classes are all moving to the Gpu process. Exo and other clients cannot access SurfaceFactory directly. They will talk to the display compositor via what's called today "MojoCompositorFrameSink". GpuCompositorFrameSink is a good example of this. As an intermediate step for exo, we should move access to SurfaceFactory to an "ExoCompositorFrameSink" or some such name, put it behind a mojo interface, so that exo does not directly talk to SurfaceFactory, then unify it with other "CompositorFrameSink" implementations as they all converge.
https://codereview.chromium.org/2493223002/diff/80001/components/exo/exo_comp... File components/exo/exo_compositor_frame_sink.h (right): https://codereview.chromium.org/2493223002/diff/80001/components/exo/exo_comp... components/exo/exo_compositor_frame_sink.h:60: const cc::FrameSinkId& frame_sink_id() { return frame_sink_id_; } On 2016/11/15 23:19:46, Fady Samuel wrote: > nit: do you need this? Done. https://codereview.chromium.org/2493223002/diff/80001/components/exo/exo_comp... components/exo/exo_compositor_frame_sink.h:61: cc::SurfaceManager* surface_manager() { return surface_manager_; } On 2016/11/15 23:19:46, Fady Samuel wrote: > nit: do you need this? Yes. In CommitSurfaceHierarchy(), the SurfaceManager is needed for window_->layer()->SetShowSurface() (https://cs.chromium.org/chromium/src/components/exo/surface.cc?sq=package:chr...) https://codereview.chromium.org/2493223002/diff/80001/components/exo/exo_comp... components/exo/exo_compositor_frame_sink.h:74: release_callbacks_; On 2016/11/15 23:19:46, Fady Samuel wrote: > What is this for? I don't fully understand this part, but it seems the release_callbacks_ contains the callbacks needed to release resources. It is updated in exo::Surface::UpdateResource(). The callbacks keep ExoCompositorFrameSink alive. Inside ExoCompositorFrameSink::ReturnResources(), all callbacks are called and erased. Maybe I could move it into exo::Surface?
https://codereview.chromium.org/2493223002/diff/100001/components/exo/exo_com... File components/exo/exo_compositor_frame_sink.h (right): https://codereview.chromium.org/2493223002/diff/100001/components/exo/exo_com... components/exo/exo_compositor_frame_sink.h:46: // cc::SurfaceFactoryClient: Make all these overrides private to indicate that these are implementation details and not part of the public API. https://codereview.chromium.org/2493223002/diff/100001/components/exo/exo_com... components/exo/exo_compositor_frame_sink.h:55: // cc::BeginFrameObserver: Make all these overrides private to indicate that these are implementation details and not part of the public API. https://codereview.chromium.org/2493223002/diff/100001/components/exo/exo_com... components/exo/exo_compositor_frame_sink.h:60: cc::SurfaceManager* surface_manager() { return surface_manager_; } It would be nice to not read back things from ExoCompositorFrameSink. The thing that created ExoCompositorFrameSink knows about SurfaceManager right, so we can use it directly for now. https://codereview.chromium.org/2493223002/diff/100001/components/exo/exo_com... components/exo/exo_compositor_frame_sink.h:64: friend class Surface; Remove this friend. https://codereview.chromium.org/2493223002/diff/100001/components/exo/exo_com... components/exo/exo_compositor_frame_sink.h:73: release_callbacks_; I really don't understand the purpose of this. https://codereview.chromium.org/2493223002/diff/100001/components/exo/exo_com... components/exo/exo_compositor_frame_sink.h:91: }; DISALLOW_COPY_AND_ASSIGN(ExoCompositorFrameSink);
fsamuel@chromium.org changed reviewers: + jbauman@chromium.org
+jbauman@: I'm a little confused about how TransferrableResources are managed in exo. They seem different from other Chrome clients. How can we unify this code? I'd like CompositorFrameSinks to look the same everywhere. https://codereview.chromium.org/2493223002/diff/100001/components/exo/surface.cc File components/exo/surface.cc (right): https://codereview.chromium.org/2493223002/diff/100001/components/exo/surface... components/exo/surface.cc:212: return surface_id_; I think what we might want to do here is *temporarily* make the current "local_frame_id()" accessible from ExoCompositorFrameSink and combine that with the fixed |frame_sink_id_| stored here. In the longer term, local_frame_id_s will be allocated by the client anyway so we won't have this problem. https://codereview.chromium.org/2493223002/diff/100001/components/exo/surface... components/exo/surface.cc:589: compositor_frame_sink_->frame_sink_id_); The exo::Surface should have a fixed frame_sink_id allocated at creation. You don't need to read it from the CompositorFrameSink. https://codereview.chromium.org/2493223002/diff/100001/components/exo/surface... components/exo/surface.cc:595: compositor_frame_sink_->frame_sink_id_); The exo::Surface should have a fixed frame_sink_id allocated at creation. You don't need to read it from the CompositorFrameSink. https://codereview.chromium.org/2493223002/diff/100001/components/exo/surface... components/exo/surface.cc:684: cc::TransferableResource resource; I *think* what should happen is these resources should be part of the resource_list in the CompositorFrame and then when SurfaceAggregator is done when them then it should return the resources. +jbauman@ could better explain.
https://codereview.chromium.org/2493223002/diff/100001/components/exo/exo_com... File components/exo/exo_compositor_frame_sink.h (right): https://codereview.chromium.org/2493223002/diff/100001/components/exo/exo_com... components/exo/exo_compositor_frame_sink.h:46: // cc::SurfaceFactoryClient: On 2016/11/16 at 15:02:00, Fady Samuel wrote: > Make all these overrides private to indicate that these are implementation details and not part of the public API. I'm not a fan of changing access to functions when overriding them as anyone with a pointer to the base class will still be able to access it and that's confusing, I think. https://codereview.chromium.org/2493223002/diff/100001/components/exo/exo_com... components/exo/exo_compositor_frame_sink.h:73: release_callbacks_; On 2016/11/16 at 15:02:00, Fady Samuel wrote: > I really don't understand the purpose of this. jbauman can the explain the details of this map better than me but the general difference between exo and other display compositor clients in chrome is that exo clients require additional synchronization requirements as they have direct access to the gpu using a different channel than the gpu process in chrome. We need to use real gpu fences to determine when a buffer can be reused by a client and these release callbacks are related to that.
https://codereview.chromium.org/2493223002/diff/100001/components/exo/exo_com... File components/exo/exo_compositor_frame_sink.h (right): https://codereview.chromium.org/2493223002/diff/100001/components/exo/exo_com... components/exo/exo_compositor_frame_sink.h:46: // cc::SurfaceFactoryClient: On 2016/11/16 15:02:00, Fady Samuel wrote: > Make all these overrides private to indicate that these are implementation > details and not part of the public API. Done. https://codereview.chromium.org/2493223002/diff/100001/components/exo/exo_com... components/exo/exo_compositor_frame_sink.h:55: // cc::BeginFrameObserver: On 2016/11/16 15:02:00, Fady Samuel wrote: > Make all these overrides private to indicate that these are implementation > details and not part of the public API. Done. https://codereview.chromium.org/2493223002/diff/100001/components/exo/exo_com... components/exo/exo_compositor_frame_sink.h:60: cc::SurfaceManager* surface_manager() { return surface_manager_; } On 2016/11/16 15:02:00, Fady Samuel wrote: > It would be nice to not read back things from ExoCompositorFrameSink. The thing > that created ExoCompositorFrameSink knows about SurfaceManager right, so we can > use it directly for now. Done. https://codereview.chromium.org/2493223002/diff/100001/components/exo/exo_com... components/exo/exo_compositor_frame_sink.h:64: friend class Surface; On 2016/11/16 15:02:00, Fady Samuel wrote: > Remove this friend. Removing this friend prevents exo::Surface from accessing ExoCompositorFrameSink::release_callbacks_. I added ExoCompositorFrameSink::release_callbacks() as a temporary workaround. https://codereview.chromium.org/2493223002/diff/100001/components/exo/exo_com... components/exo/exo_compositor_frame_sink.h:91: }; On 2016/11/16 15:02:00, Fady Samuel wrote: > DISALLOW_COPY_AND_ASSIGN(ExoCompositorFrameSink); Done. https://codereview.chromium.org/2493223002/diff/100001/components/exo/surface.cc File components/exo/surface.cc (right): https://codereview.chromium.org/2493223002/diff/100001/components/exo/surface... components/exo/surface.cc:212: return surface_id_; On 2016/11/16 15:10:58, Fady Samuel wrote: > I think what we might want to do here is *temporarily* make the current > "local_frame_id()" accessible from ExoCompositorFrameSink and combine that with > the fixed |frame_sink_id_| stored here. > > In the longer term, local_frame_id_s will be allocated by the client anyway so > we won't have this problem. Done. https://codereview.chromium.org/2493223002/diff/100001/components/exo/surface... components/exo/surface.cc:589: compositor_frame_sink_->frame_sink_id_); On 2016/11/16 15:10:58, Fady Samuel wrote: > The exo::Surface should have a fixed frame_sink_id allocated at creation. You > don't need to read it from the CompositorFrameSink. Done. https://codereview.chromium.org/2493223002/diff/100001/components/exo/surface... components/exo/surface.cc:595: compositor_frame_sink_->frame_sink_id_); On 2016/11/16 15:10:58, Fady Samuel wrote: > The exo::Surface should have a fixed frame_sink_id allocated at creation. You > don't need to read it from the CompositorFrameSink. Done.
The CQ bit was checked by staraz@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_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/2493223002/diff/140001/components/exo/exo_com... File components/exo/exo_compositor_frame_sink.cc (right): https://codereview.chromium.org/2493223002/diff/140001/components/exo/exo_com... components/exo/exo_compositor_frame_sink.cc:30: for (auto& resource : resources) { So I see the problem here. This should actually happen in the client. Move this code back out to Surface. Then you won't need to store release_callbacks in ExoCompositorFrameSink. Instead have exo::Surface implement MojoCompositorFrameSinkClient, and give ExoCompositorFrameSink an InterfacePtr to it. Then call out to it ReclaimResources(ReturnedResourceArray resources); https://codereview.chromium.org/2493223002/diff/140001/components/exo/exo_com... File components/exo/exo_compositor_frame_sink.h (right): https://codereview.chromium.org/2493223002/diff/140001/components/exo/exo_com... components/exo/exo_compositor_frame_sink.h:48: const cc::LocalFrameId& local_frame_id); nit: local_frame_id first. https://codereview.chromium.org/2493223002/diff/140001/components/exo/exo_com... components/exo/exo_compositor_frame_sink.h:51: std::pair<scoped_refptr<ExoCompositorFrameSink>, TODO to remove this. Also, this is a ginormous type. using ResourceReleaseCallbacks = ....; Also, why does an ExoCompositorFrameSink refer to ExoCompositorFrameSinks? https://codereview.chromium.org/2493223002/diff/140001/components/exo/surface.cc File components/exo/surface.cc (right): https://codereview.chromium.org/2493223002/diff/140001/components/exo/surface... components/exo/surface.cc:205: surface_manager_->UnregisterSurfaceFactoryClient(frame_sink_id_); Moe this inside ExoCompositorFrameSink's destructor.
https://codereview.chromium.org/2493223002/diff/140001/components/exo/exo_com... File components/exo/exo_compositor_frame_sink.cc (right): https://codereview.chromium.org/2493223002/diff/140001/components/exo/exo_com... components/exo/exo_compositor_frame_sink.cc:30: for (auto& resource : resources) { On 2016/11/16 21:29:40, Fady Samuel wrote: > So I see the problem here. This should actually happen in the client. Move this > code back out to Surface. Then you won't need to store release_callbacks in > ExoCompositorFrameSink. Instead have exo::Surface implement > MojoCompositorFrameSinkClient, and give ExoCompositorFrameSink an InterfacePtr > to it. Then call out to it ReclaimResources(ReturnedResourceArray resources); The release callbacks can outlive the exo::Surface, which is why this class exists - to hold on to a reference to the SurfaceFactory until all the release callbacks have been called.
https://codereview.chromium.org/2493223002/diff/140001/components/exo/surface.cc File components/exo/surface.cc (right): https://codereview.chromium.org/2493223002/diff/140001/components/exo/surface... components/exo/surface.cc:174: compositor_frame_sink_( So it sounds like we want a CompositorFrameSinkHolder that owns the CompositorFrameSink and holds the release_callbacks.
https://codereview.chromium.org/2493223002/diff/140001/components/exo/exo_com... File components/exo/exo_compositor_frame_sink.h (right): https://codereview.chromium.org/2493223002/diff/140001/components/exo/exo_com... components/exo/exo_compositor_frame_sink.h:48: const cc::LocalFrameId& local_frame_id); On 2016/11/16 21:29:40, Fady Samuel wrote: > nit: local_frame_id first. Done. https://codereview.chromium.org/2493223002/diff/140001/components/exo/surface.cc File components/exo/surface.cc (right): https://codereview.chromium.org/2493223002/diff/140001/components/exo/surface... components/exo/surface.cc:174: compositor_frame_sink_( On 2016/11/16 23:10:52, Fady Samuel wrote: > So it sounds like we want a CompositorFrameSinkHolder that owns the > CompositorFrameSink and holds the release_callbacks. Done. https://codereview.chromium.org/2493223002/diff/140001/components/exo/surface... components/exo/surface.cc:205: surface_manager_->UnregisterSurfaceFactoryClient(frame_sink_id_); On 2016/11/16 21:29:41, Fady Samuel wrote: > Moe this inside ExoCompositorFrameSink's destructor. Done.
https://codereview.chromium.org/2493223002/diff/180001/components/exo/composi... File components/exo/compositor_frame_sink_holder.h (right): https://codereview.chromium.org/2493223002/diff/180001/components/exo/composi... components/exo/compositor_frame_sink_holder.h:23: void DidReceiveCompositorFrameAck(); Add implementations for these? TODOs are fine. https://codereview.chromium.org/2493223002/diff/180001/components/exo/composi... components/exo/compositor_frame_sink_holder.h:37: }; nit: DISALLOW_COPY_AND_ASSIGN(CompositorFrameSinkHolder); https://codereview.chromium.org/2493223002/diff/180001/components/exo/exo_com... File components/exo/exo_compositor_frame_sink.cc (right): https://codereview.chromium.org/2493223002/diff/180001/components/exo/exo_com... components/exo/exo_compositor_frame_sink.cc:33: // TODO(staraz): !ack_pending_count_ Fix this now? This should be a copy-and-paste from GpuCompositorFrameSink. https://codereview.chromium.org/2493223002/diff/180001/components/exo/exo_com... components/exo/exo_compositor_frame_sink.cc:93: if (last_local_frame_id_.is_valid()) do this before UnregisterSurfaceFactoryClient. https://codereview.chromium.org/2493223002/diff/180001/components/exo/exo_com... File components/exo/exo_compositor_frame_sink.h (right): https://codereview.chromium.org/2493223002/diff/180001/components/exo/exo_com... components/exo/exo_compositor_frame_sink.h:43: class ExoCompositorFrameSink : public base::RefCounted<ExoCompositorFrameSink>, Does this need to be refcounted? https://codereview.chromium.org/2493223002/diff/180001/components/exo/surface.cc File components/exo/surface.cc (right): https://codereview.chromium.org/2493223002/diff/180001/components/exo/surface... components/exo/surface.cc:174: compositor_frame_sink_holder_(new CompositorFrameSinkHolder()), Make this implement MojoCompositorFrameSinkClient. https://codereview.chromium.org/2493223002/diff/180001/components/exo/surface... components/exo/surface.cc:178: compositor_frame_sink_holder_.get())) { Take in a MojoCompostiorFrameSinkClientPtr here. CompositorFrameSinkHolder has a mojo::Binding<cc::mojom::MojoCompositorFrameSinkClient>. https://codereview.chromium.org/2493223002/diff/180001/components/exo/surface... components/exo/surface.cc:458: compositor_frame_sink_holder_->release_callbacks_.count( CompositorFrameSinkHolder::HasReleaseCallbacks()? https://codereview.chromium.org/2493223002/diff/180001/components/exo/surface... components/exo/surface.cc:697: compositor_frame_sink_holder_->release_callbacks_[resource.id] = This is really icky. How about CompositorFrameSinkHolder::AddResourceReleaseCallback
https://codereview.chromium.org/2493223002/diff/180001/components/exo/exo_com... File components/exo/exo_compositor_frame_sink.h (right): https://codereview.chromium.org/2493223002/diff/180001/components/exo/exo_com... components/exo/exo_compositor_frame_sink.h:80: scoped_refptr<CompositorFrameSinkHolder> client_; This is backwards. The CompositorFrameSinkHolder needs to hold on to the ExoCompositorFrameSink so that the SurfaceFactory isn't destroyed until the release callbacks have run.
https://codereview.chromium.org/2493223002/diff/180001/components/exo/surface.h File components/exo/surface.h (left): https://codereview.chromium.org/2493223002/diff/180001/components/exo/surface... components/exo/surface.h:353: cc::SurfaceManager* surface_manager_; Do we still need this? https://codereview.chromium.org/2493223002/diff/180001/components/exo/surface.h File components/exo/surface.h (right): https://codereview.chromium.org/2493223002/diff/180001/components/exo/surface... components/exo/surface.h:330: scoped_refptr<ExoCompositorFrameSink> compositor_frame_sink_; exo::Surface shouldn't hold a reference to ExoCompositorFrameSink directly. It should be managed by CompositorFrameSinkHolder.
The CQ bit was checked by staraz@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: chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...) chromeos_x86-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...) linux_chromium_chromeos_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 staraz@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: chromeos_x86-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...) linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
This is looking a lot better. https://codereview.chromium.org/2493223002/diff/260001/components/exo/surface.cc File components/exo/surface.cc (right): https://codereview.chromium.org/2493223002/diff/260001/components/exo/surface... components/exo/surface.cc:701: compositor_frame_sink_holder_->release_callbacks_[resource.id] = AddResourceReleaseCallback(ResourceId, callback)?
Description was changed from ========== Change exo::SurfaceFactoryOwner to exo::ExoCompositorFrameSink Eventually, this CL should achieve the following 3 goals: 1. Moved exo::SurfaceFactoryOwner to its own file and renamed it to exo::ExoCompositorFrameSink. 2. ExoCompositorFrameSink implements cc::mojom::MojoCompositorFrameSink. 3. ExoCompositorFrameSink doesn't know about exo::Surface. BUG=659601 ========== to ========== Change exo::SurfaceFactoryOwner to exo::ExoCompositorFrameSink Eventually, this CL should achieve the following 3 goals: 1. Moved exo::SurfaceFactoryOwner to its own file and renamed it to exo::ExoCompositorFrameSink. 2. ExoCompositorFrameSink implements cc::mojom::MojoCompositorFrameSink. 3. ExoCompositorFrameSink doesn't know about exo::Surface. BUG=659601 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
https://codereview.chromium.org/2493223002/diff/80001/components/exo/exo_comp... File components/exo/exo_compositor_frame_sink.h (right): https://codereview.chromium.org/2493223002/diff/80001/components/exo/exo_comp... components/exo/exo_compositor_frame_sink.h:75: cc::FrameSinkId frame_sink_id_; On 2016/11/15 23:19:46, Fady Samuel wrote: > Mark as const. Done. https://codereview.chromium.org/2493223002/diff/140001/components/exo/exo_com... File components/exo/exo_compositor_frame_sink.h (right): https://codereview.chromium.org/2493223002/diff/140001/components/exo/exo_com... components/exo/exo_compositor_frame_sink.h:51: std::pair<scoped_refptr<ExoCompositorFrameSink>, On 2016/11/16 21:29:40, Fady Samuel wrote: > TODO to remove this. Also, this is a ginormous type. > > using ResourceReleaseCallbacks = ....; > > Also, why does an ExoCompositorFrameSink refer to ExoCompositorFrameSinks? Done. https://codereview.chromium.org/2493223002/diff/180001/components/exo/composi... File components/exo/compositor_frame_sink_holder.h (right): https://codereview.chromium.org/2493223002/diff/180001/components/exo/composi... components/exo/compositor_frame_sink_holder.h:23: void DidReceiveCompositorFrameAck(); On 2016/11/17 21:42:54, Fady Samuel wrote: > Add implementations for these? TODOs are fine. Done. https://codereview.chromium.org/2493223002/diff/180001/components/exo/composi... components/exo/compositor_frame_sink_holder.h:37: }; On 2016/11/17 21:42:54, Fady Samuel wrote: > nit: DISALLOW_COPY_AND_ASSIGN(CompositorFrameSinkHolder); Done. https://codereview.chromium.org/2493223002/diff/180001/components/exo/exo_com... File components/exo/exo_compositor_frame_sink.cc (right): https://codereview.chromium.org/2493223002/diff/180001/components/exo/exo_com... components/exo/exo_compositor_frame_sink.cc:33: // TODO(staraz): !ack_pending_count_ On 2016/11/17 21:42:54, Fady Samuel wrote: > Fix this now? This should be a copy-and-paste from GpuCompositorFrameSink. Done. https://codereview.chromium.org/2493223002/diff/180001/components/exo/exo_com... components/exo/exo_compositor_frame_sink.cc:93: if (last_local_frame_id_.is_valid()) On 2016/11/17 21:42:54, Fady Samuel wrote: > do this before UnregisterSurfaceFactoryClient. Done. https://codereview.chromium.org/2493223002/diff/180001/components/exo/exo_com... File components/exo/exo_compositor_frame_sink.h (right): https://codereview.chromium.org/2493223002/diff/180001/components/exo/exo_com... components/exo/exo_compositor_frame_sink.h:80: scoped_refptr<CompositorFrameSinkHolder> client_; On 2016/11/17 22:47:37, jbauman wrote: > This is backwards. The CompositorFrameSinkHolder needs to hold on to the > ExoCompositorFrameSink so that the SurfaceFactory isn't destroyed until the > release callbacks have run. Done. https://codereview.chromium.org/2493223002/diff/180001/components/exo/surface.cc File components/exo/surface.cc (right): https://codereview.chromium.org/2493223002/diff/180001/components/exo/surface... components/exo/surface.cc:174: compositor_frame_sink_holder_(new CompositorFrameSinkHolder()), On 2016/11/17 21:42:54, Fady Samuel wrote: > Make this implement MojoCompositorFrameSinkClient. Done. https://codereview.chromium.org/2493223002/diff/180001/components/exo/surface... components/exo/surface.cc:178: compositor_frame_sink_holder_.get())) { On 2016/11/17 21:42:54, Fady Samuel wrote: > Take in a MojoCompostiorFrameSinkClientPtr here. > > CompositorFrameSinkHolder has a > mojo::Binding<cc::mojom::MojoCompositorFrameSinkClient>. Done. https://codereview.chromium.org/2493223002/diff/180001/components/exo/surface... components/exo/surface.cc:458: compositor_frame_sink_holder_->release_callbacks_.count( On 2016/11/17 21:42:54, Fady Samuel wrote: > CompositorFrameSinkHolder::HasReleaseCallbacks()? Done.
https://codereview.chromium.org/2493223002/diff/320001/components/exo/composi... File components/exo/compositor_frame_sink_holder.cc (right): https://codereview.chromium.org/2493223002/diff/320001/components/exo/composi... components/exo/compositor_frame_sink_holder.cc:29: release_callbacks_[id] = std::make_pair(this, std::move(callback)); This is so icky. Holding a reference to self is super icky. Maybe exo::Surface can hold onto a callback that binds CompositorFrameSinkHolder instead. That's still icky, but slightly less so in my opinion. https://codereview.chromium.org/2493223002/diff/320001/components/exo/composi... components/exo/compositor_frame_sink_holder.cc:47: void CompositorFrameSinkHolder::OnBeginFrame(const cc::BeginFrameArgs& args) {} I think you'll likely want to implement this. https://codereview.chromium.org/2493223002/diff/320001/components/exo/composi... File components/exo/compositor_frame_sink_holder.h (right): https://codereview.chromium.org/2493223002/diff/320001/components/exo/composi... components/exo/compositor_frame_sink_holder.h:27: Surface* surface, base::WeakPtr<Surface> surface https://codereview.chromium.org/2493223002/diff/320001/components/exo/exo_com... File components/exo/exo_compositor_frame_sink.cc (right): https://codereview.chromium.org/2493223002/diff/320001/components/exo/exo_com... components/exo/exo_compositor_frame_sink.cc:8: #include "components/exo/compositor_frame_sink_holder.h" You don't need this. https://codereview.chromium.org/2493223002/diff/320001/components/exo/exo_com... components/exo/exo_compositor_frame_sink.cc:9: #include "ui/aura/env.h" You don't need this. https://codereview.chromium.org/2493223002/diff/320001/components/exo/exo_com... File components/exo/exo_compositor_frame_sink.h (right): https://codereview.chromium.org/2493223002/diff/320001/components/exo/exo_com... components/exo/exo_compositor_frame_sink.h:8: #include <list> I suspect you don't need this. https://codereview.chromium.org/2493223002/diff/320001/components/exo/exo_com... components/exo/exo_compositor_frame_sink.h:10: #include <set> I suspect you don't need this. https://codereview.chromium.org/2493223002/diff/320001/components/exo/exo_com... components/exo/exo_compositor_frame_sink.h:15: #include "base/memory/ref_counted.h" You don't need this. https://codereview.chromium.org/2493223002/diff/320001/components/exo/exo_com... components/exo/exo_compositor_frame_sink.h:17: #include "base/observer_list.h" I suspect you don't need this. https://codereview.chromium.org/2493223002/diff/320001/components/exo/exo_com... components/exo/exo_compositor_frame_sink.h:24: #include "cc/surfaces/surface_id_allocator.h" You don't need surface_id_allocator.h https://codereview.chromium.org/2493223002/diff/320001/components/exo/exo_com... components/exo/exo_compositor_frame_sink.h:27: #include "third_party/skia/include/core/SkXfermode.h" I don't think you need third_party/skia. https://codereview.chromium.org/2493223002/diff/320001/components/exo/exo_com... components/exo/exo_compositor_frame_sink.h:29: #include "ui/aura/window_observer.h" You don't need to depend on aura here. https://codereview.chromium.org/2493223002/diff/320001/components/exo/exo_com... components/exo/exo_compositor_frame_sink.h:37: class CompositorFrameSinkHolder; remove this. https://codereview.chromium.org/2493223002/diff/320001/components/exo/exo_com... components/exo/exo_compositor_frame_sink.h:52: // To be cc::mojom::MojoCompositorFrameSink: Make this cc::mojom::MojoCompositorFrameSink now? https://codereview.chromium.org/2493223002/diff/320001/components/exo/exo_com... components/exo/exo_compositor_frame_sink.h:57: friend class base::RefCounted<ExoCompositorFrameSink>; remove this. https://codereview.chromium.org/2493223002/diff/320001/components/exo/surface.cc File components/exo/surface.cc (right): https://codereview.chromium.org/2493223002/diff/320001/components/exo/surface... components/exo/surface.cc:27: #include "mojo/public/cpp/bindings/strong_binding.h" I suspect you don't need this.
https://codereview.chromium.org/2493223002/diff/320001/components/exo/composi... File components/exo/compositor_frame_sink_holder.h (right): https://codereview.chromium.org/2493223002/diff/320001/components/exo/composi... components/exo/compositor_frame_sink_holder.h:27: Surface* surface, On 2016/11/23 14:39:02, Fady Samuel wrote: > base::WeakPtr<Surface> surface Done.
https://codereview.chromium.org/2493223002/diff/320001/components/exo/exo_com... File components/exo/exo_compositor_frame_sink.cc (right): https://codereview.chromium.org/2493223002/diff/320001/components/exo/exo_com... components/exo/exo_compositor_frame_sink.cc:8: #include "components/exo/compositor_frame_sink_holder.h" On 2016/11/23 14:39:02, Fady Samuel wrote: > You don't need this. Done. https://codereview.chromium.org/2493223002/diff/320001/components/exo/exo_com... components/exo/exo_compositor_frame_sink.cc:9: #include "ui/aura/env.h" On 2016/11/23 14:39:02, Fady Samuel wrote: > You don't need this. Done. https://codereview.chromium.org/2493223002/diff/320001/components/exo/exo_com... File components/exo/exo_compositor_frame_sink.h (right): https://codereview.chromium.org/2493223002/diff/320001/components/exo/exo_com... components/exo/exo_compositor_frame_sink.h:8: #include <list> On 2016/11/23 14:39:02, Fady Samuel wrote: > I suspect you don't need this. Done. https://codereview.chromium.org/2493223002/diff/320001/components/exo/exo_com... components/exo/exo_compositor_frame_sink.h:10: #include <set> On 2016/11/23 14:39:02, Fady Samuel wrote: > I suspect you don't need this. Done. https://codereview.chromium.org/2493223002/diff/320001/components/exo/exo_com... components/exo/exo_compositor_frame_sink.h:15: #include "base/memory/ref_counted.h" On 2016/11/23 14:39:02, Fady Samuel wrote: > You don't need this. Done. https://codereview.chromium.org/2493223002/diff/320001/components/exo/exo_com... components/exo/exo_compositor_frame_sink.h:17: #include "base/observer_list.h" On 2016/11/23 14:39:02, Fady Samuel wrote: > I suspect you don't need this. Done. https://codereview.chromium.org/2493223002/diff/320001/components/exo/exo_com... components/exo/exo_compositor_frame_sink.h:24: #include "cc/surfaces/surface_id_allocator.h" On 2016/11/23 14:39:02, Fady Samuel wrote: > You don't need surface_id_allocator.h Done. https://codereview.chromium.org/2493223002/diff/320001/components/exo/exo_com... components/exo/exo_compositor_frame_sink.h:27: #include "third_party/skia/include/core/SkXfermode.h" On 2016/11/23 14:39:03, Fady Samuel wrote: > I don't think you need third_party/skia. Done. https://codereview.chromium.org/2493223002/diff/320001/components/exo/exo_com... components/exo/exo_compositor_frame_sink.h:29: #include "ui/aura/window_observer.h" On 2016/11/23 14:39:02, Fady Samuel wrote: > You don't need to depend on aura here. Done. https://codereview.chromium.org/2493223002/diff/320001/components/exo/exo_com... components/exo/exo_compositor_frame_sink.h:37: class CompositorFrameSinkHolder; On 2016/11/23 14:39:02, Fady Samuel wrote: > remove this. Done. https://codereview.chromium.org/2493223002/diff/320001/components/exo/exo_com... components/exo/exo_compositor_frame_sink.h:52: // To be cc::mojom::MojoCompositorFrameSink: On 2016/11/23 14:39:02, Fady Samuel wrote: > Make this cc::mojom::MojoCompositorFrameSink now? Done. https://codereview.chromium.org/2493223002/diff/320001/components/exo/exo_com... components/exo/exo_compositor_frame_sink.h:57: friend class base::RefCounted<ExoCompositorFrameSink>; On 2016/11/23 14:39:02, Fady Samuel wrote: > remove this. Done.
https://codereview.chromium.org/2493223002/diff/320001/components/exo/surface.cc File components/exo/surface.cc (right): https://codereview.chromium.org/2493223002/diff/320001/components/exo/surface... components/exo/surface.cc:27: #include "mojo/public/cpp/bindings/strong_binding.h" On 2016/11/23 14:39:03, Fady Samuel wrote: > I suspect you don't need this. Done.
https://codereview.chromium.org/2493223002/diff/320001/ui/aura/mus/window_com... File ui/aura/mus/window_compositor_frame_sink.cc (right): https://codereview.chromium.org/2493223002/diff/320001/ui/aura/mus/window_com... ui/aura/mus/window_compositor_frame_sink.cc:114: void WindowCompositorFrameSink::WillDrawSurface() {} Add a TODO to implement this. https://codereview.chromium.org/2493223002/diff/360001/components/exo/buffer.cc File components/exo/buffer.cc (right): https://codereview.chromium.org/2493223002/diff/360001/components/exo/buffer.... components/exo/buffer.cc:435: No need for code churn. Just restore this file to its original state. https://codereview.chromium.org/2493223002/diff/360001/components/exo/test/ex... File components/exo/test/exo_test_base.h (right): https://codereview.chromium.org/2493223002/diff/360001/components/exo/test/ex... components/exo/test/exo_test_base.h:33: // base::MessageLoop loop_; nit: you don't need this right?
https://codereview.chromium.org/2493223002/diff/260001/components/exo/surface.cc File components/exo/surface.cc (right): https://codereview.chromium.org/2493223002/diff/260001/components/exo/surface... components/exo/surface.cc:701: compositor_frame_sink_holder_->release_callbacks_[resource.id] = On 2016/11/21 21:09:35, Fady Samuel wrote: > AddResourceReleaseCallback(ResourceId, callback)? Done. https://codereview.chromium.org/2493223002/diff/320001/ui/aura/mus/window_com... File ui/aura/mus/window_compositor_frame_sink.cc (right): https://codereview.chromium.org/2493223002/diff/320001/ui/aura/mus/window_com... ui/aura/mus/window_compositor_frame_sink.cc:114: void WindowCompositorFrameSink::WillDrawSurface() {} On 2016/11/23 15:39:02, Fady Samuel wrote: > Add a TODO to implement this. Done. https://codereview.chromium.org/2493223002/diff/360001/components/exo/buffer.cc File components/exo/buffer.cc (right): https://codereview.chromium.org/2493223002/diff/360001/components/exo/buffer.... components/exo/buffer.cc:435: On 2016/11/23 15:39:02, Fady Samuel wrote: > No need for code churn. Just restore this file to its original state. Done. https://codereview.chromium.org/2493223002/diff/360001/components/exo/test/ex... File components/exo/test/exo_test_base.h (right): https://codereview.chromium.org/2493223002/diff/360001/components/exo/test/ex... components/exo/test/exo_test_base.h:33: // base::MessageLoop loop_; On 2016/11/23 15:39:02, Fady Samuel wrote: > nit: you don't need this right? Done.
https://codereview.chromium.org/2493223002/diff/400001/components/exo/composi... File components/exo/compositor_frame_sink_holder.cc (right): https://codereview.chromium.org/2493223002/diff/400001/components/exo/composi... components/exo/compositor_frame_sink_holder.cc:30: release_callbacks_[id] = std::make_pair(this, std::move(callback)); compositor_frame_sink_->SetNeedsBeginFrame(true); https://codereview.chromium.org/2493223002/diff/400001/components/exo/exo_com... File components/exo/exo_compositor_frame_sink.cc (right): https://codereview.chromium.org/2493223002/diff/400001/components/exo/exo_com... components/exo/exo_compositor_frame_sink.cc:107: last_begin_frame_args_ = args; You should call out to the client from here: if (client_) client_->OnBeginFrame(args); https://codereview.chromium.org/2493223002/diff/400001/components/exo/surface.cc File components/exo/surface.cc (right): https://codereview.chromium.org/2493223002/diff/400001/components/exo/surface... components/exo/surface.cc:763: frame.render_pass_list.push_back(std::move(render_pass)); compositor_frame_sink_holder_->SetNeedsBeginFrame(true);
https://codereview.chromium.org/2493223002/diff/400001/components/exo/surface.cc File components/exo/surface.cc (left): https://codereview.chromium.org/2493223002/diff/400001/components/exo/surface... components/exo/surface.cc:252: factory_owner_->surface_factory_->Destroy(local_frame_id_); You need to still do this destroy here. Otherwise the cc::Surface will continue to exist, so its resource will never be returned even if all the consumers are done with it (they will also hold on to the CompositorFrameSinkHolder and ExoCompositorFrameSink).
https://codereview.chromium.org/2493223002/diff/400001/components/exo/composi... File components/exo/compositor_frame_sink_holder.cc (right): https://codereview.chromium.org/2493223002/diff/400001/components/exo/composi... components/exo/compositor_frame_sink_holder.cc:30: release_callbacks_[id] = std::make_pair(this, std::move(callback)); On 2016/11/23 21:14:48, Fady Samuel wrote: > compositor_frame_sink_->SetNeedsBeginFrame(true); Done. https://codereview.chromium.org/2493223002/diff/400001/components/exo/exo_com... File components/exo/exo_compositor_frame_sink.cc (right): https://codereview.chromium.org/2493223002/diff/400001/components/exo/exo_com... components/exo/exo_compositor_frame_sink.cc:107: last_begin_frame_args_ = args; On 2016/11/23 21:14:48, Fady Samuel wrote: > You should call out to the client from here: > > if (client_) > client_->OnBeginFrame(args); Done. https://codereview.chromium.org/2493223002/diff/400001/components/exo/surface.cc File components/exo/surface.cc (left): https://codereview.chromium.org/2493223002/diff/400001/components/exo/surface... components/exo/surface.cc:252: factory_owner_->surface_factory_->Destroy(local_frame_id_); On 2016/11/23 22:43:29, jbauman wrote: > You need to still do this destroy here. Otherwise the cc::Surface will continue > to exist, so its resource will never be returned even if all the consumers are > done with it (they will also hold on to the CompositorFrameSinkHolder and > ExoCompositorFrameSink). Done. https://codereview.chromium.org/2493223002/diff/400001/components/exo/surface.cc File components/exo/surface.cc (right): https://codereview.chromium.org/2493223002/diff/400001/components/exo/surface... components/exo/surface.cc:763: frame.render_pass_list.push_back(std::move(render_pass)); On 2016/11/23 21:14:48, Fady Samuel wrote: > compositor_frame_sink_holder_->SetNeedsBeginFrame(true); Done.
The CQ bit was checked by staraz@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: chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
https://codereview.chromium.org/2493223002/diff/660001/components/exo/composi... File components/exo/compositor_frame_sink_holder.cc (right): https://codereview.chromium.org/2493223002/diff/660001/components/exo/composi... components/exo/compositor_frame_sink_holder.cc:35: compositor_frame_sink_->SetNeedsBeginFrame(true); This might not be necessary. https://codereview.chromium.org/2493223002/diff/660001/components/exo/composi... components/exo/compositor_frame_sink_holder.cc:56: begin_frame_source_.reset(); Is this necessary? https://codereview.chromium.org/2493223002/diff/660001/components/exo/composi... components/exo/compositor_frame_sink_holder.cc:79: if (compositor_frame_sink_) nit: braces for multi-line blocks. https://codereview.chromium.org/2493223002/diff/660001/components/exo/composi... components/exo/compositor_frame_sink_holder.cc:87: // TODO(staraz): Implement this Put the TODO in the body. https://codereview.chromium.org/2493223002/diff/660001/components/exo/composi... components/exo/compositor_frame_sink_holder.cc:122: // ExoComopositorFrameSink, private: This comment is wrong. This is BeginFrameObserver, I think? https://codereview.chromium.org/2493223002/diff/660001/components/exo/composi... File components/exo/compositor_frame_sink_holder.h (right): https://codereview.chromium.org/2493223002/diff/660001/components/exo/composi... components/exo/compositor_frame_sink_holder.h:74: std::unique_ptr<ExoCompositorFrameSink> compositor_frame_sink_; Make this cc::mojom::MojoCompositorFrameSink https://codereview.chromium.org/2493223002/diff/660001/components/exo/surface.cc File components/exo/surface.cc (right): https://codereview.chromium.org/2493223002/diff/660001/components/exo/surface... components/exo/surface.cc:202: compositor_frame_sink_holder_->ActivateFrameCallbacks(frame_callbacks_); This is a weird API. I really don't understand what it's doing and the line above. Please clarify? Also, do we need |frame_callbacks_|. Furthermore, you shouldn't pass things by (non-const) ref. Either pass by pointer if they will change, or pass by const ref. https://codereview.chromium.org/2493223002/diff/660001/components/exo/surface... components/exo/surface.cc:566: compositor_frame_sink_holder_->UpdateNeedsBeginFrame(); I don't think you should expose this? https://codereview.chromium.org/2493223002/diff/660001/components/exo/surface... components/exo/surface.cc:766: compositor_frame_sink_holder_->SetNeedsBeginFrame(true); I don't think this line is necessary anymore? https://codereview.chromium.org/2493223002/diff/660001/components/exo/surface... File components/exo/surface_unittest.cc (right): https://codereview.chromium.org/2493223002/diff/660001/components/exo/surface... components/exo/surface_unittest.cc:54: RunAllPendingInMessageLoop(); Add a comment about why this is necessary. https://codereview.chromium.org/2493223002/diff/660001/components/exo/test/ex... File components/exo/test/exo_test_base.cc (right): https://codereview.chromium.org/2493223002/diff/660001/components/exo/test/ex... components/exo/test/exo_test_base.cc:35: RunAllPendingInMessageLoop(); Add a comment about why this is necessary. https://codereview.chromium.org/2493223002/diff/660001/ui/aura/mus/window_com... File ui/aura/mus/window_compositor_frame_sink.cc (right): https://codereview.chromium.org/2493223002/diff/660001/ui/aura/mus/window_com... ui/aura/mus/window_compositor_frame_sink.cc:114: // TODO: Implement this. Move inside of the method's block. Also, add an LDAP TODO(fsamuel, staraz):
FYI, this is making significant changes to exo so in addition to the unit tests we'll need to verify that Android apps and other wayland clients still work correctly before it lands.
https://codereview.chromium.org/2493223002/diff/660001/components/exo/surface.cc File components/exo/surface.cc (right): https://codereview.chromium.org/2493223002/diff/660001/components/exo/surface... components/exo/surface.cc:202: compositor_frame_sink_holder_->ActivateFrameCallbacks(frame_callbacks_); On 2016/11/30 16:41:57, Fady Samuel wrote: > This is a weird API. I really don't understand what it's doing and the line > above. Please clarify? Also, do we need |frame_callbacks_|. Furthermore, you > shouldn't pass things by (non-const) ref. Either pass by pointer if they will > change, or pass by const ref. The comments in surface.h says these callbacks notify the clients when it is a good time to start producing a new frame. I guess it's similar to what a BeginFrame does. The callbacks are added in Surface::RequestFrameCallback() in wayland server. They run in OnBeginFrame(). We might be able to get rid of them with more plumbing for BeginFrame. In the line above, since the Surface is being destroyed, we call all frame callbacks with a null frame time to indicate that they are canceled. CompositorFrameSinkHolder::ActivateFrameCallbacks() calls std::list::splice(), which takes either a non-const reference or an rvalue reference.
https://codereview.chromium.org/2493223002/diff/660001/components/exo/composi... File components/exo/compositor_frame_sink_holder.cc (right): https://codereview.chromium.org/2493223002/diff/660001/components/exo/composi... components/exo/compositor_frame_sink_holder.cc:35: compositor_frame_sink_->SetNeedsBeginFrame(true); On 2016/11/30 16:41:56, Fady Samuel wrote: > This might not be necessary. Done. https://codereview.chromium.org/2493223002/diff/660001/components/exo/composi... components/exo/compositor_frame_sink_holder.cc:56: begin_frame_source_.reset(); On 2016/11/30 16:41:57, Fady Samuel wrote: > Is this necessary? Done. https://codereview.chromium.org/2493223002/diff/660001/components/exo/composi... components/exo/compositor_frame_sink_holder.cc:79: if (compositor_frame_sink_) On 2016/11/30 16:41:57, Fady Samuel wrote: > nit: braces for multi-line blocks. Done. https://codereview.chromium.org/2493223002/diff/660001/components/exo/composi... components/exo/compositor_frame_sink_holder.cc:87: // TODO(staraz): Implement this On 2016/11/30 16:41:57, Fady Samuel wrote: > Put the TODO in the body. Done. https://codereview.chromium.org/2493223002/diff/660001/components/exo/composi... components/exo/compositor_frame_sink_holder.cc:122: // ExoComopositorFrameSink, private: On 2016/11/30 16:41:57, Fady Samuel wrote: > This comment is wrong. This is BeginFrameObserver, I think? Done. https://codereview.chromium.org/2493223002/diff/660001/components/exo/surface.cc File components/exo/surface.cc (right): https://codereview.chromium.org/2493223002/diff/660001/components/exo/surface... components/exo/surface.cc:566: compositor_frame_sink_holder_->UpdateNeedsBeginFrame(); On 2016/11/30 16:41:57, Fady Samuel wrote: > I don't think you should expose this? Done. https://codereview.chromium.org/2493223002/diff/660001/components/exo/surface... components/exo/surface.cc:766: compositor_frame_sink_holder_->SetNeedsBeginFrame(true); On 2016/11/30 16:41:57, Fady Samuel wrote: > I don't think this line is necessary anymore? Done. https://codereview.chromium.org/2493223002/diff/660001/components/exo/surface... File components/exo/surface_unittest.cc (right): https://codereview.chromium.org/2493223002/diff/660001/components/exo/surface... components/exo/surface_unittest.cc:54: RunAllPendingInMessageLoop(); On 2016/11/30 16:41:57, Fady Samuel wrote: > Add a comment about why this is necessary. Done. https://codereview.chromium.org/2493223002/diff/660001/components/exo/test/ex... File components/exo/test/exo_test_base.cc (right): https://codereview.chromium.org/2493223002/diff/660001/components/exo/test/ex... components/exo/test/exo_test_base.cc:35: RunAllPendingInMessageLoop(); On 2016/11/30 16:41:57, Fady Samuel wrote: > Add a comment about why this is necessary. This RunAllPendingInMessageLoop() isn't necessary and is hence removed. https://codereview.chromium.org/2493223002/diff/660001/ui/aura/mus/window_com... File ui/aura/mus/window_compositor_frame_sink.cc (right): https://codereview.chromium.org/2493223002/diff/660001/ui/aura/mus/window_com... ui/aura/mus/window_compositor_frame_sink.cc:114: // TODO: Implement this. On 2016/11/30 16:41:57, Fady Samuel wrote: > Move inside of the method's block. Also, add an LDAP TODO(fsamuel, staraz): Done.
On 2016/11/30 at 19:04:04, staraz wrote: > https://codereview.chromium.org/2493223002/diff/660001/components/exo/surface.cc > File components/exo/surface.cc (right): > > https://codereview.chromium.org/2493223002/diff/660001/components/exo/surface... > components/exo/surface.cc:202: compositor_frame_sink_holder_->ActivateFrameCallbacks(frame_callbacks_); > On 2016/11/30 16:41:57, Fady Samuel wrote: > > This is a weird API. I really don't understand what it's doing and the line > > above. Please clarify? Also, do we need |frame_callbacks_|. Furthermore, you > > shouldn't pass things by (non-const) ref. Either pass by pointer if they will > > change, or pass by const ref. > > The comments in surface.h says these callbacks notify the clients when it is a good time to start producing a new frame. I guess it's similar to what a BeginFrame does. The callbacks are added in Surface::RequestFrameCallback() in wayland server. They run in OnBeginFrame(). We might be able to get rid of them with more plumbing for BeginFrame. > > In the line above, since the Surface is being destroyed, we call all frame callbacks with a null frame time to indicate that they are canceled. Correct. These callbacks need to run even if the surface is destroyed to be wayland spec compliant. > > CompositorFrameSinkHolder::ActivateFrameCallbacks() calls std::list::splice(), which takes either a non-const reference or an rvalue reference.
https://codereview.chromium.org/2493223002/diff/660001/components/exo/composi... File components/exo/compositor_frame_sink_holder.h (right): https://codereview.chromium.org/2493223002/diff/660001/components/exo/composi... components/exo/compositor_frame_sink_holder.h:74: std::unique_ptr<ExoCompositorFrameSink> compositor_frame_sink_; On 2016/11/30 16:41:57, Fady Samuel wrote: > Make this cc::mojom::MojoCompositorFrameSink Done.
Mojo should manage the lifetime of exo::CompositorFrameSink, not CompositorFrameSinkHolder. CompositorFrameSinkHolder should just hold on to a cc::mojom::MojoCompositorFrameSinkPtr. You might want to make exo::CompositorFrameSink managed by a strong binding.
On 2016/11/30 17:13:08, reveman wrote: > FYI, this is making significant changes to exo so in addition to the unit tests > we'll need to verify that Android apps and other wayland clients still work > correctly before it lands. I am testing by running wayland apps. Sometimes I get error "surface.cc(156): Attempting to require callback on nonexistent surface". This happens when exo::Surface::RequestFrameCallback() gets called before MojoCompositorFrameSink::SubmitCompositorFrame() finishes. It doesn't crash or anything. Is this behavior okay?
On 2016/12/01 19:25:57, StarAZ1 wrote: > On 2016/11/30 17:13:08, reveman wrote: > > FYI, this is making significant changes to exo so in addition to the unit > tests > > we'll need to verify that Android apps and other wayland clients still work > > correctly before it lands. > > I am testing by running wayland apps. Sometimes I get error "surface.cc(156): > Attempting to require callback on nonexistent surface". This happens when > exo::Surface::RequestFrameCallback() gets called before > MojoCompositorFrameSink::SubmitCompositorFrame() finishes. It doesn't crash or > anything. Is this behavior okay? No, that means that the require callback wasn't executed, so the frame could be destroyed while it's in use. The require callback needs to happen after the surface is created.
On 2016/12/01 19:28:09, jbauman wrote: > On 2016/12/01 19:25:57, StarAZ1 wrote: > > On 2016/11/30 17:13:08, reveman wrote: > > > FYI, this is making significant changes to exo so in addition to the unit > > tests > > > we'll need to verify that Android apps and other wayland clients still work > > > correctly before it lands. > > > > I am testing by running wayland apps. Sometimes I get error "surface.cc(156): > > Attempting to require callback on nonexistent surface". This happens when > > exo::Surface::RequestFrameCallback() gets called before > > MojoCompositorFrameSink::SubmitCompositorFrame() finishes. It doesn't crash or > > > anything. Is this behavior okay? > > No, that means that the require callback wasn't executed, so the frame could be > destroyed while it's in use. The require callback needs to happen after the > surface is created. OK chatting with kylechar@, it seems like we need to add temporary RequireSurfaceSequence/SatisfySurfaceSequence methods to MojoCompositorFrameSink to ensure that ordering is preserved across SubmitCompositorFrameSink. Once surface references are done we can switch to that.
https://codereview.chromium.org/2493223002/diff/780001/ash/test/ash_test_help... File ash/test/ash_test_helper.cc (right): https://codereview.chromium.org/2493223002/diff/780001/ash/test/ash_test_help... ash/test/ash_test_helper.cc:166: RunAllPendingInMessageLoop(); please add a comment indicating why this is necessary. https://codereview.chromium.org/2493223002/diff/780001/cc/ipc/mojo_compositor... File cc/ipc/mojo_compositor_frame_sink.mojom (right): https://codereview.chromium.org/2493223002/diff/780001/cc/ipc/mojo_compositor... cc/ipc/mojo_compositor_frame_sink.mojom:34: EvictFrame(); Add a comment explaining what this does. https://codereview.chromium.org/2493223002/diff/780001/cc/ipc/mojo_compositor... cc/ipc/mojo_compositor_frame_sink.mojom:55: WillDrawSurface(); Add a comment explaining when this is called. https://codereview.chromium.org/2493223002/diff/780001/components/exo/composi... File components/exo/compositor_frame_sink.cc (right): https://codereview.chromium.org/2493223002/diff/780001/components/exo/composi... components/exo/compositor_frame_sink.cc:22: auto* impl = new CompositorFrameSink(frame_sink_id, surface_manager, base::MakeUnique<CompositorFrameSink>(frame_sink_id, surface_manager, std::move(client)); https://codereview.chromium.org/2493223002/diff/780001/components/exo/composi... components/exo/compositor_frame_sink.cc:25: mojo::MakeStrongBinding(base::WrapUnique(impl), std::move(request)); use MakeUnique above instead of wrapunique.
https://codereview.chromium.org/2493223002/diff/780001/components/exo/composi... File components/exo/compositor_frame_sink.cc (right): https://codereview.chromium.org/2493223002/diff/780001/components/exo/composi... components/exo/compositor_frame_sink.cc:33: : frame_sink_id_(frame_sink_id), Now that CompositorFrameSinkSupport has landed you can replace most of the implementation details in here with that: https://codereview.chromium.org/2543373002/
https://codereview.chromium.org/2493223002/diff/800001/cc/ipc/mojo_compositor... File cc/ipc/mojo_compositor_frame_sink.mojom (right): https://codereview.chromium.org/2493223002/diff/800001/cc/ipc/mojo_compositor... cc/ipc/mojo_compositor_frame_sink.mojom:40: // sequence. How about: "Add the provided |sequence| as a destruction dependency of the surface associated with the provided |local_frame_id|." Also, Add a TODO(fsamuel, kylechar, staraz): Remove these methods once surface references are ready. https://codereview.chromium.org/2493223002/diff/800001/services/ui/surfaces/g... File services/ui/surfaces/gpu_compositor_frame_sink.cc (right): https://codereview.chromium.org/2493223002/diff/800001/services/ui/surfaces/g... services/ui/surfaces/gpu_compositor_frame_sink.cc:53: NOTIMPLEMENTED(); Stick the implementation in CompositorFrameSinkSupport for now.
https://codereview.chromium.org/2493223002/diff/780001/ash/test/ash_test_help... File ash/test/ash_test_helper.cc (right): https://codereview.chromium.org/2493223002/diff/780001/ash/test/ash_test_help... ash/test/ash_test_helper.cc:166: RunAllPendingInMessageLoop(); On 2016/12/01 20:38:45, Fady Samuel wrote: > please add a comment indicating why this is necessary. Done. https://codereview.chromium.org/2493223002/diff/780001/cc/ipc/mojo_compositor... File cc/ipc/mojo_compositor_frame_sink.mojom (right): https://codereview.chromium.org/2493223002/diff/780001/cc/ipc/mojo_compositor... cc/ipc/mojo_compositor_frame_sink.mojom:34: EvictFrame(); On 2016/12/01 20:38:46, Fady Samuel wrote: > Add a comment explaining what this does. This is done is the separate CL. https://codereview.chromium.org/2493223002/diff/780001/cc/ipc/mojo_compositor... cc/ipc/mojo_compositor_frame_sink.mojom:55: WillDrawSurface(); On 2016/12/01 20:38:46, Fady Samuel wrote: > Add a comment explaining when this is called. This is done is the separate CL. https://codereview.chromium.org/2493223002/diff/780001/components/exo/composi... File components/exo/compositor_frame_sink.cc (right): https://codereview.chromium.org/2493223002/diff/780001/components/exo/composi... components/exo/compositor_frame_sink.cc:22: auto* impl = new CompositorFrameSink(frame_sink_id, surface_manager, On 2016/12/01 20:38:46, Fady Samuel wrote: > base::MakeUnique<CompositorFrameSink>(frame_sink_id, surface_manager, > std::move(client)); Done. https://codereview.chromium.org/2493223002/diff/780001/components/exo/composi... components/exo/compositor_frame_sink.cc:25: mojo::MakeStrongBinding(base::WrapUnique(impl), std::move(request)); On 2016/12/01 20:38:46, Fady Samuel wrote: > use MakeUnique above instead of wrapunique. Done. https://codereview.chromium.org/2493223002/diff/780001/components/exo/composi... components/exo/compositor_frame_sink.cc:33: : frame_sink_id_(frame_sink_id), On 2016/12/03 07:14:09, Fady Samuel wrote: > Now that CompositorFrameSinkSupport has landed you can replace most of the > implementation details in here with that: > > https://codereview.chromium.org/2543373002/ Done. https://codereview.chromium.org/2493223002/diff/800001/cc/ipc/mojo_compositor... File cc/ipc/mojo_compositor_frame_sink.mojom (right): https://codereview.chromium.org/2493223002/diff/800001/cc/ipc/mojo_compositor... cc/ipc/mojo_compositor_frame_sink.mojom:40: // sequence. On 2016/12/05 16:40:31, Fady Samuel wrote: > How about: "Add the provided |sequence| as a destruction dependency of the > surface associated with the provided |local_frame_id|." > > Also, Add a TODO(fsamuel, kylechar, staraz): Remove these methods once surface > references are ready. Done. The change has also been made to the separate CL. https://codereview.chromium.org/2493223002/diff/800001/services/ui/surfaces/g... File services/ui/surfaces/gpu_compositor_frame_sink.cc (right): https://codereview.chromium.org/2493223002/diff/800001/services/ui/surfaces/g... services/ui/surfaces/gpu_compositor_frame_sink.cc:53: NOTIMPLEMENTED(); On 2016/12/05 16:40:31, Fady Samuel wrote: > Stick the implementation in CompositorFrameSinkSupport for now. Done.
https://codereview.chromium.org/2493223002/diff/820001/components/exo/composi... File components/exo/compositor_frame_sink_holder.cc (right): https://codereview.chromium.org/2493223002/diff/820001/components/exo/composi... components/exo/compositor_frame_sink_holder.cc:50: void CompositorFrameSinkHolder::EvictFrame() { How about just providing an accessor to cc::mojom::MojoCompositorFrameSink from here, and getting rid of all this plumbing?
https://codereview.chromium.org/2493223002/diff/820001/components/exo/composi... File components/exo/compositor_frame_sink_holder.cc (right): https://codereview.chromium.org/2493223002/diff/820001/components/exo/composi... components/exo/compositor_frame_sink_holder.cc:50: void CompositorFrameSinkHolder::EvictFrame() { On 2016/12/06 20:14:40, Fady Samuel wrote: > How about just providing an accessor to cc::mojom::MojoCompositorFrameSink from > here, and getting rid of all this plumbing? Done.
I think I'm generally happy with this patch at this point. Preliminary non-OWNER lgtm.
https://codereview.chromium.org/2493223002/diff/840001/components/exo/surface.cc File components/exo/surface.cc (right): https://codereview.chromium.org/2493223002/diff/840001/components/exo/surface... components/exo/surface.cc:431: needs_commit_to_new_surface_ = false; Shouldn't this always be false here already? https://codereview.chromium.org/2493223002/diff/840001/components/exo/surface... components/exo/surface.cc:444: base::Bind( Are we sure these lifetimes are correct? If the exo::Surface goes away and it's not holding onto any resources (because no buffer's bound), then the layer could reference a CompositorFrameSinkHolder until the next browser compositor commit happens even though it's gone away. I think a similar problem could happen now, but it'd just print "Attempting to require callback on nonexistent surface" and destroy a surface with no contents (which therefore doesn't draw anything), and not be a use after free, so that's not too bad. https://codereview.chromium.org/2493223002/diff/840001/components/exo/surface... components/exo/surface.cc:766: ->SubmitCompositorFrame(local_frame_id_, std::move(frame)); Does this happen synchronously, or does it post a task that's executed asynchronously? If the latter, I'm worried about the ordering of this with respect to the browser compositor commit and draw. Also, if it's asynchronous I think we have a problem if there are two surfaces in a synchronized subtree that need to be committed together and there are no layer hierarchy changes. Both surface content changes need to be committed atomically so we don't have a case where one is updated and the other isn't.
lg generally. mostly nits https://codereview.chromium.org/2493223002/diff/840001/components/exo/composi... File components/exo/compositor_frame_sink.cc (right): https://codereview.chromium.org/2493223002/diff/840001/components/exo/composi... components/exo/compositor_frame_sink.cc:69: // ExoComopositorFrameSink, private: nit: "cc::CompositorFrameSinkSupportClient overrides:" instead https://codereview.chromium.org/2493223002/diff/840001/components/exo/composi... components/exo/compositor_frame_sink.cc:72: if (!client_) nit: here you early out when client_ == null but functions below conditionally access client_ instead. please make it consistent. https://codereview.chromium.org/2493223002/diff/840001/components/exo/composi... File components/exo/compositor_frame_sink.h (right): https://codereview.chromium.org/2493223002/diff/840001/components/exo/composi... components/exo/compositor_frame_sink.h:31: // cc::mojom::MojoCompositorFrameSink: nit: "// Overridden from cc::mojom::MojoCompositorFrameSink:" to be consistent with existing exo code https://codereview.chromium.org/2493223002/diff/840001/components/exo/composi... components/exo/compositor_frame_sink.h:41: // cc::CompositorFrameSinkSupportClient: nit: Overridden from cc::Comp... https://codereview.chromium.org/2493223002/diff/840001/components/exo/composi... components/exo/compositor_frame_sink.h:45: void WillDrawSurface() override; I'm not a fan of changing access to functions as part of inheritance. I think that's just confusing and you can always access these by upcasting https://codereview.chromium.org/2493223002/diff/840001/components/exo/composi... components/exo/compositor_frame_sink.h:48: nit: no need for this blankline and l.50, l.52 ones https://codereview.chromium.org/2493223002/diff/840001/components/exo/composi... File components/exo/compositor_frame_sink_holder.cc (right): https://codereview.chromium.org/2493223002/diff/840001/components/exo/composi... components/exo/compositor_frame_sink_holder.cc:13: // CompositorFrameSinkHolder, public: nit: blankline after this https://codereview.chromium.org/2493223002/diff/840001/components/exo/composi... components/exo/compositor_frame_sink_holder.cc:24: return release_callbacks_.count(id); release_callbacks_.find(id) != release_callbacks_.end() is more efficient https://codereview.chromium.org/2493223002/diff/840001/components/exo/composi... components/exo/compositor_frame_sink_holder.cc:28: return release_callbacks_.size() == 0; release_callbacks_.empty() is easier to read https://codereview.chromium.org/2493223002/diff/840001/components/exo/composi... components/exo/compositor_frame_sink_holder.cc:50: void CompositorFrameSinkHolder::UpdateNeedsBeginFrame() { this is private. please move down into correct section https://codereview.chromium.org/2493223002/diff/840001/components/exo/composi... components/exo/compositor_frame_sink_holder.cc:94: // cc::BeginFrameObserver: nit: cc::BeginFrameObserver overrides: https://codereview.chromium.org/2493223002/diff/840001/components/exo/composi... components/exo/compositor_frame_sink_holder.cc:107: void CompositorFrameSinkHolder::WillDrawSurface() { this is not private. please move up to the correct section https://codereview.chromium.org/2493223002/diff/840001/components/exo/composi... components/exo/compositor_frame_sink_holder.cc:114: //////////////////////////////////////////////////////////////////////////////// please move this up to match the new location in the header file https://codereview.chromium.org/2493223002/diff/840001/components/exo/composi... components/exo/compositor_frame_sink_holder.cc:115: // cc::ExternalBeginFrameSouceClient: cc::ExternalBeginFrameSouceClient overrides: https://codereview.chromium.org/2493223002/diff/840001/components/exo/composi... components/exo/compositor_frame_sink_holder.cc:116: void CompositorFrameSinkHolder::OnNeedsBeginFrames(bool needs_begin_frames) { nit: blankline before this https://codereview.chromium.org/2493223002/diff/840001/components/exo/composi... components/exo/compositor_frame_sink_holder.cc:118: } nit: blankline after this https://codereview.chromium.org/2493223002/diff/840001/components/exo/composi... File components/exo/compositor_frame_sink_holder.h (right): https://codereview.chromium.org/2493223002/diff/840001/components/exo/composi... components/exo/compositor_frame_sink_holder.h:22: using FrameCallback = base::Callback<void(base::TimeTicks frame_time)>; Can you move this into the class and just above ActivateFrameCallbacks()? It's fine to duplicate it in surface.h I think. https://codereview.chromium.org/2493223002/diff/840001/components/exo/composi... components/exo/compositor_frame_sink_holder.h:24: class CompositorFrameSinkHolder Can you add a short class description here that explains the purpose of this class? https://codereview.chromium.org/2493223002/diff/840001/components/exo/composi... components/exo/compositor_frame_sink_holder.h:35: bool HasReleaseCallbacks(cc::ResourceId id); HasReleaseCallbacksForResource or ResourceHasReleaseCallbacks? https://codereview.chromium.org/2493223002/diff/840001/components/exo/composi... components/exo/compositor_frame_sink_holder.h:36: bool IsReleaseCallbacksEmpty(); and this can then be named HasReleaseCallbacks https://codereview.chromium.org/2493223002/diff/840001/components/exo/composi... components/exo/compositor_frame_sink_holder.h:40: cc::mojom::MojoCompositorFrameSink* GetCompositorFrameSink() { nit: blankline before this? doesn't seem like it should be grouped with the 3 functions above.. https://codereview.chromium.org/2493223002/diff/840001/components/exo/composi... components/exo/compositor_frame_sink_holder.h:44: void ActivateFrameCallbacks(std::list<FrameCallback>& frame_callbacks); "std::list<FrameCallback>* frame_callbacks" as it's not const and you're modifying this parameter https://codereview.chromium.org/2493223002/diff/840001/components/exo/composi... components/exo/compositor_frame_sink_holder.h:49: // cc::mojom::MojoCompositorFrameSinkClient: Overridden from cc::Mojo.. https://codereview.chromium.org/2493223002/diff/840001/components/exo/composi... components/exo/compositor_frame_sink_holder.h:55: // cc::BeginFrameObserver: Overridden from.. https://codereview.chromium.org/2493223002/diff/840001/components/exo/composi... components/exo/compositor_frame_sink_holder.h:66: // cc::ExternalBeginFrameSouceClient: "Overridden from.." and don't make this private https://codereview.chromium.org/2493223002/diff/840001/components/exo/composi... components/exo/compositor_frame_sink_holder.h:76: nit: you can remove the blanklines between member variables here and below unless some of them makes sense to group together https://codereview.chromium.org/2493223002/diff/840001/components/exo/gamepad... File components/exo/gamepad_unittest.cc (right): https://codereview.chromium.org/2493223002/diff/840001/components/exo/gamepad... components/exo/gamepad_unittest.cc:65: base::RunLoop().RunUntilIdle(); why is this needed? https://codereview.chromium.org/2493223002/diff/840001/components/exo/gamepad... components/exo/gamepad_unittest.cc:121: RunAllPendingInMessageLoop(); and this? https://codereview.chromium.org/2493223002/diff/840001/components/exo/surface.cc File components/exo/surface.cc (right): https://codereview.chromium.org/2493223002/diff/840001/components/exo/surface... components/exo/surface.cc:167: cc::mojom::MojoCompositorFrameSinkClientPtr pholder; pholder? frame_sink_holder_ptr instead? https://codereview.chromium.org/2493223002/diff/840001/components/exo/surface... components/exo/surface.cc:168: cc::mojom::MojoCompositorFrameSinkPtr mojo_compositor_frame_sink_ptr; just "frame_sink_ptr" ? https://codereview.chromium.org/2493223002/diff/840001/components/exo/surface... components/exo/surface.cc:169: cc::mojom::MojoCompositorFrameSinkClientRequest holder_request = s/holder_request/frame_sink_client_request/ ? https://codereview.chromium.org/2493223002/diff/840001/components/exo/surface... components/exo/surface.cc:431: needs_commit_to_new_surface_ = false; why was this added here? https://codereview.chromium.org/2493223002/diff/840001/components/exo/surface.h File components/exo/surface.h (left): https://codereview.chromium.org/2493223002/diff/840001/components/exo/surface... components/exo/surface.h:121: using FrameCallback = base::Callback<void(base::TimeTicks frame_time)>; Please keep this. I prefer if this was still part of the Surface API. https://codereview.chromium.org/2493223002/diff/840001/components/exo/surface.h File components/exo/surface.h (right): https://codereview.chromium.org/2493223002/diff/840001/components/exo/surface... components/exo/surface.h:318: const cc::FrameSinkId frame_sink_id_; Does it make sense to group this and member variables down to local_frame_id_ together without blanklines? https://codereview.chromium.org/2493223002/diff/840001/components/exo/surface... components/exo/surface.h:322: base::WeakPtr<cc::SurfaceFactory> factory_; nit: surface_factory_ https://codereview.chromium.org/2493223002/diff/840001/components/exo/surface... components/exo/surface.h:390: base::WeakPtrFactory<Surface> weak_ptr_factory_; Can we use the SurfaceObserver interface instead of adding this? That provides weak ptr functionality already. https://codereview.chromium.org/2493223002/diff/840001/components/exo/surface... File components/exo/surface_unittest.cc (left): https://codereview.chromium.org/2493223002/diff/840001/components/exo/surface... components/exo/surface_unittest.cc:250: was this intentionally removed? https://codereview.chromium.org/2493223002/diff/840001/components/exo/surface... File components/exo/surface_unittest.cc (right): https://codereview.chromium.org/2493223002/diff/840001/components/exo/surface... components/exo/surface_unittest.cc:56: // the assertion below nit: s/below/below./ https://codereview.chromium.org/2493223002/diff/840001/components/exo/surface... components/exo/surface_unittest.cc:199: cc::Surface* another_surface = surface_manager->GetSurfaceForId(surface_id); s/another_surface/cc_surface/
https://codereview.chromium.org/2493223002/diff/840001/components/exo/surface.cc File components/exo/surface.cc (right): https://codereview.chromium.org/2493223002/diff/840001/components/exo/surface... components/exo/surface.cc:766: ->SubmitCompositorFrame(local_frame_id_, std::move(frame)); On 2016/12/06 23:44:49, jbauman wrote: > Does this happen synchronously, or does it post a task that's executed > asynchronously? If the latter, I'm worried about the ordering of this with > respect to the browser compositor commit and draw. > > Also, if it's asynchronous I think we have a problem if there are two surfaces > in a synchronized subtree that need to be committed together and there are no > layer hierarchy changes. Both surface content changes need to be committed > atomically so we don't have a case where one is updated and the other isn't. This is a problem. All submits are async, and will eventually be cross-process IPCs. You're saying there's an expectation for multiple surfaces to submit CompositorFrames atomically? The system the surfaces system uses is to allocate a new surface ID to do this. I think we're already doing this if needs_commit_to_new_surface_?
https://codereview.chromium.org/2493223002/diff/840001/components/exo/surface.cc File components/exo/surface.cc (right): https://codereview.chromium.org/2493223002/diff/840001/components/exo/surface... components/exo/surface.cc:444: base::Bind( On 2016/12/06 23:44:49, jbauman wrote: > Are we sure these lifetimes are correct? If the exo::Surface goes away and it's > not holding onto any resources (because no buffer's bound), then the layer could > reference a CompositorFrameSinkHolder until the next browser compositor commit > happens even though it's gone away. > > I think a similar problem could happen now, but it'd just print "Attempting to > require callback on nonexistent surface" and destroy a surface with no contents > (which therefore doesn't draw anything), and not be a use after free, so that's > not too bad. Makes sense. I think it sounds like we want to hold onto a weakptr to CompositorFrameSinkHolder in these callbacks.
On 2016/12/07 at 13:54:32, fsamuel wrote: > https://codereview.chromium.org/2493223002/diff/840001/components/exo/surface.cc > File components/exo/surface.cc (right): > > https://codereview.chromium.org/2493223002/diff/840001/components/exo/surface... > components/exo/surface.cc:766: ->SubmitCompositorFrame(local_frame_id_, std::move(frame)); > On 2016/12/06 23:44:49, jbauman wrote: > > Does this happen synchronously, or does it post a task that's executed > > asynchronously? If the latter, I'm worried about the ordering of this with > > respect to the browser compositor commit and draw. > > > > Also, if it's asynchronous I think we have a problem if there are two surfaces > > in a synchronized subtree that need to be committed together and there are no > > layer hierarchy changes. Both surface content changes need to be committed > > atomically so we don't have a case where one is updated and the other isn't. > > This is a problem. All submits are async, and will eventually be cross-process IPCs. You're saying there's an expectation for multiple surfaces to submit CompositorFrames atomically? The system the surfaces system uses is to allocate a new surface ID to do this. I think we're already doing this if needs_commit_to_new_surface_? I think ideally we'd want each hierarchy of synchronous exo::surfaces to be in the same cc::surface. Only async exo::surfaces (main surface for shell surface or ones marked async) would get their own cc::surface. Sync sub surfaces would just be quads in a cc::surface. Does that make sense?
https://codereview.chromium.org/2493223002/diff/840001/components/exo/composi... File components/exo/compositor_frame_sink.cc (right): https://codereview.chromium.org/2493223002/diff/840001/components/exo/composi... components/exo/compositor_frame_sink.cc:69: // ExoComopositorFrameSink, private: On 2016/12/07 00:46:56, reveman wrote: > nit: "cc::CompositorFrameSinkSupportClient overrides:" instead Done. https://codereview.chromium.org/2493223002/diff/840001/components/exo/composi... components/exo/compositor_frame_sink.cc:72: if (!client_) On 2016/12/07 00:46:56, reveman wrote: > nit: here you early out when client_ == null but functions below conditionally > access client_ instead. please make it consistent. Done. https://codereview.chromium.org/2493223002/diff/840001/components/exo/composi... File components/exo/compositor_frame_sink.h (right): https://codereview.chromium.org/2493223002/diff/840001/components/exo/composi... components/exo/compositor_frame_sink.h:31: // cc::mojom::MojoCompositorFrameSink: On 2016/12/07 00:46:56, reveman wrote: > nit: "// Overridden from cc::mojom::MojoCompositorFrameSink:" to be consistent > with existing exo code Done. https://codereview.chromium.org/2493223002/diff/840001/components/exo/composi... components/exo/compositor_frame_sink.h:41: // cc::CompositorFrameSinkSupportClient: On 2016/12/07 00:46:56, reveman wrote: > nit: Overridden from cc::Comp... Done. https://codereview.chromium.org/2493223002/diff/840001/components/exo/composi... components/exo/compositor_frame_sink.h:45: void WillDrawSurface() override; On 2016/12/07 00:46:56, reveman wrote: > I'm not a fan of changing access to functions as part of inheritance. I think > that's just confusing and you can always access these by upcasting Done. https://codereview.chromium.org/2493223002/diff/840001/components/exo/composi... components/exo/compositor_frame_sink.h:48: On 2016/12/07 00:46:56, reveman wrote: > nit: no need for this blankline and l.50, l.52 ones Done. https://codereview.chromium.org/2493223002/diff/840001/components/exo/composi... File components/exo/compositor_frame_sink_holder.cc (right): https://codereview.chromium.org/2493223002/diff/840001/components/exo/composi... components/exo/compositor_frame_sink_holder.cc:13: // CompositorFrameSinkHolder, public: On 2016/12/07 00:46:57, reveman wrote: > nit: blankline after this Done. https://codereview.chromium.org/2493223002/diff/840001/components/exo/composi... components/exo/compositor_frame_sink_holder.cc:24: return release_callbacks_.count(id); On 2016/12/07 00:46:57, reveman wrote: > release_callbacks_.find(id) != release_callbacks_.end() is more efficient Done. https://codereview.chromium.org/2493223002/diff/840001/components/exo/composi... components/exo/compositor_frame_sink_holder.cc:28: return release_callbacks_.size() == 0; On 2016/12/07 00:46:56, reveman wrote: > release_callbacks_.empty() is easier to read Done. https://codereview.chromium.org/2493223002/diff/840001/components/exo/composi... components/exo/compositor_frame_sink_holder.cc:50: void CompositorFrameSinkHolder::UpdateNeedsBeginFrame() { On 2016/12/07 00:46:56, reveman wrote: > this is private. please move down into correct section Done. https://codereview.chromium.org/2493223002/diff/840001/components/exo/composi... components/exo/compositor_frame_sink_holder.cc:94: // cc::BeginFrameObserver: On 2016/12/07 00:46:57, reveman wrote: > nit: cc::BeginFrameObserver overrides: Done. https://codereview.chromium.org/2493223002/diff/840001/components/exo/composi... components/exo/compositor_frame_sink_holder.cc:107: void CompositorFrameSinkHolder::WillDrawSurface() { On 2016/12/07 00:46:56, reveman wrote: > this is not private. please move up to the correct section Done. https://codereview.chromium.org/2493223002/diff/840001/components/exo/composi... components/exo/compositor_frame_sink_holder.cc:114: //////////////////////////////////////////////////////////////////////////////// On 2016/12/07 00:46:56, reveman wrote: > please move this up to match the new location in the header file Done. https://codereview.chromium.org/2493223002/diff/840001/components/exo/composi... components/exo/compositor_frame_sink_holder.cc:115: // cc::ExternalBeginFrameSouceClient: On 2016/12/07 00:46:56, reveman wrote: > cc::ExternalBeginFrameSouceClient overrides: Done. https://codereview.chromium.org/2493223002/diff/840001/components/exo/composi... components/exo/compositor_frame_sink_holder.cc:116: void CompositorFrameSinkHolder::OnNeedsBeginFrames(bool needs_begin_frames) { On 2016/12/07 00:46:57, reveman wrote: > nit: blankline before this Done. https://codereview.chromium.org/2493223002/diff/840001/components/exo/composi... components/exo/compositor_frame_sink_holder.cc:118: } On 2016/12/07 00:46:56, reveman wrote: > nit: blankline after this Done. https://codereview.chromium.org/2493223002/diff/840001/components/exo/composi... File components/exo/compositor_frame_sink_holder.h (right): https://codereview.chromium.org/2493223002/diff/840001/components/exo/composi... components/exo/compositor_frame_sink_holder.h:22: using FrameCallback = base::Callback<void(base::TimeTicks frame_time)>; On 2016/12/07 00:46:57, reveman wrote: > Can you move this into the class and just above ActivateFrameCallbacks()? It's > fine to duplicate it in surface.h I think. Done. https://codereview.chromium.org/2493223002/diff/840001/components/exo/composi... components/exo/compositor_frame_sink_holder.h:24: class CompositorFrameSinkHolder On 2016/12/07 00:46:57, reveman wrote: > Can you add a short class description here that explains the purpose of this > class? Done. https://codereview.chromium.org/2493223002/diff/840001/components/exo/composi... components/exo/compositor_frame_sink_holder.h:35: bool HasReleaseCallbacks(cc::ResourceId id); On 2016/12/07 00:46:57, reveman wrote: > HasReleaseCallbacksForResource or ResourceHasReleaseCallbacks? Done. https://codereview.chromium.org/2493223002/diff/840001/components/exo/composi... components/exo/compositor_frame_sink_holder.h:36: bool IsReleaseCallbacksEmpty(); On 2016/12/07 00:46:57, reveman wrote: > and this can then be named HasReleaseCallbacks It turns out this is no longer being used so I removed it. https://codereview.chromium.org/2493223002/diff/840001/components/exo/composi... components/exo/compositor_frame_sink_holder.h:40: cc::mojom::MojoCompositorFrameSink* GetCompositorFrameSink() { On 2016/12/07 00:46:57, reveman wrote: > nit: blankline before this? doesn't seem like it should be grouped with the 3 > functions above.. Done. https://codereview.chromium.org/2493223002/diff/840001/components/exo/composi... components/exo/compositor_frame_sink_holder.h:44: void ActivateFrameCallbacks(std::list<FrameCallback>& frame_callbacks); On 2016/12/07 00:46:57, reveman wrote: > "std::list<FrameCallback>* frame_callbacks" as it's not const and you're > modifying this parameter Done. https://codereview.chromium.org/2493223002/diff/840001/components/exo/composi... components/exo/compositor_frame_sink_holder.h:55: // cc::BeginFrameObserver: On 2016/12/07 00:46:57, reveman wrote: > Overridden from.. Done. https://codereview.chromium.org/2493223002/diff/840001/components/exo/composi... components/exo/compositor_frame_sink_holder.h:66: // cc::ExternalBeginFrameSouceClient: On 2016/12/07 00:46:57, reveman wrote: > "Overridden from.." and don't make this private Done. https://codereview.chromium.org/2493223002/diff/840001/components/exo/composi... components/exo/compositor_frame_sink_holder.h:76: On 2016/12/07 00:46:57, reveman wrote: > nit: you can remove the blanklines between member variables here and below > unless some of them makes sense to group together Done. https://codereview.chromium.org/2493223002/diff/840001/components/exo/gamepad... File components/exo/gamepad_unittest.cc (right): https://codereview.chromium.org/2493223002/diff/840001/components/exo/gamepad... components/exo/gamepad_unittest.cc:65: base::RunLoop().RunUntilIdle(); On 2016/12/07 00:46:57, reveman wrote: > why is this needed? Good catch. It is not. It is some leftover from debugging that I missed. https://codereview.chromium.org/2493223002/diff/840001/components/exo/gamepad... components/exo/gamepad_unittest.cc:121: RunAllPendingInMessageLoop(); On 2016/12/07 00:46:57, reveman wrote: > and this? Same as above. https://codereview.chromium.org/2493223002/diff/840001/components/exo/surface.cc File components/exo/surface.cc (right): https://codereview.chromium.org/2493223002/diff/840001/components/exo/surface... components/exo/surface.cc:167: cc::mojom::MojoCompositorFrameSinkClientPtr pholder; On 2016/12/07 00:46:57, reveman wrote: > pholder? frame_sink_holder_ptr instead? Done. https://codereview.chromium.org/2493223002/diff/840001/components/exo/surface... components/exo/surface.cc:168: cc::mojom::MojoCompositorFrameSinkPtr mojo_compositor_frame_sink_ptr; On 2016/12/07 00:46:57, reveman wrote: > just "frame_sink_ptr" ? Done. https://codereview.chromium.org/2493223002/diff/840001/components/exo/surface... components/exo/surface.cc:169: cc::mojom::MojoCompositorFrameSinkClientRequest holder_request = On 2016/12/07 00:46:57, reveman wrote: > s/holder_request/frame_sink_client_request/ ? Done. https://codereview.chromium.org/2493223002/diff/840001/components/exo/surface... components/exo/surface.cc:431: needs_commit_to_new_surface_ = false; On 2016/12/06 23:44:49, jbauman wrote: > Shouldn't this always be false here already? Done. https://codereview.chromium.org/2493223002/diff/840001/components/exo/surface... components/exo/surface.cc:444: base::Bind( On 2016/12/07 14:34:04, Fady Samuel wrote: > On 2016/12/06 23:44:49, jbauman wrote: > > Are we sure these lifetimes are correct? If the exo::Surface goes away and > it's > > not holding onto any resources (because no buffer's bound), then the layer > could > > reference a CompositorFrameSinkHolder until the next browser compositor commit > > happens even though it's gone away. > > > > I think a similar problem could happen now, but it'd just print "Attempting to > > require callback on nonexistent surface" and destroy a surface with no > contents > > (which therefore doesn't draw anything), and not be a use after free, so > that's > > not too bad. > > Makes sense. I think it sounds like we want to hold onto a weakptr to > CompositorFrameSinkHolder in these callbacks. Done. https://codereview.chromium.org/2493223002/diff/840001/components/exo/surface.h File components/exo/surface.h (left): https://codereview.chromium.org/2493223002/diff/840001/components/exo/surface... components/exo/surface.h:121: using FrameCallback = base::Callback<void(base::TimeTicks frame_time)>; On 2016/12/07 00:46:57, reveman wrote: > Please keep this. I prefer if this was still part of the Surface API. Done. https://codereview.chromium.org/2493223002/diff/840001/components/exo/surface.h File components/exo/surface.h (right): https://codereview.chromium.org/2493223002/diff/840001/components/exo/surface... components/exo/surface.h:318: const cc::FrameSinkId frame_sink_id_; On 2016/12/07 00:46:57, reveman wrote: > Does it make sense to group this and member variables down to local_frame_id_ > together without blanklines? Done. https://codereview.chromium.org/2493223002/diff/840001/components/exo/surface... components/exo/surface.h:322: base::WeakPtr<cc::SurfaceFactory> factory_; On 2016/12/07 00:46:57, reveman wrote: > nit: surface_factory_ Done. https://codereview.chromium.org/2493223002/diff/840001/components/exo/surface... components/exo/surface.h:390: base::WeakPtrFactory<Surface> weak_ptr_factory_; On 2016/12/07 00:46:58, reveman wrote: > Can we use the SurfaceObserver interface instead of adding this? That provides > weak ptr functionality already. Done. https://codereview.chromium.org/2493223002/diff/840001/components/exo/surface... File components/exo/surface_unittest.cc (left): https://codereview.chromium.org/2493223002/diff/840001/components/exo/surface... components/exo/surface_unittest.cc:250: On 2016/12/07 00:46:58, reveman wrote: > was this intentionally removed? No. I guess it was removed along with the printf's I added/removed while debugging. https://codereview.chromium.org/2493223002/diff/840001/components/exo/surface... File components/exo/surface_unittest.cc (right): https://codereview.chromium.org/2493223002/diff/840001/components/exo/surface... components/exo/surface_unittest.cc:56: // the assertion below On 2016/12/07 00:46:58, reveman wrote: > nit: s/below/below./ Done. https://codereview.chromium.org/2493223002/diff/840001/components/exo/surface... components/exo/surface_unittest.cc:199: cc::Surface* another_surface = surface_manager->GetSurfaceForId(surface_id); On 2016/12/07 00:46:58, reveman wrote: > s/another_surface/cc_surface/ This cc::Surface was added for debugging. I have reverted to the previous state.
https://codereview.chromium.org/2493223002/diff/860001/cc/ipc/mojo_compositor... File cc/ipc/mojo_compositor_frame_sink.mojom (right): https://codereview.chromium.org/2493223002/diff/860001/cc/ipc/mojo_compositor... cc/ipc/mojo_compositor_frame_sink.mojom:49: // surface associated with the provided |local_frame_id|.addressed comments nit: Remove "addressed comments" https://codereview.chromium.org/2493223002/diff/860001/components/exo/surface.cc File components/exo/surface.cc (right): https://codereview.chromium.org/2493223002/diff/860001/components/exo/surface... components/exo/surface.cc:153: if (frame_sink_holder) braces
https://codereview.chromium.org/2493223002/diff/880001/components/exo/composi... File components/exo/compositor_frame_sink_holder.cc (right): https://codereview.chromium.org/2493223002/diff/880001/components/exo/composi... components/exo/compositor_frame_sink_holder.cc:41: *frame_callbacks); UpdateNeedsBeginFrame after this line?
https://codereview.chromium.org/2493223002/diff/880001/components/exo/composi... File components/exo/compositor_frame_sink_holder.cc (right): https://codereview.chromium.org/2493223002/diff/880001/components/exo/composi... components/exo/compositor_frame_sink_holder.cc:41: *frame_callbacks); On 2016/12/07 20:27:16, Fady Samuel wrote: > UpdateNeedsBeginFrame after this line? Done.
https://codereview.chromium.org/2493223002/diff/900001/components/exo/surface.h File components/exo/surface.h (right): https://codereview.chromium.org/2493223002/diff/900001/components/exo/surface... components/exo/surface.h:324: base::WeakPtr<cc::SurfaceFactory> surface_factory_; Delete this?
The CQ bit was checked by staraz@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: chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
On 2016/12/07 13:54:32, Fady Samuel wrote: > https://codereview.chromium.org/2493223002/diff/840001/components/exo/surface.cc > File components/exo/surface.cc (right): > > https://codereview.chromium.org/2493223002/diff/840001/components/exo/surface... > components/exo/surface.cc:766: ->SubmitCompositorFrame(local_frame_id_, > std::move(frame)); > On 2016/12/06 23:44:49, jbauman wrote: > > Does this happen synchronously, or does it post a task that's executed > > asynchronously? If the latter, I'm worried about the ordering of this with > > respect to the browser compositor commit and draw. > > > > Also, if it's asynchronous I think we have a problem if there are two surfaces > > in a synchronized subtree that need to be committed together and there are no > > layer hierarchy changes. Both surface content changes need to be committed > > atomically so we don't have a case where one is updated and the other isn't. > > This is a problem. All submits are async, and will eventually be cross-process > IPCs. You're saying there's an expectation for multiple surfaces to submit > CompositorFrames atomically? The system the surfaces system uses is to allocate > a new surface ID to do this. I think we're already doing this if > needs_commit_to_new_surface_? We only do that currently if HasLayeHierarchyChanged is true, so it won't happen if the buffer contents have changed but the layer hierarchy (layer size, surface tree, things like that) hasn't. We could do that if multiple surfaces synchronized subtree have their contents change, but that would cause an unnecessary browser compositor commit and draw in that case. Either way we could still have the issue that the SubmitCompositorFrame for the new child surface could happen after the browser compositor drew a frame referencing the new surface.
The CQ bit was checked by staraz@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/2493223002/diff/80001/components/exo/exo_comp... File components/exo/exo_compositor_frame_sink.cc (right): https://codereview.chromium.org/2493223002/diff/80001/components/exo/exo_comp... components/exo/exo_compositor_frame_sink.cc:56: if (!local_frame_id_.is_valid() /* || needs_commit_to_new_surface_*/) { On 2016/11/15 23:19:46, Fady Samuel wrote: > Remove commented out code. Done. https://codereview.chromium.org/2493223002/diff/80001/components/exo/exo_comp... components/exo/exo_compositor_frame_sink.cc:60: // UpdateSurface(true); // surface_factory.SubmitCompositorFrame happens here On 2016/11/15 23:19:46, Fady Samuel wrote: > Remove commented out code Done. https://codereview.chromium.org/2493223002/diff/180001/components/exo/exo_com... File components/exo/exo_compositor_frame_sink.h (right): https://codereview.chromium.org/2493223002/diff/180001/components/exo/exo_com... components/exo/exo_compositor_frame_sink.h:43: class ExoCompositorFrameSink : public base::RefCounted<ExoCompositorFrameSink>, On 2016/11/17 21:42:54, Fady Samuel wrote: > Does this need to be refcounted? Done. https://codereview.chromium.org/2493223002/diff/180001/components/exo/surface.h File components/exo/surface.h (left): https://codereview.chromium.org/2493223002/diff/180001/components/exo/surface... components/exo/surface.h:353: cc::SurfaceManager* surface_manager_; On 2016/11/21 16:51:37, Fady Samuel wrote: > Do we still need this? Done. https://codereview.chromium.org/2493223002/diff/180001/components/exo/surface.h File components/exo/surface.h (right): https://codereview.chromium.org/2493223002/diff/180001/components/exo/surface... components/exo/surface.h:330: scoped_refptr<ExoCompositorFrameSink> compositor_frame_sink_; On 2016/11/21 16:51:37, Fady Samuel wrote: > exo::Surface shouldn't hold a reference to ExoCompositorFrameSink directly. It > should be managed by CompositorFrameSinkHolder. Done. https://codereview.chromium.org/2493223002/diff/840001/components/exo/composi... File components/exo/compositor_frame_sink_holder.h (right): https://codereview.chromium.org/2493223002/diff/840001/components/exo/composi... components/exo/compositor_frame_sink_holder.h:49: // cc::mojom::MojoCompositorFrameSinkClient: On 2016/12/07 00:46:57, reveman wrote: > Overridden from cc::Mojo.. Done. https://codereview.chromium.org/2493223002/diff/860001/cc/ipc/mojo_compositor... File cc/ipc/mojo_compositor_frame_sink.mojom (right): https://codereview.chromium.org/2493223002/diff/860001/cc/ipc/mojo_compositor... cc/ipc/mojo_compositor_frame_sink.mojom:49: // surface associated with the provided |local_frame_id|.addressed comments On 2016/12/07 20:13:00, Fady Samuel wrote: > nit: Remove "addressed comments" Done. https://codereview.chromium.org/2493223002/diff/860001/components/exo/surface.cc File components/exo/surface.cc (right): https://codereview.chromium.org/2493223002/diff/860001/components/exo/surface... components/exo/surface.cc:153: if (frame_sink_holder) On 2016/12/07 20:13:00, Fady Samuel wrote: > braces Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/2493223002/diff/960001/components/exo/composi... File components/exo/compositor_frame_sink_holder.cc (right): https://codereview.chromium.org/2493223002/diff/960001/components/exo/composi... components/exo/compositor_frame_sink_holder.cc:74: scoped_refptr<CompositorFrameSinkHolder> holder(this); I haven't seen this pattern used elsewhere in chromium. Can you instead just make sure we don't access any member variables after running the callbacks and add a comment that makes it clear that running the callbacks has to be the last thing the functions does as they might result in the instance being destroyed? https://codereview.chromium.org/2493223002/diff/960001/components/exo/composi... File components/exo/compositor_frame_sink_holder.h (right): https://codereview.chromium.org/2493223002/diff/960001/components/exo/composi... components/exo/compositor_frame_sink_holder.h:39: bool HasReleaseCallbacksForResource(cc::ResourceId id); nit: s/HasReleaseCallbacksForResource/HasReleaseCallbackForResource/ as one callback is enough https://codereview.chromium.org/2493223002/diff/960001/components/exo/composi... components/exo/compositor_frame_sink_holder.h:81: using ResourceReleaseCallbacks = nit: ResourceReleaseCallbackMap https://codereview.chromium.org/2493223002/diff/960001/components/exo/composi... components/exo/compositor_frame_sink_holder.h:85: ResourceReleaseCallbacks release_callbacks_; and "ResourceReleaseCallbackMap release_callbacks_;" here https://codereview.chromium.org/2493223002/diff/960001/components/exo/surface.cc File components/exo/surface.cc (right): https://codereview.chromium.org/2493223002/diff/960001/components/exo/surface... components/exo/surface.cc:142: void SatisfyCallback(base::WeakPtr<CompositorFrameSinkHolder> frame_sink_holder, Can you add a Satisfy() function to the CompositorFrameSinkHolder class and bind that directly instead of having this wrapper function? https://codereview.chromium.org/2493223002/diff/960001/components/exo/surface... components/exo/surface.cc:148: void RequireCallback(base::WeakPtr<CompositorFrameSinkHolder> frame_sink_holder, Same here. Can we add a Require function to CompositorFrameSinkHolder?
https://codereview.chromium.org/2493223002/diff/960001/components/exo/composi... File components/exo/compositor_frame_sink.cc (right): https://codereview.chromium.org/2493223002/diff/960001/components/exo/composi... components/exo/compositor_frame_sink.cc:26: compositor_frame_sink->binding_ = This is really weird. Anyhow, I would recommend making this a regular binding for now, and having CompositorFrameSinkHolder hold on to a std::unique_ptr<CompositorFrameSink> for now. https://codereview.chromium.org/2493223002/diff/960001/components/exo/composi... components/exo/compositor_frame_sink.cc:37: std::unique_ptr<cc::Display>()), nullptr
The CQ bit was checked by staraz@chromium.org to run a CQ dry run
https://codereview.chromium.org/2493223002/diff/960001/components/exo/composi... File components/exo/compositor_frame_sink.cc (right): https://codereview.chromium.org/2493223002/diff/960001/components/exo/composi... components/exo/compositor_frame_sink.cc:26: compositor_frame_sink->binding_ = On 2016/12/08 13:16:56, Fady Samuel wrote: > This is really weird. Anyhow, I would recommend making this a regular binding > for now, and having CompositorFrameSinkHolder hold on to a > std::unique_ptr<CompositorFrameSink> for now. Done. https://codereview.chromium.org/2493223002/diff/960001/components/exo/composi... components/exo/compositor_frame_sink.cc:37: std::unique_ptr<cc::Display>()), On 2016/12/08 13:16:56, Fady Samuel wrote: > nullptr Done. https://codereview.chromium.org/2493223002/diff/960001/components/exo/composi... File components/exo/compositor_frame_sink_holder.cc (right): https://codereview.chromium.org/2493223002/diff/960001/components/exo/composi... components/exo/compositor_frame_sink_holder.cc:74: scoped_refptr<CompositorFrameSinkHolder> holder(this); On 2016/12/07 22:13:37, reveman wrote: > I haven't seen this pattern used elsewhere in chromium. Can you instead just > make sure we don't access any member variables after running the callbacks and > add a comment that makes it clear that running the callbacks has to be the last > thing the functions does as they might result in the instance being destroyed? Done. https://codereview.chromium.org/2493223002/diff/960001/components/exo/composi... File components/exo/compositor_frame_sink_holder.h (right): https://codereview.chromium.org/2493223002/diff/960001/components/exo/composi... components/exo/compositor_frame_sink_holder.h:39: bool HasReleaseCallbacksForResource(cc::ResourceId id); On 2016/12/07 22:13:37, reveman wrote: > nit: s/HasReleaseCallbacksForResource/HasReleaseCallbackForResource/ as one > callback is enough Done. https://codereview.chromium.org/2493223002/diff/960001/components/exo/composi... components/exo/compositor_frame_sink_holder.h:81: using ResourceReleaseCallbacks = On 2016/12/07 22:13:37, reveman wrote: > nit: ResourceReleaseCallbackMap Done. https://codereview.chromium.org/2493223002/diff/960001/components/exo/composi... components/exo/compositor_frame_sink_holder.h:85: ResourceReleaseCallbacks release_callbacks_; On 2016/12/07 22:13:37, reveman wrote: > and "ResourceReleaseCallbackMap release_callbacks_;" here Done. https://codereview.chromium.org/2493223002/diff/960001/components/exo/surface.cc File components/exo/surface.cc (right): https://codereview.chromium.org/2493223002/diff/960001/components/exo/surface... components/exo/surface.cc:142: void SatisfyCallback(base::WeakPtr<CompositorFrameSinkHolder> frame_sink_holder, On 2016/12/07 22:13:37, reveman wrote: > Can you add a Satisfy() function to the CompositorFrameSinkHolder class and bind > that directly instead of having this wrapper function? Done. https://codereview.chromium.org/2493223002/diff/960001/components/exo/surface... components/exo/surface.cc:148: void RequireCallback(base::WeakPtr<CompositorFrameSinkHolder> frame_sink_holder, On 2016/12/07 22:13:37, reveman wrote: > Same here. Can we add a Require function to CompositorFrameSinkHolder? Done.
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/2493223002/diff/980001/components/exo/composi... File components/exo/compositor_frame_sink_holder.cc (right): https://codereview.chromium.org/2493223002/diff/980001/components/exo/composi... components/exo/compositor_frame_sink_holder.cc:22: mojo_compositor_frame_sink_(std::move(mojo_compositor_frame_sink)), Remove this. It's weird to have a lingering interface pointer that isn't used for anything. https://codereview.chromium.org/2493223002/diff/980001/components/exo/surface.cc File components/exo/surface.cc (right): https://codereview.chromium.org/2493223002/diff/980001/components/exo/surface... components/exo/surface.cc:152: cc::mojom::MojoCompositorFrameSinkPtr frame_sink_ptr; Delete this?
lgtm after explaining why it's now safe to access release_callbacks_ after running the callback https://codereview.chromium.org/2493223002/diff/960001/components/exo/composi... File components/exo/compositor_frame_sink_holder.cc (right): https://codereview.chromium.org/2493223002/diff/960001/components/exo/composi... components/exo/compositor_frame_sink_holder.cc:74: scoped_refptr<CompositorFrameSinkHolder> holder(this); On 2016/12/09 at 16:16:27, StarAZ1 wrote: > On 2016/12/07 22:13:37, reveman wrote: > > I haven't seen this pattern used elsewhere in chromium. Can you instead just > > make sure we don't access any member variables after running the callbacks and > > add a comment that makes it clear that running the callbacks has to be the last > > thing the functions does as they might result in the instance being destroyed? > > Done. I'm still seeing release_callbacks_ accessed below the callback. Is that not a problem anymore? Can you also explain how these callbacks can result in the instance being destroyed? Its surprising that someone can call this and have a reference to the object but then it's destroyed before the function call returns.
https://codereview.chromium.org/2493223002/diff/960001/components/exo/composi... File components/exo/compositor_frame_sink_holder.cc (right): https://codereview.chromium.org/2493223002/diff/960001/components/exo/composi... components/exo/compositor_frame_sink_holder.cc:74: scoped_refptr<CompositorFrameSinkHolder> holder(this); On 2016/12/09 16:33:59, reveman wrote: > On 2016/12/09 at 16:16:27, StarAZ1 wrote: > > On 2016/12/07 22:13:37, reveman wrote: > > > I haven't seen this pattern used elsewhere in chromium. Can you instead just > > > make sure we don't access any member variables after running the callbacks > and > > > add a comment that makes it clear that running the callbacks has to be the > last > > > thing the functions does as they might result in the instance being > destroyed? > > > > Done. > > I'm still seeing release_callbacks_ accessed below the callback. Is that not a > problem anymore? Can you also explain how these callbacks can result in the > instance being destroyed? Its surprising that someone can call this and have a > reference to the object but then it's destroyed before the function call > returns. I am moving the release callback to a local variable before erasing it from the map. https://codereview.chromium.org/2493223002/diff/980001/components/exo/composi... File components/exo/compositor_frame_sink_holder.cc (right): https://codereview.chromium.org/2493223002/diff/980001/components/exo/composi... components/exo/compositor_frame_sink_holder.cc:22: mojo_compositor_frame_sink_(std::move(mojo_compositor_frame_sink)), On 2016/12/09 16:29:18, Fady Samuel wrote: > Remove this. It's weird to have a lingering interface pointer that isn't used > for anything. Done. https://codereview.chromium.org/2493223002/diff/980001/components/exo/surface.cc File components/exo/surface.cc (right): https://codereview.chromium.org/2493223002/diff/980001/components/exo/surface... components/exo/surface.cc:152: cc::mojom::MojoCompositorFrameSinkPtr frame_sink_ptr; On 2016/12/09 16:29:19, Fady Samuel wrote: > Delete this? Done.
staraz@chromium.org changed reviewers: + sky@chromium.org
sky@chromium.org: Please review changes in ash_test_helper.cc and added dependency in BUILD.gn and DEPS files.
LGTM
Please update the CL description to better represent your final changes.
The CQ bit was checked by staraz@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 ========== Change exo::SurfaceFactoryOwner to exo::ExoCompositorFrameSink Eventually, this CL should achieve the following 3 goals: 1. Moved exo::SurfaceFactoryOwner to its own file and renamed it to exo::ExoCompositorFrameSink. 2. ExoCompositorFrameSink implements cc::mojom::MojoCompositorFrameSink. 3. ExoCompositorFrameSink doesn't know about exo::Surface. BUG=659601 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== Moved exo::SurfaceFactoryOwner to its own file and renamed it to exo::CompositorFrameSink. CompositorFrameSink implements cc::mojom::MojoCompositorFrameSink. CompositorFrameSink is no longer a friend class of exo::Surface. Added exo::CompositorFrameSinkHolder class that implements cc::mojom::MojoCompositorFrameSinkClient. BUG=659601 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
The CQ bit was checked by staraz@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from fsamuel@chromium.org, reveman@chromium.org, sky@chromium.org Link to the patchset: https://codereview.chromium.org/2493223002/#ps1050001 (title: "Added comments to explain the reset()")
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": 1050001, "attempt_start_ts": 1481321627738710,
"parent_rev": "c4723fa73a43cd8072fa7dcf35d28787863d8eae", "commit_rev":
"63273f431d335e1c6b59d99e70c1849f046f05f2"}
Message was sent while issue was closed.
Description was changed from ========== Moved exo::SurfaceFactoryOwner to its own file and renamed it to exo::CompositorFrameSink. CompositorFrameSink implements cc::mojom::MojoCompositorFrameSink. CompositorFrameSink is no longer a friend class of exo::Surface. Added exo::CompositorFrameSinkHolder class that implements cc::mojom::MojoCompositorFrameSinkClient. BUG=659601 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== Moved exo::SurfaceFactoryOwner to its own file and renamed it to exo::CompositorFrameSink. CompositorFrameSink implements cc::mojom::MojoCompositorFrameSink. CompositorFrameSink is no longer a friend class of exo::Surface. Added exo::CompositorFrameSinkHolder class that implements cc::mojom::MojoCompositorFrameSinkClient. BUG=659601 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel Review-Url: https://codereview.chromium.org/2493223002 ==========
Message was sent while issue was closed.
Committed patchset #54 (id:1050001)
Message was sent while issue was closed.
A revert of this CL (patchset #54 id:1050001) has been created in https://codereview.chromium.org/2566813002/ by sadrul@chromium.org. The reason for reverting is: Breaks test (DesktopMediaListAshTest.ScreenOnly) in chromeos-ozone builds..
Message was sent while issue was closed.
On 2016/12/10 04:45:02, sadrul wrote: > A revert of this CL (patchset #54 id:1050001) has been created in > https://codereview.chromium.org/2566813002/ by mailto:sadrul@chromium.org. > > The reason for reverting is: Breaks test (DesktopMediaListAshTest.ScreenOnly) in > chromeos-ozone builds.. Link to a failure: https://build.chromium.org/p/chromium.chromiumos/builders/Linux%20ChromiumOS%... Log from failure: DesktopMediaListAshTest.ScreenOnly (run #1): [ RUN ] DesktopMediaListAshTest.ScreenOnly [17120:17120:1209/172302.331434:4962087224:FATAL:shell.cc(245)] Check failed: HasInstance(). #0 0x000004561b2e base::debug::StackTrace::StackTrace() #1 0x000004573b6a logging::LogMessage::~LogMessage() #2 0x0000059db851 ash::Shell::GetAllRootWindows() #3 0x0000043e8c51 DesktopMediaListAsh::EnumerateSources() #4 0x0000043e8b8e DesktopMediaListAsh::Refresh() #5 0x0000045e11ee base::debug::TaskAnnotator::RunTask() #6 0x000004579a6d base::MessageLoop::RunTask() #7 0x00000457a30e base::MessageLoop::DoDelayedWork() #8 0x00000457b6dd base::MessagePumpLibevent::Run() #9 0x0000045797a8 base::MessageLoop::RunHandler() #10 0x000004596d2e base::RunLoop::Run() #11 0x0000073862a7 ash::test::AshTestHelper::TearDown() #12 0x0000073859e7 ash::test::AshTestBase::TearDown() #13 0x000003c58bd0 testing::TestInfo::Run() #14 0x000003c59067 testing::TestCase::Run() #15 0x000003c60357 testing::internal::UnitTestImpl::RunAllTests() #16 0x000003c5ff6a testing::UnitTest::Run() #17 0x0000039b98c1 base::TestSuite::Run() #18 0x0000039bad74 base::LaunchUnitTests() #19 0x0000039b5611 main #20 0x7f219fd2b7ed __libc_start_main #21 0x0000006a5541 <unknown>
Message was sent while issue was closed.
sadrul@chromium.org changed reviewers: + sadrul@chromium.org
Message was sent while issue was closed.
https://codereview.chromium.org/2493223002/diff/1050001/chrome/browser/media/... File chrome/browser/media/webrtc/desktop_media_list_ash_unittest.cc (right): https://codereview.chromium.org/2493223002/diff/1050001/chrome/browser/media/... chrome/browser/media/webrtc/desktop_media_list_ash_unittest.cc:48: I think you need to override TearDown() here: void TearDown() override { list_.reset(); AshTestBase::TearDown(); } So that |list_| is destroyed before AshTestBase::TearDown() goes into a nested message-loop cycle (and it happens for all tests). Also, AshTestBase::TearDown() has a RunAllPendingInMessageLoop(). Are you sure you need to do it again in AshTestHelper::TearDown()?
Message was sent while issue was closed.
https://codereview.chromium.org/2493223002/diff/1050001/chrome/browser/media/... File chrome/browser/media/webrtc/desktop_media_list_ash_unittest.cc (right): https://codereview.chromium.org/2493223002/diff/1050001/chrome/browser/media/... chrome/browser/media/webrtc/desktop_media_list_ash_unittest.cc:48: On 2016/12/10 05:04:43, sadrul wrote: > I think you need to override TearDown() here: > > void TearDown() override { > list_.reset(); > AshTestBase::TearDown(); > } > > So that |list_| is destroyed before AshTestBase::TearDown() goes into a nested > message-loop cycle (and it happens for all tests). > > Also, AshTestBase::TearDown() has a RunAllPendingInMessageLoop(). Are you sure > you need to do it again in AshTestHelper::TearDown()? Yes, there needs to be a RenAllPendingInMessageLoop() after Shell::DeleteInstance(). Done.
Message was sent while issue was closed.
Description was changed from ========== Moved exo::SurfaceFactoryOwner to its own file and renamed it to exo::CompositorFrameSink. CompositorFrameSink implements cc::mojom::MojoCompositorFrameSink. CompositorFrameSink is no longer a friend class of exo::Surface. Added exo::CompositorFrameSinkHolder class that implements cc::mojom::MojoCompositorFrameSinkClient. BUG=659601 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel Review-Url: https://codereview.chromium.org/2493223002 ========== to ========== Moved exo::SurfaceFactoryOwner to its own file and renamed it to exo::CompositorFrameSink. CompositorFrameSink implements cc::mojom::MojoCompositorFrameSink. CompositorFrameSink is no longer a friend class of exo::Surface. Added exo::CompositorFrameSinkHolder class that implements cc::mojom::MojoCompositorFrameSinkClient. BUG=659601 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel Review-Url: https://codereview.chromium.org/2493223002 ==========
The CQ bit was checked by staraz@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_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by staraz@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_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by staraz@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from fsamuel@chromium.org, sky@chromium.org, reveman@chromium.org Link to the patchset: https://codereview.chromium.org/2493223002/#ps1070001 (title: "Override DesktopMediaListAshTest::TearDown() to reset list_")
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": 1070001, "attempt_start_ts": 1481410376199910,
"parent_rev": "a6d0bd73f65a139a6278658ce4f7603ea0208a97", "commit_rev":
"3f2cf22f5d292c192963b11a66ddd217b9418a36"}
Message was sent while issue was closed.
Description was changed from ========== Moved exo::SurfaceFactoryOwner to its own file and renamed it to exo::CompositorFrameSink. CompositorFrameSink implements cc::mojom::MojoCompositorFrameSink. CompositorFrameSink is no longer a friend class of exo::Surface. Added exo::CompositorFrameSinkHolder class that implements cc::mojom::MojoCompositorFrameSinkClient. BUG=659601 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel Review-Url: https://codereview.chromium.org/2493223002 ========== to ========== Moved exo::SurfaceFactoryOwner to its own file and renamed it to exo::CompositorFrameSink. CompositorFrameSink implements cc::mojom::MojoCompositorFrameSink. CompositorFrameSink is no longer a friend class of exo::Surface. Added exo::CompositorFrameSinkHolder class that implements cc::mojom::MojoCompositorFrameSinkClient. BUG=659601 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel Review-Url: https://codereview.chromium.org/2493223002 Review-Url: https://codereview.chromium.org/2493223002 ==========
Message was sent while issue was closed.
Committed patchset #55 (id:1070001)
Message was sent while issue was closed.
A revert of this CL (patchset #55 id:1070001) has been created in https://codereview.chromium.org/2562263002/ by yhirano@chromium.org. The reason for reverting is: Speculative revert for a chrome_public_test_apk breakage. https://build.chromium.org/p/chromium.linux/builders/Android%20Tests%20%28dbg....
Message was sent while issue was closed.
Description was changed from ========== Moved exo::SurfaceFactoryOwner to its own file and renamed it to exo::CompositorFrameSink. CompositorFrameSink implements cc::mojom::MojoCompositorFrameSink. CompositorFrameSink is no longer a friend class of exo::Surface. Added exo::CompositorFrameSinkHolder class that implements cc::mojom::MojoCompositorFrameSinkClient. BUG=659601 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel Review-Url: https://codereview.chromium.org/2493223002 Review-Url: https://codereview.chromium.org/2493223002 ========== to ========== Moved exo::SurfaceFactoryOwner to its own file and renamed it to exo::CompositorFrameSink. CompositorFrameSink implements cc::mojom::MojoCompositorFrameSink. CompositorFrameSink is no longer a friend class of exo::Surface. Added exo::CompositorFrameSinkHolder class that implements cc::mojom::MojoCompositorFrameSinkClient. BUG=659601 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel Committed: https://crrev.com/f9fcdd795898705903130024a995ae6fc644ecde Cr-Commit-Position: refs/heads/master@{#437705} ==========
Message was sent while issue was closed.
Patchset 55 (id:??) landed as https://crrev.com/f9fcdd795898705903130024a995ae6fc644ecde Cr-Commit-Position: refs/heads/master@{#437705}
Message was sent while issue was closed.
Description was changed from ========== Moved exo::SurfaceFactoryOwner to its own file and renamed it to exo::CompositorFrameSink. CompositorFrameSink implements cc::mojom::MojoCompositorFrameSink. CompositorFrameSink is no longer a friend class of exo::Surface. Added exo::CompositorFrameSinkHolder class that implements cc::mojom::MojoCompositorFrameSinkClient. BUG=659601 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel Committed: https://crrev.com/f9fcdd795898705903130024a995ae6fc644ecde Cr-Commit-Position: refs/heads/master@{#437705} ========== to ========== Moved exo::SurfaceFactoryOwner to its own file and renamed it to exo::CompositorFrameSink. CompositorFrameSink implements cc::mojom::MojoCompositorFrameSink. CompositorFrameSink is no longer a friend class of exo::Surface. Added exo::CompositorFrameSinkHolder class that implements cc::mojom::MojoCompositorFrameSinkClient. BUG=659601 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel Review-Url: https://codereview.chromium.org/2493223002 Committed: https://crrev.com/d8f4f642d72fc5fdb428f5a90ca207c2143a6f79 Cr-Commit-Position: refs/heads/master@{#437780} ==========
Message was sent while issue was closed.
Patchset 55 (id:??) landed as https://crrev.com/d8f4f642d72fc5fdb428f5a90ca207c2143a6f79 Cr-Commit-Position: refs/heads/master@{#437780} |
