[Media Router] Add ProvideSinks() Mojo API

[Media Router] Add ProvideSinks() Mojo API

- added ProvideSinks() to media_router.mojom
- added ProvideSinks() to media_router.h

cl/146753004 added ProvideSinks() to component extension.

BUG=687365
Review-Url: https://codereview.chromium.org/2679893002
Cr-Commit-Position: refs/heads/master@{#462333}
Committed: https://chromium.googlesource.com/chromium/src/+/b254a456ab6527124ce5aff9acc231eed34605c4

[zhaobin, 2017-02-07 22:20:48]

zhaobin@chromium.org changed reviewers:
+ dcheng@chromium.org, imcheng@chromium.org, mfoltz@chromium.org

[dcheng, 2017-02-08 23:03:21]

mojo lgtm

[mark a. foltz, 2017-02-10 01:23:55]

https://codereview.chromium.org/2679893002/diff/1/chrome/browser/media/router...
File chrome/browser/media/router/media_router.h (right):

https://codereview.chromium.org/2679893002/diff/1/chrome/browser/media/router...
chrome/browser/media/router/media_router.h:62: using MediaSinkList =
std::vector<std::unique_ptr<MediaSink>>;
Should MediaSink be move-only? Or is there a use case for copying?  When do we
copy and when do we pass pointers?

https://codereview.chromium.org/2679893002/diff/1/chrome/browser/media/router...
chrome/browser/media/router/media_router.h:172: // Called when DIAL or CAST
MediaSinkService finishes sink discovery.
Are there multiple services running at once?   Do they provide a combined list
or call this API separately?

https://codereview.chromium.org/2679893002/diff/1/chrome/browser/media/router...
chrome/browser/media/router/media_router.h:174: virtual void
OnSinksDiscovered(std::unique_ptr<MediaSinkList> sinks) = 0;
Passing a std::vector by unique_ptr isn't common.  It's more common to pass by
const reference.  The implementation can cheaply copy the vector if it needs to
(it's just a list of pointers) or use std::move.

This would also avoid writing trampoline methods for mocking in unit tests.

https://codereview.chromium.org/2679893002/diff/1/chrome/browser/media/router...
File chrome/browser/media/router/mojo/media_router.mojom (right):

https://codereview.chromium.org/2679893002/diff/1/chrome/browser/media/router...
chrome/browser/media/router/mojo/media_router.mojom:357:
OnSinksDiscovered(array<MediaSink> sinks);
It's a little tricky to distinguish this from OnSinksReceived below.

Maybe name this ProvideSinks() to make it clear this are sinks discovered by the
MR and passed to the MRP?  Derek, do you have any ideas?

Do you need to handle the origin and media_source data passed with
OnSinksReceived?   Maybe we should prioritize simplifying the API to eliminate
those.

https://codereview.chromium.org/2679893002/diff/1/chrome/browser/media/router...
File chrome/browser/media/router/mojo/media_router_mojo_metrics.h (right):

https://codereview.chromium.org/2679893002/diff/1/chrome/browser/media/router...
chrome/browser/media/router/mojo/media_router_mojo_metrics.h:46:
ON_SINKS_DISCOVERED = 18,
I think you'll need to update tools/metrics/histograms/histograms.xml with this
new value.

[imcheng, 2017-02-10 22:45:19]

https://codereview.chromium.org/2679893002/diff/1/chrome/browser/media/router...
File chrome/browser/media/router/media_router.h (right):

https://codereview.chromium.org/2679893002/diff/1/chrome/browser/media/router...
chrome/browser/media/router/media_router.h:62: using MediaSinkList =
std::vector<std::unique_ptr<MediaSink>>;
On 2017/02/10 01:23:55, mark a. foltz wrote:
> Should MediaSink be move-only? Or is there a use case for copying?  When do we
> copy and when do we pass pointers?

I think this just needs to be updated according to changes in
https://codereview.chromium.org/2675033002.

Previously we were exploring options with subtyping MediaSinks but found it did
not work well with downcasting / passing ptrs in the APIs.

After discussing with Bin, we came up with a wrapper for MediaSinks with a union
of DIAL/Cast specific data that needs to be passed to the extension. This seems
to work reasonably well without affecting consumers of MediaSinkObservers.

https://codereview.chromium.org/2679893002/diff/1/chrome/browser/media/router...
chrome/browser/media/router/media_router.h:172: // Called when DIAL or CAST
MediaSinkService finishes sink discovery.
On 2017/02/10 01:23:55, mark a. foltz wrote:
> Are there multiple services running at once?   Do they provide a combined list
> or call this API separately?

It seems this will be called directly by each service but I wonder if it makes
sense: without any additional info (such as provider name), it seems we don't
have the ability to tell an individual MRP that there are no sinks.

Also, I don't think sending a combined sink list to the extension makes sense,
as it means someone has to ask each service for current list of sinks before
pushing to the extension. So I would prefer this to be called per-service, but
we'd need to add back a provider name here.

https://codereview.chromium.org/2679893002/diff/1/chrome/browser/media/router...
chrome/browser/media/router/media_router.h:174: virtual void
OnSinksDiscovered(std::unique_ptr<MediaSinkList> sinks) = 0;
On 2017/02/10 01:23:55, mark a. foltz wrote:
> Passing a std::vector by unique_ptr isn't common.  It's more common to pass by
> const reference.  The implementation can cheaply copy the vector if it needs
to
> (it's just a list of pointers) or use std::move.
> 
> This would also avoid writing trampoline methods for mocking in unit tests.

I don't think a vector<unique_ptr<T>> is copyable but I don't think the vector
itself needs to be wrapped in a unique_ptr (you can std::move the vector when
calling this method). But see comment in L62 above.

https://codereview.chromium.org/2679893002/diff/1/chrome/browser/media/router...
File chrome/browser/media/router/mojo/media_router.mojom (right):

https://codereview.chromium.org/2679893002/diff/1/chrome/browser/media/router...
chrome/browser/media/router/mojo/media_router.mojom:357:
OnSinksDiscovered(array<MediaSink> sinks);
On 2017/02/10 01:23:55, mark a. foltz wrote:
> It's a little tricky to distinguish this from OnSinksReceived below.
> 
> Maybe name this ProvideSinks() to make it clear this are sinks discovered by
the
> MR and passed to the MRP?  Derek, do you have any ideas?
> 
> Do you need to handle the origin and media_source data passed with
> OnSinksReceived?   Maybe we should prioritize simplifying the API to eliminate
> those.

ProvideSinks sounds good. This method does not need |origin| or |media_source|
as it is not a response to a sink query. 

Also the comment could be something like:

Called when the list of MediaSinks discovered by Media Router has been updated.
The sinks are supplied to the MediaRouteProvider so that they can be used for
other operations, such as route creation.

[zhaobin, 2017-02-16 22:56:36]

https://codereview.chromium.org/2679893002/diff/1/chrome/browser/media/router...
File chrome/browser/media/router/media_router.h (right):

https://codereview.chromium.org/2679893002/diff/1/chrome/browser/media/router...
chrome/browser/media/router/media_router.h:62: using MediaSinkList =
std::vector<std::unique_ptr<MediaSink>>;
On 2017/02/10 22:45:18, imcheng wrote:
> On 2017/02/10 01:23:55, mark a. foltz wrote:
> > Should MediaSink be move-only? Or is there a use case for copying?  When do
we
> > copy and when do we pass pointers?
> 
> I think this just needs to be updated according to changes in
> https://codereview.chromium.org/2675033002.
> 
> Previously we were exploring options with subtyping MediaSinks but found it
did
> not work well with downcasting / passing ptrs in the APIs.
> 
> After discussing with Bin, we came up with a wrapper for MediaSinks with a
union
> of DIAL/Cast specific data that needs to be passed to the extension. This
seems
> to work reasonably well without affecting consumers of MediaSinkObservers.

Done.

https://codereview.chromium.org/2679893002/diff/1/chrome/browser/media/router...
chrome/browser/media/router/media_router.h:172: // Called when DIAL or CAST
MediaSinkService finishes sink discovery.
On 2017/02/10 22:45:18, imcheng wrote:
> On 2017/02/10 01:23:55, mark a. foltz wrote:
> > Are there multiple services running at once?   Do they provide a combined
list
> > or call this API separately?
> 
> It seems this will be called directly by each service but I wonder if it makes
> sense: without any additional info (such as provider name), it seems we don't
> have the ability to tell an individual MRP that there are no sinks.
> 
> Also, I don't think sending a combined sink list to the extension makes sense,
> as it means someone has to ask each service for current list of sinks before
> pushing to the extension. So I would prefer this to be called per-service, but
> we'd need to add back a provider name here.

Done.

https://codereview.chromium.org/2679893002/diff/1/chrome/browser/media/router...
chrome/browser/media/router/media_router.h:174: virtual void
OnSinksDiscovered(std::unique_ptr<MediaSinkList> sinks) = 0;
On 2017/02/10 22:45:18, imcheng wrote:
> On 2017/02/10 01:23:55, mark a. foltz wrote:
> > Passing a std::vector by unique_ptr isn't common.  It's more common to pass
by
> > const reference.  The implementation can cheaply copy the vector if it needs
> to
> > (it's just a list of pointers) or use std::move.
> > 
> > This would also avoid writing trampoline methods for mocking in unit tests.
> 
> I don't think a vector<unique_ptr<T>> is copyable but I don't think the vector
> itself needs to be wrapped in a unique_ptr (you can std::move the vector when
> calling this method). But see comment in L62 above.

Done.

https://codereview.chromium.org/2679893002/diff/1/chrome/browser/media/router...
File chrome/browser/media/router/mojo/media_router.mojom (right):

https://codereview.chromium.org/2679893002/diff/1/chrome/browser/media/router...
chrome/browser/media/router/mojo/media_router.mojom:357:
OnSinksDiscovered(array<MediaSink> sinks);
On 2017/02/10 22:45:18, imcheng wrote:
> On 2017/02/10 01:23:55, mark a. foltz wrote:
> > It's a little tricky to distinguish this from OnSinksReceived below.
> > 
> > Maybe name this ProvideSinks() to make it clear this are sinks discovered by
> the
> > MR and passed to the MRP?  Derek, do you have any ideas?
> > 
> > Do you need to handle the origin and media_source data passed with
> > OnSinksReceived?   Maybe we should prioritize simplifying the API to
eliminate
> > those.
> 
> ProvideSinks sounds good. This method does not need |origin| or |media_source|
> as it is not a response to a sink query. 
> 
> Also the comment could be something like:
> 
> Called when the list of MediaSinks discovered by Media Router has been
updated.
> The sinks are supplied to the MediaRouteProvider so that they can be used for
> other operations, such as route creation.

Done.

https://codereview.chromium.org/2679893002/diff/1/chrome/browser/media/router...
File chrome/browser/media/router/mojo/media_router_mojo_metrics.h (right):

https://codereview.chromium.org/2679893002/diff/1/chrome/browser/media/router...
chrome/browser/media/router/mojo/media_router_mojo_metrics.h:46:
ON_SINKS_DISCOVERED = 18,
On 2017/02/10 01:23:55, mark a. foltz wrote:
> I think you'll need to update tools/metrics/histograms/histograms.xml with
this
> new value.

Done.

[mark a. foltz, 2017-03-01 06:31:43]

lgtm

https://codereview.chromium.org/2679893002/diff/40001/chrome/browser/media/ro...
File chrome/browser/media/router/media_router.h (right):

https://codereview.chromium.org/2679893002/diff/40001/chrome/browser/media/ro...
chrome/browser/media/router/media_router.h:67: using MediaSinkList =
std::vector<MediaSinkInternal>;
Nit: For consistency with other APIs, use std::vector explicitly instead of
using.

https://codereview.chromium.org/2679893002/diff/40001/chrome/browser/media/ro...
File chrome/browser/media/router/mojo/media_router_mojo_impl.cc (right):

https://codereview.chromium.org/2679893002/diff/40001/chrome/browser/media/ro...
chrome/browser/media/router/mojo/media_router_mojo_impl.cc:399:
base::ConstRef(sinks)));
base::Passed?

https://codereview.chromium.org/2679893002/diff/40001/chrome/browser/media/ro...
File chrome/browser/media/router/mojo/media_router_mojo_impl_unittest.cc
(right):

https://codereview.chromium.org/2679893002/diff/40001/chrome/browser/media/ro...
chrome/browser/media/router/mojo/media_router_mojo_impl_unittest.cc:1160: const
MediaRouter::MediaSinkList& actual_sinks) {
Does MediaSink implement == and a copy constructor?  Then it looks like you
should just be able to write ProvideSinks("cast", sinks).

[imcheng, 2017-03-01 23:28:36]

lgtm

https://codereview.chromium.org/2679893002/diff/40001/chrome/browser/media/ro...
File chrome/browser/media/router/media_router.h (right):

https://codereview.chromium.org/2679893002/diff/40001/chrome/browser/media/ro...
chrome/browser/media/router/media_router.h:177: // Called when DIAL or CAST
MediaSinkService finishes sink discovery.
How about:

Notifies the Media Router that the list of MediaSinks discovered by a
MediaSinkService has been updated.
|provider_name|: Name of the MediaSinkService providing the sinks.
...

https://codereview.chromium.org/2679893002/diff/40001/chrome/browser/media/ro...
File chrome/browser/media/router/mojo/media_router_mojo_impl.cc (right):

https://codereview.chromium.org/2679893002/diff/40001/chrome/browser/media/ro...
chrome/browser/media/router/mojo/media_router_mojo_impl.cc:399:
base::ConstRef(sinks)));
On 2017/03/01 06:31:43, mark a. foltz wrote:
> base::Passed?

or just |sinks|? I assume that the reference is to a vector in a
MediaSinkService, so we should be making a copy in the callback (in case it gets
queued).

[btolsch, 2017-03-02 06:04:42]

https://codereview.chromium.org/2679893002/diff/60001/extensions/renderer/res...
File extensions/renderer/resources/media_router_bindings.js (right):

https://codereview.chromium.org/2679893002/diff/60001/extensions/renderer/res...
extensions/renderer/resources/media_router_bindings.js:857:
MediaRouteProvider.prototype.provideSinks = function(sinks) {
This seems to be inconsistent with cl/146753004 and media_router.mojom (missing
providerName argument).

[zhaobin, 2017-03-02 20:24:03]

zhaobin@chromium.org changed reviewers:
+ haraken@chromium.org

[zhaobin, 2017-03-02 20:24:04]

+haraken@ to review histograms.xml changes:

tools/metrics/histograms/histograms.xml

https://codereview.chromium.org/2679893002/diff/40001/chrome/browser/media/ro...
File chrome/browser/media/router/media_router.h (right):

https://codereview.chromium.org/2679893002/diff/40001/chrome/browser/media/ro...
chrome/browser/media/router/media_router.h:67: using MediaSinkList =
std::vector<MediaSinkInternal>;
On 2017/03/01 06:31:43, mark a. foltz wrote:
> Nit: For consistency with other APIs, use std::vector explicitly instead of
> using.

Done.

https://codereview.chromium.org/2679893002/diff/40001/chrome/browser/media/ro...
chrome/browser/media/router/media_router.h:177: // Called when DIAL or CAST
MediaSinkService finishes sink discovery.
On 2017/03/01 23:28:36, imcheng wrote:
> How about:
> 
> Notifies the Media Router that the list of MediaSinks discovered by a
> MediaSinkService has been updated.
> |provider_name|: Name of the MediaSinkService providing the sinks.
> ...

Done.

https://codereview.chromium.org/2679893002/diff/40001/chrome/browser/media/ro...
File chrome/browser/media/router/mojo/media_router_mojo_impl.cc (right):

https://codereview.chromium.org/2679893002/diff/40001/chrome/browser/media/ro...
chrome/browser/media/router/mojo/media_router_mojo_impl.cc:399:
base::ConstRef(sinks)));
On 2017/03/01 23:28:36, imcheng wrote:
> On 2017/03/01 06:31:43, mark a. foltz wrote:
> > base::Passed?
> 
> or just |sinks|? I assume that the reference is to a vector in a
> MediaSinkService, so we should be making a copy in the callback (in case it
gets
> queued).

Done.

https://codereview.chromium.org/2679893002/diff/40001/chrome/browser/media/ro...
File chrome/browser/media/router/mojo/media_router_mojo_impl_unittest.cc
(right):

https://codereview.chromium.org/2679893002/diff/40001/chrome/browser/media/ro...
chrome/browser/media/router/mojo/media_router_mojo_impl_unittest.cc:1160: const
MediaRouter::MediaSinkList& actual_sinks) {
On 2017/03/01 06:31:43, mark a. foltz wrote:
> Does MediaSink implement == and a copy constructor?  Then it looks like you
> should just be able to write ProvideSinks("cast", sinks).

Done.

https://codereview.chromium.org/2679893002/diff/60001/extensions/renderer/res...
File extensions/renderer/resources/media_router_bindings.js (right):

https://codereview.chromium.org/2679893002/diff/60001/extensions/renderer/res...
extensions/renderer/resources/media_router_bindings.js:857:
MediaRouteProvider.prototype.provideSinks = function(sinks) {
On 2017/03/02 06:04:42, btolsch wrote:
> This seems to be inconsistent with cl/146753004 and media_router.mojom
(missing
> providerName argument).

Done.

[haraken, 2017-03-02 23:18:06]

haraken@chromium.org changed reviewers:
+ isherman@chromium.org

[haraken, 2017-03-02 23:18:07]

+isherman

(I'm eligible to review UseCounter-only changes in histograms.)

[Ilya Sherman, 2017-03-02 23:26:07]

histograms.xml lgtm

[mark a. foltz, 2017-03-18 18:16:38]

On 2017/03/02 at 23:26:07, isherman wrote:
> histograms.xml lgtm

Does this need additional review/work?  (Just going back through my queue.)

[zhaobin, 2017-03-20 18:29:20]

On 2017/03/18 18:16:38, mark a. foltz wrote:
> On 2017/03/02 at 23:26:07, isherman wrote:
> > histograms.xml lgtm
> 
> Does this need additional review/work?  (Just going back through my queue.)

Not now. It depends on cl/146753004 and Derek suggests that we need to figure
out how to correctly map mediaRouterMojom.MediaSink in MR and mr.Sink in MRP
before submitting that CL.

[zhaobin, 2017-04-05 23:14:02]

Description was changed from

==========
[Media Router] Add OnSinksDiscovered() Mojo API

- added OnSinksDiscovered() to media_router.mojom
- added OnSinksDiscovered() to media_router.h

cl/146753004 added OnSinksDiscovered() to component extension.

BUG=687365
==========

to

==========
[Media Router] Add ProvideSinks() Mojo API

- added ProvideSinks() to media_router.mojom
- added ProvideSinks() to media_router.h

cl/146753004 added ProvideSinks() to component extension.

BUG=687365
==========

[zhaobin, 2017-04-06 00:27:01]

The CQ bit was checked by zhaobin@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-06 00:27:27]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-06 00:56:51]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-06 00:56:53]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[zhaobin, 2017-04-06 01:36:45]

The CQ bit was checked by zhaobin@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-06 01:37:07]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-06 02:38:53]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-06 02:38:54]

Dry run: This issue passed the CQ dry run.

[zhaobin, 2017-04-06 02:39:56]

The CQ bit was checked by zhaobin@chromium.org

[zhaobin, 2017-04-06 02:39:57]

The patchset sent to the CQ was uploaded after l-g-t-m from dcheng@chromium.org,
mfoltz@chromium.org, imcheng@chromium.org, isherman@chromium.org
Link to the patchset: https://codereview.chromium.org/2679893002/#ps140001
(title: "fix android compile error")

[commit-bot: I haz the power, 2017-04-06 02:40:14]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-06 02:45:17]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1491446396911260,
"parent_rev": "c21b004df95e3c751bcef1d8bfc0a793140d9410", "commit_rev":
"b254a456ab6527124ce5aff9acc231eed34605c4"}

[commit-bot: I haz the power, 2017-04-06 02:46:11]

Description was changed from

==========
[Media Router] Add ProvideSinks() Mojo API

- added ProvideSinks() to media_router.mojom
- added ProvideSinks() to media_router.h

cl/146753004 added ProvideSinks() to component extension.

BUG=687365
==========

to

==========
[Media Router] Add ProvideSinks() Mojo API

- added ProvideSinks() to media_router.mojom
- added ProvideSinks() to media_router.h

cl/146753004 added ProvideSinks() to component extension.

BUG=687365

Review-Url: https://codereview.chromium.org/2679893002
Cr-Commit-Position: refs/heads/master@{#462333}
Committed:
https://chromium.googlesource.com/chromium/src/+/b254a456ab6527124ce5aff9acc2...
==========

[commit-bot: I haz the power, 2017-04-06 02:46:13]

Committed patchset #8 (id:140001) as
https://chromium.googlesource.com/chromium/src/+/b254a456ab6527124ce5aff9acc2...