[Messages][NG] Layout mozMobileMessageShim structure

RESOLVED FIXED in 2.2 S14 (12june)

Status

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

People

(Reporter: steveck, Assigned: steveck)

Tracking

(Depends on: 1 bug)

unspecified
2.2 S14 (12june)
ARM
Gonk (Firefox OS)
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [p=1])

Attachments

(1 attachment)

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

Description

3 years ago
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).
Whiteboard: [p=1]
Target Milestone: --- → 2.2 S14 (12june)
(Assignee)

Comment 1

3 years ago
Created attachment 8616724 [details] [review]
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!
Attachment #8616724 - Flags: feedback?(felash)
Attachment #8616724 - Flags: feedback?(azasypkin)
Comment on attachment 8616724 [details] [review]
Link to github

I left some comments but this looks good
Attachment #8616724 - Flags: feedback?(felash) → feedback+
Comment on attachment 8616724 [details] [review]
Link to github

lgtm as well, just minor nits at GitHub.
Attachment #8616724 - Flags: feedback?(azasypkin) → feedback+
Depends on: 1172794
Assignee: nobody → schung
Status: NEW → ASSIGNED
Blocks: 1172906
(Assignee)

Comment 4

3 years ago
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.
Attachment #8616724 - Flags: review?(felash)
Attachment #8616724 - Flags: review?(azasypkin)
Looks good overall, left some comments and questions at gitHub, mostly nits.

Keeping r? for now.

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

I'll leave Oleg handle this tomorrow as I'm traveling.
Attachment #8616724 - Flags: review?(felash)
(Assignee)

Comment 7

3 years ago
Hi Oleg, I updated the patch with a question about the init part. Thanks for all the review!
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!
Attachment #8616724 - Flags: review?(azasypkin)
(Assignee)

Comment 9

3 years ago
Comment on attachment 8616724 [details] [review]
Link to github

Ok, init removed with other suggestion addressed.
Attachment #8616724 - Flags: review?(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!
Flags: needinfo?(felash)
Attachment #8616724 - Flags: review?(azasypkin) → review+
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 ?).
Flags: needinfo?(felash)
(Assignee)

Comment 12

3 years ago
Thanks for all the reviews :)

In master: 6be1d1fdd54c999b5f733de5f80e61cb81fbe9a0
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
(Assignee)

Updated

3 years ago
Blocks: 1179628
No longer blocks: 1179628
You need to log in before you can comment on or make changes to this bug.