Rework wifi_data_provider_linux.cc with our D-Bus library.

Rework wifi_data_provider_linux.cc with our D-Bus library.

The code is now a lot simpler, and unit tested.
The dlopen hack and the timeout hack are now gone.

BUG=90036
TEST=confirm that the geolocation works on maps.google.com on a device with wifi network

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=98307

[satorux1, 2011-08-24 18:30:08]

It took quite sometime since I proposed to write our own D-Bus library, but
finally we are now ready to use it.

As the first client of the D-Bus library, I've reworked the wifi data provider
for linux, with newly written unit tests. Hope you'll like the new code.

[satorux1, 2011-08-24 20:17:03]

On 2011/08/24 18:30:08, satorux1 wrote:
> It took quite sometime since I proposed to write our own D-Bus library, but
> finally we are now ready to use it.
> 
> As the first client of the D-Bus library, I've reworked the wifi data provider
> for linux, with newly written unit tests. Hope you'll like the new code.

oops, the newly written tests failed on the linux try bot. I wasn't aware of the
DCHECK, as I was testing with Release build. Will fix it shortly.

[satorux1, 2011-08-24 20:48:46]

On 2011/08/24 20:17:03, satorux1 wrote:
> On 2011/08/24 18:30:08, satorux1 wrote:
> > It took quite sometime since I proposed to write our own D-Bus library, but
> > finally we are now ready to use it.
> > 
> > As the first client of the D-Bus library, I've reworked the wifi data
provider
> > for linux, with newly written unit tests. Hope you'll like the new code.
> 
> oops, the newly written tests failed on the linux try bot. I wasn't aware of
the
> DCHECK, as I was testing with Release build. Will fix it shortly.

Fixed the unit tests and uploaded a new patch.

[stevenjb, 2011-08-24 23:32:04]

http://codereview.chromium.org/7725001/diff/1/content/browser/geolocation/wif...
File content/browser/geolocation/wifi_data_provider_linux.cc (right):

http://codereview.chromium.org/7725001/diff/1/content/browser/geolocation/wif...
content/browser/geolocation/wifi_data_provider_linux.cc:316: dbus::Response*
NetworkManagerWlanApi::GetAccessPointProperty(
nit: rename this GetAccessPointPropertyAndBlock?

http://codereview.chromium.org/7725001/diff/3001/content/browser/geolocation/...
File content/browser/geolocation/wifi_data_provider_linux.cc (right):

http://codereview.chromium.org/7725001/diff/3001/content/browser/geolocation/...
content/browser/geolocation/wifi_data_provider_linux.cc:50: //
WifiDataProviderCommon::WlanApiInterface
Should probably comment that this makes blocking DBus calls.

http://codereview.chromium.org/7725001/diff/3001/content/browser/geolocation/...
File content/browser/geolocation/wifi_data_provider_linux_unittest.cc (right):

http://codereview.chromium.org/7725001/diff/3001/content/browser/geolocation/...
content/browser/geolocation/wifi_data_provider_linux_unittest.cc:141: const int
kDeviceTypeWifi = 2;
Is the '2' to match TYPE_WIFI in network_library.h? If so and it is inconvenient
to include that header, we should comment that.

http://codereview.chromium.org/7725001/diff/3001/content/browser/geolocation/...
content/browser/geolocation/wifi_data_provider_linux_unittest.cc:177: uint8
ssid[] = {0x74, 0x65, 0x73, 0x74};  // "test"
nit: Use consts for all of these, hear and below, so it is easier to maintain
them.

[satorux1, 2011-08-25 06:03:16]

http://codereview.chromium.org/7725001/diff/1/content/browser/geolocation/wif...
File content/browser/geolocation/wifi_data_provider_linux.cc (right):

http://codereview.chromium.org/7725001/diff/1/content/browser/geolocation/wif...
content/browser/geolocation/wifi_data_provider_linux.cc:316: dbus::Response*
NetworkManagerWlanApi::GetAccessPointProperty(
On 2011/08/24 23:32:04, Steven Bennetts wrote:
> nit: rename this GetAccessPointPropertyAndBlock?

I'd keep the original name, as blocking D-Bus calls are used in many places in
the code, which is ok as the code runs in "Geolocation" thread.

http://codereview.chromium.org/7725001/diff/3001/content/browser/geolocation/...
File content/browser/geolocation/wifi_data_provider_linux.cc (right):

http://codereview.chromium.org/7725001/diff/3001/content/browser/geolocation/...
content/browser/geolocation/wifi_data_provider_linux.cc:50: //
WifiDataProviderCommon::WlanApiInterface
On 2011/08/24 23:32:04, Steven Bennetts wrote:
> Should probably comment that this makes blocking DBus calls.

Done.

http://codereview.chromium.org/7725001/diff/3001/content/browser/geolocation/...
File content/browser/geolocation/wifi_data_provider_linux_unittest.cc (right):

http://codereview.chromium.org/7725001/diff/3001/content/browser/geolocation/...
content/browser/geolocation/wifi_data_provider_linux_unittest.cc:141: const int
kDeviceTypeWifi = 2;
On 2011/08/24 23:32:04, Steven Bennetts wrote:
> Is the '2' to match TYPE_WIFI in network_library.h? If so and it is
inconvenient
> to include that header, we should comment that.

This matches NM_DEVICE_TYPE_WIFI in wifi_data_provider_linux.cc. We could share
the constant via wifi_data_provider_linux.h, but I'd rather keep the header file
concise. Added a comment about it.

http://codereview.chromium.org/7725001/diff/3001/content/browser/geolocation/...
content/browser/geolocation/wifi_data_provider_linux_unittest.cc:177: uint8
ssid[] = {0x74, 0x65, 0x73, 0x74};  // "test"
On 2011/08/24 23:32:04, Steven Bennetts wrote:
> nit: Use consts for all of these, hear and below, so it is easier to maintain
> them.

Done. Most values are converted to some other format like "00:11:22:33:44:55" =>
"00-11-22-33-44-55", so I didn't try to share them with the test defined below.

[John Knottenbelt, 2011-08-25 10:02:11]

LGTM.

With regards to testing with maps.google.com, is there  a way to disable other
methods of geolocation (e.g. network-based) so that we can be sure that the wifi
code is being exercised?

[John Knottenbelt, 2011-08-25 10:13:50]

+Marcus who is familiar with the code and Leandro who has been working on a way
to log the URL that is being sent to the geolocation server. We could inspect
this URL to make sure that the wifi data is being sent correctly.

[bulach, 2011-08-25 13:22:45]

LGTM

thanks, this is a very nice simplification and I loved the tests! :)

as for the manual tests on maps.google.com, the easiest is to run on a box with
wifi.. if the "accuracy" is high and the maps is showing at a street-level zoom,
it probably means it's using the wifi.. if it's city-level, it's probably using
IP-based.

this is done in
network_location_provider.cc::NetworkLocationProvider::RequestPosition, the
"wifi_data_" param is the one provided by this patch.

http://codereview.chromium.org/7725001/diff/8001/content/browser/geolocation/...
File content/browser/geolocation/wifi_data_provider_linux.cc (right):

http://codereview.chromium.org/7725001/diff/8001/content/browser/geolocation/...
content/browser/geolocation/wifi_data_provider_linux.cc:92:
NetworkManagerWlanApi::NetworkManagerWlanApi() {
not strictly needed, but maybe zero-initialize network_manager_proxy_ ?

[satorux1, 2011-08-25 17:00:16]

Thank you for the comments. Glad to know that the patch was well received.

As for manual testing on maps.google.com, I've tested this on a machine with a
wifi device enabled. The accuracy is very hight (it precisely points to the
building I'm in). Without wifi, the accuracy is like city level.

To see the wifi code is actually exercised, I've also checked output from
VLOG(1) by adding --vmodule=wifi_data_provider_linux=1.

http://codereview.chromium.org/7725001/diff/8001/content/browser/geolocation/...
File content/browser/geolocation/wifi_data_provider_linux.cc (right):

http://codereview.chromium.org/7725001/diff/8001/content/browser/geolocation/...
content/browser/geolocation/wifi_data_provider_linux.cc:92:
NetworkManagerWlanApi::NetworkManagerWlanApi() {
On 2011/08/25 13:22:45, bulach wrote:
> not strictly needed, but maybe zero-initialize network_manager_proxy_ ?

Good catch! Done.

[stevenjb, 2011-08-25 17:32:22]

lgtm