Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2890913002:
Add CompositorFrameSinkClientBinding To Be Used By FrameGenerator (Closed)
DescriptionAdd CompositorFrameSinkClientBinding To Be Used By FrameGenerator
Added CompositorFrameSinkClientBinding interface.
CompositorFrameSinkClientBinding's default implementation holds interface
pointers to DisplayPrivate and MojoCompositorFrameSink. It binds FrameGenerator
to a MojoCompositorFrameSinkClient request. PlatformDisplayDefault sets up the
mojo connections before passing them to the binding object.
FrameGenerator unit tests use a mock of CompositorFrameSinkClientBinding to keep
track of the submitted frames.
FrameGenerator no longer uses cc::CompositorFrameSink after this CL.
cc::CompositorFrameSink should not be used by anything other than LayerTreeHost.
BUG=721810
Review-Url: https://codereview.chromium.org/2890913002
Cr-Commit-Position: refs/heads/master@{#473231}
Committed: https://chromium.googlesource.com/chromium/src/+/0bcca1e6eb7d275c808c26a37f0878e0b66be1d9
Patch Set 1 #Patch Set 2 : virtual destructor #
Total comments: 8
Patch Set 3 : Address comments #
Total comments: 10
Patch Set 4 : Address Comments In Unit Tests #Patch Set 5 : BeginFrameDidNotSwap #Patch Set 6 : BeginFrameDidNotSwap #
Total comments: 20
Patch Set 7 : Address Comments In Tests #
Total comments: 14
Patch Set 8 : Address Comments #Patch Set 9 : LastBeginFrameAck() #
Total comments: 3
Patch Set 10 : Implements MojoCFS #Patch Set 11 : Cleanup #Patch Set 12 : More Cleanup #Patch Set 13 : FrameGenerator::SetNeedsBeginFrame #
Total comments: 6
Patch Set 14 : Address Comments #
Total comments: 10
Patch Set 15 : Address nits #
Total comments: 2
Patch Set 16 : Remove redundant SetNeedsBeginFrame #Messages
Total messages: 69 (38 generated)
The CQ bit was checked by staraz@chromium.org to run a CQ dry run
staraz@chromium.org changed reviewers: + sky@chromium.org
sky@: PTAL. Thank you!
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
fsamuel@chromium.org changed reviewers: + fsamuel@chromium.org
https://codereview.chromium.org/2890913002/diff/20001/services/ui/ws/platform... File services/ui/ws/platform_display_default.cc (left): https://codereview.chromium.org/2890913002/diff/20001/services/ui/ws/platform... services/ui/ws/platform_display_default.cc:266: base::MakeUnique<DisplayClientCompositorFrameSink>( Do you still need DisplayClientCompositorFrameSink or can you delete it?
https://codereview.chromium.org/2890913002/diff/20001/services/ui/ws/frame_ge... File services/ui/ws/frame_generator.h (right): https://codereview.chromium.org/2890913002/diff/20001/services/ui/ws/frame_ge... services/ui/ws/frame_generator.h:66: cc::LocalSurfaceIdAllocator id_allocator_; Nothing uses these? https://codereview.chromium.org/2890913002/diff/20001/services/ui/ws/frame_ge... services/ui/ws/frame_generator.h:68: gfx::Size last_submitted_frame_size_; Nothing uses this? https://codereview.chromium.org/2890913002/diff/20001/services/ui/ws/frame_ge... File services/ui/ws/frame_generator_unittest.cc (right): https://codereview.chromium.org/2890913002/diff/20001/services/ui/ws/frame_ge... services/ui/ws/frame_generator_unittest.cc:74: MOCK_METHOD0(SubmitCompositorFrameInternal, void()); I think we prefer not to use GMOCK in ws code?
https://codereview.chromium.org/2890913002/diff/20001/services/ui/ws/frame_ge... File services/ui/ws/frame_generator.h (right): https://codereview.chromium.org/2890913002/diff/20001/services/ui/ws/frame_ge... services/ui/ws/frame_generator.h:66: cc::LocalSurfaceIdAllocator id_allocator_; On 2017/05/17 20:11:43, Fady Samuel wrote: > Nothing uses these? Done. https://codereview.chromium.org/2890913002/diff/20001/services/ui/ws/frame_ge... services/ui/ws/frame_generator.h:68: gfx::Size last_submitted_frame_size_; On 2017/05/17 20:11:43, Fady Samuel wrote: > Nothing uses this? Done. https://codereview.chromium.org/2890913002/diff/20001/services/ui/ws/frame_ge... File services/ui/ws/frame_generator_unittest.cc (right): https://codereview.chromium.org/2890913002/diff/20001/services/ui/ws/frame_ge... services/ui/ws/frame_generator_unittest.cc:74: MOCK_METHOD0(SubmitCompositorFrameInternal, void()); On 2017/05/17 20:11:43, Fady Samuel wrote: > I think we prefer not to use GMOCK in ws code? The frames_submitted_ is restored. Is there any reason for this preference? https://codereview.chromium.org/2890913002/diff/20001/services/ui/ws/platform... File services/ui/ws/platform_display_default.cc (left): https://codereview.chromium.org/2890913002/diff/20001/services/ui/ws/platform... services/ui/ws/platform_display_default.cc:266: base::MakeUnique<DisplayClientCompositorFrameSink>( On 2017/05/17 20:08:18, Fady Samuel wrote: > Do you still need DisplayClientCompositorFrameSink or can you delete it? Done.
https://codereview.chromium.org/2890913002/diff/40001/services/ui/ws/composit... File services/ui/ws/compositor_frame_sink_client_binding.h (right): https://codereview.chromium.org/2890913002/diff/40001/services/ui/ws/composit... services/ui/ws/compositor_frame_sink_client_binding.h:23: class CompositorFrameSinkClientBinding { What if this implements cc::mojom::MojoCompositorFrameSink? That way, the binding API never goes stale... https://codereview.chromium.org/2890913002/diff/40001/services/ui/ws/frame_ge... File services/ui/ws/frame_generator.cc (right): https://codereview.chromium.org/2890913002/diff/40001/services/ui/ws/frame_ge... services/ui/ws/frame_generator.cc:83: if (begin_frame_args.type == cc::BeginFrameArgs::MISSED) I think you need to add a BeginFrameDidNotSwap to the binding? https://codereview.chromium.org/2890913002/diff/40001/services/ui/ws/frame_ge... File services/ui/ws/frame_generator_unittest.cc (left): https://codereview.chromium.org/2890913002/diff/40001/services/ui/ws/frame_ge... services/ui/ws/frame_generator_unittest.cc:347: EXPECT_EQ(2, NumberOfFramesReceived()); If you restore this method then you don't need this change? https://codereview.chromium.org/2890913002/diff/40001/services/ui/ws/frame_ge... File services/ui/ws/frame_generator_unittest.cc (right): https://codereview.chromium.org/2890913002/diff/40001/services/ui/ws/frame_ge... services/ui/ws/frame_generator_unittest.cc:288: TestClientBinding* binding = test_client_binding.get(); This pattern seems to be used over and over again... Can we simply move this to InitWithSurfaceInfo?
https://codereview.chromium.org/2890913002/diff/40001/services/ui/ws/composit... File services/ui/ws/compositor_frame_sink_client_binding.h (right): https://codereview.chromium.org/2890913002/diff/40001/services/ui/ws/composit... services/ui/ws/compositor_frame_sink_client_binding.h:23: class CompositorFrameSinkClientBinding { On 2017/05/17 20:33:04, Fady Samuel wrote: > What if this implements cc::mojom::MojoCompositorFrameSink? That way, the > binding API never goes stale... Would it be weird if there is a MojoCFS implementation on the client side? I'm fine with it otherwise.
https://codereview.chromium.org/2890913002/diff/40001/services/ui/ws/composit... File services/ui/ws/compositor_frame_sink_client_binding.h (right): https://codereview.chromium.org/2890913002/diff/40001/services/ui/ws/composit... services/ui/ws/compositor_frame_sink_client_binding.h:23: class CompositorFrameSinkClientBinding { On 2017/05/17 20:33:04, Fady Samuel wrote: > What if this implements cc::mojom::MojoCompositorFrameSink? That way, the > binding API never goes stale... A few more concerns: ClientBinding starts to seem redundant once it implements MojoCFS: it doesn't have anything in addition to MojoCFS methods. The Default implementation handles LocalSurfaceId allocation (ClientBinding::SubmitCompositorFrame doesn't take in a LocalSurfaceId). DisplayPrivate::ResizeDisplay and SetLocalSurfaceId need to be called when we allocate a new LocalSurfaceId. If we move the LocalSurfaceId allocation back to FrameGenerator, either FrameGenerator needs to know about DisplayPrivate (which defeats the point of this CL) or we store a last_submitted_local_surface_id_ in both FrameGenerator and ClientBinding. WDYT? https://codereview.chromium.org/2890913002/diff/40001/services/ui/ws/frame_ge... File services/ui/ws/frame_generator_unittest.cc (left): https://codereview.chromium.org/2890913002/diff/40001/services/ui/ws/frame_ge... services/ui/ws/frame_generator_unittest.cc:347: EXPECT_EQ(2, NumberOfFramesReceived()); On 2017/05/17 20:33:04, Fady Samuel wrote: > If you restore this method then you don't need this change? Done. https://codereview.chromium.org/2890913002/diff/40001/services/ui/ws/frame_ge... File services/ui/ws/frame_generator_unittest.cc (right): https://codereview.chromium.org/2890913002/diff/40001/services/ui/ws/frame_ge... services/ui/ws/frame_generator_unittest.cc:288: TestClientBinding* binding = test_client_binding.get(); On 2017/05/17 20:33:04, Fady Samuel wrote: > This pattern seems to be used over and over again... Can we simply move this to > InitWithSurfaceInfo? Done.
The CQ bit was checked by staraz@chromium.org to run a CQ dry run
https://codereview.chromium.org/2890913002/diff/40001/services/ui/ws/frame_ge... File services/ui/ws/frame_generator.cc (right): https://codereview.chromium.org/2890913002/diff/40001/services/ui/ws/frame_ge... services/ui/ws/frame_generator.cc:83: if (begin_frame_args.type == cc::BeginFrameArgs::MISSED) On 2017/05/17 20:33:04, Fady Samuel wrote: > I think you need to add a BeginFrameDidNotSwap to the binding? Done.
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/2890913002/diff/40001/services/ui/ws/composit... File services/ui/ws/compositor_frame_sink_client_binding.h (right): https://codereview.chromium.org/2890913002/diff/40001/services/ui/ws/composit... services/ui/ws/compositor_frame_sink_client_binding.h:23: class CompositorFrameSinkClientBinding { On 2017/05/17 20:52:01, Alex Z. wrote: > On 2017/05/17 20:33:04, Fady Samuel wrote: > > What if this implements cc::mojom::MojoCompositorFrameSink? That way, the > > binding API never goes stale... > > Would it be weird if there is a MojoCFS implementation on the client > side? I'm fine with it otherwise. Not at all. The InterfacePtr probably also implements the MojoCompositorFrameSInk interface. https://codereview.chromium.org/2890913002/diff/40001/services/ui/ws/composit... services/ui/ws/compositor_frame_sink_client_binding.h:23: class CompositorFrameSinkClientBinding { On 2017/05/17 21:20:24, Alex Z. wrote: > On 2017/05/17 20:33:04, Fady Samuel wrote: > > What if this implements cc::mojom::MojoCompositorFrameSink? That way, the > > binding API never goes stale... > > A few more concerns: > ClientBinding starts to seem redundant once it implements MojoCFS: it > doesn't have anything in addition to MojoCFS methods. > > The Default implementation handles LocalSurfaceId allocation > (ClientBinding::SubmitCompositorFrame doesn't take in a > LocalSurfaceId). DisplayPrivate::ResizeDisplay and SetLocalSurfaceId > need to be called when we allocate a new LocalSurfaceId. If we move > the LocalSurfaceId allocation back to FrameGenerator, either > FrameGenerator needs to know about DisplayPrivate (which defeats the > point of this CL) or we store a last_submitted_local_surface_id_ in > both FrameGenerator and ClientBinding. > > WDYT? Yea, we store the LocalSurfaceId in both places (client and service-side) in Chrome too. I think that's fine. Don't expose DisplayPrivate to FrameGenerator though. https://codereview.chromium.org/2890913002/diff/100001/services/ui/ws/frame_g... File services/ui/ws/frame_generator_unittest.cc (left): https://codereview.chromium.org/2890913002/diff/100001/services/ui/ws/frame_g... services/ui/ws/frame_generator_unittest.cc:211: // After SetUP(), frame_generator() has its |is_window_visible_| set to true Why delete this? https://codereview.chromium.org/2890913002/diff/100001/services/ui/ws/frame_g... services/ui/ws/frame_generator_unittest.cc:220: EXPECT_EQ(0, NumberOfFramesReceived()); Why delete this? https://codereview.chromium.org/2890913002/diff/100001/services/ui/ws/frame_g... services/ui/ws/frame_generator_unittest.cc:269: IssueBeginFrame(); Are we losing part of the test? https://codereview.chromium.org/2890913002/diff/100001/services/ui/ws/frame_g... services/ui/ws/frame_generator_unittest.cc:274: EXPECT_EQ(1, NumberOfFramesReceived()); Why delete this? https://codereview.chromium.org/2890913002/diff/100001/services/ui/ws/frame_g... services/ui/ws/frame_generator_unittest.cc:280: EXPECT_EQ(1, NumberOfFramesReceived()); Why delete this? https://codereview.chromium.org/2890913002/diff/100001/services/ui/ws/frame_g... services/ui/ws/frame_generator_unittest.cc:339: EXPECT_EQ(2, NumberOfFramesReceived()); Why delete this? https://codereview.chromium.org/2890913002/diff/100001/services/ui/ws/frame_g... File services/ui/ws/frame_generator_unittest.cc (right): https://codereview.chromium.org/2890913002/diff/100001/services/ui/ws/frame_g... services/ui/ws/frame_generator_unittest.cc:7: #include "base/debug/stack_trace.h" Delete this. https://codereview.chromium.org/2890913002/diff/100001/services/ui/ws/frame_g... services/ui/ws/frame_generator_unittest.cc:187: frame_generator()->Bind(std::move(test_client_binding)); Why can't this be part of the setup? https://codereview.chromium.org/2890913002/diff/100001/services/ui/ws/frame_g... services/ui/ws/frame_generator_unittest.cc:198: frame_generator()->OnWindowSizeChanged(gfx::Size(1, 2)); Why introduce this? https://codereview.chromium.org/2890913002/diff/100001/services/ui/ws/frame_g... services/ui/ws/frame_generator_unittest.cc:212: EXPECT_EQ(expected_ack, LastBeginFrameAck(binding)); Maybe store binding in FrameGeneratorTest instead?
https://codereview.chromium.org/2890913002/diff/40001/services/ui/ws/composit... File services/ui/ws/compositor_frame_sink_client_binding.h (right): https://codereview.chromium.org/2890913002/diff/40001/services/ui/ws/composit... services/ui/ws/compositor_frame_sink_client_binding.h:23: class CompositorFrameSinkClientBinding { On 2017/05/17 21:20:24, Alex Z. wrote: > On 2017/05/17 20:33:04, Fady Samuel wrote: > > What if this implements cc::mojom::MojoCompositorFrameSink? That way, the > > binding API never goes stale... > > A few more concerns: > ClientBinding starts to seem redundant once it implements MojoCFS: it > doesn't have anything in addition to MojoCFS methods. > > The Default implementation handles LocalSurfaceId allocation > (ClientBinding::SubmitCompositorFrame doesn't take in a > LocalSurfaceId). DisplayPrivate::ResizeDisplay and SetLocalSurfaceId > need to be called when we allocate a new LocalSurfaceId. If we move > the LocalSurfaceId allocation back to FrameGenerator, either > FrameGenerator needs to know about DisplayPrivate (which defeats the > point of this CL) or we store a last_submitted_local_surface_id_ in > both FrameGenerator and ClientBinding. > > WDYT? Yea, we store the LocalSurfaceId in both places (client and service-side) in Chrome too. I think that's fine. Don't expose DisplayPrivate to FrameGenerator though. https://codereview.chromium.org/2890913002/diff/100001/services/ui/ws/frame_g... File services/ui/ws/frame_generator_unittest.cc (left): https://codereview.chromium.org/2890913002/diff/100001/services/ui/ws/frame_g... services/ui/ws/frame_generator_unittest.cc:211: // After SetUP(), frame_generator() has its |is_window_visible_| set to true Why delete this? https://codereview.chromium.org/2890913002/diff/100001/services/ui/ws/frame_g... services/ui/ws/frame_generator_unittest.cc:220: EXPECT_EQ(0, NumberOfFramesReceived()); Why delete this? https://codereview.chromium.org/2890913002/diff/100001/services/ui/ws/frame_g... services/ui/ws/frame_generator_unittest.cc:269: IssueBeginFrame(); Are we losing part of the test? https://codereview.chromium.org/2890913002/diff/100001/services/ui/ws/frame_g... services/ui/ws/frame_generator_unittest.cc:274: EXPECT_EQ(1, NumberOfFramesReceived()); Why delete this? https://codereview.chromium.org/2890913002/diff/100001/services/ui/ws/frame_g... services/ui/ws/frame_generator_unittest.cc:280: EXPECT_EQ(1, NumberOfFramesReceived()); Why delete this? https://codereview.chromium.org/2890913002/diff/100001/services/ui/ws/frame_g... services/ui/ws/frame_generator_unittest.cc:339: EXPECT_EQ(2, NumberOfFramesReceived()); Why delete this? https://codereview.chromium.org/2890913002/diff/100001/services/ui/ws/frame_g... File services/ui/ws/frame_generator_unittest.cc (right): https://codereview.chromium.org/2890913002/diff/100001/services/ui/ws/frame_g... services/ui/ws/frame_generator_unittest.cc:7: #include "base/debug/stack_trace.h" Delete this. https://codereview.chromium.org/2890913002/diff/100001/services/ui/ws/frame_g... services/ui/ws/frame_generator_unittest.cc:187: frame_generator()->Bind(std::move(test_client_binding)); Why can't this be part of the setup? https://codereview.chromium.org/2890913002/diff/100001/services/ui/ws/frame_g... services/ui/ws/frame_generator_unittest.cc:198: frame_generator()->OnWindowSizeChanged(gfx::Size(1, 2)); Why introduce this? https://codereview.chromium.org/2890913002/diff/100001/services/ui/ws/frame_g... services/ui/ws/frame_generator_unittest.cc:212: EXPECT_EQ(expected_ack, LastBeginFrameAck(binding)); Maybe store binding in FrameGeneratorTest instead?
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...
Thanks for taking the time, Fady! I have addressed your comments in frame_generator_unittest.cc. I'm working on the binding's implementing MojoCFS next. PTAL. https://codereview.chromium.org/2890913002/diff/100001/services/ui/ws/frame_g... File services/ui/ws/frame_generator_unittest.cc (left): https://codereview.chromium.org/2890913002/diff/100001/services/ui/ws/frame_g... services/ui/ws/frame_generator_unittest.cc:211: // After SetUP(), frame_generator() has its |is_window_visible_| set to true On 2017/05/18 11:55:28, Fady Samuel wrote: > Why delete this? It's restored. It was deleted because there wasn't much to test when the test bindging was using gmock. https://codereview.chromium.org/2890913002/diff/100001/services/ui/ws/frame_g... services/ui/ws/frame_generator_unittest.cc:220: EXPECT_EQ(0, NumberOfFramesReceived()); On 2017/05/18 11:55:28, Fady Samuel wrote: > Why delete this? It's moved to InitWithSurfaceInfo in the next patch. https://codereview.chromium.org/2890913002/diff/100001/services/ui/ws/frame_g... services/ui/ws/frame_generator_unittest.cc:269: IssueBeginFrame(); On 2017/05/18 11:55:28, Fady Samuel wrote: > Are we losing part of the test? This is now in InitWithSurfaceInfo. It's fine since the SurfaceInfo part is also tested in OnSurfaceCreated test. https://codereview.chromium.org/2890913002/diff/100001/services/ui/ws/frame_g... services/ui/ws/frame_generator_unittest.cc:274: EXPECT_EQ(1, NumberOfFramesReceived()); On 2017/05/18 11:55:28, Fady Samuel wrote: > Why delete this? The "FrameGenerator stopping requesting BeginFrames after submitting a CompositorFrame" is tested in OnSurfaceCreated test. https://codereview.chromium.org/2890913002/diff/100001/services/ui/ws/frame_g... services/ui/ws/frame_generator_unittest.cc:280: EXPECT_EQ(1, NumberOfFramesReceived()); On 2017/05/18 11:55:28, Fady Samuel wrote: > Why delete this? Thanks for pointing it out. It was removed when I used gmock. It's restored now. https://codereview.chromium.org/2890913002/diff/100001/services/ui/ws/frame_g... services/ui/ws/frame_generator_unittest.cc:339: EXPECT_EQ(2, NumberOfFramesReceived()); On 2017/05/18 11:55:28, Fady Samuel wrote: > Why delete this? It was deleted while converting to use gmock. It's restored. https://codereview.chromium.org/2890913002/diff/100001/services/ui/ws/frame_g... File services/ui/ws/frame_generator_unittest.cc (right): https://codereview.chromium.org/2890913002/diff/100001/services/ui/ws/frame_g... services/ui/ws/frame_generator_unittest.cc:7: #include "base/debug/stack_trace.h" On 2017/05/18 11:55:28, Fady Samuel wrote: > Delete this. Done. https://codereview.chromium.org/2890913002/diff/100001/services/ui/ws/frame_g... services/ui/ws/frame_generator_unittest.cc:187: frame_generator()->Bind(std::move(test_client_binding)); On 2017/05/18 11:55:28, Fady Samuel wrote: > Why can't this be part of the setup? It's using InitWithSurfaceInfo() now. I didn't use it because I wanted it to be more explicit. https://codereview.chromium.org/2890913002/diff/100001/services/ui/ws/frame_g... services/ui/ws/frame_generator_unittest.cc:198: frame_generator()->OnWindowSizeChanged(gfx::Size(1, 2)); On 2017/05/18 11:55:27, Fady Samuel wrote: > Why introduce this? Thanks for pointing this out. It was part of the remainder of some of my experiments with the tests. It has been removed. https://codereview.chromium.org/2890913002/diff/100001/services/ui/ws/frame_g... services/ui/ws/frame_generator_unittest.cc:212: EXPECT_EQ(expected_ack, LastBeginFrameAck(binding)); On 2017/05/18 11:55:28, Fady Samuel wrote: > Maybe store binding in FrameGeneratorTest instead? Done.
https://codereview.chromium.org/2890913002/diff/120001/services/ui/ws/frame_g... File services/ui/ws/frame_generator_unittest.cc (left): https://codereview.chromium.org/2890913002/diff/120001/services/ui/ws/frame_g... services/ui/ws/frame_generator_unittest.cc:140: FrameGeneratorTest() {} nit: default https://codereview.chromium.org/2890913002/diff/120001/services/ui/ws/frame_g... File services/ui/ws/frame_generator_unittest.cc (right): https://codereview.chromium.org/2890913002/diff/120001/services/ui/ws/frame_g... services/ui/ws/frame_generator_unittest.cc:12: #include "services/ui/ws/server_window.h" Not needed? https://codereview.chromium.org/2890913002/diff/120001/services/ui/ws/frame_g... services/ui/ws/frame_generator_unittest.cc:13: #include "services/ui/ws/server_window_delegate.h" Not needed? https://codereview.chromium.org/2890913002/diff/120001/services/ui/ws/frame_g... services/ui/ws/frame_generator_unittest.cc:14: #include "services/ui/ws/test_utils.h" Is this needed? https://codereview.chromium.org/2890913002/diff/120001/services/ui/ws/frame_g... services/ui/ws/frame_generator_unittest.cc:15: #include "testing/gtest/include/gtest/gtest.h" Not needed? https://codereview.chromium.org/2890913002/diff/120001/services/ui/ws/frame_g... services/ui/ws/frame_generator_unittest.cc:207: frame_generator()->Bind(std::move(test_client_binding)); Why are you doing this here? Is this still necessary now that the ClientBinding is in FrameGeneratorTest? https://codereview.chromium.org/2890913002/diff/120001/services/ui/ws/frame_g... services/ui/ws/frame_generator_unittest.cc:225: EXPECT_EQ(expected_ack, LastBeginFrameAck(binding())); Do we need this param if binding() is part of FrameGeneratorTest?
The CQ bit was checked by staraz@chromium.org to run a CQ dry run
https://codereview.chromium.org/2890913002/diff/120001/services/ui/ws/frame_g... File services/ui/ws/frame_generator_unittest.cc (left): https://codereview.chromium.org/2890913002/diff/120001/services/ui/ws/frame_g... services/ui/ws/frame_generator_unittest.cc:140: FrameGeneratorTest() {} On 2017/05/18 14:48:08, Fady Samuel wrote: > nit: default Done. https://codereview.chromium.org/2890913002/diff/120001/services/ui/ws/frame_g... File services/ui/ws/frame_generator_unittest.cc (right): https://codereview.chromium.org/2890913002/diff/120001/services/ui/ws/frame_g... services/ui/ws/frame_generator_unittest.cc:12: #include "services/ui/ws/server_window.h" On 2017/05/18 14:48:09, Fady Samuel wrote: > Not needed? Done. https://codereview.chromium.org/2890913002/diff/120001/services/ui/ws/frame_g... services/ui/ws/frame_generator_unittest.cc:13: #include "services/ui/ws/server_window_delegate.h" On 2017/05/18 14:48:09, Fady Samuel wrote: > Not needed? Done. https://codereview.chromium.org/2890913002/diff/120001/services/ui/ws/frame_g... services/ui/ws/frame_generator_unittest.cc:14: #include "services/ui/ws/test_utils.h" On 2017/05/18 14:48:09, Fady Samuel wrote: > Is this needed? No. It's removed. https://codereview.chromium.org/2890913002/diff/120001/services/ui/ws/frame_g... services/ui/ws/frame_generator_unittest.cc:15: #include "testing/gtest/include/gtest/gtest.h" On 2017/05/18 14:48:08, Fady Samuel wrote: > Not needed? gtest.h is included in begin_frame_args_test.h, but do we want to take advantage of nested inclusions? https://codereview.chromium.org/2890913002/diff/120001/services/ui/ws/frame_g... services/ui/ws/frame_generator_unittest.cc:207: frame_generator()->Bind(std::move(test_client_binding)); On 2017/05/18 14:48:08, Fady Samuel wrote: > Why are you doing this here? Is this still necessary now that the ClientBinding > is in FrameGeneratorTest? I moved part of the InitWithSurfaceInfo into SetUp and now this is not needed anymore.
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by 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/2890913002/diff/120001/services/ui/ws/frame_g... File services/ui/ws/frame_generator_unittest.cc (right): https://codereview.chromium.org/2890913002/diff/120001/services/ui/ws/frame_g... services/ui/ws/frame_generator_unittest.cc:225: EXPECT_EQ(expected_ack, LastBeginFrameAck(binding())); On 2017/05/18 14:48:08, Fady Samuel wrote: > Do we need this param if binding() is part of FrameGeneratorTest? Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
https://codereview.chromium.org/2890913002/diff/160001/services/ui/ws/composi... File services/ui/ws/compositor_frame_sink_client_binding.h (right): https://codereview.chromium.org/2890913002/diff/160001/services/ui/ws/composi... services/ui/ws/compositor_frame_sink_client_binding.h:21: class CompositorFrameSinkClientBinding { Back to my earlier question. Maybe this can implement MojoCompositorFrameSink?
https://codereview.chromium.org/2890913002/diff/160001/services/ui/ws/composi... File services/ui/ws/compositor_frame_sink_client_binding.h (right): https://codereview.chromium.org/2890913002/diff/160001/services/ui/ws/composi... services/ui/ws/compositor_frame_sink_client_binding.h:21: class CompositorFrameSinkClientBinding { On 2017/05/18 17:03:57, Fady Samuel wrote: > Back to my earlier question. Maybe this can implement MojoCompositorFrameSink? Yes, I'm working on it.
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...
I'm looking into the failed tests. https://codereview.chromium.org/2890913002/diff/160001/services/ui/ws/composi... File services/ui/ws/compositor_frame_sink_client_binding.h (right): https://codereview.chromium.org/2890913002/diff/160001/services/ui/ws/composi... services/ui/ws/compositor_frame_sink_client_binding.h:21: class CompositorFrameSinkClientBinding { On 2017/05/18 17:03:57, Fady Samuel wrote: > Back to my earlier question. Maybe this can implement MojoCompositorFrameSink? 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 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 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_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/2890913002/diff/240001/services/ui/ws/composi... File services/ui/ws/compositor_frame_sink_client_binding.h (right): https://codereview.chromium.org/2890913002/diff/240001/services/ui/ws/composi... services/ui/ws/compositor_frame_sink_client_binding.h:21: class CompositorFrameSinkClientBinding Ahh this is pretty cool. Maybe we don't need this at all! Maybe FrameGenerator can take in std::unique_ptr<MojoCompositorFrameSink> directly instead of this CompositorFrameSinkClientBinding.
https://codereview.chromium.org/2890913002/diff/240001/services/ui/ws/frame_g... File services/ui/ws/frame_generator.cc (right): https://codereview.chromium.org/2890913002/diff/240001/services/ui/ws/frame_g... services/ui/ws/frame_generator.cc:95: if (!frame.render_pass_list.empty()) It is no longer possible for render_pass_list to be empty. Remove this line. https://codereview.chromium.org/2890913002/diff/240001/services/ui/ws/frame_g... services/ui/ws/frame_generator.cc:98: frame_size != last_submitted_frame_size_) { We should probably allocate a new ID if device scale factor changes too.
The CQ bit was checked by staraz@chromium.org to run a CQ dry run
https://codereview.chromium.org/2890913002/diff/240001/services/ui/ws/composi... File services/ui/ws/compositor_frame_sink_client_binding.h (right): https://codereview.chromium.org/2890913002/diff/240001/services/ui/ws/composi... services/ui/ws/compositor_frame_sink_client_binding.h:21: class CompositorFrameSinkClientBinding On 2017/05/19 11:51:03, Fady Samuel wrote: > Ahh this is pretty cool. Maybe we don't need this at all! > > Maybe FrameGenerator can take in std::unique_ptr<MojoCompositorFrameSink> > directly instead of this CompositorFrameSinkClientBinding. Done. https://codereview.chromium.org/2890913002/diff/240001/services/ui/ws/frame_g... File services/ui/ws/frame_generator.cc (right): https://codereview.chromium.org/2890913002/diff/240001/services/ui/ws/frame_g... services/ui/ws/frame_generator.cc:95: if (!frame.render_pass_list.empty()) On 2017/05/19 12:10:15, Fady Samuel wrote: > It is no longer possible for render_pass_list to be empty. Remove this line. Done. https://codereview.chromium.org/2890913002/diff/240001/services/ui/ws/frame_g... services/ui/ws/frame_generator.cc:98: frame_size != last_submitted_frame_size_) { On 2017/05/19 12:10:15, Fady Samuel wrote: > We should probably allocate a new ID if device scale factor changes too. Done.
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
LGTM overall + nits https://codereview.chromium.org/2890913002/diff/260001/services/ui/ws/composi... File services/ui/ws/compositor_frame_sink_client_binding.h (right): https://codereview.chromium.org/2890913002/diff/260001/services/ui/ws/composi... services/ui/ws/compositor_frame_sink_client_binding.h:5: #ifndef SERVICES_UI_WS_COMPOSITOR_FRAME_SINK_CLIENT_BINDING_H_ Rename file to compositor_frame_sink_binding.h https://codereview.chromium.org/2890913002/diff/260001/services/ui/ws/composi... services/ui/ws/compositor_frame_sink_client_binding.h:21: class CompositorFrameSinkClientBinding nit: CompositorFrameSinkClientBinding => CompsitorFrameSinkBinding https://codereview.chromium.org/2890913002/diff/260001/services/ui/ws/frame_g... File services/ui/ws/frame_generator.cc (right): https://codereview.chromium.org/2890913002/diff/260001/services/ui/ws/frame_g... services/ui/ws/frame_generator.cc:64: std::unique_ptr<cc::mojom::MojoCompositorFrameSink> binding) { nit: binding => compositor_frame_sink https://codereview.chromium.org/2890913002/diff/260001/services/ui/ws/frame_g... services/ui/ws/frame_generator.cc:66: client_binding_ = std::move(binding); client_binding_ => compositor_frame_sink_; https://codereview.chromium.org/2890913002/diff/260001/services/ui/ws/frame_g... services/ui/ws/frame_generator.cc:95: frame_size = frame.render_pass_list.back()->output_rect.size(); This doesn't make sense anymore. Replace the two lines above: gfx::Size frame_size = frame.render_pass_list.back()->output_rect.size(); https://codereview.chromium.org/2890913002/diff/260001/services/ui/ws/frame_g... services/ui/ws/frame_generator.cc:98: frame.metadata.device_scale_factor != last_device_scale_factor_) { Update the last_submitted_frame_size_ here too: last_submitted_frame_size_ = frame_size;
https://codereview.chromium.org/2890913002/diff/260001/services/ui/ws/composi... File services/ui/ws/compositor_frame_sink_client_binding.h (right): https://codereview.chromium.org/2890913002/diff/260001/services/ui/ws/composi... services/ui/ws/compositor_frame_sink_client_binding.h:5: #ifndef SERVICES_UI_WS_COMPOSITOR_FRAME_SINK_CLIENT_BINDING_H_ On 2017/05/19 14:21:51, Fady Samuel wrote: > Rename file to compositor_frame_sink_binding.h I followed the naming convention from WindowTreeBinding: WindowTreeBinding holds a mojo binding for WindowTree and interface pointer to WindowTreeClient and is called WindowTreeBinding. This class holds a binding to MojoCompositorFrameSinkClient and an InterfacePtr to MojoCFS so I call it MojoCFSClientBinding. WDYT?
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/2890913002/diff/260001/services/ui/ws/frame_g... File services/ui/ws/frame_generator.cc (right): https://codereview.chromium.org/2890913002/diff/260001/services/ui/ws/frame_g... services/ui/ws/frame_generator.cc:64: std::unique_ptr<cc::mojom::MojoCompositorFrameSink> binding) { On 2017/05/19 14:21:51, Fady Samuel wrote: > nit: binding => compositor_frame_sink Done. https://codereview.chromium.org/2890913002/diff/260001/services/ui/ws/frame_g... services/ui/ws/frame_generator.cc:66: client_binding_ = std::move(binding); On 2017/05/19 14:21:51, Fady Samuel wrote: > client_binding_ => compositor_frame_sink_; Done. https://codereview.chromium.org/2890913002/diff/260001/services/ui/ws/frame_g... services/ui/ws/frame_generator.cc:95: frame_size = frame.render_pass_list.back()->output_rect.size(); On 2017/05/19 14:21:51, Fady Samuel wrote: > This doesn't make sense anymore. Replace the two lines above: > > gfx::Size frame_size = frame.render_pass_list.back()->output_rect.size(); Done.
LGTM, yea, ignore my renaming suggestion. This is fine.
LGTM, yea, ignore my renaming suggestion. This is fine.
https://codereview.chromium.org/2890913002/diff/280001/services/ui/ws/composi... File services/ui/ws/compositor_frame_sink_client_binding.cc (right): https://codereview.chromium.org/2890913002/diff/280001/services/ui/ws/composi... services/ui/ws/compositor_frame_sink_client_binding.cc:38: compositor_frame_sink_->SetNeedsBeginFrame(false); Oops, just noticed this. It doesn't make sense to do this twice. You already do this in FrameGenerator.
The CQ bit was checked by staraz@chromium.org to run a CQ dry run
https://codereview.chromium.org/2890913002/diff/280001/services/ui/ws/composi... File services/ui/ws/compositor_frame_sink_client_binding.cc (right): https://codereview.chromium.org/2890913002/diff/280001/services/ui/ws/composi... services/ui/ws/compositor_frame_sink_client_binding.cc:38: compositor_frame_sink_->SetNeedsBeginFrame(false); On 2017/05/19 14:40:33, Fady Samuel wrote: > Oops, just noticed this. It doesn't make sense to do this twice. You already do > this in FrameGenerator. Good catch. This line is removed.
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
sky@: PTAL at BUILD.gn and platform_display_default.cc. Thank you!
LGTM
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by staraz@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from fsamuel@chromium.org Link to the patchset: https://codereview.chromium.org/2890913002/#ps300001 (title: "Remove redundant SetNeedsBeginFrame")
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": 300001, "attempt_start_ts": 1495214704733170,
"parent_rev": "97d4d455f11785e8681ad01335b9239032c658ba", "commit_rev":
"0bcca1e6eb7d275c808c26a37f0878e0b66be1d9"}
Message was sent while issue was closed.
Description was changed from ========== Add CompositorFrameSinkClientBinding To Be Used By FrameGenerator Added CompositorFrameSinkClientBinding interface. CompositorFrameSinkClientBinding's default implementation holds interface pointers to DisplayPrivate and MojoCompositorFrameSink. It binds FrameGenerator to a MojoCompositorFrameSinkClient request. PlatformDisplayDefault sets up the mojo connections before passing them to the binding object. FrameGenerator unit tests use a mock of CompositorFrameSinkClientBinding to keep track of the submitted frames. FrameGenerator no longer uses cc::CompositorFrameSink after this CL. cc::CompositorFrameSink should not be used by anything other than LayerTreeHost. BUG=721810 ========== to ========== Add CompositorFrameSinkClientBinding To Be Used By FrameGenerator Added CompositorFrameSinkClientBinding interface. CompositorFrameSinkClientBinding's default implementation holds interface pointers to DisplayPrivate and MojoCompositorFrameSink. It binds FrameGenerator to a MojoCompositorFrameSinkClient request. PlatformDisplayDefault sets up the mojo connections before passing them to the binding object. FrameGenerator unit tests use a mock of CompositorFrameSinkClientBinding to keep track of the submitted frames. FrameGenerator no longer uses cc::CompositorFrameSink after this CL. cc::CompositorFrameSink should not be used by anything other than LayerTreeHost. BUG=721810 Review-Url: https://codereview.chromium.org/2890913002 Cr-Commit-Position: refs/heads/master@{#473231} Committed: https://chromium.googlesource.com/chromium/src/+/0bcca1e6eb7d275c808c26a37f08... ==========
Message was sent while issue was closed.
Committed patchset #16 (id:300001) as https://chromium.googlesource.com/chromium/src/+/0bcca1e6eb7d275c808c26a37f08... |
