Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1417533006:
Unit test for WebMediaPlayerMS (Closed)
|
Created:
5 years, 2 months ago by qiangchen Modified:
5 years ago Reviewers:
mcasas CC:
chromium-reviews, mlamouri+watch-content_chromium.org, posciak+watch_chromium.org, jam, mcasas+watch_chromium.org, feature-media-reviews_chromium.org, darin-cc_chromium.org, mkwst+moarreviews-renderer_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionWe add a unit test for WebMediaPlayerMS in this CL.
BUG=
Committed: https://crrev.com/cb1fbb8a0c10b67e623bdbe57ca82680465c5cbb
Cr-Commit-Position: refs/heads/master@{#365575}
Patch Set 1 : Unittest #
Total comments: 12
Patch Set 2 : Except Mock Classes Restructure #Patch Set 3 : Rebase and merge some classes together #
Total comments: 26
Patch Set 4 : Style #
Total comments: 24
Patch Set 5 : Rebase #Patch Set 6 : Rebase #Patch Set 7 : Resolving Trybot Issues #
Total comments: 30
Patch Set 8 : Style fix #Patch Set 9 : Kill Memory Leak #
Messages
Total messages: 31 (11 generated)
Description was changed from ========== We add a unit test for WebMediaPlayerMS in this CL. BUG= ========== to ========== We add a unit test for WebMediaPlayerMS in this CL. BUG= ==========
Patchset #1 (id:1) has been deleted
qiangchen@chromium.org changed reviewers: + mcasas@chromium.org
Hi, Miguel:
Can you glance at this CL a little bit?
Before you dive into any detail, I have a question on file structure. As you
know to write the unittest for WebMediaPlayerMS, I need to write several Fake
classes to work with WebMediaPlayerMS (to replace the VideoFrameProvider, the
VideoFrameProviderClient etc).
Currently all those Fake classes stay in the same file with the unittest,
which makes the file very large and thus less readable. Is there a better way to
arrange those Fake classes?
Qiang
Some comments to get you going. https://chromiumcodereview.appspot.com/1417533006/diff/20001/content/renderer... File content/renderer/media/webmediaplayer_ms.h (right): https://chromiumcodereview.appspot.com/1417533006/diff/20001/content/renderer... content/renderer/media/webmediaplayer_ms.h:161: cc::VideoFrameProvider* LoadForTesting(bool algorithm_enabled); Load what exactly? Write a more significant comment. https://chromiumcodereview.appspot.com/1417533006/diff/20001/content/renderer... File content/renderer/media/webmediaplayer_ms_compositor.h (right): https://chromiumcodereview.appspot.com/1417533006/diff/20001/content/renderer... content/renderer/media/webmediaplayer_ms_compositor.h:48: // MediaStreamDescriptor. Incorrect, |url| is used to find the MediaStream WMPMSC is supposed to play. The usage of a sophisticated or more conservative rendering algorithm is decided elsewhere. https://chromiumcodereview.appspot.com/1417533006/diff/20001/content/renderer... content/renderer/media/webmediaplayer_ms_compositor.h:55: const bool algorithm_enabled); I think you could get rid of this ctor by: a) registering your MS with a MockMediaStreamRegistry [1] like is done in MediaRecorderHandlerTest [2] b) injecting the flag when appropriate using AppendSwitch() e.g. [3] [1] https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/m... [2] https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/m... [3] https://code.google.com/p/chromium/codesearch#chromium/src/content/test/webrt... https://chromiumcodereview.appspot.com/1417533006/diff/20001/content/renderer... File content/renderer/media/webmediaplayer_ms_unittest.cc (right): https://chromiumcodereview.appspot.com/1417533006/diff/20001/content/renderer... content/renderer/media/webmediaplayer_ms_unittest.cc:27: #include "third_party/libjingle/source/talk/app/webrtc/mediastreaminterface.h" Needed? (check all header please). https://chromiumcodereview.appspot.com/1417533006/diff/20001/content/renderer... content/renderer/media/webmediaplayer_ms_unittest.cc:33: class MockCB { There's indeed plenty of Mock/Test dependent classes. Check if you can find some other Mocks in other parts, if not (which I assume is the case), and you foresee this mocks/test classes to be reused, we can create a test utils file. Otherwise I'm afraid we'll have to keep them :) https://chromiumcodereview.appspot.com/1417533006/diff/20001/content/renderer... content/renderer/media/webmediaplayer_ms_unittest.cc:107: void QueueFrames(const std::string& str); I don't see much added value in creating/passing the Frame input array as a parsed string, and adds more complexity, why not a static array of timestamps and using negative numbers for markers?
Patchset #2 (id:40001) has been deleted
Fixed the minor issues. Still asking for opinion on where to put those Test classes. https://codereview.chromium.org/1417533006/diff/20001/content/renderer/media/... File content/renderer/media/webmediaplayer_ms.h (right): https://codereview.chromium.org/1417533006/diff/20001/content/renderer/media/... content/renderer/media/webmediaplayer_ms.h:161: cc::VideoFrameProvider* LoadForTesting(bool algorithm_enabled); On 2015/10/21 19:54:02, mcasas wrote: > Load what exactly? > Write a more significant comment. Done. https://codereview.chromium.org/1417533006/diff/20001/content/renderer/media/... File content/renderer/media/webmediaplayer_ms_compositor.h (right): https://codereview.chromium.org/1417533006/diff/20001/content/renderer/media/... content/renderer/media/webmediaplayer_ms_compositor.h:48: // MediaStreamDescriptor. On 2015/10/21 19:54:02, mcasas wrote: > Incorrect, |url| is used to find the MediaStream WMPMSC > is supposed to play. The usage of a sophisticated or more > conservative rendering algorithm is decided elsewhere. It is decided here. And the main usage of |url| is to decide algorithm or not. |url| --> remote? --> use algorithm? Added more comments. https://codereview.chromium.org/1417533006/diff/20001/content/renderer/media/... content/renderer/media/webmediaplayer_ms_compositor.h:55: const bool algorithm_enabled); On 2015/10/21 19:54:02, mcasas wrote: > I think you could get rid of this ctor by: > > a) registering your MS with a MockMediaStreamRegistry [1] > like is done in MediaRecorderHandlerTest [2] > b) injecting the flag when appropriate using AppendSwitch() > e.g. [3] > > [1] > https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/m... > [2] > https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/m... > [3] > https://code.google.com/p/chromium/codesearch#chromium/src/content/test/webrt... Plan a) sounds better, but here is some difficulty to implement it, so I'll follow plan b): 1. In load(), we used static function lookupMediaStreamDescriptor to find web stream, where static function can't be overriden. 2. In order to avoid that we have to pass web stream to load() as a parameter. However changing signature of load() is a big project itself, because it is an overriden function, and the origin function is in Blink. https://codereview.chromium.org/1417533006/diff/20001/content/renderer/media/... File content/renderer/media/webmediaplayer_ms_unittest.cc (right): https://codereview.chromium.org/1417533006/diff/20001/content/renderer/media/... content/renderer/media/webmediaplayer_ms_unittest.cc:27: #include "third_party/libjingle/source/talk/app/webrtc/mediastreaminterface.h" On 2015/10/21 19:54:02, mcasas wrote: > Needed? > > (check all header please). Done. Looks like many includes are included in test_helpers.h https://codereview.chromium.org/1417533006/diff/20001/content/renderer/media/... content/renderer/media/webmediaplayer_ms_unittest.cc:33: class MockCB { On 2015/10/21 19:54:02, mcasas wrote: > There's indeed plenty of Mock/Test dependent classes. > > Check if you can find some other Mocks in other parts, > if not (which I assume is the case), and you foresee this > mocks/test classes to be reused, we can create a test > utils file. Otherwise I'm afraid we'll have to keep them :) My investigation shows only content::VideoFrameProvider has a test subclass. But that test class can only generate frames at constant rate, and has no concept of special frames like "test brake" which is needed in our test case. All other classes has no test subclass at all. For reusability, I can only foresee that ReusableMessageLoopEvent could be used elsewhere. What's your opinion on where we should put those classes? https://codereview.chromium.org/1417533006/diff/20001/content/renderer/media/... content/renderer/media/webmediaplayer_ms_unittest.cc:107: void QueueFrames(const std::string& str); On 2015/10/21 19:54:02, mcasas wrote: > I don't see much added value in creating/passing the Frame > input array as a parsed string, and adds more complexity, > why not a static array of timestamps and using negative > numbers for markers? Done.
Patchset #3 (id:80001) has been deleted
Finally get time to do this CL. Main change in this patch: 1. Rebased 2. Merged call MockClient and MockVideoProviderClient into the Test class 3. For those Mock classes, merge definition and declaration together, a little more compact. The main framework of the unittest is to use message loop to simulate the frame incoming and rendering. You mentioned a simplified model as in one test we first input frames, and then do the rendering and test expectations. But I found some difficulty trying to implement that way: 1. For the algorithm disabled case, WebMediaPlayerMS stores only one frame, then we can't test anything on the simplified model. 2. For some sophisticated test case, I can't think of a way to realize on the simplified model: 2.a Calling pause(), we expect a frame freezing; 2.b For background rendering (real case is when the tab containing WebMediaPlayerMS is inactive, cc will no longer actively pull frames from WebMediaPlayerMS), we expect WebMediaPlayerMS to digest old frames in a timely manner rather than let frames pile up. 2.c For above two cases, we also expect the normal rendering can continue, when play() is called, or when background rendering is switched back to foreground rendering. I think one advantage of the realistic model is that in the long term if you encounter some bug and think that we should guard against it, we can easily write a test case.
A few comments to get you going. https://codereview.chromium.org/1417533006/diff/100001/content/renderer/media... File content/renderer/media/webmediaplayer_ms.cc (right): https://codereview.chromium.org/1417533006/diff/100001/content/renderer/media... content/renderer/media/webmediaplayer_ms.cc:67: if (!security_origin.isNull()) { no need for {} actually, you should prefer initialization over assignment, i.e. move this to the initializer list: initial_security_origin_(security_origin.isNull() ? url::Origin() : url::Origin(security_origin)); and probably you should keep |initial_security_origin_| const :) https://codereview.chromium.org/1417533006/diff/100001/content/renderer/media... File content/renderer/media/webmediaplayer_ms_compositor.cc (right): https://codereview.chromium.org/1417533006/diff/100001/content/renderer/media... content/renderer/media/webmediaplayer_ms_compositor.cc:389: } This is missing a // namespace content (it was missing it before, FTR). Add an empty line between l.388-389 https://codereview.chromium.org/1417533006/diff/100001/content/renderer/media... File content/renderer/media/webmediaplayer_ms_unittest.cc (right): https://codereview.chromium.org/1417533006/diff/100001/content/renderer/media... content/renderer/media/webmediaplayer_ms_unittest.cc:19: class WebMediaPlayerMSTest; Why this forward declaration? https://codereview.chromium.org/1417533006/diff/100001/content/renderer/media... content/renderer/media/webmediaplayer_ms_unittest.cc:51: class ReusableMessageLoopEvent { How's this class different from media::WaitableMessageLoopEvent? IOW, private inheritance == encapsulation of a single class is somehow seldom seen. I would be less surprised if you would do a class ReusableMessageLoopEvent : public media::WaitableMessageLoopEvent and override RunAndWait()/RunAndWaitForStatus() so the the parent's class |run_loop_| is reused. https://codereview.chromium.org/1417533006/diff/100001/content/renderer/media... content/renderer/media/webmediaplayer_ms_unittest.cc:91: started_ = true; Do not inline method bodies if the body is >= 2 lines. This is a flexible rule, if one of the 2 lines is a DCHECK(), use your common sense. https://codereview.chromium.org/1417533006/diff/100001/content/renderer/media... content/renderer/media/webmediaplayer_ms_unittest.cc:121: if (token == -1) { please add anonymous enums for -1 and -2, enum { FRAME_THAT_I_LIKE_A_LOT = -2, // I like this frame. FRAME_I_DONT_WANT_TO_SEE = -1, // I really dislike this one. ... }; https://codereview.chromium.org/1417533006/diff/100001/content/renderer/media... content/renderer/media/webmediaplayer_ms_unittest.cc:157: void InjectFrame() { If this is just meant to be run on |message_loop_|, could you please add a DCHECK(message_loop_->BelongsToCurrentThread()); ? https://codereview.chromium.org/1417533006/diff/100001/content/renderer/media... content/renderer/media/webmediaplayer_ms_unittest.cc:207: VideoFrameProvider::RepaintCB repaint_cb_; I believe all these 4 members can be const. https://codereview.chromium.org/1417533006/diff/100001/content/renderer/media... content/renderer/media/webmediaplayer_ms_unittest.cc:213: // The class is used to generate MockVideoProvider "... used to generate _a_ MockVideoProvider". https://codereview.chromium.org/1417533006/diff/100001/content/renderer/media... content/renderer/media/webmediaplayer_ms_unittest.cc:247: base::MessageLoop* message_loop_; const? https://codereview.chromium.org/1417533006/diff/100001/content/renderer/media... content/renderer/media/webmediaplayer_ms_unittest.cc:249: ReusableMessageLoopEvent* message_loop_controller_; const? https://codereview.chromium.org/1417533006/diff/100001/content/renderer/media... content/renderer/media/webmediaplayer_ms_unittest.cc:283: compositor_ = player_.GetCompositorForTesting(); You don't need GetCompositorForTesting() since WebMediaPlayerMSTest is a friend of WebMediaPlayerMS, remove it. https://codereview.chromium.org/1417533006/diff/100001/content/renderer/media... content/renderer/media/webmediaplayer_ms_unittest.cc:323: } No need for {} [1] " In general, curly braces are not required for single-line statements " [1] https://google.github.io/styleguide/cppguide.html#Conditionals
On 2015/11/04 22:20:32, mcasas wrote: > A few comments to get you going. > > https://codereview.chromium.org/1417533006/diff/100001/content/renderer/media... > File content/renderer/media/webmediaplayer_ms.cc (right): > > https://codereview.chromium.org/1417533006/diff/100001/content/renderer/media... > content/renderer/media/webmediaplayer_ms.cc:67: if (!security_origin.isNull()) { > no need for {} > > actually, you should prefer initialization over assignment, i.e. > move this to the initializer list: > > initial_security_origin_(security_origin.isNull() ? url::Origin() : > url::Origin(security_origin)); > > and probably you should keep |initial_security_origin_| const :) > > https://codereview.chromium.org/1417533006/diff/100001/content/renderer/media... > File content/renderer/media/webmediaplayer_ms_compositor.cc (right): > > https://codereview.chromium.org/1417533006/diff/100001/content/renderer/media... > content/renderer/media/webmediaplayer_ms_compositor.cc:389: } > This is missing a > // namespace content > (it was missing it before, FTR). > > Add an empty line between l.388-389 > > https://codereview.chromium.org/1417533006/diff/100001/content/renderer/media... > File content/renderer/media/webmediaplayer_ms_unittest.cc (right): > > https://codereview.chromium.org/1417533006/diff/100001/content/renderer/media... > content/renderer/media/webmediaplayer_ms_unittest.cc:19: class > WebMediaPlayerMSTest; > Why this forward declaration? > > https://codereview.chromium.org/1417533006/diff/100001/content/renderer/media... > content/renderer/media/webmediaplayer_ms_unittest.cc:51: class > ReusableMessageLoopEvent { > How's this class different from media::WaitableMessageLoopEvent? > > IOW, private inheritance == encapsulation of a single class > is somehow seldom seen. I would be less surprised if you > would do a > > class ReusableMessageLoopEvent : public media::WaitableMessageLoopEvent > > and override RunAndWait()/RunAndWaitForStatus() so > the the parent's class |run_loop_| is reused. > > https://codereview.chromium.org/1417533006/diff/100001/content/renderer/media... > content/renderer/media/webmediaplayer_ms_unittest.cc:91: started_ = true; > Do not inline method bodies if the body is >= 2 lines. > This is a flexible rule, if one of the 2 lines is a DCHECK(), > use your common sense. > > https://codereview.chromium.org/1417533006/diff/100001/content/renderer/media... > content/renderer/media/webmediaplayer_ms_unittest.cc:121: if (token == -1) { > please add anonymous enums for -1 and -2, > > enum { > FRAME_THAT_I_LIKE_A_LOT = -2, // I like this frame. > FRAME_I_DONT_WANT_TO_SEE = -1, // I really dislike this one. > ... > }; > > https://codereview.chromium.org/1417533006/diff/100001/content/renderer/media... > content/renderer/media/webmediaplayer_ms_unittest.cc:157: void InjectFrame() { > If this is just meant to be run on |message_loop_|, could you > please add a > DCHECK(message_loop_->BelongsToCurrentThread()); > ? > > https://codereview.chromium.org/1417533006/diff/100001/content/renderer/media... > content/renderer/media/webmediaplayer_ms_unittest.cc:207: > VideoFrameProvider::RepaintCB repaint_cb_; > I believe all these 4 members can be const. > > https://codereview.chromium.org/1417533006/diff/100001/content/renderer/media... > content/renderer/media/webmediaplayer_ms_unittest.cc:213: // The class is used > to generate MockVideoProvider > "... used to generate _a_ MockVideoProvider". > > https://codereview.chromium.org/1417533006/diff/100001/content/renderer/media... > content/renderer/media/webmediaplayer_ms_unittest.cc:247: base::MessageLoop* > message_loop_; > const? > > https://codereview.chromium.org/1417533006/diff/100001/content/renderer/media... > content/renderer/media/webmediaplayer_ms_unittest.cc:249: > ReusableMessageLoopEvent* message_loop_controller_; > const? > > https://codereview.chromium.org/1417533006/diff/100001/content/renderer/media... > content/renderer/media/webmediaplayer_ms_unittest.cc:283: compositor_ = > player_.GetCompositorForTesting(); > You don't need GetCompositorForTesting() since > WebMediaPlayerMSTest is a friend of > WebMediaPlayerMS, remove it. > > https://codereview.chromium.org/1417533006/diff/100001/content/renderer/media... > content/renderer/media/webmediaplayer_ms_unittest.cc:323: } > No need for {} [1] > " In general, curly braces are not required for single-line statements " > > [1] https://google.github.io/styleguide/cppguide.html#Conditionals Improve description too. What are you testing? WMPMS, or its compositor, or both or what? Have you got a BUG? otherwise remove BUG=
Fixed most. But I found some difficulty using inheritance for the WaitableMessageLoopEvent. See my comment for detail. https://codereview.chromium.org/1417533006/diff/100001/content/renderer/media... File content/renderer/media/webmediaplayer_ms.cc (right): https://codereview.chromium.org/1417533006/diff/100001/content/renderer/media... content/renderer/media/webmediaplayer_ms.cc:67: if (!security_origin.isNull()) { On 2015/11/04 22:20:30, mcasas wrote: > no need for {} > > actually, you should prefer initialization over assignment, i.e. > move this to the initializer list: > > initial_security_origin_(security_origin.isNull() ? url::Origin() : > url::Origin(security_origin)); > > and probably you should keep |initial_security_origin_| const :) Done. Nice idea. https://codereview.chromium.org/1417533006/diff/100001/content/renderer/media... File content/renderer/media/webmediaplayer_ms_compositor.cc (right): https://codereview.chromium.org/1417533006/diff/100001/content/renderer/media... content/renderer/media/webmediaplayer_ms_compositor.cc:389: } On 2015/11/04 22:20:30, mcasas wrote: > This is missing a > // namespace content > (it was missing it before, FTR). > > Add an empty line between l.388-389 Done. https://codereview.chromium.org/1417533006/diff/100001/content/renderer/media... File content/renderer/media/webmediaplayer_ms_unittest.cc (right): https://codereview.chromium.org/1417533006/diff/100001/content/renderer/media... content/renderer/media/webmediaplayer_ms_unittest.cc:19: class WebMediaPlayerMSTest; On 2015/11/04 22:20:31, mcasas wrote: > Why this forward declaration? Done. https://codereview.chromium.org/1417533006/diff/100001/content/renderer/media... content/renderer/media/webmediaplayer_ms_unittest.cc:51: class ReusableMessageLoopEvent { On 2015/11/04 22:20:31, mcasas wrote: > How's this class different from media::WaitableMessageLoopEvent? > > IOW, private inheritance == encapsulation of a single class > is somehow seldom seen. I would be less surprised if you > would do a > > class ReusableMessageLoopEvent : public media::WaitableMessageLoopEvent > > and override RunAndWait()/RunAndWaitForStatus() so > the the parent's class |run_loop_| is reused. The main reason I made this class is to deal with the following situation if we do not define the ReusableEvent: Say Class A has an instance of WaitableMessageLoopEvent, and Class B holds a pointer to that WaitableMessageLoopEvent. Say in A::method(), it calls WaitableMessageLoopEvent::RunAndWait(), and then it replaces the WaitableMessageLoopEvent with a brand new one for further testing work. The problem is that Class B now is holding a pointer to the old WaitableMessageLoopEvent which is useless any more. Inheritance and function overriding has a difficulty for now: the functions are not virtual. If we want to do that way, we have to modify test_helpers.h and .cc files. https://codereview.chromium.org/1417533006/diff/100001/content/renderer/media... content/renderer/media/webmediaplayer_ms_unittest.cc:91: started_ = true; On 2015/11/04 22:20:31, mcasas wrote: > Do not inline method bodies if the body is >= 2 lines. > This is a flexible rule, if one of the 2 lines is a DCHECK(), > use your common sense. Done. https://codereview.chromium.org/1417533006/diff/100001/content/renderer/media... content/renderer/media/webmediaplayer_ms_unittest.cc:121: if (token == -1) { On 2015/11/04 22:20:31, mcasas wrote: > please add anonymous enums for -1 and -2, > > enum { > FRAME_THAT_I_LIKE_A_LOT = -2, // I like this frame. > FRAME_I_DONT_WANT_TO_SEE = -1, // I really dislike this one. > ... > }; Done. Just pull the TestFrameType out to be an anonymous enum. https://codereview.chromium.org/1417533006/diff/100001/content/renderer/media... content/renderer/media/webmediaplayer_ms_unittest.cc:157: void InjectFrame() { On 2015/11/04 22:20:31, mcasas wrote: > If this is just meant to be run on |message_loop_|, could you > please add a > DCHECK(message_loop_->BelongsToCurrentThread()); > ? Done. https://codereview.chromium.org/1417533006/diff/100001/content/renderer/media... content/renderer/media/webmediaplayer_ms_unittest.cc:207: VideoFrameProvider::RepaintCB repaint_cb_; On 2015/11/04 22:20:31, mcasas wrote: > I believe all these 4 members can be const. Done. https://codereview.chromium.org/1417533006/diff/100001/content/renderer/media... content/renderer/media/webmediaplayer_ms_unittest.cc:213: // The class is used to generate MockVideoProvider On 2015/11/04 22:20:31, mcasas wrote: > "... used to generate _a_ MockVideoProvider". Done. https://codereview.chromium.org/1417533006/diff/100001/content/renderer/media... content/renderer/media/webmediaplayer_ms_unittest.cc:247: base::MessageLoop* message_loop_; On 2015/11/04 22:20:31, mcasas wrote: > const? Done. https://codereview.chromium.org/1417533006/diff/100001/content/renderer/media... content/renderer/media/webmediaplayer_ms_unittest.cc:249: ReusableMessageLoopEvent* message_loop_controller_; On 2015/11/04 22:20:31, mcasas wrote: > const? Done. https://codereview.chromium.org/1417533006/diff/100001/content/renderer/media... content/renderer/media/webmediaplayer_ms_unittest.cc:283: compositor_ = player_.GetCompositorForTesting(); On 2015/11/04 22:20:31, mcasas wrote: > You don't need GetCompositorForTesting() since > WebMediaPlayerMSTest is a friend of > WebMediaPlayerMS, remove it. Done. https://codereview.chromium.org/1417533006/diff/100001/content/renderer/media... content/renderer/media/webmediaplayer_ms_unittest.cc:323: } On 2015/11/04 22:20:30, mcasas wrote: > No need for {} [1] > " In general, curly braces are not required for single-line statements " > > [1] https://google.github.io/styleguide/cppguide.html#Conditionals Done.
ping mcasas@ to take a look at this.
ping mcasas@ to take a look at this.
Another round of comments about the accessory classes, getting there, sorry for the delay :P https://codereview.chromium.org/1417533006/diff/120001/content/renderer/media... File content/renderer/media/webmediaplayer_ms.cc (right): https://codereview.chromium.org/1417533006/diff/120001/content/renderer/media... content/renderer/media/webmediaplayer_ms.cc:67: : url::Origin(security_origin)) { url::Origin() has a ctor with no arguments, so you can just do initial_security_otiring_(security_origin) https://codereview.chromium.org/1417533006/diff/120001/content/renderer/media... File content/renderer/media/webmediaplayer_ms_unittest.cc (right): https://codereview.chromium.org/1417533006/diff/120001/content/renderer/media... content/renderer/media/webmediaplayer_ms_unittest.cc:33: }; Suggestion: Move all these MOCK_METHOD? to WebMediaPlayerMSTest, but prepend a "Do" to each name, e.g.: MOCK_METHOD0(DoStartRendering, void()); and set expectations on the Do... variants. Then: void WebMediaPlayerMSTest::StartRendering() { // Do stuff DoStartRendering(); // used to be mock_cb_.StartRendering(); } (You could bundle all those actions into the MOCKs via a .WillByDefault() on construction but is probably not worth it). https://codereview.chromium.org/1417533006/diff/120001/content/renderer/media... content/renderer/media/webmediaplayer_ms_unittest.cc:36: NORMAL_FRAME = 0, make this an enum class, instead of manipulating it as an integer. https://codereview.chromium.org/1417533006/diff/120001/content/renderer/media... content/renderer/media/webmediaplayer_ms_unittest.cc:48: }; I'd use here std::pair<scoped_refptr<VideoFrame>, FrameType>> no reason to add yet another data type. You can also rename it for convenience, e.g. using VideoFrameAndType = std::pair<scoped_refptr<VideoFrame>, FrameType>>; (assuming you rename the enum in l.35 to enum class FrameType) then just std::deque<VideoFrameAndType> frames_; https://codereview.chromium.org/1417533006/diff/120001/content/renderer/media... content/renderer/media/webmediaplayer_ms_unittest.cc:75: class MockVideoFrameProvider : public VideoFrameProvider { There's a similar class called TestVideoFrameProvider [1] and has most of the boilerplate ready, so I'd say why not making class MockVideoFrameProvider : public TestVideoFrameProvider and override the methods that interest you, i.e. GenerateFrame() ? (You might need to tweak that class making such method public virtual, and possibly move the file to another location). [1] https://code.google.com/p/chromium/codesearch#chromium/src/content/shell/rend... https://codereview.chromium.org/1417533006/diff/120001/content/renderer/media... content/renderer/media/webmediaplayer_ms_unittest.cc:205: } What about l.197-here as: for (const size_t i = 0; i < frames_.size(); ++i) { if (frames_[i].category == NORMAL_FRAME) { delay_ = (frames_[i].frame->timestamp() - frame.frame->timestamp()) / (i + 1); break; } } ? https://codereview.chromium.org/1417533006/diff/120001/content/renderer/media... content/renderer/media/webmediaplayer_ms_unittest.cc:220: // The class is used to generate a MockVideoProvider // This class is used to generate a MockVideoFrameProvider in WebMediaPlayerMS::load() https://codereview.chromium.org/1417533006/diff/120001/content/renderer/media... content/renderer/media/webmediaplayer_ms_unittest.cc:237: return static_cast<MockVideoFrameProvider*>(provider_.get()); Why not reinterpret_cast<MockVideoFrameProvider>(provider_).get() ? DCHECK(provider_); Actually never mind, read my next comment plz. https://codereview.chromium.org/1417533006/diff/120001/content/renderer/media... content/renderer/media/webmediaplayer_ms_unittest.cc:250: scoped_refptr<VideoFrameProvider> provider_; Suggestion: make this a const scoped_refptr<MockVideoFrameProvider> provider_; and construct it in ctor. Rename and inline the second GetVideoFrameProvider() to simply |provider()| and make it return the (shivers) naked pointer, whereas the first GetVideoFrameProvider returns a return reinterpret_cast<VideoFrameProvider>(provider_).get(); provider_(new MockVideoFrameProvider( message_loop_, message_loop_controller_, error_cb, repaint_cb)), and inline both GetVideoFrameProvider to just return it. Less verbose :) https://codereview.chromium.org/1417533006/diff/120001/content/renderer/media... content/renderer/media/webmediaplayer_ms_unittest.cc:339: base::MessageLoop message_loop_; All this passing around |message_loop_| as naked pointer is strange. Instead, why not passing its task_runner() and holding onto it in all the other classes as a scoped_refptr<SingleThreadTaskRunner> task_runner_; ? https://codereview.chromium.org/1417533006/diff/120001/content/renderer/media... content/renderer/media/webmediaplayer_ms_unittest.cc:370: EXPECT_NE(nullptr, provider); If you follow my recommendation in MockRenderFactory you don't need this nor l.357. (and you could also do EXPECT_TRUE(!!provider); )
Patchset #5 (id:140001) has been deleted
After a rebase. Fixed and replied. For the test class inheritance, I do not see much benefit doing that, as we cannot borrow any essential function from the exising TestVideoFrameProvider class. https://codereview.chromium.org/1417533006/diff/120001/content/renderer/media... File content/renderer/media/webmediaplayer_ms.cc (right): https://codereview.chromium.org/1417533006/diff/120001/content/renderer/media... content/renderer/media/webmediaplayer_ms.cc:67: : url::Origin(security_origin)) { On 2015/12/02 19:42:31, mcasas wrote: > url::Origin() has a ctor with no arguments, so you > can just do > initial_security_otiring_(security_origin) Ah, that does not work. Here security_origin is of type blink::WebSecurityOrigin initial_security_origin_ is url::Origin initial_security_origin_(security_origin) is essentially doing a implicit cast. The code is at https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... Where Unqiue() function asserts m_private, which means if security_origin.isNull(), an exception will be thrown, and then crashes. https://codereview.chromium.org/1417533006/diff/120001/content/renderer/media... File content/renderer/media/webmediaplayer_ms_unittest.cc (right): https://codereview.chromium.org/1417533006/diff/120001/content/renderer/media... content/renderer/media/webmediaplayer_ms_unittest.cc:33: }; On 2015/12/02 19:42:31, mcasas wrote: > Suggestion: > Move all these MOCK_METHOD? to WebMediaPlayerMSTest, > but prepend a "Do" to each name, e.g.: > > MOCK_METHOD0(DoStartRendering, void()); > > and set expectations on the Do... variants. Then: > > void WebMediaPlayerMSTest::StartRendering() { > // Do stuff > DoStartRendering(); // used to be mock_cb_.StartRendering(); > } > > (You could bundle all those actions into the MOCKs via a > .WillByDefault() on construction but is probably not worth it). Done. https://codereview.chromium.org/1417533006/diff/120001/content/renderer/media... content/renderer/media/webmediaplayer_ms_unittest.cc:36: NORMAL_FRAME = 0, On 2015/12/02 19:42:31, mcasas wrote: > make this an enum class, instead of manipulating > it as an integer. Done. https://codereview.chromium.org/1417533006/diff/120001/content/renderer/media... content/renderer/media/webmediaplayer_ms_unittest.cc:48: }; On 2015/12/02 19:42:31, mcasas wrote: > I'd use here > > std::pair<scoped_refptr<VideoFrame>, FrameType>> > > no reason to add yet another data type. > You can also rename it for convenience, e.g. > > using VideoFrameAndType = std::pair<scoped_refptr<VideoFrame>, FrameType>>; > > (assuming you rename the enum in l.35 to enum class FrameType) > > then just > std::deque<VideoFrameAndType> frames_; Done. https://codereview.chromium.org/1417533006/diff/120001/content/renderer/media... content/renderer/media/webmediaplayer_ms_unittest.cc:75: class MockVideoFrameProvider : public VideoFrameProvider { On 2015/12/02 19:42:31, mcasas wrote: > There's a similar class called TestVideoFrameProvider [1] and > has most of the boilerplate ready, so I'd say why not making > class MockVideoFrameProvider : public TestVideoFrameProvider > and override the methods that interest you, i.e. GenerateFrame() ? > (You might need to tweak that class making such method > public virtual, and possibly move the file to another location). > > > [1] > https://code.google.com/p/chromium/codesearch#chromium/src/content/shell/rend... I noticed that class, too on code search. But I decided not to inherit from that class, because the only functions that I do not need to override are the trivial ones like Stop, Play and Pause. The essential working functions like GenerateFrames (corresponding to InjectFrames in my class) are totally different. On the other hand, some settings in TestVideoFrameProvider as frame_duration will not be used in our case. So I do not see the benefit to inherit from that class. WDYT? https://codereview.chromium.org/1417533006/diff/120001/content/renderer/media... content/renderer/media/webmediaplayer_ms_unittest.cc:205: } On 2015/12/02 19:42:31, mcasas wrote: > What about l.197-here as: > > for (const size_t i = 0; i < frames_.size(); ++i) { > if (frames_[i].category == NORMAL_FRAME) { > delay_ = (frames_[i].frame->timestamp() - frame.frame->timestamp()) / > (i + 1); > break; > } > } > > ? Done. https://codereview.chromium.org/1417533006/diff/120001/content/renderer/media... content/renderer/media/webmediaplayer_ms_unittest.cc:220: // The class is used to generate a MockVideoProvider On 2015/12/02 19:42:31, mcasas wrote: > // This class is used to generate a MockVideoFrameProvider in > WebMediaPlayerMS::load() Done. https://codereview.chromium.org/1417533006/diff/120001/content/renderer/media... content/renderer/media/webmediaplayer_ms_unittest.cc:237: return static_cast<MockVideoFrameProvider*>(provider_.get()); On 2015/12/02 19:42:31, mcasas wrote: > Why not reinterpret_cast<MockVideoFrameProvider>(provider_).get() ? > > DCHECK(provider_); > > Actually never mind, read my next comment plz. Did the DCHECK. In my opinion, we should avoid reinterpret_cast as much as we can, because it is too strong that C++ almost allows you to cast anything to anything else. Let's keep using static_cast if possible. https://codereview.chromium.org/1417533006/diff/120001/content/renderer/media... content/renderer/media/webmediaplayer_ms_unittest.cc:250: scoped_refptr<VideoFrameProvider> provider_; On 2015/12/02 19:42:31, mcasas wrote: > Suggestion: make this a > const scoped_refptr<MockVideoFrameProvider> provider_; > > and construct it in ctor. Rename and inline the second > GetVideoFrameProvider() to simply |provider()| and make > it return the (shivers) naked pointer, whereas the first > GetVideoFrameProvider returns a > > return reinterpret_cast<VideoFrameProvider>(provider_).get(); > > > provider_(new MockVideoFrameProvider( > message_loop_, message_loop_controller_, error_cb, repaint_cb)), > > and inline both GetVideoFrameProvider to just return it. Less verbose :) One difficulty doing that way is the |error_cb| and |repaint_cb| are passed in when the function GetVideoFrameProvider is called, and we need that to generate a VideoFrameProvider. But at the Factory construction time, we do not have that information. We have to do this, because WebMediaPlayerMS::OnFrameAvailable is a private function, unless WebMediaPlayerMS pass it out as a callback, we cannot call it in the usual way. So let's keep doing it in the original way. https://codereview.chromium.org/1417533006/diff/120001/content/renderer/media... content/renderer/media/webmediaplayer_ms_unittest.cc:339: base::MessageLoop message_loop_; On 2015/12/02 19:42:31, mcasas wrote: > All this passing around |message_loop_| as naked pointer > is strange. Instead, why not passing its task_runner() and > holding onto it in all the other classes as a > scoped_refptr<SingleThreadTaskRunner> task_runner_; > ? Done. https://codereview.chromium.org/1417533006/diff/120001/content/renderer/media... content/renderer/media/webmediaplayer_ms_unittest.cc:370: EXPECT_NE(nullptr, provider); On 2015/12/02 19:42:31, mcasas wrote: > If you follow my recommendation in MockRenderFactory > you don't need this nor l.357. > (and you could also do EXPECT_TRUE(!!provider); ) N/A here, as I cannot follow the previous instruction.
https://codereview.chromium.org/1417533006/diff/120001/content/renderer/media... File content/renderer/media/webmediaplayer_ms_unittest.cc (right): https://codereview.chromium.org/1417533006/diff/120001/content/renderer/media... content/renderer/media/webmediaplayer_ms_unittest.cc:237: return static_cast<MockVideoFrameProvider*>(provider_.get()); On 2015/12/05 00:24:15, qiangchenC wrote: > On 2015/12/02 19:42:31, mcasas wrote: > > Why not reinterpret_cast<MockVideoFrameProvider>(provider_).get() ? > > > > DCHECK(provider_); > > > > Actually never mind, read my next comment plz. > > Did the DCHECK. > > In my opinion, we should avoid reinterpret_cast as much as we can, because it is > too strong that C++ almost allows you to cast anything to anything else. Let's > keep using static_cast if possible. Excuse me, my bad, I meant dynamic_cast, and not reinterpret_cast, since VideoFrameProvider and MockVFP are related by inheritance right? https://codereview.chromium.org/1417533006/diff/200001/content/renderer/media... File content/renderer/media/webmediaplayer_ms_unittest.cc (right): https://codereview.chromium.org/1417533006/diff/200001/content/renderer/media... content/renderer/media/webmediaplayer_ms_unittest.cc:6: #include "base/strings/string_split.h" Are these two includes used? Check the rest as well plz. https://codereview.chromium.org/1417533006/diff/200001/content/renderer/media... content/renderer/media/webmediaplayer_ms_unittest.cc:19: class MockCB { Unused? https://codereview.chromium.org/1417533006/diff/200001/content/renderer/media... content/renderer/media/webmediaplayer_ms_unittest.cc:61: // The class mainly used to inject VideoFrames into WebMediaPlayerMS. // This class is used mainly to... https://codereview.chromium.org/1417533006/diff/200001/content/renderer/media... content/renderer/media/webmediaplayer_ms_unittest.cc:83: void PrepareFrame(FrameType category, When I read this method's name I thought it would do sth to the |frame| argument, since it's "preparing" it, but the impl actually makes a pair with category and inserts it into |frames_|, so I propose renaming this method to sth more intuitive, e.g. AddCategoryAndFrame or similar. https://codereview.chromium.org/1417533006/diff/200001/content/renderer/media... content/renderer/media/webmediaplayer_ms_unittest.cc:85: void QueueFrames(const std::vector<int>& input); s/input/timestamps/, or s/input/categories/? |input| does not seem to explain what is inside... https://codereview.chromium.org/1417533006/diff/200001/content/renderer/media... content/renderer/media/webmediaplayer_ms_unittest.cc:92: // Main function that pushes frame into WebMediaPlayerMS .. to push a frame or that pushes a frame, because is only injecting 1 frame, right? https://codereview.chromium.org/1417533006/diff/200001/content/renderer/media... content/renderer/media/webmediaplayer_ms_unittest.cc:104: base::TimeDelta delay_; is this the |delay_of_next_generated_frame_| or what type of a delay is it...? https://codereview.chromium.org/1417533006/diff/200001/content/renderer/media... content/renderer/media/webmediaplayer_ms_unittest.cc:157: CHECK(false) << "Unrecognized token: " << token; Unreachable? Bc of l.137 and l.143, which could be just an if-else. https://codereview.chromium.org/1417533006/diff/200001/content/renderer/media... content/renderer/media/webmediaplayer_ms_unittest.cc:199: // This will pause the |message_loop_|, and the purpose is to allow main test to allow _the_ main... https://codereview.chromium.org/1417533006/diff/200001/content/renderer/media... content/renderer/media/webmediaplayer_ms_unittest.cc:350: EXPECT_EQ(nullptr, provider); Hmm no need to hold on to |provider| here, I'd say. Just write a simpler line, and perhaps add some helper line for the developers of the future. EXPECT_FALSE(render_factory->provider()) << "There shouldn't be a FrameProvider yet"; (this might complain in Win bots though, then just !! it?) then in l.362 MockVideoFrameProvider* const provider =... EXPECT_TRUE(provider); (same !! caveat as before). https://codereview.chromium.org/1417533006/diff/200001/content/renderer/media... content/renderer/media/webmediaplayer_ms_unittest.cc:426: // background rendering is not initiated from compositor_. |compositor_| here and in the previous line. https://codereview.chromium.org/1417533006/diff/200001/content/renderer/media... content/renderer/media/webmediaplayer_ms_unittest.cc:455: // resumed. I'd say this is a nice description that applies, with minor adjustments, to (almost) all the tests, so why not move it to WMPMSTest class, and instead write, for each test, a short description of what it looks for? E.g. for this test, you could write sth like: // This test sends a bunch of normal frames with increasing timestamps // and verifies that they are produced by WMPMS in the appropriate order. etc. (Try not using acronyms though. I know. It's a pain). https://codereview.chromium.org/1417533006/diff/200001/content/renderer/media... content/renderer/media/webmediaplayer_ms_unittest.cc:482: std::vector<int> timestamps(tokens, tokens + sizeof(tokens) / sizeof(int)); If you make |tokens| a static array, we have the arraysize() [1] utility. However I'd say just use directly std::array instead of std::vector, so it'd read const std::array<int, kArrayLength>timestamps = {0, 33, ... }; while you can still use .data(), .front(), .size() on it. [1] https://code.google.com/p/chromium/codesearch#chromium/src/base/macros.h&q=ar... https://codereview.chromium.org/1417533006/diff/200001/content/renderer/media... content/renderer/media/webmediaplayer_ms_unittest.cc:515: // Rendering will be "paused" at the TestBrake. |kTestBrake|. https://codereview.chromium.org/1417533006/diff/200001/content/renderer/media... content/renderer/media/webmediaplayer_ms_unittest.cc:520: // Here No more rendering. 1. Render should not be called, 2. Frame freeze. Correct syntax plz.
fixed most. For array vs vector. One obstruction is that array has length as part of the type definition. Then for function QueueFrames(std::array<int,?> input), whatever number you replace ? will force all test cases to have exactly same number of frames. https://codereview.chromium.org/1417533006/diff/120001/content/renderer/media... File content/renderer/media/webmediaplayer_ms_unittest.cc (right): https://codereview.chromium.org/1417533006/diff/120001/content/renderer/media... content/renderer/media/webmediaplayer_ms_unittest.cc:237: return static_cast<MockVideoFrameProvider*>(provider_.get()); On 2015/12/08 22:54:48, mcasas wrote: > On 2015/12/05 00:24:15, qiangchenC wrote: > > On 2015/12/02 19:42:31, mcasas wrote: > > > Why not reinterpret_cast<MockVideoFrameProvider>(provider_).get() ? > > > > > > DCHECK(provider_); > > > > > > Actually never mind, read my next comment plz. > > > > Did the DCHECK. > > > > In my opinion, we should avoid reinterpret_cast as much as we can, because it > is > > too strong that C++ almost allows you to cast anything to anything else. Let's > > keep using static_cast if possible. > > Excuse me, my bad, I meant dynamic_cast, and not > reinterpret_cast, since VideoFrameProvider and > MockVFP are related by inheritance right? Ah, I tried. But we need to enable RTTI for dynamic_cast to work. I do not think it worths here, because we are pretty sure the object we created in the factory is a MockVideoFrameProvider. The following link is where chromium enabled RTTI for one target, and it states "we typically do not support RTTI." https://code.google.com/p/chromium/codesearch#chromium/src/breakpad/BUILD.gn&... https://codereview.chromium.org/1417533006/diff/200001/content/renderer/media... File content/renderer/media/webmediaplayer_ms_unittest.cc (right): https://codereview.chromium.org/1417533006/diff/200001/content/renderer/media... content/renderer/media/webmediaplayer_ms_unittest.cc:6: #include "base/strings/string_split.h" On 2015/12/08 22:54:48, mcasas wrote: > Are these two includes used? Check the rest as well plz. Done. https://codereview.chromium.org/1417533006/diff/200001/content/renderer/media... content/renderer/media/webmediaplayer_ms_unittest.cc:19: class MockCB { On 2015/12/08 22:54:49, mcasas wrote: > Unused? Done. Sorry, unremoved Junk code. https://codereview.chromium.org/1417533006/diff/200001/content/renderer/media... content/renderer/media/webmediaplayer_ms_unittest.cc:61: // The class mainly used to inject VideoFrames into WebMediaPlayerMS. On 2015/12/08 22:54:49, mcasas wrote: > // This class is used mainly to... Done. https://codereview.chromium.org/1417533006/diff/200001/content/renderer/media... content/renderer/media/webmediaplayer_ms_unittest.cc:83: void PrepareFrame(FrameType category, On 2015/12/08 22:54:49, mcasas wrote: > When I read this method's name I thought it would do sth to > the |frame| argument, since it's "preparing" it, but the impl > actually makes a pair with category and inserts it into |frames_|, > so I propose renaming this method to sth more intuitive, e.g. > AddCategoryAndFrame or similar. I just changed it to AddFrame, and move it to private. https://codereview.chromium.org/1417533006/diff/200001/content/renderer/media... content/renderer/media/webmediaplayer_ms_unittest.cc:85: void QueueFrames(const std::vector<int>& input); On 2015/12/08 22:54:49, mcasas wrote: > s/input/timestamps/, or s/input/categories/? > |input| does not seem to explain what is inside... Done. https://codereview.chromium.org/1417533006/diff/200001/content/renderer/media... content/renderer/media/webmediaplayer_ms_unittest.cc:92: // Main function that pushes frame into WebMediaPlayerMS On 2015/12/08 22:54:49, mcasas wrote: > .. to push a frame > or > that pushes a frame, > because is only injecting 1 frame, right? Yep, each function call, one frame. https://codereview.chromium.org/1417533006/diff/200001/content/renderer/media... content/renderer/media/webmediaplayer_ms_unittest.cc:104: base::TimeDelta delay_; On 2015/12/08 22:54:48, mcasas wrote: > is this the |delay_of_next_generated_frame_| or what type > of a delay is it...? Done. https://codereview.chromium.org/1417533006/diff/200001/content/renderer/media... content/renderer/media/webmediaplayer_ms_unittest.cc:157: CHECK(false) << "Unrecognized token: " << token; On 2015/12/08 22:54:48, mcasas wrote: > Unreachable? Bc of l.137 and l.143, which could be > just an if-else. Done. From old string parse method. Add a MIN_TYPE, and do numeric check at beginning. https://codereview.chromium.org/1417533006/diff/200001/content/renderer/media... content/renderer/media/webmediaplayer_ms_unittest.cc:199: // This will pause the |message_loop_|, and the purpose is to allow main test On 2015/12/08 22:54:49, mcasas wrote: > to allow _the_ main... Done. https://codereview.chromium.org/1417533006/diff/200001/content/renderer/media... content/renderer/media/webmediaplayer_ms_unittest.cc:350: EXPECT_EQ(nullptr, provider); On 2015/12/08 22:54:48, mcasas wrote: > Hmm no need to hold on to |provider| here, I'd say. > Just write a simpler line, and perhaps add some helper line > for the developers of the future. > > EXPECT_FALSE(render_factory->provider()) << "There shouldn't be a FrameProvider > yet"; > (this might complain in Win bots though, then just !! it?) > > then in l.362 > MockVideoFrameProvider* const provider =... > EXPECT_TRUE(provider); > (same !! caveat as before). Done. https://codereview.chromium.org/1417533006/diff/200001/content/renderer/media... content/renderer/media/webmediaplayer_ms_unittest.cc:426: // background rendering is not initiated from compositor_. On 2015/12/08 22:54:48, mcasas wrote: > |compositor_| here and in the previous line. Done. https://codereview.chromium.org/1417533006/diff/200001/content/renderer/media... content/renderer/media/webmediaplayer_ms_unittest.cc:455: // resumed. On 2015/12/08 22:54:49, mcasas wrote: > I'd say this is a nice description that applies, with minor adjustments, > to (almost) all the tests, so why not move it to WMPMSTest class, and > instead write, for each test, a short description of what it looks for? > E.g. for this test, you could write sth like: > // This test sends a bunch of normal frames with increasing timestamps > // and verifies that they are produced by WMPMS in the appropriate order. > > etc. > (Try not using acronyms though. I know. It's a pain). Done. https://codereview.chromium.org/1417533006/diff/200001/content/renderer/media... content/renderer/media/webmediaplayer_ms_unittest.cc:482: std::vector<int> timestamps(tokens, tokens + sizeof(tokens) / sizeof(int)); On 2015/12/08 22:54:48, mcasas wrote: > If you make |tokens| a static array, we have the arraysize() [1] utility. > However I'd say just use directly std::array instead of std::vector, > so it'd read > > const std::array<int, kArrayLength>timestamps = {0, 33, ... }; > while you can still use .data(), .front(), .size() on it. > > [1] > https://code.google.com/p/chromium/codesearch#chromium/src/base/macros.h&q=ar... Ah, that's a good suggestion, but we will have to make the array size identical for all test cases, which I think is not a good idea, and inconvenient if we later need to add more test cases. Otherwise, what is the type of QueueFrames' parameter? std::array<int,?> https://codereview.chromium.org/1417533006/diff/200001/content/renderer/media... content/renderer/media/webmediaplayer_ms_unittest.cc:515: // Rendering will be "paused" at the TestBrake. On 2015/12/08 22:54:49, mcasas wrote: > |kTestBrake|. Let me remove this confusing comment. Because this pause is for message_loop_ not for WebMediaPlayerMS. It clearly states in the comment of Ln 199. https://codereview.chromium.org/1417533006/diff/200001/content/renderer/media... content/renderer/media/webmediaplayer_ms_unittest.cc:520: // Here No more rendering. 1. Render should not be called, 2. Frame freeze. On 2015/12/08 22:54:48, mcasas wrote: > Correct syntax plz. Done.
LGTM. std::vector<> is also OK if the input seqs are of different lengths.
The CQ bit was checked by qiangchen@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1417533006/240001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1417533006/240001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_arm64_dbg_recipe on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromeos_x86-generic_chromium_compile_only_ng on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by qiangchen@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1417533006/240001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1417533006/240001
Message was sent while issue was closed.
Description was changed from ========== We add a unit test for WebMediaPlayerMS in this CL. BUG= ========== to ========== We add a unit test for WebMediaPlayerMS in this CL. BUG= ==========
Message was sent while issue was closed.
Committed patchset #9 (id:240001)
Message was sent while issue was closed.
Description was changed from ========== We add a unit test for WebMediaPlayerMS in this CL. BUG= ========== to ========== We add a unit test for WebMediaPlayerMS in this CL. BUG= Committed: https://crrev.com/cb1fbb8a0c10b67e623bdbe57ca82680465c5cbb Cr-Commit-Position: refs/heads/master@{#365575} ==========
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/cb1fbb8a0c10b67e623bdbe57ca82680465c5cbb Cr-Commit-Position: refs/heads/master@{#365575}
Message was sent while issue was closed.
A revert of this CL (patchset #9 id:240001) has been created in https://codereview.chromium.org/1531073002/ by qiangchen@chromium.org. The reason for reverting is: Break test. Use of Uninitialized Variable.. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
