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

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

Index: chrome/browser/media/router/mojo/media_route_controller.cc
diff --git a/chrome/browser/media/router/mojo/media_route_controller.cc b/chrome/browser/media/router/mojo/media_route_controller.cc
new file mode 100644
index 0000000000000000000000000000000000000000..acc8f4501a33ba5b41d56e2d59e4f3304e293584
--- /dev/null
+++ b/chrome/browser/media/router/mojo/media_route_controller.cc
@@ -0,0 +1,79 @@
+// 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 <utility>
+
+namespace media_router {
+
+MediaRouteController::Observer::Observer(
+    scoped_refptr<MediaRouteController> controller)
+    : controller_(std::move(controller)) {

[mark a. foltz, 2017/03/20 23:47:03]

Why did you choose move semantics here?
What if the caller wants to create multiple observers from the same controller?

If this is necessary, please document it in the .h.

[takumif, 2017/03/21 04:00:07]

My understanding is that passing-by-value then moving is similar to
passing-by-const-ref then copying, in that the original instance owned by the
caller is retained (when necessary). I think Daniel's comment on
media_route_controller.h in patch #5 confirms this, and in
media_route_controller.cc in the same patch he mentioned that this is
preferable. Please let me know if you think it is confusing and I should use the
latter though.

[mark a. foltz, 2017/03/22 00:41:43]

Sorry, I missed that this was an argument passed by copy so std::move makes
sense here.  My bad.

+  controller_->AddObserver(this);
+}
+
+MediaRouteController::Observer::~Observer() {
+  if (controller_)
+    controller_->RemoveObserver(this);
+}
+
+void MediaRouteController::Observer::OnControllerInvalidated() {
+  controller_ = nullptr;
+  OnControllerDisposed();
+}
+
+void MediaRouteController::Observer::OnControllerDisposed() {}

[mark a. foltz, 2017/03/20 23:47:03]

Maybe this should be OnControllerInvalidated() as it's a hook that subclasses
use for cleanup (acting like a callback).   This makes the naming consistent for
this code path.

[takumif, 2017/03/21 04:00:07]

Done.

+
+MediaRouteController::MediaRouteController(
+    const MediaRoute::Id& route_id,
+    mojom::MediaControllerPtr media_controller)
+    : route_id_(route_id), media_controller_(std::move(media_controller)) {
+  DCHECK(media_controller_.is_bound());
+  media_controller_.set_connection_error_handler(base::Bind(
+      &MediaRouteController::OnControllerInvalidated, base::Unretained(this)));
+}
+
+void MediaRouteController::Play() {
+  media_controller_->Play();
+}
+
+void MediaRouteController::Pause() {
+  media_controller_->Pause();
+}
+
+void MediaRouteController::Seek(base::TimeDelta time) {
+  media_controller_->Seek(time);
+}
+
+void MediaRouteController::SetMute(bool mute) {
+  media_controller_->SetMute(mute);
+}
+
+void MediaRouteController::SetVolume(float volume) {
+  media_controller_->SetVolume(volume);
+}
+
+void MediaRouteController::OnMediaStatusUpdated(const MediaStatus& status) {
+  for (Observer& observer : observers_)
+    observer.OnMediaStatusUpdated(status);
+}
+
+void MediaRouteController::OnControllerInvalidated() {

[mark a. foltz, 2017/03/20 23:47:03]

"On*" names I usually reserve for methods that must be bound or otherwise act as
callbacks.
If that's not the case, suggest "Invalidate()" here.

[takumif, 2017/03/21 04:00:07]

Changing to Invalidate().

+  for (Observer& observer : observers_)
+    observer.OnControllerInvalidated();

[mark a. foltz, 2017/03/20 23:47:03]

observer.Invalidate()

[takumif, 2017/03/21 04:00:07]

Changing to InvaildateController().

+  // |this| is deleted here!
+}
+
+MediaRouteController::~MediaRouteController() {}
+
+void MediaRouteController::AddObserver(Observer* observer) {
+  observers_.AddObserver(observer);
+}
+
+void MediaRouteController::RemoveObserver(Observer* observer) {
+  observers_.RemoveObserver(observer);
+}
+
+}  // namespace media_router