Closed Bug 1160054 Opened 5 years ago Closed 3 years ago

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

Categories

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

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(Not tracked)

RESOLVED WONTFIX

People

(Reporter: steveck, Unassigned, Mentored)

References

Details

(Whiteboard: [lang=js][good first bug])

Attachments

(1 file)

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.
Assignee: schung → nobody
Mentor: schung
Whiteboard: [lang=js][good first bug]
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
Flags: needinfo?(schung)
(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.
Flags: needinfo?(schung)
Assignee: nobody → rishav006
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
Flags: needinfo?(schung)
(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)
Flags: needinfo?(schung)
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
Flags: needinfo?(schung)
Please ignore above comment.
Flags: needinfo?(schung)
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 on attachment 8678169 [details] [review]
[gaia] kumarrishav:Bug-1160054 > mozilla-b2g:master

Here is the patch. Hope it's fine.
Attachment #8678169 - Flags: review?(schung)
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 :)
Attachment #8678169 - Flags: review?(schung)
Assignee: rishav006 → nobody
Mass closing of Gaia::SMS bugs. End of an era :(
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → WONTFIX
Mass closing of Gaia::SMS bugs. End of an era :(
You need to log in before you can comment on or make changes to this bug.