Closed Bug 994093 Opened 10 years ago Closed 10 years ago

Provide toggle for Geolocation service (geo.enabled) in Privacy & Security preference pane

Categories

(SeaMonkey :: Preferences, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
seamonkey2.30

People

(Reporter: rsx11m.pub, Assigned: rsx11m.pub)

References

Details

Attachments

(2 files, 2 obsolete files)

While working on the about:rights update (bug 884098) I've noticed the awkward wording of the Location Aware Browsing disable instructions in the Firefox version. I know that this is an opt-in service (subject to bug 903439 to make it work again) but an easier way to disable it completely would be desirable.

For the main Privacy & Security pane, I'd envision something like:

 --- User Tracking ---
 ...

 --- Location Aware Browsing ---
 Websites may request to obtain more information about your current location.
 (*) Prompt me for permission if a website makes a request
 ( ) Disable this feature and deny all requests

 --- Safe Browsing ---
 ...

I'd prefer the 2-radiobutton version over a single checkbox as it allows to better convey what the options are doing (where the main point of annoyance is the doorhanger showing up when you don't really want to use that service). Using "more" is on purpose given that the IP address will be available anyway.
Depends on: 903439
Blocks: 844098
Blocks: 494424
Tentatively taking myself, opinions welcome.
Assignee: nobody → rsx11m.pub
> Tentatively taking myself, opinions welcome.
I say go for it!
Here we go. The patch is fairly straight-forward and just defines the suggested additional group for geo.enabled with the description slightly shortened to fit into a single line. While I'm there, I'm also cleaning up the attribute alignment in the DNT prefs introduced with bug 835134 and remove the redundant orientation attribute from that radiogroup.

The more interesting question is how to deal with bug 903439, given that this preference is a no-op right now. My suggestion here is to disable the group for release builds (assuming that #ifdef RELEASE_BUILD works in SeaMonkey the same way as it does for Firefox and Thunderbird where it's used for this purpose), thus it's available for the testers (and especially the strings are in already). Once the API issue is solved, it takes a simple follow-up patch to make it visible in release builds as well (I'll post that patch in bug 903439 once this gets approved with the exclusion).

Adding Help content for the Geolocation feature is bug 494424, and bug 998787 is aiming for a change in the doorhanger notification, thus I'm not adding anything here. I'll attend that bug once the preferences/notification UI is finalized.
Attachment #8409654 - Flags: ui-review?(neil)
Attachment #8409654 - Flags: review?(iann_bugzilla)
Status: NEW → ASSIGNED
Attached patch Patch with correct #ifndef (obsolete) — Splinter Review
Now, if I only had attached the correct patch...  :-[
Attachment #8409654 - Attachment is obsolete: true
Attachment #8409654 - Flags: ui-review?(neil)
Attachment #8409654 - Flags: review?(iann_bugzilla)
Attachment #8409655 - Flags: ui-review?(neil)
Attachment #8409655 - Flags: review?(iann_bugzilla)
(Why does this need an #ifdef?)
So that the UI is visible in builds of all channels but release until the Geo API is back.
Is there a better way to determine which channel we are on while waiting for bug 903439?

I don't want to overdo it and introduce some scripting for this, the preprocessor handling is what Thunderbird and Firefox are doing in such cases and can be easily reverted in a follow-up.
Something like

  if(Services.prefs.getDefaultBranch("").getCharPref("app.update.channel") == "release")
    document.getElementById("geoLocationGroup").hidden = true;

should do the same on the script side, if you'd prefer that over the preprocessing.
(In reply to rsx11m from comment #7)
> So that the UI is visible in builds of all channels but release until the
> Geo API is back.
> Is there a better way to determine which channel we are on while waiting for
> bug 903439?

Well, that's three months away, you never know, bug 903439 might have been resolved by then... if it hasn't, perhaps we can back out the content part of this patch on the appropriate repo, if Callek's OK with that.

(In reply to rsx11m from comment #8)
> Something like
> 
>   if(Services.prefs.getDefaultBranch("").getCharPref("app.update.channel")
> == "release")
>     document.getElementById("geoLocationGroup").hidden = true;
> 
> should do the same on the script side, if you'd prefer that over the
> preprocessing.

Actually it wouldn't work for distro packages, because they use the default channel.
Flags: needinfo?(bugspam.Callek)
(In reply to neil@parkwaycc.co.uk from comment #9)
> Well, that's three months away, you never know, bug 903439 might have been
> resolved by then...

Sure, would be nice, but then - that issue is pending since August 2013 now, thus my thinking.
Comment on attachment 8409655 [details] [diff] [review]
Patch with correct #ifndef

>+<!ENTITY geoLocation.label                "Location Aware Browsing">
>+<!ENTITY geoIntro.label                   "Websites may request more information about your current location.">
>+
>+<!ENTITY geoEnabled.label                 "Prompt me for permission if a website makes a request">
Perhaps "Prompt me for permission if a request is made"?
>+<!ENTITY geoEnabled.accesskey             "m">
>+<!ENTITY geoDisabled.label                "Disable this feature and deny all requests">
>+<!ENTITY geoDisabled.accesskey            "D">

Yet to test.
(In reply to Ian Neal from comment #11)
> Perhaps "Prompt me for permission if a request is made"?

Makes sense, given that the description makes the "website requests" context of the label unambiguous.
Comment on attachment 8409655 [details] [diff] [review]
Patch with correct #ifndef

r=me with that wording change
Attachment #8409655 - Flags: review?(iann_bugzilla) → review+
Per today's discussion with mcsmurf and Ratty over IRC in the status meeting, it appears that a solution to the Geolocation API key isn't coming any time soon. Thus, I'll stick with the #ifndef solution unless Callek tells me otherwise, and we can back it out again at the time when such a key becomes available.

Carrying forward r=IanN from attachment 8409655 [details] [diff] [review] with string changed.
Attachment #8409655 - Attachment is obsolete: true
Attachment #8409655 - Flags: ui-review?(neil)
Attachment #8414447 - Flags: ui-review?(neil)
Attachment #8414447 - Flags: review+
Neil: Ping for ui-review, based on his silence it doesn't seem that Callek has any strong preference.
Blocks: 1016577
No longer blocks: 844098
Neil, Callek: Any suggestion how to proceed here?
Flags: needinfo?(neil)
Comment on attachment 8414447 [details] [diff] [review]
Patch with updated label

Sorry for dropping the ball here.
Attachment #8414447 - Flags: ui-review?(neil) → ui-review+
Thanks, cancelling the needinfo requests and adding this to the check-in queue then.
Flags: needinfo?(neil)
Flags: needinfo?(bugspam.Callek)
Keywords: checkin-needed
Bah, my comment didn't auto-cancel the needinfo :-(
https://hg.mozilla.org/comm-central/rev/8613ef2656da
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → seamonkey2.30
You need to log in before you can comment on or make changes to this bug.