Merge lp:~thomas-voss/location-service/fix-1394204 into lp:location-service/trunk

[Thomas Voß]

Fix #1394204 by:

(1.) Relying on the cached devices for querying device properties. This is sufficient, as the cache is always consistent in this particular scenario.
(2.) Not explicitly reacting to modem state changes coming in via Ofono but instead only reacting to changes to the PrimaryConnection property of NetworkManager.

Fix #1394204 by:

(1.) Relying on the cached devices for querying device properties. This is sufficient, as the cache is always consistent in this particular scenario.
(2.) Not explicitly reacting to modem state changes coming in via Ofono but instead only reacting to changes to the PrimaryConnection property of NetworkManager.

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:154
http://jenkins.qa.ubuntu.com/job/location-service-ci/338/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/location-service-vivid-amd64-ci/10/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/location-service-vivid-armhf-ci/10
        deb: http://jenkins.qa.ubuntu.com/job/location-service-vivid-armhf-ci/10/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/location-service-vivid-i386-ci/10

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/location-service-ci/338/rebuild

review: Needs Fixing (continuous-integration)

[Loïc Minier (lool)]

This looks good to me, albeit I didn't finish reviewing the various code pathes; I did test binaries and it worked fine for me (but I wasn't experiencing the dbus load)

review: Approve

=== modified file 'src/location_service/com/ubuntu/location/connectivity/ofono.h'
--- src/location_service/com/ubuntu/location/connectivity/ofono.h	2014-10-09 13:02:08 +0000
+++ src/location_service/com/ubuntu/location/connectivity/ofono.h	2015-01-06 12:37:51 +0000
@@ -612,10 +612,13 @@
             }
 
             template<typename Property>
-            typename Property::ValueType get() const
-            {                
-                if (not refresh_properties())
-                    return typename Property::ValueType{};
+            typename Property::ValueType get(bool refresh = true) const
+            {
+                if (refresh)
+                {
+                    if (not refresh_properties())
+                        return typename Property::ValueType{};
+                }
 
                 auto it = properties.find(Property::name());
 

=== modified file 'src/location_service/com/ubuntu/location/connectivity/ofono_nm_connectivity_manager.cpp'
--- src/location_service/com/ubuntu/location/connectivity/ofono_nm_connectivity_manager.cpp	2014-11-10 20:12:56 +0000
+++ src/location_service/com/ubuntu/location/connectivity/ofono_nm_connectivity_manager.cpp	2015-01-06 12:37:51 +0000
@@ -286,24 +286,6 @@
             signals.connected_cell_removed(sp);
     });
 
-    // And we react to changes to the roaming state.
-    cell_result.first->second->is_roaming().changed().connect([this](bool roaming)
-    {
-        VLOG(10) << "Roaming state of cell changed: " << std::boolalpha << roaming << std::endl;
-
-        // Unblocking the bus here and scheduling re-evaluation of the
-        // active connection characteristics.
-        dispatcher.service.post([this]()
-        {
-            // Bail out if the network manager instance went away.
-            if (not network_manager)
-                return;
-
-            active_connection_characteristics.set(
-                        characteristics_for_connection(
-                            network_manager->properties.primary_connection->get()));
-        });
-    });
     // Cool, we have reached here, updated all our caches and created a connected radio cell.
     // We are thus good to go and release the lock manually prior to announcing the new cell
     // to interested parties.
@@ -652,17 +634,12 @@
         ac.enumerate_devices([this, &characteristics](const core::dbus::types::ObjectPath& path)
         {
             xdg::NetworkManager::Device::Type type{xdg::NetworkManager::Device::Type::unknown};
-
             {
+                // We only consider cached devices and do not reach out to enumerate all of the devices
+                // to prevent from excessive dbus roundtrips.
                 std::lock_guard<std::mutex> lg{cached.guard};
                 if (cached.wireless_devices.count(path) > 0)
-                    type = cached.wireless_devices.at(path).type();
-                else
-                    type = xdg::NetworkManager::Device
-                    {
-                        network_manager->service,
-                        network_manager->service->object_for_path(path)
-                    }.type();
+                    type = cached.wireless_devices.at(path).type();                
             }
 
             // We interpret a primary connection over a modem device as
@@ -681,7 +658,7 @@
                     // This call might throw as it reaches out to ofono to query the current status of the modem.
                     try
                     {
-                        auto status = pair.second.network_registration.get<org::Ofono::Manager::Modem::NetworkRegistration::Status>();
+                        auto status = pair.second.network_registration.get<org::Ofono::Manager::Modem::NetworkRegistration::Status>(false);
                         if (org::Ofono::Manager::Modem::NetworkRegistration::Status::roaming == status)
                             characteristics = characteristics | connectivity::Characteristics::connection_is_roaming;
                     }