1129633 - Default Windows 8 to using OS provider for geolocation

Status: RESOLVED FIXED

(Core :: DOM: Geolocation, defect)

Description

[:garvan]

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 +++

Comment 1

[:garvan]

Correction: 8.x -> I meant 8.0.

Comment 2

[:garvan]

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.

Comment 3

[:garvan]

Ping to see if you have time to take a look at this, thanks!

Comment 4

[Makoto Kato [:m_kato]]

I take this.

Comment 5

[Makoto Kato [:m_kato]]

Attachment: Add fallback MLS provider in WindowsLocationProvider

Comment 6

[Makoto Kato [: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.

Comment 7

[:garvan]

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.

Comment 8

[Makoto Kato [:m_kato]]

(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.

Comment 9

[:garvan]

> > 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

Comment 10

[:garvan]

Attachment: Part 2) Using MLSFallback to delay calling MLS.

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.

Comment 11

[:garvan]

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.

Comment 12

[Makoto Kato [:m_kato]]

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)

Comment 13

[:garvan]

Attachment: Part 1. Use win8 provider with a fallback to MLS

Makato, is this ok that I merge all this into one patch? 
(Instead of your patch and mine as separate patches)

Comment 14

[:garvan]

Attachment: Part 2. Set default to win8 in firefox.js for RELEASE_BUILD only

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()

Comment 15

[:garvan]

My try link:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=5dfefc7a3428

Comment 16

[Chris Peterson [:cpeterson]]

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).

Comment 17

[:garvan]

> 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.

Comment 18

[:garvan]

Confirmed on IRC with Chris that my assumption in Comment 17 is correct.

Comment 19

[Makoto Kato [:m_kato]]

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.

Comment 20

[:garvan]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a908a824a5ea
https://hg.mozilla.org/integration/mozilla-inbound/rev/0d8e51a3e6ac

Thanks for the review and the initial patch Makoto

Comment 21

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/0d8e51a3e6ac
https://hg.mozilla.org/mozilla-central/rev/a908a824a5ea

Comment 22

[SweetLow]

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)

Comment 23

[Hanno Schlichting]

(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.

Comment 24

[SweetLow]

(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.