Stop sending events through the DIAL API when the device list is unchanged.

Currently the chrome.dial API fires one device list event per discovery cycle, even if there are no devices or the device list has not changed from the previous cycle.

This changes the behavior to only fires events if the device list has actually changed or a new event handler has been added, to prevent waking up the MR event page when unnecessary.

It also cleans up the unit test to use InSequence properly and remove unnecessary calls to Times(1).

Finally update OWNERS of c/b/extensions/api/dial to reflect current maintainers.

BUG=537385

Review URL: https://codereview.chromium.org/1561863002

Cr-Commit-Position: refs/heads/master@{#369068}

5 files changed, 77 insertions, 98 deletions

diff --git a/chrome/browser/extensions/api/dial/OWNERS b/chrome/browser/extensions/api/dial/OWNERS
index cb84d70..68dd516 100644
--- a/chrome/browser/extensions/api/dial/OWNERS
+++ b/chrome/browser/extensions/api/dial/OWNERS
@@ -1,2 +1,3 @@
-justinlin@chromium.org
 mfoltz@chromium.org
+imcheng@chromium.org
+
diff --git a/chrome/browser/extensions/api/dial/dial_registry.cc b/chrome/browser/extensions/api/dial/dial_registry.cc
index 800a7e8..ef3c21d 100644
--- a/chrome/browser/extensions/api/dial/dial_registry.cc
+++ b/chrome/browser/extensions/api/dial/dial_registry.cc
@@ -27,9 +27,7 @@ DialRegistry::DialRegistry(Observer* dial_api,
                            const base::TimeDelta& expiration,
                            const size_t max_devices)
   : num_listeners_(0),
-    discovery_generation_(0),
     registry_generation_(0),
-    last_event_discovery_generation_(0),
     last_event_registry_generation_(0),
     label_count_(0),
     refresh_interval_delta_(refresh_interval),
@@ -63,6 +61,9 @@ void DialRegistry::OnListenerAdded() {
   if (++num_listeners_ == 1) {
     VLOG(2) << "Listener added; starting periodic discovery.";
     StartPeriodicDiscovery();
+  } else {
+    // Event the new listener with the current device list.
+    SendEvent();
   }
 }
 
@@ -104,7 +105,6 @@ bool DialRegistry::DiscoverNow() {
 
   if (!dial_->HasObserver(this))
     NOTREACHED() << "DiscoverNow() called without observing dial_";
-  discovery_generation_++;
   return dial_->Discover();
 }
 
@@ -125,8 +125,7 @@ void DialRegistry::StartPeriodicDiscovery() {
 void DialRegistry::DoDiscovery() {
   DCHECK(thread_checker_.CalledOnValidThread());
   DCHECK(dial_.get());
-  discovery_generation_++;
-  VLOG(2) << "About to discover! Generation = " << discovery_generation_;
+  VLOG(2) << "About to discover!";
   dial_->Discover();
 }
 
@@ -186,22 +185,15 @@ void DialRegistry::Clear() {
 }
 
 void DialRegistry::MaybeSendEvent() {
-  DCHECK(thread_checker_.CalledOnValidThread());
-
-  // We need to send an event if:
-  // (1) We haven't sent one yet in this round of discovery, or
-  // (2) The device list changed since the last MaybeSendEvent.
-  bool needs_event =
-      (last_event_discovery_generation_ < discovery_generation_ ||
-       last_event_registry_generation_ < registry_generation_);
-  VLOG(2) << "ledg = " << last_event_discovery_generation_ << ", dg = "
-          << discovery_generation_
-          << ", lerg = " << last_event_registry_generation_ << ", rg = "
-          << registry_generation_
-          << ", needs_event = " << needs_event;
-  if (!needs_event)
-    return;
+  // Send an event if the device list has changed since the last event.
+  bool needs_event = last_event_registry_generation_ < registry_generation_;
+  VLOG(2) << "lerg = " << last_event_registry_generation_ << ", rg = "
+          << registry_generation_ << ", needs_event = " << needs_event;
+  if (needs_event)
+    SendEvent();
+}
 
+void DialRegistry::SendEvent() {
   DeviceList device_list;
   for (DeviceByLabelMap::const_iterator i = device_by_label_map_.begin();
        i != device_by_label_map_.end(); i++) {
@@ -209,8 +201,7 @@ void DialRegistry::MaybeSendEvent() {
   }
   dial_api_->OnDialDeviceEvent(device_list);
 
-  // Reset watermarks.
-  last_event_discovery_generation_ = discovery_generation_;
+  // Reset watermark.
   last_event_registry_generation_ = registry_generation_;
 }
 
diff --git a/chrome/browser/extensions/api/dial/dial_registry.h b/chrome/browser/extensions/api/dial/dial_registry.h
index f5040228..c9ddf0b 100644
--- a/chrome/browser/extensions/api/dial/dial_registry.h
+++ b/chrome/browser/extensions/api/dial/dial_registry.h
@@ -129,25 +129,21 @@ class DialRegistry : public DialService::Observer,
   // active set.
   bool IsDeviceExpired(const DialDeviceData& device) const;
 
-  // Notify clients with the current device list if necessary.
+  // Notify listeners with the current device list if the list has changed.
   void MaybeSendEvent();
 
+  // Notify listeners with the current device list.
+  void SendEvent();
+
   // Returns the next label to use for a newly-seen device.
   std::string NextLabel();
 
   // The current number of event listeners attached to this registry.
   int num_listeners_;
 
-  // Incremented each time we DoDiscovery().
-  int discovery_generation_;
-
   // Incremented each time we modify the registry of active devices.
   int registry_generation_;
 
-  // The discovery generation associated with the last time we sent an event.
-  // Used to ensure that we generate at least one event per round of discovery.
-  int last_event_discovery_generation_;
-
   // The registry generation associated with the last time we sent an event.
   // Used to suppress events with duplicate device lists.
   int last_event_registry_generation_;
@@ -180,6 +176,8 @@ class DialRegistry : public DialService::Observer,
   FRIEND_TEST_ALL_PREFIXES(DialRegistryTest, TestAddRemoveListeners);
   FRIEND_TEST_ALL_PREFIXES(DialRegistryTest, TestNoDevicesDiscovered);
   FRIEND_TEST_ALL_PREFIXES(DialRegistryTest, TestDevicesDiscovered);
+  FRIEND_TEST_ALL_PREFIXES(DialRegistryTest,
+                           TestDevicesDiscoveredWithTwoListeners);
   FRIEND_TEST_ALL_PREFIXES(DialRegistryTest, TestDeviceExpires);
   FRIEND_TEST_ALL_PREFIXES(DialRegistryTest, TestExpiredDeviceIsRediscovered);
   FRIEND_TEST_ALL_PREFIXES(DialRegistryTest,
diff --git a/chrome/browser/extensions/api/dial/dial_registry_unittest.cc b/chrome/browser/extensions/api/dial/dial_registry_unittest.cc
index e6fd99a..66f526b 100644
--- a/chrome/browser/extensions/api/dial/dial_registry_unittest.cc
+++ b/chrome/browser/extensions/api/dial/dial_registry_unittest.cc
@@ -110,18 +110,15 @@ class DialRegistryTest : public testing::Test {
 
   void SetListenerExpectations() {
     EXPECT_CALL(registry_->mock_service(),
-                AddObserver(A<DialService::Observer*>()))
-      .Times(1);
+                AddObserver(A<DialService::Observer*>()));
     EXPECT_CALL(registry_->mock_service(),
-                RemoveObserver(A<DialService::Observer*>()))
-      .Times(1);
+                RemoveObserver(A<DialService::Observer*>()));
   }
 };
 
 TEST_F(DialRegistryTest, TestAddRemoveListeners) {
   SetListenerExpectations();
-  EXPECT_CALL(registry_->mock_service(), Discover())
-    .Times(1);
+  EXPECT_CALL(registry_->mock_service(), Discover());
 
   EXPECT_FALSE(registry_->repeating_timer_.IsRunning());
   registry_->OnListenerAdded();
@@ -136,16 +133,13 @@ TEST_F(DialRegistryTest, TestAddRemoveListeners) {
 
 TEST_F(DialRegistryTest, TestNoDevicesDiscovered) {
   SetListenerExpectations();
-  EXPECT_CALL(registry_->mock_service(), Discover())
-    .Times(1);
-  EXPECT_CALL(mock_observer_, OnDialDeviceEvent(empty_list_))
-      .Times(1);
+  EXPECT_CALL(registry_->mock_service(), Discover());
 
   registry_->OnListenerAdded();
   registry_->OnDiscoveryRequest(NULL);
   registry_->OnDiscoveryFinished(NULL);
   registry_->OnListenerRemoved();
-};
+}
 
 TEST_F(DialRegistryTest, TestDevicesDiscovered) {
   DialRegistry::DeviceList expected_list2;
@@ -153,35 +147,59 @@ TEST_F(DialRegistryTest, TestDevicesDiscovered) {
   expected_list2.push_back(second_device_);
 
   SetListenerExpectations();
-  EXPECT_CALL(registry_->mock_service(), Discover())
-      .Times(2);
-  EXPECT_CALL(mock_observer_, OnDialDeviceEvent(empty_list_))
-      .Times(1);
+  InSequence s;
+  EXPECT_CALL(registry_->mock_service(), Discover());
+  EXPECT_CALL(mock_observer_, OnDialDeviceEvent(list_with_first_device_));
+  EXPECT_CALL(registry_->mock_service(), Discover());
+  EXPECT_CALL(mock_observer_, OnDialDeviceEvent(expected_list2));
+
+  registry_->OnListenerAdded();
+  registry_->OnDiscoveryRequest(NULL);
+  registry_->OnDeviceDiscovered(NULL, first_device_);
+  registry_->OnDiscoveryFinished(NULL);
+
+  registry_->DoDiscovery();
+  registry_->OnDiscoveryRequest(NULL);
+  registry_->OnDeviceDiscovered(NULL, second_device_);
+  registry_->OnDiscoveryFinished(NULL);
+  registry_->OnListenerRemoved();
+}
+
+TEST_F(DialRegistryTest, TestDevicesDiscoveredWithTwoListeners) {
+  DialRegistry::DeviceList expected_list2;
+  expected_list2.push_back(first_device_);
+  expected_list2.push_back(second_device_);
+
+  SetListenerExpectations();
+  InSequence s;
+  EXPECT_CALL(registry_->mock_service(), Discover());
   EXPECT_CALL(mock_observer_, OnDialDeviceEvent(list_with_first_device_))
       .Times(2);
-  EXPECT_CALL(mock_observer_, OnDialDeviceEvent(expected_list2))
-      .Times(1);
+  EXPECT_CALL(registry_->mock_service(), Discover());
+  EXPECT_CALL(mock_observer_, OnDialDeviceEvent(expected_list2));
 
   registry_->OnListenerAdded();
   registry_->OnDiscoveryRequest(NULL);
   registry_->OnDeviceDiscovered(NULL, first_device_);
   registry_->OnDiscoveryFinished(NULL);
 
+  registry_->OnListenerAdded();
+
   registry_->DoDiscovery();
   registry_->OnDiscoveryRequest(NULL);
   registry_->OnDeviceDiscovered(NULL, second_device_);
   registry_->OnDiscoveryFinished(NULL);
   registry_->OnListenerRemoved();
+  registry_->OnListenerRemoved();
 }
 
 TEST_F(DialRegistryTest, TestDeviceExpires) {
   SetListenerExpectations();
-  EXPECT_CALL(registry_->mock_service(), Discover())
-      .Times(2);
-  EXPECT_CALL(mock_observer_, OnDialDeviceEvent(empty_list_))
-      .Times(2);
-  EXPECT_CALL(mock_observer_, OnDialDeviceEvent(list_with_first_device_))
-      .Times(2);
+  InSequence s;
+  EXPECT_CALL(registry_->mock_service(), Discover());
+  EXPECT_CALL(mock_observer_, OnDialDeviceEvent(list_with_first_device_));
+  EXPECT_CALL(registry_->mock_service(), Discover());
+  EXPECT_CALL(mock_observer_, OnDialDeviceEvent(empty_list_));
 
   registry_->OnListenerAdded();
   registry_->OnDiscoveryRequest(NULL);
@@ -208,20 +226,12 @@ TEST_F(DialRegistryTest, TestExpiredDeviceIsRediscovered) {
 
   SetListenerExpectations();
 
-  // TODO(mfoltz): Convert other tests to use InSequence to make expectations
-  // more obvious.
   InSequence s;
-
   EXPECT_CALL(registry_->mock_service(), Discover());
-  EXPECT_CALL(mock_observer_, OnDialDeviceEvent(empty_list_));
   EXPECT_CALL(mock_observer_, OnDialDeviceEvent(list_with_first_device_));
-
   EXPECT_CALL(registry_->mock_service(), Discover());
-  EXPECT_CALL(mock_observer_, OnDialDeviceEvent(list_with_first_device_));
   EXPECT_CALL(mock_observer_, OnDialDeviceEvent(empty_list_));
-
   EXPECT_CALL(registry_->mock_service(), Discover());
-  EXPECT_CALL(mock_observer_, OnDialDeviceEvent(empty_list_));
   EXPECT_CALL(mock_observer_, OnDialDeviceEvent(list_with_first_device_));
 
   registry_->time_ = discovery_times[0];
@@ -254,15 +264,9 @@ TEST_F(DialRegistryTest, TestRemovingListenerDoesNotClearList) {
   EXPECT_CALL(registry_->mock_service(),
               RemoveObserver(A<DialService::Observer*>()))
       .Times(2);
-
   EXPECT_CALL(registry_->mock_service(), Discover())
       .Times(2);
-
-  InSequence s;
-  EXPECT_CALL(mock_observer_, OnDialDeviceEvent(empty_list_))
-      .Times(1);
-  EXPECT_CALL(mock_observer_, OnDialDeviceEvent(list_with_first_device_))
-      .Times(2);
+  EXPECT_CALL(mock_observer_, OnDialDeviceEvent(list_with_first_device_));
 
   registry_->OnListenerAdded();
   registry_->OnDiscoveryRequest(NULL);
@@ -279,18 +283,12 @@ TEST_F(DialRegistryTest, TestRemovingListenerDoesNotClearList) {
 TEST_F(DialRegistryTest, TestNetworkEventConnectionLost) {
   SetListenerExpectations();
 
-  EXPECT_CALL(registry_->mock_service(), Discover())
-      .Times(1);
-
   InSequence s;
-  EXPECT_CALL(mock_observer_, OnDialDeviceEvent(empty_list_))
-      .Times(1);
-  EXPECT_CALL(mock_observer_, OnDialDeviceEvent(list_with_first_device_))
-      .Times(1);
-  EXPECT_CALL(mock_observer_, OnDialError(
-      DialRegistry::DIAL_NETWORK_DISCONNECTED)).Times(1);
-  EXPECT_CALL(mock_observer_, OnDialDeviceEvent(empty_list_))
-      .Times(1);
+  EXPECT_CALL(registry_->mock_service(), Discover());
+  EXPECT_CALL(mock_observer_, OnDialDeviceEvent(list_with_first_device_));
+  EXPECT_CALL(mock_observer_,
+              OnDialError(DialRegistry::DIAL_NETWORK_DISCONNECTED));
+  EXPECT_CALL(mock_observer_, OnDialDeviceEvent(empty_list_));
 
   registry_->OnListenerAdded();
   registry_->OnDiscoveryRequest(NULL);
@@ -318,23 +316,17 @@ TEST_F(DialRegistryTest, TestNetworkEventConnectionRestored) {
               RemoveObserver(A<DialService::Observer*>()))
     .Times(2);
 
-  EXPECT_CALL(registry_->mock_service(), Discover())
-    .Times(2);
-
   InSequence s;
-  EXPECT_CALL(mock_observer_, OnDialDeviceEvent(empty_list_))
-      .Times(1);
-  EXPECT_CALL(mock_observer_, OnDialDeviceEvent(list_with_first_device_))
-      .Times(1);
+  EXPECT_CALL(registry_->mock_service(), Discover());
+  EXPECT_CALL(mock_observer_, OnDialDeviceEvent(list_with_first_device_));
+
   EXPECT_CALL(mock_observer_,
-              OnDialError(DialRegistry::DIAL_NETWORK_DISCONNECTED))
-      .Times(1);
-  EXPECT_CALL(mock_observer_, OnDialDeviceEvent(empty_list_))
-      .Times(2);
-  EXPECT_CALL(mock_observer_, OnDialDeviceEvent(list_with_second_device_))
-      .Times(1);
-  EXPECT_CALL(mock_observer_, OnDialDeviceEvent(expected_list3))
-      .Times(1);
+              OnDialError(DialRegistry::DIAL_NETWORK_DISCONNECTED));
+  EXPECT_CALL(mock_observer_, OnDialDeviceEvent(empty_list_));
+
+  EXPECT_CALL(registry_->mock_service(), Discover());
+  EXPECT_CALL(mock_observer_, OnDialDeviceEvent(list_with_second_device_));
+  EXPECT_CALL(mock_observer_, OnDialDeviceEvent(expected_list3));
 
   registry_->OnListenerAdded();
   registry_->OnDiscoveryRequest(NULL);
diff --git a/chrome/browser/extensions/api/dial/dial_service.h b/chrome/browser/extensions/api/dial/dial_service.h
index 90df700..9869f29 100644
--- a/chrome/browser/extensions/api/dial/dial_service.h
+++ b/chrome/browser/extensions/api/dial/dial_service.h
@@ -51,9 +51,6 @@ class DialDeviceData;
 // Calling Discover() again between T1 and Tf has no effect.
 //
 // All relevant constants are defined in dial_service.cc.
-//
-// TODO(mfoltz): Port this into net/.
-// See https://code.google.com/p/chromium/issues/detail?id=164473
 class DialService {
  public:
   enum DialServiceErrorCode {