774506 - Implement toast notification support

Status: RESOLVED FIXED

(Firefox Graveyard :: SocialAPI, defect)

Comment 1

[Mark Hammond [:markh] [:mhammond]]

Attachment: Add 'social.notification-create' message to the worker api.

A patch that borrows the same basic functionality from the addon.

* The test passes but *only* if you manually click on the notification that appears when running the tests.  I obviously hope to fix that but thought I'd get feedback first.

* The addon has a provider attribute 'notificationsPermitted' which indicates whether notifications for the provider are actually shown.  Do we still want this attribute?  If so, it implies some UI for managing it, but the code currently displays such notifications unconditionally.

* The addon used to allow the provider to set the 'title' of the notification, but as per bug 755138, the provider 'name' is always used as the title.  Sound ok?

* The code appears to allow a provider to have a worker URL, which seems wrong, but is leveraged in this test (a data: url is used).  Not sure what thoughts are on this.

Comment 2

[Mark Hammond [:markh] [:mhammond]]

(In reply to Mark Hammond (:markh) from comment #1)
> * The code appears to allow a provider to have a worker URL, which seems
> wrong,

I meant to say "to allow a provider to have a worker URL in a different origin than the provider itself, which seems wrong ..."

Comment 3

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 643276 [details] [diff] [review]
Add 'social.notification-create' message to the worker api.


>+      alertsService.showAlertNotification(icon,
>+        this._provider.name, // notification title
>+        body,
>+        !!id, // text clicakble if an ID was provided.
>+        null, // cookie
>+        listener, // listener
>+        null); // name


We should set name to something (data.name ?), per docs at https://developer.mozilla.org/en/XPCOM_Interface_Reference/nsIAlertsService, some alert handlers use name for filtering.  We could document a couple standard names, such as below, or at least document that the name should be consistent for any given type of notification:

"social-chat-request" - a friend wants to chat
"social-activity-update" - a friend updated their activity stream
"social-recommend" - a friend recommended something

I'm also wondering if we should support cookie, it may be useful for bug 770366.  When a user clicks on a "social-chat-request", we will open the service window directly.  Passing a provider-provided-cookie to the service window may be useful for the provider to hook up the chat with the correct user.  OTH, the cookie could be the url to the chat window, or perhaps a json blob with both.  So perhaps cookie is { data: {}, serviceURL: https://... } and we'd validate same-origin of the serviceURL.

What is the content of id?

I'm not clear about your comment on the worker url, I dont see where any url could be passed through in this patch.

Comment 4

[Shane Caraveo (:mixedpuppy)]

(In reply to Mark Hammond (:markh) from comment #1)

> * The addon has a provider attribute 'notificationsPermitted' which
> indicates whether notifications for the provider are actually shown.  Do we
> still want this attribute?  If so, it implies some UI for managing it, but
> the code currently displays such notifications unconditionally.

I'd put that in another bug for fx17, and ignore it for fx16, just allow notifications.

> * The addon used to allow the provider to set the 'title' of the
> notification, but as per bug 755138, the provider 'name' is always used as
> the title.  Sound ok?

+1.  The body can contain any necessary detail info.

Comment 5

[Mark Hammond [:markh] [:mhammond]]

(In reply to Shane Caraveo (:mixedpuppy) from comment #3)

> We should set name to something (data.name ?), per docs at
> https://developer.mozilla.org/en/XPCOM_Interface_Reference/nsIAlertsService,
> some alert handlers use name for filtering.  We could document a couple
> standard names, such as below, or at least document that the name should be
> consistent for any given type of notification:
> 
> "social-chat-request" - a friend wants to chat
> "social-activity-update" - a friend updated their activity stream
> "social-recommend" - a friend recommended something

How would this be implemented in practice?  Are you saying we just accept that name as a param, document a list then rely on the providers do do the right thing?

> I'm also wondering if we should support cookie, it may be useful for bug
> 770366.  When a user clicks on a "social-chat-request", we will open the
> service window directly.  Passing a provider-provided-cookie to the service
> window may be useful for the provider to hook up the chat with the correct
> user.  OTH, the cookie could be the url to the chat window, or perhaps a
> json blob with both.  So perhaps cookie is { data: {}, serviceURL:
> https://... } and we'd validate same-origin of the serviceURL.
> 
> What is the content of id?

The ID is currently being used as a 'cookie' and I think it would be fine to pass that ID as the cookie param, even though it isn't necessary in the patch (as the 'id' is available via a closure and is already passed back in the click handler).

I'm disagree we should formalize a mapping of name->action but instead let the provider take whatever action it desires in the click callback - eg, the provider may choose to do something directly in the sidebar and may not want us to take some predetermined action.  IOW, I'm concerned that us attempting to guess all possible actions will not cover all the actual use-cases of all providers.

> I'm not clear about your comment on the worker url, I dont see where any url
> could be passed through in this patch.

That comment was a side-note that nothing in the system currently enforces a workerURL is in the same origin as the provider, which allowed me to register a provider with a data: URL.  IOW, this patch doesn't *cause* that behaviour, just that the test *leverages* it.

So if I'm reading this correctly, the only thing we need is a new param called "name" and some decision on whether we should tightly couple a name->action mapping which covers all possible provider use-cases for such notifications?

Comment 6

[Shane Caraveo (:mixedpuppy)]

(In reply to Mark Hammond (:markh) from comment #5)
> (In reply to Shane Caraveo (:mixedpuppy) from comment #3)
> 
> > We should set name to something (data.name ?), per docs at
> > https://developer.mozilla.org/en/XPCOM_Interface_Reference/nsIAlertsService,
> > some alert handlers use name for filtering.  We could document a couple
> > standard names, such as below, or at least document that the name should be
> > consistent for any given type of notification:
> > 
> > "social-chat-request" - a friend wants to chat
> > "social-activity-update" - a friend updated their activity stream
> > "social-recommend" - a friend recommended something
> 
> How would this be implemented in practice?  Are you saying we just accept
> that name as a param, document a list then rely on the providers do do the
> right thing?

yes.

> > I'm also wondering if we should support cookie, it may be useful for bug
> > 770366.  When a user clicks on a "social-chat-request", we will open the
> > service window directly.  Passing a provider-provided-cookie to the service
> > window may be useful for the provider to hook up the chat with the correct
> > user.  OTH, the cookie could be the url to the chat window, or perhaps a
> > json blob with both.  So perhaps cookie is { data: {}, serviceURL:
> > https://... } and we'd validate same-origin of the serviceURL.
> > 
> > What is the content of id?
> 
> The ID is currently being used as a 'cookie' and I think it would be fine to
> pass that ID as the cookie param, even though it isn't necessary in the
> patch (as the 'id' is available via a closure and is already passed back in
> the click handler).

We probably dont need to pass anything for cookie then, it was just a thought after looking at the api. 

> I'm disagree we should formalize a mapping of name->action but instead let
> the provider take whatever action it desires in the click callback - eg, the
> provider may choose to do something directly in the sidebar and may not want
> us to take some predetermined action.  IOW, I'm concerned that us attempting
> to guess all possible actions will not cover all the actual use-cases of all
> providers.

We currently have a requirement (bug 770366) that clicking on the notification for a chat will open the chat window.  The worker wont be able to, and the sidebar may not be available to handle that.

> > I'm not clear about your comment on the worker url, I dont see where any url
> > could be passed through in this patch.
> 
> That comment was a side-note that nothing in the system currently enforces a
> workerURL is in the same origin as the provider, which allowed me to
> register a provider with a data: URL.  IOW, this patch doesn't *cause* that
> behaviour, just that the test *leverages* it.
> 
> So if I'm reading this correctly, the only thing we need is a new param
> called "name" and some decision on whether we should tightly couple a
> name->action mapping which covers all possible provider use-cases for such
> notifications?

We dont need to cover all possible use cases, we can document recommended names for common use cases that help providers be good citizens.  As long as the names are consistent (at least per-provider) for the same type of notification, the user can filter.

We also need the ability to get at any data that we will need to initiate the chat window, which is what I am suggesting the use of cookie for, but I'm sure there are plenty of ways to slice that bread.

Comment 7

[Mark Hammond [:markh] [:mhammond]]

We could support an additional "action" param and support 2 actions in the first cut:

{action: "callback"} - means that we simply notify the worker when the item was clicked and the worker can take any action it can.  This is basically doing what the patch does now.

{action: "openServiceWindow", actionArgs: {...}} - means we call the openServiceWindow function from bug 770695 using the params in actionArgs.

Later we can invent new "action" strings - eg, I can foresee an action "openSidebar"...)

Does that sound like what you are after?

Comment 8

[Shane Caraveo (:mixedpuppy)]

looking towards a future where we may or may not have openServiceWindow I would rather see the action named more to the action, perhaps something like "social-chat-session".  Otherwise, all sounds cool.

Comment 9

[Mark Hammond [:markh] [:mhammond]]

Attachment: updated based on feedback

A new version based on discussions with Shane.  This version:

* Wants 2 new params - 'action' and 'actionArgs' - the contents of the latter depends on the value of the former.

* An action 'callback' is fully implemented.  If the user clicks on the toast, we immediately send a message to the worker.

* An action 'openServiceWindow' is partially implemented (as openServiceWindow hasn't landed yet).  On click, a serviceWindow is opened and when it is ready we send a message to the worker.  Note that 'openServiceWindow' is really a temporary solution and will likely be replaced later with 'openChatSession' or something similar.

* In all cases, the message sent back to the worker has the same basic layout - the topic is 'social.notification-action' and we send back the same 'action' and 'actionArgs' they initially specified.

* There is no 'id' or 'cookie' param - it is unnecessary given we send back whatever actionArgs they sent us (ie, they could stick some unique identifier directly in actionArgs if they choose).

* There is a 'name' param which growl filters can use, but currently this isn't required.

* There is a test but it only works on platforms without a "system alert service" - ie, in practice, it probably only works on Windows.  It *should* gracefully skip the test on other platform but I've not tested that.

Looking for feedback and if its OK I'll update the patch once openServiceWindow lands.

Comment 10

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 644121 [details] [diff] [review]
updated based on feedback

I think there should be a non-empty default for name, maybe "social.notification", so there is some default for filtering.  Also, the implementation for the service window no longer has a callback, if it needs to be done after the window open, use a load listener on the window returned by openServiceWindow.  Since the service window can make its own connection to the worker, I'm not certain it needs to wait for that.

Comment 11

[Shane Caraveo (:mixedpuppy)]

openServiceWindow has landed

Comment 12

[Shane Caraveo (:mixedpuppy)]

Attachment: updated patch

update of marks patch with openServiceWindow enabled, test passes for me, though I have to manually click on the growl notification.

Comment 13

[:Gavin Sharp [email: [e-mail]]]

Attachment: modified patch

I made a few minor tweaks and updated the patch to the new openServiceWindow signature (no callback, need to pass provider and window) but ran into one problem: what window should this code use to open the service window? there's no obvious choice, since there may not be an open sidebar or panel window, and we can't really use the hidden window. Seems like we probably need a better opening solution that doesn't depend on having an existing window for that origin.

(I haven't touched the test, I'm not sure that it still works.)

Comment 14

[Shane Caraveo (:mixedpuppy)]

Attachment: dueling patches

here's a patch with a working window opener for the toasts.  as well it makes the default click action open a url in tab.  haven't modified tests, and haven't tested if a window originally opened in sidebar will be correctly selected (or vise versa).  This is primarily for feedback on the window opening function.

Comment 15

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 647376 [details] [diff] [review]
dueling patches

I'm afraid I can't see the justification for the duplicate code.  It also means that the "service window" will be subtly different depending on who first gets to create it (at the very least, window.opener will be null in one case and a sidebar in another IIUC).  However, I'm happy for Gavin to make a call on this, so I'll just remove myself from the feedback request.

Comment 16

[Shane Caraveo (:mixedpuppy)]

Attachment: notifications with modified openServiceWindow

Notifications and click support for openServiceWindow

There is no way to do a sync openServiceWindow from the notification click, as well our older openServiceWindow was not as synchronous as thought.  This changes openServiceWindow back to having a callback, but now with the window passed as an argument to that.

Comment 20

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 648096 [details] [diff] [review]
notifications with modified openServiceWindow

Few trivial notes I mentioned to Shane in IRC, but this looks good.

Comment 21

[Shane Caraveo (:mixedpuppy)]

Attachment: notifications with modified openServiceWindow

rebased on top of bugs 779686 (chat windows) and 779923 (flyout panels)

Comment 22

[Shane Caraveo (:mixedpuppy)]

Attachment: rebased

Comment 23

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 653975 [details] [diff] [review]
rebased

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

tiny nit: please check these files for trailing whitespace and remove it :)

::: browser/base/content/test/social_sidebar.html
@@ +19,5 @@
>                });
>                break;
>              case "test-service-window":
> +              navigator.mozSocial.openServiceWindow("https://example.com/browser/browser/base/content/test/social_window.html", "test-service-window", "width=300,height=300",
> +                                                    function(theWin) {

nit: please wrap these long lines to ~80 chars per line, since it's getting hard to do side-by-side review

::: toolkit/components/social/MozSocialAPI.jsm
@@ +97,5 @@
>      openServiceWindow: {
>        enumerable: true,
>        configurable: true,
>        writable: true,
> +      value: function(toURL, name, options, callback) {

Why do we need to allow the window opening "options" to be provided by the caller? It seems that we should be able to determine the correct window options and not let callers change them.

@@ +325,5 @@
> +    // we dont want the default title the browser produces, we'll fixup whenever
> +    // it changes.
> +    chromeWindow.addEventListener("DOMTitleChanged", function() {
> +      let sep = chromeWindow.document.documentElement.getAttribute("titlemenuseparator");
> +      chromeWindow.document.title = provider.name + sep + chromeWindow.gBrowser.contentWindow.document.title;

This should be |document.title + sep + provider.name| so that it is easier to find the window that the user is looking for.

Will this cause an infinite loop of DOMTitleChanged events?

Axel, will constructing a window title like this be OK for localization? I'm worried about the fixed ordering here.

::: toolkit/components/social/WorkerAPI.jsm
@@ +90,5 @@
> +              let xulWindow = Services.wm.getMostRecentWindow("navigator:browser").getTopWin();
> +              openChatWindow(xulWindow, provider, actionArgs.toURL);
> +              break;
> +            case "openServiceWindow":
> +              openServiceWindow(provider,

Let's remove the openServiceWindow additions from this patch. The comment earlier in this file says it is "likely to be deprecated and replace with something like 'openChatWindow'. Now that openChatWindow has landed, we can stop adding support for openServiceWindow.

@@ +109,5 @@
> +                    let xulWindow = Services.wm.getMostRecentWindow("navigator:browser");
> +                    xulWindow.openUILink(nUri.spec);
> +                  }
> +                } catch(e) {
> +                  // skip it

Cu.reportError the requested action to help providers debug.

@@ +114,5 @@
> +                }
> +              }
> +              break;
> +            default:
> +              break;

Cu.reportError the topic here to help providers debug why their implementation might not be working.

@@ +121,5 @@
> +      }
> +      alertsService.showAlertNotification(icon,
> +                                          this._provider.name, // title
> +                                          body,
> +                                          !!action, // text clicakble if an action

clickable

::: toolkit/components/social/test/browser/browser_notifications.js
@@ +30,5 @@
> +
> +let tests = {
> +  testNotificationCallback: function(cbnext) {
> +    if (!'@mozilla.org/system-alerts-service;1' in Cc) {
> +      // This is a platform that has a system-alerts-service so the "toast"

Why is this negated? ('@mozilla.org/system-alerts-service;1' in Cc) should be true if the platform has a system-alerts-service.

@@ +38,5 @@
> +      cbnext();
> +      return;
> +    }
> +    if (!("@mozilla.org/alerts-service;1" in Cc)) {
> +      todo(false, "Alerts service does not exist in this application");

change this to an info().

@@ +48,5 @@
> +      notifier = Cc["@mozilla.org/alerts-service;1"].
> +                 getService(Ci.nsIAlertsService);
> +    } catch (ex) {
> +      todo(false, 
> +           "Alerts service is not available. (Mac OS X without Growl?)", ex);

change this to an info().

Comment 24

[Axel Hecht [:Pike]]

Comment on attachment 653975 [details] [diff] [review]
rebased

Could you provide some screenshots or so for me as context?

Gotta be frank, I have no idea what this bug is about from glancing at it, and I don't know really anything about the social stuff yet, so I couldn't tell if this is a problem or not.

Comment 25

[Shane Caraveo (:mixedpuppy)]

Attachment: windowtitle.png

This is kind of unrelated to this bug since we were doing this before, but for your benefit:

window title showing "Provider Name - Document Title".  Provider Name comes from the provider manifest, the second part is from the title tag.  This window is a chromeless dialog originally used as a chat container.

Comment 26

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 654828 [details]
windowtitle.png

This is the same kind of title concatenating that we do for the main browser window, so I don't think it's an issue.

Comment 27

[Shane Caraveo (:mixedpuppy)]

(In reply to Jared Wein [:jaws] from comment #23)

> ::: toolkit/components/social/MozSocialAPI.jsm
> @@ +97,5 @@
> >      openServiceWindow: {
> >        enumerable: true,
> >        configurable: true,
> >        writable: true,
> > +      value: function(toURL, name, options, callback) {
> 
> Why do we need to allow the window opening "options" to be provided by the
> caller? It seems that we should be able to determine the correct window
> options and not let callers change them.

Essentially, openServiceWindow is meant to work pretty much like window.open would from content, except that we want to remove all chrome which is not possible from content.

> @@ +325,5 @@
> > +    // we dont want the default title the browser produces, we'll fixup whenever
> > +    // it changes.
> > +    chromeWindow.addEventListener("DOMTitleChanged", function() {
> > +      let sep = chromeWindow.document.documentElement.getAttribute("titlemenuseparator");
> > +      chromeWindow.document.title = provider.name + sep + chromeWindow.gBrowser.contentWindow.document.title;
> 
> This should be |document.title + sep + provider.name| so that it is easier
> to find the window that the user is looking for.
> 
> Will this cause an infinite loop of DOMTitleChanged events?

It looks like it would, but I haven't seen that happen, I can look at that a bit more.

Comment 28

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to Shane Caraveo (:mixedpuppy) from comment #27)
> Essentially, openServiceWindow is meant to work pretty much like window.open
> would from content, except that we want to remove all chrome which is not
> possible from content.

I don't think we need to match window.open arguments here.

Comment 29

[Shane Caraveo (:mixedpuppy)]

Attachment: v2

removes service window entirely.
leaves openChatWindow fn available from module for use by bug 784535

Comment 30

[Shane Caraveo (:mixedpuppy)]

https://tbpl.mozilla.org/?tree=Try&rev=2894ea6571f1

Comment 31

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 655208 [details] [diff] [review]
v2

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

::: toolkit/components/social/WorkerAPI.jsm
@@ +98,5 @@
> +                    let xulWindow = Services.wm.getMostRecentWindow("navigator:browser");
> +                    xulWindow.openUILink(nUri.spec);
> +                  }
> +                } catch(e) {
> +                  Cu.reportError("social.notification-create error: "+e);

nit: spaces around the + operator

@@ +114,5 @@
> +                                          !!action, // text clickable if an
> +                                                    // action was provided.
> +                                          null,
> +                                          listener,
> +                                          type); 

nit: trailing whitespace

Comment 32

[Shane Caraveo (:mixedpuppy)]

new try push https://tbpl.mozilla.org/?tree=Try&rev=f537ba476b5f

Comment 33

[Shane Caraveo (:mixedpuppy)]

Attachment: v2.1

fixes add/remove of provider for test

Comment 34

[Shane Caraveo (:mixedpuppy)]

new try https://tbpl.mozilla.org/?tree=Try&rev=b71f2afe3741

Comment 35

[Mark Hammond [:markh] [:mhammond]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/224d02609af4

Comment 36

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

https://hg.mozilla.org/mozilla-central/rev/224d02609af4

Comment 37

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to Jared Wein [:jaws] from comment #31)
> Comment on attachment 655208 [details] [diff] [review]
> v2
> 
> Review of attachment 655208 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: toolkit/components/social/WorkerAPI.jsm
> @@ +98,5 @@
> > +                    let xulWindow = Services.wm.getMostRecentWindow("navigator:browser");
> > +                    xulWindow.openUILink(nUri.spec);
> > +                  }
> > +                } catch(e) {
> > +                  Cu.reportError("social.notification-create error: "+e);
> 
> nit: spaces around the + operator

Note that this nit, while low severity, wasn't addressed in the patch that got landed. Please double-check that the review comments were addressed before landing.