[Media Router] Use DialMediaSinkService in MediaRouterMojoImpl

chrome/browser/media/router/discovery/dial/dial_media_sink_service_proxy.cc

Index: chrome/browser/media/router/discovery/dial/dial_media_sink_service_proxy.cc
diff --git a/chrome/browser/media/router/discovery/dial/dial_media_sink_service_proxy.cc b/chrome/browser/media/router/discovery/dial/dial_media_sink_service_proxy.cc
new file mode 100644
index 0000000000000000000000000000000000000000..a8bf4e180d1e68f8f21329ae3ee90084211330d9
--- /dev/null
+++ b/chrome/browser/media/router/discovery/dial/dial_media_sink_service_proxy.cc
@@ -0,0 +1,76 @@
+// Copyright 2017 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.
+
+#include "chrome/browser/media/router/discovery/dial/dial_media_sink_service_proxy.h"
+
+#include "chrome/browser/media/router/discovery/dial/dial_media_sink_service.h"
+
+using content::BrowserThread;
+
+namespace media_router {
+
+DialMediaSinkServiceProxy::DialMediaSinkServiceProxy(
+    const MediaSinkService::OnSinksDiscoveredCallback& callback,
+    net::URLRequestContextGetter* request_context)
+    : MediaSinkService(callback) {

[Kevin M, 2017/05/15 19:56:58]

This is what I was referring to about making MediaSinkService into a pure
virtual interface. The complexity of introducing a subclass seems overkill if
all we are doing is storing a callback.

[mark a. foltz, 2017/05/15 21:36:27]

The main purpose is to isolate threading.

[zhaobin, 2017/05/16 21:26:49]

Besides, we are going to have CastMediaSinkService in future.

+  DCHECK_CURRENTLY_ON(BrowserThread::UI);
+  dial_media_sink_service_.reset(new DialMediaSinkService(
+      base::Bind(&DialMediaSinkServiceProxy::OnSinksDiscoveredOnIOThread,

[Kevin M, 2017/05/15 19:56:58]

I think there might be a deletion race condition here. You might need to have
the OnSinksDiscoveredOnIOThread trampoline use a UI threadbound WeakPtr to get
back to the UI thread.

(seeing base::Unretained() used for cross-thread interactions makes me nervous)

[zhaobin, 2017/05/16 21:26:49]

I think it is safe. |this| owns dial_media_sink_service_,
dial_media_sink_service_ just store this callback, does not post it anywhere. If
|this| is destroyed, this callback wont be invoked. Inside
OnSinksDiscoveredOnIOThread, task is posted with |this|.

+                 base::Unretained(this)),
+      request_context));
+}
+
+DialMediaSinkServiceProxy::~DialMediaSinkServiceProxy() {}
+
+void DialMediaSinkServiceProxy::Start() {
+  DCHECK_CURRENTLY_ON(BrowserThread::UI);
+
+  content::BrowserThread::PostTask(
+      content::BrowserThread::IO, FROM_HERE,
+      base::Bind(&DialMediaSinkServiceProxy::StartOnIOThread, this));
+}
+
+void DialMediaSinkServiceProxy::Stop() {
+  DCHECK_CURRENTLY_ON(BrowserThread::UI);
+  sink_discovery_callback_.Reset();
+
+  content::BrowserThread::PostTask(
+      content::BrowserThread::IO, FROM_HERE,
+      base::Bind(&DialMediaSinkServiceProxy::StopOnIOThread, this));
+}
+
+void DialMediaSinkServiceProxy::SetDialMediaSinkServiceForTest(
+    std::unique_ptr<DialMediaSinkService,
+                    content::BrowserThread::DeleteOnIOThread>
+        dial_media_sink_service) {
+  DCHECK(dial_media_sink_service);
+  dial_media_sink_service_ = std::move(dial_media_sink_service);
+}
+
+void DialMediaSinkServiceProxy::StartOnIOThread() {
+  DCHECK(dial_media_sink_service_);

[Kevin M, 2017/05/15 19:56:58]

What is the benefit of these methods vs. just posting tasks to
MediaSinkService::Start() directly?

[zhaobin, 2017/05/16 21:26:49]

|this| is RefCountedThreadSafe, MediaSinkService is not. If we post task
direclty to MediaSinkService::Start(), |this| and MediaSinkService object could
be destroyed before callback is invoked.

+  dial_media_sink_service_->Start();
+}
+
+void DialMediaSinkServiceProxy::StopOnIOThread() {
+  DCHECK(dial_media_sink_service_);
+  dial_media_sink_service_->Stop();
+}
+
+void DialMediaSinkServiceProxy::OnSinksDiscoveredOnIOThread(
+    const std::vector<MediaSinkInternal>& sinks) {
+  DCHECK_CURRENTLY_ON(BrowserThread::IO);
+  content::BrowserThread::PostTask(
+      content::BrowserThread::UI, FROM_HERE,
+      base::Bind(&DialMediaSinkServiceProxy::OnSinksDiscoveredOnUIThread, this,
+                 std::vector<MediaSinkInternal>(sinks.begin(), sinks.end())));
+}
+
+void DialMediaSinkServiceProxy::OnSinksDiscoveredOnUIThread(
+    const std::vector<MediaSinkInternal>& sinks) {
+  if (sink_discovery_callback_)

[Kevin M, 2017/05/15 19:56:58]

Can we PostTask to sink_discovery_callback_ directly instead?

[zhaobin, 2017/05/16 21:26:49]

Similar to above. sink_discovery_callback_ binds to
MediaRouterMojoImpl::ProvideSinks(), MediaRouteMojoImpl is not
RefCountedThreadSafe.

+    sink_discovery_callback_.Run(sinks);
+}
+
+}  // namespace media_router