[Media Router] Custom Controls 1 - Add MediaStatus, MediaRouteController, and mojo interfaces

chrome/browser/media/router/mojo/media_route_controller_unittest.cc

+// Copyright 2017 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 "chrome/browser/media/router/mojo/media_route_controller.h"
+
+#include <string>
+#include <utility>
+
+#include "base/run_loop.h"
+#include "chrome/browser/media/router/mojo/media_router_mojo_test.h"
+#include "testing/gmock/include/gmock/gmock.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+using ::testing::StrictMock;
+
+namespace media_router {
+
+class MockMediaController : public mojom::MediaController {
+ public:
+  MOCK_METHOD0(Play, void());
+  MOCK_METHOD0(Pause, void());
+  MOCK_METHOD1(SetMute, void(bool mute));
+  MOCK_METHOD1(SetVolume, void(float volume));
+  MOCK_METHOD1(Seek, void(base::TimeDelta time));
+};
+
+class MockMediaRouteControllerObserver : public MediaRouteController::Observer {
+ public:
+  MockMediaRouteControllerObserver(
+      scoped_refptr<MediaRouteController> controller)
+      : MediaRouteController::Observer(controller) {}
+
+  MOCK_METHOD1(OnMediaStatusUpdated, void(const MediaStatus& status));
+};
+
+class MediaRouteControllerUnitTest : public ::testing::Test {

[mark a. foltz, 2017/03/17 23:55:04]

Nit: drop "Unit"

[takumif, 2017/03/20 22:10:45]

Done.

+ public:
+  MediaRouteControllerUnitTest() {}
+  ~MediaRouteControllerUnitTest() override {}
+
+  void SetUp() override {
+    mojom::MediaControllerPtr media_controller_ptr;
+    mojom::MediaControllerRequest media_controller_request =
+        mojo::MakeRequest(&media_controller_ptr);
+    mock_media_controller_ = base::MakeUnique<MockMediaController>();

[mark a. foltz, 2017/03/17 23:55:04]

This can be a class member.

[takumif, 2017/03/20 22:10:45]

Done.

+    media_controller_binding_ =
+        base::MakeUnique<mojo::Binding<mojom::MediaController>>(
+            mock_media_controller_.get(), std::move(media_controller_request));
+
+    observer_ = base::MakeUnique<MockMediaRouteControllerObserver>(
+        new MediaRouteController(
+            "routeId", std::move(media_controller_ptr),
+            base::Bind(&MediaRouteControllerUnitTest::OnControllerDestroyed,
+                       base::Unretained(this))));
+  }
+
+  MediaRouteController* GetController() {
+    return observer_ ? observer_->controller() : nullptr;

[mark a. foltz, 2017/03/17 23:55:05]

I think this will be non-null while the test cases execute.

[takumif, 2017/03/20 22:10:45]

In DestroyControllerOnNoObservers we reset |observer_| so that the controller
has one fewer observer. I'm moving that test case to the next CL though.

+  }
+
+  MOCK_METHOD0(OnControllerDestroyed, void());
+
+ protected:
+  std::unique_ptr<MockMediaController> mock_media_controller_;
+  std::unique_ptr<mojo::Binding<mojom::MediaController>>
+      media_controller_binding_;
+  std::unique_ptr<MockMediaRouteControllerObserver> observer_;
+
+  content::TestBrowserThreadBundle test_thread_bundle_;
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(MediaRouteControllerUnitTest);
+};
+
+TEST_F(MediaRouteControllerUnitTest, ForwardControllerCommands) {
+  const float volume = 0.5;
+  const base::TimeDelta time = base::TimeDelta::FromSeconds(42);
+
+  EXPECT_CALL(*mock_media_controller_, Play());
+  GetController()->Play();
+  EXPECT_CALL(*mock_media_controller_, Pause());
+  GetController()->Pause();
+  EXPECT_CALL(*mock_media_controller_, SetMute(true));
+  GetController()->SetMute(true);
+  EXPECT_CALL(*mock_media_controller_, SetVolume(volume));
+  GetController()->SetVolume(volume);
+  EXPECT_CALL(*mock_media_controller_, Seek(time));
+  GetController()->Seek(time);
+}
+
+TEST_F(MediaRouteControllerUnitTest, NotifyMediaRouteControllerObservers) {
+  std::unique_ptr<StrictMock<MockMediaRouteControllerObserver>> observer1 =
+      base::MakeUnique<StrictMock<MockMediaRouteControllerObserver>>(
+          GetController());
+  std::unique_ptr<StrictMock<MockMediaRouteControllerObserver>> observer2 =
+      base::MakeUnique<StrictMock<MockMediaRouteControllerObserver>>(
+          GetController());
+
+  MediaStatus status;
+  status.title = "test media status";
+
+  EXPECT_CALL(*observer1, OnMediaStatusUpdated(status));
+  EXPECT_CALL(*observer2, OnMediaStatusUpdated(status));
+  // TODO(takumif): Use the controller's mojo interface pointer here.

[mark a. foltz, 2017/03/17 23:55:04]

Not sure I follow this comment? I didn't see a code path where the controller
gets an update from Mojo.

[takumif, 2017/03/20 22:10:45]

As seen in the patch before this one, the controller will be receiving status
updates through a MediaStatusObserverPtr bound to it. The method to obtain an
observer pointer was moved to the next CL.

+  GetController()->OnMediaStatusUpdated(status);
+
+  observer1.reset();
+  std::unique_ptr<StrictMock<MockMediaRouteControllerObserver>> observer3 =
+      base::MakeUnique<StrictMock<MockMediaRouteControllerObserver>>(
+          GetController());
+
+  EXPECT_CALL(*observer2, OnMediaStatusUpdated(status));
+  EXPECT_CALL(*observer3, OnMediaStatusUpdated(status));
+  // TODO(takumif): Use the controller's mojo interface pointer here.
+  GetController()->OnMediaStatusUpdated(status);
+}
+
+TEST_F(MediaRouteControllerUnitTest, DestroyControllerOnDisconnect) {
+  // OnControllerDestroyed() should be called when the connection to
+  // |mock_media_controller_| is invalidated.
+  EXPECT_CALL(*this, OnControllerDestroyed()).Times(1);
+  media_controller_binding_.reset();
+}
+
+TEST_F(MediaRouteControllerUnitTest, DestroyControllerOnNoObservers) {
+  std::unique_ptr<MockMediaRouteControllerObserver> observer1 =

[mark a. foltz, 2017/03/17 23:55:05]

IMO, this would be less boilerplate with:

std::unique_ptr<> observer1(getController()->AddObserver())

[takumif, 2017/03/20 22:10:45]

Is this tied to your other comment that the Observer's ctor should be private
and accessible by MRController? I don't think MRController should know about
specific implementations of Observer.

[mark a. foltz, 2017/03/21 00:05:21]

Yeah, if we are constructing the Observers separately then does not seem
avoidable.

Consider factoring out a utility method to shorten this:

auto observer1 = CreateObserver();

+      base::MakeUnique<MockMediaRouteControllerObserver>(GetController());
+  std::unique_ptr<MockMediaRouteControllerObserver> observer2 =
+      base::MakeUnique<MockMediaRouteControllerObserver>(GetController());
+  // Get rid of |observer_| and its reference to the controller.
+  observer_.reset();
+
+  EXPECT_CALL(*this, OnControllerDestroyed()).Times(0);
+  observer1.reset();
+
+  // OnControllerDestroyed() should be called when the controller no longer has
+  // any observers.
+  EXPECT_CALL(*this, OnControllerDestroyed()).Times(1);
+  observer2.reset();
+}
+
+}  // namespace media_router