605706 - Add desktop notifications to clearable list of permissions

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect, P2)

Description

[Mark Finkle (:mfinkle) (use needinfo?)]

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

Comment 1

[Madhava Enros [:madhava]]

But, within the "Clear Site Prefs" button -- let's not call it "desktop notifications."  Maybe "Website notfications" ?

Comment 2

[Doug Turner (:dougt)]

madhava, no user facing change will occur with this bug.  it's all under the hood.

Comment 3

[Mark Finkle (:mfinkle) (use needinfo?)]

(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

Comment 4

[Mark Finkle (:mfinkle) (use needinfo?)]

I have a patch in the works

Comment 5

[Doug Turner (:dougt)]

oh, weak!  sorry.


"Web Notifications" is the spec name.  How about that?

Comment 6

[Madhava Enros [:madhava]]

Yeah, "Web Notifications" would work.

Comment 7

[Madhava Enros [:madhava]]

Actually, if this is a clearable pref, then a user will have been asked whether he/she will allow it first, no?

Comment 8

[Mark Finkle (:mfinkle) (use needinfo?)]

(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.

Comment 9

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: 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.

Comment 10

[Matt Brubeck (:mbrubeck)]

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).

Comment 11

[Mark Finkle (:mfinkle) (use needinfo?)]

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.

Comment 12

[Matt Brubeck (:mbrubeck)]

(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.

Comment 13

[Mark Finkle (:mfinkle) (use needinfo?)]

pushed with kEntities tweak:
http://hg.mozilla.org/mobile-browser/rev/1491d9596ae2

Comment 14

[Martijn Wargers (dead)]

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.

Comment 15

[Mark Finkle (:mfinkle) (use needinfo?)]

(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?

Comment 16

[Martijn Wargers (dead)]

(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.