1160054 - [Messages][refactoring] Migrate mozActivity DOM request to promise base

Status: RESOLVED WONTFIX

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

Description

[Steve Chung [:steveck]]

After Bug 839838 the mozActivity DOM request should return promise and it might be more readable and easier to use in some case that could use promise chain. Also the unit test for mock mozActivity could be simpler as well.

Comment 1

[kumar rishav (:rishav_)]

Hey steve,
What i understand from the description is, as DOMrequest now supports .then() properties. We have to replace .onsuccess/onerror etc with .then() . Am i correct?

Thanks

Comment 2

[Steve Chung [:steveck]]

(In reply to kumar rishav (:rishav_) from comment #1)
> Hey steve,
> What i understand from the description is, as DOMrequest now supports
> .then() properties. We have to replace .onsuccess/onerror etc with .then() .
> Am i correct?
> 
> Thanks

Sorry for the late reply, and yes you are right. It seem not a difficult task but you still need to pay attention about the unit test and adjust the mock activity. And we also need to rethink about the interface of activity picker.

Comment 3

[kumar rishav (:rishav_)]

hey steve,
"And we also need to rethink about the interface of activity picker" ?
I didn't get this.  Are we planning for new interface for activity picker?

Thanks

Comment 4

[Steve Chung [:steveck]]

(In reply to kumar rishav (:rishav_) from comment #3)
> hey steve,
> "And we also need to rethink about the interface of activity picker" ?
> I didn't get this.  Are we planning for new interface for activity picker?
> 
> Thanks

If we apply promise base for MozActivity in ActivityPicker, we can remove handleActivity function in ActivityPicker and return MozActivity promise directly. For example:

ActivityPicker.dial(number, onsuccess, onerror) could become

ActivityPicker.dial(number).then(onsuccess, onerror)

Comment 5

[kumar rishav (:rishav_)]

Hey steve,
Apart from unit test, Do activity picker need .then method (though it's better to have it) ? As rest of codes in method (where ActivityPicker is used), this async method seems independent from rest of code as these name 'new' 'view' 'update' 'open' 'dial', doesn't need onsuccess/onerror. 

Thanks

Comment 6

[kumar rishav (:rishav_)]

Please ignore above comment.

Comment 7

[Steve Chung [:steveck]]

Well, some methods like createNewContact does not have onsuccess/onerror originally, does not mean that we can't handle it in the new ActivityPicker. We can still return the MozActivity promise and developers can decide whether they want to handle anything else in success or error.

Comment 8

[GitHub Autolander Bot]

Attachment: [gaia] kumarrishav:Bug-1160054 > mozilla-b2g:master

Comment 9

[kumar rishav (:rishav_)]

Comment on attachment 8678169 [details] [review]
[gaia] kumarrishav:Bug-1160054 > mozilla-b2g:master

Here is the patch. Hope it's fine.

Comment 10

[Steve Chung [:steveck]]

Comment on attachment 8678169 [details] [review]
[gaia] kumarrishav:Bug-1160054 > mozilla-b2g:master

I left some thoughts about the unit test on github. Even the part you rewrite didn't break the tests, I think it would be great if we can clean up the outdated test case and mock setup all together since we are not rush for the refactoring right now and it's not difficult to rewrite the activity picker test, so let's make everything ready in this patch :)

Comment 11

[Julien Wajsberg [:julienw]]

Mass closing of Gaia::SMS bugs. End of an era :(

Comment 12

[Julien Wajsberg [:julienw]]

Mass closing of Gaia::SMS bugs. End of an era :(