940259 - B2G SMS: Support Wap Push over CDMA SMS

Status: RESOLVED FIXED

(Firefox OS Graveyard :: RIL, defect)

Description

[Bevis Tseng[:bevis][:btseng](Exited)]

According to 4.3.6 Wireless Application Protocol (WAP) in 3GPP2 C.S0015-B: Short Message Service (SMS) for Wideband Spread Spectrum Systems:
This Teleservice uses the SMS Deliver Message & SMS Submit Message. WAP is a point-to point service only.
Following are the sub-parameters used by WAP for SMS Delivery Message:
Message-Identifier |Mandatory
User Data          |Mandatory

File this bug to support parse WAP Push PDU from this teleservice type of SMS.

Comment 1

[Bevis Tseng[:bevis][:btseng](Exited)]

More detail information about how WDP is mapped into CDMA SMS is available in Chapter 6.5 of WAP-295-WDP:
http://technical.openmobilealliance.org/tech/affiliates/wap/wap-259-wdp-20010614-a.pdf

1. Format of a WDP datagram:
Field             | Length (bits)
----------------------------------
SOURCE_PORT       | 16
DESTINATION_PORT  | 16
DATA              | N*8

2. A WDP datagram is converted into multiple segments of CDMA SMS by mapping it into the subparameter of USER_DATA as followed:
Field             | Length (bits) 
-----------------------------------------
MSG_TYPE          | 8 
TOTAL_SEGMENTS    | 8 
SEGMENT_NUMBER    | 8 
DATAGRAM          | (NUM_FIELDS – 3) * 8 

3. How to ressemble segments into original WDP datagram:
   - Originator Address in SMS-TL + Message_Id in SMS-TS are used to identify a unique WDP datagram.
   - TOTAL_SEGMENTS, SEGMENT_NUMBER are used to verify that a complete 
datagram has been received and is ready to be passed to a higher layer.

Comment 2

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Patch_Part_1_v1: Support Wap Push over CDMA SMS. r=gene

This Patch is to support the WAP Push over CDMA SMS according to 
1. |4.3.6 Wireless Application Protocol (WAP)| in |3GPP2 C.S0015-B|:
   http://www.3gpp2.org/public_html/specs/C.S0015-B_v1.0_040514.pdf
2. |6.5. MAPPING OF WDP TO CDMA SMS| in |OMA wap-259-wdp|:
   http://technical.openmobilealliance.org/tech/affiliates/wap/wap-259-wdp-20010614-a.pdf

The detailed format is explained in comment#1, comment#2.

The implementation detail is
1. Store the userdata encoded in OCTET as opaque byte array for further decoding.
2. Identify Teleservice-Id WAP as Wap Push.
3. decode the WDP PDU including the segment info & the originator/destination ports to retrieve the WSP data for WapPushManager to process the incoming WAP Push message.

Comment 3

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Patch_Part_2_v1: Test Case for the support of CDMA WAP Push. r=gene

This test case includes 3 tests as followed:
1. WAP Push in Single CDMA SMS.
2. Concatenated WAP PDUs in multiple CDMA SMS.
3. Concatenated WAP PDUs received in reversed order.

Comment 4

[Bevis Tseng[:bevis][:btseng](Exited)]

Note: 
1. Only the formats defined in OMA/3GPP2 mentioned in comment#1 and comment#2 are supported in this patch.
2. Several CDMA Operators like CT, APTG use carrier-specific teleservice format for delivering WAP Push over CDMA SMS instead.

Comment 5

[Gene Lian (I already quit Mozilla)]

Comment on attachment 8338412 [details] [diff] [review]
Patch_Part_1_v1: Support Wap Push over CDMA SMS. r=gene

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

Overall, it looks nice! Just hope to take another glance after the nits are fixed.

::: dom/system/gonk/ril_worker.js
@@ +4376,5 @@
> +    //    datagram has been received and is ready to be passed to a higher layer.
> +    message.header = {
> +      segmentRef     : message.msgId,
> +      segmentMaxSeq  : message.data[index++],
> +      segmentSeq     : message.data[index++] + 1 // It's zero-based in CDMA WAP Push.

nits: please don't align ":" like this. Please do:

message.header = {
  segmentRef: message.msgId,
  segmentMaxSeq: message.data[index++],
  segmentSeq: message.data[index++] + 1 // It's zero-based in CDMA WAP Push.
}

Or if you want to align the attributes:

message.header = {
  segmentRef:    message.msgId,
  segmentMaxSeq: message.data[index++],
  segmentSeq:    message.data[index++] + 1 // It's zero-based in CDMA WAP Push.
}

Also, the namings look a bit redundant ("segment" actually means "seq"). I'd prefer:

s/segmentMaxSeq/segmentMaxNum
s/segmentSeq/segmentNum

What do you think?

@@ +4385,5 @@
> +      return PDU_FCS_OK;
> +    }
> +
> +    // Ports are only specified in 1st segment.
> +    if (message.header.segmentSeq == 1) {

s/segmentSeq/segmentNum

@@ +4386,5 @@
> +    }
> +
> +    // Ports are only specified in 1st segment.
> +    if (message.header.segmentSeq == 1) {
> +      message.header.originatorPort  = message.data[index++] << 8;

Please just use one space around operator.

@@ +4387,5 @@
> +
> +    // Ports are only specified in 1st segment.
> +    if (message.header.segmentSeq == 1) {
> +      message.header.originatorPort  = message.data[index++] << 8;
> +      message.header.originatorPort  |= message.data[index++];

Please just use one space around operator.

@@ +4436,5 @@
>      options.receivedSegments++;
> +
> +    // The port information is only available in 1st segment for CDMA WAP Push.
> +    if (original.teleservice === PDU_CDMA_MSG_TELESERIVCIE_ID_WAP && seq === 1) {
> +      if (!options.header.originatorPort && original.header.originatorPort) {

Do we really need to check the existence of options.header.originatorPort first? Can we directly override it for PDU_CDMA_MSG_TELESERIVCIE_ID_WAP?

@@ +4440,5 @@
> +      if (!options.header.originatorPort && original.header.originatorPort) {
> +        options.header.originatorPort = original.header.originatorPort;
> +      }
> +
> +      if (!options.header.destinationPort && original.header.destinationPort) {

Ditto.

@@ +8504,5 @@
>      return result;
>    },
>  
> +  backwardReadPilot: function backwardReadPilot(length) {
> +    if (length <= 0) return false;

if (length <= 0) {
  return false;
}

@@ +8506,5 @@
>  
> +  backwardReadPilot: function backwardReadPilot(length) {
> +    if (length <= 0) return false;
> +
> +    // zero-based position.

// Zero-based position.

@@ +8509,5 @@
> +
> +    // zero-based position.
> +    let bitIndexToRead = this.readIndex * 8 - this.readCacheSize - length;
> +
> +    if (bitIndexToRead < 0) return false;

if (bitIndexToRead < 0) {
  return false;
}

@@ +8511,5 @@
> +    let bitIndexToRead = this.readIndex * 8 - this.readCacheSize - length;
> +
> +    if (bitIndexToRead < 0) return false;
> +
> +    //update readIndex, readCache, readCacheSize accordingly.

// Update readIndex, readCache, readCacheSize accordingly.

@@ +8513,5 @@
> +    if (bitIndexToRead < 0) return false;
> +
> +    //update readIndex, readCache, readCacheSize accordingly.
> +    let readBits = bitIndexToRead % 8;
> +    this.readIndex = Math.floor(bitIndexToRead / 8) + ((readBits)? 1 : 0);

Add one space around "?".

@@ +8514,5 @@
> +
> +    //update readIndex, readCache, readCacheSize accordingly.
> +    let readBits = bitIndexToRead % 8;
> +    this.readIndex = Math.floor(bitIndexToRead / 8) + ((readBits)? 1 : 0);
> +    this.readCache = (readBits)? this.readBuffer[this.readIndex - 1]: 0;

Add one space around "?" and ":".

@@ +8515,5 @@
> +    //update readIndex, readCache, readCacheSize accordingly.
> +    let readBits = bitIndexToRead % 8;
> +    this.readIndex = Math.floor(bitIndexToRead / 8) + ((readBits)? 1 : 0);
> +    this.readCache = (readBits)? this.readBuffer[this.readIndex - 1]: 0;
> +    this.readCacheSize = (readBits)? (8 - readBits): 0;

Add one space around "?" and ":".

@@ +8987,5 @@
>  
>      // Bearer Data Sub-Parameter: User Data
>      let userData = message[PDU_CDMA_MSG_USERDATA_BODY];
> +    [message.header, message.body, message.encoding, message.data] =
> +      (userData)? [userData.header, userData.body, userData.encoding, userData.data]

Add one space around "?".

@@ +8995,5 @@
>      // Success Delivery (0) if both Message Status and User Data are absent.
>      // Message Status absent (-1) if only User Data is available.
>      let msgStatus = message[PDU_CDMA_MSG_USER_DATA_MSG_STATUS];
>      [message.errorClass, message.msgStatus] =
>        (msgStatus)? [msgStatus.errorClass, msgStatus.msgStatus]

Please correct this as well since you're here: add one space around "?".

@@ +9465,5 @@
>      }
>  
> +    // Store original payload if enconding is OCTET for further handling of WAP Push, etc.
> +    if (encoding === PDU_CDMA_MSG_CODING_OCTET
> +          && msgBodySize > 0) {

Please don't wrap this line if it doesn't exceed 80 chars.

@@ +9470,5 @@
> +      result.data = new Uint8Array(msgBodySize);
> +      for (let i = 0; i < msgBodySize; i++) {
> +        result.data[i] = BitBufferHelper.readBits(8);
> +      }
> +      BitBufferHelper.backwardReadPilot(8 * msgBodySize);

You didn't handle the returned value but you design this function as it is. Why? May we do some error handling here or just drop the true/false return?

Comment 6

[Gene Lian (I already quit Mozilla)]

Comment on attachment 8338414 [details] [diff] [review]
Patch_Part_2_v1: Test Case for the support of CDMA WAP Push. r=gene

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

Nice! But I still hope to take a second round.

::: dom/system/gonk/tests/test_ril_worker_sms_cdma.js
@@ +25,5 @@
>      }
>    };
>  }
>  
> +function hexStringToBytes(hexString) {

Please add a simple function explanation:

/*
 * Helper function to covert a HEX string to a byte array.
 *
 * @param hexString
 *        XXX
 */

@@ +40,5 @@
> +
> +/*
> + * Note: bytes could be regular array or Uint8Array
> + */
> +function bytesToHexString(bytes) {

/*
 * Helper function to covert a byte array to a HEX string.
 *
 * @param bytes
 *        Could be a regular byte array or Uint8Array.
 */

@@ +45,5 @@
> +  let hexString = "";
> +  let hex;
> +
> +  for (let i = 0; i < bytes.length; i++) {
> +    hex = bytes[i].toString(16).toUpperCase();

let hex = ...

and drop the declaration outside the for-loop.

@@ +61,5 @@
> + *
> + * @param msg_type
> + *        PDU_CDMA_MSG_TYPE_SUBMIT or PDU_CDMA_MSG_TYPE_DELIVER
> + * @param data
> + *        the byte array of opaque data to be encoded.

s/the/The/

@@ +70,5 @@
> +
> +  //Msg-Id
> +  bitBufferHelper.writeBits(PDU_CDMA_MSG_USERDATA_MSG_ID, 8);
> +  bitBufferHelper.writeBits(3, 8);
> +  bitBufferHelper.writeBits(options.msg_type, 4);  // MSG_TYPE

Just put one space before "//".

@@ +81,5 @@
> +  bitBufferHelper.writeBits(2 + dataLength, 8);   // 2 bytes for MSG_ENCODING, NUM_FIELDS
> +  bitBufferHelper.writeBits(PDU_CDMA_MSG_CODING_OCTET, 5); //MSG_ENCODING
> +                                                  // MSG_TYPE is omitted if MSG_ENCODING is CODING_OCTET
> +  bitBufferHelper.writeBits(dataLength, 8);       // NUM_FIELDS
> +  for (let i = 0; i < dataLength; i++) {          // CHARi

What is "CHARi"?

@@ +84,5 @@
> +  bitBufferHelper.writeBits(dataLength, 8);       // NUM_FIELDS
> +  for (let i = 0; i < dataLength; i++) {          // CHARi
> +    bitBufferHelper.writeBits(options.data[i], 8);
> +  }
> +  bitBufferHelper.flushWithPadding();             //RESERVED (3 filling bits)

// RESERVED (3 filling bits)

One space after "//".

@@ +108,5 @@
> + *        The Orginating or Destinating address.
> + * @param bearerData
> + *        The byte array of the encoded bearer data.
> + */
> +function pduToParcelData(cdmaPDUHelper, pdu) {

s/cdmaPDUHelper/cdmaPduHelper and all the below.

@@ +262,5 @@
> +        hexString,
> +        fullDataHexString = "";
> +
> +    for (let i = 0; i < wdpData.length; i++) {
> +      let dataIndex = (reversed)? (wdpData.length - i - 1): i;

Add spaces around "?" and ":".

@@ +266,5 @@
> +      let dataIndex = (reversed)? (wdpData.length - i - 1): i;
> +      hexString  = "00";                                // MSG_TYPE
> +      hexString += bytesToHexString([wdpData.length]);  // TOTAL_SEG
> +      hexString += bytesToHexString([dataIndex]);       // SEG_NUM (zero-based)
> +      if ((dataIndex === 0)) hexString += "23F00B84";   // SOURCE_PORT, DEST_PORT for 1st segment

if (dataIndex === 0) {
  hexString += "23F00B84"; // SOURCE_PORT, DEST_PORT for 1st segment
}

@@ +281,5 @@
> +                      bitBufferHelper,
> +                      {
> +                        msg_type: PDU_CDMA_MSG_TYPE_DELIVER,
> +                        data:     hexStringToBytes(hexString)
> +                      })

I'd prefer the following alignment:

let pdu = {
  teleservice: PDU_CDMA_MSG_TELESERIVCIE_ID_WAP,
  address:     orig_address,
  bearerData:  encodeOpaqueUserData(bitBufferHelper,
                 { msg_type: PDU_CDMA_MSG_TYPE_DELIVER,
                   data: hexStringToBytes(hexString) })
};

Comment 7

[Bevis Tseng[:bevis][:btseng](Exited)]

Hi Gene,

Thanks for your suggestion. I'll modify the nits you marked.
In additon, I would like to answer the questions you've raised inline. :)

(In reply to Gene Lian [:gene] (needinfo? encouraged) from comment #5)
> Comment on attachment 8338412 [details] [diff] [review]
> Patch_Part_1_v1: Support Wap Push over CDMA SMS. r=gene
> 
> Review of attachment 8338412 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> message.header = {
>   segmentRef: message.msgId,
>   segmentMaxSeq: message.data[index++],
>   segmentSeq: message.data[index++] + 1 // It's zero-based in CDMA WAP Push.
> }
> 
> Or if you want to align the attributes:
> 
> message.header = {
>   segmentRef:    message.msgId,
>   segmentMaxSeq: message.data[index++],
>   segmentSeq:    message.data[index++] + 1 // It's zero-based in CDMA WAP
> Push.
> }
> 
> Also, the namings look a bit redundant ("segment" actually means "seq"). I'd
> prefer:
> 
> s/segmentMaxSeq/segmentMaxNum
> s/segmentSeq/segmentNum
> 
> What do you think?

Actually, the seq means "sequence number of current message" instead of segment. :p
By my understanding, segment here is more likely to be a prefix of these segmentation information (Ref, MaxNum, Seq). :)
Hence, I would like apply "s/segmentMaxSeq/segmentMaxNum" but keep "s/segmentSeq/segmentNum" not changed.
(BTW, to keep the consistency of |segmentMaxNum|, the following files will also be modified:
tests/test_ril_worker_sms.js
tests/test_ril_worker_sms_segment_info.js
RadioInterfaceLayer.js

Is it okay?

> 
> @@ +4436,5 @@
> >      options.receivedSegments++;
> > +
> > +    // The port information is only available in 1st segment for CDMA WAP Push.
> > +    if (original.teleservice === PDU_CDMA_MSG_TELESERIVCIE_ID_WAP && seq === 1) {
> > +      if (!options.header.originatorPort && original.header.originatorPort) {
> 
> Do we really need to check the existence of options.header.originatorPort
> first? Can we directly override it for PDU_CDMA_MSG_TELESERIVCIE_ID_WAP?
> 
> @@ +4440,5 @@
> > +      if (!options.header.originatorPort && original.header.originatorPort) {
> > +        options.header.originatorPort = original.header.originatorPort;
> > +      }
> > +
> > +      if (!options.header.destinationPort && original.header.destinationPort) {
> 
> Ditto.

The original reason for this is that the segments of the same WAP push might not be received in the order of the sequence.
Hence, it is possible that the |options| kept in _receivedSmsSegmentsMap[hash] is not come from the 1st segment and the port information will be lost.

remove the checking of |if (!options.header.originatorPort && original.header.originatorPort)| and |if (!options.header.destinationPort && original.header.destinationPort)| will cause 2 re-assignments again of originatorPort & destinationPort when options === original.

Hence, it depends on if re-assignment is better than condition checking. :)
Do you expect to reassign it again in this case?

> 
> @@ +9470,5 @@
> > +      result.data = new Uint8Array(msgBodySize);
> > +      for (let i = 0; i < msgBodySize; i++) {
> > +        result.data[i] = BitBufferHelper.readBits(8);
> > +      }
> > +      BitBufferHelper.backwardReadPilot(8 * msgBodySize);
> 
> You didn't handle the returned value but you design this function as it is.
> Why? May we do some error handling here or just drop the true/false return?

Sorry, my bad. I'd like to drop the true/false return instead.

Comment 8

[Gene Lian (I already quit Mozilla)]

(In reply to Bevis Tseng [:bevistseng] ([e-mail]) from comment #7)
> Actually, the seq means "sequence number of current message" instead of
> segment. :p

OK. I misunderstood. Thanks for the clarification. How about segmentMaxSeqNum and segmentSeqNum?

> remove the checking of |if (!options.header.originatorPort &&
> original.header.originatorPort)| and |if (!options.header.destinationPort &&
> original.header.destinationPort)| will cause 2 re-assignments again of
> originatorPort & destinationPort when options === original.
> 
> Hence, it depends on if re-assignment is better than condition checking. :)
> Do you expect to reassign it again in this case?

OK I see. No need to reassign the values. Please check it's availability first. Also, please add some comments around here to explain why you need to check them first. :)

Comment 9

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Patch_Part_1_v2: Support Wap Push over CDMA SMS. r=gene

1. Add More explanation of how to correctly handle the port numbers of CDMA WAP Push in multiple segment scenario.
2. drop the return value in BitBufferHelper.backwardReadPilot().
3. Clear nits.

Comment 10

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Patch_Part_2_v2: Test Case for the support of CDMA WAP Push. r=gene

Fix nits.

Comment 11

[Gene Lian (I already quit Mozilla)]

Comment on attachment 8344516 [details] [diff] [review]
Patch_Part_1_v2: Support Wap Push over CDMA SMS. r=gene

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

::: dom/system/gonk/ril_worker.js
@@ +4315,5 @@
>    /**
> +   * Helper for processing CDMA SMS WAP Push Message
> +   *
> +   * @param message
> +   *        decoded WAP message from CdmaPDUHelper.

s/CdmaPDUHelper/CdmaPduHelper

@@ +4344,5 @@
> +    // 2. TOTAL_SEGMENTS, SEGMENT_NUMBER are used to verify that a complete
> +    //    datagram has been received and is ready to be passed to a higher layer.
> +    message.header = {
> +      segmentRef:     message.msgId,
> +      segmentMaxSeq:  message.data[index++],

nit: one space after ":" is enough.

@@ +4363,5 @@
> +    }
> +
> +    message.data = message.data.subarray(index);
> +
> +    return this._processSmsMultipart(message);

nit: drop the blank line below this.

@@ +4407,5 @@
> +    // The port information is only available in 1st segment for CDMA WAP Push.
> +    // If the segments of a WAP Push are not received in sequence
> +    // (e.g., SMS with seq == 1 is not the 1st segment received by the device),
> +    // we have to retrieve the port information from 1st segment and
> +    // save it into the cached options.header.

Nice comment! :)

@@ +8082,5 @@
>      return result;
>    },
>  
> +  backwardReadPilot: function backwardReadPilot(length) {
> +    if (length <= 0) return;

nit: putting statements in one line is not welcomed in our codes.

if (length <= 0) {
  return;
}

@@ +8566,5 @@
>      // Bearer Data Sub-Parameter: User Data
>      let userData = message[PDU_CDMA_MSG_USERDATA_BODY];
> +    [message.header, message.body, message.encoding, message.data] =
> +      (userData) ? [userData.header, userData.body, userData.encoding, userData.data]:
> +                   [null, null, null, null];

Aligning "?" and ":" as it originally was is clear. Please don't modify it.

[message.header, message.body, message.encoding, message.data] =
  (userData) ? [userData.header, userData.body, userData.encoding, userData.data]
             : [null, null, null, null];

@@ +8573,5 @@
>      // Success Delivery (0) if both Message Status and User Data are absent.
>      // Message Status absent (-1) if only User Data is available.
>      let msgStatus = message[PDU_CDMA_MSG_USER_DATA_MSG_STATUS];
>      [message.errorClass, message.msgStatus] =
> +      (msgStatus) ? [msgStatus.errorClass, msgStatus.msgStatus]:

Ditto.

Comment 12

[Bevis Tseng[:bevis][:btseng](Exited)]

Hi Gene,

Need some minor opinions from you again for these minor change described inline. :)

(In reply to Gene Lian [:gene] (needinfo? encouraged) from comment #11)
> Comment on attachment 8344516 [details] [diff] [review]
> Patch_Part_1_v2: Support Wap Push over CDMA SMS. r=gene
> 
> Review of attachment 8344516 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/system/gonk/ril_worker.js
> @@ +4315,5 @@
> >    /**
> > +   * Helper for processing CDMA SMS WAP Push Message
> > +   *
> > +   * @param message
> > +   *        decoded WAP message from CdmaPDUHelper.
> 
> s/CdmaPDUHelper/CdmaPduHelper

Actually, the variable of |CdmaPDUHelper| originally-defined in ril_worker is named in this way.
I only change the naming of the variable declared in the test case as you suggested and keep it unchanged in ril_worker.js. Otherwise, serveral files will also has to be changed accordingly.

Is it okay for you?

> 
> @@ +8566,5 @@
> >      // Bearer Data Sub-Parameter: User Data
> >      let userData = message[PDU_CDMA_MSG_USERDATA_BODY];
> > +    [message.header, message.body, message.encoding, message.data] =
> > +      (userData) ? [userData.header, userData.body, userData.encoding, userData.data]:
> > +                   [null, null, null, null];
> 
> Aligning "?" and ":" as it originally was is clear. Please don't modify it.
> 
> [message.header, message.body, message.encoding, message.data] =
>   (userData) ? [userData.header, userData.body, userData.encoding,
> userData.data]
>              : [null, null, null, null];
> 
> @@ +8573,5 @@
> >      // Success Delivery (0) if both Message Status and User Data are absent.
> >      // Message Status absent (-1) if only User Data is available.
> >      let msgStatus = message[PDU_CDMA_MSG_USER_DATA_MSG_STATUS];
> >      [message.errorClass, message.msgStatus] =
> > +      (msgStatus) ? [msgStatus.errorClass, msgStatus.msgStatus]:
> 
> Ditto.

Sorry, I aligned these by referring to other cases in the same file.
But I agree with you that the readability is better if we put the ":" in the new line and aligned with "?".
I'll modify it again with your suggestion. :)

Comment 13

[Gene Lian (I already quit Mozilla)]

OK. Please keep the CdmaPDUHelper since ril_worker.js has already used that in this way. :) However, it looks strange to me as you can see in the same name: we use Cdma to represent CDMA but use PDU to represent PDU, which are all abbreviations.

Even if you don't want to put the ":" in the second line, please add spaces around ":" (before it and after it). ;)

Comment 14

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Patch_Part_1_v3: Support Wap Push over CDMA SMS. r=gene

Update Patch_Part_1_v3 to correct the nits in comment#11.

Comment 15

[Bevis Tseng[:bevis][:btseng](Exited)]

update try server test result:
https://tbpl.mozilla.org/?tree=Try&rev=8709c64d145d

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/ef3a260bc2ef
https://hg.mozilla.org/integration/b2g-inbound/rev/66bc0f216f6e

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/ef3a260bc2ef
https://hg.mozilla.org/mozilla-central/rev/66bc0f216f6e