Closed Bug 1052843 Opened 7 years ago Closed 7 years ago

Use enums for MobileRadioState in nsIMobileConnectionProvider.idl

Categories

(Firefox OS Graveyard :: RIL, defect)

x86_64
Windows 7
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
2.1 S7 (24Oct)

People

(Reporter: anshulj, Assigned: jessica)

References

Details

Attachments

(3 files, 4 obsolete files)

The bug 898445 introduced the  MobileRadioState enum in the MozMobileConnection.webidl but no corresponding constants were added in nsIMobileConnectionProvider.idl. 

Please consider adding enums for radio state for getRadioState API in nsIMobileConnectionProvider.idl to strictly define the IPDL interface.
Assignee: nobody → jjong
Attached patch Part 1: idl changes, v1. (obsolete) — Splinter Review
Attached patch Part 3: gonk changes, v1. (obsolete) — Splinter Review
Attached patch Part 2: dom/ipc changes, v2. (obsolete) — Splinter Review
Added assertion stuff.
Attachment #8509331 - Attachment is obsolete: true
Comment on attachment 8509328 [details] [diff] [review]
Part 1: idl changes, v1.

Hsinyi, may I have your review? Thanks.
Attachment #8509328 - Flags: review?(htsai)
Comment on attachment 8510033 [details] [diff] [review]
Part 2: dom/ipc changes, v2.

Edgar, may I have your review? Thanks.
Attachment #8510033 - Flags: review?(echen)
Attachment #8509333 - Flags: review?(echen)
Comment on attachment 8509328 [details] [diff] [review]
Part 1: idl changes, v1.

Review of attachment 8509328 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with comment addressed.

::: dom/mobileconnection/gonk/nsIGonkMobileConnectionService.idl
@@ +39,5 @@
>  
>    void notifyIccChanged(in unsigned long clientId, in DOMString iccId);
>  
>    void notifyNetworkSelectModeChanged(in unsigned long clientId,
> +                                      in long mode);

This shouldn't be here ;)
Attachment #8509328 - Flags: review?(htsai) → review+
Comment on attachment 8509333 [details] [diff] [review]
Part 3: gonk changes, v1.

Review of attachment 8509333 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me, thank you.
Attachment #8509333 - Flags: review?(echen) → review+
Attachment #8510033 - Flags: review?(echen) → review+
The marionette tests in try server is broken now. Please remember to run the tests manually in your local environment before pushing into inbound. Thank you. :)
(In reply to Hsin-Yi Tsai [:hsinyi] from comment #7)
> Comment on attachment 8509328 [details] [diff] [review]
> Part 1: idl changes, v1.
> 
> Review of attachment 8509328 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me with comment addressed.
> 
> ::: dom/mobileconnection/gonk/nsIGonkMobileConnectionService.idl
> @@ +39,5 @@
> >  
> >    void notifyIccChanged(in unsigned long clientId, in DOMString iccId);
> >  
> >    void notifyNetworkSelectModeChanged(in unsigned long clientId,
> > +                                      in long mode);
> 
> This shouldn't be here ;)

oh, this was a miss in bug 1052842, I though to fix it here, or should I file a follow-up bug to bug 1052842?
(In reply to Edgar Chen [:edgar][:echen] from comment #9)
> The marionette tests in try server is broken now. Please remember to run the
> tests manually in your local environment before pushing into inbound. Thank
> you. :)

I have ran the tests locally, tests looks good but I did find some test_wifi_* failures and noticed Henry about them. Thanks for the remind. :)
- Per comment 7, revert changes for notifyNetworkSelectModeChanged(), this will be done in Bug 1052842.
- Add r=hsinyi.
Attachment #8509328 - Attachment is obsolete: true
Attachment #8510881 - Flags: review+
Add r=echen
Attachment #8510033 - Attachment is obsolete: true
Attachment #8510882 - Flags: review+
Add r=echen
Attachment #8509333 - Attachment is obsolete: true
Attachment #8510884 - Flags: review+
You need to log in before you can comment on or make changes to this bug.