Skip to content

Commit

Permalink
[Fast Pair] Switch gatt service client to new flag
Browse files Browse the repository at this point in the history
The gatt service client previously used the flag for the short term fast
pair handshake refactor which is going to be deprecated, this cl creates
a new flag which will be used for the long term fast pair handshake
refactor.

Test: Updated unittests and manually tested on DUT
Change-Id: I54986c08b58d3e5e64b56112c4c0343187117e0e
Bug: b/265853116
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4706333
Reviewed-by: Daniel Classon <dclasson@google.com>
Reviewed-by: Jack Shira <jackshira@google.com>
Commit-Queue: Alex Kingsborough <akingsb@google.com>
Cr-Commit-Position: refs/heads/main@{#1188082}
  • Loading branch information
Alex Kingsborough authored and Chromium LUCI CQ committed Aug 24, 2023
1 parent 0f8e628 commit b430688
Show file tree
Hide file tree
Showing 9 changed files with 59 additions and 12 deletions.
9 changes: 9 additions & 0 deletions ash/constants/ash_features.cc
Original file line number Diff line number Diff line change
Expand Up @@ -994,6 +994,11 @@ BASE_FEATURE(kFastPairHandshakeRefactor,
"FastPairHandshakeRefactor",
base::FEATURE_ENABLED_BY_DEFAULT);

// Enables using longterm Handshake retry logic for Fast Pair.
BASE_FEATURE(kFastPairHandshakeLongTermRefactor,
"FastPairHandshakeLongTermRefactor",
base::FEATURE_DISABLED_BY_DEFAULT);

// Enables prototype support for Fast Pair HID.
BASE_FEATURE(kFastPairHID, "FastPairHID", base::FEATURE_DISABLED_BY_DEFAULT);

Expand Down Expand Up @@ -3182,6 +3187,10 @@ bool IsFastPairHandshakeRefactorEnabled() {
return base::FeatureList::IsEnabled(kFastPairHandshakeRefactor);
}

bool IsFastPairHandshakeLongTermRefactorEnabled() {
return base::FeatureList::IsEnabled(kFastPairHandshakeLongTermRefactor);
}

bool IsFastPairHIDEnabled() {
return base::FeatureList::IsEnabled(kFastPairHID);
}
Expand Down
4 changes: 4 additions & 0 deletions ash/constants/ash_features.h
Original file line number Diff line number Diff line change
Expand Up @@ -289,6 +289,8 @@ COMPONENT_EXPORT(ASH_CONSTANTS) BASE_DECLARE_FEATURE(kFastPairDebugMetadata);
COMPONENT_EXPORT(ASH_CONSTANTS)
BASE_DECLARE_FEATURE(kFastPairHandshakeRefactor);
COMPONENT_EXPORT(ASH_CONSTANTS)
BASE_DECLARE_FEATURE(kFastPairHandshakeLongTermRefactor);
COMPONENT_EXPORT(ASH_CONSTANTS)
BASE_DECLARE_FEATURE(kFastPairHID);
COMPONENT_EXPORT(ASH_CONSTANTS)
BASE_DECLARE_FEATURE(kFastPairSavedDevicesNicknames);
Expand Down Expand Up @@ -895,6 +897,8 @@ COMPONENT_EXPORT(ASH_CONSTANTS) bool IsFastPairEnabled();
COMPONENT_EXPORT(ASH_CONSTANTS) bool IsFastPairBleRotationEnabled();
COMPONENT_EXPORT(ASH_CONSTANTS) bool IsFastPairDebugMetadataEnabled();
COMPONENT_EXPORT(ASH_CONSTANTS) bool IsFastPairHandshakeRefactorEnabled();
COMPONENT_EXPORT(ASH_CONSTANTS)
bool IsFastPairHandshakeLongTermRefactorEnabled();
COMPONENT_EXPORT(ASH_CONSTANTS) bool IsFastPairHIDEnabled();
COMPONENT_EXPORT(ASH_CONSTANTS) bool IsFastPairSavedDevicesNicknamesEnabled();
COMPONENT_EXPORT(ASH_CONSTANTS) bool IsFastPairLowPowerEnabled();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -250,7 +250,7 @@ FastPairGattServiceClientImpl::Factory::Create(
}
auto gatt_service = base::WrapUnique(new FastPairGattServiceClientImpl(
device, adapter, std::move(on_initialized_callback)));
if (!ash::features::IsFastPairHandshakeRefactorEnabled()) {
if (ash::features::IsFastPairHandshakeLongTermRefactorEnabled()) {
RecordGattInitializationStep(
FastPairGattConnectionSteps::kConnectionStarted);
gatt_service->AttemptGattConnection();
Expand All @@ -276,7 +276,7 @@ FastPairGattServiceClientImpl::FastPairGattServiceClientImpl(
adapter_(std::move(adapter)) {
adapter_observation_.Observe(adapter_.get());

if (ash::features::IsFastPairHandshakeRefactorEnabled()) {
if (!ash::features::IsFastPairHandshakeLongTermRefactorEnabled()) {
QP_LOG(INFO) << __func__ << ": Starting the GATT connection to device";
RecordGattInitializationStep(
FastPairGattConnectionSteps::kConnectionStarted);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -747,7 +747,8 @@ TEST_F(FastPairGattServiceClientTest, FailedGattConnection) {
TEST_F(FastPairGattServiceClientTest,
GattConnectionSuccess_HandshakeRefactorDisabled) {
base::test::ScopedFeatureList feature_list;
feature_list.InitAndDisableFeature(ash::features::kFastPairHandshakeRefactor);
feature_list.InitAndDisableFeature(
ash::features::kFastPairHandshakeLongTermRefactor);
histogram_tester().ExpectTotalCount(kTotalGattConnectionTime, 0);
histogram_tester().ExpectTotalCount(kGattConnectionResult, 0);
histogram_tester().ExpectTotalCount(kGattConnectionEffectiveSuccessRate, 0);
Expand Down Expand Up @@ -779,7 +780,8 @@ TEST_F(FastPairGattServiceClientTest,
TEST_F(FastPairGattServiceClientTest,
GattConnectionSuccess_HandshakeRefactorEnabled) {
base::test::ScopedFeatureList feature_list;
feature_list.InitAndEnableFeature(ash::features::kFastPairHandshakeRefactor);
feature_list.InitAndEnableFeature(
ash::features::kFastPairHandshakeLongTermRefactor);
histogram_tester().ExpectTotalCount(kTotalGattConnectionTime, 0);
histogram_tester().ExpectTotalCount(kGattConnectionResult, 0);
histogram_tester().ExpectTotalCount(kGattConnectionEffectiveSuccessRate, 0);
Expand Down Expand Up @@ -844,7 +846,8 @@ TEST_F(FastPairGattServiceClientTest, FailedPasskeyCharacteristics) {
TEST_F(FastPairGattServiceClientTest,
SuccessfulCharacteristicsStartNotify_HandshakeRefactorDisabled) {
base::test::ScopedFeatureList feature_list;
feature_list.InitAndDisableFeature(ash::features::kFastPairHandshakeRefactor);
feature_list.InitAndDisableFeature(
ash::features::kFastPairHandshakeLongTermRefactor);
histogram_tester().ExpectTotalCount(kNotifyKeyBasedCharacteristicTime, 0);
histogram_tester().ExpectTotalCount(kFastPairGattConnectionStep, 0);
SetKeybasedCharacteristicError(false);
Expand All @@ -863,7 +866,8 @@ TEST_F(FastPairGattServiceClientTest,
TEST_F(FastPairGattServiceClientTest,
SuccessfulCharacteristicsStartNotify_HandshakeRefactorEnabled) {
base::test::ScopedFeatureList feature_list;
feature_list.InitAndEnableFeature(ash::features::kFastPairHandshakeRefactor);
feature_list.InitAndEnableFeature(
ash::features::kFastPairHandshakeLongTermRefactor);
histogram_tester().ExpectTotalCount(kNotifyKeyBasedCharacteristicTime, 0);
histogram_tester().ExpectTotalCount(kFastPairGattConnectionStep, 0);
SetKeybasedCharacteristicError(false);
Expand Down Expand Up @@ -941,7 +945,8 @@ TEST_F(FastPairGattServiceClientTest, KeyBasedStartNotifyTimeout) {
TEST_F(FastPairGattServiceClientTest,
WriteKeyBasedRequest_HandshakeRefactorDisabled) {
base::test::ScopedFeatureList feature_list;
feature_list.InitAndDisableFeature(ash::features::kFastPairHandshakeRefactor);
feature_list.InitAndDisableFeature(
ash::features::kFastPairHandshakeLongTermRefactor);
histogram_tester().ExpectTotalCount(kWriteKeyBasedCharacteristicGattError, 0);
histogram_tester().ExpectTotalCount(kNotifyKeyBasedCharacteristicTime, 0);
histogram_tester().ExpectTotalCount(kFastPairGattConnectionStep, 0);
Expand All @@ -964,7 +969,8 @@ TEST_F(FastPairGattServiceClientTest,
TEST_F(FastPairGattServiceClientTest,
WriteKeyBasedRequest_HandshakeRefactorEnabled) {
base::test::ScopedFeatureList feature_list;
feature_list.InitAndEnableFeature(ash::features::kFastPairHandshakeRefactor);
feature_list.InitAndEnableFeature(
ash::features::kFastPairHandshakeLongTermRefactor);
histogram_tester().ExpectTotalCount(kWriteKeyBasedCharacteristicGattError, 0);
histogram_tester().ExpectTotalCount(kNotifyKeyBasedCharacteristicTime, 0);
histogram_tester().ExpectTotalCount(kFastPairGattConnectionStep, 0);
Expand Down Expand Up @@ -1017,7 +1023,8 @@ TEST_F(FastPairGattServiceClientTest, WriteKeyBasedRequestTimeout) {
TEST_F(FastPairGattServiceClientTest,
WritePasskeyRequest_HandshakeRefactorDisabled) {
base::test::ScopedFeatureList feature_list;
feature_list.InitAndDisableFeature(ash::features::kFastPairHandshakeRefactor);
feature_list.InitAndDisableFeature(
ash::features::kFastPairHandshakeLongTermRefactor);
histogram_tester().ExpectTotalCount(kWritePasskeyCharacteristicGattError, 0);
histogram_tester().ExpectTotalCount(kNotifyPasskeyCharacteristicTime, 0);
SuccessfulGattConnectionSetUp();
Expand All @@ -1038,7 +1045,8 @@ TEST_F(FastPairGattServiceClientTest,
TEST_F(FastPairGattServiceClientTest,
WritePasskeyRequest_HandshakeRefactorEnabled) {
base::test::ScopedFeatureList feature_list;
feature_list.InitAndEnableFeature(ash::features::kFastPairHandshakeRefactor);
feature_list.InitAndEnableFeature(
ash::features::kFastPairHandshakeLongTermRefactor);
histogram_tester().ExpectTotalCount(kWritePasskeyCharacteristicGattError, 0);
histogram_tester().ExpectTotalCount(kNotifyPasskeyCharacteristicTime, 0);
SuccessfulGattConnectionSetUp();
Expand Down Expand Up @@ -1090,7 +1098,8 @@ TEST_F(FastPairGattServiceClientTest, WritePasskeyRequestTimeout) {
TEST_F(FastPairGattServiceClientTest,
WriteAccountKey_HandshakeRefactorDisabled) {
base::test::ScopedFeatureList feature_list;
feature_list.InitAndDisableFeature(ash::features::kFastPairHandshakeRefactor);
feature_list.InitAndDisableFeature(
ash::features::kFastPairHandshakeLongTermRefactor);
histogram_tester().ExpectTotalCount(kWriteAccountKeyCharacteristicGattError,
0);
histogram_tester().ExpectTotalCount(kWriteAccountKeyTimeMetric, 0);
Expand All @@ -1113,7 +1122,8 @@ TEST_F(FastPairGattServiceClientTest,
TEST_F(FastPairGattServiceClientTest,
WriteAccountKey_HandshakeRefactorEnabled) {
base::test::ScopedFeatureList feature_list;
feature_list.InitAndEnableFeature(ash::features::kFastPairHandshakeRefactor);
feature_list.InitAndEnableFeature(
ash::features::kFastPairHandshakeLongTermRefactor);
histogram_tester().ExpectTotalCount(kWriteAccountKeyCharacteristicGattError,
0);
histogram_tester().ExpectTotalCount(kWriteAccountKeyTimeMetric, 0);
Expand Down
5 changes: 5 additions & 0 deletions chrome/browser/about_flags.cc
Original file line number Diff line number Diff line change
Expand Up @@ -7577,6 +7577,11 @@ const FeatureEntry kFeatureEntries[] = {
flag_descriptions::kFastPairHandshakeRefactorDescription, kOsCrOS,
FEATURE_VALUE_TYPE(ash::features::kFastPairHandshakeRefactor)},

{"fast-pair-handshake-long-term-refactor",
flag_descriptions::kFastPairHandshakeLongTermRefactorName,
flag_descriptions::kFastPairHandshakeLongTermRefactorDescription, kOsCrOS,
FEATURE_VALUE_TYPE(ash::features::kFastPairHandshakeLongTermRefactor)},

{"fast-pair-hid", flag_descriptions::kFastPairHIDName,
flag_descriptions::kFastPairHIDDescription, kOsCrOS,
FEATURE_VALUE_TYPE(ash::features::kFastPairHID)},
Expand Down
8 changes: 8 additions & 0 deletions chrome/browser/flag-metadata.json
Original file line number Diff line number Diff line change
Expand Up @@ -4167,6 +4167,14 @@
],
"expiry_milestone": 125
},
{
"name": "fast-pair-handshake-long-term-refactor",
"owners": [
"//ash/quick_pair/OWNERS",
"jackshira"
],
"expiry_milestone": 125
},
{
"name": "fast-pair-handshake-refactor",
"owners": [
Expand Down
6 changes: 6 additions & 0 deletions chrome/browser/flag_descriptions.cc
Original file line number Diff line number Diff line change
Expand Up @@ -5809,6 +5809,12 @@ const char kFastPairHandshakeRefactorName[] =
const char kFastPairHandshakeRefactorDescription[] =
"Enables refactored handshake logic for Google Fast Pair service.";

const char kFastPairHandshakeLongTermRefactorName[] =
"Enable Fast Pair Handshake Long Term Refactor";
const char kFastPairHandshakeLongTermRefactorDescription[] =
"Enables long term refactored handshake logic for Google Fast Pair "
"service.";

const char kFastPairHIDName[] = "Enable Fast Pair HID";
const char kFastPairHIDDescription[] =
"Enables prototype support for Fast Pair HID.";
Expand Down
3 changes: 3 additions & 0 deletions chrome/browser/flag_descriptions.h
Original file line number Diff line number Diff line change
Expand Up @@ -3348,6 +3348,9 @@ extern const char kFastPairDescription[];
extern const char kFastPairHandshakeRefactorName[];
extern const char kFastPairHandshakeRefactorDescription[];

extern const char kFastPairHandshakeLongTermRefactorName[];
extern const char kFastPairHandshakeLongTermRefactorDescription[];

extern const char kFastPairHIDName[];
extern const char kFastPairHIDDescription[];

Expand Down
2 changes: 2 additions & 0 deletions tools/metrics/histograms/enums.xml
Original file line number Diff line number Diff line change
Expand Up @@ -62412,6 +62412,7 @@ from previous Chrome versions.
<int value="-1039889738" label="NativeNotifications:enabled"/>
<int value="-1039555838" label="GamepadExtensions:enabled"/>
<int value="-1039219684" label="ShoppingPageTypes:disabled"/>
<int value="-1037955654" label="FastPairHandshakeLongTermRefactor:disabled"/>
<int value="-1037950113"
label="ResamplingScrollEventsExperimentalPrediction:enabled"/>
<int value="-1037620891"
Expand Down Expand Up @@ -63951,6 +63952,7 @@ from previous Chrome versions.
label="AutofillSaveCreditCardUsesStrikeSystemV2:enabled"/>
<int value="-268357961" label="enable-feature-policy"/>
<int value="-265697837" label="PhoneHub:disabled"/>
<int value="-265277473" label="FastPairHandshakeLongTermRefactor:enabled"/>
<int value="-263645996" label="InteractiveWindowCycleList:enabled"/>
<int value="-263150202" label="BundledConnectionHelp:disabled"/>
<int value="-262768194" label="TouchTextEditingRedesign:enabled"/>
Expand Down

0 comments on commit b430688

Please sign in to comment.