Closed Bug 831702 Opened 7 years ago Closed 7 years ago

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

Categories

(Core :: DOM: Device Interfaces, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla21

People

(Reporter: allstars.chh, Assigned: chucklee)

References

Details

Attachments

(9 files, 14 obsolete files)

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.
Assignee: nobody → chulee
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)
Replace data length calculation in |sendStkTerminalReponse()| and |sendICCEnvelopeCommand()| with new function, and remove unused constants.
Attachment #704797 - Flags: review?(allstars.chh)
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)
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)
Send a Event Download : Location Status envelope and examine parcel content
Attachment #704804 - Flags: review?(allstars.chh)
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+
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 ?
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+
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)
Modify calculation function addressing review comments
Attachment #704796 - Attachment is obsolete: true
Attachment #705212 - Flags: review?(allstars.chh)
Modify based on calculation function change and review comments
Attachment #704797 - Attachment is obsolete: true
Attachment #705213 - Flags: review?(allstars.chh)
Attached patch 0003. Fix data member name typo (obsolete) — Splinter Review
Already review+, update patch line due to previous patch.
Attachment #704798 - Attachment is obsolete: true
Already review+, update patch line due to previous patch.
Attachment #704799 - Attachment is obsolete: true
Add new test worker supporting outgoingIndex instead of modifying existing one.
Attachment #704802 - Attachment is obsolete: true
Attachment #705217 - Flags: review?(allstars.chh)
Same test case as previous patch, just use new worker.
Attachment #704804 - Attachment is obsolete: true
Attachment #705219 - Flags: review?(allstars.chh)
Same test case as previous patch, just use new worker.
Attachment #704805 - Attachment is obsolete: true
Attachment #705220 - Flags: review?(allstars.chh)
Attachment #705212 - Flags: review?(allstars.chh) → review+
Attachment #705217 - Flags: review?(allstars.chh) → review+
Attachment #705219 - Flags: review?(allstars.chh) → review+
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()|.
Attachment #705213 - Flags: review?(allstars.chh) → review+
Add bug number in patch comment
Attachment #705212 - Attachment is obsolete: true
Add bug number in patch comment
Attachment #705214 - Attachment is obsolete: true
Add bug number in patch comment
Attachment #705220 - Attachment is obsolete: true
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.
[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?
[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-
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.