bluetooth: Use WebBluetoothDeviceId instead of string

bluetooth: Use WebBluetoothDeviceId instead of string

This adds more type safety to our current code and also allows us check the format
of the string that we get from the renderer.

Also moves the chromium variant bindings generation out of blink.gyp and into
content_common_mojo_bindings.gyp. This avoids adding a dependency to content/common
in blink.gyp since now the chromium bindings depend on
content/common/bluetooth/web_bluetooth_device_id

CQ_INCLUDE_TRYJOBS=master.tryserver.chromium.mac:mac_chromium_gyp_rel,master.tryserver.chromium.mac:mac_chromium_gyp_dbg

BUG=577962
Committed: https://crrev.com/bd27a25399b274afabd4338a28c5c1f72b37c78c
Committed: https://crrev.com/d0b0acd381a26f12f629092423f533d6179de0e9
Cr-Original-Commit-Position: refs/heads/master@{#405806}
Cr-Commit-Position: refs/heads/master@{#409820}

[ortuno, 2016-06-01 18:10:44]

Description was changed from

==========
bluetooth: (WIP) Introduces BluetoothDeviceId

BUG=
==========

to

==========
bluetooth: Use BluetoothDeviceId instead of string

This new class adds type-safety when dealing with device id's and also allows us
to do validation when sending them over IPC.

BUG=577962
==========

[ortuno, 2016-06-01 18:11:53]

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

[ortuno, 2016-06-01 18:11:54]

rockot: PTAL at mojo bits in content/common/. More specifically are the
struct_traits/mojoms in the right place?

[Ken Rockot(use gerrit already), 2016-06-01 19:15:46]

lgtm

[ortuno, 2016-06-01 19:26:59]

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

[ortuno, 2016-06-01 19:27:00]

jyasskin: PTAL

[Jeffrey Yasskin, 2016-06-03 17:22:05]

https://codereview.chromium.org/2019853002/diff/100001/content/browser/blueto...
File content/browser/bluetooth/bluetooth_allowed_devices_map.cc (right):

https://codereview.chromium.org/2019853002/diff/100001/content/browser/blueto...
content/browser/bluetooth/bluetooth_allowed_devices_map.cc:95: return
base::make_optional(id_iter->second);
It'd be nice to avoid needing a copy here. Have this function return a pointer
instead?

https://codereview.chromium.org/2019853002/diff/100001/content/common/bluetoo...
File content/common/bluetooth/bluetooth_device_id.cc (right):

https://codereview.chromium.org/2019853002/diff/100001/content/common/bluetoo...
content/common/bluetooth/bluetooth_device_id.cc:21:
BluetoothDeviceId::BluetoothDeviceId(const std::string& device_id) {
Take the string by value, and then move it into device_id_.

It's also a little better to initialize device_id_ as ": device_id_(device_id) {
..." than "{ device_id_ = device_id; }".

https://codereview.chromium.org/2019853002/diff/100001/content/common/bluetoo...
content/common/bluetooth/bluetooth_device_id.cc:51: if
(!base::Base64Decode(device_id, &decoded)) {
There are some strings that will successfully decode, but aren't the output of
any real base64 encoding. Specifically if the bottom 4 bits of the last 6-bit
block aren't 0, they're just ignored by Base64Decode. I think we can ignore
that?

https://codereview.chromium.org/2019853002/diff/100001/content/common/bluetoo...
content/common/bluetooth/bluetooth_device_id.cc:60: DCHECK_EQ(24u,
device_id.size());
I think we don't need these: the Base64Decode won't produce a 16-byte result if
they're not true.

https://codereview.chromium.org/2019853002/diff/100001/content/common/bluetoo...
content/common/bluetooth/bluetooth_device_id.cc:68: DCHECK(IsValid(device_id_)
&& IsValid(device_id.device_id_));
Since str() does these DCHECKs again, I'd compare .device_id_ directly.

https://codereview.chromium.org/2019853002/diff/100001/content/common/bluetoo...
content/common/bluetooth/bluetooth_device_id.cc:73: bool
BluetoothDeviceId::operator!=(const BluetoothDeviceId& device_id) const {
Generally implement != as "return !(*this == device_id);"

https://codereview.chromium.org/2019853002/diff/100001/content/common/bluetoo...
File content/common/bluetooth/bluetooth_device_id.h (right):

https://codereview.chromium.org/2019853002/diff/100001/content/common/bluetoo...
content/common/bluetooth/bluetooth_device_id.h:17: class CONTENT_EXPORT
BluetoothDeviceId {
I wish I could think of a way to make it clear that this is the origin-specific
ID, and not, say, the ID that BluetoothDevice::GetIdentifier() returns. But I
don't have a good alternative name (BluetoothDeviceIdForOrigin is too long), so
this is fine.

https://codereview.chromium.org/2019853002/diff/100001/content/common/bluetoo...
content/common/bluetooth/bluetooth_device_id.h:19: // Default constructor that
does nothing. We implement it so that instances
s/does nothing/creates an invalid Id/

https://codereview.chromium.org/2019853002/diff/100001/content/common/bluetoo...
content/common/bluetooth/bluetooth_device_id.h:33: // string and
base64-enconding it.
sp: enconding

https://codereview.chromium.org/2019853002/diff/100001/content/common/bluetoo...
content/common/bluetooth/bluetooth_device_id.h:47: void CONTENT_EXPORT
PrintTo(const BluetoothDeviceId& device_id,
I'd prefer to just define operator<< so that we can stream these to LOGs easily.
It's also not banned anymore:
https://google.github.io/styleguide/cppguide.html#Streams.

https://codereview.chromium.org/2019853002/diff/100001/content/common/bluetoo...
File content/common/bluetooth/bluetooth_device_id_struct_traits.h (right):

https://codereview.chromium.org/2019853002/diff/100001/content/common/bluetoo...
content/common/bluetooth/bluetooth_device_id_struct_traits.h:33: *output =
content::BluetoothDeviceId(result);
std::move(result)?

https://codereview.chromium.org/2019853002/diff/100001/content/common/bluetoo...
File content/common/bluetooth/bluetooth_device_id_unittest.cc (right):

https://codereview.chromium.org/2019853002/diff/100001/content/common/bluetoo...
content/common/bluetooth/bluetooth_device_id_unittest.cc:31:
ASSERT_TRUE(BluetoothDeviceId::IsValid(valid_id.str()));
Also EXPECT_FALSE(BluetoothDeviceId::IsValid(default_id1));

https://codereview.chromium.org/2019853002/diff/100001/content/common/bluetoo...
content/common/bluetooth/bluetooth_device_id_unittest.cc:43:
TEST(BluetoothDeviceIdTest, StrConstructor) {
Assert on == and != too.

https://codereview.chromium.org/2019853002/diff/100001/content/common/bluetoo...
content/common/bluetooth/bluetooth_device_id_unittest.cc:47: EXPECT_TRUE(true);
This doesn't add anything: the test will fail if it crashes and won't run the
EXPECT_TRUE anyway.

https://codereview.chromium.org/2019853002/diff/100001/content/common/bluetoo...
content/common/bluetooth/bluetooth_device_id_unittest.cc:76: << "This should
never fail.";
That's implied by it being in a test. :) Do you mean that if it's broken it
might fail flakily, and folks should ignore that less than usual?

https://codereview.chromium.org/2019853002/diff/100001/content/renderer/bluet...
File content/renderer/bluetooth/web_bluetooth_impl.cc (right):

https://codereview.chromium.org/2019853002/diff/100001/content/renderer/bluet...
content/renderer/bluetooth/web_bluetooth_impl.cc:50:
connected_devices_[device_id.utf8()] = device;
Could you convert connected_devices_ to store BluetoothDeviceIds too?

https://codereview.chromium.org/2019853002/diff/100001/third_party/WebKit/pub...
File third_party/WebKit/public/BUILD.gn (right):

https://codereview.chromium.org/2019853002/diff/100001/third_party/WebKit/pub...
third_party/WebKit/public/BUILD.gn:189:
"//content/common/bluetooth:content_mojom_bluetooth_device_id",
This seems like a wrong-direction dependency. Check with Elliott what we're
supposed to do here?

https://codereview.chromium.org/2019853002/diff/100001/third_party/WebKit/pub...
File third_party/WebKit/public/platform/modules/bluetooth/web_bluetooth.mojom
(right):

https://codereview.chromium.org/2019853002/diff/100001/third_party/WebKit/pub...
third_party/WebKit/public/platform/modules/bluetooth/web_bluetooth.mojom:109: //
TODO(ortuno): Define Bluetooth Service.
OT: I think this TODO is obsolete.

https://codereview.chromium.org/2019853002/diff/100001/third_party/WebKit/pub...
third_party/WebKit/public/platform/modules/bluetooth/web_bluetooth.mojom:127:
RemoteServerConnect(content.mojom.BluetoothDeviceId device_id) =>
(WebBluetoothError error);
I wish we had 'using' declarations in mojo.

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

Thanks!

https://codereview.chromium.org/2019853002/diff/100001/content/browser/blueto...
File content/browser/bluetooth/bluetooth_allowed_devices_map.cc (right):

https://codereview.chromium.org/2019853002/diff/100001/content/browser/blueto...
content/browser/bluetooth/bluetooth_allowed_devices_map.cc:95: return
base::make_optional(id_iter->second);
On 2016/06/03 at 17:22:04, Jeffrey Yasskin wrote:
> It'd be nice to avoid needing a copy here. Have this function return a pointer
instead?

Done.

https://codereview.chromium.org/2019853002/diff/100001/content/common/bluetoo...
File content/common/bluetooth/bluetooth_device_id.cc (right):

https://codereview.chromium.org/2019853002/diff/100001/content/common/bluetoo...
content/common/bluetooth/bluetooth_device_id.cc:21:
BluetoothDeviceId::BluetoothDeviceId(const std::string& device_id) {
On 2016/06/03 at 17:22:05, Jeffrey Yasskin wrote:
> Take the string by value, and then move it into device_id_.
> 

Done. Why is it better? Either way results in only one copy being made, right?

> It's also a little better to initialize device_id_ as ": device_id_(device_id)
{ ..." than "{ device_id_ = device_id; }".

Done.

https://codereview.chromium.org/2019853002/diff/100001/content/common/bluetoo...
content/common/bluetooth/bluetooth_device_id.cc:51: if
(!base::Base64Decode(device_id, &decoded)) {
On 2016/06/03 at 17:22:04, Jeffrey Yasskin wrote:
> There are some strings that will successfully decode, but aren't the output of
any real base64 encoding. Specifically if the bottom 4 bits of the last 6-bit
block aren't 0, they're just ignored by Base64Decode. I think we can ignore
that?

I can't see how someone could use that for any type of attack. But we could add
a check to make sure that the third-to-last character is one of the 4 expected
characters.

https://codereview.chromium.org/2019853002/diff/100001/content/common/bluetoo...
content/common/bluetooth/bluetooth_device_id.cc:60: DCHECK_EQ(24u,
device_id.size());
On 2016/06/03 at 17:22:05, Jeffrey Yasskin wrote:
> I think we don't need these: the Base64Decode won't produce a 16-byte result
if they're not true.

Done. This was just me being paranoid.

https://codereview.chromium.org/2019853002/diff/100001/content/common/bluetoo...
content/common/bluetooth/bluetooth_device_id.cc:68: DCHECK(IsValid(device_id_)
&& IsValid(device_id.device_id_));
On 2016/06/03 at 17:22:05, Jeffrey Yasskin wrote:
> Since str() does these DCHECKs again, I'd compare .device_id_ directly.

Done.

https://codereview.chromium.org/2019853002/diff/100001/content/common/bluetoo...
content/common/bluetooth/bluetooth_device_id.cc:73: bool
BluetoothDeviceId::operator!=(const BluetoothDeviceId& device_id) const {
On 2016/06/03 at 17:22:04, Jeffrey Yasskin wrote:
> Generally implement != as "return !(*this == device_id);"

Ok! Done.

https://codereview.chromium.org/2019853002/diff/100001/content/common/bluetoo...
File content/common/bluetooth/bluetooth_device_id.h (right):

https://codereview.chromium.org/2019853002/diff/100001/content/common/bluetoo...
content/common/bluetooth/bluetooth_device_id.h:17: class CONTENT_EXPORT
BluetoothDeviceId {
On 2016/06/03 at 17:22:05, Jeffrey Yasskin wrote:
> I wish I could think of a way to make it clear that this is the
origin-specific ID, and not, say, the ID that BluetoothDevice::GetIdentifier()
returns. But I don't have a good alternative name (BluetoothDeviceIdForOrigin is
too long), so this is fine.

Ack.

https://codereview.chromium.org/2019853002/diff/100001/content/common/bluetoo...
content/common/bluetooth/bluetooth_device_id.h:19: // Default constructor that
does nothing. We implement it so that instances
On 2016/06/03 at 17:22:05, Jeffrey Yasskin wrote:
> s/does nothing/creates an invalid Id/

Done.

https://codereview.chromium.org/2019853002/diff/100001/content/common/bluetoo...
content/common/bluetooth/bluetooth_device_id.h:33: // string and
base64-enconding it.
On 2016/06/03 at 17:22:05, Jeffrey Yasskin wrote:
> sp: enconding

Done.

https://codereview.chromium.org/2019853002/diff/100001/content/common/bluetoo...
content/common/bluetooth/bluetooth_device_id.h:47: void CONTENT_EXPORT
PrintTo(const BluetoothDeviceId& device_id,
On 2016/06/03 at 17:22:05, Jeffrey Yasskin wrote:
> I'd prefer to just define operator<< so that we can stream these to LOGs
easily. It's also not banned anymore:
https://google.github.io/styleguide/cppguide.html#Streams.

Done.

https://codereview.chromium.org/2019853002/diff/100001/content/common/bluetoo...
File content/common/bluetooth/bluetooth_device_id_struct_traits.h (right):

https://codereview.chromium.org/2019853002/diff/100001/content/common/bluetoo...
content/common/bluetooth/bluetooth_device_id_struct_traits.h:33: *output =
content::BluetoothDeviceId(result);
On 2016/06/03 at 17:22:05, Jeffrey Yasskin wrote:
> std::move(result)?

Done.

https://codereview.chromium.org/2019853002/diff/100001/content/common/bluetoo...
File content/common/bluetooth/bluetooth_device_id_unittest.cc (right):

https://codereview.chromium.org/2019853002/diff/100001/content/common/bluetoo...
content/common/bluetooth/bluetooth_device_id_unittest.cc:31:
ASSERT_TRUE(BluetoothDeviceId::IsValid(valid_id.str()));
On 2016/06/03 at 17:22:05, Jeffrey Yasskin wrote:
> Also EXPECT_FALSE(BluetoothDeviceId::IsValid(default_id1));

IsValid takes a string and we can't get a string because str DCHECKs. But I did
add an ASSERT_DEATH(default_id1.str(), ""), which makes sense and I should have
included to begin with.

https://codereview.chromium.org/2019853002/diff/100001/content/common/bluetoo...
content/common/bluetooth/bluetooth_device_id_unittest.cc:43:
TEST(BluetoothDeviceIdTest, StrConstructor) {
On 2016/06/03 at 17:22:05, Jeffrey Yasskin wrote:
> Assert on == and != too.

Do you mean add statements that compare the objects returned by this
constructor? If so, done.

https://codereview.chromium.org/2019853002/diff/100001/content/common/bluetoo...
content/common/bluetooth/bluetooth_device_id_unittest.cc:47: EXPECT_TRUE(true);
On 2016/06/03 at 17:22:05, Jeffrey Yasskin wrote:
> This doesn't add anything: the test will fail if it crashes and won't run the
EXPECT_TRUE anyway.

Done.

https://codereview.chromium.org/2019853002/diff/100001/content/common/bluetoo...
content/common/bluetooth/bluetooth_device_id_unittest.cc:76: << "This should
never fail.";
On 2016/06/03 at 17:22:05, Jeffrey Yasskin wrote:
> That's implied by it being in a test. :) Do you mean that if it's broken it
might fail flakily, and folks should ignore that less than usual?

Hmm actually the opposite haha. I'm afraid that since the string is random it
might find a bug in Base64Encode. So if this test ever fails we should be
notified.

https://codereview.chromium.org/2019853002/diff/100001/content/renderer/bluet...
File content/renderer/bluetooth/web_bluetooth_impl.cc (right):

https://codereview.chromium.org/2019853002/diff/100001/content/renderer/bluet...
content/renderer/bluetooth/web_bluetooth_impl.cc:50:
connected_devices_[device_id.utf8()] = device;
On 2016/06/03 at 17:22:05, Jeffrey Yasskin wrote:
> Could you convert connected_devices_ to store BluetoothDeviceIds too?

Done.

https://codereview.chromium.org/2019853002/diff/100001/third_party/WebKit/pub...
File third_party/WebKit/public/BUILD.gn (right):

https://codereview.chromium.org/2019853002/diff/100001/third_party/WebKit/pub...
third_party/WebKit/public/BUILD.gn:189:
"//content/common/bluetooth:content_mojom_bluetooth_device_id",
On 2016/06/03 at 17:22:05, Jeffrey Yasskin wrote:
> This seems like a wrong-direction dependency. Check with Elliott what we're
supposed to do here?

It does seem weird to depend on content/ from blink. But it's only really the
bindings that depend on it. Blink code will never see the BluetoothDeviceId
type. One option is to move the type to components/web_bluetooth/ which aligns
with the direction jam and rockot want us to go, but it's basically the same but
without the word "content" in the path.

https://codereview.chromium.org/2019853002/diff/100001/third_party/WebKit/pub...
File third_party/WebKit/public/platform/modules/bluetooth/web_bluetooth.mojom
(right):

https://codereview.chromium.org/2019853002/diff/100001/third_party/WebKit/pub...
third_party/WebKit/public/platform/modules/bluetooth/web_bluetooth.mojom:109: //
TODO(ortuno): Define Bluetooth Service.
On 2016/06/03 at 17:22:05, Jeffrey Yasskin wrote:
> OT: I think this TODO is obsolete.

http://crrev.com/2041143002 :)

https://codereview.chromium.org/2019853002/diff/100001/third_party/WebKit/pub...
third_party/WebKit/public/platform/modules/bluetooth/web_bluetooth.mojom:127:
RemoteServerConnect(content.mojom.BluetoothDeviceId device_id) =>
(WebBluetoothError error);
On 2016/06/03 at 17:22:05, Jeffrey Yasskin wrote:
> I wish we had 'using' declarations in mojo.

Me too :(

[Jeffrey Yasskin, 2016-06-06 23:44:02]

lgtm

https://codereview.chromium.org/2019853002/diff/100001/content/common/bluetoo...
File content/common/bluetooth/bluetooth_device_id.cc (right):

https://codereview.chromium.org/2019853002/diff/100001/content/common/bluetoo...
content/common/bluetooth/bluetooth_device_id.cc:21:
BluetoothDeviceId::BluetoothDeviceId(const std::string& device_id) {
On 2016/06/06 22:22:59, ortuno wrote:
> On 2016/06/03 at 17:22:05, Jeffrey Yasskin wrote:
> > Take the string by value, and then move it into device_id_.
> > 
> 
> Done. Why is it better? Either way results in only one copy being made, right?

Not quite: in the call at
https://codereview.chromium.org/2019853002/diff/120001/content/common/bluetoo...,
moving the argument into this constructor means there aren't any copies. If this
took |device_id| by const&, there would be one copy.

In the call in BluetoothDeviceId::Create(), the fact that |bytes| is passed
without a move means there's one unnecessary copy, which we could remove by
changing that to "BluetoothDeviceId(std::move(bytes))".

https://codereview.chromium.org/2019853002/diff/100001/content/common/bluetoo...
content/common/bluetooth/bluetooth_device_id.cc:51: if
(!base::Base64Decode(device_id, &decoded)) {
On 2016/06/06 22:22:59, ortuno wrote:
> On 2016/06/03 at 17:22:04, Jeffrey Yasskin wrote:
> > There are some strings that will successfully decode, but aren't the output
of
> any real base64 encoding. Specifically if the bottom 4 bits of the last 6-bit
> block aren't 0, they're just ignored by Base64Decode. I think we can ignore
> that?
> 
> I can't see how someone could use that for any type of attack. But we could
add
> a check to make sure that the third-to-last character is one of the 4 expected
> characters.

Right. I don't think we need to check it, but we should comment it. Include or
omit the check depending on which produces a simpler comment?

https://codereview.chromium.org/2019853002/diff/100001/content/common/bluetoo...
File content/common/bluetooth/bluetooth_device_id_unittest.cc (right):

https://codereview.chromium.org/2019853002/diff/100001/content/common/bluetoo...
content/common/bluetooth/bluetooth_device_id_unittest.cc:31:
ASSERT_TRUE(BluetoothDeviceId::IsValid(valid_id.str()));
On 2016/06/06 22:23:00, ortuno wrote:
> On 2016/06/03 at 17:22:05, Jeffrey Yasskin wrote:
> > Also EXPECT_FALSE(BluetoothDeviceId::IsValid(default_id1));
> 
> IsValid takes a string and we can't get a string because str DCHECKs. But I
did
> add an ASSERT_DEATH(default_id1.str(), ""), which makes sense and I should
have
> included to begin with.

Oh, right. What you've done looks good.

https://codereview.chromium.org/2019853002/diff/100001/content/common/bluetoo...
content/common/bluetooth/bluetooth_device_id_unittest.cc:43:
TEST(BluetoothDeviceIdTest, StrConstructor) {
On 2016/06/06 22:23:00, ortuno wrote:
> On 2016/06/03 at 17:22:05, Jeffrey Yasskin wrote:
> > Assert on == and != too.
> 
> Do you mean add statements that compare the objects returned by this
> constructor? If so, done.

Yep, that. Thanks.

https://codereview.chromium.org/2019853002/diff/100001/content/common/bluetoo...
content/common/bluetooth/bluetooth_device_id_unittest.cc:76: << "This should
never fail.";
On 2016/06/06 22:23:00, ortuno wrote:
> On 2016/06/03 at 17:22:05, Jeffrey Yasskin wrote:
> > That's implied by it being in a test. :) Do you mean that if it's broken it
> might fail flakily, and folks should ignore that less than usual?
> 
> Hmm actually the opposite haha. I'm afraid that since the string is random it
> might find a bug in Base64Encode. So if this test ever fails we should be
> notified.

Say something like that in the failure message.

https://codereview.chromium.org/2019853002/diff/100001/third_party/WebKit/pub...
File third_party/WebKit/public/BUILD.gn (right):

https://codereview.chromium.org/2019853002/diff/100001/third_party/WebKit/pub...
third_party/WebKit/public/BUILD.gn:189:
"//content/common/bluetooth:content_mojom_bluetooth_device_id",
On 2016/06/06 22:23:00, ortuno wrote:
> On 2016/06/03 at 17:22:05, Jeffrey Yasskin wrote:
> > This seems like a wrong-direction dependency. Check with Elliott what we're
> supposed to do here?
> 
> It does seem weird to depend on content/ from blink. But it's only really the
> bindings that depend on it. Blink code will never see the BluetoothDeviceId
> type. One option is to move the type to components/web_bluetooth/ which aligns
> with the direction jam and rockot want us to go, but it's basically the same
but
> without the word "content" in the path.

I guess content/common doesn't depend on WebKit, so there aren't any cycles
here. I'm happy leaving it like this until your WebKit reviewer complains. ;)

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

https://codereview.chromium.org/2019853002/diff/120001/content/browser/blueto...
content/browser/bluetooth/bluetooth_allowed_devices_map.cc:46: VLOG(1) << "Id
generated for device: " << device_id.str();
You can omit the .str() now.

[ortuno, 2016-06-24 17:39:43]

rockot: Could you take another look? This is not yet finished. I still need to
do gyp (T_T) but I wanted to make sure this is the right structure first.

https://codereview.chromium.org/2019853002/diff/100001/content/common/bluetoo...
File content/common/bluetooth/bluetooth_device_id.cc (right):

https://codereview.chromium.org/2019853002/diff/100001/content/common/bluetoo...
content/common/bluetooth/bluetooth_device_id.cc:21:
BluetoothDeviceId::BluetoothDeviceId(const std::string& device_id) {
On 2016/06/06 at 23:44:02, jyasskin (OOO until July 11) wrote:
> On 2016/06/06 22:22:59, ortuno wrote:
> > On 2016/06/03 at 17:22:05, Jeffrey Yasskin wrote:
> > > Take the string by value, and then move it into device_id_.
> > > 
> > 
> > Done. Why is it better? Either way results in only one copy being made,
right?
> 
> Not quite: in the call at
https://codereview.chromium.org/2019853002/diff/120001/content/common/bluetoo...,
moving the argument into this constructor means there aren't any copies. If this
took |device_id| by const&, there would be one copy.
> 

Ah right. Forgot about the move in the struct traits.

> In the call in BluetoothDeviceId::Create(), the fact that |bytes| is passed
without a move means there's one unnecessary copy, which we could remove by
changing that to "BluetoothDeviceId(std::move(bytes))".

Done.

https://codereview.chromium.org/2019853002/diff/100001/content/common/bluetoo...
content/common/bluetooth/bluetooth_device_id.cc:51: if
(!base::Base64Decode(device_id, &decoded)) {
On 2016/06/06 at 23:44:02, jyasskin (OOO until July 11) wrote:
> On 2016/06/06 22:22:59, ortuno wrote:
> > On 2016/06/03 at 17:22:04, Jeffrey Yasskin wrote:
> > > There are some strings that will successfully decode, but aren't the
output of
> > any real base64 encoding. Specifically if the bottom 4 bits of the last
6-bit
> > block aren't 0, they're just ignored by Base64Decode. I think we can ignore
> > that?
> > 
> > I can't see how someone could use that for any type of attack. But we could
add
> > a check to make sure that the third-to-last character is one of the 4
expected
> > characters.
> 
> Right. I don't think we need to check it, but we should comment it. Include or
omit the check depending on which produces a simpler comment?

Added an if statement for it.

https://codereview.chromium.org/2019853002/diff/100001/content/common/bluetoo...
File content/common/bluetooth/bluetooth_device_id_unittest.cc (right):

https://codereview.chromium.org/2019853002/diff/100001/content/common/bluetoo...
content/common/bluetooth/bluetooth_device_id_unittest.cc:76: << "This should
never fail.";
On 2016/06/06 at 23:44:02, jyasskin (OOO until July 11) wrote:
> On 2016/06/06 22:23:00, ortuno wrote:
> > On 2016/06/03 at 17:22:05, Jeffrey Yasskin wrote:
> > > That's implied by it being in a test. :) Do you mean that if it's broken
it
> > might fail flakily, and folks should ignore that less than usual?
> > 
> > Hmm actually the opposite haha. I'm afraid that since the string is random
it
> > might find a bug in Base64Encode. So if this test ever fails we should be
> > notified.
> 
> Say something like that in the failure message.

After some more thinking I decided to remove the message. I think it does not
make sense to test for a bug in Base64 in our code. A fuzzer should be used to
find any bugs in it.

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

https://codereview.chromium.org/2019853002/diff/120001/content/browser/blueto...
content/browser/bluetooth/bluetooth_allowed_devices_map.cc:46: VLOG(1) << "Id
generated for device: " << device_id.str();
On 2016/06/06 at 23:44:02, jyasskin (OOO until July 11) wrote:
> You can omit the .str() now.

Done.

[ortuno, 2016-06-24 22:08:39]

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

[commit-bot: I haz the power, 2016-06-24 22:09:12]

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

[ortuno, 2016-06-24 22:11:45]

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

[commit-bot: I haz the power, 2016-06-24 22:12:06]

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, 2016-06-24 22:28:24]

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

[commit-bot: I haz the power, 2016-06-24 22:28:25]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)

[Ken Rockot(use gerrit already), 2016-06-30 14:54:14]

Oops, sorry for the delay. Still LGTM. I lost track of this because I had
already LG'd it once. Please pester me on IM if I ever take more than a day to
respond to a review request. :P

[ortuno, 2016-07-07 01:15:04]

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

[commit-bot: I haz the power, 2016-07-07 01:15:37]

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

[ortuno, 2016-07-07 01:29:49]

Description was changed from

==========
bluetooth: Use BluetoothDeviceId instead of string

This new class adds type-safety when dealing with device id's and also allows us
to do validation when sending them over IPC.

BUG=577962
==========

to

==========
bluetooth: Use BluetoothDeviceId instead of string

Adds "web_bluetooth" to components. This folder will eventually hold the 
web_bluetooth.mojom (and maybe its implementation) which will be consumed by
blink.

For now though, this folder only contains the WebBluetoothDeviceId type and its
typemap. WebBluetoothDeviceId is used in web_bluetooth.mojom and in content to
add
type-safety when dealing with device ids and also allows us to do validation
when
sending them over IPC.

BUG=577962
==========

[commit-bot: I haz the power, 2016-07-07 01:33:09]

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

[commit-bot: I haz the power, 2016-07-07 01:33:10]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[ortuno, 2016-07-07 01:52:43]

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

[commit-bot: I haz the power, 2016-07-07 01:53:23]

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, 2016-07-07 03:09:46]

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

[commit-bot: I haz the power, 2016-07-07 03:09:47]

Dry run: This issue passed the CQ dry run.

[ortuno, 2016-07-08 15:59:52]

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

[ortuno, 2016-07-08 15:59:54]

jbroman: esprehn is OOO, would you mind taking a look at the blink part in the
mean time?

[jbroman, 2016-07-08 17:27:59]

third_party/WebKit/ LGTM (but you'll need someone from public/OWNERS to land)

[ortuno, 2016-07-08 17:37:20]

Thanks!

[dcheng, 2016-07-11 01:54:58]

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

[dcheng, 2016-07-11 01:55:00]

rs lgtm for //third_party/WebKit/public changes.

[ortuno, 2016-07-11 16:20:04]

ortuno@chromium.org changed reviewers:
+ blundell@chromium.org, jam@chromium.org, palmer@chromium.org

[ortuno, 2016-07-11 16:20:05]

blundell: PTAL at inclusion of new folder in components/ 

jam: PTAL at content/browser/BUILD.gn, content/browser/DEPS and
content/test/BUILD.gn 

palmer: PTAL at mojo changes in components/web_bluetooth/public/interfaces/ and
third_party/WebKit/public/platform/modules/bluetooth/web_bluetooth.mojom

[palmer, 2016-07-11 18:34:50]

lgtm

https://codereview.chromium.org/2019853002/diff/280001/components/web_bluetoo...
File components/web_bluetooth/web_bluetooth_device_id_unittest.cc (right):

https://codereview.chromium.org/2019853002/diff/280001/components/web_bluetoo...
components/web_bluetooth/web_bluetooth_device_id_unittest.cc:25:
TEST(WebBluetoothDeviceIdTest, DefaulConstructor) {
Typo: DefaultConstructor

[jam, 2016-07-11 19:45:45]

(transcribing in-person discussions)

It's not clear to me that we need a new component. web_bluetooth_device_id.mojom
can move along
third_party/WebKit/public/platform/modules/bluetooth/web_bluetooth.mojom which
consumes it. The typemaps can stay in content which is the only directory that
needs them. Given that we don't know yet whether we'll need a new component for
bluetooth (i.e. with the addition of permission service and security checks
through mojo pipe capability passing), let's be conservative in adding new
components/ and reconsider when a new use case comes up.

[ortuno, 2016-07-12 00:15:03]

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

[commit-bot: I haz the power, 2016-07-12 00:16:16]

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, 2016-07-12 00:30:24]

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

[commit-bot: I haz the power, 2016-07-12 00:30:26]

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

[ortuno, 2016-07-12 17:17:43]

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

[commit-bot: I haz the power, 2016-07-12 17:19:00]

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

[ortuno, 2016-07-13 19:33:27]

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

[commit-bot: I haz the power, 2016-07-13 19:35:35]

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, 2016-07-13 20:59:26]

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

[commit-bot: I haz the power, 2016-07-13 20:59:27]

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

[ortuno, 2016-07-13 22:53:02]

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

[ortuno, 2016-07-13 22:53:04]

removing blundell since we no longer add a component. The latest patch set only
moves some files from components to content/common so just pinging two people.

jbroman: PTAL at t/WebKit
jam: PTAL at content/common and content/*.gyp*

https://codereview.chromium.org/2019853002/diff/280001/components/web_bluetoo...
File components/web_bluetooth/web_bluetooth_device_id_unittest.cc (right):

https://codereview.chromium.org/2019853002/diff/280001/components/web_bluetoo...
components/web_bluetooth/web_bluetooth_device_id_unittest.cc:25:
TEST(WebBluetoothDeviceIdTest, DefaulConstructor) {
On 2016/07/11 at 18:34:49, palmer wrote:
> Typo: DefaultConstructor

Done.

[jbroman, 2016-07-14 15:14:24]

I'm a little confused by the gyp changes.

Also, please update the CL description (it still mentions
components/web_bluetooth).

https://codereview.chromium.org/2019853002/diff/400001/third_party/WebKit/pub...
File third_party/WebKit/public/blink.gyp (left):

https://codereview.chromium.org/2019853002/diff/400001/third_party/WebKit/pub...
third_party/WebKit/public/blink.gyp:164:
'../../../device/bluetooth/bluetooth.gyp:bluetooth_mojom',
Why are these no longer needed? device.mojom.BluetoothUUID still seems to be in
use.

https://codereview.chromium.org/2019853002/diff/400001/third_party/WebKit/pub...
File third_party/WebKit/public/blink.gyp (right):

https://codereview.chromium.org/2019853002/diff/400001/third_party/WebKit/pub...
third_party/WebKit/public/blink.gyp:116:
'platform/modules/bluetooth/web_bluetooth.mojom',
Why is this being taken out of the Chromium variant? I'd expect both targets to
include the same .mojom files.

[ortuno, 2016-07-14 16:57:09]

Description was changed from

==========
bluetooth: Use BluetoothDeviceId instead of string

Adds "web_bluetooth" to components. This folder will eventually hold the 
web_bluetooth.mojom (and maybe its implementation) which will be consumed by
blink.

For now though, this folder only contains the WebBluetoothDeviceId type and its
typemap. WebBluetoothDeviceId is used in web_bluetooth.mojom and in content to
add
type-safety when dealing with device ids and also allows us to do validation
when
sending them over IPC.

BUG=577962
==========

to

==========
bluetooth: Use BluetoothDeviceId instead of string

Adds "web_bluetooth" to components. This folder will eventually hold the 
web_bluetooth.mojom (and maybe its implementation) which will be consumed by
blink.

For now though, this folder only contains the WebBluetoothDeviceId type and its
typemap. WebBluetoothDeviceId is used in web_bluetooth.mojom and in content to
add
type-safety when dealing with device ids and also allows us to do validation
when
sending them over IPC.

CQ_INCLUDE_TRYJOBS=master.tryserver.chromium.mac:mac_chromium_gyp_rel,master.tryserver.chromium.mac:mac_chromium_gyp_dbg

BUG=577962
==========

[ortuno, 2016-07-14 17:04:07]

Description was changed from

==========
bluetooth: Use BluetoothDeviceId instead of string

Adds "web_bluetooth" to components. This folder will eventually hold the 
web_bluetooth.mojom (and maybe its implementation) which will be consumed by
blink.

For now though, this folder only contains the WebBluetoothDeviceId type and its
typemap. WebBluetoothDeviceId is used in web_bluetooth.mojom and in content to
add
type-safety when dealing with device ids and also allows us to do validation
when
sending them over IPC.

CQ_INCLUDE_TRYJOBS=master.tryserver.chromium.mac:mac_chromium_gyp_rel,master.tryserver.chromium.mac:mac_chromium_gyp_dbg

BUG=577962
==========

to

==========
bluetooth: Use WebBluetoothDeviceId instead of string

This adds more type safety to our current code and also allows us check the
format
of the string that we get from the renderer.

Also moves the chromium variant bindings generation out of blink.gyp and into
content_common_mojo_bindings.gyp. This avoids adding a dependency to
content/common
in blink.gyp since now the chromium bindings depend on
content/common/bluetooth/web_bluetooth_device_id

CQ_INCLUDE_TRYJOBS=master.tryserver.chromium.mac:mac_chromium_gyp_rel,master.tryserver.chromium.mac:mac_chromium_gyp_dbg

BUG=577962
==========

[jam, 2016-07-14 20:11:49]

On 2016/07/13 22:53:04, ortuno wrote:
> removing blundell since we no longer add a component. The latest patch set
only
> moves some files from components to content/common so just pinging two people.
> 
> jbroman: PTAL at t/WebKit
> jam: PTAL at content/common and content/*.gyp*

Why are the new files in content/common instead of content/browser?

content/common is for code that's shared between the browser process and other
child processes.

[jam, 2016-07-14 20:29:29]

On 2016/07/14 20:11:49, jam wrote:
> On 2016/07/13 22:53:04, ortuno wrote:
> > removing blundell since we no longer add a component. The latest patch set
> only
> > moves some files from components to content/common so just pinging two
people.
> > 
> > jbroman: PTAL at t/WebKit
> > jam: PTAL at content/common and content/*.gyp*
> 
> Why are the new files in content/common instead of content/browser?
> 
> content/common is for code that's shared between the browser process and other
> child processes.

lgtm

Giovanni explained on IM that it's included through the generated bindings.

[ortuno, 2016-07-15 00:05:12]

ah, forgot to send drafts.

https://codereview.chromium.org/2019853002/diff/400001/third_party/WebKit/pub...
File third_party/WebKit/public/blink.gyp (left):

https://codereview.chromium.org/2019853002/diff/400001/third_party/WebKit/pub...
third_party/WebKit/public/blink.gyp:164:
'../../../device/bluetooth/bluetooth.gyp:bluetooth_mojom',
On 2016/07/14 at 15:14:24, jbroman wrote:
> Why are these no longer needed? device.mojom.BluetoothUUID still seems to be
in use.

Ah right. I accidentally moved this to content.

https://codereview.chromium.org/2019853002/diff/400001/third_party/WebKit/pub...
File third_party/WebKit/public/blink.gyp (right):

https://codereview.chromium.org/2019853002/diff/400001/third_party/WebKit/pub...
third_party/WebKit/public/blink.gyp:116:
'platform/modules/bluetooth/web_bluetooth.mojom',
On 2016/07/14 at 15:14:24, jbroman wrote:
> Why is this being taken out of the Chromium variant? I'd expect both targets
to include the same .mojom files.

blink.gyp generates both chromium and blink variant bindings e.g.
web_bluetooth.mojom.h and web_bluetooth.mojom-blink.h. I think this is wrong,
blink.gyp should only generate blink bindings and chromium should generate its
own bindings. This becomes apparent in this case because the chromium bindings
now depend on content/common. This would mean that we would need to add a
dependency on content/common in this file, which I believe we don't want. To
avoid that, this patch moves the Web Bluetooth chromium binding generation to
content/content_common_mojo_bindings.gyp. I'm happy to move the whole non-blink
variant to content if we think that's the right solution.

[jbroman, 2016-07-15 17:10:33]

https://codereview.chromium.org/2019853002/diff/400001/third_party/WebKit/pub...
File third_party/WebKit/public/blink.gyp (right):

https://codereview.chromium.org/2019853002/diff/400001/third_party/WebKit/pub...
third_party/WebKit/public/blink.gyp:116:
'platform/modules/bluetooth/web_bluetooth.mojom',
On 2016/07/15 at 00:05:12, ortuno wrote:
> On 2016/07/14 at 15:14:24, jbroman wrote:
> > Why is this being taken out of the Chromium variant? I'd expect both targets
to include the same .mojom files.
> 
> blink.gyp generates both chromium and blink variant bindings e.g.
web_bluetooth.mojom.h and web_bluetooth.mojom-blink.h. I think this is wrong,
blink.gyp should only generate blink bindings and chromium should generate its
own bindings. This becomes apparent in this case because the chromium bindings
now depend on content/common. This would mean that we would need to add a
dependency on content/common in this file, which I believe we don't want. To
avoid that, this patch moves the Web Bluetooth chromium binding generation to
content/content_common_mojo_bindings.gyp. I'm happy to move the whole non-blink
variant to content if we think that's the right solution.

Have you brought this up on chromium-mojo@? I'd rather not see us get into an
inconsistent state here. I can see arguments in both directions, but I really
think this needs to be predictable so that Chromium developers can easily locate
the correct target to use mojo bindings.

[ortuno, 2016-07-15 18:23:16]

https://codereview.chromium.org/2019853002/diff/400001/third_party/WebKit/pub...
File third_party/WebKit/public/blink.gyp (right):

https://codereview.chromium.org/2019853002/diff/400001/third_party/WebKit/pub...
third_party/WebKit/public/blink.gyp:116:
'platform/modules/bluetooth/web_bluetooth.mojom',
On 2016/07/15 at 17:10:33, jbroman wrote:
> On 2016/07/15 at 00:05:12, ortuno wrote:
> > On 2016/07/14 at 15:14:24, jbroman wrote:
> > > Why is this being taken out of the Chromium variant? I'd expect both
targets to include the same .mojom files.
> > 
> > blink.gyp generates both chromium and blink variant bindings e.g.
web_bluetooth.mojom.h and web_bluetooth.mojom-blink.h. I think this is wrong,
blink.gyp should only generate blink bindings and chromium should generate its
own bindings. This becomes apparent in this case because the chromium bindings
now depend on content/common. This would mean that we would need to add a
dependency on content/common in this file, which I believe we don't want. To
avoid that, this patch moves the Web Bluetooth chromium binding generation to
content/content_common_mojo_bindings.gyp. I'm happy to move the whole non-blink
variant to content if we think that's the right solution.
> 
> Have you brought this up on chromium-mojo@? I'd rather not see us get into an
inconsistent state here. I can see arguments in both directions, but I really
think this needs to be predictable so that Chromium developers can easily locate
the correct target to use mojo bindings.

I did not, mainly because whatever we decide to do is very temporary. Last time
I checked there were no gyp bots in the cq, and I've heard that we are not going
to have to worry about gyp in the next week or two.

[jbroman, 2016-07-15 18:23:54]

On 2016/07/15 at 18:23:16, ortuno wrote:
>
https://codereview.chromium.org/2019853002/diff/400001/third_party/WebKit/pub...
> File third_party/WebKit/public/blink.gyp (right):
> 
>
https://codereview.chromium.org/2019853002/diff/400001/third_party/WebKit/pub...
> third_party/WebKit/public/blink.gyp:116:
'platform/modules/bluetooth/web_bluetooth.mojom',
> On 2016/07/15 at 17:10:33, jbroman wrote:
> > On 2016/07/15 at 00:05:12, ortuno wrote:
> > > On 2016/07/14 at 15:14:24, jbroman wrote:
> > > > Why is this being taken out of the Chromium variant? I'd expect both
targets to include the same .mojom files.
> > > 
> > > blink.gyp generates both chromium and blink variant bindings e.g.
web_bluetooth.mojom.h and web_bluetooth.mojom-blink.h. I think this is wrong,
blink.gyp should only generate blink bindings and chromium should generate its
own bindings. This becomes apparent in this case because the chromium bindings
now depend on content/common. This would mean that we would need to add a
dependency on content/common in this file, which I believe we don't want. To
avoid that, this patch moves the Web Bluetooth chromium binding generation to
content/content_common_mojo_bindings.gyp. I'm happy to move the whole non-blink
variant to content if we think that's the right solution.
> > 
> > Have you brought this up on chromium-mojo@? I'd rather not see us get into
an inconsistent state here. I can see arguments in both directions, but I really
think this needs to be predictable so that Chromium developers can easily locate
the correct target to use mojo bindings.
> 
> I did not, mainly because whatever we decide to do is very temporary. Last
time I checked there were no gyp bots in the cq, and I've heard that we are not
going to have to worry about gyp in the next week or two.

ok, lgtm

[ortuno, 2016-07-15 18:26:28]

The CQ bit was checked by ortuno@chromium.org

[ortuno, 2016-07-15 18:26:29]

The patchset sent to the CQ was uploaded after l-g-t-m from
jyasskin@chromium.org, rockot@chromium.org, dcheng@chromium.org,
palmer@chromium.org, jam@chromium.org
Link to the patchset: https://codereview.chromium.org/2019853002/#ps440001
(title: "Explicitly include web_bluetooth_device_id.h")

[commit-bot: I haz the power, 2016-07-15 18:27:05]

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, 2016-07-15 19:00:45]

Committed patchset #23 (id:440001)

[commit-bot: I haz the power, 2016-07-15 19:01:13]

CQ bit was unchecked.

[commit-bot: I haz the power, 2016-07-15 19:03:08]

Description was changed from

==========
bluetooth: Use WebBluetoothDeviceId instead of string

This adds more type safety to our current code and also allows us check the
format
of the string that we get from the renderer.

Also moves the chromium variant bindings generation out of blink.gyp and into
content_common_mojo_bindings.gyp. This avoids adding a dependency to
content/common
in blink.gyp since now the chromium bindings depend on
content/common/bluetooth/web_bluetooth_device_id

CQ_INCLUDE_TRYJOBS=master.tryserver.chromium.mac:mac_chromium_gyp_rel,master.tryserver.chromium.mac:mac_chromium_gyp_dbg

BUG=577962
==========

to

==========
bluetooth: Use WebBluetoothDeviceId instead of string

This adds more type safety to our current code and also allows us check the
format
of the string that we get from the renderer.

Also moves the chromium variant bindings generation out of blink.gyp and into
content_common_mojo_bindings.gyp. This avoids adding a dependency to
content/common
in blink.gyp since now the chromium bindings depend on
content/common/bluetooth/web_bluetooth_device_id

CQ_INCLUDE_TRYJOBS=master.tryserver.chromium.mac:mac_chromium_gyp_rel,master.tryserver.chromium.mac:mac_chromium_gyp_dbg

BUG=577962

Committed: https://crrev.com/bd27a25399b274afabd4338a28c5c1f72b37c78c
Cr-Commit-Position: refs/heads/master@{#405806}
==========

[commit-bot: I haz the power, 2016-07-15 19:03:11]

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

[wjmaclean, 2016-07-15 19:44:44]

A revert of this CL (patchset #23 id:440001) has been created in
https://codereview.chromium.org/2157493003/ by wjmaclean@chromium.org.

The reason for reverting is: Appears to be a possible cause for failing tests:

https://build.chromium.org/p/chromium.linux/builders/Linux%20Tests/builds/43401
https://build.chromium.org/p/chromium.mac/builders/Mac10.9%20Tests/builds/24953

Doing speculative revert..

[findit-for-me, 2016-07-15 20:29:15]

FYI: Findit try jobs (rerunning failed compile or tests) identified this CL
at revision 405806 as the culprit for failures in the build cycles as shown on:
https://findit-for-me.appspot.com/waterfall/culprit?key=ag9zfmZpbmRpdC1mb3Itb...

[ortuno, 2016-07-31 01:38:27]

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

[commit-bot: I haz the power, 2016-07-31 01:38:30]

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, 2016-07-31 01:57:20]

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

[commit-bot: I haz the power, 2016-07-31 01:57:21]

Dry run: Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)

[ortuno, 2016-08-04 14:15:12]

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

[commit-bot: I haz the power, 2016-08-04 14:15:38]

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, 2016-08-04 14:18:57]

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

[commit-bot: I haz the power, 2016-08-04 14:18:59]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
  chromeos_x86-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_clobber_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[ortuno, 2016-08-04 14:33:16]

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

[commit-bot: I haz the power, 2016-08-04 14:34:42]

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, 2016-08-04 16:23:29]

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

[commit-bot: I haz the power, 2016-08-04 16:23:31]

Dry run: This issue passed the CQ dry run.

[ortuno, 2016-08-04 17:15:48]

The CQ bit was checked by ortuno@chromium.org

[ortuno, 2016-08-04 17:15:49]

The patchset sent to the CQ was uploaded after l-g-t-m from rockot@chromium.org,
jam@chromium.org, jbroman@chromium.org, dcheng@chromium.org,
palmer@chromium.org, jyasskin@chromium.org
Link to the patchset: https://codereview.chromium.org/2019853002/#ps500001
(title: "Rebase")

[commit-bot: I haz the power, 2016-08-04 17:16:20]

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, 2016-08-04 17:20:56]

Committed patchset #26 (id:500001)

[commit-bot: I haz the power, 2016-08-04 17:22:49]

Description was changed from

==========
bluetooth: Use WebBluetoothDeviceId instead of string

This adds more type safety to our current code and also allows us check the
format
of the string that we get from the renderer.

Also moves the chromium variant bindings generation out of blink.gyp and into
content_common_mojo_bindings.gyp. This avoids adding a dependency to
content/common
in blink.gyp since now the chromium bindings depend on
content/common/bluetooth/web_bluetooth_device_id

CQ_INCLUDE_TRYJOBS=master.tryserver.chromium.mac:mac_chromium_gyp_rel,master.tryserver.chromium.mac:mac_chromium_gyp_dbg

BUG=577962

Committed: https://crrev.com/bd27a25399b274afabd4338a28c5c1f72b37c78c
Cr-Commit-Position: refs/heads/master@{#405806}
==========

to

==========
bluetooth: Use WebBluetoothDeviceId instead of string

This adds more type safety to our current code and also allows us check the
format
of the string that we get from the renderer.

Also moves the chromium variant bindings generation out of blink.gyp and into
content_common_mojo_bindings.gyp. This avoids adding a dependency to
content/common
in blink.gyp since now the chromium bindings depend on
content/common/bluetooth/web_bluetooth_device_id

CQ_INCLUDE_TRYJOBS=master.tryserver.chromium.mac:mac_chromium_gyp_rel,master.tryserver.chromium.mac:mac_chromium_gyp_dbg

BUG=577962

Committed: https://crrev.com/bd27a25399b274afabd4338a28c5c1f72b37c78c
Committed: https://crrev.com/d0b0acd381a26f12f629092423f533d6179de0e9
Cr-Original-Commit-Position: refs/heads/master@{#405806}
Cr-Commit-Position: refs/heads/master@{#409820}
==========

[commit-bot: I haz the power, 2016-08-04 17:22:51]

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