0

bluetooth: Remove implicit value copying during Mojo IPC

Update the Web Bluetooth Mojo interface to use the ReadOnlyBuffer type
so that the generated C++ bindings use base::span, which doesn't require
copying the GATT characteristic or descriptor value out of the Mojo
message just so that it can be put into an std::vector.

Right now in the write case the value still needs to be copied into a
std::vector in order to call into //device/bluetooth. Updating this
interface to use base::span can be done in a follow-up patch to remove
the now explicit std::vector constructor invocations.

Change-Id: I1f21e97ebf28ea38ddb49bed9e7500ee04809968
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5954717
Auto-Submit: Reilly Grant <reillyg@chromium.org>
Reviewed-by: Jack Hsieh <chengweih@chromium.org>
Reviewed-by: Matthew Denton <mpdenton@chromium.org>
Reviewed-by: Dave Tapuska <dtapuska@chromium.org>
Commit-Queue: Reilly Grant <reillyg@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1372994}
This commit is contained in:
Reilly Grant
2024-10-23 22:40:31 +00:00
committed by Chromium LUCI CQ
parent 11e05a6e26
commit 8002d42fd5
16 changed files with 113 additions and 121 deletions

@@ -7,6 +7,7 @@
#include <string>
#include "base/containers/span.h"
#include "base/functional/callback_forward.h"
#include "content/public/browser/bluetooth_delegate.h"
#include "device/bluetooth/bluetooth_device.h"
@@ -72,7 +73,7 @@ class WebBluetoothPairingManagerDelegate {
// error.
virtual void RemoteCharacteristicWriteValue(
const std::string& characteristic_instance_id,
const std::vector<uint8_t>& value,
base::span<const uint8_t> value,
blink::mojom::WebBluetoothWriteType write_type,
blink::mojom::WebBluetoothService::RemoteCharacteristicWriteValueCallback
callback) = 0;
@@ -94,7 +95,7 @@ class WebBluetoothPairingManagerDelegate {
// error.
virtual void RemoteDescriptorWriteValue(
const std::string& descriptor_instance_id,
const std::vector<uint8_t>& value,
base::span<const uint8_t> value,
blink::mojom::WebBluetoothService::RemoteDescriptorWriteValueCallback
callback) = 0;

@@ -28,7 +28,7 @@ void OnPairForReadCharacteristicCallback(
if (error_code) {
std::move(callback).Run(
WebBluetoothServiceImpl::TranslateConnectErrorAndRecord(*error_code),
/*value=*/std::nullopt);
/*value=*/{});
return;
}
pairing_manager_delegate->RemoteCharacteristicReadValue(
@@ -59,7 +59,7 @@ void OnPairForReadDescriptorCallback(
if (error_code) {
std::move(callback).Run(
WebBluetoothServiceImpl::TranslateConnectErrorAndRecord(*error_code),
/*value=*/std::nullopt);
/*value=*/{});
return;
}
pairing_manager_delegate->RemoteDescriptorReadValue(descriptor_instance_id,
@@ -129,7 +129,7 @@ void WebBluetoothPairingManagerImpl::PairForCharacteristicReadValue(
std::move(read_callback)
.Run(WebBluetoothServiceImpl::TranslateConnectErrorAndRecord(
BluetoothDevice::ConnectErrorCode::ERROR_UNKNOWN),
/*value=*/std::nullopt);
/*value=*/{});
return;
}
@@ -171,7 +171,7 @@ void WebBluetoothPairingManagerImpl::PairForDescriptorReadValue(
std::move(read_callback)
.Run(WebBluetoothServiceImpl::TranslateConnectErrorAndRecord(
BluetoothDevice::ConnectErrorCode::ERROR_UNKNOWN),
/*value=*/std::nullopt);
/*value=*/{});
return;
}

@@ -204,7 +204,7 @@ class BluetoothPairingManagerTest : public testing::Test,
}
std::move(callback).Run(WebBluetoothResult::CONNECT_AUTH_REJECTED,
std::nullopt);
/*value=*/{});
}
void RemoteDescriptorReadValue(
@@ -222,12 +222,12 @@ class BluetoothPairingManagerTest : public testing::Test,
}
std::move(callback).Run(WebBluetoothResult::CONNECT_AUTH_REJECTED,
std::nullopt);
/*value=*/{});
}
void RemoteCharacteristicWriteValue(
const std::string& characteristic_instance_id,
const std::vector<uint8_t>& value,
base::span<const uint8_t> value,
WebBluetoothWriteType write_type,
WebBluetoothService::RemoteCharacteristicWriteValueCallback callback)
override {
@@ -237,7 +237,7 @@ class BluetoothPairingManagerTest : public testing::Test,
return;
}
if (device_paired_) {
characteristic_value_ = value;
characteristic_value_ = std::vector<uint8_t>(value.begin(), value.end());
std::move(callback).Run(WebBluetoothResult::SUCCESS);
return;
}
@@ -247,7 +247,7 @@ class BluetoothPairingManagerTest : public testing::Test,
void RemoteDescriptorWriteValue(
const std::string& descriptor_instance_id,
const std::vector<uint8_t>& value,
base::span<const uint8_t> value,
WebBluetoothService::RemoteDescriptorWriteValueCallback callback)
override {
if (descriptor_instance_id != kValidTestData.descriptor_instance_id) {
@@ -255,7 +255,7 @@ class BluetoothPairingManagerTest : public testing::Test,
return;
}
if (device_paired_) {
descriptor_value_ = value;
descriptor_value_ = std::vector<uint8_t>(value.begin(), value.end());
std::move(callback).Run(WebBluetoothResult::SUCCESS);
return;
}
@@ -336,9 +336,8 @@ TEST_F(BluetoothPairingManagerTest, ReadSuccessfulAuthFirstSuccess) {
pairing_manager()->PairForCharacteristicReadValue(
kValidTestData.characteristic_instance_id,
base::BindLambdaForTesting(
[&loop, &expected_value](
WebBluetoothResult result,
const std::optional<std::vector<uint8_t>>& value) {
[&loop, &expected_value](WebBluetoothResult result,
base::span<const uint8_t> value) {
EXPECT_EQ(WebBluetoothResult::SUCCESS, result);
EXPECT_EQ(value, expected_value)
<< "Incorrect characteristic value";
@@ -357,9 +356,8 @@ TEST_F(BluetoothPairingManagerTest, ReadSuccessfulAuthSecondSuccess) {
pairing_manager()->PairForCharacteristicReadValue(
kValidTestData.characteristic_instance_id,
base::BindLambdaForTesting(
[&loop, &expected_value](
WebBluetoothResult result,
const std::optional<std::vector<uint8_t>>& value) {
[&loop, &expected_value](WebBluetoothResult result,
base::span<const uint8_t> value) {
EXPECT_EQ(WebBluetoothResult::SUCCESS, result);
EXPECT_EQ(value, expected_value)
<< "Incorrect characteristic value";
@@ -377,10 +375,9 @@ TEST_F(BluetoothPairingManagerTest, ReadFailAllAuthsFail) {
pairing_manager()->PairForCharacteristicReadValue(
kValidTestData.characteristic_instance_id,
base::BindLambdaForTesting(
[&loop](WebBluetoothResult result,
const std::optional<std::vector<uint8_t>>& value) {
[&loop](WebBluetoothResult result, base::span<const uint8_t> value) {
EXPECT_EQ(WebBluetoothResult::CONNECT_AUTH_REJECTED, result);
EXPECT_FALSE(value.has_value());
EXPECT_TRUE(value.empty());
loop.Quit();
}));
@@ -396,10 +393,9 @@ TEST_F(BluetoothPairingManagerTest, ReadInvalidCharacteristicId) {
pairing_manager()->PairForCharacteristicReadValue(
kValidNonTestData.characteristic_instance_id,
base::BindLambdaForTesting(
[&loop](WebBluetoothResult result,
const std::optional<std::vector<uint8_t>>& value) {
[&loop](WebBluetoothResult result, base::span<const uint8_t> value) {
EXPECT_EQ(WebBluetoothResult::CONNECT_UNKNOWN_ERROR, result);
EXPECT_FALSE(value.has_value());
EXPECT_TRUE(value.empty());
loop.Quit();
}));
@@ -414,10 +410,9 @@ TEST_F(BluetoothPairingManagerTest, ReadCharacteristicDeleteDelegate) {
pairing_manager()->PairForCharacteristicReadValue(
kValidTestData.characteristic_instance_id,
base::BindLambdaForTesting(
[&loop](WebBluetoothResult result,
const std::optional<std::vector<uint8_t>>& value) {
[&loop](WebBluetoothResult result, base::span<const uint8_t> value) {
EXPECT_EQ(WebBluetoothResult::CONNECT_AUTH_CANCELED, result);
EXPECT_FALSE(value.has_value());
EXPECT_TRUE(value.empty());
loop.Quit();
}));
@@ -438,8 +433,7 @@ TEST_F(BluetoothPairingManagerTest, ReadCharacteristicDoublePair) {
kValidTestData.characteristic_instance_id,
base::BindLambdaForTesting(
[&loop, &callback_count, &expected_value](
WebBluetoothResult result,
const std::optional<std::vector<uint8_t>>& value) {
WebBluetoothResult result, base::span<const uint8_t> value) {
EXPECT_EQ(WebBluetoothResult::SUCCESS, result);
EXPECT_EQ(value, expected_value);
if (++callback_count == 2)
@@ -451,9 +445,8 @@ TEST_F(BluetoothPairingManagerTest, ReadCharacteristicDoublePair) {
pairing_manager()->PairForCharacteristicReadValue(
kValidTestData.characteristic_instance_id,
base::BindLambdaForTesting(
[&loop, &callback_count](
WebBluetoothResult result,
const std::optional<std::vector<uint8_t>>& value) {
[&loop, &callback_count](WebBluetoothResult result,
base::span<const uint8_t> value) {
EXPECT_EQ(WebBluetoothResult::CONNECT_AUTH_CANCELED, result);
if (++callback_count == 2)
loop.Quit();
@@ -616,9 +609,8 @@ TEST_F(BluetoothPairingManagerTest, DescriptorReadSuccessfulAuthFirstSuccess) {
pairing_manager()->PairForDescriptorReadValue(
kValidTestData.descriptor_instance_id,
base::BindLambdaForTesting(
[&loop, &expected_value](
WebBluetoothResult result,
const std::optional<std::vector<uint8_t>>& value) {
[&loop, &expected_value](WebBluetoothResult result,
base::span<const uint8_t> value) {
EXPECT_EQ(WebBluetoothResult::SUCCESS, result);
EXPECT_EQ(value, expected_value) << "Incorrect descriptor value";
loop.Quit();
@@ -636,9 +628,8 @@ TEST_F(BluetoothPairingManagerTest, DescriptorReadSuccessfulAuthSecondSuccess) {
pairing_manager()->PairForDescriptorReadValue(
kValidTestData.descriptor_instance_id,
base::BindLambdaForTesting(
[&loop, &expected_value](
WebBluetoothResult result,
const std::optional<std::vector<uint8_t>>& value) {
[&loop, &expected_value](WebBluetoothResult result,
base::span<const uint8_t> value) {
EXPECT_EQ(WebBluetoothResult::SUCCESS, result);
EXPECT_EQ(value, expected_value) << "Incorrect descriptor value";
loop.Quit();
@@ -655,10 +646,9 @@ TEST_F(BluetoothPairingManagerTest, DescriptorReadFailAllAuthsFail) {
pairing_manager()->PairForDescriptorReadValue(
kValidTestData.descriptor_instance_id,
base::BindLambdaForTesting(
[&loop](WebBluetoothResult result,
const std::optional<std::vector<uint8_t>>& value) {
[&loop](WebBluetoothResult result, base::span<const uint8_t> value) {
EXPECT_EQ(WebBluetoothResult::CONNECT_AUTH_REJECTED, result);
EXPECT_FALSE(value.has_value());
EXPECT_TRUE(value.empty());
loop.Quit();
}));
@@ -674,8 +664,7 @@ TEST_F(BluetoothPairingManagerTest, DescriptorReadInvalidDescriptorId) {
pairing_manager()->PairForDescriptorReadValue(
kValidNonTestData.descriptor_instance_id,
base::BindLambdaForTesting(
[&loop](WebBluetoothResult result,
const std::optional<std::vector<uint8_t>>& value) {
[&loop](WebBluetoothResult result, base::span<const uint8_t> value) {
EXPECT_EQ(WebBluetoothResult::CONNECT_UNKNOWN_ERROR, result);
loop.Quit();
}));
@@ -691,8 +680,7 @@ TEST_F(BluetoothPairingManagerTest, ReadDescriptorDeleteDelegate) {
pairing_manager()->PairForDescriptorReadValue(
kValidTestData.descriptor_instance_id,
base::BindLambdaForTesting(
[&loop](WebBluetoothResult result,
const std::optional<std::vector<uint8_t>>& value) {
[&loop](WebBluetoothResult result, base::span<const uint8_t> value) {
EXPECT_EQ(WebBluetoothResult::CONNECT_AUTH_CANCELED, result);
loop.Quit();
}));
@@ -714,8 +702,7 @@ TEST_F(BluetoothPairingManagerTest, ReadDescriptorDoublePair) {
kValidTestData.descriptor_instance_id,
base::BindLambdaForTesting(
[&loop, &callback_count, &expected_value](
WebBluetoothResult result,
const std::optional<std::vector<uint8_t>>& value) {
WebBluetoothResult result, base::span<const uint8_t> value) {
EXPECT_EQ(WebBluetoothResult::SUCCESS, result);
EXPECT_EQ(value, expected_value) << "Incorrect descriptor value";
if (++callback_count == 2)
@@ -727,11 +714,10 @@ TEST_F(BluetoothPairingManagerTest, ReadDescriptorDoublePair) {
pairing_manager()->PairForDescriptorReadValue(
kValidTestData.descriptor_instance_id,
base::BindLambdaForTesting(
[&loop, &callback_count](
WebBluetoothResult result,
const std::optional<std::vector<uint8_t>>& value) {
[&loop, &callback_count](WebBluetoothResult result,
base::span<const uint8_t> value) {
EXPECT_EQ(WebBluetoothResult::CONNECT_AUTH_CANCELED, result);
EXPECT_FALSE(value.has_value());
EXPECT_TRUE(value.empty());
if (++callback_count == 2)
loop.Quit();
}));

@@ -1111,7 +1111,7 @@ void WebBluetoothServiceImpl::RemoteCharacteristicReadValue(
if (query_result.outcome != CacheQueryOutcome::kSuccess) {
RecordCharacteristicReadValueOutcome(query_result.outcome);
std::move(callback).Run(query_result.GetWebResult(),
std::nullopt /* value */);
/*value=*/{});
return;
}
@@ -1119,7 +1119,7 @@ void WebBluetoothServiceImpl::RemoteCharacteristicReadValue(
query_result.characteristic->GetUUID())) {
RecordCharacteristicReadValueOutcome(UMAGATTOperationOutcome::kBlocklisted);
std::move(callback).Run(blink::mojom::WebBluetoothResult::BLOCKLISTED_READ,
std::nullopt /* value */);
/*value=*/{});
return;
}
@@ -1131,7 +1131,7 @@ void WebBluetoothServiceImpl::RemoteCharacteristicReadValue(
void WebBluetoothServiceImpl::RemoteCharacteristicWriteValue(
const std::string& characteristic_instance_id,
const std::vector<uint8_t>& value,
base::span<const uint8_t> value,
blink::mojom::WebBluetoothWriteType write_type,
RemoteCharacteristicWriteValueCallback callback) {
DCHECK_CURRENTLY_ON(BrowserThread::UI);
@@ -1175,22 +1175,26 @@ void WebBluetoothServiceImpl::RemoteCharacteristicWriteValue(
BluetoothGattCharacteristic::ErrorCallback write_error_callback =
base::BindOnce(&WebBluetoothServiceImpl::OnCharacteristicWriteValueFailed,
weak_ptr_factory_.GetWeakPtr(), characteristic_instance_id,
value, write_type, std::move(split_callback.second));
std::vector<uint8_t>(value.begin(), value.end()),
write_type, std::move(split_callback.second));
using WebBluetoothWriteType = blink::mojom::WebBluetoothWriteType;
using WriteType = BluetoothRemoteGattCharacteristic::WriteType;
switch (write_type) {
case WebBluetoothWriteType::kWriteDefaultDeprecated:
query_result.characteristic->DeprecatedWriteRemoteCharacteristic(
value, std::move(write_callback), std::move(write_error_callback));
std::vector<uint8_t>(value.begin(), value.end()),
std::move(write_callback), std::move(write_error_callback));
break;
case WebBluetoothWriteType::kWriteWithResponse:
query_result.characteristic->WriteRemoteCharacteristic(
value, WriteType::kWithResponse, std::move(write_callback),
std::vector<uint8_t>(value.begin(), value.end()),
WriteType::kWithResponse, std::move(write_callback),
std::move(write_error_callback));
break;
case WebBluetoothWriteType::kWriteWithoutResponse:
query_result.characteristic->WriteRemoteCharacteristic(
value, WriteType::kWithoutResponse, std::move(write_callback),
std::vector<uint8_t>(value.begin(), value.end()),
WriteType::kWithoutResponse, std::move(write_callback),
std::move(write_error_callback));
break;
}
@@ -1330,14 +1334,14 @@ void WebBluetoothServiceImpl::RemoteDescriptorReadValue(
if (query_result.outcome != CacheQueryOutcome::kSuccess) {
std::move(callback).Run(query_result.GetWebResult(),
std::nullopt /* value */);
/*value=*/{});
return;
}
if (BluetoothBlocklist::Get().IsExcludedFromReads(
query_result.descriptor->GetUUID())) {
std::move(callback).Run(blink::mojom::WebBluetoothResult::BLOCKLISTED_READ,
std::nullopt /* value */);
/*value=*/{});
return;
}
@@ -1349,7 +1353,7 @@ void WebBluetoothServiceImpl::RemoteDescriptorReadValue(
void WebBluetoothServiceImpl::RemoteDescriptorWriteValue(
const std::string& descriptor_instance_id,
const std::vector<uint8_t>& value,
base::span<const uint8_t> value,
RemoteDescriptorWriteValueCallback callback) {
DCHECK_CURRENTLY_ON(BrowserThread::UI);
@@ -1384,13 +1388,14 @@ void WebBluetoothServiceImpl::RemoteDescriptorWriteValue(
// the callee interface.
auto split_callback = base::SplitOnceCallback(std::move(callback));
query_result.descriptor->WriteRemoteDescriptor(
value,
std::vector<uint8_t>(value.begin(), value.end()),
base::BindOnce(&WebBluetoothServiceImpl::OnDescriptorWriteValueSuccess,
weak_ptr_factory_.GetWeakPtr(),
std::move(split_callback.first)),
base::BindOnce(&WebBluetoothServiceImpl::OnDescriptorWriteValueFailed,
weak_ptr_factory_.GetWeakPtr(), descriptor_instance_id,
value, std::move(split_callback.second)));
std::vector<uint8_t>(value.begin(), value.end()),
std::move(split_callback.second)));
}
void WebBluetoothServiceImpl::RequestScanningStart(
@@ -1935,7 +1940,7 @@ void WebBluetoothServiceImpl::OnCharacteristicReadValue(
std::move(callback).Run(
TranslateGATTErrorAndRecord(error_code.value(),
UMAGATTOperation::kCharacteristicRead),
/*value=*/std::nullopt);
/*value=*/{});
return;
}
RecordCharacteristicReadValueOutcome(UMAGATTOperationOutcome::kSuccess);
@@ -2082,7 +2087,7 @@ void WebBluetoothServiceImpl::OnDescriptorReadValue(
std::move(callback).Run(
TranslateGATTErrorAndRecord(error_code.value(),
UMAGATTOperation::kDescriptorReadObsolete),
/*value=*/std::nullopt);
/*value=*/{});
return;
}
std::move(callback).Run(blink::mojom::WebBluetoothResult::SUCCESS, value);

@@ -260,7 +260,7 @@ class CONTENT_EXPORT WebBluetoothServiceImpl
RemoteCharacteristicReadValueCallback callback) override;
void RemoteCharacteristicWriteValue(
const std::string& characteristic_instance_id,
const std::vector<uint8_t>& value,
base::span<const uint8_t> value,
blink::mojom::WebBluetoothWriteType write_type,
RemoteCharacteristicWriteValueCallback callback) override;
void RemoteCharacteristicStartNotifications(
@@ -286,7 +286,7 @@ class CONTENT_EXPORT WebBluetoothServiceImpl
RemoteDescriptorReadValueCallback callback) override;
void RemoteDescriptorWriteValue(
const std::string& descriptor_instance_id,
const std::vector<uint8_t>& value,
base::span<const uint8_t> value,
RemoteDescriptorWriteValueCallback callback) override;
void RequestScanningStart(
mojo::PendingAssociatedRemote<

@@ -186,7 +186,7 @@ class FakeWebBluetoothCharacteristicClient : WebBluetoothCharacteristicClient {
protected:
// WebBluetoothCharacteristicClient implementation:
void RemoteCharacteristicValueChanged(
const std::vector<uint8_t>& value) override {
base::span<const uint8_t> value) override {
NOTREACHED_IN_MIGRATION();
}
@@ -933,12 +933,12 @@ TEST_F(WebBluetoothServiceImplTest,
characteristic_instance_id,
base::BindLambdaForTesting(
[&callback_called](blink::mojom::WebBluetoothResult result,
const std::optional<std::vector<uint8_t>>& value) {
base::span<const uint8_t> value) {
callback_called = true;
EXPECT_EQ(
blink::mojom::WebBluetoothResult::GATT_OPERATION_IN_PROGRESS,
result);
EXPECT_FALSE(value.has_value());
EXPECT_TRUE(value.empty());
}),
device::BluetoothGattService::GattErrorCode::kInProgress,
read_error_value);
@@ -968,13 +968,12 @@ TEST_F(WebBluetoothServiceImplTest, ReadCharacteristicValueNotAuthorized) {
service_ptr_->OnCharacteristicReadValue(
test_characteristic.GetIdentifier(),
base::BindLambdaForTesting(
[&read_value_callback_called](
blink::mojom::WebBluetoothResult result,
const std::optional<std::vector<uint8_t>>& value) {
[&read_value_callback_called](blink::mojom::WebBluetoothResult result,
base::span<const uint8_t> value) {
read_value_callback_called = true;
EXPECT_EQ(blink::mojom::WebBluetoothResult::GATT_NOT_AUTHORIZED,
result);
EXPECT_FALSE(value.has_value());
EXPECT_TRUE(value.empty());
}),
device::BluetoothGattService::GattErrorCode::kNotAuthorized,
read_error_value);