Last Comment Bug 884921 - Align navigator.geolocation with spec
: Align navigator.geolocation with spec
Status: RESOLVED FIXED
: dev-doc-complete, site-compat
Product: Core
Classification: Components
Component: DOM (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla25
Assigned To: Guilherme Gonçalves [:ggp]
:
: Andrew Overholt [:overholt]
Mentors:
Depends on: 838146
Blocks:
  Show dependency treegraph
 
Reported: 2013-06-19 10:27 PDT by Boris Zbarsky [:bz] (still a bit busy)
Modified: 2013-08-03 11:34 PDT (History)
6 users (show)
ryanvm: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
navigator.geolocation should never be null. (3.93 KB, patch)
2013-07-19 14:41 PDT, Guilherme Gonçalves [:ggp]
no flags Details | Diff | Splinter Review
navigator.geolocation should never be null. (3.93 KB, patch)
2013-07-19 14:52 PDT, Guilherme Gonçalves [:ggp]
doug.turner: review+
bzbarsky: feedback+
Details | Diff | Splinter Review

Description Boris Zbarsky [:bz] (still a bit busy) 2013-06-19 10:27:27 PDT
Per spec, this never returns null, which is totally not what our implementation does when geo.enabled is not set....
Comment 1 Mounir Lamouri (:mounir) 2013-06-20 00:17:50 PDT
Shouldn't that be solved by having the properties behind [PrefEnabled] when Navigator is converted to WebIDL?
Comment 2 Boris Zbarsky [:bz] (still a bit busy) 2013-06-20 07:14:37 PDT
I think so, yes; I'm just not willing to make that change as part of the navigator webidl conversion, since Doug thinks it's risky.
Comment 3 Guilherme Gonçalves [:ggp] 2013-07-19 10:46:23 PDT
I'm not sure I understand the desired behavior here. Should navigator.geolocation never be null, but issue error events for all calls to getCurrentPosition/watchPosition when "geo.enabled" is false?
Comment 4 Boris Zbarsky [:bz] (still a bit busy) 2013-07-19 11:03:22 PDT
That's a good question.

The options are either to do that or to have |"geolocation" in navigator| test false if geo.enabled is false and have navigator.geolocation be undefined.  And also have the geolocation interface object disappear when the pref is not set, if we take this approach.
Comment 5 Guilherme Gonçalves [:ggp] 2013-07-19 14:41:08 PDT
Created attachment 778682 [details] [diff] [review]
navigator.geolocation should never be null.

Here's my stab at the second option. This causes navigator.geolocation
to be undefined when the pref is false on page load.

However, if the pref is set to false after being true on page load, that
doesn't cause navigator.geolocation to be undefined. Is that a problem?
navigator.getGamepads seems to also behave like that.


Feedback: bzbarsky@mit.edu
Comment 6 Guilherme Gonçalves [:ggp] 2013-07-19 14:52:21 PDT
Created attachment 778687 [details] [diff] [review]
navigator.geolocation should never be null.
Comment 7 Boris Zbarsky [:bz] (still a bit busy) 2013-07-19 19:52:16 PDT
Comment on attachment 778687 [details] [diff] [review]
navigator.geolocation should never be null.

> Is that a problem?

No.

And looks like Geolocation is [NoInterfaceObject] (why?) so there is no need to condition its interface object on the pref.
Comment 8 Guilherme Gonçalves [:ggp] 2013-07-24 14:09:05 PDT
Comment on attachment 778687 [details] [diff] [review]
navigator.geolocation should never be null.

Doug, I don't know what concerns you had regarding this bug, so please let me know if anything is missing. Seems to work on try: https://tbpl.mozilla.org/?tree=Try&rev=ed06767dda70
Comment 9 Mounir Lamouri (:mounir) 2013-07-26 09:33:57 PDT
(In reply to Boris Zbarsky (:bz) from comment #7)
> And looks like Geolocation is [NoInterfaceObject] (why?) so there is no need
> to condition its interface object on the pref.

It is likely a bug in the specification.
Comment 10 Ryan VanderMeulen [:RyanVM] 2013-07-26 13:16:12 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/b54402f21eb8
Comment 11 Ryan VanderMeulen [:RyanVM] 2013-07-26 19:37:15 PDT
https://hg.mozilla.org/mozilla-central/rev/b54402f21eb8

Note You need to log in before you can comment on or make changes to this bug.