Closed Bug 1129633 Opened 5 years ago Closed 5 years ago

Default Windows 8 to using OS provider for geolocation

Categories

(Core :: DOM: Geolocation, defect)

All
Windows 7
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla39
Tracking Status
firefox39 --- fixed

People

(Reporter: garvan, Assigned: m_kato)

References

()

Details

Attachments

(2 files, 2 obsolete files)

This is ready to be turned on as a default for windows versions that support it. I can only find docs for support in Windows 8.1. I don't see any mention of this functioning in 8.x.

http://www.jayway.com/2014/04/22/windows-phone-8-1-for-developers-geolocation-and-geofencing
http://www.silverlightshow.net/items/Windows-8.1-Location-aware-apps-Part-1-What-s-new-in-geolocation.aspx

+++ This bug was initially created as a clone of Bug #512407 +++
Correction: 8.x -> I meant 8.0.
I assume a run-time check is needed, and I assume checking the windows version is straightforward. 
So, if firefox.js has the default setting removed, then firefox could check at first run what the version of windows is and write that pref. 
This seems like a non-unique problem; there is probably an existing precedent for how this is to be done.

I don't have a windows build setup to test anything locally (well, not yet anyway), but there _is_ always try.
If no one can get to this soon, I'll take a look when I have time.
Ping to see if you have time to take a look at this, thanks!
Flags: needinfo?(m_kato)
I take this.
Assignee: nobody → m_kato
Flags: needinfo?(m_kato)
Comment on attachment 8563226 [details] [diff] [review]
Add fallback MLS provider in WindowsLocationProvider

When using Windows 7 or easier, we use MLS.  When Widnows 8+, we use MLS API until location API returns RUNNING state.  Then after RUNNING state, we will use Location API data.

If location API cannot return current location, state isn't changed from initialized to running.  So we keep MLS as location provider.
Attachment #8563226 - Flags: review?(gkeeley)
Comment on attachment 8563226 [details] [diff] [review]
Add fallback MLS provider in WindowsLocationProvider

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

After my questions are answered I'll finish the review, 
Thanks Makato

::: dom/geolocation/nsGeolocation.cpp
@@ +814,5 @@
>    }
>  #endif
>  
>  #ifdef XP_WIN
> +  if (Preferences::GetBool("geo.provider.ms-windows-location", true)) {

Is it possible to add IsWin8OrLater() here instead of determining this in the WindowsLocationProvider? 

Is the runtime fallback to MLS logic in WindowsLocationProvider needed? What are the risks of not having that? 

Should we have telemetry to decide whether the fallback code is needed?

::: dom/system/windows/WindowsLocationProvider.cpp
@@ +15,5 @@
>  class LocationEvent MOZ_FINAL : public ILocationEvents
>  {
>  public:
> +  LocationEvent(nsIGeolocationUpdate* aCallback, WindowsLocationProvider *aProvider)
> +    : mCallback(aCallback), mProvider(aProvider), mCount(0) {

teeny ws nit: T* var and T *var

@@ +85,5 @@
> +  }
> +
> +  // Cannot get current location at this time.  We use MLS instead until
> +  // Location API returns RUNNING status.
> +  if (NS_SUCCEEDED(mProvider->CreateAndWatchMLSProvider(mCallback))) {

Why is this initialized when REPORT_INITIALIZING comes in?

I thought CreateAndWatchMLSProvider would be called in OnStatusChanged only on an error state as a fallback.
(In reply to Garvan Keeley [:garvank] from comment #7)
> Comment on attachment 8563226 [details] [diff] [review]
> Add fallback MLS provider in WindowsLocationProvider
> 
> Review of attachment 8563226 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> After my questions are answered I'll finish the review, 
> Thanks Makato
> 
> ::: dom/geolocation/nsGeolocation.cpp
> @@ +814,5 @@
> >    }
> >  #endif
> >  
> >  #ifdef XP_WIN
> > +  if (Preferences::GetBool("geo.provider.ms-windows-location", true)) {
> 
> Is it possible to add IsWin8OrLater() here instead of determining this in
> the WindowsLocationProvider? 

OK. I will add it.

> Is the runtime fallback to MLS logic in WindowsLocationProvider needed? What
> are the risks of not having that? 

When I test on Windows 8.1 desktop without WiFi (disable WiFi), Location API didn't change to RUNNING state.  So if this situation, we should use MLS to use GeoIP based location provider.

> Should we have telemetry to decide whether the fallback code is needed?

Humm, maybe, we should add for non-WiFi environment.

> ::: dom/system/windows/WindowsLocationProvider.cpp
> @@ +15,5 @@
> >  class LocationEvent MOZ_FINAL : public ILocationEvents
> >  {
> >  public:
> > +  LocationEvent(nsIGeolocationUpdate* aCallback, WindowsLocationProvider *aProvider)
> > +    : mCallback(aCallback), mProvider(aProvider), mCount(0) {
> 
> teeny ws nit: T* var and T *var

I will modify it.

> @@ +85,5 @@
> > +  }
> > +
> > +  // Cannot get current location at this time.  We use MLS instead until
> > +  // Location API returns RUNNING status.
> > +  if (NS_SUCCEEDED(mProvider->CreateAndWatchMLSProvider(mCallback))) {
> 
> Why is this initialized when REPORT_INITIALIZING comes in?

Even if location API cannot get location, state will be changed to REPORT_INITIALIZING and it keeps this value.  Should I use one shot timer for timeout instead?

Also, Location API state is
1. Set REPORT_INITIALIZING at first.
2. If found location, it changes to REPORT_RUNNING.
   If error, it changes to REPORT_ERROR.
   Even if timeout, it will keep REPORT_INITIALIZING.
See Also: → 1134019
> > Is it possible to add IsWin8OrLater() here instead of determining this in
> > the WindowsLocationProvider? 
> 
> OK. I will add it.

And also remove it from WindowsLocationProvider::Startup()?
 
> > Is the runtime fallback to MLS logic in WindowsLocationProvider needed? What
> > are the risks of not having that? 
> 
> When I test on Windows 8.1 desktop without WiFi (disable WiFi), Location API
> didn't change to RUNNING state.  So if this situation, we should use MLS to
> use GeoIP based location provider.

I have access to a windows machine but with no wifi card, and I get GeoIP lookups. Weird, maybe this is this a specific case where the PC must have a disabled wifi card? 

> Even if location API cannot get location, state will be changed to
> REPORT_INITIALIZING and it keeps this value.  Should I use one shot timer
> for timeout instead?
> 
> Also, Location API state is
> 1. Set REPORT_INITIALIZING at first.
> 2. If found location, it changes to REPORT_RUNNING.
>    If error, it changes to REPORT_ERROR.
>    Even if timeout, it will keep REPORT_INITIALIZING.

It think it could do this:
1) REPORT_INITIALIZING, create MLS provider
2) MLS provider responds 
3) REPORT_RUNNING, windows provider responds

If so, it should have a timer so that the windows provider has adequate time to respond. 

Interestingly this is the same type of problem the Apple Core Location API has, whereby the status callback is unreliable for determining all error states.
I refactored out the MLS fallback code so the Core Location provider and this one can share that logic:
https://bugzilla.mozilla.org/show_bug.cgi?id=1134019
Using the fallback class from bug 1134019 (which hasn't landed yet), we could do the method in this patch.

Note that in this patch, 
WindowsLocationProvider::MLSUpdate::Update(nsIDOMGeoPosition *position)
does not cancel the WindowsLocationProvider. I am not sure if this is a concern or not, have to think about that a bit more.
Attachment #8566298 - Flags: review?(m_kato)
Comment on attachment 8563226 [details] [diff] [review]
Add fallback MLS provider in WindowsLocationProvider

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

With the added delay from MLSFallback, (and my minor comments), I think this is great.

Of course, if I am misunderstanding the need for a delay in calling to MLS please let me know.
Attachment #8563226 - Flags: review?(gkeeley) → review+
Comment on attachment 8566298 [details] [diff] [review]
Part 2) Using MLSFallback to delay calling MLS.

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

thanks!

::: dom/system/windows/WindowsLocationProvider.cpp
@@ +7,3 @@
>  #include "nsIDOMGeoPositionError.h"
>  #include "nsComponentManagerUtils.h"
>  #include "mozilla/WindowsVersion.h"

IsWin8orLater() is removed, so we should remove #include "mozllla/WindowsVersion.h" too.

@@ +19,5 @@
> +{
> +}
> +
> +NS_IMETHODIMP
> +WindowsLocationProvider::MLSUpdate::Update(nsIDOMGeoPosition *position)

We should use "a" prefix for argument (ex aPosition)

@@ +43,5 @@
> +  return NS_OK;
> +}
> +
> +NS_IMETHODIMP
> +WindowsLocationProvider::MLSUpdate::NotifyError(uint16_t error)

ditto (use aError)
Attachment #8566298 - Flags: review?(m_kato) → review+
Makato, is this ok that I merge all this into one patch? 
(Instead of your patch and mine as separate patches)
Attachment #8563226 - Attachment is obsolete: true
Attachment #8566298 - Attachment is obsolete: true
Attachment #8571986 - Flags: review?(m_kato)
Chris the actual line that uses this is in part1, in nsGeolocation.cpp:

if (Preferences::GetBool("geo.provider.ms-windows-location", false) && IsWin8OrLater()) then use WindowsLocationProvider()
Attachment #8571989 - Flags: review?(cpeterson)
Blocks: 1139012
Comment on attachment 8571989 [details] [diff] [review]
Part 2. Set default to win8 in firefox.js for RELEASE_BUILD only

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

LGTM

::: browser/app/profile/firefox.js
@@ +1774,5 @@
>  #endif
>  #endif
>  
> +#ifdef XP_WIN
> +#ifdef RELEASE_BUILD

You can combine these #ifdef lines into #if defined(XP_WIN) && defined(RELEASE_BUILD).
Attachment #8571989 - Flags: review?(cpeterson) → review+
> You can combine these #ifdef lines into #if defined(XP_WIN) &&
> defined(RELEASE_BUILD).

I thought we wanted available prefs shown whenever possible, and just have the value of the pref change. Rather than the pref itself existing/not-existing in firefox.js.
Confirmed on IRC with Chris that my assumption in Comment 17 is correct.
Comment on attachment 8571986 [details] [diff] [review]
Part 1. Use win8 provider with a fallback to MLS

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

sorry for delay.
Attachment #8571986 - Flags: review?(m_kato) → review+
See Also: → 1147897
When using Windows 7 or easier, we use MLS. (In reply to Makoto Kato (:m_kato) from comment #6)
> When using Windows 7 ... we use MLS.
What the logic of this behaviour? Win 7 have support of geolocation sensors (and this work fine in Firefox 38 after https://bugzilla.mozilla.org/show_bug.cgi?id=512407 commit @ Windows 7 x64 sp1 as minimum)
(In reply to SweetLow from comment #22)
> When using Windows 7 or easier, we use MLS. (In reply to Makoto Kato
> (:m_kato) from comment #6)
> > When using Windows 7 ... we use MLS.
> What the logic of this behaviour? Win 7 have support of geolocation sensors
> (and this work fine in Firefox 38 after
> https://bugzilla.mozilla.org/show_bug.cgi?id=512407 commit @ Windows 7 x64
> sp1 as minimum)

We detailed that in https://bugzilla.mozilla.org/show_bug.cgi?id=512407#c3 and later comments.

Windows 7 just had an abstract sensor API, but Microsoft didn't actually provider a free online service for it. That only came with Windows 8. Which means this would only work for the very few users Windows 7 users that had a special third-party plugin or extra hardware installed.
(In reply to Hanno Schlichting [:hannosch] from comment #23)
> (In reply to SweetLow from comment #22)

> We detailed that in https://bugzilla.mozilla.org/show_bug.cgi?id=512407#c3
> and later comments.
I read all three (512407,1129633,1147897) related topics before posting.

> Windows 7 just had an abstract sensor API, but Microsoft didn't actually
> provider a free online service for it.
And what? Hardware sensors support is fully implemented (and work fine). Reliable algo will - try MS service (when geo.provider.ms-windows-location==true), if it absent or location still not determined  - fall back to google location service. Without any OS version dependencies.

>extra hardware installed
Hardware GPS sensor not so rare thing.
You need to log in before you can comment on or make changes to this bug.