1171865 - [Messages][NG] Layout mozMobileMessageShim structure

Status: RESOLVED FIXED

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

Description

[Steve Chung [:steveck]]

After spec confirmed, we decided to have a API shim for wrapping all the mozMobilemessage API and it will be used by messaging service and conversation service(maybe some controller as well).

Comment 1

[Steve Chung [:steveck]]

Attachment: Link to github

Hey, it's the first draft of the mozMobileMessage shim. It simply proxy the API/broadcast event without any(or very few) additional logic inside. Please tell me if you have other thought about the shim, thanks!

Comment 2

[Julien Wajsberg [:julienw]]

Comment on attachment 8616724 [details] [review]
Link to github

I left some comments but this looks good

Comment 3

[Oleg Zasypkin [:azasypkin]]

Comment on attachment 8616724 [details] [review]
Link to github

lgtm as well, just minor nits at GitHub.

Comment 4

[Steve Chung [:steveck]]

Comment on attachment 8616724 [details] [review]
Link to github

Patch updated with unit test. Not sure if these tests could actually cover the real use case as a shim.

Comment 5

[Oleg Zasypkin [:azasypkin]]

Looks good overall, left some comments and questions at gitHub, mostly nits.

Keeping r? for now.

Thanks!

Comment 6

[Julien Wajsberg [:julienw]]

Comment on attachment 8616724 [details] [review]
Link to github

I'll leave Oleg handle this tomorrow as I'm traveling.

Comment 7

[Steve Chung [:steveck]]

Hi Oleg, I updated the patch with a question about the init part. Thanks for all the review!

Comment 8

[Oleg Zasypkin [:azasypkin]]

Comment on attachment 8616724 [details] [review]
Link to github

Hey Steve,

Looks good now!

I've left last nits. + Could you please also remove MozMobileMessageShim.init() call from the shim file (that should simplify tests as well) and ask for review as soon as you're ready?


Thanks!

Comment 9

[Steve Chung [:steveck]]

Comment on attachment 8616724 [details] [review]
Link to github

Ok, init removed with other suggestion addressed.

Comment 10

[Oleg Zasypkin [:azasypkin]]

Comment on attachment 8616724 [details] [review]
Link to github

I think we're good to go here, we'll polish shim later if needed in the scope of bugs like bug 1169576 and bug 1169573!

Julien do you have anything to add?

Thanks!

Comment 11

[Julien Wajsberg [:julienw]]

I just left one nit in the unit test but otherwise this looks good; anyway this can be changed if we need it in the future (for example we could use the Service Mixin ?).

Comment 12

[Steve Chung [:steveck]]

Thanks for all the reviews :)

In master: 6be1d1fdd54c999b5f733de5f80e61cb81fbe9a0