Closed
Bug 762760
Opened 12 years ago
Closed 12 years ago
Add IDL for radioState of RadioInterfaceLayer
Categories
(Core :: DOM: Device Interfaces, defect)
Core
DOM: Device Interfaces
Tracking
()
RESOLVED
FIXED
mozilla16
People
(Reporter: kanru, Assigned: allstars.chh)
References
Details
Attachments
(2 files, 7 obsolete files)
3.05 KB,
patch
|
allstars.chh
:
review+
|
Details | Diff | Splinter Review |
20.27 KB,
patch
|
Details | Diff | Splinter Review |
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•12 years ago
|
||
Should we still use the term "nsIRadioState"?
Assignee: nobody → allstars.chh
Reporter | ||
Comment 2•12 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•12 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•12 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•12 years ago
|
||
Hi, philikon I expose icc and cell as IDLs. How do you think about this? thanks
Attachment #631832 -
Flags: feedback?(philipp)
Updated•12 years ago
|
Attachment #631832 -
Flags: feedback?(philipp) → feedback+
Assignee | ||
Comment 6•12 years ago
|
||
Revise to use a more general term : rilContext
Attachment #631832 -
Attachment is obsolete: true
Attachment #633052 -
Flags: review?(philipp)
Assignee | ||
Comment 7•12 years ago
|
||
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 8•12 years ago
|
||
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 9•12 years ago
|
||
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•12 years ago
|
||
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•12 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
Comment 12•12 years ago
|
||
Note: if you change an interface, you must change its IID. It looks like this is the case here for nsIRadioInterfaceLayer
Assignee | ||
Comment 13•12 years ago
|
||
Found extra spaces, qref again carry over r+ by philikon in comment 8
Attachment #633944 -
Flags: review+
Assignee | ||
Comment 14•12 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•12 years ago
|
||
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•12 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•12 years ago
|
||
remove extra space Carry over r+ by philikon in comment 8
Attachment #633946 -
Attachment is obsolete: true
Attachment #633960 -
Flags: review+
Assignee | ||
Comment 18•12 years ago
|
||
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 19•12 years ago
|
||
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•12 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
Comment 21•12 years ago
|
||
(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•12 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•12 years ago
|
||
Try server result: https://tbpl.mozilla.org/?tree=Try&rev=33afabb085a9
Assignee | ||
Comment 24•12 years ago
|
||
http://hg.mozilla.org/integration/mozilla-inbound/rev/ddf9d7173d72 http://hg.mozilla.org/integration/mozilla-inbound/rev/421ed10b0e81
Assignee | ||
Updated•12 years ago
|
Target Milestone: --- → mozilla16
Comment 25•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/ddf9d7173d72
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Comment 26•12 years ago
|
||
(had incorrect bug #) https://hg.mozilla.org/mozilla-central/rev/421ed10b0e81
Assignee | ||
Comment 27•12 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•12 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
Assignee | ||
Comment 30•12 years ago
|
||
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.
Description
•