Add desktop notifications to clearable list of permissions

VERIFIED FIXED

Status

Firefox for Android Graveyard
General
P2
normal
VERIFIED FIXED
7 years ago
7 years ago

People

(Reporter: mfinkle, Assigned: mfinkle)

Tracking

Bug Flags:
in-testsuite ?

Details

(Whiteboard: [e1][strings][has-patch])

Attachments

(1 attachment)

We want to be able to track and clear the "allow desktop notifications" permission from the sitemenu. Needs to be added here:

http://mxr.mozilla.org/mobile-browser/source/chrome/content/browser-ui.js#1227
Flags: in-testsuite?

Updated

7 years ago
tracking-fennec: --- → 2.0+
But, within the "Clear Site Prefs" button -- let's not call it "desktop notifications."  Maybe "Website notfications" ?

Comment 2

7 years ago
madhava, no user facing change will occur with this bug.  it's all under the hood.
(In reply to comment #2)
> madhava, no user facing change will occur with this bug.  it's all under the
> hood.

Actually, there is a string involved. Fennec shows you a list of perms that would be reset if you push the "Clear Site Prefs" button
I have a patch in the works
Assignee: doug.turner → mark.finkle

Comment 5

7 years ago
oh, weak!  sorry.


"Web Notifications" is the spec name.  How about that?
Yeah, "Web Notifications" would work.
Actually, if this is a clearable pref, then a user will have been asked whether he/she will allow it first, no?
(In reply to comment #7)
> Actually, if this is a clearable pref, then a user will have been asked whether
> he/she will allow it first, no?

Correct. This already happens.
(Assignee)

Updated

7 years ago
Priority: -- → P2
Whiteboard: [e1]
Created attachment 507918 [details] [diff] [review]
patch

This patch does a few things. I found a couple bugs while working on the patch;
* Adds a clearable permission for web notifications
* Breaks the link between permission type and string entity. That's just too fragile.
* Resetting the geolocation perms was broken since "geo" != "geolocation"
* Resets the "remember" counter for geolocation and web notifications. We were just letting the counter go and it breaks the way the content permission manager works. It would always keep showing the user permission prompt after clearing the perms since the counter never == 5 again.
Attachment #507918 - Flags: review?(mbrubeck)
Whiteboard: [e1] → [e1][strings][has-patch]
Comment on attachment 507918 [details] [diff] [review]
patch

>+++ b/chrome/content/common-ui.js

>   // This is easy for an addon to add his own perm type here
>+  _permissions: ["popup", "offline-app", "geolocation", "desktop-notification"],

It occurs to me this comment might be a lie, since we use these names to look up strings in browser.properties, which add-ons can't easily extend (as far as I know).

>+++ b/components/ContentPermissionPrompt.js

>+const gEntities = { "geolocation": "geolocation", "desktop-notification": "desktopNotification" };

"const kEntities" to match our other global constants.

How much do we gain from adding this mapping?  It doesn't seem like these permission types should change in the future.  If we stuck with "desktop-notification" in the string IDs, we would avoid most of the l10n churn from this patch.  Also, it would be more consistent with the pageaction.* strings, which still use the permission type (e.g. "pageactions.desktop-notification").

r=mbrubeck but I would consider just using the permission types to build the string IDs (as we do now).
Attachment #507918 - Flags: review?(mbrubeck) → review+
I am more worried about the opposite problem. When the strings change, as strings do, we need to change the entity as well. The less we couple to the permission, the less likely someone will mistakenly change the wrong part of the entity and break the strings.
(In reply to comment #11)
> I am more worried about the opposite problem. When the strings change, as
> strings do, we need to change the entity as well. The less we couple to the
> permission, the less likely someone will mistakenly change the wrong part of
> the entity and break the strings.

Oh, that makes sense.
pushed with kEntities tweak:
http://hg.mozilla.org/mobile-browser/rev/1491d9596ae2
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
I've tested this, using:
https://developer.mozilla.org/samples/domref/desktopnotification.html

I can verify that this works correctly, using:
Mozilla/5.0 (Maemo; Linux armv7l; rv:2.0b13pre) Gecko/20110314
Firefox/4.0b13pre Fennec/4.0b6pre

and:
Mozilla/5.0 (Android; Linux armv7l; rv:2.0b13pre) Gecko/20110314
Firefox/4.0b13pre Fennec/4.0b6pre

However, I have some other (more or less) related questions about this.

- I'm getting the prompt: "developer.mozilla.org wants to use notifications", shouldn't that be called "web notifications"?
- I keep getting that prompt, even after repeatedly tapping on the "Allow" button. Only after tapping on the web notification itself (and repeating this a couple of times, 3 times it seems), the site preference for web notications gets stored finally.
- On Maemo, the desktop notification is like the popup notification, which disappears by itself after a while. But shouldn't then the close event fire?
- Tapping 4 times on the "Click here" link and then tapping on the web notification itself triggers 4 times the clicked event, then 4 times the closed event. Is this correct behavior? I'm only getting one web notification item on Maemo and one item in the notification bar on Android.
Status: RESOLVED → VERIFIED
(In reply to comment #14)

> However, I have some other (more or less) related questions about this.
> 
> - I'm getting the prompt: "developer.mozilla.org wants to use notifications",
> shouldn't that be called "web notifications"?

This string was already being used in the code. We can change it for Fennec.next if needed

> - I keep getting that prompt, even after repeatedly tapping on the "Allow"
> button. Only after tapping on the web notification itself (and repeating this a
> couple of times, 3 times it seems), the site preference for web notications
> gets stored finally.

Just like geo-location, notifications use a "if you answer the same 5 times in a row, we won't ask again" approach.

> - On Maemo, the desktop notification is like the popup notification, which
> disappears by itself after a while. But shouldn't then the close event fire?

No, only tapping on the nofitication should fire the event

> - Tapping 4 times on the "Click here" link and then tapping on the web
> notification itself triggers 4 times the clicked event, then 4 times the closed
> event. Is this correct behavior? I'm only getting one web notification item on
> Maemo and one item in the notification bar on Android.

I don't understand this one. Can you explain in more detail?
(In reply to comment #15)
> (In reply to comment #14)
> This string was already being used in the code. We can change it for
> Fennec.next if needed

I filed that as bug 641768 (not a big deal, though).

> Just like geo-location, notifications use a "if you answer the same 5 times in
> a row, we won't ask again" approach.

Ok, thanks for the info.

> > - On Maemo, the desktop notification is like the popup notification, which
> > disappears by itself after a while. But shouldn't then the close event fire?
> 
> No, only tapping on the nofitication should fire the event

When tapping on the "Clear" button on Android (When you open the notification slider, at the top, there is a Clear button right next to "Searching for Service"), the close event is fired too (only the close event, not the click event).

> > - Tapping 4 times on the "Click here" link and then tapping on the web
> > notification itself triggers 4 times the clicked event, then 4 times the closed
> > event. Is this correct behavior? I'm only getting one web notification item on
> > Maemo and one item in the notification bar on Android.
> 
> I don't understand this one. Can you explain in more detail?

Steps to reproduce:
- Tap 3 times on the "Click here" link (tap on the "Allow" button if needed)
- Tap on the notification alert

Result:
3 times "The notification was clicked." appears, then 3 times the "The notification was closed." appears.

I'm not sure if that is what should happen.
You need to log in before you can comment on or make changes to this bug.