494549 - jetpack.notifications.show doesn't allow skipping options

Status: RESOLVED FIXED

(Mozilla Labs :: Jetpack Prototype, defect, P3)

Description

[Eli Grey (:sephr)]

User-Agent:       Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9.0.10) Gecko/2009042523 Ubuntu/9.04 (jaunty) Firefox/3.0.10 GTB5
Build Identifier: 

There is no sane way to skip the body, title, or icon properties for jetpack.notifications.show without the function going back to defaults. Of course you could specify a string with a space, but you shouldn't have to do that. What should be checked is if the properties exist, not if they == true.

IMO, I think that the default title should be changed to "Firefox Notification" and there be no default icon. If anyone else shares this view, maybe it should be considered in the fixed jetpack.notifications.show.

Reproducible: Always

Steps to Reproduce:
1. jetpack.notifications.show({icon:null, body:"This notification has no icon", title:"Notification"})
Actual Results:  
Mozilla favicon is shown in notification.

Expected Results:  
No icon is shown in notification.

Maby a good fix could be something along the lines of this function (I also changed the error handling to log body instead of message):

function (message) {
    var body = message,
    title = "Firefox Notification",
    icon = null;
    if (typeof message == "object") {
        body = message.body;
        if ("title" in message) {
            title = message.title;
        }
        if ("icon" in message) {
            icon = message.icon;
        }
    }
    try {
        var classObj = Cc['@mozilla.org/alerts-service;1'];
        var alertService = classObj.getService(Ci.nsIAlertsService);
        alertService.showAlertNotification(icon, title, body);
        return true;
    } catch (e) {
        console.log("Unable to display notification:", body);
        return false;
    }
}

Comment 1

[Steven Ramkumar]

I also think there should be no default icon.

Comment 2

[Eli Grey (:sephr)]

In my exception catch, I just remembered that Firebug would just list the properties so
    console.log("Unable to display notification:", body);
should be
    console.log("Unable to display notification:", message);
instead.

Comment 3

[Steven Ramkumar]

Attachment: Patch v0.1

Also added callbacks, cookies, and clickable features as outlined in nsIAlertsService.

Comment 4

[Steven Ramkumar]

Attachment: Patch v0.2

Refactored v0.1

Comment 5

[timeless]

Comment on attachment 379395 [details] [diff] [review]
Patch v0.2

you still have tabs

+             title = message.title;

is overindented

and of course, you're using 'firefox' a branded string and 'notification' an unlocalized string.

Comment 6

[Atul Varma [:atul]]

Thanks for the patch!  Hm, this looks good overall, but for now could you remove the extra options for "cookie", "callback", and "clickable" that you added?

The reason for this is because of the fact that jetpacks can be unloaded, we'll need to be able to remove that callback whenever the jetpack is unloaded.  You can look at the code for status-bar-panel.js or tabs.js for information on how to do this, but we'd really prefer that a new bug be created for that.  Hope you don't mind!

Comment 7

[Steven Ramkumar]

Yeah I can take those out for sure and file a new bug maybe. Although I think the new bug is kinda low priority right now for Jetpack. But yeah I'll submit a new patch soon.

Comment 8

[Nick Nguyen [:osunick]]

atul to review for .5

Comment 9

[Nick Nguyen [:osunick]]

Hi steven, 

Are you submitting the new patch with the changes Atul requested?

Comment 10

[Atul Varma [:atul]]

Attachment: Alternate patch that sets default title to "(app name) Notification" and default icon to null (which is ultimately the app's icon), and supports setting of title to null.

Comment 11

[Atul Varma [:atul]]

Ok, I've gone ahead and committed a version of the patch here:

  http://hg.mozilla.org/labs/jetpack/rev/a6b81b27db2c

Thanks!