Notifications work in service workers. Resolves #14.

Notifications work in service workers. Resolves #14.

https://github.com/chromium/caterpillar/issues/14

R=raymes@chromium.org

Committed: 380d776e312c55fd62c4efd8b06ea26c849b540f

[Matthew Alger, 2016-01-28 02:49:44]

alger@google.com changed reviewers:
+ raymes@chromium.org

[raymes, 2016-01-28 04:36:56]

https://codereview.chromium.org/1646633003/diff/1/src/js/polyfills/notificati...
File src/js/polyfills/notifications.polyfill.js (right):

https://codereview.chromium.org/1646633003/diff/1/src/js/polyfills/notificati...
src/js/polyfills/notifications.polyfill.js:189:
navigator.serviceWorker.getRegistration) {
nit: 4 space indent, or in-line with the ( on the previous line

https://codereview.chromium.org/1646633003/diff/1/src/js/polyfills/notificati...
src/js/polyfills/notifications.polyfill.js:191: if (reg === undefined)
if (!reg)

https://codereview.chromium.org/1646633003/diff/1/src/js/polyfills/notificati...
src/js/polyfills/notifications.polyfill.js:211: var createNotification =
function(title, notificationOptions) {
function createNotification(title, ...

https://codereview.chromium.org/1646633003/diff/1/src/js/polyfills/notificati...
src/js/polyfills/notifications.polyfill.js:218: return Promise.resolve([new
Notification(title, notificationOptions)]);
Is this useful?

https://codereview.chromium.org/1646633003/diff/1/src/js/polyfills/notificati...
src/js/polyfills/notifications.polyfill.js:222: return
Promise.resolve(notifications[notifications.length - 1]);
It should be easy to inline this .then() and just resolve the promise in the
above .then() and .catch()

https://codereview.chromium.org/1646633003/diff/1/tests/js/polyfills/notifica...
File tests/js/polyfills/notifications.polyfill.test.js (right):

https://codereview.chromium.org/1646633003/diff/1/tests/js/polyfills/notifica...
tests/js/polyfills/notifications.polyfill.test.js:20: var close = this.close;
Can this be inlined?

https://codereview.chromium.org/1646633003/diff/1/tests/js/polyfills/notifica...
tests/js/polyfills/notifications.polyfill.test.js:26: this.getRegistration =
chrome.caterpillar.notifications.getRegistration;
This seems unneeded

https://codereview.chromium.org/1646633003/diff/1/tests/js/polyfills/notifica...
tests/js/polyfills/notifications.polyfill.test.js:30: });
This would be clearer if you defined:
this.registration = {
};

Then you could return the registration directly in getRegistration below. You
could also check this.registration.showNotificaiton.calledOnce.

https://codereview.chromium.org/1646633003/diff/1/tests/js/polyfills/notifica...
tests/js/polyfills/notifications.polyfill.test.js:60: var creator =
Notification.called ? Notification : showNotification;
This would be simpler if we just had the one codepath for creating
notifications, which I would recommend.

[Matthew Alger, 2016-01-28 05:18:02]

https://codereview.chromium.org/1646633003/diff/1/src/js/polyfills/notificati...
File src/js/polyfills/notifications.polyfill.js (right):

https://codereview.chromium.org/1646633003/diff/1/src/js/polyfills/notificati...
src/js/polyfills/notifications.polyfill.js:189:
navigator.serviceWorker.getRegistration) {
On 2016/01/28 04:36:56, raymes wrote:
> nit: 4 space indent, or in-line with the ( on the previous line

Not sure what happened here - fixed!

https://codereview.chromium.org/1646633003/diff/1/src/js/polyfills/notificati...
src/js/polyfills/notifications.polyfill.js:191: if (reg === undefined)
On 2016/01/28 04:36:56, raymes wrote:
> if (!reg)

Done.

https://codereview.chromium.org/1646633003/diff/1/src/js/polyfills/notificati...
src/js/polyfills/notifications.polyfill.js:211: var createNotification =
function(title, notificationOptions) {
On 2016/01/28 04:36:56, raymes wrote:
> function createNotification(title, ...

Done. Is this syntax preferred?

https://codereview.chromium.org/1646633003/diff/1/src/js/polyfills/notificati...
src/js/polyfills/notifications.polyfill.js:218: return Promise.resolve([new
Notification(title, notificationOptions)]);
On 2016/01/28 04:36:56, raymes wrote:
> Is this useful?

Yes, I think so - new Notification was part of the spec before showNotification
was, as far as I am aware, so it's possible that browsers may implement new
Notification but not showNotification.

Specifically, Chrome 22-41 only support new Notification. Firefox 22-43 only
support new Notification - note that 44 is the latest version.

https://codereview.chromium.org/1646633003/diff/1/src/js/polyfills/notificati...
src/js/polyfills/notifications.polyfill.js:222: return
Promise.resolve(notifications[notifications.length - 1]);
On 2016/01/28 04:36:56, raymes wrote:
> It should be easy to inline this .then() and just resolve the promise in the
> above .then() and .catch()

I'm not sure how to do that without adding a .then on the back of
registration.getNotifications. Is that what you mean?

https://codereview.chromium.org/1646633003/diff/1/tests/js/polyfills/notifica...
File tests/js/polyfills/notifications.polyfill.test.js (right):

https://codereview.chromium.org/1646633003/diff/1/tests/js/polyfills/notifica...
tests/js/polyfills/notifications.polyfill.test.js:20: var close = this.close;
On 2016/01/28 04:36:56, raymes wrote:
> Can this be inlined?

Yup. Done.

https://codereview.chromium.org/1646633003/diff/1/tests/js/polyfills/notifica...
tests/js/polyfills/notifications.polyfill.test.js:26: this.getRegistration =
chrome.caterpillar.notifications.getRegistration;
On 2016/01/28 04:36:56, raymes wrote:
> This seems unneeded

It was so that I could restore getRegistration later, but I guess that's
pointless. Removed.

https://codereview.chromium.org/1646633003/diff/1/tests/js/polyfills/notifica...
tests/js/polyfills/notifications.polyfill.test.js:30: });
On 2016/01/28 04:36:56, raymes wrote:
> This would be clearer if you defined:
> this.registration = {
> };
> 
> Then you could return the registration directly in getRegistration below. You
> could also check this.registration.showNotificaiton.calledOnce.

Done.

https://codereview.chromium.org/1646633003/diff/1/tests/js/polyfills/notifica...
tests/js/polyfills/notifications.polyfill.test.js:60: var creator =
Notification.called ? Notification : showNotification;
On 2016/01/28 04:36:56, raymes wrote:
> This would be simpler if we just had the one codepath for creating
> notifications, which I would recommend.

Do you mean dropping support for new Notification? I am not sure whether that's
a good idea.

[raymes, 2016-02-01 00:14:24]

https://codereview.chromium.org/1646633003/diff/1/src/js/polyfills/notificati...
File src/js/polyfills/notifications.polyfill.js (right):

https://codereview.chromium.org/1646633003/diff/1/src/js/polyfills/notificati...
src/js/polyfills/notifications.polyfill.js:211: var createNotification =
function(title, notificationOptions) {
It's just a bit simpler

https://codereview.chromium.org/1646633003/diff/1/src/js/polyfills/notificati...
src/js/polyfills/notifications.polyfill.js:222: return
Promise.resolve(notifications[notifications.length - 1]);
On 2016/01/28 05:18:02, Matthew Alger wrote:
> On 2016/01/28 04:36:56, raymes wrote:
> > It should be easy to inline this .then() and just resolve the promise in the
> > above .then() and .catch()
> 
> I'm not sure how to do that without adding a .then on the back of
> registration.getNotifications. Is that what you mean?

Oh right, I didn't realise it returned a promise. This looks ok.

https://codereview.chromium.org/1646633003/diff/20001/tests/js/polyfills/noti...
File tests/js/polyfills/notifications.polyfill.test.js (right):

https://codereview.chromium.org/1646633003/diff/20001/tests/js/polyfills/noti...
tests/js/polyfills/notifications.polyfill.test.js:48:
assert.ok(Notification.calledOnce || showNotification.calledOnce);
It seems like the old Notification codepath will never be tested anyway. Could
we instead just check the new version by default in tests and then have an extra
test or two which test the deprecated version? (I think you would need to
provide a different getRegistration function to test the deprecated Notification
codepath)

[Matthew Alger, 2016-02-01 00:50:42]

https://codereview.chromium.org/1646633003/diff/1/src/js/polyfills/notificati...
File src/js/polyfills/notifications.polyfill.js (right):

https://codereview.chromium.org/1646633003/diff/1/src/js/polyfills/notificati...
src/js/polyfills/notifications.polyfill.js:211: var createNotification =
function(title, notificationOptions) {
On 2016/02/01 00:14:23, raymes wrote:
> It's just a bit simpler

Acknowledged.

https://codereview.chromium.org/1646633003/diff/1/src/js/polyfills/notificati...
src/js/polyfills/notifications.polyfill.js:222: return
Promise.resolve(notifications[notifications.length - 1]);
On 2016/02/01 00:14:23, raymes wrote:
> On 2016/01/28 05:18:02, Matthew Alger wrote:
> > On 2016/01/28 04:36:56, raymes wrote:
> > > It should be easy to inline this .then() and just resolve the promise in
the
> > > above .then() and .catch()
> > 
> > I'm not sure how to do that without adding a .then on the back of
> > registration.getNotifications. Is that what you mean?
> 
> Oh right, I didn't realise it returned a promise. This looks ok.

Acknowledged.

https://codereview.chromium.org/1646633003/diff/20001/tests/js/polyfills/noti...
File tests/js/polyfills/notifications.polyfill.test.js (right):

https://codereview.chromium.org/1646633003/diff/20001/tests/js/polyfills/noti...
tests/js/polyfills/notifications.polyfill.test.js:48:
assert.ok(Notification.calledOnce || showNotification.calledOnce);
On 2016/02/01 00:14:23, raymes wrote:
> It seems like the old Notification codepath will never be tested anyway. Could
> we instead just check the new version by default in tests and then have an
extra
> test or two which test the deprecated version? (I think you would need to
> provide a different getRegistration function to test the deprecated
Notification
> codepath)

Good idea. Done.

[raymes, 2016-02-01 01:50:01]

lgtm

https://codereview.chromium.org/1646633003/diff/40001/src/js/polyfills/notifi...
File src/js/polyfills/notifications.polyfill.js (right):

https://codereview.chromium.org/1646633003/diff/40001/src/js/polyfills/notifi...
src/js/polyfills/notifications.polyfill.js:184: // Exported so we can override
for testing.
nit: add this to the method description instead

https://codereview.chromium.org/1646633003/diff/40001/src/js/polyfills/notifi...
src/js/polyfills/notifications.polyfill.js:217: .catch(function() {
nit: this should probably go on the previous line

https://codereview.chromium.org/1646633003/diff/40001/src/js/polyfills/notifi...
src/js/polyfills/notifications.polyfill.js:220: .then(function(notifications) {
nit: this should probably go on the previous line

[Matthew Alger, 2016-02-01 01:54:56]

https://codereview.chromium.org/1646633003/diff/40001/src/js/polyfills/notifi...
File src/js/polyfills/notifications.polyfill.js (right):

https://codereview.chromium.org/1646633003/diff/40001/src/js/polyfills/notifi...
src/js/polyfills/notifications.polyfill.js:184: // Exported so we can override
for testing.
On 2016/02/01 01:50:01, raymes wrote:
> nit: add this to the method description instead

Done.

https://codereview.chromium.org/1646633003/diff/40001/src/js/polyfills/notifi...
src/js/polyfills/notifications.polyfill.js:217: .catch(function() {
On 2016/02/01 01:50:01, raymes wrote:
> nit: this should probably go on the previous line

Done.

https://codereview.chromium.org/1646633003/diff/40001/src/js/polyfills/notifi...
src/js/polyfills/notifications.polyfill.js:220: .then(function(notifications) {
On 2016/02/01 01:50:01, raymes wrote:
> nit: this should probably go on the previous line

Done.

[Matthew Alger, 2016-02-02 04:24:24]

Description was changed from

==========
Notifications work in service workers. Resolves #14.

https://github.com/chromium/caterpillar/issues/14
==========

to

==========
Notifications work in service workers. Resolves #14.

https://github.com/chromium/caterpillar/issues/14

R=raymes@chromium.org

Committed: 380d776e312c55fd62c4efd8b06ea26c849b540f
==========

[Matthew Alger, 2016-02-02 04:24:26]

Committed patchset #4 (id:60001) manually as
380d776e312c55fd62c4efd8b06ea26c849b540f (presubmit successful).