Deprecate ShillNetworkClient and add GeolocationHandler

chromeos/network/geolocation_handler.cc

+// Copyright (c) 2013 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "chromeos/network/geolocation_handler.h"
+
+#include "base/bind.h"
+#include "base/string_number_conversions.h"
+#include "base/values.h"
+#include "chromeos/dbus/dbus_thread_manager.h"
+#include "chromeos/dbus/shill_manager_client.h"
+#include "third_party/cros_system_api/dbus/service_constants.h"
+
+// Time until geolocation data becomes invalid. This is intentionally long;
+// the geolocation code itself should do periodic polling for more accuracy,

[gauravsh, 2013/01/14 06:19:59]

See my comment in GetWifiAccessPoints(). If the expectation is periodic polling
and not relying on cached information, exposing an asynchronous interface would
be better IMO.

+// even when data is initially available.
+const int64 kGeolocationValidSeconds = 10 * 60;

[gauravsh, 2013/01/14 06:19:59]

Is it by design that the caller can't determine how recent the returned
information is? (except that it's newer than 600 seconds) 

+
+namespace chromeos {
+
+static GeolocationHandler* g_geolocation_handler = NULL;
+
+GeolocationHandler::GeolocationHandler()
+    : weak_ptr_factory_(ALLOW_THIS_IN_INITIALIZER_LIST(this)) {
+}
+
+GeolocationHandler::~GeolocationHandler() {
+}
+
+// static
+void GeolocationHandler::Initialize() {
+  CHECK(!g_geolocation_handler);
+  g_geolocation_handler = new GeolocationHandler();
+}
+
+// static
+void GeolocationHandler::Shutdown() {
+  CHECK(g_geolocation_handler);
+  delete g_geolocation_handler;
+  g_geolocation_handler = NULL;
+}
+
+// static
+GeolocationHandler* GeolocationHandler::Get() {
+  CHECK(g_geolocation_handler)
+      << "GeolocationHandler::Get() called before Initialize()";
+  return g_geolocation_handler;
+}
+
+bool GeolocationHandler::GetWifiAccessPoints(
+    WifiAccessPointVector* access_points) {
+  // Always request updated access points.
+  DBusThreadManager::Get()->GetShillManagerClient()->GetNetworksForGeolocation(

[gauravsh, 2013/01/14 06:19:59]

What's the usual usage pattern for geolocation information requests? Is it more
of "give me location so I can serve the right content" or "i need regular
updates"?

I ask because I am trying to understand the utility of providing cached
information if we are going to make a request to Shill anyway. Why not just
expose an async interface?

+      base::Bind(&GeolocationHandler::GeolocationCallback,
+                 weak_ptr_factory_.GetWeakPtr()));
+  // Return latest access point data if valid.
+  base::TimeDelta dtime = base::Time::Now() - geolocation_received_time_;
+  if (dtime.InSeconds() < kGeolocationValidSeconds) {
+    *access_points = wifi_access_points_;
+    return true;
+  }
+  return false;
+}
+
+void GeolocationHandler::GeolocationCallback(
+    DBusMethodCallStatus call_status,
+    const base::DictionaryValue& properties) {
+  if (call_status != DBUS_METHOD_CALL_SUCCESS) {
+    LOG(ERROR) << "Failed to get Geolocation data: " << call_status;
+    return;
+  }
+  wifi_access_points_.clear();
+  if (properties.empty())
+    return;  // No enabled devices, don't update received time.
+
+  // Dictionary<device_type, entry_list>
+  for (base::DictionaryValue::Iterator iter(properties);
+       iter.HasNext(); iter.Advance()) {
+    const base::ListValue* entry_list = NULL;
+    if (!iter.value().GetAsList(&entry_list)) {
+      LOG(WARNING) << "Geolocation dictionary value not a List: " << iter.key();
+      continue;
+    }
+    // List[Dictionary<key, value_str>]
+    for (size_t i = 0; i< entry_list->GetSize(); ++i) {

[gauravsh, 2013/01/14 06:19:59]

NIT: i <

+      const base::DictionaryValue* entry = NULL;
+      if (!entry_list->GetDictionary(i, &entry) || !entry) {
+        LOG(WARNING) << "Geolocation list value not a Dictionary: " << i;
+        continue;
+      }
+      // Docs: developers.google.com/maps/documentation/business/geolocation
+      WifiAccessPoint wap;
+      entry->GetString(shill::kGeoMacAddressProperty, &wap.mac_address);
+      std::string age_str;
+      if (entry->GetString(shill::kGeoAgeProperty, &age_str)) {
+        int64 age_ms;
+        if (base::StringToInt64(age_str, &age_ms)) {
+          wap.timestamp =
+              base::Time::Now() - base::TimeDelta::FromMilliseconds(age_ms);
+        }
+      }
+      std::string strength_str;
+      if (entry->GetString(shill::kGeoSignalStrengthProperty, &strength_str))
+        base::StringToInt(strength_str, &wap.signal_strength);
+      std::string signal_str;
+      if (entry->GetString(shill::kGeoSignalToNoiseRatioProperty, &signal_str))
+        base::StringToInt(signal_str, &wap.signal_to_noise);
+      std::string channel_str;
+      if (entry->GetString(shill::kGeoChannelProperty, &channel_str))
+        base::StringToInt(channel_str, &wap.channel);
+      wifi_access_points_.push_back(wap);
+    }
+  }
+  geolocation_received_time_ = base::Time::Now();
+}
+
+}  // namespace chromeos