You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@cordova.apache.org by sarangan12 <gi...@git.apache.org> on 2016/02/12 01:41:04 UTC
[GitHub] cordova-plugin-geolocation pull request: CB-10574: MobileSpec can'...
GitHub user sarangan12 opened a pull request:
https://github.com/apache/cordova-plugin-geolocation/pull/70
CB-10574: MobileSpec can't get results for WP8.1 Builds
The MobileSpec can't get results for WP8.1 Builds.
**Why?**
Because, one of the tests in the geolocation plugin is failing and causes the app to crash. This test gets the "0x80004004 - JavaScript runtime error: Operation aborted" error. So, the app crashes. The issue around this test must be fixed for the CI to finish successfully for Windows Phone 8.1.
**What is the scenario for the crash?**
The scenario could be seen manually also. When you run the mobilespec test for geolocatio API, try the following steps:
1. Start Watching
2. Stop Watching
3. Start Watching
4. Start Watching
The app will crash in step 4. But it will not crash on step 3.
**Why does the app crash on step 4?**
Per code, the loc object (instance of Windows.Devices.Geolocation.Geolocator object) is being reused in both the steps - which causes the crash.
**What is the fix?**
The current fix is to create a new instance of the object instead of resuing the same object. I have validated the fix by running the geolocation tests and all of them are passing.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/sarangan12/cordova-plugin-geolocation CB-10574
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/cordova-plugin-geolocation/pull/70.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #70
----
commit fbe3bdaef60155cefe2cf789cc62cb27e5a8a3af
Author: Sarangan Rajamanickam <sa...@microsoft.com>
Date: 2016-02-12T00:19:04Z
CB-10574: MobileSpec can't get results for WP8.1 Builds
MobileSpec can't get results for WP8.1 Builds
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-plugin-geolocation pull request: CB-10574: MobileSpec can'...
Posted by daserge <gi...@git.apache.org>.
Github user daserge commented on the pull request:
https://github.com/apache/cordova-plugin-geolocation/pull/70#issuecomment-183385836
The geolocation plugin has several issues:
1. `geolocation.js->getCurrentPosition` and `geolocation.js->watchPosition` both don't immediately return `timerId`/`watchId` therefore the tests `watchPosition method->error callback->geolocation.spec.7`, `watchPosition method->error callback->geolocation.spec.10` and `success callback->geolocation.spec.8` get their errorWatch/successWatch(es) mixed.
2. On Windows after Geolocator had some of the event handlers attached we can't set any of its properties like `desiredAccuracy`, `reportInterval` or `movementThreshold` as this causes "0x80004004 - JavaScript runtime error: Operation aborted".
Related SO question: http://stackoverflow.com/questions/13720945/is-it-possible-to-change-desiredaccuracy-reportinterval-of-geolocator-in-posit
3. Manual test for `addWatch` doesn't clear the previous `watchId` so it's causing the issues on Windows.
I will update my PR soon with these issues addressed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-plugin-geolocation pull request: CB-10574: MobileSpec can'...
Posted by dblotsky <gi...@git.apache.org>.
Github user dblotsky commented on the pull request:
https://github.com/apache/cordova-plugin-geolocation/pull/70#issuecomment-183142774
It looks like reusing the `Geolocator` object was done *by design*, as is suggested by the naming of `ensureLocator`. Also it is assumed to be a singleton, so creating a new one on every call to `getLocation` will lead to incorrect behaviour.
What exactly causes the crash when the `loc` object is reused? Does `addEventListener` not allow for more than one listener?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-plugin-geolocation pull request: CB-10574: MobileSpec can'...
Posted by sarangan12 <gi...@git.apache.org>.
Github user sarangan12 commented on the pull request:
https://github.com/apache/cordova-plugin-geolocation/pull/70#issuecomment-183587579
@dblotsky Can you review and merge this PR?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-plugin-geolocation pull request: CB-10574: MobileSpec can'...
Posted by daserge <gi...@git.apache.org>.
Github user daserge commented on the pull request:
https://github.com/apache/cordova-plugin-geolocation/pull/70#issuecomment-183411928
Update the PR #69
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-plugin-geolocation pull request: CB-10574: MobileSpec can'...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/cordova-plugin-geolocation/pull/70
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-plugin-geolocation pull request: CB-10574: MobileSpec can'...
Posted by sarangan12 <gi...@git.apache.org>.
Github user sarangan12 commented on the pull request:
https://github.com/apache/cordova-plugin-geolocation/pull/70#issuecomment-183441405
@dblotsky 'ensureLocator' need not necessarily mean than the 'loc' has to be singleton. It can simply mean that the "Geolocation access has been allowed by the user" (Refer one of the error scenarios in ensureLocator function).
But, I do agree it would be great if we could reuse the value. Unfortunately, there is no clean solution in this case. You can either create a new object or remove the existing listeners and use the old object. I think removing the existing listeners is not a good option (since an application can start watching at two different places and want to keep both event listeners alive).
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-plugin-geolocation pull request: CB-10574: MobileSpec can'...
Posted by daserge <gi...@git.apache.org>.
Github user daserge commented on the pull request:
https://github.com/apache/cordova-plugin-geolocation/pull/70#issuecomment-184211059
@sarangan12, LGTM, thank you for updating the PR!
A few notes:
- Could you please add a fallback for [Number.EPSILON](https://github.com/sarangan12/cordova-plugin-geolocation/commit/a6da270a0bb8d6c83911d8d559c1bb37abe7bbcc#diff-670b1e761968f9ba2d95017f9a8a6187L169) as it is undefined on Windows Phone 8.1?
- [success](https://github.com/sarangan12/cordova-plugin-geolocation/commit/a6da270a0bb8d6c83911d8d559c1bb37abe7bbcc#diff-670b1e761968f9ba2d95017f9a8a6187L190) is null in clearWatch so does it make sense to verify it has a value before executing it? (`success && success()`)
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-plugin-geolocation pull request: CB-10574: MobileSpec can'...
Posted by daserge <gi...@git.apache.org>.
Github user daserge commented on the pull request:
https://github.com/apache/cordova-plugin-geolocation/pull/70#issuecomment-183443464
@sarangan12, what happens with loc object properties being set in a call to `watchLocation` when `ensureLocator` is called?
watchLocation first calls getLocation method and it calls `ensureLocator` before it's execution - woun't the loc properties be lost in this case?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-plugin-geolocation pull request: CB-10574: MobileSpec can'...
Posted by dblotsky <gi...@git.apache.org>.
Github user dblotsky commented on the pull request:
https://github.com/apache/cordova-plugin-geolocation/pull/70#issuecomment-183560875
@daserge keep in mind that a previous watch doesn't need to be cleared for a new one to be created; that's the whole point of `id`s on watches. Keeping several `loc` objects sounds like the correct design to implement the multiple-watch semantics of the geolocation API.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-plugin-geolocation pull request: CB-10574: MobileSpec can'...
Posted by dblotsky <gi...@git.apache.org>.
Github user dblotsky commented on a diff in the pull request:
https://github.com/apache/cordova-plugin-geolocation/pull/70#discussion_r52819999
--- Diff: src/windows/GeolocationProxy.js ---
@@ -181,10 +178,11 @@ module.exports = {
callbacks = ids[clientId];
--- End diff --
Nitpick: one declaration per line.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-plugin-geolocation pull request: CB-10574: MobileSpec can'...
Posted by dblotsky <gi...@git.apache.org>.
Github user dblotsky commented on a diff in the pull request:
https://github.com/apache/cordova-plugin-geolocation/pull/70#discussion_r52820033
--- Diff: src/windows/GeolocationProxy.js ---
@@ -181,10 +178,11 @@ module.exports = {
callbacks = ids[clientId];
--- End diff --
Also, it might be worth it to either add an assertion or throw an exception here if the passed `clienId` is not in `locs` or `ids`.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-plugin-geolocation pull request: CB-10574: MobileSpec can'...
Posted by sarangan12 <gi...@git.apache.org>.
Github user sarangan12 commented on a diff in the pull request:
https://github.com/apache/cordova-plugin-geolocation/pull/70#discussion_r52822810
--- Diff: src/windows/GeolocationProxy.js ---
@@ -14,47 +14,43 @@
* limitations under the License.
*/
-var PositionError = require('./PositionError'),
- ids = {},
- loc;
+var PositionError = require('./PositionError');
+var ids = {};
--- End diff --
Done
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-plugin-geolocation pull request: CB-10574: MobileSpec can'...
Posted by sarangan12 <gi...@git.apache.org>.
Github user sarangan12 commented on a diff in the pull request:
https://github.com/apache/cordova-plugin-geolocation/pull/70#discussion_r52822803
--- Diff: src/windows/GeolocationProxy.js ---
@@ -181,10 +178,11 @@ module.exports = {
callbacks = ids[clientId];
--- End diff --
Changed the declarations. I am not sure about adding an assertion or throwing an exception. Consider a scenario in which I press the "Stop watching" in mobilespec before "Start Watching". This is a valid scenario. I just don't want an assertion or exception in such a scenario. (as the reason is simply there is nothing to stop)
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-plugin-geolocation pull request: CB-10574: MobileSpec can'...
Posted by sarangan12 <gi...@git.apache.org>.
Github user sarangan12 commented on the pull request:
https://github.com/apache/cordova-plugin-geolocation/pull/70#issuecomment-183526822
@dblotsky @daserge Addressed the concerns. Please take a look at this revised PR.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-plugin-geolocation pull request: CB-10574: MobileSpec can'...
Posted by dblotsky <gi...@git.apache.org>.
Github user dblotsky commented on a diff in the pull request:
https://github.com/apache/cordova-plugin-geolocation/pull/70#discussion_r52820086
--- Diff: src/windows/GeolocationProxy.js ---
@@ -14,47 +14,43 @@
* limitations under the License.
*/
-var PositionError = require('./PositionError'),
- ids = {},
- loc;
+var PositionError = require('./PositionError');
+var ids = {};
--- End diff --
Nitpick: `ids` is not a very descriptive name. This object holds on to subscribed callbacks, so maybe rename it to something related to that?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org