444642 - Firefox - Hookup geolocation prompt to notification prompt.

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[Doug Turner (:dougt)]

We need a way to alert the user that the website they are visiting is asking for geolocation data.

Aza was thinking something like this:
http://azarask.in/blog/wp-content/uploads/2008/05/geolocation_mockupv3.jpg

I am going to implement this simply using the existing notificationBox.

Comment 1

[Doug Turner (:dougt)]

Attachment: png of the notification bar in action.

Comment 2

[Doug Turner (:dougt)]

Attachment: patch v.1

I may need string wordsmithing and confirmation that the accessKeys are correct.

Comment 3

[Doug Turner (:dougt)]

Comment on attachment 328959 [details] [diff] [review]
patch v.1

um.  ignore that change to the "editBookmarkTitle" string.  fixed locally.

Comment 4

[Doug Turner (:dougt)]

i created an addon for FF:

https://addons.mozilla.org/en-US/firefox/addon/8420


mike, is there a process we can look at for inclusion with firefox.next?

Comment 5

[Doug Turner (:dougt)]

Attachment: patch v.2

this uses the right browser infobar. There might be a better way to find the notification box.

Comment 6

[Doug Turner (:dougt)]

changes to strings:

gelocation -> geolocation for the property names
not modifying the bookmarks string

w/ those changes, okay'ed by mconnor via irc

Comment 7

[Doug Turner (:dougt)]

Attachment: patch v.3

Comment 8

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 336555 [details] [diff] [review]
patch v.3

>diff --git a/browser/base/content/browser.js b/browser/base/content/browser.js

>+      var bundle_browser = document.getElementById("bundle_browser");

You can use gNavigatorBundle instead.

>+      var buttons = [{

>+        callback: function(){request.allow()},

You could use the shortened style without braces:

function () request.allow()

>+      return 1;

Does this have some special meaning?

> function delayedStartup()

>+  // hook up the geolocation prompt to our notificationBox
>+  setupGeolocationPrompt();

Putting it in delayedStartup means it might not be set up before a page tries to use this API on first load (e.g. your home page). I wonder whether we can get away with doing this in BrowserStartup() without a perf hit? How expensive is geolocation service initialization?

>diff --git a/browser/locales/en-US/chrome/browser/browser.properties b/browser/locales/en-US/chrome/browser/browser.properties

>+geolocation.nothingLocation=Nothing
>+geolocation.nothingLocationKey=C

Seems to me like this should use a letter that's in the string. "o"?

Comment 9

[Doug Turner (:dougt)]

Attachment: patch v.4

gavin's suggestions + a better way to find the right notification bar.

Comment 10

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 336570 [details] [diff] [review]
patch v.4

This won't work with multiple windows (only the browsers from the first window will be searched).

The previous patch really should work. Are you sure the geolocation service is passing in the window object you're expecting it to?

Comment 11

[Doug Turner (:dougt)]

Attachment: patch v.5

what gavin and I discussed over irc.

Comment 12

[Vlado Valastiak [:wladow]]

Comment on attachment 336599 [details] [diff] [review]
patch v.5

>+# Geolocation UI
>+geolocation.exactLocation=Exact Location (within 10 feet)
>+geolocation.exactLocationKey=E
>+geolocation.neighborhoodLocation=Neighborhood (within 1 mile)
>+geolocation.neighborhoodLocationKey=N

What about other distance units? We use metres and kilometres in Europe, feet and miles don't make sense here:
1 mile = 1.609344 kilometers
10 feet = 3.04800 meters 

What numbers should we use in localizations? Localization note would help here too.

Comment 13

[Doug Turner (:dougt)]

Sorry about that.  These do not have to be exact values, but instead approximations.  For example, if you said 2km for the 1mile value, or even 1km, it would be perfectly valid.  The idea here is to give the user the general "feeling" of how accurate a position will be.

Comment 14

[Doug Turner (:dougt)]

Attachment: l10n note

Comment 15

[Doug Turner (:dougt)]

Vlado, please let me know if this l10n note will work.

Comment 16

[Vlado Valastiak [:wladow]]

Comment on attachment 336736 [details] [diff] [review]
l10n note

Sure, this will work. Maybe you should mention
# examples: Exact Location (within 3 m)
too, so many localizers can use it without the need to google a foot -> meter conversion

Comment 17

[Doug Turner (:dougt)]

[e-mail]
Wed Sep 03 15:05:29 2008 -0700	5075df1da267	Doug Turner — Bug 444642 - Hookup geolocation prompt to notification prompt. r=gavin/mconnor

and 

[e-mail]
Wed Sep 03 16:04:48 2008 -0700	d4b972bd979b	Doug Turner — Follow up to Bug 444642 - Add l10n note to geolocation properties. r=[e-mail]

Comment 18

[Doug Wright]

Perhaps I am being thick, but there seems to be a large part of the functionality missing/broken here - I can get a website (http://browserspy.dk/geolocation.php) to ask the browser where I am, which triggers the notification bar (so far so good). I then click on a button (e.g. Neighborhood) and that's it.

At that point, since Minefield has no idea what part of planet Earth I'm at, I'm expecting some kind of dialog to pop up, so I can tell it. Instead the bar just sits there all useless and the error console has the following:

Error: uncaught exception: [Exception... "Component returned failure code: 0x80040111 (NS_ERROR_NOT_AVAILABLE) [nsIGeolocationRequest.allowButFuzz]"  nsresult: "0x80040111 (NS_ERROR_NOT_AVAILABLE)"  location: "JS frame :: chrome://browser/content/browser.js :: anonymous :: line 795"  data: no]

Tested with today's Minefield nightly

Is this *supposed* to work yet, or is there another bug somewhere? There's nothing on the dependency list

Comment 19

[Simon Bünzli]

Is it intentional that we don't allow the user to have Firefox remember a decision for a site (cf. the screenshot for Geode at <http://labs.mozilla.com/2008/10/introducing-geode/#ibi_>)?

Comment 20

[Doug Turner (:dougt)]

@doug  this was fixed for beta.


@simon  please file a seperate bug.  I think this is a good idea.  we should discuss what should the default value should be.

Comment 21

[Axel Hecht [:Pike]]

Adrian filed bug 462199 to make the string keys line up with what our tools like.