bluetooth: Browser-side implementation of getCharacteristic

bluetooth: Browser-side implementation of getCharacteristic

This patch also adds the necessary mocks for LayoutTests in
the following patch.

Updates to the Chrome Implementation Notes corresponding to
this patch:
https://github.com/WebBluetoothChrome/web-bluetooth/pull/9

This is the second of a set of three patches to implement
getCharacteristic:

  [1] http://crrev.com/1153343002
  [2] This patch.
  [3] http://crrev.com/1146163005

BUG=483344
Committed: https://crrev.com/a90b6a258b57779ac848cbcef3be13d78a7922b0
Cr-Commit-Position: refs/heads/master@{#334036}

[ortuno, 2015-06-02 00:54:47]

Patchset #1 (id:1) has been deleted

[ortuno, 2015-06-02 01:00:37]

Patchset #1 (id:20001) has been deleted

[ortuno, 2015-06-02 16:52:10]

Patchset #1 (id:40001) has been deleted

[ortuno, 2015-06-02 17:03:01]

ortuno@chromium.org changed reviewers:
+ jyasskin@google.com

[ortuno, 2015-06-02 17:03:02]

@jyasskin: PTAL

[Jeffrey Yasskin, 2015-06-02 22:12:27]

jyasskin@chromium.org changed reviewers:
+ jyasskin@chromium.org

[Jeffrey Yasskin, 2015-06-02 22:12:29]

https://codereview.chromium.org/1156573005/diff/80001/content/browser/bluetoo...
File content/browser/bluetooth/bluetooth_dispatcher_host.cc (right):

https://codereview.chromium.org/1156573005/diff/80001/content/browser/bluetoo...
content/browser/bluetooth/bluetooth_dispatcher_host.cc:164:
adapter_->GetDevice(service_to_device_[service_instance_id]);
Generally avoid using operator[] on a map to check if a key is present: it'll
insert the key into the map, which can make the map grow indefinitely.

Use `auto device_iter = service_to_device_.find(service_instance_id); if
(device_iter == service_to_device_.end()) Send(NETWORK_ERROR);` instead, or, if
only a malicious renderer could send an unknown service_id, kill the renderer
instead.

https://codereview.chromium.org/1156573005/diff/80001/content/browser/bluetoo...
content/browser/bluetooth/bluetooth_dispatcher_host.cc:276: // There was an
error 'creating the ATT Bearer so we reject with
s/'//

https://codereview.chromium.org/1156573005/diff/80001/content/browser/bluetoo...
File content/browser/bluetooth/bluetooth_dispatcher_host.h (right):

https://codereview.chromium.org/1156573005/diff/80001/content/browser/bluetoo...
content/browser/bluetooth/bluetooth_dispatcher_host.h:102: // Maps to get the
object's parent based on it's instanceID
We could also make the renderer pass both a
service_id/characteristic_id/descriptor_id and device_id when it calls over to
the browser, which would allow service_ids to only be unique within a device,
but it may not be worth it.

https://codereview.chromium.org/1156573005/diff/80001/content/child/bluetooth...
File content/child/bluetooth/bluetooth_dispatcher.cc (right):

https://codereview.chromium.org/1156573005/diff/80001/content/child/bluetooth...
content/child/bluetooth/bluetooth_dispatcher.cc:291:
pending_characteristic_requests_.Lookup(request_id);
Do you need to remove 'request_id' from 'pending_characteristic_requests_' after
calling onSuccess()?

https://codereview.chromium.org/1156573005/diff/80001/content/child/bluetooth...
content/child/bluetooth/bluetooth_dispatcher.cc:308:
pending_characteristic_requests_.Remove(request_id);
I'd probably combine the 2 Remove() calls by putting the onError() call in an
else clause.

https://codereview.chromium.org/1156573005/diff/80001/content/common/bluetoot...
File content/common/bluetooth/bluetooth_messages.h (right):

https://codereview.chromium.org/1156573005/diff/80001/content/common/bluetoot...
content/common/bluetooth/bluetooth_messages.h:138: // Informs the renderer that
the primary_service request |request_id| failed.
Why the extra '_' here?

https://codereview.chromium.org/1156573005/diff/80001/content/common/bluetoot...
content/common/bluetooth/bluetooth_messages.h:182: // Gets a GATT Characteristic
from a bluetooth device.
s/from a bluetooth device/within a GATT Service/?

https://codereview.chromium.org/1156573005/diff/80001/content/shell/browser/l...
File content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.cc
(right):

https://codereview.chromium.org/1156573005/diff/80001/content/shell/browser/l...
content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.cc:215:
.WillByDefault(::GetMockService(empty_device.get()));
I'm unhappy with the name collision between the ::GetMockService action and
LayoutTestBluetoothAdapterProvider::GetMockService. Maybe call the action
"GetMockServiceWithin" or similar? You should change GetMockDevice to match.

[Jeffrey Yasskin, 2015-06-03 00:13:51]

https://codereview.chromium.org/1156573005/diff/80001/content/shell/browser/l...
File content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.h
(right):

https://codereview.chromium.org/1156573005/diff/80001/content/shell/browser/l...
content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.h:75:
//  - |GetGattServices| returns a list with two services "Generic Access" and
Please mention the characteristics on these services.

[ortuno, 2015-06-03 20:27:23]

https://codereview.chromium.org/1156573005/diff/80001/content/browser/bluetoo...
File content/browser/bluetooth/bluetooth_dispatcher_host.cc (right):

https://codereview.chromium.org/1156573005/diff/80001/content/browser/bluetoo...
content/browser/bluetooth/bluetooth_dispatcher_host.cc:164:
adapter_->GetDevice(service_to_device_[service_instance_id]);
On 2015/06/02 at 22:12:28, Jeffrey Yasskin wrote:
> Generally avoid using operator[] on a map to check if a key is present: it'll
insert the key into the map, which can make the map grow indefinitely.
> 
> Use `auto device_iter = service_to_device_.find(service_instance_id); if
(device_iter == service_to_device_.end()) Send(NETWORK_ERROR);` instead, or, if
only a malicious renderer could send an unknown service_id, kill the renderer
instead.

Done.

https://codereview.chromium.org/1156573005/diff/80001/content/browser/bluetoo...
content/browser/bluetooth/bluetooth_dispatcher_host.cc:276: // There was an
error 'creating the ATT Bearer so we reject with
On 2015/06/02 at 22:12:29, Jeffrey Yasskin wrote:
> s/'//

Done.

https://codereview.chromium.org/1156573005/diff/80001/content/browser/bluetoo...
File content/browser/bluetooth/bluetooth_dispatcher_host.h (right):

https://codereview.chromium.org/1156573005/diff/80001/content/browser/bluetoo...
content/browser/bluetooth/bluetooth_dispatcher_host.h:102: // Maps to get the
object's parent based on it's instanceID
On 2015/06/02 at 22:12:29, Jeffrey Yasskin wrote:
> We could also make the renderer pass both a
service_id/characteristic_id/descriptor_id and device_id when it calls over to
the browser, which would allow service_ids to only be unique within a device,
but it may not be worth it.

Hmm not sure how passing both a service_id and a device_id allows service_id to
be unique within a device. In general the identifiers are unique per adapter.

https://codereview.chromium.org/1156573005/diff/80001/content/child/bluetooth...
File content/child/bluetooth/bluetooth_dispatcher.cc (right):

https://codereview.chromium.org/1156573005/diff/80001/content/child/bluetooth...
content/child/bluetooth/bluetooth_dispatcher.cc:291:
pending_characteristic_requests_.Lookup(request_id);
On 2015/06/02 at 22:12:29, Jeffrey Yasskin wrote:
> Do you need to remove 'request_id' from 'pending_characteristic_requests_'
after calling onSuccess()?

Done.

https://codereview.chromium.org/1156573005/diff/80001/content/child/bluetooth...
content/child/bluetooth/bluetooth_dispatcher.cc:308:
pending_characteristic_requests_.Remove(request_id);
On 2015/06/02 at 22:12:29, Jeffrey Yasskin wrote:
> I'd probably combine the 2 Remove() calls by putting the onError() call in an
else clause.

Done.

https://codereview.chromium.org/1156573005/diff/80001/content/common/bluetoot...
File content/common/bluetooth/bluetooth_messages.h (right):

https://codereview.chromium.org/1156573005/diff/80001/content/common/bluetoot...
content/common/bluetooth/bluetooth_messages.h:138: // Informs the renderer that
the primary_service request |request_id| failed.
On 2015/06/02 at 22:12:29, Jeffrey Yasskin wrote:
> Why the extra '_' here?

Done.

https://codereview.chromium.org/1156573005/diff/80001/content/common/bluetoot...
content/common/bluetooth/bluetooth_messages.h:182: // Gets a GATT Characteristic
from a bluetooth device.
On 2015/06/02 at 22:12:29, Jeffrey Yasskin wrote:
> s/from a bluetooth device/within a GATT Service/?

Done.

https://codereview.chromium.org/1156573005/diff/80001/content/shell/browser/l...
File content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.cc
(right):

https://codereview.chromium.org/1156573005/diff/80001/content/shell/browser/l...
content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.cc:215:
.WillByDefault(::GetMockService(empty_device.get()));
On 2015/06/02 at 22:12:29, Jeffrey Yasskin wrote:
> I'm unhappy with the name collision between the ::GetMockService action and
LayoutTestBluetoothAdapterProvider::GetMockService. Maybe call the action
"GetMockServiceWithin" or similar? You should change GetMockDevice to match.

What about changing LayoutTestBluetoothAdapterProvider::GetMockService to
GetService instead?

https://codereview.chromium.org/1156573005/diff/80001/content/shell/browser/l...
File content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.h
(right):

https://codereview.chromium.org/1156573005/diff/80001/content/shell/browser/l...
content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.h:75:
//  - |GetGattServices| returns a list with two services "Generic Access" and
On 2015/06/03 at 00:13:50, Jeffrey Yasskin wrote:
> Please mention the characteristics on these services.

Done.

[Jeffrey Yasskin, 2015-06-03 22:56:24]

lgtm

https://codereview.chromium.org/1156573005/diff/80001/content/shell/browser/l...
File content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.cc
(right):

https://codereview.chromium.org/1156573005/diff/80001/content/shell/browser/l...
content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.cc:215:
.WillByDefault(::GetMockService(empty_device.get()));
On 2015/06/03 20:27:23, ortuno wrote:
> On 2015/06/02 at 22:12:29, Jeffrey Yasskin wrote:
> > I'm unhappy with the name collision between the ::GetMockService action and
> LayoutTestBluetoothAdapterProvider::GetMockService. Maybe call the action
> "GetMockServiceWithin" or similar? You should change GetMockDevice to match.
> 
> What about changing LayoutTestBluetoothAdapterProvider::GetMockService to
> GetService instead?

Sure.

https://codereview.chromium.org/1156573005/diff/120001/content/browser/blueto...
File content/browser/bluetooth/bluetooth_dispatcher_host.cc (right):

https://codereview.chromium.org/1156573005/diff/120001/content/browser/blueto...
content/browser/bluetooth/bluetooth_dispatcher_host.cc:174:
adapter_->GetDevice(service_to_device_[service_instance_id]);
Since you've looked up the iterator above, just use GetDevice(*device_iter)
here.

[Jeffrey Yasskin, 2015-06-03 22:56:38]

jyasskin@chromium.org changed reviewers:
- jyasskin@google.com

[ortuno, 2015-06-03 23:26:55]

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

[ortuno, 2015-06-03 23:26:56]

@jyasskin: Thanks!
@scheib: PTAL

https://codereview.chromium.org/1156573005/diff/120001/content/browser/blueto...
File content/browser/bluetooth/bluetooth_dispatcher_host.cc (right):

https://codereview.chromium.org/1156573005/diff/120001/content/browser/blueto...
content/browser/bluetooth/bluetooth_dispatcher_host.cc:174:
adapter_->GetDevice(service_to_device_[service_instance_id]);
On 2015/06/03 at 22:56:24, Jeffrey Yasskin wrote:
> Since you've looked up the iterator above, just use GetDevice(*device_iter)
here.

Done.

[scheib, 2015-06-04 06:30:49]

https://codereview.chromium.org/1156573005/diff/140001/content/browser/blueto...
File content/browser/bluetooth/bluetooth_dispatcher_host.cc (right):

https://codereview.chromium.org/1156573005/diff/140001/content/browser/blueto...
content/browser/bluetooth/bluetooth_dispatcher_host.cc:162: // There are no
cases in which a normal renderer could pass
Move this comment either just above the 'if' line, or inside the conditional
block.
Reword:
 // A service_instance_id not in the map implies a hostile renderer
 // because a renderer obtains the service id from this class and
 // it will be added to the map at that time.

https://codereview.chromium.org/1156573005/diff/140001/content/browser/blueto...
content/browser/bluetooth/bluetooth_dispatcher_host.cc:313:
service_to_device_.insert(
Maybe for paranoia:

auto insert_result = service_to_device_.insert(
    make_pair(service_identifier, device_instance_id));
// DCHECK that insert found no previous entry, or entry value matches:
DCHECK(!insert_result.second 
    || insert_result.first.second == device_instance_id);

https://codereview.chromium.org/1156573005/diff/140001/content/child/bluetoot...
File content/child/bluetooth/bluetooth_dispatcher.cc (right):

https://codereview.chromium.org/1156573005/diff/140001/content/child/bluetoot...
content/child/bluetooth/bluetooth_dispatcher.cc:315:
WebBluetoothErrorFromBluetoothError(error_type), ""));
Error messages...
https://codereview.chromium.org/1159523002/diff/60001/content/browser/bluetoo...
OK, this patch was already written when that last comment was made so it passes,
but before new code added please.

https://codereview.chromium.org/1156573005/diff/140001/content/child/bluetoot...
File content/child/bluetooth/web_bluetooth_impl.h (right):

https://codereview.chromium.org/1156573005/diff/140001/content/child/bluetoot...
content/child/bluetooth/web_bluetooth_impl.h:40: void getCharacteristic(
Remove previous 3 blank lines.

https://codereview.chromium.org/1156573005/diff/140001/content/shell/browser/...
File content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.cc
(right):

https://codereview.chromium.org/1156573005/diff/140001/content/shell/browser/...
content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.cc:71:
ACTION_P(GetMockService, device) {
Add GetMockService to MockBluetoothDevice.

https://codereview.chromium.org/1156573005/diff/140001/content/shell/browser/...
content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.cc:211:
// The call to BluetoothDevice::GetService will invoke ::GetMockService
GetService -> GetGattService

https://codereview.chromium.org/1156573005/diff/140001/device/bluetooth/test/...
File device/bluetooth/test/mock_bluetooth_gatt_service.h (right):

https://codereview.chromium.org/1156573005/diff/140001/device/bluetooth/test/...
device/bluetooth/test/mock_bluetooth_gatt_service.h:46: // BluetoothService
manages the lifetime of its BluetoothGATTCharacteristics.
BluetoothGATTService

https://codereview.chromium.org/1156573005/diff/140001/device/bluetooth/test/...
device/bluetooth/test/mock_bluetooth_gatt_service.h:47: // This methods takes
ownership of the BluetoothGATTCharacteristics. This is
"of the Mock____"
Fix the adapter and device comments also.

[ortuno, 2015-06-05 19:37:44]

@scheib: Ready for review again.

https://codereview.chromium.org/1156573005/diff/140001/content/browser/blueto...
File content/browser/bluetooth/bluetooth_dispatcher_host.cc (right):

https://codereview.chromium.org/1156573005/diff/140001/content/browser/blueto...
content/browser/bluetooth/bluetooth_dispatcher_host.cc:162: // There are no
cases in which a normal renderer could pass
On 2015/06/04 at 06:30:48, scheib wrote:
> Move this comment either just above the 'if' line, or inside the conditional
block.
> Reword:
>  // A service_instance_id not in the map implies a hostile renderer
>  // because a renderer obtains the service id from this class and
>  // it will be added to the map at that time.

Done.

https://codereview.chromium.org/1156573005/diff/140001/content/browser/blueto...
content/browser/bluetooth/bluetooth_dispatcher_host.cc:313:
service_to_device_.insert(
On 2015/06/04 at 06:30:48, scheib wrote:
> Maybe for paranoia:
> 
> auto insert_result = service_to_device_.insert(
>     make_pair(service_identifier, device_instance_id));
> // DCHECK that insert found no previous entry, or entry value matches:
> DCHECK(!insert_result.second 
>     || insert_result.first.second == device_instance_id);

If you call getPrimaryService("some uuid") twice the DCHECK would execute.

https://codereview.chromium.org/1156573005/diff/140001/content/child/bluetoot...
File content/child/bluetooth/bluetooth_dispatcher.cc (right):

https://codereview.chromium.org/1156573005/diff/140001/content/child/bluetoot...
content/child/bluetooth/bluetooth_dispatcher.cc:315:
WebBluetoothErrorFromBluetoothError(error_type), ""));
On 2015/06/04 at 06:30:49, scheib wrote:
> Error messages...
https://codereview.chromium.org/1159523002/diff/60001/content/browser/bluetoo...
> OK, this patch was already written when that last comment was made so it
passes, but before new code added please.

Ah ok. My plan was to do that after we get something demoable i.e. after
readValue is implemented.

https://codereview.chromium.org/1156573005/diff/140001/content/child/bluetoot...
File content/child/bluetooth/web_bluetooth_impl.h (right):

https://codereview.chromium.org/1156573005/diff/140001/content/child/bluetoot...
content/child/bluetooth/web_bluetooth_impl.h:40: void getCharacteristic(
On 2015/06/04 at 06:30:49, scheib wrote:
> Remove previous 3 blank lines.

Done.

https://codereview.chromium.org/1156573005/diff/140001/content/shell/browser/...
File content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.cc
(right):

https://codereview.chromium.org/1156573005/diff/140001/content/shell/browser/...
content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.cc:71:
ACTION_P(GetMockService, device) {
On 2015/06/04 at 06:30:49, scheib wrote:
> Add GetMockService to MockBluetoothDevice.

Done. I'll move GetMockDevice to MockBluetoothDevice future patch.

https://codereview.chromium.org/1156573005/diff/140001/content/shell/browser/...
content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.cc:211:
// The call to BluetoothDevice::GetService will invoke ::GetMockService
On 2015/06/04 at 06:30:49, scheib wrote:
> GetService -> GetGattService

Done.

https://codereview.chromium.org/1156573005/diff/140001/device/bluetooth/test/...
File device/bluetooth/test/mock_bluetooth_gatt_service.h (right):

https://codereview.chromium.org/1156573005/diff/140001/device/bluetooth/test/...
device/bluetooth/test/mock_bluetooth_gatt_service.h:46: // BluetoothService
manages the lifetime of its BluetoothGATTCharacteristics.
On 2015/06/04 at 06:30:49, scheib wrote:
> BluetoothGATTService

Done.

https://codereview.chromium.org/1156573005/diff/140001/device/bluetooth/test/...
device/bluetooth/test/mock_bluetooth_gatt_service.h:47: // This methods takes
ownership of the BluetoothGATTCharacteristics. This is
On 2015/06/04 at 06:30:49, scheib wrote:
> "of the Mock____"
> Fix the adapter and device comments also.

Done.

[ortuno, 2015-06-05 21:07:36]

ortuno@chromium.org changed reviewers:
+ nick@chromium.org

[ortuno, 2015-06-05 21:07:37]

@nick: PTAL at bad_message.h

[ncarter (slow), 2015-06-06 18:19:46]

lgtm, thanks for using bad_message

https://codereview.chromium.org/1156573005/diff/180001/content/browser/bad_me...
File content/browser/bad_message.h (right):

https://codereview.chromium.org/1156573005/diff/180001/content/browser/bad_me...
content/browser/bad_message.h:104: BDH_INVALID_SERVICE_ID = 78,
Please run python tools/metrics/histograms/update_bad_message_reasons.py to
update histograms.xml with this new enum value.

(Soon I'll add a presubmit check for this)

[scheib, 2015-06-08 16:54:41]

LGTM

[scheib, 2015-06-08 17:02:07]

https://codereview.chromium.org/1156573005/diff/140001/content/browser/blueto...
File content/browser/bluetooth/bluetooth_dispatcher_host.cc (right):

https://codereview.chromium.org/1156573005/diff/140001/content/browser/blueto...
content/browser/bluetooth/bluetooth_dispatcher_host.cc:313:
service_to_device_.insert(
On 2015/06/05 19:37:44, ortuno wrote:
> On 2015/06/04 at 06:30:48, scheib wrote:
> > Maybe for paranoia:
> > 
> > auto insert_result = service_to_device_.insert(
> >     make_pair(service_identifier, device_instance_id));
> > // DCHECK that insert found no previous entry, or entry value matches:
> > DCHECK(!insert_result.second 
> >     || insert_result.first.second == device_instance_id);
> 
> If you call getPrimaryService("some uuid") twice the DCHECK would execute.

Whoops, overlooked this. Discussed in person too, but for record, this check is
that either it wasn't found in map, or if there was already an entry it checks
that the value stored already matches what we wanted to store.

[ortuno, 2015-06-08 17:41:33]

ortuno@chromium.org changed reviewers:
+ armansito@chromium.org, tsepez@chromium.org

[ortuno, 2015-06-08 17:41:34]

@armansito: PTAL at device/bluetooth/test
@tsepez: PTAL at bluetooth_messages

[Tom Sepez, 2015-06-08 17:59:04]

Messages LGTM.

[ortuno, 2015-06-08 18:01:40]

ortuno@chromium.org changed reviewers:
+ asvitkine@chromium.org

[ortuno, 2015-06-08 18:01:41]

@asvitkine: PTAL at histogram.xml

[Alexei Svitkine (slow), 2015-06-08 21:15:33]

lgtm

[armansito, 2015-06-09 19:23:49]

https://codereview.chromium.org/1156573005/diff/200001/device/bluetooth/test/...
File device/bluetooth/test/mock_bluetooth_adapter.h (right):

https://codereview.chromium.org/1156573005/diff/200001/device/bluetooth/test/...
device/bluetooth/test/mock_bluetooth_adapter.h:105: // This methods takes
ownership of the MockBluetoothDevice. This is only for
nit: s/This methods/This method/

https://codereview.chromium.org/1156573005/diff/200001/device/bluetooth/test/...
File device/bluetooth/test/mock_bluetooth_device.cc (right):

https://codereview.chromium.org/1156573005/diff/200001/device/bluetooth/test/...
device/bluetooth/test/mock_bluetooth_device.cc:79: }
nit: No need for curly braces here.

https://codereview.chromium.org/1156573005/diff/200001/device/bluetooth/test/...
File device/bluetooth/test/mock_bluetooth_device.h (right):

https://codereview.chromium.org/1156573005/diff/200001/device/bluetooth/test/...
device/bluetooth/test/mock_bluetooth_device.h:86: // This methods takes
ownership of the MockBluetoothGATTServices. This is only
nit: s/This methods/This method/

https://codereview.chromium.org/1156573005/diff/200001/device/bluetooth/test/...
File device/bluetooth/test/mock_bluetooth_gatt_service.cc (right):

https://codereview.chromium.org/1156573005/diff/200001/device/bluetooth/test/...
device/bluetooth/test/mock_bluetooth_gatt_service.cc:45:
characteristics.push_back(characteristic);
Why is this copying necessary? Can't you change the return type to const-ref and
return |mock_characteristics_|?

https://codereview.chromium.org/1156573005/diff/200001/device/bluetooth/test/...
File device/bluetooth/test/mock_bluetooth_gatt_service.h (right):

https://codereview.chromium.org/1156573005/diff/200001/device/bluetooth/test/...
device/bluetooth/test/mock_bluetooth_gatt_service.h:48: // This methods takes
ownership of the MockBluetoothGATTCharacteristics. This
nit: s/This methods/This method/

https://codereview.chromium.org/1156573005/diff/200001/device/bluetooth/test/...
device/bluetooth/test/mock_bluetooth_gatt_service.h:56: void
AddMockCharacteristic(
Will you add similar methods to MockBluetoothGattCharacteristic for descriptors?

[ortuno, 2015-06-09 20:46:43]

@armansito: Thanks!

https://codereview.chromium.org/1156573005/diff/200001/device/bluetooth/test/...
File device/bluetooth/test/mock_bluetooth_adapter.h (right):

https://codereview.chromium.org/1156573005/diff/200001/device/bluetooth/test/...
device/bluetooth/test/mock_bluetooth_adapter.h:105: // This methods takes
ownership of the MockBluetoothDevice. This is only for
On 2015/06/09 at 19:23:48, armansito wrote:
> nit: s/This methods/This method/

Done.

https://codereview.chromium.org/1156573005/diff/200001/device/bluetooth/test/...
File device/bluetooth/test/mock_bluetooth_device.cc (right):

https://codereview.chromium.org/1156573005/diff/200001/device/bluetooth/test/...
device/bluetooth/test/mock_bluetooth_device.cc:79: }
On 2015/06/09 at 19:23:49, armansito wrote:
> nit: No need for curly braces here.

Done.

https://codereview.chromium.org/1156573005/diff/200001/device/bluetooth/test/...
File device/bluetooth/test/mock_bluetooth_device.h (right):

https://codereview.chromium.org/1156573005/diff/200001/device/bluetooth/test/...
device/bluetooth/test/mock_bluetooth_device.h:86: // This methods takes
ownership of the MockBluetoothGATTServices. This is only
On 2015/06/09 at 19:23:49, armansito wrote:
> nit: s/This methods/This method/

Done.

https://codereview.chromium.org/1156573005/diff/200001/device/bluetooth/test/...
File device/bluetooth/test/mock_bluetooth_gatt_service.cc (right):

https://codereview.chromium.org/1156573005/diff/200001/device/bluetooth/test/...
device/bluetooth/test/mock_bluetooth_gatt_service.cc:45:
characteristics.push_back(characteristic);
On 2015/06/09 at 19:23:49, armansito wrote:
> Why is this copying necessary? Can't you change the return type to const-ref
and return |mock_characteristics_|?

The compiler complains when trying to convert from ScopedVector<T> to
std::vector<T*>.

https://codereview.chromium.org/1156573005/diff/200001/device/bluetooth/test/...
File device/bluetooth/test/mock_bluetooth_gatt_service.h (right):

https://codereview.chromium.org/1156573005/diff/200001/device/bluetooth/test/...
device/bluetooth/test/mock_bluetooth_gatt_service.h:48: // This methods takes
ownership of the MockBluetoothGATTCharacteristics. This
On 2015/06/09 at 19:23:49, armansito wrote:
> nit: s/This methods/This method/

Done.

https://codereview.chromium.org/1156573005/diff/200001/device/bluetooth/test/...
device/bluetooth/test/mock_bluetooth_gatt_service.h:56: void
AddMockCharacteristic(
On 2015/06/09 at 19:23:49, armansito wrote:
> Will you add similar methods to MockBluetoothGattCharacteristic for
descriptors?

Yes.

[armansito, 2015-06-10 02:40:47]

https://codereview.chromium.org/1156573005/diff/200001/device/bluetooth/test/...
File device/bluetooth/test/mock_bluetooth_gatt_service.cc (right):

https://codereview.chromium.org/1156573005/diff/200001/device/bluetooth/test/...
device/bluetooth/test/mock_bluetooth_gatt_service.cc:45:
characteristics.push_back(characteristic);
On 2015/06/09 20:46:42, ortuno wrote:
> On 2015/06/09 at 19:23:49, armansito wrote:
> > Why is this copying necessary? Can't you change the return type to const-ref
> and return |mock_characteristics_|?
> 
> The compiler complains when trying to convert from ScopedVector<T> to
> std::vector<T*>.

If this is a test only method, why not just return a "const
ScopedVector<BluetoothGattCharacteristic>&"?

[ortuno, 2015-06-10 16:32:10]

On 2015/06/10 at 02:40:47, armansito wrote:
>
https://codereview.chromium.org/1156573005/diff/200001/device/bluetooth/test/...
> File device/bluetooth/test/mock_bluetooth_gatt_service.cc (right):
> 
>
https://codereview.chromium.org/1156573005/diff/200001/device/bluetooth/test/...
> device/bluetooth/test/mock_bluetooth_gatt_service.cc:45:
characteristics.push_back(characteristic);
> On 2015/06/09 20:46:42, ortuno wrote:
> > On 2015/06/09 at 19:23:49, armansito wrote:
> > > Why is this copying necessary? Can't you change the return type to
const-ref
> > and return |mock_characteristics_|?
> > 
> > The compiler complains when trying to convert from ScopedVector<T> to
> > std::vector<T*>.
> 
> If this is a test only method, why not just return a "const
ScopedVector<BluetoothGattCharacteristic>&"?

It needs to return a std::vector<T*> Because of the way it's used in the test
code:

ON_CALL(*service, GetCharacteristics())
      .WillByDefault(Invoke(service.get(),
                            &MockBluetoothGattService::GetMockCharacteristics));

[ortuno, 2015-06-11 19:48:38]

On 2015/06/10 at 16:32:10, ortuno wrote:
> On 2015/06/10 at 02:40:47, armansito wrote:
> >
https://codereview.chromium.org/1156573005/diff/200001/device/bluetooth/test/...
> > File device/bluetooth/test/mock_bluetooth_gatt_service.cc (right):
> > 
> >
https://codereview.chromium.org/1156573005/diff/200001/device/bluetooth/test/...
> > device/bluetooth/test/mock_bluetooth_gatt_service.cc:45:
characteristics.push_back(characteristic);
> > On 2015/06/09 20:46:42, ortuno wrote:
> > > On 2015/06/09 at 19:23:49, armansito wrote:
> > > > Why is this copying necessary? Can't you change the return type to
const-ref
> > > and return |mock_characteristics_|?
> > > 
> > > The compiler complains when trying to convert from ScopedVector<T> to
> > > std::vector<T*>.
> > 
> > If this is a test only method, why not just return a "const
ScopedVector<BluetoothGattCharacteristic>&"?
> 
> It needs to return a std::vector<T*> Because of the way it's used in the test
code:
> 
> ON_CALL(*service, GetCharacteristics())
>       .WillByDefault(Invoke(service.get(),
>                            
&MockBluetoothGattService::GetMockCharacteristics));

@armansito: ping?

[armansito, 2015-06-11 19:55:11]

On 2015/06/11 19:48:38, ortuno wrote:
> On 2015/06/10 at 16:32:10, ortuno wrote:
> > On 2015/06/10 at 02:40:47, armansito wrote:
> > >
>
https://codereview.chromium.org/1156573005/diff/200001/device/bluetooth/test/...
> > > File device/bluetooth/test/mock_bluetooth_gatt_service.cc (right):
> > > 
> > >
>
https://codereview.chromium.org/1156573005/diff/200001/device/bluetooth/test/...
> > > device/bluetooth/test/mock_bluetooth_gatt_service.cc:45:
> characteristics.push_back(characteristic);
> > > On 2015/06/09 20:46:42, ortuno wrote:
> > > > On 2015/06/09 at 19:23:49, armansito wrote:
> > > > > Why is this copying necessary? Can't you change the return type to
> const-ref
> > > > and return |mock_characteristics_|?
> > > > 
> > > > The compiler complains when trying to convert from ScopedVector<T> to
> > > > std::vector<T*>.
> > > 
> > > If this is a test only method, why not just return a "const
> ScopedVector<BluetoothGattCharacteristic>&"?
> > 
> > It needs to return a std::vector<T*> Because of the way it's used in the
test
> code:
> > 
> > ON_CALL(*service, GetCharacteristics())
> >       .WillByDefault(Invoke(service.get(),
> >                            
> &MockBluetoothGattService::GetMockCharacteristics));
> 
> @armansito: ping?

I don't love this but I guess there is no way around it for now and it's test
only. lgtm

[ortuno, 2015-06-11 19:58:55]

On 2015/06/11 at 19:55:11, armansito wrote:
> On 2015/06/11 19:48:38, ortuno wrote:
> > On 2015/06/10 at 16:32:10, ortuno wrote:
> > > On 2015/06/10 at 02:40:47, armansito wrote:
> > > >
> >
https://codereview.chromium.org/1156573005/diff/200001/device/bluetooth/test/...
> > > > File device/bluetooth/test/mock_bluetooth_gatt_service.cc (right):
> > > > 
> > > >
> >
https://codereview.chromium.org/1156573005/diff/200001/device/bluetooth/test/...
> > > > device/bluetooth/test/mock_bluetooth_gatt_service.cc:45:
> > characteristics.push_back(characteristic);
> > > > On 2015/06/09 20:46:42, ortuno wrote:
> > > > > On 2015/06/09 at 19:23:49, armansito wrote:
> > > > > > Why is this copying necessary? Can't you change the return type to
> > const-ref
> > > > > and return |mock_characteristics_|?
> > > > > 
> > > > > The compiler complains when trying to convert from ScopedVector<T> to
> > > > > std::vector<T*>.
> > > > 
> > > > If this is a test only method, why not just return a "const
> > ScopedVector<BluetoothGattCharacteristic>&"?
> > > 
> > > It needs to return a std::vector<T*> Because of the way it's used in the
test
> > code:
> > > 
> > > ON_CALL(*service, GetCharacteristics())
> > >       .WillByDefault(Invoke(service.get(),
> > >                            
> > &MockBluetoothGattService::GetMockCharacteristics));
> > 
> > @armansito: ping?
> 
> I don't love this but I guess there is no way around it for now and it's test
only. lgtm

Thanks!

[ortuno, 2015-06-11 20:51:52]

The CQ bit was checked by ortuno@chromium.org

[ortuno, 2015-06-11 20:51:52]

The patchset sent to the CQ was uploaded after l-g-t-m from
jyasskin@chromium.org, nick@chromium.org, scheib@chromium.org,
asvitkine@chromium.org, tsepez@chromium.org, armansito@chromium.org
Link to the patchset: https://codereview.chromium.org/1156573005/#ps240001
(title: "Fix histograms.xml and bad_messages.h merge conflict")

[commit-bot: I haz the power, 2015-06-11 20:52:31]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1156573005/240001

[commit-bot: I haz the power, 2015-06-11 21:13:13]

Committed patchset #10 (id:240001)

[commit-bot: I haz the power, 2015-06-11 21:14:54]

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