Closed
Bug 1052836
Opened 10 years ago
Closed 10 years ago
Use enums for preferred network type in nsIMobileConnectionProvider.idl
Categories
(Firefox OS Graveyard :: RIL, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
2.1 S8 (7Nov)
People
(Reporter: anshulj, Assigned: jessica)
References
Details
Attachments
(3 files, 4 obsolete files)
5.86 KB,
patch
|
jessica
:
review+
|
Details | Diff | Splinter Review |
11.61 KB,
patch
|
jessica
:
review+
|
Details | Diff | Splinter Review |
2.93 KB,
patch
|
jessica
:
review+
|
Details | Diff | Splinter Review |
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.
Comment 1•10 years ago
|
||
(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 | ||
Updated•10 years ago
|
Assignee: nobody → jjong
Assignee | ||
Comment 2•10 years ago
|
||
Assignee | ||
Comment 3•10 years ago
|
||
Assignee | ||
Comment 4•10 years ago
|
||
Assignee | ||
Comment 5•10 years ago
|
||
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)
Assignee | ||
Comment 6•10 years ago
|
||
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)
Assignee | ||
Comment 7•10 years ago
|
||
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 8•10 years ago
|
||
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 9•10 years ago
|
||
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 10•10 years ago
|
||
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+
Assignee | ||
Comment 11•10 years ago
|
||
(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.
Comment 12•10 years ago
|
||
(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.
Assignee | ||
Comment 13•10 years ago
|
||
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 14•10 years ago
|
||
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+
Assignee | ||
Comment 15•10 years ago
|
||
(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/
Assignee | ||
Comment 16•10 years ago
|
||
Attachment #8512386 -
Attachment is obsolete: true
Attachment #8514129 -
Flags: review+
Assignee | ||
Comment 17•10 years ago
|
||
Add r=echen
Attachment #8514076 -
Attachment is obsolete: true
Attachment #8514130 -
Flags: review+
Assignee | ||
Comment 18•10 years ago
|
||
Add r=echen
Attachment #8512388 -
Attachment is obsolete: true
Attachment #8514131 -
Flags: review+
Assignee | ||
Comment 19•10 years ago
|
||
try looks good! https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=a6e99a3bb9e5
Keywords: checkin-needed
Comment 20•10 years ago
|
||
https://hg.mozilla.org/integration/b2g-inbound/rev/32f9ff2d2f85 https://hg.mozilla.org/integration/b2g-inbound/rev/55ea4de526aa https://hg.mozilla.org/integration/b2g-inbound/rev/9435080b942d
Keywords: checkin-needed
Comment 21•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/32f9ff2d2f85 https://hg.mozilla.org/mozilla-central/rev/55ea4de526aa https://hg.mozilla.org/mozilla-central/rev/9435080b942d
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → 2.1 S8 (7Nov)
You need to log in
before you can comment on or make changes to this bug.
Description
•