shill: Implement service sorting

Sort the list of Services for presentation to
RPC callers, essentially copying the current flimflam
sorting criteria.  Introduce the TechnologyOrder to
the Manager.

BUG=chromium-os:20114
TEST=New unit tests

Change-Id: I766b2297ba3170a7a6ab5adfe68425a8011be4fb
Reviewed-on: http://gerrit.chromium.org/gerrit/8205
Tested-by: Paul Stewart <pstew@chromium.org>
Reviewed-by: mukesh agrawal <quiche@chromium.org>
diff --git a/cellular_service.cc b/cellular_service.cc
index f2c2b10..be5af33 100644
--- a/cellular_service.cc
+++ b/cellular_service.cc
@@ -55,6 +55,10 @@
   cellular_->Activate(carrier, error);
 }
 
+bool CellularService::TechnologyIs(const Technology::Identifier type) const {
+  return cellular_->TechnologyIs(type);
+}
+
 string CellularService::GetStorageIdentifier() {
   string id = base::StringPrintf("%s_%s_%s",
                                  kServiceType,
diff --git a/cellular_service.h b/cellular_service.h
index 072a08e..d9c0cd2 100644
--- a/cellular_service.h
+++ b/cellular_service.h
@@ -33,6 +33,7 @@
   virtual void Connect(Error *error);
   virtual void Disconnect();
   virtual void ActivateCellularModem(const std::string &carrier, Error *error);
+  virtual bool TechnologyIs(const Technology::Identifier type) const;
 
   // cellular_<MAC>_<Service_Operator_Name>
   std::string GetStorageIdentifier();
diff --git a/cellular_unittest.cc b/cellular_unittest.cc
index 1e9f63a..12d6a28 100644
--- a/cellular_unittest.cc
+++ b/cellular_unittest.cc
@@ -639,6 +639,7 @@
   EXPECT_EQ(kPaymentURL, device_->service_->payment_url());
   EXPECT_EQ(kUsageURL, device_->service_->usage_url());
   EXPECT_EQ(kTestCarrier, device_->service_->serving_operator().GetName());
+  EXPECT_TRUE(device_->service_->TechnologyIs(Technology::kCellular));
 }
 
 namespace {
diff --git a/ethernet_service.cc b/ethernet_service.cc
index 679266c..783cbf8 100644
--- a/ethernet_service.cc
+++ b/ethernet_service.cc
@@ -45,6 +45,10 @@
 
 void EthernetService::Disconnect() { }
 
+bool EthernetService::TechnologyIs(const Technology::Identifier type) const {
+  return ethernet_->TechnologyIs(type);
+}
+
 std::string EthernetService::GetDeviceRpcId() {
   return ethernet_->GetRpcIdentifier();
 }
diff --git a/ethernet_service.h b/ethernet_service.h
index e132f50..7f75da6 100644
--- a/ethernet_service.h
+++ b/ethernet_service.h
@@ -28,6 +28,7 @@
   // Inherited from Service.
   virtual void Connect(Error *error);
   virtual void Disconnect();
+  virtual bool TechnologyIs(const Technology::Identifier type) const;
 
   // ethernet_<MAC>
   virtual std::string GetStorageIdentifier();
diff --git a/manager.cc b/manager.cc
index 85160cd..1df0dc3 100644
--- a/manager.cc
+++ b/manager.cc
@@ -7,6 +7,7 @@
 #include <time.h>
 #include <stdio.h>
 
+#include <algorithm>
 #include <map>
 #include <string>
 #include <vector>
@@ -14,6 +15,8 @@
 #include <base/file_util.h>
 #include <base/logging.h>
 #include <base/memory/ref_counted.h>
+#include <base/string_split.h>
+#include <base/string_util.h>
 #include <chromeos/dbus/service_constants.h>
 
 #include "shill/adaptor_interfaces.h"
@@ -25,6 +28,7 @@
 #include "shill/ephemeral_profile.h"
 #include "shill/error.h"
 #include "shill/key_file_store.h"
+#include "shill/service_sorter.h"
 #include "shill/profile.h"
 #include "shill/property_accessor.h"
 #include "shill/resolver.h"
@@ -33,6 +37,7 @@
 #include "shill/wifi.h"
 #include "shill/wifi_service.h"
 
+using std::map;
 using std::string;
 using std::vector;
 
@@ -195,6 +200,7 @@
       return;
   }
   services_.push_back(to_manage);
+  SortServices();
 
   vector<string> service_paths;
   for (it = services_.begin(); it != services_.end(); ++it) {
@@ -213,6 +219,7 @@
   for (it = services_.begin(); it != services_.end(); ++it) {
     if (to_forget->UniqueName() == (*it)->UniqueName()) {
       services_.erase(it);
+      SortServices();
       return;
     }
   }
@@ -222,7 +229,7 @@
   LOG(INFO) << "Service " << to_update->UniqueName() << " updated;"
             << " state: " << to_update->state() << " failure: "
             << to_update->failure();
-  // TODO(pstew): This should trigger re-sorting of services, autoconnect, etc.
+  SortServices();
 }
 
 void Manager::FilterByTechnology(Technology::Identifier tech,
@@ -235,7 +242,7 @@
   }
 }
 
-ServiceRefPtr Manager::FindService(const std::string& name) {
+ServiceRefPtr Manager::FindService(const string& name) {
   vector<ServiceRefPtr>::iterator it;
   for (it = services_.begin(); it != services_.end(); ++it) {
     if (name == (*it)->UniqueName())
@@ -260,6 +267,10 @@
       StringsAccessor(new CustomAccessor<Manager, Strings>(this, get, set)));
 }
 
+void Manager::SortServices() {
+  sort(services_.begin(), services_.end(), ServiceSorter(technology_order_));
+}
+
 string Manager::CalculateState() {
   return flimflam::kStateOffline;
 }
@@ -313,7 +324,7 @@
 WiFiServiceRefPtr Manager::GetWifiService(const KeyValueStore &args,
                                           Error *error) {
   std::vector<DeviceRefPtr> wifi_devices;
-  FilterByTechnology(Device::kWifi, &wifi_devices);
+  FilterByTechnology(Technology::kWifi, &wifi_devices);
   if (wifi_devices.empty()) {
     error->Populate(Error::kInvalidArguments, kManagerErrorNoDevice);
     return NULL;
@@ -325,7 +336,7 @@
 }
 
 // called via RPC (e.g., from ManagerDBusAdaptor)
-void Manager::RequestScan(const std::string &technology, Error *error) {
+void Manager::RequestScan(const string &technology, Error *error) {
   if (technology == flimflam::kTypeWifi || technology == "") {
     vector<DeviceRefPtr> wifi_devices;
     FilterByTechnology(Technology::kWifi, &wifi_devices);
@@ -344,4 +355,46 @@
   }
 }
 
+string Manager::GetTechnologyOrder() {
+  vector<string> technology_names;
+  for (vector<Technology::Identifier>::iterator it = technology_order_.begin();
+       it != technology_order_.end();
+       ++it) {
+    technology_names.push_back(Technology::NameFromIdentifier(*it));
+  }
+
+  return JoinString(technology_names, ',');
+}
+
+void Manager::SetTechnologyOrder(const string &order, Error *error) {
+  vector<Technology::Identifier> new_order;
+  map<Technology::Identifier, bool> seen;
+
+  vector<string> order_parts;
+  base::SplitString(order, ',', &order_parts);
+
+  for (vector<string>::iterator it = order_parts.begin();
+       it != order_parts.end();
+       ++it) {
+    Technology::Identifier identifier = Technology::IdentifierFromName(*it);
+
+    if (identifier == Technology::kUnknown) {
+      error->Populate(Error::kInvalidArguments, *it +
+                      " is an unknown technology name");
+      return;
+    }
+
+    if (ContainsKey(seen, identifier)) {
+      error->Populate(Error::kInvalidArguments, *it +
+                      " is duplicated in the list");
+      return;
+    }
+    seen[identifier] = true;
+    new_order.push_back(identifier);
+  }
+
+  technology_order_ = new_order;
+  SortServices();
+}
+
 }  // namespace shill
diff --git a/manager.h b/manager.h
index 6c29c1e..ede4f49 100644
--- a/manager.h
+++ b/manager.h
@@ -12,6 +12,7 @@
 #include <base/memory/ref_counted.h>
 #include <base/memory/scoped_ptr.h>
 #include <chromeos/dbus/service_constants.h>
+#include <gtest/gtest_prod.h>  // for FRIEND_TEST
 
 #include "shill/device.h"
 #include "shill/device_info.h"
@@ -72,6 +73,8 @@
   // called via RPC (e.g., from ManagerDBusAdaptor)
   WiFiServiceRefPtr GetWifiService(const KeyValueStore &args, Error *error);
   void RequestScan(const std::string &technology, Error *error);
+  std::string GetTechnologyOrder();
+  void SetTechnologyOrder(const std::string &order, Error *error);
 
   virtual DeviceInfo *device_info() { return &device_info_; }
   PropertyStore *mutable_store() { return &store_; }
@@ -84,6 +87,8 @@
 
  private:
   friend class ManagerAdaptorInterface;
+  friend class ManagerTest;
+  FRIEND_TEST(ManagerTest, SortServices);
 
   static const char kManagerErrorNoDevice[];
 
@@ -104,6 +109,9 @@
                                   Strings(Manager::*get)(void),
                                   bool(Manager::*set)(const Strings&));
 
+  bool OrderServices(ServiceRefPtr a, ServiceRefPtr b);
+  void SortServices();
+
   const FilePath run_path_;
   const FilePath storage_path_;
   const std::string user_storage_format_;
@@ -119,6 +127,9 @@
   ControlInterface *control_interface_;
   GLib *glib_;
 
+  // The priority order of technologies
+  std::vector<Technology::Identifier> technology_order_;
+
   // Properties to be get/set via PropertyStore calls.
   Properties props_;
   PropertyStore store_;
diff --git a/manager_dbus_adaptor.cc b/manager_dbus_adaptor.cc
index 9318401..7c071cf 100644
--- a/manager_dbus_adaptor.cc
+++ b/manager_dbus_adaptor.cc
@@ -194,11 +194,14 @@
 }
 
 string ManagerDBusAdaptor::GetServiceOrder(::DBus::Error &error) {
-  return string();
+  return manager_->GetTechnologyOrder();
 }
 
-void ManagerDBusAdaptor::SetServiceOrder(const string &,
+void ManagerDBusAdaptor::SetServiceOrder(const string &order,
                                          ::DBus::Error &error) {
+  Error e;
+  manager_->SetTechnologyOrder(order, &e);
+  e.ToDBusError(&error);
 }
 
 }  // namespace shill
diff --git a/manager_unittest.cc b/manager_unittest.cc
index 673be36..d50b7aa 100644
--- a/manager_unittest.cc
+++ b/manager_unittest.cc
@@ -34,6 +34,7 @@
 
 namespace shill {
 using ::testing::_;
+using ::testing::Ne;
 using ::testing::NiceMock;
 using ::testing::Return;
 using ::testing::Test;
@@ -41,31 +42,30 @@
 class ManagerTest : public PropertyStoreTest {
  public:
   ManagerTest()
-      : mock_device_(new NiceMock<MockDevice>(control_interface(),
-                                              dispatcher(),
-                                              manager(),
-                                              "null0",
-                                              "addr0",
-                                              0)),
-        mock_device2_(new NiceMock<MockDevice>(control_interface(),
-                                               dispatcher(),
-                                               manager(),
-                                               "null2",
-                                               "addr2",
-                                               2)),
-        mock_device3_(new NiceMock<MockDevice>(control_interface(),
-                                               dispatcher(),
-                                               manager(),
-                                               "null3",
-                                               "addr3",
-                                               3)),
-        mock_wifi_(new NiceMock<MockWiFi>(control_interface(),
+      : mock_wifi_(new NiceMock<MockWiFi>(control_interface(),
                                           dispatcher(),
                                           manager(),
                                           "wifi0",
                                           "addr4",
-                                          4))
-  {
+                                          4)) {
+    mock_devices_.push_back(new NiceMock<MockDevice>(control_interface(),
+                                                     dispatcher(),
+                                                     manager(),
+                                                     "null0",
+                                                     "addr0",
+                                                     0));
+    mock_devices_.push_back(new NiceMock<MockDevice>(control_interface(),
+                                                     dispatcher(),
+                                                     manager(),
+                                                     "null1",
+                                                     "addr1",
+                                                     1));
+    mock_devices_.push_back(new NiceMock<MockDevice>(control_interface(),
+                                                     dispatcher(),
+                                                     manager(),
+                                                     "null2",
+                                                     "addr2",
+                                                     2));
   }
   virtual ~ManagerTest() {}
 
@@ -75,55 +75,59 @@
     manager()->FilterByTechnology(tech, &devices);
     return (devices.size() == 1 && devices[0].get() == device.get());
   }
+  bool ServiceOrderIs(ServiceRefPtr svc1, ServiceRefPtr svc2);
 
  protected:
-  scoped_refptr<MockDevice> mock_device_;
-  scoped_refptr<MockDevice> mock_device2_;
-  scoped_refptr<MockDevice> mock_device3_;
   scoped_refptr<MockWiFi> mock_wifi_;
+  vector<scoped_refptr<MockDevice> > mock_devices_;
 };
 
+bool ManagerTest::ServiceOrderIs(ServiceRefPtr svc0, ServiceRefPtr svc1) {
+  return (svc0.get() == manager()->services_[0].get() &&
+          svc1.get() == manager()->services_[1].get());
+}
+
 TEST_F(ManagerTest, Contains) {
   EXPECT_TRUE(manager()->store().Contains(flimflam::kStateProperty));
   EXPECT_FALSE(manager()->store().Contains(""));
 }
 
 TEST_F(ManagerTest, DeviceRegistration) {
-  ON_CALL(*mock_device_.get(), TechnologyIs(Technology::kEthernet))
+  ON_CALL(*mock_devices_[0].get(), TechnologyIs(Technology::kEthernet))
       .WillByDefault(Return(true));
-  ON_CALL(*mock_device2_.get(), TechnologyIs(Technology::kWifi))
+  ON_CALL(*mock_devices_[1].get(), TechnologyIs(Technology::kWifi))
       .WillByDefault(Return(true));
-  ON_CALL(*mock_device3_.get(), TechnologyIs(Technology::kCellular))
+  ON_CALL(*mock_devices_[2].get(), TechnologyIs(Technology::kCellular))
       .WillByDefault(Return(true));
 
-  manager()->RegisterDevice(mock_device_);
-  manager()->RegisterDevice(mock_device2_);
-  manager()->RegisterDevice(mock_device3_);
+  manager()->RegisterDevice(mock_devices_[0]);
+  manager()->RegisterDevice(mock_devices_[1]);
+  manager()->RegisterDevice(mock_devices_[2]);
 
-  EXPECT_TRUE(IsDeviceRegistered(mock_device_, Technology::kEthernet));
-  EXPECT_TRUE(IsDeviceRegistered(mock_device2_, Technology::kWifi));
-  EXPECT_TRUE(IsDeviceRegistered(mock_device3_, Technology::kCellular));
+  EXPECT_TRUE(IsDeviceRegistered(mock_devices_[0], Technology::kEthernet));
+  EXPECT_TRUE(IsDeviceRegistered(mock_devices_[1], Technology::kWifi));
+  EXPECT_TRUE(IsDeviceRegistered(mock_devices_[2], Technology::kCellular));
 }
 
 TEST_F(ManagerTest, DeviceDeregistration) {
-  ON_CALL(*mock_device_.get(), TechnologyIs(Technology::kEthernet))
+  ON_CALL(*mock_devices_[0].get(), TechnologyIs(Technology::kEthernet))
       .WillByDefault(Return(true));
-  ON_CALL(*mock_device2_.get(), TechnologyIs(Technology::kWifi))
+  ON_CALL(*mock_devices_[1].get(), TechnologyIs(Technology::kWifi))
       .WillByDefault(Return(true));
 
-  manager()->RegisterDevice(mock_device_.get());
-  manager()->RegisterDevice(mock_device2_.get());
+  manager()->RegisterDevice(mock_devices_[0].get());
+  manager()->RegisterDevice(mock_devices_[1].get());
 
-  ASSERT_TRUE(IsDeviceRegistered(mock_device_, Technology::kEthernet));
-  ASSERT_TRUE(IsDeviceRegistered(mock_device2_, Technology::kWifi));
+  ASSERT_TRUE(IsDeviceRegistered(mock_devices_[0], Technology::kEthernet));
+  ASSERT_TRUE(IsDeviceRegistered(mock_devices_[1], Technology::kWifi));
 
-  EXPECT_CALL(*mock_device_.get(), Stop());
-  manager()->DeregisterDevice(mock_device_.get());
-  EXPECT_FALSE(IsDeviceRegistered(mock_device_, Technology::kEthernet));
+  EXPECT_CALL(*mock_devices_[0].get(), Stop());
+  manager()->DeregisterDevice(mock_devices_[0].get());
+  EXPECT_FALSE(IsDeviceRegistered(mock_devices_[0], Technology::kEthernet));
 
-  EXPECT_CALL(*mock_device2_.get(), Stop());
-  manager()->DeregisterDevice(mock_device2_.get());
-  EXPECT_FALSE(IsDeviceRegistered(mock_device2_, Technology::kWifi));
+  EXPECT_CALL(*mock_devices_[1].get(), Stop());
+  manager()->DeregisterDevice(mock_devices_[1].get());
+  EXPECT_FALSE(IsDeviceRegistered(mock_devices_[1], Technology::kWifi));
 }
 
 TEST_F(ManagerTest, ServiceRegistration) {
@@ -198,8 +202,8 @@
 }
 
 TEST_F(ManagerTest, GetDevicesProperty) {
-  manager()->RegisterDevice(mock_device_.get());
-  manager()->RegisterDevice(mock_device2_.get());
+  manager()->RegisterDevice(mock_devices_[0].get());
+  manager()->RegisterDevice(mock_devices_[1].get());
   {
     map<string, ::DBus::Variant> props;
     ::DBus::Error dbus_error;
@@ -310,14 +314,14 @@
 TEST_F(ManagerTest, RequestScan) {
   {
     Error error;
-    manager()->RegisterDevice(mock_device_.get());
-    manager()->RegisterDevice(mock_device2_.get());
-    EXPECT_CALL(*mock_device_, TechnologyIs(Technology::kWifi))
+    manager()->RegisterDevice(mock_devices_[0].get());
+    manager()->RegisterDevice(mock_devices_[1].get());
+    EXPECT_CALL(*mock_devices_[0], TechnologyIs(Technology::kWifi))
         .WillRepeatedly(Return(true));
-    EXPECT_CALL(*mock_device_, Scan(_));
-    EXPECT_CALL(*mock_device2_, TechnologyIs(Technology::kWifi))
+    EXPECT_CALL(*mock_devices_[0], Scan(_));
+    EXPECT_CALL(*mock_devices_[1], TechnologyIs(Technology::kWifi))
         .WillRepeatedly(Return(false));
-    EXPECT_CALL(*mock_device2_, Scan(_)).Times(0);
+    EXPECT_CALL(*mock_devices_[1], Scan(_)).Times(0);
     manager()->RequestScan(flimflam::kTypeWifi, &error);
   }
 
@@ -347,4 +351,103 @@
   manager()->GetWifiService(args, &e);
 }
 
+TEST_F(ManagerTest, TechnologyOrder) {
+  Error error;
+  manager()->SetTechnologyOrder(string(flimflam::kTypeEthernet) + "," +
+                                string(flimflam::kTypeWifi), &error);
+  ASSERT_TRUE(error.IsSuccess());
+  EXPECT_EQ(manager()->GetTechnologyOrder(),
+            string(flimflam::kTypeEthernet) + "," +
+            string(flimflam::kTypeWifi));
+
+  manager()->SetTechnologyOrder(string(flimflam::kTypeEthernet) + "x," +
+                                string(flimflam::kTypeWifi), &error);
+  ASSERT_FALSE(error.IsSuccess());
+  EXPECT_EQ(Error::kInvalidArguments, error.type());
+  EXPECT_EQ(string(flimflam::kTypeEthernet) + "," +
+            string(flimflam::kTypeWifi),
+            manager()->GetTechnologyOrder());
+}
+
+TEST_F(ManagerTest, SortServices) {
+  scoped_refptr<MockService> mock_service0(
+      new NiceMock<MockService>(control_interface(),
+                                dispatcher(),
+                                manager()));
+  scoped_refptr<MockService> mock_service1(
+      new NiceMock<MockService>(control_interface(),
+                                dispatcher(),
+                                manager()));
+  string service1_name(mock_service0->UniqueName());
+  string service2_name(mock_service1->UniqueName());
+
+  manager()->RegisterService(mock_service0);
+  manager()->RegisterService(mock_service1);
+
+  // Services should already be sorted by UniqueName
+  EXPECT_TRUE(ServiceOrderIs(mock_service0, mock_service1));
+
+  // Asking explictly to sort services should not change anything
+  manager()->SortServices();
+  EXPECT_TRUE(ServiceOrderIs(mock_service0, mock_service1));
+
+  // Two otherwise equal services should be reordered by strength
+  mock_service1->set_strength(1);
+  manager()->UpdateService(mock_service1);
+  EXPECT_TRUE(ServiceOrderIs(mock_service1, mock_service0));
+
+  // Security
+  mock_service0->set_security(1);
+  manager()->UpdateService(mock_service0);
+  EXPECT_TRUE(ServiceOrderIs(mock_service0, mock_service1));
+
+  // Technology
+  EXPECT_CALL(*mock_service0.get(), TechnologyIs(Technology::kWifi))
+      .WillRepeatedly(Return(true));
+  EXPECT_CALL(*mock_service1.get(), TechnologyIs(Technology::kEthernet))
+      .WillRepeatedly(Return(true));
+  // NB: Redefine default (false) return values so we don't use the default rule
+  // which makes the logs noisier
+  EXPECT_CALL(*mock_service0.get(), TechnologyIs(Ne(Technology::kWifi)))
+      .WillRepeatedly(Return(false));
+  EXPECT_CALL(*mock_service1.get(), TechnologyIs(Ne(Technology::kEthernet)))
+      .WillRepeatedly(Return(false));
+
+  Error error;
+  manager()->SetTechnologyOrder(string(flimflam::kTypeEthernet) + "," +
+                                string(flimflam::kTypeWifi), &error);
+  EXPECT_TRUE(error.IsSuccess());
+  EXPECT_TRUE(ServiceOrderIs(mock_service1, mock_service0));
+
+  manager()->SetTechnologyOrder(string(flimflam::kTypeWifi) + "," +
+                                string(flimflam::kTypeEthernet), &error);
+  EXPECT_TRUE(error.IsSuccess());
+  EXPECT_TRUE(ServiceOrderIs(mock_service0, mock_service1));
+
+  // Priority
+  mock_service0->set_priority(1);
+  manager()->UpdateService(mock_service0);
+  EXPECT_TRUE(ServiceOrderIs(mock_service0, mock_service1));
+
+  // Favorite
+  mock_service1->set_favorite(true);
+  manager()->UpdateService(mock_service1);
+  EXPECT_TRUE(ServiceOrderIs(mock_service1, mock_service0));
+
+  // Connecting
+  EXPECT_CALL(*mock_service0.get(), state())
+      .WillRepeatedly(Return(Service::kStateAssociating));
+  manager()->UpdateService(mock_service0);
+  EXPECT_TRUE(ServiceOrderIs(mock_service0, mock_service1));
+
+  // Connected
+  EXPECT_CALL(*mock_service1.get(), state())
+      .WillRepeatedly(Return(Service::kStateConnected));
+  manager()->UpdateService(mock_service1);
+  EXPECT_TRUE(ServiceOrderIs(mock_service1, mock_service0));
+
+  manager()->DeregisterService(mock_service0);
+  manager()->DeregisterService(mock_service1);
+}
+
 }  // namespace shill
diff --git a/mock_service.cc b/mock_service.cc
index 6125479..fafb8cd 100644
--- a/mock_service.cc
+++ b/mock_service.cc
@@ -13,6 +13,7 @@
 #include "shill/refptr_types.h"
 
 using std::string;
+using testing::_;
 using testing::Return;
 
 namespace shill {
@@ -26,6 +27,9 @@
                          Manager *manager)
     : Service(control_interface, dispatcher, manager, "mock") {
   ON_CALL(*this, GetRpcIdentifier()).WillByDefault(Return(""));
+  ON_CALL(*this, state()).WillByDefault(Return(kStateUnknown));
+  ON_CALL(*this, failure()).WillByDefault(Return(kFailureUnknown));
+  ON_CALL(*this, TechnologyIs(_)).WillByDefault(Return(false));
 }
 
 MockService::~MockService() {}
diff --git a/mock_service.h b/mock_service.h
index b3a720e..2f77a6f 100644
--- a/mock_service.h
+++ b/mock_service.h
@@ -24,6 +24,8 @@
   MOCK_METHOD1(Connect, void(Error *error));
   MOCK_METHOD0(Disconnect, void());
   MOCK_METHOD0(CalculateState, std::string());
+  MOCK_CONST_METHOD1(TechnologyIs,
+                     bool(const Technology::Identifier technology));
   MOCK_METHOD1(SetState, void(ConnectState state));
   MOCK_CONST_METHOD0(state, ConnectState());
   MOCK_METHOD1(SetFailure, void(ConnectFailure failure));
@@ -31,8 +33,8 @@
   MOCK_METHOD0(GetDeviceRpcId, std::string());
   MOCK_CONST_METHOD0(GetRpcIdentifier, std::string());
   MOCK_METHOD0(GetStorageIdentifier, std::string());
-  MOCK_METHOD1(Load, bool(StoreInterface *));
-  MOCK_METHOD1(Save, bool(StoreInterface *));
+  MOCK_METHOD1(Load, bool(StoreInterface *store_interface));
+  MOCK_METHOD1(Save, bool(StoreInterface *store_interface));
 
  private:
   DISALLOW_COPY_AND_ASSIGN(MockService);
diff --git a/service.cc b/service.cc
index f4867bc..4a5f967 100644
--- a/service.cc
+++ b/service.cc
@@ -74,6 +74,8 @@
       connectable_(false),
       favorite_(false),
       priority_(kPriorityNone),
+      security_(0),
+      strength_(0),
       save_credentials_(true),
       type_(type),
       dispatcher_(dispatcher),
@@ -163,6 +165,10 @@
   error->Populate(Error::kInvalidArguments, kMessage);
 }
 
+bool Service::TechnologyIs(const Technology::Identifier type) const {
+  return false;
+}
+
 bool Service::IsActive() {
   return state_ != kStateUnknown &&
     state_ != kStateIdle &&
@@ -264,6 +270,56 @@
   return true;
 }
 
+bool Service::DecideBetween(int a, int b, bool *decision) {
+  if (a == b)
+    return false;
+  *decision = (a > b);
+  return true;
+}
+
+bool Service::Compare(ServiceRefPtr a,
+                      ServiceRefPtr b,
+                      const vector<Technology::Identifier> &tech_order) {
+  bool ret;
+
+  if (a->state() != b->state()) {
+    if (DecideBetween(a->IsConnected(), b->IsConnected(), &ret)) {
+      return ret;
+    }
+
+    // TODO(pstew): Services don't know about portal state yet
+
+    if (DecideBetween(a->IsConnecting(), b->IsConnecting(), &ret)) {
+      return ret;
+    }
+  }
+
+  if (DecideBetween(a->favorite(), b->favorite(), &ret) ||
+      DecideBetween(a->priority(), b->priority(), &ret)) {
+    return ret;
+  }
+
+  // TODO(pstew): Below this point we are making value judgements on
+  // services that are not related to anything intrinsic or
+  // user-specified.  These heuristics should be richer (contain
+  // historical information, for example) and be subject to user
+  // customization.
+
+  for (vector<Technology::Identifier>::const_iterator it = tech_order.begin();
+       it != tech_order.end();
+       ++it) {
+    if (DecideBetween(a->TechnologyIs(*it), b->TechnologyIs(*it), &ret))
+      return ret;
+  }
+
+  if (DecideBetween(a->security(), b->security(), &ret) ||
+      DecideBetween(a->strength(), b->strength(), &ret)) {
+    return ret;
+  }
+
+  return a->UniqueName() < b->UniqueName();
+}
+
 const ProfileRefPtr &Service::profile() const { return profile_; }
 
 void Service::set_profile(const ProfileRefPtr &p) { profile_ = p; }
diff --git a/service.h b/service.h
index 032693f..d6912e8 100644
--- a/service.h
+++ b/service.h
@@ -16,6 +16,7 @@
 #include "shill/accessor_interface.h"
 #include "shill/property_store.h"
 #include "shill/refptr_types.h"
+#include "shill/technology.h"
 
 namespace shill {
 
@@ -97,6 +98,9 @@
   // The default implementation sets |error| to kInvalidArguments.
   virtual void ActivateCellularModem(const std::string &carrier, Error *error);
 
+  // Base method always returns false.
+  virtual bool TechnologyIs(const Technology::Identifier type) const;
+
   virtual bool IsActive();
 
   virtual ConnectState state() const { return state_; }
@@ -104,6 +108,12 @@
   // clears |failure_| if the new state isn't a failure.
   virtual void SetState(ConnectState state);
 
+  // State utility functions
+  bool IsConnected() const { return state() == kStateConnected; }
+  bool IsConnecting() const {
+    return state() == kStateAssociating || state() == kStateConfiguring;
+  }
+
   virtual ConnectFailure failure() const { return failure_; }
   // Records the failure mode, and sets the Service state to "Failure".
   virtual void SetFailure(ConnectFailure failure);
@@ -126,9 +136,27 @@
   bool auto_connect() const { return auto_connect_; }
   void set_auto_connect(bool connect) { auto_connect_ = connect; }
 
+  bool favorite() const { return favorite_; }
+  void set_favorite(bool favorite) { favorite_ = favorite; }
+
+  int32 priority() const { return priority_; }
+  void set_priority(int32 priority) { priority_ = priority; }
+
+  int32 security() const { return security_; }
+  void set_security(int32 security) { security_ = security; }
+
+  int32 strength() const { return strength_; }
+  void set_strength(int32 strength) { strength_ = strength; }
+
   const std::string &error() const { return error_; }
   void set_error(const std::string &error) { error_ = error; }
 
+  // Compare two services.  Returns true if Service a should be displayed
+  // above Service b
+  static bool Compare(ServiceRefPtr a,
+                      ServiceRefPtr b,
+                      const std::vector<Technology::Identifier> &tech_order);
+
   // These are defined in service.cc so that we don't have to include profile.h
   // TODO(cmasone): right now, these are here only so that we can get the
   // profile name as a property.  Can we store just the name, and then handle
@@ -212,6 +240,10 @@
     return "";  // Will need to call Profile to get this.
   }
 
+  // Utility function that returns true if a is different from b.  When they
+  // are, "decision" is populated with the boolean value of "a > b".
+  static bool DecideBetween(int a, int b, bool *decision);
+
   ConnectState state_;
   ConnectFailure failure_;
   bool auto_connect_;
@@ -220,6 +252,8 @@
   std::string error_;
   bool favorite_;
   int32 priority_;
+  int32 security_;
+  int32 strength_;
   std::string proxy_config_;
   bool save_credentials_;
   EapCredentials eap_;  // Only saved if |save_credentials_| is true.
diff --git a/service_sorter.h b/service_sorter.h
new file mode 100644
index 0000000..7707a17
--- /dev/null
+++ b/service_sorter.h
@@ -0,0 +1,36 @@
+// Copyright (c) 2011 The Chromium OS Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef SERVICE_SORTER_
+#define SERVICE_SORTER_
+
+#include <vector>
+
+#include "shill/refptr_types.h"
+#include "shill/service.h"
+
+namespace shill {
+
+class Manager;
+
+// This is a closure used by the Manager for STL sorting of the
+// Service array.  We pass instances of this object to STL sort(),
+// which in turn will call the selected function in the Manager to
+// compare two Service objects at a time.
+class ServiceSorter {
+ public:
+  explicit ServiceSorter(const std::vector<Technology::Identifier> &tech_order)
+      : technology_order_(tech_order) {}
+  bool operator() (ServiceRefPtr a, ServiceRefPtr b) {
+    return Service::Compare(a, b, technology_order_);
+  }
+
+ private:
+  const std::vector<Technology::Identifier> &technology_order_;
+  // We can't DISALLOW_COPY_AND_ASSIGN since this is passed by value to STL sort
+};
+
+}  // namespace shill
+
+#endif  // SERVICE_SORTER_
diff --git a/wifi_service.cc b/wifi_service.cc
index 6ed8b79..cba5a2a 100644
--- a/wifi_service.cc
+++ b/wifi_service.cc
@@ -78,6 +78,10 @@
   // XXX remove from favorite networks list?
 }
 
+bool WiFiService::TechnologyIs(const Technology::Identifier type) const {
+  return wifi_->TechnologyIs(type);
+}
+
 string WiFiService::GetStorageIdentifier() {
   return StringToLowerASCII(base::StringPrintf("%s_%s_%s_%s_%s",
                                                flimflam::kTypeWifi,
diff --git a/wifi_service.h b/wifi_service.h
index e0176af..66c0630 100644
--- a/wifi_service.h
+++ b/wifi_service.h
@@ -34,6 +34,8 @@
   virtual void Connect(Error *error);
   virtual void Disconnect();
 
+  virtual bool TechnologyIs(const Technology::Identifier type) const;
+
   // wifi_<MAC>_<BSSID>_<mode_string>_<security_string>
   std::string GetStorageIdentifier();
 
diff --git a/wifi_service_unittest.cc b/wifi_service_unittest.cc
index 5d9d309..e142d86 100644
--- a/wifi_service_unittest.cc
+++ b/wifi_service_unittest.cc
@@ -56,6 +56,7 @@
                 isxdigit(id[i]) ||
                 (isalpha(id[i]) && islower(id[i])));
   }
+  EXPECT_TRUE(wifi_service->TechnologyIs(Technology::kWifi));
   size_t mac_pos = id.find(StringToLowerASCII(string(fake_mac)));
   EXPECT_NE(mac_pos, string::npos);
   EXPECT_NE(id.find(string(flimflam::kModeManaged), mac_pos), string::npos);