Replace misc. network stub flags with more flexible ones

chromeos/dbus/fake_shill_manager_client.cc

Index: chromeos/dbus/fake_shill_manager_client.cc
diff --git a/chromeos/dbus/fake_shill_manager_client.cc b/chromeos/dbus/fake_shill_manager_client.cc
index d3ad56641aa07e74879092b1a6ad95559420655a..19a1fac2a66e6acf77c29584d9546c959b537889 100644
--- a/chromeos/dbus/fake_shill_manager_client.cc
+++ b/chromeos/dbus/fake_shill_manager_client.cc
@@ -5,10 +5,12 @@
 #include "chromeos/dbus/fake_shill_manager_client.h"
 
 #include "base/bind.h"
-#include "base/command_line.h"
 #include "base/message_loop/message_loop.h"
+#include "base/strings/string_number_conversions.h"
+#include "base/strings/string_util.h"
 #include "base/values.h"
 #include "chromeos/chromeos_switches.h"
+#include "chromeos/dbus/dbus_command_line_helper.h"
 #include "chromeos/dbus/dbus_thread_manager.h"
 #include "chromeos/dbus/shill_device_client.h"
 #include "chromeos/dbus/shill_profile_client.h"
@@ -73,10 +75,19 @@ void AppendServicesForType(
   }
 }
 
+void UpdatePortalledWifiState(const std::string& service_path) {
+  DBusThreadManager::Get()->GetShillServiceClient()->GetTestInterface()
+      ->SetServiceProperty(service_path,
+                           shill::kStateProperty,
+                           base::StringValue(shill::kStatePortal));
+}
+
 }  // namespace
 
 FakeShillManagerClient::FakeShillManagerClient()
-    : weak_ptr_factory_(this) {
+    : interactive_delay_(0),
+      weak_ptr_factory_(this) {
+  ParseCommandLineSwitch();
 }
 
 FakeShillManagerClient::~FakeShillManagerClient() {}
@@ -118,7 +129,7 @@ void FakeShillManagerClient::SetProperty(const std::string& name,
                                          const base::Closure& callback,
                                          const ErrorCallback& error_callback) {
   stub_properties_.SetWithoutPathExpansion(name, value.DeepCopy());
-  CallNotifyObserversPropertyChanged(name, 0);
+  CallNotifyObserversPropertyChanged(name);
   base::MessageLoop::current()->PostTask(FROM_HERE, callback);
 }
 
@@ -133,21 +144,16 @@ void FakeShillManagerClient::RequestScan(const std::string& type,
       DBusThreadManager::Get()->GetShillDeviceClient()->GetTestInterface();
   std::string device_path = device_client->GetDevicePathForType(device_type);
   if (!device_path.empty()) {
-    device_client->SetDeviceProperty(device_path,
-                                     shill::kScanningProperty,
-                                     base::FundamentalValue(true));
-  }
-  const int kScanDurationSeconds = 3;
-  int scan_duration_seconds = kScanDurationSeconds;
-  if (!CommandLine::ForCurrentProcess()->HasSwitch(
-      chromeos::switches::kEnableStubInteractive)) {
-    scan_duration_seconds = 0;
+    device_client->SetDeviceProperty(
+        device_path, shill::kScanningProperty, base::FundamentalValue(true));
   }
   base::MessageLoop::current()->PostDelayedTask(
       FROM_HERE,
       base::Bind(&FakeShillManagerClient::ScanCompleted,
-                 weak_ptr_factory_.GetWeakPtr(), device_path, callback),
-      base::TimeDelta::FromSeconds(scan_duration_seconds));
+                 weak_ptr_factory_.GetWeakPtr(),
+                 device_path,
+                 callback),
+      base::TimeDelta::FromSeconds(interactive_delay_));
 }
 
 void FakeShillManagerClient::EnableTechnology(
@@ -156,21 +162,22 @@ void FakeShillManagerClient::EnableTechnology(
     const ErrorCallback& error_callback) {
   base::ListValue* enabled_list = NULL;
   if (!stub_properties_.GetListWithoutPathExpansion(
-      shill::kEnabledTechnologiesProperty, &enabled_list)) {
+          shill::kAvailableTechnologiesProperty, &enabled_list)) {
     base::MessageLoop::current()->PostTask(FROM_HERE, callback);
     base::MessageLoop::current()->PostTask(
         FROM_HERE,
         base::Bind(error_callback, "StubError", "Property not found"));
     return;
   }
-  if (CommandLine::ForCurrentProcess()->HasSwitch(
-          chromeos::switches::kEnableStubInteractive)) {
-    const int kEnableTechnologyDelaySeconds = 3;
+  if (interactive_delay_) {

[pneubeck (no reviews), 2014/03/03 20:36:19]

actually, why not PostDelayedTask(..., 0) and thereby ensuring that the same
control-flow is tested? (if so, better change it in a separate patch)

[stevenjb, 2014/03/05 01:03:50]

Good point, and I think that's pretty low risk; if the unit tests don't break
I'll go ahead and change that.

     base::MessageLoop::current()->PostDelayedTask(
         FROM_HERE,
         base::Bind(&FakeShillManagerClient::SetTechnologyEnabled,
-                   weak_ptr_factory_.GetWeakPtr(), type, callback, true),
-        base::TimeDelta::FromSeconds(kEnableTechnologyDelaySeconds));
+                   weak_ptr_factory_.GetWeakPtr(),
+                   type,
+                   callback,
+                   true),
+        base::TimeDelta::FromSeconds(interactive_delay_));
   } else {
     SetTechnologyEnabled(type, callback, true);
   }
@@ -182,20 +189,21 @@ void FakeShillManagerClient::DisableTechnology(
     const ErrorCallback& error_callback) {
   base::ListValue* enabled_list = NULL;
   if (!stub_properties_.GetListWithoutPathExpansion(
-      shill::kEnabledTechnologiesProperty, &enabled_list)) {
+          shill::kAvailableTechnologiesProperty, &enabled_list)) {
     base::MessageLoop::current()->PostTask(
         FROM_HERE,
         base::Bind(error_callback, "StubError", "Property not found"));
     return;
   }
-  if (CommandLine::ForCurrentProcess()->HasSwitch(
-          chromeos::switches::kEnableStubInteractive)) {
-    const int kDisableTechnologyDelaySeconds = 3;
+  if (interactive_delay_) {
     base::MessageLoop::current()->PostDelayedTask(
         FROM_HERE,
         base::Bind(&FakeShillManagerClient::SetTechnologyEnabled,
-                   weak_ptr_factory_.GetWeakPtr(), type, callback, false),
-        base::TimeDelta::FromSeconds(kDisableTechnologyDelaySeconds));
+                   weak_ptr_factory_.GetWeakPtr(),
+                   type,
+                   callback,
+                   false),
+        base::TimeDelta::FromSeconds(interactive_delay_));
   } else {
     SetTechnologyEnabled(type, callback, false);
   }
@@ -306,8 +314,8 @@ void FakeShillManagerClient::VerifyAndEncryptData(
     const std::string& data,
     const StringCallback& callback,
     const ErrorCallback& error_callback) {
-  base::MessageLoop::current()->PostTask(FROM_HERE,
-                                   base::Bind(callback, "encrypted_data"));
+  base::MessageLoop::current()->PostTask(
+      FROM_HERE, base::Bind(callback, "encrypted_data"));
 }
 
 void FakeShillManagerClient::ConnectToBestServices(
@@ -322,71 +330,87 @@ ShillManagerClient::TestInterface* FakeShillManagerClient::GetTestInterface() {
 // ShillManagerClient::TestInterface overrides.
 
 void FakeShillManagerClient::AddDevice(const std::string& device_path) {
-  if (GetListProperty(shill::kDevicesProperty)->AppendIfNotPresent(
-      base::Value::CreateStringValue(device_path))) {
-    CallNotifyObserversPropertyChanged(shill::kDevicesProperty, 0);
+  if (GetListProperty(shill::kDevicesProperty)
+          ->AppendIfNotPresent(base::Value::CreateStringValue(device_path))) {
+    CallNotifyObserversPropertyChanged(shill::kDevicesProperty);
   }
 }
 
 void FakeShillManagerClient::RemoveDevice(const std::string& device_path) {
   base::StringValue device_path_value(device_path);
-  if (GetListProperty(shill::kDevicesProperty)->Remove(
-      device_path_value, NULL)) {
-    CallNotifyObserversPropertyChanged(shill::kDevicesProperty, 0);
+  if (GetListProperty(shill::kDevicesProperty)
+          ->Remove(device_path_value, NULL)) {
+    CallNotifyObserversPropertyChanged(shill::kDevicesProperty);
   }
 }
 
 void FakeShillManagerClient::ClearDevices() {
   GetListProperty(shill::kDevicesProperty)->Clear();
-  CallNotifyObserversPropertyChanged(shill::kDevicesProperty, 0);
+  CallNotifyObserversPropertyChanged(shill::kDevicesProperty);
 }
 
 void FakeShillManagerClient::AddTechnology(const std::string& type,
                                            bool enabled) {
-  if (GetListProperty(shill::kAvailableTechnologiesProperty)->
-      AppendIfNotPresent(base::Value::CreateStringValue(type))) {
-    CallNotifyObserversPropertyChanged(
-        shill::kAvailableTechnologiesProperty, 0);
+  if (GetListProperty(shill::kAvailableTechnologiesProperty)
+          ->AppendIfNotPresent(base::Value::CreateStringValue(type))) {
+    CallNotifyObserversPropertyChanged(shill::kAvailableTechnologiesProperty);
   }
   if (enabled &&
-      GetListProperty(shill::kEnabledTechnologiesProperty)->
-      AppendIfNotPresent(base::Value::CreateStringValue(type))) {
-    CallNotifyObserversPropertyChanged(
-        shill::kEnabledTechnologiesProperty, 0);
+      GetListProperty(shill::kEnabledTechnologiesProperty)
+          ->AppendIfNotPresent(base::Value::CreateStringValue(type))) {
+    CallNotifyObserversPropertyChanged(shill::kEnabledTechnologiesProperty);
   }
 }
 
 void FakeShillManagerClient::RemoveTechnology(const std::string& type) {
   base::StringValue type_value(type);
-  if (GetListProperty(shill::kAvailableTechnologiesProperty)->Remove(
-      type_value, NULL)) {
-    CallNotifyObserversPropertyChanged(
-        shill::kAvailableTechnologiesProperty, 0);
+  if (GetListProperty(shill::kAvailableTechnologiesProperty)
+          ->Remove(type_value, NULL)) {
+    CallNotifyObserversPropertyChanged(shill::kAvailableTechnologiesProperty);
   }
-  if (GetListProperty(shill::kEnabledTechnologiesProperty)->Remove(
-      type_value, NULL)) {
-    CallNotifyObserversPropertyChanged(
-        shill::kEnabledTechnologiesProperty, 0);
+  if (GetListProperty(shill::kEnabledTechnologiesProperty)
+          ->Remove(type_value, NULL)) {
+    CallNotifyObserversPropertyChanged(shill::kEnabledTechnologiesProperty);
   }
 }
 
 void FakeShillManagerClient::SetTechnologyInitializing(const std::string& type,
                                                        bool initializing) {
   if (initializing) {
-    if (GetListProperty(shill::kUninitializedTechnologiesProperty)->
-        AppendIfNotPresent(base::Value::CreateStringValue(type))) {
+    if (GetListProperty(shill::kUninitializedTechnologiesProperty)
+            ->AppendIfNotPresent(base::Value::CreateStringValue(type))) {
       CallNotifyObserversPropertyChanged(
-          shill::kUninitializedTechnologiesProperty, 0);
+          shill::kUninitializedTechnologiesProperty);
     }
   } else {
-    if (GetListProperty(shill::kUninitializedTechnologiesProperty)->Remove(
-            base::StringValue(type), NULL)) {
+    if (GetListProperty(shill::kUninitializedTechnologiesProperty)
+            ->Remove(base::StringValue(type), NULL)) {
       CallNotifyObserversPropertyChanged(
-          shill::kUninitializedTechnologiesProperty, 0);
+          shill::kUninitializedTechnologiesProperty);
     }
   }
 }
 
+void FakeShillManagerClient::AddGeoNetwork(
+    const std::string& technology,
+    const base::DictionaryValue& network) {
+  base::ListValue* list_value = NULL;
+  if (!stub_geo_networks_.GetListWithoutPathExpansion(technology,
+                                                      &list_value)) {
+    list_value = new base::ListValue;
+    stub_geo_networks_.SetWithoutPathExpansion(technology, list_value);
+  }
+  list_value->Append(network.DeepCopy());
+}
+
+void FakeShillManagerClient::AddProfile(const std::string& profile_path) {
+  const char* key = shill::kProfilesProperty;
+  if (GetListProperty(key)
+          ->AppendIfNotPresent(new base::StringValue(profile_path))) {
+    CallNotifyObserversPropertyChanged(key);
+  }
+}
+
 void FakeShillManagerClient::ClearProperties() {
   stub_properties_.Clear();
 }
@@ -399,9 +423,9 @@ void FakeShillManagerClient::AddManagerService(const std::string& service_path,
       base::Value::CreateStringValue(service_path));
   // If visible, add to Services and notify if new.
   if (add_to_visible_list &&
-      GetListProperty(shill::kServicesProperty)->AppendIfNotPresent(
-      base::Value::CreateStringValue(service_path))) {
-    CallNotifyObserversPropertyChanged(shill::kServicesProperty, 0);
+      GetListProperty(shill::kServicesProperty)
+          ->AppendIfNotPresent(base::Value::CreateStringValue(service_path))) {
+    CallNotifyObserversPropertyChanged(shill::kServicesProperty);
   }
   if (add_to_watch_list)
     AddServiceToWatchList(service_path);
@@ -410,16 +434,15 @@ void FakeShillManagerClient::AddManagerService(const std::string& service_path,
 void FakeShillManagerClient::RemoveManagerService(
     const std::string& service_path) {
   base::StringValue service_path_value(service_path);
-  if (GetListProperty(shill::kServicesProperty)->Remove(
-      service_path_value, NULL)) {
-    CallNotifyObserversPropertyChanged(shill::kServicesProperty, 0);
+  if (GetListProperty(shill::kServicesProperty)
+          ->Remove(service_path_value, NULL)) {
+    CallNotifyObserversPropertyChanged(shill::kServicesProperty);
   }
-  GetListProperty(shill::kServiceCompleteListProperty)->Remove(
-      service_path_value, NULL);
-  if (GetListProperty(shill::kServiceWatchListProperty)->Remove(
-      service_path_value, NULL)) {
-    CallNotifyObserversPropertyChanged(
-        shill::kServiceWatchListProperty, 0);
+  GetListProperty(shill::kServiceCompleteListProperty)
+      ->Remove(service_path_value, NULL);
+  if (GetListProperty(shill::kServiceWatchListProperty)
+          ->Remove(service_path_value, NULL)) {
+    CallNotifyObserversPropertyChanged(shill::kServiceWatchListProperty);
   }
 }
 
@@ -427,8 +450,8 @@ void FakeShillManagerClient::ClearManagerServices() {
   GetListProperty(shill::kServicesProperty)->Clear();
   GetListProperty(shill::kServiceCompleteListProperty)->Clear();
   GetListProperty(shill::kServiceWatchListProperty)->Clear();
-  CallNotifyObserversPropertyChanged(shill::kServicesProperty, 0);
-  CallNotifyObserversPropertyChanged(shill::kServiceWatchListProperty, 0);
+  CallNotifyObserversPropertyChanged(shill::kServicesProperty);
+  CallNotifyObserversPropertyChanged(shill::kServiceWatchListProperty);
 }
 
 void FakeShillManagerClient::SortManagerServices() {
@@ -454,38 +477,209 @@ void FakeShillManagerClient::SortManagerServices() {
   for (size_t i = 0; i < inactive_services.size(); ++i)
     service_list->AppendString(inactive_services[i]);
 
-  CallNotifyObserversPropertyChanged(shill::kServicesProperty, 0);
+  CallNotifyObserversPropertyChanged(shill::kServicesProperty);
 }
 
-void FakeShillManagerClient::AddGeoNetwork(
-    const std::string& technology,
-    const base::DictionaryValue& network) {
-  base::ListValue* list_value = NULL;
-  if (!stub_geo_networks_.GetListWithoutPathExpansion(
-      technology, &list_value)) {
-    list_value = new base::ListValue;
-    stub_geo_networks_.SetWithoutPathExpansion(technology, list_value);
-  }
-  list_value->Append(network.DeepCopy());
+
+int FakeShillManagerClient::GetInteractiveDelay() const {
+  return interactive_delay_;
 }
 
-void FakeShillManagerClient::AddProfile(const std::string& profile_path) {
-  const char* key = shill::kProfilesProperty;
-  if (GetListProperty(key)->AppendIfNotPresent(
-          new base::StringValue(profile_path))) {
-    CallNotifyObserversPropertyChanged(key, 0);
+std::string FakeShillManagerClient::GetInitialState(
+    const std::string& type) const {
+  std::map<std::string, std::string>::const_iterator iter =
+      shill_initial_state_map_.find(type);
+  if (iter == shill_initial_state_map_.end())
+    return shill::kStateOffline;  // Not available

[pneubeck (no reviews), 2014/03/03 20:36:19]

this comment isn't not informative. please make it a sentence.

[stevenjb, 2014/03/05 01:03:50]

I made this function private now that the portal detection logic doesn't depend
on it. Changed this to use a local constant kTechnologyUnavailable instead.

+  return iter->second;
+}
+
+void FakeShillManagerClient::SetupDefaultEnvironment() {
+  DBusThreadManager* dbus_manager = DBusThreadManager::Get();
+  ShillServiceClient::TestInterface* services =
+      dbus_manager->GetShillServiceClient()->GetTestInterface();
+  ShillProfileClient::TestInterface* profiles =
+      dbus_manager->GetShillProfileClient()->GetTestInterface();
+  ShillDeviceClient::TestInterface* devices =
+      dbus_manager->GetShillDeviceClient()->GetTestInterface();
+  if (!services || !profiles || !devices)
+    return;
+
+  const std::string shared_profile = ShillProfileClient::GetSharedProfilePath();
+  profiles->AddProfile(shared_profile, std::string());
+
+  const bool add_to_visible = true;
+  const bool add_to_watchlist = true;
+
+  bool enabled, portaled;
+  std::string state;
+
+  // Ethernet. No service is not added unless connected.

[pneubeck (no reviews), 2014/03/03 20:36:19]

'No ... not added unless' ?
Remove 'not'?

[stevenjb, 2014/03/05 01:03:50]

Removed comment, behavior is now consistent across technologies. Comment for
Ethernet is in SetInitialNetworkState.

+  state = ParseInitialState(shill::kTypeEthernet, &enabled, &portaled);
+  if (state == shill::kStateOnline) {
+    AddTechnology(shill::kTypeEthernet, enabled);
+    devices->AddDevice(
+        "/device/eth1", shill::kTypeEthernet, "stub_eth_device1");
+    services->AddService("eth1", "eth1",
+                         shill::kTypeEthernet,
+                         state,
+                         add_to_visible, add_to_watchlist);
+    profiles->AddService(shared_profile, "eth1");
+  }
+
+  // Wifi
+  state = ParseInitialState(shill::kTypeWifi, &enabled, &portaled);
+  if (state != shill::kStateOffline) {
+    AddTechnology(shill::kTypeWifi, enabled);
+    devices->AddDevice("/device/wifi1", 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(shared_profile, "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(shared_profile, "wifi2");
+
+    if (portaled) {
+      const std::string kPortalledWifiPath = "portalled_wifi";
+      services->AddService(kPortalledWifiPath,
+                           "Portalled Wifi",
+                           shill::kTypeWifi,
+                           shill::kStatePortal,
+                           add_to_visible, add_to_watchlist);
+      services->SetServiceProperty(kPortalledWifiPath,
+                                   shill::kSecurityProperty,
+                                   base::StringValue(shill::kSecurityNone));
+      services->SetConnectBehavior(kPortalledWifiPath,
+                                   base::Bind(&UpdatePortalledWifiState,
+                                              "portalled_wifi"));
+      services->SetServiceProperty(kPortalledWifiPath,
+                                   shill::kConnectableProperty,
+                                   base::FundamentalValue(true));
+      profiles->AddService(shared_profile, kPortalledWifiPath);
+    }
+  }
+
+  // Wimax
+  state = ParseInitialState(shill::kTypeWimax, &enabled, &portaled);
+  if (state != shill::kStateOffline) {
+    AddTechnology(shill::kTypeWimax, enabled);
+    devices->AddDevice(
+        "/device/wimax1", 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
+  state = ParseInitialState(shill::kTypeCellular, &enabled, &portaled);
+  if (state != shill::kStateOffline) {
+    bool activated = false;
+    if (state == shill::kActivationStateActivated) {
+      activated = true;
+      state = shill::kStateIdle;
+    }
+    AddTechnology(shill::kTypeCellular, enabled);
+    devices->AddDevice(
+        "/device/cellular1", shill::kTypeCellular, "stub_cellular_device1");
+    devices->SetDeviceProperty("/device/cellular1",
+                               shill::kCarrierProperty,
+                               base::StringValue(shill::kCarrierSprint));
+
+    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
+  state = ParseInitialState(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(shared_profile, "vpn1");
+
+    services->AddService("vpn2",
+                         "vpn2",
+                         shill::kTypeVPN,
+                         shill::kStateIdle,
+                         add_to_visible, add_to_watchlist);
+    services->SetServiceProperty(
+        "vpn2", shill::kProviderProperty, provider_properties);
   }
+
+  SortManagerServices();
 }
 
+// Private methods
+
 void FakeShillManagerClient::AddServiceToWatchList(
     const std::string& service_path) {
   // Remove and insert the service, moving it to the front of the watch list.
-  GetListProperty(shill::kServiceWatchListProperty)->Remove(
-      base::StringValue(service_path), NULL);
-  GetListProperty(shill::kServiceWatchListProperty)->Insert(
-      0, base::Value::CreateStringValue(service_path));
-  CallNotifyObserversPropertyChanged(
-      shill::kServiceWatchListProperty, 0);
+  GetListProperty(shill::kServiceWatchListProperty)
+      ->Remove(base::StringValue(service_path), NULL);
+  GetListProperty(shill::kServiceWatchListProperty)
+      ->Insert(0, base::Value::CreateStringValue(service_path));
+  CallNotifyObserversPropertyChanged(shill::kServiceWatchListProperty);
 }
 
 void FakeShillManagerClient::PassStubProperties(
@@ -508,22 +702,16 @@ void FakeShillManagerClient::PassStubGeoNetworks(
 }
 
 void FakeShillManagerClient::CallNotifyObserversPropertyChanged(
-    const std::string& property,
-    int delay_ms) {
+    const std::string& property) {
   // Avoid unnecessary delayed task if we have no observers (e.g. during
   // initial setup).
   if (!observer_list_.might_have_observers())
     return;
-  if (!CommandLine::ForCurrentProcess()->HasSwitch(
-          chromeos::switches::kEnableStubInteractive)) {
-    delay_ms = 0;
-  }
-  base::MessageLoop::current()->PostDelayedTask(
+  base::MessageLoop::current()->PostTask(
       FROM_HERE,
       base::Bind(&FakeShillManagerClient::NotifyObserversPropertyChanged,
                  weak_ptr_factory_.GetWeakPtr(),
-                 property),
-                 base::TimeDelta::FromMilliseconds(delay_ms));
+                 property));
 }
 
 void FakeShillManagerClient::NotifyObserversPropertyChanged(
@@ -585,20 +773,17 @@ void FakeShillManagerClient::SetTechnologyEnabled(
     const std::string& type,
     const base::Closure& callback,
     bool enabled) {
-  base::ListValue* enabled_list = NULL;
-  stub_properties_.GetListWithoutPathExpansion(
-      shill::kEnabledTechnologiesProperty, &enabled_list);
-  DCHECK(enabled_list);
+  base::ListValue* enabled_list =
+      GetListProperty(shill::kEnabledTechnologiesProperty);
   if (enabled)
     enabled_list->AppendIfNotPresent(new base::StringValue(type));
   else
     enabled_list->Remove(base::StringValue(type), NULL);
-  CallNotifyObserversPropertyChanged(
-      shill::kEnabledTechnologiesProperty, 0 /* already delayed */);
+  CallNotifyObserversPropertyChanged(shill::kEnabledTechnologiesProperty);
   base::MessageLoop::current()->PostTask(FROM_HERE, callback);
   // May affect available services
-  CallNotifyObserversPropertyChanged(shill::kServicesProperty, 0);
-  CallNotifyObserversPropertyChanged(shill::kServiceWatchListProperty, 0);
+  CallNotifyObserversPropertyChanged(shill::kServicesProperty);
+  CallNotifyObserversPropertyChanged(shill::kServiceWatchListProperty);
 }
 
 base::ListValue* FakeShillManagerClient::GetEnabledServiceList(
@@ -638,9 +823,102 @@ void FakeShillManagerClient::ScanCompleted(const std::string& device_path,
                           shill::kScanningProperty,
                           base::FundamentalValue(false));
   }
-  CallNotifyObserversPropertyChanged(shill::kServicesProperty, 0);
-  CallNotifyObserversPropertyChanged(shill::kServiceWatchListProperty, 0);
+  CallNotifyObserversPropertyChanged(shill::kServicesProperty);
+  CallNotifyObserversPropertyChanged(shill::kServiceWatchListProperty);
   base::MessageLoop::current()->PostTask(FROM_HERE, callback);
 }
 
+void FakeShillManagerClient::ParseCommandLineSwitch() {

[pneubeck (no reviews), 2014/03/03 20:36:19]

we need a documentation of the format of this command line argument.

[stevenjb, 2014/03/05 01:03:50]

Documentation is in dbus_command_line_helper plus I added comments to
ParseOptions in the header.

+  if (dbus_command_line_helper::ParseOptions(
+          switches::kShillStub,
+          base::Bind(&FakeShillManagerClient::ParseOption,
+                     base::Unretained(this))))

[pneubeck (no reviews), 2014/03/03 20:36:19]

nit: missing { }

[stevenjb, 2014/03/05 01:03:50]

Done.

+    return;
+  // Default setup
+  SetInitialNetworkState(shill::kTypeEthernet, shill::kStateOnline);
+  SetInitialNetworkState(shill::kTypeWifi, shill::kStateOnline);
+  SetInitialNetworkState(shill::kTypeCellular, shill::kStateIdle);
+  SetInitialNetworkState(shill::kTypeVPN, shill::kStateIdle);
+}
+
+bool FakeShillManagerClient::ParseOption(const std::string& arg0,
+                                         const std::string& arg1) {
+  if (arg0 == "interactive") {
+    int seconds = 3;
+    if (!arg1.empty())
+      base::StringToInt(arg1, &seconds);
+    interactive_delay_ = seconds;
+    return true;
+  }
+  return SetInitialNetworkState(arg0, arg1);
+}
+
+bool FakeShillManagerClient::SetInitialNetworkState(std::string type_arg,
+                                                    std::string state_arg) {

[pneubeck (no reviews), 2014/03/03 20:36:19]

I'd fail on state_arg.empty() instead of doing a default.
Just make the logic as trivial as possible.

[stevenjb, 2014/03/05 01:03:50]

I specifically want an empty state arg to default to enabled to support, e.g.,
'shill_stub=wifi,cellular'
I'll add a comment.

+  std::string state;
+  state_arg = StringToLowerASCII(state_arg);
+  if (state_arg == "0" || state_arg == "off" || state_arg == "inactive" ||

[pneubeck (no reviews), 2014/03/03 20:36:19]

this is only a testing interface for developers/testers. I don't think that we
need synonyms.

same for the other options.

[stevenjb, 2014/03/05 01:03:50]

I feel the opposite. Since it's a testing interface it should be as forgiving as
possible.

+      state_arg == shill::kStateIdle)
+    state = shill::kStateIdle;  // Enabled but not connected
+  else if (state_arg == "disabled" || state_arg == "disconnect")
+    state = shill::kStateDisconnect;  // Diabled but available

[pneubeck (no reviews), 2014/03/03 20:36:19]

typo: Diabled -> Disabled

[stevenjb, 2014/03/05 01:03:50]

Done.

+  else if (state_arg == "none" || state_arg == "offline")
+    state = shill::kStateOffline;  // Not available

[pneubeck (no reviews), 2014/03/03 20:36:19]

this comment doesn't seem very explanatory. maybe make it a sentence (also in
the other cases).

[stevenjb, 2014/03/05 01:03:50]

Improved all comments.

+  else if (state_arg == "portal")
+    state = shill::kStatePortal;  // Connected to portal
+  else if (state_arg == "active" || state_arg == "activated")
+    state = shill::kActivationStateActivated;  // Connected and activated

[pneubeck (no reviews), 2014/03/03 20:36:19]

*teeth grinding*
maybe not such a good idea to assign constants from different "enums" to the
same variable (kState* vs. kActivationState* )

you could put a struct into the initial state map.

if you would use a dictionary argument per network type, then you could put
'active' in to a separate option. then the command line arg would e.g. look
like:
--shill-stub='{"cellular": {"active": True, "state": "idle"}}'

I know, that the syntax is very verbose compared to your solution. but we should
weigh up code simplicity against usability. considering that there are not many
people using this, I would go for code simplicity.

[stevenjb, 2014/03/05 01:03:50]

1. I used local values for special states "Unavailable" and "Activated"
2. I really want to keep the syntax as simple and versatile as possible. If
there is demand at some point for parsing JSON and someone implements it, great,
but I still think that being able to add something like
"--shill_stub=wifi,cellular=disabled" is really useful.

+  else
+    state = shill::kStateOnline;  // Connected
+
+  std::string type;
+  type_arg = StringToLowerASCII(type_arg);
+  if (type_arg == "ethernet" || type_arg == "eth") {
+    if (state == shill::kStateIdle || state == shill::kStateDisconnect)
+      state = shill::kStateOffline;  // No unconnected or disabled ethernet.
+    shill_initial_state_map_[shill::kTypeEthernet] = state;

[pneubeck (no reviews), 2014/03/03 20:36:19]

the if-branches could decide on the shill type string and the map assigments
could be merged into one statement after the if-branches.

[stevenjb, 2014/03/05 01:03:50]

Sure. I thought I might have more synonyms, but I can just check for valid types
and handle the synonyms separately.

+  } else if (type_arg == "wifi") {
+    shill_initial_state_map_[shill::kTypeWifi] = state;
+  } else if (type_arg == "cellular") {
+    shill_initial_state_map_[shill::kTypeCellular] = state;
+  } else if (type_arg == "wireless") {

[pneubeck (no reviews), 2014/03/03 20:36:19]

considering that this is only an interface for testing,
couldn't you just forward the type_arg as a Shill network type? And to provide
some type checking, just verify that type_arg is an element of a list of all
network types?
For "wireless" you could keep the special handling.

[stevenjb, 2014/03/05 01:03:50]

Yes. This was easier/faster, but sure.

+    shill_initial_state_map_[shill::kTypeWifi] = state;
+    shill_initial_state_map_[shill::kTypeCellular] = state;
+  } else if (type_arg == "wimax") {
+    shill_initial_state_map_[shill::kTypeWimax] = state;
+  } else if (type_arg == "vpn") {
+    shill_initial_state_map_[shill::kTypeVPN] = state;
+  } else {
+    LOG(WARNING) << "Unrecognized Shill network type: " << type_arg;
+    return false;
+  }
+  return true;
+}
+
+std::string FakeShillManagerClient::ParseInitialState(const std::string& type,

[pneubeck (no reviews), 2014/03/03 20:36:19]

this is not really about parsing. maybe call it something like
GetInitialStateProperties

[stevenjb, 2014/03/05 01:03:50]

Done.

+                                                      bool* enabled,
+                                                      bool* portaled) {
+  std::string state = GetInitialState(type);
+  if (state == shill::kStateDisconnect) {
+    *enabled = false;
+    *portaled = false;
+    return shill::kStateIdle;
+  }
+  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) {

[pneubeck (no reviews), 2014/03/03 20:36:19]

better make this the default case and let it skip all previous if-branches if
the state is invalid.

[stevenjb, 2014/03/05 01:03:50]

Done.

+    LOG(WARNING) << "Invalid state: " << state << " for " << type;
+    state = shill::kStateIdle;
+  }
+  *enabled = true;
+  *portaled = false;
+  return state;
+}
+
 }  // namespace chromeos