[Messaging] Rename Notification object from apps/sms/js/notification.js to avoid conflict with new Notification API

RESOLVED FIXED

Status

Firefox OS
Gaia::SMS
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: caiolima, Assigned: caiolima)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [mentor=:julienw])

Attachments

(1 attachment, 1 obsolete attachment)

46 bytes, text/x-github-pull-request
julienw
: review+
Details | Review | Splinter Review
(Assignee)

Description

4 years ago
User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:22.0) Gecko/20100101 Firefox/22.0 (Beta/Release)
Build ID: 20130618035212



Actual results:

If we try to use the new Notification API implemented in bug https://bugzilla.mozilla.org/show_bug.cgi?id=782211 in sms Application, it will conflict with the Notification object from apps/sms/js/notification.js

The ideia is to rename the Notification object to NotificationUtils because it's badly named, given it has methods like vibrate() and and ringtone()
Blocks: 855165
good for me.

Now I understand why you had the constructor problem too... ;)

thanks a lot !
Assignee: nobody → ticaiolima
Status: UNCONFIRMED → NEW
Ever confirmed: true
Summary: [Messaging] Refactor to Notification object from apps/sms/js/notification.js to avoid conflict with new Notification API → [Messaging] Rename Notification object from apps/sms/js/notification.js to avoid conflict with new Notification API
(Assignee)

Comment 2

4 years ago
Julien, about the tests, the Unit tests of sms application are enought to trust if the rename of this object will not break the application?
nope, the unit tests only test individual files therefore it won't catch regressions because of this. We would need to have integration tests for this but we have not yet :)

You'll probably need to change the existing unit tests though !
(Assignee)

Comment 4

4 years ago
(In reply to Julien Wajsberg [:julienw] from comment #3)
> nope, the unit tests only test individual files therefore it won't catch
> regressions because of this. We would need to have integration tests for
> this but we have not yet :)
> 
> You'll probably need to change the existing unit tests though !

I see your point. I think that it could be a simple modification, but with a right risk of introduce new bugs on application(or even crash it). I'll study about the current Unit test of the application and analyse if they say me something about this modification. However, I think that start developing integration/unit tests is the right way, since It'll be used to solve new bugs.

There is any group working on Unit/integration tests for sms application?
Not yet, but Mike (:jugglinmike) might now more as he worked both on Sms and on the integration test framework.
I'd be happy if you start doing integration tests but maybe you can move forward here already and do integration tests in another bug ? So that we try to finish something :)
(Assignee)

Comment 7

4 years ago
(In reply to Julien Wajsberg [:julienw] from comment #6)
> I'd be happy if you start doing integration tests but maybe you can move
> forward here already and do integration tests in another bug ? So that we
> try to finish something :)

Ok then.
(Assignee)

Comment 8

4 years ago
Created attachment 793008 [details] [review]
pr link.txt
Attachment #793008 - Flags: review?(felash)
https://github.com/mozilla-b2g/gaia/pull/11644/files#r5878218
Comment on attachment 793008 [details] [review]
pr link.txt

comments on github

please ask review again once you're ready

don't hesitate to ask on IRC if necessary :)

Thanks !
Attachment #793008 - Flags: review?(felash)
(Assignee)

Comment 11

4 years ago
Created attachment 793826 [details] [review]
pr-906544.txt
Attachment #793008 - Attachment is obsolete: true
Attachment #793826 - Flags: review?(felash)
Comment on attachment 793826 [details] [review]
pr-906544.txt

r=me

thanks, I'll land it :)

As a side note that you could just flip the flag "review" on the previous attachment since it was the same pull request :)
Attachment #793826 - Flags: review?(felash) → review+
master: e46923c49c6c9e574ae00b52b6431c2cf1e3ca5c
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Whiteboard: [mentor=:julienw]
Attachment mime type: text/plain text/plain → text/x-github-pull-request text/x-github-pull-request
You need to log in before you can comment on or make changes to this bug.