bluetooth: Use WebBluetoothDeviceId instead of string

content/common/bluetooth/bluetooth_device_id.cc

Index: content/common/bluetooth/bluetooth_device_id.cc
diff --git a/content/common/bluetooth/bluetooth_device_id.cc b/content/common/bluetooth/bluetooth_device_id.cc
new file mode 100644
index 0000000000000000000000000000000000000000..84415ee1491637224bca0e29577b0c93064b2800
--- /dev/null
+++ b/content/common/bluetooth/bluetooth_device_id.cc
@@ -0,0 +1,83 @@
+// Copyright 2016 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "content/common/bluetooth/bluetooth_device_id.h"
+
+#include "base/base64.h"
+#include "base/strings/string_util.h"
+#include "crypto/random.h"
+
+namespace content {
+
+namespace {
+
+enum { kDeviceIdLength = 16 /* 128 bits */ };
+
+}  // namespace
+
+BluetoothDeviceId::BluetoothDeviceId() {}
+
+BluetoothDeviceId::BluetoothDeviceId(const std::string& device_id) {

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

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; }".

[ortuno, 2016/06/06 22:22:59]

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

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

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))".

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

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

+  DCHECK(IsValid(device_id));
+  device_id_ = device_id;
+}
+
+BluetoothDeviceId::~BluetoothDeviceId() {}
+
+const std::string& BluetoothDeviceId::str() const {
+  DCHECK(IsValid(device_id_));
+  return device_id_;
+}
+
+// static
+BluetoothDeviceId BluetoothDeviceId::Create() {
+  std::string bytes(
+      kDeviceIdLength + 1 /* to avoid bytes being reallocated by WriteInto */,
+      '\0');
+
+  crypto::RandBytes(base::WriteInto(&bytes /* str */,
+                                    kDeviceIdLength + 1 /* length_with_null */),
+                    kDeviceIdLength);
+
+  base::Base64Encode(bytes, &bytes);
+
+  return BluetoothDeviceId(bytes);
+}
+
+// static
+bool BluetoothDeviceId::IsValid(const std::string& device_id) {
+  std::string decoded;
+  if (!base::Base64Decode(device_id, &decoded)) {

[Jeffrey Yasskin, 2016/06/03 17:22:04]

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?

[ortuno, 2016/06/06 22:22:59]

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.

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

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?

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

Added an if statement for it.

+    return false;
+  }
+
+  if (decoded.size() != kDeviceIdLength) {
+    return false;
+  }
+
+  // Expected length of a 128bit string encoded to Base64.
+  DCHECK_EQ(24u, device_id.size());

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

I think we don't need these: the Base64Decode won't produce a 16-byte result if
they're not true.

[ortuno, 2016/06/06 22:22:59]

Done. This was just me being paranoid.

+  // Expected padding characters for a 128bit string encoded to Base64.
+  DCHECK(device_id[22] == '=' && (device_id[23] == '='));
+
+  return true;
+}
+
+bool BluetoothDeviceId::operator==(const BluetoothDeviceId& device_id) const {
+  DCHECK(IsValid(device_id_) && IsValid(device_id.device_id_));

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

Since str() does these DCHECKs again, I'd compare .device_id_ directly.

[ortuno, 2016/06/06 22:22:59]

Done.

+
+  return str() == device_id.str();
+}
+
+bool BluetoothDeviceId::operator!=(const BluetoothDeviceId& device_id) const {

[Jeffrey Yasskin, 2016/06/03 17:22:04]

Generally implement != as "return !(*this == device_id);"

[ortuno, 2016/06/06 22:22:59]

Ok! Done.

+  DCHECK(IsValid(device_id_) && IsValid(device_id.device_id_));
+
+  return str() != device_id.str();
+}
+
+void PrintTo(const BluetoothDeviceId& device_id, std::ostream* out) {
+  *out << device_id.str();
+}
+
+}  // namespace content