bluetooth: Implement Service.getCharacteristics

bluetooth: Implement Service.getCharacteristics

1. Adds bindings for getCharacteristics
2. Adds getCharacteristics function to public interface
3. Implements getCharacteristics from public interface
4. Adds necessary IPC messages
5. Modifies HeartRateService so that it has two Body Sensor Location
   characteristics. This way we can test what happens when two characteristics
   have the same UUID. Also modify the GetBaseGATTCharacteristic function to
   take an identifier. Before we just used the UUID but we need to change it
   now that multiple characteristics have the same UUID.
6. Adds LayoutTests.

BUG=588876
Committed: https://crrev.com/42d4354380744ef74e17b79d5ab4e5f2e42e17f7
Cr-Commit-Position: refs/heads/master@{#378001}

[ortuno, 2016-02-23 00:00:06]

Description was changed from

==========
bluetooth: Implement Service.getCharacteristics

1. Adds bindings for getCharacteristics
2. Adds getCharacteristics function to public interface
3. Implements getCharacteristics from public interface
4. Adds necessary IPC messages
5. Modifies HeartRateService so that it has two Body Sensor Location
   characteristics. This way we can test what happens when to
6. Adds LayoutTests.

BUG=588876
==========

to

==========
bluetooth: Implement Service.getCharacteristics

1. Adds bindings for getCharacteristics
2. Adds getCharacteristics function to public interface
3. Implements getCharacteristics from public interface
4. Adds necessary IPC messages
5. Modifies HeartRateService so that it has two Body Sensor Location
   characteristics. This way we can test what happens when two characteristics
   have the same UUID. Also modify the GetBaseGATTCharacteristic function to 
   take an identifier. Before we just used the UUID but we need to change it
   now that multiple characteristics have the same UUID.
6. Adds LayoutTests.

BUG=588876
==========

[ortuno, 2016-02-23 19:09:57]

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

[ortuno, 2016-02-23 19:09:58]

jyasskin: PTAL

[Jeffrey Yasskin, 2016-02-24 23:20:30]

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

https://codereview.chromium.org/1723723002/diff/60001/content/browser/bluetoo...
content/browser/bluetooth/bluetooth_dispatcher_host.cc:862:
std::vector<std::string> characteristics_instance_ids;
Can we transpose these three vectors? That is, make an IPC struct holding an
instance_id, a UUID, and a properties bit-array, and send a vector of those? I
don't want to have to rely on 3 parallel arrays matching up.

https://codereview.chromium.org/1723723002/diff/60001/content/browser/bluetoo...
content/browser/bluetooth/bluetooth_dispatcher_host.cc:869: characteristic_uuid
== characteristic->GetUUID().canonical_value()) {
Could you add a TODO to incorporate the blacklist here, in case your and
Vincent's patches collide?

https://codereview.chromium.org/1723723002/diff/60001/content/browser/bluetoo...
content/browser/bluetooth/bluetooth_dispatcher_host.cc:885:
RecordGetCharacteristicsOutcome(UMAGetCharacteristicOutcome::NOT_FOUND);
Probably split this by characteristic_uuid.empty() too.

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

https://codereview.chromium.org/1723723002/diff/60001/content/common/bluetoot...
content/common/bluetooth/bluetooth_messages.h:167: // Informs the renderer that
characteristic request |request_id| succeeded.
s/characteristic/multiple-characteristic/?

https://codereview.chromium.org/1723723002/diff/60001/content/renderer/blueto...
File content/renderer/bluetooth/bluetooth_dispatcher.cc (right):

https://codereview.chromium.org/1723723002/diff/60001/content/renderer/blueto...
content/renderer/bluetooth/bluetooth_dispatcher.cc:723: (*characteristics)[i] =
new WebBluetoothRemoteGATTCharacteristicInit(
I'm sad that we wind up with raw pointers in this vector. Can you store
WebPassOwnPtr<>s or plain instances of WebBluetoothRemoteGATTCharacteristicInit
instead?

https://codereview.chromium.org/1723723002/diff/60001/content/renderer/blueto...
content/renderer/bluetooth/bluetooth_dispatcher.cc:725:
request->service_instance_id,
Nit: I'd put the service instance ID first, so the sequence forms a path:
[device] / service / characteristic.

https://codereview.chromium.org/1723723002/diff/60001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/bluetooth/getCharacteristics/characteristics-not-found-with-uuid.html
(right):

https://codereview.chromium.org/1723723002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/bluetooth/getCharacteristics/characteristics-not-found-with-uuid.html:17:
service.getCharacteristics(battery_level.alias), expected),
I don't know that we need the alias/name/uuid triple for this test.

https://codereview.chromium.org/1723723002/diff/60001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/bluetooth/getCharacteristics/characteristics-not-found.html
(right):

https://codereview.chromium.org/1723723002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/bluetooth/getCharacteristics/characteristics-not-found.html:9:
let expected = new DOMException('No Characteristics found in service.',
Just inline this?

https://codereview.chromium.org/1723723002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTService.cpp
(right):

https://codereview.chromium.org/1723723002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTService.cpp:71:
HeapVector<Member<BluetoothRemoteGATTCharacteristic>> characteristics;
Use characteristics.reserveInitialCapacity(webCharacteristics.size()) to avoid
reallocations and make sure the result is sized exactly.

https://codereview.chromium.org/1723723002/diff/60001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1723723002/diff/60001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:82529: +  <int value="9"
label="Service.getCharacteristics()"/>
Please update the label on "getCharacteristic()" too, to match.

[ortuno, 2016-02-25 23:08:27]

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

https://codereview.chromium.org/1723723002/diff/60001/content/browser/bluetoo...
content/browser/bluetooth/bluetooth_dispatcher_host.cc:862:
std::vector<std::string> characteristics_instance_ids;
On 2016/02/24 at 23:20:29, Jeffrey Yasskin wrote:
> Can we transpose these three vectors? That is, make an IPC struct holding an
instance_id, a UUID, and a properties bit-array, and send a vector of those? I
don't want to have to rely on 3 parallel arrays matching up.

Ah. I asked a while ago if three arrays were better than an whole new IPC struct
and the conclusion was that three arrays were much less code and therefore OK.

https://codereview.chromium.org/1723723002/diff/60001/content/browser/bluetoo...
content/browser/bluetooth/bluetooth_dispatcher_host.cc:869: characteristic_uuid
== characteristic->GetUUID().canonical_value()) {
On 2016/02/24 at 23:20:29, Jeffrey Yasskin wrote:
> Could you add a TODO to incorporate the blacklist here, in case your and
Vincent's patches collide?

Added blacklist check since Vince's patch landed already.

https://codereview.chromium.org/1723723002/diff/60001/content/browser/bluetoo...
content/browser/bluetooth/bluetooth_dispatcher_host.cc:885:
RecordGetCharacteristicsOutcome(UMAGetCharacteristicOutcome::NOT_FOUND);
On 2016/02/24 at 23:20:29, Jeffrey Yasskin wrote:
> Probably split this by characteristic_uuid.empty() too.

Done.

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

https://codereview.chromium.org/1723723002/diff/60001/content/common/bluetoot...
content/common/bluetooth/bluetooth_messages.h:167: // Informs the renderer that
characteristic request |request_id| succeeded.
On 2016/02/24 at 23:20:29, Jeffrey Yasskin wrote:
> s/characteristic/multiple-characteristic/?

Done.

https://codereview.chromium.org/1723723002/diff/60001/content/renderer/blueto...
File content/renderer/bluetooth/bluetooth_dispatcher.cc (right):

https://codereview.chromium.org/1723723002/diff/60001/content/renderer/blueto...
content/renderer/bluetooth/bluetooth_dispatcher.cc:723: (*characteristics)[i] =
new WebBluetoothRemoteGATTCharacteristicInit(
On 2016/02/24 at 23:20:29, Jeffrey Yasskin wrote:
> I'm sad that we wind up with raw pointers in this vector. Can you store
WebPassOwnPtr<>s or plain instances of WebBluetoothRemoteGATTCharacteristicInit
instead?

WebPassOwnPtr is move only. WebVector doesn't support move only types. I don't
think we should pass the instance since we want to pass the ownership of the web
type all the way down to BluetoothRemoteGATTCharacteristic::take.

I can't wait to move to mojo so we can get rid of these web types.

https://codereview.chromium.org/1723723002/diff/60001/content/renderer/blueto...
content/renderer/bluetooth/bluetooth_dispatcher.cc:725:
request->service_instance_id,
On 2016/02/24 at 23:20:29, Jeffrey Yasskin wrote:
> Nit: I'd put the service instance ID first, so the sequence forms a path:
[device] / service / characteristic.

Done.

https://codereview.chromium.org/1723723002/diff/60001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/bluetooth/getCharacteristics/characteristics-not-found-with-uuid.html
(right):

https://codereview.chromium.org/1723723002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/bluetooth/getCharacteristics/characteristics-not-found-with-uuid.html:17:
service.getCharacteristics(battery_level.alias), expected),
On 2016/02/24 at 23:20:29, Jeffrey Yasskin wrote:
> I don't know that we need the alias/name/uuid triple for this test.

Removed. But I added it to characteristic-found-with-uuid.html otherwise we
would never test if we actually support all three possibilities.

https://codereview.chromium.org/1723723002/diff/60001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/bluetooth/getCharacteristics/characteristics-not-found.html
(right):

https://codereview.chromium.org/1723723002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/bluetooth/getCharacteristics/characteristics-not-found.html:9:
let expected = new DOMException('No Characteristics found in service.',
On 2016/02/24 at 23:20:30, Jeffrey Yasskin wrote:
> Just inline this?

Done.

https://codereview.chromium.org/1723723002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTService.cpp
(right):

https://codereview.chromium.org/1723723002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTService.cpp:71:
HeapVector<Member<BluetoothRemoteGATTCharacteristic>> characteristics;
On 2016/02/24 at 23:20:30, Jeffrey Yasskin wrote:
> Use characteristics.reserveInitialCapacity(webCharacteristics.size()) to avoid
reallocations and make sure the result is sized exactly.

Done.

https://codereview.chromium.org/1723723002/diff/60001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1723723002/diff/60001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:82529: +  <int value="9"
label="Service.getCharacteristics()"/>
On 2016/02/24 at 23:20:30, Jeffrey Yasskin wrote:
> Please update the label on "getCharacteristic()" too, to match.

Done.

[Jeffrey Yasskin, 2016-02-25 23:46:57]

LGTM.

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

https://codereview.chromium.org/1723723002/diff/60001/content/browser/bluetoo...
content/browser/bluetooth/bluetooth_dispatcher_host.cc:862:
std::vector<std::string> characteristics_instance_ids;
On 2016/02/25 23:08:27, ortuno wrote:
> On 2016/02/24 at 23:20:29, Jeffrey Yasskin wrote:
> > Can we transpose these three vectors? That is, make an IPC struct holding an
> instance_id, a UUID, and a properties bit-array, and send a vector of those? I
> don't want to have to rely on 3 parallel arrays matching up.
> 
> Ah. I asked a while ago if three arrays were better than an whole new IPC
struct
> and the conclusion was that three arrays were much less code and therefore OK.

Ah, sorry if I've given two contradictory answers to that question. Once we
switch to Mojo, it'd be nice to switch to the struct.

https://codereview.chromium.org/1723723002/diff/60001/content/renderer/blueto...
File content/renderer/bluetooth/bluetooth_dispatcher.cc (right):

https://codereview.chromium.org/1723723002/diff/60001/content/renderer/blueto...
content/renderer/bluetooth/bluetooth_dispatcher.cc:723: (*characteristics)[i] =
new WebBluetoothRemoteGATTCharacteristicInit(
On 2016/02/25 23:08:27, ortuno wrote:
> On 2016/02/24 at 23:20:29, Jeffrey Yasskin wrote:
> > I'm sad that we wind up with raw pointers in this vector. Can you store
> WebPassOwnPtr<>s or plain instances of
WebBluetoothRemoteGATTCharacteristicInit
> instead?
> 
> WebPassOwnPtr is move only. WebVector doesn't support move only types.

Oh, I see. WebPassOwnPtr doesn't have an assignment operator at all, even a move
assignment. I can't wait until we switch to all standard types.

> I don't
> think we should pass the instance since we want to pass the ownership of the
web
> type all the way down to BluetoothRemoteGATTCharacteristic::take.

Sure.

> I can't wait to move to mojo so we can get rid of these web types.

https://codereview.chromium.org/1723723002/diff/60001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/bluetooth/getCharacteristics/characteristics-not-found-with-uuid.html
(right):

https://codereview.chromium.org/1723723002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/bluetooth/getCharacteristics/characteristics-not-found-with-uuid.html:17:
service.getCharacteristics(battery_level.alias), expected),
On 2016/02/25 23:08:27, ortuno wrote:
> On 2016/02/24 at 23:20:29, Jeffrey Yasskin wrote:
> > I don't know that we need the alias/name/uuid triple for this test.
> 
> Removed. But I added it to characteristic-found-with-uuid.html otherwise we
> would never test if we actually support all three possibilities.

Yep, that looks like a better place.

[ortuno, 2016-02-26 00:09:28]

ortuno@chromium.org changed reviewers:
+ isherman@chromium.org, palmer@chromium.org

[ortuno, 2016-02-26 00:09:29]

palmer: PTAL at bluetooth_messages
isherman: PTAL at histograms.xml

[Ilya Sherman, 2016-02-26 01:16:27]

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

https://codereview.chromium.org/1723723002/diff/80001/content/browser/bluetoo...
content/browser/bluetooth/bluetooth_metrics.cc:184: NOTREACHED() << "No need to
record a success or renderer crash";
nit: Could this text be moved into a comment, rather than a string which will be
compiled into the Chrome binary?

https://codereview.chromium.org/1723723002/diff/80001/content/browser/bluetoo...
content/browser/bluetooth/bluetooth_metrics.cc:200: HashUUID(characteristic));
Just to confirm: Are you reasonably sure that the set of characteristics is not
tremendously large -- say, under 1,000 distinct values on any given day?  It's
important that even sparse histograms don't have too many distinct buckets, when
aggregated across all Chrome clients.

If you're not quite sure, is there a way to whitelist a subset of
characteristics that are particularly interesting?  Or is it the goal to try to
detect whether there are characteristics that we wouldn't even expect to be
common, which nonetheless are?

https://codereview.chromium.org/1723723002/diff/80001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1723723002/diff/80001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:3560: +    what errors developers are
getting so we can target efforst toward the most
nit: s/efforst/efforts

[ortuno, 2016-02-26 16:24:58]

ortuno@chromium.org changed reviewers:
- palmer@chromium.org

[ortuno, 2016-02-26 16:24:59]

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

https://codereview.chromium.org/1723723002/diff/80001/content/browser/bluetoo...
content/browser/bluetooth/bluetooth_metrics.cc:184: NOTREACHED() << "No need to
record a success or renderer crash";
On 2016/02/26 at 01:16:27, Ilya Sherman wrote:
> nit: Could this text be moved into a comment, rather than a string which will
be compiled into the Chrome binary?

Done. Fixed other instances as well.

https://codereview.chromium.org/1723723002/diff/80001/content/browser/bluetoo...
content/browser/bluetooth/bluetooth_metrics.cc:200: HashUUID(characteristic));
On 2016/02/26 at 01:16:27, Ilya Sherman wrote:
> Just to confirm: Are you reasonably sure that the set of characteristics is
not tremendously large -- say, under 1,000 distinct values on any given day? 
It's important that even sparse histograms don't have too many distinct buckets,
when aggregated across all Chrome clients.
> 

What's the reason for this? I checked the histograms for the past couple of
days. The number of unique UUIDs never goes above 50. For now we are under a
flag and only available in whitelisted origins. I opened a bug to revisit this
decision before launching. http://crbug.com/590228

> If you're not quite sure, is there a way to whitelist a subset of
characteristics that are particularly interesting?  Or is it the goal to try to
detect whether there are characteristics that we wouldn't even expect to be
common, which nonetheless are?

The latter.

https://codereview.chromium.org/1723723002/diff/80001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1723723002/diff/80001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:3560: +    what errors developers are
getting so we can target efforst toward the most
On 2016/02/26 at 01:16:27, Ilya Sherman wrote:
> nit: s/efforst/efforts

Done. Also fix the other instance of this typo.

[ortuno, 2016-02-26 16:26:34]

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

[ortuno, 2016-02-26 16:26:34]

tsepez: PTAL at bluetooth_messages

[Jeffrey Yasskin, 2016-02-26 16:42:06]

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

https://codereview.chromium.org/1723723002/diff/80001/content/browser/bluetoo...
content/browser/bluetooth/bluetooth_metrics.cc:200: HashUUID(characteristic));
On 2016/02/26 01:16:27, Ilya Sherman wrote:
> Just to confirm: Are you reasonably sure that the set of characteristics is
not
> tremendously large -- say, under 1,000 distinct values on any given day?  It's
> important that even sparse histograms don't have too many distinct buckets,
when
> aggregated across all Chrome clients.
> 
> If you're not quite sure, is there a way to whitelist a subset of
> characteristics that are particularly interesting?  Or is it the goal to try
to
> detect whether there are characteristics that we wouldn't even expect to be
> common, which nonetheless are?

The set of characteristics could be large; a couple times the number of
Services, where each manufacturer might invent a new service or two for each of
their types of devices.

Maybe we don't need to collect Characteristic UUIDs at all: we're looking for
evidence that particular tasks/protocols are popular enough to build a dedicated
API around, but that'll happen at the Service level, not the Characteristic
level.

The number of Service UUIDs could also be large, but probably not over 1000 a
day? And we'll see the number of services (or characteristics) ramping up over
time, as websites adopt the API, so we'll have time to bail out if it looks too
high.

We could probably subset by adding an enum for known service UUIDs. We'd gather
UUIDs by crawling popular websites, and then add them for the next Chrome
release. This isn't as good as gathering the hashes, since it means we don't
know if a bunch of long-tail sites happen to be using a particular Bluetooth
service a lot, although we could use a high 'unknown' count as a sign to keep
adding UUIDs.

+1 for not discussing too long before the branch point, and fixing it in M51 if
we pick a different route. We definitely won't have "too many" characteristics
before then.

[Tom Sepez, 2016-02-26 16:57:30]

Messages themselves LGTM.

[Ilya Sherman, 2016-02-26 18:00:23]

isherman@chromium.org changed reviewers:
+ vadimb@chromium.org

[Ilya Sherman, 2016-02-26 18:00:25]

/cc Vadim as an FYI about possible impact on our data processing pipeline. 
(TL;DR: Adding a sparse histogram with potentially very many buckets, though
it's expected to grow over time, rather than being huge immediately.)

Metrics LGTM, thanks.

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

https://codereview.chromium.org/1723723002/diff/80001/content/browser/bluetoo...
content/browser/bluetooth/bluetooth_metrics.cc:200: HashUUID(characteristic));
On 2016/02/26 16:24:58, ortuno wrote:
> On 2016/02/26 at 01:16:27, Ilya Sherman wrote:
> > Just to confirm: Are you reasonably sure that the set of characteristics is
> not tremendously large -- say, under 1,000 distinct values on any given day? 
> It's important that even sparse histograms don't have too many distinct
buckets,
> when aggregated across all Chrome clients.
> > 
> 
> What's the reason for this? I checked the histograms for the past couple of
> days. The number of unique UUIDs never goes above 50. For now we are under a
> flag and only available in whitelisted origins. I opened a bug to revisit this
> decision before launching. http://crbug.com/590228

On the server-side processing backend, we aggregated data from all clients, and
build up a single histogram that contains all of the reported buckets.  The data
processing pipeline isn't designed to handle arbitrarily many buckets.

It sounds fine to emit this histogram as-is for now.  If, at some point in the
future, we start to see too many buckets in use, then we might have to find an
alternate approach.

> > If you're not quite sure, is there a way to whitelist a subset of
> characteristics that are particularly interesting?  Or is it the goal to try
to
> detect whether there are characteristics that we wouldn't even expect to be
> common, which nonetheless are?
> 
> The latter.

[Ilya Sherman, 2016-02-26 18:00:41]

Description was changed from

==========
bluetooth: Implement Service.getCharacteristics

1. Adds bindings for getCharacteristics
2. Adds getCharacteristics function to public interface
3. Implements getCharacteristics from public interface
4. Adds necessary IPC messages
5. Modifies HeartRateService so that it has two Body Sensor Location
   characteristics. This way we can test what happens when two characteristics
   have the same UUID. Also modify the GetBaseGATTCharacteristic function to 
   take an identifier. Before we just used the UUID but we need to change it
   now that multiple characteristics have the same UUID.
6. Adds LayoutTests.

BUG=588876
==========

to

==========
bluetooth: Implement Service.getCharacteristics

1. Adds bindings for getCharacteristics
2. Adds getCharacteristics function to public interface
3. Implements getCharacteristics from public interface
4. Adds necessary IPC messages
5. Modifies HeartRateService so that it has two Body Sensor Location
   characteristics. This way we can test what happens when two characteristics
   have the same UUID. Also modify the GetBaseGATTCharacteristic function to 
   take an identifier. Before we just used the UUID but we need to change it
   now that multiple characteristics have the same UUID.
6. Adds LayoutTests.

BUG=588876
==========

[Ilya Sherman, 2016-02-26 18:00:41]

isherman@chromium.org changed reviewers:
- vadimb@chromium.org

[ortuno, 2016-02-26 18:11:48]

The CQ bit was checked by ortuno@chromium.org

[ortuno, 2016-02-26 18:11:49]

The patchset sent to the CQ was uploaded after l-g-t-m from
jyasskin@chromium.org
Link to the patchset: https://codereview.chromium.org/1723723002/#ps100001
(title: "Address isherman's comments")

[commit-bot: I haz the power, 2016-02-26 18:14:43]

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

[commit-bot: I haz the power, 2016-02-26 19:07:47]

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

[commit-bot: I haz the power, 2016-02-26 19:07:49]

Try jobs failed on following builders:
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[ortuno, 2016-02-26 20:01:13]

The CQ bit was checked by ortuno@chromium.org

[ortuno, 2016-02-26 20:01:14]

The patchset sent to the CQ was uploaded after l-g-t-m from tsepez@chromium.org,
jyasskin@chromium.org, isherman@chromium.org
Link to the patchset: https://codereview.chromium.org/1723723002/#ps120001
(title: "Merge with ToT")

[commit-bot: I haz the power, 2016-02-26 20:03:28]

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

[commit-bot: I haz the power, 2016-02-26 20:50:00]

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

[commit-bot: I haz the power, 2016-02-26 20:50:01]

Try jobs failed on following builders:
  linux_android_rel_ng on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[ortuno, 2016-02-26 21:02:19]

The CQ bit was checked by ortuno@chromium.org

[commit-bot: I haz the power, 2016-02-26 21:03:20]

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

[commit-bot: I haz the power, 2016-02-26 22:16:30]

Description was changed from

==========
bluetooth: Implement Service.getCharacteristics

1. Adds bindings for getCharacteristics
2. Adds getCharacteristics function to public interface
3. Implements getCharacteristics from public interface
4. Adds necessary IPC messages
5. Modifies HeartRateService so that it has two Body Sensor Location
   characteristics. This way we can test what happens when two characteristics
   have the same UUID. Also modify the GetBaseGATTCharacteristic function to 
   take an identifier. Before we just used the UUID but we need to change it
   now that multiple characteristics have the same UUID.
6. Adds LayoutTests.

BUG=588876
==========

to

==========
bluetooth: Implement Service.getCharacteristics

1. Adds bindings for getCharacteristics
2. Adds getCharacteristics function to public interface
3. Implements getCharacteristics from public interface
4. Adds necessary IPC messages
5. Modifies HeartRateService so that it has two Body Sensor Location
   characteristics. This way we can test what happens when two characteristics
   have the same UUID. Also modify the GetBaseGATTCharacteristic function to 
   take an identifier. Before we just used the UUID but we need to change it
   now that multiple characteristics have the same UUID.
6. Adds LayoutTests.

BUG=588876
==========

[commit-bot: I haz the power, 2016-02-26 22:16:32]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-02-26 22:17:47]

Description was changed from

==========
bluetooth: Implement Service.getCharacteristics

1. Adds bindings for getCharacteristics
2. Adds getCharacteristics function to public interface
3. Implements getCharacteristics from public interface
4. Adds necessary IPC messages
5. Modifies HeartRateService so that it has two Body Sensor Location
   characteristics. This way we can test what happens when two characteristics
   have the same UUID. Also modify the GetBaseGATTCharacteristic function to 
   take an identifier. Before we just used the UUID but we need to change it
   now that multiple characteristics have the same UUID.
6. Adds LayoutTests.

BUG=588876
==========

to

==========
bluetooth: Implement Service.getCharacteristics

1. Adds bindings for getCharacteristics
2. Adds getCharacteristics function to public interface
3. Implements getCharacteristics from public interface
4. Adds necessary IPC messages
5. Modifies HeartRateService so that it has two Body Sensor Location
   characteristics. This way we can test what happens when two characteristics
   have the same UUID. Also modify the GetBaseGATTCharacteristic function to
   take an identifier. Before we just used the UUID but we need to change it
   now that multiple characteristics have the same UUID.
6. Adds LayoutTests.

BUG=588876

Committed: https://crrev.com/42d4354380744ef74e17b79d5ab4e5f2e42e17f7
Cr-Commit-Position: refs/heads/master@{#378001}
==========

[commit-bot: I haz the power, 2016-02-26 22:17:50]

Patchset 7 (id:??) landed as
https://crrev.com/42d4354380744ef74e17b79d5ab4e5f2e42e17f7
Cr-Commit-Position: refs/heads/master@{#378001}

[sof, 2016-02-29 10:53:27]

sigbjornf@opera.com changed reviewers:
+ sigbjornf@opera.com

[sof, 2016-02-29 10:53:28]

Three of the added tests are reported as leaking,


https://storage.googleapis.com/chromium-layout-test-archives/WebKit_Linux_Lea...

[ortuno, 2016-02-29 16:02:28]

On 2016/02/29 at 10:53:28, sigbjornf wrote:
> Three of the added tests are reported as leaking,
> 
> 
https://storage.googleapis.com/chromium-layout-test-archives/WebKit_Linux_Lea...

Is there any documentation on the leak detector? How do I run it locally?

[sof, 2016-02-29 16:10:37]

On 2016/02/29 16:02:28, ortuno wrote:
> On 2016/02/29 at 10:53:28, sigbjornf wrote:
> > Three of the added tests are reported as leaking,
> > 
> > 
>
https://storage.googleapis.com/chromium-layout-test-archives/WebKit_Linux_Lea...
> 
> Is there any documentation on the leak detector? How do I run it locally?

https://docs.google.com/presentation/d/12bh9_zp91c2_-iEle3TzilKHVcNqYrTkfTtMX...
is one resource.

Add --enable-leak-detection to your run_layout_tests.py command line to use.

[ortuno, 2016-02-29 17:15:45]

On 2016/02/29 at 16:10:37, sigbjornf wrote:
> On 2016/02/29 16:02:28, ortuno wrote:
> > On 2016/02/29 at 10:53:28, sigbjornf wrote:
> > > Three of the added tests are reported as leaking,
> > > 
> > > 
> >
https://storage.googleapis.com/chromium-layout-test-archives/WebKit_Linux_Lea...
> > 
> > Is there any documentation on the leak detector? How do I run it locally?
> 
>
https://docs.google.com/presentation/d/12bh9_zp91c2_-iEle3TzilKHVcNqYrTkfTtMX...
is one resource.
> 
> Add --enable-leak-detection to your run_layout_tests.py command line to use.

Thanks, I was able to reproduce locally. Will look into it.