Skip to content
This repository has been archived by the owner on Nov 3, 2021. It is now read-only.

Bug 855165 - Migrate to new Notification API and close notifications #15307

Merged
merged 1 commit into from Jan 16, 2014

Conversation

lissyx
Copy link
Contributor

@lissyx lissyx commented Jan 14, 2014

No description provided.

var options = {
icon: iconURL,
body: body,
tag: 'messages:' + threadId
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this have to be a string? If not, then it would be preferrable to use an object:

tag: {
   threadId: threadId
}

EDIT

If Notification.get({ tag: ... }) expects a string, then at very least, this string should be: "threadId:" + threadId

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, the spec requires a string: http://notifications.spec.whatwg.org/#api.

@rwaldron
Copy link
Contributor

Sorry, I should've mentioned this before but it slipped my mind until just now: we run make hint APP=sms for all Message app code (this is jshint). I just ran it for this and there are a few bits that need to be addressed:

apps/sms/js/activity_handler.js: line 431, col 38, 'Notification' is not defined.
apps/sms/js/message_manager.js: line 362, col 9, 'Notification' is not defined.
apps/sms/test/unit/activity_handler_test.js: line 6, col 42, 'MockNotificationHelper' is defined but never used.
apps/sms/test/unit/message_manager_test.js: line 564, col 19, 'MockNotification' is not defined.
apps/sms/test/unit/message_manager_test.js: line 569, col 23, 'MockNotification' is not defined.
apps/sms/test/unit/message_manager_test.js: line 664, col 37, 'MockNotification' is not defined.
apps/sms/test/unit/message_manager_test.js: line 684, col 13, 'Notification' is not defined.

In all of these cases, all you need to do is add those identifiers to the /*globals ...*/ list at the top of the appropriate file. These following errors you can ignore, they are being addressed in other tickets:

apps/sms/js/desktop-only/contacts.js: line 11, col 82, Line is too long.
apps/sms/test/marionette/phone_number_service_test.js: line 3, col 5, Redefinition of 'assert'.
apps/sms/test/marionette/phone_number_service_test.js: line 8, col 1, 'marionette' is not defined.
apps/sms/test/marionette/phone_number_service_test.js: line 9, col 16, 'marionette' is not defined.
apps/sms/test/marionette/phone_number_service_test.js: line 40, col 11, 'marionetteScriptFinished' is not defined.
apps/sms/test/marionette/phone_number_service_test.js: line 6, col 24, 'TARGET_APP_MANIFEST' is defined but never used.

Again, sorry for this last minute thing, but appreciate your patience!

lissyx added a commit that referenced this pull request Jan 16, 2014
Bug 855165 - Migrate to new Notification API and close notifications r=rwaldron
@lissyx lissyx merged commit 4efcdad into mozilla-b2g:master Jan 16, 2014
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
2 participants