[Media Router] Add some SetForTest() functions to DialMediaSinkService and update unit tests

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

 // 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"
 #include "net/url_request/url_request_context_getter.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,
     net::URLRequestContextGetter* request_context)
-    : MediaSinkService(callback), request_context_(request_context) {
+    : MediaSinkService(callback),
+      request_context_(request_context),
+      discovery_started_(false) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
   DCHECK(request_context_);
 }
 
-DialMediaSinkService::~DialMediaSinkService() {}
+DialMediaSinkService::~DialMediaSinkService() {
+  DCHECK_CURRENTLY_ON(BrowserThread::IO);
+  Stop();
+}
 
 void DialMediaSinkService::Start() {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
-  dial_registry()->RegisterObserver(this);
-  dial_registry()->StartPeriodicDiscovery();
+  if (discovery_started_)
+    return;
+  discovery_started_ = true;
+
+  if (!dial_registry_)
+    dial_registry_ = DialRegistry::GetInstance();
+
+  dial_registry_->RegisterObserver(this);
+  dial_registry_->OnListenerAdded();
 }
 
 void DialMediaSinkService::Stop() {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
-  dial_registry()->UnregisterObserver(this);
-}
+  if (!discovery_started_)
+    return;
+  discovery_started_ = false;
 
-DialRegistry* DialMediaSinkService::dial_registry() {
-  DCHECK_CURRENTLY_ON(BrowserThread::IO);
-  return DialRegistry::GetInstance();
+  DCHECK(dial_registry_);

[Kevin M, 2017/05/10 23:22:01]

It's not possible for dial_registry to be non-null if the service had been
successfully started; this seems too defensive?

[zhaobin, 2017/05/11 19:46:25]

Done.

+  dial_registry_->OnListenerRemoved();
+  dial_registry_->UnregisterObserver(this);
 }
 
 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::SetDialRegistryForTest(DialRegistry* dial_registry) {

[Kevin M, 2017/05/10 23:22:01]

Maybe DCHECK for !discovery_started_ on these SetForTest methods, otherwise the
listeners/observers could be in a bad state

[zhaobin, 2017/05/11 19:46:25]

|discovery_started_| removed. Maybe test code could ensure those logic instead
of adding checks here?

+  dial_registry_ = dial_registry;
+}
+
+void DialMediaSinkService::SetDescriptionServiceForTest(
+    std::unique_ptr<DeviceDescriptionService> description_service) {
+  description_service_ = std::move(description_service);
+}
+
+void DialMediaSinkService::SetTimerForTest(std::unique_ptr<base::Timer> timer) {
+  finish_timer_ = std::move(timer);
+}
+
 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);
+  StartTimer();
 
   current_sinks_.clear();
   current_devices_ = devices;
 
   GetDescriptionService()->GetDeviceDescriptions(devices,
                                                  request_context_.get());
 }
 
 void DialMediaSinkService::OnDialError(DialRegistry::DialErrorCode type) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
@@ skipping 19 matching lines @@
   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;
-
   // Start fetch timer again if device description comes back after
   // |finish_timer_| fires.
-  base::TimeDelta finish_delay =
-      base::TimeDelta::FromSeconds(kFetchCompleteTimeoutSecs);
-  finish_timer_.reset(new base::OneShotTimer());
-  finish_timer_->Start(FROM_HERE, finish_delay, this,
-                       &DialMediaSinkService::OnFetchCompleted);
+  DCHECK(finish_timer_);
+  if (!finish_timer_->IsRunning())
+    StartTimer();
 }
 
 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_) {
+  if (current_sinks_ == mrp_sinks_) {
     DVLOG(2) << "No update to sink list.";
     return;
   }
 
+  auto sinks = current_sinks_;

[Kevin Marshall, 2017/05/10 23:25:10]

This seems like an avoidable copy.

[zhaobin, 2017/05/11 19:46:25]

Done.

   DVLOG(2) << "Send sinks to media router, [size]: " << sinks.size();
   sink_discovery_callback_.Run(
       std::vector<MediaSinkInternal>(sinks.begin(), sinks.end()));
   mrp_sinks_ = std::move(sinks);
 }
 
+void DialMediaSinkService::StartTimer() {
+  DCHECK_CURRENTLY_ON(BrowserThread::IO);
+  // Create a finish timer.
+  if (!finish_timer_)
+    finish_timer_.reset(new base::OneShotTimer());
+
+  base::TimeDelta finish_delay =
+      base::TimeDelta::FromSeconds(kFetchCompleteTimeoutSecs);
+  finish_timer_->Start(FROM_HERE, finish_delay,
+                       base::Bind(&DialMediaSinkService::OnFetchCompleted,
+                                  base::Unretained(this)));
+}
+
 }  // namespace media_router