From e789b78f857a26feeab4b51e0f2ec0e2cb67a219 Mon Sep 17 00:00:00 2001
From: rudranshd <rudranshd@google.com>
Date: Fri, 6 Oct 2023 19:45:53 +0000
Subject: [PATCH] [Instant Hotspot] Replace kMobile with kCellular in QS view
controller
Replaces instances of kMobile with kCellular in QS view controller if
kInstantHotspot feature flag is enabled. Also reverts CL that removed
tether networks from mobile section: crrev.com/c/4706048.
Tested on cellular DUT with feature flags on/off and connected tether
device.
Screenshot(feature flag disabled):
https://screenshot.googleplex.com/4pYxjn6D2vWCAUL
Screenshot(feature flag enabled): https://screenshot.googleplex.com/3AXhvtutMFCybz9
Bug: b/301288153
Change-Id: I4549825cf9eb17a076a98abb3e6f07b993d913e3
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4879618
Auto-Submit: Rudransh Dikshit <rudranshd@google.com>
Reviewed-by: Chad Duffin <chadduffin@chromium.org>
Commit-Queue: Rudransh Dikshit <rudranshd@google.com>
Cr-Commit-Position: refs/heads/main@{#1206572}
---
ash/constants/ash_features.cc | 5 +++
ash/constants/ash_features.h | 1 +
.../network_detailed_network_view_impl.cc | 14 +++++--
.../network_list_view_controller_impl.cc | 40 ++++++++++++------
.../network_list_view_controller_impl.h | 3 +-
.../network_list_view_controller_unittest.cc | 42 ++++++++++++++-----
.../network_config/public/cpp/BUILD.gn | 1 -
.../public/cpp/cros_network_config_util.cc | 9 +---
8 files changed, 80 insertions(+), 35 deletions(-)
diff --git a/ash/constants/ash_features.cc b/ash/constants/ash_features.cc
index aa513dc1788a3..6c57276911c86 100644
--- a/ash/constants/ash_features.cc
+++ b/ash/constants/ash_features.cc
@@ -3489,6 +3489,11 @@ bool IsHotspotEnabled() {
return base::FeatureList::IsEnabled(kHotspot);
}
+bool IsInstantHotspotRebrandEnabled() {
+ return base::FeatureList::IsEnabled(kInstantHotspotRebrand) &&
+ base::FeatureList::IsEnabled(kQsRevamp);
+}
+
bool IsScreenSaverDurationEnabled() {
return base::FeatureList::IsEnabled(kScreenSaverDuration);
}
diff --git a/ash/constants/ash_features.h b/ash/constants/ash_features.h
index 1eee435fd473f..16354a07bde83 100644
--- a/ash/constants/ash_features.h
+++ b/ash/constants/ash_features.h
@@ -1004,6 +1004,7 @@ COMPONENT_EXPORT(ASH_CONSTANTS) bool IsHomeButtonQuickAppAccessEnabled();
COMPONENT_EXPORT(ASH_CONSTANTS) bool IsHomeButtonWithTextEnabled();
COMPONENT_EXPORT(ASH_CONSTANTS) bool IsHostnameSettingEnabled();
COMPONENT_EXPORT(ASH_CONSTANTS) bool IsHotspotEnabled();
+COMPONENT_EXPORT(ASH_CONSTANTS) bool IsInstantHotspotRebrandEnabled();
COMPONENT_EXPORT(ASH_CONSTANTS) bool IsInputNoiseCancellationUiEnabled();
COMPONENT_EXPORT(ASH_CONSTANTS)
bool IsInstantTetheringBackgroundAdvertisingSupported();
diff --git a/ash/system/network/network_detailed_network_view_impl.cc b/ash/system/network/network_detailed_network_view_impl.cc
index 76c5e51a49310..bb1412d805052 100644
--- a/ash/system/network/network_detailed_network_view_impl.cc
+++ b/ash/system/network/network_detailed_network_view_impl.cc
@@ -40,13 +40,16 @@ using chromeos::network_config::mojom::InhibitReason;
constexpr auto kMainContainerMargins = gfx::Insets::TLBR(2, 0, 0, 0);
constexpr auto kTopContainerBorder = gfx::Insets::TLBR(4, 0, 4, 4);
-// The following getter methods should only be used for `NetworkType::kWiFi` and
-// `NetworkType::kMobile` types otherwise a crash will occur.
+// The following getter methods should only be used for `NetworkType::kWiFi`,
+// `NetworkType::kMobile`, or `NetworkType::kCellular` types otherwise a crash
+// will occur.
std::u16string GetLabelForWifiAndMobileNetwork(NetworkType type) {
switch (type) {
case NetworkType::kWiFi:
return l10n_util::GetStringUTF16(
IDS_ASH_QUICK_SETTINGS_JOIN_WIFI_NETWORK);
+ case NetworkType::kCellular:
+ [[fallthrough]];
case NetworkType::kMobile:
return l10n_util::GetStringUTF16(IDS_ASH_QUICK_SETTINGS_ADD_ESIM);
default:
@@ -58,6 +61,8 @@ std::u16string GetTooltipForWifiAndMobileNetwork(NetworkType type) {
switch (type) {
case NetworkType::kWiFi:
return l10n_util::GetStringUTF16(IDS_ASH_STATUS_TRAY_OTHER_WIFI);
+ case NetworkType::kCellular:
+ [[fallthrough]];
case NetworkType::kMobile:
return l10n_util::GetStringUTF16(GetAddESimTooltipMessageId());
default:
@@ -69,6 +74,8 @@ int GetViewIDForWifiAndMobileNetwork(NetworkType type) {
switch (type) {
case NetworkType::kWiFi:
return VIEW_ID_JOIN_WIFI_NETWORK_ENTRY;
+ case NetworkType::kCellular:
+ [[fallthrough]];
case NetworkType::kMobile:
return VIEW_ID_ADD_ESIM_ENTRY;
default:
@@ -123,7 +130,8 @@ NetworkListNetworkItemView* NetworkDetailedNetworkViewImpl::AddNetworkListItem(
HoverHighlightView* NetworkDetailedNetworkViewImpl::AddConfigureNetworkEntry(
NetworkType type) {
- CHECK(type == NetworkType::kWiFi || type == NetworkType::kMobile);
+ CHECK(type == NetworkType::kWiFi || type == NetworkType::kMobile ||
+ type == NetworkType::kCellular);
HoverHighlightView* entry = GetNetworkList(type)->AddChildView(
std::make_unique<HoverHighlightView>(/*listener=*/this));
entry->SetID(GetViewIDForWifiAndMobileNetwork(type));
diff --git a/ash/system/network/network_list_view_controller_impl.cc b/ash/system/network/network_list_view_controller_impl.cc
index d5a8ad8d5818f..12a7d697a9341 100644
--- a/ash/system/network/network_list_view_controller_impl.cc
+++ b/ash/system/network/network_list_view_controller_impl.cc
@@ -24,6 +24,7 @@
#include "ash/system/network/tray_network_state_model.h"
#include "ash/system/tray/tray_info_label.h"
#include "ash/system/tray/tri_view.h"
+#include "base/feature_list.h"
#include "base/timer/timer.h"
#include "chromeos/ash/components/dbus/hermes/hermes_manager_client.h"
#include "chromeos/constants/chromeos_features.h"
@@ -130,6 +131,13 @@ bool IsCellularSimLocked() {
!cellular_device->sim_lock_status->lock_type.empty();
}
+NetworkType GetMobileSectionNetworkType() {
+ if (features::IsInstantHotspotRebrandEnabled()) {
+ return NetworkType::kCellular;
+ }
+ return NetworkType::kMobile;
+}
+
} // namespace
NetworkListViewControllerImpl::NetworkListViewControllerImpl(
@@ -259,20 +267,20 @@ void NetworkListViewControllerImpl::OnGetNetworkStateList(
size_t mobile_item_index = 0;
mobile_item_index = CreateItemViewsIfMissingAndReorder(
- NetworkType::kMobile, mobile_item_index, networks,
+ GetMobileSectionNetworkType(), mobile_item_index, networks,
&previous_network_views);
// Add mobile status message to NetworkDetailedNetworkView's
// `mobile_network_list_view_` if it exist.
if (mobile_status_message_) {
network_detailed_network_view()
- ->GetNetworkList(NetworkType::kMobile)
+ ->GetNetworkList(GetMobileSectionNetworkType())
->ReorderChildView(mobile_status_message_, mobile_item_index++);
}
if (ShouldAddESimEntry()) {
mobile_item_index = CreateConfigureNetworkEntry(
- &add_esim_entry_, NetworkType::kMobile, mobile_item_index);
+ &add_esim_entry_, GetMobileSectionNetworkType(), mobile_item_index);
} else {
RemoveAndResetViewIfExists(&add_esim_entry_);
}
@@ -280,17 +288,18 @@ void NetworkListViewControllerImpl::OnGetNetworkStateList(
network_detailed_network_view()->ReorderMobileListView(index++);
} else {
network_detailed_network_view()
- ->GetNetworkList(NetworkType::kMobile)
+ ->GetNetworkList(GetMobileSectionNetworkType())
->ReorderChildView(mobile_header_view_, index++);
- index = CreateItemViewsIfMissingAndReorder(
- NetworkType::kMobile, index, networks, &previous_network_views);
+ index = CreateItemViewsIfMissingAndReorder(GetMobileSectionNetworkType(),
+ index, networks,
+ &previous_network_views);
// Add mobile status message to NetworkDetailedNetworkView scroll list if
// it exist.
if (mobile_status_message_) {
network_detailed_network_view()
- ->GetNetworkList(NetworkType::kMobile)
+ ->GetNetworkList(GetMobileSectionNetworkType())
->ReorderChildView(mobile_status_message_, index++);
}
}
@@ -388,15 +397,18 @@ void NetworkListViewControllerImpl::OnGetNetworkStateList(
void NetworkListViewControllerImpl::UpdateNetworkTypeExistence(
const std::vector<NetworkStatePropertiesPtr>& networks) {
- has_mobile_networks_ = false;
+ has_cellular_networks_ = false;
has_wifi_networks_ = false;
+ has_tether_networks_ = false;
connected_vpn_guid_ = std::string();
for (auto& network : networks) {
- if (NetworkTypeMatchesType(network->type, NetworkType::kMobile)) {
- has_mobile_networks_ = true;
+ if (NetworkTypeMatchesType(network->type, NetworkType::kCellular)) {
+ has_cellular_networks_ = true;
} else if (NetworkTypeMatchesType(network->type, NetworkType::kWiFi)) {
has_wifi_networks_ = true;
+ } else if (NetworkTypeMatchesType(network->type, NetworkType::kTether)) {
+ has_tether_networks_ = true;
} else if (NetworkTypeMatchesType(network->type, NetworkType::kVPN) &&
StateIsConnected(network->connection_state)) {
connected_vpn_guid_ = network->guid;
@@ -820,7 +832,9 @@ void NetworkListViewControllerImpl::UpdateMobileToggleAndSetStatusMessage() {
network_detailed_network_view()->UpdateMobileStatus(cellular_enabled);
if (cellular_enabled) {
- if (has_mobile_networks_) {
+ if (has_cellular_networks_ ||
+ (has_tether_networks_ &&
+ !features::IsInstantHotspotRebrandEnabled())) {
RemoveAndResetViewIfExists(&mobile_status_message_);
return;
}
@@ -877,7 +891,7 @@ void NetworkListViewControllerImpl::UpdateMobileToggleAndSetStatusMessage() {
/*animate_toggle=*/true);
network_detailed_network_view()->UpdateMobileStatus(tether_enabled);
- if (tether_enabled && !has_mobile_networks_) {
+ if (tether_enabled && !has_tether_networks_ && !has_cellular_networks_) {
CreateInfoLabelIfMissingAndUpdate(
IDS_ASH_STATUS_TRAY_NO_MOBILE_DEVICES_FOUND, &mobile_status_message_);
return;
@@ -906,7 +920,7 @@ void NetworkListViewControllerImpl::CreateInfoLabelIfMissingAndUpdate(
info->SetID(static_cast<int>(
NetworkListViewControllerViewChildId::kMobileStatusMessage));
*info_label_ptr = network_detailed_network_view()
- ->GetNetworkList(NetworkType::kMobile)
+ ->GetNetworkList(GetMobileSectionNetworkType())
->AddChildView(std::move(info));
} else if (info_label_ptr == &wifi_status_message_) {
info->SetID(static_cast<int>(
diff --git a/ash/system/network/network_list_view_controller_impl.h b/ash/system/network/network_list_view_controller_impl.h
index f72dd2df93b83..77de67a8a08e5 100644
--- a/ash/system/network/network_list_view_controller_impl.h
+++ b/ash/system/network/network_list_view_controller_impl.h
@@ -265,8 +265,9 @@ class ASH_EXPORT NetworkListViewControllerImpl
// for: #addr-of
RAW_PTR_EXCLUSION HoverHighlightView* add_esim_entry_ = nullptr;
- bool has_mobile_networks_;
+ bool has_cellular_networks_;
bool has_wifi_networks_;
+ bool has_tether_networks_;
bool is_mobile_network_enabled_;
bool is_wifi_enabled_;
std::string connected_vpn_guid_;
diff --git a/ash/system/network/network_list_view_controller_unittest.cc b/ash/system/network/network_list_view_controller_unittest.cc
index 700b4bc821d5b..e1ad6153acf16 100644
--- a/ash/system/network/network_list_view_controller_unittest.cc
+++ b/ash/system/network/network_list_view_controller_unittest.cc
@@ -172,8 +172,9 @@ class FakeNetworkDetailedNetworkViewDelegate
} // namespace
-class NetworkListViewControllerTest : public AshTestBase,
- public testing::WithParamInterface<bool> {
+class NetworkListViewControllerTest
+ : public AshTestBase,
+ public testing::WithParamInterface<std::tuple<bool, bool>> {
public:
NetworkListViewControllerTest()
: AshTestBase(base::test::TaskEnvironment::TimeSource::MOCK_TIME) {}
@@ -183,11 +184,20 @@ class NetworkListViewControllerTest : public AshTestBase,
~NetworkListViewControllerTest() override = default;
void SetUp() override {
+ std::vector<base::test::FeatureRef> enabled_features;
+ std::vector<base::test::FeatureRef> disabled_features;
if (IsQsRevampEnabled()) {
- feature_list_.InitAndEnableFeature(features::kQsRevamp);
+ enabled_features.push_back(features::kQsRevamp);
} else {
- feature_list_.InitAndDisableFeature(features::kQsRevamp);
+ disabled_features.push_back(features::kQsRevamp);
}
+ if (IsInstantHotspotRebrandEnabled()) {
+ enabled_features.push_back(features::kInstantHotspotRebrand);
+ } else {
+ disabled_features.push_back(features::kInstantHotspotRebrand);
+ }
+ feature_list_.InitWithFeatures(enabled_features, disabled_features);
+
AshTestBase::SetUp();
cros_network_ = std::make_unique<FakeCrosNetworkConfig>();
@@ -215,7 +225,9 @@ class NetworkListViewControllerTest : public AshTestBase,
network_detailed_network_view_);
}
- bool IsQsRevampEnabled() { return GetParam(); }
+ bool IsQsRevampEnabled() { return std::get<0>(GetParam()); }
+
+ bool IsInstantHotspotRebrandEnabled() { return std::get<1>(GetParam()); }
void TearDown() override {
network_list_view_controller_impl_.reset();
@@ -554,9 +566,9 @@ class NetworkListViewControllerTest : public AshTestBase,
network_list_view_controller_impl_;
};
-INSTANTIATE_TEST_SUITE_P(QsRevamp,
+INSTANTIATE_TEST_SUITE_P(All,
NetworkListViewControllerTest,
- testing::Bool() /* IsQsRevampEnabled() */);
+ testing::Combine(testing::Bool(), testing::Bool()));
TEST_P(NetworkListViewControllerTest, MobileDataSectionIsShown) {
EXPECT_THAT(GetMobileSubHeader(), IsNull());
@@ -872,9 +884,19 @@ TEST_P(NetworkListViewControllerTest, HasCorrectMobileNetworkList) {
CrosNetworkConfigTestHelper::CreateStandaloneNetworkProperties(
kTetherName, NetworkType::kTether, ConnectionStateType::kConnected));
- CheckNetworkListOrdering(/*ethernet_network_count=*/0,
- /*mobile_network_count=*/1,
- /*wifi_network_count=*/0);
+ if (IsInstantHotspotRebrandEnabled() && IsQsRevampEnabled()) {
+ CheckNetworkListOrdering(/*ethernet_network_count=*/0,
+ /*mobile_network_count=*/0,
+ /*wifi_network_count=*/0);
+ } else {
+ CheckNetworkListOrdering(/*ethernet_network_count=*/0,
+ /*mobile_network_count=*/1,
+ /*wifi_network_count=*/0);
+ }
+
+ if (IsInstantHotspotRebrandEnabled()) {
+ return;
+ }
if (IsQsRevampEnabled()) {
CheckNetworkListItem(NetworkType::kTether, /*index=*/0u,
/*guid=*/kTetherName);
diff --git a/chromeos/services/network_config/public/cpp/BUILD.gn b/chromeos/services/network_config/public/cpp/BUILD.gn
index 63e23b48f7247..2b66185585cde 100644
--- a/chromeos/services/network_config/public/cpp/BUILD.gn
+++ b/chromeos/services/network_config/public/cpp/BUILD.gn
@@ -14,7 +14,6 @@ source_set("cpp") {
public_deps = [ "//chromeos/services/network_config/public/mojom" ]
deps = [
- "//ash/constants",
"//components/device_event_log:device_event_log",
"//components/onc",
]
diff --git a/chromeos/services/network_config/public/cpp/cros_network_config_util.cc b/chromeos/services/network_config/public/cpp/cros_network_config_util.cc
index 3c2f1f36db85d..d52def178485c 100644
--- a/chromeos/services/network_config/public/cpp/cros_network_config_util.cc
+++ b/chromeos/services/network_config/public/cpp/cros_network_config_util.cc
@@ -3,7 +3,6 @@
// found in the LICENSE file.
#include "chromeos/services/network_config/public/cpp/cros_network_config_util.h"
-#include "ash/constants/ash_features.h"
#include "components/device_event_log/device_event_log.h"
#include "components/onc/onc_constants.h"
@@ -265,12 +264,8 @@ bool NetworkTypeMatchesType(mojom::NetworkType network_type,
case mojom::NetworkType::kAll:
return true;
case mojom::NetworkType::kMobile:
- if (base::FeatureList::IsEnabled(ash::features::kInstantHotspotRebrand)) {
- return network_type == mojom::NetworkType::kCellular;
- } else {
- return network_type == mojom::NetworkType::kCellular ||
- network_type == mojom::NetworkType::kTether;
- }
+ return network_type == mojom::NetworkType::kCellular ||
+ network_type == mojom::NetworkType::kTether;
case mojom::NetworkType::kWireless:
return network_type == mojom::NetworkType::kCellular ||
network_type == mojom::NetworkType::kTether ||