B2G CDMA display carrier name according to SPN

RESOLVED FIXED in mozilla22

Status

()

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

People

(Reporter: kk1fff, Assigned: kk1fff)

Tracking

(Blocks: 1 bug)

unspecified
mozilla22
ARM
Gonk (Firefox OS)
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(5 attachments, 9 obsolete attachments)

4.46 KB, patch
vicamo
: review+
Details | Diff | Splinter Review
2.79 KB, patch
vicamo
: review+
Details | Diff | Splinter Review
8.27 KB, patch
vicamo
: review+
Details | Diff | Splinter Review
2.48 KB, patch
allstars
: review+
Details | Diff | Splinter Review
3.67 KB, patch
allstars
: review+
Details | Diff | Splinter Review
We should read CDMA SPN to display carrier name according to display condition.
Created attachment 722140 [details] [diff] [review]
Patch: Part 1: make CdmaPDUHelper.decodeUserDataMsg() reusable.
Created attachment 722141 [details] [diff] [review]
Patch: Part 2 - Reading SPN file
Created attachment 722142 [details] [diff] [review]
Patch: Part 3 - update display condition for CDMA
Created attachment 722143 [details] [diff] [review]
Patch: Part 4 - test case for reading spn file.
Created attachment 722144 [details] [diff] [review]
Patch: Part 5 - test case for updating display condition for cdma.
Created attachment 722152 [details] [diff] [review]
Patch: Part 3 - update display condition for CDMA
Attachment #722142 - Attachment is obsolete: true
Created attachment 722157 [details] [diff] [review]
Patch: Part 3 - update display condition for CDMA
Attachment #722152 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #722140 - Flags: review?(vyang)
(Assignee)

Updated

5 years ago
Attachment #722141 - Flags: review?(allstars.chh)
(Assignee)

Updated

5 years ago
Attachment #722157 - Flags: review?(vyang)
Created attachment 722605 [details] [diff] [review]
Patch: Part 4 - test case for reading spn file.
Attachment #722143 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #722605 - Flags: review?(allstars.chh)
Created attachment 722606 [details] [diff] [review]
Patch: Part 5 - test case for updating display condition for cdma.
Attachment #722144 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #722606 - Flags: review?(vyang)
Attachment #722140 - Flags: review?(vyang) → review+
Comment on attachment 722157 [details] [diff] [review]
Patch: Part 3 - update display condition for CDMA

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

r=me with nits fixed.

::: dom/system/gonk/ril_worker.js
@@ +10948,5 @@
> +            }
> +            if (homeSid == sid) {
> +              if (homeNid == 0) {
> +                // Reserved network id.
> +                continue;

if (homeSid == 0 || homeNid == 0 || // Reserved system/network id.
    homeSid != sid) {
  continue;
}

@@ +10950,5 @@
> +              if (homeNid == 0) {
> +                // Reserved network id.
> +                continue;
> +              }
> +              // According to 3GPP2 C.S0005 Sec. 2.6.5.2, NID number 0 means all

s/0/65535/
Attachment #722157 - Flags: review?(vyang) → review+
Comment on attachment 722157 [details] [diff] [review]
Patch: Part 3 - update display condition for CDMA

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

r=me with nits fixed.

::: dom/system/gonk/ril_worker.js
@@ +10916,5 @@
> +      let cdmaHome = RIL.cdmaHome;
> +      let sid = RIL.cdmaSubscription.systemId;
> +      let nid = RIL.cdmaSubscription.networkId;
> +
> +      iccInfo.isDisplayNetworkNameRequired = false;

I'm not sure whether this is correct. Per offline discuss, maybe spend some more time digging specifications for a more solid answer. If still in vain, just let it be and open a follow-up if necessary.
Comment on attachment 722606 [details] [diff] [review]
Patch: Part 5 - test case for updating display condition for cdma.

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

Excellent test cases!
Attachment #722606 - Flags: review?(vyang) → review+
Comment on attachment 722141 [details] [diff] [review]
Patch: Part 2 - Reading SPN file

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

::: dom/system/gonk/ril_worker.js
@@ +11517,5 @@
> +
> +      if (ICCUtilsHelper.isICCServiceAvailable("SPN")) {
> +        if (DEBUG) debug("SPN: SPN is available");
> +        this.readSPN();
> +      } else {

Remove else

@@ +11527,5 @@
> +  },
> +
> +  readSPN: function readSPN() {
> +    function callback() {
> +      let length = Buf.readUint32();

s/length/strLen/

@@ +11531,5 @@
> +      let length = Buf.readUint32();
> +      let displayCondition = GsmPDUHelper.readHexOctet();
> +      let codingScheme = GsmPDUHelper.readHexOctet();
> +      let lang = GsmPDUHelper.readHexOctet();
> +      let msgLength;

msgLen

@@ +11534,5 @@
> +      let lang = GsmPDUHelper.readHexOctet();
> +      let msgLength;
> +
> +      // SPN String ends up with 0xff.
> +      let spnOctetLength = 0;

s/spnOctetLength/octetLen/

@@ +11536,5 @@
> +
> +      // SPN String ends up with 0xff.
> +      let spnOctetLength = 0;
> +      let userDataBuffer = [];
> +      for (; spnOctetLength < (length - 3); spnOctetLength++) {

length should be string length,
I guess you're trying to use octets length.

@@ +11560,5 @@
> +
> +      if (DEBUG) {
> +        debug("CDMA SPN: " + spn +
> +              " Message Length: " + msgLength +
> +              " SPN String octet length: " + spnOctetLength);

Why do you need to print some many information?

@@ +11563,5 @@
> +              " Message Length: " + msgLength +
> +              " SPN String octet length: " + spnOctetLength);
> +      }
> +      RIL.iccInfoPrivate.SPN = {
> +        spn : spn,

Why do we have two copies of spn in iccInfoPrivate and iccInfo?
Attachment #722141 - Flags: review?(allstars.chh)
Comment on attachment 722140 [details] [diff] [review]
Patch: Part 1: make CdmaPDUHelper.decodeUserDataMsg() reusable.

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

::: dom/system/gonk/ril_worker.js
@@ +8173,5 @@
> +    }
> +    return null;
> +  },
> +
> +  decodeCdmaPduMsg: function decodeCdmaPduMsg(encoding, msgType, msgBodySize) {

Can we use 'PDU' here to make it consistent with GsmPDUHelper?
Comment on attachment 722605 [details] [diff] [review]
Patch: Part 4 - test case for reading spn file.

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

::: dom/system/gonk/tests/test_ril_worker_ruim.js
@@ +114,5 @@
>    run_next_test();
>  });
> +
> +/**
> + * Verify reading CDMA spn

nit, s/spn/EF_SPN/
Attachment #722605 - Flags: review?(allstars.chh) → review+
(In reply to Vicamo Yang [:vicamo][:vyang] from comment #11)
> I'm not sure whether this is correct. Per offline discuss, maybe spend some
> more time digging specifications for a more solid answer. If still in vain,
> just let it be and open a follow-up if necessary.

After digging 3gpp2 spec and AOSP implementation, value like GSM's network name is not always available in CDMA, and display condition for network name is not defined in spec of RUIM EF_SPN. Let's keep it false.
Created attachment 722720 [details] [diff] [review]
Patch: Part 2 - Reading SPN file
Attachment #722141 - Attachment is obsolete: true
Attachment #722720 - Flags: review?(allstars.chh)
Created attachment 722725 [details] [diff] [review]
Patch: Part 1 - make CdmaPDUHelper.decodeUserDataMsg() reusable.

There's no logical change, just changing function name to decodeCdmaPDUMsg to make "PDU" term consistent. Requesting review again for the new function name.
Attachment #722140 - Attachment is obsolete: true
Attachment #722725 - Flags: review?(vyang)
Attachment #722725 - Flags: review?(vyang) → review+
Comment on attachment 722720 [details] [diff] [review]
Patch: Part 2 - Reading SPN file

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

::: dom/system/gonk/ril_worker.js
@@ +11551,5 @@
> +      let lang = GsmPDUHelper.readHexOctet();
> +      let msgLen;
> +
> +      // SPN String ends up with 0xff.
> +      let octetLen = 0;

We use octetLen for total octets in this parcel,
so octetLen = strLen / 2;

We also use another var called 'readLen',
to show the number of octet read.

@@ +11553,5 @@
> +
> +      // SPN String ends up with 0xff.
> +      let octetLen = 0;
> +      let userDataBuffer = [];
> +      for (; octetLen < (strLen / PDU_HEX_OCTET_SIZE - 3); octetLen++) {

PDU_HEX_OCTET_SIZE is the size of 'octet',
strLen is the length of string,
two are unrelated.

@@ +11578,5 @@
> +      if (DEBUG) {
> +        debug("CDMA SPN: " + spn + ", Display condition: " + displayCondition);
> +      }
> +      RIL.iccInfoPrivate.SPN = {
> +        spnDisplayCondition: displayCondition

In GSM,
I saw you also add spn into iccInfoPrivate,
please file another bug to fix it.
Attachment #722720 - Flags: review?(allstars.chh)
Created attachment 723339 [details] [diff] [review]
Patch: Part 2 - Reading SPN file
Attachment #722720 - Attachment is obsolete: true
Attachment #723339 - Flags: review?(allstars.chh)
Created attachment 723340 [details] [diff] [review]
Patch: Part 4 - test case for reading spn file.

Adding a test to test when there is not tailing 0xff in SPN file.
Attachment #722605 - Attachment is obsolete: true
Attachment #723340 - Flags: review?(allstars.chh)
(Assignee)

Updated

5 years ago
Attachment #723339 - Flags: review?(allstars.chh) → review?(allstars.chh)
(In reply to Yoshi Huang[:allstars.chh][:yoshi] from comment #19)
> In GSM,
> I saw you also add spn into iccInfoPrivate,
> please file another bug to fix it.

Bug 849725 is filed for removing the property.
Comment on attachment 723339 [details] [diff] [review]
Patch: Part 2 - Reading SPN file

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

::: dom/system/gonk/ril_worker.js
@@ +11548,5 @@
> +      let strLen = Buf.readUint32();
> +      let octetLen = strLen / 2;
> +      let displayCondition = GsmPDUHelper.readHexOctet();
> +      let codingScheme = GsmPDUHelper.readHexOctet();
> +      let lang = GsmPDUHelper.readHexOctet();

Where is this 'lang' used ?

@@ +11559,5 @@
> +        let octet = GsmPDUHelper.readHexOctet();
> +        readLen++;
> +        if (octet == 0xff) break;
> +        userDataBuffer.push(octet);
> +      }

Seems you can remove spnOctetLen and replace it with octetLen, 
no?
Attachment #723339 - Flags: review?(allstars.chh)
Attachment #723340 - Flags: review?(allstars.chh) → review+
(In reply to Yoshi Huang[:allstars.chh][:yoshi] from comment #23)
> Comment on attachment 723339 [details] [diff] [review]
> Patch: Part 2 - Reading SPN file
> 
> Review of attachment 723339 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/system/gonk/ril_worker.js
> @@ +11548,5 @@
> > +      let strLen = Buf.readUint32();
> > +      let octetLen = strLen / 2;
> > +      let displayCondition = GsmPDUHelper.readHexOctet();
> > +      let codingScheme = GsmPDUHelper.readHexOctet();
> > +      let lang = GsmPDUHelper.readHexOctet();
> 
> Where is this 'lang' used ?
> 
It indicates the language of the string value. We don't need this value here.

> @@ +11559,5 @@
> > +        let octet = GsmPDUHelper.readHexOctet();
> > +        readLen++;
> > +        if (octet == 0xff) break;
> > +        userDataBuffer.push(octet);
> > +      }
> 
> Seems you can remove spnOctetLen and replace it with octetLen, 
> no?

spnOctetLen stands for the octet length of the SPN string, while octetLen is length of file, which is (3 + spnOctetLen + length of tailing 0xffs). We need spnOctetLen to calculate the string length for decoding.
Comment on attachment 723339 [details] [diff] [review]
Patch: Part 2 - Reading SPN file

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

::: dom/system/gonk/ril_worker.js
@@ +11559,5 @@
> +        let octet = GsmPDUHelper.readHexOctet();
> +        readLen++;
> +        if (octet == 0xff) break;
> +        userDataBuffer.push(octet);
> +      }

Correction

Seems you can remove spnOctetLen and replace it with readLen?
Created attachment 723372 [details] [diff] [review]
Patch: Part 2 - Reading SPN file
Attachment #723339 - Attachment is obsolete: true
Attachment #723372 - Flags: review?(allstars.chh)
Comment on attachment 723372 [details] [diff] [review]
Patch: Part 2 - Reading SPN file

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

Much better now,

thank you.

::: dom/system/gonk/ril_worker.js
@@ +11548,5 @@
> +      let strLen = Buf.readUint32();
> +      let octetLen = strLen / 2;
> +      let displayCondition = GsmPDUHelper.readHexOctet();
> +      let codingScheme = GsmPDUHelper.readHexOctet();
> +      let lang = GsmPDUHelper.readHexOctet();

If you didn't use lang for now,

Either: 
1. Add a comment,
2. Use Buf.SeekIncoming

@@ +11557,5 @@
> +
> +      while (readLen < octetLen) {
> +        let octet = GsmPDUHelper.readHexOctet();
> +        readLen++;
> +        if (octet == 0xff) break;

nit, add { } even it's just one line.
This is mentioned in Mozilla Coding Style.

@@ +11558,5 @@
> +      while (readLen < octetLen) {
> +        let octet = GsmPDUHelper.readHexOctet();
> +        readLen++;
> +        if (octet == 0xff) break;
> +        userDataBuffer.push(octet);        

trailing spaces.
Attachment #723372 - Flags: review?(allstars.chh) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/17b65305f131
https://hg.mozilla.org/integration/mozilla-inbound/rev/14415ab6c873
https://hg.mozilla.org/integration/mozilla-inbound/rev/be270cb736a5
https://hg.mozilla.org/integration/mozilla-inbound/rev/4e06397565a2
https://hg.mozilla.org/integration/mozilla-inbound/rev/a59a323e52f0
https://hg.mozilla.org/mozilla-central/rev/17b65305f131
https://hg.mozilla.org/mozilla-central/rev/14415ab6c873
https://hg.mozilla.org/mozilla-central/rev/be270cb736a5
https://hg.mozilla.org/mozilla-central/rev/4e06397565a2
https://hg.mozilla.org/mozilla-central/rev/a59a323e52f0
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla22
You need to log in before you can comment on or make changes to this bug.