Skip to content

Commit

Permalink
[CrOS Cellular] Add success callback to CNC's CreateCustomApn()
Browse files Browse the repository at this point in the history 
Add success callback for CreateCustomApn() method in CrosNetworkConfig.

Needed so that ApnMigrator completes migrating default APN before
attempting to migrate attach APN if a migration involves both.

  browser_test *ApnDetailDiaog*

Bug: b/303565348
Test: unit_test *ApnMigrator* chromeos_unittest *CrosNetworkConfig*
Change-Id: Ib865c08ae34eaa764390eb905f130bd9c7d48d73
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4961198
Commit-Queue: Regan Hsu <hsuregan@chromium.org>
Reviewed-by: Sergey Poromov <poromov@chromium.org>
Reviewed-by: Gordon Seto <gordonseto@google.com>
Reviewed-by: Roland Bock <rbock@google.com>
Reviewed-by: danakj <danakj@chromium.org>
Reviewed-by: Jason Zhang <jiajunz@google.com>
Cr-Commit-Position: refs/heads/main@{#1215791}
  • Loading branch information
Regan Hsu authored and Chromium LUCI CQ committed Oct 26, 2023
1 parent 5f7d44a commit e01d091
Show file tree
Hide file tree
Showing 12 changed files with 371 additions and 126 deletions.
92 changes: 66 additions & 26 deletions chrome/browser/ash/net/apn_migrator.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -191,10 +191,7 @@ void ApnMigrator::OnSetShillCustomApnListSuccess(const std::string iccid) {

// The network has just been migrated.
if (!managed_cellular_pref_handler_->ContainsApnMigratedIccid(iccid)) {
NET_LOG(EVENT) << "ApnMigrator: Mark network with ICCID: " << iccid
<< " as migrated";
managed_cellular_pref_handler_->AddApnMigratedIccid(iccid);
iccids_in_migration_.erase(iccid);
CompleteMigrationAttempt(iccid, /*success=*/true);
}
}

Expand All @@ -206,7 +203,7 @@ void ApnMigrator::OnSetShillCustomApnListFailure(
<< "list in Shill for network: " << guid << ": [" << error_name
<< ']';

iccids_in_migration_.erase(iccid);
CompleteMigrationAttempt(iccid, /*success=*/false);
}

void ApnMigrator::MigrateNetwork(const NetworkState& network) {
Expand Down Expand Up @@ -261,21 +258,21 @@ void ApnMigrator::OnGetManagedProperties(
if (error.has_value()) {
NET_LOG(ERROR) << "Error fetching managed properties for " << iccid
<< ", error: " << error.value();
iccids_in_migration_.erase(iccid);
CompleteMigrationAttempt(iccid, /*success=*/false);
return;
}

if (!properties) {
NET_LOG(ERROR) << "Error fetching managed properties for " << iccid;
iccids_in_migration_.erase(iccid);
CompleteMigrationAttempt(iccid, /*success=*/false);
return;
}

const NetworkState* network =
network_state_handler_->GetNetworkStateFromGuid(guid);
if (!network) {
NET_LOG(ERROR) << "Network no longer exists: " << guid;
iccids_in_migration_.erase(iccid);
CompleteMigrationAttempt(iccid, /*success=*/false);
return;
}

Expand Down Expand Up @@ -336,8 +333,7 @@ void ApnMigrator::OnGetManagedProperties(
CellularNetworkMetricsLogger::LogManagedCustomApnMigrationType(
CellularNetworkMetricsLogger::ManagedApnMigrationType::
kMatchesSelectedApn);
remote_cros_network_config_->CreateCustomApn(
guid, std::move(pre_revamp_custom_apn));
CreateCustomApn(iccid, guid, std::move(pre_revamp_custom_apn));
} else {
NET_LOG(EVENT)
<< "Managed network's selected APN doesn't match the saved custom "
Expand Down Expand Up @@ -383,8 +379,8 @@ void ApnMigrator::OnGetManagedProperties(
}
pre_revamp_custom_apn->apn_types =
GetMigratedApnTypes(pre_revamp_custom_apn);
remote_cros_network_config_->CreateCustomApn(
guid, std::move(pre_revamp_custom_apn));
CreateCustomApn(iccid, guid, std::move(pre_revamp_custom_apn));

} else if (last_connected_attach_apn && last_connected_default_apn &&
pre_revamp_custom_apn->access_point_name ==
(*last_connected_attach_apn)->access_point_name &&
Expand All @@ -400,8 +396,7 @@ void ApnMigrator::OnGetManagedProperties(
CellularNetworkMetricsLogger::LogUnmanagedCustomApnMigrationType(
CellularNetworkMetricsLogger::UnmanagedApnMigrationType::
kMatchesLastConnectedAttachAndDefault);
remote_cros_network_config_->CreateCustomApn(
guid, std::move(pre_revamp_custom_apn));
CreateCustomApn(iccid, guid, std::move(pre_revamp_custom_apn));
} else if (last_connected_attach_apn && last_connected_default_apn &&
pre_revamp_custom_apn->access_point_name ==
(*last_connected_attach_apn)->access_point_name &&
Expand All @@ -427,11 +422,42 @@ void ApnMigrator::OnGetManagedProperties(
CellularNetworkMetricsLogger::LogUnmanagedCustomApnMigrationType(
CellularNetworkMetricsLogger::UnmanagedApnMigrationType::
kMatchesLastConnectedAttachHasMatchingDatabaseApn);
remote_cros_network_config_->CreateCustomApn(
guid, last_connected_default_apn->Clone());

// Default custom APN in the enabled state must be created before attach
// APN is created in the enabled state. See b/303565348.
auto create_custom_attach_apn_callback = base::BindOnce(
&ApnMigrator::CreateCustomApn, weak_factory_.GetWeakPtr(), iccid,
guid, last_connected_attach_apn->Clone(), absl::nullopt);

auto on_create_default_apn_callback = base::BindOnce(
[](base::OnceClosure success_callback, const std::string& guid,
bool success) {
if (success) {
NET_LOG(EVENT)
<< "ApnMigrator: Succeeded migrating custom default APN "
"for network guid in the enabled state: "
<< guid
<< ", now migrating different custom attach APN in enabled "
"state.";
std::move(success_callback).Run();
return;
}
NET_LOG(ERROR)
<< "ApnMigrator: Failed to create custom default APN for "
"network of guid: "
<< guid
<< ", so will not proceed to create associated custom attach "
"APN";
},
std::move(create_custom_attach_apn_callback), guid);

CreateCustomApn(iccid, guid, last_connected_default_apn->Clone(),
std::move(on_create_default_apn_callback));
} else {
// Fallback to the catch-all case where the attach APN with a disabled
// state is migrated so that Shill will know to use the revamped logic.
// TODO(b/303565348): Fix CrosNetworkConfig to allow creating disabled
// attach apns.
NET_LOG(EVENT)
<< "Network's last connected default APN does not match an "
<< "APN in the network list, migrating last connected "
Expand All @@ -442,10 +468,8 @@ void ApnMigrator::OnGetManagedProperties(
CellularNetworkMetricsLogger::LogUnmanagedCustomApnMigrationType(
CellularNetworkMetricsLogger::UnmanagedApnMigrationType::
kMatchesLastConnectedAttachHasNoMatchingDatabaseApn);
CreateCustomApn(iccid, guid, last_connected_attach_apn->Clone());
}

remote_cros_network_config_->CreateCustomApn(
guid, last_connected_attach_apn->Clone());
} else if (!last_connected_attach_apn && last_connected_default_apn &&
pre_revamp_custom_apn->access_point_name ==
(*last_connected_default_apn)->access_point_name) {
Expand All @@ -460,8 +484,7 @@ void ApnMigrator::OnGetManagedProperties(
CellularNetworkMetricsLogger::LogUnmanagedCustomApnMigrationType(
CellularNetworkMetricsLogger::UnmanagedApnMigrationType::
kMatchesLastConnectedDefaultNoLastConnectedAttach);
remote_cros_network_config_->CreateCustomApn(
guid, std::move(pre_revamp_custom_apn));
CreateCustomApn(iccid, guid, std::move(pre_revamp_custom_apn));
} else {
NET_LOG(EVENT) << "Network's last connected default APN and attach APN "
<< "do not match the saved custom APN, migrating APN: "
Expand All @@ -473,15 +496,32 @@ void ApnMigrator::OnGetManagedProperties(
CellularNetworkMetricsLogger::LogUnmanagedCustomApnMigrationType(
CellularNetworkMetricsLogger::UnmanagedApnMigrationType::
kNoMatchingConnectedApn);
remote_cros_network_config_->CreateCustomApn(
guid, std::move(pre_revamp_custom_apn));
CreateCustomApn(iccid, guid, std::move(pre_revamp_custom_apn));
}
}
}

NET_LOG(EVENT) << "ApnMigrator: Mark network with ICCID: " << iccid
<< " as migrated";
managed_cellular_pref_handler_->AddApnMigratedIccid(iccid);
void ApnMigrator::CreateCustomApn(
const std::string& iccid,
const std::string& network_guid,
chromeos::network_config::mojom::ApnPropertiesPtr apn,
absl::optional<base::OnceCallback<void(bool)>> success_callback) {
remote_cros_network_config_->CreateCustomApn(
network_guid, std::move(apn),
success_callback ? std::move(*success_callback)
: base::BindOnce(&ApnMigrator::CompleteMigrationAttempt,
weak_factory_.GetWeakPtr(), iccid));
}

void ApnMigrator::CompleteMigrationAttempt(const std::string& iccid,
bool success) {
iccids_in_migration_.erase(iccid);

if (success) {
NET_LOG(EVENT) << "ApnMigrator: Mark network with ICCID: " << iccid
<< " as migrated";
managed_cellular_pref_handler_->AddApnMigratedIccid(iccid);
}
}

NetworkMetadataStore* ApnMigrator::GetNetworkMetadataStore() {
Expand Down
8 changes: 8 additions & 0 deletions chrome/browser/ash/net/apn_migrator.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,14 @@ class COMPONENT_EXPORT(CHROMEOS_NETWORK) ApnMigrator
absl::optional<base::Value::Dict> properties,
absl::optional<std::string> error);

void CreateCustomApn(const std::string& iccid,
const std::string& network_guid,
chromeos::network_config::mojom::ApnPropertiesPtr apn,
absl::optional<base::OnceCallback<void(bool)>>
success_callback = absl::nullopt);

void CompleteMigrationAttempt(const std::string& iccid, bool success);

NetworkMetadataStore* GetNetworkMetadataStore();

void set_network_metadata_store_for_testing(
Expand Down

0 comments on commit e01d091

Please sign in to comment.