[Media Router] Add DialMediaSinkService and DeviceDescriptionService

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

Index: chrome/browser/media/router/discovery/dial/dial_media_sink_service.cc
diff --git a/chrome/browser/media/router/discovery/dial/dial_media_sink_service.cc b/chrome/browser/media/router/discovery/dial/dial_media_sink_service.cc
new file mode 100644
index 0000000000000000000000000000000000000000..2259cce9d6b36a7bbe94fb7fb22fed0159a54421
--- /dev/null
+++ b/chrome/browser/media/router/discovery/dial/dial_media_sink_service.cc
@@ -0,0 +1,148 @@
+// 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.h"
+
+#include "chrome/browser/media/router/discovery/dial/dial_device_data.h"
+#include "chrome/browser/profiles/profile.h"
+#include "content/public/browser/browser_thread.h"
+
+using content::BrowserThread;
+
+namespace {
+// Time interval when media sink service sends sinks to MRP.
+const int kFetchCompleteTimeoutSecs = 3;
+}
+
+namespace media_router {
+
+DialMediaSinkService::DialMediaSinkService(
+    const OnSinksDiscoveredCallback& callback,
+    content::BrowserContext* browser_context)
+    : MediaSinkService(callback), browser_context_(browser_context) {
+  DCHECK_CURRENTLY_ON(BrowserThread::UI);
+  auto* profile = Profile::FromBrowserContext(browser_context_);
+  request_context_ = profile->GetRequestContext();
+
+  DCHECK(request_context_);
+}
+
+DialMediaSinkService::~DialMediaSinkService() {}
+
+void DialMediaSinkService::Start() {

[imcheng, 2017/04/25 01:40:12]

The threading model isn't clear to me; we should clarify it in
MediaSinkService's documentation. If MediaRouter is supposed to call Start() on
the UI thread, then in here we need to PostTask to the IO thread.

[zhaobin, 2017/04/26 18:50:05]

Will do PostTask in MediaRouter. Will make DialMediaSinkService always live on
IO and update documentation.

+  DCHECK_CURRENTLY_ON(BrowserThread::IO);
+  dial_registry()->RegisterObserver(this);
+  dial_registry()->DiscoverNow();
+}
+
+void DialMediaSinkService::Stop() {

[imcheng, 2017/04/25 01:40:13]

Is the plan to have MediaRouter call Stop() when shutting down?

[mark a. foltz, 2017/04/25 21:06:26]

I would assume the MR would call Start/Stop depending on whether there are
MediaSinkObservers for DIAL (to stay in sync with the discovery logic in the
component).  Does this service also need to run for media route discovery?

[zhaobin, 2017/04/26 18:50:05]

Figure out where to call Stop in https://codereview.chromium.org/2837363002/ ?

+  DCHECK_CURRENTLY_ON(BrowserThread::IO);
+  dial_registry()->UnregisterObserver(this);
+}
+
+void DialMediaSinkService::AddSinkQuery(MediaSinksObserver* observer) {
+  // TODO(crbug.com/687375): Implement DIAL sink query.
+  NOTIMPLEMENTED();
+}
+
+void DialMediaSinkService::RemoveSinkQuery(MediaSinksObserver* observer) {
+  // TODO(crbug.com/687375): Implement DIAL sink query.
+  NOTIMPLEMENTED();
+}
+
+DialRegistry* DialMediaSinkService::dial_registry() {

[mark a. foltz, 2017/04/25 21:06:26]

Does this need to be exposed through the API - what needs access to the
DialRegistry?

[zhaobin, 2017/04/26 18:50:05]

For unit tests to override this function...

+  DCHECK_CURRENTLY_ON(BrowserThread::IO);
+  return DialRegistry::GetInstance();
+}
+
+DeviceDescriptionService* DialMediaSinkService::GetDescriptionService() {
+  DCHECK_CURRENTLY_ON(BrowserThread::IO);
+  if (!description_service_.get()) {
+    description_service_.reset(new DeviceDescriptionService(
+        base::Bind(&DialMediaSinkService::OnDeviceDescriptionAvailable,
+                   base::Unretained(this)),
+        base::Bind(&DialMediaSinkService::OnDeviceDescriptionError,
+                   base::Unretained(this))));
+  }
+  return description_service_.get();
+}
+
+void DialMediaSinkService::OnDialDeviceEvent(
+    const DialRegistry::DeviceList& devices) {
+  DCHECK_CURRENTLY_ON(BrowserThread::IO);
+  DVLOG(2) << "DialMediaSinkService::OnDialDeviceEvent found " << devices.size()
+           << " devices";
+
+  // Add a finish timer.
+  finish_timer_.reset(new base::OneShotTimer());
+  base::TimeDelta finish_delay =
+      base::TimeDelta::FromSeconds(kFetchCompleteTimeoutSecs);
+  finish_timer_->Start(FROM_HERE, finish_delay, this,
+                       &DialMediaSinkService::OnFetchCompleted);
+
+  current_sinks_.clear();
+  GetDescriptionService()->GetDeviceDescriptions(devices, request_context_);
+}
+
+void DialMediaSinkService::OnDialError(DialRegistry::DialErrorCode type) {
+  DCHECK_CURRENTLY_ON(BrowserThread::IO);
+  DVLOG(2) << "OnDialError [DialErrorCode]: " << static_cast<int>(type);
+}
+
+void DialMediaSinkService::OnDeviceDescriptionAvailable(
+    const DialDeviceData& device_data,
+    const ParsedDialDeviceDescription& description_data) {
+  DCHECK_CURRENTLY_ON(BrowserThread::IO);
+
+  MediaSink sink(description_data.unique_id, description_data.friendly_name,
+                 MediaSink::IconType::GENERIC);
+  DialSinkExtraData extra_data;
+  extra_data.app_url = description_data.app_url;
+  extra_data.model_name = description_data.model_name;
+  std::string ip_address = device_data.device_description_url().host();
+  if (!extra_data.ip_address.AssignFromIPLiteral(ip_address)) {
+    DVLOG(1) << "Invalid ip_address: " << ip_address;
+    return;
+  }
+
+  current_sinks_.insert(MediaSinkInternal(sink, extra_data));
+
+  if (finish_timer_)
+    return;

[imcheng, 2017/04/26 22:52:15]

IIUC, it seems safe to reset the 3s timer even if the device description came
late. This is so that the extension will be notified quickly. See also comment
below.

+
+  // Start fetch timer again if device description comes back after
+  // |finish_timer_| fires.
+  base::TimeDelta finish_delay =
+      base::TimeDelta::FromSeconds(kFetchCompleteTimeoutSecs);

[imcheng, 2017/04/25 01:40:12]

Seems like DelayTimer is better suited for this:
https://cs.chromium.org/chromium/src/base/timer/timer.h?type=cs&q=f:base+butt...

[zhaobin, 2017/04/26 18:50:05]

I think we may want to fire the first timer event in exact 3s. Use DelayTimer it
may take longer and we cannot control the exact fire time. Since our users have
relative small device list, OneShotTimer() seems ok to me...Would it be a big
performance concern?

[imcheng, 2017/04/26 22:52:16]

What you are doing here is to set a 3s timer initially in OnDialDeviceEvent()
and then resetting it with a new 3s timer whenever OnDeviceDescriptionAvailable
in OnDeviceDescriptionAvailable(). It seems the equivalent would be to creating
a DelayTimer, and calling DelayTimer::Reset() in OnDialDeviceEvent() and
OnDeviceDescriptionAvilable.

Performance wise, I think it would be slightly better if we call
finish_timer_->Reset() instead of re-allocating a new Timer. 

+  finish_timer_.reset(new base::OneShotTimer());
+  finish_timer_->Start(FROM_HERE, finish_delay, this,
+                       &DialMediaSinkService::OnFetchCompleted);
+}
+
+void DialMediaSinkService::OnDeviceDescriptionError(
+    const DialDeviceData& device,
+    const std::string& error_message) {
+  DCHECK_CURRENTLY_ON(BrowserThread::IO);
+  DVLOG(2) << "OnDescriptionFetchesError [message]: " << error_message;
+}
+
+void DialMediaSinkService::OnFetchCompleted() {
+  DCHECK_CURRENTLY_ON(BrowserThread::IO);
+  DCHECK(!sink_discovery_callback_.is_null());
+
+  finish_timer_.reset();
+
+  auto sinks = current_sinks_;
+  if (sinks == mrp_sinks_) {
+    DVLOG(2) << "No update to sink list.";
+    return;
+  }
+
+  DVLOG(2) << "Send sinks to media router, [size]: " << sinks.size();
+  sink_discovery_callback_.Run(

[imcheng, 2017/04/25 01:40:12]

We should be invoking the callback on the UI thread; use PostTask?

[zhaobin, 2017/04/26 18:50:05]

Will do that in MediaRouter...

+      std::vector<MediaSinkInternal>(sinks.begin(), sinks.end()));
+  mrp_sinks_ = std::move(sinks);
+}
+
+}  // namespace media_router