[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..2f122329b312c434608af05530ff57a53e92c27b
--- /dev/null
+++ b/chrome/browser/media/router/mojo/media_route_controller.cc
@@ -0,0 +1,87 @@
+// 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)) {

[imcheng, 2017/03/16 20:00:43]

No need to move, scoped_refptr supports copying.

[dcheng, 2017/03/17 06:15:14]

This is actually the preferred idiom.

[takumif, 2017/03/17 21:48:01]

To clarify, are you saying that pass-by-value and moving are preferred? Is the
reason for that because pass-by-value might be able to move instead of copy?
(Why is pass-by-ref preferred for closures below?)

+  controller_->AddObserver(this);
+}
+
+MediaRouteController::Observer::~Observer() {
+  if (controller_)
+    controller_->RemoveObserver(this);
+}
+
+void MediaRouteController::Observer::OnControllerInvalidated() {
+  controller_ = nullptr;
+}
+
+MediaRouteController::MediaRouteController(
+    MediaRoute::Id route_id,
+    mojom::MediaControllerPtr media_controller,
+    base::Closure destructor_callback)
+    : destructor_callback_(destructor_callback),

[dcheng, 2017/03/17 06:15:14]

Pass closures by const ref (you can also pass by value and std::move() if you
want, but the convention is to pass by const ref)

[takumif, 2017/03/17 21:48:01]

Done.

+      media_controller_(std::move(media_controller)),
+      binding_(this),
+      route_id_(route_id) {}
+
+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() {
+  for (Observer& observer : observers_)
+    observer.OnControllerInvalidated();
+  // |this| is deleted here!

[imcheng, 2017/03/16 20:00:43]

This may be true, but only if all references were held by the Observers.
Regardless, it is best to assume this object is destroyed at this point.

There may be cases in the future where observer isn't immediately added after
obtaining a reference to it. In that case, it might be helpful to have a valid
bit on the controller. (no change needed for this patch)

[takumif, 2017/03/17 21:48:01]

Documenting in the comments that all the references must be held by the
observers. I guess it's possible to make the controller's ctor private and
necessary to be accessed through an observer to really enforce it?

[imcheng, 2017/03/18 00:11:32]

Well MediaRouteController is created by MediaRouter, but I'm not sure if you can
prevent others from AddRef() to it.

So either we need to document that adding the Observer immediately after getting
obtaining the controller from MediaRouter is the only proper usage of this API
or that we need a way to tell if the controller is still valid without using
observers.

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

I wonder if it is sufficient to document that only the observers should hold on
to a scoped_refptr.

+}
+
+void MediaRouteController::OnControllerBound() {
+  media_controller_.set_connection_error_handler(base::Bind(

[imcheng, 2017/03/16 20:00:43]

Can you set this in the ctor? IIRC, you can set the error handler as soon as the
IntefacePtr is bound to a message pipe.

[takumif, 2017/03/17 21:48:01]

Yes, we can set it in the ctor by enforcing that the controller ptr is already
bound. Doing so.

+      &MediaRouteController::OnControllerInvalidated, base::Unretained(this)));
+}
+
+mojom::MediaStatusObserverPtr MediaRouteController::GetObserverPtr() {

[dcheng, 2017/03/17 06:15:14]

I would suggest adding this in a followup where it's actually used.

[takumif, 2017/03/17 21:48:01]

Done.

+  return binding_.CreateInterfacePtrAndBind();

[imcheng, 2017/03/16 20:00:43]

You can also set_connection_error_handler on the binding at this point. (I
believe MediaController and the MediaStatusObserver are 2 separate message
pipes)

[takumif, 2017/03/17 21:48:01]

Moving this to CL #2, but yes I think it'd make sense to destroy MRC when status
updates are no longer coming in. We can notify the WebUI to go back to the sink
list view when MRC is destroyed.

+}
+
+MediaRouteController::~MediaRouteController() {
+  destructor_callback_.Run();
+}
+
+void MediaRouteController::AddObserver(Observer* observer) {
+  observers_.AddObserver(observer);
+}
+
+void MediaRouteController::RemoveObserver(Observer* observer) {
+  observers_.RemoveObserver(observer);
+}
+
+}  // namespace media_router