bluetooth: Add control for reading/writing of characteristics to internals page.

bluetooth: Add control for reading/writing of characteristics to internals page.

Adds ValueControl to allow a user to read values from and write values to
    characteristics.
Adds Permission enum to device.mojom for allowing reading/writing based on
    permissions/properties specified by the characteristic.
Adds WriteValueForCharacteristic and ReadValueForCharacteristic to Device Mojo
    service.

GIF: https://goo.gl/photos/izDvvyH7d56Uh4kE7

BUG=651282
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2627243002
Cr-Commit-Position: refs/heads/master@{#446526}
Committed: https://chromium.googlesource.com/chromium/src/+/91de29301d3e16e93a3789b737a4aa44af4a0bd0

[mbrunson, 2017-01-12 02:08:18]

Description was changed from

==========
bluetooth: Add control for reading/writing of characteristics to internals page.

Adds ValueControl to allow a user to read values from and write values to
    characteristics.
Adds WriteValueForCharacteristic and ReadValueForCharacteristic to Device Mojo
    service.

BUG=651282
==========

to

==========
bluetooth: Add control for reading/writing of characteristics to internals page.

Adds ValueControl to allow a user to read values from and write values to
    characteristics.
Adds WriteValueForCharacteristic and ReadValueForCharacteristic to Device Mojo
    service.

BUG=651282
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[mbrunson, 2017-01-13 22:36:41]

The CQ bit was checked by mbrunson@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-13 22:37:14]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[mbrunson, 2017-01-20 00:05:20]

The CQ bit was checked by mbrunson@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-20 00:06:12]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[mbrunson, 2017-01-20 00:11:46]

Description was changed from

==========
bluetooth: Add control for reading/writing of characteristics to internals page.

Adds ValueControl to allow a user to read values from and write values to
    characteristics.
Adds WriteValueForCharacteristic and ReadValueForCharacteristic to Device Mojo
    service.

BUG=651282
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
bluetooth: Add control for reading/writing of characteristics to internals page.

Adds ValueControl to allow a user to read values from and write values to
    characteristics.
Adds WriteValueForCharacteristic and ReadValueForCharacteristic to Device Mojo
    service.

GIF: https://goo.gl/photos/izDvvyH7d56Uh4kE7

BUG=651282
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[mbrunson, 2017-01-20 00:14:00]

Description was changed from

==========
bluetooth: Add control for reading/writing of characteristics to internals page.

Adds ValueControl to allow a user to read values from and write values to
    characteristics.
Adds WriteValueForCharacteristic and ReadValueForCharacteristic to Device Mojo
    service.

GIF: https://goo.gl/photos/izDvvyH7d56Uh4kE7

BUG=651282
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
bluetooth: Add control for reading/writing of characteristics to internals page.

Adds ValueControl to allow a user to read values from and write values to
    characteristics.
Adds Permission enum to device.mojom for allowing reading/writing based on
    permissions/properties specified by the characteristic.
Adds WriteValueForCharacteristic and ReadValueForCharacteristic to Device Mojo
    service.

GIF: https://goo.gl/photos/izDvvyH7d56Uh4kE7

BUG=651282
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[mbrunson, 2017-01-20 00:17:22]

mbrunson@chromium.org changed reviewers:
+ ortuno@chromium.org, scheib@chromium.org

[commit-bot: I haz the power, 2017-01-20 01:58:21]

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

[commit-bot: I haz the power, 2017-01-20 01:58:22]

Dry run: This issue passed the CQ dry run.

[ortuno, 2017-01-20 03:45:22]

https://codereview.chromium.org/2627243002/diff/100001/chrome/browser/resourc...
File chrome/browser/resources/bluetooth_internals/value_control.js (right):

https://codereview.chromium.org/2627243002/diff/100001/chrome/browser/resourc...
chrome/browser/resources/bluetooth_internals/value_control.js:110: //
TODO(crbug.com/682856): Remove once permissions for characteristics on
We should always use properties. Neither BlueZ nor macOS expose permissions.

https://codereview.chromium.org/2627243002/diff/100001/device/bluetooth/devic...
File device/bluetooth/device.cc (right):

https://codereview.chromium.org/2627243002/diff/100001/device/bluetooth/devic...
device/bluetooth/device.cc:138: pending_services_requests_.push_back(base::Bind(
q: Why do we queue instead of just returning an error?

https://codereview.chromium.org/2627243002/diff/100001/device/bluetooth/devic...
device/bluetooth/device.cc:224: DCHECK(service);
There are no guarantees, in code or docs, that the service_id/characteristic_id
you were passed corresponds to an actual service/characteristic. We should
return an error.

Even if the service_id corresponds to an actual service there are no guarantees
that it will still be there once discovery is complete. I think we shouldn't
bother with a queue and just return an error.

https://codereview.chromium.org/2627243002/diff/100001/device/bluetooth/publi...
File device/bluetooth/public/interfaces/device.mojom (right):

https://codereview.chromium.org/2627243002/diff/100001/device/bluetooth/publi...
device/bluetooth/public/interfaces/device.mojom:58:
WRITE_ENCRYPTED_AUTHENTICATED = 32
So sad that we have to manually write these... Are there any opened issues to
fix this?

https://codereview.chromium.org/2627243002/diff/100001/device/bluetooth/publi...
device/bluetooth/public/interfaces/device.mojom:97: array<uint8> value;
I would rename this to last_known_value.

https://codereview.chromium.org/2627243002/diff/100001/device/bluetooth/publi...
device/bluetooth/public/interfaces/device.mojom:118: (GattResult result,
array<uint8> value);
shouldn't value be optional?

https://codereview.chromium.org/2627243002/diff/100001/device/bluetooth/publi...
device/bluetooth/public/interfaces/device.mojom:122:
WriteValueForCharacteristic(array<uint8> value, string service_id,
optional: put value at the end.

[mbrunson, 2017-01-21 01:49:01]

https://codereview.chromium.org/2627243002/diff/100001/chrome/browser/resourc...
File chrome/browser/resources/bluetooth_internals/value_control.js (right):

https://codereview.chromium.org/2627243002/diff/100001/chrome/browser/resourc...
chrome/browser/resources/bluetooth_internals/value_control.js:110: //
TODO(crbug.com/682856): Remove once permissions for characteristics on
On 2017/01/20 03:45:22, ortuno wrote:
> We should always use properties. Neither BlueZ nor macOS expose permissions.

Done.

https://codereview.chromium.org/2627243002/diff/100001/device/bluetooth/devic...
File device/bluetooth/device.cc (right):

https://codereview.chromium.org/2627243002/diff/100001/device/bluetooth/devic...
device/bluetooth/device.cc:224: DCHECK(service);
On 2017/01/20 03:45:22, ortuno wrote:
> There are no guarantees, in code or docs, that the
service_id/characteristic_id
> you were passed corresponds to an actual service/characteristic. We should
> return an error.
> 
> Even if the service_id corresponds to an actual service there are no
guarantees
> that it will still be there once discovery is complete. I think we shouldn't
> bother with a queue and just return an error.

Ok. Done.

https://codereview.chromium.org/2627243002/diff/100001/device/bluetooth/publi...
File device/bluetooth/public/interfaces/device.mojom (right):

https://codereview.chromium.org/2627243002/diff/100001/device/bluetooth/publi...
device/bluetooth/public/interfaces/device.mojom:58:
WRITE_ENCRYPTED_AUTHENTICATED = 32
On 2017/01/20 03:45:22, ortuno wrote:
> So sad that we have to manually write these... Are there any opened issues to
> fix this?

There are no open issues for this. rockot has told me that the enum has to have
a defined IPC::ParamTraits to even begin the process for this feature. According
to the docs[1], you can only use the matching enum in C++, so JavaScript would
be a no-go.

[1]
https://www.chromium.org/developers/design-documents/mojo/type-mapping#TOC-Us...

https://codereview.chromium.org/2627243002/diff/100001/device/bluetooth/publi...
device/bluetooth/public/interfaces/device.mojom:97: array<uint8> value;
On 2017/01/20 03:45:22, ortuno wrote:
> I would rename this to last_known_value.

Done.

https://codereview.chromium.org/2627243002/diff/100001/device/bluetooth/publi...
device/bluetooth/public/interfaces/device.mojom:118: (GattResult result,
array<uint8> value);
On 2017/01/20 03:45:22, ortuno wrote:
> shouldn't value be optional?

Done.

https://codereview.chromium.org/2627243002/diff/100001/device/bluetooth/publi...
device/bluetooth/public/interfaces/device.mojom:122:
WriteValueForCharacteristic(array<uint8> value, string service_id,
On 2017/01/20 03:45:22, ortuno wrote:
> optional: put value at the end.

Done.

[ortuno, 2017-01-22 22:38:01]

https://codereview.chromium.org/2627243002/diff/100001/device/bluetooth/publi...
File device/bluetooth/public/interfaces/device.mojom (right):

https://codereview.chromium.org/2627243002/diff/100001/device/bluetooth/publi...
device/bluetooth/public/interfaces/device.mojom:58:
WRITE_ENCRYPTED_AUTHENTICATED = 32
On 2017/01/21 at 01:49:00, mbrunson wrote:
> On 2017/01/20 03:45:22, ortuno wrote:
> > So sad that we have to manually write these... Are there any opened issues
to
> > fix this?
> 
> There are no open issues for this. rockot has told me that the enum has to
have a defined IPC::ParamTraits to even begin the process for this feature.
According to the docs[1], you can only use the matching enum in C++, so
JavaScript would be a no-go.
> 
> [1]
https://www.chromium.org/developers/design-documents/mojo/type-mapping#TOC-Us...

Ah I should have been more explicit. I'm sad that we have to manually write 0,
1, 2, 4, 8, etc. Are there any issues open that would simplify that?

https://codereview.chromium.org/2627243002/diff/160001/chrome/browser/resourc...
File chrome/browser/resources/bluetooth_internals/value_control.js (right):

https://codereview.chromium.org/2627243002/diff/160001/chrome/browser/resourc...
chrome/browser/resources/bluetooth_internals/value_control.js:133: }, '');
Why not make this '0x'? instead of having checking for the first element?

https://codereview.chromium.org/2627243002/diff/160001/chrome/browser/resourc...
chrome/browser/resources/bluetooth_internals/value_control.js:178: value =
value.replace('0x', '');
We should show an error if the string in value is not of the right format. OK to
do in follow up.

https://codereview.chromium.org/2627243002/diff/160001/chrome/browser/resourc...
chrome/browser/resources/bluetooth_internals/value_control.js:179: return
Array.from(value).reduce(function(result, value, index) {
I appreciate the use of reduce but I think that just makes the code harder to
follow. I would just do:

var result = [];
for (var i = 2; i < value.length; i += 2) {
  result.push(parseInt(value.substr(i, 2), 16));
}
return result;

https://codereview.chromium.org/2627243002/diff/160001/chrome/browser/resourc...
chrome/browser/resources/bluetooth_internals/value_control.js:184: return
result;
What's our current policy on tests? Should we have tests to make sure the
correct values are being read/written?

https://codereview.chromium.org/2627243002/diff/160001/chrome/browser/resourc...
chrome/browser/resources/bluetooth_internals/value_control.js:206:
device_broker.connectToDevice(this.deviceAddress_).then(function(device) {
Why do we need to connect? Can't we just check if there is a connected devices
already and use that?

https://codereview.chromium.org/2627243002/diff/160001/device/bluetooth/devic...
File device/bluetooth/device.cc (right):

https://codereview.chromium.org/2627243002/diff/160001/device/bluetooth/devic...
device/bluetooth/device.cc:134: callback.Run(mojom::GattResult::FAILED,
base::nullopt /* value */);
Would it be too much to add a specific error for this?

https://codereview.chromium.org/2627243002/diff/160001/device/bluetooth/publi...
File device/bluetooth/public/interfaces/gatt_result_type_converter.h (right):

https://codereview.chromium.org/2627243002/diff/160001/device/bluetooth/publi...
device/bluetooth/public/interfaces/gatt_result_type_converter.h:18: struct
TypeConverter<bluetooth::mojom::GattResult,
Why are we not using a typemap?

[mbrunson, 2017-01-24 00:25:31]

https://codereview.chromium.org/2627243002/diff/100001/device/bluetooth/publi...
File device/bluetooth/public/interfaces/device.mojom (right):

https://codereview.chromium.org/2627243002/diff/100001/device/bluetooth/publi...
device/bluetooth/public/interfaces/device.mojom:58:
WRITE_ENCRYPTED_AUTHENTICATED = 32
On 2017/01/22 22:38:00, ortuno wrote:
> On 2017/01/21 at 01:49:00, mbrunson wrote:
> > On 2017/01/20 03:45:22, ortuno wrote:
> > > So sad that we have to manually write these... Are there any opened issues
> to
> > > fix this?
> > 
> > There are no open issues for this. rockot has told me that the enum has to
> have a defined IPC::ParamTraits to even begin the process for this feature.
> According to the docs[1], you can only use the matching enum in C++, so
> JavaScript would be a no-go.
> > 
> > [1]
>
https://www.chromium.org/developers/design-documents/mojo/type-mapping#TOC-Us...
> 
> Ah I should have been more explicit. I'm sad that we have to manually write 0,
> 1, 2, 4, 8, etc. Are there any issues open that would simplify that?

I didn't find any open issues for this, so I made one: crbug.com/684168

https://codereview.chromium.org/2627243002/diff/160001/chrome/browser/resourc...
File chrome/browser/resources/bluetooth_internals/value_control.js (right):

https://codereview.chromium.org/2627243002/diff/160001/chrome/browser/resourc...
chrome/browser/resources/bluetooth_internals/value_control.js:133: }, '');
On 2017/01/22 22:38:01, ortuno wrote:
> Why not make this '0x'? instead of having checking for the first element?

What if |value_| is empty? I didn't want '0x' printing by itself, so I just put
an if statement in the reduce function.

Alternatively, I could put the if statement on the outside and put the reduce in
an else statement.

https://codereview.chromium.org/2627243002/diff/160001/chrome/browser/resourc...
chrome/browser/resources/bluetooth_internals/value_control.js:178: value =
value.replace('0x', '');
On 2017/01/22 22:38:01, ortuno wrote:
> We should show an error if the string in value is not of the right format. OK
to
> do in follow up.

Done.

https://codereview.chromium.org/2627243002/diff/160001/chrome/browser/resourc...
chrome/browser/resources/bluetooth_internals/value_control.js:179: return
Array.from(value).reduce(function(result, value, index) {
On 2017/01/22 22:38:01, ortuno wrote:
> I appreciate the use of reduce but I think that just makes the code harder to
> follow. I would just do:
> 
> var result = [];
> for (var i = 2; i < value.length; i += 2) {
>   result.push(parseInt(value.substr(i, 2), 16));
> }
> return result;

Done.

https://codereview.chromium.org/2627243002/diff/160001/chrome/browser/resourc...
chrome/browser/resources/bluetooth_internals/value_control.js:184: return
result;
On 2017/01/22 22:38:01, ortuno wrote:
> What's our current policy on tests? Should we have tests to make sure the
> correct values are being read/written?

Added tests. Caught some issues. :-)

Done.

https://codereview.chromium.org/2627243002/diff/160001/chrome/browser/resourc...
chrome/browser/resources/bluetooth_internals/value_control.js:206:
device_broker.connectToDevice(this.deviceAddress_).then(function(device) {
On 2017/01/22 22:38:01, ortuno wrote:
> Why do we need to connect? Can't we just check if there is a connected devices
> already and use that?

The connectToDevice function does that internally. If a connection exists, the
existing DevicePtr is returned. If no connection exists, it creates one and
returns the new DevicePtr.

https://codereview.chromium.org/2627243002/diff/160001/device/bluetooth/devic...
File device/bluetooth/device.cc (right):

https://codereview.chromium.org/2627243002/diff/160001/device/bluetooth/devic...
device/bluetooth/device.cc:134: callback.Run(mojom::GattResult::FAILED,
base::nullopt /* value */);
On 2017/01/22 22:38:01, ortuno wrote:
> Would it be too much to add a specific error for this?

Done.

https://codereview.chromium.org/2627243002/diff/160001/device/bluetooth/publi...
File device/bluetooth/public/interfaces/gatt_result_type_converter.h (right):

https://codereview.chromium.org/2627243002/diff/160001/device/bluetooth/publi...
device/bluetooth/public/interfaces/gatt_result_type_converter.h:18: struct
TypeConverter<bluetooth::mojom::GattResult,
On 2017/01/22 22:38:01, ortuno wrote:
> Why are we not using a typemap?

This is a similar situation we had with mojom::ConnectResult where the mojom
values are a superset of the corresponding C++ enum. In this case, GattResult
has more values than GattErrorCode: SUCCESS (and possibly more such as
SERVICE_NOT_FOUND, CHARACTERISTIC_NOT_FOUND, or DESCRIPTOR_NOT_FOUND for better
communication of errors as you mentioned here [1]).

[1]
https://codereview.chromium.org/2627243002/diff/160001/device/bluetooth/devic...

[mbrunson, 2017-01-24 01:52:40]

mbrunson@chromium.org changed reviewers:
+ dpapad@chromium.org

[mbrunson, 2017-01-24 01:52:40]

dpapad:
 chrome/browser/browser_resources.grd
 chrome/browser/resources/bluetooth_internals/bluetooth_internals.css
 chrome/browser/resources/bluetooth_internals/bluetooth_internals.html
 chrome/browser/resources/bluetooth_internals/characteristic_list.js
 chrome/browser/resources/bluetooth_internals/value_control.js
 chrome/browser/ui/webui/bluetooth_internals/bluetooth_internals_ui.cc
 chrome/test/data/webui/bluetooth_internals_browsertest.js

[mbrunson, 2017-01-24 01:53:08]

dpapad: WebUI OWNERS review, please.

[ortuno, 2017-01-24 02:09:17]

A couple of suggestions. lgtm

https://codereview.chromium.org/2627243002/diff/220001/chrome/browser/resourc...
File chrome/browser/resources/bluetooth_internals/value_control.js (right):

https://codereview.chromium.org/2627243002/diff/220001/chrome/browser/resourc...
chrome/browser/resources/bluetooth_internals/value_control.js:184: throw new
Error('Expected value to start with "0x".');
How hard would it be to show a snackbar instead?

https://codereview.chromium.org/2627243002/diff/220001/chrome/test/data/webui...
File chrome/test/data/webui/bluetooth_internals_browsertest.js (right):

https://codereview.chromium.org/2627243002/diff/220001/chrome/test/data/webui...
chrome/test/data/webui/bluetooth_internals_browsertest.js:929: return;
Can you assert that the error is the one you expect?

https://codereview.chromium.org/2627243002/diff/220001/chrome/test/data/webui...
chrome/test/data/webui/bluetooth_internals_browsertest.js:932:
expectNotReached();
Sad that there is no expectThrow()

[mbrunson, 2017-01-24 04:21:54]

https://codereview.chromium.org/2627243002/diff/220001/chrome/browser/resourc...
File chrome/browser/resources/bluetooth_internals/value_control.js (right):

https://codereview.chromium.org/2627243002/diff/220001/chrome/browser/resourc...
chrome/browser/resources/bluetooth_internals/value_control.js:184: throw new
Error('Expected value to start with "0x".');
On 2017/01/24 02:09:17, ortuno wrote:
> How hard would it be to show a snackbar instead?

I put that code in the function that calls this.

https://codereview.chromium.org/2627243002/diff/220001/chrome/test/data/webui...
File chrome/test/data/webui/bluetooth_internals_browsertest.js (right):

https://codereview.chromium.org/2627243002/diff/220001/chrome/test/data/webui...
chrome/test/data/webui/bluetooth_internals_browsertest.js:929: return;
On 2017/01/24 02:09:17, ortuno wrote:
> Can you assert that the error is the one you expect?

Done.

https://codereview.chromium.org/2627243002/diff/220001/chrome/test/data/webui...
chrome/test/data/webui/bluetooth_internals_browsertest.js:932:
expectNotReached();
On 2017/01/24 02:09:17, ortuno wrote:
> Sad that there is no expectThrow()

Added it to test_api.js. Done.

[mbrunson, 2017-01-24 04:22:44]

The CQ bit was checked by mbrunson@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-24 04:23:13]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-24 04:25:14]

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

[commit-bot: I haz the power, 2017-01-24 04:25:15]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[dpapad, 2017-01-24 19:52:30]

https://codereview.chromium.org/2627243002/diff/240001/chrome/browser/resourc...
File chrome/browser/resources/bluetooth_internals/value_control.js (right):

https://codereview.chromium.org/2627243002/diff/240001/chrome/browser/resourc...
chrome/browser/resources/bluetooth_internals/value_control.js:54: this.value_ =
this.convertValueToArray_(this.valueInput_.value);
Why are we converting the input to an array, and then using Array#reduce? Can't
we process it as a string instead? For example to convert to Hex by doing
this.valueInput_.value.toString(16).

https://codereview.chromium.org/2627243002/diff/240001/chrome/browser/resourc...
chrome/browser/resources/bluetooth_internals/value_control.js:129: case
'Hexadecimal':
We are repeating those string literals multiple times in this file. Can we do
the following instead?

1. Change VALUE_DATA_TYPES to an enum instead of an array.

/** @enum {string} */
var ValueDataTypes = {
  HEXADECIMAL: 'Hexadecimal',
  ...
};

2. Use ValueDataTypes.HEXADECIMAL here and elsewhere (similar for other values).
3. At line 62, use

Object.keys(ValueDataTypes).forEach(function(key) {
  var type = ValueDataTypes[key];
  ...
}, this);

https://codereview.chromium.org/2627243002/diff/240001/chrome/browser/resourc...
chrome/browser/resources/bluetooth_internals/value_control.js:133: if (index ===
0)
Nit:

return index === 0 ? '0x' + answer : answer;

https://codereview.chromium.org/2627243002/diff/240001/chrome/browser/resourc...
chrome/browser/resources/bluetooth_internals/value_control.js:148:
function(result, value, index) {
Nit:
return result + value.toString() +
    (index === this.value_.length - 1 ? '' : '-');

https://codereview.chromium.org/2627243002/diff/240001/chrome/browser/resourc...
chrome/browser/resources/bluetooth_internals/value_control.js:180: var type =
this.typeSelect_.selectedOptions[0].value;
Isn't this the same as this.typeSelect_.value?

[mbrunson, 2017-01-24 23:14:53]

The CQ bit was checked by mbrunson@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-24 23:15:55]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[mbrunson, 2017-01-24 23:21:51]

mbrunson@chromium.org changed reviewers:
+ dcheng@chromium.org

[mbrunson, 2017-01-24 23:21:52]

Mojom Review, please

dcheng:
 device/bluetooth/public/interfaces/device.mojom
 device/bluetooth/public/interfaces/gatt_result_type_converter.h

Also, have we come up with a replacement for type converters yet? We originally
put one in because we couldn't use a typemap for a mojom enum that has more
values than the matching C++ version. Maybe I should use EnumTraits directly?
Let me know what you think.

https://codereview.chromium.org/2627243002/diff/240001/chrome/browser/resourc...
File chrome/browser/resources/bluetooth_internals/value_control.js (right):

https://codereview.chromium.org/2627243002/diff/240001/chrome/browser/resourc...
chrome/browser/resources/bluetooth_internals/value_control.js:54: this.value_ =
this.convertValueToArray_(this.valueInput_.value);
On 2017/01/24 19:52:30, dpapad wrote:
> Why are we converting the input to an array, and then using Array#reduce?
Can't
> we process it as a string instead? For example to convert to Hex by doing
> this.valueInput_.value.toString(16).

Having the array seemed to be a good middle ground for both communication with
the Bluetooth API and conversions between formats.

The Bluetooth system expects an array of 8-bit integers for a value. I thought
it would be more complicated to check for the previous format and convert the
value accordingly when the user requested a write. If I type a value as hex
(e.g. 0x6162), I don't want the UTF-8 version of that to be "0x6162". I want it
to be the UTF-8 equivalent of that hex string (i.e. "ab"). This is preferred
because when a user requests a read from the Bluetooth system, they may want to
view the same value in different formats.

https://codereview.chromium.org/2627243002/diff/240001/chrome/browser/resourc...
chrome/browser/resources/bluetooth_internals/value_control.js:129: case
'Hexadecimal':
On 2017/01/24 19:52:30, dpapad wrote:
> We are repeating those string literals multiple times in this file. Can we do
> the following instead?
> 
> 1. Change VALUE_DATA_TYPES to an enum instead of an array.
> 
> /** @enum {string} */
> var ValueDataTypes = {
>   HEXADECIMAL: 'Hexadecimal',
>   ...
> };
> 
> 2. Use ValueDataTypes.HEXADECIMAL here and elsewhere (similar for other
values).
> 3. At line 62, use
> 
> Object.keys(ValueDataTypes).forEach(function(key) {
>   var type = ValueDataTypes[key];
>   ...
> }, this);

Done.

https://codereview.chromium.org/2627243002/diff/240001/chrome/browser/resourc...
chrome/browser/resources/bluetooth_internals/value_control.js:133: if (index ===
0)
On 2017/01/24 19:52:30, dpapad wrote:
> Nit:
> 
> return index === 0 ? '0x' + answer : answer;

Done.

https://codereview.chromium.org/2627243002/diff/240001/chrome/browser/resourc...
chrome/browser/resources/bluetooth_internals/value_control.js:148:
function(result, value, index) {
On 2017/01/24 19:52:30, dpapad wrote:
> Nit:
> return result + value.toString() +
>     (index === this.value_.length - 1 ? '' : '-');

Done.

https://codereview.chromium.org/2627243002/diff/240001/chrome/browser/resourc...
chrome/browser/resources/bluetooth_internals/value_control.js:180: var type =
this.typeSelect_.selectedOptions[0].value;
On 2017/01/24 19:52:30, dpapad wrote:
> Isn't this the same as this.typeSelect_.value?

Done.

[dpapad, 2017-01-24 23:54:34]

https://codereview.chromium.org/2627243002/diff/240001/chrome/browser/resourc...
File chrome/browser/resources/bluetooth_internals/value_control.js (right):

https://codereview.chromium.org/2627243002/diff/240001/chrome/browser/resourc...
chrome/browser/resources/bluetooth_internals/value_control.js:54: this.value_ =
this.convertValueToArray_(this.valueInput_.value);
On 2017/01/24 at 23:21:52, mbrunson wrote:
> On 2017/01/24 19:52:30, dpapad wrote:
> > Why are we converting the input to an array, and then using Array#reduce?
Can't
> > we process it as a string instead? For example to convert to Hex by doing
> > this.valueInput_.value.toString(16).
> 
> Having the array seemed to be a good middle ground for both communication with
the Bluetooth API and conversions between formats.
> 
> The Bluetooth system expects an array of 8-bit integers for a value. I thought
it would be more complicated to check for the previous format and convert the
value accordingly when the user requested a write. If I type a value as hex
(e.g. 0x6162), I don't want the UTF-8 version of that to be "0x6162". I want it
to be the UTF-8 equivalent of that hex string (i.e. "ab"). This is preferred
because when a user requests a read from the Bluetooth system, they may want to
view the same value in different formats.

Trying to understand the tradeoffs here, but still I don't think I am familiar
enough with the rest of the code. Here is a suggestion that might help:

How about encapsulating all the value conversion related stuff to a dedicated
Value class, and decouple it from the rest of this UI code. The API would looks
something as follows

Value#toHex()
Value#toDecimal()
Value#toUtf8()
Value#toArray()

Internally in Value I would store the actual value in a single format (not
Array, probably I would choose UTF8), and then I would convert on-the-fly as
needed, depending on which to*() method was called. toArray() would only be
called when passing data to the Blueotooth API. WDYT?

[mbrunson, 2017-01-25 02:43:36]

https://codereview.chromium.org/2627243002/diff/240001/chrome/browser/resourc...
File chrome/browser/resources/bluetooth_internals/value_control.js (right):

https://codereview.chromium.org/2627243002/diff/240001/chrome/browser/resourc...
chrome/browser/resources/bluetooth_internals/value_control.js:54: this.value_ =
this.convertValueToArray_(this.valueInput_.value);
On 2017/01/24 23:54:34, dpapad wrote:
> On 2017/01/24 at 23:21:52, mbrunson wrote:
> > On 2017/01/24 19:52:30, dpapad wrote:
> > > Why are we converting the input to an array, and then using Array#reduce?
> Can't
> > > we process it as a string instead? For example to convert to Hex by doing
> > > this.valueInput_.value.toString(16).
> > 
> > Having the array seemed to be a good middle ground for both communication
with
> the Bluetooth API and conversions between formats.
> > 
> > The Bluetooth system expects an array of 8-bit integers for a value. I
thought
> it would be more complicated to check for the previous format and convert the
> value accordingly when the user requested a write. If I type a value as hex
> (e.g. 0x6162), I don't want the UTF-8 version of that to be "0x6162". I want
it
> to be the UTF-8 equivalent of that hex string (i.e. "ab"). This is preferred
> because when a user requests a read from the Bluetooth system, they may want
to
> view the same value in different formats.
> 
> Trying to understand the tradeoffs here, but still I don't think I am familiar
> enough with the rest of the code. Here is a suggestion that might help:
> 
> How about encapsulating all the value conversion related stuff to a dedicated
> Value class, and decouple it from the rest of this UI code. The API would
looks
> something as follows
> 
> Value#toHex()
> Value#toDecimal()
> Value#toUtf8()
> Value#toArray()
> 
> Internally in Value I would store the actual value in a single format (not
> Array, probably I would choose UTF8), and then I would convert on-the-fly as
> needed, depending on which to*() method was called. toArray() would only be
> called when passing data to the Blueotooth API. WDYT?
> 

Ok. I've added Value at the top of this file.

[mbrunson, 2017-01-25 02:43:49]

The CQ bit was checked by mbrunson@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-25 02:44:25]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-25 02:47:10]

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

[commit-bot: I haz the power, 2017-01-25 02:47:10]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[mbrunson, 2017-01-25 03:52:25]

The CQ bit was checked by mbrunson@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-25 03:52:52]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dpapad, 2017-01-25 19:20:07]

https://codereview.chromium.org/2627243002/diff/350001/chrome/browser/resourc...
File chrome/browser/resources/bluetooth_internals/value_control.js (right):

https://codereview.chromium.org/2627243002/diff/350001/chrome/browser/resourc...
chrome/browser/resources/bluetooth_internals/value_control.js:57: }, '');
Can we use the reduce's initial value instead of checking for index === 0?

return this.value_.reduce(function(result, value) {
  return result + ('0' + value.toString(16)).substr(-2);
}, '0x');

If you need to return the empty string when this.value_ is an empty array you
can add a check before the call to reduce().

https://codereview.chromium.org/2627243002/diff/350001/chrome/browser/resourc...
chrome/browser/resources/bluetooth_internals/value_control.js:131: return
parseInt(val);
Perhaps be more explicit, 
return parseInt(val, 10);

https://codereview.chromium.org/2627243002/diff/350001/chrome/browser/resourc...
chrome/browser/resources/bluetooth_internals/value_control.js:174: switch
(this.typeSelect_.value) {
Optional nit: Can we move this switch logic within Value class?

setValue = function(valueType, value) {
  switch (valueType) {
    ...
  }
},

...
// Then here simply do
this.valueInput_.addEventListener('change', function() {
  try {
    this.value_.setValue(this.typeSelect_.value, this.valueInput_.value);
  } catch (e) {
    Snackbar.show(e.message, SnackbarType.ERROR);
  }
}.bind(this));

Then you can make all setValueFrom* methods private if not called elsewhere.

https://codereview.chromium.org/2627243002/diff/350001/chrome/browser/resourc...
chrome/browser/resources/bluetooth_internals/value_control.js:260: var type =
this.typeSelect_.selectedOptions[0].value;
Equivalent to this.typeSelect_.value

https://codereview.chromium.org/2627243002/diff/350001/chrome/browser/resourc...
chrome/browser/resources/bluetooth_internals/value_control.js:262: switch (type)
{
Similar optional nit here. Perhaps move this switch logic in
Value#getAs(valueType). Then simply do

this.valueInput_.value = this.value_.getAs(this.typeSelect_.value);

https://codereview.chromium.org/2627243002/diff/350001/chrome/browser/resourc...
chrome/browser/resources/bluetooth_internals/value_control.js:346: var
errorString = Object.keys(GattResult).find(function(key) {
Can we make a helper getErrorString_(result) and re-use here and in readValue_?

https://codereview.chromium.org/2627243002/diff/350001/chrome/test/data/webui...
File chrome/test/data/webui/bluetooth_internals_browsertest.js (right):

https://codereview.chromium.org/2627243002/diff/350001/chrome/test/data/webui...
chrome/test/data/webui/bluetooth_internals_browsertest.js:843:
valueControl.typeSelect_.selectedIndex = 0;
The setup for this test is repeated in every single test case. Can we avoid the
repetition?

suite('BluetoothInternalsUnitTests', function() {
  var valueControl = null;
  ...
  setup(function() {
    valueControl = value_control.ValueControl();
    valueControl.load(device1.address, service1.id, characteristic1);
    valueControl.typeSelect_.selectedIndex = 0;
  });

  test('foo', function() {
    // Only change the index if it is not 0
    valueControl.typeSelect_.selectedIndex = 1;
   
    ... // assertions
  });
});

https://codereview.chromium.org/2627243002/diff/350001/chrome/test/data/webui...
chrome/test/data/webui/bluetooth_internals_browsertest.js:927: expectThrows(
Probably more readable as

expectThrows(
  function() { valueControl.value_.setValueFromHex('d0x'); },
  'Expected new value to start with "0x"');

[mbrunson, 2017-01-25 22:14:44]

https://codereview.chromium.org/2627243002/diff/350001/chrome/browser/resourc...
File chrome/browser/resources/bluetooth_internals/value_control.js (right):

https://codereview.chromium.org/2627243002/diff/350001/chrome/browser/resourc...
chrome/browser/resources/bluetooth_internals/value_control.js:57: }, '');
On 2017/01/25 19:20:06, dpapad wrote:
> Can we use the reduce's initial value instead of checking for index === 0?
> 
> return this.value_.reduce(function(result, value) {
>   return result + ('0' + value.toString(16)).substr(-2);
> }, '0x');
> 
> If you need to return the empty string when this.value_ is an empty array you
> can add a check before the call to reduce().

Done.

https://codereview.chromium.org/2627243002/diff/350001/chrome/browser/resourc...
chrome/browser/resources/bluetooth_internals/value_control.js:131: return
parseInt(val);
On 2017/01/25 19:20:07, dpapad wrote:
> Perhaps be more explicit, 
> return parseInt(val, 10);

Done.

https://codereview.chromium.org/2627243002/diff/350001/chrome/browser/resourc...
chrome/browser/resources/bluetooth_internals/value_control.js:174: switch
(this.typeSelect_.value) {
On 2017/01/25 19:20:06, dpapad wrote:
> Optional nit: Can we move this switch logic within Value class?
> 
> setValue = function(valueType, value) {
>   switch (valueType) {
>     ...
>   }
> },
> 
> ...
> // Then here simply do
> this.valueInput_.addEventListener('change', function() {
>   try {
>     this.value_.setValue(this.typeSelect_.value, this.valueInput_.value);
>   } catch (e) {
>     Snackbar.show(e.message, SnackbarType.ERROR);
>   }
> }.bind(this));
> 
> Then you can make all setValueFrom* methods private if not called elsewhere.

Done.

https://codereview.chromium.org/2627243002/diff/350001/chrome/browser/resourc...
chrome/browser/resources/bluetooth_internals/value_control.js:260: var type =
this.typeSelect_.selectedOptions[0].value;
On 2017/01/25 19:20:06, dpapad wrote:
> Equivalent to this.typeSelect_.value

Done.

https://codereview.chromium.org/2627243002/diff/350001/chrome/browser/resourc...
chrome/browser/resources/bluetooth_internals/value_control.js:262: switch (type)
{
On 2017/01/25 19:20:07, dpapad wrote:
> Similar optional nit here. Perhaps move this switch logic in
> Value#getAs(valueType). Then simply do
> 
> this.valueInput_.value = this.value_.getAs(this.typeSelect_.value);

Done.

https://codereview.chromium.org/2627243002/diff/350001/chrome/browser/resourc...
chrome/browser/resources/bluetooth_internals/value_control.js:346: var
errorString = Object.keys(GattResult).find(function(key) {
On 2017/01/25 19:20:07, dpapad wrote:
> Can we make a helper getErrorString_(result) and re-use here and in
readValue_?

Done.

https://codereview.chromium.org/2627243002/diff/350001/chrome/test/data/webui...
File chrome/test/data/webui/bluetooth_internals_browsertest.js (right):

https://codereview.chromium.org/2627243002/diff/350001/chrome/test/data/webui...
chrome/test/data/webui/bluetooth_internals_browsertest.js:843:
valueControl.typeSelect_.selectedIndex = 0;
On 2017/01/25 19:20:07, dpapad wrote:
> The setup for this test is repeated in every single test case. Can we avoid
the
> repetition?
> 
> suite('BluetoothInternalsUnitTests', function() {
>   var valueControl = null;
>   ...
>   setup(function() {
>     valueControl = value_control.ValueControl();
>     valueControl.load(device1.address, service1.id, characteristic1);
>     valueControl.typeSelect_.selectedIndex = 0;
>   });
> 
>   test('foo', function() {
>     // Only change the index if it is not 0
>     valueControl.typeSelect_.selectedIndex = 1;
>    
>     ... // assertions
>   });
> });

Done.

https://codereview.chromium.org/2627243002/diff/350001/chrome/test/data/webui...
chrome/test/data/webui/bluetooth_internals_browsertest.js:927: expectThrows(
On 2017/01/25 19:20:07, dpapad wrote:
> Probably more readable as
> 
> expectThrows(
>   function() { valueControl.value_.setValueFromHex('d0x'); },
>   'Expected new value to start with "0x"');

Done.

[dpapad, 2017-01-25 23:50:20]

LGTM for JS/webui

https://codereview.chromium.org/2627243002/diff/410001/chrome/browser/resourc...
File chrome/browser/resources/bluetooth_internals/value_control.js (right):

https://codereview.chromium.org/2627243002/diff/410001/chrome/browser/resourc...
chrome/browser/resources/bluetooth_internals/value_control.js:80: break;
Since you are returning in previous line, this break statement is dead code,
similarly below.

https://codereview.chromium.org/2627243002/diff/410001/chrome/test/data/webui...
File chrome/test/data/webui/bluetooth_internals_browsertest.js (right):

https://codereview.chromium.org/2627243002/diff/410001/chrome/test/data/webui...
chrome/test/data/webui/bluetooth_internals_browsertest.js:894: var valueArr =
valueControl.value_.getArray();
Nit (optional): No need for local var, just inline it along with the assertion.

https://codereview.chromium.org/2627243002/diff/410001/chrome/test/data/webui...
chrome/test/data/webui/bluetooth_internals_browsertest.js:950: 
Nit (optional): Unnecessary blank line.

[dcheng, 2017-01-26 04:37:04]

On 2017/01/24 23:21:52, mbrunson wrote:
> Mojom Review, please
> 
> dcheng:
>  device/bluetooth/public/interfaces/device.mojom
>  device/bluetooth/public/interfaces/gatt_result_type_converter.h
> 
> Also, have we come up with a replacement for type converters yet? We
originally
> put one in because we couldn't use a typemap for a mojom enum that has more
> values than the matching C++ version. Maybe I should use EnumTraits directly?
> Let me know what you think.
> 

EnumTraits should already work. https://codereview.chromium.org/2626093004/
might be helpful as a reference CL.

(Sorry for the code review reply latency)

[mbrunson, 2017-01-26 19:16:26]

https://codereview.chromium.org/2627243002/diff/410001/chrome/browser/resourc...
File chrome/browser/resources/bluetooth_internals/value_control.js (right):

https://codereview.chromium.org/2627243002/diff/410001/chrome/browser/resourc...
chrome/browser/resources/bluetooth_internals/value_control.js:80: break;
On 2017/01/25 23:50:20, dpapad wrote:
> Since you are returning in previous line, this break statement is dead code,
> similarly below.

Done.

https://codereview.chromium.org/2627243002/diff/410001/chrome/test/data/webui...
File chrome/test/data/webui/bluetooth_internals_browsertest.js (right):

https://codereview.chromium.org/2627243002/diff/410001/chrome/test/data/webui...
chrome/test/data/webui/bluetooth_internals_browsertest.js:894: var valueArr =
valueControl.value_.getArray();
On 2017/01/25 23:50:20, dpapad wrote:
> Nit (optional): No need for local var, just inline it along with the
assertion.

Done.

https://codereview.chromium.org/2627243002/diff/410001/chrome/test/data/webui...
chrome/test/data/webui/bluetooth_internals_browsertest.js:950: 
On 2017/01/25 23:50:20, dpapad wrote:
> Nit (optional): Unnecessary blank line.

Done.

[mbrunson, 2017-01-26 19:17:06]

On 2017/01/26 04:37:04, dcheng wrote:
> On 2017/01/24 23:21:52, mbrunson wrote:
> > Mojom Review, please
> > 
> > dcheng:
> >  device/bluetooth/public/interfaces/device.mojom
> >  device/bluetooth/public/interfaces/gatt_result_type_converter.h
> > 
> > Also, have we come up with a replacement for type converters yet? We
> originally
> > put one in because we couldn't use a typemap for a mojom enum that has more
> > values than the matching C++ version. Maybe I should use EnumTraits
directly?
> > Let me know what you think.
> > 
> 
> EnumTraits should already work. https://codereview.chromium.org/2626093004/
> might be helpful as a reference CL.
> 
> (Sorry for the code review reply latency)

Ok. I've replaced that type converter with enum traits.

[dcheng, 2017-01-26 19:32:25]

https://codereview.chromium.org/2627243002/diff/450001/device/bluetooth/devic...
File device/bluetooth/device.cc (right):

https://codereview.chromium.org/2627243002/diff/450001/device/bluetooth/devic...
device/bluetooth/device.cc:268:
callback.Run(GattResultTraits::ToMojom(error_code),
I think you need a typemap file as well (see the CL I linked previously): then
you can just write:

callback.Run(error_code, base::nullopt);

and it will Just Work (tm).

https://codereview.chromium.org/2627243002/diff/450001/device/bluetooth/publi...
File device/bluetooth/public/interfaces/gatt_result_enum_traits.h (right):

https://codereview.chromium.org/2627243002/diff/450001/device/bluetooth/publi...
device/bluetooth/public/interfaces/gatt_result_enum_traits.h:5: #ifndef
DEVICE_BLUETOOTH_GATT_RESULT_STRUCT_TRAITS_H_
Please name this file _struct_traits.h so OWNERS rules will be properly
enforced.

[mbrunson, 2017-01-26 21:27:40]

https://codereview.chromium.org/2627243002/diff/450001/device/bluetooth/devic...
File device/bluetooth/device.cc (right):

https://codereview.chromium.org/2627243002/diff/450001/device/bluetooth/devic...
device/bluetooth/device.cc:268:
callback.Run(GattResultTraits::ToMojom(error_code),
On 2017/01/26 19:32:25, dcheng wrote:
> I think you need a typemap file as well (see the CL I linked previously): then
> you can just write:
> 
> callback.Run(error_code, base::nullopt);
> 
> and it will Just Work (tm).

Ah...yes. The issue with that is I can't do things like this:

  callback.Run(mojom::GattResult::SERVICE_NOT_FOUND);
  callback.Run(mojom::GattResult::CHARACTERISTIC_NOT_FOUND);
  callback.Run(mojom::GattResult::SUCCESS);

with a typemap because there is no matching C++ value for this. The generated
callback only accepts the C++ version of the enum once a typemap is specified.
Adding these values to the C++ enum will require a large change to existing
code. This is what I was referring to in my original question.

[mbrunson, 2017-01-26 22:12:59]

The CQ bit was checked by mbrunson@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-26 22:13:42]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[mbrunson, 2017-01-26 22:14:57]

On 2017/01/26 04:37:04, dcheng wrote:
> On 2017/01/24 23:21:52, mbrunson wrote:
> > Mojom Review, please
> > 
> > dcheng:
> >  device/bluetooth/public/interfaces/device.mojom
> >  device/bluetooth/public/interfaces/gatt_result_type_converter.h
> > 
> > Also, have we come up with a replacement for type converters yet? We
> originally
> > put one in because we couldn't use a typemap for a mojom enum that has more
> > values than the matching C++ version. Maybe I should use EnumTraits
directly?
> > Let me know what you think.
> > 
> 
> EnumTraits should already work. https://codereview.chromium.org/2626093004/
> might be helpful as a reference CL.
> 
> (Sorry for the code review reply latency)

As discussed, I have reverted back to using TypeConverter.

[commit-bot: I haz the power, 2017-01-26 22:16:22]

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

[commit-bot: I haz the power, 2017-01-26 22:16:23]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[dcheng, 2017-01-26 22:22:47]

mojo lgtm, sorry for the back and forth here (I missed the fact that the mojo
enum has values the C++ ones doesn't)

https://codereview.chromium.org/2627243002/diff/470001/device/bluetooth/publi...
File device/bluetooth/public/interfaces/gatt_result_type_converter.h (right):

https://codereview.chromium.org/2627243002/diff/470001/device/bluetooth/publi...
device/bluetooth/public/interfaces/gatt_result_type_converter.h:16: //
TODO(crbug.com/666561): Replace because TypeConverter is deprecated.
Nit: add a note that this is a type converter needed to work around the fact
that the mojo enum has values not present in the C++ enum.

[mbrunson, 2017-01-26 22:47:12]

The CQ bit was checked by mbrunson@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-26 22:48:05]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[scheib, 2017-01-26 22:56:13]

https://codereview.chromium.org/2627243002/diff/470001/device/bluetooth/publi...
File device/bluetooth/public/interfaces/gatt_result_type_converter.h (right):

https://codereview.chromium.org/2627243002/diff/470001/device/bluetooth/publi...
device/bluetooth/public/interfaces/gatt_result_type_converter.h:16: //
TODO(crbug.com/666561): Replace because TypeConverter is deprecated.
On 2017/01/26 22:22:47, dcheng wrote:
> Nit: add a note that this is a type converter needed to work around the fact
> that the mojo enum has values not present in the C++ enum.

BTW I'd recently updated
https://bugs.chromium.org/p/chromium/issues/detail?id=666561 to include more of
that detail -- adding there and to the in-code comments SGTM.

[commit-bot: I haz the power, 2017-01-27 01:06:57]

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

[commit-bot: I haz the power, 2017-01-27 01:06:58]

Dry run: This issue passed the CQ dry run.

[mbrunson, 2017-01-27 01:22:40]

The CQ bit was checked by mbrunson@chromium.org

[mbrunson, 2017-01-27 01:22:40]

The patchset sent to the CQ was uploaded after l-g-t-m from ortuno@chromium.org,
dpapad@chromium.org, dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/2627243002/#ps490001
(title: "Merge upstream, add comment detail for type converter")

[mbrunson, 2017-01-27 01:22:54]

https://codereview.chromium.org/2627243002/diff/470001/device/bluetooth/publi...
File device/bluetooth/public/interfaces/gatt_result_type_converter.h (right):

https://codereview.chromium.org/2627243002/diff/470001/device/bluetooth/publi...
device/bluetooth/public/interfaces/gatt_result_type_converter.h:16: //
TODO(crbug.com/666561): Replace because TypeConverter is deprecated.
On 2017/01/26 22:56:13, scheib wrote:
> On 2017/01/26 22:22:47, dcheng wrote:
> > Nit: add a note that this is a type converter needed to work around the fact
> > that the mojo enum has values not present in the C++ enum.
> 
> BTW I'd recently updated
> https://bugs.chromium.org/p/chromium/issues/detail?id=666561 to include more
of
> that detail -- adding there and to the in-code comments SGTM.

Done.

[commit-bot: I haz the power, 2017-01-27 01:23:39]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-27 01:36:44]

CQ is committing da patch.

Bot data: {"patchset_id": 490001, "attempt_start_ts": 1485480160400070,
"parent_rev": "a4aed8758b516fb6b3e84a93ba7b303347415715", "commit_rev":
"91de29301d3e16e93a3789b737a4aa44af4a0bd0"}

[commit-bot: I haz the power, 2017-01-27 01:37:15]

Description was changed from

==========
bluetooth: Add control for reading/writing of characteristics to internals page.

Adds ValueControl to allow a user to read values from and write values to
    characteristics.
Adds Permission enum to device.mojom for allowing reading/writing based on
    permissions/properties specified by the characteristic.
Adds WriteValueForCharacteristic and ReadValueForCharacteristic to Device Mojo
    service.

GIF: https://goo.gl/photos/izDvvyH7d56Uh4kE7

BUG=651282
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
bluetooth: Add control for reading/writing of characteristics to internals page.

Adds ValueControl to allow a user to read values from and write values to
    characteristics.
Adds Permission enum to device.mojom for allowing reading/writing based on
    permissions/properties specified by the characteristic.
Adds WriteValueForCharacteristic and ReadValueForCharacteristic to Device Mojo
    service.

GIF: https://goo.gl/photos/izDvvyH7d56Uh4kE7

BUG=651282
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2627243002
Cr-Commit-Position: refs/heads/master@{#446526}
Committed:
https://chromium.googlesource.com/chromium/src/+/91de29301d3e16e93a3789b737a4...
==========

[commit-bot: I haz the power, 2017-01-27 01:37:18]

Committed patchset #26 (id:490001) as
https://chromium.googlesource.com/chromium/src/+/91de29301d3e16e93a3789b737a4...