Don't consider a Bluetooth adapter present until it has an address.

Since we don't have an observer method that an address has changed,
and one doesn't really make sense, change the previous behavior on
change of a default adapter to inform observers that an adapter was
removed and a new one added rather than pretend nothing happened.

This also makes it consistent with the (more likely) case where the
new adapter address is not yet known, and the observer has to be
informed of the old adapter removal anyway.

BUG=chromium-os:34283
TEST=unit_tests

Review URL: https://chromiumcodereview.appspot.com/10905190

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@155908 0039d316-1c4b-4281-b951-d872f2087c98

3 files changed, 231 insertions, 21 deletions

diff --git a/chrome/browser/chromeos/bluetooth/bluetooth_adapter.cc b/chrome/browser/chromeos/bluetooth/bluetooth_adapter.cc
index bd0cada..62b8ecb 100644
--- a/chrome/browser/chromeos/bluetooth/bluetooth_adapter.cc
+++ b/chrome/browser/chromeos/bluetooth/bluetooth_adapter.cc
@@ -64,7 +64,7 @@ void BluetoothAdapter::RemoveObserver(Observer* observer) {
 }
 
 bool BluetoothAdapter::IsPresent() const {
-  return !object_path_.value().empty();
+  return !object_path_.value().empty() && !address_.empty();
 }
 
 bool BluetoothAdapter::IsPowered() const {
@@ -191,21 +191,15 @@ void BluetoothAdapter::AdapterRemoved(const dbus::ObjectPath& adapter_path) {
 }
 
 void BluetoothAdapter::ChangeAdapter(const dbus::ObjectPath& adapter_path) {
-  if (adapter_path == object_path_)
-    return;
-
-  // Determine whether this is a change of adapter or gaining an adapter,
-  // remember for later so we can send the right notification.
-  const bool new_adapter = object_path_.value().empty();
-  if (new_adapter) {
+  if (object_path_.value().empty()) {
     DVLOG(1) << "Adapter path initialized to " << adapter_path.value();
-  } else {
+  } else if (object_path_.value() != adapter_path.value()) {
     DVLOG(1) << "Adapter path changed from " << object_path_.value()
              << " to " << adapter_path.value();
 
-    // Invalidate the devices list, since the property update does not
-    // remove them.
-    ClearDevices();
+    RemoveAdapter();
+  } else {
+    DVLOG(1) << "Adapter address updated";
   }
 
   object_path_ = adapter_path;
@@ -218,18 +212,23 @@ void BluetoothAdapter::ChangeAdapter(const dbus::ObjectPath& adapter_path) {
   address_ = properties->address.value();
   name_ = properties->name.value();
 
+  // Delay announcing a new adapter until we have an address.
+  if (address_.empty()) {
+    DVLOG(1) << "Adapter address not yet known";
+    return;
+  }
+
   PoweredChanged(properties->powered.value());
   DiscoveringChanged(properties->discovering.value());
   DevicesChanged(properties->devices.value());
 
-  // Notify observers if we did not have an adapter before, the case of
-  // moving from one to another is hidden from layers above.
-  if (new_adapter)
-    FOR_EACH_OBSERVER(BluetoothAdapter::Observer, observers_,
-                      AdapterPresentChanged(this, true));
+  FOR_EACH_OBSERVER(BluetoothAdapter::Observer, observers_,
+                    AdapterPresentChanged(this, true));
 }
 
 void BluetoothAdapter::RemoveAdapter() {
+  const bool adapter_was_present = IsPresent();
+
   DVLOG(1) << "Adapter lost.";
   PoweredChanged(false);
   DiscoveringChanged(false);
@@ -239,8 +238,9 @@ void BluetoothAdapter::RemoveAdapter() {
   address_.clear();
   name_.clear();
 
-  FOR_EACH_OBSERVER(BluetoothAdapter::Observer, observers_,
-                    AdapterPresentChanged(this, false));
+  if (adapter_was_present)
+    FOR_EACH_OBSERVER(BluetoothAdapter::Observer, observers_,
+                      AdapterPresentChanged(this, false));
 }
 
 void BluetoothAdapter::OnSetPowered(const base::Closure& callback,
@@ -335,7 +335,7 @@ void BluetoothAdapter::AdapterPropertyChanged(
     DevicesChanged(properties->devices.value());
 
   } else if (property_name == properties->address.name()) {
-    address_ = properties->address.value();
+    ChangeAdapter(object_path_);
 
   } else if (property_name == properties->name.name()) {
     name_ = properties->name.value();
diff --git a/chrome/browser/chromeos/bluetooth/bluetooth_adapter.h b/chrome/browser/chromeos/bluetooth/bluetooth_adapter.h
index 4ee79d1..673aac5 100644
--- a/chrome/browser/chromeos/bluetooth/bluetooth_adapter.h
+++ b/chrome/browser/chromeos/bluetooth/bluetooth_adapter.h
@@ -104,7 +104,8 @@ class BluetoothAdapter : public base::RefCounted<BluetoothAdapter>,
   const std::string& name() const { return name_; }
 
   // Indicates whether the adapter is actually present on the system, for
-  // the default adapter this indicates whether any adapter is present.
+  // the default adapter this indicates whether any adapter is present. An
+  // adapter is only considered present if the address has been obtained.
   virtual bool IsPresent() const;
 
   // Indicates whether the adapter radio is powered.
diff --git a/chrome/browser/chromeos/bluetooth/bluetooth_adapter_unittest.cc b/chrome/browser/chromeos/bluetooth/bluetooth_adapter_unittest.cc
index 2a3d520..8b3f1b4 100644
--- a/chrome/browser/chromeos/bluetooth/bluetooth_adapter_unittest.cc
+++ b/chrome/browser/chromeos/bluetooth/bluetooth_adapter_unittest.cc
@@ -103,6 +103,56 @@ TEST_F(BluetoothAdapterTest, DefaultAdapterWithAddress) {
   EXPECT_EQ(adapter_address, adapter->address());
 }
 
+TEST_F(BluetoothAdapterTest, DefaultAdapterWithoutAddress) {
+  const dbus::ObjectPath adapter_path("/fake/hci0");
+  const std::string adapter_address = "CA:FE:4A:C0:FE:FE";
+
+  // Create the default adapter instance;
+  // BluetoothManagerClient::DefaultAdapter will be called once, passing
+  // a callback to obtain the adapter path.
+  BluetoothManagerClient::AdapterCallback adapter_callback;
+  EXPECT_CALL(*mock_manager_client_, DefaultAdapter(_))
+      .WillOnce(SaveArg<0>(&adapter_callback));
+
+  scoped_refptr<BluetoothAdapter> adapter = BluetoothAdapter::DefaultAdapter();
+
+  // Call the adapter callback;
+  // BluetoothAdapterClient::GetProperties will be called once to obtain
+  // the property set.
+  MockBluetoothAdapterClient::Properties adapter_properties;
+
+  EXPECT_CALL(*mock_adapter_client_, GetProperties(adapter_path))
+      .WillRepeatedly(Return(&adapter_properties));
+
+  // BluetoothAdapter::Observer::AdapterPresentChanged must not be called
+  // yet.
+  MockBluetoothAdapter::Observer adapter_observer;
+  adapter->AddObserver(&adapter_observer);
+
+  EXPECT_CALL(adapter_observer, AdapterPresentChanged(adapter.get(), _))
+      .Times(0);
+
+  adapter_callback.Run(adapter_path, true);
+
+  // Adapter should not be present yet.
+  EXPECT_FALSE(adapter->IsPresent());
+
+  // Tell the adapter the address now;
+  // BluetoothAdapter::Observer::AdapterPresentChanged now must be called.
+  adapter_properties.address.ReplaceValue(adapter_address);
+
+  EXPECT_CALL(adapter_observer, AdapterPresentChanged(adapter.get(), true))
+      .Times(1);
+
+  static_cast<BluetoothAdapterClient::Observer*>(adapter.get())
+      ->AdapterPropertyChanged(adapter_path,
+                               adapter_properties.address.name());
+
+  // Adapter should be present with the given address.
+  EXPECT_TRUE(adapter->IsPresent());
+  EXPECT_EQ(adapter_address, adapter->address());
+}
+
 TEST_F(BluetoothAdapterTest, DefaultAdapterBecomesPresentWithAddress) {
   const dbus::ObjectPath adapter_path("/fake/hci0");
   const std::string adapter_address = "CA:FE:4A:C0:FE:FE";
@@ -178,6 +228,49 @@ TEST_F(BluetoothAdapterTest, DefaultAdapterReplacedWithAddress) {
   EXPECT_CALL(*mock_adapter_client_, GetProperties(new_adapter_path))
       .WillRepeatedly(Return(&new_adapter_properties));
 
+  // BluetoothAdapter::Observer::AdapterPresentChanged must be called once
+  // with false to indicate the old adapter being removed and once with true
+  // to announce the new adapter.
+  MockBluetoothAdapter::Observer adapter_observer;
+  adapter->AddObserver(&adapter_observer);
+
+  EXPECT_CALL(adapter_observer, AdapterPresentChanged(adapter.get(), false))
+      .Times(1);
+  EXPECT_CALL(adapter_observer, AdapterPresentChanged(adapter.get(), true))
+      .Times(1);
+
+  static_cast<BluetoothManagerClient::Observer*>(adapter.get())
+      ->DefaultAdapterChanged(new_adapter_path);
+
+  // Adapter should be present with the new address.
+  EXPECT_TRUE(adapter->IsPresent());
+  EXPECT_EQ(new_adapter_address, adapter->address());
+}
+
+TEST_F(BluetoothAdapterTest, DefaultAdapterBecomesPresentWithoutAddress) {
+  const dbus::ObjectPath adapter_path("/fake/hci0");
+  const std::string adapter_address = "CA:FE:4A:C0:FE:FE";
+
+  // Create the default adapter instance;
+  // BluetoothManagerClient::DefaultAdapter will be called once, passing
+  // a callback to obtain the adapter path.
+  BluetoothManagerClient::AdapterCallback adapter_callback;
+  EXPECT_CALL(*mock_manager_client_, DefaultAdapter(_))
+      .WillOnce(SaveArg<0>(&adapter_callback));
+
+  scoped_refptr<BluetoothAdapter> adapter = BluetoothAdapter::DefaultAdapter();
+
+  // Call the adapter callback; make out it failed.
+  adapter_callback.Run(dbus::ObjectPath(""), false);
+
+  // Tell the adapter the default adapter changed;
+  // BluetoothAdapterClient::GetProperties will be called once to obtain
+  // the property set.
+  MockBluetoothAdapterClient::Properties adapter_properties;
+
+  EXPECT_CALL(*mock_adapter_client_, GetProperties(adapter_path))
+      .WillRepeatedly(Return(&adapter_properties));
+
   // BluetoothAdapter::Observer::AdapterPresentChanged must not be called.
   MockBluetoothAdapter::Observer adapter_observer;
   adapter->AddObserver(&adapter_observer);
@@ -186,8 +279,86 @@ TEST_F(BluetoothAdapterTest, DefaultAdapterReplacedWithAddress) {
       .Times(0);
 
   static_cast<BluetoothManagerClient::Observer*>(adapter.get())
+      ->DefaultAdapterChanged(adapter_path);
+
+  // Adapter should not be present yet.
+  EXPECT_FALSE(adapter->IsPresent());
+
+  // Tell the adapter the address now;
+  // BluetoothAdapter::Observer::AdapterPresentChanged now must be called.
+  adapter_properties.address.ReplaceValue(adapter_address);
+
+  EXPECT_CALL(adapter_observer, AdapterPresentChanged(adapter.get(), true))
+      .Times(1);
+
+  static_cast<BluetoothAdapterClient::Observer*>(adapter.get())
+      ->AdapterPropertyChanged(adapter_path,
+                               adapter_properties.address.name());
+
+  // Adapter should be present with the new address.
+  EXPECT_TRUE(adapter->IsPresent());
+  EXPECT_EQ(adapter_address, adapter->address());
+}
+
+TEST_F(BluetoothAdapterTest, DefaultAdapterReplacedWithoutAddress) {
+  const dbus::ObjectPath initial_adapter_path("/fake/hci0");
+  const dbus::ObjectPath new_adapter_path("/fake/hci1");
+  const std::string initial_adapter_address = "CA:FE:4A:C0:FE:FE";
+  const std::string new_adapter_address = "BA:C0:11:CO:FE:FE";
+
+  // Create the default adapter instance;
+  // BluetoothManagerClient::DefaultAdapter will be called once, passing
+  // a callback to obtain the adapter path.
+  BluetoothManagerClient::AdapterCallback adapter_callback;
+  EXPECT_CALL(*mock_manager_client_, DefaultAdapter(_))
+      .WillOnce(SaveArg<0>(&adapter_callback));
+
+  scoped_refptr<BluetoothAdapter> adapter = BluetoothAdapter::DefaultAdapter();
+
+  // Call the adapter callback;
+  // BluetoothAdapterClient::GetProperties will be called once to obtain
+  // the property set.
+  MockBluetoothAdapterClient::Properties initial_adapter_properties;
+  initial_adapter_properties.address.ReplaceValue(initial_adapter_address);
+
+  EXPECT_CALL(*mock_adapter_client_, GetProperties(initial_adapter_path))
+      .WillRepeatedly(Return(&initial_adapter_properties));
+
+  adapter_callback.Run(initial_adapter_path, true);
+
+  // Tell the adapter the default adapter changed;
+  // BluetoothAdapterClient::GetProperties will be called once to obtain
+  // the property set.
+  MockBluetoothAdapterClient::Properties new_adapter_properties;
+
+  EXPECT_CALL(*mock_adapter_client_, GetProperties(new_adapter_path))
+      .WillRepeatedly(Return(&new_adapter_properties));
+
+  // BluetoothAdapter::Observer::AdapterPresentChanged must be called to
+  // indicate the adapter has gone away.
+  MockBluetoothAdapter::Observer adapter_observer;
+  adapter->AddObserver(&adapter_observer);
+
+  EXPECT_CALL(adapter_observer, AdapterPresentChanged(adapter.get(), false))
+      .Times(1);
+
+  static_cast<BluetoothManagerClient::Observer*>(adapter.get())
       ->DefaultAdapterChanged(new_adapter_path);
 
+  // Adapter should be now marked not present.
+  EXPECT_FALSE(adapter->IsPresent());
+
+  // Tell the adapter the address now;
+  // BluetoothAdapter::Observer::AdapterPresentChanged now must be called.
+  new_adapter_properties.address.ReplaceValue(new_adapter_address);
+
+  EXPECT_CALL(adapter_observer, AdapterPresentChanged(adapter.get(), true))
+      .Times(1);
+
+  static_cast<BluetoothAdapterClient::Observer*>(adapter.get())
+      ->AdapterPropertyChanged(new_adapter_path,
+                               new_adapter_properties.address.name());
+
   // Adapter should be present with the new address.
   EXPECT_TRUE(adapter->IsPresent());
   EXPECT_EQ(new_adapter_address, adapter->address());
@@ -233,4 +404,42 @@ TEST_F(BluetoothAdapterTest, DefaultAdapterRemoved) {
   EXPECT_FALSE(adapter->IsPresent());
 }
 
+TEST_F(BluetoothAdapterTest, DefaultAdapterWithoutAddressRemoved) {
+  const dbus::ObjectPath adapter_path("/fake/hci0");
+
+  // Create the default adapter instance;
+  // BluetoothManagerClient::DefaultAdapter will be called once, passing
+  // a callback to obtain the adapter path.
+  BluetoothManagerClient::AdapterCallback adapter_callback;
+  EXPECT_CALL(*mock_manager_client_, DefaultAdapter(_))
+      .WillOnce(SaveArg<0>(&adapter_callback));
+
+  scoped_refptr<BluetoothAdapter> adapter = BluetoothAdapter::DefaultAdapter();
+
+  // Call the adapter callback;
+  // BluetoothAdapterClient::GetProperties will be called once to obtain
+  // the property set.
+  MockBluetoothAdapterClient::Properties adapter_properties;
+
+  EXPECT_CALL(*mock_adapter_client_, GetProperties(adapter_path))
+      .WillRepeatedly(Return(&adapter_properties));
+
+  adapter_callback.Run(adapter_path, true);
+
+  // Report that the adapter has been removed;
+  // BluetoothAdapter::Observer::AdapterPresentChanged must not be called
+  // since we never should have announced it in the first place.
+  MockBluetoothAdapter::Observer adapter_observer;
+  adapter->AddObserver(&adapter_observer);
+
+  EXPECT_CALL(adapter_observer, AdapterPresentChanged(adapter.get(), _))
+      .Times(0);
+
+  static_cast<BluetoothManagerClient::Observer*>(adapter.get())
+      ->AdapterRemoved(adapter_path);
+
+  // Adapter should be still no longer present.
+  EXPECT_FALSE(adapter->IsPresent());
+}
+
 }  // namespace chromeos