Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2795683003:
[cc]Replace use of SurfaceFactory with CompositorFrameSinkSupport in tests (Closed)
DescriptionReplace use of SurfaceFactory with CompositorFrameSinkSupport in cc tests
SurfaceFactory is being deleted since all non-test usages have been replaced by
CompositorFrameSinkSupport. This CL replaces usages in all but SurfaceAggregator
tests.
BUG=707105
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
Review-Url: https://codereview.chromium.org/2795683003
Cr-Commit-Position: refs/heads/master@{#464194}
Committed: https://chromium.googlesource.com/chromium/src/+/04278218c9ef29904b209f077fd3ee5869898ba2
Patch Set 1 #
Total comments: 12
Patch Set 2 : Address comments #
Total comments: 10
Patch Set 3 : Remove SurfaceFacotry Usages in SurfaceAggregatorTest And Address Comments #
Total comments: 6
Patch Set 4 : Add EmptyCompositorFrameSinkSupportClient Class #
Total comments: 16
Patch Set 5 : Address Nits #
Total comments: 2
Patch Set 6 : Address Nit #
Total comments: 8
Patch Set 7 : Fix Constant Naming #Patch Set 8 : Remove SurfaceFactory From SurfaceManagerRefTest #Patch Set 9 : Fix SurfaceManagerRefTest #
Total comments: 16
Patch Set 10 : EXPECT_CALL in MockCompositorFrameSinkSupport() #Patch Set 11 : Make MockCompositorFrameSinkSupport a friend of CompositorFrameSinkSupport #Patch Set 12 : Remove #include gmock #Patch Set 13 : Revert Changes In surface_manager_unittest.cc #Patch Set 14 : Cleanup #Patch Set 15 : Rebase To Use CompositorFrameSinkSupport::Create #Patch Set 16 : Update returned_resources_ in FakeCompositorFrameSinkSupportClient::DidReceiveCompositorFrameAck #Messages
Total messages: 97 (62 generated)
Description was changed from ========== Most CC Unit Tests Use CompositorFrameSinkSupport Instead Of SurfaceFactory SurfaceFactory is being deleted since all non-test usages have been replaced by CompositorFrameSinkSupport. This CL replaces usages in all but SurfaceAggregator tests. BUG=707105 ========== to ========== Most CC Unit Tests Use CompositorFrameSinkSupport Instead Of SurfaceFactory SurfaceFactory is being deleted since all non-test usages have been replaced by CompositorFrameSinkSupport. This CL replaces usages in all but SurfaceAggregator tests. BUG=707105 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
The CQ bit was checked by staraz@chromium.org to run a CQ dry run
staraz@chromium.org changed reviewers: + fsamuel@chromium.org
PTAL.
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/2795683003/diff/1/cc/surfaces/surface_hittest... File cc/surfaces/surface_hittest_unittest.cc (right): https://codereview.chromium.org/2795683003/diff/1/cc/surfaces/surface_hittest... cc/surfaces/surface_hittest_unittest.cc:63: static constexpr bool needs_sync_points = true; Put these in the anonymous namespace above and drop the static. Looks like we have a static in an anonymous namespace..that's redundant. Please delete that static keyword there too. https://codereview.chromium.org/2795683003/diff/1/cc/surfaces/surface_manager... File cc/surfaces/surface_manager_unittest.cc (right): https://codereview.chromium.org/2795683003/diff/1/cc/surfaces/surface_manager... cc/surfaces/surface_manager_unittest.cc:14: class FakeSurfaceFactoryClient : public CompositorFrameSinkSupport { Can we get rid of this entirely? https://codereview.chromium.org/2795683003/diff/1/cc/surfaces/surface_unittes... File cc/surfaces/surface_unittest.cc (left): https://codereview.chromium.org/2795683003/diff/1/cc/surfaces/surface_unittes... cc/surfaces/surface_unittest.cc:22: static constexpr FrameSinkId kArbitraryFrameSinkId(1, 1); get rid of static keyword. I probably introduced this through a search-and-replace but it's unnecessary. https://codereview.chromium.org/2795683003/diff/1/cc/surfaces/surface_unittes... File cc/surfaces/surface_unittest.cc (right): https://codereview.chromium.org/2795683003/diff/1/cc/surfaces/surface_unittes... cc/surfaces/surface_unittest.cc:24: static constexpr bool need_sync_points = true; Get rid of static if in anonymous namespace. https://codereview.chromium.org/2795683003/diff/1/cc/surfaces/surfaces_pixelt... File cc/surfaces/surfaces_pixeltest.cc (right): https://codereview.chromium.org/2795683003/diff/1/cc/surfaces/surfaces_pixelt... cc/surfaces/surfaces_pixeltest.cc:26: static constexpr FrameSinkId kArbitraryRightFrameSinkId(4, 4); get rid of all these statics. https://codereview.chromium.org/2795683003/diff/1/cc/surfaces/surfaces_pixelt... cc/surfaces/surfaces_pixeltest.cc:41: static constexpr bool needs_sync_points_ = true; Put these in the anonymous namespace about. Don't use _.
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 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/2795683003/diff/1/cc/surfaces/surface_hittest... File cc/surfaces/surface_hittest_unittest.cc (right): https://codereview.chromium.org/2795683003/diff/1/cc/surfaces/surface_hittest... cc/surfaces/surface_hittest_unittest.cc:63: static constexpr bool needs_sync_points = true; On 2017/04/03 22:33:25, Fady Samuel wrote: > Put these in the anonymous namespace above and drop the static. Looks like we > have a static in an anonymous namespace..that's redundant. Please delete that > static keyword there too. Done. https://codereview.chromium.org/2795683003/diff/1/cc/surfaces/surface_manager... File cc/surfaces/surface_manager_unittest.cc (right): https://codereview.chromium.org/2795683003/diff/1/cc/surfaces/surface_manager... cc/surfaces/surface_manager_unittest.cc:14: class FakeSurfaceFactoryClient : public CompositorFrameSinkSupport { On 2017/04/03 22:33:25, Fady Samuel wrote: > Can we get rid of this entirely? Done. https://codereview.chromium.org/2795683003/diff/1/cc/surfaces/surface_unittes... File cc/surfaces/surface_unittest.cc (left): https://codereview.chromium.org/2795683003/diff/1/cc/surfaces/surface_unittes... cc/surfaces/surface_unittest.cc:22: static constexpr FrameSinkId kArbitraryFrameSinkId(1, 1); On 2017/04/03 22:33:25, Fady Samuel wrote: > get rid of static keyword. > > I probably introduced this through a search-and-replace but it's unnecessary. Done. https://codereview.chromium.org/2795683003/diff/1/cc/surfaces/surface_unittes... File cc/surfaces/surface_unittest.cc (right): https://codereview.chromium.org/2795683003/diff/1/cc/surfaces/surface_unittes... cc/surfaces/surface_unittest.cc:24: static constexpr bool need_sync_points = true; On 2017/04/03 22:33:25, Fady Samuel wrote: > Get rid of static if in anonymous namespace. Done. https://codereview.chromium.org/2795683003/diff/1/cc/surfaces/surfaces_pixelt... File cc/surfaces/surfaces_pixeltest.cc (right): https://codereview.chromium.org/2795683003/diff/1/cc/surfaces/surfaces_pixelt... cc/surfaces/surfaces_pixeltest.cc:26: static constexpr FrameSinkId kArbitraryRightFrameSinkId(4, 4); On 2017/04/03 22:33:25, Fady Samuel wrote: > get rid of all these statics. Done. https://codereview.chromium.org/2795683003/diff/1/cc/surfaces/surfaces_pixelt... cc/surfaces/surfaces_pixeltest.cc:41: static constexpr bool needs_sync_points_ = true; On 2017/04/03 22:33:25, Fady Samuel wrote: > Put these in the anonymous namespace about. Don't use _. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm https://codereview.chromium.org/2795683003/diff/20001/cc/surfaces/compositor_... File cc/surfaces/compositor_frame_sink_support.h (right): https://codereview.chromium.org/2795683003/diff/20001/cc/surfaces/compositor_... cc/surfaces/compositor_frame_sink_support.h:59: BeginFrameSource* begin_frame_source() { return begin_frame_source_; } BeginFrameSourceForTesting() const https://codereview.chromium.org/2795683003/diff/20001/cc/surfaces/surface_hit... File cc/surfaces/surface_hittest_unittest.cc (right): https://codereview.chromium.org/2795683003/diff/20001/cc/surfaces/surface_hit... cc/surfaces/surface_hittest_unittest.cc:26: constexpr bool needs_sync_points = true; nit: linebreak. https://codereview.chromium.org/2795683003/diff/20001/cc/surfaces/surface_man... File cc/surfaces/surface_manager_unittest.cc (right): https://codereview.chromium.org/2795683003/diff/20001/cc/surfaces/surface_man... cc/surfaces/surface_manager_unittest.cc:342: EXPECT_TRUE(client_b_.get() == nullptr || Can this happen? https://codereview.chromium.org/2795683003/diff/20001/cc/surfaces/surface_man... cc/surfaces/surface_manager_unittest.cc:344: EXPECT_TRUE(client_c_.get() == nullptr || Can this happen? https://codereview.chromium.org/2795683003/diff/20001/cc/surfaces/surface_man... cc/surfaces/surface_manager_unittest.cc:360: const FrameSinkId frame_sink_id_c_ = FrameSinkId(3, 3); Move these to the anonymous namespace.
Sorry not lgtm. Please address comments first.
The CQ bit was checked by staraz@chromium.org to run a CQ dry run
https://codereview.chromium.org/2795683003/diff/20001/cc/surfaces/compositor_... File cc/surfaces/compositor_frame_sink_support.h (right): https://codereview.chromium.org/2795683003/diff/20001/cc/surfaces/compositor_... cc/surfaces/compositor_frame_sink_support.h:59: BeginFrameSource* begin_frame_source() { return begin_frame_source_; } On 2017/04/04 22:48:16, Fady Samuel wrote: > BeginFrameSourceForTesting() const Done. https://codereview.chromium.org/2795683003/diff/20001/cc/surfaces/surface_hit... File cc/surfaces/surface_hittest_unittest.cc (right): https://codereview.chromium.org/2795683003/diff/20001/cc/surfaces/surface_hit... cc/surfaces/surface_hittest_unittest.cc:26: constexpr bool needs_sync_points = true; On 2017/04/04 22:48:16, Fady Samuel wrote: > nit: linebreak. Done. https://codereview.chromium.org/2795683003/diff/20001/cc/surfaces/surface_man... File cc/surfaces/surface_manager_unittest.cc (right): https://codereview.chromium.org/2795683003/diff/20001/cc/surfaces/surface_man... cc/surfaces/surface_manager_unittest.cc:342: EXPECT_TRUE(client_b_.get() == nullptr || On 2017/04/04 22:48:16, Fady Samuel wrote: > Can this happen? Yes. It happens in SurfaceManagerOrderingTest(), ~SurfaceManagerOrderingTest() and UnregisterClients(). https://codereview.chromium.org/2795683003/diff/20001/cc/surfaces/surface_man... cc/surfaces/surface_manager_unittest.cc:344: EXPECT_TRUE(client_c_.get() == nullptr || On 2017/04/04 22:48:17, Fady Samuel wrote: > Can this happen? Yes. It happens in SurfaceManagerOrderingTest(), ~SurfaceManagerOrderingTest() and UnregisterClients(). https://codereview.chromium.org/2795683003/diff/20001/cc/surfaces/surface_man... cc/surfaces/surface_manager_unittest.cc:360: const FrameSinkId frame_sink_id_c_ = FrameSinkId(3, 3); On 2017/04/04 22:48:17, Fady Samuel wrote: > Move these to the anonymous namespace. Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Please don't add state to CompositorFrameSinkSupport that is only used for testing. This is generally considered an anti-pattern. https://codereview.chromium.org/2795683003/diff/40001/cc/surfaces/compositor_... File cc/surfaces/compositor_frame_sink_support.cc (right): https://codereview.chromium.org/2795683003/diff/40001/cc/surfaces/compositor_... cc/surfaces/compositor_frame_sink_support.cc:191: CompositorFrameSinkSupport::LastReturnedResourcesForTesting() const { Remove this? https://codereview.chromium.org/2795683003/diff/40001/cc/surfaces/compositor_... cc/surfaces/compositor_frame_sink_support.cc:217: last_returned_resources_ = resources; Don't expose this? https://codereview.chromium.org/2795683003/diff/40001/cc/surfaces/compositor_... cc/surfaces/compositor_frame_sink_support.cc:244: last_damage_rect_ = damage_rect; Don't expose these. Implement CompositorFrameSinkSupportClient instead in test?
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 to run a CQ dry run
https://codereview.chromium.org/2795683003/diff/40001/cc/surfaces/compositor_... File cc/surfaces/compositor_frame_sink_support.cc (right): https://codereview.chromium.org/2795683003/diff/40001/cc/surfaces/compositor_... cc/surfaces/compositor_frame_sink_support.cc:191: CompositorFrameSinkSupport::LastReturnedResourcesForTesting() const { On 2017/04/05 14:42:32, Fady Samuel wrote: > Remove this? Done. https://codereview.chromium.org/2795683003/diff/40001/cc/surfaces/compositor_... cc/surfaces/compositor_frame_sink_support.cc:217: last_returned_resources_ = resources; On 2017/04/05 14:42:32, Fady Samuel wrote: > Don't expose this? Done. https://codereview.chromium.org/2795683003/diff/40001/cc/surfaces/compositor_... cc/surfaces/compositor_frame_sink_support.cc:244: last_damage_rect_ = damage_rect; On 2017/04/05 14:42:32, Fady Samuel wrote: > Don't expose these. Implement CompositorFrameSinkSupportClient instead in test? Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Getting closer! A bunch of mostly nits. https://codereview.chromium.org/2795683003/diff/60001/cc/surfaces/surface_man... File cc/surfaces/surface_manager_unittest.cc (right): https://codereview.chromium.org/2795683003/diff/60001/cc/surfaces/surface_man... cc/surfaces/surface_manager_unittest.cc:16: constexpr bool is_root_child = false; nit: is_child_root https://codereview.chromium.org/2795683003/diff/60001/cc/surfaces/surface_man... cc/surfaces/surface_manager_unittest.cc:19: const FrameSinkId frame_sink_id_a = FrameSinkId(1, 1); can this be constexpr? constexpr FrameSinkId frame_sink_id_a(1, 1); and below. https://codereview.chromium.org/2795683003/diff/60001/cc/surfaces/surface_man... cc/surfaces/surface_manager_unittest.cc:335: client_a_->BeginFrameSourceForTesting() == nullptr); EXPECT_TRUE(!client_a || !client_a->BeginFrameSourceForTesting()); here and below. https://codereview.chromium.org/2795683003/diff/60001/cc/test/empty_composito... File cc/test/empty_compositor_frame_sink_support_client.cc (right): https://codereview.chromium.org/2795683003/diff/60001/cc/test/empty_composito... cc/test/empty_compositor_frame_sink_support_client.cc:9: namespace cc { nit: new line. https://codereview.chromium.org/2795683003/diff/60001/cc/test/empty_composito... cc/test/empty_compositor_frame_sink_support_client.cc:31: } nit: new line. https://codereview.chromium.org/2795683003/diff/60001/cc/test/empty_composito... File cc/test/empty_compositor_frame_sink_support_client.h (right): https://codereview.chromium.org/2795683003/diff/60001/cc/test/empty_composito... cc/test/empty_compositor_frame_sink_support_client.h:29: // CompositorFrameSinkSupportClient implementation. cc/ OWNERS prefer to keep this public. https://codereview.chromium.org/2795683003/diff/60001/cc/test/empty_composito... cc/test/empty_compositor_frame_sink_support_client.h:36: // Member variables used by SurfaceAggregatorTest. This will go stale very quickly. Delete this? https://codereview.chromium.org/2795683003/diff/60001/cc/test/empty_composito... cc/test/empty_compositor_frame_sink_support_client.h:39: ReturnedResourceArray returned_resources_; DISALLOW_COPY_AND_ASSIGN(EmptyCompositorFrameSinkSupportClient);
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 to run a CQ dry run
https://codereview.chromium.org/2795683003/diff/60001/cc/surfaces/surface_man... File cc/surfaces/surface_manager_unittest.cc (right): https://codereview.chromium.org/2795683003/diff/60001/cc/surfaces/surface_man... cc/surfaces/surface_manager_unittest.cc:16: constexpr bool is_root_child = false; On 2017/04/05 21:32:42, Fady Samuel wrote: > nit: is_child_root Done. https://codereview.chromium.org/2795683003/diff/60001/cc/surfaces/surface_man... cc/surfaces/surface_manager_unittest.cc:19: const FrameSinkId frame_sink_id_a = FrameSinkId(1, 1); On 2017/04/05 21:32:42, Fady Samuel wrote: > can this be constexpr? > > constexpr FrameSinkId frame_sink_id_a(1, 1); > > and below. Done. https://codereview.chromium.org/2795683003/diff/60001/cc/surfaces/surface_man... cc/surfaces/surface_manager_unittest.cc:335: client_a_->BeginFrameSourceForTesting() == nullptr); On 2017/04/05 21:32:42, Fady Samuel wrote: > EXPECT_TRUE(!client_a || !client_a->BeginFrameSourceForTesting()); here and > below. Done. https://codereview.chromium.org/2795683003/diff/60001/cc/test/empty_composito... File cc/test/empty_compositor_frame_sink_support_client.cc (right): https://codereview.chromium.org/2795683003/diff/60001/cc/test/empty_composito... cc/test/empty_compositor_frame_sink_support_client.cc:9: namespace cc { On 2017/04/05 21:32:42, Fady Samuel wrote: > nit: new line. Done. https://codereview.chromium.org/2795683003/diff/60001/cc/test/empty_composito... cc/test/empty_compositor_frame_sink_support_client.cc:31: } On 2017/04/05 21:32:42, Fady Samuel wrote: > nit: new line. Done. https://codereview.chromium.org/2795683003/diff/60001/cc/test/empty_composito... File cc/test/empty_compositor_frame_sink_support_client.h (right): https://codereview.chromium.org/2795683003/diff/60001/cc/test/empty_composito... cc/test/empty_compositor_frame_sink_support_client.h:29: // CompositorFrameSinkSupportClient implementation. On 2017/04/05 21:32:42, Fady Samuel wrote: > cc/ OWNERS prefer to keep this public. Done. https://codereview.chromium.org/2795683003/diff/60001/cc/test/empty_composito... cc/test/empty_compositor_frame_sink_support_client.h:36: // Member variables used by SurfaceAggregatorTest. On 2017/04/05 21:32:42, Fady Samuel wrote: > This will go stale very quickly. Delete this? Done. https://codereview.chromium.org/2795683003/diff/60001/cc/test/empty_composito... cc/test/empty_compositor_frame_sink_support_client.h:39: ReturnedResourceArray returned_resources_; On 2017/04/05 21:32:42, Fady Samuel wrote: > DISALLOW_COPY_AND_ASSIGN(EmptyCompositorFrameSinkSupportClient); Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm + nit. Please pass this along to danakj@ https://codereview.chromium.org/2795683003/diff/80001/cc/surfaces/surface_man... File cc/surfaces/surface_manager_unittest.cc (right): https://codereview.chromium.org/2795683003/diff/80001/cc/surfaces/surface_man... cc/surfaces/surface_manager_unittest.cc:355: client_b_->BeginFrameSourceForTesting() == nullptr); Here too and below...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2795683003/diff/80001/cc/surfaces/surface_man... File cc/surfaces/surface_manager_unittest.cc (right): https://codereview.chromium.org/2795683003/diff/80001/cc/surfaces/surface_man... cc/surfaces/surface_manager_unittest.cc:355: client_b_->BeginFrameSourceForTesting() == nullptr); On 2017/04/06 00:08:36, Fady Samuel wrote: > Here too and below... Done.
staraz@chromium.org changed reviewers: + danakj@chromium.org
danakj@chromium.org: PTAL.
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 just noticed the constant variable names...they should probably be kConstantNaming... https://codereview.chromium.org/2795683003/diff/100001/cc/surfaces/surface_hi... File cc/surfaces/surface_hittest_unittest.cc (right): https://codereview.chromium.org/2795683003/diff/100001/cc/surfaces/surface_hi... cc/surfaces/surface_hittest_unittest.cc:26: constexpr bool needs_sync_points = true; Ohh just noticed. These are all constants so I think the style guide says kConstantNaming: https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md https://codereview.chromium.org/2795683003/diff/100001/cc/surfaces/surface_ma... File cc/surfaces/surface_manager_unittest.cc (right): https://codereview.chromium.org/2795683003/diff/100001/cc/surfaces/surface_ma... cc/surfaces/surface_manager_unittest.cc:21: constexpr FrameSinkId frame_sink_id_c = FrameSinkId(3, 3); kConstantNaming all of the variables here is more appropriate I guess..although I do think we're pretty inconsistent in the code base with this. https://codereview.chromium.org/2795683003/diff/100001/cc/surfaces/surface_un... File cc/surfaces/surface_unittest.cc (right): https://codereview.chromium.org/2795683003/diff/100001/cc/surfaces/surface_un... cc/surfaces/surface_unittest.cc:24: constexpr bool need_sync_points = true; kConstantNaming for all these constants. https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md https://codereview.chromium.org/2795683003/diff/100001/cc/surfaces/surfaces_p... File cc/surfaces/surfaces_pixeltest.cc (right): https://codereview.chromium.org/2795683003/diff/100001/cc/surfaces/surfaces_p... cc/surfaces/surfaces_pixeltest.cc:28: constexpr bool needs_sync_points = true; kConstantNaming https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md
https://codereview.chromium.org/2795683003/diff/100001/cc/surfaces/surface_hi... File cc/surfaces/surface_hittest_unittest.cc (right): https://codereview.chromium.org/2795683003/diff/100001/cc/surfaces/surface_hi... cc/surfaces/surface_hittest_unittest.cc:26: constexpr bool needs_sync_points = true; On 2017/04/06 12:17:22, Fady Samuel wrote: > Ohh just noticed. These are all constants so I think the style guide says > kConstantNaming: > https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md Done. https://codereview.chromium.org/2795683003/diff/100001/cc/surfaces/surface_ma... File cc/surfaces/surface_manager_unittest.cc (right): https://codereview.chromium.org/2795683003/diff/100001/cc/surfaces/surface_ma... cc/surfaces/surface_manager_unittest.cc:21: constexpr FrameSinkId frame_sink_id_c = FrameSinkId(3, 3); On 2017/04/06 12:17:22, Fady Samuel wrote: > kConstantNaming all of the variables here is more appropriate I guess..although > I do think we're pretty inconsistent in the code base with this. Done. https://codereview.chromium.org/2795683003/diff/100001/cc/surfaces/surface_un... File cc/surfaces/surface_unittest.cc (right): https://codereview.chromium.org/2795683003/diff/100001/cc/surfaces/surface_un... cc/surfaces/surface_unittest.cc:24: constexpr bool need_sync_points = true; On 2017/04/06 12:17:22, Fady Samuel wrote: > kConstantNaming for all these constants. > https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md Done. https://codereview.chromium.org/2795683003/diff/100001/cc/surfaces/surfaces_p... File cc/surfaces/surfaces_pixeltest.cc (right): https://codereview.chromium.org/2795683003/diff/100001/cc/surfaces/surfaces_p... cc/surfaces/surfaces_pixeltest.cc:28: constexpr bool needs_sync_points = true; On 2017/04/06 12:17:22, Fady Samuel wrote: > kConstantNaming > https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md Done.
The CQ bit was checked by fsamuel@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: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
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: This issue passed the CQ dry run.
Hey, some feedback on CL descriptions based on https://chris.beams.io/posts/git-commit/#imperative The title "Most CC Unit Tests Use CompositorFrameSinkSupport Instead Of SurfaceFactory" breaks #5 there, because "If applied, this commit will Most CC Unit Tests Use CompositorFrameSinkSupport Instead Of SurfaceFactory" does not make sense. Maybe something like "cc:: Replace use of SurfaceFactory with CompositorFrameSinkSupport in tests" or so? That would follow the guidance.
Description was changed from ========== Most CC Unit Tests Use CompositorFrameSinkSupport Instead Of SurfaceFactory SurfaceFactory is being deleted since all non-test usages have been replaced by CompositorFrameSinkSupport. This CL replaces usages in all but SurfaceAggregator tests. BUG=707105 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== Replace use of SurfaceFactory with CompositorFrameSinkSupport in cc tests SurfaceFactory is being deleted since all non-test usages have been replaced by CompositorFrameSinkSupport. This CL replaces usages in all but SurfaceAggregator tests. BUG=707105 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
On 2017/04/07 15:03:53, danakj wrote: > Hey, some feedback on CL descriptions based on > https://chris.beams.io/posts/git-commit/#imperative > > The title "Most CC Unit Tests Use CompositorFrameSinkSupport Instead Of > SurfaceFactory" breaks #5 there, because "If applied, this commit will Most CC > Unit Tests Use CompositorFrameSinkSupport Instead Of SurfaceFactory" does not > make sense. > > Maybe something like "cc:: Replace use of SurfaceFactory with > CompositorFrameSinkSupport in tests" or so? That would follow the guidance. Thanks for the suggestion! I have updated the subject line.
Few comments, but looks good https://codereview.chromium.org/2795683003/diff/160001/cc/surfaces/surface_ag... File cc/surfaces/surface_aggregator_unittest.cc (right): https://codereview.chromium.org/2795683003/diff/160001/cc/surfaces/surface_ag... cc/surfaces/surface_aggregator_unittest.cc:44: constexpr bool is_root = true; right, consts out in the namespace (or class) scope should be kFooBar style. Locals inside a method we're inconsistent at best and I don't really mind which one you do. I suggest maybe kRootIsRoot and kChildIsRoot here? https://codereview.chromium.org/2795683003/diff/160001/cc/surfaces/surface_hi... File cc/surfaces/surface_hittest_unittest.cc (right): https://codereview.chromium.org/2795683003/diff/160001/cc/surfaces/surface_hi... cc/surfaces/surface_hittest_unittest.cc:23: constexpr bool kIsRoot = true; Oh looks like you changed some global consts to kFoo but not all, just need to replicate this https://codereview.chromium.org/2795683003/diff/160001/cc/surfaces/surface_ma... File cc/surfaces/surface_manager_unittest.cc (right): https://codereview.chromium.org/2795683003/diff/160001/cc/surfaces/surface_ma... cc/surfaces/surface_manager_unittest.cc:23: } // namespace we usually wrap the whole test file in anon namespace fwiw, this is the easiest way to ensure no symbols collide https://codereview.chromium.org/2795683003/diff/160001/cc/surfaces/surface_ma... cc/surfaces/surface_manager_unittest.cc:55: EXPECT_EQ(nullptr, client.BeginFrameSourceForTesting()); I don't think you need to add a ForTesting() method to do this. That's the last kind of resort to take. In this case you're testing the use of the SurfaceFactoryClient interface on CompositorFrameSinkSupport. So you could make a mock subclass of CompositorFrameSink support that has a mock SetBeginFrameSource method, and you can EXPECT_CALL on it, instead of peeking at internal state of CompositorFrameSupport? https://codereview.chromium.org/2795683003/diff/160001/cc/test/empty_composit... File cc/test/empty_compositor_frame_sink_support_client.cc (right): https://codereview.chromium.org/2795683003/diff/160001/cc/test/empty_composit... cc/test/empty_compositor_frame_sink_support_client.cc:11: EmptyCompositorFrameSinkSupportClient::EmptyCompositorFrameSinkSupportClient() { = default https://codereview.chromium.org/2795683003/diff/160001/cc/test/empty_composit... cc/test/empty_compositor_frame_sink_support_client.cc:15: ~EmptyCompositorFrameSinkSupportClient() {} = default https://codereview.chromium.org/2795683003/diff/160001/cc/test/empty_composit... File cc/test/empty_compositor_frame_sink_support_client.h (right): https://codereview.chromium.org/2795683003/diff/160001/cc/test/empty_composit... cc/test/empty_compositor_frame_sink_support_client.h:14: class EmptyCompositorFrameSinkSupportClient I know you're copying this name from where it came from but a) I don't think Empty describes this class well b) The bar for a class up in cc/test/ is a bit higher than for a class defined in a test file for a single test c) Our pattern here is to make a cc/test/ class named FakeFooBar when it is a FooBar that provides knobs for tests to change behaviour or peek at states. So, FakeCompositorFrameSinkSupportClient? In general, our naming scheme tries to follow the somewhat standard "Stub" (empty interface impl), "Mock" (used to verify usage with gmock), "Fake" (a lightweight impl for testing). This describes much more detail: https://www.martinfowler.com/articles/mocksArentStubs.html
https://codereview.chromium.org/2795683003/diff/160001/cc/surfaces/surface_ag... File cc/surfaces/surface_aggregator_unittest.cc (right): https://codereview.chromium.org/2795683003/diff/160001/cc/surfaces/surface_ag... cc/surfaces/surface_aggregator_unittest.cc:44: constexpr bool is_root = true; On 2017/04/07 15:52:44, danakj wrote: > right, consts out in the namespace (or class) scope should be kFooBar style. > Locals inside a method we're inconsistent at best and I don't really mind which > one you do. I suggest maybe kRootIsRoot and kChildIsRoot here? Done. https://codereview.chromium.org/2795683003/diff/160001/cc/surfaces/surface_hi... File cc/surfaces/surface_hittest_unittest.cc (right): https://codereview.chromium.org/2795683003/diff/160001/cc/surfaces/surface_hi... cc/surfaces/surface_hittest_unittest.cc:23: constexpr bool kIsRoot = true; On 2017/04/07 15:52:45, danakj wrote: > Oh looks like you changed some global consts to kFoo but not all, just need to > replicate this Done. https://codereview.chromium.org/2795683003/diff/160001/cc/surfaces/surface_ma... File cc/surfaces/surface_manager_unittest.cc (right): https://codereview.chromium.org/2795683003/diff/160001/cc/surfaces/surface_ma... cc/surfaces/surface_manager_unittest.cc:23: } // namespace On 2017/04/07 15:52:45, danakj wrote: > we usually wrap the whole test file in anon namespace fwiw, this is the easiest > way to ensure no symbols collide Done. https://codereview.chromium.org/2795683003/diff/160001/cc/surfaces/surface_ma... cc/surfaces/surface_manager_unittest.cc:55: EXPECT_EQ(nullptr, client.BeginFrameSourceForTesting()); On 2017/04/07 15:52:45, danakj wrote: > I don't think you need to add a ForTesting() method to do this. That's the last > kind of resort to take. > > In this case you're testing the use of the SurfaceFactoryClient interface on > CompositorFrameSinkSupport. So you could make a mock subclass of > CompositorFrameSink support that has a mock SetBeginFrameSource method, and you > can EXPECT_CALL on it, instead of peeking at internal state of > CompositorFrameSupport? I added a mock class (see the next patchset) but it causes the Ordering test to fail. I think it's because CompositorFrameSinkSupport() calls RegisterSurfaceFactoryClient() before the mock gets set up. Is there any work around? https://codereview.chromium.org/2795683003/diff/160001/cc/test/empty_composit... File cc/test/empty_compositor_frame_sink_support_client.cc (right): https://codereview.chromium.org/2795683003/diff/160001/cc/test/empty_composit... cc/test/empty_compositor_frame_sink_support_client.cc:11: EmptyCompositorFrameSinkSupportClient::EmptyCompositorFrameSinkSupportClient() { On 2017/04/07 15:52:45, danakj wrote: > = default Done. https://codereview.chromium.org/2795683003/diff/160001/cc/test/empty_composit... cc/test/empty_compositor_frame_sink_support_client.cc:15: ~EmptyCompositorFrameSinkSupportClient() {} On 2017/04/07 15:52:45, danakj wrote: > = default Done. https://codereview.chromium.org/2795683003/diff/160001/cc/test/empty_composit... File cc/test/empty_compositor_frame_sink_support_client.h (right): https://codereview.chromium.org/2795683003/diff/160001/cc/test/empty_composit... cc/test/empty_compositor_frame_sink_support_client.h:14: class EmptyCompositorFrameSinkSupportClient On 2017/04/07 15:52:45, danakj wrote: > I know you're copying this name from where it came from but > a) I don't think Empty describes this class well > b) The bar for a class up in cc/test/ is a bit higher than for a class defined > in a test file for a single test > c) Our pattern here is to make a cc/test/ class named FakeFooBar when it is a > FooBar that provides knobs for tests to change behaviour or peek at states. > > So, FakeCompositorFrameSinkSupportClient? > > In general, our naming scheme tries to follow the somewhat standard "Stub" > (empty interface impl), "Mock" (used to verify usage with gmock), "Fake" (a > lightweight impl for testing). This describes much more detail: > https://www.martinfowler.com/articles/mocksArentStubs.html Done.
Patchset #10 (id:180001) has been deleted
https://codereview.chromium.org/2795683003/diff/160001/cc/surfaces/surface_ma... File cc/surfaces/surface_manager_unittest.cc (right): https://codereview.chromium.org/2795683003/diff/160001/cc/surfaces/surface_ma... cc/surfaces/surface_manager_unittest.cc:55: EXPECT_EQ(nullptr, client.BeginFrameSourceForTesting()); On 2017/04/07 20:43:58, Alex Z. wrote: > On 2017/04/07 15:52:45, danakj wrote: > > I don't think you need to add a ForTesting() method to do this. That's the > last > > kind of resort to take. > > > > In this case you're testing the use of the SurfaceFactoryClient interface on > > CompositorFrameSinkSupport. So you could make a mock subclass of > > CompositorFrameSink support that has a mock SetBeginFrameSource method, and > you > > can EXPECT_CALL on it, instead of peeking at internal state of > > CompositorFrameSupport? > > I added a mock class (see the next patchset) but it causes the > Ordering test to fail. I think it's because > CompositorFrameSinkSupport() calls RegisterSurfaceFactoryClient() > before the mock gets set up. Is there any work around? Does this imply that virtual methods on CompositorFrameSupport are being called from inside its constructor? If so that's bad C++. Or did I misunderstand?
On 2017/04/07 21:16:17, danakj wrote: > https://codereview.chromium.org/2795683003/diff/160001/cc/surfaces/surface_ma... > File cc/surfaces/surface_manager_unittest.cc (right): > > https://codereview.chromium.org/2795683003/diff/160001/cc/surfaces/surface_ma... > cc/surfaces/surface_manager_unittest.cc:55: EXPECT_EQ(nullptr, > client.BeginFrameSourceForTesting()); > On 2017/04/07 20:43:58, Alex Z. wrote: > > On 2017/04/07 15:52:45, danakj wrote: > > > I don't think you need to add a ForTesting() method to do this. That's the > > last > > > kind of resort to take. > > > > > > In this case you're testing the use of the SurfaceFactoryClient interface on > > > CompositorFrameSinkSupport. So you could make a mock subclass of > > > CompositorFrameSink support that has a mock SetBeginFrameSource method, and > > you > > > can EXPECT_CALL on it, instead of peeking at internal state of > > > CompositorFrameSupport? > > > > I added a mock class (see the next patchset) but it causes the > > Ordering test to fail. I think it's because > > CompositorFrameSinkSupport() calls RegisterSurfaceFactoryClient() > > before the mock gets set up. Is there any work around? > > Does this imply that virtual methods on CompositorFrameSupport are being called > from inside its constructor? If so that's bad C++. Or did I misunderstand? Yes, they are. CompositorFrameSinkSupport calls SurfaceManager:::RegisterSurfaceFactoryClient(..., this), and it's a SurfaceFactoryClient...SurfaceManager calls SetBeginFrameSource... All this gets fixed in staraz@'s next CL when SurfaceFactoryClient goes away and SurfaceManager knows about CompositorFrameSinkSupport directly.
On Fri, Apr 7, 2017 at 5:20 PM, <fsamuel@chromium.org> wrote: > On 2017/04/07 21:16:17, danakj wrote: > > > https://codereview.chromium.org/2795683003/diff/160001/cc/ > surfaces/surface_manager_unittest.cc > > File cc/surfaces/surface_manager_unittest.cc (right): > > > > > https://codereview.chromium.org/2795683003/diff/160001/cc/ > surfaces/surface_manager_unittest.cc#newcode55 > > cc/surfaces/surface_manager_unittest.cc:55: EXPECT_EQ(nullptr, > > client.BeginFrameSourceForTesting()); > > On 2017/04/07 20:43:58, Alex Z. wrote: > > > On 2017/04/07 15:52:45, danakj wrote: > > > > I don't think you need to add a ForTesting() method to do this. > That's the > > > last > > > > kind of resort to take. > > > > > > > > In this case you're testing the use of the SurfaceFactoryClient > interface > on > > > > CompositorFrameSinkSupport. So you could make a mock subclass of > > > > CompositorFrameSink support that has a mock SetBeginFrameSource > method, > and > > > you > > > > can EXPECT_CALL on it, instead of peeking at internal state of > > > > CompositorFrameSupport? > > > > > > I added a mock class (see the next patchset) but it causes the > > > Ordering test to fail. I think it's because > > > CompositorFrameSinkSupport() calls RegisterSurfaceFactoryClient() > > > before the mock gets set up. Is there any work around? > > > > Does this imply that virtual methods on CompositorFrameSupport are being > called > > from inside its constructor? If so that's bad C++. Or did I > misunderstand? > > Yes, they are. CompositorFrameSinkSupport calls > SurfaceManager:::RegisterSurfaceFactoryClient(..., this), and it's a > SurfaceFactoryClient...SurfaceManager calls SetBeginFrameSource... > > All this gets fixed in staraz@'s next CL when SurfaceFactoryClient goes > away and > SurfaceManager knows about CompositorFrameSinkSupport directly. > I gave this to staraz@ but https://google.github.io/styleguide/cppguide.html#Doing_Work_in_Constructors This constructor seems to be doing too much. We should move it out of the constructor. -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
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...
https://codereview.chromium.org/2795683003/diff/160001/cc/surfaces/surface_ma... File cc/surfaces/surface_manager_unittest.cc (right): https://codereview.chromium.org/2795683003/diff/160001/cc/surfaces/surface_ma... cc/surfaces/surface_manager_unittest.cc:55: EXPECT_EQ(nullptr, client.BeginFrameSourceForTesting()); On 2017/04/07 21:16:17, danakj wrote: > On 2017/04/07 20:43:58, Alex Z. wrote: > > On 2017/04/07 15:52:45, danakj wrote: > > > I don't think you need to add a ForTesting() method to do this. That's the > > last > > > kind of resort to take. > > > > > > In this case you're testing the use of the SurfaceFactoryClient interface on > > > CompositorFrameSinkSupport. So you could make a mock subclass of > > > CompositorFrameSink support that has a mock SetBeginFrameSource method, and > > you > > > can EXPECT_CALL on it, instead of peeking at internal state of > > > CompositorFrameSupport? > > > > I added a mock class (see the next patchset) but it causes the > > Ordering test to fail. I think it's because > > CompositorFrameSinkSupport() calls RegisterSurfaceFactoryClient() > > before the mock gets set up. Is there any work around? > > Does this imply that virtual methods on CompositorFrameSupport are being called > from inside its constructor? If so that's bad C++. Or did I misunderstand? MockCompositorFrameSinkSupport is a friend of CompositorFrameSinkSupport in the new patchset following fsamuel@'s suggestion. This way we can access begin_frame_source_ in the tests without exposing it to everywhere else. MockCompositorFrameSinkSupport also doesn't need the complexity of the mock macros.
The CQ bit was checked by staraz@chromium.org to run a CQ dry run
Patchset #13 (id:260001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2017/04/07 21:29:01, Alex Z. wrote: > https://codereview.chromium.org/2795683003/diff/160001/cc/surfaces/surface_ma... > File cc/surfaces/surface_manager_unittest.cc (right): > > https://codereview.chromium.org/2795683003/diff/160001/cc/surfaces/surface_ma... > cc/surfaces/surface_manager_unittest.cc:55: EXPECT_EQ(nullptr, > client.BeginFrameSourceForTesting()); > On 2017/04/07 21:16:17, danakj wrote: > > On 2017/04/07 20:43:58, Alex Z. wrote: > > > On 2017/04/07 15:52:45, danakj wrote: > > > > I don't think you need to add a ForTesting() method to do this. That's the > > > last > > > > kind of resort to take. > > > > > > > > In this case you're testing the use of the SurfaceFactoryClient interface > on > > > > CompositorFrameSinkSupport. So you could make a mock subclass of > > > > CompositorFrameSink support that has a mock SetBeginFrameSource method, > and > > > you > > > > can EXPECT_CALL on it, instead of peeking at internal state of > > > > CompositorFrameSupport? > > > > > > I added a mock class (see the next patchset) but it causes the > > > Ordering test to fail. I think it's because > > > CompositorFrameSinkSupport() calls RegisterSurfaceFactoryClient() > > > before the mock gets set up. Is there any work around? > > > > Does this imply that virtual methods on CompositorFrameSupport are being > called > > from inside its constructor? If so that's bad C++. Or did I misunderstand? > > MockCompositorFrameSinkSupport is a friend of > CompositorFrameSinkSupport in the new patchset following fsamuel@'s > suggestion. This way we can access begin_frame_source_ in the tests > without exposing it to everywhere else. MockCompositorFrameSinkSupport > also doesn't need the complexity of the mock macros. I'm not sure why that's what we want? a) It's not a mock now, but there's nothing bad about EXPECT_CALL macros. b) We want to verify that the SetBeginFrameSource call happens on CompositorFrameSinkSupport. Now it's verifying internal states of CFSSupport instead. c) We shouldn't need to peek at private fields to write a test. d) The constructor is still violating the style guide and is dangerous C++.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
On 2017/04/07 21:34:34, danakj wrote: > On 2017/04/07 21:29:01, Alex Z. wrote: > > > https://codereview.chromium.org/2795683003/diff/160001/cc/surfaces/surface_ma... > > File cc/surfaces/surface_manager_unittest.cc (right): > > > > > https://codereview.chromium.org/2795683003/diff/160001/cc/surfaces/surface_ma... > > cc/surfaces/surface_manager_unittest.cc:55: EXPECT_EQ(nullptr, > > client.BeginFrameSourceForTesting()); > > On 2017/04/07 21:16:17, danakj wrote: > > > On 2017/04/07 20:43:58, Alex Z. wrote: > > > > On 2017/04/07 15:52:45, danakj wrote: > > > > > I don't think you need to add a ForTesting() method to do this. That's > the > > > > last > > > > > kind of resort to take. > > > > > > > > > > In this case you're testing the use of the SurfaceFactoryClient > interface > > on > > > > > CompositorFrameSinkSupport. So you could make a mock subclass of > > > > > CompositorFrameSink support that has a mock SetBeginFrameSource method, > > and > > > > you > > > > > can EXPECT_CALL on it, instead of peeking at internal state of > > > > > CompositorFrameSupport? > > > > > > > > I added a mock class (see the next patchset) but it causes the > > > > Ordering test to fail. I think it's because > > > > CompositorFrameSinkSupport() calls RegisterSurfaceFactoryClient() > > > > before the mock gets set up. Is there any work around? > > > > > > Does this imply that virtual methods on CompositorFrameSupport are being > > called > > > from inside its constructor? If so that's bad C++. Or did I misunderstand? > > > > MockCompositorFrameSinkSupport is a friend of > > CompositorFrameSinkSupport in the new patchset following fsamuel@'s > > suggestion. This way we can access begin_frame_source_ in the tests > > without exposing it to everywhere else. MockCompositorFrameSinkSupport > > also doesn't need the complexity of the mock macros. > > I'm not sure why that's what we want? > > a) It's not a mock now, but there's nothing bad about EXPECT_CALL macros. > b) We want to verify that the SetBeginFrameSource call happens on > CompositorFrameSinkSupport. Now it's verifying internal states of CFSSupport > instead. > c) We shouldn't need to peek at private fields to write a test. > d) The constructor is still violating the style guide and is dangerous C++. Agreed that this isn't good C++. It's something I missed when writing CompositorFrameSinkSupport initially. I think we need an Init method. Alex, maybe write that patch first? Move work out of CompositorFrameSinkSupport's constructor into an Init, and call the Init() in all places that construct a CompositorFrameSinkSupport...
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...
On 2017/04/07 21:34:34, danakj wrote: > On 2017/04/07 21:29:01, Alex Z. wrote: > > > https://codereview.chromium.org/2795683003/diff/160001/cc/surfaces/surface_ma... > > File cc/surfaces/surface_manager_unittest.cc (right): > > > > > https://codereview.chromium.org/2795683003/diff/160001/cc/surfaces/surface_ma... > > cc/surfaces/surface_manager_unittest.cc:55: EXPECT_EQ(nullptr, > > client.BeginFrameSourceForTesting()); > > On 2017/04/07 21:16:17, danakj wrote: > > > On 2017/04/07 20:43:58, Alex Z. wrote: > > > > On 2017/04/07 15:52:45, danakj wrote: > > > > > I don't think you need to add a ForTesting() method to do this. That's > the > > > > last > > > > > kind of resort to take. > > > > > > > > > > In this case you're testing the use of the SurfaceFactoryClient > interface > > on > > > > > CompositorFrameSinkSupport. So you could make a mock subclass of > > > > > CompositorFrameSink support that has a mock SetBeginFrameSource method, > > and > > > > you > > > > > can EXPECT_CALL on it, instead of peeking at internal state of > > > > > CompositorFrameSupport? > > > > > > > > I added a mock class (see the next patchset) but it causes the > > > > Ordering test to fail. I think it's because > > > > CompositorFrameSinkSupport() calls RegisterSurfaceFactoryClient() > > > > before the mock gets set up. Is there any work around? > > > > > > Does this imply that virtual methods on CompositorFrameSupport are being > > called > > > from inside its constructor? If so that's bad C++. Or did I misunderstand? > > > > MockCompositorFrameSinkSupport is a friend of > > CompositorFrameSinkSupport in the new patchset following fsamuel@'s > > suggestion. This way we can access begin_frame_source_ in the tests > > without exposing it to everywhere else. MockCompositorFrameSinkSupport > > also doesn't need the complexity of the mock macros. > > I'm not sure why that's what we want? > > a) It's not a mock now, but there's nothing bad about EXPECT_CALL macros. > b) We want to verify that the SetBeginFrameSource call happens on > CompositorFrameSinkSupport. Now it's verifying internal states of CFSSupport > instead. > c) We shouldn't need to peek at private fields to write a test. > d) The constructor is still violating the style guide and is dangerous C++. I agree with mocking a class is better than peeking at internal states. To mock CompositorFrameSinkSupport for SurfaceManagerTest, we need SetBeginFrameSource to be public/protected and non-virtual. I am reverting changes in surface_manager_unittest.cc so we can land this CL first to get rid of SurfaceFactory in other cc tests. The changes in surface_manager_unittest.cc will be done in the same CL that removes SurfaceFactoryClient interface so that SetBeginFrameSource becomes non-virtual at that point.
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: This issue passed the CQ dry run.
staraz@chromium.org changed reviewers: + piman@chromium.org
piman@: Please review changes in cc/ while danakj@ is out sick.
lgtm
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/2795683003/#ps300001 (title: "Cleanup")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
LGTM but looks like u need to update the tests to use CFSSupport::Create()
The CQ bit was checked by staraz@chromium.org to run a CQ dry run
On 2017/04/12 14:10:10, danakj (out sick) wrote: > LGTM but looks like u need to update the tests to use CFSSupport::Create() Rebased.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On Wed, Apr 12, 2017 at 3:29 PM, <staraz@chromium.org> wrote: > On 2017/04/12 14:10:10, danakj (out sick) wrote: > > LGTM but looks like u need to update the tests to use > CFSSupport::Create() > > Rebased. > You can go ahead and land if it was straightforward, my previous LG stands. If you want me to look at anything lmk. -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
The CQ bit was checked by staraz@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from danakj@chromium.org, fsamuel@chromium.org, piman@chromium.org Link to the patchset: https://codereview.chromium.org/2795683003/#ps340001 (title: "Update returned_resources_ in FakeCompositorFrameSinkSupportClient::DidReceiveCompositorFrameAck")
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": 340001, "attempt_start_ts": 1492031646512900,
"parent_rev": "a95a63804d48bfb9acfcaabc124504419dcd3668", "commit_rev":
"04278218c9ef29904b209f077fd3ee5869898ba2"}
Message was sent while issue was closed.
Description was changed from ========== Replace use of SurfaceFactory with CompositorFrameSinkSupport in cc tests SurfaceFactory is being deleted since all non-test usages have been replaced by CompositorFrameSinkSupport. This CL replaces usages in all but SurfaceAggregator tests. BUG=707105 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== Replace use of SurfaceFactory with CompositorFrameSinkSupport in cc tests SurfaceFactory is being deleted since all non-test usages have been replaced by CompositorFrameSinkSupport. This CL replaces usages in all but SurfaceAggregator tests. BUG=707105 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel Review-Url: https://codereview.chromium.org/2795683003 Cr-Commit-Position: refs/heads/master@{#464194} Committed: https://chromium.googlesource.com/chromium/src/+/04278218c9ef29904b209f077fd3... ==========
Message was sent while issue was closed.
Committed patchset #16 (id:340001) as https://chromium.googlesource.com/chromium/src/+/04278218c9ef29904b209f077fd3... |
