Closed Bug 762760 Opened 12 years ago Closed 12 years ago

Add IDL for radioState of RadioInterfaceLayer

Categories

(Core :: DOM: Device Interfaces, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla16

People

(Reporter: kanru, Assigned: allstars.chh)

References

Details

Attachments

(2 files, 7 obsolete files)

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;
  };
Should we still use the term "nsIRadioState"?
Assignee: nobody → allstars.chh
(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?
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
(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),
Attached patch Part 1 : IDL (obsolete) — Splinter Review
Hi, philikon
I expose icc and cell as IDLs.
How do you think about this?

thanks
Attachment #631832 - Flags: feedback?(philipp)
Attachment #631832 - Flags: feedback?(philipp) → feedback+
Attached patch Part 1: IDL (obsolete) — Splinter Review
Revise to use a more general term : rilContext
Attachment #631832 - Attachment is obsolete: true
Attachment #633052 - Flags: review?(philipp)
Attached patch Part 2: Implementations (obsolete) — Splinter Review
RIL implementations.
I change the naming to lower case, 
also make mcc, mnc, lac and cid from string to number.

thanks
Attachment #633055 - Flags: review?(philipp)
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?
Attachment #633052 - Flags: review?(philipp) → review+
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.)
Attachment #633055 - Flags: review?(philipp) → review+
Attached patch Part 1: IDL v2 (obsolete) — Splinter Review
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.
Attachment #633052 - Attachment is obsolete: true
Attachment #633943 - Flags: review+
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,
Attachment #633943 - Attachment is obsolete: true
Note: if you change an interface, you must change its IID. It looks like this is the case here for nsIRadioInterfaceLayer
Attached patch Part 1: IDL v2 (obsolete) — Splinter Review
Found extra spaces, qref again

carry over r+ by philikon in comment 8
Attachment #633944 - Flags: review+
(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
Attached patch IDL v3 (obsolete) — Splinter Review
Update UUID of nsIRadioInterfaceLayer.
Thanks, fabrice !! 

Carry over r+ by philikon in comment 8
Attachment #633944 - Attachment is obsolete: true
Attachment #633946 - Flags: review+
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'
Attachment #633946 - Flags: review+
Attached patch Part 1: IDL v4Splinter Review
remove extra space

Carry over r+ by philikon in comment 8
Attachment #633946 - Attachment is obsolete: true
Attachment #633960 - Flags: review+
Attached patch Part 2: RIL Implementations. v2 (obsolete) — Splinter Review
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
Attachment #633055 - Attachment is obsolete: true
Attachment #633962 - Flags: review+
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".
(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
(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.
(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. :)
Target Milestone: --- → mozilla16
https://hg.mozilla.org/mozilla-central/rev/ddf9d7173d72
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
(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
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
Attachment #633962 - Attachment is obsolete: true
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: