Closed Bug 444642 Opened 13 years ago Closed 13 years ago

Firefox - Hookup geolocation prompt to notification prompt.

Categories

(Firefox :: General, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: dougt, Assigned: dougt)

References

(Blocks 1 open bug)

Details

Attachments

(3 files, 4 obsolete files)

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.
Attached patch patch v.1 (obsolete) — Splinter Review
I may need string wordsmithing and confirmation that the accessKeys are correct.
Attachment #328959 - Flags: review?
Comment on attachment 328959 [details] [diff] [review]
patch v.1

um.  ignore that change to the "editBookmarkTitle" string.  fixed locally.
Blocks: 445180
Summary: Hookup geolocation prompt to FF's notification prompt. → Firefox - Hookup geolocation prompt to notification prompt.
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?
Assignee: doug.turner → nobody
Assignee: nobody → doug.turner
Attached patch patch v.2 (obsolete) — Splinter Review
this uses the right browser infobar. There might be a better way to find the notification box.
Attachment #328959 - Attachment is obsolete: true
Attachment #328959 - Flags: review?
changes to strings:

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

w/ those changes, okay'ed by mconnor via irc
Attached patch patch v.3 (obsolete) — Splinter Review
Attachment #336543 - Attachment is obsolete: true
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"?
Attachment #336555 - Flags: review+
Attached patch patch v.4 (obsolete) — Splinter Review
gavin's suggestions + a better way to find the right notification bar.
Attachment #336555 - Attachment is obsolete: true
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?
Attached patch patch v.5Splinter Review
what gavin and I discussed over irc.
Attachment #336570 - Attachment is obsolete: true
OS: Mac OS X → All
Hardware: PC → All
Version: unspecified → Trunk
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.
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.
Attached patch l10n noteSplinter Review
Vlado, please let me know if this l10n note will work.
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
dougt@mozilla.com
Wed Sep 03 15:05:29 2008 -0700	5075df1da267	Doug Turner — Bug 444642 - Hookup geolocation prompt to notification prompt. r=gavin/mconnor

and 

dougt@mozilla.com
Wed Sep 03 16:04:48 2008 -0700	d4b972bd979b	Doug Turner — Follow up to Bug 444642 - Add l10n note to geolocation properties. r=wladow@gmail.com
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
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
Blocks: 459413
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_>)?
@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.
Blocks: 459449
Blocks: 459893
Adrian filed bug 462199 to make the string keys line up with what our tools like.
Blocks: 462199
You need to log in before you can comment on or make changes to this bug.