[Media Router] Support dual discovery

[Media Router] Support dual discovery

- Added MediaSinkServiceObserver class to monitor OnMediaSinkAdded() and OnMediaSinksRemoved()
- CastMediaSinkService implements MediaSinkServiceObserver
- MediaRouterMojoImpl registers CastMediaSinkService as an observer of DialMediaSinkService

Dual discovery process:
  DialMediaSinkServiceImpl::OnDeviceDescriptionAvailable()
    CastMediaSinkService::OnMediaSinkAdded()
      CastMediaSinkService::OpenChannelOnIOThread()
        ... async open channel
          CastMediaSinkService::OnChannelOpenedOnUIThread() if succeeded, add cast_sink to |current_sinks_by_dial_map_|

CastMediaSinkService::OnFetchCompleted() is invoked when timer expires. It will merge sinks discovered by mDNS service and DIAL.

Moved to: https://chromium-review.googlesource.com/c/590510

BUG=687377

[zhaobin, 2017-07-05 17:28:44]

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

[imcheng, 2017-07-06 22:53:30]

Looks fine overall. Since dual discovery is quite specific, I would not mind
that there are specific interactions between the CastMediaSinkService and
DialMediaSinkService, rather than introducing generic observer APIs in the base
class. That is, DialMediaSinkService can hold a reference to
CastMediaSinkService and inform it directly when sinks are available. WDYT?

https://codereview.chromium.org/2965843002/diff/40001/chrome/browser/media/ro...
File chrome/browser/media/router/discovery/dial/dial_media_sink_service_impl.cc
(right):

https://codereview.chromium.org/2965843002/diff/40001/chrome/browser/media/ro...
chrome/browser/media/router/discovery/dial/dial_media_sink_service_impl.cc:82:
MediaSinkServiceBase::RemoveSinks();
Does Cast care that sinks are being removed from DIAL's perspective?

https://codereview.chromium.org/2965843002/diff/40001/chrome/browser/media/ro...
File
chrome/browser/media/router/discovery/dial/dial_media_sink_service_impl_unittest.cc
(right):

https://codereview.chromium.org/2965843002/diff/40001/chrome/browser/media/ro...
chrome/browser/media/router/discovery/dial/dial_media_sink_service_impl_unittest.cc:112:
EXPECT_EQ(size_t(1), media_sink_service_->GetCurrentSinksSize());
1u

https://codereview.chromium.org/2965843002/diff/40001/chrome/browser/media/ro...
File chrome/browser/media/router/discovery/mdns/cast_media_sink_service.cc
(right):

https://codereview.chromium.org/2965843002/diff/40001/chrome/browser/media/ro...
chrome/browser/media/router/discovery/mdns/cast_media_sink_service.cc:225: this,
cast_sink));
We can enable moves for MediaSinkInternal to make passing it to tasks a bit more
efficient. WDYT?

https://codereview.chromium.org/2965843002/diff/40001/chrome/browser/media/ro...
chrome/browser/media/router/discovery/mdns/cast_media_sink_service.cc:234: const
cast_channel::CastSocket::OnOpenCallback& on_open_callback) {
Does this need to take a callback as input, or can it take a cast_sink and we
create the callback here?

https://codereview.chromium.org/2965843002/diff/40001/chrome/browser/media/ro...
File chrome/browser/media/router/discovery/mdns/cast_media_sink_service.h
(right):

https://codereview.chromium.org/2965843002/diff/40001/chrome/browser/media/ro...
chrome/browser/media/router/discovery/mdns/cast_media_sink_service.h:48: static
const char kCastServiceType[];
Can this and kCastControlPort be constexpr and be defined here?

[zhaobin, 2017-07-10 19:16:20]

Patchset #5 (id:80001) has been deleted

[zhaobin, 2017-07-10 19:22:06]

Patchset #5 (id:100001) has been deleted

[zhaobin, 2017-07-10 20:21:15]

https://codereview.chromium.org/2965843002/diff/40001/chrome/browser/media/ro...
File chrome/browser/media/router/discovery/dial/dial_media_sink_service_impl.cc
(right):

https://codereview.chromium.org/2965843002/diff/40001/chrome/browser/media/ro...
chrome/browser/media/router/discovery/dial/dial_media_sink_service_impl.cc:82:
MediaSinkServiceBase::RemoveSinks();
On 2017/07/06 22:53:30, imcheng wrote:
> Does Cast care that sinks are being removed from DIAL's perspective?

I think it is needed in current implementation because we do not remove sinks
from CastMediaSinkService on ChannelError (working on a patch to fix that but
want to land this first).

https://codereview.chromium.org/2965843002/diff/40001/chrome/browser/media/ro...
File
chrome/browser/media/router/discovery/dial/dial_media_sink_service_impl_unittest.cc
(right):

https://codereview.chromium.org/2965843002/diff/40001/chrome/browser/media/ro...
chrome/browser/media/router/discovery/dial/dial_media_sink_service_impl_unittest.cc:112:
EXPECT_EQ(size_t(1), media_sink_service_->GetCurrentSinksSize());
On 2017/07/06 22:53:30, imcheng wrote:
> 1u

Done.

https://codereview.chromium.org/2965843002/diff/40001/chrome/browser/media/ro...
File chrome/browser/media/router/discovery/mdns/cast_media_sink_service.cc
(right):

https://codereview.chromium.org/2965843002/diff/40001/chrome/browser/media/ro...
chrome/browser/media/router/discovery/mdns/cast_media_sink_service.cc:225: this,
cast_sink));
On 2017/07/06 22:53:30, imcheng wrote:
> We can enable moves for MediaSinkInternal to make passing it to tasks a bit
more
> efficient. WDYT?

Done.

https://codereview.chromium.org/2965843002/diff/40001/chrome/browser/media/ro...
chrome/browser/media/router/discovery/mdns/cast_media_sink_service.cc:234: const
cast_channel::CastSocket::OnOpenCallback& on_open_callback) {
On 2017/07/06 22:53:30, imcheng wrote:
> Does this need to take a callback as input, or can it take a cast_sink and we
> create the callback here?

Done.

https://codereview.chromium.org/2965843002/diff/40001/chrome/browser/media/ro...
File chrome/browser/media/router/discovery/mdns/cast_media_sink_service.h
(right):

https://codereview.chromium.org/2965843002/diff/40001/chrome/browser/media/ro...
chrome/browser/media/router/discovery/mdns/cast_media_sink_service.h:48: static
const char kCastServiceType[];
On 2017/07/06 22:53:30, imcheng wrote:
> Can this and kCastControlPort be constexpr and be defined here?

Yes, but still need to define it again in .cc file. Leave it as it is?

[mark a. foltz, 2017-07-11 23:52:51]

Overall looks good - a couple of design suggestions.  I didn't review all of the
unit test in this pass.

https://codereview.chromium.org/2965843002/diff/120001/chrome/browser/media/r...
File chrome/browser/media/router/discovery/dial/dial_media_sink_service_impl.cc
(right):

https://codereview.chromium.org/2965843002/diff/120001/chrome/browser/media/r...
chrome/browser/media/router/discovery/dial/dial_media_sink_service_impl.cc:89:
cast_media_sink_service_->OnDialSinksRemoved();
It looks like the DMSS doesn't need to take a dependency directly onto
CastMediaSinkService.  This can be captured by factoring out a
DialMediaSinkServiceDelegate interface that contains only OnDialSinksRemoved()
and OnDialSinkAdded(), and having CMSS implement that.

https://codereview.chromium.org/2965843002/diff/120001/chrome/browser/media/r...
File chrome/browser/media/router/discovery/dial/dial_media_sink_service_impl.h
(right):

https://codereview.chromium.org/2965843002/diff/120001/chrome/browser/media/r...
chrome/browser/media/router/discovery/dial/dial_media_sink_service_impl.h:30:
scoped_refptr<CastMediaSinkService> cast_media_sink_service,
Since the CMSS is optional for the DialMediaSinkService to function, I would
prefer that it be passed through a setter on the DMSS.
This would remove the need to pass nullptr in the unit tests.

https://codereview.chromium.org/2965843002/diff/120001/chrome/browser/media/r...
File chrome/browser/media/router/discovery/mdns/cast_media_sink_service.cc
(right):

https://codereview.chromium.org/2965843002/diff/120001/chrome/browser/media/r...
chrome/browser/media/router/discovery/mdns/cast_media_sink_service.cc:42:
net::HostPortPair host_port_pair =
It looks like the local_discovery::ServiceDescription already passes this as a
net::HostPortPair.

https://cs.chromium.org/chromium/src/chrome/browser/local_discovery/service_d...

For some reason we convert it to a string along the way.  Would it be possible
to eliminate this parsing by just passing the original struct?

(Fine to do as a followup patch as that will touch several layers of unrelated
code.)

https://codereview.chromium.org/2965843002/diff/120001/chrome/browser/media/r...
File chrome/browser/media/router/discovery/mdns/cast_media_sink_service.h
(right):

https://codereview.chromium.org/2965843002/diff/120001/chrome/browser/media/r...
chrome/browser/media/router/discovery/mdns/cast_media_sink_service.h:137:
std::set<net::IPEndPoint> current_service_ip_endpoints_;
Can this be derived by iterating over current_sinks_by_mdns_map_?

https://codereview.chromium.org/2965843002/diff/120001/chrome/browser/media/r...
chrome/browser/media/router/discovery/mdns/cast_media_sink_service.h:140:
std::map<net::IPEndPoint, MediaSinkInternal> current_sinks_by_mdns_map_;
Nit: I would drop the 'map_' at end.

https://codereview.chromium.org/2965843002/diff/120001/chrome/browser/media/r...
chrome/browser/media/router/discovery/mdns/cast_media_sink_service.h:143:
std::map<net::IPEndPoint, MediaSinkInternal> current_sinks_by_dial_map_;
Consider defining a type alias (e.g. MediaSinkInternalMap) if it helps
readability.

https://codereview.chromium.org/2965843002/diff/120001/chrome/browser/media/r...
File
chrome/browser/media/router/discovery/mdns/cast_media_sink_service_unittest.cc
(right):

https://codereview.chromium.org/2965843002/diff/120001/chrome/browser/media/r...
chrome/browser/media/router/discovery/mdns/cast_media_sink_service_unittest.cc:30:
constexpr char SERVICE_TYPE[] = "_googlecast._tcp.local";
Can you use the constant already declared in CastMediaSinkService?

https://codereview.chromium.org/2965843002/diff/120001/chrome/browser/media/r...
chrome/browser/media/router/discovery/mdns/cast_media_sink_service_unittest.cc:34:
struct TestSinkParams {
I wonder if this code to construct test data is useful in other unit tests?

[imcheng, 2017-07-12 00:51:36]

https://codereview.chromium.org/2965843002/diff/40001/chrome/browser/media/ro...
File chrome/browser/media/router/discovery/dial/dial_media_sink_service_impl.cc
(right):

https://codereview.chromium.org/2965843002/diff/40001/chrome/browser/media/ro...
chrome/browser/media/router/discovery/dial/dial_media_sink_service_impl.cc:82:
MediaSinkServiceBase::RemoveSinks();
On 2017/07/10 20:21:15, zhaobin wrote:
> On 2017/07/06 22:53:30, imcheng wrote:
> > Does Cast care that sinks are being removed from DIAL's perspective?
> 
> I think it is needed in current implementation because we do not remove sinks
> from CastMediaSinkService on ChannelError (working on a patch to fix that but
> want to land this first).

Ok, so IIUC, once the ChannelError fix is landed, then this won't be necessary?

https://codereview.chromium.org/2965843002/diff/40001/chrome/browser/media/ro...
File
chrome/browser/media/router/discovery/dial/dial_media_sink_service_impl_unittest.cc
(right):

https://codereview.chromium.org/2965843002/diff/40001/chrome/browser/media/ro...
chrome/browser/media/router/discovery/dial/dial_media_sink_service_impl_unittest.cc:112:
EXPECT_EQ(size_t(1), media_sink_service_->GetCurrentSinksSize());
On 2017/07/10 20:21:15, zhaobin wrote:
> On 2017/07/06 22:53:30, imcheng wrote:
> > 1u
> 
> Done.

Not done? :)

https://codereview.chromium.org/2965843002/diff/120001/chrome/browser/media/r...
File chrome/browser/media/router/discovery/mdns/cast_media_sink_service.cc
(right):

https://codereview.chromium.org/2965843002/diff/120001/chrome/browser/media/r...
chrome/browser/media/router/discovery/mdns/cast_media_sink_service.cc:164:
content::BrowserThread::IO);
It might be easier to migrate this code to use TaskTraits if we kept a
scoped_refptr<SequencedTaskRunner> in this class, and use it throughout when we
need to post a task to it.

Optional to fix in this patch: replace calls of
DCHECK_CURRENTLY_ON(content::BrowserThread::IO) with a SequenceChecker. It might
be less confusing to change the current ThreadChecker calls to
DCHECK_CURRENTLY_ON(content::BrowserThread::UI).

https://codereview.chromium.org/2965843002/diff/120001/chrome/browser/media/r...
chrome/browser/media/router/discovery/mdns/cast_media_sink_service.cc:167:
base::BindOnce(&CastMediaSinkService::GetCastSinksOnIOThread, this),
Why do we need a thread hop now, but did not before in prior patchsets? This
seems a bit more complicated...

https://codereview.chromium.org/2965843002/diff/120001/chrome/browser/media/r...
chrome/browser/media/router/discovery/mdns/cast_media_sink_service.cc:213:
cast_sinks.push_back(cast_sink);
std::move(cast_sink)

https://codereview.chromium.org/2965843002/diff/120001/chrome/browser/media/r...
chrome/browser/media/router/discovery/mdns/cast_media_sink_service.cc:231: for
(const auto& cast_sink : cast_sinks) {
No const, so you can std::move in L236.

https://codereview.chromium.org/2965843002/diff/120001/chrome/browser/media/r...
chrome/browser/media/router/discovery/mdns/cast_media_sink_service.cc:253:
OpenChannelOnIOThread(ip_endpoint, cast_sink);
Inline CreateCastSinkFromDialSink here.

https://codereview.chromium.org/2965843002/diff/120001/chrome/browser/media/r...
chrome/browser/media/router/discovery/mdns/cast_media_sink_service.cc:264: const
MediaSinkInternal& cast_sink) {
pass by value here, since it will be moved.

https://codereview.chromium.org/2965843002/diff/120001/chrome/browser/media/r...
chrome/browser/media/router/discovery/mdns/cast_media_sink_service.cc:273:
base::Bind(&CastMediaSinkService::OnChannelOpenedOnIOThread, this,
BindOnce?

https://codereview.chromium.org/2965843002/diff/120001/chrome/browser/media/r...
chrome/browser/media/router/discovery/mdns/cast_media_sink_service.cc:320:
current_sinks_by_dial_map_[ip_endpoint] = updated_sink;
nit: I personally think what you had in PS3 (updating the current_sinks_* maps)
was a bit simpler, but I guess it comes down to whether it makes more sense to
do a thread hop here vs. in OnFetchCompleted. Your call.

https://codereview.chromium.org/2965843002/diff/120001/chrome/common/media_ro...
File chrome/common/media_router/discovery/media_sink_internal.h (right):

https://codereview.chromium.org/2965843002/diff/120001/chrome/common/media_ro...
chrome/common/media_router/discovery/media_sink_internal.h:68:
MediaSinkInternal();
You'll need to implement move constructor and the move assignment operator in
order to support std::move, otherwise std::move has no effect. You will also
need to make sure all of the members support std::move as well.

[mark a. foltz, 2017-07-27 20:33:44]

Would you mind uploading this patch to Gerritt?  I'm no longer checking Reitveld
(old code review tool) for patches to review.

Thanks!

[chromium-reviews, 2017-07-27 20:35:20]

Sure.

On Thu, Jul 27, 2017 at 1:33 PM, <mfoltz@chromium.org> wrote:

> Would you mind uploading this patch to Gerritt? I'm no longer checking
> Reitveld
> (old code review tool) for patches to review.
>
> Thanks!
>
>
> https://codereview.chromium.org/2965843002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[zhaobin, 2017-08-17 19:16:13]

Description was changed from

==========
[Media Router] Support dual discovery

- Added MediaSinkServiceObserver class to monitor OnMediaSinkAdded() and
OnMediaSinksRemoved()
- CastMediaSinkService implements MediaSinkServiceObserver
- MediaRouterMojoImpl registers CastMediaSinkService as an observer of
DialMediaSinkService

Dual discovery process:
  DialMediaSinkServiceImpl::OnDeviceDescriptionAvailable()
    CastMediaSinkService::OnMediaSinkAdded()
      CastMediaSinkService::OpenChannelOnIOThread()
        ... async open channel
          CastMediaSinkService::OnChannelOpenedOnUIThread() if succeeded, add
cast_sink to |current_sinks_by_dial_map_|

CastMediaSinkService::OnFetchCompleted() is invoked when timer expires. It will
merge sinks discovered by mDNS service and DIAL.

BUG=687377
==========

to

==========
[Media Router] Support dual discovery

- Added MediaSinkServiceObserver class to monitor OnMediaSinkAdded() and
OnMediaSinksRemoved()
- CastMediaSinkService implements MediaSinkServiceObserver
- MediaRouterMojoImpl registers CastMediaSinkService as an observer of
DialMediaSinkService

Dual discovery process:
  DialMediaSinkServiceImpl::OnDeviceDescriptionAvailable()
    CastMediaSinkService::OnMediaSinkAdded()
      CastMediaSinkService::OpenChannelOnIOThread()
        ... async open channel
          CastMediaSinkService::OnChannelOpenedOnUIThread() if succeeded, add
cast_sink to |current_sinks_by_dial_map_|

CastMediaSinkService::OnFetchCompleted() is invoked when timer expires. It will
merge sinks discovered by mDNS service and DIAL.

Moved to: https://chromium-review.googlesource.com/c/590510

BUG=687377
==========