Bluetooth: Add support for pairing display notifications

Bluetooth: Add support for pairing display notifications

The HID 1.1 spec defines a notification signal sent from the device
every time the user types a new digit of the provided passkey during
the SSP pairing.

This patch extends the BluetoothDevice::PairingDelegate interface to
support a new method DisplayEnteredKeys to update the number of
pressed keys.

BUG=221105
TEST=device_unittest and manual test.

Manual test part:
=================
1. Open the chrome://settings page in the chromebook and click on "Inspect element". A new panel with the HTML is displayed.
2. Go to the "Console" tab in the opened panel and switch the "<top frame>" drop down in the status line at the bottom of the screen to "settings".
3. Run the following command:
options.BrowserOptions.addBluetoothDevice({"name":"MyMouse","address":"00:11:22:33:44:55","pairing":"bluetoothRemotePasskey","entered":4,"passkey":123456})
4. The passkey 123456 should be displayed on the screen with the first 4 keys in a different style.
5. Replacing "entered":4 by "entered":7 should also change the style of the "enter" key.
6. Removing the "entered":4, should show the keys darker than the "entered":0 state.

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

[deymo, 2013-04-17 23:17:53]

glotov@ for chrome/browser/ui/webui/options/chromeos/
keybuk@ for device/bluetooth/

[keybuk, 2013-04-17 23:43:37]

https://codereview.chromium.org/13870020/diff/2001/device/bluetooth/bluetooth...
File device/bluetooth/bluetooth_device.h (right):

https://codereview.chromium.org/13870020/diff/2001/device/bluetooth/bluetooth...
device/bluetooth/bluetooth_device.h:127: // of a key entered in the device
|device| while pairing with the device
nit: "on" the device

https://codereview.chromium.org/13870020/diff/2001/device/bluetooth/bluetooth...
device/bluetooth/bluetooth_device.h:128: // using a PIN code or a Passkey. This
method will be called only after
nit: paragraph/line break here to de-dense the text

Also this is never called for DisplayPinCode?

https://codereview.chromium.org/13870020/diff/2001/device/bluetooth/bluetooth...
device/bluetooth/bluetooth_device.h:130: // corresponding
DismissDisplayOrConfirm() is called, but is not warrantied
nit: warranted

https://codereview.chromium.org/13870020/diff/2001/device/bluetooth/bluetooth...
device/bluetooth/bluetooth_device.h:133: // The |entered| value describes the
number of keys entered so far,
nit: paragraph/line break before this one

https://codereview.chromium.org/13870020/diff/2001/device/bluetooth/bluetooth...
device/bluetooth/bluetooth_device.h:135: // |entered| as 0 will be effectuated
when the device supports this feature.
Isn't this only supported by DisplayPasskey?

Or are you anticipating eventual support for the Apple Keyboard?

https://codereview.chromium.org/13870020/diff/2001/device/bluetooth/bluetooth...
device/bluetooth/bluetooth_device.h:135: // |entered| as 0 will be effectuated
when the device supports this feature.
nit: replace effectuated with "sent" :)

[deymo, 2013-04-18 00:26:01]

Description updated with manual test. New patch uploaded. PTAL

https://codereview.chromium.org/13870020/diff/2001/device/bluetooth/bluetooth...
File device/bluetooth/bluetooth_device.h (right):

https://codereview.chromium.org/13870020/diff/2001/device/bluetooth/bluetooth...
device/bluetooth/bluetooth_device.h:127: // of a key entered in the device
|device| while pairing with the device
On 2013/04/17 23:43:37, keybuk wrote:
> nit: "on" the device

Done.

https://codereview.chromium.org/13870020/diff/2001/device/bluetooth/bluetooth...
device/bluetooth/bluetooth_device.h:128: // using a PIN code or a Passkey. This
method will be called only after
> Also this is never called for DisplayPinCode?
Right now, with the current BlueZ, no, it is never called. Buuuut, it may be
called if we fix the apple keyboard notification someday.

https://codereview.chromium.org/13870020/diff/2001/device/bluetooth/bluetooth...
device/bluetooth/bluetooth_device.h:130: // corresponding
DismissDisplayOrConfirm() is called, but is not warrantied
On 2013/04/17 23:43:37, keybuk wrote:
> nit: warranted

Done.

https://codereview.chromium.org/13870020/diff/2001/device/bluetooth/bluetooth...
device/bluetooth/bluetooth_device.h:133: // The |entered| value describes the
number of keys entered so far,
On 2013/04/17 23:43:37, keybuk wrote:
> nit: paragraph/line break before this one

Done.

https://codereview.chromium.org/13870020/diff/2001/device/bluetooth/bluetooth...
device/bluetooth/bluetooth_device.h:135: // |entered| as 0 will be effectuated
when the device supports this feature.
> Or are you anticipating eventual support for the Apple Keyboard?
Yes, apple keyboard could be supported eventually and I don't see how ignoring
that support will simplify this interface anyway... so I just added the method
assuming both can be supported by the underlying daemon.

[keybuk, 2013-04-18 00:30:12]

https://codereview.chromium.org/13870020/diff/6001/chrome/browser/ui/webui/op...
File chrome/browser/ui/webui/options/chromeos/bluetooth_options_handler.h
(right):

https://codereview.chromium.org/13870020/diff/6001/chrome/browser/ui/webui/op...
chrome/browser/ui/webui/options/chromeos/bluetooth_options_handler.h:99: 
missing //

https://codereview.chromium.org/13870020/diff/6001/chrome/browser/ui/webui/op...
chrome/browser/ui/webui/options/chromeos/bluetooth_options_handler.h:101: // of
a key entered in the device |device| while pairing with the device
make the same text fixes here that were made on the comment in BluetoothDevice

[keybuk, 2013-04-18 00:56:25]

https://chromiumcodereview.appspot.com/13870020/diff/13001/device/bluetooth/b...
File device/bluetooth/bluetooth_device.h (right):

https://chromiumcodereview.appspot.com/13870020/diff/13001/device/bluetooth/b...
device/bluetooth/bluetooth_device.h:140: uint32 entered) = 0;
Should this be called something like:

  KeypressNotification ?

(that's the name of the event from the spec)

[deymo, 2013-04-18 01:23:14]

There it is the new change.

https://chromiumcodereview.appspot.com/13870020/diff/13001/device/bluetooth/b...
File device/bluetooth/bluetooth_device.h (right):

https://chromiumcodereview.appspot.com/13870020/diff/13001/device/bluetooth/b...
device/bluetooth/bluetooth_device.h:140: uint32 entered) = 0;
On 2013/04/18 00:56:25, keybuk wrote:
> Should this be called something like:
> 
>   KeypressNotification ?
> 
> (that's the name of the event from the spec)

For the record: Changing this to KeysEntered

[keybuk, 2013-04-18 06:17:09]

This CL should probably add the appropriate line of code to
BluetoothDeviceExperimentalChromeOS that makes this work

[deymo, 2013-04-18 06:57:10]

On 2013/04/18 06:17:09, keybuk wrote:
> This CL should probably add the appropriate line of code to
> BluetoothDeviceExperimentalChromeOS that makes this work

This does the implementation of the new KeysEntered function. The usage of that
function will go in a separate CL.

[glotov, 2013-04-18 11:06:34]

lgtm

[keybuk, 2013-04-18 15:33:54]

On 2013/04/18 06:57:10, deymo wrote:
> On 2013/04/18 06:17:09, keybuk wrote:
> > This CL should probably add the appropriate line of code to
> > BluetoothDeviceExperimentalChromeOS that makes this work
> 
> This does the implementation of the new KeysEntered function. The usage of
that
> function will go in a separate CL.

Can you give a good reason? It's a small enough change in code you're already
nearly touching - it doesn't seem worth an entirely new CL just to hook this up

[deymo, 2013-04-18 16:37:16]

> Can you give a good reason? It's a small enough change in code you're already
> nearly touching - it doesn't seem worth an entirely new CL just to hook this
up

Sure, it's not about the size of the CL, is about different logical parts of the
code. As you mention in the bug, this work wasn't blocked on a particular
platform, so this CL is the common part to all the platforms. I don't a reason
to not commit the common part of the code based on the lack of implementation
for a given platform.

[keybuk, 2013-04-18 17:14:50]

On 2013/04/18 16:37:16, deymo wrote:
> > Can you give a good reason? It's a small enough change in code you're
already
> > nearly touching - it doesn't seem worth an entirely new CL just to hook this
> up
> 
> Sure, it's not about the size of the CL, is about different logical parts of
the
> code. As you mention in the bug, this work wasn't blocked on a particular
> platform, so this CL is the common part to all the platforms. I don't a reason
> to not commit the common part of the code based on the lack of implementation
> for a given platform.

The other way of looking at that is since the work that would have blocked it is
now in - there's no reason to commit code without an implementation

Also since this adds a method to BluetoothDevice::PairingDelegate, and there's
one of those in bluetooth_experimental_chromeos_unittests.cc - you're going to
have to change that file annnnyway ;-)

Adding an empty method to a test case is bad mojo - so you will probably add a
entered_count_ and last_entered_ or something so at least the test delegate
method is doing something.

So since you're doing that, you should add an EXPECT_EQ or two to the
appropriate test case (hint: there's a TODO waiting for it)

And then since you're testing it, you may as well add the _one_line_of_code_ to
hook it up in the implementation

[deymo, 2013-04-18 23:15:25]

PTAL

[keybuk, 2013-04-18 23:50:36]

https://codereview.chromium.org/13870020/diff/25001/device/bluetooth/bluetoot...
File device/bluetooth/bluetooth_experimental_chromeos_unittest.cc (right):

https://codereview.chromium.org/13870020/diff/25001/device/bluetooth/bluetoot...
device/bluetooth/bluetooth_experimental_chromeos_unittest.cc:1237: 
Doesn't check the KeysEntered call here.

Should verify that last_entered is 0 and keys_entered_count is 1

https://codereview.chromium.org/13870020/diff/25001/device/bluetooth/bluetoot...
device/bluetooth/bluetooth_experimental_chromeos_unittest.cc:1244:
EXPECT_EQ((uint32_t)i, pairing_delegate.last_entered_);
style: use static_cast<>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Casting

(probably much cleaner to make "i" uint32_t)

[deymo, 2013-04-19 01:35:00]

Please, take yet another look.

https://codereview.chromium.org/13870020/diff/25001/device/bluetooth/bluetoot...
File device/bluetooth/bluetooth_experimental_chromeos_unittest.cc (right):

https://codereview.chromium.org/13870020/diff/25001/device/bluetooth/bluetoot...
device/bluetooth/bluetooth_experimental_chromeos_unittest.cc:1237: 
On 2013/04/18 23:50:36, keybuk wrote:
> Doesn't check the KeysEntered call here.
> 
> Should verify that last_entered is 0 and keys_entered_count is 1

Done.

https://codereview.chromium.org/13870020/diff/25001/device/bluetooth/bluetoot...
device/bluetooth/bluetooth_experimental_chromeos_unittest.cc:1244:
EXPECT_EQ((uint32_t)i, pairing_delegate.last_entered_);
On 2013/04/18 23:50:36, keybuk wrote:
> style: use static_cast<>
> http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Casting
> 
> (probably much cleaner to make "i" uint32_t)

Done.

[keybuk, 2013-04-19 01:58:24]

lgtm

[commit-bot: I haz the power, 2013-04-19 07:13:24]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/deymo@chromium.org/13870020/34001

[commit-bot: I haz the power, 2013-04-19 07:15:10]

Step "update" is always a major failure.
Look at the try server FAQ for more details.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=ios_dbg_si...

[commit-bot: I haz the power, 2013-04-19 08:00:19]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/deymo@chromium.org/13870020/34001

[commit-bot: I haz the power, 2013-04-19 09:16:28]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/deymo@chromium.org/13870020/41003

[commit-bot: I haz the power, 2013-04-19 13:08:18]

Change committed as 195153