[Media Router] Design MediaRouter interface with stub implementation.

chrome/browser/media/router/media_route_provider_manager_host.h

+// Copyright 2015 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 CHROME_BROWSER_MEDIA_ROUTER_MEDIA_ROUTE_PROVIDER_MANAGER_HOST_H_
+#define CHROME_BROWSER_MEDIA_ROUTER_MEDIA_ROUTE_PROVIDER_MANAGER_HOST_H_
+
+#include <string>
+#include <vector>
+
+#include "base/callback.h"
+#include "chrome/browser/media/router/media_route.h"
+#include "chrome/browser/media/router/media_sink.h"
+#include "chrome/browser/media/router/media_source.h"
+
+namespace media_router {
+
+class MediaRoute;
+
+// Acts as the Chrome endpoint for communication between Media Router and
+// Media Route Providers (via the Media Route Provider Manager).
+class MediaRouteProviderManagerHost {

[xhwang, 2015/04/08 17:46:25]

We don't have MediaRouteProvider and MediaRouteProviderManager yet. I assume
they will come in a later CL?

Could you please add more comments/TODO about what MediaRouteProvider and
MediaRouteProviderManager will do, and what's the relation between them and
MediaRouteProviderManagerHost?

[imcheng, 2015/04/08 19:21:31]

Our implementation of MediaRouteProvider and MediaRouteProviderManager are
actually extension code not in Chromium repository. Added more comments to
clarify the relationship between MR, MRPMH, and MRPM/MRP.

+ public:
+  virtual ~MediaRouteProviderManagerHost() {}
+
+  // Delegate called by the MediaRouteProviderManagerHost when a response is
+  // received from the MRPM.
+  class Delegate {

[xhwang, 2015/04/08 17:46:24]

It seems not all methods of this class are responses. For example, OnMessage()
is initiated from the sink. Please make the comments of each method clear about
whether it's a response or not. If it is, which method is it responding to? Is
it a one-to-one mapping between the call and the response, or you could have
multiple responses for one call?

Either way, is using callbacks an option? You can have permanent callbacks which
could be called multiple times (e.g. for all following updates).

Also, this MRPMH interface looks very similar to MediaRouter interface. But
there are a lot of differences between them. For example: StartRouteRequest vs
RequestRoute. Also, we use a Delegate here. But we are using callbacks and
observers in MediaRouter. Is there any particular reason for these differences?

[imcheng, 2015/04/08 19:21:31]

I changed the class level comment here. But, since we already described the
relationship in the MediaRouteProviderManagerHost APIs below, I think it's fine
to not include the comments here as we would be duplicating comments.
I think this is a matter of preference. To me, a Delegate/Observer pattern makes
more sense than callbacks if it can be invoked multiple times. I also prefer to
have one interface/pattern rather than a mix of observer/callbacks so it's more
uniform and easier to understand.
Re: StartRouteRequest vs RequestRoute. This is one where it's better to have
consistent naming, so I've changed it to RequestRoute in both places. The others
are named differently for valid reasons.

Re: Delegate vs. cbs/observers in MR. A MediaRouter instance corresponds 1:1 to
a MediaRouteProviderManagerHost instance and throughout its lifetime it may
receive many calls forwarded from MediaRouteProviderManagerHost. A Delegate
interface is used because of the preference over callbacks for this situation,
as mentioned above. In MR, Observers are used for receiving continuous updates
for queries and callbacks are used for one-time responses. As for naming of
Delegate vs Observer, I find Delegate more appropriate when the two components
are 1:1, whereas Observer is used for 1:n.

[xhwang, 2015/04/08 20:32:51]

Ack. Thanks!
Hmm, you are actually having a mix of observer/callbacks in MediaRouter
interface.
I see your point. But I still wonder why we make different choices on
MediaRouteProviderManagerHost and MediaRouter interfaces. Maybe you can help on
understand this better.

If you take query sinks as an example. On MRPMH, you have:

AddMediaSinksQuery(const MediaSource& source);
Delegate::OnSinksReceived(const MediaSource& source, const
std::vector<MediaSink>& sinks) = 0;

Then on MR, you have:

RegisterMediaSinksObserver(MediaSinksObserver* observer); where observer has
source()
MediaSinksObserver::OnSinksReceived(const std::vector<MediaSink>& sinks);

I don't really see any fundamental differences here. They are pretty much
interchangable. Then the question is, why do we have two patterns?

This also makes me wonder why we have both MediaRouteProviderManagerHost and
MediaRouter interfaces? Can we just have one interface?

[imcheng, 2015/04/08 23:28:09]

Discussed offline. Will merged the interfaces into one interface "MediaRouter",
and remove MediaRouteProviderManagerHost. Trying to map the new APIs in my head,
I think that should work, but we will know for sure when this patch is merged
back into private repo.

+   public:
+    virtual ~Delegate() {}
+
+    // Called when results from a media sinks query have been received.
+    // |source|: Media source of the query.
+    // |sinks|: List of sinks compatible with the source.
+    virtual void OnSinksReceived(const MediaSource& source,
+                                 const std::vector<MediaSink>& sinks) = 0;
+
+    // Called when there is a message from a route.
+    // |route_id|: The route ID.
+    // |message|: The message.
+    virtual void OnMessage(const MediaRouteId& route_id,
+                           const std::string& message) = 0;
+
+    // Called when a response to a route request has been received and
+    // a route has been established.
+    // |request_id|: ID of original request.
+    // |route|: Description of the established route.
+    virtual void OnRouteResponseReceived(const RouteRequestId& request_id,
+                                         const MediaRoute& route) = 0;
+
+    // Called when an error occurred while establishing a route.
+    // |request_id|: ID of original request.
+    // |error_text|: Text describing the error.
+    virtual void OnRouteResponseError(const RouteRequestId& request_id,
+                                      const std::string& error_text) = 0;

[xhwang, 2015/04/08 17:46:24]

OnRouteResponseReceived() or OnRouteResponseError() will be called only once as
a result of RequestRoute(). Is that correct?

Can you combine these into one response? something like:

virtual void OnRouteResponseReceived(const RouteRequestId& request_id,
                                     scoped_ptr<MediaRoute> route,
                                     const std::string& error_text) = 0;

Also, see my comment above about the possibility of using callbacks instead of
delegate methods.

[imcheng, 2015/04/08 19:21:30]

Done.

+
+    // Called when the list of currently MediaRoutes and their MediaSinks

[xhwang, 2015/04/08 17:46:24]

nit: s/currently/current

[imcheng, 2015/04/08 19:21:31]

Done.

+    // has been updated.
+    // |routes|: List of MediaRoutes.
+    // |sinks|: List of MediaSinks associated with the routes.
+    virtual void OnRoutesUpdated(const std::vector<MediaRoute>& routes,
+                                 const std::vector<MediaSink>& sinks) = 0;

[xhwang, 2015/04/08 17:46:24]

Ditto. |routes| contains sinks. What's the relationship between
routes[i].media_sink() and sinks[j]?

[imcheng, 2015/04/08 19:21:30]

Removed |sinks|.

+
+    // Called when the MRPMHost is being destroyed.
+    virtual void OnHostDestroyed(MediaRouteProviderManagerHost* host) = 0;
+  };
+
+  // The following APIs allow the media router to send commands to the MRPM.
+
+  // Instructs MRPM to start a query for compatible media sinks for |source|.
+  // Results from the query and subsequent updates will be received via
+  // |Delegate::OnSinksReceived| until the query is removed from MRPM
+  // via |RemoveMediaSinksQuery|.
+  virtual void AddMediaSinksQuery(const MediaSource& source) = 0;
+
+  // Instructs MRPM to stop a query for compatible media sinks for |source|.
+  // The delegate will no longer receive updates.
+  virtual void RemoveMediaSinksQuery(const MediaSource& source) = 0;
+
+  // Requests MRPM to establish a route between |source| and a media sink given
+  // by |sink_id|. |request_id| is generated by Media Router and passed
+  // to MRPM to identify the request.
+  // Result of the request will be received via either
+  // |Delegate::OnRouteResponseReceived| or |OnRouteResponseError|.
+  virtual void RequestRoute(const RouteRequestId& request_id,
+                            const MediaSource& source,
+                            const MediaSinkId& sink_id) = 0;

[xhwang, 2015/04/08 17:46:24]

If you use a callback for the response, you shouldn't need a |request_id|...

[imcheng, 2015/04/08 19:21:31]

See uniformity comment above.

+
+  // Requests MRPM to close a route specified by |route_id|.
+  virtual void CloseRoute(const MediaRouteId& route_id) = 0;
+
+  // Requests MRPM to post |message| to a MediaSink connected by MediaRoute
+  // with |route_id|.
+  virtual void PostMessage(const MediaRouteId& route_id,
+                           const std::string& message) = 0;
+
+  // Requests MRPM to start a query for all MediaRoutes and their MediaSinks.
+  // The result and subsequent updates will be received via
+  // |Delegate::OnRoutesUpdated|.

[xhwang, 2015/04/08 17:46:24]

Please see my comments above on callbacks.

[imcheng, 2015/04/08 19:21:30]

Acknowledged.

+  virtual void StartMediaRoutesQuery() = 0;
+
+  // Requests MRPM to stop the query. The delegate will no longer receive
+  // updates.
+  virtual void StopMediaRoutesQuery() = 0;
+};
+
+}  // namespace media_router
+
+#endif  // CHROME_BROWSER_MEDIA_ROUTER_MEDIA_ROUTE_PROVIDER_MANAGER_HOST_H_