Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Eliminate duplicated code when dealing with pointer data #36822

Merged
merged 11 commits into from
Nov 10, 2022
1 change: 1 addition & 0 deletions ci/licenses_golden/licenses_flutter
Original file line number Diff line number Diff line change
Expand Up @@ -1762,6 +1762,7 @@ FILE: ../../../flutter/lib/ui/window/pointer_data_packet.h
FILE: ../../../flutter/lib/ui/window/pointer_data_packet_converter.cc
FILE: ../../../flutter/lib/ui/window/pointer_data_packet_converter.h
FILE: ../../../flutter/lib/ui/window/pointer_data_packet_converter_unittests.cc
FILE: ../../../flutter/lib/ui/window/pointer_data_packet_unittests.cc
FILE: ../../../flutter/lib/ui/window/viewport_metrics.cc
FILE: ../../../flutter/lib/ui/window/viewport_metrics.h
FILE: ../../../flutter/lib/ui/window/window.cc
Expand Down
1 change: 1 addition & 0 deletions lib/ui/BUILD.gn
Original file line number Diff line number Diff line change
Expand Up @@ -239,6 +239,7 @@ if (enable_unittests) {
"window/platform_message_response_dart_port_unittests.cc",
"window/platform_message_response_dart_unittests.cc",
"window/pointer_data_packet_converter_unittests.cc",
"window/pointer_data_packet_unittests.cc",
]

deps = [
Expand Down
13 changes: 13 additions & 0 deletions lib/ui/window/pointer_data_packet.cc
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
// found in the LICENSE file.

#include "flutter/lib/ui/window/pointer_data_packet.h"
#include "flutter/fml/logging.h"

#include <cstring>

Expand All @@ -17,7 +18,19 @@ PointerDataPacket::PointerDataPacket(uint8_t* data, size_t num_bytes)
PointerDataPacket::~PointerDataPacket() = default;

void PointerDataPacket::SetPointerData(size_t i, const PointerData& data) {
FML_DCHECK(i < GetLength());
memcpy(&data_[i * sizeof(PointerData)], &data, sizeof(PointerData));
}

PointerData PointerDataPacket::GetPointerData(size_t i) const {
FML_DCHECK(i < GetLength());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

DCHECK only runs in debug mode. if the goal of this PR is to add bounds checking to make it safe, you need to always perform the bounds check

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added DCHECK because originally there is no check at all and I dare not change semantics of the code (e.g. if no check because previous dev deliberately do so to avoid being too slow?). If you like, I will change to CHECK

PointerData result;
memcpy(&result, &data_[i * sizeof(PointerData)], sizeof(PointerData));
fzyzcjy marked this conversation as resolved.
Show resolved Hide resolved
return result;
}

size_t PointerDataPacket::GetLength() const {
return data_.size() / sizeof(PointerData);
}

} // namespace flutter
2 changes: 2 additions & 0 deletions lib/ui/window/pointer_data_packet.h
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,8 @@ class PointerDataPacket {
~PointerDataPacket();

void SetPointerData(size_t i, const PointerData& data);
PointerData GetPointerData(size_t i) const;
size_t GetLength() const;
const std::vector<uint8_t>& data() const { return data_; }

private:
Expand Down
10 changes: 2 additions & 8 deletions lib/ui/window/pointer_data_packet_converter.cc
Original file line number Diff line number Diff line change
Expand Up @@ -17,17 +17,11 @@ PointerDataPacketConverter::~PointerDataPacketConverter() = default;

std::unique_ptr<PointerDataPacket> PointerDataPacketConverter::Convert(
std::unique_ptr<PointerDataPacket> packet) {
size_t kBytesPerPointerData = kPointerDataFieldCount * kBytesPerField;
auto buffer = packet->data();
size_t buffer_length = buffer.size();

std::vector<PointerData> converted_pointers;
// Converts each pointer data in the buffer and stores it in the
// converted_pointers.
for (size_t i = 0; i < buffer_length / kBytesPerPointerData; i++) {
PointerData pointer_data;
memcpy(&pointer_data, &buffer[i * kBytesPerPointerData],
sizeof(PointerData));
for (size_t i = 0; i < packet->GetLength(); i++) {
PointerData pointer_data = packet->GetPointerData(i);
ConvertPointerData(pointer_data, converted_pointers);
}

Expand Down
10 changes: 2 additions & 8 deletions lib/ui/window/pointer_data_packet_converter_unittests.cc
Original file line number Diff line number Diff line change
Expand Up @@ -133,14 +133,8 @@ void CreateSimulatedTrackpadGestureData(PointerData& data, // NOLINT

void UnpackPointerPacket(std::vector<PointerData>& output, // NOLINT
std::unique_ptr<PointerDataPacket> packet) {
size_t kBytesPerPointerData = kPointerDataFieldCount * kBytesPerField;
auto buffer = packet->data();
size_t buffer_length = buffer.size();

for (size_t i = 0; i < buffer_length / kBytesPerPointerData; i++) {
PointerData pointer_data;
memcpy(&pointer_data, &buffer[i * kBytesPerPointerData],
sizeof(PointerData));
for (size_t i = 0; i < packet->GetLength(); i++) {
PointerData pointer_data = packet->GetPointerData(i);
output.push_back(pointer_data);
}
packet.reset();
Expand Down
69 changes: 69 additions & 0 deletions lib/ui/window/pointer_data_packet_unittests.cc
Original file line number Diff line number Diff line change
@@ -0,0 +1,69 @@
// Copyright 2013 The Flutter 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 "flutter/lib/ui/window/pointer_data.h"

#include <cstring>

#include "gtest/gtest.h"
#include "pointer_data_packet.h"

namespace flutter {
namespace testing {

void CreateSimpleSimulatedPointerData(PointerData& data, // NOLINT
PointerData::Change change,
int64_t device,
double dx,
double dy,
int64_t buttons) {
data.time_stamp = 0;
data.change = change;
data.kind = PointerData::DeviceKind::kTouch;
data.signal_kind = PointerData::SignalKind::kNone;
data.device = device;
data.pointer_identifier = 0;
data.physical_x = dx;
data.physical_y = dy;
data.physical_delta_x = 0.0;
data.physical_delta_y = 0.0;
data.buttons = buttons;
data.obscured = 0;
data.synthesized = 0;
data.pressure = 0.0;
data.pressure_min = 0.0;
data.pressure_max = 0.0;
data.distance = 0.0;
data.distance_max = 0.0;
data.size = 0.0;
data.radius_major = 0.0;
data.radius_minor = 0.0;
data.radius_min = 0.0;
data.radius_max = 0.0;
data.orientation = 0.0;
data.tilt = 0.0;
data.platformData = 0;
data.scroll_delta_x = 0.0;
data.scroll_delta_y = 0.0;
}

TEST(PointerDataPacketTest, CanGetPointerData) {
auto packet = std::make_unique<PointerDataPacket>(1);
PointerData data;
CreateSimpleSimulatedPointerData(data, PointerData::Change::kAdd, 1, 2.0, 3.0,
4);
packet->SetPointerData(0, data);

PointerData data_recovered = packet->GetPointerData(0);
ASSERT_EQ(data_recovered.physical_x, 2.0);
ASSERT_EQ(data_recovered.physical_y, 3.0);
}

TEST(PointerDataPacketTest, CanGetLength) {
auto packet = std::make_unique<PointerDataPacket>(6);
ASSERT_EQ(packet->GetLength(), (size_t)6);
}

} // namespace testing
} // namespace flutter
11 changes: 2 additions & 9 deletions shell/platform/fuchsia/flutter/platform_view_unittest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -379,15 +379,8 @@ std::string ToString(const fml::Mapping& mapping) {
// Stolen from pointer_data_packet_converter_unittests.cc.
void UnpackPointerPacket(std::vector<flutter::PointerData>& output, // NOLINT
std::unique_ptr<flutter::PointerDataPacket> packet) {
size_t kBytesPerPointerData =
flutter::kPointerDataFieldCount * flutter::kBytesPerField;
auto buffer = packet->data();
size_t buffer_length = buffer.size();

for (size_t i = 0; i < buffer_length / kBytesPerPointerData; i++) {
flutter::PointerData pointer_data;
memcpy(&pointer_data, &buffer[i * kBytesPerPointerData],
sizeof(flutter::PointerData));
for (size_t i = 0; i < packet->GetLength(); i++) {
flutter::PointerData pointer_data = packet->GetPointerData(i);
output.push_back(pointer_data);
}
packet.reset();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -484,15 +484,8 @@ std::string ToString(const fml::Mapping& mapping) {
// Stolen from pointer_data_packet_converter_unittests.cc.
void UnpackPointerPacket(std::vector<flutter::PointerData>& output, // NOLINT
std::unique_ptr<flutter::PointerDataPacket> packet) {
size_t kBytesPerPointerData =
flutter::kPointerDataFieldCount * flutter::kBytesPerField;
auto buffer = packet->data();
size_t buffer_length = buffer.size();

for (size_t i = 0; i < buffer_length / kBytesPerPointerData; i++) {
flutter::PointerData pointer_data;
memcpy(&pointer_data, &buffer[i * kBytesPerPointerData],
sizeof(flutter::PointerData));
for (size_t i = 0; i < packet->GetLength(); i++) {
flutter::PointerData pointer_data = packet->GetPointerData(i);
output.push_back(pointer_data);
}
packet.reset();
Expand Down