Add support for Gonk to NetworkGeolocationProvider.js

RESOLVED FIXED in Firefox 18

Status

()

defect
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: mwu, Assigned: dougt)

Tracking

unspecified
mozilla19
All
Gonk (Firefox OS)
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(blocking-basecamp:+, firefox18 fixed, firefox19 fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

Reporter

Description

7 years ago
No description provided.
Reporter

Comment 1

7 years ago
Posted patch WIP (obsolete) — Splinter Review
WIP - apparently DOMWifiManager can't be used from a js component context so this doesn't work.
Assignee

Updated

7 years ago
Assignee: nobody → doug.turner
Here's a write-up of what I think needs to happen to be able to access the necessary information from C++:

In order to get at wifi information from chrome, things should be
relatively simple.

The relevant source code is in dom/wifi. You probably want to look at
WifiWorker.js (the chrome implementation) and nsIWifi.idl. The other
main file is DOMWifiManager.js (the implementation of the DOM API).

We already have an API, nsIWifi, which is accessible via
SystemWorkerManager::GetInterface(nsIWifi). In turn, the
SystemWorkerManager is available via its contract ID
(@mozilla.org/telephony/system-worker-manager;1). Because we're
guaranteed to only ever have one chrome process, we can simply add the
APIs needed to nsIWifi. I would suggest using callbacks for things that
are asynchronous in the DOM API (for example, getNetworks) instead of
trying do anything fancy. My understanding is that geolocation only
needs the available networks (including information like signal
strength). In order to do this, I would add:

interface nsIWifiScanResult : nsISupports {
  readonly attribute DOMString SSID;
  readonly attribute DOMString BSSID;

  const int32_t WPA2_PSK = ...;
  const int32_t WEP = ...;
  readonly attribute uint32_t capabilities;

  readonly attribute uint32_t signalStrength;
  readonly attribute uint32_t relSignalStrength;
  readonly attribute boolean connected;
};

interface nsIWifiScanResultsReady : nsISupports {
  void onready(in int32_t count, [array, size_is(count)] in
nsIWifiScanResult results);
  void onfailure();
};

and then:

interface nsIWifi : nsISupports {
  // ...
  void getScanResults(in nsIWifiScanResultsReady callback);
};

The implementation can then use a similar implementation as the
getNetworks
(http://hg.mozilla.org/mozilla-central/file/fd4d9c386f97/dom/wifi/WifiWorker.js#l2014)
method, but with the proper conversion to call the
nsIWifiScanResultsReady functions instead of the DOMRequest stuff when
the results come back.
Assignee

Comment 3

7 years ago
Posted patch patch v.1 (obsolete) — Splinter Review
Attachment #652985 - Attachment is obsolete: true
Attachment #669583 - Flags: review?(josh)
Comment on attachment 669583 [details] [diff] [review]
patch v.1

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

I'd like some answers before this gets r+.

::: netwerk/wifi/nsWifiAccessPoint.h
@@ +32,5 @@
>    }
>  
> +  void setMacRaw(const char* aString)
> +  {
> +    memcpy(mMac, aString, 18);

mozilla::ArrayLength?

@@ +56,5 @@
>    }
>  
> +  void setSSIDRaw(const char* aSSID, unsigned long len) {
> +    memcpy(mSsid, aSSID, 33);
> +    mSsidLen = PR_MIN(len, 33);

mozilla::ArrayLength for these as well?

::: netwerk/wifi/nsWifiMonitor.cpp
@@ +25,5 @@
>  #if defined(PR_LOGGING)
>  PRLogModuleInfo *gWifiMonitorLog;
>  #endif
>  
> +#ifdef MOZ_WIDGET_GONK

There's enough code here that I would really like to see this implementation in a separate file.

@@ +83,5 @@
> +      break;
> +    }
> +  }
> +
> +  if(mListeners.Length() == 0) {

nit: space before (

@@ +158,5 @@
> +  nsTArray<nsIWifiAccessPoint*> ac;
> +  uint32_t resultCount = accessPoints.Count();
> +  for (uint32_t i = 0; i < resultCount; i++) {
> +    ac.AppendElement(accessPoints[i]);
> +  }

Why are we a) duplicating the contents of accessPoints, b) not addrefing them?

::: netwerk/wifi/nsWifiMonitor.h
@@ +70,5 @@
>  
>  };
> +#else
> +
> +#include "nsIWifi.h"

Similarly, other file please.
Attachment #669583 - Flags: review?(josh) → review-
Also I suspect you've got interfaces missing from this, like nsIWifiScanResultsReady.
(In reply to Josh Matthews [:jdm] from comment #6)
> Also I suspect you've got interfaces missing from this, like
> nsIWifiScanResultsReady.

Those were added in bug 792952.
Depends on: 792952
Assignee

Comment 8

7 years ago
> There's enough code here that I would really like to see this implementation in a separate file.

I was on the fence about this.  I wasn't convinced.

I'll split off the gonk implementation into its own source file, but the header file is pretty clean.

> Why are we a) duplicating the contents of accessPoints, b) not addrefing them?

'ac' is a local variable and owns a weak reference back to the actual access points held in 'accessPoints'.  Were doing this because the 'OnChange' listener requires a nsIWifiAccessPoint** and count.

patching coming up...
Assignee

Comment 9

7 years ago
Posted patch patch v.2Splinter Review
Attachment #669583 - Attachment is obsolete: true
Attachment #669712 - Flags: review?(josh)
Attachment #669712 - Flags: review?(josh) → review+
https://hg.mozilla.org/mozilla-central/rev/e76aae8e80c9
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
Assignee

Comment 12

7 years ago
We probably want this on b2g v.1.
blocking-basecamp: --- → ?
blocking-basecamp: ? → +
Assignee

Updated

7 years ago
Depends on: 799391
Assignee

Updated

7 years ago
No longer depends on: 799391
You need to log in before you can comment on or make changes to this bug.