Deprecate ShillNetworkClient and add GeolocationHandler

Deprecate ShillNetworkClient and add GeolocationHandler

Shill deprecated Manager.Network which was being used to get wifi access point data.
This eliminates the dead code and adds GeolocationHandler which currently queries Shill.Manager for geolocation data, and caches and returns the result.

BUG=167987
For chrome/browser/geolocation:
TBR=joth@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=176858

[Greg Spencer (Chromium), 2013-01-12 00:52:36]

https://codereview.chromium.org/11887008/diff/1/chrome/browser/geolocation/wi...
File chrome/browser/geolocation/wifi_data_provider_chromeos.cc (right):

https://codereview.chromium.org/11887008/diff/1/chrome/browser/geolocation/wi...
chrome/browser/geolocation/wifi_data_provider_chromeos.cc:79: // TODO(stevenjb):
Re-implement this with ne network handlers.
ne->new

Also, what ultimately depends on this info? And if this is still broken, won't
there still be problems (and so should have a bug associated with it)?

https://codereview.chromium.org/11887008/diff/1/chromeos/network/geolocation_...
File chromeos/network/geolocation_handler.cc (right):

https://codereview.chromium.org/11887008/diff/1/chromeos/network/geolocation_...
chromeos/network/geolocation_handler.cc:1: // Copyright (c) 2012 The Chromium
Authors. All rights reserved.
2013

https://codereview.chromium.org/11887008/diff/1/chromeos/network/geolocation_...
chromeos/network/geolocation_handler.cc:80: LOG(WARNING) << "Geolocation
dictionary value not a List: " << iter.key();
Shouldn't this be a DCHECK and log an error?  Afterall, if we get a type that
we're not expecting here, that's kind of a programming issue.  Same below.

https://codereview.chromium.org/11887008/diff/1/chromeos/network/geolocation_...
File chromeos/network/geolocation_handler.h (right):

https://codereview.chromium.org/11887008/diff/1/chromeos/network/geolocation_...
chromeos/network/geolocation_handler.h:1: // Copyright (c) 2012 The Chromium
Authors. All rights reserved.
It's 2013...

https://codereview.chromium.org/11887008/diff/1/chromeos/network/geolocation_...
File chromeos/network/geolocation_handler_unittest.cc (right):

https://codereview.chromium.org/11887008/diff/1/chromeos/network/geolocation_...
chromeos/network/geolocation_handler_unittest.cc:1: // Copyright (c) 2012 The
Chromium Authors. All rights reserved.
2013

[stevenjb, 2013-01-12 01:55:59]

https://codereview.chromium.org/11887008/diff/1/chrome/browser/geolocation/wi...
File chrome/browser/geolocation/wifi_data_provider_chromeos.cc (right):

https://codereview.chromium.org/11887008/diff/1/chrome/browser/geolocation/wi...
chrome/browser/geolocation/wifi_data_provider_chromeos.cc:79: // TODO(stevenjb):
Re-implement this with ne network handlers.
On 2013/01/12 00:52:37, Greg Spencer (Chromium) wrote:
> ne->new
> 
> Also, what ultimately depends on this info? And if this is still broken, won't
> there still be problems (and so should have a bug associated with it)?

This is part 1/2 of the fix. I added the issue to the comment.

https://codereview.chromium.org/11887008/diff/1/chromeos/network/geolocation_...
File chromeos/network/geolocation_handler.cc (right):

https://codereview.chromium.org/11887008/diff/1/chromeos/network/geolocation_...
chromeos/network/geolocation_handler.cc:1: // Copyright (c) 2012 The Chromium
Authors. All rights reserved.
On 2013/01/12 00:52:37, Greg Spencer (Chromium) wrote:
> 2013

Done.

https://codereview.chromium.org/11887008/diff/1/chromeos/network/geolocation_...
File chromeos/network/geolocation_handler.h (right):

https://codereview.chromium.org/11887008/diff/1/chromeos/network/geolocation_...
chromeos/network/geolocation_handler.h:1: // Copyright (c) 2012 The Chromium
Authors. All rights reserved.
On 2013/01/12 00:52:37, Greg Spencer (Chromium) wrote:
> It's 2013...
Meh. Done.

https://codereview.chromium.org/11887008/diff/1/chromeos/network/geolocation_...
File chromeos/network/geolocation_handler_unittest.cc (right):

https://codereview.chromium.org/11887008/diff/1/chromeos/network/geolocation_...
chromeos/network/geolocation_handler_unittest.cc:1: // Copyright (c) 2012 The
Chromium Authors. All rights reserved.
On 2013/01/12 00:52:37, Greg Spencer (Chromium) wrote:
> 2013

Done.

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

questions about usage. looks good otherwise.

https://codereview.chromium.org/11887008/diff/5002/chromeos/dbus/shill_manage...
File chromeos/dbus/shill_manager_client.cc (right):

https://codereview.chromium.org/11887008/diff/5002/chromeos/dbus/shill_manage...
chromeos/dbus/shill_manager_client.cc:507: // Dictionary of name -> value
redundant comment

https://codereview.chromium.org/11887008/diff/5002/chromeos/network/geolocati...
File chromeos/network/geolocation_handler.cc (right):

https://codereview.chromium.org/11887008/diff/5002/chromeos/network/geolocati...
chromeos/network/geolocation_handler.cc:15: // the geolocation code itself
should do periodic polling for more accuracy,
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.

https://codereview.chromium.org/11887008/diff/5002/chromeos/network/geolocati...
chromeos/network/geolocation_handler.cc:17: const int64 kGeolocationValidSeconds
= 10 * 60;
Is it by design that the caller can't determine how recent the returned
information is? (except that it's newer than 600 seconds)

https://codereview.chromium.org/11887008/diff/5002/chromeos/network/geolocati...
chromeos/network/geolocation_handler.cc:53:
DBusThreadManager::Get()->GetShillManagerClient()->GetNetworksForGeolocation(
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?

https://codereview.chromium.org/11887008/diff/5002/chromeos/network/geolocati...
chromeos/network/geolocation_handler.cc:85: for (size_t i = 0; i<
entry_list->GetSize(); ++i) {
NIT: i <

[stevenjb, 2013-01-14 19:38:48]

I agree that the API is not ideal. It was written to match the current polling
based usage.

The feedback was helpful, and I thought about the implementation a bit. I
realized that using age in two places was a bad idea. So I:
* Documented the polling usage in the header
* Removed the age check from GeolocationHandler
* Modified the API to include age so that we can use the age data in the
implementation instead

I will also update the follow-up CL (which includes the usage implementation)
shortly: https://codereview.chromium.org/11881011/

https://codereview.chromium.org/11887008/diff/5002/chromeos/dbus/shill_manage...
File chromeos/dbus/shill_manager_client.cc (right):

https://codereview.chromium.org/11887008/diff/5002/chromeos/dbus/shill_manage...
chromeos/dbus/shill_manager_client.cc:507: // Dictionary of name -> value
On 2013/01/14 06:19:59, gauravsh wrote:
> redundant comment
I added the comment because it's not actually obvious what the Value is (i.e. it
is a fundamental value, not a list or dictionary). I will clarify the comment.

[Greg Spencer (Chromium), 2013-01-14 19:43:22]

LGTM

[commit-bot: I haz the power, 2013-01-14 20:16:40]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/stevenjb@chromium.org/11887008/2007

[commit-bot: I haz the power, 2013-01-14 20:16:57]

Presubmit check for 11887008-2007 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    chrome/browser/geolocation

Presubmit checks took 3.8s to calculate.

[commit-bot: I haz the power, 2013-01-14 21:04:42]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/stevenjb@chromium.org/11887008/2007

[commit-bot: I haz the power, 2013-01-14 21:58:23]

Sorry for I got bad news for ya.
Compile failed with a clobber build on linux_chromeos.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-01-14 21:59:32]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/stevenjb@chromium.org/11887008/6003

[gauravsh, 2013-01-15 03:19:00]

lgtm

https://codereview.chromium.org/11887008/diff/6003/chromeos/network/geolocati...
File chromeos/network/geolocation_handler.h (right):

https://codereview.chromium.org/11887008/diff/6003/chromeos/network/geolocati...
chromeos/network/geolocation_handler.h:23: // A typical usage pattern, assuming
a wifi device is enabeld, is:
enabled

https://codereview.chromium.org/11887008/diff/6003/chromeos/network/geolocati...
chromeos/network/geolocation_handler.h:71: // Wimax enabled state
Wifi

[commit-bot: I haz the power, 2013-01-15 08:51:19]

Change committed as 176858