Implement debugging local unlock keys in chrome://proximity-auth.

Implement debugging local unlock keys in chrome://proximity-auth.

This CL also updates some older log messages to use the new PA_LOG macro, and removes a metadata check that is not being set on the GMS Core side.

BUG=409158
TEST=manual

Committed: https://crrev.com/da506c83c44c94f391a282e94fa4ef417861611d
Cr-Commit-Position: refs/heads/master@{#341634}

[Tim Song, 2015-07-30 00:25:03]

Patchset #2 (id:20001) has been deleted

[Tim Song, 2015-07-30 00:25:08]

Patchset #1 (id:1) has been deleted

[Tim Song, 2015-07-30 00:25:28]

tengs@chromium.org changed reviewers:
+ sacomoto@chromium.org

[sacomoto, 2015-07-31 15:56:27]

https://codereview.chromium.org/1264703003/diff/40001/components/proximity_au...
File components/proximity_auth/bluetooth_connection.cc (right):

https://codereview.chromium.org/1264703003/diff/40001/components/proximity_au...
components/proximity_auth/bluetooth_connection.cc:30: if (status() !=
DISCONNECTED)
Why do you need this? |Disconnect()| already does nothing when |status() ==
DISONNECTED|.

https://codereview.chromium.org/1264703003/diff/40001/components/proximity_au...
File components/proximity_auth/device_to_device_initiator_operations.cc (right):

https://codereview.chromium.org/1264703003/diff/40001/components/proximity_au...
components/proximity_auth/device_to_device_initiator_operations.cc:222:
gcm_metadata.type() != cryptauth::UNLOCK_KEY_SIGNED_CHALLENGE) {
Add on the CL description that you are also fixing this bug.

https://codereview.chromium.org/1264703003/diff/40001/components/proximity_au...
File components/proximity_auth/webui/proximity_auth_webui_handler.cc (right):

https://codereview.chromium.org/1264703003/diff/40001/components/proximity_au...
components/proximity_auth/webui/proximity_auth_webui_handler.cc:196:
PA_LOG(INFO) << "New unlock keys obatined after device sync:\n"
nit: s/obatined/obtained/

https://codereview.chromium.org/1264703003/diff/40001/components/proximity_au...
components/proximity_auth/webui/proximity_auth_webui_handler.cc:255: connection
= client_->connection();
Refactor this in a |GetConnection()| method. 

Also add DCHECK(!connection) inside the if to make clear that
|bluetooth_connection_| and
|client_->connection()| cannot point to two different (non-null) instances.

https://codereview.chromium.org/1264703003/diff/40001/components/proximity_au...
components/proximity_auth/webui/proximity_auth_webui_handler.cc:442:
secure_message_delegate_.reset();
nit: You don't need to reset it here, as it's already rested in the only place
it's created.

https://codereview.chromium.org/1264703003/diff/40001/components/proximity_au...
components/proximity_auth/webui/proximity_auth_webui_handler.cc:452:
device::BluetoothUUID("704EE561-3782-405A-A14B-2D47A2DDCDDF")));
nit: Use a constant.

https://codereview.chromium.org/1264703003/diff/40001/components/proximity_au...
components/proximity_auth/webui/proximity_auth_webui_handler.cc:456: // it is
not already known to the local device.
What do you mean by known? Contained in |BluetoothAdapter::GetDevices()|? This
seems like a hacky alternative to scan for a device
(BluetoothAdapter::StartDiscoverySession), is it the case?

Anyway, I'm ok with this approach, as this class is used for debugging. I just
want to understand this better.

https://codereview.chromium.org/1264703003/diff/40001/components/proximity_au...
components/proximity_auth/webui/proximity_auth_webui_handler.cc:477:
PA_LOG(WARNING) << "Failed to seek device by address: "
nit: s/by/with/.

https://codereview.chromium.org/1264703003/diff/40001/components/proximity_au...
components/proximity_auth/webui/proximity_auth_webui_handler.cc:487: // the last
authentication message sent over the connection.
You should make this comment more explicit. Add: |client_| registers itself as
an observer of |bluetooth_connection_|, so creating it synchronously would
trigger |OnSendComplete()| as an observer call for |client_|.

https://codereview.chromium.org/1264703003/diff/40001/components/proximity_au...
components/proximity_auth/webui/proximity_auth_webui_handler.cc:488:
base::ThreadTaskRunnerHandle::Get()->PostDelayedTask(
nit: Use |PostTask()| instead of |PostDelayedTask()| with zero delay.

https://codereview.chromium.org/1264703003/diff/40001/components/proximity_au...
components/proximity_auth/webui/proximity_auth_webui_handler.cc:538: connection
= client_->connection();
Refactor this in a |GetConnection()| method.

[Tim Song, 2015-07-31 22:48:44]

https://codereview.chromium.org/1264703003/diff/40001/components/proximity_au...
File components/proximity_auth/bluetooth_connection.cc (right):

https://codereview.chromium.org/1264703003/diff/40001/components/proximity_au...
components/proximity_auth/bluetooth_connection.cc:30: if (status() !=
DISCONNECTED)
On 2015/07/31 15:56:26, sacomoto wrote:
> Why do you need this? |Disconnect()| already does nothing when |status() ==
> DISONNECTED|.

This spams the logs as the warning will be printed for every destructor call. On
the other hand, we want to keep the warning so we can detect when multiple
Disconnect() calls are being made.

https://codereview.chromium.org/1264703003/diff/40001/components/proximity_au...
File components/proximity_auth/device_to_device_initiator_operations.cc (right):

https://codereview.chromium.org/1264703003/diff/40001/components/proximity_au...
components/proximity_auth/device_to_device_initiator_operations.cc:222:
gcm_metadata.type() != cryptauth::UNLOCK_KEY_SIGNED_CHALLENGE) {
On 2015/07/31 15:56:26, sacomoto wrote:
> Add on the CL description that you are also fixing this bug. 

Done.

https://codereview.chromium.org/1264703003/diff/40001/components/proximity_au...
File components/proximity_auth/webui/proximity_auth_webui_handler.cc (right):

https://codereview.chromium.org/1264703003/diff/40001/components/proximity_au...
components/proximity_auth/webui/proximity_auth_webui_handler.cc:196:
PA_LOG(INFO) << "New unlock keys obatined after device sync:\n"
On 2015/07/31 15:56:27, sacomoto wrote:
> nit: s/obatined/obtained/

Done.

https://codereview.chromium.org/1264703003/diff/40001/components/proximity_au...
components/proximity_auth/webui/proximity_auth_webui_handler.cc:196:
PA_LOG(INFO) << "New unlock keys obatined after device sync:\n"
On 2015/07/31 15:56:27, sacomoto wrote:
> nit: s/obatined/obtained/

Done.

https://codereview.chromium.org/1264703003/diff/40001/components/proximity_au...
components/proximity_auth/webui/proximity_auth_webui_handler.cc:255: connection
= client_->connection();
On 2015/07/31 15:56:27, sacomoto wrote:
> Refactor this in a |GetConnection()| method. 
> 
> Also add DCHECK(!connection) inside the if to make clear that
> |bluetooth_connection_| and
> |client_->connection()| cannot point to two different (non-null) instances.

Done.

https://codereview.chromium.org/1264703003/diff/40001/components/proximity_au...
components/proximity_auth/webui/proximity_auth_webui_handler.cc:442:
secure_message_delegate_.reset();
On 2015/07/31 15:56:27, sacomoto wrote:
> nit: You don't need to reset it here, as it's already rested in the only place
> it's created.

Done.

https://codereview.chromium.org/1264703003/diff/40001/components/proximity_au...
components/proximity_auth/webui/proximity_auth_webui_handler.cc:452:
device::BluetoothUUID("704EE561-3782-405A-A14B-2D47A2DDCDDF")));
On 2015/07/31 15:56:27, sacomoto wrote:
> nit: Use a constant.

Done.

https://codereview.chromium.org/1264703003/diff/40001/components/proximity_au...
components/proximity_auth/webui/proximity_auth_webui_handler.cc:456: // it is
not already known to the local device.
On 2015/07/31 15:56:26, sacomoto wrote:
> What do you mean by known? Contained in |BluetoothAdapter::GetDevices()|? This
> seems like a hacky alternative to scan for a device
> (BluetoothAdapter::StartDiscoverySession), is it the case?
> 
> Anyway, I'm ok with this approach, as this class is used for debugging. I just
> want to understand this better. 

This is actually another way to "discover" a Bluetooth device without performing
discovery.

If you know the address before hand, discovery is unnecessary and just adds
additional latency.

https://codereview.chromium.org/1264703003/diff/40001/components/proximity_au...
components/proximity_auth/webui/proximity_auth_webui_handler.cc:487: // the last
authentication message sent over the connection.
On 2015/07/31 15:56:27, sacomoto wrote:
> You should make this comment more explicit. Add: |client_| registers itself as
> an observer of |bluetooth_connection_|, so creating it synchronously would
> trigger |OnSendComplete()| as an observer call for |client_|.

Done.

https://codereview.chromium.org/1264703003/diff/40001/components/proximity_au...
components/proximity_auth/webui/proximity_auth_webui_handler.cc:488:
base::ThreadTaskRunnerHandle::Get()->PostDelayedTask(
On 2015/07/31 15:56:27, sacomoto wrote:
> nit: Use |PostTask()| instead of |PostDelayedTask()| with zero delay.

Done.

https://codereview.chromium.org/1264703003/diff/40001/components/proximity_au...
components/proximity_auth/webui/proximity_auth_webui_handler.cc:538: connection
= client_->connection();
On 2015/07/31 15:56:27, sacomoto wrote:
> Refactor this in a |GetConnection()| method. 

Done.

[sacomoto, 2015-08-03 12:06:14]

LGTM. Thanks!

https://codereview.chromium.org/1264703003/diff/40001/components/proximity_au...
File components/proximity_auth/bluetooth_connection.cc (right):

https://codereview.chromium.org/1264703003/diff/40001/components/proximity_au...
components/proximity_auth/bluetooth_connection.cc:30: if (status() !=
DISCONNECTED)
On 2015/07/31 22:48:43, Tim Song wrote:
> On 2015/07/31 15:56:26, sacomoto wrote:
> > Why do you need this? |Disconnect()| already does nothing when |status() ==
> > DISONNECTED|.
> 
> This spams the logs as the warning will be printed for every destructor call.
On
> the other hand, we want to keep the warning so we can detect when multiple
> Disconnect() calls are being made.

Ok. That makes sense.

https://codereview.chromium.org/1264703003/diff/40001/components/proximity_au...
File components/proximity_auth/webui/proximity_auth_webui_handler.cc (right):

https://codereview.chromium.org/1264703003/diff/40001/components/proximity_au...
components/proximity_auth/webui/proximity_auth_webui_handler.cc:456: // it is
not already known to the local device.
On 2015/07/31 22:48:44, Tim Song wrote:
> On 2015/07/31 15:56:26, sacomoto wrote:
> > What do you mean by known? Contained in |BluetoothAdapter::GetDevices()|?
This
> > seems like a hacky alternative to scan for a device
> > (BluetoothAdapter::StartDiscoverySession), is it the case?
> > 
> > Anyway, I'm ok with this approach, as this class is used for debugging. I
just
> > want to understand this better. 
> 
> This is actually another way to "discover" a Bluetooth device without
performing
> discovery.
> 
> If you know the address before hand, discovery is unnecessary and just adds
> additional latency.

Ok.

[Tim Song, 2015-08-03 22:35:06]

The CQ bit was checked by tengs@chromium.org

[commit-bot: I haz the power, 2015-08-03 22:35:39]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1264703003/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1264703003/60001

[commit-bot: I haz the power, 2015-08-03 22:49:14]

Committed patchset #2 (id:60001)

[commit-bot: I haz the power, 2015-08-03 22:49:54]

Patchset 2 (id:??) landed as
https://crrev.com/da506c83c44c94f391a282e94fa4ef417861611d
Cr-Commit-Position: refs/heads/master@{#341634}