Replace misc. network stub flags with more flexible ones

chromeos/dbus/shill_stub_helper.cc

Index: chromeos/dbus/shill_stub_helper.cc
diff --git a/chromeos/dbus/shill_stub_helper.cc b/chromeos/dbus/shill_stub_helper.cc
index fe1e0d079857cb729351410b5b9fcfbaf88c66e8..c41c46b00c0300ab98936dba88e0b95a4d2a737c 100644
--- a/chromeos/dbus/shill_stub_helper.cc
+++ b/chromeos/dbus/shill_stub_helper.cc
@@ -5,9 +5,7 @@
 #include "chromeos/dbus/shill_stub_helper.h"
 
 #include "base/bind.h"
-#include "base/command_line.h"
 #include "base/strings/string_tokenizer.h"
-#include "chromeos/chromeos_switches.h"
 #include "chromeos/dbus/dbus_thread_manager.h"
 #include "chromeos/dbus/shill_device_client.h"
 #include "chromeos/dbus/shill_manager_client.h"
@@ -37,22 +35,30 @@ void UpdatePortalledWifiState() {
                                base::StringValue(shill::kStatePortal));
 }
 
-// Returns true if |network_type| is found in the comma separated list given
-// with kEnabledStubNetworkTypes switch.
-bool IsStubNetworkTypeEnabled(const std::string& network_type) {
-  CommandLine* command_line = CommandLine::ForCurrentProcess();
-  // If the switch is not present, enabled by default.
-  if (!command_line->HasSwitch(switches::kEnabledStubNetworkTypes))
-    return true;
-
-  const std::string value =
-      command_line->GetSwitchValueASCII(switches::kEnabledStubNetworkTypes);
-  base::StringTokenizer tokenizer(value, ",");
-  while (tokenizer.GetNext()) {
-    if (tokenizer.token() == network_type)
-      return true;
+std::string GetShillInitialState(const std::string& type,
+                                 bool* enabled,
+                                 bool* portaled) {
+  std::string state = DBusThreadManager::Get()->GetShillInitialState(type);
+  if (state == shill::kStateDisconnect) {
+    *enabled = false;
+    *portaled = false;
+    return shill::kStateIdle;
   }
-  return false;
+  if (state == shill::kStatePortal) {
+    if (type != shill::kTypeWifi)
+      LOG(WARNING) << "Invalid state: " << state << " for " << type;
+    *enabled = true;
+    *portaled = true;
+    return shill::kStateIdle;
+  }
+  if (state == shill::kActivationStateActivated &&
+      type != shill::kTypeCellular) {
+    LOG(WARNING) << "Invalid state: " << state << " for " << type;
+    state = shill::kStateIdle;
+  }
+  *enabled = true;
+  *portaled = false;
+  return state;
 }
 
 }  // namespace
@@ -60,155 +66,184 @@ bool IsStubNetworkTypeEnabled(const std::string& network_type) {
 const char kSharedProfilePath[] = "/profile/default";
 
 bool IsStubPortalledWifiEnabled(const std::string& path) {

[pneubeck (no reviews), 2014/02/27 09:04:37]

this super hacky function could move for example to
ShillServiceClient->GetTestInterface.

or the network_portal_detector_impl.cc:363 gets it's trigger more directly, e.g.
by reading the command line again.

or network_portal_detector_impl.cc:363 checks for default_service_path_ ==
kStubPortalledWifiPath, which is correct if that path is not a correct
Shill/DBus path (which should be commented at the definition).

[stevenjb, 2014/02/28 02:44:06]

Looking at the code that calls this, this shouldn't be needed at all; removed.

-  if (!CommandLine::ForCurrentProcess()->HasSwitch(
-           chromeos::switches::kEnableStubPortalledWifi)) {
-    return false;
-  }
-  return path == kStubPortalledWifiPath;
+  std::string state =
+      DBusThreadManager::Get()->GetShillInitialState(shill::kTypeWifi);
+  return state == shill::kStatePortal && path == kStubPortalledWifiPath;
 }
 
 void SetupDefaultEnvironment() {

[pneubeck (no reviews), 2014/02/27 09:04:37]

please pass FakeDBusThreadManager as an argument

[stevenjb, 2014/02/28 02:44:06]

I ended up eliminating this class entirely. After eliminating some cruft, it
really just belongs in FakeShillManagerClient.

+  DBusThreadManager* dbus_manager = DBusThreadManager::Get();
   ShillServiceClient::TestInterface* services =
-      DBusThreadManager::Get()->GetShillServiceClient()->GetTestInterface();
+      dbus_manager->GetShillServiceClient()->GetTestInterface();
   ShillProfileClient::TestInterface* profiles =
-      DBusThreadManager::Get()->GetShillProfileClient()->GetTestInterface();
+      dbus_manager->GetShillProfileClient()->GetTestInterface();
   ShillManagerClient::TestInterface* manager =
-      DBusThreadManager::Get()->GetShillManagerClient()->GetTestInterface();
+      dbus_manager->GetShillManagerClient()->GetTestInterface();
   ShillDeviceClient::TestInterface* devices =
-      DBusThreadManager::Get()->GetShillDeviceClient()->GetTestInterface();
+      dbus_manager->GetShillDeviceClient()->GetTestInterface();
   if (!services || !profiles || !manager | !devices)
     return;
 
-  // Stub Technologies.
-  manager->AddTechnology(shill::kTypeEthernet, true);
-  manager->AddTechnology(shill::kTypeWifi, true);
-  manager->AddTechnology(shill::kTypeCellular, true);
-  manager->AddTechnology(shill::kTypeWimax, true);
-
   profiles->AddProfile(kSharedProfilePath, std::string());
 
-  devices->AddDevice(
-      kDevicePathEthernet, shill::kTypeEthernet, "stub_eth_device1");
-  devices->AddDevice(kDevicePathWifi, shill::kTypeWifi, "stub_wifi_device1");
-
-  devices->AddDevice(
-      kDevicePathCellular, shill::kTypeCellular, "stub_cellular_device1");
-  devices->SetDeviceProperty(kDevicePathCellular,
-                             shill::kCarrierProperty,
-                             base::StringValue(shill::kCarrierSprint));
-
-  devices->AddDevice(kDevicePathWimax, shill::kTypeWimax, "stub_wimax_device1");
-
   const bool add_to_visible = true;
   const bool add_to_watchlist = true;
 
-  // On real devices, service is not added for ethernet if cable is disconneted.
-  if (IsStubNetworkTypeEnabled(shill::kTypeEthernet)) {
+  bool enabled, portaled;
+  std::string state;
+
+  // Ethernet. No service is not added unless connected.
+  state = GetShillInitialState(shill::kTypeEthernet, &enabled, &portaled);
+  if (state == shill::kStateOnline) {
+    manager->AddTechnology(shill::kTypeEthernet, enabled);
+    devices->AddDevice(
+        kDevicePathEthernet, shill::kTypeEthernet, "stub_eth_device1");
     services->AddService("eth1", "eth1",
                          shill::kTypeEthernet,
-                         shill::kStateOnline,
+                         state,
                          add_to_visible, add_to_watchlist);
     profiles->AddService(kSharedProfilePath, "eth1");
   }
 
   // Wifi
+  state = GetShillInitialState(shill::kTypeWifi, &enabled, &portaled);
+  if (state != shill::kStateOffline) {
+    manager->AddTechnology(shill::kTypeWifi, enabled);
+    devices->AddDevice(kDevicePathWifi, shill::kTypeWifi, "stub_wifi_device1");
+
+    services->AddService("wifi1",
+                         "wifi1",
+                         shill::kTypeWifi,
+                         state,
+                         add_to_visible, add_to_watchlist);
+    services->SetServiceProperty("wifi1",
+                                 shill::kSecurityProperty,
+                                 base::StringValue(shill::kSecurityWep));
+    profiles->AddService(kSharedProfilePath, "wifi1");
 
-  services->AddService("wifi1",
-                       "wifi1",
-                       shill::kTypeWifi,
-                       IsStubNetworkTypeEnabled(shill::kTypeWifi) ?
-                           shill::kStateOnline : shill::kStateIdle,
-                       add_to_visible, add_to_watchlist);
-  services->SetServiceProperty("wifi1",
-                               shill::kSecurityProperty,
-                               base::StringValue(shill::kSecurityWep));
-  profiles->AddService(kSharedProfilePath, "wifi1");
-
-  services->AddService("wifi2",
-                       "wifi2_PSK",
-                       shill::kTypeWifi,
-                       shill::kStateIdle,
-                       add_to_visible, add_to_watchlist);
-  services->SetServiceProperty("wifi2",
-                               shill::kSecurityProperty,
-                               base::StringValue(shill::kSecurityPsk));
-  base::FundamentalValue strength_value(80);
-  services->SetServiceProperty(
-      "wifi2", shill::kSignalStrengthProperty, strength_value);
-  profiles->AddService(kSharedProfilePath, "wifi2");
-
-  if (CommandLine::ForCurrentProcess()->HasSwitch(
-          chromeos::switches::kEnableStubPortalledWifi)) {
-    services->AddService(kStubPortalledWifiPath,
-                         kStubPortalledWifiName,
+    services->AddService("wifi2",
+                         "wifi2_PSK",
                          shill::kTypeWifi,
-                         shill::kStatePortal,
+                         shill::kStateIdle,
                          add_to_visible, add_to_watchlist);
-    services->SetServiceProperty(kStubPortalledWifiPath,
+    services->SetServiceProperty("wifi2",
                                  shill::kSecurityProperty,
-                                 base::StringValue(shill::kSecurityNone));
-    services->SetConnectBehavior(kStubPortalledWifiPath,
-                                 base::Bind(&UpdatePortalledWifiState));
-    services->SetServiceProperty(kStubPortalledWifiPath,
-                                 shill::kConnectableProperty,
-                                 base::FundamentalValue(true));
+                                 base::StringValue(shill::kSecurityPsk));
+
+    base::FundamentalValue strength_value(80);
+    services->SetServiceProperty(
+        "wifi2", shill::kSignalStrengthProperty, strength_value);
+    profiles->AddService(kSharedProfilePath, "wifi2");
+
+    if (portaled) {
+      services->AddService(kStubPortalledWifiPath,
+                           kStubPortalledWifiName,
+                           shill::kTypeWifi,
+                           shill::kStatePortal,
+                           add_to_visible, add_to_watchlist);
+      services->SetServiceProperty(kStubPortalledWifiPath,
+                                   shill::kSecurityProperty,
+                                   base::StringValue(shill::kSecurityNone));
+      services->SetConnectBehavior(kStubPortalledWifiPath,
+                                   base::Bind(&UpdatePortalledWifiState));
+      services->SetServiceProperty(kStubPortalledWifiPath,
+                                   shill::kConnectableProperty,
+                                   base::FundamentalValue(true));
+      profiles->AddService(kSharedProfilePath, kStubPortalledWifiPath);
+    }
   }
 
   // Wimax
-
-  services->AddService("wimax1",
-                       "wimax1",
-                       shill::kTypeWimax,
-                       shill::kStateIdle,
-                       add_to_visible, add_to_watchlist);
-  services->SetServiceProperty(
-      "wimax1", shill::kConnectableProperty, base::FundamentalValue(true));
+  state = GetShillInitialState(shill::kTypeWimax, &enabled, &portaled);
+  if (state != shill::kStateOffline) {
+    manager->AddTechnology(shill::kTypeWimax, enabled);
+    devices->AddDevice(
+        kDevicePathWimax, shill::kTypeWimax, "stub_wimax_device1");
+
+    services->AddService("wimax1",
+                         "wimax1",
+                         shill::kTypeWimax,
+                         state,
+                         add_to_visible, add_to_watchlist);
+    services->SetServiceProperty(
+        "wimax1", shill::kConnectableProperty, base::FundamentalValue(true));
+  }
 
   // Cellular
-
-  services->AddService("cellular1",
-                       "cellular1",
-                       shill::kTypeCellular,
-                       shill::kStateIdle,
-                       add_to_visible, add_to_watchlist);
-  base::StringValue technology_value(shill::kNetworkTechnologyGsm);
-  services->SetServiceProperty(
-      "cellular1", shill::kNetworkTechnologyProperty, technology_value);
-  services->SetServiceProperty(
-      "cellular1",
-      shill::kActivationStateProperty,
-      base::StringValue(shill::kActivationStateNotActivated));
-  services->SetServiceProperty("cellular1",
-                               shill::kRoamingStateProperty,
-                               base::StringValue(shill::kRoamingStateHome));
+  state = GetShillInitialState(shill::kTypeCellular, &enabled, &portaled);
+  if (state != shill::kStateOffline) {
+    bool activated = false;
+    if (state == shill::kActivationStateActivated) {
+      activated = true;
+      state = shill::kStateIdle;
+    }
+    manager->AddTechnology(shill::kTypeCellular, enabled);
+    devices->AddDevice(
+        kDevicePathCellular, shill::kTypeCellular, "stub_cellular_device1");
+    devices->SetDeviceProperty(kDevicePathCellular,
+                               shill::kCarrierProperty,
+                               base::StringValue(shill::kCarrierSprint));
+
+    LOG(ERROR) << "Cellular state: " << state;
+    services->AddService("cellular1",
+                         "cellular1",
+                         shill::kTypeCellular,
+                         state,
+                         add_to_visible, add_to_watchlist);
+    base::StringValue technology_value(shill::kNetworkTechnologyGsm);
+    services->SetServiceProperty(
+        "cellular1", shill::kNetworkTechnologyProperty, technology_value);
+
+    if (activated) {
+      services->SetServiceProperty(
+          "cellular1",
+          shill::kActivationStateProperty,
+          base::StringValue(shill::kActivationStateActivated));
+      services->SetServiceProperty("cellular1",
+                                   shill::kConnectableProperty,
+                                   base::FundamentalValue(true));
+    } else {
+      services->SetServiceProperty(
+          "cellular1",
+          shill::kActivationStateProperty,
+          base::StringValue(shill::kActivationStateNotActivated));
+    }
+
+    services->SetServiceProperty("cellular1",
+                                 shill::kRoamingStateProperty,
+                                 base::StringValue(shill::kRoamingStateHome));
+  }
 
   // VPN
 
-  // Set the "Provider" dictionary properties. Note: when setting these in
-  // Shill, "Provider.Type", etc keys are used, but when reading the values
-  // "Provider" . "Type", etc keys are used. Here we are setting the values
-  // that will be read (by the UI, tests, etc).
-  base::DictionaryValue provider_properties;
-  provider_properties.SetString(shill::kTypeProperty, shill::kProviderOpenVpn);
-  provider_properties.SetString(shill::kHostProperty, "vpn_host");
-
-  services->AddService("vpn1",
-                       "vpn1",
-                       shill::kTypeVPN,
-                       IsStubNetworkTypeEnabled(shill::kTypeVPN) ?
-                           shill::kStateOnline : shill::kStateIdle,
-                       add_to_visible, add_to_watchlist);
-  services->SetServiceProperty(
-      "vpn1", shill::kProviderProperty, provider_properties);
-  profiles->AddService(kSharedProfilePath, "vpn1");
-
-  services->AddService("vpn2",
-                       "vpn2",
-                       shill::kTypeVPN,
-                       shill::kStateIdle,
-                       add_to_visible, add_to_watchlist);
-  services->SetServiceProperty(
-      "vpn2", shill::kProviderProperty, provider_properties);
+  state = GetShillInitialState(shill::kTypeVPN, &enabled, &portaled);
+  if (state != shill::kStateOffline) {
+    // Set the "Provider" dictionary properties. Note: when setting these in
+    // Shill, "Provider.Type", etc keys are used, but when reading the values
+    // "Provider" . "Type", etc keys are used. Here we are setting the values
+    // that will be read (by the UI, tests, etc).
+    base::DictionaryValue provider_properties;
+    provider_properties.SetString(shill::kTypeProperty,
+                                  shill::kProviderOpenVpn);
+    provider_properties.SetString(shill::kHostProperty, "vpn_host");
+
+    services->AddService("vpn1",
+                         "vpn1",
+                         shill::kTypeVPN,
+                         state,
+                         add_to_visible, add_to_watchlist);
+    services->SetServiceProperty(
+        "vpn1", shill::kProviderProperty, provider_properties);
+    profiles->AddService(kSharedProfilePath, "vpn1");
+
+    services->AddService("vpn2",
+                         "vpn2",
+                         shill::kTypeVPN,
+                         shill::kStateIdle,
+                         add_to_visible, add_to_watchlist);
+    services->SetServiceProperty(
+        "vpn2", shill::kProviderProperty, provider_properties);
+  }
 
   manager->SortManagerServices();
 }