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

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::SMS, defect)

Description

[Caio Lima(:caiolima)]

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()

Comment 1

[Julien Wajsberg [:julienw]]

good for me.

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

thanks a lot !

Comment 2

[Caio Lima(:caiolima)]

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?

Comment 3

[Julien Wajsberg [:julienw]]

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 !

Comment 4

[Caio Lima(:caiolima)]

(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?

Comment 5

[Julien Wajsberg [:julienw]]

Not yet, but Mike (:jugglinmike) might now more as he worked both on Sms and on the integration test framework.

Comment 6

[Julien Wajsberg [:julienw]]

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 :)

Comment 7

[Caio Lima(:caiolima)]

(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.

Comment 8

[Caio Lima(:caiolima)]

Attachment: pr link.txt

Comment 9

[Rick Waldron [:rwaldron]]

https://github.com/mozilla-b2g/gaia/pull/11644/files#r5878218

Comment 10

[Julien Wajsberg [:julienw]]

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 !

Comment 11

[Caio Lima(:caiolima)]

Attachment: pr-906544.txt

Comment 12

[Julien Wajsberg [:julienw]]

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 :)

Comment 13

[Julien Wajsberg [:julienw]]

master: e46923c49c6c9e574ae00b52b6431c2cf1e3ca5c