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

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
index 562dcd5b8a213b51bed1f6e38abd4a3ed215c299..5e5e9562f35c054c5e58d32cd65e6efe6084cbc8 100644
--- a/chrome/browser/media/router/discovery/dial/dial_media_sink_service.cc
+++ b/chrome/browser/media/router/discovery/dial/dial_media_sink_service.cc
@@ -26,22 +26,29 @@ DialMediaSinkService::DialMediaSinkService(
   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 (dial_registry_)
+    return;
+
+  dial_registry_ = DialRegistry::GetInstance();
+  dial_registry_->RegisterObserver(this);
+  dial_registry_->OnListenerAdded();
 }
 
 void DialMediaSinkService::Stop() {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
-  dial_registry()->UnregisterObserver(this);
-}
+  if (!dial_registry_)
+    return;
 
-DialRegistry* DialMediaSinkService::dial_registry() {
-  DCHECK_CURRENTLY_ON(BrowserThread::IO);
-  return DialRegistry::GetInstance();
+  dial_registry_->OnListenerRemoved();
+  dial_registry_->UnregisterObserver(this);
+  dial_registry_ = nullptr;
 }
 
 DeviceDescriptionService* DialMediaSinkService::GetDescriptionService() {
@@ -56,18 +63,30 @@ DeviceDescriptionService* DialMediaSinkService::GetDescriptionService() {
   return description_service_.get();
 }
 
+void DialMediaSinkService::SetDialRegistryForTest(DialRegistry* dial_registry) {
+  dial_registry_ = dial_registry;

[mark a. foltz, 2017/05/11 23:12:14]

DCHECK(!dial_registry_) before assigning?

I assume tests won't call Start() since you want to replace it with a mock.

[zhaobin, 2017/05/12 00:36:09]

Done.

+  if (dial_registry_) {

[mark a. foltz, 2017/05/11 23:12:14]

When would it be okay to pass a nullptr?
Should there be a DCHECK(dial_registry)?

[zhaobin, 2017/05/12 00:36:09]

Done.

+    dial_registry_->RegisterObserver(this);
+    dial_registry_->OnListenerAdded();
+  }
+}
+
+void DialMediaSinkService::SetDescriptionServiceForTest(
+    std::unique_ptr<DeviceDescriptionService> description_service) {
+  description_service_ = std::move(description_service);

[mark a. foltz, 2017/05/11 23:12:14]

It seems like you don't want the test to call GetDeviceDescriptionService()
before this, so DCHECK(!description_service_) before assigning?

[zhaobin, 2017/05/12 00:36:09]

Done.

+}
+
+void DialMediaSinkService::SetTimerForTest(std::unique_ptr<base::Timer> timer) {
+  finish_timer_ = std::move(timer);

[mark a. foltz, 2017/05/11 23:12:14]

Similar comment - you don't want the test to start a timer before assigning the
mock, so 
DCHECK(!finish_timer_) before assignment

[zhaobin, 2017/05/12 00:36:09]

Done.

+}
+
 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;
@@ -107,16 +126,11 @@ void DialMediaSinkService::OnDeviceDescriptionAvailable(
 
   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_);

[mark a. foltz, 2017/05/11 23:12:13]

Is it possible for this to fail?  It looks like this can only be called after
OnDialDeviceEvent() which calls StartTimer().

Also, if finish_timer_ is not set, the following line will crash so the DCHECK
doesn't add a lot of new information here.

[zhaobin, 2017/05/12 00:36:09]

Done.

+  if (!finish_timer_->IsRunning())
+    StartTimer();
 }
 
 void DialMediaSinkService::OnDeviceDescriptionError(
@@ -130,18 +144,28 @@ void DialMediaSinkService::OnFetchCompleted() {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);

[mark a. foltz, 2017/05/11 23:12:14]

Super nit: This is only called from a callback passed to a timer owned by this
class; as long as the method creating the timer is run on the IO thread, this
will be too.  In that case, checking the thread again seems unnecessary to me.

[zhaobin, 2017/05/12 00:36:09]

Done.

   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;
   }
 
-  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);
+  DVLOG(2) << "Send sinks to media router, [size]: " << current_sinks_.size();
+  sink_discovery_callback_.Run(std::vector<MediaSinkInternal>(
+      current_sinks_.begin(), current_sinks_.end()));
+  mrp_sinks_ = current_sinks_;
+}
+
+void DialMediaSinkService::StartTimer() {
+  DCHECK_CURRENTLY_ON(BrowserThread::IO);

[mark a. foltz, 2017/05/11 23:12:14]

Similar nit: This is only called from within this class.  As long as the public
API and exported callbacks have thread checks, this is not necessary.

[zhaobin, 2017/05/12 00:36:09]

Done.

+  // 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