Closed Bug 1209602 Opened 9 years ago Closed 9 years ago

XUL: Implement disabling notifications for a site

Categories

(Toolkit Graveyard :: Notifications and Alerts, defect)

Product:
Toolkit Graveyard
Component:
Notifications and Alerts
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(firefox44 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla44
Tracking Flags:
Tracking Status
firefox44 --- fixed

People

(Reporter: lina, Assigned: jaws)

References

Details

Attachments

(1 file, 2 obsolete files)

Patch with test (test file added to patch now)
14.43 KB, patch
MattN
: review+
Details | Diff | Splinter Review 
Add the gear icon and drop-down menu to `alert.xul`.
I'll pick this up since Kit is out on PTO.
Assignee: kcambridge → jaws
Requesting review from Gijs since Matt is out on PTO.
Attachment #8671599 - Flags: review?(gijskruitbosch+bugs)
Attachment #8671599 - Attachment description: Patch → Patch (dependent on the patch in bug 1201397)
Depends on: 1201397
Comment on attachment 8671599 [details] [diff] [review]
Patch (dependent on the patch in bug 1201397)

Review of attachment 8671599 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/alerts/resources/content/alert.js
@@ +13,5 @@
>  
>  Cu.import("resource://gre/modules/Services.jsm");
>  
> +const ALERT_BUNDLE = Services.strings.createBundle(
> +  "chrome://alerts/locale/alert.properties");

This can be moved inside of the `if (uri) {}` block.
Comment on attachment 8671599 [details] [diff] [review]
Patch (dependent on the patch in bug 1201397)

Review of attachment 8671599 [details] [diff] [review]:
-----------------------------------------------------------------

I think this is pretty close, but this is a pretty quick and somewhat shaky review - sorry, but I'm on (birthday) PTO myself tomorrow. :-(
Matt is only gone tomorrow still as well, so I'm under the impression you're under time pressure here.

I would prefer we use actual URI origins here, that I understood the permissions model here better, and that there was some kind of test for this stuff you're adding. If you want to follow-up-bug that and we're under time-pressure here, that works, but if not, it might be nice to see the updated patch.

::: toolkit/components/alerts/resources/content/alert.js
@@ +13,5 @@
>  
>  Cu.import("resource://gre/modules/Services.jsm");
>  
> +const ALERT_BUNDLE = Services.strings.createBundle(
> +  "chrome://alerts/locale/alert.properties");

Can you make this a lazy getter instead?

@@ +42,5 @@
>      case 11: {
> +      let uri = window.arguments[10] &&
> +                window.arguments[10].QueryInterface(Ci.nsIURI);
> +      if (uri) {
> +        let hostPort = uri.hostPort;

This will throw for e.g. about: or data: uris, so we should deal with that.

@@ +55,5 @@
> +        disableForOrigin.setAttribute("label",
> +          ALERT_BUNDLE.formatStringFromName("disableNotificationsForOrigin.label",
> +                                            [hostPort],
> +                                            1));
> +        gPrincipal = uri.scheme + "://" + hostPort;

If this is what we really want, maybe just pass the principal and use its origin property instead? That seems like it'll return the string you want here, and then we don't have to mess about ourselves (does this cope correctly with username + password in the URI, for instance?)

@@ +233,5 @@
> +function disableForOrigin() {
> +  if (gPrincipal) {
> +    Services.perms.remove(Services.io.newURI(gPrincipal, null, null),
> +                          "desktop-notification",
> +                          Ci.nsIPermissionManager.ALLOW_ACTION);

So is this deny by default or something? Should we make it explicitly record a deny instead? I'm actually not amazingly familiar with the permissions manager...
Attachment #8671599 - Flags: review?(gijskruitbosch+bugs) → review+
(In reply to :Gijs Kruitbosch (PTO oct 9) from comment #4)
> ::: toolkit/components/alerts/resources/content/alert.js
> @@ +13,5 @@
> >  
> >  Cu.import("resource://gre/modules/Services.jsm");
> >  
> > +const ALERT_BUNDLE = Services.strings.createBundle(
> > +  "chrome://alerts/locale/alert.properties");
> 
> Can you make this a lazy getter instead?

What benefit does a lazy getter here add? It will not be cached across instances of the window, so I just see it as adding more code.
 
> @@ +42,5 @@
> >      case 11: {
> > +      let uri = window.arguments[10] &&
> > +                window.arguments[10].QueryInterface(Ci.nsIURI);
> > +      if (uri) {
> > +        let hostPort = uri.hostPort;
> 
> This will throw for e.g. about: or data: uris, so we should deal with that.

Good catch.

> @@ +55,5 @@
> > +        disableForOrigin.setAttribute("label",
> > +          ALERT_BUNDLE.formatStringFromName("disableNotificationsForOrigin.label",
> > +                                            [hostPort],
> > +                                            1));
> > +        gPrincipal = uri.scheme + "://" + hostPort;
> 
> If this is what we really want, maybe just pass the principal and use its
> origin property instead? That seems like it'll return the string you want
> here, and then we don't have to mess about ourselves (does this cope
> correctly with username + password in the URI, for instance?)

Okay, I'll switch to passing in the principal. This code should be fine for username/password in the URL since it was not constructing it with the uri.userPass.

> @@ +233,5 @@
> > +function disableForOrigin() {
> > +  if (gPrincipal) {
> > +    Services.perms.remove(Services.io.newURI(gPrincipal, null, null),
> > +                          "desktop-notification",
> > +                          Ci.nsIPermissionManager.ALLOW_ACTION);
> 
> So is this deny by default or something? Should we make it explicitly record
> a deny instead? I'm actually not amazingly familiar with the permissions
> manager...

We decided to have "disable" mean "reset", which switched the permission back to "ask" (requiring an acceptance through the doorhanger before they can be shown again). Switching "allow" to "block" here was seen as too much of a foot-gun, as the site will no longer be able to prompt to reshow later.
Attached patch Patch with test (obsolete) — — Splinter Review 
Added test, redirecting final review to MattN since he should be back now.
Attachment #8671599 - Attachment is obsolete: true
Attachment #8672804 - Flags: review?(MattN+bmo)
I guess it's a good idea to `hg add` the test :P
Attachment #8672804 - Attachment is obsolete: true
Attachment #8672804 - Flags: review?(MattN+bmo)
Attachment #8672805 - Flags: review?(MattN+bmo)
Comment on attachment 8672805 [details] [diff] [review]
Patch with test (test file added to patch now)

Review of attachment 8672805 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks. If you make big changes I can take another look.

::: browser/base/content/test/alerts/browser.ini
@@ +2,5 @@
>  support-files =
>    file_dom_notifications.html
>  
>  [browser_notification_open_settings.js]
> +[browser_notification_remove_permission.js]

You will need to skip-if = e10s until the e10s bugs are fixed.

::: browser/base/content/test/alerts/browser_notification_remove_permission.js
@@ +1,3 @@
> +/* Any copyright is dedicated to the Public Domain.
> + * http://creativecommons.org/publicdomain/zero/1.0/
> + */

Nit: not necessary anymore

@@ +23,5 @@
> +  pm.add(makeURI(notificationURL), "desktop-notification", pm.ALLOW_ACTION);
> +
> +  tab = gBrowser.addTab(notificationURL);
> +  gBrowser.selectedTab = tab;
> +  tab.linkedBrowser.addEventListener("load", onLoad, true);

Why aren't you using add_task and BrowserTestUtils for a more readable test? I could like with this but for the future prefer add_task

@@ +29,5 @@
> +
> +function onLoad() {
> +  tab.linkedBrowser.removeEventListener("load", onLoad, true);
> +  let win = tab.linkedBrowser.contentWindow.wrappedJSObject;
> +  notification = win.showNotification2();

This may also have problems with e10s so ContentTask should be used…

::: toolkit/components/alerts/nsAlertsUtils.h
@@ -33,5 @@
> -   * Sets |aHostPort| to the host and port from |aPrincipal|'s URI, or an
> -   * empty string if |aPrincipal| is not actionable.
> -   */
> -  static void
> -  GetSourceHostPort(nsIPrincipal* aPrincipal, nsAString& aHostPort);

At least one of these methods are used by OS X so this will break those builds. We will need something similar for libnotify too.

::: toolkit/components/alerts/nsXULAlerts.cpp
@@ +137,5 @@
>    NS_ENSURE_SUCCESS(rv, rv);
>  
> +  // The principalURI contains the scheme and hostPort of the
> +  // website that requested the notification. Optional.
> +  if (nsAlertsUtils::IsActionablePrincipal(aPrincipal)) {

I think this comment needs to be updated as I don't think principalURI is a thing in this patch.

::: toolkit/components/alerts/resources/content/alert.js
@@ +32,5 @@
>    // arguments[6] --> bidi
>    // arguments[7] --> lang
>    // arguments[8] --> replaced alert window (nsIDOMWindow)
>    // arguments[9] --> an optional callback listener (nsIObserver)
> +  // arguments[10] -> the URI of the site that requested the notification, optional

s/URI/nsIPrincipal/

@@ +40,5 @@
>      case 11: {
> +      let principal = window.arguments[10] &&
> +                      window.arguments[10].QueryInterface(Ci.nsIPrincipal);
> +      let uri = principal.URI;
> +      if (uri && uri.scheme.startsWith("http")) {

It's discouraged to hard-code schemes. try…catch the hostPort getter instead to do like the C++ was. You're missing ftp: for example.

Ideally we would add a .idl for nsAlertUtils and share the implementation.

@@ +237,5 @@
>  }
>  
> +function disableForOrigin() {
> +  if (gPrincipal) {
> +    Services.perms.removeFromPrincipal(gPrincipal,

I think you should assume gPrincipal is truthy here and have it throw so tests fail if we ever call this function when there was no principal.

::: toolkit/locales/en-US/chrome/alerts/alert.properties
@@ +16,5 @@
>  source.label=via %1$S
>  webActions.settings.label = Notification settings
> +# LOCALIZATION NOTE(disableNotificationsForOrigin.label): %S is replaced
> +# with the hostname origin of the notification.
> +disableNotificationsForOrigin.label = Disable notifications from %S

Can you move this beside webActions.disable.label and follow that naming pattern e.g. "webAction.disableForOrigin.label"? We could possibly replace that string now that the OS X code has the origin available to do the substitution.
Attachment #8672805 - Flags: review?(MattN+bmo) → review+
Jared, I don't understand the benefit of getting rid of GetSource and GetSourceHostPort? We should have exposed it to XUL notifications via XPIDL instead of removing it and duplicating the logic 3 times.
Flags: needinfo?(jaws)
Comment on attachment 8672805 [details] [diff] [review]
Patch with test (test file added to patch now)

Review of attachment 8672805 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/alerts/resources/content/alert.js
@@ +238,5 @@
>  
> +function disableForOrigin() {
> +  if (gPrincipal) {
> +    Services.perms.removeFromPrincipal(gPrincipal,
> +                                       "desktop-notification");

I totally missed the fact that you were duplicating existing logic. You should have observed "alertdisablecallback" instead. Then you wouldn't have had to change AlertUtils and arguments[10].

if (gAlertListener) {
  gAlertListener.observe(null, "alertdisablecallback", gAlertCookie);
}
(In reply to Matthew N. [:MattN] (PTO Oct. 8-12) from comment #13)
> if (gAlertListener) {
>   gAlertListener.observe(null, "alertdisablecallback", gAlertCookie);
> }

You actually shouldn't need the `if` here for the same reason as what I said about `if (gPrincipal)`
Depends on: 1215409
In reply to comment 12 through comment 14:
Sorry for that confusion. It didn't seem duplicate to me until I had to make the same changes to the OSX notification code. I filed bug 1215409 to correct these, and have assigned it to myself.
Flags: needinfo?(jaws)
(In reply to Matthew N. [:MattN] (PTO Oct. 8-12) from comment #13)
> I totally missed the fact that you were duplicating existing logic. You
> should have observed "alertdisablecallback" instead. Then you wouldn't have
> had to change AlertUtils and arguments[10].

Either arguments[10] has to change or we need to add an arguments[11], since the hostPort is needed for the menuitem label ("Disable notifications for example.com"). nsAlertUtils::GetSource and GetSourceHostPort both take an nsIPrincipal, so exposing via XPIDL will still require that the JS has a reference to the principal.

So I see the two possible solutions as:

A) Remove nsAlertUtils::GetSource and use arguments[10] to pass in the nsIURI.hostPort. The consumer of this can combine this with a string bundle if they want to augment it with "via %S" or "Disable notifications from %S".

or B) Add an arguments[11] that is either the "Disable notifications from %S" or just the nsIURI.hostPort.
Flags: needinfo?(MattN+bmo)
I prefer option C which I believe I hinted at before:
Only pass the principal to the window, expose nsAlertUtils through IDL and use GetSource to substitute in "Disable notifications from %S"
Flags: needinfo?(MattN+bmo)
(In reply to Matthew N. [:MattN] from comment #19)
> I prefer option C which I believe I hinted at before:

See comment 12
After talking on IRC, I'm fine with option A if we don't need the principal for anything else.
But the code that passes the arguments to the window should use the shared nsAlertUtils logic.
Depends on: 1216585
No longer depends on: 1216585
https://hg.mozilla.org/mozilla-central/rev/1a8291d0b026
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox44: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
Product: Toolkit → Toolkit Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: