502911 - geolocation wrapper should provide a toJSON method

Status: RESOLVED WONTFIX

(Core :: DOM: Geolocation, enhancement)

Description

[Doug Turner (:dougt)]

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.

Comment 1

[Boris Zbarsky [:bzbarsky]]

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.

Comment 2

[Doug Turner (:dougt)]

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?

Comment 3

[Robert Sayre]

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

Comment 4

[Boris Zbarsky [:bzbarsky]]

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

Comment 5

[Doug Turner (:dougt)]

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

[Robert Sayre]

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

Comment 7

[Doug Turner (:dougt)]

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

[Robert Sayre]

(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

[Saint Wesonga]

(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

[Hanno Schlichting]

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.

Comment 11

[Boris Zbarsky [:bzbarsky]]

This bug wasn't about debugging.  Think a script that wants to send a Position to the server...

Comment 12

[Boris Zbarsky [:bzbarsky]]

But fundamentally, this bug is asking for a spec change and we should take it to the relevant W3C working group...

Comment 14

[Boris Zbarsky [:bzbarsky]]

Doug, who owns this stuff nowadays, both in terms of spec and impl?

Comment 15

[:garvan]

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

Comment 16

[Doug Turner (:dougt)]

Garvan++

Comment 17

[:garvan]

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.

Comment 18

[Boris Zbarsky [: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.

Comment 19

[:garvan]

The Position object is defined as part of the accepted W3C geo spec, isn't compliance with the spec our primary concern?

Comment 20

[Boris Zbarsky [: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.

Comment 21

[:garvan]

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

Comment 22

[Boris Zbarsky [:bzbarsky]]

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