Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2755573002:
Add FrameGenerator Unit Tests (Closed)
DescriptionAdd FrameGenerator Unit Tests
Added FrameGeneratorTest and several mock classes to facilitate unit tests for
FrameGenerator.
Also added some explanatory comments to DisplayClientCompositorFrameSink.
BUG=683732
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
Review-Url: https://codereview.chromium.org/2755573002
Cr-Commit-Position: refs/heads/master@{#457954}
Committed: https://chromium.googlesource.com/chromium/src/+/8e89706698b46b5391e000fbd7175fd63cdf1a64
Patch Set 1 #
Total comments: 6
Patch Set 2 : Clean up #
Total comments: 4
Patch Set 3 : Changed OnCreation test to OnSurfaceCreated #
Total comments: 6
Patch Set 4 : Added SetDeviceScaleFactor test. #Patch Set 5 : FrameGenerator ignores MISSED BeginFrameArgs #
Total comments: 5
Patch Set 6 : update observing_begin_frames_ first #Patch Set 7 : ReferencedSurfaces() #
Total comments: 12
Patch Set 8 : Addressed comments #
Total comments: 12
Patch Set 9 : Addressed comments #
Total comments: 22
Patch Set 10 : Addressed comments #
Total comments: 9
Patch Set 11 : Removed dependency on gmock.h #Patch Set 12 : Addressed comments #Patch Set 13 : Added file specified DEPS rule since bots were complaining #Patch Set 14 : Added cc:test_support to pulic_deps #
Total comments: 2
Patch Set 15 : Removed TestCompositorFrameSink due to name conflict #
Messages
Total messages: 57 (21 generated)
Description was changed from ========== Add FrameGenerator Unit Tests Added FrameGeneratorTest and several mock classes to facilitate unit tests for FrameGenerator. Also added some explanatory comments to DisplayClientCompositorFrameSink. BUG=683732 ========== to ========== Add FrameGenerator Unit Tests Added FrameGeneratorTest and several mock classes to facilitate unit tests for FrameGenerator. Also added some explanatory comments to DisplayClientCompositorFrameSink. BUG=683732 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
staraz@chromium.org changed reviewers: + fsamuel@chromium.org
This is still WIP and I intent to finish all the TODOs I added before landing. PTAL and let me know what you think so that I can change it before spending too much time on it.
This seems like it's moving in the right direction. I've added some suggestions for simplifying the unit test. https://codereview.chromium.org/2755573002/diff/1/services/ui/ws/frame_genera... File services/ui/ws/frame_generator_unittest.cc (right): https://codereview.chromium.org/2755573002/diff/1/services/ui/ws/frame_genera... services/ui/ws/frame_generator_unittest.cc:100: std::move(compositor_frame_sink)); I would move most of the above to a FrameGeneratorTest::SetUp given that all unit tests will deal with FrameGenerator. https://codereview.chromium.org/2755573002/diff/1/services/ui/ws/frame_genera... services/ui/ws/frame_generator_unittest.cc:104: begin_frame_source.TestOnBeginFrame(kArgs); Make this a public method "IssueBeginFrame" on FrameGeneratorTest. https://codereview.chromium.org/2755573002/diff/1/services/ui/ws/frame_genera... services/ui/ws/frame_generator_unittest.cc:106: } This unit test can probably collapse to look like this once you move things into FrameGeneratorTest: TEST_F(FrameGeneratorTest, Creation) { EXPECT_EQ(0, compositor_frame_sink()->number_frames_received()); IssueBeginFrame(); EXPECT_EQ(0, compositor_frame_sink()->number_frames_received()); }
Patchset #2 (id:20001) has been deleted
I'm working on the rest of the tests now. https://codereview.chromium.org/2755573002/diff/1/services/ui/ws/frame_genera... File services/ui/ws/frame_generator_unittest.cc (right): https://codereview.chromium.org/2755573002/diff/1/services/ui/ws/frame_genera... services/ui/ws/frame_generator_unittest.cc:100: std::move(compositor_frame_sink)); On 2017/03/15 00:06:07, Fady Samuel wrote: > I would move most of the above to a FrameGeneratorTest::SetUp given that all > unit tests will deal with FrameGenerator. Done. https://codereview.chromium.org/2755573002/diff/1/services/ui/ws/frame_genera... services/ui/ws/frame_generator_unittest.cc:104: begin_frame_source.TestOnBeginFrame(kArgs); On 2017/03/15 00:06:07, Fady Samuel wrote: > Make this a public method "IssueBeginFrame" on FrameGeneratorTest. Done. https://codereview.chromium.org/2755573002/diff/1/services/ui/ws/frame_genera... services/ui/ws/frame_generator_unittest.cc:106: } On 2017/03/15 00:06:07, Fady Samuel wrote: > This unit test can probably collapse to look like this once you move things into > FrameGeneratorTest: > > TEST_F(FrameGeneratorTest, Creation) { > EXPECT_EQ(0, compositor_frame_sink()->number_frames_received()); > IssueBeginFrame(); > EXPECT_EQ(0, compositor_frame_sink()->number_frames_received()); > } Done.
Looking good so far! https://codereview.chromium.org/2755573002/diff/40001/services/ui/ws/frame_ge... File services/ui/ws/frame_generator.cc (right): https://codereview.chromium.org/2755573002/diff/40001/services/ui/ws/frame_ge... services/ui/ws/frame_generator.cc:42: if (window_manager_surface_info_.is_valid()) { nit: this change is unnecessary. https://codereview.chromium.org/2755573002/diff/40001/services/ui/ws/frame_ge... File services/ui/ws/frame_generator_unittest.cc (right): https://codereview.chromium.org/2755573002/diff/40001/services/ui/ws/frame_ge... services/ui/ws/frame_generator_unittest.cc:152: EXPECT_EQ(0, NumberOfFramesRecieved()); I'd like to see this do a bit more: EXPECT_EQ(0, NumberOfFramesRecieved()); // FrameGenerator does not request BeginFrames upon creation. IssueBeginFrame(...); EXPECT_EQ(0, NumberOfFramesRecieved()); frame_generator()->OnSurfaceCreated(cc::SurfaceInfo(MakeSurfaceId(), SomeSize, SomeDeviceScaleFactor)); EXPECT_EQ(0, NumberOfFramesRecieved()); // Changing the window manager's surface ID should trigger the creation // of a new CompositorFrame from FrameGenerator. IssueBeginFrame(...); EXPECT_EQ(1, NumberOfFramesRecieved()); // Might be useful inspecting the content of the frame here.
https://codereview.chromium.org/2755573002/diff/40001/services/ui/ws/frame_ge... File services/ui/ws/frame_generator.cc (right): https://codereview.chromium.org/2755573002/diff/40001/services/ui/ws/frame_ge... services/ui/ws/frame_generator.cc:42: if (window_manager_surface_info_.is_valid()) { On 2017/03/15 17:47:00, Fady Samuel wrote: > nit: this change is unnecessary. Done. https://codereview.chromium.org/2755573002/diff/40001/services/ui/ws/frame_ge... File services/ui/ws/frame_generator_unittest.cc (right): https://codereview.chromium.org/2755573002/diff/40001/services/ui/ws/frame_ge... services/ui/ws/frame_generator_unittest.cc:152: EXPECT_EQ(0, NumberOfFramesRecieved()); On 2017/03/15 17:47:00, Fady Samuel wrote: > I'd like to see this do a bit more: > > EXPECT_EQ(0, NumberOfFramesRecieved()); > > // FrameGenerator does not request BeginFrames upon creation. > IssueBeginFrame(...); > EXPECT_EQ(0, NumberOfFramesRecieved()); > > frame_generator()->OnSurfaceCreated(cc::SurfaceInfo(MakeSurfaceId(), SomeSize, > SomeDeviceScaleFactor)); > EXPECT_EQ(0, NumberOfFramesRecieved()); > > // Changing the window manager's surface ID should trigger the creation > // of a new CompositorFrame from FrameGenerator. > IssueBeginFrame(...); > EXPECT_EQ(1, NumberOfFramesRecieved()); > > // Might be useful inspecting the content of the frame here. Done.
https://codereview.chromium.org/2755573002/diff/60001/services/ui/ws/frame_ge... File services/ui/ws/frame_generator_unittest.cc (right): https://codereview.chromium.org/2755573002/diff/60001/services/ui/ws/frame_ge... services/ui/ws/frame_generator_unittest.cc:138: const cc::SurfaceId& id_last_frame() { Make this a bit more generic. ReferencedSurfaces()? Also, what if we haven't received a frame? perhaps referenced_surfaces can return an const std::vector<...>* instead of const ref? return nullptr if we don't have a CompositorFrame. https://codereview.chromium.org/2755573002/diff/60001/services/ui/ws/frame_ge... services/ui/ws/frame_generator_unittest.cc:175: EXPECT_EQ(kArbitrarySurfaceId, id); Make this a bit more generic? https://codereview.chromium.org/2755573002/diff/60001/services/ui/ws/frame_ge... services/ui/ws/frame_generator_unittest.cc:178: // TODO(staraz): Add OnSurfaceCreated test and SetDeviceScaleFactor test. A test for SetDeviceScaleFactor should be fairly simple too: SetDeviceScaleFactor(2); // Verify we don't yet have a new CompositorFrame. IssueBeginFrame(..); // Verify we have a new CompositorFrame. SetDeviceScaleFactor(2); // Verify we don't have a new CompositorFrame. IssueBeginFrame(..); // Verify we have a new CompositorFrame.
Is this ready to be reviewed again?
On 2017/03/16 20:34:20, Fady Samuel wrote: > Is this ready to be reviewed again? No. I'm to address your other comment about changing last_frame() into ReferencedSurfaces().
brianderson@chromium.org changed reviewers: + brianderson@chromium.org
https://codereview.chromium.org/2755573002/diff/100001/services/ui/ws/frame_g... File services/ui/ws/frame_generator_unittest.cc (right): https://codereview.chromium.org/2755573002/diff/100001/services/ui/ws/frame_g... services/ui/ws/frame_generator_unittest.cc:204: EXPECT_EQ(0, NumberOfFramesReceived()); @staraz mentioned this is 1 when MISSED frames are processed. On it's own, this is okay. https://codereview.chromium.org/2755573002/diff/100001/services/ui/ws/frame_g... services/ui/ws/frame_generator_unittest.cc:207: EXPECT_EQ(1, NumberOfFramesReceived()); @staraz mentioned this is 2 when MISSED frames are processed. This is the unexpected part. It seems like it would be better to not submit a frame if there isn't anything new, rather than ignore MISSED frames. Is that difficult to do? It would save an average of 8ms to get the frame up. I also worry about what happens if you get two NORMAL BeginFrames.
https://codereview.chromium.org/2755573002/diff/100001/services/ui/ws/frame_g... File services/ui/ws/frame_generator_unittest.cc (right): https://codereview.chromium.org/2755573002/diff/100001/services/ui/ws/frame_g... services/ui/ws/frame_generator_unittest.cc:207: EXPECT_EQ(1, NumberOfFramesReceived()); On 2017/03/16 20:46:07, brianderson wrote: > @staraz mentioned this is 2 when MISSED frames are processed. > > This is the unexpected part. It seems like it would be better to not submit a > frame if there isn't anything new, rather than ignore MISSED frames. > > Is that difficult to do? It would save an average of 8ms to get the frame up. I > also worry about what happens if you get two NORMAL BeginFrames. We got 2 here because in FrameGenerator::SetNeedsBeginFrame(), observing_begin_frames_ is updated after AddObserver(). AddObserver() calls OnBeginFrame(MISSED args) calls SetNeedsBeginFrames(false) but both the argument and observing_begin_frames_ are false at this point so it early returns at the first if statement. If we update observing_begin_frames_ before calling AddObserver()/RemoveObserver(), FrameGenerator only submit one CompositorFrame for the first (MISSED) args.
https://codereview.chromium.org/2755573002/diff/100001/services/ui/ws/frame_g... File services/ui/ws/frame_generator_unittest.cc (right): https://codereview.chromium.org/2755573002/diff/100001/services/ui/ws/frame_g... services/ui/ws/frame_generator_unittest.cc:207: EXPECT_EQ(1, NumberOfFramesReceived()); On 2017/03/16 20:52:16, Alex Z. wrote: > On 2017/03/16 20:46:07, brianderson wrote: > > @staraz mentioned this is 2 when MISSED frames are processed. > > > > This is the unexpected part. It seems like it would be better to not submit a > > frame if there isn't anything new, rather than ignore MISSED frames. > > > > Is that difficult to do? It would save an average of 8ms to get the frame up. > I > > also worry about what happens if you get two NORMAL BeginFrames. > > We got 2 here because in FrameGenerator::SetNeedsBeginFrame(), > observing_begin_frames_ is updated after AddObserver(). AddObserver() > calls OnBeginFrame(MISSED args) calls SetNeedsBeginFrames(false) but > both the argument and observing_begin_frames_ are false at this point > so it early returns at the first if statement. > > If we update observing_begin_frames_ before calling > AddObserver()/RemoveObserver(), FrameGenerator only submit one > CompositorFrame for the first (MISSED) args. Ohh, yea, that's a bug. I think you're right. We should move the observing_begin_frames_ BEFORE AddObserver...
https://codereview.chromium.org/2755573002/diff/60001/services/ui/ws/frame_ge... File services/ui/ws/frame_generator_unittest.cc (right): https://codereview.chromium.org/2755573002/diff/60001/services/ui/ws/frame_ge... services/ui/ws/frame_generator_unittest.cc:138: const cc::SurfaceId& id_last_frame() { On 2017/03/15 20:44:58, Fady Samuel wrote: > Make this a bit more generic. > > ReferencedSurfaces()? Also, what if we haven't received a frame? > > perhaps referenced_surfaces can return an const std::vector<...>* instead of > const ref? return nullptr if we don't have a CompositorFrame. Done. https://codereview.chromium.org/2755573002/diff/60001/services/ui/ws/frame_ge... services/ui/ws/frame_generator_unittest.cc:175: EXPECT_EQ(kArbitrarySurfaceId, id); On 2017/03/15 20:44:58, Fady Samuel wrote: > Make this a bit more generic? Done. https://codereview.chromium.org/2755573002/diff/60001/services/ui/ws/frame_ge... services/ui/ws/frame_generator_unittest.cc:178: // TODO(staraz): Add OnSurfaceCreated test and SetDeviceScaleFactor test. On 2017/03/15 20:44:58, Fady Samuel wrote: > A test for SetDeviceScaleFactor should be fairly simple too: > > SetDeviceScaleFactor(2); > // Verify we don't yet have a new CompositorFrame. > IssueBeginFrame(..); > // Verify we have a new CompositorFrame. > SetDeviceScaleFactor(2); > // Verify we don't have a new CompositorFrame. > IssueBeginFrame(..); > // Verify we have a new CompositorFrame. Done. https://codereview.chromium.org/2755573002/diff/100001/services/ui/ws/frame_g... File services/ui/ws/frame_generator_unittest.cc (right): https://codereview.chromium.org/2755573002/diff/100001/services/ui/ws/frame_g... services/ui/ws/frame_generator_unittest.cc:207: EXPECT_EQ(1, NumberOfFramesReceived()); On 2017/03/16 21:06:35, Fady Samuel wrote: > On 2017/03/16 20:52:16, Alex Z. wrote: > > On 2017/03/16 20:46:07, brianderson wrote: > > > @staraz mentioned this is 2 when MISSED frames are processed. > > > > > > This is the unexpected part. It seems like it would be better to not submit > a > > > frame if there isn't anything new, rather than ignore MISSED frames. > > > > > > Is that difficult to do? It would save an average of 8ms to get the frame > up. > > I > > > also worry about what happens if you get two NORMAL BeginFrames. > > > > We got 2 here because in FrameGenerator::SetNeedsBeginFrame(), > > observing_begin_frames_ is updated after AddObserver(). AddObserver() > > calls OnBeginFrame(MISSED args) calls SetNeedsBeginFrames(false) but > > both the argument and observing_begin_frames_ are false at this point > > so it early returns at the first if statement. > > > > If we update observing_begin_frames_ before calling > > AddObserver()/RemoveObserver(), FrameGenerator only submit one > > CompositorFrame for the first (MISSED) args. > > Ohh, yea, that's a bug. I think you're right. We should move the > observing_begin_frames_ BEFORE AddObserver... Done.
https://codereview.chromium.org/2755573002/diff/140001/services/ui/ws/frame_g... File services/ui/ws/frame_generator.cc (right): https://codereview.chromium.org/2755573002/diff/140001/services/ui/ws/frame_g... services/ui/ws/frame_generator.cc:192: observing_begin_frames_ = true; So I was looking at compositor_frame_sink_support.cc and I realized we are a bit more succinct there. Perhaps this is nicer: if (needs_begin_frame == observing_begin_frames_) return; observing_begin_frames_ = needs_begin_frame; if (needs_begin_frame) begin_frame_source_->AddObserver(this); else begin_frame_source_->RemoveObserver(this); https://codereview.chromium.org/2755573002/diff/140001/services/ui/ws/frame_g... File services/ui/ws/frame_generator_unittest.cc (right): https://codereview.chromium.org/2755573002/diff/140001/services/ui/ws/frame_g... services/ui/ws/frame_generator_unittest.cc:207: // FrameGenerator gets the missed BeginFrame when AddObserver() is called. This comment is weird here. I would just remove it. https://codereview.chromium.org/2755573002/diff/140001/services/ui/ws/frame_g... services/ui/ws/frame_generator_unittest.cc:217: EXPECT_EQ(kArbitrarySurfaceId, referenced_surfaces->front()); Use EXPECT_THAT? https://cs.chromium.org/chromium/src/cc/surfaces/compositor_frame_sink_suppor... https://codereview.chromium.org/2755573002/diff/140001/services/ui/ws/frame_g... services/ui/ws/frame_generator_unittest.cc:238: DCHECK_EQ(1, NumberOfFramesReceived()); Verify that the device scale factor in the CompositorFrame is the default? https://codereview.chromium.org/2755573002/diff/140001/services/ui/ws/frame_g... services/ui/ws/frame_generator_unittest.cc:241: DCHECK_EQ(1, NumberOfFramesReceived()); Either remove this or comment about what it does? https://codereview.chromium.org/2755573002/diff/140001/services/ui/ws/frame_g... services/ui/ws/frame_generator_unittest.cc:251: EXPECT_EQ(2, NumberOfFramesReceived()); Verify that the new compositorframe gets the new device scale factor?
https://codereview.chromium.org/2755573002/diff/140001/services/ui/ws/frame_g... File services/ui/ws/frame_generator.cc (right): https://codereview.chromium.org/2755573002/diff/140001/services/ui/ws/frame_g... services/ui/ws/frame_generator.cc:192: observing_begin_frames_ = true; On 2017/03/17 14:06:40, Fady Samuel wrote: > So I was looking at compositor_frame_sink_support.cc and I realized we are a bit > more succinct there. Perhaps this is nicer: > > if (needs_begin_frame == observing_begin_frames_) > return; > > observing_begin_frames_ = needs_begin_frame; > if (needs_begin_frame) > begin_frame_source_->AddObserver(this); > else > begin_frame_source_->RemoveObserver(this); Done. https://codereview.chromium.org/2755573002/diff/140001/services/ui/ws/frame_g... File services/ui/ws/frame_generator_unittest.cc (right): https://codereview.chromium.org/2755573002/diff/140001/services/ui/ws/frame_g... services/ui/ws/frame_generator_unittest.cc:207: // FrameGenerator gets the missed BeginFrame when AddObserver() is called. On 2017/03/17 14:06:40, Fady Samuel wrote: > This comment is weird here. I would just remove it. Done. I took this note while debugging and missed it while cleaning up. https://codereview.chromium.org/2755573002/diff/140001/services/ui/ws/frame_g... services/ui/ws/frame_generator_unittest.cc:217: EXPECT_EQ(kArbitrarySurfaceId, referenced_surfaces->front()); On 2017/03/17 14:06:40, Fady Samuel wrote: > Use EXPECT_THAT? > https://cs.chromium.org/chromium/src/cc/surfaces/compositor_frame_sink_suppor... Done. https://codereview.chromium.org/2755573002/diff/140001/services/ui/ws/frame_g... services/ui/ws/frame_generator_unittest.cc:238: DCHECK_EQ(1, NumberOfFramesReceived()); On 2017/03/17 14:06:40, Fady Samuel wrote: > Verify that the device scale factor in the CompositorFrame is the default? Done. https://codereview.chromium.org/2755573002/diff/140001/services/ui/ws/frame_g... services/ui/ws/frame_generator_unittest.cc:241: DCHECK_EQ(1, NumberOfFramesReceived()); On 2017/03/17 14:06:40, Fady Samuel wrote: > Either remove this or comment about what it does? Done. https://codereview.chromium.org/2755573002/diff/140001/services/ui/ws/frame_g... services/ui/ws/frame_generator_unittest.cc:251: EXPECT_EQ(2, NumberOfFramesReceived()); On 2017/03/17 14:06:40, Fady Samuel wrote: > Verify that the new compositorframe gets the new device scale factor? Done.
https://codereview.chromium.org/2755573002/diff/160001/services/ui/ws/frame_g... File services/ui/ws/frame_generator_unittest.cc (right): https://codereview.chromium.org/2755573002/diff/160001/services/ui/ws/frame_g... services/ui/ws/frame_generator_unittest.cc:205: DCHECK_EQ(0, NumberOfFramesReceived()); EXPECT_EQ https://codereview.chromium.org/2755573002/diff/160001/services/ui/ws/frame_g... services/ui/ws/frame_generator_unittest.cc:206: DCHECK_EQ(nullptr, ReferencedSurfaces()); EXPECT_EQ https://codereview.chromium.org/2755573002/diff/160001/services/ui/ws/frame_g... services/ui/ws/frame_generator_unittest.cc:236: DCHECK_EQ(0, NumberOfFramesReceived()); EXPECT_EQ https://codereview.chromium.org/2755573002/diff/160001/services/ui/ws/frame_g... services/ui/ws/frame_generator_unittest.cc:248: DCHECK_EQ(1, NumberOfFramesReceived()); EXPECT_EQ https://codereview.chromium.org/2755573002/diff/160001/services/ui/ws/frame_g... services/ui/ws/frame_generator_unittest.cc:252: DCHECK_EQ(1, NumberOfFramesReceived()); EXPECT_EQ https://codereview.chromium.org/2755573002/diff/160001/services/ui/ws/frame_g... services/ui/ws/frame_generator_unittest.cc:260: DCHECK(last_metadata); There's no point DCHECK'ing in a test. You should add an expectation (which works in a release build as well).
https://codereview.chromium.org/2755573002/diff/160001/services/ui/ws/frame_g... File services/ui/ws/frame_generator_unittest.cc (right): https://codereview.chromium.org/2755573002/diff/160001/services/ui/ws/frame_g... services/ui/ws/frame_generator_unittest.cc:205: DCHECK_EQ(0, NumberOfFramesReceived()); On 2017/03/17 15:04:19, Fady Samuel wrote: > EXPECT_EQ Done. https://codereview.chromium.org/2755573002/diff/160001/services/ui/ws/frame_g... services/ui/ws/frame_generator_unittest.cc:206: DCHECK_EQ(nullptr, ReferencedSurfaces()); On 2017/03/17 15:04:19, Fady Samuel wrote: > EXPECT_EQ Done. https://codereview.chromium.org/2755573002/diff/160001/services/ui/ws/frame_g... services/ui/ws/frame_generator_unittest.cc:236: DCHECK_EQ(0, NumberOfFramesReceived()); On 2017/03/17 15:04:20, Fady Samuel wrote: > EXPECT_EQ Done. https://codereview.chromium.org/2755573002/diff/160001/services/ui/ws/frame_g... services/ui/ws/frame_generator_unittest.cc:248: DCHECK_EQ(1, NumberOfFramesReceived()); On 2017/03/17 15:04:19, Fady Samuel wrote: > EXPECT_EQ Done. https://codereview.chromium.org/2755573002/diff/160001/services/ui/ws/frame_g... services/ui/ws/frame_generator_unittest.cc:252: DCHECK_EQ(1, NumberOfFramesReceived()); On 2017/03/17 15:04:19, Fady Samuel wrote: > EXPECT_EQ Done. https://codereview.chromium.org/2755573002/diff/160001/services/ui/ws/frame_g... services/ui/ws/frame_generator_unittest.cc:260: DCHECK(last_metadata); On 2017/03/17 15:04:19, Fady Samuel wrote: > There's no point DCHECK'ing in a test. You should add an expectation (which > works in a release build as well). Done.
lgtm
staraz@chromium.org changed reviewers: + msw@chromium.org
msw@chromium.org: Please review my changes. This CL adds unit tests to FrameGenerator. It also fixed a small bug exposed by the unit tests. Consider this as a follow up CL to the one you reviewed last week.
Again, I understand very little of frame sinks/generators, etc.; I'm relying on Fady's review competence here. Rubber stamp lgtm with nits and minor comments. https://codereview.chromium.org/2755573002/diff/180001/services/ui/ws/frame_g... File services/ui/ws/frame_generator_unittest.cc (right): https://codereview.chromium.org/2755573002/diff/180001/services/ui/ws/frame_g... services/ui/ws/frame_generator_unittest.cc:15: #include "testing/gmock/include/gmock/gmock.h" Is this used? (if just for EXPECT_THAT... UnorderedElementsAre), consider avoiding gmock and using some other expectation statements instead (should be simple to test for a vector with one member). https://codereview.chromium.org/2755573002/diff/180001/services/ui/ws/frame_g... services/ui/ws/frame_generator_unittest.cc:24: class TestFrameGeneratorDelegate : public FrameGeneratorDelegate { nit: class comment? https://codereview.chromium.org/2755573002/diff/180001/services/ui/ws/frame_g... services/ui/ws/frame_generator_unittest.cc:30: bool IsInHighContrastMode() override { return false; } aside: It's unfortunate if we need a delegate subclass just for this. https://codereview.chromium.org/2755573002/diff/180001/services/ui/ws/frame_g... services/ui/ws/frame_generator_unittest.cc:31: }; nit: DISALLOW_COPY_AND_ASSIGN? https://codereview.chromium.org/2755573002/diff/180001/services/ui/ws/frame_g... services/ui/ws/frame_generator_unittest.cc:33: class TestServerWindowDelegate : public ServerWindowDelegate { nit: class comment? https://codereview.chromium.org/2755573002/diff/180001/services/ui/ws/frame_g... services/ui/ws/frame_generator_unittest.cc:39: cc::mojom::DisplayCompositor* GetDisplayCompositor() override { ditto: It's unfortunate if we need a delegate subclass just for returning null. https://codereview.chromium.org/2755573002/diff/180001/services/ui/ws/frame_g... services/ui/ws/frame_generator_unittest.cc:46: }; nit: DISALLOW_COPY_AND_ASSIGN? https://codereview.chromium.org/2755573002/diff/180001/services/ui/ws/frame_g... services/ui/ws/frame_generator_unittest.cc:49: // CompositorFrame from a FrameGenerator. nit: 'a CompositorFrame', 'CompositorFrames' or 'CompositorFrame instances'? https://codereview.chromium.org/2755573002/diff/180001/services/ui/ws/frame_g... services/ui/ws/frame_generator_unittest.cc:143: }; nit: DISALLOW_COPY_AND_ASSIGN? https://codereview.chromium.org/2755573002/diff/180001/services/ui/ws/frame_g... services/ui/ws/frame_generator_unittest.cc:250: // FrameGenerator stops requesting BeginFrames after receiving one. nit: s/requesting/accepting/? https://codereview.chromium.org/2755573002/diff/180001/services/ui/ws/frame_g... services/ui/ws/frame_generator_unittest.cc:254: // FrameGenerator does not request BeginFrames if its device scale factor nit: s/request/accept/?
https://codereview.chromium.org/2755573002/diff/180001/services/ui/ws/frame_g... File services/ui/ws/frame_generator_unittest.cc (right): https://codereview.chromium.org/2755573002/diff/180001/services/ui/ws/frame_g... services/ui/ws/frame_generator_unittest.cc:15: #include "testing/gmock/include/gmock/gmock.h" On 2017/03/17 16:35:38, msw wrote: > Is this used? (if just for EXPECT_THAT... UnorderedElementsAre), consider > avoiding gmock and using some other expectation statements instead (should be > simple to test for a vector with one member). Ahh I suggested using gmock as other cc uses it. If we prefer to avoid gmock here then sgtm, let's avoid it. https://codereview.chromium.org/2755573002/diff/180001/services/ui/ws/frame_g... services/ui/ws/frame_generator_unittest.cc:30: bool IsInHighContrastMode() override { return false; } On 2017/03/17 16:35:38, msw wrote: > aside: It's unfortunate if we need a delegate subclass just for this. This will go away in a future CL. https://codereview.chromium.org/2755573002/diff/180001/services/ui/ws/frame_g... services/ui/ws/frame_generator_unittest.cc:250: // FrameGenerator stops requesting BeginFrames after receiving one. On 2017/03/17 16:35:38, msw wrote: > nit: s/requesting/accepting/? I think requesting is appropriate here. https://codereview.chromium.org/2755573002/diff/180001/services/ui/ws/frame_g... services/ui/ws/frame_generator_unittest.cc:254: // FrameGenerator does not request BeginFrames if its device scale factor On 2017/03/17 16:35:38, msw wrote: > nit: s/request/accept/? I think requesting is appropriate here.
https://codereview.chromium.org/2755573002/diff/180001/services/ui/ws/frame_g... File services/ui/ws/frame_generator_unittest.cc (right): https://codereview.chromium.org/2755573002/diff/180001/services/ui/ws/frame_g... services/ui/ws/frame_generator_unittest.cc:15: #include "testing/gmock/include/gmock/gmock.h" On 2017/03/17 16:55:31, Fady Samuel wrote: > On 2017/03/17 16:35:38, msw wrote: > > Is this used? (if just for EXPECT_THAT... UnorderedElementsAre), consider > > avoiding gmock and using some other expectation statements instead (should be > > simple to test for a vector with one member). > > Ahh I suggested using gmock as other cc uses it. If we prefer to avoid gmock > here then sgtm, let's avoid it. Done. https://codereview.chromium.org/2755573002/diff/180001/services/ui/ws/frame_g... services/ui/ws/frame_generator_unittest.cc:24: class TestFrameGeneratorDelegate : public FrameGeneratorDelegate { On 2017/03/17 16:35:38, msw wrote: > nit: class comment? Done. https://codereview.chromium.org/2755573002/diff/180001/services/ui/ws/frame_g... services/ui/ws/frame_generator_unittest.cc:31: }; On 2017/03/17 16:35:38, msw wrote: > nit: DISALLOW_COPY_AND_ASSIGN? Done. https://codereview.chromium.org/2755573002/diff/180001/services/ui/ws/frame_g... services/ui/ws/frame_generator_unittest.cc:33: class TestServerWindowDelegate : public ServerWindowDelegate { On 2017/03/17 16:35:38, msw wrote: > nit: class comment? Done. https://codereview.chromium.org/2755573002/diff/180001/services/ui/ws/frame_g... services/ui/ws/frame_generator_unittest.cc:46: }; On 2017/03/17 16:35:38, msw wrote: > nit: DISALLOW_COPY_AND_ASSIGN? Done. https://codereview.chromium.org/2755573002/diff/180001/services/ui/ws/frame_g... services/ui/ws/frame_generator_unittest.cc:49: // CompositorFrame from a FrameGenerator. On 2017/03/17 16:35:38, msw wrote: > nit: 'a CompositorFrame', 'CompositorFrames' or 'CompositorFrame instances'? Done. https://codereview.chromium.org/2755573002/diff/180001/services/ui/ws/frame_g... services/ui/ws/frame_generator_unittest.cc:143: }; On 2017/03/17 16:35:38, msw wrote: > nit: DISALLOW_COPY_AND_ASSIGN? Done.
https://codereview.chromium.org/2755573002/diff/200001/services/ui/ws/frame_g... File services/ui/ws/frame_generator_unittest.cc (right): https://codereview.chromium.org/2755573002/diff/200001/services/ui/ws/frame_g... services/ui/ws/frame_generator_unittest.cc:83: last_metadata_ = &last_frame_.metadata; No need for these two lines: last_referenced_surfaces_ and last_metadata_. https://codereview.chromium.org/2755573002/diff/200001/services/ui/ws/frame_g... services/ui/ws/frame_generator_unittest.cc:118: return last_referenced_surfaces_; I'm not entirely sure why this is returning a pointer? const std::vector<cc:SurfaceId>& last_referenced_surfaces() { return last_frame_.metadata.referenced_surfaces; } https://codereview.chromium.org/2755573002/diff/200001/services/ui/ws/frame_g... services/ui/ws/frame_generator_unittest.cc:121: const cc::CompositorFrameMetadata* last_metadata() { return last_metadata_; } I'm not entirely sure why this is returning a pointer? const cc::CompositorFrameMetadata& last_metadata() { return last_frame_.metadata; } https://codereview.chromium.org/2755573002/diff/200001/services/ui/ws/frame_g... services/ui/ws/frame_generator_unittest.cc:148: const std::vector<cc::SurfaceId>* last_referenced_surfaces_ = nullptr; remove this? https://codereview.chromium.org/2755573002/diff/200001/services/ui/ws/frame_g... services/ui/ws/frame_generator_unittest.cc:149: const cc::CompositorFrameMetadata* last_metadata_ = nullptr; remove this?
https://codereview.chromium.org/2755573002/diff/200001/services/ui/ws/frame_g... File services/ui/ws/frame_generator_unittest.cc (right): https://codereview.chromium.org/2755573002/diff/200001/services/ui/ws/frame_g... services/ui/ws/frame_generator_unittest.cc:83: last_metadata_ = &last_frame_.metadata; On 2017/03/17 19:22:39, Fady Samuel wrote: > No need for these two lines: last_referenced_surfaces_ and last_metadata_. Done. https://codereview.chromium.org/2755573002/diff/200001/services/ui/ws/frame_g... services/ui/ws/frame_generator_unittest.cc:118: return last_referenced_surfaces_; On 2017/03/17 19:22:39, Fady Samuel wrote: > I'm not entirely sure why this is returning a pointer? > > const std::vector<cc:SurfaceId>& last_referenced_surfaces() { > return last_frame_.metadata.referenced_surfaces; > } Done. https://codereview.chromium.org/2755573002/diff/200001/services/ui/ws/frame_g... services/ui/ws/frame_generator_unittest.cc:148: const std::vector<cc::SurfaceId>* last_referenced_surfaces_ = nullptr; On 2017/03/17 19:22:39, Fady Samuel wrote: > remove this? Done. https://codereview.chromium.org/2755573002/diff/200001/services/ui/ws/frame_g... services/ui/ws/frame_generator_unittest.cc:149: const cc::CompositorFrameMetadata* last_metadata_ = nullptr; On 2017/03/17 19:22:39, Fady Samuel wrote: > remove this? Done.
I don't see the addressed changes?
Still LGTM. This is ready for landing, now, I think.
Thanks!
The CQ bit was checked by staraz@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from msw@chromium.org Link to the patchset: https://codereview.chromium.org/2755573002/#ps240001 (title: "Addressed comments")
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: linux_trusty_blink_rel on master.tryserver.blink (JOB_FAILED, http://build.chromium.org/p/tryserver.blink/builders/linux_trusty_blink_rel/b...) cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) 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_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by 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: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...) linux_chromium_asan_rel_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_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...) win_clang on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)
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, msw@chromium.org Link to the patchset: https://codereview.chromium.org/2755573002/#ps280001 (title: "Added cc:test_support to pulic_deps")
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: 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
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: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) win_clang on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)
https://codereview.chromium.org/2755573002/diff/280001/services/ui/ws/frame_g... File services/ui/ws/frame_generator_unittest.cc (right): https://codereview.chromium.org/2755573002/diff/280001/services/ui/ws/frame_g... services/ui/ws/frame_generator_unittest.cc:23: class TestFrameGeneratorDelegate : public FrameGeneratorDelegate { Looks like you have a name collision with this: https://cs.chromium.org/chromium/src/services/ui/ws/test_utils.h?type=cs&q=Te... I would just use the one in test_utils.h
https://codereview.chromium.org/2755573002/diff/280001/services/ui/ws/frame_g... File services/ui/ws/frame_generator_unittest.cc (right): https://codereview.chromium.org/2755573002/diff/280001/services/ui/ws/frame_g... services/ui/ws/frame_generator_unittest.cc:23: class TestFrameGeneratorDelegate : public FrameGeneratorDelegate { On 2017/03/18 02:44:06, Fady Samuel wrote: > Looks like you have a name collision with this: > https://cs.chromium.org/chromium/src/services/ui/ws/test_utils.h?type=cs&q=Te... > > I would just use the one in test_utils.h Done.
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, msw@chromium.org Link to the patchset: https://codereview.chromium.org/2755573002/#ps300001 (title: "Removed TestCompositorFrameSink due to name conflict")
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": 1489806213031020,
"parent_rev": "fb239514f323bd9999a3e13fb5d8e3a27eddabd8", "commit_rev":
"8e89706698b46b5391e000fbd7175fd63cdf1a64"}
Message was sent while issue was closed.
Description was changed from ========== Add FrameGenerator Unit Tests Added FrameGeneratorTest and several mock classes to facilitate unit tests for FrameGenerator. Also added some explanatory comments to DisplayClientCompositorFrameSink. BUG=683732 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== Add FrameGenerator Unit Tests Added FrameGeneratorTest and several mock classes to facilitate unit tests for FrameGenerator. Also added some explanatory comments to DisplayClientCompositorFrameSink. BUG=683732 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel Review-Url: https://codereview.chromium.org/2755573002 Cr-Commit-Position: refs/heads/master@{#457954} Committed: https://chromium.googlesource.com/chromium/src/+/8e89706698b46b5391e000fbd717... ==========
Message was sent while issue was closed.
Committed patchset #15 (id:300001) as https://chromium.googlesource.com/chromium/src/+/8e89706698b46b5391e000fbd717... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
