778668 - Bubble the application name/origin/manifest in the desktop-notification mozChromeEvent

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect)

Description

[Etienne Segonzac (:etienne)]

As per UX request and for anti-spoofing reason, we want to display the application name and icon when showing a desktop notification.

Currently the mozChromeEvent sent when a mozNotification is shown do not provide any of those information.

We could probably get away by just adding the application origin to the event.

Comment 1

[[:fabrice] Fabrice Desré]

Attachment: patch

Etienne, I tested with gaia UI tests and it looks ok. I'm doing the manifest loading and icon/title resolution in shell.js so there's no gaia changes required.

Doug, I'm hijacking the "cookie" field of the alerts service since if was unused by the notification api so far to pass the manifest URL when the notification comes from an app.

Comment 2

[Doug Turner (:dougt)]

Comment on attachment 647520 [details] [diff] [review]
patch

wchen should review since he is making lots of changes to the AlertService

Comment 3

[William Chen [:wchen]]

Comment on attachment 647520 [details] [diff] [review]
patch

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

::: b2g/chrome/content/shell.js
@@ +512,5 @@
> +    }
> +
> +    // If we have a cookie, this is the manifestURL.
> +    // Get the icon and title from the manifest to prevent spoofing.
> +    if (cookie.length) {

We shouldn't be depending on the value of the cookie. It should be something that we only use to pass arbitrary data to alert observers. It just happens to be the case that we don't use it for DesktopNotifications. Can we alternatively obtain the manifest information from the browser object in b2g/components/AlertsService.js and then pass in into the AlertsHelper?

::: dom/src/notification/nsDesktopNotification.cpp
@@ +36,5 @@
>      mObserver = new AlertServiceObserver(this);
>  
> +  nsCOMPtr<nsPIDOMWindow> window = GetOwner();
> +  nsCOMPtr<mozIDOMApplication> app;
> +  nsresult rv = static_cast<nsGlobalWindow*>(window.get())->GetApp(getter_AddRefs(app));

Will |GetApp| work if called from a content process such as a webpage in the browser app?

Comment 4

[[:fabrice] Fabrice Desré]

(In reply to William Chen [:wchen] from comment #3)
> Comment on attachment 647520 [details] [diff] [review]
> patch
> 
> Review of attachment 647520 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: b2g/chrome/content/shell.js
> @@ +512,5 @@
> > +    }
> > +
> > +    // If we have a cookie, this is the manifestURL.
> > +    // Get the icon and title from the manifest to prevent spoofing.
> > +    if (cookie.length) {
> 
> We shouldn't be depending on the value of the cookie. It should be something
> that we only use to pass arbitrary data to alert observers. It just happens
> to be the case that we don't use it for DesktopNotifications. Can we
> alternatively obtain the manifest information from the browser object in
> b2g/components/AlertsService.js and then pass in into the AlertsHelper?

In AlertsService.js we have no idea which window initiated the call. And in b2g the only usage of the AlertsService is the desktop notification API. I'll be happier with the desktop notification API not using directly the alerts service, but is this a change we can make now?

> ::: dom/src/notification/nsDesktopNotification.cpp
> @@ +36,5 @@
> >      mObserver = new AlertServiceObserver(this);
> >  
> > +  nsCOMPtr<nsPIDOMWindow> window = GetOwner();
> > +  nsCOMPtr<mozIDOMApplication> app;
> > +  nsresult rv = static_cast<nsGlobalWindow*>(window.get())->GetApp(getter_AddRefs(app));
> 
> Will |GetApp| work if called from a content process such as a webpage in the
> browser app?

Yes, it will - it relies on the AppsService which is e10s compliant.

Comment 5

[William Chen [:wchen]]

(In reply to Fabrice Desré [:fabrice] from comment #4)
> (In reply to William Chen [:wchen] from comment #3)
> > Comment on attachment 647520 [details] [diff] [review]
> > patch
> > 
> > Review of attachment 647520 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: b2g/chrome/content/shell.js
> > @@ +512,5 @@
> > > +    }
> > > +
> > > +    // If we have a cookie, this is the manifestURL.
> > > +    // Get the icon and title from the manifest to prevent spoofing.
> > > +    if (cookie.length) {
> > 
> > We shouldn't be depending on the value of the cookie. It should be something
> > that we only use to pass arbitrary data to alert observers. It just happens
> > to be the case that we don't use it for DesktopNotifications. Can we
> > alternatively obtain the manifest information from the browser object in
> > b2g/components/AlertsService.js and then pass in into the AlertsHelper?
> 
> In AlertsService.js we have no idea which window initiated the call. And in
> b2g the only usage of the AlertsService is the desktop notification API.
> I'll be happier with the desktop notification API not using directly the
> alerts service, but is this a change we can make now?
> 
The main problem I have with this is that it defeats the purpose of the cookie parameter, we might as well call it manifestURL because we can't use it for anything else. If we must go down this route I rather have this added as another parameter than to hijack the cookie.

Also, I don't see much gain to having an intermediate API between the desktop notification API and the alerts service because either way we will have some B2G specific parameter for the manifest url.

Comment 6

[[:fabrice] Fabrice Desré]

So how do you want me to add another parameter to the AlertsService?

Comment 7

[William Chen [:wchen]]

(In reply to Fabrice Desré [:fabrice] from comment #6)
> So how do you want me to add another parameter to the AlertsService?
Add an optional parameter to the end of showAlertNotification in nsIAlertsService.idl, update the signature of showAlertNotification in the implementations of nsIAlertsService and update the consumers of the interface.

Comment 8

[[:fabrice] Fabrice Desré]

I'm waiting for try results with the new patch, but it's incredibly invasive (even comm-central needs to be patched) for what we need here :(

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

Comment 9

[[:fabrice] Fabrice Desré]

Attachment: m-c patch

m-c part, along with the b2g specific code.

Comment 10

[[:fabrice] Fabrice Desré]

Attachment: c-c part

comm-central part.

Comment 11

[William Chen [:wchen]]

Comment on attachment 648744 [details] [diff] [review]
m-c patch

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

::: b2g/chrome/content/shell.js
@@ +516,5 @@
> +      let app = DOMApplicationRegistry.getAppByManifestURL(manifestURL);
> +      DOMApplicationRegistry.getManifestFor(app.origin, function(aManifest) {
> +        let helper = new DOMApplicationManifest(aManifest, app.origin);
> +        imageUrl = helper.iconURLForSize(128);
> +        title = helper.name;

We need to think of a better way to show the application name and icon than simply overwriting the user specified values. We should find some way to show this information along side the user's notification, perhaps in a little bar above the notification. Being able to specify a title and icon is useful and is part of the requirements for the new notification spec that I've implemented on top of the alerts service.

::: toolkit/components/alerts/nsIAlertsService.idl
@@ +27,5 @@
>      *                       only used on OS X with Growl and Android.
>      *                       On OS X with Growl, users can disable notifications
>      *                       with a given name. On Android the name is hashed
>      *                       and used as a notification ID.
> +    * @param manifestURL    The url of the app's manifest, if any.

Lets rename manifestURL to manifestUrl throughout the patch so that the style is consistent with imageUrl.

Comment 12

[[:fabrice] Fabrice Desré]

Attachment: updated m-c patch

Updated patch to address comments.

Comment 13

[William Chen [:wchen]]

Comment on attachment 648841 [details] [diff] [review]
updated m-c patch

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

::: dom/src/notification/nsDesktopNotification.cpp
@@ +36,5 @@
>      mObserver = new AlertServiceObserver(this);
>  
> +  nsCOMPtr<nsPIDOMWindow> window = GetOwner();
> +  nsCOMPtr<mozIDOMApplication> app;
> +  nsresult rv = static_cast<nsGlobalWindow*>(window.get())->GetApp(getter_AddRefs(app));

I tried this in the browser app and |GetApp| returned null. I investigated this a bit and it looks like there is some kind of "barrier" for mozbrowser to prevent it from having certain privileges (I'm not familiar with the code I was reading but that's what I gather).

At the very least a null check should be added when |app| is dereferenced. It would be even better if we could find some way to obtain the manifest url for the browser app.

Comment 14

[[:fabrice] Fabrice Desré]

I added the check for |app| locally already. When you say that you tried in the browser app, this was by loading a web page, right? If so, this is expecting that the page does not obtain the browser manifest, since it's not an app.

Comment 15

[William Chen [:wchen]]

Comment on attachment 648841 [details] [diff] [review]
updated m-c patch

r+ with null check on |app|

Comment 16

[William Chen [:wchen]]

(In reply to Fabrice Desré [:fabrice] from comment #14)
> I added the check for |app| locally already. When you say that you tried in
> the browser app, this was by loading a web page, right? If so, this is
> expecting that the page does not obtain the browser manifest, since it's not
> an app.

I tried notifications in a webpage from the browser app in b2g.

Comment 17

[Josh Carpenter [:jcarpenter]]

> We need to think of a better way to show the application name and icon than simply overwriting the user specified values. We should find some way to show this information along side the user's notification, perhaps in a little bar above the notification. Being able to specify a title and icon is useful and is part of the requirements for the new notification spec that I've implemented on top of the alerts service.

Hi William, Josh from Gaia UX here.

Just to clarify, we will not overwrite user specified values. Users will see both the contents of the notification (iconURL, Title, and Body) and the AppName + AppIcon. We'll find a way to fit it all in.

One early concept: https://www.dropbox.com/s/m50vt3iq0cu87em/Notifications_1.mov

* The AppIcon crossfades w/ the iconURL (shared screen real estate)
* The AppName goes where the Title otherwise would
* The Title string is appended the start of the Body, and bolded.

That's probably not the precise approach we'll go with, but you get the idea.

Comment 18

[Dão Gottwald [:dao]]

Comment on attachment 648841 [details] [diff] [review]
updated m-c patch

>--- a/toolkit/components/alerts/nsIAlertsService.idl
>+++ b/toolkit/components/alerts/nsIAlertsService.idl

>@@ -23,16 +23,17 @@ interface nsIAlertsService : nsISupports
>     *                       consumer during the alert listener callbacks.
>     * @param alertListener  Used for callbacks. May be null if the caller 
>     *                       doesn't care about callbacks.
>     * @param name           The name of the notification. This is currently
>     *                       only used on OS X with Growl and Android.
>     *                       On OS X with Growl, users can disable notifications
>     *                       with a given name. On Android the name is hashed
>     *                       and used as a notification ID.
>+    * @param manifestUrl    The url of the app's manifest, if any.

nsIAlertsService is a general-purpose API that knows nothing about apps, so this comment is somewhat unhelpful.

This will also need super-review. I'd suggest mossop, the toolkit module owner.

Comment 19

[Dave Townsend [:mossop]]

I'm probably missing a lot of understanding here. Why do we need to use the alerts service for this? Could you instead define an nsIAppAlertsService say with the parameters you need that AlertsHelper then implements?

If we have to go through nsIAlertsService what do you think to adding appName and appIcon instead? That might be more sensible and usable even in Firefox where f.e. any in-page notifications API might want to send the page title and favicon along.

Comment 20

[[:fabrice] Fabrice Desré]

wchen, can you answer Dave first question?

Comment 21

[Dave Townsend [:mossop]]

Comment on attachment 648841 [details] [diff] [review]
updated m-c patch

Please re-request review once you've given some answers here

Comment 22

[William Chen [:wchen]]

(In reply to Dave Townsend (:Mossop) from comment #19)
> I'm probably missing a lot of understanding here. Why do we need to use the
> alerts service for this? Could you instead define an nsIAppAlertsService say
> with the parameters you need that AlertsHelper then implements?

We are using nsIAlertsService because desktop notifications should work on all platforms, not just for apps on B2G. It would be nice for nsIAlertsService to work on all platforms instead of having two services, one that is used for b2g and one for all the rest. Unfortunately it means having some B2G specific parameters.

Comment 23

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Noming. This is needed in order to unblock a blocker.

Comment 24

[[:fabrice] Fabrice Desré]

Attachment: patch, take 2

Slightly different approach, in which I add a dedicated interface for apps notifications that is only implemented on b2g.

Comment 25

[Etienne Segonzac (:etienne)]

Comment on attachment 685187 [details] [diff] [review]
patch, take 2

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

::: b2g/chrome/content/shell.js
@@ +692,5 @@
> +    // If we have a manifest URL, get the icon and title from the manifest to prevent spoofing.
> +    if (manifestUrl && manifestUrl.length) {
> +      let app = DOMApplicationRegistry.getAppByManifestURL(manifestUrl);
> +      DOMApplicationRegistry.getManifestFor(app.origin, function(aManifest) {
> +        let helper = new DOMApplicationManifest(aManifest, app.origin);

new ManifestHelper ?

Comment 26

[Etienne Segonzac (:etienne)]

Comment on attachment 685187 [details] [diff] [review]
patch, take 2

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

::: b2g/chrome/content/shell.js
@@ +718,5 @@
> +                                                          textClickable,
> +                                                          manifestUrl,
> +                                                          alertListener) {
> +    this.showNotification(imageUrl, title, text, textClickable, null,
> +                          alertListener, null, manifestURL);

manifestURL -> manifestUrl

Comment 27

[[:fabrice] Fabrice Desré]

Attachment: patch v3

This patch removes all dependence on the existing AlertsService, and only adds b2g specific code to manage the "app" case.

Comment 28

[Doug Turner (:dougt)]

Comment on attachment 685654 [details] [diff] [review]
patch v3

deferring to wchen.

Comment 29

[William Chen [:wchen]]

Comment on attachment 685654 [details] [diff] [review]
patch v3

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

r+ with comments addressed.

::: b2g/chrome/content/shell.js
@@ +645,5 @@
> +      mm: mm,
> +      title: title,
> +      text: text,
> +      manifestURL: manifestURL,
> +      imageURL: imageURL

Are title, text, manifestURL and imageURL used for anything or do we anticipate using them for anything? If not then we should remove them.

::: dom/src/notification/nsDesktopNotification.cpp
@@ +32,4 @@
>  {
>    nsCOMPtr<nsIAlertsService> alerts = do_GetService("@mozilla.org/alerts-service;1");
>    if (!alerts)
>      return NS_ERROR_NOT_IMPLEMENTED;

This should be moved below the MOZ_B2G block so that alerts will still show if the alerts-service is unavailable on B2G.

Comment 30

[[:fabrice] Fabrice Desré]

https://hg.mozilla.org/integration/mozilla-inbound/rev/60378fae4e94

Comment 31

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/60378fae4e94

Comment 32

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/60378fae4e94

Comment 33

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/2d21b6d0731d
https://hg.mozilla.org/releases/mozilla-beta/rev/553e3bfccbca