geolocation wrapper should provide a toJSON method

RESOLVED WONTFIX

Status

()

Core
Geolocation
--
enhancement
RESOLVED WONTFIX
8 years ago
2 years ago

People

(Reporter: dougt, Unassigned)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [W3C GEO SPEC], URL)

(Reporter)

Description

8 years ago
if you pass a geolocation position object to JSON.stringify we fail.

We create position objects for geolocation here:

http://mxr.mozilla.org/mozilla-central/source/dom/src/geolocation/NetworkGeolocationProvider.js#83

These are handed back to the js context when a location is determined.  If a user tries to stringify them, they get an empty string.

I am not sure if js should be doing something it isn't, or if my object needs more stuff on it to get this working.
(Reporter)

Updated

8 years ago
Summary: native json looses its nuts on the geolocation wrapper → native json loses its nuts on the geolocation wrapper
I assume you're talking about objects created with |new WifiGeoPositionObject|?  I'm not sure what behavior you expect here, exactly...  The user is NOT getting the object you create (which is a very good thing!).  The user is getting an XPCWrappedNative around the XPCWrappedJS for that object.  The XPCWrappedNative has no own properties; its prototype does have getters and setters that call C++ methods on the XPCWrappedJS, which turns them into property gets on your JS object.

It's just like passing a DOM node to JSON.stringify.  It has no own properties, though its prototype has getters for things like nextSibling, etc.
(Reporter)

Comment 2

8 years ago
is there a way to define a method on WifiGeoPositionObject such that the XPCWrappedNative knows what to do when stringify is called?  eg. a toString or something?

Updated

8 years ago
Summary: native json loses its nuts on the geolocation wrapper → geolocation wrapper should provide a toJSON method

Comment 3

8 years ago
(In reply to comment #2)
> is there a way to define a method on WifiGeoPositionObject such that the
> XPCWrappedNative knows what to do when stringify is called?  eg. a toString or
> something?

give the wrapper a toJSON method that returns a plain JS object with own properties, e.g.:

toJSON: function() { return {lon: this.long, lat: this.lat} }
Assuming "the wrapper" in comment 3 is the XPCWrappedNative, the answer to comment 2 is almost certainly "no".

Again, this is the same behavior as all other IDL-exposed objects have in our setup.

You could probably add some xpconnect glue to make this work differently somehow...
Assignee: general → nobody
Component: JavaScript Engine → Geolocation
QA Contact: general → geolocation
(Reporter)

Comment 5

8 years ago
rob, do you think that we can create some xpconnect magic to produce toJSON method's based on the class info of a given object?

Comment 6

8 years ago
(In reply to comment #5)
> rob, do you think that we can create some xpconnect magic to produce toJSON
> method's based on the class info of a given object?

well, I don't know if I would call it magic. But you can declare a void toJSON method in IDL and set the return value JSObject on the XPCCallContext.
(Reporter)

Comment 7

8 years ago
robert, get this.

I haven't changed anything, but now stringify is returning the following json on the trunk.  I am happyface with the first stuff (the stuff that is specific to geolocation).  But am sadface about the stuff that is classinfo related.  Any ideas what caused this, or how I can filter this out?  I tried creating my own toJSON() method, but that didn't work (or i messed it up).


{"timestamp":1251421954767,"coords":{"latitude":37.313823,"longitude":-121.9587774,"altitude":0,"accuracy":150,"altitudeAccuracy":0,"heading":null,"speed":null,"contractID":"","classDescription":"wifi geo position coords object","classID":null,"implementationLanguage":2,"flags":8,"SINGLETON":1,"THREADSAFE":2,"MAIN_THREAD_ONLY":4,"DOM_OBJECT":8,"PLUGIN_OBJECT":16,"EAGER_CLASSINFO":32,"CONTENT_NODE":64,"RESERVED":2147483648},"address":{"streetNumber":"724","street":"Ardis Ave","premises":null,"city":"San Jose","county":null,"region":"California","country":"United States","countryCode":"US","postalCode":"95117","contractID":"","classDescription":"wifi geo position address object","classID":null,"implementationLanguage":2,"flags":8,"SINGLETON":1,"THREADSAFE":2,"MAIN_THREAD_ONLY":4,"DOM_OBJECT":8,"PLUGIN_OBJECT":16,"EAGER_CLASSINFO":32,"CONTENT_NODE":64,"RESERVED":2147483648},"contractID":"","classDescription":"wifi geo location position object","classID":null,"implementationLanguage":2,"flags":8,"SINGLETON":1,"THREADSAFE":2,"MAIN_THREAD_ONLY":4,"DOM_OBJECT":8,"PLUGIN_OBJECT":16,"EAGER_CLASSINFO":32,"CONTENT_NODE":64,"RESERVED":2147483648}

Comment 8

8 years ago
(In reply to comment #7)
> robert, get this.
> 

I don't get it. But, we should get a test checked in to cover whatever it is JSON should be stringifying.

Comment 9

7 years ago
(In reply to comment #7)
> ....  But am sadface about the stuff that is classinfo related. 
> Any ideas what caused this, or how I can filter this out?  I tried creating my
> own toJSON() method, but that didn't work (or i messed it up).

Not sure I understand exactly what the problem is here, but I suspect you might be running into prototype chain related issues like https://bugzilla.mozilla.org/show_bug.cgi?id=533139#c1 - don't take my word for it though.

Comment 10

2 years ago
These days console.log can be used for debugging purposes, which results in:

console.log(position):

Position { coords: Coordinates, timestamp: 1435348476501 }

console.log(position.coords):

Coordinates { latitude: 50.1131753, longitude: -122.9555973, altitude: 0, accuracy: 120.9730509, altitudeAccuracy: 0, heading: NaN, speed: NaN }

I think this is good enough, especially since this bug has seen no recent attention.

Updated

2 years ago
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → WORKSFORME
This bug wasn't about debugging.  Think a script that wants to send a Position to the server...
Status: RESOLVED → REOPENED
Resolution: WORKSFORME → ---
But fundamentally, this bug is asking for a spec change and we should take it to the relevant W3C working group...

Updated

2 years ago
Severity: normal → enhancement
OS: Mac OS X → All
Hardware: x86 → All

Updated

2 years ago
Duplicate of this bug: 827037
Doug, who owns this stuff nowadays, both in terms of spec and impl?
Flags: needinfo?(dougt)

Comment 15

2 years ago
(In reply to Boris Zbarsky [:bz] from comment #14)
> Doug, who owns this stuff nowadays, both in terms of spec and impl?

Me. Although I've dealt with implementation of the spec -not getting it changed- and it seems to be closed since I have been involved the past year. A toJSON on the Position object makes sense to have, which I assume is the spec change you are referring to in comment 12.

ni myself to follow up further.
Flags: needinfo?(gkeeley)

Updated

2 years ago
Flags: needinfo?(gkeeley)
Whiteboard: [W3C GEO SPEC]

Updated

2 years ago
Flags: needinfo?(gkeeley)
(Reporter)

Comment 16

2 years ago
Garvan++
Flags: needinfo?(dougt)

Comment 17

2 years ago
I checked a few other device APIs, and I am confused as to the bug title here.
Why _should_ the Position object provide a toJSON()? 
That is, toJSON() doesn't appear to be standard as a function on returned objects in other device APIs. 

I understand the use case from comment 11, but it is an extreme edge case. Moreover the Position object only has x,y,timestamp and turning it into JSON is trivial for the dev.
Flags: needinfo?(gkeeley) → needinfo?(bzbarsky)
> Why _should_ the Position object provide a toJSON()? 

Back when the bug was filed, I would have said "because Chrome and Safari effectively do", but since then they've moved attributes to the proto chain and I can't check what IE's behavior is because it doesn't seem to succeed at geolocation on browserstack.

So the only real answer I have is that it makes web devs lives simpler, and costs us very little (except some autogenerated codesize, which is a valid concern).

I can live with wontfixing this bug if that's what we want to do.
Flags: needinfo?(bzbarsky)

Comment 19

2 years ago
The Position object is defined as part of the accepted W3C geo spec, isn't compliance with the spec our primary concern?
Flags: needinfo?(bzbarsky)
Our primary concern is exposing the right APIs to the web.  Specs can and do get updated or errata'd as needed.  It's not as if this would be a non-backwards-compatible change to the spec.

So I wouldn't worry about the spec status of this until we decide whether we want to do it.  If we do, _then_ we worry about how to get the specs updated, other UAs on board, etc.
Flags: needinfo?(bzbarsky)

Comment 21

2 years ago
> So I wouldn't worry about the spec status of this until we decide whether we
> want to do it.  If we do, _then_ we worry about how to get the specs
> updated, other UAs on board, etc.

I do worry. That is an added cost. The code change is trivial, the efforts to get the spec updated are not. If we were to implement this, gather telemetry, and find no one is using it, we then have to try take it out, as there would be no reason to be non-spec compliant for no benefit. More cost.

Mostly however, I don't see webdevs writing:
if (typeof position.toJSON === 'function') {
    json = position.toJSON()
} else {
    json = // format the position as JSON
}

rather than just:
    json = // format the position as JSON

If we were discussing adding some critical missing piece of info/functionality -and the geo spec has a few of these- then I would agree that deviating from the standard is worthwhile, that there would be significant adoption and the adoption rate would provide fodder for arguments with the W3C spec group.
Status: REOPENED → RESOLVED
Last Resolved: 2 years ago2 years ago
Resolution: --- → WONTFIX
> Mostly however, I don't see webdevs writing:

The idea is that they would write:

  json = JSON.stringify(position);

and it would work correctly.  But, again, I'm ok with wontfixing it, since other browsers changed away from the behavior they used to have here.
You need to log in before you can comment on or make changes to this bug.