Upstreaming review for Media Router Mojo interface.

chrome/browser/media/router/media_router_api.mojom

Index: chrome/browser/media/router/media_router_api.mojom
diff --git a/chrome/browser/media/router/media_router_api.mojom b/chrome/browser/media/router/media_router_api.mojom
new file mode 100644
index 0000000000000000000000000000000000000000..0630e3de62a026a19940ba1c938d878f3ab17057
--- /dev/null
+++ b/chrome/browser/media/router/media_router_api.mojom
@@ -0,0 +1,194 @@
+// 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.
+
+// Defines communication between the Media Router and the
+// Media Router Provider Component Extension.

[xhwang, 2015/04/27 17:52:02]

This interface should be generic, it's not limited to extensions. Sounds like
"extension" is just an implementation detail.

[Kevin Marshall, 2015/04/27 19:23:51]

Done.

+
+module media_router.mojom;

[xhwang, 2015/04/27 17:52:02]

Typically we don't put mojom in the module name. But without it you'll end up
with name conflict :( I am fighting with this in media/ as well and couldn't
find a good solution. See [1] for discussion.

If you like nested namespace, how about media_router.mojo or
media_router.interfaces? Using "mojom" here seems wrong because it's the file
type.

[1] https://groups.google.com/a/chromium.org/forum/#!topic/mojo-dev/j6NXQXUvg8E

[Kevin Marshall, 2015/04/27 19:23:51]

I'd prefer to use a nested type for naming conflicts, since there are so many
structures that are reimplemented in Mojo.

I'll go with "interfaces" because IMO a "mojo" prefix might cause some confusion
between the Mojo common components and our generated code.

[xhwang, 2015/04/28 06:08:18]

Agreed. I might do the same for media mojom interfaces.

+
+struct Provider {

[xhwang, 2015/04/27 17:52:02]

This struct is not used anywhere in this mojom file. In that case can we drop
it?

[Kevin Marshall, 2015/04/27 19:23:51]

Done.

+  // State transitions:
+  // UNINITIALIZED => LOADING => INITIALIZING
+  //      ^              \/            |
+  //      +=========(error handling)===|
+  //                                   |
+  //                                  \/
+  //                  LOADING ====> READY
+  //                     ^            |  (when idle)
+  //                  UNLOADED <======+

[xhwang, 2015/04/27 17:52:01]

nit: Will it cause confusing that LOADING appears in two places?

[Kevin Marshall, 2015/04/27 19:23:51]

Removed.

+  enum Status {
+    UNINITIALIZED,
+    LOADING,
+    INITIALIZING,
+    UNLOADED,
+    READY,
+  };
+  string name;
+  Status load_status;
+};
+
+// Represents an output sink to which media can be routed.
+struct MediaSink {
+  // The sink identifier, e.g. "rs71w7mFzYLFlabir_qO4NHl6SUc."
+  // Required.

[xhwang, 2015/04/27 17:52:02]

Not needed? Optional/nullable members are marked with "?", which is clear.

[Kevin Marshall, 2015/04/27 19:23:50]

Done.

+  string sink_id;
+  // The human-readable name, e.g. "Janet's Chromecast".
+  // Required.
+  string name;
+};
+
+// Should be kept in sync with media_route.h.
+struct MediaRoute {
+  // The ID of this media route, e.g. "r_PR1O_blkC9dsKp-tb1ti8qurOo".
+  // Required.
+  string media_route_id;
+  // The media source being sent through this media route.
+  // May be missing if route is not local.
+  // Optional.
+  string? media_source;

[xhwang, 2015/04/27 17:52:02]

Is this an id or a name?

[Kevin Marshall, 2015/04/27 19:23:51]

Done.

+  // The ID of the sink receiving the media.
+  // e.g. "rs71w7mFzYLFlabir_qO4NHl6SUc."
+  // Required.
+  string sink_id;
+  // Human readable name of the sink.
+  string? sink_name;

[xhwang, 2015/04/27 17:52:02]

In MediaSink, both id and name are required. But here id is required but name is
optional? Why is this difference? Can we just use a MediaSink here?

[Kevin Marshall, 2015/04/27 19:23:51]

Good catch. In the past there was a call that returned sinks and routes
together, so the sink was made optional to reduce redundancy. That call was
taken out in the previous upstreaming CL, so this optimization is now N/A.

Done.

+  // Human readable description of this route, e.g.
+  // "Tab casting".
+  // Required.
+  string description;
+  // An icon that is associated with this media source.
+  // Optional.
+  string? icon_url;
+  // Specifies that the route is requested locally.
+  // Required.
+  bool is_local;
+};
+
+// Issue created by a media route provider and shown to the user.

[xhwang, 2015/04/27 17:52:02]

nit: Could you please define "issue"?

[Kevin Marshall, 2015/04/27 19:23:51]

Done.

+// When is_blocking is true, media router UI shows issue only:
+//
+//       Title
+//       Message
+//       default_action_button secondary_action_button
+//
+// When is_blocking is false, media router UI uses banner:
+//
+//       Title default_action_link secondary_action_link
+//
+// above receiver list if route_id is not provided; otherwise it is
+// above route detail and controls.

[xhwang, 2015/04/27 17:52:02]

hmm, seems like these are just UI implementation details. You probably want to
explain what a "issue" is, and what is_blocking means.

[Kevin Marshall, 2015/04/27 19:23:51]

Done.

+struct Issue {
+  enum Severity {
+    FATAL,
+    WARNING,
+    NOTIFICATION
+  };
+
+  enum ActionType {
+    OK,
+    CANCEL,
+    DISMISS,
+    LEARN_MORE
+  };
+
+  // If set, the ID of the route to which this issue pertains.
+  // If not set (default), then this is a global issue.
+  // Optional
+  string? route_id;
+
+  // Required
+  Severity severity;
+
+  // When true, this issue takes the whole dialog.
+  // Required
+  bool is_blocking;
+
+  // Short description about the issue.
+  // Required.
+  string title;
+
+  // Message about issue detail or how to handle issue.
+  // Messages should be suitable for end users to decide which actions to take.
+  // Optional
+  string? message;
+
+  // Required
+  ActionType default_action;
+
+  // Optional
+  array<ActionType>? secondary_actions;
+
+  // A help page to be opened if users select learn_more.
+  // Optional
+  string? help_url;
+};
+
+// Interface for sending messages from the MediaRouter (MR)
+// to the Media Router client.  For the component extension, this is the Media

[xhwang, 2015/04/27 17:52:01]

ditto about "extension" is implementation detail.

[Kevin Marshall, 2015/04/27 19:23:51]

Done.

+// Route Provider Manager (MRPM).
+interface ProviderManagerService {
+  // Signals the client to route the media located
+  // at |source_urn| to |sink_id|.

[xhwang, 2015/04/27 17:52:02]

What's the relationship between this |source_run| and the |media_source| in
MediaRoute interface (l.49)?

[Kevin Marshall, 2015/04/27 19:23:51]

The same thing.

Fixed everywhere: s/source_urn/media_source/g.

+  // If the operation was successful, |route| will be defined and
+  //     |error_text| will be null.
+  // If the operation failed, |route| will be null and |error_text|
+  //     will be set.
+  CreateRoute(string source, string sink_id) =>
+      (MediaRoute? route, string? error_text);
+
+  // Signals the client to join an established route given by |presentation_id|
+  // with media located at |source_urn|. |origin| and |tab_id| are used for
+  // enforcing same origin/tab scopes.
+  // Returns the route on success. Otherwise an error message is returned.
+  JoinRoute(string source_urn,
+            string presentation_id,
+            string origin,
+            int64 tab_id) => (MediaRoute? route, string? error_text);
+
+  // Signals the client to close the route specified by |route_id|.
+  CloseRoute(string route_id);

[xhwang, 2015/04/27 17:52:01]

hmm... With CreateRoute/JoinRoute/CloseRoute/PostMessage, this
(ProviderManagerService) looks more like a "MediaRouter" interface. In
MediaRouterApi you have a lot of On* methods, which looks more like a "Client".
Does this make any sense?

[Kevin Marshall, 2015/04/27 19:23:51]

I agree that the methods are the same as the MediaRouter, but the interface is
named this way because it's used by the MR to call the MRPM, not the other way
around.

Is "Client" in common parlance these days? I think it might be easiest to name
the interfaces by their backing component (you use an interfaces::MediaRouter to
talk to the MediaRouter; interfaces::ProviderManager to talk to the
ProviderManager, etc.)

[xhwang, 2015/04/28 06:08:18]

Client is still pretty popular in the mojo world. In a lot of IPC/Mojo
communication, one side asks the other side to do something (e.g. to get
geolocation, to decode media streams, etc). So a mojo interface defines what to
do. For example, for Foo interface, the caller will hold a FooPtr, and in the
other side you'll have a Foo implementation. Sometimes the implementation needs
to send something back to the caller, and this is where we have a FooClient
interface.

Currently in C++, MediaRouter interface is the interface that provides and
manages routes, e.g. RequestRoute, CloseRoute. It would be nice that the mojom
interface is consistent with that model. 

The current interfaces' naming is very confusing:
- Mojo ProviderManagerService <-> C++ MediaRouter
- Mojo MediaRouter <-> C++ MediaRoutner::Delegate

[Kevin M, 2015/04/28 17:20:29]

Done.

+
+  // Sends |message| with optional |extra_info_json| via the media route
+  // |media_route_id|.
+  PostMessage(string media_route_id, string message);
+
+  // Signals the client to start querying for sinks
+  // capable of displaying |source_urn|.
+  StartObservingMediaSinks(string source_urn);
+
+  // Signals the client to stop querying for sinks
+  // capable of displaying |source_urn|.
+  StopObservingMediaSinks(string source_urn);
+
+  // Signals the to start a query on all routes. Updates are received from
+  // |OnRoutesUpdated()|.
+  StartObservingMediaRoutes();
+
+  // Signals the to stop query on all routes.
+  StopObservingMediaRoutes();
+
+  // "Clears" an issue after it has been processed.
+  ClearIssue(string issue_id);
+};
+
+// Interface for sending messages from the client to the MR.
+interface MediaRouterApi {

[xhwang, 2015/04/27 17:52:02]

Just MediaRouter? You have nested namespace so there's no naming conflict.

[Kevin Marshall, 2015/04/27 19:23:51]

Done.

+  // Sets a service object for issuing calls back to the MRPM.
+  // Returns a string that uniquely identifies the Media Router browser
+  // process.
+  SetProviderManagerService(ProviderManagerService observer) =>
+      (string instance_id);

[xhwang, 2015/04/27 17:52:02]

See naming comment above. In mojo, usually we have a Foo interface, then we have
a FooClient interface. Then this call will become something like SetClient().

[Kevin Marshall, 2015/04/27 19:23:51]

I'm unsure if "Client" is useful in this case, because the usual observer roles
are inverted for this API. The ProviderManagerService is the primary actor here,
and the MediaRouter is just receiving the results.

[xhwang, 2015/04/28 06:08:18]

Ack. But from API's perspective, this is confusing. See comments above. Mabe I
am missing some big picture.

[Kevin M, 2015/04/28 17:20:29]

Done.

+
+  // Called when the Media Route Manager receives a new list of sinks.
+  OnSinksReceived(string source_urn, array<MediaSink> sinks);
+
+  // Called when the Media Route Manager receives a route message.
+  OnMessage(string source_urn, string message);
+
+  // Called when providers send an issue.
+  OnIssue(Issue issue);
+
+  // Called when list of routes has been updated.
+  OnRoutesUpdated(array<MediaRoute> routes);
+};