817529 - B2G RIL: SPN should be shown if device is in PLMN that is listed in EF_SPDI

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect)

Description

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

This is a follow-up bug of bug 793111.
According to:
* TS 51.011 Sec. 10.3.11 and
* TS 31.102 Sec. 4.2.12
SPN should be shown carrier name if device is in PLMN that is listed in EF_SPDI.

Comment 1

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

Attachment: Patch

1. Reading EF_SPDI for PLMN list in both SIM and USIM.
2. Set isDisplaySpnRequired = true when current PLMN is HPLMN or is one of PLMNs that are listed in EF_SPDI.

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

Comment 2

[Mozilla RelEng Bot]

Try run for bfcaeff4fcb3 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=bfcaeff4fcb3
Results (out of 2 total builds):
    success: 2
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-bfcaeff4fcb3

Comment 3

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

Comment on attachment 687704 [details] [diff] [review]
Patch

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

Nice job!

::: dom/system/gonk/ril_worker.js
@@ +1237,5 @@
>        if (isOnMatchingPlmn) {
>          // The first bit of display condition tells us if we should display
>          // registered PLMN.
>          if (DEBUG) debug("updateDisplayCondition: PLMN is HPLMN or PLMN is in PLMN list");
> +        iccInfo.isDisplaySpnRequired = true;

Since the logic here is so complex and non-trivial that we have had several rounds of discussion/fix on it, I think it deserves some excerpts from the specification to make each line more clear.

@@ +1242,1 @@
>          if (iccSpn.spnDisplayCondition & 0x01) {

Ditto

@@ +1249,5 @@
>          // registered PLMN.
>          if (DEBUG) debug("updateICCDisplayName: PLMN isn't HPLMN and PLMN isn't in PLMN list");
>          if (iccSpn.spnDisplayCondition & 0x02) {
>            iccInfo.isDisplayNetworkNameRequired = false;
>            iccInfo.isDisplaySpnRequired = false;

Per previous discussion, we can't find whether should we turn isDisplaySpnRequired on here. So, some comments here are also appreciated.

@@ +1496,5 @@
>  
>    /**
>     * Read the SPDI (Service Provider Display Information) from the ICC.
>     *
>     * See TS 131.102 section 4.2.66

Please also add: See TS 51.011 section 10.3.50

Comment 4

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

Attachment: Patch

r+ in comment 3. Add comment in the patch according to reviewer's comment.

Comment 5

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

Attachment: Patch

r+ in comment 3, fix wrong comment in previous version.

Comment 6

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

Attachment: Patch

Comment is still wrong, sorry. Update again.

Comment 7

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

Attachment: Patch

Rebase.

Comment 8

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

Finally, try finished after rebase: https://tbpl.mozilla.org/?tree=Try&rev=1c031401a882

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/43344ee15385

Comment 10

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/43344ee15385

Comment 11

[Ryan VanderMeulen [:RyanVM]]

This is marked blocking-basecamp+ but doesn't apply cleanly to aurora/beta. Please get bb+ on whatever this depends on or post a branch-specific patch.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

Whoops, wrong bug. Ignore that.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/9a231e539ea3
https://hg.mozilla.org/releases/mozilla-beta/rev/47e1012871b5