Closed Bug 477557 Opened 13 years ago Closed 13 years ago

Implement geolocation provider on windows mobile using wince geo api

Categories

(Core :: DOM: Geolocation, enhancement)

All
Windows CE
enhancement
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: ndaversa, Assigned: ndaversa)

References

Details

(Keywords: fixed1.9.1)

Attachments

(1 file, 6 obsolete files)

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.1b2) Gecko/20081201 Firefox/3.1b2 (.NET CLR 3.5.30729)
Build Identifier: 

Add Geolocation Support to Fennec for Windows Mobile by creating a WinMobileLocationProvider similar to the MaemoLocationProvider.

Reproducible: Always
OS: Other → Windows Mobile 6 Professional
Hardware: Other → ARM
OS: Windows Mobile 6 Professional → Windows CE
Hardware: ARM → All
The bits in ClassInfo are probably not needed (dougt mentioned an automated way of generating those) but they have been hardcoded here for a quick and dirty way to get this patch going.

I believe some work needs to be done on bug 461736 in order for this code to be called properly.  Currently in nsGeolocation.cpp the nsGeolocation::WatchPosition does not handle the prompt or request stuff properly, so the device is never started.

However I have tested this code by invoking the WinMobileLocationProvider::Startup() method in nsGeolocation.cpp and the Geolocation data that comes through is accurate and the LocationUpdated method is called at the correct interval (currently set to 1 second, as WinMobile doesn't offer a LocationChanged callback API - from what I can find).

Some changes will be necessary here once the Geolocation API stuff is updated (I believe dougt said it would be this week), however I think this patch is fairly close from a functionality perspective (comments on design/style are always welcomed).
Assignee: nobody → ninodaversa
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Duplicate of this bug: 476981
Summary: Add Geolocation Support to Fennec for Windows Mobile → Implement geolocation provider on windows mobile using wince geo api
Not sure if the bits about ClassInfo are needed, or if they can be generated in another way, for now I have hardcoded them in.

Currently this patch is using two classes to handle the geoposition data. Experimentation was done to unify them into one class but I ran into problems trying to map two interfaces onto one class. I know dougt has asked jst for some input on flattening our interfaces so we can unify this into one class, but in the meantime I will leave this patch with the two distinct classes, if an when it becomes possible I have the code written for the unified class.

As always I welcome any criticisms/comments.
Attachment #361223 - Attachment is obsolete: true
Attachment #363254 - Flags: review?(doug.turner)
Comment on attachment 363254 [details] [diff] [review]
Add Geolocation Support to Windows Mobile v2

looking good.  here are some comments:

+        nsGeoPositionCoords(double aLat, double aLong, double aAlt, double aHError, double aVError, double aHeading, double aSpeed)
+        : mLat(aLat), mLong(aLong), mAlt(aAlt), mHError(aHError), mVError(aVError), mHeading(aHeading), mSpeed(aSpeed){};

+    nsGeoPosition(double aLat, double aLong, double aAlt, double aHError, double aVError, double aHeading, double aSpeed, long long aTimestamp): mTimestamp(aTimestamp){
+       mCoords = new nsGeoPositionCoords(aLat, aLong, aAlt, aHError, aVError, aHeading, aSpeed);
+    };



Try keeping the line length down by putting returns after the commas


+       mCoords = new nsGeoPositionCoords(aLat, aLong, aAlt, aHError, aVError, aHeading, aSpeed);


Add an assertion. if this fails all bets are off.


+: mGPSDevice(NULL)


nsnull


+        mGPSInst = LoadLibraryW(L"gpsapi.dll");

Test for error.  Also test each of the GetProcAddress() calls.  Early return.  something like:

if (!mOpenDevice || !mCloseDevice || !mGetPosition || !mGetDeviceState)
    return NS_ERROR_FAILURE;


+        mUpdateTimer = do_CreateInstance("@mozilla.org/timer;1", &rv);
+
+        if (NS_SUCCEEDED(rv)) {


Do this the other way around.  If there is a failure, return.


+    FreeLibrary(mGPSInst);

Is it okay to free this library?  I recall some issues with freeing libraries that were currently in use.



+#include <windows.h>
+#undef GetMessage //prevent collision with nsDOMGeoPositionError::GetMessage(nsAString & aMessage)
+#include <gpsapi.h>


nit: Add some white space around that undef,


+  static void WinMobileLocationProvider::LocationUpdated(nsITimer *aTimer, void *aClosure);

Just static void LocationUpdated(nsITimer *aTimer, void *aClosure);


+            const PRUint32 kDelay = 3000; //update location every 3 seconds


How about a preference?  Other providers could also honor this:  geo.default.update or if you have a better name


+            NS_IF_ADDREF(this);


Why do you do this?



+void WinMobileLocationProvider::LocationUpdated(nsITimer *aTimer, void *aClosure)


I am somewhat worried about doing this on the main thread (via a timer).  How long does this take?  Specifically the call to |mGetPosition|?

also, why do we call it 10 times before giving up?  why not once?
Attachment #363254 - Flags: review?(doug.turner) → review-
(In reply to comment #4)
> (From update of attachment 363254 [details] [diff] [review])
> looking good.  here are some comments:
> 
> +        nsGeoPositionCoords(double aLat, double aLong, double aAlt, double
> aHError, double aVError, double aHeading, double aSpeed)
> +        : mLat(aLat), mLong(aLong), mAlt(aAlt), mHError(aHError),
> mVError(aVError), mHeading(aHeading), mSpeed(aSpeed){};
> 
> +    nsGeoPosition(double aLat, double aLong, double aAlt, double aHError,
> double aVError, double aHeading, double aSpeed, long long aTimestamp):
> mTimestamp(aTimestamp){
> +       mCoords = new nsGeoPositionCoords(aLat, aLong, aAlt, aHError, aVError,
> aHeading, aSpeed);
> +    };
> 
> 
> 
> Try keeping the line length down by putting returns after the commas

Done.

> 
> +       mCoords = new nsGeoPositionCoords(aLat, aLong, aAlt, aHError, aVError,
> aHeading, aSpeed);
> 
> 
> Add an assertion. if this fails all bets are off.
> 

Done.

> +: mGPSDevice(NULL)
> 
> 
> nsnull
> 

Done.

> +        mGPSInst = LoadLibraryW(L"gpsapi.dll");
> 
> Test for error.  Also test each of the GetProcAddress() calls.  Early return. 
> something like:
> 
> if (!mOpenDevice || !mCloseDevice || !mGetPosition || !mGetDeviceState)
>     return NS_ERROR_FAILURE;
> 

Done.
 
> +        mUpdateTimer = do_CreateInstance("@mozilla.org/timer;1", &rv);
> +
> +        if (NS_SUCCEEDED(rv)) {
> 
> 
> Do this the other way around.  If there is a failure, return.
> 

Done.

> +    FreeLibrary(mGPSInst);
> 
> Is it okay to free this library?  I recall some issues with freeing libraries
> that were currently in use.
> 

Done? This is being called in the shutdown after all the timer's and code calling any functions in the library have ceased. I think it's okay to leave this, unless you feel otherwise.
 
> 
> +#include <windows.h>
> +#undef GetMessage //prevent collision with
> nsDOMGeoPositionError::GetMessage(nsAString & aMessage)
> +#include <gpsapi.h>
> 
> 
> nit: Add some white space around that undef,
> 

Done.

> +  static void WinMobileLocationProvider::LocationUpdated(nsITimer *aTimer,
> void *aClosure);
> 
> Just static void LocationUpdated(nsITimer *aTimer, void *aClosure);

Done. 

> 
> +            const PRUint32 kDelay = 3000; //update location every 3 seconds
> 
> 
> How about a preference?  Other providers could also honor this: 
> geo.default.update or if you have a better name
> 

Done.
 
> +            NS_IF_ADDREF(this);
> 
> 
> Why do you do this?
> 

Done. I thought I needed it cause i passed the reference to the timer, but I didn't. removed.

> 
> +void WinMobileLocationProvider::LocationUpdated(nsITimer *aTimer, void
> *aClosure)
> 
> 
> I am somewhat worried about doing this on the main thread (via a timer).  How
> long does this take?  Specifically the call to |mGetPosition|?
> 
> also, why do we call it 10 times before giving up?  why not once?

Done. Okay so I have removed the 10 attempts, now I just make one attempt, if I don't get what I want I do an early return. Does this alleviate the need for it to move off the main thread? or would you still like me to move it to another thread?


Also note I'm attaching another patch for the pref I added to mobile.js in mobile-browser tree, not sure what the proper procedure is here so if i'm doing this wrong, someone please let me know. Thanks!
Attachment #363254 - Attachment is obsolete: true
Attachment #363513 - Flags: review?(doug.turner)
This is supplemental to my v3 patch, it adds the pref to the mobile.js
Attachment #363514 - Flags: review?(doug.turner)
Comment on attachment 363513 [details] [diff] [review]
Add Geolocation Support to Windows Mobile v3

maximum age magic value?  could you add a comment.

+    provider->mGetPosition(provider->mGPSDevice, &pos, 500000, 0);

align this so that all of the variables are lined up:

+    nsRefPtr<nsGeoPosition> somewhere = new nsGeoPosition(pos.dblLatitude,
+        pos.dblLongitude,
+        (double)pos.flAltitudeWRTSeaLevel,
+        (double)pos.flHorizontalDilutionOfPrecision,
+        (double)pos.flVerticalDilutionOfPrecision,
+        (double)pos.flHeading,
+        (double)pos.flSpeed,
+        time);




no printfs in production code:

+        printf ("lat = %.3lf    long = %.3lf    alt = %.3lf    heading = %.3lf    speed = %.3lf    time = %lld\n", pos.dblLatitude, pos.dblLongitude, (double)pos.flAltitudeWRTSeaLevel, (double)pos.flHeading, (double)pos.flSpeed, time);




useless.  not need to set this to null first:

+            mGPSDevice = nsnull;
+            mGPSDevice = mOpenDevice(NULL, NULL, NULL, 0);

what if mGPSDevice is null after calling mOpenDevice?

does returning early if the CreateInstance cause any problems?  leaking mGPSDevice

allign these too:

mUpdateTimer->InitWithFuncCallback(WinMobileLocationProvider::LocationUpdated,
+                        (void*)this, kUpdate, nsITimer::TYPE_REPEATING_SLACK);   


Also, i think you can just use LocationUpdated as the symbol (instead of mentioning the class::)

You want a default kUpdate value in case it isn't specified in preferences.  a good idea, is to have a default value in code, and only if the preference is set, do you modify the default.  This avoids you having to add any pref to mobile-browser. :-)

In WinMobileLocationProvider::Startup, just return early by doing:

     if (!mGPSInst) {
          you may want an assertion too.  this shouldn't happen, right?
          return NS_ERROR_FAILURE;
     }


fwiw, i am going to change this so that it is a strong references.  (make it a nsCOMPtr in the header) You should probabably do the same:

+    mCallback = callback; // weak ref

You must free it in Shutdown (mCallBack == nsnull)

Also, you might want to only allow Watch to be called once:

{
    if (mCallback) return ns_ok;

    mCallback = callback; // weak ref
    return NS_OK;
}


mLastPosition is not used.  you can removed it.

closer!
Attachment #363513 - Flags: review?(doug.turner) → review-
Attachment #363514 - Flags: review?(doug.turner) → review-
Comment on attachment 363514 [details] [diff] [review]
Add Geolocation Support to Windows Mobile v3 - Pref Added to Mobile-Browser

based on my comment in the other patch, this value isn't required.
btw, i checked in an update to the dom, so some of the class info bits in attachment 363513 [details] [diff] [review] are no longer required.
Attachment #363513 - Attachment is obsolete: true
Attachment #363514 - Attachment is obsolete: true
(In reply to comment #7)
> (From update of attachment 363513 [details] [diff] [review])
> maximum age magic value?  could you add a comment.
> 

Done.

> 
> align this so that all of the variables are lined up:
> 

Done.

> 
> 
> 
> 
> no printfs in production code:
> 

Done.

> 
> 
> 
> 
> useless.  not need to set this to null first:
> 

Done.

> 
> what if mGPSDevice is null after calling mOpenDevice?
> 
> does returning early if the CreateInstance cause any problems?  leaking
> mGPSDevice

Done. I have added an assert and early return for !mGPSDevice
If we return early in these situations we do not create a timer or do any
updating or reporting of geolocation data, there should be no leaking

> 
> allign these too:
> 

Done.

> 
> 
> Also, i think you can just use LocationUpdated as the symbol (instead of
> mentioning the class::)

Done. 

> You want a default kUpdate value in case it isn't specified in preferences.  a
> good idea, is to have a default value in code, and only if the preference is
> set, do you modify the default.  This avoids you having to add any pref to
> mobile-browser. :-)
> 

Done. Pref removed from mobile.js

> In WinMobileLocationProvider::Startup, just return early by doing:
> 
>      if (!mGPSInst) {
>           you may want an assertion too.  this shouldn't happen, right?
>           return NS_ERROR_FAILURE;
>      }

Done. with assertion as well. but yea this shouldn't happen gpsapi.dll should be
there.

> 
> fwiw, i am going to change this so that it is a strong references.  (make it a
> nsCOMPtr in the header) You should probabably do the same:
> 
> +    mCallback = callback; // weak ref

Done.

> 
> You must free it in Shutdown (mCallBack == nsnull)
> 

Done.

> Also, you might want to only allow Watch to be called once:
> 
> {
>     if (mCallback) return ns_ok;
> 
>     mCallback = callback; // weak ref
>     return NS_OK;
> }
> 

Done.

> mLastPosition is not used.  you can removed it.

Done.


I think I got all the issues you mentioned, if you have any other concerns
please let me know and I'll gladly address them.
Attachment #365256 - Flags: review?(doug.turner)
Comment on attachment 365256 [details] [diff] [review]
Add Geolocation Support to Windows Mobile v4

the spacing is inconsistent.  2 spaces.


//3000ms is maximum age for location data

add a TODO.  what we want to do soon is have the geolocation pass a hit to startup so that you can make better decisions about this value.

also, if you are going to pass PR_Now() to nsGeoPosition, you might has well specify a very small age (eg. 0).

don't use the pref "k" for anything that changes:

+           prefs->GetIntPref("geo.default.update", &kUpdate);


+        mUpdateTimer->InitWithFuncCallback(LocationUpdated, (void*)this,
+                                           kUpdate, nsITimer::TYPE_REPEATING_SLACK);

Just use initWithCallback() and implement http://mxr.mozilla.org/mozilla-central/source/xpcom/threads/nsITimer.idl#70

This way you don't have to pass |this|.

Other than these, i think you're g2g.
Attachment #365256 - Flags: review?(doug.turner) → review-
(In reply to comment #11)
> (From update of attachment 365256 [details] [diff] [review])
> the spacing is inconsistent.  2 spaces.
>

Done. emacs ftw. (humph will be happy)

> 
> //3000ms is maximum age for location data
> 
> add a TODO.  what we want to do soon is have the geolocation pass a hit to
> startup so that you can make better decisions about this value.
> 

Done. TODO comment added.

> also, if you are going to pass PR_Now() to nsGeoPosition, you might has well
> specify a very small age (eg. 0).
> 

Done. Using 100ms, 0ms won't work because the validFields 

check done right after acquistion fails since the data is 

already too old, 100ms seems to be enough to get past 

those checks.


> don't use the pref "k" for anything that changes:
> 
> +           prefs->GetIntPref("geo.default.update", &kUpdate);
> 

Done. renamed variable to update.

> 
> +        mUpdateTimer->InitWithFuncCallback(LocationUpdated, (void*)this,
> +                                           kUpdate,
> nsITimer::TYPE_REPEATING_SLACK);
> 
> Just use initWithCallback() and implement
> http://mxr.mozilla.org/mozilla-central/source/xpcom/threads/nsITimer.idl#70
> 
> This way you don't have to pass |this|.
> 

Done. However I'm still required to pass |this| although 

I have removed the need to pass the function itself, 

LocationUpdated has been renamed Notify to correspond 

with the implementention of the timer.

> Other than these, i think you're g2g.

Awesome!
Attachment #365256 - Attachment is obsolete: true
Attachment #366057 - Flags: review?(doug.turner)
Attachment #366057 - Flags: superreview?(jst)
Attachment #366057 - Flags: review?(doug.turner)
Attachment #366057 - Flags: review+
Comment on attachment 366057 [details] [diff] [review]
Add Geolocation Support to Windows Mobile v5

+  //TODO: add ability to control maximum age with startup variable

could you change this to say "with the PositionOptions"

jst needs to sr.
(In reply to comment #13)
> (From update of attachment 366057 [details] [diff] [review])
> +  //TODO: add ability to control maximum age with startup variable
> 

Done.

> could you change this to say "with the PositionOptions"
> 
> jst needs to sr.

Carried forward the review, not sure if I did this right.
Attachment #366057 - Attachment is obsolete: true
Attachment #366682 - Flags: superreview?(jst)
Attachment #366682 - Flags: review+
Attachment #366057 - Flags: superreview?(jst)
Attachment #366682 - Flags: superreview?(jst) → superreview+
Keywords: checkin-needed
http://hg.mozilla.org/mozilla-central/rev/d4c33fcc4f95

congrats on your first patch Nino.
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Flags: wanted-fennec1.0?
Flags: wanted-fennec1.0? → wanted-fennec1.0+
Assignee: ninodaversa → nobody
Component: General → Geolocation
Product: Fennec → Core
QA Contact: general → geolocation
Attachment #366682 - Flags: approval1.9.1?
Assignee: nobody → ninodaversa
Keywords: checkin-needed
Comment on attachment 366682 [details] [diff] [review]
Add Geolocation Support to Windows Mobile v6

a191=beltzner
Attachment #366682 - Flags: approval1.9.1? → approval1.9.1+
Blocks: 515839
You need to log in before you can comment on or make changes to this bug.