shill: Converts many metrics calls to take references, not pointers.
I noticed some code that takes pointers that could crash if given a NULL
pointer. While I fixed that, I decided that I'd modify other methods
that take pointers but where NULL is not a valid parameter. This seems
consistent with Google style.
BUG=None.
TEST=unittest
Change-Id: I368fd66e1aa9d3ff81ec0fc2902af4721d6c1dde
Reviewed-on: https://chromium-review.googlesource.com/171510
Reviewed-by: mukesh agrawal <quiche@chromium.org>
Reviewed-by: Wade Guthrie <wdg@chromium.org>
Tested-by: Wade Guthrie <wdg@chromium.org>
Commit-Queue: Wade Guthrie <wdg@chromium.org>
diff --git a/metrics.cc b/metrics.cc
index 87ff7bc..efec578 100644
--- a/metrics.cc
+++ b/metrics.cc
@@ -475,28 +475,28 @@
SLOG(Metrics, 2) << __func__;
}
-void Metrics::RegisterService(const Service *service) {
+void Metrics::RegisterService(const Service &service) {
SLOG(Metrics, 2) << __func__;
- LOG_IF(WARNING, service && ContainsKey(services_metrics_, service))
- << "Repeatedly registering " << service->unique_name();
+ LOG_IF(WARNING, ContainsKey(services_metrics_, &service))
+ << "Repeatedly registering " << service.unique_name();
shared_ptr<ServiceMetrics> service_metrics(new ServiceMetrics());
- services_metrics_[service] = service_metrics;
+ services_metrics_[&service] = service_metrics;
InitializeCommonServiceMetrics(service);
}
-void Metrics::DeregisterService(const Service *service) {
- services_metrics_.erase(service);
+void Metrics::DeregisterService(const Service &service) {
+ services_metrics_.erase(&service);
}
void Metrics::AddServiceStateTransitionTimer(
- const Service *service,
+ const Service &service,
const string &histogram_name,
Service::ConnectState start_state,
Service::ConnectState stop_state) {
SLOG(Metrics, 2) << __func__ << ": adding " << histogram_name << " for "
<< Service::ConnectStateToString(start_state) << " -> "
<< Service::ConnectStateToString(stop_state);
- ServiceMetricsLookupMap::iterator it = services_metrics_.find(service);
+ ServiceMetricsLookupMap::iterator it = services_metrics_.find(&service);
if (it == services_metrics_.end()) {
SLOG(Metrics, 1) << "service not found";
DCHECK(false);
@@ -554,9 +554,9 @@
was_online_ = (service != NULL);
}
-void Metrics::NotifyServiceStateChanged(const Service *service,
+void Metrics::NotifyServiceStateChanged(const Service &service,
Service::ConnectState new_state) {
- ServiceMetricsLookupMap::iterator it = services_metrics_.find(service);
+ ServiceMetricsLookupMap::iterator it = services_metrics_.find(&service);
if (it == services_metrics_.end()) {
SLOG(Metrics, 1) << "service not found";
DCHECK(false);
@@ -572,8 +572,8 @@
bootstat_log(
StringPrintf("network-%s-%s",
Technology::NameFromIdentifier(
- service->technology()).c_str(),
- service->GetStateString().c_str()).c_str());
+ service.technology()).c_str(),
+ service.GetStateString().c_str()).c_str());
}
if (new_state != Service::kStateConnected)
@@ -582,7 +582,7 @@
base::TimeDelta time_resume_to_ready;
time_resume_to_ready_timer_->GetElapsedTime(&time_resume_to_ready);
time_resume_to_ready_timer_->Reset();
- service->SendPostReadyStateMetrics(time_resume_to_ready.InMilliseconds());
+ service.SendPostReadyStateMetrics(time_resume_to_ready.InMilliseconds());
}
string Metrics::GetFullMetricName(const char *metric_name,
@@ -592,11 +592,11 @@
return base::StringPrintf(metric_name, technology.c_str());
}
-void Metrics::NotifyServiceDisconnect(const Service *service) {
- Technology::Identifier technology = service->technology();
+void Metrics::NotifyServiceDisconnect(const Service &service) {
+ Technology::Identifier technology = service.technology();
string histogram = GetFullMetricName(kMetricDisconnect, technology);
SendToUMA(histogram,
- service->explicitly_disconnected(),
+ service.explicitly_disconnected(),
kMetricDisconnectMin,
kMetricDisconnectMax,
kMetricDisconnectNumBuckets);
@@ -1030,8 +1030,8 @@
return library_->SendToUMA(name, sample, min, max, num_buckets);
}
-void Metrics::InitializeCommonServiceMetrics(const Service *service) {
- Technology::Identifier technology = service->technology();
+void Metrics::InitializeCommonServiceMetrics(const Service &service) {
+ Technology::Identifier technology = service.technology();
string histogram = GetFullMetricName(kMetricTimeToConfigMilliseconds,
technology);
AddServiceStateTransitionTimer(
@@ -1075,9 +1075,9 @@
}
}
-void Metrics::SendServiceFailure(const Service *service) {
+void Metrics::SendServiceFailure(const Service &service) {
library_->SendEnumToUMA(kMetricNetworkServiceErrors,
- service->failure(),
+ service.failure(),
kMetricNetworkServiceErrorsMax);
}
diff --git a/metrics.h b/metrics.h
index daa1ba6..4091582 100644
--- a/metrics.h
+++ b/metrics.h
@@ -450,16 +450,16 @@
// Registers a service with this object so it can use the timers to track
// state transition metrics.
- void RegisterService(const Service *service);
+ void RegisterService(const Service &service);
// Deregisters the service from this class. All state transition timers
// will be removed.
- void DeregisterService(const Service *service);
+ void DeregisterService(const Service &service);
// Tracks the time it takes |service| to go from |start_state| to
// |stop_state|. When |stop_state| is reached, the time is sent to UMA.
virtual void AddServiceStateTransitionTimer(
- const Service *service, const std::string &histogram_name,
+ const Service &service, const std::string &histogram_name,
Service::ConnectState start_state, Service::ConnectState stop_state);
// Specializes |metric_name| for the specified |technology_id|.
@@ -471,11 +471,11 @@
virtual void NotifyDefaultServiceChanged(const Service *service);
// Notifies this object that |service| state has changed.
- virtual void NotifyServiceStateChanged(const Service *service,
+ virtual void NotifyServiceStateChanged(const Service &service,
Service::ConnectState new_state);
// Notifies this object that |service| has been disconnected.
- void NotifyServiceDisconnect(const Service *service);
+ void NotifyServiceDisconnect(const Service &service);
// Notifies this object of power at disconnect.
void NotifySignalAtDisconnect(const Service &service,
@@ -645,10 +645,10 @@
static const uint16 kWiFiFrequency5745;
static const uint16 kWiFiFrequency5825;
- void InitializeCommonServiceMetrics(const Service *service);
+ void InitializeCommonServiceMetrics(const Service &service);
void UpdateServiceStateTransitionMetrics(ServiceMetrics *service_metrics,
Service::ConnectState new_state);
- void SendServiceFailure(const Service *service);
+ void SendServiceFailure(const Service &service);
DeviceMetrics *GetDeviceMetrics(int interface_index) const;
void AutoConnectMetricsReset(DeviceMetrics *device_metrics);
diff --git a/metrics_unittest.cc b/metrics_unittest.cc
index 81be9db..c3b1ab1 100644
--- a/metrics_unittest.cc
+++ b/metrics_unittest.cc
@@ -126,8 +126,8 @@
Metrics::kTimerHistogramMillisecondsMin,
Metrics::kTimerHistogramMillisecondsMax,
Metrics::kTimerHistogramNumBuckets));
- metrics_.NotifyServiceStateChanged(service_, Service::kStateConfiguring);
- metrics_.NotifyServiceStateChanged(service_, Service::kStateConnected);
+ metrics_.NotifyServiceStateChanged(*service_, Service::kStateConfiguring);
+ metrics_.NotifyServiceStateChanged(*service_, Service::kStateConnected);
}
TEST_F(MetricsTest, TimeToPortal) {
@@ -136,8 +136,8 @@
Metrics::kTimerHistogramMillisecondsMin,
Metrics::kTimerHistogramMillisecondsMax,
Metrics::kTimerHistogramNumBuckets));
- metrics_.NotifyServiceStateChanged(service_, Service::kStateConnected);
- metrics_.NotifyServiceStateChanged(service_, Service::kStatePortal);
+ metrics_.NotifyServiceStateChanged(*service_, Service::kStateConnected);
+ metrics_.NotifyServiceStateChanged(*service_, Service::kStatePortal);
}
TEST_F(MetricsTest, TimeToOnline) {
@@ -146,17 +146,17 @@
Metrics::kTimerHistogramMillisecondsMin,
Metrics::kTimerHistogramMillisecondsMax,
Metrics::kTimerHistogramNumBuckets));
- metrics_.NotifyServiceStateChanged(service_, Service::kStateConnected);
- metrics_.NotifyServiceStateChanged(service_, Service::kStateOnline);
+ metrics_.NotifyServiceStateChanged(*service_, Service::kStateConnected);
+ metrics_.NotifyServiceStateChanged(*service_, Service::kStateOnline);
}
TEST_F(MetricsTest, ServiceFailure) {
- EXPECT_CALL(*service_.get(), failure())
+ EXPECT_CALL(*service_, failure())
.WillRepeatedly(Return(Service::kFailureBadPassphrase));
EXPECT_CALL(library_, SendEnumToUMA(Metrics::kMetricNetworkServiceErrors,
Service::kFailureBadPassphrase,
Metrics::kMetricNetworkServiceErrorsMax));
- metrics_.NotifyServiceStateChanged(service_, Service::kStateFailure);
+ metrics_.NotifyServiceStateChanged(*service_, Service::kStateFailure);
}
TEST_F(MetricsTest, WiFiServiceTimeToJoin) {
@@ -165,9 +165,9 @@
Metrics::kTimerHistogramMillisecondsMin,
Metrics::kTimerHistogramMillisecondsMax,
Metrics::kTimerHistogramNumBuckets));
- metrics_.NotifyServiceStateChanged(open_wifi_service_,
+ metrics_.NotifyServiceStateChanged(*open_wifi_service_,
Service::kStateAssociating);
- metrics_.NotifyServiceStateChanged(open_wifi_service_,
+ metrics_.NotifyServiceStateChanged(*open_wifi_service_,
Service::kStateConfiguring);
}
@@ -192,7 +192,7 @@
wep_wifi_service_->frequency_ = 2412;
wep_wifi_service_->physical_mode_ = Metrics::kWiFiNetworkPhyMode11a;
wep_wifi_service_->raw_signal_strength_ = kStrength;
- metrics_.NotifyServiceStateChanged(wep_wifi_service_,
+ metrics_.NotifyServiceStateChanged(*wep_wifi_service_,
Service::kStateConnected);
Mock::VerifyAndClearExpectations(&library_);
@@ -211,7 +211,7 @@
WillOnce(DoAll(SetArgumentPointee<0>(non_zero_time_delta), Return(true)));
metrics_.NotifyPowerStateChange(PowerManagerProxyDelegate::kMem);
metrics_.NotifyPowerStateChange(PowerManagerProxyDelegate::kOn);
- metrics_.NotifyServiceStateChanged(wep_wifi_service_,
+ metrics_.NotifyServiceStateChanged(*wep_wifi_service_,
Service::kStateConnected);
Mock::VerifyAndClearExpectations(&library_);
Mock::VerifyAndClearExpectations(mock_time_resume_to_ready_timer);
@@ -224,7 +224,7 @@
-kStrength);
EXPECT_CALL(library_, SendToUMA("Network.Shill.Wifi.TimeResumeToReady",
_, _, _, _)).Times(0);
- metrics_.NotifyServiceStateChanged(wep_wifi_service_,
+ metrics_.NotifyServiceStateChanged(*wep_wifi_service_,
Service::kStateConnected);
}
@@ -239,7 +239,7 @@
eap_wifi_service_->physical_mode_ = Metrics::kWiFiNetworkPhyMode11a;
eap_wifi_service_->raw_signal_strength_ = kStrength;
EXPECT_CALL(*eap_, OutputConnectionMetrics(&metrics_, Technology::kWifi));
- metrics_.NotifyServiceStateChanged(eap_wifi_service_,
+ metrics_.NotifyServiceStateChanged(*eap_wifi_service_,
Service::kStateConnected);
}
@@ -263,7 +263,7 @@
adhoc_wifi_service->frequency_ = 2412;
adhoc_wifi_service->physical_mode_ = Metrics::kWiFiNetworkPhyMode11b;
adhoc_wifi_service->raw_signal_strength_ = kStrength;
- metrics_.NotifyServiceStateChanged(adhoc_wifi_service,
+ metrics_.NotifyServiceStateChanged(*adhoc_wifi_service,
Service::kStateConnected);
}
@@ -304,7 +304,7 @@
chromeos_metrics::TimerMock *mock_time_to_drop_timer =
new chromeos_metrics::TimerMock;
metrics_.set_time_to_drop_timer(mock_time_to_drop_timer);
- EXPECT_CALL(*service_.get(), technology()).
+ EXPECT_CALL(*service_, technology()).
WillOnce(Return(Technology::kEthernet));
EXPECT_CALL(library_, SendToUMA("Network.Shill.Ethernet.TimeOnline",
Ge(0),
@@ -337,25 +337,25 @@
}
TEST_F(MetricsTest, Disconnect) {
- EXPECT_CALL(*service_.get(), technology()).
+ EXPECT_CALL(*service_, technology()).
WillRepeatedly(Return(Technology::kWifi));
- EXPECT_CALL(*service_.get(), explicitly_disconnected()).
+ EXPECT_CALL(*service_, explicitly_disconnected()).
WillOnce(Return(false));
EXPECT_CALL(library_, SendToUMA("Network.Shill.Wifi.Disconnect",
false,
Metrics::kMetricDisconnectMin,
Metrics::kMetricDisconnectMax,
Metrics::kMetricDisconnectNumBuckets));
- metrics_.NotifyServiceDisconnect(service_);
+ metrics_.NotifyServiceDisconnect(*service_);
- EXPECT_CALL(*service_.get(), explicitly_disconnected()).
+ EXPECT_CALL(*service_, explicitly_disconnected()).
WillOnce(Return(true));
EXPECT_CALL(library_, SendToUMA("Network.Shill.Wifi.Disconnect",
true,
Metrics::kMetricDisconnectMin,
Metrics::kMetricDisconnectMax,
Metrics::kMetricDisconnectNumBuckets));
- metrics_.NotifyServiceDisconnect(service_);
+ metrics_.NotifyServiceDisconnect(*service_);
}
TEST_F(MetricsTest, PortalDetectionResultToEnum) {
diff --git a/mock_metrics.h b/mock_metrics.h
index 370ad35..4309402 100644
--- a/mock_metrics.h
+++ b/mock_metrics.h
@@ -19,7 +19,7 @@
MOCK_METHOD0(Start, void());
MOCK_METHOD0(Stop, void());
MOCK_METHOD4(AddServiceStateTransitionTimer,
- void(const Service *service, const std::string &histogram_name,
+ void(const Service &service, const std::string &histogram_name,
Service::ConnectState start_state,
Service::ConnectState stop_state));
MOCK_METHOD1(NotifyDeviceScanStarted, void (int interface_index));
@@ -31,7 +31,7 @@
MOCK_METHOD1(ResetConnectTimer, void(int interface_index));
MOCK_METHOD1(NotifyDefaultServiceChanged, void(const Service *service));
MOCK_METHOD2(NotifyServiceStateChanged,
- void(const Service *service, Service::ConnectState new_state));
+ void(const Service &service, Service::ConnectState new_state));
MOCK_METHOD0(Notify3GPPRegistrationDelayedDropPosted, void());
MOCK_METHOD0(Notify3GPPRegistrationDelayedDropCanceled, void());
MOCK_METHOD0(NotifyCorruptedProfile, void());
diff --git a/service.cc b/service.cc
index 8fea472..d351482 100644
--- a/service.cc
+++ b/service.cc
@@ -206,7 +206,7 @@
&Service::GetDisconnectsProperty);
HelpRegisterConstDerivedStrings(kDiagnosticsMisconnectsProperty,
&Service::GetMisconnectsProperty);
- metrics_->RegisterService(this);
+ metrics_->RegisterService(*this);
static_ip_parameters_.PlumbPropertyStore(&store_);
@@ -218,7 +218,7 @@
}
Service::~Service() {
- metrics_->DeregisterService(this);
+ metrics_->DeregisterService(*this);
LOG(INFO) << "Service " << unique_name_ << " destroyed.";
}
@@ -335,7 +335,7 @@
}
UpdateErrorProperty();
manager_->UpdateService(this);
- metrics_->NotifyServiceStateChanged(this, state);
+ metrics_->NotifyServiceStateChanged(*this, state);
adaptor_->EmitStringChanged(kStateProperty, GetStateString());
}
diff --git a/wifi.cc b/wifi.cc
index 785b9b5..b94c8f2 100644
--- a/wifi.cc
+++ b/wifi.cc
@@ -838,7 +838,7 @@
metrics()->NotifySignalAtDisconnect(*affected_service,
affected_service->SignalLevel());
affected_service->NotifyCurrentEndpoint(NULL);
- metrics()->NotifyServiceDisconnect(affected_service);
+ metrics()->NotifyServiceDisconnect(*affected_service);
if (affected_service == pending_service_.get()) {
// The attempt to connect to |pending_service_| failed. Clear
diff --git a/wifi_service.cc b/wifi_service.cc
index 0f45082..ea70ab9 100644
--- a/wifi_service.cc
+++ b/wifi_service.cc
@@ -370,7 +370,7 @@
string histogram = metrics()->GetFullMetricName(
Metrics::kMetricTimeToJoinMilliseconds,
technology());
- metrics()->AddServiceStateTransitionTimer(this,
+ metrics()->AddServiceStateTransitionTimer(*this,
histogram,
Service::kStateAssociating,
Service::kStateConfiguring);