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

RESOLVED FIXED in mozilla21

Status

()

RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: allstars.chh, Assigned: chucklee)

Tracking

unspecified
mozilla21
ARM
Gonk (Firefox OS)
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(9 attachments, 14 obsolete attachments)

6.69 KB, patch
Details | Diff | Splinter Review
6.91 KB, patch
Details | Diff | Splinter Review
1.12 KB, patch
Details | Diff | Splinter Review
3.08 KB, patch
Details | Diff | Splinter Review
1.21 KB, patch
Details | Diff | Splinter Review
3.47 KB, patch
Details | Diff | Splinter Review
2.94 KB, patch
Details | Diff | Splinter Review
1.12 KB, patch
Details | Diff | Splinter Review
3.52 KB, patch
Details | Diff | Splinter Review
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.
(Reporter)

Updated

6 years ago
Blocks: 791161
(Reporter)

Updated

6 years ago
Blocks: 831627
(Reporter)

Updated

6 years ago
Blocks: 831628
Assignee: nobody → chulee
Created attachment 704796 [details] [diff] [review]
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.
Attachment #704796 - Flags: review?(allstars.chh)
Created attachment 704797 [details] [diff] [review]
0002.Calculate data length by data length calculation function

Replace data length calculation in |sendStkTerminalReponse()| and |sendICCEnvelopeCommand()| with new function, and remove unused constants.
Attachment #704797 - Flags: review?(allstars.chh)
Created attachment 704798 [details] [diff] [review]
0003. Fix name typo of data member
Attachment #704798 - Flags: review?(allstars.chh)
Created attachment 704799 [details] [diff] [review]
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.
Attachment #704799 - Flags: review?(allstars.chh)
Created attachment 704802 [details] [diff] [review]
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.
Attachment #704802 - Flags: review?(allstars.chh)
Created attachment 704804 [details] [diff] [review]
0006. Test case for STK Event Download - Location Status

Send a Event Download : Location Status envelope and examine parcel content
Attachment #704804 - Flags: review?(allstars.chh)
Created attachment 704805 [details] [diff] [review]
0007. Test case for STK Terminal Response

Send a STK Terminal Response and examine parcel content
Attachment #704805 - Flags: review?(allstars.chh)
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 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;
}
Attachment #704796 - Flags: review?(allstars.chh) → review+
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
Attachment #704797 - Flags: review?(allstars.chh) → review+
(Reporter)

Updated

6 years ago
Attachment #704798 - Flags: review?(allstars.chh) → review+
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 ?
(Reporter)

Updated

6 years ago
Attachment #704799 - Flags: review?(allstars.chh) → review+
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 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);
Attachment #704804 - Flags: review?(allstars.chh) → review+
(Reporter)

Updated

6 years ago
Attachment #704805 - Flags: review?(allstars.chh) → review+
(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.
(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.
(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 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?
Attachment #704802 - Flags: review?(allstars.chh)
Created attachment 705212 [details] [diff] [review]
0001. Add data size calculation function

Modify calculation function addressing review comments
Attachment #704796 - Attachment is obsolete: true
Attachment #705212 - Flags: review?(allstars.chh)
Created attachment 705213 [details] [diff] [review]
0002. Calculate data size by data size calculation function, v2

Modify based on calculation function change and review comments
Attachment #704797 - Attachment is obsolete: true
Attachment #705213 - Flags: review?(allstars.chh)
Created attachment 705214 [details] [diff] [review]
0003. Fix data member name typo

Already review+, update patch line due to previous patch.
Attachment #704798 - Attachment is obsolete: true
Created attachment 705215 [details] [diff] [review]
0004. Fix data not set while its value is zero

Already review+, update patch line due to previous patch.
Attachment #704799 - Attachment is obsolete: true
Created attachment 705217 [details] [diff] [review]
0005. Add worker which supports outgoingIndex in xpcshell ril_worker_icc test

Add new test worker supporting outgoingIndex instead of modifying existing one.
Attachment #704802 - Attachment is obsolete: true
Attachment #705217 - Flags: review?(allstars.chh)
Created attachment 705219 [details] [diff] [review]
0006. Test case for STK Event Download - Location Status, V1.1

Same test case as previous patch, just use new worker.
Attachment #704804 - Attachment is obsolete: true
Attachment #705219 - Flags: review?(allstars.chh)
Created attachment 705220 [details] [diff] [review]
0007. Test case for STK Terminal Response, V1.1

Same test case as previous patch, just use new worker.
Attachment #704805 - Attachment is obsolete: true
Attachment #705220 - Flags: review?(allstars.chh)
(Reporter)

Updated

6 years ago
Attachment #705212 - Flags: review?(allstars.chh) → review+
(Reporter)

Updated

6 years ago
Attachment #705217 - Flags: review?(allstars.chh) → review+
(Reporter)

Updated

6 years ago
Attachment #705219 - Flags: review?(allstars.chh) → review+
(Reporter)

Updated

6 years ago
Attachment #705220 - Flags: review?(allstars.chh) → review+
(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()|.
(Reporter)

Updated

6 years ago
Attachment #705213 - Flags: review?(allstars.chh) → review+
Created attachment 705249 [details] [diff] [review]
0001. Add data size calculation function, v2.1

Add bug number in patch comment
Attachment #705212 - Attachment is obsolete: true
Created attachment 705250 [details] [diff] [review]
0002. Calculate data size by data size calculation function, v2.1

Add bug number in patch comment
Attachment #705213 - Attachment is obsolete: true
Created attachment 705251 [details] [diff] [review]
0003. Fix data member name typo, v1.1

Add bug number in patch comment
Attachment #705214 - Attachment is obsolete: true
Created attachment 705252 [details] [diff] [review]
0004. Fix data not set while its value is zero, v1.1

Add bug number in patch comment
Attachment #705215 - Attachment is obsolete: true
Created attachment 705254 [details] [diff] [review]
0005. Add worker which supports outgoingIndex in xpcshell ril_worker_icc test, v1.1

Add bug number in patch comment
Attachment #705217 - Attachment is obsolete: true
Created attachment 705255 [details] [diff] [review]
0006. Test case for STK Event Download - Location Status, V1.2

Add bug number in patch comment
Attachment #705219 - Attachment is obsolete: true
Created attachment 705256 [details] [diff] [review]
0007. Test case for STK Terminal Response, V1.2

Add bug number in patch comment
Attachment #705220 - Attachment is obsolete: true
Keywords: checkin-needed
Keywords: checkin-needed
Hi, Chuck
Can you make Part 3 and Part 4 patches for b2g18?
You could see Bug 831183 for example.
(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.
Created attachment 706213 [details] [diff] [review]
(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
Attachment #706213 - Flags: approval-mozilla-b2g18?
Created attachment 706224 [details] [diff] [review]
(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
Attachment #706224 - Flags: approval-mozilla-b2g18?
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.
Attachment #706213 - Flags: approval-mozilla-b2g18? → approval-mozilla-b2g18-

Updated

6 years ago
Attachment #706224 - Flags: approval-mozilla-b2g18? → approval-mozilla-b2g18-
You need to log in before you can comment on or make changes to this bug.