1094984 - B2G RIL: Handling empty record for EFPNN more properly.

Status: RESOLVED FIXED

(Firefox OS Graveyard :: RIL, defect)

Description

[shawn ku [:sku]]

+++ This bug was initially created as a clone of Bug #1087906 +++

Handling empty record for EFPNN more properly.

Comment 1

[shawn ku [:sku]]

Attachment: Bug 1094984 - B2G RIL: Handling empty record for EFPNN more properly.

Comment 3

[kai.du]

Dear Shawn ku,
   
    你们每修改一处代码，都会有相应的测试case，请问是怎么去执行测试case？怎么去查看测试的运行结果？谢谢！例如在文件test_ril_worker_icc_SimRecordHelper.js中，我看到你们增加的一个函数
Verify reading EF_PNN with different content.
add_test(function test_pnn_with_different_content()；

Thanks
KaiDu

SWD3-HZ-PIC
Mobile: [phone]

Comment 4

[shawn ku [:sku]]

(In reply to kai.du from comment #3)
> Dear Shawn ku,
>    
>    
> 你们每修改一处代码，都会有相应的测试case，请问是怎么去执行测试case？怎么去查看测试的运行结果？谢谢！例如在文件test_ril_worker_ic
> c_SimRecordHelper.js中，我看到你们增加的一个函数
> Verify reading EF_PNN with different content.
> add_test(function test_pnn_with_different_content()；
> 
> Thanks
> KaiDu
> 
> SWD3-HZ-PIC
> Mobile: [phone]

Hi Kai:
 The xpcshell/marionette will be used to confirm if the implementation is verified by test cases.
you can see [1] for the introduction of XPCShell.

Under emulator environment:
./mach xpcshell-test <path-to-gecko>/../<test-file>|<test-folder>
./mach marionette-webapi <path-to-gecko>/../<test-file>|<test-folder>


Before submit the patch (after r+), we need to confirm if we got pass on local trial.
and, mozilla also provide try server (ex: https://tbpl.mozilla.org/?tree=Try&rev=913d0b2186e6) to make sure there is no any cases fail.


[1] - https://developer.mozilla.org/en-US/Firefox_OS/Platform/Automated_testing/XPCShell

Comment 5

[kai.du]

Dear Shawn ku,
    
   多谢支持!

Thanks
KaiDu

SWD3-HZ-PIC
Mobile: [phone]

Comment 6

[Edgar Chen [:edgar]]

Comment on attachment 8518291 [details] [diff] [review]
Bug 1094984 - B2G RIL: Handling empty record for EFPNN more properly.

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

Thanks for your hard work.
Please see my comments below.

::: dom/system/gonk/tests/test_ril_worker_icc_SimRecordHelper.js
@@ +1599,5 @@
> +    // [4]: undefined
> +    pnn: [0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF],
> +  }];
> +
> +  function buildParcel(options) {

How about overriding `RIL.iccIO` with the same code of this function?
And trigger options.callback there.

@@ +1620,5 @@
> +    io.loadLinearFixedEF = function fakeLoadLinearFixedEF(options) {
> +      options.p1 = 1;
> +      options.totalRecords = test_data.length;
> +
> +      buildParcel(options);

Calls `ril.iccIo(options)` instead here.

@@ +1622,5 @@
> +      options.totalRecords = test_data.length;
> +
> +      buildParcel(options);
> +
> +      if (options.callback) {

Don't need this, triggering callback will be handled in `ril.iccIO()`.

@@ +1627,5 @@
> +        options.callback(options);
> +      }
> +    };
> +
> +    io.loadNextRecord = function fakeLoadNextRecord(options) {

Since ril.iccIO is overridden, we don't need this, either.

Comment 7

[shawn ku [:sku]]

Attachment: Bug 1094984 - B2G RIL: Handling empty record for EFPNN more properly.

Comment 8

[Edgar Chen [:edgar]]

Comment on attachment 8519516 [details] [diff] [review]
Bug 1094984 - B2G RIL: Handling empty record for EFPNN more properly.

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

Thank you.

Comment 9

[shawn ku [:sku]]

Attachment: Bug 1094984 - B2G RIL: Handling empty record for EFPNN more properly. r=Edgar.

Comment 10

[shawn ku [:sku]]

Attachment: [2.0m] Bug 1094984 - B2G RIL: Handling empty record for EFPNN more properly. r=Edgar.

Comment 11

[shawn ku [:sku]]

m-c try result - https://tbpl.mozilla.org/?tree=Try&rev=91e0290c7bcc
(failure was caused by nfc, not by my patch)

2.0m try result - https://tbpl.mozilla.org/?tree=Try&rev=06c42487b059
(failure was caused by https://bugzilla.mozilla.org/show_bug.cgi?id=986308#c49)

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/d1b7ed051d44

Comment 13

[Ryan VanderMeulen [:RyanVM]]

Does this need a b2g34 nomination for v2.1 as well?

Comment 14

[shawn ku [:sku]]

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #13)
> Does this need a b2g34 nomination for v2.1 as well?

Hi Ryan:
 Thanks for reminding.

Yes, but there is only one blocking-b2g flag which was occupied by 2.0m now. I will nominate for 2.1 once 2.0m is landed.

Comment 15

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/d1b7ed051d44

Comment 16

[Kai-Zhen Li [:seinlin][:kli]]

http://hg.mozilla.org/releases/mozilla-b2g32_v2_0m/rev/af28f106df7e

Comment 17

[shawn ku [:sku]]

Comment on attachment 8519631 [details] [diff] [review]
Bug 1094984 - B2G RIL: Handling empty record for EFPNN more properly. r=Edgar.

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
User impact if declined: 
Testing completed: 
Risk to taking this patch (and alternatives if risky): 
String or UUID changes made by this patch:

Comment 18

[shawn ku [:sku]]

[Approval Request Comment]
Bug caused by (feature/regressing bug #): feature
User impact if declined: EONS feature broken if we have empty record in the middle of EFPNN. That means operator name is incorrect to user if hit this case.
Testing completed: Yes
Risk to taking this patch (and alternatives if risky): Low 
String or UUID changes made by this patch: No

Comment 19

[bhavana bajaj [:bajaj]]

Comment on attachment 8519631 [details] [diff] [review]
Bug 1094984 - B2G RIL: Handling empty record for EFPNN more properly. r=Edgar.

Refer https://bugzilla.mozilla.org/show_bug.cgi?id=1085765#c35, for 2.1 approval.

Comment 20

[Josh Cheng [:josh]]

Hi Shawn,
partner uploaded new log. https://bugzilla.mozilla.org/attachment.cgi?id=8530001
 Could you check again? Thanks!

Comment 21

[shawn ku [:sku]]

(In reply to Josh Cheng [:josh] from comment #20)
> Hi Shawn,
> partner uploaded new log.
> https://bugzilla.mozilla.org/attachment.cgi?id=8530001
>  Could you check again? Thanks!

Hi Josh:
 Please let me know which bug does this attachment referring to!!

After checking the log, we should show C Vodafone (PLMN + SPN) per spec. definition.
And, we did that too.
(The screenshot shows nothing about operator name, but home screen only)

Per my understanding, we should do it right.


// Log snippet.
11-26 23:59:31.243   160   601 I Gecko   : RIL Worker: [0] SPN: spn = Vodafone, spnDisplayCondition = 1
11-27 00:01:34.076   888   888 I Gecko   : -*- RILContentHelper: Received message 'RIL:VoiceInfoChanged': {"clientId":0,"data":{"connected":true,"emergencyCallsOnly":false,"roaming":false,"network":{"longName":"C","shortName":"","mcc":"001","mnc":"01"},"cell":{"gsmLocationAreaCode":1,"gsmCellId":1},"type":"gprs","signalStrength":-53,"relSignalStrength":100,"state":"registered"}}
11-27 00:01:45.380   160   160 E GeckoConsole: Content JS LOG at app://system.gaiamobile.org/js/operator_name.js:101 in sb_updateLabel: lxp:: operatorInfos.operator = C Vodafone


// 3GPP TS 31.102, 4.2.12 EFSPN (Service Provider Name) 
b1=0: display of registered PLMN name not required when registered PLMN is either HPLMN or a PLMN in the service provider PLMN list (see EFSPDI).

B1=1: display of registered PLMN name required when registered PLMN is either HPLMN or a PLMN in the service provider PLMN list(see EFSPDI). 

B2=0: display of the service provider name is required when registered PLMN is neither HPLMN nor a PLMN in the service provider PLMN list(see EFSPDI).
B2=1: display of the service provider name is not required when registered PLMN is neither HPLMN nor a PLMN in the service provider PLMN list(see EFSPDI). RFU (see TS 31.101)

Comment 22

[Josh Cheng [:josh]]

Hi Shawn,
It's bug 1087906. Sorry for not mentioned it.

Comment 23

[shawn ku [:sku]]

see reply on Comment 26 of Bug 1087906.