835729 - B2G RIL: we shall update operator's mcc/mnc first then longName/shortName

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect)

Description

[Edgar Chen [:edgar]]

The flow of updating operator's mcc/mnc/longName/shortName seems wrong. We shall update operator's mcc/mnc first then name. But in current flow [1], we update operator's name first then mcc/mnc, so we may use the old mcc/mnc to get operator's name.

[1] http://hg.mozilla.org/mozilla-central/file/0c45e6378f1f/dom/system/gonk/ril_worker.js#l3057

Comment 1

[Edgar Chen [:edgar]]

Attachment: Part 1: Fix updating operator's info, v1

Correct the sequence of updating operator's info.

Comment 2

[Edgar Chen [:edgar]]

Attachment: Part 2: xpcshell tests, v2

xpcshell tests

Comment 3

[Edgar Chen [:edgar]]

try server: https://tbpl.mozilla.org/?tree=Try&rev=da697e0d3f8a

Comment 4

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

Comment on attachment 707521 [details] [diff] [review]
Part 1: Fix updating operator's info, v1

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

I think your patch did fixed the problem,
but I think the problem is the function 'updateNetworkName' is way too complicated,
The function name seems not very well expressed,
it reads so many global data,
and some code seems confusing, for example 
...
    if (iccInfoPriv.OPL) {
      for (let i in iccInfoPriv.OPL) {
...

I leave you to decide whether we could fix it in the bug or file another for it.

Comment 5

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

Comment on attachment 707522 [details] [diff] [review]
Part 2: xpcshell tests, v2

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

I think we should still keep the unit test for single function, for example, updateNetworkName.
Your test looks more like a huge test for _processOperator, which should test several functions.

I propose to make your test simpler and easier.

::: dom/system/gonk/tests/test_ril_worker_icc.js
@@ +1090,5 @@
>      }
>    }).RIL;
>  
> +  function testNetworkName(operatorData, expectedMCC, expectedMNC,
> +                                         expectedLongName, expectedShortName) {

I think the parameters should be
(operatorData, expectedData), that makes this function more simple.

Comment 6

[Edgar Chen [:edgar]]

(In reply to Yoshi Huang[:yoshi][:allstars.chh] from comment #4)
> Comment on attachment 707521 [details] [diff] [review]
> Part 1: Fix updating operator's info, v1
> 
> Review of attachment 707521 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I think your patch did fixed the problem,
> but I think the problem is the function 'updateNetworkName' is way too
> complicated,
> The function name seems not very well expressed,
> it reads so many global data,
> and some code seems confusing, for example 
> ...
>     if (iccInfoPriv.OPL) {
>       for (let i in iccInfoPriv.OPL) {
> ...
> 
> I leave you to decide whether we could fix it in the bug or file another for
> it.

Can't agree more. I will try to refactor it in this bug.
Thanks.

Comment 7

[Edgar Chen [:edgar]]

Attachment: Part 2: Refactor updateNetworkName and _processOperator, v1

Refactor updateNetworkName() and _processOperator()

Comment 8

[Edgar Chen [:edgar]]

Attachment: Part 1: Fix updating operator's info, v2

1). Rebase. (Based on bug 838096)
2). Correct debug logging.

Comment 9

[Edgar Chen [:edgar]]

Attachment: Part 2: Refactor updateNetworkName and _processOperator, v2

Rebase. (Based on bug 838096)

Comment 10

[Edgar Chen [:edgar]]

Attachment: Part 2: Refactor updateNetworkName and _processOperator, v3

Update comment

Comment 11

[Edgar Chen [:edgar]]

Attachment: Part 3: xpcshell tests, v2

Comment 12

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

Comment on attachment 711147 [details] [diff] [review]
Part 1: Fix updating operator's info, v2

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

::: dom/system/gonk/ril_worker.js
@@ +3087,5 @@
> +      } else {
> +        // According to ril.h, the operator fields will be NULL when the operator
> +        // is not currently registered. We can avoid trying to parse the numeric
> +        // tuple in that case.
> +        if (DEBUG) {

In the original code there's a check for !longName
Can you explain why it's removed?

Comment 13

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

Comment on attachment 711161 [details] [diff] [review]
Part 2: Refactor updateNetworkName and _processOperator, v3

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

::: dom/system/gonk/ril_worker.js
@@ +1282,5 @@
>      Buf.sendParcel();
>    },
>  
>    /**
> +   * Get network names using EF_OPL and EF_PNN

by using

@@ +1291,5 @@
> +   * @param mcc   The mobile country code of the network.
> +   * @param mnc   The mobile network code of the network.
> +   * @param lac   The location area code of the network.
> +   */
> +  getNetworkName: function getNetworkName(mcc, mnc, lac) {

Although it's called get*Network*Name, but actually the name is from SIM card, with information from OPL and PNN.

To avoid confusion, can you use a better name for this?
Also, can we move this function to ICCUtilsHelper ?
In that case, you also need to rename some data below, for example, this.iccInfo -> RIL.iccInfo.

@@ +1301,5 @@
>        return null;
>      }
>  
> +    // We won't get network name if there is no PNN file.
> +    if (!iccInfoPriv.PNN) {

Can we also add a check for OPL here?
so we won't need to 
if (iccInfoPriv.OPL) below.

Comment 14

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

Comment on attachment 711163 [details] [diff] [review]
Part 3: xpcshell tests, v2

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

This looks okay to me,
but I'd like to review this again when you update Part 2.

::: dom/system/gonk/tests/test_ril_worker_icc.js
@@ +1150,5 @@
>        // Do nothing
>      },
>      postMessage: function fakePostMessage(message) {
>        // Do nothing
>      }

Can you also remove these two dummy methods?
The original newWorker has done this.

Comment 15

[Edgar Chen [:edgar]]

(In reply to Yoshi Huang(Happy Chinese New Year 2.9~2.17)[:yoshi][:allstars.chh] from comment #12)
> Comment on attachment 711147 [details] [diff] [review]
> Part 1: Fix updating operator's info, v2
> 
> Review of attachment 711147 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/system/gonk/ril_worker.js
> @@ +3087,5 @@
> > +      } else {
> > +        // According to ril.h, the operator fields will be NULL when the operator
> > +        // is not currently registered. We can avoid trying to parse the numeric
> > +        // tuple in that case.
> > +        if (DEBUG) {
> 
> In the original code there's a check for !longName
> Can you explain why it's removed?


The check for longName is used for printing debug log when the network is unregistered.
And if unregistered, the networkTuple will be *null* as well (Same behavior as longName).

Because the check for longName there is a little bit weird to me, so I use networkTuple to make it more clear. How do you think? :)

Comment 16

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

Comment on attachment 711147 [details] [diff] [review]
Part 1: Fix updating operator's info, v2

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

okay to me.

Comment 17

[Edgar Chen [:edgar]]

(In reply to Yoshi Huang[:allstars.chh] from comment #13)
> Comment on attachment 711161 [details] [diff] [review]
> Part 2: Refactor updateNetworkName and _processOperator, v3
> 
> Review of attachment 711161 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/system/gonk/ril_worker.js
> @@ +1301,5 @@
> >        return null;
> >      }
> >  
> > +    // We won't get network name if there is no PNN file.
> > +    if (!iccInfoPriv.PNN) {
> 
> Can we also add a check for OPL here?
> so we won't need to 
> if (iccInfoPriv.OPL) below.

Because OPL and PNN are depended on different service, we may need to check them separately.
PNN -> Service 45: PLMN Network Name
OPL -> Service 46: Operator PLMN List

Comment 18

[Edgar Chen [:edgar]]

Attachment: Part 2: Refactor updateNetworkName and _processOperator, v4

1). Rename updateNetworkName() as getNetworkNameFromICC().
2). Move getNetworkNameFromICC() to ICCUtilsHelper.

Comment 19

[Edgar Chen [:edgar]]

Attachment: Part 3: xpcshell tests, v3

Comment 20

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

Comment on attachment 715031 [details] [diff] [review]
Part 2: Refactor updateNetworkName and _processOperator, v4

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

::: dom/system/gonk/ril_worker.js
@@ +9600,5 @@
> +    // the ME shall use this EF_OPL in association with the EF_PNN in place
> +    // of any network name stored within the ME's internal list and any network
> +    // name received when registered to the PLMN.
> +    if (iccInfoPriv.OPL) {
> +      for (let i = 0; i < iccInfoPriv.OPL.length; i++) {

Using two levels of indents seems ugly to me.

Can we use 

let length = iccInfoPriv.OPL ? iccInfoPriv.OPL.length: 0;
for (let i = 0; i < length; i++) {
...
}

@@ +9631,5 @@
> +    // value to it.
> +    if (!pnnEntry && mcc == iccInfo.mcc && mnc == iccInfo.mnc) {
> +      pnnEntry = iccInfoPriv.PNN[0]
> +    }
> +

Seems we don't need 'ret' below.


if (!pnnEntry) {
  return null;
}

return {fullName: pnnEntry.fullName || "",
        shortName: pnnEntry.shortName || ""};

Comment 21

[Edgar Chen [:edgar]]

Attachment: Part 2: Refactor updateNetworkName and _processOperator, v5, r=allstars.chh

Address review comment #20.

Comment 22

[Edgar Chen [:edgar]]

try server: https://tbpl.mozilla.org/?tree=Try&rev=ca233825ec36

Comment 23

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/785ce2473812
https://hg.mozilla.org/integration/mozilla-inbound/rev/86c7d1b67175
https://hg.mozilla.org/integration/mozilla-inbound/rev/9c6fe95e2668

Comment 24

[Gregory Szorc [:gps]]

https://hg.mozilla.org/mozilla-central/rev/785ce2473812
https://hg.mozilla.org/mozilla-central/rev/86c7d1b67175
https://hg.mozilla.org/mozilla-central/rev/9c6fe95e2668