793111 - Mobile Virtual Network Operator Service Provider Name display

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

[:gerard-majax]

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).

Comment 1

[:gerard-majax]

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.

Comment 2

[:gerard-majax]

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.

Comment 3

[:gerard-majax]

Attachment: First proposal, request for comments

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.

Comment 4

[:gerard-majax]

Attachment: Second proposal, request for comments

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

Comment 5

[:gerard-majax]

Attachment: Patch proposal, request for comments. v3

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.

Comment 6

[:gerard-majax]

Attachment: Patch proposal, request for comments. v4

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".

Comment 7

[:gerard-majax]

Attachment: Patch proposal, request for comments. v5

Some cosmetic changes.

Comment 8

[:gerard-majax]

Attachment: Patch proposal, request for comments. v6

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

Comment 9

[:gerard-majax]

Attachment: Patch proposal, request for comments. v7

Comment 10

[:gerard-majax]

Comment on attachment 663717 [details] [diff] [review]
Patch proposal, request for comments. v7

Moving the updateSpnDisplay() to ril_worker, as suggested on IRC.

Comment 11

[:gerard-majax]

Attachment: Patch proposal, request for comments. v8

Updated to current master.

Comment 12

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

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?

Comment 13

[:gerard-majax]

(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 ...

Comment 14

[:gerard-majax]

Attachment: Patch proposal, request for comments. v8-2

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

Comment 15

[Philipp von Weitershausen [:philikon]]

(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?

Comment 16

[:gerard-majax]

(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.

Comment 17

[Philipp von Weitershausen [:philikon]]

(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 18

[Philipp von Weitershausen [:philikon]]

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.

Comment 19

[:gerard-majax]

(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 ?

Comment 20

[:gerard-majax]

Attachment: Patch proposal, request for comments. v9

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

Comment 21

[Philipp von Weitershausen [:philikon]]

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.

Comment 22

[:gerard-majax]

(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 ...

Comment 23

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

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.

Comment 24

[:gerard-majax]

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.

Comment 25

[Vicamo Yang [:vicamo][:vyang]]

(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.

Comment 26

[:gerard-majax]

(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.

Comment 27

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Attachment: Patch proposal v10

Rebase previous patch on current inbound.

Comment 28

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

(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.

Comment 29

[:gerard-majax]

(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 30

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Attachment: Patch 1: IDL Change

Comment 31

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Attachment: Patch 2: Getting SPDI, SPN, PLMN file

Comment 32

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

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 33

[Vicamo Yang [:vicamo][:vyang]]

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)'

Comment 34

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Attachment: Patch 2: Getting SPDI, SPN, PLMN file

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

Comment 35

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

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.

Comment 36

[Hubert Figuiere [:hub]]

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?

Comment 37

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

(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.

Comment 38

[Hubert Figuiere [:hub]]

> 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.

Comment 39

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Attachment: Patch 2: Getting SPDI, SPN, PLMN file

Comment 40

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Attachment: Patch 3: Move roaming checking into ril_worker.

Comment 41

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Attachment: Patch: Part 3 - Move roaming checking into ril_worker.

spn and plmn is removed from RadioInterfaceLayer.js now.

Comment 42

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Attachment: Patch: Part 1 - IDL Change

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.

Comment 43

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Attachment: Patch: Part 2 - compose carrier name using spn/longname/shortname in ril_worker

Comment 44

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Attachment: Patch: Part 2 - compose carrier name using spn/longname in ril_worker

Fix logic of generating name.

Comment 45

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Attachment: Patch: Part 4 - test case

Comment 46

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Attachment: Patch: Part 2 - compose carrier name using spn/longname in ril_worker

Comment 47

[Vicamo Yang [:vicamo][:vyang]]

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 48

[Vicamo Yang [:vicamo][:vyang]]

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 49

[Vicamo Yang [:vicamo][:vyang]]

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 50

[Vicamo Yang [:vicamo][:vyang]]

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.

Comment 51

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Attachment: Patch 1: IDL Change

Comment 52

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Attachment: Patch: Part 2 - checking roaming and composing carrier name using spn/longname in ril_worker

Comment 53

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Attachment: Patch: Part 3 - test case

Comment 54

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Attachment: Patch: Part 2 - checking roaming and composing carrier name using spn/longname in ril_worker

Fix error in reading PLMN entries.

Comment 55

[Andreas Gal :gal]

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)

Comment 56

[Andreas Gal :gal]

Impressive patch and nice test. Thanks!

Comment 57

[Hubert Figuiere [:hub]]

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

Comment 58

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

(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.

Comment 59

[Hubert Figuiere [:hub]]

(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"

Comment 60

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

(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.

Comment 61

[Hubert Figuiere [:hub]]

Attachment: adb logcat output

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

Comment 62

[:gerard-majax]

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

Comment 63

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Attachment: Patch: Part 2 - checking roaming and composing carrier name using spn/longname in ril_worker

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.

Comment 64

[Hubert Figuiere [:hub]]

It works ! Thanks !

Comment 65

[Hubert Figuiere [:hub]]

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

Comment 66

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

(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.

Comment 67

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

(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?

Comment 68

[Hubert Figuiere [:hub]]

Attachment: Bug 793111 - MVNO support in Gaia. we use the new property 'name' instead of 'shortName'.

Patch for Gaia.

Comment 69

[Hubert Figuiere [:hub]]

Patch 2 no longer applies. :-(

Comment 70

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

(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.

Comment 71

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Attachment: Patch: Part 2 - checking roaming and composing carrier name using spn/longname in ril_worker

Rebased.

Comment 72

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Attachment: Patch: Part 2 - checking roaming and composing carrier name using spn/longname in ril_worker

Change 'longname' variable name to 'plmnName' in updateICCDisplayName().

Hi Vicamo, would you help to review this? Thanks.

Comment 73

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Attachment: Patch: Part 3 - test case

Rebased

Comment 74

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

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 75

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Comment on attachment 681404 [details] [diff] [review]
Patch 1: IDL Change

After discussed with Vicamo, we will have some modification on interface.

Comment 76

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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

Comment 77

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Attachment: Patch: Part 1: IDL Change

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

Comment 78

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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.

Comment 79

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Attachment: Patch: Part 2 - Get PLMN list and decide required part of carrier name with SPN display condition.

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

Comment 80

[Vicamo Yang [:vicamo][:vyang]]

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

Comment 81

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Attachment: Patch: Part 1: IDL Change

Hi Jonas,

Would you help to review this IDL change?

Thanks

Comment 82

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Attachment: Patch: Part 2 - checking roaming and composing carrier name using spn/longname in ril_worker

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.

Comment 83

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Attachment: Patch: Part 3 - test case

Comment 84

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Attachment: Patch in qemu.

Hi Vicamo,

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

Comment 85

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

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 86

[Vicamo Yang [:vicamo][:vyang]]

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.

Comment 87

[Hubert Figuiere [:hub]]

(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?

Comment 88

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

(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.

Comment 89

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Attachment: Patch: Part 3 - test case

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

Comment 90

[Vicamo Yang [:vicamo][:vyang]]

Comment on attachment 685062 [details]
Patch in qemu.

Excellent!

Comment 91

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Attachment: Patch: Part 3 - test case

Sorry that I forgot to change the xpcshell tests.

Comment 92

[Vicamo Yang [:vicamo][:vyang]]

Comment on attachment 685541 [details] [diff] [review]
Patch: Part 3 - test case

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

Stellar work!

Comment 93

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Attachment: Patch 1: IDL Change

Update patch message: adding super reviewer

Comment 94

[Vicamo Yang [:vicamo][:vyang]]

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

Comment 95

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Gaia part is bug 815595

Comment 96

[Ed Morley [:emorley]]

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

Comment 97

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

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

Comment 98

[Vicamo Yang [:vicamo][:vyang]]

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

Comment 99

[Ryan VanderMeulen [:RyanVM]]

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

Comment 100

[Ryan VanderMeulen [:RyanVM]]

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