bluetooth: Move requestDevice to mojo

content/browser/bluetooth/bluetooth_device_provider.h

+// Copyright 2016 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.
+
+#ifndef CONTENT_BROWSER_BLUETOOTH_BLUETOOTH_DEVICE_PROVIDER_H_
+#define CONTENT_BROWSER_BLUETOOTH_BLUETOOTH_DEVICE_PROVIDER_H_
+
+#include <string>
+
+#include "base/memory/weak_ptr.h"
+#include "base/timer/timer.h"
+#include "content/public/browser/bluetooth_chooser.h"
+#include "third_party/WebKit/public/platform/modules/bluetooth/web_bluetooth.mojom.h"
+
+namespace device {
+class BluetoothAdapter;
+class BluetoothDevice;
+class BluetoothDiscoveryFilter;
+class BluetoothDiscoverySession;
+}
+
+namespace content {
+
+class RenderFrameHost;
+class WebContents;
+
+// Class that interacts with a chooser and starts a bluetooth discovery session.

[Jeffrey Yasskin, 2016/05/13 04:41:59]

Can this class manage more than one successive GetDevice() call, or does it need
to be re-instantiated for each one?

[ortuno, 2016/05/13 20:11:17]

Added comment.

+class BluetoothDeviceProvider final {

[Jeffrey Yasskin, 2016/05/13 04:41:59]

I'm not totally happy with this name, but I don't have a great suggestion. Maybe
BluetoothDeviceChooserManager? or ...Controller?

[ortuno, 2016/05/13 20:11:17]

Changed to BluetoothDeviceChooserController.

+ public:
+  typedef base::Callback<void(const std::string& device_address)>
+      SuccessCallback;
+  typedef base::Callback<void(blink::mojom::WebBluetoothError error)>
+      ErrorCallback;
+
+  // |render_frame_host| should be the RenderFrameHost that owns the
+  // WebBluetoothServiceImpl that owns this class.
+  // |adapter| should be the adapter used to scan for Bluetooth devices.
+  // |scan_duration| is how long will a discovery session be active for.

[Jeffrey Yasskin, 2016/05/13 04:41:59]

Nit: "is how long a discovery session will be active." "for" at the end is ok
but unnecessary.

[ortuno, 2016/05/13 20:11:17]

Done.

+  BluetoothDeviceProvider(RenderFrameHost* render_frame_host,
+                          device::BluetoothAdapter* adapter,
+                          int scan_duration);

[Jeffrey Yasskin, 2016/05/13 04:41:59]

Lengths of time should be base::TimeDelta so that the units are built in:
https://code.google.com/p/chromium/codesearch/#chromium/src/base/time/time.h

[ortuno, 2016/05/13 20:11:18]

Done.

+  ~BluetoothDeviceProvider();
+
+  // This function performs the following checks before starting a discovery

[Jeffrey Yasskin, 2016/05/13 04:41:59]

If this function is called again before the previous call runs the success or
error callback, what happens?

[ortuno, 2016/05/13 20:11:17]

It will DCHECK. Added comment.

+  // session:
+  //   - Validates filters in |request_device_options|.
+  //   - Removes any blacklisted UUIDs from
+  //     |request_device_options.optinal_services|.
+  //   - Checks if the request came from a cross-origin iframe.
+  //   - Checks if the request came from a unique origin.
+  //   - Checks if the adapter is present.
+  //   - Checks if the Web Bluetooth API has been disabled.
+  //   - Checks if we are allowed to ask for scanning permission.
+  // If any of the previous checks failed then this function runs
+  // |error_callback| with the corresponding error. Otherwise this function
+  // populates the embedder provided BluetoothChooser with existing devices and
+  // starts a new discovery session.
+  void GetDevice(
+      blink::mojom::WebBluetoothRequestDeviceOptionsPtr request_device_options,
+      const SuccessCallback& success_callback,
+      const ErrorCallback& error_callback);
+
+  // Adds a device to the chooser.

[Jeffrey Yasskin, 2016/05/13 04:41:59]

Must this be called only while a GetDevice() is active?

[ortuno, 2016/05/13 20:11:18]

Added comment.

+  void AddFilteredDevice(const device::BluetoothDevice& device);
+
+  // Stops the current discovery session and notifies the chooser

[Jeffrey Yasskin, 2016/05/13 04:41:59]

It's odd that this class would need something external to tell it that the
adapter has been turned on or off, but would be able to stop a discovery session
on its own.

[ortuno, 2016/05/13 20:11:17]

I agree it's a bit weird. The way I think about it is that this class is allowed
to talk to the adapter, but the adapter is not allowed to talk to this class.

+  // that the adapter changed states.
+  void AdapterPoweredChanged(bool powered);
+
+ private:
+  // Notifies the chooser that discovery is starting and starts a discovery
+  // session.
+  void StartDeviceDiscovery();
+
+  // Stops the discovery session and notifies the chooser.
+  void StopDeviceDiscovery();
+
+  // StartDiscoverySessionWithFilter callbacks:
+  void OnStartDiscoverySessionSuccess(
+      std::unique_ptr<device::BluetoothDiscoverySession> discovery_session);
+  void OnStartDiscoverySessionFailed();
+
+  // BluetoothChooser::EventHandler:
+  // Runs error_callback_ if the chooser was cancelled or if we weren't able
+  // to show the chooser. Otherwise runs success_callback_ with
+  // |device_address|.
+  void OnBluetoothChooserEvent(BluetoothChooser::Event event,
+                               const std::string& device_address);
+
+  // Helper function to asynchronously run success_callback_.
+  void PostSuccessCallback(const std::string& device_address);
+  // Helper function to asynchronously run error_callback_.
+  void PostErrorCallback(blink::mojom::WebBluetoothError error);
+
+  // The adapter used to get existing devices and start a discovery session.
+  device::BluetoothAdapter* adapter_;
+  // The RenderFrameHost owning the WebBluetoothServiceImpl that owns this
+  // instance.
+  RenderFrameHost* render_frame_host_;
+  // The WebContents that owns render_frame_host_.
+  WebContents* web_contents_;
+
+  // Contains the filters and optional services used when scanning.
+  blink::mojom::WebBluetoothRequestDeviceOptionsPtr options_;
+
+  // Callbacks to be called with the result of the chooser.
+  SuccessCallback success_callback_;
+  ErrorCallback error_callback_;
+
+  // The currently opened BluetoothChooser.
+  std::unique_ptr<BluetoothChooser> chooser_;
+
+  // Automatically stops Bluetooth discovery a set amount of time after it was
+  // started.
+  base::Timer discovery_session_timer_;
+
+  // The last discovery session to be started.

[Jeffrey Yasskin, 2016/05/13 04:41:59]

So this doesn't get nulled out when no discovery session is active?

[ortuno, 2016/05/13 20:11:17]

We could null it when we stop it but we would also have to null it when the
adapter stops it for some reason and for that we need to override another
Observer function. Do you think we should do it?

+  std::unique_ptr<device::BluetoothDiscoverySession> discovery_session_;
+
+  // Weak pointer factory for generating 'this' pointers that might live longer
+  // than we do.
+  // Note: This should remain the last member so it'll be destroyed and
+  // invalidate its weak pointers before any other members are destroyed.
+  base::WeakPtrFactory<BluetoothDeviceProvider> weak_ptr_factory_;
+};
+
+}  // namespace content
+
+#endif  // CONTENT_BROWSER_BLUETOOTH_BLUETOOTH_DEVICE_PROVIDER_H_