831702 - B2G STK: Refactor writing string length in send Terminal Response and Envelope command

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

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

Currently in STK implementation,
when sending Terminal Response (sendStkTerminalReponse) and Envelope (sendICCEnvelopeCommand), we need to calculate total octets to be written beforehand, and convert the octets length to string length.
This makes code hard to write/maintain.

We could use something like Buf.writeParcelSize to shift the outgoing index.

Comment 1

[Chuck Lee [:chucklee, Triggered by NI, Review, and Feedback requests]]

Attachment: 0001. Add data size calculation function

Support dynamic calculation of written data length to outgoingBuffer:
1. Use |startCalOutgoingSize()| to allocate buffer for write data length, and use as start position for calculating written data size.
2. Use |stopCalOutgoingSize| to finish a data length calculation segment, and write data length to corresponding position.
3. data length buffer is not included in data length calculation.
4. start-stop can be nested in pair.

Comment 2

[Chuck Lee [:chucklee, Triggered by NI, Review, and Feedback requests]]

Attachment: 0002.Calculate data length by data length calculation function

Replace data length calculation in |sendStkTerminalReponse()| and |sendICCEnvelopeCommand()| with new function, and remove unused constants.

Comment 3

[Chuck Lee [:chucklee, Triggered by NI, Review, and Feedback requests]]

Attachment: 0003. Fix name typo of data member

Comment 4

[Chuck Lee [:chucklee, Triggered by NI, Review, and Feedback requests]]

Attachment: 0004. Fix data is not set while its value is zero

Some data accepts zero as value, but its will be ignored event its exist.
For certain data, check if its null instead.

Comment 5

[Chuck Lee [:chucklee, Triggered by NI, Review, and Feedback requests]]

Attachment: 0005. Modify xpcshell test script to support data length calculation function

Data length calculation function is based on outgoingIndex, support it in dummy ril_worker in test script so test can work.

Comment 6

[Chuck Lee [:chucklee, Triggered by NI, Review, and Feedback requests]]

Attachment: 0006. Test case for STK Event Download - Location Status

Send a Event Download : Location Status envelope and examine parcel content

Comment 7

[Chuck Lee [:chucklee, Triggered by NI, Review, and Feedback requests]]

Attachment: 0007. Test case for STK Terminal Response

Send a STK Terminal Response and examine parcel content

Comment 8

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

Comment on attachment 704796 [details] [diff] [review]
0001. Add data size calculation function

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

つつぐ.....

::: dom/system/gonk/ril_worker.js
@@ +157,5 @@
> +   *        Function to write data length into outgoingBuffer, this function is
> +   *        also used to allocate buffer for data length.
> +   **/
> +  startCalOutgoingSize: function startCalOutgoingSize(writeFunction) {
> +    var sizeInfo = {index: this.outgoingIndex,

let

@@ +183,5 @@
> +    var sizeInfo = this.outgoingBufferCalSizeQueue.pop(),
> +        // Remember current outgoingIndex.
> +        currentOutgoingIndex = this.outgoingIndex,
> +        // Calculate data length, in uint8.
> +        writeSize = this.outgoingIndex - sizeInfo.index - sizeInfo.size;

Why don't you just make it several lines?

Comment 9

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

Comment on attachment 704796 [details] [diff] [review]
0001. Add data size calculation function

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

::: dom/system/gonk/ril_worker.js
@@ +173,5 @@
> +
> +  /**
> +   * Calculate data length since last mark, and write it into mark position.
> +   *
> +   * @param lengthAdjust

Since you use 'size' in your code and caller, I'd suggest you use 'sizeAdujst'

@@ +179,5 @@
> +   *        is in uint8, which might be different from the unit required for
> +   *        writing. We can adjust the data length using this function.
> +   **/
> +  stopCalOutgoingSize: function stopCalOutgoingSize(lengthAdjust) {
> +    var sizeInfo = this.outgoingBufferCalSizeQueue.pop(),

let

@@ +184,5 @@
> +        // Remember current outgoingIndex.
> +        currentOutgoingIndex = this.outgoingIndex,
> +        // Calculate data length, in uint8.
> +        writeSize = this.outgoingIndex - sizeInfo.index - sizeInfo.size;
> +

We could add a check here 
if (typeof (sizeAdjust) != "function) {
  sizeAdjust = false;
}

Comment 10

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

Comment on attachment 704797 [details] [diff] [review]
0002.Calculate data length by data length calculation function

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

::: dom/system/gonk/ril_worker.js
@@ +2403,5 @@
>          }
> +
> +        // Calculate and write text length to 2nd mark
> +        Buf.stopCalOutgoingSize(function(size) {
> +          // text length is in number of hexOctet, which cost 4 uint8 per hexOctet.

s/text/Text/, s/hexOctet/hexOctets/, s/cost/costs/

Fix the same problem below

Comment 11

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

Comment on attachment 704796 [details] [diff] [review]
0001. Add data size calculation function

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

::: dom/system/gonk/ril_worker.js
@@ +173,5 @@
> +
> +  /**
> +   * Calculate data length since last mark, and write it into mark position.
> +   *
> +   * @param lengthAdjust

Also seems this function or factor could be provided in start,
any reason to use it in stop ?

Comment 12

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

Comment on attachment 704802 [details] [diff] [review]
0005. Modify xpcshell test script to support data length calculation function

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

Can you explain more in this patch?

Comment 13

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

Comment on attachment 704804 [details] [diff] [review]
0006. Test case for STK Event Download - Location Status

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

::: dom/system/gonk/tests/test_ril_worker_icc.js
@@ +1421,5 @@
> +      gsmCellId: 0
> +    }
> +  };
> +  worker.RIL.sendStkEventDownload({
> +    event: event

why not use event directly?
worker.RIL.sendStkEventDownload(event);

Comment 14

[Chuck Lee [:chucklee, Triggered by NI, Review, and Feedback requests]]

(In reply to Yoshi Huang[:yoshi][:allstars.chh] from comment #11)
> Comment on attachment 704796 [details] [diff] [review]
> 0001. Add data size calculation function
> 
> Review of attachment 704796 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/system/gonk/ril_worker.js
> @@ +173,5 @@
> > +
> > +  /**
> > +   * Calculate data length since last mark, and write it into mark position.
> > +   *
> > +   * @param lengthAdjust
> 
> Also seems this function or factor could be provided in start,
> any reason to use it in stop ?

Just for I thought its easier to know how to adjust the size at end of writes(where stop is called) while coding.
If we move it to start, maybe we could adjust the length in write function, so we don't have to pass another parameter.

Comment 15

[Chuck Lee [:chucklee, Triggered by NI, Review, and Feedback requests]]

(In reply to Yoshi Huang[:yoshi][:allstars.chh] from comment #12)
> Comment on attachment 704802 [details] [diff] [review]
> 0005. Modify xpcshell test script to support data length calculation function
> 
> Review of attachment 704802 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Can you explain more in this patch?

The test case overrides |writeUint8()| in ril_worker.Buf, this works fine but lost sync. with |ril_work.Buf.outgoingIndex|.
So in the test case, writing data doesn't change the value of |ril_worker.Buf.outgoingIndex|. Also, changing the value of |ril_worker.BufoutgoingIndex| can't affect the writing position.

But size calculate function works based on manipulating |ril_worker.Buf.outgoingIndex|. So I modify |writeUint8()| to make the |ril_work.Buf.outgoingIndex| back to work in the test case.

Comment 16

[Chuck Lee [:chucklee, Triggered by NI, Review, and Feedback requests]]

(In reply to Yoshi Huang[:yoshi][:allstars.chh] from comment #13)
> Comment on attachment 704804 [details] [diff] [review]
> 0006. Test case for STK Event Download - Location Status
> 
> Review of attachment 704804 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/system/gonk/tests/test_ril_worker_icc.js
> @@ +1421,5 @@
> > +      gsmCellId: 0
> > +    }
> > +  };
> > +  worker.RIL.sendStkEventDownload({
> > +    event: event
> 
> why not use event directly?
> worker.RIL.sendStkEventDownload(event);

According to [1], |sendStkEventDownload()| access the event data by command.event.*, so I have to pass |event| as a key of object.

[1] https://mxr.mozilla.org/mozilla-central/source/dom/system/gonk/ril_worker.js#2492

Comment 17

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

Comment on attachment 704802 [details] [diff] [review]
0005. Modify xpcshell test script to support data length calculation function

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

Can you explain more in this patch?

This Uint8Worker is very simple so we could write our test case easily.
The way I see in this patch complicates the simple worker,
as we don't want to test this worker in test case again,
also we don't want this Uint8Worker does more thing than manipulating Uint8.

I'd suggest you move your code to another worker.

::: dom/system/gonk/tests/test_ril_worker_icc.js
@@ +16,4 @@
>    let buf = [];
>  
>    worker.Buf.writeUint8 = function (value) {
> +    let transformIndex = worker.Buf.outgoingIndex - 4;

4 is PARCEL_SIZE_SIZE right?

@@ +18,5 @@
>    worker.Buf.writeUint8 = function (value) {
> +    let transformIndex = worker.Buf.outgoingIndex - 4;
> +    if (transformIndex >= buf.length) {
> +      buf.push(value);
> +    } else {

Can you also explain when we will go into 'else' part?

Comment 18

[Chuck Lee [:chucklee, Triggered by NI, Review, and Feedback requests]]

Attachment: 0001. Add data size calculation function

Modify calculation function addressing review comments

Comment 19

[Chuck Lee [:chucklee, Triggered by NI, Review, and Feedback requests]]

Attachment: 0002. Calculate data size by data size calculation function, v2

Modify based on calculation function change and review comments

Comment 20

[Chuck Lee [:chucklee, Triggered by NI, Review, and Feedback requests]]

Attachment: 0003. Fix data member name typo

Already review+, update patch line due to previous patch.

Comment 21

[Chuck Lee [:chucklee, Triggered by NI, Review, and Feedback requests]]

Attachment: 0004. Fix data not set while its value is zero

Already review+, update patch line due to previous patch.

Comment 22

[Chuck Lee [:chucklee, Triggered by NI, Review, and Feedback requests]]

Attachment: 0005. Add worker which supports outgoingIndex in xpcshell ril_worker_icc test

Add new test worker supporting outgoingIndex instead of modifying existing one.

Comment 23

[Chuck Lee [:chucklee, Triggered by NI, Review, and Feedback requests]]

Attachment: 0006. Test case for STK Event Download - Location Status, V1.1

Same test case as previous patch, just use new worker.

Comment 24

[Chuck Lee [:chucklee, Triggered by NI, Review, and Feedback requests]]

Attachment: 0007. Test case for STK Terminal Response, V1.1

Same test case as previous patch, just use new worker.

Comment 25

[Chuck Lee [:chucklee, Triggered by NI, Review, and Feedback requests]]

(In reply to Yoshi Huang[:yoshi][:allstars.chh] from comment #17)
> Comment on attachment 704802 [details] [diff] [review]
> 0005. Modify xpcshell test script to support data length calculation function
> 
>
> I'd suggest you move your code to another worker.

I will do that.

> 
> ::: dom/system/gonk/tests/test_ril_worker_icc.js
> @@ +16,4 @@
> >    let buf = [];
> >  
> >    worker.Buf.writeUint8 = function (value) {
> > +    let transformIndex = worker.Buf.outgoingIndex - 4;
> 
> 4 is PARCEL_SIZE_SIZE right?

Yes, it will dismissed by setting initial value in new worker.

> 
> @@ +18,5 @@
> >    worker.Buf.writeUint8 = function (value) {
> > +    let transformIndex = worker.Buf.outgoingIndex - 4;
> > +    if (transformIndex >= buf.length) {
> > +      buf.push(value);
> > +    } else {
> 
> Can you also explain when we will go into 'else' part?

When |stopCalOutgoingSize()| is called, it will change outgoingIndex to a value smaller than |buf.length| to write data size into buffer preserved by |startCalOutgoingSize()|.

Comment 26

[Chuck Lee [:chucklee, Triggered by NI, Review, and Feedback requests]]

Try server : https://tbpl.mozilla.org/?tree=Try&rev=5e1f198a4b46

Comment 27

[Chuck Lee [:chucklee, Triggered by NI, Review, and Feedback requests]]

Attachment: 0001. Add data size calculation function, v2.1

Add bug number in patch comment

Comment 28

[Chuck Lee [:chucklee, Triggered by NI, Review, and Feedback requests]]

Attachment: 0002. Calculate data size by data size calculation function, v2.1

Add bug number in patch comment

Comment 29

[Chuck Lee [:chucklee, Triggered by NI, Review, and Feedback requests]]

Attachment: 0003. Fix data member name typo, v1.1

Add bug number in patch comment

Comment 30

[Chuck Lee [:chucklee, Triggered by NI, Review, and Feedback requests]]

Attachment: 0004. Fix data not set while its value is zero, v1.1

Add bug number in patch comment

Comment 31

[Chuck Lee [:chucklee, Triggered by NI, Review, and Feedback requests]]

Attachment: 0005. Add worker which supports outgoingIndex in xpcshell ril_worker_icc test, v1.1

Add bug number in patch comment

Comment 32

[Chuck Lee [:chucklee, Triggered by NI, Review, and Feedback requests]]

Attachment: 0006. Test case for STK Event Download - Location Status, V1.2

Add bug number in patch comment

Comment 33

[Chuck Lee [:chucklee, Triggered by NI, Review, and Feedback requests]]

Attachment: 0007. Test case for STK Terminal Response, V1.2

Add bug number in patch comment

Comment 34

[Chuck Lee [:chucklee, Triggered by NI, Review, and Feedback requests]]

Update Try server : https://tbpl.mozilla.org/?tree=Try&rev=9e4b76400e4b

Comment 35

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/ec664ff50de3
https://hg.mozilla.org/integration/mozilla-inbound/rev/d87a31d6b5da
https://hg.mozilla.org/integration/mozilla-inbound/rev/7757be36c407
https://hg.mozilla.org/integration/mozilla-inbound/rev/e6a9468ae5af
https://hg.mozilla.org/integration/mozilla-inbound/rev/f4d657dba5c6
https://hg.mozilla.org/integration/mozilla-inbound/rev/19eab2095963
https://hg.mozilla.org/integration/mozilla-inbound/rev/333d198ec6bc

Comment 36

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/ec664ff50de3
https://hg.mozilla.org/mozilla-central/rev/d87a31d6b5da
https://hg.mozilla.org/mozilla-central/rev/7757be36c407
https://hg.mozilla.org/mozilla-central/rev/e6a9468ae5af
https://hg.mozilla.org/mozilla-central/rev/f4d657dba5c6
https://hg.mozilla.org/mozilla-central/rev/19eab2095963
https://hg.mozilla.org/mozilla-central/rev/333d198ec6bc

Comment 37

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

Hi, Chuck
Can you make Part 3 and Part 4 patches for b2g18?
You could see Bug 831183 for example.

Comment 38

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

(In reply to Yoshi Huang[:yoshi][:allstars.chh] from comment #37)
> Hi, Chuck
> Can you make Part 3 and Part 4 patches for b2g18?
> You could see Bug 831183 for example.

Forgot to mention you could try to push to your local b2g18 branch first, if it fails you need to prepare another same patch but for b2g18.

Comment 39

[Chuck Lee [:chucklee, Triggered by NI, Review, and Feedback requests]]

Attachment: (b2g18) 0003. Fix data member name typo, v1.1

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
User impact if declined: STK Envelope Download Command, including  STK_EVENT_TYPE_MT_CALL, STK_EVENT_TYPE_CALL_DISCONNECTED, STK_EVENT_TYPE_CALL_CONNECTED, can't work properly, which leads to malfunction of corresponding web API icc.sendStkEventDownload.icc.sendStkEventDownload
Testing completed: xpcshell tests
Risk to taking this patch (and alternatives if risky): No
String or UUID changes made by this patch: No

Comment 40

[Chuck Lee [:chucklee, Triggered by NI, Review, and Feedback requests]]

Attachment: (b2g18) 0004. Fix data not set while its value is zero, v1.1

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
User impact if declined: Malfunction of web API icc.sendStkEventDownload.icc.sendStkEventDownload and incorrect handling of some STK Proactive Commands, while parameters of these commands are set to zero.
Testing completed: xpcshell tests
Risk to taking this patch (and alternatives if risky): Conflict while merge to central because some new functions on central fixed by this patch doesn't exist on b2g18. Also, some removed functions still exists on b2g18 which needs to be patched.
String or UUID changes made by this patch: No

Comment 41

[Johnny Stenback (:jst)]

Comment on attachment 706213 [details] [diff] [review]
(b2g18)  0003. Fix data member name typo, v1.1

This doesn't seem important enough to take on b2g18 at this point.