shill: don't update FrequenciesConnectedEver until we complete L2
shill has been reporting Network.Shill.WiFi.FrequenciesConnectedEver when
supplicant reports a change in BSS. However, a change in BSS may only
indicate the start of a connection attempt, not its success.
It's probably more relevant to record the number of frequencies where
we successfully connect. So change to reporting the data after the
layer-2 is established.
We could go further and wait until DHCP is complete, or even further,
and wait until we get online. But let's keep this a layer-2 statistic
for now.
While there: make some EXPECTations in wifi_provider_unittest a little
more specific.
BUG=chromium:274595
TEST=unit tests
Change-Id: I67547db507498c130a2a26822b679306ad3ede4f
Reviewed-on: https://gerrit.chromium.org/gerrit/66113
Tested-by: mukesh agrawal <quiche@chromium.org>
Reviewed-by: Paul Stewart <pstew@chromium.org>
Commit-Queue: mukesh agrawal <quiche@chromium.org>
diff --git a/wifi.cc b/wifi.cc
index a668272..e30f144 100644
--- a/wifi.cc
+++ b/wifi.cc
@@ -852,7 +852,6 @@
<< " " << LogSSID(endpoint->ssid_string());
service->NotifyCurrentEndpoint(endpoint);
- provider_->IncrementConnectCount(endpoint->frequency());
if (pending_service_.get() &&
service.get() != pending_service_.get()) {
@@ -1304,21 +1303,24 @@
EnableHighBitrates();
} else if (has_already_completed_) {
LOG(INFO) << link_name() << " L3 configuration already started.";
- } else if (AcquireIPConfigWithLeaseName(
- GetServiceLeaseName(*affected_service))) {
- LOG(INFO) << link_name() << " is up; started L3 configuration.";
- affected_service->SetState(Service::kStateConfiguring);
- if (affected_service->IsSecurityMatch(flimflam::kSecurityWep)) {
- // With the overwhelming majority of WEP networks, we cannot assume
- // our credentials are correct just because we have successfully
- // connected. It is more useful to track received data as the L3
- // configuration proceeds to see if we can decrypt anything.
- receive_byte_count_at_connect_ = GetReceiveByteCount();
- } else {
- affected_service->ResetSuspectedCredentialFailures();
- }
} else {
- LOG(ERROR) << "Unable to acquire DHCP config.";
+ provider_->IncrementConnectCount(affected_service->frequency());
+ if (AcquireIPConfigWithLeaseName(
+ GetServiceLeaseName(*affected_service))) {
+ LOG(INFO) << link_name() << " is up; started L3 configuration.";
+ affected_service->SetState(Service::kStateConfiguring);
+ if (affected_service->IsSecurityMatch(flimflam::kSecurityWep)) {
+ // With the overwhelming majority of WEP networks, we cannot assume
+ // our credentials are correct just because we have successfully
+ // connected. It is more useful to track received data as the L3
+ // configuration proceeds to see if we can decrypt anything.
+ receive_byte_count_at_connect_ = GetReceiveByteCount();
+ } else {
+ affected_service->ResetSuspectedCredentialFailures();
+ }
+ } else {
+ LOG(ERROR) << "Unable to acquire DHCP config.";
+ }
}
has_already_completed_ = true;
} else if (new_state == WPASupplicant::kInterfaceStateAssociated) {
diff --git a/wifi_provider_unittest.cc b/wifi_provider_unittest.cc
index 9eef99f..f3c2b67 100644
--- a/wifi_provider_unittest.cc
+++ b/wifi_provider_unittest.cc
@@ -1370,7 +1370,8 @@
EXPECT_CALL(time_, GetSecondsSinceEpoch()).WillOnce(Return(kThisWeekSeconds));
EXPECT_CALL(manager_, UpdateWiFiProvider());
- EXPECT_CALL(metrics_, SendToUMA(_, _, _, _, _));
+ EXPECT_CALL(metrics_, SendToUMA(Metrics::kMetricFrequenciesConnectedEver,
+ _, _, _, _));
time_t newest_week_at_start =
provider_.connect_count_by_frequency_dated_.crbegin()->first;
provider_.IncrementConnectCount(6002);
@@ -1401,7 +1402,8 @@
EXPECT_CALL(time_, GetSecondsSinceEpoch()).
WillOnce(Return(this_week * kSecondsPerWeek));
EXPECT_CALL(manager_, UpdateWiFiProvider());
- EXPECT_CALL(metrics_, SendToUMA(_, _, _, _, _));
+ EXPECT_CALL(metrics_, SendToUMA(Metrics::kMetricFrequenciesConnectedEver,
+ _, _, _, _));
time_t newest_week_at_start =
provider_.connect_count_by_frequency_dated_.crbegin()->first;
provider_.IncrementConnectCount(6001);
diff --git a/wifi_service.h b/wifi_service.h
index 5f50e3f..4a1e871 100644
--- a/wifi_service.h
+++ b/wifi_service.h
@@ -91,6 +91,7 @@
const std::string &bssid() const { return bssid_; }
const std::vector<uint16> &frequency_list() const { return frequency_list_; }
uint16 physical_mode() const { return physical_mode_; }
+ uint16 frequency() const { return frequency_; }
// WiFi services can load from profile entries other than their current
// storage identifier. Override the methods from the parent Service
diff --git a/wifi_unittest.cc b/wifi_unittest.cc
index eedb543..98c4724 100644
--- a/wifi_unittest.cc
+++ b/wifi_unittest.cc
@@ -580,6 +580,7 @@
EXPECT_CALL(*service, ResetSuspectedCredentialFailures());
EXPECT_CALL(*dhcp_provider(), CreateConfig(_, _, _, _)).Times(AnyNumber());
EXPECT_CALL(*dhcp_config_.get(), RequestIP()).Times(AnyNumber());
+ EXPECT_CALL(wifi_provider_, IncrementConnectCount(_));
ReportStateChanged(WPASupplicant::kInterfaceStateCompleted);
Mock::VerifyAndClearExpectations(service);
@@ -1971,6 +1972,7 @@
ReportCurrentBSSChanged(bss_path1);
EXPECT_CALL(*service1, SetState(Service::kStateConfiguring));
EXPECT_CALL(*service1, ResetSuspectedCredentialFailures());
+ EXPECT_CALL(*wifi_provider(), IncrementConnectCount(_));
ReportStateChanged(WPASupplicant::kInterfaceStateCompleted);
EXPECT_EQ(service1.get(), GetCurrentService().get());
Mock::VerifyAndClearExpectations(service0);