As already reported in gaia bug https://github.com/mozilla-b2g/gaia/issues/4861 the current handling of carrier name display is erroneous for Mobile Virtual Network Operators. I think it has something to do with the SPN display condition bits defined with the EF_SPN data of SIM card. See http://www.etsi.org/deliver/etsi_ts/131100_131199/131102/08.04.00_60/ts_131102v080400p.pdf#27. Looking at Android's way of handling this topic, we can find that in https://android.googlesource.com/platform/frameworks/opt/telephony/+/master/src/java/com/android/internal/telephony/gsm/GsmServiceStateTracker.java and more precisely in the updateSpnDisplay() method. Basically, it seems to read the display condition bits associated with the EF_SPN field and depending on its value, it exposes either the SIM's SPN value or PLMN value. The ss.getOperatorAlphaLong() that is used to build plmn in this code comes from the RIL, through a parcel. So it is the longName of the network we are currently connected to. The display rule used in updateSpnDisplay() comes from https://android.googlesource.com/platform/frameworks/opt/telephony/+/master/src/java/com/android/internal/telephony/gsm/SIMRecords.java in method getDisplayRule(). And it shows reference to isOnMatchingPlmn(), which works with the PLMN list stored in the SIM card. Which we currently do not retrieve. See EF_OPL and EF_PNN in TS 131 102 (http://www.etsi.org/deliver/etsi_ts/131100_131199/131102/08.04.00_60/ts_131102v080400p.pdf).

https://android.googlesource.com/platform/frameworks/opt/telephony/+/master/src/java/com/android/internal/telephony/gsm/SIMRecords.java parseEfSpdi() we need to add EF_SPDI retrieval, in fact.

I'm currently trying to address this. I already have EF_SPDI retrieval working, now it's just a matter of using display condition correctly.

Here is a first patch proposal. It's a RFC, there is a log of debug inside that I will have to remove after. Currently, it computes the correct displayName in my case (Free Mobile in France, roaming on Orange network), and from the bits I have it should also compute the correct displayName for the "NRJ Mobile" instance (which is a MVNO in France, and was the reason I found the bug). Yet I don't have my hands on the NRJ SIM card. The overall idea is to: - retrieve the list of known network (EF_SPDI) from SIM card - check if the current network is: - the main network (home plmn), as defined in icc.{mcc,mnc} - an authorized network (in the EF_SPDI list) - follow the specs to display either the network name registered in the SIM card (SPN, Service Provider Name, in icc.spn) or the current network name. The first case is the missing bit for NRJ Mobile, while the last one is the expected behavior when roaming abroad. - the computed name to be displayed is added in nsIDOMMozMobileNetworkInfo as "displayName" and should be used by Gaia. This part of the work is trivial. One thing I do not understand for now is why with my "chatr" SIM card (Canadian MVNO using Rogers network) I have a list of PLMN (when getting EF_SPDI response) but all PLMN are just empty strings.

Removing some empty lines, and adding the missing spnDisplayCondition field of nsIDOMMozMobileICCInfo which is set and used now.

It looks like EF_SPDI is only USIM application, and that the SIM equivalent is EF_PLMNsel. Adding handler for this one too, and checking if the service is available in both case. Not that I only have USIM, can't test the SIM only part.

Rewriting the PLMN list reading code, and factorizing it along EF_PLMN and EF_SPDI handlers. I still do not understand why my 'chatr' SIM card returns a number of 85 entries while only the 4 first are readable and only contains '0xFF 0xFF 0xFF' which according to the specs means "Unused PLMN entry".

Some cosmetic changes.

Fixing typo and cosmetic. There is still debug (a lot).

Comment on attachment 663717 [details] [diff] [review] Patch proposal, request for comments. v7 Moving the updateSpnDisplay() to ril_worker, as suggested on IRC.

Updated to current master.

Comment on attachment 663755 [details] [diff] [review] Patch proposal, request for comments. v8 Review of attachment 663755 [details] [diff] [review]: ----------------------------------------------------------------- Hi, Lissy Thanks for your patch. Sorry for my late replied. I checked your code and provided some comments below. Can you check that, please? This time I just check Icc IO related functions, might need more time to understand this patch Thanks ::: dom/system/gonk/ril_consts.js @@ +706,5 @@ > ADN: 2, > FDN: 3, > SDN: 18, > + BDN: 31, > + PLMN: 48 Could you specify more clear about PLMN? Since there are many terms related to PLMN. Also, this number should be wrong, right? Please check TS 51.011, PLMN_Selector should be in 7. (I also check TS 100.977, the same, it should be 7) ::: dom/system/gonk/ril_worker.js @@ +1243,5 @@ > + */ > + getPLMN: function getPLMN() { > + function callback() { > + let length = Buf.readUint32(); > + this.iccInfo.plmn_list = this.readPLMNEntries(length / 3); here the 'length' is the length of string(response). Given that each octet is encoded into two characters, so there should be length/2 octets, hence (length/2) / 3 entries. @@ +1262,5 @@ > + if (DEBUG) { > + debug("PLMN: Not a SIM card, no point in EF_PLMNsel."); > + } > + return; > + } I think we should do this check before we call this function. @@ +1301,5 @@ > + debug("SPDI: tag=" + tag); > + } > + > + // Skip SPDI tag > + if (tag == 0xA3) { Can you const this, please? @@ +1303,5 @@ > + > + // Skip SPDI tag > + if (tag == 0xA3) { > + let bufLen2 = Buf.readUint32(); > + tag = GsmPDUHelper.readHexOctet(); Sorry I don't quite understand here. Do you mean the SPDI-TLV might come first? @@ +1310,5 @@ > + debug("SPDI: tag2=" + tag); > + } > + } > + > + if (tag == 0x80) { const, too. @@ +1331,5 @@ > + if (DEBUG) { > + debug("SPDI: Not a USIM card, no point in EF_SPDI."); > + } > + return; > + } check this before calling this function. @@ +1698,5 @@ > + if (plmn[0] != 0xFF && > + plmn[1] != 0xFF && > + plmn[2] != 0xFF) { > + let plmnStr = ""; > + for (var idx = 0; idx < plmn.length; idx++) { var? @@ +1707,5 @@ > + plmnStr += GsmPDUHelper.semiOctetToBcdChar(nibbleL); > + if (nibbleH != 0x0F) { > + plmnStr += GsmPDUHelper.semiOctetToBcdChar(nibbleH); > + } > + } Can you check GsmPDUHelper.readSwappedNibbleBcdString(3) fits your needs?

(In reply to Yoshi Huang[:yoshi][:allstars.chh] from comment #12) > Comment on attachment 663755 [details] [diff] [review] > Patch proposal, request for comments. v8 > > Review of attachment 663755 [details] [diff] [review]: > ----------------------------------------------------------------- > > Hi, Lissy > Thanks for your patch. > Sorry for my late replied. > > I checked your code and provided some comments below. > Can you check that, please? > > This time I just check Icc IO related functions, > might need more time to understand this patch > > Thanks > > ::: dom/system/gonk/ril_consts.js > @@ +706,5 @@ > > ADN: 2, > > FDN: 3, > > SDN: 18, > > + BDN: 31, > > + PLMN: 48 > > Could you specify more clear about PLMN? > Since there are many terms related to PLMN. > Also, this number should be wrong, right? > > Please check TS 51.011, PLMN_Selector should be in 7. > (I also check TS 100.977, the same, it should be 7) Yeah, I figured it out later, it's 7 the correct one. Maybe PLMNsel (matches the EF_PLMNsel defined in the spec). > > ::: dom/system/gonk/ril_worker.js > @@ +1243,5 @@ > > + */ > > + getPLMN: function getPLMN() { > > + function callback() { > > + let length = Buf.readUint32(); > > + this.iccInfo.plmn_list = this.readPLMNEntries(length / 3); > > here the 'length' is the length of string(response). > Given that each octet is encoded into two characters, > so there should be length/2 octets, > hence (length/2) / 3 entries. Maybe, I don't really get how it's read anyway ... > > @@ +1262,5 @@ > > + if (DEBUG) { > > + debug("PLMN: Not a SIM card, no point in EF_PLMNsel."); > > + } > > + return; > > + } > > I think we should do this check before we call this function. > > @@ +1301,5 @@ > > + debug("SPDI: tag=" + tag); > > + } > > + > > + // Skip SPDI tag > > + if (tag == 0xA3) { > > Can you const this, please? Yeah, this was why I stated it's preliminary ... > > @@ +1303,5 @@ > > + > > + // Skip SPDI tag > > + if (tag == 0xA3) { > > + let bufLen2 = Buf.readUint32(); > > + tag = GsmPDUHelper.readHexOctet(); > > Sorry I don't quite understand here. > Do you mean the SPDI-TLV might come first? I don't either, Android does this, and I have this behavior on my SIM card, so it looks this case can happen. > > @@ +1310,5 @@ > > + debug("SPDI: tag2=" + tag); > > + } > > + } > > + > > + if (tag == 0x80) { > > const, too. > > @@ +1331,5 @@ > > + if (DEBUG) { > > + debug("SPDI: Not a USIM card, no point in EF_SPDI."); > > + } > > + return; > > + } > > check this before calling this function. > > @@ +1698,5 @@ > > + if (plmn[0] != 0xFF && > > + plmn[1] != 0xFF && > > + plmn[2] != 0xFF) { > > + let plmnStr = ""; > > + for (var idx = 0; idx < plmn.length; idx++) { > > var? > > @@ +1707,5 @@ > > + plmnStr += GsmPDUHelper.semiOctetToBcdChar(nibbleL); > > + if (nibbleH != 0x0F) { > > + plmnStr += GsmPDUHelper.semiOctetToBcdChar(nibbleH); > > + } > > + } > > Can you check > GsmPDUHelper.readSwappedNibbleBcdString(3) fits your needs? If you provide a way to check for 0xFF without readSwapped... throwing up ...

Moving back updateSpnDisplay to RadioInterfaceLayer, seems to be less prone to race condition.

(In reply to Alexandre LISSY :gerard-majax from comment #14) > Moving back updateSpnDisplay to RadioInterfaceLayer, seems to be less prone > to race condition. Can you explain what you mean by that? Which patch do you want me to review?

(In reply to Philipp von Weitershausen [:philikon] from comment #15) > (In reply to Alexandre LISSY :gerard-majax from comment #14) > > Moving back updateSpnDisplay to RadioInterfaceLayer, seems to be less prone > > to race condition. > > Can you explain what you mean by that? Which patch do you want me to review? I had spurious behavior: sometimes operatorchange comes before icc infos are ready and in that case we cannot use the plmn_list, henc ewe have to wait for the next operatorchange ... There is somehow a need for a barrier between those ICC infos and operatorchange, and maybe throwing a new specific event to the system to notify of spn display change. Since you said it's not a priority for now, and since I don't have the time to hack deeper in this, let's let this pending for now.

(In reply to Alexandre LISSY :gerard-majax from comment #16) > (In reply to Philipp von Weitershausen [:philikon] from comment #15) > > (In reply to Alexandre LISSY :gerard-majax from comment #14) > > > Moving back updateSpnDisplay to RadioInterfaceLayer, seems to be less prone > > > to race condition. > > > > Can you explain what you mean by that? Which patch do you want me to review? > > I had spurious behavior: sometimes operatorchange comes before icc infos are > ready and in that case we cannot use the plmn_list, henc ewe have to wait > for the next operatorchange ... Or just force an operatorchange message!

Comment on attachment 663755 [details] [diff] [review] Patch proposal, request for comments. v8 Review of attachment 663755 [details] [diff] [review]: ----------------------------------------------------------------- Nice. Thank for this work. Sorry for the long wait, but since it's not a blocker, it was de-prioritized. Technically this maybe classified as a feature. In any case it amends the MobileConnection DOM API, so I'm flagging sicking for superreview. As for the actual patch, Yoshi raised some good questions. Please address those, too. Thanks! ::: dom/network/interfaces/nsIDOMMobileConnection.idl @@ +378,5 @@ > /** > + * Service Provider Name (SPN) display condition, controls whether > + * we will display the SPN or the current PLMN. > + */ > + readonly attribute DOMString spnDisplayCondition; I'm confused. A "condition" seems to suggest a Boolean type value. You declare it as DOMString here. But the code in ril_worker.js seems to assign an integer to it. Which is it? Whichever type is the correct one, the interface could be more descriptive. If it's a number or string, which values are allowed and what do they mean? And maybe "condition" is the wrong word? ::: dom/system/gonk/ril_worker.js @@ +1351,5 @@ > + }); > + } else { > + if (DEBUG) { > + debug("SPDI: No service."); > + } I generally prefer bail-out-early style, especially for error handling: if (!this.isICCServiceAvailable("SPDI")) { if (DEBUG) debug("SPDI: No service."); return; } // PLMN List is Servive 51 in USIM, EF_SPDI this.iccIO({ ... }); (Also relevant to getPLMN().) @@ +2866,5 @@ > + } > + } > + > + /* Now determine whether we display PLMN or SPN > + */ Style nit: use // comments inside a function body. @@ +2895,5 @@ > + if (spnDisplay != "") { > + network.displayName = spnDisplay; > + } > + > + debug("updateSpnDisplay: spnDisplay=" + spnDisplay); All calls to `debug()` in ril_worker.js *must* be prefixed with `if (DEBUG)`. Please fix *all* call sites in your patch. @@ +2962,5 @@ > } > } > > + this._updateSpnDisplay(pending.operator); > + What I said in comment 17: If the ICC comes online or the getSPDI() and getPLMN() respond after the first 'operatorchange' or 'networkinfochanged' message, we should still update the spn display name via a message.

(In reply to Philipp von Weitershausen [:philikon] from comment #18) > Comment on attachment 663755 [details] [diff] [review] > Patch proposal, request for comments. v8 > > Review of attachment 663755 [details] [diff] [review]: > ----------------------------------------------------------------- > > Nice. Thank for this work. Sorry for the long wait, but since it's not a > blocker, it was de-prioritized. > > Technically this maybe classified as a feature. In any case it amends the > MobileConnection DOM API, so I'm flagging sicking for superreview. > > As for the actual patch, Yoshi raised some good questions. Please address > those, too. Thanks! I replied to those. > > ::: dom/network/interfaces/nsIDOMMobileConnection.idl > @@ +378,5 @@ > > /** > > + * Service Provider Name (SPN) display condition, controls whether > > + * we will display the SPN or the current PLMN. > > + */ > > + readonly attribute DOMString spnDisplayCondition; > > I'm confused. A "condition" seems to suggest a Boolean type value. You > declare it as DOMString here. But the code in ril_worker.js seems to assign > an integer to it. Which is it? > > Whichever type is the correct one, the interface could be more descriptive. > If it's a number or string, which values are allowed and what do they mean? > And maybe "condition" is the wrong word? It's an integer, and the same comes from the specs. See TS 131.102, section 4.2.12 (for USIM), it explains everything. Maybe I should add a reference to this in the comment ? > > ::: dom/system/gonk/ril_worker.js > @@ +1351,5 @@ > > + }); > > + } else { > > + if (DEBUG) { > > + debug("SPDI: No service."); > > + } > > I generally prefer bail-out-early style, especially for error handling: > > if (!this.isICCServiceAvailable("SPDI")) { > if (DEBUG) debug("SPDI: No service."); > return; > } > // PLMN List is Servive 51 in USIM, EF_SPDI > this.iccIO({ > ... > }); > > (Also relevant to getPLMN().) > > @@ +2866,5 @@ > > + } > > + } > > + > > + /* Now determine whether we display PLMN or SPN > > + */ > > Style nit: use // comments inside a function body. > > @@ +2895,5 @@ > > + if (spnDisplay != "") { > > + network.displayName = spnDisplay; > > + } > > + > > + debug("updateSpnDisplay: spnDisplay=" + spnDisplay); > > All calls to `debug()` in ril_worker.js *must* be prefixed with `if > (DEBUG)`. Please fix *all* call sites in your patch. > > @@ +2962,5 @@ > > } > > } > > > > + this._updateSpnDisplay(pending.operator); > > + > > What I said in comment 17: If the ICC comes online or the getSPDI() and > getPLMN() respond after the first 'operatorchange' or 'networkinfochanged' > message, we should still update the spn display name via a message. How to do so ?

Addressing most of the remarks, not all (readPLMNEntries and length are not addressed).

Comment on attachment 666189 [details] [diff] [review] Patch proposal, request for comments. v9 Review of attachment 666189 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/network/interfaces/nsIDOMMobileConnection.idl @@ +398,5 @@ > + /** > + * Mobile Station ISDN Number (MSISDN) of the subscriber's, aka > + * his phone number. > + */ > + readonly attribute DOMString msisdn; Scope creep: I thought bug 788191 was going to be about exposing msisdn and spn? Remember how there's a test in that patch? Please don't just merge patches together. r- for that. Also, this is not a blocker and I think adds too much risk at this point (it has no tests, for instance.) If that were to change (somebody nominated it for blocking-basecamp and it got approved, because we cannot ship a phone without it), then the good news is that we'd already have a patch.

(In reply to Philipp von Weitershausen [:philikon] (afk until 2012-10-14) from comment #21) > Comment on attachment 666189 [details] [diff] [review] > Patch proposal, request for comments. v9 > > Review of attachment 666189 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: dom/network/interfaces/nsIDOMMobileConnection.idl > @@ +398,5 @@ > > + /** > > + * Mobile Station ISDN Number (MSISDN) of the subscriber's, aka > > + * his phone number. > > + */ > > + readonly attribute DOMString msisdn; > > Scope creep: I thought bug 788191 was going to be about exposing msisdn and > spn? Remember how there's a test in that patch? Please don't just merge > patches together. Which explains why bug 788191 is a blocker for this one. Never intended to merge it in your back ...

Hi Lissy, just found that this patch haven't been updated for a while, are you still working on this bug? If you are too busy now, I can continue to work on this basecamp-blocking bug.

Nope, I'm sorry I do not have the time right now, so I'm glad you want to take it. Basically, to summarize, you need: - ensure correctness of the readPLMNEntries() method: right now, it reimplements a lot of other GsmPDUHelper because I did not wanted to take the risk of changing their behavior, due to the presence of 0xFF 0xFF 0xFF that is expected as of the specs - move back most of the code to ril_worker, as suggested by philikon. - ensure correct synchronization: to be able to work, the current updateSpnDisplay method needs both iccInfo to be ready, and an operatorchange message. There is probably a smarter way than waiting for the operatorchange message, but for a first hacking try I did not pursue moreover. Whatever, you will still need to have the network name, and icc ready in any case. - Test on USIM and SIM card. I only had a USIM, I don't know if there are still SIM out in the live but at least the emulator has. - Update Gaia to support the new displayName property I introduced to expose the name of the network that we should expose to the user, according to the specifications rules (display conditions, home plmn, etc.) Other than that, and of course rebasing to update to the current code, but it's not that intrusive so it should be quite straightforward, the main logic is here and should be okay, as far as I've been able to test.

(In reply to Alexandre LISSY :gerard-majax from comment #24) > Nope, I'm sorry I do not have the time right now, so I'm glad you want to > take it. Basically, to summarize, you need: Patrick and I have been study related issues like bug 798000 and bug 798008 these days. I would like to know whether or not should we expose so many SIM entries to content, especially those duplicated-in-meaning entries SPN/longName/shortName/displayName. We suppose, might need more code trace/document study to ensure, we only have to expose an 'name' attribute. Let all the calculation be in ril_worker and report final result only.

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #25) > (In reply to Alexandre LISSY :gerard-majax from comment #24) > > Nope, I'm sorry I do not have the time right now, so I'm glad you want to > > take it. Basically, to summarize, you need: > > Patrick and I have been study related issues like bug 798000 and bug 798008 > these days. I would like to know whether or not should we expose so many SIM > entries to content, especially those duplicated-in-meaning entries > SPN/longName/shortName/displayName. We suppose, might need more code > trace/document study to ensure, we only have to expose an 'name' attribute. > Let all the calculation be in ril_worker and report final result only. I agree, you probably have just to expose what I called 'displayName' (and you called 'name') to Gaia, altough one can find useful to have access to the other for writing applications.

Rebase previous patch on current inbound.

(In reply to Alexandre LISSY :gerard-majax from comment #26) > I agree, you probably have just to expose what I called 'displayName' (and > you called 'name') to Gaia, altough one can find useful to have access to > the other for writing applications. Do we use SPN somewhere other than forming carrier name string? If SPN is only for carrier name, after we move the task of computing carrier name string into ril_worker, we might able to replace SPN by displayname in iccInfo object.

(In reply to Patrick Wang [:kk1fff] from comment #28) > (In reply to Alexandre LISSY :gerard-majax from comment #26) > > I agree, you probably have just to expose what I called 'displayName' (and > > you called 'name') to Gaia, altough one can find useful to have access to > > the other for writing applications. > > Do we use SPN somewhere other than forming carrier name string? If SPN is > only for carrier name, after we move the task of computing carrier name > string into ril_worker, we might able to replace SPN by displayname in > iccInfo object. It is being used in the roaming checking method in RadioInterfaceLayer, checkRoamingBetweenOperators().

Comment on attachment 677714 [details] [diff] [review] Patch 2: Getting SPDI, SPN, PLMN file Still WIP, I think we may need to study more to see if it's safe to keep longName, shortName and spn only in ril_worker. But at least, displayName is generated in ril_worker in this patch. Vicamo, would you help to put some feedback on this? Thanks.

Comment on attachment 677714 [details] [diff] [review] Patch 2: Getting SPDI, SPN, PLMN file Review of attachment 677714 [details] [diff] [review]: ----------------------------------------------------------------- About moving roaming checking into ril_worker, I think it's possible: _processVoiceRegistrationState: function _processVoiceRegistrationState() { ... if (stateChanged) { // Insert code here. Note that SPN/PLMN/SPDI might have not // been fetched at the time. } } ::: dom/system/gonk/RadioInterfaceLayer.js @@ +1444,5 @@ > oldIcc.iccid != message.iccid || > oldIcc.mcc != message.mcc || > oldIcc.mnc != message.mnc || > oldIcc.spn != message.spn || > + oldIcc.plmn_list != message.plmn_list || Indentation ::: dom/system/gonk/ril_consts.js @@ +559,5 @@ > this.COMPREHENSIONTLV_TAG_IMMEDIATE_RESPONSE = 0x2b; > this.COMPREHENSIONTLV_TAG_URL = 0x31; > > +// Tags for Service Provider Display Information TLV > +const SPDI_TAG_SPDI = 0xa3; After bug 798491, please use `this.xxxx` in any jsm files, inclusive of this one. @@ +792,5 @@ > FDN: 2, > SDN: 4, > BDN: 6, > + DATA_DOWNLOAD_SMS_PP: 28, > + SPN: 19, Please sort them in order ::: dom/system/gonk/ril_worker.js @@ +1189,5 @@ > /** > + * This will compute the spnDisplay field of the network. > + * See TS 22.101 Annex A and TS 51.011 10.3.11 for details. > + */ > + _updateDisplayedCarrier: function _updateDisplayedCarrier() { I'd like to name it 'updateICCDisplayName'. @@ +1413,1 @@ > this._handleICCInfoChange(); We should only fetch SPN/PLMN/SPDI for once and report "correct" icc info for once. Maybe add some guarding code to wait untill SPN/PLMN/SPDI are either all fetched or unsupported. For example: getSPN: function getSPN() { this.iccIO({ ... callback: function oncallback() { this.iccInfo.SPN = xxx; // Read SPN from parcel. this._updateDisplayedCarrier(); }, onerror: function onerror() { this.iccInfo.SPN = null; this._updateDisplayedCarrier(); } }); }, _processOperator: function _processOperator() { // Original stuff if (...) { this.updateDisplayedCarrier(); this._sendNetworkInfoMessage(NETWORK_INFO_OPERATOR, this.operator); } }, _updateDisplayedCarrier: function _updateDisplayedCarrier() { if (!("SPN" in this.iccInfo) || !("PLMN" in this.iccInfo)) { // Either SPN or PLMN/SPDI is not processed yet, wait a minute. return; } // Do something to correct displayName. this._handleICCInfoChange(); }, @@ +1414,5 @@ > } > > + function error() { > + debug("SPN: error"); > + } I don't think you really need this. It does nothing but print a simple error message that says nothing. @@ +1453,5 @@ > + } > + > + function error() { > + debug("PLMN: error"); > + } Ditto. @@ +1455,5 @@ > + function error() { > + debug("PLMN: error"); > + } > + > + if (!this.appType == CARD_APPTYPE_SIM) { Actually, operator '!' has higher precedence than '=='. This makes the line read as: if ((!this.appType) == CARD_APPTYPE_SIM) ... Besides, this checking should also be handled by isICCServiceAvailable(). @@ +1496,5 @@ > + debug("SPDI: tag=" + tag); > + } > + > + // Skip SPDI tag > + if (tag == SPDI_TAG_SPDI) { switch (tag) ? @@ +1520,5 @@ > + > + function error() { > + if (DEBUG) { > + debug("SPDI: error"); > + } Ditto @@ +1523,5 @@ > + debug("SPDI: error"); > + } > + } > + > + if (!this.appType == CARD_APPTYPE_USIM) { Ditto @@ +1891,5 @@ > + try { > + let plmn = [GsmPDUHelper.readHexOctet(), > + GsmPDUHelper.readHexOctet(), > + GsmPDUHelper.readHexOctet()]; > + debug("readPLMNEntries: Reading PLMN entry: '" + plmn + "'"); wrap it by 'if (DEBUG)'

Waits for SPN, PLMN and SPDI before ril_worker sends displayName to RadioInterfaceLayer.js.

Comment on attachment 677714 [details] [diff] [review] Patch 2: Getting SPDI, SPN, PLMN file Review of attachment 677714 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/system/gonk/ril_worker.js @@ +1436,5 @@ > + * Read the PLMNsel (Public Land Mobile Network) from the ICC. > + * > + * See ETSI TS 100.977 section 10.3.4 EF_PLMNsel > + */ > + getPLMN: function getPLMN() { Please see my comment in comment 12, please rename this function to a better name. There are many ICC entries related to PLMN.

I just tried these two patches, because I am on a MVNO, and I get "No Network" before I got "Rogers Wireless" (which is the actual carrier). I expected to get "Speak Out Wireless" or something like that. Do you need help to debug that?

(In reply to Hub Figuiere [:hub] from comment #36) > I just tried these two patches, because I am on a MVNO, and I get "No > Network" before I got "Rogers Wireless" (which is the actual carrier). I > expected to get "Speak Out Wireless" or something like that. > > Do you need help to debug that? Thank you for the information. This patch uses a new property to expose displayname. So we will need a corresponding change in Gaia to read the display name.

> Thank you for the information. This patch uses a new property to expose > displayname. So we will need a corresponding change in Gaia to read the > display name. This would explain. Thanks.

spn and plmn is removed from RadioInterfaceLayer.js now.

After searched the usage of long name/short name in Android, Vicamo and I found that long name and short name might not necessary to be exposed to app. So we removed longname and shortname from the IDL. We provide a 'name' field to send the carrier name to Gaia.

Fix logic of generating name.

Comment on attachment 679404 [details] [diff] [review] Patch: Part 1 - IDL Change Review of attachment 679404 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/network/interfaces/nsIDOMMobileConnection.idl @@ -371,5 @@ > - > - /** > - * Long name of the network operator > - */ > - readonly attribute DOMString longName; should also remove nsIDOMMozMobileICCInfo.spn

Comment on attachment 680200 [details] [diff] [review] Patch: Part 2 - compose carrier name using spn/longname in ril_worker Review of attachment 680200 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/system/gonk/RILContentHelper.js @@ -163,5 @@ > > // nsIDOMMozMobileNetworkInfo > > - shortName: null, > - longName: null, Should also remove MobileICCInfo.spn ::: dom/system/gonk/ril_worker.js @@ +1178,5 @@ > this.getAD(); > this.getSPN(); > this.getSST(); > + this.getPLMNSelector(); > + this.getSPDI(); Move into: function getSST() { function callback() { // Here! } } @@ +1238,5 @@ > + // Test to see if operator's mcc/mnc match mcc/mnc of PLMN. > + if (!isOnMatchingPlmn) { > + for (let plmn in iccPlmn) { > + let plmnMcc = parseInt(iccPlmn[plmn].substr(0, 3)); > + let plmnMnc = parseInt(iccPlmn[plmn].substr(3)); Move parseInt() calls into readPLMNEntries() so that we only calculate once each. @@ +1249,5 @@ > + > + // Now determine whether we display PLMN or SPN. > + let rule = 0; > + let spnDisplay = ""; > + if (!iccSpn || !iccSpn.spn || iccSpn.spnDisplayCondition == -1) { "if (!iccSpn)" is enough @@ +1474,5 @@ > + this.iccInfoPrivate.PLMN = null; > + this.updateICCDisplayName(); > + } > + > + if (!this.isICCServiceAvailable("PLMNSEL")) { Move this checking into getSST() callback function. @@ +1540,5 @@ > + this.iccInfo.PLMN = null; > + this.updateICCDisplayName(); > + } > + > + if (!this.isICCServiceAvailable("SPDI")) { Ditto @@ +1928,5 @@ > + plmnList.push(plmnStr); > + } > + } catch (e) { > + if (DEBUG) debug("readPLMNEntries: PLMN entry " + index + " is invalid."); > + break; indentation

Comment on attachment 679286 [details] [diff] [review] Patch: Part 3 - Move roaming checking into ril_worker. Review of attachment 679286 [details] [diff] [review]: ----------------------------------------------------------------- Still have something to fix but almost there :) ::: dom/system/gonk/ril_worker.js @@ +3536,5 @@ > + let icc = this.iccInfo; > + let operatorPriv = this.operatorPrivate; > + let operator = this.operator; > + if (!icc || !iccPriv || !iccPriv.SPN || !registration || > + !registration.connected || !operatorPriv || !operator) { I've opened bug 810510 for these dependency problems, but let's just move on if we can fix this issue first. @@ +3537,5 @@ > + let operatorPriv = this.operatorPrivate; > + let operator = this.operator; > + if (!icc || !iccPriv || !iccPriv.SPN || !registration || > + !registration.connected || !operatorPriv || !operator) { > + return false; We can't just return here, because it might results in incorrect roaming attribute being sent in _sendPendingNetworkInfo().

Comment on attachment 680199 [details] [diff] [review] Patch: Part 4 - test case Review of attachment 680199 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/system/gonk/tests/test_ril_worker_spn.js @@ +33,5 @@ > + > + RIL.iccInfo = { > + mnc: "123", > + mcc: "45", > + }; Please extract all the duplicated code into a shared utility function. I think this script can be squeezed into 100 lines or so.

Fix error in reading PLMN entries.

Comment on attachment 681405 [details] [diff] [review] Patch: Part 2 - checking roaming and composing carrier name using spn/longname in ril_worker Review of attachment 681405 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/system/gonk/ril_worker.js @@ +1194,5 @@ > + */ > + updateICCDisplayName: function updateICCDisplayName() { > + // This function is executed only when operator info, SPN and PLMN are > + // all ready. > + if (!this.iccInfoPrivate.hasOwnProperty("SPN") || please do a let iccInfoPrivate = this.iccInfoPrivate, that saves a ton of typing and also makes for more compact bytecode and compiled code @@ +1195,5 @@ > + updateICCDisplayName: function updateICCDisplayName() { > + // This function is executed only when operator info, SPN and PLMN are > + // all ready. > + if (!this.iccInfoPrivate.hasOwnProperty("SPN") || > + !this.iccInfoPrivate.hasOwnProperty("PLMN") || no need for hasOwnProperty here, just !iccInfoPrivate.PLMN @@ +1200,5 @@ > + !this.operatorPrivate) { > + if (DEBUG) { > + debug("updateICCDisplayName: one or more of SPN/PLMN/operator haven't ready"); > + debug(" this.iccInfoPrivate = " + this.iccInfoPrivate); > + if (this.iccInfoPrivate) { if !this.iccInfoPrivate we would have thrown an exception above where you do this.iccInfoPrivate.hasOwnProperty, please double check the logic here @@ +3562,5 @@ > } > }, > > /** > + * Fix the roaming. RIL can report roaming in some case it is not "Fix roaming status." @@ +3576,5 @@ > + let icc = this.iccInfo; > + let operatorPriv = this.operatorPrivate; > + let operator = this.operator; > + if (!iccPriv.hasOwnProperty('SPN') || !registration.connected) { > + // FIXME: If checkRoamingFromOperator is called before SPN is ready, we File a bug for this (always file a bug for FIXMEs and reference the bug)

Impressive patch and nice test. Thanks!

I applied the patches. Now I get "Emergency calls only".

(In reply to Hub Figuiere [:hub] from comment #57) > I applied the patches. Now I get "Emergency calls only". I have a corresponding Gaia change, but I haven't send a pull request to Gaia yet. Would you test with this patch and Gaia from https://github.com/kk1fff/gaia/tree/work793111 (or work793111 branch in git://github.com/kk1fff/gaia.git)? I only changed the lock screen in this Gaia patch, so the carrier name other than lock screen might be wrong.

(In reply to Patrick Wang [:kk1fff] from comment #58) > I have a corresponding Gaia change, but I haven't send a pull request to > Gaia yet. Would you test with this patch and Gaia from > https://github.com/kk1fff/gaia/tree/work793111 (or work793111 branch in > git://github.com/kk1fff/gaia.git)? I only changed the lock screen in this > Gaia patch, so the carrier name other than lock screen might be wrong. Applying that patch, the lock screen says "No Network"

(In reply to Hub Figuiere [:hub] from comment #59) > Applying that patch, the lock screen says "No Network" I didn't meet this error, and I guess it's because we are in different network environments. If you don't mind, would you try again with this.DEBUG_ALL = true; in ril_const.js and get the adb log for me? It would be helpful. Thanks.

The adb logcat output after enabling full debug as requested in comment 60

Looks like your PLMN does not get read. This reminds me some spurious behavior I experienced while doing first hackish version of this.

Thanks Andreas for comment, and thanks Hub for log. I updated the patch according comment 55 and Hub's log. It seems that previous patch didn't handle SPDI correctly.

It works ! Thanks !

I have the patch for gaia that change it wherever it is need. Do you want me to attach it?

(In reply to Hub Figuiere [:hub] from comment #65) > I have the patch for gaia that change it wherever it is need. Do you want me > to attach it? Would you file another bug for Gaia side change or just send a pull request to Gaia when this bug is ready? I plan to request review for these patches in days.

(In reply to Patrick Wang [:kk1fff] from comment #66) > Would you file another bug for Gaia side change or just send a pull request > to Gaia when this bug is ready? I plan to request review for these patches > in days. I mean, I am glad that you've finished Gaia side change. Will you attach your patch to this bug or open a new bug?

Patch for Gaia.

Patch 2 no longer applies. :-(

(In reply to Hub Figuiere [:hub] from comment #69) > Patch 2 no longer applies. :-( I have a rebased version, but it is in my office computer. I'll submit it when I can use my office pc.

Rebased.

Change 'longname' variable name to 'plmnName' in updateICCDisplayName(). Hi Vicamo, would you help to review this? Thanks.

Rebased

Comment on attachment 681404 [details] [diff] [review] Patch 1: IDL Change Hi Jonas, this is the change in interface. Would you help to review this? Thanks

Comment on attachment 681404 [details] [diff] [review] Patch 1: IDL Change After discussed with Vicamo, we will have some modification on interface.

FWIW, the interface changes look great to me,assuming we can get away with the smaller number of properties.

We tried to make the interface simpler. However, after studied TS 31.102 spec and Android's implementation again, we found that Android allows apps to compose carrier name in its format. For example, carrier name on lockscreen is different from one in settings. As spec only defines whether SPN or PLMN Name is required to be a part of displayed carrier name, we might have to tell Gaia PLMN Name, SPN and required part, so Gaia app can compose the carrier name in its own format. With this interface, Gaia app that wants to display carrier name needs to listen to 'iccinfochange' event as well as 'voicechange' event. When display condition changes, 'iccinfochange' event will be issued, and listening to 'voicechange' event is for watching longname/shortname change. When 'iccinfochange' or 'voicechange' is issued, the app should fetch the display condition (the 'isDisplay*Required' properties) and spn from iccInfo object, and longname/shortname from voice object, and use them to compose a new carrier name string. When the phone just turned on, if SPN file is ready before we received the PLMN name from network, and display condition says that PLMN name is required, the app should check if PLMN name is null, or the displayed carrier string might be wrong. Hi Jonas, This interface may be complex then previous one, but provides more flexibility. Would you give us some feedback about this interface? Thanks

This is fine with me. I really don't have strong opinions on this stuff given that most/all of it seems to be driven by regulations.

For testing, the corresponding Gaia change in lock screen is at https://github.com/kk1fff/gaia/tree/work793111.

Comment on attachment 684645 [details] [diff] [review] Patch: Part 2 - Get PLMN list and decide required part of carrier name with SPN display condition. Review of attachment 684645 [details] [diff] [review]: ----------------------------------------------------------------- \^O^/ ::: dom/system/gonk/ril_worker.js @@ +1447,5 @@ > } > > + function onerror() { > + if (DEBUG) debug("SPN: error"); > + } If this function is only for printing such debug message, maybe we don't need it at all. @@ +1486,5 @@ > + } > + > + function onerror() { > + if (DEBUG) debug("PLMN: [PLMN Selector] error"); > + } Ditto @@ +1554,5 @@ > + } > + > + function onerror() { > + if (DEBUG) debug("PLMN: [SPDI] Error"); > + } Ditto

Hi Jonas, Would you help to review this IDL change? Thanks

Fixed according to comment 80. Requesting review again because I found the display condition was wrong when I was writing the test case. This patch correct the result display condition: when PLMN is HPLMN or in PLMN list, network name is required when the first bit is 1; when PLMN isn't HPLMN nor in PLMN list, spn is required when the second bit is 0. Vicamo, would you help to review this? Thanks.

Hi Vicamo, This is the patch in qemu that changes display condition in sim. Would you help to review this? Thanks.

Comment on attachment 683079 [details] [diff] [review] Bug 793111 - MVNO support in Gaia. we use the new property 'name' instead of 'shortName'. Thank Hub for the patch. As we changed the IDL again, I will file another bug in Gaia to modify displayed carrier name. Mark this obsolete.

Comment on attachment 685060 [details] [diff] [review] Patch: Part 3 - test case Review of attachment 685060 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/network/tests/marionette/test_mobile_iccinfo.js @@ +51,5 @@ > is(connection.iccInfo.mcc, 310); > is(connection.iccInfo.mnc, 260); > is(connection.iccInfo.spn, "Android"); > +is(connection.iccInfo.isDisplayNetworkNameRequired, true); > +is(connection.iccInfo.isDisplaySpnRequired, false); Please try to extract SPN related test code into some test_case() function. You can keep these lines here, but I think we'll need: function repeat(func, caseArray, oncomplete) { (function do_call(index) { let next = index < (caseArray.length - 1) ? do_call.bind(null, index + 1) : oncomplete; caseArray[index].push(next); func.apply(null, caseArray); })(0); } function test_SPN(mcc, mnc, expectedIsDisplayNetworkNameRequired, expectedIsDisplaySpnRequired, callback) { waitForIccInfoChange(function() { is(connection.iccInfo.isDisplayNetworkNameRequired, expectedIsDisplayNetworkNameRequired); is(connection.iccInfo.isDisplaySpnRequired, expectedIsDisplaySpnRequired); window.setTimeout(callback, 0); } setEmulatorMccMnc(mcc, mnc); } repeat(test_SPN, [ [123, 456, false, true], [xxx, yyy, zzz, ttt], ], finalize); @@ +63,5 @@ > + is(connection.iccInfo.isDisplaySpnRequired, true); > + setEmulatorMccMnc(310, 260, finalize); > +}); > +setEmulatorMccMnc(123, 456); > +console.log("12345"); I know you don't really mean it. ::: dom/system/gonk/ril_consts.js @@ +13,5 @@ > * limitations under the License. > */ > > // Set to true to debug all RIL layers > +this.DEBUG_ALL = true; Ditto ::: dom/system/gonk/tests/test_ril_worker_icc.js @@ +483,5 @@ > + do_check_eq(RIL.updateDisplayCondition(), true); > + do_check_eq(RIL.iccInfo.isDisplayNetworkNameRequired, true); > + do_check_eq(RIL.iccInfo.isDisplaySpnRequired, false); > + > + // Test current PLMN is HPLMN, and the first bit of display condition is 1 Please try to extract common code into some test_case() function, so that we can add every possible combinations of input data as test cases.

(In reply to Patrick Wang [:kk1fff] from comment #85) > Comment on attachment 683079 [details] [diff] [review] > Bug 793111 - MVNO support in Gaia. we use the new property 'name' instead of > 'shortName'. > > Thank Hub for the patch. As we changed the IDL again, I will file another > bug in Gaia to modify displayed carrier name. Mark this obsolete. How is it supposed to be done (which property do we use)? What is the bug # for Gaia?

(In reply to Hub Figuiere [:hub] from comment #87) > How is it supposed to be done (which property do we use)? What is the bug # > for Gaia? The longName/shortName will still be available (say, we don't need to land gecko side and gaia side at the same time). They stand for PLMN name. The displayed carrier name will be decided by PLMN name, SPN and display conditions. The display conditions change when PLMN changes. App which wants to show carrier name needs to listen to iccinfochange event for display condition change. The display conditions tell the app whether PLMN name or SPN should be a part of displayed name, but the app is allowed to concatenate the string in its own format. I have a gaia side change for lock screen, the patch is at https://github.com/kk1fff/gaia/commit/c06cfae5625af37a8c732f54f8ef0a376ef2c10a. But I haven't filed the bug.

Fixed according to comment 86. The corresponding change in qemu is also submitted. Vicamo, would you help to review this? Thanks.

Comment on attachment 685062 [details] Patch in qemu. Excellent!

Sorry that I forgot to change the xpcshell tests.

Comment on attachment 685541 [details] [diff] [review] Patch: Part 3 - test case Review of attachment 685541 [details] [diff] [review]: ----------------------------------------------------------------- Stellar work!

Update patch message: adding super reviewer

https://hg.mozilla.org/integration/mozilla-inbound/rev/8cdd43b76d19 https://hg.mozilla.org/integration/mozilla-inbound/rev/131bc42cd637 https://hg.mozilla.org/integration/mozilla-inbound/rev/66f3565f4817

Gaia part is bug 815595

Backed out for causing marionette failures: https://tbpl.mozilla.org/php/getParsedLog.php?id=17372827&tree=Mozilla-Inbound https://hg.mozilla.org/integration/mozilla-inbound/rev/0d4143b11eea

Thank Jonathan. Got green in marionette now. Try: https://tbpl.mozilla.org/?tree=Try&rev=d02503d54d32

Re-push after bug 816161 being resolved https://hg.mozilla.org/integration/mozilla-inbound/rev/71d28da5c50e https://hg.mozilla.org/integration/mozilla-inbound/rev/afd9eb3d5e5a https://hg.mozilla.org/integration/mozilla-inbound/rev/985e380010e3

https://hg.mozilla.org/mozilla-central/rev/71d28da5c50e https://hg.mozilla.org/mozilla-central/rev/afd9eb3d5e5a https://hg.mozilla.org/mozilla-central/rev/985e380010e3

https://hg.mozilla.org/releases/mozilla-aurora/rev/87264fc62bfe https://hg.mozilla.org/releases/mozilla-aurora/rev/9223e0870700 https://hg.mozilla.org/releases/mozilla-aurora/rev/7a54aed85f4a https://hg.mozilla.org/releases/mozilla-beta/rev/af45db3a957e https://hg.mozilla.org/releases/mozilla-beta/rev/f16c7ec1529c https://hg.mozilla.org/releases/mozilla-beta/rev/af28fcc04174