Bluetooth extension api fixes for WebUI

extensions/browser/api/bluetooth/bluetooth_event_router.cc

 // Copyright (c) 2012 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 "extensions/browser/api/bluetooth/bluetooth_event_router.h"
 
 #include <map>
 #include <string>
 
 #include "base/bind.h"
@@ skipping 19 matching lines @@
 #include "extensions/common/api/bluetooth.h"
 #include "extensions/common/api/bluetooth_private.h"
 
 namespace extensions {
 
 namespace bluetooth = api::bluetooth;
 namespace bt_private = api::bluetooth_private;
 
 BluetoothEventRouter::BluetoothEventRouter(content::BrowserContext* context)
     : browser_context_(context),
-      adapter_(NULL),
+      adapter_(nullptr),
       num_event_listeners_(0),
       extension_registry_observer_(this),
       weak_ptr_factory_(this) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
   DCHECK(browser_context_);
   registrar_.Add(this,
                  extensions::NOTIFICATION_EXTENSION_HOST_DESTROYED,
                  content::Source<content::BrowserContext>(browser_context_));
   extension_registry_observer_.Add(ExtensionRegistry::Get(browser_context_));
 }
 
 BluetoothEventRouter::~BluetoothEventRouter() {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
   if (adapter_.get()) {
     adapter_->RemoveObserver(this);
-    adapter_ = NULL;
+    adapter_ = nullptr;
   }
   CleanUpAllExtensions();
 }
 
 bool BluetoothEventRouter::IsBluetoothSupported() const {
   return adapter_.get() ||
          device::BluetoothAdapterFactory::IsBluetoothAdapterAvailable();
 }
 
 void BluetoothEventRouter::GetAdapter(
     const device::BluetoothAdapterFactory::AdapterCallback& callback) {
   if (adapter_.get()) {
     callback.Run(scoped_refptr<device::BluetoothAdapter>(adapter_));
     return;
   }
 
   device::BluetoothAdapterFactory::GetAdapter(callback);
 }
 
 void BluetoothEventRouter::StartDiscoverySession(
     device::BluetoothAdapter* adapter,
     const std::string& extension_id,
     const base::Closure& callback,
     const base::Closure& error_callback) {
-  if (adapter != adapter_.get()) {

[armansito, 2015/10/21 23:19:19]

Since you removed this check, then maybe add a DHECK(adapter == adapter_.get())
here?

[stevenjb, 2015/10/21 23:36:40]

Actually, the test is effectively moved to StartDiscoverySessionImpl below.

+  if (!adapter_.get() && IsBluetoothSupported()) {
+    GetAdapter(base::Bind(
+        &BluetoothEventRouter::OnAdapterInitialized,
+        weak_ptr_factory_.GetWeakPtr(),
+        base::Bind(&BluetoothEventRouter::StartDiscoverySessionImpl,
+                   weak_ptr_factory_.GetWeakPtr(), make_scoped_refptr(adapter),
+                   extension_id, callback, error_callback)));
+    return;
+  }
+  StartDiscoverySessionImpl(adapter, extension_id, callback, error_callback);
+}
+
+void BluetoothEventRouter::StartDiscoverySessionImpl(
+    device::BluetoothAdapter* adapter,
+    const std::string& extension_id,
+    const base::Closure& callback,
+    const base::Closure& error_callback) {
+  if (!adapter_.get() || adapter != adapter_.get()) {

[armansito, 2015/10/21 23:19:19]

You removed the "adapter != adapter_.get()" above but kept it here. Let's be
consistent.

[stevenjb, 2015/10/21 23:36:40]

This is called only from StartDiscoverySession() (thus
StartDiscoverySessionImpl()). I added a clarifying comment.

+    if (adapter_.get()) {
+      LOG(ERROR) << "Unable to get addapter";

[armansito, 2015/10/21 23:19:19]

s/addapter/adapter/

[stevenjb, 2015/10/21 23:36:40]

Done.

+    } else {
+      LOG(ERROR) << "Adapter mismatch";

[armansito, 2015/10/21 23:19:19]

Also this logic is extremely convoluted for a mostly uninformative log message.
Just simplify it all entirely like this:

if (!adapter_.get()) {
  LOG(ERROR) << "|adapter_| was not initialized".
  error_callback.Run();
  return;
}

if (adapter != adapter_.get()) {
  LOG(ERROR) << "Adapters don't match";
  error_callback.Run();
  return;
}

Again for the second part, I'm not sure if it's better to just have
DCHECK(adapter == adapter_.get());

[stevenjb, 2015/10/21 23:36:40]

Logging clarified.

+    }
     error_callback.Run();
     return;
   }
   DiscoverySessionMap::iterator iter =
       discovery_session_map_.find(extension_id);
   if (iter != discovery_session_map_.end() && iter->second->IsActive()) {
     DVLOG(1) << "An active discovery session exists for extension.";
     error_callback.Run();
     return;
   }
@@ skipping 62 matching lines @@
 
   // extension is already running discovery, update it's discovery filter
   iter->second->SetDiscoveryFilter(discovery_filter.Pass(), callback,
                                    error_callback);
 }
 
 BluetoothApiPairingDelegate* BluetoothEventRouter::GetPairingDelegate(
     const std::string& extension_id) {
   return ContainsKey(pairing_delegate_map_, extension_id)
              ? pairing_delegate_map_[extension_id]
-             : NULL;
+             : nullptr;
 }
 
 void BluetoothEventRouter::OnAdapterInitialized(
     const base::Closure& callback,
     scoped_refptr<device::BluetoothAdapter> adapter) {
   if (!adapter_.get()) {
     adapter_ = adapter;
     adapter_->AddObserver(this);
   }
 
   callback.Run();
 }
 
 void BluetoothEventRouter::MaybeReleaseAdapter() {
   if (adapter_.get() && num_event_listeners_ == 0 &&
       pairing_delegate_map_.empty()) {
+    VLOG(1) << "Releasing Adapter.";
     adapter_->RemoveObserver(this);
-    adapter_ = NULL;
+    adapter_ = nullptr;
   }
 }
 
 void BluetoothEventRouter::AddPairingDelegate(const std::string& extension_id) {
-  if (!adapter_.get()) {
-    base::Closure self_callback =
-        base::Bind(&BluetoothEventRouter::AddPairingDelegate,
+  if (!adapter_.get() && IsBluetoothSupported()) {
+    GetAdapter(
+        base::Bind(&BluetoothEventRouter::OnAdapterInitialized,
                    weak_ptr_factory_.GetWeakPtr(),
-                   extension_id);
-    GetAdapter(base::Bind(&BluetoothEventRouter::OnAdapterInitialized,
-                          weak_ptr_factory_.GetWeakPtr(),
-                          self_callback));
+                   base::Bind(&BluetoothEventRouter::AddPairingDelegateImpl,
+                              weak_ptr_factory_.GetWeakPtr(), extension_id)));
     return;
   }
+  AddPairingDelegateImpl(extension_id);
+}
 
+void BluetoothEventRouter::AddPairingDelegateImpl(
+    const std::string& extension_id) {
+  if (!adapter_.get()) {
+    LOG(ERROR) << "Unable to get adatper.";
+    return;
+  }
   if (!ContainsKey(pairing_delegate_map_, extension_id)) {
     BluetoothApiPairingDelegate* delegate =
         new BluetoothApiPairingDelegate(extension_id, browser_context_);
     DCHECK(adapter_.get());
     adapter_->AddPairingDelegate(
         delegate, device::BluetoothAdapter::PAIRING_DELEGATE_PRIORITY_HIGH);
     pairing_delegate_map_[extension_id] = delegate;
   } else {
     LOG(ERROR) << "Pairing delegate already exists for extension. "
                << "There should be at most one onPairing listener.";
@@ skipping 51 matching lines @@
          iter != discovery_session_map_.end();
          ++iter) {
       device::BluetoothDiscoverySession* session = iter->second;
       if (session->IsActive()) {
         active_session_map[iter->first] = session;
         continue;
       }
       delete session;
     }
     discovery_session_map_.swap(active_session_map);
-    MaybeReleaseAdapter();
   }
 
   DispatchAdapterStateEvent();
+
+  // Release the adapter after dispatching the event.
+  if (!discovering)
+    MaybeReleaseAdapter();
 }
 
 void BluetoothEventRouter::DeviceAdded(device::BluetoothAdapter* adapter,
                                        device::BluetoothDevice* device) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
   if (adapter != adapter_.get()) {
     DVLOG(1) << "Ignoring event for adapter " << adapter->GetAddress();
     return;
   }
 
@@ skipping 37 matching lines @@
 
 void BluetoothEventRouter::OnListenerRemoved() {
   if (num_event_listeners_ > 0)
     num_event_listeners_--;
   MaybeReleaseAdapter();
 }
 
 void BluetoothEventRouter::DispatchAdapterStateEvent() {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
   api::bluetooth::AdapterState state;
+  CHECK(adapter_.get());
   PopulateAdapterState(*adapter_.get(), &state);
 
   scoped_ptr<base::ListValue> args =
       bluetooth::OnAdapterStateChanged::Create(state);
   scoped_ptr<Event> event(
       new Event(events::BLUETOOTH_ON_ADAPTER_STATE_CHANGED,
                 bluetooth::OnAdapterStateChanged::kEventName, args.Pass()));
   EventRouter::Get(browser_context_)->BroadcastEvent(event.Pass());
 }
 
 void BluetoothEventRouter::DispatchDeviceEvent(
     events::HistogramValue histogram_value,
     const std::string& event_name,
     device::BluetoothDevice* device) {
   bluetooth::Device extension_device;
+  CHECK(device);
   bluetooth::BluetoothDeviceToApiDevice(*device, &extension_device);
 
   scoped_ptr<base::ListValue> args =
       bluetooth::OnDeviceAdded::Create(extension_device);
   scoped_ptr<Event> event(new Event(histogram_value, event_name, args.Pass()));
   EventRouter::Get(browser_context_)->BroadcastEvent(event.Pass());
 }
 
 void BluetoothEventRouter::CleanUpForExtension(
     const std::string& extension_id) {
@@ skipping 62 matching lines @@
 }
 
 void BluetoothEventRouter::OnExtensionUnloaded(
     content::BrowserContext* browser_context,
     const Extension* extension,
     UnloadedExtensionInfo::Reason reason) {
   CleanUpForExtension(extension->id());
 }
 
 }  // namespace extensions