- Invoke FinishDiscovery() if no network interfaces are detected.

chrome/browser/extensions/api/dial/dial_service.cc

Index: chrome/browser/extensions/api/dial/dial_service.cc
diff --git a/chrome/browser/extensions/api/dial/dial_service.cc b/chrome/browser/extensions/api/dial/dial_service.cc
index a18711a9799c6211bb2978491b8d593d63fcc34e..d576795bcb7552432547e14fd9187294234937d3 100644
--- a/chrome/browser/extensions/api/dial/dial_service.cc
+++ b/chrome/browser/extensions/api/dial/dial_service.cc
@@ -42,8 +42,8 @@ namespace extensions {
 
 namespace {
 
-// The total number of requests to make.
-const int kDialNumRequests = 1;
+// The total number of requests to make per discovery cycle.
+const int kDialMaxRequests = 4;
 
 // The interval to wait between successive requests.
 const int kDialRequestIntervalMillis = 1000;
@@ -102,8 +102,16 @@ std::string BuildRequest() {
 }  // namespace
 
 DialServiceImpl::DialServiceImpl(net::NetLog* net_log)
-  : is_writing_(false), is_reading_(false), discovery_active_(false),
-    num_requests_sent_(0) {
+  : is_writing_(false),
+    is_reading_(false),
+    discovery_active_(false),
+    num_requests_sent_(0),
+    max_requests_(kDialMaxRequests),
+    finish_delay_(TimeDelta::FromMilliseconds((kDialMaxRequests - 1) *
+                                                kDialRequestIntervalMillis) +
+                  TimeDelta::FromSeconds(kDialResponseTimeoutSecs)),
+    request_interval_(TimeDelta::FromMilliseconds(kDialRequestIntervalMillis))
+ {
   IPAddressNumber address;
   bool result = net::ParseIPLiteralToNumber(kDialRequestAddress, &address);
   DCHECK(result);
@@ -112,9 +120,6 @@ DialServiceImpl::DialServiceImpl(net::NetLog* net_log)
   net_log_ = net_log;
   net_log_source_.type = net::NetLog::SOURCE_UDP_SOCKET;
   net_log_source_.id = net_log_->NextID();
-  finish_delay_ = TimeDelta::FromMilliseconds((kDialNumRequests - 1) *
-                                              kDialRequestIntervalMillis) +
-    TimeDelta::FromSeconds(kDialResponseTimeoutSecs);
 }
 
 DialServiceImpl::~DialServiceImpl() {
@@ -144,24 +149,26 @@ bool DialServiceImpl::Discover() {
 
   DVLOG(1) << "Discovery started.";
 
-  // TODO(mfoltz): Send multiple requests.
-  StartRequest();
+  StartDiscovery();
   return true;
 }
 
-void DialServiceImpl::FinishDiscovery() {
+void DialServiceImpl::StartDiscovery() {
   DCHECK(thread_checker_.CalledOnValidThread());
   DCHECK(discovery_active_);
-  DVLOG(1) << "Discovery finished.";
-  CloseSocket();
-  finish_timer_.Stop();
-  discovery_active_ = false;
-  num_requests_sent_ = 0;
-  FOR_EACH_OBSERVER(Observer, observer_list_, OnDiscoveryFinished(this));
+  if (socket_.get())
+    return;
+
+  // TODO(mfoltz): Add a net::NetworkChangeNotifier() to listen for connection

[justinlin, 2013/02/07 09:03:24]

This TODO no longer needed.

[mark a. foltz, 2013/02/07 22:36:55]

Done.

+  // type/IP address changes, and notify via observer.  Also sanity check the
+  // connection type, i.e. !IsOffline && !IsCellular
+  // http://crbug.com/165290
+  BrowserThread::PostTask(BrowserThread::FILE, FROM_HERE, base::Bind(
+      &DialServiceImpl::DoGetNetworkList, this));
 }
 
-bool DialServiceImpl::BindAndWriteSocket(
-      const NetworkInterface& bind_interface) {
+bool DialServiceImpl::BindSocketAndSendRequest(
+      const IPAddressNumber& bind_ip_address) {
   DCHECK(thread_checker_.CalledOnValidThread());
   DCHECK(!socket_.get());
 
@@ -173,29 +180,43 @@ bool DialServiceImpl::BindAndWriteSocket(
   socket_->AllowBroadcast();
 
   // Schedule a timer to finish the discovery process (and close the socket).
-  finish_timer_.Start(FROM_HERE,
-                      finish_delay_,
-                      this,
-                      &DialServiceImpl::FinishDiscovery);
+  if (finish_delay_ > TimeDelta::FromSeconds(0)) {
+    finish_timer_.Start(FROM_HERE,
+                        finish_delay_,
+                        this,
+                        &DialServiceImpl::FinishDiscovery);
+  }
 
   // 0 means bind a random port
-  IPEndPoint address(bind_interface.address, 0);
+  IPEndPoint address(bind_ip_address, 0);
 
   if (!CheckResult("Bind", socket_->Bind(address)))
     return false;
 
-  if (!socket_.get()) {
-    DLOG(WARNING) << "Socket not connected.";
+  recv_buffer_ = new IOBufferWithSize(kDialRecvBufferSize);
+  if (!ReadSocket()) {

[justinlin, 2013/02/07 09:03:24]

no braces

[mark a. foltz, 2013/02/07 22:36:55]

Done.

     return false;
   }
+  SendOneRequest();
+  return true;
+}
 
-  recv_buffer_ = new IOBufferWithSize(kDialRecvBufferSize);
-  ReadSocket();
-
+void DialServiceImpl::SendOneRequest() {
+  if (num_requests_sent_ == max_requests_) {
+    request_timer_.Stop();
+    return;
+  }
+  num_requests_sent_++;
+  if (!socket_.get()) {
+    DLOG(WARNING) << "Socket not connected.";
+    return;
+  }
   if (is_writing_) {
     DVLOG(1) << "Already writing.";
-    return false;
+    return;
   }
+  DVLOG(1) << "Sending request " << num_requests_sent_ << "/"
+           << max_requests_;
   is_writing_ = true;
   int result = socket_->SendTo(
       send_buffer_.get(),
@@ -206,21 +227,7 @@ bool DialServiceImpl::BindAndWriteSocket(
     // Synchronous write.
     OnSocketWrite(result);
   }
-  return result_ok;
-}
-
-void DialServiceImpl::StartRequest() {
-  DCHECK(thread_checker_.CalledOnValidThread());
-  DCHECK(discovery_active_);
-  if (socket_.get())
-    return;
-
-  // TODO(mfoltz): Add a net::NetworkChangeNotifier() to listen for connection
-  // type/IP address changes, and notify via observer.  Also sanity check the
-  // connection type, i.e. !IsOffline && !IsCellular
-  // http://crbug.com/165290
-  BrowserThread::PostTask(BrowserThread::FILE, FROM_HERE, base::Bind(
-      &DialServiceImpl::DoGetNetworkList, this));
+  return;
 }
 
 void DialServiceImpl::OnSocketWrite(int result) {
@@ -228,7 +235,6 @@ void DialServiceImpl::OnSocketWrite(int result) {
   is_writing_ = false;
   if (!CheckResult("OnSocketWrite", result))
     return;
-
   if (result != send_buffer_->size()) {
     DLOG(ERROR) << "Sent " << result << " chars, expected "
                << send_buffer_->size() << " chars";
@@ -239,7 +245,19 @@ void DialServiceImpl::OnSocketWrite(int result) {
     return;
   }
   FOR_EACH_OBSERVER(Observer, observer_list_, OnDiscoveryRequest(this));
-  num_requests_sent_++;
+  // If we need to send additional requests, schedule a timer to do so.
+  if (num_requests_sent_ < max_requests_) {
+    if (request_interval_ <= TimeDelta::FromSeconds(0)) {

[justinlin, 2013/02/07 09:03:24]

Is this check needed? Seems like scheduling a timer at 0 seconds would pretty
much do the same thing.

[mark a. foltz, 2013/02/07 22:36:55]

Scheduling a zero-delay timer didn't result in a transfer of control flow to the
posted task, so the unit test would exit before any of the posted tasks fired. 
It might be clearer to disable the timers by setting a flag like
DisableTimersForTest()?

[justinlin, 2013/02/07 22:42:05]

Ah, if it's for testing, then I think it's possible to test without needing
this. You just need a MessageLoop, and use "RunLoop run_loop;
run_loop.RunUntilIdle();".
If you need to wait for an event, you can also use "run_loop.QuitClosure()" and
run_loop.Run()". I have an example here:
https://code.google.com/p/chromium/codesearch#chrome/src/content/browser/rend...

But, I think, depending on the test, run_loop.RunUntilIdle should be enough.

[mark a. foltz, 2013/02/08 00:04:50]

Thanks for the tip.  I wasn't able to use RunLoop since the socket libraries
expect a MessageLoop with type = TYPE_IO, but RunUntilIdle worked there too. 

+      SendOneRequest();
+    } else if (num_requests_sent_ == 1) {
+      request_timer_.Start(FROM_HERE,
+                           request_interval_,
+                           this,
+                           &DialServiceImpl::SendOneRequest);
+    } else {
+      DCHECK(request_timer_.IsRunning());

[justinlin, 2013/02/07 09:03:24]

I'm not sure this DCHECK is valid. Since the DialService can be shutdown
arbitrarily due to network events, it can probably happen that we scheduled a
SocketRead, but we end up being shutdown when this is run.

[mark a. foltz, 2013/02/07 22:36:55]

Good point.  Removed.

+    }
+  }
 }
 
 bool DialServiceImpl::ReadSocket() {
@@ -371,7 +389,8 @@ bool DialServiceImpl::ParseResponse(const std::string& response,
 void DialServiceImpl::SendNetworkList(const NetworkInterfaceList& networks) {
   DCHECK(thread_checker_.CalledOnValidThread());
   if (!networks.size()) {
-    DVLOG(1) << "No network interfaces found!";
+    DVLOG(1) << "No valid network interfaces found!";
+    FinishDiscovery();
     return;
   }
 
@@ -392,8 +411,9 @@ void DialServiceImpl::SendNetworkList(const NetworkInterfaceList& networks) {
 
   if (interface == NULL) {
     DVLOG(1) << "Could not find a valid interface to bind.";
+    FinishDiscovery();
   } else {
-    BindAndWriteSocket(*interface);
+    BindSocketAndSendRequest(interface->address);
   }
 }
 
@@ -402,12 +422,24 @@ void DialServiceImpl::DoGetNetworkList() {
   bool success = net::GetNetworkList(&list);
   if (!success) {
     DVLOG(1) << "Could not retrieve network list!";
-    return;
+    list.clear();

[justinlin, 2013/02/07 09:03:24]

This isn't needed as the vector is already initialized to empty, also please
sync to head and rebase as some of this has changed, I think.

[mark a. foltz, 2013/02/07 22:36:55]

Done.  Will rebase.

   }
   BrowserThread::PostTask(BrowserThread::IO, FROM_HERE, base::Bind(
       &DialServiceImpl::SendNetworkList, this, list));
 }
 
+void DialServiceImpl::FinishDiscovery() {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  DCHECK(discovery_active_);
+  DVLOG(1) << "Discovery finished.";
+  CloseSocket();
+  finish_timer_.Stop();
+  request_timer_.Stop();
+  discovery_active_ = false;
+  num_requests_sent_ = 0;
+  FOR_EACH_OBSERVER(Observer, observer_list_, OnDiscoveryFinished(this));
+}
+
 void DialServiceImpl::CloseSocket() {
   DCHECK(thread_checker_.CalledOnValidThread());
   is_reading_ = false;