Last Comment Bug 668194 - linux - iwlib only returns one ac resulting in an awful accuracy.
: linux - iwlib only returns one ac resulting in an awful accuracy.
Status: VERIFIED FIXED
: regression
Product: Core
Classification: Components
Component: Geolocation (show other bugs)
: 5 Branch
: x86_64 Linux
: P1 major with 2 votes (vote)
: mozilla18
Assigned To: Doug Turner (:dougt)
:
Mentors:
: 684568 (view as bug list)
Depends on: 724496 849204
Blocks: 794897 677256 799391
  Show dependency treegraph
 
Reported: 2011-06-29 05:58 PDT by lionel.delourme_mozillabugzilla@m4x.org
Modified: 2014-05-29 11:15 PDT (History)
13 users (show)
dougt: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
verified


Attachments
opera chrome firefox running a test case on ubuntu 11.04 (592.14 KB, image/png)
2011-07-02 01:25 PDT, jean.meninno
no flags Details
Geo test showing accuracy problems (262 bytes, application/octet-stream)
2011-07-12 09:56 PDT, jean.meninno
no flags Details
patch v.1 (1.17 KB, patch)
2011-11-01 14:06 PDT, Doug Turner (:dougt)
mark.finkle: review+
christian: approval‑mozilla‑aurora+
Details | Diff | Splinter Review
Bug 668194 geolocation with dbus. (18.74 KB, patch)
2012-08-31 10:38 PDT, bsurender
dougt: review-
Details | Diff | Splinter Review
Bug 668194 geolocation with dbus. (18.83 KB, patch)
2012-09-03 12:16 PDT, bsurender
no flags Details | Diff | Splinter Review
Bug 668194 geolocation with dbus. (18.08 KB, patch)
2012-09-03 14:03 PDT, bsurender
no flags Details | Diff | Splinter Review
Bug 668194 geolocation with dbus. (18.26 KB, patch)
2012-09-03 14:07 PDT, bsurender
dougt: review-
Details | Diff | Splinter Review
Bug 668194 geolocation with dbus. (19.95 KB, patch)
2012-09-05 14:14 PDT, bsurender
dougt: review-
Details | Diff | Splinter Review
Bug 668194 geolocation with dbus. (20.25 KB, patch)
2012-09-06 16:36 PDT, bsurender
no flags Details | Diff | Splinter Review
Bug 668194 geolocation with dbus. (20.25 KB, patch)
2012-09-06 16:44 PDT, bsurender
no flags Details | Diff | Splinter Review
Bug 668194 geolocation with dbus. (15.77 KB, patch)
2012-09-10 13:15 PDT, bsurender
no flags Details | Diff | Splinter Review
Bug 668194 geolocation with dbus. Latest changes only. (7.43 KB, patch)
2012-09-10 13:18 PDT, bsurender
no flags Details | Diff | Splinter Review
Bug 668194 geolocation with dbus. (15.96 KB, patch)
2012-09-10 19:19 PDT, bsurender
no flags Details | Diff | Splinter Review
Bug 668194 geolocation with dbus. (16.02 KB, patch)
2012-09-10 21:40 PDT, bsurender
dougt: review+
Details | Diff | Splinter Review

Description lionel.delourme_mozillabugzilla@m4x.org 2011-06-29 05:58:32 PDT
It is a setup-specific bug (more info on my setup below). Under linux-mint 11, the bug affects firefox (versions between 3.6 and 5.0 it seems) and opera (tested on 11.10).

Example urls :
http://html5demos.com/geo
http://html5doctor.com/demos/geolocation/geolocation-getcurrentposition-example.html
http://html5doctor.com/demos/geolocation/geolocation-watchposition-example.html

For instance, when testing (from https://developer.mozilla.org/en/using_geolocation) :

http://www.thedotproduct.org/experiments/geo/

I get results like (this was yesterday) :
"Accuracy not sufficient (18000m vs 150m) - last reading taken at: 13:43:14"
or today (worse and worse) :
"Accuracy not sufficient (166000m vs 150m) - last reading taken at: 14:53:00"

and then always "Timeout!"

My best guess is that something is wrong with wifi-enabled geolocation. Maybe the browser can't access a proper list of wifi access points, maybe it sends them using the wrong protocol or mucks things up some other way, or maybe google location services doesn't send an answer for some weird reason.

My favorite would be hypothesis number one, but then, what does chrome do right that FF and opera do wrong ? Or alternatively why something in my setup would specifically affect these browsers and not chrome ?

Details on my setup :

Fresh install of FF

Help > Troubleshooting

  Application Basics

        Name
        Firefox

        Version
        5.0

        User Agent
        Mozilla/5.0 (X11; Linux i686 on x86_64; rv:5.0) Gecko/20100101 Firefox/5.0

        Profile Directory

          Open Containing Folder

        Enabled Plugins

          about:plugins

        Build Configuration

          about:buildconfig

  Extensions

        Name

        Version

        Enabled

        ID

  Modified Preferences

      Name

      Value

        browser.places.smartBookmarksVersion
        2

        browser.startup.homepage_override.buildID
        20110615151330

        browser.startup.homepage_override.mstone
        rv:5.0

        extensions.lastAppVersion
        5.0

        network.cookie.prefsMigrated
        true

        places.history.expiration.transient_current_max_pages
        118037

        privacy.sanitize.migrateFx3Prefs
        true

  Graphics

        GPU Accelerated Windows
        0/1

Thanks for helping !

-- Lionel
Comment 1 jean.meninno 2011-06-29 08:09:33 PDT
I'm getting the same behavior on two ubuntu machines one is version 10.10 and the other is 11.04, also it was tested using ff 3.6.1 on both of them, then I upgrade to the latest versions from the repository not working either, also following the instruction on mozilla's site to make the installation (which happens to be an ubuntu help site) and the result it's the same, also i tried to compare the about:config regarding geopositioning with a windows instalation and all the configs are just the same on both versions 3.6.1 and 5. later i tried to install from tar.bz2 on mozilla site getting the exact same results.

it should notice that the windows version it's working good
Comment 2 Justin Dolske [:Dolske] 2011-07-01 15:11:44 PDT
It sounds to me like you're just in a location where there isn't WiFi location data available (insufficient coverage, or just in an area not yet scanned). In which case the geolocation service will fallback (I believe) to GeoIP lookups.
Comment 3 jean.meninno 2011-07-02 00:54:50 PDT
Let me expand the information, i ran the same tests, meaning that i tested ff 3.6.1 and ff 5 on the same machine running ubuntu 11.04 and windows 7 (dual booting), the windows version works flawlessly non the less the linux version it's having problems, so the WiFi conections i belive won't be the culprit here, also the chrome and opera browser in my ubuntu box are working good, and as far i know booth uses the google geo location service.

also i tried at work where i have a ubuntu 11.04 and a debian stable box and they behave the same. if you need more technical details i will have no problem to provide them.
Comment 4 jean.meninno 2011-07-02 01:25:12 PDT
Created attachment 543584 [details]
opera chrome firefox running a test case on ubuntu 11.04
Comment 5 George Carstoiu 2011-07-12 04:21:39 PDT
Mozilla/5.0 (X11; Linux i686; rv:8.0a1) Gecko/20110711 Firefox/8.0a1

Works fine for me using the latest trunk.

Accessing http://html5demos.com/geo points me to same place on the map on Opera, Chrome and Firefox.
Comment 6 lionel.delourme_mozillabugzilla@m4x.org 2011-07-12 07:22:27 PDT
http://www.thedotproduct.org/experiments/geo/

In firefox :

Accuracy not sufficient (140000m vs 150m) - last reading taken at: 16:20:09

In chrome :

Current positon: lat=48.9050817, long=2.221826 (accuracy 72m)
Speed: min=Not recorded/0m/s, max=Not recorded/0m/s
Altitude: min=Not recorded/0m, max=Not recorded/0m (accuracy 0m)
last reading taken at: 16:21:10
Comment 7 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2011-07-12 08:36:03 PDT
From Vancouver, Canada:
Accuracy not sufficient (18000m vs 150m) - last reading taken at: 08:31:39

It's not as bad as comment 6 though. I think physical location (maybe ISP) has a lot to do with this bug. I'm going to mark this bug as NEW for now simply because other browsers are much better and we should at least strive for parity.

There are probably multiple factors at play here which are simply exposing an issue in Firefox. Any chance we can get a minimized testcase here based on the link in comment 6?
Comment 8 jean.meninno 2011-07-12 09:56:14 PDT
Created attachment 545396 [details]
Geo test showing accuracy problems

load the reffered file in a blank html or execute it using firebug
Comment 9 jean.meninno 2011-07-12 09:59:48 PDT
The data from the test case uploaded

FIREFOX 5 nigthly build from 20110711 distributions i686 and x64 
Location: Madrid, Spain
lat: 40.41669
lon: -3.700345
accuracy: 18000

CHROME
Location: Madrid, Spain
lat: 40.4682481
lon: -3.6534677
accuracy: 44
Comment 10 Jo Hermans 2011-09-04 15:43:33 PDT
*** Bug 684568 has been marked as a duplicate of this bug. ***
Comment 11 Bao Thien Ngo 2011-09-04 17:57:40 PDT
I have two different laptops, one running Windows 7 64-bit and one running Windows XP SP3 32-bit; both using the same FF 6.0.1 build. The Windows 7 seems to be fine; but Windows XP is having the accuracy issue.
Comment 12 Rubin110 2011-10-31 11:14:53 PDT
Under Debian Sid, running both 7.0.1 off of the Debian repo and the binary downloaded off of the getfirefox site, I'm seeing the same issue. It's fairly apparently that the browser is unable to either get wifi information or pass that along to whatever 3rd party geolocation service Mozilla is using. Living in San Francisco the returned location under maps.google.com is basically all of the bay area.

Using Chromium under Debian Sid maps.google.com is able to locate the roof to my house pretty quickly.

Is there anyway I can get Firefox to spit out some debug regarding geolocaiton services to the console or a log somewhere?
Comment 13 Doug Turner (:dougt) 2011-11-01 14:06:29 PDT
Created attachment 571141 [details] [diff] [review]
patch v.1
Comment 14 Doug Turner (:dougt) 2011-11-01 21:05:51 PDT
https://hg.mozilla.org/mozilla-central/rev/392fa68084a8
Comment 15 Doug Turner (:dougt) 2011-11-01 21:06:57 PDT
Comment on attachment 571141 [details] [diff] [review]
patch v.1

this is a significant regression, with little risk for regression.
Comment 16 Doug Turner (:dougt) 2011-11-01 21:08:06 PDT
um.  this is a significant regression.  the risk to further breaking anything is small.
Comment 17 christian 2011-11-07 13:31:50 PST
Comment on attachment 571141 [details] [diff] [review]
patch v.1

[triage comment]

Approved for aurora. Please land today if at all possible.
Comment 19 Virgil Dicu [:virgil] [QA] 2011-11-30 05:04:54 PST
Can anyone with Linux and Wi-fi available check the behavior of Firefox 11, 10 and 9 now that this is resolved? 
http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/latest-trunk/
http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/latest-mozilla-aurora/
http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/9.0b3-candidates/
Comment 20 Rubin110 2011-11-30 12:16:25 PST
Fix failed under Firefox nightly 11.0a1 (2011-11-30) x64, Debian Sid.
Comment 21 Rubin110 2011-11-30 12:21:25 PST
Test steps:

1. Go somewhere densely populated where WPS is most likely possible (like at the corner of Mission and 14th St in San Francisco).
2. Connect to a wifi access point for the internets.
3. Open Firefox.
4. Go to http://html5demos.com/geo
5. Allow to share location.
6. Observe.

Expected results:
A map is shown with an accurate fixed location of where the user physically is in the world (Mission and 14th).

Actual results:
Location returned is as accurate as my city (the middle of San Francisco).
Comment 22 Doug Turner (:dougt) 2011-11-30 13:58:03 PST
Hey Rubin, how many Access Points do you think around around you?
Comment 23 Rubin110 2011-11-30 14:19:14 PST
Oh plenty. Trust me when I say that this is a very well wardriven/cellphonded part of San Francisco. My Android has no problem detecting location through WPS. Not sure if Mozilla is using the Google WPS service for location info, but seriously, should work here just fine here.

I'll be happy to repeat the test in some other part of San Francisco later tonight.
Comment 24 Rubin110 2011-11-30 21:55:09 PST
Same results, fix failed.
Comment 25 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2011-12-13 13:35:35 PST
Doug, should this be reopened given Rubin's testing?
Comment 26 Doug Turner (:dougt) 2011-12-13 16:23:14 PST
Hey Rubin,
Could you set the preference "geo.wifi.logging" to true and restart.  Then look in the console log and post here what you see.  (or email it to me if you are uncomfortable about making that data public)
Comment 27 Rubin110 2011-12-23 03:21:04 PST
Any particular switches I should use when firing up Firefox from a console? If not, I see nothing dumped into the console when loading up the first link for the bug description, additionally the Web Console nor the Error Console spit out anything relevant.

Firefox Nightly 12.0a1 (2011-12-22) on Debian Sid x64.
Comment 28 paul 2011-12-26 10:12:38 PST
I've encountered the same issue with Firefox 8 on Ubuntu and Firefox 9 on Windows.

I have an OpenStreetMap based map, with geolocation enabled (http://paul-lockett.co.uk/map)

On Ubuntu, using three separate browsers for testing, the stated accuracies at my current location are:

Opera: 45 metres
Chromium: 32 metres
Firefox: 122000 metres

In earlier versions of Firefox, the accuracy was much higher.
Comment 29 paul 2011-12-30 13:25:44 PST
As an experiment, I switched the geolocation provider to openbmaps and the accuracy was much improved, being approximately house level.

On that basis, it would appear that Firefox is able to obtain the wifi data, but with the default provider, it is failing to pass the data across, resulting in an IP look-up, whereas, with openbmaps, it is passing the wifi data across successfully.
Comment 30 Doug Turner (:dougt) 2011-12-30 15:26:00 PST
this is probably a dup of the newer bug 713991 (which has a patch and was pushed to try this morning)

*** This bug has been marked as a duplicate of bug 713991 ***
Comment 31 Rubin110 2012-01-11 11:27:13 PST
Patch from bug 713991 doesn't fix this issue. running Firefox Night 12.0a1 (2012-01-11), Debian Sid 64bit.

My current physical location is Metalab in Vienna, I'm tunneling my tubes back to a machine somewhere in the San Francisco Bay Area. Chromium finds me at the exact corner where Metalab is located while Firefox throws me in the middle of SF. If I turn off tunneling Firefox will place me in the "middle" of Vienna no where near Metalab. Others here running current stable builds of Firefox under Windows and Mac are able to geo locate within a block of Metalab or better.

Again if someone wants to provide me with some details on how I can get the particular reporting logs they want to see I'll be happy to provide.
Comment 32 paul 2012-01-11 11:54:55 PST
I can confirm that with both Aurora and Nightly, I am still experiencing the same level of inaccuracy as before, with Firefox offering IP address level accuracy and Chromium providing wifi access point level accuracy.
Having checked the http logs for both browsers, the difference appears to be that Chromium is sending all the visible access points to the location provider when requesting the location, while Firefox is only sending the details for the access point that the laptop is connected to.
Comment 33 Josh Matthews [:jdm] 2012-01-12 02:11:11 PST
Paul, what operating system are you using? I see multiple APs being used on OS X 10.6.8.
Comment 34 Doug Turner (:dougt) 2012-01-12 14:42:39 PST
Rubin110 and Paul, in comment #26 I explained how to enable logging.  I'd love to see that output.
Comment 35 paul 2012-01-12 15:22:06 PST
Josh, I'm running Ubuntu 11.10.  I've also seen the same level of accuracy when running a test on a Windows XP machine, although I can't verify the number of access points which were being sent during that test.
Comment 36 paul 2012-01-12 15:44:02 PST
Doug, where do I find the log output?

I've gone into "about:config", entered "geo.wifi.logging" as a new Preference Name and set the value to true.  I've restarted, run a geo look-up and checked the web console, error console and Firebug, but I can't see anything relevant, so I guess I must be going about it incorrectly.
Comment 37 Rubin110 2012-01-12 17:36:04 PST
I am in the same boat as Paul.
Comment 38 Josh Matthews [:jdm] 2012-01-12 18:54:41 PST
The pref is geo.wifi.logging.enabled. The output will be available in the Error Console.
Comment 40 Doug Turner (:dougt) 2012-01-13 19:04:56 PST
paul, let me restate the problem that I think you are seeing -- please correct me if I am off:

On some/all linux distro's, you are only seeing the AP that the device/machine is connected to being sent.  On other operating systems, you are not seeing this problem.

(also marking your last post private since it has your router's mac)
Comment 41 paul 2012-01-14 03:52:15 PST
Doug, I'm only running Ubuntu 11.10, so I can only confirm the AP issue on that system.  However, I installed Firefox on a friend's laptop, which was running Windows XP, a couple of weeks ago and saw a similar level of accuracy, so I suspect it maybe a cross-platform issue.
Comment 42 Jo Hermans 2012-01-14 07:10:12 PST
I'm always seeing this request to maps.googleapis.com :

*** WIFI GEO: ************************************* Sending request:
https://maps.googleapis.com/maps/api/browserlocation/json?browser=firefox&sensor=true

That is, no access-token or access-points were added either. The result is that only my IP-address was used to determine the location, and thus the accuracy was 250000.

*** WIFI GEO: service returned: {
   "accuracy" : 25000.0,
   "location" : {
      "lat" : 51.2095670,
      "lng" : 4.4348190
   },
   "status" : "OK"
}

I'm on a Lenovo T400 laptop with Windows XP SP 3.  I can see that there's a wlanapi.dll in C:\WINNT\system32, so nsWifiMonitor::DoScan() should be able to do its job. I'm not seeing any logging in DoScan(), so I don't know how to debug it.

Lenovo uses a GUI called ThinkVantage Access Connections to manage wireless access, but I noticed that the Windows Wireless Network Setup thinks I don't have a wireless setup, so it might be doing something different behind the scenes. For instance, I can to to 'Wireless Network Connection' in the Control Panel, but I can't view the wireless networks around me, since it keep insisting on choosing a wireless network first (which it thinks there's is none). That might explain it.

Other applications like inSSIDer are fine.
Comment 43 Jo Hermans 2012-01-14 07:30:47 PST
I tried creating my own URL, but then I noticed that the lookup was cached, and I kept receiving the same result again. Keep that in mind if you want to test.I you want to test too, just add this to the end of the url above :

&wifi=mac:$MAC|ssid:$SSID|ss:$SIGNAL

where $MAC is the mac-address (use a dash as a separator, not a colon), $SSID is your SSID (be careful to encode correctly), and $SIGNAL is the signal strength (a random number between -40 and -80 or so will do fine)

If I passed in a single MAC, I got the same result was above, but the accuracy was halved to 122000 meters. When I added 2 more accesspoints from my closest neighbors, I got this result :

{
   "accuracy" : 40.0,
   "location" : {
      "lat" : 51.18229359999999,
      "lng" : 4.4318110
   },
   "status" : "OK"
}

And it's indeed correct, 40 meters is the distance from my flat to the cordinates. And more important: the exact same result as Google Chrome reports, even with many more accesspoints.

So, despite the handicap of not having any accesspoints found in my setup, I can verify that the location check itself works correctly, and there's not difference at all with Google's own browser. If your setup (granted, on Linux) can't do that, then it must be because you also have the problem that no accesspoints could be found, and then the result is always based on the ipaddress with a very large accuracy radius.
Comment 44 Doug Turner (:dougt) 2012-01-15 22:22:41 PST
So, I know that our wifi listening code for linux only returns the AP you are connected to.  That should be fixed - we can file a follow up for that.

I am unaware of the windows failure.  Jo, do you know if Chrome fails in the same way?
Comment 45 Jo Hermans 2012-01-16 16:43:39 PST
No, Chrome does it correctly.

When I compare their code (wifi_data_provider_win.cc for Chrome versus nsWifiScannerWin.cpp for Gecko), I see that they use a different parameter in the WlanGetNetworkBssList hook : dot11_BSS_type_any (3) instead of DOT11_BSS_TYPE_UNUSED (0). But that might be because they use WLAN on Vista, and NDIS on older operating systems. Gecko only uses WLAN.
Comment 47 Bao Thien Ngo 2012-08-19 14:21:15 PDT
(In reply to Jo Hermans from comment #42)
> I'm always seeing this request to maps.googleapis.com :
> 
> *** WIFI GEO: ************************************* Sending request:
> https://maps.googleapis.com/maps/api/browserlocation/
> json?browser=firefox&sensor=true
> 
> That is, no access-token or access-points were added either. The result is
> that only my IP-address was used to determine the location, and thus the
> accuracy was 250000.
> 
> *** WIFI GEO: service returned: {
>    "accuracy" : 25000.0,
>    "location" : {
>       "lat" : 51.2095670,
>       "lng" : 4.4348190
>    },
>    "status" : "OK"
> }
> 
> I'm on a Lenovo T400 laptop with Windows XP SP 3.  I can see that there's a
> wlanapi.dll in C:\WINNT\system32, so nsWifiMonitor::DoScan() should be able
> to do its job. I'm not seeing any logging in DoScan(), so I don't know how
> to debug it.
> 
> Lenovo uses a GUI called ThinkVantage Access Connections to manage wireless
> access, but I noticed that the Windows Wireless Network Setup thinks I don't
> have a wireless setup, so it might be doing something different behind the
> scenes. For instance, I can to to 'Wireless Network Connection' in the
> Control Panel, but I can't view the wireless networks around me, since it
> keep insisting on choosing a wireless network first (which it thinks there's
> is none). That might explain it.
> 
> Other applications like inSSIDer are fine.

I have an IBM Thinkpad T41, essentially from the same family as Lenovo T400 I'm guessing. I also have problems with geolocation on that laptop, running Windows XP SP3. I opted to use a driver without all that ThinkVantage stuff, so I can see the different WiFi Access Points using Windows networking, but for some reason, when I use inSSIDer, the application doesn't pick up any wireless signals. The Lenovo/Thinkpads definitely seem fishy in that regard.
Comment 48 Rubin110 2012-08-19 14:59:22 PDT
Still an issue under Debian Sid, Firefox nightly 17.0a1 (2012-08-03), works fine if I boot into Windows whatever. I don't see how hardware configuration has anything to do with this but I'm on a ThinkPad X220.
Comment 49 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-08-19 16:46:57 PDT
> I don't see how hardware configuration has anything to do with this

It seems like there are only certain conditions where this occurs (ie. some people encounter it often, some not at all). Given that there are numerous different wireless chipsets made by several different manufacturers with different driver versions on different OS versions with different updates installed and running different networking software. This creates a landscape of huge invariability which makes it hard to debug this issue.

I think the only way we are going to be able to narrow this down (a requirement before we can even begin talking about a fix) we'll need to start cataloguing the various conditions where this occurs so we can try to nail down a reproducible test case so we can debug the code path(s).

All that said, I think it would be helpful if everyone who can reproduce this issue report their wireless chipset version, driver version, OS version, software being used for WiFi, wireless network type and encryption type. If we can start pulling out some commonalities from this data we can start trying to narrow down an internally reproducible testcase. Then and only then will we be able to start debugging for a fix.

Computer model/manufacturer is less important since many manufacturers use the same wireless chipsets and many models use varying versions of that chipset.

For those of you on Windows, you should be able to get the information from Device Manager in Contol Panel. Those of you using Linux should be able to get it through Network Manager > Connection Information.

Thanks
Comment 50 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-08-19 16:48:36 PDT
> This creates a landscape of huge invariability which makes it hard to debug this issue.

Sorry, that should read "variability", meaning there are just too many noisy variables to this issue right now. We need more data to help narrow this down.
Comment 51 Rubin110 2012-08-19 17:08:53 PDT
This bug is currently specific for Linux architecture. I am simply a not part of Mozilla QA engineer, but I would recommend starting a separate bug to track what sounds like a similar unexpected result on Windows.

I really don't feel like this is a hardware issue. Chromium while running Network Manager can pick out a wifi-centric geolocation just fine on my machine. Others have also reported similar results with Chrome and Opera. This leads me to believe that Firefox is simply having a hard time poking Network Manager for a list of near by networks.

Both Firefox and Chromium fail at doing wifi look ups when I'm running Wicd instead of Network Manager.

I think this is simply an issue of Firefox having a hard time at tickling whatever wifi manager software we're running for a listing of local networks between different Linux distros and release versions, and less of some odd ball combination of hardware and wifi encryption type (seriously?).

Though I could be totally wrong, again not a Mozilla employee. Is there some public spec/PRD outlining how this WPS lookup business should work?
Comment 52 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-08-19 17:11:50 PDT
Good questions, Rubin. I'm personally not familiar enough with Firefox's geolocation code nor any official PRD/Spec. I'll refrain from further comment until a Firefox developer has had a chance to consider your comments (there are a few cc'd on this bug already).
Comment 53 Doug Turner (:dougt) 2012-08-19 20:18:09 PDT
updating summary.  This is a mozilla wifi scanning linux implementation bug.
Comment 54 Nikos Roussos [:nikos] 2012-08-20 03:01:45 PDT
I agree with Rubin (comment 51). Same here. Chromium manages to get accurate location on my laptop (running Fedora) while Firefox fails. I don't think it has anything to do with hardware. It seems that Firefox fails to cooperate with Network Manager.
Comment 55 Doug Turner (:dougt) 2012-08-20 09:59:40 PDT
Nikos, yes.  the problem is that iwlib only returns on access point.  See comment #53.
Comment 56 bsurender 2012-08-31 10:38:16 PDT
Created attachment 657350 [details] [diff] [review]
Bug 668194 geolocation with dbus.
Comment 57 Josh Matthews [:jdm] 2012-08-31 12:08:31 PDT
Comment on attachment 657350 [details] [diff] [review]
Bug 668194 geolocation with dbus.

Review of attachment 657350 [details] [diff] [review]:
-----------------------------------------------------------------

::: netwerk/wifi/Makefile.in
@@ +42,5 @@
>  else
>  ifeq ($(OS_ARCH),Linux)
>  CPPSRCS += nsWifiScannerUnix.cpp
> +CPPSRCS += nsWifiScannerDBus.cpp
> +CPPSRCS += nsWifiAPScannerDBus.cpp

Add these to a \ list instead.

@@ +57,5 @@
>  
>  DEFINES += -DIMPL_NS_NET
> +
> +CXXFLAGS += $(TK_CFLAGS) $(MOZ_DBUS_GLIB_CFLAGS)
> +CXXFLAGS := $(subst -pedantic,,$(CXXFLAGS))

Combine these, and probably move them into the Linux block?

::: netwerk/wifi/nsWifiAPScannerDBus.cpp
@@ +6,5 @@
> +
> +namespace mozilla {
> +
> +nsWifiAPScannerDBus::nsWifiAPScannerDBus()
> +{

MOZ_COUNT_CTOR for non refcounted objects

@@ +8,5 @@
> +
> +nsWifiAPScannerDBus::nsWifiAPScannerDBus()
> +{
> +  mAccessPoints = new nsCOMArray<nsWifiAccessPoint>();
> +  MOZ_ASSERT(mAccessPoints);

new is infallible, so this is superfluous.

@@ +25,5 @@
> +}
> +
> +nsresult
> +nsWifiAPScannerDBus::GetDBusIterator(DBusMessage* aMsg,
> +                                     DBusMessageIter& aIterArray)

Let's make this a pointer so it's clear that it's being modified in callers?

@@ +84,5 @@
> +  if (!dbus_message_iter_init(aMsg, &args)) {
> +    return;
> +  }
> +
> +  if (dbus_message_iter_get_arg_type (&args) != DBUS_TYPE_ARRAY) {

Nit: superfluous space after function name.

@@ +92,5 @@
> +  DBusMessageIter arr;
> +  dbus_message_iter_recurse(&args, &arr);
> +
> +  nsWifiAccessPoint* ap = new nsWifiAccessPoint();
> +  MOZ_ASSERT(ap);

Infallible new.

@@ +105,5 @@
> +      dbus_message_iter_next(&dict);
> +      DBusMessageIter variant;
> +      dbus_message_iter_recurse(&dict, &variant);
> +
> +      nsString key;

This doesn't need to exist, does it? Can't you just strcmp the propertyKey with your literals?

@@ +112,5 @@
> +        StoreSsid(&variant, ap);
> +        break;
> +      }
> +
> +    if (key.EqualsLiteral("HwAddress")) {

You lose a level of indentation here.

@@ +119,5 @@
> +    }
> +
> +    if (key.EqualsLiteral("Strength")) {
> +      uint8_t strength;
> +      if (dbus_message_iter_get_arg_type (&variant) != DBUS_TYPE_BYTE) {

Nit: extra space after function name.

@@ +124,5 @@
> +        return;
> +      }
> +      dbus_message_iter_get_basic(&variant, &strength);
> +      ap->setSignal(strength);
> +      MOZ_ASSERT(mAccessPoints);

Superfluous.

@@ +174,5 @@
> +  uint8_t singleByte;
> +  uint32_t j = 0;
> +  nsresult rv;
> +
> +  for (uint32_t i = 0; i < 16; i+=3) {

Magic number?

@@ +175,5 @@
> +  uint32_t j = 0;
> +  nsresult rv;
> +
> +  for (uint32_t i = 0; i < 16; i+=3) {
> +    MOZ_ASSERT(j < 6);

Want to add an explanatory message to this assertion? MOZ_ASSERT can take an additional string argument. In fact, can we scrap the magic number and use MOZ_ARRAY_LENGTH instead?

@@ +182,5 @@
> +      return;
> +    }
> +
> +    macAddress[j] <<= 4;
> +    rv = GetInteger(hwAddress[i+1], singleByte);

Nit: space out i+1

@@ +187,5 @@
> +    if (NS_FAILED(rv)) {
> +      return;
> +    }
> +
> +    macAddress[j] |= singleByte;

This doesn't need to be |=, does it?

@@ +195,5 @@
> +  aAp->setMac(macAddress);
> +}
> +
> +nsresult
> +nsWifiAPScannerDBus::GetInteger(const uint8_t& aChar, uint8_t& aSingleByte)

Make aSingleByte a pointer to indicate its outparam-ness? Also, make this a static non-member.

@@ +219,5 @@
> +  if(NS_FAILED(rv)) {
> +    return;
> +  }
> +
> +  nsCString path;

Nit: move this down where it's used.

@@ +225,5 @@
> +  nsString propertiesInterface =
> +    NS_LITERAL_STRING("org.freedesktop.DBus.Properties");
> +
> +  do {
> +    if (dbus_message_iter_get_arg_type (&iter_array) != DBUS_TYPE_OBJECT_PATH) {

Nit: extra space.

@@ +229,5 @@
> +    if (dbus_message_iter_get_arg_type (&iter_array) != DBUS_TYPE_OBJECT_PATH) {
> +      return;
> +    }
> +    dbus_message_iter_get_basic(&iter_array, &path);
> +    rv = RegisterWithConnection(propertiesInterface,

Do something with rv or get rid of it, please.

@@ +241,5 @@
> +nsWifiAPScannerDBus::IdentifyDevices(DBusMessage* aMsg)
> +{
> +  DBusMessageIter iter_array;
> +  nsresult rv = GetDBusIterator(aMsg, iter_array);
> +  if(NS_FAILED(rv)) {

Nit: add a space after if. There are a couple other instances in this file.

@@ +247,5 @@
> +    return;
> +  }
> +
> +  nsCString devicePath;
> +  nsString path;

Nit: move this down by where it's used.

@@ +258,5 @@
> +      return;
> +    }
> +
> +    dbus_message_iter_get_basic(&iter_array, &devicePath);
> +    rv = RegisterWithConnection(wirelessInterface,

Do something with rv or get rid of it, please.

@@ +269,5 @@
> +nsresult
> +nsWifiAPScannerDBus::ScanWifiAccessPoints()
> +{
> +  nsString interface = NS_LITERAL_STRING("org.freedesktop.NetworkManager");
> +  nsCString path;

Should this be set to something? If not, can you pass EmptyCString() instead of a variable below?

@@ +295,5 @@
> +  }
> +
> +  nsString service = NS_LITERAL_STRING("org.freedesktop.NetworkManager");
> +  DBusMessage* msg =
> +    dbus_message_new_method_call(NS_ConvertUTF16toUTF8(service).get(),

Just use the literal here and avoid all the conversion.

@@ +298,5 @@
> +  DBusMessage* msg =
> +    dbus_message_new_method_call(NS_ConvertUTF16toUTF8(service).get(),
> +                                 aPath.Data(),
> +                                 NS_ConvertUTF16toUTF8(aInterface).get(),
> +                                 NS_ConvertUTF16toUTF8(aFuncCall).get());

If you're always in control of the data being passed in here, can you just pass a const char* (or an nsCString if necessary) instead of doing extra conversion?

@@ +305,5 @@
> +  }
> +
> +  if (aInterface.EqualsLiteral("org.freedesktop.DBus.Properties")) {
> +    nsString param;
> +    param.Truncate();

Unnecessary.

@@ +309,5 @@
> +    param.Truncate();
> +    DBusMessageIter argsIter;
> +    dbus_message_iter_init_append(msg, &argsIter);
> +    if (!dbus_message_iter_append_basic(&argsIter,
> +                                        DBUS_TYPE_STRING, &param)) {

What exactly is this doing? Passing the address of some random XPCOM string class to the DBUS api doesn't seem right.

@@ +331,5 @@
> +nsWifiAPScannerDBus::SendWithReply(DBusMessage* aMsg)
> +{
> +  DBusPendingCall* reply = nullptr;
> +  if (mConnection) {
> +    nsresult rv = CreateDBusConnection();

We have a connection yet are creating one?

@@ +347,5 @@
> +void
> +nsWifiAPScannerDBus::DropDBusConnection()
> +{
> +  if (mConnection) {
> +    dbus_connection_unref(mConnection);

I know nothing about the DBUS api, but the presence of an unref with no corresponding ref looks suspicious.

@@ +353,5 @@
> +  }
> +}
> +
> +nsWifiAPScannerDBus::~nsWifiAPScannerDBus()
> +{

MOZ_COUNT_DTOR

::: netwerk/wifi/nsWifiAPScannerDBus.h
@@ +4,5 @@
> +
> +#ifndef NSWIFIAPSCANNERDBUS_H_
> +#define NSWIFIAPSCANNERDBUS_H_
> +
> +#include "nsWifiAccessPoint.h"

I think you can just forward declare this class.

@@ +5,5 @@
> +#ifndef NSWIFIAPSCANNERDBUS_H_
> +#define NSWIFIAPSCANNERDBUS_H_
> +
> +#include "nsWifiAccessPoint.h"
> +#include "nsIMutableArray.h"

How about just nsCOMArray.h?

@@ +9,5 @@
> +#include "nsIMutableArray.h"
> +#include "nsAutoPtr.h"
> +
> +#define DBUS_API_SUBJECT_TO_CHANGE
> +#include <dbus/dbus.h>

Move this to the source file and forward declare any classes you need.

@@ +29,5 @@
> +   * Iterates through the dbus message received to retrieve the devices.
> +   * Sets up the DBus interace, path, method to get the access points
> +   * for each device.
> +   * Enumerates through list of devices and sets up the "GetAccessPoints"
> +   * network manager method call with dbus via function RegisterWithConnection().

No need to go into what methods it calls.

@@ +41,5 @@
> +   * Iterates through the dbus message received to retrieve the access points.
> +   * Sets up the DBus interace, path, method to get the access point properties
> +   * for each access point.
> +   * Enumerates through list of access points and sets up the "GetAll"
> +   * network manager method call with dbus via function RegisterWithConnection().

Ditto.

@@ +58,5 @@
> +   * @param      aMsg, DBusMessage received from network manager "GetAll"
> +   *             method call.
> +   */
> +  void IdentifyAPProperties(DBusMessage* aMsg);
> +  nsCOMArray<nsWifiAccessPoint>*

Newline, please. Also, let's just return a const reference to the array.

@@ +86,5 @@
> +  nsresult RegisterWithConnection(const nsAString& aInterface,
> +                                  const nsACString& aPath,
> +                                  const nsAString& aFuncCall,
> +                                  void (*aCallbackFunction)
> +                                  (DBusPendingCall*, void*));

Move this to the previous line.

@@ +143,5 @@
> +   */
> +  void SetMac(DBusMessageIter* aVariant, nsWifiAccessPoint* aAp);
> +
> +  DBusConnection* mConnection;
> +  nsAutoPtr<nsCOMArray<nsWifiAccessPoint> > mAccessPoints;

Why is this a pointer? It's created in the constructor and never changed after that, as far as I can see.

::: netwerk/wifi/nsWifiMonitor.cpp
@@ +145,5 @@
>    LOG(("@@@@@ wifi monitor run called\n"));
>  
>    PR_SetCurrentThreadName("Wifi Monitor");
>  
> +  nsresult rv = DoScanDBus();

This looks like it will break other platforms. Can you get rid of the iwlib stuff and move the DoScanDBUS code into nsWifiMonitorUnix::DoScan?

::: netwerk/wifi/nsWifiScannerDBus.cpp
@@ +14,5 @@
> +nsWifiMonitor::DoScanDBus()
> +{
> +  nsWifiAPScannerDBus wifiScannerAP;
> +  nsCOMArray<nsWifiAccessPoint>* accessPoints;
> +  nsCOMArray<nsWifiAccessPoint> lastAccessPoints;

Move these down to where they're used.

@@ +18,5 @@
> +  nsCOMArray<nsWifiAccessPoint> lastAccessPoints;
> +
> +  while (mKeepGoing) {
> +    nsresult rv = wifiScannerAP.ScanWifiAccessPoints();
> +    NS_ENSURE_SUCCESS(rv, rv);

Do we want to give up on all scanning if this fails?

@@ +21,5 @@
> +    nsresult rv = wifiScannerAP.ScanWifiAccessPoints();
> +    NS_ENSURE_SUCCESS(rv, rv);
> +
> +    LOG(("waiting on monitor\n"));
> +    ReentrantMonitorAutoEnter mon(mReentrantMonitor);

Can we scope this to only encompass the block in which we need the monitor? I don't know that we want to hold it while calling listeners, etc.

@@ +26,5 @@
> +    mon.Wait(PR_SecondsToInterval(4));
> +
> +    accessPoints = wifiScannerAP.GetWifiAccessPoints();
> +    if (!accessPoints || !accessPoints->Count()) {
> +      return NS_ERROR_FAILURE;

Really? No access points aborts all scanning?
Comment 58 Doug Turner (:dougt) 2012-08-31 12:18:11 PDT
Comment on attachment 657350 [details] [diff] [review]
Bug 668194 geolocation with dbus.

Review of attachment 657350 [details] [diff] [review]:
-----------------------------------------------------------------

Let me save these now.  I'd like to see another patch when these issues and jdm's issues are fixed.

::: netwerk/wifi/Makefile.in
@@ +40,5 @@
>  ifneq (,$(filter WINNT,$(OS_ARCH)))
>  CPPSRCS += nsWifiScannerWin.cpp
>  else
>  ifeq ($(OS_ARCH),Linux)
>  CPPSRCS += nsWifiScannerUnix.cpp

Lets go ahead and remove the iwlib version.  it will make things easier.

@@ +42,5 @@
>  else
>  ifeq ($(OS_ARCH),Linux)
>  CPPSRCS += nsWifiScannerUnix.cpp
> +CPPSRCS += nsWifiScannerDBus.cpp
> +CPPSRCS += nsWifiAPScannerDBus.cpp

Why two files?

@@ +57,5 @@
>  
>  DEFINES += -DIMPL_NS_NET
> +
> +CXXFLAGS += $(TK_CFLAGS) $(MOZ_DBUS_GLIB_CFLAGS)
> +CXXFLAGS := $(subst -pedantic,,$(CXXFLAGS))

Will this compile on all platforms?

::: netwerk/wifi/nsWifiAPScannerDBus.cpp
@@ +12,5 @@
> +  MOZ_ASSERT(mAccessPoints);
> +}
> +
> +nsresult
> +nsWifiAPScannerDBus::CreateDBusConnection()

Lets just do this in the constructor, and never try to connect anywhere else.

Looking at the docs, if the connection is null, the dbus_* functions will politely fail.  However, you'll probably want to add an assertion if mConnection is null after you try to create it.

@@ +14,5 @@
> +
> +nsresult
> +nsWifiAPScannerDBus::CreateDBusConnection()
> +{
> +  mConnection = dbus_bus_get(DBUS_BUS_SYSTEM, NULL);

NULL -> nullptr

@@ +41,5 @@
> +  return NS_OK;
> +}
> +
> +static void
> +NetworkDevicesNotify(DBusPendingCall *aPending, void* aUserData)

You should mvoe all of the static functions in this file into the nsWifiAPScannerDBus class.

@@ +60,5 @@
> +  if (!msg) {
> +    return;
> +  }
> +
> +  static_cast<nsWifiAPScannerDBus*>(aUserData)->IdentifyAccessPoints(msg);

Instead of these static functions, can't you just make the the methods you created take the DBUS arguments.  For example,

Remove NetworkDevicesAPNotify().
Change IdentifyAccessPoints() to take (DBusPendingCall *aPending, void* aUserData) as params.
Change RegisterWithConnection() to use IdentifyAccessPoints instead of NetworkDevicesAPNotify.

This would allow you to remove 4-5 functions (saving space and reducing the complexity of this file)

@@ +77,5 @@
> +  dbus_message_unref(msg);
> +}
> +
> +void
> +nsWifiAPScannerDBus::IdentifyAPProperties(DBusMessage* aMsg)

When do you clear mAccessPoints.  From the reading, it looks like this array will continue to grow indefinitely as we scan for APs.

@@ +126,5 @@
> +      dbus_message_iter_get_basic(&variant, &strength);
> +      ap->setSignal(strength);
> +      MOZ_ASSERT(mAccessPoints);
> +      if(!mAccessPoints->AppendObject(ap)) {
> +        return;

Not continue here?

@@ +174,5 @@
> +  uint8_t singleByte;
> +  uint32_t j = 0;
> +  nsresult rv;
> +
> +  for (uint32_t i = 0; i < 16; i+=3) {

Why +3

@@ +181,5 @@
> +    if (NS_FAILED(rv)) {
> +      return;
> +    }
> +
> +    macAddress[j] <<= 4;

Why <<= 4.  Use comments to explain things like this.

@@ +195,5 @@
> +  aAp->setMac(macAddress);
> +}
> +
> +nsresult
> +nsWifiAPScannerDBus::GetInteger(const uint8_t& aChar, uint8_t& aSingleByte)

Isn't this what strtoul kind of does?

@@ +271,5 @@
> +{
> +  nsString interface = NS_LITERAL_STRING("org.freedesktop.NetworkManager");
> +  nsCString path;
> +  path.AssignLiteral("/org/freedesktop/NetworkManager");
> +  nsString funcCall = NS_LITERAL_STRING("GetDevices");

You defined strings above in to different ways.  Be consistent.  Can you do something like:

nsCString path = NS_LITERAL_CSTRING("/org/freedesktop/NetworkManager");

@@ +276,5 @@
> +
> +  nsresult rv = RegisterWithConnection(interface,
> +                                       path, funcCall,
> +                                       &NetworkDevicesNotify);
> +  NS_ENSURE_SUCCESS(rv, rv);

remove

@@ +278,5 @@
> +                                       path, funcCall,
> +                                       &NetworkDevicesNotify);
> +  NS_ENSURE_SUCCESS(rv, rv);
> +
> +  return NS_OK;

return rv;

@@ +298,5 @@
> +  DBusMessage* msg =
> +    dbus_message_new_method_call(NS_ConvertUTF16toUTF8(service).get(),
> +                                 aPath.Data(),
> +                                 NS_ConvertUTF16toUTF8(aInterface).get(),
> +                                 NS_ConvertUTF16toUTF8(aFuncCall).get());

if the dbus* functions all take c strings, you should just use c strings exclusively.  No need for these conversions.

@@ +344,5 @@
> +  return reply;
> +}
> +
> +void
> +nsWifiAPScannerDBus::DropDBusConnection()

No need for a separate method for this (now).  Just move this into the destructor.

::: netwerk/wifi/nsWifiMonitor.cpp
@@ +145,5 @@
>    LOG(("@@@@@ wifi monitor run called\n"));
>  
>    PR_SetCurrentThreadName("Wifi Monitor");
>  
> +  nsresult rv = DoScanDBus();

This should be DoScan().

::: netwerk/wifi/nsWifiMonitor.h
@@ +53,5 @@
>   private:
>    ~nsWifiMonitor();
>  
>    nsresult DoScan();
> +  nsresult DoScanDBus();

You do not need a DoScanDBus()

::: netwerk/wifi/nsWifiScannerDBus.cpp
@@ +10,5 @@
> +
> +using namespace mozilla;
> +
> +nsresult
> +nsWifiMonitor::DoScanDBus()

You should just name this DoScan().  This will conflict with the iwlib one.  So, lets just remove it.

@@ +22,5 @@
> +    NS_ENSURE_SUCCESS(rv, rv);
> +
> +    LOG(("waiting on monitor\n"));
> +    ReentrantMonitorAutoEnter mon(mReentrantMonitor);
> +    mon.Wait(PR_SecondsToInterval(4));

I do not understand this at all.  Why do we need to wait for 4seconds?  A comment here.
Comment 59 bsurender 2012-09-03 11:59:21 PDT
(In reply to Josh Matthews [:jdm] from comment #57)
> Comment on attachment 657350 [details] [diff] [review]
> Bug 668194 geolocation with dbus.
> 
> Review of attachment 657350 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> @@ +347,5 @@
> > +void
> > +nsWifiAPScannerDBus::DropDBusConnection()
> > +{
> > +  if (mConnection) {
> > +    dbus_connection_unref(mConnection);
> 
> I know nothing about the DBUS api, but the presence of an unref with no
> corresponding ref looks suspicious.

The code uses a shared connection via dbus_bus_get. From the dbus API for dbus_connection_unref, "libdbus will own a reference as long as the connection is connected, so you can know that either you don't have the last reference, or it's OK to drop the last reference."

So it's ok to drop the reference and an explicit disconnect does not need to be called.

> 
> @@ +9,5 @@
> > +#include "nsIMutableArray.h"
> > +#include "nsAutoPtr.h"
> > +
> > +#define DBUS_API_SUBJECT_TO_CHANGE
> > +#include <dbus/dbus.h>
> 
> Move this to the source file and forward declare any classes you need.

This is a header file for a .c file which has no class. The .c file includes this header.

> ::: netwerk/wifi/nsWifiScannerDBus.cpp
> @@ +14,5 @@
> > +nsWifiMonitor::DoScanDBus()
> > +{
> > +  nsWifiAPScannerDBus wifiScannerAP;
> > +  nsCOMArray<nsWifiAccessPoint>* accessPoints;
> > +  nsCOMArray<nsWifiAccessPoint> lastAccessPoints;
> 
> Move these down to where they're used.

These are used immediately within a while loop.
Comment 60 bsurender 2012-09-03 12:03:28 PDT
(In reply to Doug Turner (:dougt) from comment #58)
> Comment on attachment 657350 [details] [diff] [review]
> Bug 668194 geolocation with dbus.
> 
> Review of attachment 657350 [details] [diff] [review]:
> -----------------------------------------------------------------
> @@ +77,5 @@
> > +  dbus_message_unref(msg);
> > +}
> > +
> > +void
> > +nsWifiAPScannerDBus::IdentifyAPProperties(DBusMessage* aMsg)
> 
> When do you clear mAccessPoints.  From the reading, it looks like this array
> will continue to grow indefinitely as we scan for APs.

I clear the mAccessPoints in nsWifiScannerDBus::DoScanDBus() at the end of each iteration of the while loop This prevents the array from growing indefinitely. This is in the patch related to this review comment.

nsWifiScannerDBus::DoScanDBus() has now been moved to nsWifiMonitor::DoScan().
Comment 61 bsurender 2012-09-03 12:14:36 PDT
(In reply to Josh Matthews [:jdm] from comment #57)
> Comment on attachment 657350 [details] [diff] [review]
> Bug 668194 geolocation with dbus.
> 
> Review of attachment 657350 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> @@ +57,5 @@
> >  
> >  DEFINES += -DIMPL_NS_NET
> > +
> > +CXXFLAGS += $(TK_CFLAGS) $(MOZ_DBUS_GLIB_CFLAGS)
> > +CXXFLAGS := $(subst -pedantic,,$(CXXFLAGS))
> 
> Combine these, and probably move them into the Linux block?
> 
This cannot be combined because the second line removes the pedantic compiler warning flag from the last arg passed to subst. $(subst from,to,text) takes only one 'text' arg at a time. So the TK_CFLAGS and MOZ...CFLAGS need to be combined before the pedantic option is removed.

I could leave the pedantic compiler warnings option in the CXXFLAGS if it's ok to do so.
Comment 62 bsurender 2012-09-03 12:16:38 PDT
Created attachment 657892 [details] [diff] [review]
Bug 668194 geolocation with dbus.
Comment 63 bsurender 2012-09-03 12:20:17 PDT
Comment on attachment 657892 [details] [diff] [review]
Bug 668194 geolocation with dbus.

I have uploaded the entire patch instead of just the diff from the previous patch to this one because there are a reasonable number of changes.
Comment 64 Doug Turner (:dougt) 2012-09-03 13:25:16 PDT
> I could leave the pedantic compiler warnings option in the CXXFLAGS if it's ok to do so.

You're disabling the warning which is usually a bad idea.

 ifeq ($(OS_ARCH),Linux)
 CPPSRCS += nsWifiScannerDBus.cpp
+OS_INCLUDES += $(MOZ_DBUS_GLIB_CFLAGS)
 else


Don't mess with the CXXFlags, and instead do the above.

Also, fix your warning:

/builds/mozilla-central/netwerk/wifi/nsWifiScannerDBus.cpp:180:14: warning: unused variable ‘appended’ [-Wunused-variable]
Comment 65 bsurender 2012-09-03 14:03:39 PDT
Created attachment 657911 [details] [diff] [review]
Bug 668194 geolocation with dbus.
Comment 66 bsurender 2012-09-03 14:07:52 PDT
Created attachment 657912 [details] [diff] [review]
Bug 668194 geolocation with dbus.
Comment 67 Doug Turner (:dougt) 2012-09-04 09:57:33 PDT
Comment on attachment 657912 [details] [diff] [review]
Bug 668194 geolocation with dbus.

Review of attachment 657912 [details] [diff] [review]:
-----------------------------------------------------------------

::: netwerk/wifi/Makefile.in
@@ +41,5 @@
>  CPPSRCS += nsWifiScannerWin.cpp
>  else
>  ifeq ($(OS_ARCH),Linux)
> +CPPSRCS += nsWifiScannerDBus.cpp
> +OS_INCLUDES += $(MOZ_DBUS_GLIB_CFLAGS) $(TK_CFLAGS)

Do you really need TK_CFLAGS?

::: netwerk/wifi/nsWifiMonitor.cpp
@@ +16,5 @@
>  #include "nsServiceManagerUtils.h"
>  #include "nsComponentManagerUtils.h"
>  #include "mozilla/Services.h"
>  
> +#include "nsWifiScannerDBus.h"

This shouldn't be included.  Take a look at what the other implementations do.  In a nut shell, they implement one method: nsWifiMonitor::DoScan().  I think you can just move your implementation of this into nsWifiScannderDBus.cpp.  This will allow other non-dbus platforms to build.

@@ +248,5 @@
>      return NS_OK;
>  }
> +
> +nsresult
> +nsWifiMonitor::DoScan()

This should not be defined in this file, but instead should be defined in nsWifiScannderDBus*

@@ +257,5 @@
> +
> +  while (mKeepGoing) {
> +    wifiScanner.RegisterWithConnection("org.freedesktop.NetworkManager",
> +                                       "/org/freedesktop/NetworkManager",
> +                                       "GetDevices");

If this method ever fails for whatever reason, you'll return zero access points.  This is fine, but the problem is that you will continually callback dbus.  The error probably will not be resolved, so, in effect, you will spin very tightly hitting dbus.

What you need to do is have RegisterWithConnection return some sort of error code indicating a fatal error.  If that happens, bail out of this loop.

@@ +271,5 @@
> +
> +    nsresult rv = CallWifiListeners(lastAccessPoints, accessPointsChanged);
> +    NS_ENSURE_SUCCESS(rv, rv);
> +
> +    accessPoints->Clear();

Move this to the top of the loop.

::: netwerk/wifi/nsWifiScannerDBus.cpp
@@ +27,5 @@
> +    return;
> +  }
> +
> +  nsString interface = NS_LITERAL_STRING("org.freedesktop.DBus.Properties");
> +  if (interface.EqualsASCII(aInterface)) {

no, you have things in c strings.  no need to use mozilla strings.

Just do:

if(!strcmp("org.freedesktop.DBus.Properties", aInterface)) {....}

@@ +37,5 @@
> +      return;
> +    }
> +  }
> +
> +  DBusMessage* reply = SendWithReply(msg);

Remove this method and just put the implementation write here.  It is only a few lines of code and it does a dbus_message_unref() which is odd for another method to do.

@@ +46,5 @@
> +  interface = NS_LITERAL_STRING("org.freedesktop.NetworkManager");
> +  if (interface.EqualsASCII(aInterface)) {
> +    IdentifyDevices(reply);
> +    dbus_message_unref(reply);
> +    return;

It might be better not to do these early returns here since dbus_message_unref(reply) has to be called in all cases.

@@ +72,5 @@
> +  DBusError err;
> +  dbus_error_init(&err);
> +
> +  DBusMessage* reply = nullptr;
> +  reply = dbus_connection_send_with_reply_and_block(mConnection, aMsg, -1, &err);

instead of -1, you should consider using DBUS_TIMEOUT_USE_DEFAULT instead.

@@ +74,5 @@
> +
> +  DBusMessage* reply = nullptr;
> +  reply = dbus_connection_send_with_reply_and_block(mConnection, aMsg, -1, &err);
> +  if (!reply) {
> +    dbus_error_free(&err);

I think you want something like:


if (dbus_error_is_set(&err)) {
  dbus_error_free(&err);
}

You can put that outside of the if (!reply) block

@@ +78,5 @@
> +    dbus_error_free(&err);
> +    return nullptr;
> +  }
> +
> +  dbus_message_unref(aMsg);

This should be done even if reply is null, right?

@@ +135,5 @@
> +    return;
> +  }
> +
> +  DBusMessageIter arr;
> +  dbus_message_iter_recurse(&args, &arr);

Shouldn't you just use GetDBusIterator() here too?

@@ +137,5 @@
> +
> +  DBusMessageIter arr;
> +  dbus_message_iter_recurse(&args, &arr);
> +
> +  nsWifiAccessPoint* ap = new nsWifiAccessPoint();

This will leak at every early return.  do something like:

nsRefPtr<nsWifiAccessPoint> ap = new nsWifiAccessPoint();

@@ +152,5 @@
> +      DBusMessageIter variant;
> +      dbus_message_iter_recurse(&dict, &variant);
> +
> +      const uint32_t keyLen = strlen(key);
> +      if (keyLen == strlen("Ssid") && !strncmp(key, "Ssid", strlen("Ssid"))) {

just use strncmp().

@@ +161,5 @@
> +        break;
> +      }
> +
> +      if (keyLen == strlen("HwAddress") &&
> +          !strncmp(key, "HwAddress", strlen("HwAddress"))) {

Same - I don't understand the need for both strlen and strncmp.  I am pretty sure you can just use strncmp.

@@ +176,5 @@
> +
> +        uint8_t strength;
> +        dbus_message_iter_get_basic(&variant, &strength);
> +        ap->setSignal(strength);
> +        mAccessPoints.AppendObject(ap);

Do you really want to append here? Are you always guaranteed order of these things in the dictionary?

What I am worried about is if you have the dictionary returning strength first, you add it to the array, then for some reason we faile to parse the mac or ssid.

@@ +203,5 @@
> +    dbus_message_iter_get_basic(&variantMember, &byte);
> +    ssid.Append(byte);
> +  } while (dbus_message_iter_next(&variantMember));
> +
> +  aAp->setSSID(ssid.get(), ssid.Length());

doesn't ssid.Length() just return strlen()?  If that is true, then any ssid that has a null in it will be truncated.  Instead of using mozilla strings here, do this with c strings to ensure that no truncation can happen.

@@ +220,5 @@
> +  dbus_message_iter_get_basic(aVariant, &hwAddress);
> +
> +  const uint32_t MAC_LEN = 6;
> +  uint8_t macAddress[MAC_LEN];
> +  char* token = strtok(hwAddress, ":");;

remove extra semi.

@@ +249,5 @@
> +  dbus_message_iter_recurse(&iter, aIterArray);
> +  return NS_OK;
> +}
> +
> +nsWifiScannerDBus::~nsWifiScannerDBus()

The constructor and destructor should always be paired together in a file.  Move this immediately below the constructor.

::: netwerk/wifi/nsWifiScannerDBus.h
@@ +21,5 @@
> +  nsWifiScannerDBus();
> +  ~nsWifiScannerDBus();
> +
> +  nsCOMArray<nsWifiAccessPoint>*
> +  GetWifiAccessPoints()

This signature worries me.  You are returning an address of a member variable.  If the class, for whatever reason, is destoryed, this array will point to garbage.

Something like:

GetWifiAccessPoints(nsCOMArray<nsWifiAccessPoint>& aAccessPoints)
{
  aAccessPoints = mAccessPoints;
}

@@ +27,5 @@
> +    return &mAccessPoints;
> +  }
> +
> +  /**
> +   * Constructs a new dbus message and queues the message to be sent.

This comment is wrong now -- doesn't it block now for a reply (it doesn't just queue). quickly skimming the code, this comment is definitely not telling the whole story.

@@ +35,5 @@
> +   * @param        aPath, a path corresponding to a remote object.
> +   * @param        aFuncCall, the function call, e.g. "GetDevices"
> +   *               to the remote object.
> +   */
> +  void RegisterWithConnection(const char* aInterface,

This is a pretty odd name.

@@ +43,5 @@
> +private:
> +
> +  /**
> +   * Queues a given DBus message with a pending call. This is used to receive
> +   * the reply/ message returned.

replay/message
Comment 68 bsurender 2012-09-05 14:14:53 PDT
Created attachment 658623 [details] [diff] [review]
Bug 668194 geolocation with dbus.

I have uploaded the entire patch because I've made some flow changes.
Comment 69 Doug Turner (:dougt) 2012-09-06 11:56:23 PDT
Comment on attachment 658623 [details] [diff] [review]
Bug 668194 geolocation with dbus.

Review of attachment 658623 [details] [diff] [review]:
-----------------------------------------------------------------

::: netwerk/wifi/nsWifiScannerDBus.cpp
@@ +7,5 @@
> +
> +namespace mozilla {
> +
> +nsWifiScannerDBus::
> +nsWifiScannerDBus(nsCOMArray<nsWifiAccessPoint>& aAccessPoints)

one line for the constructor.  I think you are trying for the 80 column rule, but I don't care that much -- especially if it means if you have to break up a line like this.

@@ +35,5 @@
> +                                 aPath, aInterface, aFuncCall);
> +  if (!msg) {
> +    return NS_ERROR_FAILURE;
> +  }
> +

You use these two lines in both seconds of the if/else block:

DBusMessageIter argsIter;
dbus_message_iter_init_append(msg, &argsIter);

Put them outside the if/else() block.

@@ +59,5 @@
> +    const char* param = "";
> +    if (!dbus_message_iter_append_basic(&argsIter, DBUS_TYPE_STRING, &param)) {
> +      return NS_ERROR_FAILURE;
> +    }
> +  }

Do you handle anything else here?  If not add:

 else {
   return NS_ERROR_FAILURE;
 }

@@ +70,5 @@
> +                                                    DBUS_TIMEOUT_USE_DEFAULT,
> +                                                    &err);
> +  if (dbus_error_is_set(&err)) {
> +    dbus_error_free(&err);
> +    if (!strcmp(aFuncCall, "GetAccessPoints")) {

You need to add a comment here as to why you are special casing GetAccessPoints().

@@ +85,5 @@
> +  } else if (!strcmp(aFuncCall, "GetAccessPoints")) {
> +    rv = IdentifyAccessPoints(reply);
> +  } else if (!strcmp(aFuncCall, "GetAll")) {
> +    rv = IdentifyAPProperties(reply);
> +  }

do you handle anything else?  If not then add:

else {
  rv = NS_ERROR_FAILURE;
}

Or set the declaration of rv to NS_ERROR_FAILURE.

@@ +134,5 @@
> +  dbus_message_iter_get_basic(&variantIter, &deviceType);
> +
> +  nsresult rv = NS_OK;
> +  const uint32_t NM_DEVICE_TYPE_WIFI = 2;
> +  if (deviceType == NM_DEVICE_TYPE_WIFI) {

you need to add a comment here.  Why the special value 2; where is it defined?  Having a comment in the header doesn't count.

@@ +221,5 @@
> +  DBusMessageIter variantMember;
> +  dbus_message_iter_recurse(aVariant, &variantMember);
> +
> +  const uint32_t MAX_SSID_LEN = 32;
> +  char ssid[MAX_SSID_LEN + 1];

why the extra 1 char?  I do not think that you need to null terminated this ssid (it can have embedded nulls)

@@ +222,5 @@
> +  dbus_message_iter_recurse(aVariant, &variantMember);
> +
> +  const uint32_t MAX_SSID_LEN = 32;
> +  char ssid[MAX_SSID_LEN + 1];
> +  memset(ssid, '\0', MAX_SSID_LEN + 1);

use ArrayLength(ssid) instead.

@@ +233,5 @@
> +    dbus_message_iter_get_basic(&variantMember, &ssid[i]);
> +    i++;
> +  } while (dbus_message_iter_next(&variantMember) && i < MAX_SSID_LEN);
> +
> +  ssid[MAX_SSID_LEN] = '\0';

not needed.

@@ +234,5 @@
> +    i++;
> +  } while (dbus_message_iter_next(&variantMember) && i < MAX_SSID_LEN);
> +
> +  ssid[MAX_SSID_LEN] = '\0';
> +  aAp->setSSID(ssid, strlen(ssid));

strlen() is wrong.

just aAp->setSSID(ssid, i);

should work.

@@ +247,5 @@
> +  }
> +
> +  // hwAddress format is XX:XX:XX:XX:XX:XX. Need to convert to XXXXXX format.
> +  char* hwAddress;
> +  dbus_message_iter_get_basic(aVariant, &hwAddress);

test for error here and/or hwAddress for null.

@@ +252,5 @@
> +
> +  const uint32_t MAC_LEN = 6;
> +  uint8_t macAddress[MAC_LEN];
> +  char* token = strtok(hwAddress, ":");
> +  for (uint32_t i = 0; i < MAC_LEN; i++) {

use ArrayLength(macAddress) instead of MAC_LEN.

@@ +297,5 @@
> +                                    "/org/freedesktop/NetworkManager",
> +                                    "GetDevices");
> +    NS_ENSURE_SUCCESS(rv, rv);
> +    if (!accessPoints.Count()) {
> +      break;

Do not break if there are no access points.  Only break if there is an error.

::: netwerk/wifi/nsWifiScannerDBus.h
@@ +20,5 @@
> +  nsWifiScannerDBus(nsCOMArray<nsWifiAccessPoint>& aAccessPoints);
> +  ~nsWifiScannerDBus();
> +
> +  /**
> +   * Constructs a new dbus message, sends the message via dbus to the network

Honestly, the comments here are nice, but they are probably not required at this level of detail.  This is a very private header and the details are in the code, not the header declaration.  I am tempted to tell you just to remove all of them.

@@ +32,5 @@
> +   * @return       nsresult indicates failure to successfully communicate
> +   *               with network manager via dbus to get a return message from
> +   *               the remote object.
> +   */
> +  nsresult CommunicateWithNM(const char* aInterface,

How about instead of CommunicateWithNM(), you call this SendMessage instead.

@@ +88,5 @@
> +  nsresult IdentifyAccessPoints(DBusMessage* aMsg);
> +
> +  /**
> +   * Iterates through the dbus message received to retrieve the properties
> +   * aquired for an access point.

acquired for an access point

@@ +90,5 @@
> +  /**
> +   * Iterates through the dbus message received to retrieve the properties
> +   * aquired for an access point.
> +   * Enumerates through list of properties for the access point and stores
> +   * the SSID, SSID length, MAC address and signal strength.

stores them where?

@@ +133,5 @@
> +   */
> +  nsresult GetDBusIterator(DBusMessage* aMsg, DBusMessageIter* aIterArray);
> +
> +  DBusConnection* mConnection;
> +  nsCOMArray<nsWifiAccessPoint>& mAccessPoints;

This shouldn't be a reference.
Comment 70 bsurender 2012-09-06 16:36:14 PDT
Created attachment 659043 [details] [diff] [review]
Bug 668194 geolocation with dbus.
Comment 71 bsurender 2012-09-06 16:44:51 PDT
Created attachment 659045 [details] [diff] [review]
Bug 668194 geolocation with dbus.
Comment 72 Doug Turner (:dougt) 2012-09-10 09:13:46 PDT
Comment on attachment 659045 [details] [diff] [review]
Bug 668194 geolocation with dbus.

Review of attachment 659045 [details] [diff] [review]:
-----------------------------------------------------------------

::: netwerk/wifi/nsWifiScannerDBus.h
@@ +20,5 @@
> +  nsWifiScannerDBus(nsCOMArray<nsWifiAccessPoint>* aAccessPoints);
> +  ~nsWifiScannerDBus();
> +
> +  /**
> +   * @param        aInterface, an interface corresponding to a method call to

Please remove all method comments here.  They are verbose and don't really explain much.

@@ +130,5 @@
> +   */
> +  nsresult GetDBusIterator(DBusMessage* aMsg, DBusMessageIter* aIterArray);
> +
> +  DBusConnection* mConnection;
> +  nsCOMArray<nsWifiAccessPoint>* mAccessPoints;

shouldn't this just be:
   nsCOMArray<nsWifiAccessPoint> mAccessPoints;
Comment 73 bsurender 2012-09-10 11:34:23 PDT
(In reply to Doug Turner (:dougt) from comment #72)
> Comment on attachment 659045 [details] [diff] [review]
> Bug 668194 geolocation with dbus.
> 
> Review of attachment 659045 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: netwerk/wifi/nsWifiScannerDBus.h
> @@ +20,5 @@
> > +  nsWifiScannerDBus(nsCOMArray<nsWifiAccessPoint>* aAccessPoints);
> > +  ~nsWifiScannerDBus();
> > +
> > +  /**
> > +   * @param        aInterface, an interface corresponding to a method call to
> 
> Please remove all method comments here.  They are verbose and don't really
> explain much.
> 
> @@ +130,5 @@
> > +   */
> > +  nsresult GetDBusIterator(DBusMessage* aMsg, DBusMessageIter* aIterArray);
> > +
> > +  DBusConnection* mConnection;
> > +  nsCOMArray<nsWifiAccessPoint>* mAccessPoints;
> 
> shouldn't this just be:
>    nsCOMArray<nsWifiAccessPoint> mAccessPoints;

I create the nsCOMArray of access points in nsWifiMonitor::DoScan and I pass that to the nsWifiScannerDBus constructor hence, the ptr.

OR 

I could change this to what you suggested and have a GetAP() function with a reference.
Comment 74 Doug Turner (:dougt) 2012-09-10 12:37:50 PDT
I am talking about something like.


diff --git a/netwerk/wifi/nsWifiScannerDBus.cpp b/netwerk/wifi/nsWifiScannerDBus.cpp
--- a/netwerk/wifi/nsWifiScannerDBus.cpp
+++ b/netwerk/wifi/nsWifiScannerDBus.cpp
@@ -2,17 +2,17 @@
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 #include "nsWifiScannerDBus.h"
 #include "nsWifiAccessPoint.h"
 
 namespace mozilla {
 
-nsWifiScannerDBus::nsWifiScannerDBus(nsCOMArray<nsWifiAccessPoint> *aAccessPoints)
+nsWifiScannerDBus::nsWifiScannerDBus(nsCOMArray<nsWifiAccessPoint>& aAccessPoints)
 : mAccessPoints(aAccessPoints)
 {
   mConnection = dbus_bus_get(DBUS_BUS_SYSTEM, nullptr);
   MOZ_ASSERT(mConnection);
   dbus_connection_set_exit_on_disconnect(mConnection, false);
   MOZ_COUNT_CTOR(nsWifiScannerDBus);
 }
 
@@ -218,17 +218,17 @@ nsWifiScannerDBus::IdentifyAPProperties(
 
         uint8_t strength;
         dbus_message_iter_get_basic(&variant, &strength);
         ap->setSignal(strength);
       }
     } while (dbus_message_iter_next(&dict));
   } while (dbus_message_iter_next(&arr));
 
-  mAccessPoints->AppendObject(ap);
+  mAccessPoints.AppendObject(ap);
   return NS_OK;
 }
 
 nsresult
 nsWifiScannerDBus::StoreSsid(DBusMessageIter* aVariant, nsWifiAccessPoint* aAp)
 {
   if (dbus_message_iter_get_arg_type(aVariant) != DBUS_TYPE_ARRAY) {
     return NS_ERROR_FAILURE;
@@ -300,19 +300,20 @@ nsWifiScannerDBus::GetDBusIterator(DBusM
 }
 
 } // mozilla
 
 nsresult
 nsWifiMonitor::DoScan()
 {
   nsCOMArray<nsWifiAccessPoint> accessPoints;
-  mozilla::nsWifiScannerDBus wifiScanner(&accessPoints);
   nsCOMArray<nsWifiAccessPoint> lastAccessPoints;
 
+  mozilla::nsWifiScannerDBus wifiScanner(accessPoints);
+
   while (mKeepGoing) {
     accessPoints.Clear();
     nsresult rv =
       wifiScanner.SendMessage("org.freedesktop.NetworkManager",
                               "/org/freedesktop/NetworkManager",
                               "GetDevices");
     NS_ENSURE_SUCCESS(rv, rv);
     bool accessPointsChanged = !AccessPointsEqual(accessPoints,
diff --git a/netwerk/wifi/nsWifiScannerDBus.h b/netwerk/wifi/nsWifiScannerDBus.h
--- a/netwerk/wifi/nsWifiScannerDBus.h
+++ b/netwerk/wifi/nsWifiScannerDBus.h
@@ -12,17 +12,17 @@
 
 class nsWifiAccessPoint;
 
 namespace mozilla {
 
 class nsWifiScannerDBus MOZ_FINAL
 {
 public:
-  nsWifiScannerDBus(nsCOMArray<nsWifiAccessPoint>* aAccessPoints);
+  nsWifiScannerDBus(nsCOMArray<nsWifiAccessPoint>& aAccessPoints);
   ~nsWifiScannerDBus();
 
   /**
    * @param        aInterface, an interface corresponding to a method call to
    *               the remote object.
    * @param        aPath, a path corresponding to a remote object.
    * @param        aFuncCall, the function call, e.g. "GetDevices"
    *               to the remote object.
@@ -126,14 +126,14 @@ private:
    * through the child values.
    *
    * @param        aMsg, message which contains the arguments.
    * @return       aIterArray, sub-iterator.
    */
   nsresult GetDBusIterator(DBusMessage* aMsg, DBusMessageIter* aIterArray);
 
   DBusConnection* mConnection;
-  nsCOMArray<nsWifiAccessPoint>* mAccessPoints;
+  nsCOMArray<nsWifiAccessPoint> mAccessPoints;
 };
 
 } // mozilla
 
 #endif // NSWIFIAPSCANNERDBUS_H_
Comment 75 Doug Turner (:dougt) 2012-09-10 12:40:02 PDT
or better:  Change wifiScanner.SendMessage() to take a nsCOMArray<nsWifiAccessPoint> reference.  In this way, you will not need to have a mAccessPoints at all.
Comment 76 bsurender 2012-09-10 13:15:10 PDT
Created attachment 659831 [details] [diff] [review]
Bug 668194 geolocation with dbus.
Comment 77 bsurender 2012-09-10 13:18:27 PDT
Created attachment 659832 [details] [diff] [review]
Bug 668194 geolocation with dbus. Latest changes only.

This diff file is just a list of the latest set of changes for your convenience.
Comment 78 Doug Turner (:dougt) 2012-09-10 13:19:21 PDT
see comment 39
Comment 79 bsurender 2012-09-10 16:12:05 PDT
(In reply to Doug Turner (:dougt) from comment #78)
> see comment 39

I think you meant comment 75. 

Wrt comment 75, if I change nsWifiScannerDBus::SendMessage() to take an nsCOMArray<nsWifiAccessPoint> then I will have to change each of the functions that call SendMessage() to pass in the array of access points. These functions are nsWifiScannerDBus::IdentifyDevices, nsWifiScannerDBus::IdentifyDeviceType, nsWifiScannerDBus::IdentifyAccessPoints and finally nsWifiScannerDBus::IdentifyAPProperties. nsWifiScannerDBus::IdentifyAPProperties is the function within which the access point is finally added to the array. IdentifyDevices, IdentifyDeviceType and IdentifyAccessPoints call SendMessage from within them but they do not need to know about the access points array. They will be taking an extra parameter and passing it down to the next level till it hits the IdentifyAPProperties function. How should I proceed?
Comment 80 bsurender 2012-09-10 19:19:03 PDT
Created attachment 659935 [details] [diff] [review]
Bug 668194 geolocation with dbus.
Comment 81 Doug Turner (:dougt) 2012-09-10 20:54:32 PDT
Comment on attachment 659935 [details] [diff] [review]
Bug 668194 geolocation with dbus.

Review of attachment 659935 [details] [diff] [review]:
-----------------------------------------------------------------

please fix up the nits, and attach a new patch that has a checkin comment.

::: netwerk/wifi/nsWifiScannerDBus.h
@@ +18,5 @@
> +{
> +public:
> +  nsWifiScannerDBus(nsCOMArray<nsWifiAccessPoint>* aAccessPoints);
> +  ~nsWifiScannerDBus();
> +  nsresult SendMessage(const char* aInterface,

Nit: Make SendMessage() private.  Add a new method called nsresult Scan();  Have the DoScan() call that.  In this way, you hide some of the details of the DBus from the DoScan method

@@ +23,5 @@
> +                       const char* aPath,
> +                       const char* aFuncCall);
> +
> +private:
> +

nit: extra new line after private.
Comment 82 bsurender 2012-09-10 21:40:43 PDT
Created attachment 659963 [details] [diff] [review]
Bug 668194 geolocation with dbus.
Comment 83 bsurender 2012-09-10 21:44:50 PDT
Comment on attachment 659963 [details] [diff] [review]
Bug 668194 geolocation with dbus.

Comments have been addressed. Please check-in.
Comment 84 Doug Turner (:dougt) 2012-09-11 13:37:28 PDT
Comment on attachment 659963 [details] [diff] [review]
Bug 668194 geolocation with dbus.

https://hg.mozilla.org/integration/mozilla-inbound/rev/7612ff8a7fce
Comment 85 Rubin110 2012-09-11 13:42:59 PDT
If I wanted to request support for Wicd, should I pester for it here or start a new issue?
Comment 86 Doug Turner (:dougt) 2012-09-11 14:22:10 PDT
new bug.  assign it to me for now.  thx!
Comment 87 Ryan VanderMeulen [:RyanVM] 2012-09-11 18:43:29 PDT
https://hg.mozilla.org/mozilla-central/rev/7612ff8a7fce
Comment 88 Rubin110 2012-09-19 14:57:09 PDT
Debian Sid 64bit, XFCE 4.8.0.3, Network Manager 0.9.4.0-6, Firefox Nightly 18.0a1 (2012-09-19).

Fix verified, tested using example URLs provided in the description of the issue. I was able to pull up a map with a pointer pretty much over the building I'm currently in while running wifi (and connected to an AP). If I kill my wifi radio and connect over ethernet, the pointer ends up in the center of the city I'm currently in and not anywhere near where my actual location is.

Note: This fix only works with Network Manager, I was still able to reproduce the problem when using Wicd. As per Doug Turner's recommendation, I've created bug 790431 regarding this.

I would recommend assigning this issue to the reporter for further testing in Ubuntu land before marking as fix verified and closing out.
Comment 89 Nikos Roussos [:nikos] 2012-09-20 10:20:59 PDT
I'm also confirming that it works great on Nightly, getting the expected accuracy.&#013;&#013;Fedora 17 (64bit), Gnome with Network Manager 0.9.4.0
Comment 90 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-09-20 10:24:48 PDT
Doug, can this bug be marked verified and should we update the status flags? Also, in regards to the in-litmus flag, we already have a geolocation testcase in Mozmill. Is there anything we need to do to extend that testcase to cover this scenario?
Comment 91 Doug Turner (:dougt) 2012-09-20 12:13:02 PDT
> can this bug be marked verified and should we update the status flags?

Yup.

> in-litmus flag, we already have a geolocation testcase

Did these test ever work?  an outside contributor found and tested this issue.  


I'd make sure the litmus tests ensured that the accuracy of the geolocation response was under 100m.
Comment 92 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-09-20 12:17:34 PDT
(In reply to Doug Turner (:dougt) from comment #91)
> > can this bug be marked verified and should we update the status flags?
> 
> Yup.

Done.

> Did these test ever work?  an outside contributor found and tested this
> issue. 

The tests are pretty high level, in that they check to make sure geolocation is working, not the accuracy of the geolocation.

> I'd make sure the litmus tests ensured that the accuracy of the geolocation
> response was under 100m.

Do we have a minimized testcase which reports accuracy?

Sidebar, Litmus is dead, we use MozTrap now.

Note You need to log in before you can comment on or make changes to this bug.