Closed Bug 1052836 Opened 7 years ago Closed 7 years ago

Use enums for preferred network type in nsIMobileConnectionProvider.idl

Categories

(Firefox OS Graveyard :: RIL, defect)

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
2.1 S8 (7Nov)

People

(Reporter: anshulj, Assigned: jessica)

References

Details

Attachments

(3 files, 4 obsolete files)

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

Please consider adding enums for preferred network type for setPreferredNetworkType API in nsIMobileConnectionProvider.idl to strictly define the IPDL interface.
(In reply to Anshul from comment #0)
> The bug 898445 introduced the MobilePreferredNetworkType enum in the
> MozMobileConnection.webidl but no corresponding constants were added in
> nsIMobileConnectionProvider.idl. 
> 
> Please consider adding enums for preferred network type for
> setPreferredNetworkType API in nsIMobileConnectionProvider.idl to strictly
> define the IPDL interface.

I had filed Bug 1047203 since our last meeting. Will add this as dependency, thanks!
Assignee: nobody → jjong
Attached patch Part 1: idl changes, v1. (obsolete) — Splinter Review
Attached patch Part 3: gonk changes, v1. (obsolete) — Splinter Review
Comment on attachment 8512386 [details] [diff] [review]
Part 1: idl changes, v1.

Hsinyi, may I have you review on this? Thanks.
Attachment #8512386 - Flags: review?(htsai)
Comment on attachment 8512387 [details] [diff] [review]
Part 2: dom/ipc changes, v1.

Edgar, may I have your feedback on this? Thanks.
Attachment #8512387 - Flags: feedback?(echen)
Comment on attachment 8512388 [details] [diff] [review]
Part 3: gonk changes, v1.

Edgar, may I have your review on this? Thanks.
Attachment #8512388 - Flags: review?(echen)
Comment on attachment 8512387 [details] [diff] [review]
Part 2: dom/ipc changes, v1.

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

Please remember to add checks in Assertions.cpp, thank you.

::: dom/mobileconnection/MobileConnection.cpp
@@ +476,5 @@
>      aRv.Throw(NS_ERROR_FAILURE);
>      return nullptr;
>    }
>  
> +  int32_t type = int32_t(aType);

static_cast<int32_t>
Attachment #8512387 - Flags: feedback?(echen)
Comment on attachment 8512386 [details] [diff] [review]
Part 1: idl changes, v1.

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

Looks good to me!
Attachment #8512386 - Flags: review?(htsai) → review+
Comment on attachment 8512388 [details] [diff] [review]
Part 3: gonk changes, v1.

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

Thank you.

::: dom/system/gonk/ril_worker.js
@@ -6292,5 @@
>      return;
>    }
>  
> -  let networkType = this.context.Buf.readInt32List()[0];
> -  options.type = RIL_PREFERRED_NETWORK_TYPE_TO_GECKO[networkType];

The item in RIL_PREFERRED_NETWORK_TYPE_TO_GECKO array is no longer been referenced. But |setPreferredNetworkType()| still use RIL_PREFERRED_NETWORK_TYPE_TO_GECKO.length for parameter checking. And I don't have really better suggestion.
Attachment #8512388 - Flags: review?(echen) → review+
(In reply to Edgar Chen [:edgar][:echen] from comment #10)
> Comment on attachment 8512388 [details] [diff] [review]
> Part 3: gonk changes, v1.
> 
> Review of attachment 8512388 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thank you.
> 
> ::: dom/system/gonk/ril_worker.js
> @@ -6292,5 @@
> >      return;
> >    }
> >  
> > -  let networkType = this.context.Buf.readInt32List()[0];
> > -  options.type = RIL_PREFERRED_NETWORK_TYPE_TO_GECKO[networkType];
> 
> The item in RIL_PREFERRED_NETWORK_TYPE_TO_GECKO array is no longer been
> referenced. But |setPreferredNetworkType()| still use
> RIL_PREFERRED_NETWORK_TYPE_TO_GECKO.length for parameter checking. And I
> don't have really better suggestion.

Thanks Edgar for the review. Hmm, |_getSupportedNetworkTypes()| still uses RIL_PREFERRED_NETWORK_TYPE_TO_GECKO as well, so I thought to keep it for now.
(In reply to Jessica Jong [:jjong] [:jessica] from comment #11)
> (In reply to Edgar Chen [:edgar][:echen] from comment #10)
> > Comment on attachment 8512388 [details] [diff] [review]
> > Part 3: gonk changes, v1.
> > 
> > Review of attachment 8512388 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > Thank you.
> > 
> > ::: dom/system/gonk/ril_worker.js
> > @@ -6292,5 @@
> > >      return;
> > >    }
> > >  
> > > -  let networkType = this.context.Buf.readInt32List()[0];
> > > -  options.type = RIL_PREFERRED_NETWORK_TYPE_TO_GECKO[networkType];
> > 
> > The item in RIL_PREFERRED_NETWORK_TYPE_TO_GECKO array is no longer been
> > referenced. But |setPreferredNetworkType()| still use
> > RIL_PREFERRED_NETWORK_TYPE_TO_GECKO.length for parameter checking. And I
> > don't have really better suggestion.
> 
> Thanks Edgar for the review. Hmm, |_getSupportedNetworkTypes()| still uses
> RIL_PREFERRED_NETWORK_TYPE_TO_GECKO as well, so I thought to keep it for now.

Ah, yes, you are right.
Attached patch Part 2: dom/ipc changes, v2. (obsolete) — Splinter Review
Changes since v1 (address review comment 8):
- use static_cast instead of function-style cast
- add assertion part

Edgar, may I have your feedback again? Thanks.
Attachment #8512387 - Attachment is obsolete: true
Attachment #8514076 - Flags: feedback?(echen)
Comment on attachment 8514076 [details] [diff] [review]
Part 2: dom/ipc changes, v2.

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

The changes here are trivial, so give r+. Thank you.
Attachment #8514076 - Flags: feedback?(echen) → review+
(In reply to Edgar Chen [:edgar][:echen] from comment #14)
> Comment on attachment 8514076 [details] [diff] [review]
> Part 2: dom/ipc changes, v2.
> 
> Review of attachment 8514076 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> The changes here are trivial, so give r+. Thank you.

Thank you! \o/
Attachment #8512386 - Attachment is obsolete: true
Attachment #8514129 - Flags: review+
Add r=echen
Attachment #8514076 - Attachment is obsolete: true
Attachment #8514130 - Flags: review+
Add r=echen
Attachment #8512388 - Attachment is obsolete: true
Attachment #8514131 - Flags: review+
You need to log in before you can comment on or make changes to this bug.