[Media Router] Design MediaRouter interface with stub implementation.

chrome/browser/media/router/media_router.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_ROUTER_H_
+#define CHROME_BROWSER_MEDIA_ROUTER_MEDIA_ROUTER_H_
+
+#include <string>
+#include <vector>
+
+#include "chrome/browser/media/router/media_route_id.h"
+#include "chrome/browser/media/router/media_sink.h"
+
+namespace media_router {
+
+class MediaRoute;
+class MediaRouteResponse;
+class MediaRoutesObserver;
+class MediaSource;
+class MediaSinksObserver;
+
+using MediaRouteResponseCallback =
+    base::Callback<void(const MediaRouteResponse&)>;
+
+// An interface for handling resources related to media routing.
+// Responsible for registering observers for receiving sink availability updates
+// as well as handling route requests/responses.

[mark a. foltz, 2015/03/27 21:21:58]

Update docstring to include PostMessage behavior.

[mark a. foltz, 2015/03/27 21:21:58]

Threading assumptions should be documented here in relation to the observer and
callback objects passed in.

[imcheng, 2015/03/30 22:49:09]

I will do that in media_router_impl.h.

[imcheng, 2015/03/30 22:49:10]

Done.

+class MediaRouter {
+ public:
+  virtual ~MediaRouter();
+
+  // Requests a media route from |source| to |sink_id|.
+  // |callback| is invoked with a response indicating success or failure.

[mark a. foltz, 2015/03/27 21:21:58]

Explain use of return value (i.e. it can be used to later unregister).

[imcheng, 2015/03/30 22:49:10]

Done.

+  virtual RouteRequestId StartRouteRequest(
+      const MediaSource& source,
+      const MediaSinkId& sink_id,
+      const MediaRouteResponseCallback& callback) = 0;
+
+  // Unregisters a pending media route request, e.g. when the MR UI is closed.

[mark a. foltz, 2015/03/27 21:21:58]

e.g., when the user has canceled presentation by closing the media router
dialog.

[mark a. foltz, 2015/03/27 21:21:58]

Should this have a return value?  As long as it guarantees that the request is
discarded (if it exists) probably not, but wanted to check.

[imcheng, 2015/03/30 22:49:09]

Done.

[imcheng, 2015/03/30 22:49:10]

It will always discard the request, so no return value is needed.

+  virtual void UnregisterMediaRouteResponseCallback(

[mark a. foltz, 2015/03/27 21:21:58]

- A better name might be CancelRouteRequest?
- Mention the request_id must be a value returned from StartRouteRequest
- What is the fate of the callback registered with StartRouteRequest -  is it
invoked with an error or never run?
- If an error is returned, should the caller here pass a reason for the
cancellation (i.e., USER_CANCELLED)?

[imcheng, 2015/03/30 22:49:09]

This function does NOT cancel a route request, only the callback. I updated the
comments to clarify that.
This function is typically used when a registered callback is about to go out of
scope.

+      const RouteRequestId& request_id) = 0;
+
+  // Closes a media route specified by |route_id|.

[mark a. foltz, 2015/03/27 21:21:58]

s/a/the/

[imcheng, 2015/03/30 22:49:10]

Done.

+  virtual void CloseRoute(const MediaRouteId& route_id) = 0;
+
+  // Registers |observer| with MediaRouter so that it will receive updates on

[mark a. foltz, 2015/03/27 21:21:58]

I might write this as:

Registers |observer| with this MediaRouter.  |observer| specifies a media source
and will receive updates with media sinks that are compatible with that source. 
The initial update may happen synchronously.
<newline>

[imcheng, 2015/03/30 22:49:10]

Done.

+  // sinks that are compatible with the source specified in it.
+  // Initial set of updates may be returned synchronously to |observer|.
+  // NOTE: This class does not assume ownership of |observer|. Callers must
+  // manage |observer| and make sure |UnregisterObserver()| is called
+  // before the observer is destroyed.

[mark a. foltz, 2015/03/27 21:21:58]

Is Unregister() always called immediately before Observer destruction?  Maybe
the dtor for Observer should be responsible for unregistering it from the router
(by the router providing a callback to run in the dtor).

If not, is the idea that the observer can be unregistered and re-registered if
needed?  Are observers ever shared between instances of the router?  If it's 1:1
at a minimum I would keep a 'registered' bit in the observer and DCHECK on it in
the observer dtor.

[imcheng, 2015/03/30 22:49:09]

It is typically the case that destruction follows from unregistration, but
lifetime and (un)registration are isolated. The idea is as you mentioned. The
owner of the observers may have better context so it's responible for issuing
the MediaRouter commands. 

I suppose we can combine lifetime with registration. One way is to inject
MediaRouter (or 2 callbacks) to the observer's ctor.
RegisterObserver/UnregisterObserver will be called in ctor/dtor respectively.
One drawback will be that you can no longer create an observer without having a
MediaRouter first. Will that approach be more preferrable?

+  // Returns true if registration succeeded or the |observer| already exists.
+  // If the MRPM Host is not available, the registration request will fail
+  // immediately.
+  virtual bool RegisterObserver(MediaSinksObserver* observer) = 0;
+
+  // Unregisters |observer| from MediaRouter.

[mark a. foltz, 2015/03/27 21:21:58]

What are the side effects? I assume that |observer| will stop receiving further
updates?

[imcheng, 2015/03/30 22:49:09]

Done.

+  virtual void UnregisterObserver(MediaSinksObserver* observer) = 0;
+
+  // Posts |message| with optional |extra_info_json| to a MediaSink connected
+  // via MediaRoute with |route_id|.
+  virtual void PostMessage(const MediaRouteId& route_id,

[mark a. foltz, 2015/03/27 21:21:58]

This (at a minimum) needs a TODO to add overrides for the additional data types
that are now available from the spec: Blob, ArrayBuffer, ArrayBufferView.  See
the pending changes for postMessage() in Blink.

[imcheng, 2015/03/30 22:49:09]

Does the API and downstream need to differentiate the between the different data
types? If so, would it be sufficient to use a std::string to hold the data and
provide an extra enum arg to specify the data type?

+                           const std::string& message,
+                           const std::string& extra_info_json) = 0;

[mark a. foltz, 2015/03/27 21:21:58]

Is extra_info_json being used?

[imcheng, 2015/03/30 22:49:09]

No it's not being used. I can remove it for now to simplify things.

+
+  // Adds a MediaRoutesObserver to listen for updates on MediaRoutes.
+  // MediaRouter does not own |observer|. |RemoveMediaRoutesObserver| should
+  // be called before |observer| is destroyed.

[mark a. foltz, 2015/03/27 21:21:58]

Similar comments to the pattern above for Register/Unregister.

[imcheng, 2015/03/30 22:49:09]

See other comment.

+  virtual void AddMediaRoutesObserver(MediaRoutesObserver* observer) = 0;

[mark a. foltz, 2015/03/27 21:21:58]

Would prefer similar terminology to the sinks case, maybe:

[Un]RegisterMediaSinksObserver()
[Un]RegisterMediaRoutesObserver()

[imcheng, 2015/03/30 22:49:09]

Done.

+
+  // Removes a previously added MediaRoutesObserver. |observer| will stop
+  // receiving updates from MediaRouter.
+  virtual void RemoveMediaRoutesObserver(MediaRoutesObserver* observer) = 0;
+};
+
+}  // namespace media_router
+
+#endif  // CHROME_BROWSER_MEDIA_ROUTER_MEDIA_ROUTER_H_