1205399 - OS X: Implement disabling Notifications for the site

Status: RESOLVED FIXED

(Toolkit :: Notifications and Alerts, defect)

Description

[Bill Maggs (bmaggs)]

+++ This bug was initially created as a clone of Bug #1201397 +++

The FF Windows user needs an actionable anchor to access an interface to turn off or turn down Push notifications for the sites the user has granted permission to send them. To do this currently the user must click on the message, then know where to go on the page to access Control Center, which exposes the permission to receive push notifications, as well as show notifications. It only works for one site, and if the user finds herself making a company presentation after allowing several website pages in FF to receive push notifications, it becomes a huge problem to manage them site by site. She needs an immediate way to get to the notification permissions in one place. 

A simple anchor would be something like a settings cog in the corner of every message on FF Windows (see attachment for concept, NOT finished mock). 

In FF 43 this cog would link to a centralized interface to manage Push notifications, described in Bug  

For FF 42, this cog graphic involves no strings and could possibly be uplifted  A more simple management interface than the one described in this bug would probably be necessary when the cog is clicked on.


This bug requires using the documented (and perhaps undocumented) features of Notifications in OSX to provide as similar an experience as possible to messages shown and managed on Ff Windows. This includes a button or control to allow the user to manage the notifications per site or centrally, as well as a Do Not Disturb button.

Comment 1

[Matthew N. [:MattN]]

Attachment: MozReview Request: Bug 1205399 - Remove some leftover Growl strings. r=mstange

Bug 1205399 - Remove some leftover Growl strings. r=mstange

Comment 2

[Matthew N. [:MattN]]

Attachment: MozReview Request: Bug 1205399 - Backend for disabling of notifications for a site from the UI. r=nsm

Bug 1205399 - OS X: Implement disabling Notifications for the site from the native notifications.

Comment 3

[Matthew N. [:MattN]]

Comment on attachment 8665035 [details]
MozReview Request: Bug 1205399 - Backend for disabling of notifications for a site from the UI. r=nsm

William, Nikhil: What is a good architecture to support revoking the permission from the 3 contexts I see: regular content, web worker and service worker? Do I have to duplicate logic in all of them to remove the permission or is there a better place I should put an observer so that we don't need to duplicate this for each context (worker, etc.) or UI backend (XUL/OS X/libnotify)? Any observer in the correct process can easily remove the permission since the nsIPrincipal is passed as the subject.

Feel free to ping me to discuss on IRC if you prefer.

Comment 4

[Matthew N. [:MattN]]

Also, I'm proposing that the alert service will take care of adding additional actions for any nsIPrincipal representing a domain rather than add more arguments to the alert service method and passing all of the additional option strings in. Any objections or thoughts on that?

Comment 5

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

<nsm|away> so one way to refactor the observer code in Notification.cpp is to have a common observer that does all the main thread only actions like revoking permissions, and then delegate to the individuals for the click/close. also 'onshow' has been removed, so we can get rid of it
<-- Dexter_ ([e-mail]) has quit (Quit: Leaving)
<nsm|away> since ::Observe always runs on the main thread, this should be fine
<nsm|away> the worker observer is only complicated due to ownership stuff

Comment 6

[Matthew N. [:MattN]]

Comment on attachment 8665035 [details]
MozReview Request: Bug 1205399 - Backend for disabling of notifications for a site from the UI. r=nsm

Bug 1205399 - Backend for supporting disabling of notifications for a site from the UI. r=nsm

Comment 7

[Matthew N. [:MattN]]

Attachment: MozReview Request: Bug 1205399 - OS X: Implement disabling Notifications for the site from the native notifications. r?mstange

Bug 1205399 - OS X: Implement disabling Notifications for the site from the native notifications.

Comment 8

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Comment on attachment 8665035 [details]
MozReview Request: Bug 1205399 - Backend for disabling of notifications for a site from the UI. r=nsm

https://reviewboard.mozilla.org/r/20129/#review18095

Almost there.

::: dom/notification/Notification.cpp:658
(Diff revision 2)
>  class NotificationObserver : public nsIObserver

`class NotificationObserver final`

::: dom/notification/Notification.cpp:666
(Diff revision 2)
> -  explicit NotificationObserver(UniquePtr<NotificationRef> aRef)
> +  explicit NotificationObserver(nsIObserver* aObserver, nsIPrincipal* aPrincipal)

Nit: Don't need explicit anymore since it takes 2 arguments.

::: dom/notification/Notification.cpp:668
(Diff revision 2)
> +    mObserver = aObserver;
> +    mPrincipal = aPrincipal;

Please use member initialization list syntax for setting members in the constructor - http://stackoverflow.com/questions/1711990/what-is-this-weird-colon-member-syntax-in-the-constructor

::: dom/notification/Notification.cpp:1154
(Diff revision 2)
> +  MOZ_ASSERT(mObserver);

This assert can be moved to the constructor.

::: dom/notification/Notification.cpp:1157
(Diff revision 2)
> +    nsCOMPtr<nsIPermissionManager> permissionManager = services::GetPermissionManager();
> +    MOZ_ASSERT(mPrincipal);

I'd prefer using the fully qualified `mozilla::services::GetPermissionManager()`

Please add a 

    if (!permissionManager) {
      return NS_ERROR_FAILURE;
    }
    
since it is quite possible we are shutting down or something and may not get a permission manager.

::: dom/notification/Notification.cpp:1158
(Diff revision 2)
> +    MOZ_ASSERT(mPrincipal);

No need for this, the fact that you are using nsCOMPtr<> guarantees this will be valid. I'd rather you move this assertion to the constructor so nobody passes a null principal to the constructor.

::: dom/notification/Notification.cpp:1160
(Diff revision 2)
> +    printf("removing permission\n");

You need to return here so we don't call mObserver->Observe() when we already dealt with the event.

Nit: get rid of the prinft.

::: dom/notification/Notification.cpp:1436
(Diff revision 2)
> -    // The observer is wholly owned by the alerts service.
> +    // The observer is wholly owned by the alerts service. // TODO

I think the only TODO here is that it is now owned by the top level observer, so only the comment needs fixing.

::: dom/notification/Notification.cpp:1477
(Diff revision 2)
>          appNotifier->ShowAppNotification(iconUrl, mTitle, mBody,
>                                           observer, val);
>          return;
>        }
>      }

We have this other callsite for b2g where you'll have to do the same thing :(

::: dom/notification/Notification.cpp:1508
(Diff revision 2)
> +
> +  nsCOMPtr<nsIObserver> alertObserver = new NotificationObserver(observer,
> +                                                                 GetPrincipal());
> +

I'd highly recommend you move this initialization to the top around line 196 right after the `MOZ_ASSERT(observer)`.

Comment 9

[Matthew N. [:MattN]]

Comment on attachment 8665035 [details]
MozReview Request: Bug 1205399 - Backend for disabling of notifications for a site from the UI. r=nsm

Bug 1205399 - Backend for disabling of notifications for a site from the UI. r=nsm

Comment 10

[Matthew N. [:MattN]]

Comment on attachment 8665148 [details]
MozReview Request: Bug 1205399 - OS X: Implement disabling Notifications for the site from the native notifications. r?mstange

Bug 1205399 - OS X: Implement disabling Notifications for the site from the native notifications.

Comment 11

[Matthew N. [:MattN]]

Comment on attachment 8665034 [details]
MozReview Request: Bug 1205399 - Remove some leftover Growl strings. r=mstange

Bug 1205399 - Remove some leftover Growl strings. r=mstange

Comment 12

[Matthew N. [:MattN]]

Comment on attachment 8665035 [details]
MozReview Request: Bug 1205399 - Backend for disabling of notifications for a site from the UI. r=nsm

Bug 1205399 - Backend for disabling of notifications for a site from the UI. r=nsm

Comment 13

[Matthew N. [:MattN]]

Comment on attachment 8665148 [details]
MozReview Request: Bug 1205399 - OS X: Implement disabling Notifications for the site from the native notifications. r?mstange

Bug 1205399 - OS X: Implement disabling Notifications for the site from the native notifications. r=mstange

Comment 14

[Markus Stange [:mstange]]

Comment on attachment 8665034 [details]
MozReview Request: Bug 1205399 - Remove some leftover Growl strings. r=mstange

https://reviewboard.mozilla.org/r/20127/#review18137

Comment 15

[Markus Stange [:mstange]]

Comment on attachment 8665148 [details]
MozReview Request: Bug 1205399 - OS X: Implement disabling Notifications for the site from the native notifications. r?mstange

https://reviewboard.mozilla.org/r/20149/#review18135

::: widget/cocoa/OSXNotificationCenter.mm:32
(Diff revision 3)
> +  NSUserNotificationActivationTypeReplied = 3,
> +  NSUserNotificationActivationTypeAdditionalActionClicked = 4

These two need to be moved out of the MAC_OS_X_VERSION_10_8 #if because they were introduced in 10.9 and 10.10, respectively. You need to wrap each of them in their own enum and #if block.
With the current code, people who compile with the 10.8 SDK or the 10.9 SDK will probably get compilation errors, because this #if block will not be compiled and the 10.8/10.9 SDK header won't contain the definitions of these enums either.

::: widget/cocoa/OSXNotificationCenter.mm:96
(Diff revision 3)
> -  mOSXNC->OnClick([[notification userInfo] valueForKey:@"name"]);
> +  NSNumber *alternateActionIndex = [(NSObject*)notification valueForKey:@"_alternateActionIndex"];

Is this the right way to do this even on 10.10, where additionalActions was introduced as a public API?

::: widget/cocoa/OSXNotificationCenter.mm:231
(Diff revision 3)
> +  // Don't show additional actions dealing with permissions for application/extension alerts.

I think it would be slightly clearer if this comment were reversed: "If this is not an application/extension alert, show additional actions dealing with permissions."

Comment 16

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Comment on attachment 8665035 [details]
MozReview Request: Bug 1205399 - Backend for disabling of notifications for a site from the UI. r=nsm

https://reviewboard.mozilla.org/r/20129/#review18217

Comment 17

[Matthew N. [:MattN]]

https://reviewboard.mozilla.org/r/20149/#review18135

> Is this the right way to do this even on 10.10, where additionalActions was introduced as a public API?

notification.additionalActivationAction and notification.additionalActions seems to always be `nil` on 10.10 for me (even with the 10.10 SDK), even if I make the notification an "alert" type. I couldn't find any other code on Github or Google succesfully using these new properties either. It's also weird that the NSUserNotificationAction class isn't documented.

Comment 18

[Matthew N. [:MattN]]

Comment on attachment 8665148 [details]
MozReview Request: Bug 1205399 - OS X: Implement disabling Notifications for the site from the native notifications. r?mstange

Bug 1205399 - OS X: Implement disabling Notifications for the site from the native notifications. r?mstange

Comment 19

[Matthew N. [:MattN]]

Comment on attachment 8665148 [details]
MozReview Request: Bug 1205399 - OS X: Implement disabling Notifications for the site from the native notifications. r?mstange

I ended up having to implement didDismissAlert otherwise the webpage wouldn't get the close event if the user clicked the Close button.

Comment 20

[Matthew N. [:MattN]]

Philipp, your spec[1] says to set the permission to "block" but my understanding is that the url bar icon with a strikethrough for blocked permissions isn't being worked on for 44 so it may not be discoverable how to change ones mind. With "block" specified, the site won't be able to ask for the permission again until the user reverts their decision in some secondary UI.

Do you think we should set to "block" for 44 or the default which means they won't work but a site can still make the doorhanger appear again upon future visits?

[1] https://mozilla.invisionapp.com/share/3A4A895XZ#/screens/103249711

Comment 21

[Markus Stange [:mstange]]

Comment on attachment 8665148 [details]
MozReview Request: Bug 1205399 - OS X: Implement disabling Notifications for the site from the native notifications. r?mstange

https://reviewboard.mozilla.org/r/20149/#review18307

Comment 22

[Bill Maggs (bmaggs)]

(In reply to Matthew N. [:MattN] (behind on mail) from comment #20)
> Philipp, your spec[1] says to set the permission to "block" but my
> understanding is that the url bar icon with a strikethrough for blocked
> permissions isn't being worked on for 44 so it may not be discoverable how
> to change ones mind. With "block" specified, the site won't be able to ask
> for the permission again until the user reverts their decision in some
> secondary UI.
> 
> Do you think we should set to "block" for 44 or the default which means they
> won't work but a site can still make the doorhanger appear again upon future
> visits?
> 
> [1] https://mozilla.invisionapp.com/share/3A4A895XZ#/screens/103249711

I think we set to the default because is what the user will expect. When the user gets bugged agina by the site they respond in the prompt to block permanently.

Comment 23

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/a79534562dd8
https://hg.mozilla.org/integration/fx-team/rev/1c884fcfdf38
https://hg.mozilla.org/integration/fx-team/rev/9898e4c13876

Comment 24

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/e597556a8433

Comment 25

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/a79534562dd8
https://hg.mozilla.org/mozilla-central/rev/1c884fcfdf38
https://hg.mozilla.org/mozilla-central/rev/9898e4c13876
https://hg.mozilla.org/mozilla-central/rev/e597556a8433

Comment 26

[Kartikaya Gupta (email:[e-mail])]

This patch appears to have broken my build on OS X 10.9.5 because it can't find NSUserNotificationActivationTypeAdditionalActionClicked. Do I need to upgrade XCode or something? Relevant portion of the build log at https://pastebin.mozilla.org/8847697 - I verified that backing out these patches gets my build past that error.

Comment 27

[Kartikaya Gupta (email:[e-mail])]

Attachment: Follow-up to fix build bustage

Casting the switch statement as per http://stackoverflow.com/a/11076977 also worked for me.

Comment 28

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Re-opening until build-bustage fix in comment 27 has landed, or separate bug has been filed.

Comment 29

[Markus Stange [:mstange]]

Comment on attachment 8666743 [details] [diff] [review]
Follow-up to fix build bustage

stealing review

Comment 30

[Matthew N. [:MattN]]

(In reply to Markus Stange [:mstange] from comment #29)
> stealing review

Sorry, I was afk the last two days.

Comment 31

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5cb68f2221bc

Comment 32

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/5cb68f2221bc