Add IDL for radioState of RadioInterfaceLayer

RESOLVED FIXED in mozilla16

Status

()

Core
DOM: Device Interfaces
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: kanru, Assigned: allstars)

Tracking

Trunk
mozilla16
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 7 obsolete attachments)

(Reporter)

Description

5 years ago
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;
  };
(Assignee)

Comment 1

5 years ago
Should we still use the term "nsIRadioState"?
Assignee: nobody → allstars.chh
(Reporter)

Comment 2

5 years ago
(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?
(Assignee)

Comment 3

5 years ago
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
(Assignee)

Comment 4

5 years ago
(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),
(Assignee)

Comment 5

5 years ago
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
Attachment #631832 - Flags: feedback?(philipp)
Attachment #631832 - Flags: feedback?(philipp) → feedback+
(Assignee)

Comment 6

5 years ago
Created attachment 633052 [details] [diff] [review]
Part 1: IDL

Revise to use a more general term : rilContext
Attachment #631832 - Attachment is obsolete: true
Attachment #633052 - Flags: review?(philipp)
(Assignee)

Comment 7

5 years ago
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
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+
(Assignee)

Comment 10

5 years ago
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.
Attachment #633052 - Attachment is obsolete: true
Attachment #633943 - Flags: review+
(Assignee)

Comment 11

5 years ago
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
(Assignee)

Comment 13

5 years ago
Created attachment 633944 [details] [diff] [review]
Part 1: IDL v2

Found extra spaces, qref again

carry over r+ by philikon in comment 8
Attachment #633944 - Flags: review+
(Assignee)

Comment 14

5 years ago
(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
(Assignee)

Comment 15

5 years ago
Created attachment 633946 [details] [diff] [review]
IDL v3

Update UUID of nsIRadioInterfaceLayer.
Thanks, fabrice !! 

Carry over r+ by philikon in comment 8
Attachment #633944 - Attachment is obsolete: true
Attachment #633946 - Flags: review+
(Assignee)

Comment 16

5 years ago
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+
(Assignee)

Comment 17

5 years ago
Created attachment 633960 [details] [diff] [review]
Part 1: IDL v4

remove extra space

Carry over r+ by philikon in comment 8
Attachment #633946 - Attachment is obsolete: true
Attachment #633960 - Flags: review+
(Assignee)

Comment 18

5 years ago
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
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".
(Assignee)

Comment 20

5 years ago
(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.
(Assignee)

Comment 22

5 years ago
(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. :)
(Assignee)

Comment 23

5 years ago
Try server result:
https://tbpl.mozilla.org/?tree=Try&rev=33afabb085a9
(Assignee)

Comment 24

5 years ago
http://hg.mozilla.org/integration/mozilla-inbound/rev/ddf9d7173d72
http://hg.mozilla.org/integration/mozilla-inbound/rev/421ed10b0e81
(Assignee)

Updated

5 years ago
Target Milestone: --- → mozilla16
https://hg.mozilla.org/mozilla-central/rev/ddf9d7173d72
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
(had incorrect bug #)
https://hg.mozilla.org/mozilla-central/rev/421ed10b0e81
(Assignee)

Comment 27

5 years ago
(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
(Assignee)

Comment 28

5 years ago
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
https://hg.mozilla.org/mozilla-central/rev/d509d8abf5ba
(Assignee)

Comment 30

5 years ago
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
Attachment #633962 - Attachment is obsolete: true
You need to log in before you can comment on or make changes to this bug.