Last Comment Bug 738132 - B2G SMS: Support application port addressing
: B2G SMS: Support application port addressing
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM: Device Interfaces (show other bugs)
: Trunk
: ARM Gonk (Firefox OS)
: -- normal (vote)
: mozilla14
Assigned To: Vicamo Yang [:vicamo][:vyang]
:
Mentors:
Depends on: 736941
Blocks: b2g-sms b2g-mms 713471 744360
  Show dependency treegraph
 
Reported: 2012-03-21 20:11 PDT by Vicamo Yang [:vicamo][:vyang]
Modified: 2012-04-24 04:21 PDT (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Part 1: add GsmPDUHelper.readDataCodingScheme (4.85 KB, patch)
2012-04-11 03:45 PDT, Vicamo Yang [:vicamo][:vyang]
philipp: review+
Details | Diff | Splinter Review
Part 2: support 8-bit encoding (3.27 KB, patch)
2012-04-11 03:45 PDT, Vicamo Yang [:vicamo][:vyang]
philipp: review-
Details | Diff | Splinter Review
Part 3: parse application port address for incoming messages (4.47 KB, patch)
2012-04-11 03:47 PDT, Vicamo Yang [:vicamo][:vyang]
philipp: review+
Details | Diff | Splinter Review
Part 2: support 8-bit encoding : V2 (6.33 KB, patch)
2012-04-19 08:45 PDT, Vicamo Yang [:vicamo][:vyang]
no flags Details | Diff | Splinter Review
Part 3: parse application port address for incoming messages : V2 (5.28 KB, patch)
2012-04-19 08:48 PDT, Vicamo Yang [:vicamo][:vyang]
philipp: review+
Details | Diff | Splinter Review
Part 2: support 8-bit encoding : V3 (6.33 KB, patch)
2012-04-19 23:51 PDT, Vicamo Yang [:vicamo][:vyang]
philipp: review+
Details | Diff | Splinter Review
Part 3: parse application port address for incoming messages : V3 (5.41 KB, patch)
2012-04-23 00:30 PDT, Vicamo Yang [:vicamo][:vyang]
philipp: review+
Details | Diff | Splinter Review

Description Vicamo Yang [:vicamo][:vyang] 2012-03-21 20:11:06 PDT
See 3GPP TS 23.040 9.2.3.24.3. This issue is also the first stepping stone to WebMMS.
Comment 1 Vicamo Yang [:vicamo][:vyang] 2012-03-21 22:35:40 PDT
SMS IEI Application Port Addressing 8/16 bit allows short messages to be routed to one of multiple applications, using a method similar to TCP/UDP ports in a TCP/IP network. An application entity is uniquely identified by the pair of TP-DA/TP-OA and the port address.

This issue should give a way to send[1]/receive octet data via SMS. I suggest new APIs for send:

1) dom/sms/interfaces/nsIDOMSmsManager:

  jsval sendData(in jsval number, in octet[] data);

2) dom/sms/interfaces/nsISmsService:

  void sendData(in DOMString number, in octet[] data,
                in long requestId, [optional] in unsigned long long processId);

3) dom/sms/interfaces/nsIRadioInterfaceLayer[2]:

  void sendSMSData(in DOMString number, in octet[] data,
                   in long requestId, in unsigned long long processId);

4) embedding/android/SmsManager.java.in[3]:

  public void sendData(String aNumber, byte[] aData,
                       int aRequestId, long aProcessId);

PS1: Sending outgoing data via SMS is not required for WAP Push/MMS.
PS2: Also need changes in dom/system/gonk/nsIRadioInterfaceLayer.js
PS3: Also need changes in embedding/android/GeckoAppShell.java,
                          mobile/android/base/GeckoAppShell.java,
                          mobile/android/base/GeckoSmsManager.java,
                          widget/android/AndroidBridge.cpp,
                          widget/android/AndroidBridge.h

For routing received data PDU, some application registration mechanism is mandatory. The original RadioInterfaceLayer uses kSmsReceivedObjserverTopic notification, but the created SmsMessage is text-based. Maybe we should another topic, say kSmsDataReceivedObjserverTopic.

Any suggestions?
Comment 2 Vicamo Yang [:vicamo][:vyang] 2012-03-21 22:38:26 PDT
(In reply to Vicamo Yang [:vicamo] from comment #1)
> This issue should give a way to send[1]/receive octet data via SMS. I
> suggest new APIs for send:
> 
> 1) dom/sms/interfaces/nsIDOMSmsManager:
> 
>   jsval sendData(in jsval number, in octet[] data);

Should existing send/sendSMS/sendMessage methods be renamed to sendText/sendTextSMS/sendTextMessage as well?
Comment 3 Mounir Lamouri (:mounir) 2012-03-22 01:49:55 PDT
sendText() would be ok for me.

However, I would have prefer to pass Blob[] instead of octet[]. It seems way nicer to web authors. Is there a reason why you prefer octet[]?

And I guess we would like to allow passing a SMSC to the sendData() method, right?
Comment 4 Vicamo Yang [:vicamo][:vyang] 2012-03-22 08:54:13 PDT
(In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #3)
> sendText() would be ok for me.

I meant, there are 3 different names but related to the same function. It is `send` in nsIDOMMozSmsManager and nsISmsService, `sendSMS` in nsIRadioInterfaceLayer and `sendMessage` in android backend code. Should we rename all of them to the same one or keep these differences? I would vote for an unique `sendTextMessage`.

> However, I would have prefer to pass Blob[] instead of octet[]. It seems way
> nicer to web authors. Is there a reason why you prefer octet[]?

Because I don't know there is some class named Blob ...

I have to emphasize again that sending outgoing data via SMS is not required for WAP Push/MMS. This sentDataMessage API will probably be used as a utility function to implement/test port addressed data SMS receiving. That is, it's unlikely that any user will ever try to use this API. Besides, port addressed data SMS receiving is only for WAP Push PDUs, which are mostly for MMS availability indication. Another potential user would be network initiated AGPS[1], which is supported by Android. Again, it's sent via WAP Push, no outgoing data need. Both of them take raw bytes inside the SMS message. You may call them bytes/octets/blobs, anything that refers to a continuous space for data. And it will be "in Blob aBlob", not "in Blob[] aBlobs", right?

Reference:
1) http://www.openmobilealliance.org/technical/release_program/supl_v1_0.aspx

> And I guess we would like to allow passing a SMSC to the sendData() method,
> right?

That's in bug 736701, Reply Path. You can't reply a data based SMS message. Data based SMS messages are dispatched to registered applications and never get stored in SMS database. So we won't need originating SMSC/TP-RP/pid(for bug 736708) here.
Comment 5 Vicamo Yang [:vicamo][:vyang] 2012-03-22 20:23:56 PDT
(In reply to Vicamo Yang [:vicamo] from comment #4)
> (In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #3)
> > And I guess we would like to allow passing a SMSC to the sendData() method,
> > right?
> 
> That's in bug 736701, Reply Path. You can't reply a data based SMS message.
> Data based SMS messages are dispatched to registered applications and never
> get stored in SMS database. So we won't need originating SMSC/TP-RP/pid(for
> bug 736708) here.

I'm wrong, Android API has it.
Comment 6 Vicamo Yang [:vicamo][:vyang] 2012-03-22 20:35:10 PDT
GAIA issue link is here: https://github.com/andreasgal/gaia/issues/921
Comment 7 Vicamo Yang [:vicamo][:vyang] 2012-04-11 03:38:59 PDT
See bug 740698 comment 5, we'll not implement sendData in this task thus it should no longer depends on bug 740698.
Comment 8 Vicamo Yang [:vicamo][:vyang] 2012-04-11 03:45:09 PDT
Created attachment 613940 [details] [diff] [review]
Part 1: add GsmPDUHelper.readDataCodingScheme
Comment 9 Vicamo Yang [:vicamo][:vyang] 2012-04-11 03:45:56 PDT
Created attachment 613941 [details] [diff] [review]
Part 2: support 8-bit encoding
Comment 10 Vicamo Yang [:vicamo][:vyang] 2012-04-11 03:47:05 PDT
Created attachment 613942 [details] [diff] [review]
Part 3: parse application port address for incoming messages
Comment 11 Vicamo Yang [:vicamo][:vyang] 2012-04-11 04:40:46 PDT
(In reply to Vicamo Yang [:vicamo] from comment #9)
> Created attachment 613941 [details] [diff] [review]
> Part 2: support 8-bit encoding

This patch depends on bug 736941 attachment 610819 [details] [diff] [review] "Part 3: Add GsmPDUHelper.read/writeHexOctetArray"
Comment 12 Vicamo Yang [:vicamo][:vyang] 2012-04-11 22:41:17 PDT
Comment on attachment 613942 [details] [diff] [review]
Part 3: parse application port address for incoming messages

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

::: dom/system/gonk/RadioInterfaceLayer.js
@@ +421,5 @@
>  
> +    // If application port addressing is available ...
> +    // Note that it can possibly be zero when representing a UDP/TCP port.
> +    if (typeof message.header.destinationPort != "undefined") {
> +      // TODO: dispatch to registered application

Shall we have some kind of port registration in RadioInterfaceLayer?

::: dom/system/gonk/ril_worker.js
@@ +2932,5 @@
> +          header.originatorPort = orip;
> +          break;
> +        }
> +        case PDU_IEI_APPLICATION_PORT_ADDREESING_SCHEME_16BIT: {
> +          let dstp = (this.readHexOctet() << 8) | this.readHexOctet());

unpaired parenthesis
Comment 13 Philipp von Weitershausen [:philikon] 2012-04-17 20:24:21 PDT
Comment on attachment 613941 [details] [diff] [review]
Part 2: support 8-bit encoding

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

::: dom/system/gonk/ril_worker.js
@@ +1730,5 @@
>      delete this._receivedSmsSegmentsMap[hash];
>  
>      // Rebuild full body
> +    if (options.encoding == PDU_DCS_MSG_CODING_8BITS_ALPHABET) {
> +      options.fullBody = Array.concat(options.segments);

What are you trying to do here? This will simply make a copy of the array, not return a string. I'm pretty sure you want something else, but I don't know what. :)
Comment 14 Philipp von Weitershausen [:philikon] 2012-04-17 20:26:46 PDT
Comment on attachment 613942 [details] [diff] [review]
Part 3: parse application port address for incoming messages

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

r+ with points addressed.

::: dom/system/gonk/RadioInterfaceLayer.js
@@ +418,5 @@
>  
>    handleSmsReceived: function handleSmsReceived(message) {
>      debug("handleSmsReceived: " + JSON.stringify(message));
>  
> +    // If application port addressing is available ...

Then what?

@@ +420,5 @@
>      debug("handleSmsReceived: " + JSON.stringify(message));
>  
> +    // If application port addressing is available ...
> +    // Note that it can possibly be zero when representing a UDP/TCP port.
> +    if (typeof message.header.destinationPort != "undefined") {

`if (message.header.destinationPort)`? Or can the port be zero? In that case, `if (message.header.destinationPort != null)` (we should accept both `undefined` and `null`, and undefined == null).
Comment 15 Vicamo Yang [:vicamo][:vyang] 2012-04-17 20:41:51 PDT
(In reply to Philipp von Weitershausen [:philikon] from comment #13)
> Comment on attachment 613941 [details] [diff] [review]
> Part 2: support 8-bit encoding
> 
> Review of attachment 613941 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/system/gonk/ril_worker.js
> @@ +1730,5 @@
> >      delete this._receivedSmsSegmentsMap[hash];
> >  
> >      // Rebuild full body
> > +    if (options.encoding == PDU_DCS_MSG_CODING_8BITS_ALPHABET) {
> > +      options.fullBody = Array.concat(options.segments);
> 
> What are you trying to do here? This will simply make a copy of the array,
> not return a string. I'm pretty sure you want something else, but I don't
> know what. :)

Yes, I do want a concatenated array. In 8-bit encoding scheme applied and as a result of current text only SMS API, binary data will be processed in RadioInterfaceLayer.handleSmsReceived() and DISCARDED. We need binary data for WAP Push.
Comment 16 Philipp von Weitershausen [:philikon] 2012-04-17 21:00:38 PDT
(In reply to Vicamo Yang [:vicamo] from comment #15)
> > ::: dom/system/gonk/ril_worker.js
> > @@ +1730,5 @@
> > >      delete this._receivedSmsSegmentsMap[hash];
> > >  
> > >      // Rebuild full body
> > > +    if (options.encoding == PDU_DCS_MSG_CODING_8BITS_ALPHABET) {
> > > +      options.fullBody = Array.concat(options.segments);
> > 
> > What are you trying to do here? This will simply make a copy of the array,
> > not return a string. I'm pretty sure you want something else, but I don't
> > know what. :)
> 
> Yes, I do want a concatenated array.

What's a "concatenated array"?

> In 8-bit encoding scheme applied and as
> a result of current text only SMS API, binary data will be processed in
> RadioInterfaceLayer.handleSmsReceived() and DISCARDED. We need binary data
> for WAP Push.

I see. How is binary data going to be represented?
Comment 17 Vicamo Yang [:vicamo][:vyang] 2012-04-17 22:52:00 PDT
(In reply to Philipp von Weitershausen [:philikon] from comment #16)
> (In reply to Vicamo Yang [:vicamo] from comment #15)
> > > ::: dom/system/gonk/ril_worker.js
> > > @@ +1730,5 @@
> > > >      delete this._receivedSmsSegmentsMap[hash];
> > > >  
> > > >      // Rebuild full body
> > > > +    if (options.encoding == PDU_DCS_MSG_CODING_8BITS_ALPHABET) {
> > > > +      options.fullBody = Array.concat(options.segments);
> > > 
> > > What are you trying to do here? This will simply make a copy of the array,
> > > not return a string. I'm pretty sure you want something else, but I don't
> > > know what. :)
> > 
> > Yes, I do want a concatenated array.
> 
> What's a "concatenated array"?

I mean, the WSP(sessionless)/WDP/... delegate fragmentation handling to its bearer. They want a full PDU frame at each processing transaction. Therefore SMS segments of 8-bit encoding should be concatenated before submitting to upper layer.

> > In 8-bit encoding scheme applied and as
> > a result of current text only SMS API, binary data will be processed in
> > RadioInterfaceLayer.handleSmsReceived() and DISCARDED. We need binary data
> > for WAP Push.
> 
> I see. How is binary data going to be represented?

The binary data submitted to registered application will be an array of octets. For now we have only one such app, WAP Push, whose PDU parser will be implemented in bug 744360.
Comment 18 Philipp von Weitershausen [:philikon] 2012-04-17 23:08:08 PDT
(In reply to Vicamo Yang [:vicamo] from comment #17)
> (In reply to Philipp von Weitershausen [:philikon] from comment #16)
> > (In reply to Vicamo Yang [:vicamo] from comment #15)
> > > > ::: dom/system/gonk/ril_worker.js
> > > > @@ +1730,5 @@
> > > > >      delete this._receivedSmsSegmentsMap[hash];
> > > > >  
> > > > >      // Rebuild full body
> > > > > +    if (options.encoding == PDU_DCS_MSG_CODING_8BITS_ALPHABET) {
> > > > > +      options.fullBody = Array.concat(options.segments);
> > > > 
> > > > What are you trying to do here? This will simply make a copy of the array,
> > > > not return a string. I'm pretty sure you want something else, but I don't
> > > > know what. :)
> > > 
> > > Yes, I do want a concatenated array.
> > 
> > What's a "concatenated array"?
> 
> I mean, the WSP(sessionless)/WDP/... delegate fragmentation handling to its
> bearer. They want a full PDU frame at each processing transaction. Therefore
> SMS segments of 8-bit encoding should be concatenated before submitting to
> upper layer.

Concatenated with what? Array.concat with just one argument doesn't make a whole lot of sense because it will just work like Array.slice(array). Array.concat takes several arrays and concatenates them together to make one big one. That doesn't seem to be what you want here, but I don't know what else you want. Also, options.fullBody is normally a string and I don't think we should change that; it would be very confusing.

> > > In 8-bit encoding scheme applied and as
> > > a result of current text only SMS API, binary data will be processed in
> > > RadioInterfaceLayer.handleSmsReceived() and DISCARDED. We need binary data
> > > for WAP Push.
> > 
> > I see. How is binary data going to be represented?
> 
> The binary data submitted to registered application will be an array of
> octets.

As in, a JS array of numbers? Or a JS string? Or a JS Typed Array?
Comment 19 Vicamo Yang [:vicamo][:vyang] 2012-04-18 01:39:58 PDT
(In reply to Philipp von Weitershausen [:philikon] from comment #18)
> (In reply to Vicamo Yang [:vicamo] from comment #17)
> Concatenated with what? Array.concat with just one argument doesn't make a
> whole lot of sense because it will just work like Array.slice(array).
> Array.concat takes several arrays and concatenates them together to make one
> big one. That doesn't seem to be what you want here, but I don't know what
> else you want.

`concat creates a new array consisting of the elements in the this object on which it is called, followed in order by, for each argument, the elements of that argument (if the argument is an array) or the argument itself (if the argument is not an array).` ~ https://developer.mozilla.org/en/JavaScript/Reference/Global_Objects/Array/concat

So `Array.concat(options.segments)` == `Array.concat(options.segments[0], options.segments[1], ...)`. Verified on FF.

> Also, options.fullBody is normally a string and I don't think
> we should change that; it would be very confusing.

That's a good point. I can rename it.

> As in, a JS array of numbers? Or a JS string? Or a JS Typed Array?

Yup, a JS array of numbers.
Comment 20 Philipp von Weitershausen [:philikon] 2012-04-18 02:09:47 PDT
(In reply to Vicamo Yang [:vicamo] from comment #19)
> (In reply to Philipp von Weitershausen [:philikon] from comment #18)
> > (In reply to Vicamo Yang [:vicamo] from comment #17)
> > Concatenated with what? Array.concat with just one argument doesn't make a
> > whole lot of sense because it will just work like Array.slice(array).
> > Array.concat takes several arrays and concatenates them together to make one
> > big one. That doesn't seem to be what you want here, but I don't know what
> > else you want.
> 
> `concat creates a new array consisting of the elements in the this object on
> which it is called,

Which is `options.segments` in your case.

> followed in order by, for each argument, the elements of
> that argument (if the argument is an array) or the argument itself (if the
> argument is not an array).` ~
> https://developer.mozilla.org/en/JavaScript/Reference/Global_Objects/Array/concat

You're not passing any extra arguments, so essentially you're just creating a copy of the options.segments array.

> So `Array.concat(options.segments)` == `Array.concat(options.segments[0],
> options.segments[1], ...)`. Verified on FF.

You're misunderstanding the semantics of the first argument. The Array.* functions are shortcuts for Array.prototype.*.call, so the first argument is always the `this` of the operation. So it must be an array:

  Array.concat(a, b, c, ...)

translates to

  Array.prototype.concat.call(a, b, c, ...)

which is the long way of spelling

  a.concat(b, c, ...);

If `a` is not an array, you're doing something very "interesting" that definitely doesn't make sense.

> > Also, options.fullBody is normally a string and I don't think
> > we should change that; it would be very confusing.
> 
> That's a good point. I can rename it.
> 
> > As in, a JS array of numbers? Or a JS string? Or a JS Typed Array?
> 
> Yup, a JS array of numbers.

Why not a typed array of the Uint8Array kind, if we're dealing with octets here anyway?
Comment 21 Vicamo Yang [:vicamo][:vyang] 2012-04-18 02:44:35 PDT
(In reply to Philipp von Weitershausen [:philikon] from comment #20)
> (In reply to Vicamo Yang [:vicamo] from comment #19)
> You're not passing any extra arguments, so essentially you're just creating
> a copy of the options.segments array.

Great! I should have JSON.stringified it before output. Ok, I'm wrong. Any suggestions? I find `Array.concat([], options.segments)` works for me, but I have no idea about its performance impact.

> > Yup, a JS array of numbers.
> 
> Why not a typed array of the Uint8Array kind, if we're dealing with octets
> here anyway?

That array is read from GsmPDUHelper.readHexOctetArray() being introduced in bug 736941 attachment 614307 [details] [diff] [review]. So I should modify the patch instead?

I'll cancel review request for these patches for now and return when your comments are fully addressed.
Comment 22 Philipp von Weitershausen [:philikon] 2012-04-18 02:57:46 PDT
(In reply to Vicamo Yang [:vicamo] from comment #21)
> (In reply to Philipp von Weitershausen [:philikon] from comment #20)
> > (In reply to Vicamo Yang [:vicamo] from comment #19)
> > You're not passing any extra arguments, so essentially you're just creating
> > a copy of the options.segments array.
> 
> Great! I should have JSON.stringified it before output. Ok, I'm wrong. Any
> suggestions? I find `Array.concat([], options.segments)` works for me, but I
> have no idea about its performance impact.

`[].concat(options.segments)` will just create a copy of `options.segments` as well, since it adds the elements of `options.segments` to an empty array.

Really, I would just like to know what you're trying to do. Do you want to flatten an array of arrays or something? What are the elements of `options.segments`?

> > > Yup, a JS array of numbers.
> > 
> > Why not a typed array of the Uint8Array kind, if we're dealing with octets
> > here anyway?
> 
> That array is read from GsmPDUHelper.readHexOctetArray() being introduced in
> bug 736941 attachment 614307 [details] [diff] [review]. So I should modify the patch instead?
> 
> I'll cancel review request for these patches for now and return when your
> comments are fully addressed.

Yeah, I think having GsmPDUHelper.readHexOctetArray() return a typed array makes sense since it obviously returns octets and we know the size in advance.
Comment 23 Vicamo Yang [:vicamo][:vyang] 2012-04-19 01:18:14 PDT
(In reply to Philipp von Weitershausen [:philikon] from comment #22)
> (In reply to Vicamo Yang [:vicamo] from comment #21)
> > (In reply to Philipp von Weitershausen [:philikon] from comment #20)
> Really, I would just like to know what you're trying to do. Do you want to
> flatten an array of arrays or something? What are the elements of
> `options.segments`?

`options.segments` is an array of string/octets bodies of each SMS message segment. For 7bit/UCS2 encoding, each of its elements is a string; for 8bit, a array of octet numbers.

> > That array is read from GsmPDUHelper.readHexOctetArray() being introduced in
> > bug 736941 attachment 614307 [details] [diff] [review]. So I should modify the patch instead?
> > 
> > I'll cancel review request for these patches for now and return when your
> > comments are fully addressed.
> 
> Yeah, I think having GsmPDUHelper.readHexOctetArray() return a typed array
> makes sense since it obviously returns octets and we know the size in
> advance.

I just found the typed array has fewer API than a normal one. For example, there is no `concat` and `toString` for it. To concatenate full PDU payload back, one have to write several lines to create a new typed array with appropriate length and reassign all elements into the new array. The debug print takes also a hand-write for-loop as well. There isn't something like `toArray` API. If there isn't any way more conventient or any advantage that we can get from Uint8Array, I really want to avoid them.
Comment 24 Vicamo Yang [:vicamo][:vyang] 2012-04-19 08:45:47 PDT
Created attachment 616591 [details] [diff] [review]
Part 2: support 8-bit encoding : V2

1) assign 8-bit encoded user data to msg.data/fullData to prevent name confusion. See comment 18.
2) remove writeHexOctetArray reference for now.
3) fix array concatenation. See comment 13.
Comment 25 Vicamo Yang [:vicamo][:vyang] 2012-04-19 08:48:17 PDT
Created attachment 616592 [details] [diff] [review]
Part 3: parse application port address for incoming messages : V2

1) fix unpaired parenthesis. See comment 12
2) add application registration code. See comment 14
Comment 26 Vicamo Yang [:vicamo][:vyang] 2012-04-19 22:48:48 PDT
Comment on attachment 616591 [details] [diff] [review]
Part 2: support 8-bit encoding : V2

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

::: dom/system/gonk/ril_worker.js
@@ +1859,5 @@
>  
>      if (message.header && (message.header.segmentMaxSeq > 1)) {
>        message = this._processReceivedSmsSegment(message);
>      } else {
> +      if (options.encoding == PDU_DCS_MSG_CODING_8BITS_ALPHABET) {

should be `message.encoding`
Comment 27 Vicamo Yang [:vicamo][:vyang] 2012-04-19 23:51:26 PDT
Created attachment 616883 [details] [diff] [review]
Part 2: support 8-bit encoding : V3

Fix comment 26.
Comment 28 Philipp von Weitershausen [:philikon] 2012-04-22 17:49:03 PDT
Comment on attachment 616883 [details] [diff] [review]
Part 2: support 8-bit encoding : V3

Nice!
Comment 29 Philipp von Weitershausen [:philikon] 2012-04-22 17:57:19 PDT
Comment on attachment 616592 [details] [diff] [review]
Part 3: parse application port address for incoming messages : V2

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

::: dom/system/gonk/RadioInterfaceLayer.js
@@ +419,4 @@
>    handleSmsReceived: function handleSmsReceived(message) {
>      debug("handleSmsReceived: " + JSON.stringify(message));
>  
> +    // If application port addressing is available ...

If would be good if you could still complete that sentence or rephrase the comment somehow to be a coherent English sentence otherwise.
Comment 30 Philipp von Weitershausen [:philikon] 2012-04-22 17:58:01 PDT
(In reply to Vicamo Yang [:vicamo] from comment #23)
> (In reply to Philipp von Weitershausen [:philikon] from comment #22)
> > (In reply to Vicamo Yang [:vicamo] from comment #21)
> > > (In reply to Philipp von Weitershausen [:philikon] from comment #20)
> > Really, I would just like to know what you're trying to do. Do you want to
> > flatten an array of arrays or something? What are the elements of
> > `options.segments`?
> 
> `options.segments` is an array of string/octets bodies of each SMS message
> segment. For 7bit/UCS2 encoding, each of its elements is a string; for 8bit,
> a array of octet numbers.

Got it. Thanks for explaining.

> > > That array is read from GsmPDUHelper.readHexOctetArray() being introduced in
> > > bug 736941 attachment 614307 [details] [diff] [review]. So I should modify the patch instead?
> > > 
> > > I'll cancel review request for these patches for now and return when your
> > > comments are fully addressed.
> > 
> > Yeah, I think having GsmPDUHelper.readHexOctetArray() return a typed array
> > makes sense since it obviously returns octets and we know the size in
> > advance.
> 
> I just found the typed array has fewer API than a normal one. For example,
> there is no `concat` and `toString` for it.

Yeah, Typed Arrays have the standard `toString` method which is not very helpful on any object. I also feel your pain with `concat`, I ran into the same thing in `Buf.growIncomingBuffer()`. That said, I think a Uint8Array is still the right choice for binary data (octets).

> To concatenate full PDU payload
> back, one have to write several lines to create a new typed array with
> appropriate length and reassign all elements into the new array.

We can create a helper. Also, perhaps we should just add a `concat` method to Typed Arrays, and perhaps even a useful `toString` method! Boot to Gecko is all about finding the pain points of the web and fixing them. :)

> The debug print takes also a hand-write for-loop as well.

You can use Array comprehensions:

  [x for each (x in typedarray)]

It's not super short, but it's shorter than a loop ;)
Comment 31 Vicamo Yang [:vicamo][:vyang] 2012-04-23 00:30:01 PDT
Created attachment 617408 [details] [diff] [review]
Part 3: parse application port address for incoming messages : V3

Address to comment 29, and rebase to latest m-c.

Note You need to log in before you can comment on or make changes to this bug.