Last Comment Bug 762760 - Add IDL for radioState of RadioInterfaceLayer
: Add IDL for radioState of RadioInterfaceLayer
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM: Device Interfaces (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla16
Assigned To: Yoshi Huang[:allstars.chh] (OOO ~ 9.30)
:
:
Mentors:
Depends on:
Blocks: b2g-ril 715788
  Show dependency treegraph
 
Reported: 2012-06-07 19:21 PDT by Kan-Ru Chen [:kanru] (UTC+8)
Modified: 2012-06-26 02:19 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Part 1 : IDL (2.91 KB, patch)
2012-06-11 02:15 PDT, Yoshi Huang[:allstars.chh] (OOO ~ 9.30)
philipp: feedback+
Details | Diff | Splinter Review
Part 1: IDL (2.91 KB, patch)
2012-06-14 00:17 PDT, Yoshi Huang[:allstars.chh] (OOO ~ 9.30)
philipp: review+
Details | Diff | Splinter Review
Part 2: Implementations (24.26 KB, patch)
2012-06-14 00:19 PDT, Yoshi Huang[:allstars.chh] (OOO ~ 9.30)
philipp: review+
Details | Diff | Splinter Review
Part 1: IDL v2 (2.93 KB, patch)
2012-06-17 20:57 PDT, Yoshi Huang[:allstars.chh] (OOO ~ 9.30)
allstars.chh: review+
Details | Diff | Splinter Review
Part 1: IDL v2 (2.95 KB, patch)
2012-06-17 21:04 PDT, Yoshi Huang[:allstars.chh] (OOO ~ 9.30)
allstars.chh: review+
Details | Diff | Splinter Review
IDL v3 (3.05 KB, patch)
2012-06-17 21:12 PDT, Yoshi Huang[:allstars.chh] (OOO ~ 9.30)
no flags Details | Diff | Splinter Review
Part 1: IDL v4 (3.05 KB, patch)
2012-06-18 00:08 PDT, Yoshi Huang[:allstars.chh] (OOO ~ 9.30)
allstars.chh: review+
Details | Diff | Splinter Review
Part 2: RIL Implementations. v2 (20.22 KB, patch)
2012-06-18 00:14 PDT, Yoshi Huang[:allstars.chh] (OOO ~ 9.30)
allstars.chh: review+
Details | Diff | Splinter Review
Part 2: RIL Implementation v3 (20.27 KB, patch)
2012-06-25 22:22 PDT, Yoshi Huang[:allstars.chh] (OOO ~ 9.30)
no flags Details | Diff | Splinter Review

Description Kan-Ru Chen [:kanru] (UTC+8) 2012-06-07 19:21:12 PDT
The information in radioState is used by NetworkManager and A-GPS backend. RadioInterfaceLayer might its internal implementation, in order to not break them when that happens, we should add interfaces that describe radioState and its properties.

It would be something like:

  interface nsIRadioState {
    readonly attribute int radioState;
    readonly attribute int cardState;
    readonly attribute nsIIccInfo icc;
    readonly attribute nsICellInfo cell;
    readonly attribute nsIDOMMozMobileConnectionInfo voice;
    readonly attribute nsIDOMMozMobileConnectionInfo data;
  };
Comment 1 Yoshi Huang[:allstars.chh] (OOO ~ 9.30) 2012-06-07 23:36:48 PDT
Should we still use the term "nsIRadioState"?
Comment 2 Kan-Ru Chen [:kanru] (UTC+8) 2012-06-08 01:12:30 PDT
(In reply to Yoshi Huang[:yoshi][:allstars.chh] from comment #1)
> Should we still use the term "nsIRadioState"?

I have no preference. Do you have a better name?
Comment 3 Yoshi Huang[:allstars.chh] (OOO ~ 9.30) 2012-06-08 04:34:22 PDT
Hi philikon:
Do you have any suggestion? Or should I use the original naming?

Also should we also define IDL for the IccInfo and CellInfo?
Or simply jsval will be fine?
If we need to make them as IDL,
I would change some attribute from DOMString to unsigned short, i.e. MCC MNC LAC CID.

thanks
Comment 4 Yoshi Huang[:allstars.chh] (OOO ~ 9.30) 2012-06-11 00:52:14 PDT
(In reply to Yoshi Huang[:yoshi][:allstars.chh] from comment #3)
> Hi philikon:
> If we need to make them as IDL,
> I would change some attribute from DOMString to unsigned short, i.e. MCC MNC
> LAC CID.
> 
Also to make it consistent(i.e. for the mcc in nsIDOMMozMobileOperatorInfo) , I'll make those attributes into lower cases,(change from MCC to mcc),
Comment 5 Yoshi Huang[:allstars.chh] (OOO ~ 9.30) 2012-06-11 02:15:08 PDT
Created attachment 631832 [details] [diff] [review]
Part 1 : IDL

Hi, philikon
I expose icc and cell as IDLs.
How do you think about this?

thanks
Comment 6 Yoshi Huang[:allstars.chh] (OOO ~ 9.30) 2012-06-14 00:17:58 PDT
Created attachment 633052 [details] [diff] [review]
Part 1: IDL

Revise to use a more general term : rilContext
Comment 7 Yoshi Huang[:allstars.chh] (OOO ~ 9.30) 2012-06-14 00:19:50 PDT
Created attachment 633055 [details] [diff] [review]
Part 2: Implementations

RIL implementations.
I change the naming to lower case, 
also make mcc, mnc, lac and cid from string to number.

thanks
Comment 8 Philipp von Weitershausen [:philikon] 2012-06-15 15:33:03 PDT
Comment on attachment 633052 [details] [diff] [review]
Part 1: IDL

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

r=me with points addressed

::: dom/system/gonk/nsIRadioInterfaceLayer.idl
@@ +148,5 @@
>    attribute bool speakerEnabled;
>  };
>  
> +[scriptable, uuid(fd9e8b38-b839-4d56-8482-3bf1f5c8f2ee)]
> +interface nsIIccRecords : nsISupports

Normally in Gecko we keep acronyms capitalized, so e.g. nsIICCRecords

@@ +150,5 @@
>  
> +[scriptable, uuid(fd9e8b38-b839-4d56-8482-3bf1f5c8f2ee)]
> +interface nsIIccRecords : nsISupports
> +{
> +  /* Mobile Subscriber ISDN Number */

Nit: please use JavaDoc style comments:

  /**
   * Mobile Subscriber ISDN Number
   */

@@ +186,5 @@
> +  readonly attribute unsigned long cid;
> +};
> +
> +[scriptable, uuid(a6f6977e-f4ee-42b4-ae79-798c8c47c360)]
> +interface nsIRadioState : nsISupports

I thought you were going to call this nsIRILContext?

@@ +229,5 @@
>     * Activates or deactivates radio power.
>     */
>    void setRadioEnabled(in bool value);
>  
> +  readonly attribute nsIRadioState radioState;

rilContext?
Comment 9 Philipp von Weitershausen [:philikon] 2012-06-15 15:36:43 PDT
Comment on attachment 633055 [details] [diff] [review]
Part 2: Implementations

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

r=me with comment below addressed.

::: dom/system/gonk/RadioInterfaceLayer.js
@@ +146,5 @@
> +  classID:        RILCONTEXT_CID,
> +  classInfo:      XPCOMUtils.generateCI({classID: RILCONTEXT_CID,
> +                                       classDescription: "RilContext",
> +                                       interfaces: [Ci.nsIRilContext]}),
> +  QueryInterface: XPCOMUtils.generateQI([Ci.nsIRilContext]),

Pretty sure we don't need to create this class! Just using a simple object like we have up until now with `radioState` *should* be fine. Can you please try it like that? Thanks!

(If it doesn't work, I'm ok with this class, but I'm pretty sure it doesn't need a classID and a classInfo, so please remove at least those.)
Comment 10 Yoshi Huang[:allstars.chh] (OOO ~ 9.30) 2012-06-17 20:57:26 PDT
Created attachment 633943 [details] [diff] [review]
Part 1: IDL v2

Update javadoc comments to address philikon's review and using 'ICC' as convention.

Yes the variable should be rilContext.
Sorry it seems I attached wrong version of IDL last week
Really sorry for my mistake, I'll keep in mind next time.

Carry over last r+ by philikon.

Thanks for your review.
Comment 11 Yoshi Huang[:allstars.chh] (OOO ~ 9.30) 2012-06-17 21:00:20 PDT
Comment on attachment 633943 [details] [diff] [review]
Part 1: IDL v2

># HG changeset patch
># Parent 964b11fea7f11b310250127cc096ed5798f71280
># User Yoshi Huang <yhuang@mozilla.com>
># Date 1339397413 -28800
>
>Bug 762760 - Part 1: IDL. r=philikon
>
>diff --git a/dom/system/gonk/nsIRadioInterfaceLayer.idl b/dom/system/gonk/nsIRadioInterfaceLayer.idl
>--- a/dom/system/gonk/nsIRadioInterfaceLayer.idl
>+++ b/dom/system/gonk/nsIRadioInterfaceLayer.idl
>@@ -143,16 +143,86 @@ interface nsIRILContentHelper : nsIMobil
>   void rejectCall(in unsigned long callIndex);
>   void holdCall(in unsigned long callIndex);
>   void resumeCall(in unsigned long callIndex);
> 
>   attribute bool microphoneMuted;
>   attribute bool speakerEnabled;
> };
> 
>+[scriptable, uuid(fd9e8b38-b839-4d56-8482-3bf1f5c8f2ee)]
>+interface nsIICCRecords : nsISupports
>+{
>+  /**
>+   * Mobile Subscriber ISDN Number
>+   */
>+  readonly attribute DOMString msisdn;
>+
>+  /**
>+   * Administrative Data 
>+   */
>+  readonly attribute jsval ad;
>+
>+  /**
>+   * International Mobile Subscriber Identity 
>+   */
>+  readonly attribute DOMString imsi;
>+
>+  /** 
>+   * Mobile Country Code
>+   */
>+  readonly attribute unsigned short mcc;
>+
>+  /** 
>+   * Mobile Network Code 
>+   */
>+  readonly attribute unsigned short mnc;
>+
>+  /** 
>+   * USIM Service Table
>+  */
>+  readonly attribute jsval ust;
>+
>+  /**
>+   * Abbreviated dialling numbers 
>+   */
>+  readonly attribute jsval adn;
>+
>+  /** 
>+   * Fixed Dialling Numbers 
>+   */  
>+  readonly attribute jsval fdn;
>+};
>+
>+[scriptable, uuid(1b47459d-d0bc-4e91-8509-cc106054b9ee)]
>+interface nsICellLocation : nsISupports
>+{
>+  /* Location Area Code */
>+  readonly attribute unsigned short lac;
>+
>+  /* Cell Identity */
>+  readonly attribute unsigned long cid;
>+};
>+
>+[scriptable, uuid(a6f6977e-f4ee-42b4-ae79-798c8c47c360)]
>+interface nsIRilContext : nsISupports
>+{
>+  readonly attribute DOMString radioState;
>+
>+  readonly attribute DOMString cardState;
>+
>+  readonly attribute nsIICCRecords icc;
>+
>+  readonly attribute nsICellLocation cell;
>+
>+  readonly attribute nsIDOMMozMobileConnectionInfo voice;
>+
>+  readonly attribute nsIDOMMozMobileConnectionInfo data;
>+};
>+
> [scriptable, uuid(fa923335-4cbd-40d7-8d4a-5689a6db2b6d)]
> interface nsIRadioInterfaceLayer : nsISupports
> {
>   const unsigned short CALL_STATE_UNKNOWN = 0;
>   const unsigned short CALL_STATE_DIALING = 1;
>   const unsigned short CALL_STATE_ALERTING = 2;
>   const unsigned short CALL_STATE_BUSY = 3;
>   const unsigned short CALL_STATE_CONNECTING = 4;
>@@ -171,17 +241,17 @@ interface nsIRadioInterfaceLayer : nsISu
>   const unsigned short DATACALL_STATE_DISCONNECTING = 3;
>   const unsigned short DATACALL_STATE_DISCONNECTED = 4;
> 
>   /**
>    * Activates or deactivates radio power.
>    */
>   void setRadioEnabled(in bool value);
> 
>-  readonly attribute jsval radioState;
>+  readonly attribute nsIRilContext rilContext;
> 
>   /**
>    * PDP APIs
>    */
>   void setupDataCall(in long radioTech,
>                      in DOMString apn,
>                      in DOMString user,
>                      in DOMString passwd,
Comment 12 [:fabrice] Fabrice Desré 2012-06-17 21:03:44 PDT
Note: if you change an interface, you must change its IID. It looks like this is the case here for nsIRadioInterfaceLayer
Comment 13 Yoshi Huang[:allstars.chh] (OOO ~ 9.30) 2012-06-17 21:04:30 PDT
Created attachment 633944 [details] [diff] [review]
Part 1: IDL v2

Found extra spaces, qref again

carry over r+ by philikon in comment 8
Comment 14 Yoshi Huang[:allstars.chh] (OOO ~ 9.30) 2012-06-17 21:06:00 PDT
(In reply to Fabrice Desré [:fabrice] from comment #12)
> Note: if you change an interface, you must change its IID. It looks like
> this is the case here for nsIRadioInterfaceLayer

okay, thanks for reminding that.
Doing it now
Comment 15 Yoshi Huang[:allstars.chh] (OOO ~ 9.30) 2012-06-17 21:12:48 PDT
Created attachment 633946 [details] [diff] [review]
IDL v3

Update UUID of nsIRadioInterfaceLayer.
Thanks, fabrice !! 

Carry over r+ by philikon in comment 8
Comment 16 Yoshi Huang[:allstars.chh] (OOO ~ 9.30) 2012-06-17 23:59:55 PDT
Comment on attachment 633946 [details] [diff] [review]
IDL v3

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

::: dom/system/gonk/nsIRadioInterfaceLayer.idl
@@ +195,5 @@
> +[scriptable, uuid(1b47459d-d0bc-4e91-8509-cc106054b9ee)]
> +interface nsICellLocation : nsISupports
> +{
> +  /**
> +   *  Location Area Code

an extra space before 'Location'
Comment 17 Yoshi Huang[:allstars.chh] (OOO ~ 9.30) 2012-06-18 00:08:44 PDT
Created attachment 633960 [details] [diff] [review]
Part 1: IDL v4

remove extra space

Carry over r+ by philikon in comment 8
Comment 18 Yoshi Huang[:allstars.chh] (OOO ~ 9.30) 2012-06-18 00:14:41 PDT
Created attachment 633962 [details] [diff] [review]
Part 2: RIL Implementations. v2

Address to philikon's review comments:
Don't add another class for rilContext.

I've tested in SmsDatabaseService, it still can get icc data by mRIL.rilContext.icc.XXX

Carry over r+ by philikon in comment 9

thanks
Comment 19 Mounir Lamouri (:mounir) 2012-06-18 03:09:09 PDT
Comment on attachment 633960 [details] [diff] [review]
Part 1: IDL v4

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

You seem to be abusing short names. We can afford long and understandable names. I'm not sure if those abbreviations are a norm in the telephony industry but some doesn't seem understandable like "ad".
Comment 20 Yoshi Huang[:allstars.chh] (OOO ~ 9.30) 2012-06-18 03:26:24 PDT
(In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #19)
> I'm not sure if those abbreviations are a norm in the telephony
> industry but some doesn't seem understandable like "ad".
Hi Mounir
Thanks to you comments, but the term I use is exactly from TS 31.102,
for example 
UST: clause 4.2.8
AD: clause 4.2.18
ADN: clause 4.4.2.3
FDN: clause 4.2.24

Also I have written the full name of each abbreviation in comments.

Does it still cause confusion to you?

thanks
Comment 21 Mounir Lamouri (:mounir) 2012-06-18 03:29:03 PDT
(In reply to Yoshi Huang[:yoshi][:allstars.chh] from comment #20)
> (In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #19)
> > I'm not sure if those abbreviations are a norm in the telephony
> > industry but some doesn't seem understandable like "ad".
> Hi Mounir
> Thanks to you comments, but the term I use is exactly from TS 31.102,
> for example 
> UST: clause 4.2.8
> AD: clause 4.2.18
> ADN: clause 4.4.2.3
> FDN: clause 4.2.24
> 
> Also I have written the full name of each abbreviation in comments.
> 
> Does it still cause confusion to you?

If the abbreviations you are using are part of a norm, it is okay for me.
Comment 22 Yoshi Huang[:allstars.chh] (OOO ~ 9.30) 2012-06-18 03:42:25 PDT
(In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #21)
> If the abbreviations you are using are part of a norm, it is okay for me.

Okay, thanks for your comments. :)
Comment 23 Yoshi Huang[:allstars.chh] (OOO ~ 9.30) 2012-06-18 19:12:53 PDT
Try server result:
https://tbpl.mozilla.org/?tree=Try&rev=33afabb085a9
Comment 25 Ed Morley [:emorley] 2012-06-19 01:18:18 PDT
https://hg.mozilla.org/mozilla-central/rev/ddf9d7173d72
Comment 26 Ed Morley [:emorley] 2012-06-19 01:26:16 PDT
(had incorrect bug #)
https://hg.mozilla.org/mozilla-central/rev/421ed10b0e81
Comment 27 Yoshi Huang[:allstars.chh] (OOO ~ 9.30) 2012-06-19 01:29:46 PDT
(In reply to Ed Morley [:edmorley] from comment #26)
> (had incorrect bug #)
> https://hg.mozilla.org/mozilla-central/rev/421ed10b0e81

oh my bad
I'll check how to correct that
sorry for my mistake.

thanks
Comment 28 Yoshi Huang[:allstars.chh] (OOO ~ 9.30) 2012-06-19 02:20:30 PDT
back out: http://hg.mozilla.org/integration/mozilla-inbound/rev/06aa6175c28b
qref and re-push again: http://hg.mozilla.org/integration/mozilla-inbound/rev/d509d8abf5ba

Sorry for causing confusion
Comment 29 Ed Morley [:emorley] 2012-06-20 02:23:48 PDT
https://hg.mozilla.org/mozilla-central/rev/d509d8abf5ba
Comment 30 Yoshi Huang[:allstars.chh] (OOO ~ 9.30) 2012-06-25 22:22:45 PDT
Created attachment 636599 [details] [diff] [review]
Part 2: RIL Implementation v3

Last time I re-push the part 2 patch I forgot to replace the patch in bugzilla as well, sorry for my mistake.

The patch landed in m-c has corrected the bug number in comment already.
https://hg.mozilla.org/mozilla-central/rev/d509d8abf5ba

Note You need to log in before you can comment on or make changes to this bug.