Replace MockFilterCallback with MockCallback and simplify unit tests.

We had a ton of unnecessarily duplicated code, most of which could be solved by introducing NewExpectedCallback().

BUG=none
TEST=media_unittests

Review URL: http://codereview.chromium.org/6350001

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@71537 0039d316-1c4b-4281-b951-d872f2087c98

5 files changed, 125 insertions, 149 deletions

diff --git a/media/base/composite_filter_unittest.cc b/media/base/composite_filter_unittest.cc
index fe757c3..4ce96c1 100644
--- a/media/base/composite_filter_unittest.cc
+++ b/media/base/composite_filter_unittest.cc
@@ -1,8 +1,9 @@
-// Copyright (c) 2010 The Chromium Authors. All rights reserved.
+// Copyright (c) 2011 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "media/base/composite_filter.h"
+#include "media/base/mock_callback.h"
 #include "media/base/mock_filter_host.h"
 #include "media/base/mock_filters.h"
 #include "testing/gtest/include/gtest/gtest.h"
@@ -209,9 +210,6 @@ void CompositeFilterTest::ExpectSuccess(MethodToCall method_to_call,
                                         base::TimeDelta seek_time) {
   InSequence seq;
 
-  scoped_ptr<StrictMock<MockFilterCallback> > mock_callback(
-      new StrictMock<MockFilterCallback>(false));
-
   bool is_parallel_call = (method_to_call == FLUSH);
 
   ExpectFilterCall(method_to_call, filter_1_.get(), seek_time);
@@ -221,8 +219,8 @@ void CompositeFilterTest::ExpectSuccess(MethodToCall method_to_call,
   }
 
   // Make method call on the composite.
-  DoFilterCall(method_to_call, composite_.get(), seek_time,
-               mock_callback->NewCallback());
+  StrictMock<MockCallback>* callback = new StrictMock<MockCallback>();
+  DoFilterCall(method_to_call, composite_.get(), seek_time, callback);
 
   if (is_parallel_call) {
     // Make sure both filters have their callbacks set.
@@ -243,7 +241,7 @@ void CompositeFilterTest::ExpectSuccess(MethodToCall method_to_call,
     EXPECT_TRUE(filter_2_callback_ != NULL);
   }
 
-  EXPECT_CALL(*mock_callback, OnFilterCallback());
+  callback->ExpectRunAndDelete();
 
   RunFilter2Callback();
 }
@@ -271,15 +269,11 @@ void CompositeFilterTest::DoSeek(base::TimeDelta time) {
 void CompositeFilterTest::ExpectInvalidStateFail(MethodToCall method_to_call,
                                      base::TimeDelta seek_time) {
   InSequence seq;
-  scoped_ptr<StrictMock<MockFilterCallback> > mock_callback(
-      new StrictMock<MockFilterCallback>(false));
 
   EXPECT_CALL(*mock_filter_host_, SetError(PIPELINE_ERROR_INVALID_STATE))
       .WillOnce(Return());
-  EXPECT_CALL(*mock_callback, OnFilterCallback());
 
-  DoFilterCall(method_to_call, composite_, seek_time,
-               mock_callback->NewCallback());
+  DoFilterCall(method_to_call, composite_, seek_time, NewExpectedCallback());
 
   // Make sure that neither of the filters were called by
   // verifying that the callback pointers weren't set.
@@ -364,15 +358,11 @@ TEST_F(CompositeFilterTest, TestPlay) {
   DoPlay();
 
   // At this point we are now in the kPlaying state.
-  scoped_ptr<StrictMock<MockFilterCallback> > mock_callback(
-      new StrictMock<MockFilterCallback>(false));
 
   // Try calling Play() again to make sure that we simply get a callback.
   // We are already in the Play() state so there is no point calling the
   // filters.
-  EXPECT_CALL(*mock_callback, OnFilterCallback());
-
-  composite_->Play(mock_callback->NewCallback());
+  composite_->Play(NewExpectedCallback());
 
   // Verify that neither of the filter callbacks were set.
   EXPECT_EQ((FilterCallback*)NULL, filter_1_callback_);
@@ -384,9 +374,7 @@ TEST_F(CompositeFilterTest, TestPlay) {
   // At this point we should be in the kStopped state.
 
   // Try calling Stop() again to make sure neither filter is called.
-  EXPECT_CALL(*mock_callback, OnFilterCallback());
-
-  composite_->Stop(mock_callback->NewCallback());
+  composite_->Stop(NewExpectedCallback());
 
   // Verify that neither of the filter callbacks were set.
   EXPECT_EQ((FilterCallback*)NULL, filter_1_callback_);
@@ -402,13 +390,11 @@ TEST_F(CompositeFilterTest, TestPlayErrors) {
 
   SetupAndAdd2Filters();
 
-  scoped_ptr<StrictMock<MockFilterCallback> > mock_callback(
-      new StrictMock<MockFilterCallback>(false));
-
   EXPECT_CALL(*filter_1_, Play(_));
 
   // Call Play() on the composite.
-  composite_->Play(mock_callback->NewCallback());
+  StrictMock<MockCallback>* callback = new StrictMock<MockCallback>();
+  composite_->Play(callback);
 
   EXPECT_CALL(*filter_2_, Play(_));
 
@@ -421,7 +407,7 @@ TEST_F(CompositeFilterTest, TestPlayErrors) {
 
   // Expect error to be reported and "play done" callback to be called.
   EXPECT_CALL(*mock_filter_host_, SetError(PIPELINE_ERROR_OUT_OF_MEMORY));
-  EXPECT_CALL(*mock_callback, OnFilterCallback());
+  callback->ExpectRunAndDelete();
 
   // Run callback to indicate that |filter_2_|'s Play() has completed.
   RunFilter2Callback();
@@ -441,9 +427,6 @@ TEST_F(CompositeFilterTest, TestPause) {
 
   SetupAndAdd2Filters();
 
-  scoped_ptr<StrictMock<MockFilterCallback> > mock_callback(
-      new StrictMock<MockFilterCallback>(false));
-
   // Try calling Pause() to make sure we get an error because we aren't in
   // the playing state.
   ExpectInvalidStateFail(PAUSE);
@@ -458,9 +441,7 @@ TEST_F(CompositeFilterTest, TestPause) {
 
   // Try calling Pause() again to make sure that the filters aren't called
   // because we are already in the paused state.
-  EXPECT_CALL(*mock_callback, OnFilterCallback());
-
-  composite_->Pause(mock_callback->NewCallback());
+  composite_->Pause(NewExpectedCallback());
 
   // Verify that neither of the filter callbacks were set.
   EXPECT_EQ((FilterCallback*)NULL, filter_1_callback_);
@@ -488,13 +469,11 @@ TEST_F(CompositeFilterTest, TestPauseErrors) {
 
   DoPlay();
 
-  scoped_ptr<StrictMock<MockFilterCallback> > mock_callback(
-      new StrictMock<MockFilterCallback>(false));
-
   EXPECT_CALL(*filter_1_, Pause(_));
 
   // Call Pause() on the composite.
-  composite_->Pause(mock_callback->NewCallback());
+  StrictMock<MockCallback>* callback = new StrictMock<MockCallback>();
+  composite_->Pause(callback);
 
   // Simulate an error by calling SetError() on |filter_1_|'s FilterHost
   // interface.
@@ -502,7 +481,7 @@ TEST_F(CompositeFilterTest, TestPauseErrors) {
 
   // Expect error to be reported and "pause done" callback to be called.
   EXPECT_CALL(*mock_filter_host_, SetError(PIPELINE_ERROR_OUT_OF_MEMORY));
-  EXPECT_CALL(*mock_callback, OnFilterCallback());
+  callback->ExpectRunAndDelete();
 
   RunFilter1Callback();
 
@@ -524,9 +503,6 @@ TEST_F(CompositeFilterTest, TestFlush) {
 
   SetupAndAdd2Filters();
 
-  scoped_ptr<StrictMock<MockFilterCallback> > mock_callback(
-      new StrictMock<MockFilterCallback>(false));
-
   // Make sure Flush() works before calling Play().
   DoFlush();
 
@@ -559,14 +535,12 @@ TEST_F(CompositeFilterTest, TestFlushErrors) {
 
   SetupAndAdd2Filters();
 
-  scoped_ptr<StrictMock<MockFilterCallback> > mock_callback(
-      new StrictMock<MockFilterCallback>(false));
-
   EXPECT_CALL(*filter_1_, Flush(_));
   EXPECT_CALL(*filter_2_, Flush(_));
 
   // Call Flush() on the composite.
-  composite_->Flush(mock_callback->NewCallback());
+  StrictMock<MockCallback>* callback = new StrictMock<MockCallback>();
+  composite_->Flush(callback);
 
   // Simulate an error by calling SetError() on |filter_1_|'s FilterHost
   // interface.
@@ -576,7 +550,7 @@ TEST_F(CompositeFilterTest, TestFlushErrors) {
 
   // Expect error to be reported and "pause done" callback to be called.
   EXPECT_CALL(*mock_filter_host_, SetError(PIPELINE_ERROR_OUT_OF_MEMORY));
-  EXPECT_CALL(*mock_callback, OnFilterCallback());
+  callback->ExpectRunAndDelete();
 
   RunFilter2Callback();
 
@@ -601,9 +575,6 @@ TEST_F(CompositeFilterTest, TestSeek) {
   // Verify we can issue a Play() after the Seek().
   DoPlay();
 
-  scoped_ptr<StrictMock<MockFilterCallback> > mock_callback(
-      new StrictMock<MockFilterCallback>(false));
-
   // Try calling Seek() while playing to make sure we get an error.
   ExpectInvalidStateFail(SEEK);
 
@@ -632,12 +603,11 @@ TEST_F(CompositeFilterTest, TestStop) {
 
   // Test error during Stop() sequence.
   SetupAndAdd2Filters();
-  scoped_ptr<StrictMock<MockFilterCallback> > mock_callback(
-      new StrictMock<MockFilterCallback>(false));
 
   EXPECT_CALL(*filter_1_, Stop(_));
 
-  composite_->Stop(mock_callback->NewCallback());
+  StrictMock<MockCallback>* callback = new StrictMock<MockCallback>();
+  composite_->Stop(callback);
 
   // Have |filter_1_| signal an error.
   filter_1_->host()->SetError(PIPELINE_ERROR_READ);
@@ -646,7 +616,7 @@ TEST_F(CompositeFilterTest, TestStop) {
 
   RunFilter1Callback();
 
-  EXPECT_CALL(*mock_callback, OnFilterCallback());
+  callback->ExpectRunAndDelete();
 
   RunFilter2Callback();
 }
@@ -657,21 +627,17 @@ TEST_F(CompositeFilterTest, TestStopWhilePlayPending) {
 
   SetupAndAdd2Filters();
 
-  scoped_ptr<StrictMock<MockFilterCallback> > mock_callback(
-      new StrictMock<MockFilterCallback>());
-
   EXPECT_CALL(*filter_1_, Play(_));
 
-  composite_->Play(mock_callback->NewCallback());
+  StrictMock<MockCallback>* callback = new StrictMock<MockCallback>();
+  composite_->Play(callback);
 
   // Note: Play() is pending on |filter_1_| right now.
 
-  scoped_ptr<StrictMock<MockFilterCallback> > mock_callback_2(
-      new StrictMock<MockFilterCallback>(false));
-
-  EXPECT_CALL(*mock_callback, OnCallbackDestroyed());
+  EXPECT_CALL(*callback, Destructor());
 
-  composite_->Stop(mock_callback_2->NewCallback());
+  callback = new StrictMock<MockCallback>();
+  composite_->Stop(callback);
 
   EXPECT_CALL(*filter_1_, Stop(_));
 
@@ -683,7 +649,7 @@ TEST_F(CompositeFilterTest, TestStopWhilePlayPending) {
   // Run |filter_1_|'s callback again to indicate Stop() has completed.
   RunFilter1Callback();
 
-  EXPECT_CALL(*mock_callback_2, OnFilterCallback());
+  callback->ExpectRunAndDelete();
 
   // Run |filter_2_|'s callback to indicate Stop() has completed.
   RunFilter2Callback();
@@ -695,22 +661,18 @@ TEST_F(CompositeFilterTest, TestStopWhileFlushPending) {
 
   SetupAndAdd2Filters();
 
-  scoped_ptr<StrictMock<MockFilterCallback> > mock_callback(
-      new StrictMock<MockFilterCallback>());
-
   EXPECT_CALL(*filter_1_, Flush(_));
   EXPECT_CALL(*filter_2_, Flush(_));
 
-  composite_->Flush(mock_callback->NewCallback());
+  StrictMock<MockCallback>* callback = new StrictMock<MockCallback>();
+  composite_->Flush(callback);
 
   // Note: |filter_1_| and |filter_2_| have pending Flush() calls at this point.
 
-  scoped_ptr<StrictMock<MockFilterCallback> > mock_callback_2(
-      new StrictMock<MockFilterCallback>(false));
-
-  EXPECT_CALL(*mock_callback, OnCallbackDestroyed());
+  EXPECT_CALL(*callback, Destructor());
 
-  composite_->Stop(mock_callback_2->NewCallback());
+  callback = new StrictMock<MockCallback>();
+  composite_->Stop(callback);
 
   // Run callback to indicate that |filter_1_|'s Flush() has completed.
   RunFilter1Callback();
@@ -725,7 +687,7 @@ TEST_F(CompositeFilterTest, TestStopWhileFlushPending) {
   // Run callback to indicate that |filter_1_|'s Stop() has completed.
   RunFilter1Callback();
 
-  EXPECT_CALL(*mock_callback_2, OnFilterCallback());
+  callback->ExpectRunAndDelete();
 
   // Run callback to indicate that |filter_2_|'s Stop() has completed.
   RunFilter2Callback();
@@ -767,33 +729,24 @@ TEST_F(CompositeFilterTest, TestEmptyComposite) {
 
   composite_->set_host(mock_filter_host_.get());
 
-  scoped_ptr<StrictMock<MockFilterCallback> > mock_callback(
-      new StrictMock<MockFilterCallback>(false));
-
   // Issue a Play() and expect no errors.
-  EXPECT_CALL(*mock_callback, OnFilterCallback());
-  composite_->Play(mock_callback->NewCallback());
+  composite_->Play(NewExpectedCallback());
 
   // Issue a Pause() and expect no errors.
-  EXPECT_CALL(*mock_callback, OnFilterCallback());
-  composite_->Pause(mock_callback->NewCallback());
+  composite_->Pause(NewExpectedCallback());
 
   // Issue a Flush() and expect no errors.
-  EXPECT_CALL(*mock_callback, OnFilterCallback());
-  composite_->Flush(mock_callback->NewCallback());
+  composite_->Flush(NewExpectedCallback());
 
   // Issue a Seek() and expect no errors.
-  EXPECT_CALL(*mock_callback, OnFilterCallback());
   composite_->Seek(base::TimeDelta::FromSeconds(5),
-                   mock_callback->NewCallback());
+                   NewExpectedCallback());
 
   // Issue a Play() and expect no errors.
-  EXPECT_CALL(*mock_callback, OnFilterCallback());
-  composite_->Play(mock_callback->NewCallback());
+  composite_->Play(NewExpectedCallback());
 
   // Issue a Stop() and expect no errors.
-  EXPECT_CALL(*mock_callback, OnFilterCallback());
-  composite_->Stop(mock_callback->NewCallback());
+  composite_->Stop(NewExpectedCallback());
 }
 
 }  // namespace media
diff --git a/media/base/mock_callback.cc b/media/base/mock_callback.cc
new file mode 100644
index 0000000..e948368
--- /dev/null
+++ b/media/base/mock_callback.cc
@@ -0,0 +1,29 @@
+// Copyright (c) 2011 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "media/base/mock_callback.h"
+
+using ::testing::_;
+using ::testing::StrictMock;
+
+namespace media {
+
+MockCallback::MockCallback() {}
+
+MockCallback::~MockCallback() {
+  Destructor();
+}
+
+void MockCallback::ExpectRunAndDelete() {
+  EXPECT_CALL(*this, RunWithParams(_));
+  EXPECT_CALL(*this, Destructor());
+}
+
+MockCallback* NewExpectedCallback() {
+  StrictMock<MockCallback>* callback = new StrictMock<MockCallback>();
+  callback->ExpectRunAndDelete();
+  return callback;
+}
+
+}  // namespace media
diff --git a/media/base/mock_callback.h b/media/base/mock_callback.h
new file mode 100644
index 0000000..2aef84a
--- /dev/null
+++ b/media/base/mock_callback.h
@@ -0,0 +1,57 @@
+// Copyright (c) 2011 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef MEDIA_BASE_MOCK_CALLBACK_H_
+#define MEDIA_BASE_MOCK_CALLBACK_H_
+
+#include "base/callback.h"
+#include "testing/gmock/include/gmock/gmock.h"
+
+namespace media {
+
+// Helper class used to test that callbacks are executed.
+//
+// In most cases NewExpectedCallback() can be used but if need be you can
+// manually set expectations on an MockCallback object:
+//
+//   StrictMock<MockCallback>* callback =
+//       new StrictMock<MockCallback>();
+//   EXPECT_CALL(*callback, RunWithParams(_));
+//   EXPECT_CALL(*callback, Destructor());
+//
+// ...or the equivalent and less verbose:
+//   StrictMock<MockCallback>* callback =
+//       new StrictMock<MockCallback>();
+//   callback->ExpectRunAndDelete();
+//
+// ...or if you don't care about verifying callback deletion:
+//
+//   NiceMock<MockCallback>* callback =
+//       new NiceMock<MockCallback>();
+//   EXPECT_CALL(*callback, RunWithParams(_));
+class MockCallback : public CallbackRunner<Tuple0> {
+ public:
+  MockCallback();
+  virtual ~MockCallback();
+
+  MOCK_METHOD1(RunWithParams, void(const Tuple0& params));
+
+  // Can be used to verify the object is destroyed.
+  MOCK_METHOD0(Destructor, void());
+
+  // Convenience function to set expectations for the callback to execute and
+  // deleted.
+  void ExpectRunAndDelete();
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(MockCallback);
+};
+
+// Convenience function that automatically creates and sets an expectation for
+// the callback to run.
+MockCallback* NewExpectedCallback();
+
+}  // namespace media
+
+#endif  // MEDIA_BASE_MOCK_CALLBACK_H_
diff --git a/media/base/mock_filters.cc b/media/base/mock_filters.cc
index 39ed730..90df69b 100644
--- a/media/base/mock_filters.cc
+++ b/media/base/mock_filters.cc
@@ -6,18 +6,6 @@
 
 namespace media {
 
-MockFilterCallback::MockFilterCallback() : run_destroy_callback_(true) {}
-
-MockFilterCallback::MockFilterCallback(bool run_destroy_callback) :
-    run_destroy_callback_(run_destroy_callback) {
-}
-
-MockFilterCallback::~MockFilterCallback() {}
-
-FilterCallback* MockFilterCallback::NewCallback() {
-  return new CallbackImpl(this, run_destroy_callback_);
-}
-
 MockDataSource::MockDataSource() {}
 
 MockDataSource::~MockDataSource() {}
diff --git a/media/base/mock_filters.h b/media/base/mock_filters.h
index 5f43356..45c1e29 100644
--- a/media/base/mock_filters.h
+++ b/media/base/mock_filters.h
@@ -15,7 +15,6 @@
 
 #include <string>
 
-#include "base/callback.h"
 #include "media/base/filters.h"
 #include "media/base/filter_collection.h"
 #include "media/base/video_frame.h"
@@ -44,56 +43,6 @@ class Destroyable : public MockClass {
   DISALLOW_COPY_AND_ASSIGN(Destroyable);
 };
 
-// Helper class used to test that callbacks are executed.  It is recommend you
-// combine this class with StrictMock<> to verify that the callback is executed.
-// You can reuse the same instance of a MockFilterCallback many times since
-// gmock will track the number of times the methods are executed.
-class MockFilterCallback {
- public:
-  MockFilterCallback();
-  MockFilterCallback(bool run_destroy_callback);
-  virtual ~MockFilterCallback();
-
-  MOCK_METHOD0(OnCallbackDestroyed, void());
-  MOCK_METHOD0(OnFilterCallback, void());
-
-  // Helper method to create a new callback for this mock.  The callback will
-  // call OnFilterCallback() when executed and OnCallbackDestroyed() when
-  // destroyed.  Clients should use NiceMock<> or StrictMock<> depending on the
-  // test.
-  FilterCallback* NewCallback();
-
- private:
-  // Private implementation of CallbackRunner used to trigger expectations on
-  // MockFilterCallback.
-  class CallbackImpl : public CallbackRunner<Tuple0> {
-   public:
-    explicit CallbackImpl(MockFilterCallback* mock_callback,
-                          bool run_destroy_callback)
-        : mock_callback_(mock_callback),
-          run_destroy_callback_(run_destroy_callback) {
-    }
-
-    virtual ~CallbackImpl() {
-      if (run_destroy_callback_)
-        mock_callback_->OnCallbackDestroyed();
-    }
-
-    virtual void RunWithParams(const Tuple0& params) {
-      mock_callback_->OnFilterCallback();
-    }
-
-   private:
-    MockFilterCallback* mock_callback_;
-    bool run_destroy_callback_;
-
-    DISALLOW_COPY_AND_ASSIGN(CallbackImpl);
-  };
-
-  bool run_destroy_callback_;
-  DISALLOW_COPY_AND_ASSIGN(MockFilterCallback);
-};
-
 class MockFilter : public Filter {
  public:
   MockFilter();