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.

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.

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

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

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

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

Send a STK Terminal Response and examine parcel content

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; }

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 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 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);

(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?

Modify calculation function addressing review comments

Modify based on calculation function change and review comments

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

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

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

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

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

(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()|.

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

Add bug number in patch comment

Add bug number in patch comment

Add bug number in patch comment

Add bug number in patch comment

Add bug number in patch comment

Add bug number in patch comment

Add bug number in patch comment

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

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

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

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

[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 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.