[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..a915f63eb0cf90280deee1572b810c89e35bf36f
--- /dev/null
+++ b/chrome/browser/media/router/discovery/dial/dial_media_sink_service_proxy.cc
@@ -0,0 +1,78 @@
+// 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_impl.h"
+#include "content/public/browser/browser_context.h"
+
+using content::BrowserThread;
+
+namespace media_router {
+
+DialMediaSinkServiceProxy::DialMediaSinkServiceProxy(
+    const MediaSinkService::OnSinksDiscoveredCallback& callback,
+    content::BrowserContext* context)
+    : MediaSinkService(callback), weak_factory_(this) {
+  DCHECK_CURRENTLY_ON(BrowserThread::UI);
+  auto* profile = Profile::FromBrowserContext(context);
+  request_context_ = profile->GetRequestContext();
+  DCHECK(request_context_);
+}
+
+DialMediaSinkServiceProxy::~DialMediaSinkServiceProxy() = default;
+
+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);
+
+  content::BrowserThread::PostTask(
+      content::BrowserThread::IO, FROM_HERE,
+      base::Bind(&DialMediaSinkServiceProxy::StopOnIOThread, this));
+}
+
+void DialMediaSinkServiceProxy::SetDialMediaSinkServiceForTest(
+    std::unique_ptr<DialMediaSinkServiceImpl> dial_media_sink_service) {
+  DCHECK(dial_media_sink_service);
+  DCHECK(!dial_media_sink_service_);
+  dial_media_sink_service_ = std::move(dial_media_sink_service);
+}
+
+void DialMediaSinkServiceProxy::StartOnIOThread() {
+  if (!dial_media_sink_service_) {

[imcheng, 2017/05/26 01:25:59]

Consider adding a DCHECK_CURRENTLY_ON(BrowserThread::IO); here?

[zhaobin, 2017/05/26 20:59:40]

Mark suggests no check for private methods.

+    // Need to explicitly delete |dial_media_sink_service_| outside dtor to

[imcheng, 2017/05/26 01:25:59]

Does this comment mean that we should always call StopOnIOThread()? It seems
dial_media_sink_service_ is being deleted inside the dtor, unless
StopOnIOThread() is called.

I did not fully understand why we need to use WeakPtr instead of ref-counted in
this callback. If |dial_media_sink_service_| is deleted then its reference to
DialMediaSinkServiceProxy would be released, which would allow it to be deleted
when there are no more refs to it. This can be guaranteed if we always make sure
to call StopOnIOThread().

[zhaobin, 2017/05/26 20:59:40]

Done.

+    // avoid circular dependency.
+    dial_media_sink_service_ = base::MakeUnique<DialMediaSinkServiceImpl>(
+        base::Bind(&DialMediaSinkServiceProxy::OnSinksDiscoveredOnIOThread,
+                   weak_factory_.GetWeakPtr()),
+        request_context_);
+  }
+
+  dial_media_sink_service_->Start();
+}
+
+void DialMediaSinkServiceProxy::StopOnIOThread() {
+  if (!dial_media_sink_service_)

[imcheng, 2017/05/26 01:25:59]

ditto on DCHECK_CURRENTLY_ON.

[zhaobin, 2017/05/26 20:59:40]

Mark suggests no check for private methods.

+    return;
+
+  dial_media_sink_service_->Stop();
+  dial_media_sink_service_.reset();
+}
+
+void DialMediaSinkServiceProxy::OnSinksDiscoveredOnIOThread(
+    std::vector<MediaSinkInternal> sinks) {
+  DCHECK_CURRENTLY_ON(BrowserThread::IO);
+  content::BrowserThread::PostTask(
+      content::BrowserThread::UI, FROM_HERE,
+      base::Bind(sink_discovery_callback_, std::move(sinks)));

[imcheng, 2017/05/26 01:25:59]

Why do we need std::move here? base::Bind by default makes a copy of the
arguments being passed in to its internal storage.

[zhaobin, 2017/05/26 20:59:40]

Acknowledged.

+}
+
+}  // namespace media_router