OS X: Implement disabling Notifications for the site

RESOLVED FIXED in Firefox 44

Status

()

defect
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: wmaggs, Assigned: MattN, NeedInfo)

Tracking

(Blocks 1 bug)

Trunk
mozilla44
All
macOS
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox43 affected, firefox44 fixed)

Details

User Story

Acceptance Critera: 

1. In OSX, generate Push-based notifications for all message types (title only, body text, etc) 
2. Verfiy that Disable Notifications for Site is choosable for each 
3. Verify disabelment by checking per-site permission (lock icon)

Attachments

(4 attachments)

+++ 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.
Blocks: 1201571
No longer depends on: 1201571
Assignee: nobody → MattN+bmo
Status: NEW → ASSIGNED
Iteration: --- → 44.1 - Oct 5
No longer depends on: 1201397, 1201398, 1202933, 1205172
Summary: Make the Push Notification Message Design on FF OSX match features of message design for Windows → OS X: Implement disabling Notifications for the site
Target Milestone: mozilla44 → ---
Version: 43 Branch → Trunk
Bug 1205399 - OS X: Implement disabling Notifications for the site from the native notifications.
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.
Attachment #8665035 - Flags: feedback?(wchen)
Attachment #8665035 - Flags: feedback?(nsm.nikhil)
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?
<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_ (Alessio@moz-kf56p4.retail.telecomitalia.it) 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 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
Attachment #8665035 - Attachment description: MozReview Request: Bug 1205399 - OS X: Implement disabling Notifications for the site from the native notifications. → MozReview Request: Bug 1205399 - Backend for supporting disabling of notifications for a site from the UI. r=nsm
Attachment #8665035 - Flags: review?(nsm.nikhil)
Attachment #8665035 - Flags: feedback?(wchen)
Attachment #8665035 - Flags: feedback?(nsm.nikhil)
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)`.
Attachment #8665035 - Flags: review?(nsm.nikhil)
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
Attachment #8665035 - Attachment description: MozReview Request: Bug 1205399 - Backend for supporting disabling of notifications for a site from the UI. r=nsm → MozReview Request: Bug 1205399 - Backend for disabling of notifications for a site from the UI. r=nsm
Attachment #8665035 - Flags: review?(nsm.nikhil)
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 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 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 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
Attachment #8665148 - Attachment description: MozReview Request: Bug 1205399 - OS X: Implement disabling Notifications for the site from the native notifications. → MozReview Request: Bug 1205399 - OS X: Implement disabling Notifications for the site from the native notifications. r=mstange
Attachment #8665148 - Flags: review?(mstange)
Comment on attachment 8665034 [details]
MozReview Request: Bug 1205399 - Remove some leftover Growl strings. r=mstange

https://reviewboard.mozilla.org/r/20127/#review18137
Attachment #8665034 - Flags: review?(mstange) → review+
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."
Attachment #8665148 - Flags: review?(mstange) → review+
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
Attachment #8665035 - Flags: review?(nsm.nikhil) → review+
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 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
Attachment #8665148 - Attachment description: MozReview Request: Bug 1205399 - OS X: Implement disabling Notifications for the site from the native notifications. r=mstange → MozReview Request: Bug 1205399 - OS X: Implement disabling Notifications for the site from the native notifications. r?mstange
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.
Attachment #8665148 - Flags: review+ → review?(mstange)
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
Flags: needinfo?(psackl)
Attachment #8665148 - Flags: review?(mstange) → review+
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
(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.
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.
Casting the switch statement as per http://stackoverflow.com/a/11076977 also worked for me.
Attachment #8666743 - Flags: review?(MattN+bmo)
Re-opening until build-bustage fix in comment 27 has landed, or separate bug has been filed.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment on attachment 8666743 [details] [diff] [review]
Follow-up to fix build bustage

stealing review
Attachment #8666743 - Flags: review?(MattN+bmo) → review+
(In reply to Markus Stange [:mstange] from comment #29)
> stealing review

Sorry, I was afk the last two days.
https://hg.mozilla.org/mozilla-central/rev/5cb68f2221bc
Status: REOPENED → RESOLVED
Closed: 4 years ago4 years ago
Resolution: --- → FIXED
User Story: (updated)
You need to log in before you can comment on or make changes to this bug.