Bluetooth API: improve discovery

This CL: 
- eliminates unnecessary dispatches
- correctly handles devices that are discovered before interest is
registered

TEST=ran api test
BUG=133179


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

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

chrome/browser/chromeos/bluetooth/bluetooth_adapter.h

@@ -118,7 +118,7 @@ class BluetoothAdapter : public base::RefCounted<BluetoothAdapter>,
   // note that a typical discovery process has phases of this being true
   // followed by phases of being false when the adapter interrogates the
   // devices found.
-  bool IsDiscovering() const;
+  virtual bool IsDiscovering() const;
 
   // Requests that the adapter either begin discovering new devices when
   // |discovering| is true, or cease any discovery when false.  On success,

chrome/browser/chromeos/bluetooth/test/mock_bluetooth_adapter.h

@@ -21,6 +21,7 @@ class MockBluetoothAdapter : public BluetoothAdapter {
 
   MOCK_CONST_METHOD0(IsPresent, bool());
   MOCK_CONST_METHOD0(IsPowered, bool());
+  MOCK_CONST_METHOD0(IsDiscovering, bool());
   MOCK_METHOD3(SetDiscovering,
                void(bool discovering,
                     const base::Closure& callback,

chrome/browser/chromeos/extensions/bluetooth_event_router.cc

@@ -8,6 +8,7 @@
 
 #include "base/json/json_writer.h"
 #include "base/memory/ref_counted.h"
+#include "base/memory/scoped_vector.h"
 #include "base/utf_string_conversions.h"
 #include "chrome/browser/chromeos/bluetooth/bluetooth_adapter.h"
 #include "chrome/browser/chromeos/bluetooth/bluetooth_device.h"
@@ -17,6 +18,8 @@
 #include "chrome/browser/extensions/event_router.h"
 #include "chrome/common/extensions/api/experimental_bluetooth.h"
 
+namespace experimental_bluetooth = extensions::api::experimental_bluetooth;
+
 namespace chromeos {
 
 ExtensionBluetoothEventRouter::ExtensionBluetoothEventRouter(Profile* profile)
@@ -63,7 +66,25 @@ scoped_refptr<BluetoothSocket> ExtensionBluetoothEventRouter::GetSocket(
   return socket_entry->second;
 }
 
+void ExtensionBluetoothEventRouter::SetResponsibleForDiscovery(
+    bool responsible) {
+  responsible_for_discovery_ = responsible;
+}
+
+bool ExtensionBluetoothEventRouter::IsResponsibleForDiscovery() const {
+  return responsible_for_discovery_;
+}
+
 void ExtensionBluetoothEventRouter::SetSendDiscoveryEvents(bool should_send) {
+  // At the transition into sending devices, also send past devices that
+  // were discovered as they will not be discovered again.
+  if (should_send && !send_discovery_events_) {
+    for (DeviceList::const_iterator i = discovered_devices_.begin();
+        i != discovered_devices_.end(); ++i) {
+      DispatchDeviceEvent(**i);
+    }
+  }
+
   send_discovery_events_ = should_send;
 }
 
@@ -98,6 +119,12 @@ void ExtensionBluetoothEventRouter::AdapterDiscoveringChanged(
     return;
   }
 
+  if (!discovering) {
+    send_discovery_events_ = false;
+    responsible_for_discovery_ = false;
+    discovered_devices_.clear();
+  }
+
   DispatchBooleanValueEvent(
       extensions::event_names::kBluetoothOnDiscoveringChanged,
       discovering);
@@ -105,34 +132,39 @@ void ExtensionBluetoothEventRouter::AdapterDiscoveringChanged(
 
 void ExtensionBluetoothEventRouter::DeviceAdded(
     chromeos::BluetoothAdapter* adapter, chromeos::BluetoothDevice* device) {
-  if (!send_discovery_events_)
+  if (adapter != adapter_.get()) {
+    DVLOG(1) << "Ignoring event for adapter " << adapter->address();
     return;
+  }
 
-  DCHECK(adapter == adapter_.get());
-
-  extensions::api::experimental_bluetooth::Device extension_device;
-  extensions::api::experimental_bluetooth::BluetoothDeviceToApiDevice(
-      *device, &extension_device);
+  experimental_bluetooth::Device* extension_device =
+    new experimental_bluetooth::Device();
+  experimental_bluetooth::BluetoothDeviceToApiDevice(*device, extension_device);
+  discovered_devices_.push_back(extension_device);
 
-  scoped_ptr<ListValue> args(new ListValue());
-  args->Append(extension_device.ToValue().release());
+  if (!send_discovery_events_)
+    return;
 
-  profile_->GetExtensionEventRouter()->DispatchEventToRenderers(
-      extensions::event_names::kBluetoothOnDeviceDiscovered,
-      args.Pass(),
-      NULL,
-      GURL());
+  DispatchDeviceEvent(*extension_device);
 }
 
 void ExtensionBluetoothEventRouter::DispatchBooleanValueEvent(
     const char* event_name, bool value) {
   scoped_ptr<ListValue> args(new ListValue());
   args->Append(Value::CreateBooleanValue(value));
-
-  // TODO(bryeung): only dispatch the event to interested renderers
-  // crbug.com/133179
   profile_->GetExtensionEventRouter()->DispatchEventToRenderers(
       event_name, args.Pass(), NULL, GURL());
 }
 
+void ExtensionBluetoothEventRouter::DispatchDeviceEvent(
+    const experimental_bluetooth::Device& device) {
+  scoped_ptr<ListValue> args(new ListValue());
+  args->Append(device.ToValue().release());
+  profile_->GetExtensionEventRouter()->DispatchEventToRenderers(
+      extensions::event_names::kBluetoothOnDeviceDiscovered,
+      args.Pass(),
+      NULL,
+      GURL());
+}
+
 }  // namespace chromeos

chrome/browser/chromeos/extensions/bluetooth_event_router.h

@@ -8,9 +8,11 @@
 #include <map>
 
 #include "base/memory/ref_counted.h"
+#include "base/memory/scoped_vector.h"
 #include "chrome/browser/chromeos/bluetooth/bluetooth_adapter.h"
 #include "chrome/browser/chromeos/bluetooth/bluetooth_socket.h"
 #include "chrome/browser/profiles/profile.h"
+#include "chrome/common/extensions/api/experimental_bluetooth.h"
 
 namespace chromeos {
 
@@ -35,6 +37,11 @@ class ExtensionBluetoothEventRouter
   // Get the BluetoothSocket corresponding to |id|.
   scoped_refptr<BluetoothSocket> GetSocket(int id);
 
+  // Sets whether this Profile is responsible for the discovering state of the
+  // adapter.
+  void SetResponsibleForDiscovery(bool responsible);
+  bool IsResponsibleForDiscovery() const;
+
   // Sets whether or not DeviceAdded events will be dispatched to extensions.
   void SetSendDiscoveryEvents(bool should_send);
 
@@ -54,8 +61,11 @@ class ExtensionBluetoothEventRouter
   }
  private:
   void DispatchBooleanValueEvent(const char* event_name, bool value);
+  void DispatchDeviceEvent(
+      const extensions::api::experimental_bluetooth::Device& device);
 
   bool send_discovery_events_;
+  bool responsible_for_discovery_;
 
   Profile* profile_;
   scoped_refptr<chromeos::BluetoothAdapter> adapter_;
@@ -68,6 +78,10 @@ class ExtensionBluetoothEventRouter
   typedef std::map<int, scoped_refptr<BluetoothSocket> > SocketMap;
   SocketMap socket_map_;
 
+  typedef ScopedVector<extensions::api::experimental_bluetooth::Device>
+      DeviceList;
+  DeviceList discovered_devices_;
+
   DISALLOW_COPY_AND_ASSIGN(ExtensionBluetoothEventRouter);
 };
 

chrome/browser/extensions/api/bluetooth/bluetooth_api.cc

@@ -54,8 +54,7 @@ const char kInvalidDevice[] = "Invalid device";
 const char kInvalidUuid[] = "Invalid UUID";
 const char kServiceDiscoveryFailed[] = "Service discovery failed";
 const char kSocketNotFoundError[] = "Socket not found: invalid socket id";
-const char kStartDiscoveryFailed[] =
-    "Starting discovery failed, or already discovering";
+const char kStartDiscoveryFailed[] = "Starting discovery failed";
 const char kStopDiscoveryFailed[] = "Failed to stop discovery";
 
 }  // namespace
@@ -459,6 +458,7 @@ bool BluetoothGetLocalOutOfBandPairingDataFunction::RunImpl() {
 }
 
 void BluetoothStartDiscoveryFunction::OnSuccessCallback() {
+  GetEventRouter(profile())->SetResponsibleForDiscovery(true);
   SendResponse(true);
 }
 
@@ -470,8 +470,7 @@ void BluetoothStartDiscoveryFunction::OnErrorCallback() {
 bool BluetoothStartDiscoveryFunction::RunImpl() {
   GetEventRouter(profile())->SetSendDiscoveryEvents(true);
 
-  // BluetoothAdapter will throw an error if we SetDiscovering(true) when
-  // discovery is already in progress
+  // If the adapter is already discovering, there is nothing else to do.
   if (GetMutableAdapter(profile())->IsDiscovering()) {
     SendResponse(true);
     return true;
@@ -494,9 +493,11 @@ void BluetoothStopDiscoveryFunction::OnErrorCallback() {
 
 bool BluetoothStopDiscoveryFunction::RunImpl() {
   GetEventRouter(profile())->SetSendDiscoveryEvents(false);
-  GetMutableAdapter(profile())->SetDiscovering(false,
-      base::Bind(&BluetoothStopDiscoveryFunction::OnSuccessCallback, this),
-      base::Bind(&BluetoothStopDiscoveryFunction::OnErrorCallback, this));
+  if (GetEventRouter(profile())->IsResponsibleForDiscovery()) {
+    GetMutableAdapter(profile())->SetDiscovering(false,
+        base::Bind(&BluetoothStopDiscoveryFunction::OnSuccessCallback, this),
+        base::Bind(&BluetoothStopDiscoveryFunction::OnErrorCallback, this));
+  }
   return true;
 }
 

chrome/browser/extensions/api/bluetooth/bluetooth_apitest_chromeos.cc

@@ -291,6 +291,7 @@ IN_PROC_BROWSER_TEST_F(BluetoothApiTest, SetOutOfBandPairingData) {
 
 IN_PROC_BROWSER_TEST_F(BluetoothApiTest, Discovery) {
   // Try with a failure to start
+  EXPECT_CALL(*mock_adapter_, IsDiscovering()).WillOnce(testing::Return(false));
   EXPECT_CALL(*mock_adapter_,
               SetDiscovering(true,
                              testing::_,
@@ -303,6 +304,7 @@ IN_PROC_BROWSER_TEST_F(BluetoothApiTest, Discovery) {
 
   // Reset for a successful start
   testing::Mock::VerifyAndClearExpectations(mock_adapter_);
+  EXPECT_CALL(*mock_adapter_, IsDiscovering()).WillOnce(testing::Return(false));
   EXPECT_CALL(*mock_adapter_,
               SetDiscovering(true,
                              testing::Truly(CallClosure),
@@ -333,6 +335,7 @@ IN_PROC_BROWSER_TEST_F(BluetoothApiTest, Discovery) {
 }
 
 IN_PROC_BROWSER_TEST_F(BluetoothApiTest, DiscoveryCallback) {
+  EXPECT_CALL(*mock_adapter_, IsDiscovering()).WillOnce(testing::Return(false));
   EXPECT_CALL(*mock_adapter_,
               SetDiscovering(true, testing::Truly(CallClosure), testing::_));
   EXPECT_CALL(*mock_adapter_,
@@ -360,6 +363,37 @@ IN_PROC_BROWSER_TEST_F(BluetoothApiTest, DiscoveryCallback) {
   EXPECT_TRUE(catcher.GetNextResult()) << catcher.message();
 }
 
+IN_PROC_BROWSER_TEST_F(BluetoothApiTest, DiscoveryInProgress) {
+  // Fake that the adapter is discovering
+  EXPECT_CALL(*mock_adapter_, IsDiscovering()).WillOnce(testing::Return(true));
+  event_router()->AdapterDiscoveringChanged(mock_adapter_, true);
+
+  // Cache a device before the extension starts discovering
+  event_router()->DeviceAdded(mock_adapter_, device1_.get());
+
+  ResultCatcher catcher;
+  catcher.RestrictToProfile(browser()->profile());
+
+  ExtensionTestMessageListener discovery_started("ready", true);
+  const extensions::Extension* extension =
+      LoadExtension(test_data_dir_.AppendASCII("bluetooth"));
+  GURL page_url = extension->GetResourceURL("test_discovery_in_progress.html");
+  ui_test_utils::NavigateToURL(browser(), page_url);
+  EXPECT_TRUE(discovery_started.WaitUntilSatisfied());
+
+  // This should be received in addition to the cached device above.
+  event_router()->DeviceAdded(mock_adapter_, device2_.get());
+
+  discovery_started.Reply("go");
+  ExtensionTestMessageListener discovery_stopped("ready", true);
+  EXPECT_TRUE(discovery_stopped.WaitUntilSatisfied());
+
+  event_router()->DeviceAdded(mock_adapter_, device2_.get());
+  discovery_stopped.Reply("go");
+
+  EXPECT_TRUE(catcher.GetNextResult()) << catcher.message();
+}
+
 IN_PROC_BROWSER_TEST_F(BluetoothApiTest, Events) {
   ResultCatcher catcher;
   catcher.RestrictToProfile(browser()->profile());

chrome/common/extensions/api/experimental_bluetooth.idl

@@ -190,8 +190,9 @@ namespace experimental.bluetooth {
                                         optional ResultCallback callback);
 
     // Start discovery. Discovered devices will be returned via the
-    // |onDeviceDiscovered| callback. Note that discovery can be resource
-    // intensive. stopDiscovery should be called as soon as is convenient.
+    // |onDeviceDiscovered| callback.  Discovery will fail to start if it is
+    // already in progress.  Discovery can be resource intensive: stopDiscovery
+    // should be called as soon as possible.
     // |options|  : The options for this function.
     // |callback| : Called to indicate success or failure.
     static void startDiscovery(

chrome/common/extensions/docs/apps/experimental.bluetooth.html

@@ -1173,7 +1173,7 @@
                       <var><span>options</span></var></span><span class="optional"><span>, </span><span>function</span>
                       <var><span>ResultCallback</span></var></span>)</div>
               <div class="description">
-                <p>Start discovery. Discovered devices will be returned via the |onDeviceDiscovered| callback. Note that discovery can be resource intensive. stopDiscovery should be called as soon as is convenient.</p>
+                <p>Start discovery. Discovered devices will be returned via the |onDeviceDiscovered| callback.  Discovery will fail to start if it is already in progress.  Discovery can be resource intensive: stopDiscovery should be called as soon as possible.</p>
                 <!-- PARAMETERS -->
                 <h4>Parameters</h4>
                 <dl>

chrome/common/extensions/docs/extensions/experimental.bluetooth.html

@@ -1554,7 +1554,7 @@
                       <var><span>options</span></var></span><span class="optional"><span>, </span><span>function</span>
                       <var><span>ResultCallback</span></var></span>)</div>
               <div class="description">
-                <p>Start discovery. Discovered devices will be returned via the |onDeviceDiscovered| callback. Note that discovery can be resource intensive. stopDiscovery should be called as soon as is convenient.</p>
+                <p>Start discovery. Discovered devices will be returned via the |onDeviceDiscovered| callback.  Discovery will fail to start if it is already in progress.  Discovery can be resource intensive: stopDiscovery should be called as soon as possible.</p>
                 <!-- PARAMETERS -->
                 <h4>Parameters</h4>
                 <dl>

chrome/common/extensions/docs/server2/templates/articles/app_external.html

@@ -278,4 +278,4 @@ var messageHandler = function(e) {
 window.addEventListener('message', messageHandler);
 </pre>
 
-<p class="backtotop"><a href="#top">Back to top</a></p>
+<p class="backtotop"><a href="#top">Back to top</a></p>
\ No newline at end of file

chrome/renderer/resources/extensions/experimental.bluetooth_custom_bindings.js

@@ -6,6 +6,7 @@
 
 var chromeHidden = requireNative('chrome_hidden').GetChromeHidden();
 var sendRequest = require('sendRequest').sendRequest;
+var lastError = require('last_error');
 
 // Use custom bindings to create an undocumented event listener that will
 // receive events about device discovery and call the event listener that was
@@ -14,26 +15,47 @@ chromeHidden.registerCustomHook('experimental.bluetooth', function(api) {
   var apiFunctions = api.apiFunctions;
 
   chromeHidden.bluetooth = {};
-  chromeHidden.bluetooth.handler = null;
+  chromeHidden.bluetooth.deviceDiscoveredHandler = null;
   chromeHidden.bluetooth.onDeviceDiscovered =
       new chrome.Event("experimental.bluetooth.onDeviceDiscovered");
 
-  function deviceDiscoveredListener(device) {
-    if (chromeHidden.bluetooth.handler != null)
-      chromeHidden.bluetooth.handler(device);
+  function clearDeviceDiscoveredHandler() {
+    chromeHidden.bluetooth.onDeviceDiscovered.removeListener(
+        chromeHidden.bluetooth.deviceDiscoveredHandler);
+    chromeHidden.bluetooth.deviceDiscoveredHandler = null;
   }
-  chromeHidden.bluetooth.onDeviceDiscovered.addListener(
-      deviceDiscoveredListener);
 
-  apiFunctions.setHandleRequest('startDiscovery', function() {
-    var args = arguments;
-    if (args.length > 0 && args[0] && args[0].deviceCallback) {
-      chromeHidden.bluetooth.handler = args[0].deviceCallback;
-    }
-    sendRequest(this.name, args, this.definition.parameters);
-  });
-  apiFunctions.setHandleRequest('stopDiscovery', function() {
-    chromeHidden.bluetooth.handler = null;
-    sendRequest(this.name, arguments, this.definition.parameters);
-  });
+  apiFunctions.setHandleRequest('startDiscovery',
+      function() {
+        var args = arguments;
+        if (args.length > 0 && args[0] && args[0].deviceCallback) {
+          chromeHidden.bluetooth.deviceDiscoveredHandler =
+              args[0].deviceCallback;
+          chromeHidden.bluetooth.onDeviceDiscovered.addListener(
+              chromeHidden.bluetooth.deviceDiscoveredHandler);
+          sendRequest(this.name,
+                      args,
+                      this.definition.parameters,
+                      {customCallback:this.customCallback});
+        } else {
+          if (typeof(args[args.length-1]) == "function") {
+            var callback = args[args.length-1];
+            lastError.set("deviceCallback is required in the options object");
+            callback();
+            return;
+          }
+        }
+      });
+  apiFunctions.setCustomCallback('startDiscovery',
+      function(name, request, response) {
+        if (chrome.runtime.lastError) {
+          clearDeviceDiscoveredHandler();
+          return;
+        }
+      });
+  apiFunctions.setHandleRequest('stopDiscovery',
+      function() {
+        clearDeviceDiscoveredHandler();
+        sendRequest(this.name, arguments, this.definition.parameters);
+      });
 });

chrome/test/data/extensions/api_test/bluetooth/test_discovery.html

@@ -3,4 +3,5 @@
  * source code is governed by a BSD-style license that can be found in the
  * LICENSE file.
 -->
+<script src="test_discovery_expectation.js"></script>
 <script src="test_discovery.js"></script>

chrome/test/data/extensions/api_test/bluetooth/test_discovery.js

@@ -6,7 +6,8 @@ function testDiscovery() {
   chrome.test.assertEq(kExpectedDeviceNames.length,
       discoveredDevices.length);
   for (var i = 0; i < kExpectedDeviceNames.length; ++i) {
-    chrome.test.assertEq(kExpectedDeviceNames[i], discoveredDevices[i].name);
+    chrome.test.assertEq(kExpectedDeviceNames[i],
+        discoveredDevices[i].name);
   }
 
   chrome.test.succeed();
@@ -25,7 +26,6 @@ function stopDiscoveryAndContinue() {
   sendReady(startTests);
 }
 
-var kExpectedDeviceNames = ["d1"];
 var discoveredDevices = [];
 function recordDevice(device) {
   discoveredDevices.push(device);

chrome/test/data/extensions/api_test/bluetooth/test_discovery_expectation.js

+// 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.
+var kExpectedDeviceNames = ["d1"];

chrome/test/data/extensions/api_test/bluetooth/test_discovery_in_progress.html

+<!--
+ * 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.
+-->
+<script src="test_discovery_in_progress_expectation.js"></script>
+<script src="test_discovery.js"></script>

chrome/test/data/extensions/api_test/bluetooth/test_discovery_in_progress_expectation.js

+// 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.
+var kExpectedDeviceNames = ["d1", "d2"];