Closed Bug 1094984 Opened 7 years ago Closed 7 years ago

B2G RIL: Handling empty record for EFPNN more properly.

Categories

(Firefox OS Graveyard :: RIL, defect)

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(blocking-b2g:2.0M+, b2g-v2.0 affected, b2g-v2.0M fixed, b2g-v2.1 affected, b2g-v2.2 affected)

RESOLVED FIXED
2.1 S9 (21Nov)
blocking-b2g 2.0M+
Tracking Status
b2g-v2.0 --- affected
b2g-v2.0M --- fixed
b2g-v2.1 --- affected
b2g-v2.2 --- affected

People

(Reporter: sku, Assigned: sku)

References

Details

Attachments

(2 files, 2 obsolete files)

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

Handling empty record for EFPNN more properly.
Assignee: nobody → sku
Summary: B2G RIL: Handling empty record for EFPNN more properly → B2G RIL: Handling empty record for EFPNN more properly.
Attachment #8518291 - Flags: review?(echen)
blocking-b2g: --- → 2.0M?
Duplicate of this bug: 1087906
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: +86-135 0229 4315
Group: woodduck-confidential
(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: +86-135 0229 4315

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
Dear Shawn ku,
    
   多谢支持!

Thanks
KaiDu

SWD3-HZ-PIC
Mobile: +86-135 0229 4315
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.
Attachment #8518291 - Flags: review?(echen)
blocking-b2g: 2.0M? → 2.0M+
Attachment #8518291 - Attachment is obsolete: true
Attachment #8519516 - Flags: review?(echen)
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.
Attachment #8519516 - Flags: review?(echen) → review+
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)
Keywords: checkin-needed
Does this need a b2g34 nomination for v2.1 as well?
Flags: needinfo?(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.
Flags: needinfo?(sku)
https://hg.mozilla.org/mozilla-central/rev/d1b7ed051d44
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → 2.1 S9 (21Nov)
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:
Attachment #8519631 - Flags: approval-mozilla-b2g34?
[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 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.
Attachment #8519631 - Flags: approval-mozilla-b2g34?
Hi Shawn,
partner uploaded new log. https://bugzilla.mozilla.org/attachment.cgi?id=8530001
 Could you check again? Thanks!
Flags: needinfo?(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)
Flags: needinfo?(sku) → needinfo?(jocheng)
Hi Shawn,
It's bug 1087906. Sorry for not mentioned it.
Flags: needinfo?(jocheng) → needinfo?(sku)
see reply on Comment 26 of Bug 1087906.
Flags: needinfo?(sku)
You need to log in before you can comment on or make changes to this bug.