Bluetooth: notify user of incoming pairing requests

Bluetooth: notify user of incoming pairing requests

Implement a low-priority pairing delegate in the form of an Aura Status
Tray Notification Controller that displays a rich notification when an
incoming pairing request is received and allows the user to accept or
reject it when appropriate.

BUG=345535
TESTS=chromeos=1 on Linux presents fake pairing requests

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=254697

[Daniel Erat, 2014-02-28 21:18:01]

(i won't be able to review this for a few days, so you may want to find another
reviewer for my part if you want it sooner)

[stevenjb, 2014-02-28 21:40:53]

lgtm

https://codereview.chromium.org/183853010/diff/1/chromeos/dbus/fake_bluetooth...
File chromeos/dbus/fake_bluetooth_adapter_client.cc (right):

https://codereview.chromium.org/183853010/diff/1/chromeos/dbus/fake_bluetooth...
chromeos/dbus/fake_bluetooth_adapter_client.cc:183:
dbus::ObjectPath(kAdapterPath));
nit: {}

https://codereview.chromium.org/183853010/diff/1/chromeos/dbus/fake_bluetooth...
File chromeos/dbus/fake_bluetooth_device_client.cc (right):

https://codereview.chromium.org/183853010/diff/1/chromeos/dbus/fake_bluetooth...
chromeos/dbus/fake_bluetooth_device_client.cc:67: VLOG(1) << "Error: " <<
error_name << ": " << error_message;
nit: "Bluetooth Error: "

https://codereview.chromium.org/183853010/diff/1/device/bluetooth/bluetooth_p...
File device/bluetooth/bluetooth_pairing_chromeos.cc (right):

https://codereview.chromium.org/183853010/diff/1/device/bluetooth/bluetooth_p...
device/bluetooth/bluetooth_pairing_chromeos.cc:167: if (entered > 6 &&
!device_->IsConnecting())
const for 6

[keybuk, 2014-02-28 22:28:53]

https://codereview.chromium.org/183853010/diff/1/chromeos/dbus/fake_bluetooth...
File chromeos/dbus/fake_bluetooth_adapter_client.cc (right):

https://codereview.chromium.org/183853010/diff/1/chromeos/dbus/fake_bluetooth...
chromeos/dbus/fake_bluetooth_adapter_client.cc:183:
dbus::ObjectPath(kAdapterPath));
On 2014/02/28 21:40:54, stevenjb wrote:
> nit: {}

Done.

https://codereview.chromium.org/183853010/diff/1/chromeos/dbus/fake_bluetooth...
File chromeos/dbus/fake_bluetooth_device_client.cc (right):

https://codereview.chromium.org/183853010/diff/1/chromeos/dbus/fake_bluetooth...
chromeos/dbus/fake_bluetooth_device_client.cc:67: VLOG(1) << "Error: " <<
error_name << ": " << error_message;
On 2014/02/28 21:40:54, stevenjb wrote:
> nit: "Bluetooth Error: "

Done.

https://codereview.chromium.org/183853010/diff/1/device/bluetooth/bluetooth_p...
File device/bluetooth/bluetooth_pairing_chromeos.cc (right):

https://codereview.chromium.org/183853010/diff/1/device/bluetooth/bluetooth_p...
device/bluetooth/bluetooth_pairing_chromeos.cc:167: if (entered > 6 &&
!device_->IsConnecting())
On 2014/02/28 21:40:54, stevenjb wrote:
> const for 6

Done.

[armansito, 2014-02-28 23:20:05]

https://codereview.chromium.org/183853010/diff/20001/chromeos/dbus/fake_bluet...
File chromeos/dbus/fake_bluetooth_adapter_client.cc (right):

https://codereview.chromium.org/183853010/diff/20001/chromeos/dbus/fake_bluet...
chromeos/dbus/fake_bluetooth_adapter_client.cc:183:
dbus::ObjectPath(kAdapterPath));
Should we start the simulation only if |properties_->pairable| (and also maybe
|properties_->discoverable|?) is true? I know we don't really set pairable to
false anywhere at the moment, but it might make sense to make the fake behave
that way anyway.

[keybuk, 2014-02-28 23:21:27]

https://codereview.chromium.org/183853010/diff/20001/chromeos/dbus/fake_bluet...
File chromeos/dbus/fake_bluetooth_adapter_client.cc (right):

https://codereview.chromium.org/183853010/diff/20001/chromeos/dbus/fake_bluet...
chromeos/dbus/fake_bluetooth_adapter_client.cc:183:
dbus::ObjectPath(kAdapterPath));
Since there's no way to set those in the UI right now, that'd make the fake not
work at all ;-)

[jennyz, 2014-02-28 23:24:36]

What if the pairing does not succeed, like timeout, user error wrong pairing
code, how will user see the error message?

https://codereview.chromium.org/183853010/diff/20001/ash/system/chromeos/blue...
File ash/system/chromeos/bluetooth/bluetooth_notification_controller.cc (right):

https://codereview.chromium.org/183853010/diff/20001/ash/system/chromeos/blue...
ash/system/chromeos/bluetooth/bluetooth_notification_controller.cc:160:
paired_devices_.insert(device->GetAddress());
Is the insert redundant? device->GetAddress already exist in paried_devices_.

https://codereview.chromium.org/183853010/diff/20001/ash/system/chromeos/blue...
ash/system/chromeos/bluetooth/bluetooth_notification_controller.cc:173: // audio
systems for the most part) will need pairing to be initated from
nit: initated -> initiated

[keybuk, 2014-02-28 23:28:18]

https://codereview.chromium.org/183853010/diff/20001/ash/system/chromeos/blue...
File ash/system/chromeos/bluetooth/bluetooth_notification_controller.cc (right):

https://codereview.chromium.org/183853010/diff/20001/ash/system/chromeos/blue...
ash/system/chromeos/bluetooth/bluetooth_notification_controller.cc:160:
paired_devices_.insert(device->GetAddress());
On 2014/02/28 23:24:36, jennyz wrote:
> Is the insert redundant? device->GetAddress already exist in paried_devices_.

You have the condition above backwards ... that makes sure the device is NOT in
paired_devices

https://codereview.chromium.org/183853010/diff/20001/ash/system/chromeos/blue...
ash/system/chromeos/bluetooth/bluetooth_notification_controller.cc:173: // audio
systems for the most part) will need pairing to be initated from
On 2014/02/28 23:24:36, jennyz wrote:
> nit: initated -> initiated

Done.

[jennyz, 2014-02-28 23:31:19]

https://codereview.chromium.org/183853010/diff/20001/ash/system/chromeos/blue...
File ash/system/chromeos/bluetooth/bluetooth_notification_controller.cc (right):

https://codereview.chromium.org/183853010/diff/20001/ash/system/chromeos/blue...
ash/system/chromeos/bluetooth/bluetooth_notification_controller.cc:160:
paired_devices_.insert(device->GetAddress());
On 2014/02/28 23:28:18, keybuk wrote:
> On 2014/02/28 23:24:36, jennyz wrote:
> > Is the insert redundant? device->GetAddress already exist in
paried_devices_.
> 
> You have the condition above backwards ... that makes sure the device is NOT
in
> paired_devices

My blindness, I read it oppositely. You are right.

[armansito, 2014-02-28 23:46:03]

lgtm

https://codereview.chromium.org/183853010/diff/20001/chromeos/dbus/fake_bluet...
File chromeos/dbus/fake_bluetooth_adapter_client.cc (right):

https://codereview.chromium.org/183853010/diff/20001/chromeos/dbus/fake_bluet...
chromeos/dbus/fake_bluetooth_adapter_client.cc:183:
dbus::ObjectPath(kAdapterPath));
On 2014/02/28 23:21:28, keybuk wrote:
> Since there's no way to set those in the UI right now, that'd make the fake
not
> work at all ;-)

We seem to be setting pairable to true by default but you're right, this can be
revised once/if there is ever UI for setting the adapter discoverable.

[jennyz, 2014-02-28 23:51:32]

lgtm

[keybuk, 2014-03-01 00:00:29]

The CQ bit was checked by keybuk@chromium.org

[commit-bot: I haz the power, 2014-03-01 00:02:26]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/keybuk@chromium.org/183853010/60001

[commit-bot: I haz the power, 2014-03-01 00:25:07]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-01 00:25:07]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[keybuk, 2014-03-03 19:42:11]

sky: still need LGTM for ash/

[sky, 2014-03-03 22:56:21]

https://codereview.chromium.org/183853010/diff/60001/ash/system/chromeos/blue...
File ash/system/chromeos/bluetooth/bluetooth_notification_controller.cc (right):

https://codereview.chromium.org/183853010/diff/60001/ash/system/chromeos/blue...
ash/system/chromeos/bluetooth/bluetooth_notification_controller.cc:66:
std::string address_;
nit: const

https://codereview.chromium.org/183853010/diff/60001/ash/system/chromeos/blue...
ash/system/chromeos/bluetooth/bluetooth_notification_controller.cc:113: case 0:
This seems awful fragile. How come no enum?

https://codereview.chromium.org/183853010/diff/60001/ash/system/chromeos/blue...
ash/system/chromeos/bluetooth/bluetooth_notification_controller.cc:141:
adapter_->RemoveObserver(this);
Is fetching the adapter async? If so, how do you know by the time you get here
adapter_ has been set?

https://codereview.chromium.org/183853010/diff/60001/ash/system/chromeos/blue...
ash/system/chromeos/bluetooth/bluetooth_notification_controller.cc:232: adapter_
= adapter;
DCHECK(adapter_ == NULL0?

https://codereview.chromium.org/183853010/diff/60001/ash/system/chromeos/blue...
File ash/system/chromeos/bluetooth/bluetooth_notification_controller.h (right):

https://codereview.chromium.org/183853010/diff/60001/ash/system/chromeos/blue...
ash/system/chromeos/bluetooth/bluetooth_notification_controller.h:79: // Note:
This should remain the last member so it'll be destroyed and
nit: IMO comments like this are easy to miss. Better to be explicit by having
code in the destructor.

https://codereview.chromium.org/183853010/diff/60001/ash/system/system_notifi...
File ash/system/system_notifier.h (right):

https://codereview.chromium.org/183853010/diff/60001/ash/system/system_notifi...
ash/system/system_notifier.h:31: ASH_EXPORT extern const char
kNotifierBluetooth[];
nit: keep sorted (see comment on line 16). I see power isn't alphabetical,
update it too.

[keybuk, 2014-03-03 23:40:38]

https://codereview.chromium.org/183853010/diff/60001/ash/system/chromeos/blue...
File ash/system/chromeos/bluetooth/bluetooth_notification_controller.cc (right):

https://codereview.chromium.org/183853010/diff/60001/ash/system/chromeos/blue...
ash/system/chromeos/bluetooth/bluetooth_notification_controller.cc:66:
std::string address_;
On 2014/03/03 22:56:22, sky wrote:
> nit: const

Done.

https://codereview.chromium.org/183853010/diff/60001/ash/system/chromeos/blue...
ash/system/chromeos/bluetooth/bluetooth_notification_controller.cc:113: case 0:
On 2014/03/03 22:56:22, sky wrote:
> This seems awful fragile. How come no enum?

Done.

https://codereview.chromium.org/183853010/diff/60001/ash/system/chromeos/blue...
ash/system/chromeos/bluetooth/bluetooth_notification_controller.cc:141:
adapter_->RemoveObserver(this);
On 2014/03/03 22:56:22, sky wrote:
> Is fetching the adapter async? If so, how do you know by the time you get here
> adapter_ has been set?

Done.

https://codereview.chromium.org/183853010/diff/60001/ash/system/chromeos/blue...
ash/system/chromeos/bluetooth/bluetooth_notification_controller.cc:232: adapter_
= adapter;
On 2014/03/03 22:56:22, sky wrote:
> DCHECK(adapter_ == NULL0?

Done.

https://codereview.chromium.org/183853010/diff/60001/ash/system/chromeos/blue...
File ash/system/chromeos/bluetooth/bluetooth_notification_controller.h (right):

https://codereview.chromium.org/183853010/diff/60001/ash/system/chromeos/blue...
ash/system/chromeos/bluetooth/bluetooth_notification_controller.h:79: // Note:
This should remain the last member so it'll be destroyed and
The problem this comment is referring to isn't a runtime error, as you say
that'd be easy to fix by just explicitly cleaning up members in the Destructor.

The problem is a compile-time error, the compiler ultimately doesn't trust us to
get destruction right when it comes to class members with their own destructors,
so throws an error if this isn't the last thing.

Thus this comment exists (and exists in many places WeakPtrFactory is used in
Chrome) because it's a particularly obscure compiler error, and to explain what
it means when someone decides to ignore the line and move it anyway ;-)

https://codereview.chromium.org/183853010/diff/60001/ash/system/system_notifi...
File ash/system/system_notifier.h (right):

https://codereview.chromium.org/183853010/diff/60001/ash/system/system_notifi...
ash/system/system_notifier.h:31: ASH_EXPORT extern const char
kNotifierBluetooth[];
On 2014/03/03 22:56:22, sky wrote:
> nit: keep sorted (see comment on line 16). I see power isn't alphabetical,
> update it too.

Done.

[sky, 2014-03-03 23:46:34]

LGTM

[keybuk, 2014-03-03 23:47:29]

The CQ bit was checked by keybuk@chromium.org

[commit-bot: I haz the power, 2014-03-03 23:49:32]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/keybuk@chromium.org/183853010/80001

[commit-bot: I haz the power, 2014-03-04 01:12:39]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-04 01:12:40]

Retried try job too often on win_rel for step(s) base_unittests, browser_tests,
interactive_ui_tests, net_unittests, unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[keybuk, 2014-03-04 01:47:43]

The CQ bit was checked by keybuk@chromium.org

[commit-bot: I haz the power, 2014-03-04 01:48:22]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/keybuk@chromium.org/183853010/100001

[keybuk, 2014-03-04 06:19:58]

The CQ bit was unchecked by keybuk@chromium.org

[keybuk, 2014-03-04 06:20:07]

The CQ bit was checked by keybuk@chromium.org

[commit-bot: I haz the power, 2014-03-04 06:59:53]

Change committed as 254697