First cut at implementing wifi bindigns for linux, using glib-dbus to Network...

First cut at implementing wifi bindigns for linux, using glib-dbus to NetworkManager

NOTE: Adds a new build dependency on dbus-glib, on ubuntu you can meet this with:
$ sudo aptitude install dbus-glib-1-dev 


BUG=http://crbug.com/37199
TEST=use browser with --enable-geolocation


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

[Evan Martin, 2010-03-10 03:48:05]

Broad comment: rather than name the class with "Linux", consider a name like
"NetworkManager".  This serves both to distinguish from ChromeOS, but also
because this should work on other NetworkManager platforms.

http://codereview.chromium.org/787003/diff/6001/7004
File chrome/browser/geolocation/wifi_data_provider_linux.cc (right):

http://codereview.chromium.org/787003/diff/6001/7004#newcode16
chrome/browser/geolocation/wifi_data_provider_linux.cc:16: const int
kNoChangePollingIntervalMilliseconds = 120000;  // 2 mins
Might be more clear to write these like
  2 * 60 * 1000

http://codereview.chromium.org/787003/diff/6001/7004#newcode80
chrome/browser/geolocation/wifi_data_provider_linux.cc:80: GPtrArray*
GetDeviceList();
Comments on these functions?  E.g., does this pass ownership to the caller? 
What are the pointers?  Etc.

http://codereview.chromium.org/787003/diff/6001/7004#newcode123
chrome/browser/geolocation/wifi_data_provider_linux.cc:123: // get caught up
with that nonsense here, lets just assert our requirement.
s/lets/let's/

[Evan Martin, 2010-03-10 03:49:42]

PS: the threads-init comment makes me a bit nervous, because we made a point of
not using gdk threads.  But I think these are at the glib level so we're safe.

[joth, 2010-03-10 13:24:38]

Thanks for the quick review! New snapshot uploaded.

The purpose of WifiDataProviderLinux is to be a factory for all the various was
we may get wifi data on linux distro (e.g. NM, wpa_supplicant, nl80211, iwlist,
etc) therefore it doesn't make sense to name it after any one of these. Not sure
what else I could call it?

Adam, did you still want to take a look?

Cheers
Joth

http://codereview.chromium.org/787003/diff/6001/7004
File chrome/browser/geolocation/wifi_data_provider_linux.cc (right):

http://codereview.chromium.org/787003/diff/6001/7004#newcode16
chrome/browser/geolocation/wifi_data_provider_linux.cc:16: const int
kNoChangePollingIntervalMilliseconds = 120000;  // 2 mins
On 2010/03/10 03:48:06, Evan Martin wrote:
> Might be more clear to write these like
>   2 * 60 * 1000

Done.

http://codereview.chromium.org/787003/diff/6001/7004#newcode80
chrome/browser/geolocation/wifi_data_provider_linux.cc:80: GPtrArray*
GetDeviceList();
On 2010/03/10 03:48:06, Evan Martin wrote:
> Comments on these functions?  E.g., does this pass ownership to the caller? 
> What are the pointers?  Etc.

Done.

http://codereview.chromium.org/787003/diff/6001/7004#newcode123
chrome/browser/geolocation/wifi_data_provider_linux.cc:123: // get caught up
with that nonsense here, lets just assert our requirement.
On 2010/03/10 03:48:06, Evan Martin wrote:
> s/lets/let's/

Done.

[agl, 2010-03-10 14:51:04]

LGTM. Note: don't land until the builders have the correct packages installed.

http://codereview.chromium.org/787003/diff/20001/21004
File chrome/browser/geolocation/wifi_data_provider_linux.cc (right):

http://codereview.chromium.org/787003/diff/20001/21004#newcode20
chrome/browser/geolocation/wifi_data_provider_linux.cc:20: const int
kNoChangePollingIntervalMilliseconds = 2 * 20 * 1000;  // 2 mins
this appears to be 40 seconds, which contradicts the comment.

http://codereview.chromium.org/787003/diff/20001/21004#newcode23
chrome/browser/geolocation/wifi_data_provider_linux.cc:23: const char*
kNetworkManagerServiceName = "org.freedesktop.NetworkManager";
change to const char kNetworkBlah[] = "..." to avoid additional relocations.

http://codereview.chromium.org/787003/diff/20001/21004#newcode33
chrome/browser/geolocation/wifi_data_provider_linux.cc:33: inline void
operator()(GPtrArray* x) const {
Remove inline, it's pointless these days.

http://codereview.chromium.org/787003/diff/20001/21004#newcode43
chrome/browser/geolocation/wifi_data_provider_linux.cc:43: inline void
operator()(void* x) const {
remove inline

http://codereview.chromium.org/787003/diff/20001/21004#newcode60
chrome/browser/geolocation/wifi_data_provider_linux.cc:60: static inline GValue
empty_gvalue() {
remove inline

http://codereview.chromium.org/787003/diff/20001/21004#newcode68
chrome/browser/geolocation/wifi_data_provider_linux.cc:68: // APPI binding to
NetworkManager, to allow reuse of the polling driven behavior
typo APPI? (If not then you should probably expand it.)

http://codereview.chromium.org/787003/diff/20001/21004#newcode101
chrome/browser/geolocation/wifi_data_provider_linux.cc:101:
WifiData::AccessPointDataSet* data);
indentation missing a couple of spaces.

http://codereview.chromium.org/787003/diff/20001/21004#newcode207
chrome/browser/geolocation/wifi_data_provider_linux.cc:207: // Only report
failure if no adapter successfully scann and at least one
typo "scann"

http://codereview.chromium.org/787003/diff/20001/21004#newcode330
chrome/browser/geolocation/wifi_data_provider_linux.cc:330: if (CheckError() ||
!value_out)
Just wondering: if !value_out, then haven't we just passed NULL as an out arg to
dbus_g_proxy_call? Doesn't it crash?

[joth, 2010-03-10 15:22:41]

Thanks, all fixed as you suggest.

I'll hang on until I hear about the build bots before committing....

Cheers
Joth


On 2010/03/10 14:51:04, agl wrote:
> LGTM. Note: don't land until the builders have the correct packages installed.
> 
> http://codereview.chromium.org/787003/diff/20001/21004
> File chrome/browser/geolocation/wifi_data_provider_linux.cc (right):
> 
> http://codereview.chromium.org/787003/diff/20001/21004#newcode20
> chrome/browser/geolocation/wifi_data_provider_linux.cc:20: const int
> kNoChangePollingIntervalMilliseconds = 2 * 20 * 1000;  // 2 mins
> this appears to be 40 seconds, which contradicts the comment.
> 
> http://codereview.chromium.org/787003/diff/20001/21004#newcode23
> chrome/browser/geolocation/wifi_data_provider_linux.cc:23: const char*
> kNetworkManagerServiceName = "org.freedesktop.NetworkManager";
> change to const char kNetworkBlah[] = "..." to avoid additional relocations.
> 
> http://codereview.chromium.org/787003/diff/20001/21004#newcode33
> chrome/browser/geolocation/wifi_data_provider_linux.cc:33: inline void
> operator()(GPtrArray* x) const {
> Remove inline, it's pointless these days.
> 
> http://codereview.chromium.org/787003/diff/20001/21004#newcode43
> chrome/browser/geolocation/wifi_data_provider_linux.cc:43: inline void
> operator()(void* x) const {
> remove inline
> 
> http://codereview.chromium.org/787003/diff/20001/21004#newcode60
> chrome/browser/geolocation/wifi_data_provider_linux.cc:60: static inline
GValue
> empty_gvalue() {
> remove inline
> 
> http://codereview.chromium.org/787003/diff/20001/21004#newcode68
> chrome/browser/geolocation/wifi_data_provider_linux.cc:68: // APPI binding to
> NetworkManager, to allow reuse of the polling driven behavior
> typo APPI? (If not then you should probably expand it.)
> 
> http://codereview.chromium.org/787003/diff/20001/21004#newcode101
> chrome/browser/geolocation/wifi_data_provider_linux.cc:101:
> WifiData::AccessPointDataSet* data);
> indentation missing a couple of spaces.
> 
> http://codereview.chromium.org/787003/diff/20001/21004#newcode207
> chrome/browser/geolocation/wifi_data_provider_linux.cc:207: // Only report
> failure if no adapter successfully scann and at least one
> typo "scann"
> 
> http://codereview.chromium.org/787003/diff/20001/21004#newcode330
> chrome/browser/geolocation/wifi_data_provider_linux.cc:330: if (CheckError()
||
> !value_out)
> Just wondering: if !value_out, then haven't we just passed NULL as an out arg
to
> dbus_g_proxy_call? Doesn't it crash?

[dglazkov, 2010-03-12 18:26:23]

Did we forget about the canaries?