Add Notification module

This is just the part that decides which notification to show, notification downloading and showing is not included.

[Wladimir Palant, 2013-07-19 08:16:32]

All other issues are already addressed in my changes so it's up to you to review
now.

http://codereview.adblockplus.org/11175021/diff/3001/lib/notification.js
File lib/notification.js (right):

http://codereview.adblockplus.org/11175021/diff/3001/lib/notification.js#newc...
lib/notification.js:173:
Prefs.notificationdata.shown.push(notificationToShow.timestamp);
It's probably better to call that field "id" rather than "timestamp" - it can
still be a timestamp then but it doesn't have to be.

[Felix Dahlke, 2013-07-19 14:01:45]

http://codereview.adblockplus.org/11175021/diff/3001/lib/notification.js
File lib/notification.js (right):

http://codereview.adblockplus.org/11175021/diff/3001/lib/notification.js#newc...
lib/notification.js:28: let INITIAL_DELAY = 12 * MILLIS_IN_MINUTE;
How about using const for these?

http://codereview.adblockplus.org/11175021/diff/3001/lib/notification.js#newc...
lib/notification.js:35: return (notification.severity in levels ?
levels[notification.severity] : levels.information);
If we return -1 for unknown severities, those messages will never get precedence
over any other notifications. Maybe that's better?

http://codereview.adblockplus.org/11175021/diff/3001/lib/notification.js#newc...
lib/notification.js:79: getDownloadables: function()
Discussed a bit on IRC before, but it seems like getDownloadables is only used
internally. Will we use it externally later to react to new messages right away?

http://codereview.adblockplus.org/11175021/diff/3001/lib/notification.js#newc...
lib/notification.js:82: if (typeof Prefs.notificationdata.lastError == "number")
I've noticed that Prefs.notificationdata is undefined in Chrome. I guess that's
a bug in the Prefs implementation we have in Chrome?

http://codereview.adblockplus.org/11175021/diff/3001/lib/notification.js#newc...
lib/notification.js:150: if ((typeof notification.severity == "undefined" ||
notification.severity === "information")
I think using both == and === in the same line is a bit confusing, we should
settle on one. Don't really mind which one.

http://codereview.adblockplus.org/11175021/diff/3001/lib/notification.js#newc...
lib/notification.js:173:
Prefs.notificationdata.shown.push(notificationToShow.timestamp);
On 2013/07/19 08:16:32, Wladimir Palant wrote:
> It's probably better to call that field "id" rather than "timestamp" - it can
> still be a timestamp then but it doesn't have to be.

Yeah, fine by me. You already address this, so... :)

[Felix Dahlke, 2013-07-19 14:38:38]

http://codereview.adblockplus.org/11175021/diff/3001/lib/notification.js
File lib/notification.js (right):

http://codereview.adblockplus.org/11175021/diff/3001/lib/notification.js#newc...
lib/notification.js:82: if (typeof Prefs.notificationdata.lastError == "number")
On 2013/07/19 14:01:45, Felix H. Dahlke wrote:
> I've noticed that Prefs.notificationdata is undefined in Chrome. I guess
that's
> a bug in the Prefs implementation we have in Chrome?

Oh well, just remembered that the initial pref values need to be hard coded in
Chrome.

[Wladimir Palant, 2013-07-19 14:55:11]

I've addressed the comments. Feel free to update this issue - I don't have the
permission to do that :)

http://codereview.adblockplus.org/11175021/diff/3001/lib/notification.js
File lib/notification.js (right):

http://codereview.adblockplus.org/11175021/diff/3001/lib/notification.js#newc...
lib/notification.js:28: let INITIAL_DELAY = 12 * MILLIS_IN_MINUTE;
On 2013/07/19 14:01:45, Felix H. Dahlke wrote:
> How about using const for these?

That will not have the results you expect. This code isn't running in a real
context, it's rather being loaded into an object by bootstrap.js. Using const
here will define a read-only property on that object - and an attempt to
overwrite it will fail silently rather than fail. So I decided to avoid const at
top level. This might work properly if we add "use strict" (which wasn't
available back when I implemented this module framework), we should check back
once we start using it.

http://codereview.adblockplus.org/11175021/diff/3001/lib/notification.js#newc...
lib/notification.js:35: return (notification.severity in levels ?
levels[notification.severity] : levels.information);
On 2013/07/19 14:01:45, Felix H. Dahlke wrote:
> If we return -1 for unknown severities, those messages will never get
precedence
> over any other notifications. Maybe that's better?

I would rather have "information" be the default, makes more sense IMHO,
particularly if we decide to add new severities in future that won't be
supported by old clients.

http://codereview.adblockplus.org/11175021/diff/3001/lib/notification.js#newc...
lib/notification.js:79: getDownloadables: function()
On 2013/07/19 14:01:45, Felix H. Dahlke wrote:
> Discussed a bit on IRC before, but it seems like getDownloadables is only used
> internally. Will we use it externally later to react to new messages right
away?

Pretty unlikely, I guess that _getDownloadables would be better here.

http://codereview.adblockplus.org/11175021/diff/3001/lib/notification.js#newc...
lib/notification.js:82: if (typeof Prefs.notificationdata.lastError == "number")
On 2013/07/19 14:38:38, Felix H. Dahlke wrote:
> Oh well, just remembered that the initial pref values need to be hard coded in
> Chrome.

Right, we need to add a default for it.

http://codereview.adblockplus.org/11175021/diff/3001/lib/notification.js#newc...
lib/notification.js:150: if ((typeof notification.severity == "undefined" ||
notification.severity === "information")
True :)

[Felix Dahlke, 2013-07-19 14:58:38]

On 2013/07/19 14:55:11, Wladimir Palant wrote:
> I've addressed the comments. Feel free to update this issue - I don't have the
> permission to do that :)

Probably not worth the hassle, I've checked them here:
https://hg.adblockplus.org/adblockplus/rev/f36ddf0d00e1

LGTM