Last Comment Bug 744360 - B2G MMS: Support WAP over SMS
: B2G MMS: Support WAP over SMS
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM: Device Interfaces (show other bugs)
: Trunk
: ARM Gonk (Firefox OS)
: -- normal (vote)
: mozilla15
Assigned To: Vicamo Yang [:vicamo][:vyang]
:
Mentors:
Depends on: 738132
Blocks: b2g-sms b2g-mms 713471 749856 838057 838058
  Show dependency treegraph
 
Reported: 2012-04-11 04:33 PDT by Vicamo Yang [:vicamo][:vyang]
Modified: 2016-03-28 23:57 PDT (History)
16 users (show)
vicamo: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
+


Attachments
WIP (4.00 KB, patch)
2012-04-11 23:16 PDT, Vicamo Yang [:vicamo][:vyang]
no flags Details | Diff | Review
WIP (14.11 KB, patch)
2012-04-12 05:17 PDT, Vicamo Yang [:vicamo][:vyang]
no flags Details | Diff | Review
WIP (51.69 KB, patch)
2012-04-17 03:37 PDT, Vicamo Yang [:vicamo][:vyang]
no flags Details | Diff | Review
WIP (54.03 KB, patch)
2012-04-19 08:49 PDT, Vicamo Yang [:vicamo][:vyang]
no flags Details | Diff | Review
Part 1: parse WSP Push PDU (59.46 KB, patch)
2012-04-20 01:25 PDT, Vicamo Yang [:vicamo][:vyang]
no flags Details | Diff | Review
parse WSP Push PDU headers (65.90 KB, patch)
2012-04-25 02:34 PDT, Vicamo Yang [:vicamo][:vyang]
no flags Details | Diff | Review
parse WSP Push PDU headers (61.41 KB, patch)
2012-04-26 01:43 PDT, Vicamo Yang [:vicamo][:vyang]
no flags Details | Diff | Review
Part 1: create WapPushManager interface (12.16 KB, patch)
2012-05-05 20:55 PDT, Vicamo Yang [:vicamo][:vyang]
no flags Details | Diff | Review
Part 2: parse WSP Push PDU (49.38 KB, patch)
2012-05-05 20:56 PDT, Vicamo Yang [:vicamo][:vyang]
no flags Details | Diff | Review
Part 1: IDL changes (3.22 KB, patch)
2012-05-18 12:35 PDT, Vicamo Yang [:vicamo][:vyang]
no flags Details | Diff | Review
Part 2: initial WapPushManager (12.68 KB, patch)
2012-05-18 12:36 PDT, Vicamo Yang [:vicamo][:vyang]
philipp: feedback+
Details | Diff | Review
Part 3: parse WSP Push PDU (54.61 KB, patch)
2012-05-18 12:37 PDT, Vicamo Yang [:vicamo][:vyang]
philipp: feedback+
Details | Diff | Review
Part 1: add WspPduHelper & test scripts (79.63 KB, patch)
2012-05-28 14:52 PDT, Vicamo Yang [:vicamo][:vyang]
no flags Details | Diff | Review
Part 2: add WapPushManager (6.66 KB, patch)
2012-05-28 14:56 PDT, Vicamo Yang [:vicamo][:vyang]
philipp: review+
Details | Diff | Review
Part 1: add WspPduHelper & test scripts : V2 (80.10 KB, patch)
2012-05-30 02:51 PDT, Vicamo Yang [:vicamo][:vyang]
philipp: review+
Details | Diff | Review
Part 1: add WspPduHelper & test scripts : V3 (84.32 KB, patch)
2012-05-31 15:56 PDT, Vicamo Yang [:vicamo][:vyang]
philipp: review+
Details | Diff | Review
Part 2: add WapPushManager : V2 (7.36 KB, patch)
2012-05-31 15:59 PDT, Vicamo Yang [:vicamo][:vyang]
philipp: review+
Details | Diff | Review
Part 1: add WspPduHelper & test scripts : V4 (84.35 KB, patch)
2012-06-03 19:49 PDT, Vicamo Yang [:vicamo][:vyang]
vicamo: review+
Details | Diff | Review
Part 2: add WapPushManager : V3 (7.37 KB, patch)
2012-06-03 19:51 PDT, Vicamo Yang [:vicamo][:vyang]
vicamo: review+
Details | Diff | Review

Description Vicamo Yang [:vicamo][:vyang] 2012-04-11 04:33:49 PDT
WAP Push over SMS is a common way for service provider to send MMS notifications to MMS clients. The related documentation is available in Open Mobile Alliance.

This task should focus on extracting MMS notifications from WAP Push over SMS. See WAP-259-WDP-20010614-a[1], WAP-230-WSP-20010705-a[2], for detailed information.

References:
[1]: http://www.openmobilealliance.org/tech/affiliates/LicenseAgreement.asp?DocName=/wap/wap-259-wdp-20010614-a.pdf
[2]: http://www.openmobilealliance.org/tech/affiliates/LicenseAgreement.asp?DocName=/wap/wap-230-wsp-20010705-a.pdf
Comment 1 Vicamo Yang [:vicamo][:vyang] 2012-04-11 23:16:43 PDT
Created attachment 614281 [details] [diff] [review]
WIP
Comment 2 Vicamo Yang [:vicamo][:vyang] 2012-04-12 05:17:41 PDT
Created attachment 614339 [details] [diff] [review]
WIP
Comment 3 Vicamo Yang [:vicamo][:vyang] 2012-04-17 03:37:39 PDT
Created attachment 615647 [details] [diff] [review]
WIP
Comment 4 Vicamo Yang [:vicamo][:vyang] 2012-04-19 08:49:30 PDT
Created attachment 616593 [details] [diff] [review]
WIP

failed to parse X-Wap-Initiator-URI for now
Comment 5 Vicamo Yang [:vicamo][:vyang] 2012-04-20 01:25:29 PDT
Created attachment 616901 [details] [diff] [review]
Part 1: parse WSP Push PDU

WSP sessionless PDU parser done. Since we'll only need sessionless PDU for now, let's find a way to parse contained MMS message.
Comment 6 Vicamo Yang [:vicamo][:vyang] 2012-04-20 02:13:08 PDT
Comment on attachment 616901 [details] [diff] [review]
Part 1: parse WSP Push PDU

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

::: dom/system/gonk/RadioInterfaceLayer.js
@@ +586,5 @@
>    radioState: null,
>  
> +  // WapManager
> +
> +  wapManager: null,

what about making WapManager a singleton as RadioInterfaceLayer is?

::: dom/system/gonk/WapManager.jsm
@@ +47,5 @@
> +   * 128 - 255 | It is an encoded 7-bit value; this header has no more data.
> +   *
> +   * @see WAP-259-WDP-20010614-a clause 8.4.1.2
> +   */
> +  skipValue: function skipValue(stream) {

Should address parameters, return value types

@@ +245,5 @@
> +   *
> +   * Or, in numeric, they are: 33,35-59,61,63-90,95,97-122,126
> +   * @see RFC 2396 2. URI Characters and Escape Sequences
> +   */
> +  decodeURIC: function decodeURIC(stream) {

Should address parameters, return value types

@@ +275,5 @@
> +   *        Whether should the text decoding enable LWS.
> +   *
> +   * @return A tuple whose first element is an boolean value indicating a
> +   *         successful parsing or not, and the second element is the decoded
> +   *         value or null in case of a failed parsing.

s/value/string/ for clarity.

@@ +296,5 @@
> +   *        Input octet stream for decoding.
> +   *
> +   * @return A tuple whose first element is an boolean value indicating a
> +   *         successful parsing or not, and the second element is the decoded
> +   *         value or null in case of a failed parsing.

s/value/string/ for clarity.

@@ +331,5 @@
> +   *        Whether should the text decoding enable LWS.
> +   *
> +   * @return A tuple whose first element is an boolean value indicating a
> +   *         successful parsing or not, and the second element is the decoded
> +   *         value or null in case of a failed parsing.

s/value/string/ for clarity.

@@ +353,5 @@
> +   *        Input octet stream for decoding.
> +   *
> +   * @return A tuple whose first element is an boolean value indicating a
> +   *         successful parsing or not, and the second element is the decoded
> +   *         value or null in case of a failed parsing.

s/value/string/ for clarity.

@@ +392,5 @@
> +   *        Input octet stream for decoding.
> +   *
> +   * @return A tuple whose first element is an boolean value indicating a
> +   *         successful parsing or not, and the second element is the decoded
> +   *         value or null in case of a failed parsing.

s/value/array of octets/ for clarity.

@@ +417,5 @@
> +   *        Number of octets in the multi-octet integer.
> +   *
> +   * @return A tuple whose first element is an boolean value indicating a
> +   *         successful parsing or not, and the second element is the decoded
> +   *         value or null in case of a failed parsing.

s/value/array of octets/ for clarity.

@@ +471,5 @@
> +    let cur = stream.offset;
> +    let [success, value] = this.decodeExtensionMedia(stream);
> +    if (!success) {
> +      stream.offset = cur;
> +      return this.decodeShortInteger(stream);

Shall we always return text-based media-type? That will be more convenient for users.

@@ +556,5 @@
> +   *         value or null in case of a failed parsing.
> +   *
> +   * @see WAP-259-WDP-20010614-a clause 8.4.2.3
> +   */
> +  decodeTextValue: function decodeTextValue(stream) {

This function returns two different types. It might be confusing.

@@ +583,5 @@
> +   *         value or null in case of a failed parsing.
> +   *
> +   * @see WAP-259-WDP-20010614-a clause 8.4.2.3
> +   */
> +  decodeIntegerValue: function decodeIntegerValue(stream) {

This function returns two different types because of decodeLongInteger(). However, I don't really known how to safely convert a multi-octet integer.

@@ +682,5 @@
> +   *         value or null in case of a failed parsing.
> +   *
> +   * @see WAP-259-WDP-20010614-a clause 8.4.2.3
> +   */
> +  decodeVersionValue: function decodeVersionValue(stream) {

This function returns two different types. It might be confusing.

@@ +736,5 @@
> +   *        Input octet stream for decoding.
> +   *
> +   * @return A tuple whose first element is an boolean value indicating a
> +   *         successful parsing or not, and the second element is the decoded
> +   *         value or null in case of a failed parsing.

Need info for returned parameter object.

@@ +850,5 @@
> +    return [true, value];
> +  },
> +
> +  /**
> +   * Shift-sequence = (Shift-delimiter Page-identity) | Short-cut-shift-delimiter

Header shift sequence has it's meaning in parsing header fields. How do we take care of it?

@@ +1001,5 @@
> +   *        Input octet stream for decoding.
> +   *
> +   * @see WAP-259-WDP-20010614-a clause 8.4.2.6
> +   */
> +  decodeFieldName: function decodeFieldName(stream) {

This function returns two different types. It might be confusing.

@@ +1027,5 @@
> +   *         value or null in case of a failed parsing.
> +   *
> +   * @see WAP-259-WDP-20010614-a clause 8.4.2.7 and 8.4.2.24
> +   */
> +  _decodeMedia: function _decodeMedia(stream) {

This function returns two different types. It might be confusing.

@@ +1044,5 @@
> +   *        Input octet stream for decoding.
> +   *
> +   * @see WAP-259-WDP-20010614-a clause 8.4.2.7
> +   */
> +  decodeConstrainedMedia: function decodeConstrainedMedia(stream) {

Need information of returned object.

@@ +1074,5 @@
> +
> +    return [true, value];
> +  },
> +
> +  decodeConstrainedCharset: function decodeConstrainedCharset(stream) {

documentation missed.

@@ +1161,5 @@
> +   *        Input octet stream for decoding.
> +   *
> +   * @return A tuple whose first element is an boolean value indicating a
> +   *         successful parsing or not, and the second element is the decoded
> +   *         value or null in case of a failed parsing.

return type

@@ +1229,5 @@
> +   *        Input octet stream for decoding.
> +   *
> +   * @see WAP-259-WDP-20010614-a clause 8.4.2.54
> +   */
> +  decodeApplicationIdValue: function decodeApplicationIdValue(stream) {

Shall we always return text-based app-id? That will be more convenient for users.

@@ +1265,5 @@
> +    }
> +
> +    let headersEnd = stream.offset + headersLen;
> +
> +    let contentType;

can we reuse decodeHeader logic?

@@ +1283,5 @@
> +      msg[header.name] = header.value;
> +    }
> +
> +    stream.offset = headersEnd;
> +    if (stream.offset < stream.data.length) {

Is copying data array a must here?

@@ +1298,5 @@
> +   * @param stream
> +   *        Input octet stream for decoding.
> +   * @param isSessionless
> +   *        Whether the incoming binary stream represents a sessionless WSP PDU
> +   *        or not.

Can this variable be automatically determined by other parameters? For example, we've already known it's a WAP Push, is there any relation between them?

@@ +1351,5 @@
> +    };
> +    names[name] = names[number] = entry;
> +  }
> +
> +  add("Accept",               0x11, 0x00);

Shall I also make "Accept", 0x00 constant values and refer them here? Other modules might also have chances to refer same strings/values.

@@ +1483,5 @@
> +    if (msg.error) {
> +      return;
> +    }
> +
> +    debug("msg: " + JSON.stringify(msg));

need application-id/content-type registration here.

::: dom/system/gonk/wap_consts.js
@@ +16,5 @@
> +const WSP_PDU_TYPE_CONFIRMED_PUSH = 0x07;
> +
> +// WSP Content Type Assignments
> +// @see http://www.wapforum.org/wina
> +const WSP_WELL_KNOWN_CONTENT_TYPES = (function () {

Maybe move this definition into WapManager.jsm as well. We can also add MMS message parser as done in WSP_HEADER_FIELDS.
Comment 7 Vicamo Yang [:vicamo][:vyang] 2012-04-25 02:34:24 PDT
Created attachment 618193 [details] [diff] [review]
parse WSP Push PDU headers

This patch parses WSP sessionless Push PDU for MMS initiation process. The message body is left to another MMS message parser.
Comment 8 Vicamo Yang [:vicamo][:vyang] 2012-04-25 20:31:08 PDT
Comment on attachment 618193 [details] [diff] [review]
parse WSP Push PDU headers

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

::: dom/system/gonk/WapManager.jsm
@@ +20,5 @@
> +
> +const CTLS = 32;
> +const ASCIIS = 128;
> +
> +function OutOfBoundError(message) {

Should add some documentation to this error class. Let others know with sure when will we get this exception.

@@ +27,5 @@
> +}
> +OutOfBoundError.prototype = new Error();
> +OutOfBoundError.prototype.constructor = OutOfBoundError;
> +
> +function DecodeError(message) {

Ditto.

@@ +34,5 @@
> +}
> +DecodeError.prototype = new Error();
> +DecodeError.prototype.constructor = DecodeError;
> +
> +function NotWellKnownEncodingError(message) {

Ditto.

@@ +74,5 @@
> +   */
> +  skipValue: function skipValue(stream) {
> +    let [success, value] = [true, this.decodeOctet(stream)];
> +    if (value <= 31) {
> +      if (value == 31) {

Magic number here. However, throughout the whole WSP documentation, there are multiple appearances of the term "quote" or "quoted". They represent different numbers in different places. In fact, they work as an escape to previous/next notation. The value itself is not that important. Since they're not some globally, publicly referenced constant numbers, I don't have strong motivation to correct them all.

@@ +257,5 @@
> +          || (code == 34) || (code == 40) || (code == 41) // ASCII "()
> +          || (code == 44) || (code == 47)                 // ASCII ,/
> +          || ((code >= 58) && (code <= 64))               // ASCII :;<=>?@
> +          || ((code >= 91) && (code <= 93))               // ASCII [\]
> +          || ((code >= 123) && (code <= 125))) {          // ASCII [\]

I believe there is something wrong in the two same comments.

@@ +329,5 @@
> +        || (code == 61)
> +        || ((code >=63) && (code <= 90))
> +        || (code == 95)
> +        || ((code >=97) && (code <= 122))
> +        || (code == 126)) {

I'd better have comments like `decodeToken`

@@ +356,5 @@
> +   * @see WAP-259-WDP-20010614-a clause 8.4.2.1
> +   */
> +  decodeTextString: function decodeTextString(stream) {
> +    let cur = stream.offset;
> +    let [success, quote] = this._decodeOctetEqValue(stream, 127);

Should call `decodeOctet` directly. Just like what's done in `decodeQuotedString`.

@@ +873,5 @@
> +    }
> +
> +    let cur = stream.offset;
> +    success = false;
> +    if (param.decode) {

Should remove unsupported well known parameters instead.

@@ +1011,5 @@
> +    }
> +
> +    let cur = stream.offset;
> +    success = false;
> +    if (entry.decode) {

Should remove unsupported well known header fields instead.

@@ +1407,5 @@
> +    };
> +    names[name] = names[number] = entry;
> +  }
> +
> +  add("Accept",               0x11, 0x00);

Should remove unsupported well known header fields. We can always add them back from this patch.

@@ +1500,5 @@
> +    params[name] = params[number] = entry;
> +  }
> +
> +  add("Q",                 0x11, 0x00, this.decodeQValue);
> +  add("Charset",           0x11, 0x01);

Same here.

@@ +1605,5 @@
> +      offset: 0,
> +      data: options.data,
> +    };
> +
> +    let msg;

let msg = {};

::: dom/system/gonk/wap_consts.js
@@ +12,5 @@
> +
> +// WSP PDU Type Assignments
> +// @see WAP-259-WDP-20010614-a Appendix A. Assigned Numbers.
> +const WSP_PDU_TYPE_PUSH           = 0x06;
> +const WSP_PDU_TYPE_CONFIRMED_PUSH = 0x07;

Confirmed push is not fully supported. Included only for it shares the same header structure with unconfirmed push. I'll probably remove it on next revision.

@@ +22,5 @@
> +
> +  function add(type, version, number) {
> +    let entry = {
> +      type: type,
> +      version: version,

We don't have versioning support in this patch yet. I'll probably remove others on next revision.

@@ +28,5 @@
> +    };
> +    types[type] = types[number] = entry;
> +  }
> +
> +  // Well Known Values

We need only "application/vnd.wap.mms-message" for now. I'll probably remove others on next revision. They can be added back from this patch any time.

@@ +106,5 @@
> +  add("application/vnd.oma.drm.content",           0x15, 0x49);
> +  add("application/vnd.oma.drm.rights+xml",        0x15, 0x4A);
> +  add("application/vnd.oma.drm.rights+wbxml",      0x15, 0x4B);
> +
> +  // Registered Values

Same here.
Comment 9 Vicamo Yang [:vicamo][:vyang] 2012-04-25 21:00:42 PDT
(In reply to Vicamo Yang [:vicamo] from comment #8)
> We need only "application/vnd.wap.mms-message" for now. I'll probably remove
> others on next revision. They can be added back from this patch any time.

Device management refers following Content-Types[1][2][3]:

  add("application/vnd.syncml.dm+wbxml", 0x15, 0x42);
  add("application/vnd.syncml.dm+xml",   0x15, 0x43);
  add("",                                0x00, 0x58);

And following Application-IDs[1][4]:

  add("x-wap-application:syncml.dm", 0x07);

[1]: http://www.openmobilealliance.org/Technical/release_program/docs/CopyrightClick.aspx?pck=DM&file=V1_3-20120306-C/OMA-TS-DM_PushBinding-V1_3-20120306-C.pdf clause 5.1.1 and 5.1.2
[2]: http://www.wapforum.org/wina/wsp-content-type.htm
[3]: http://www.openmobilealliance.org/Technical/release_program/docs/CopyrightClick.aspx?pck=DM&file=V1_3-20120306-C/OMA-TS-DM_Notification-V1_3-20120306-C.pdf clause 7.1
[4]: http://www.openmobilealliance.org/tech/omna/omna-push-app-id.aspx
Comment 10 Vicamo Yang [:vicamo][:vyang] 2012-04-25 23:23:54 PDT
Comment on attachment 618193 [details] [diff] [review]
parse WSP Push PDU headers

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

::: dom/system/gonk/WapManager.jsm
@@ +81,5 @@
> +
> +      if (success) {
> +        // `value` can be larger than 30, max length of a multi-octet integer
> +        // here. So we must decode it as an Uint8Array instead.
> +        let array = new Uint8Array(value);

Array length `value` could be zero here.

@@ +89,5 @@
> +        value = array;
> +      }
> +    } else if (value <= 127) {
> +      [success, value] = this._decodeNullTerminatedTexts(stream, true);
> +    }

"128 - 255 | It is an encoded 7-bit value"
Comment 11 Vicamo Yang [:vicamo][:vyang] 2012-04-26 01:43:50 PDT
Created attachment 618586 [details] [diff] [review]
parse WSP Push PDU headers

Address comment 8, comment 10
Comment 12 Philipp von Weitershausen [:philikon] 2012-04-27 16:00:22 PDT
Comment on attachment 618586 [details] [diff] [review]
parse WSP Push PDU headers

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

Great start!

I'm not sure I like the 2-tuple return value from all the `decode*()` functions. Given the nested and iterative nature of the decoding, this is *a lot* of objects we're creating. Until we get generation GC, this means a lot of pressure on the GC, resulting in frequent mainthread pauses.

If I understand this correctly, the first value in the tuple indicates success, but we mostly seem to bail out if the operation in request wasn't successful. Perhaps instead of returning `[success, value]` we just return `value` and throw an exception if we weren't successful? Another option would be to define a sentinel value, e.g. const NOT_SUCCESSFUL = {} and then check `value` for identity with that sentinel value. Yet another option would be to return `success` and attach `value` to a stack array that we pass around. What do you think?

::: dom/system/gonk/RadioInterfaceLayer.js
@@ +47,5 @@
>  var RIL = {};
>  Cu.import("resource://gre/modules/ril_consts.js", RIL);
> +var WAP = {};
> +Cu.import("resource://gre/modules/wap_consts.js", WAP);
> +Cu.import("resource://gre/modules/WapManager.jsm");

I think we can just inline wap_consts.js into WapManager.jsm. The situation here is a bit different than with ril_consts.js (which are needed on both the worker and the main thread.)

::: dom/system/gonk/WapManager.jsm
@@ +1395,5 @@
> +      if (!success) {
> +        break;
> +      }
> +      if (header) {
> +        msg.headers[header.name] = header.value;

RFC2822 headers should be treated as case-insensitive (http://tools.ietf.org/html/rfc2822#section-1.2.2). The best way to do that here probably is to do case-smashing (header.name.toLowerCase()) and then always use lower case names everywhere we explicitly refer to a header name in the code.

@@ +1522,5 @@
> +  //add("X-WAP-Security",       0x46);
> +  //add("Cache-Control",        0x47);
> +
> +  return names;
> +}).call(WspPduParser);

Why the `.call(WspPduParser)`? You don't use `this` in the function body, and even if you did, you'd be referring to the constructor function...

@@ +1687,5 @@
> +  wdp: null,
> +  wsp: null,
> +
> +  processDatagram: function processDatagram(options) {
> +    if (options.bearer != null) {

Bail out early?

  if (options.bearer == null) {
    return;
  }
  switch (...) ...
Comment 13 Vicamo Yang [:vicamo][:vyang] 2012-04-28 23:52:35 PDT
(In reply to Philipp von Weitershausen [:philikon] from comment #12)
> Comment on attachment 618586 [details] [diff] [review]
> parse WSP Push PDU headers
> 
> Review of attachment 618586 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Great start!
> 
> I'm not sure I like the 2-tuple return value from all the `decode*()`
> functions. Given the nested and iterative nature of the decoding, this is *a
> lot* of objects we're creating. Until we get generation GC, this means a lot
> of pressure on the GC, resulting in frequent mainthread pauses.
> 
> If I understand this correctly, the first value in the tuple indicates
> success, but we mostly seem to bail out if the operation in request wasn't
> successful. Perhaps instead of returning `[success, value]` we just return
> `value` and throw an exception if we weren't successful? Another option
> would be to define a sentinel value, e.g. const NOT_SUCCESSFUL = {} and then
> check `value` for identity with that sentinel value. Yet another option
> would be to return `success` and attach `value` to a stack array that we
> pass around. What do you think?

Hi Philipp,

You can see my first implementation is in try..catch form. However, throwing an exception in this one-function-per-rule design won't save GC from those temporary objects. Exceptions are also objects, aren't they? The second and third way seem better alternatives. Among them, the third one allows easier conditional statement. I'll rewrite those decode* functions to follow this suggestion.

> ::: dom/system/gonk/RadioInterfaceLayer.js
> @@ +47,5 @@
> >  var RIL = {};
> >  Cu.import("resource://gre/modules/ril_consts.js", RIL);
> > +var WAP = {};
> > +Cu.import("resource://gre/modules/wap_consts.js", WAP);
> > +Cu.import("resource://gre/modules/WapManager.jsm");
> 
> I think we can just inline wap_consts.js into WapManager.jsm. The situation
> here is a bit different than with ril_consts.js (which are needed on both
> the worker and the main thread.)

OK.

> ::: dom/system/gonk/WapManager.jsm
> @@ +1395,5 @@
> > +      if (!success) {
> > +        break;
> > +      }
> > +      if (header) {
> > +        msg.headers[header.name] = header.value;
> 
> RFC2822 headers should be treated as case-insensitive
> (http://tools.ietf.org/html/rfc2822#section-1.2.2). The best way to do that
> here probably is to do case-smashing (header.name.toLowerCase()) and then
> always use lower case names everywhere we explicitly refer to a header name
> in the code.

Sweet!

> @@ +1522,5 @@
> > +  //add("X-WAP-Security",       0x46);
> > +  //add("Cache-Control",        0x47);
> > +
> > +  return names;
> > +}).call(WspPduParser);
> 
> Why the `.call(WspPduParser)`? You don't use `this` in the function body,
> and even if you did, you'd be referring to the constructor function...

Well, that's because it was originally a instance name, not class name. I'll fix it.

> @@ +1687,5 @@
> > +  wdp: null,
> > +  wsp: null,
> > +
> > +  processDatagram: function processDatagram(options) {
> > +    if (options.bearer != null) {
> 
> Bail out early?
> 
>   if (options.bearer == null) {
>     return;
>   }
>   switch (...) ...

Ok.

By the way, I cancelled the review request and am working on creating formal interfaces for application registration and some refactoring. I'll try to return with a better looking, well formed one.
Comment 14 Vicamo Yang [:vicamo][:vyang] 2012-05-05 20:55:35 PDT
Created attachment 621382 [details] [diff] [review]
Part 1: create WapPushManager interface
Comment 15 Vicamo Yang [:vicamo][:vyang] 2012-05-05 20:56:05 PDT
Created attachment 621384 [details] [diff] [review]
Part 2: parse WSP Push PDU
Comment 16 Philipp von Weitershausen [:philikon] 2012-05-17 18:23:47 PDT
Hey Vicamo, are these patches ready for review yet?
Comment 17 Vicamo Yang [:vicamo][:vyang] 2012-05-17 18:30:45 PDT
(In reply to Philipp von Weitershausen [:philikon] from comment #16)
> Hey Vicamo, are these patches ready for review yet?

No. Since work week had ended last week, maybe we should go directly toward integrating SMS/MMS together? This will affect where should I place new files, dom/sms or dom/mms or dom/message. I had a merging proposal on webapi mailing list. Could you help reviewing it and give some feedback/opinions?
Comment 18 Philipp von Weitershausen [:philikon] 2012-05-17 18:38:34 PDT
(In reply to Vicamo Yang [:vicamo] from comment #17)
> (In reply to Philipp von Weitershausen [:philikon] from comment #16)
> > Hey Vicamo, are these patches ready for review yet?
> 
> No. Since work week had ended last week, maybe we should go directly toward
> integrating SMS/MMS together?

I'm not sure if there's a causality here. I would also like to recall our discussion at the work week where we said we could first go ahead with a separate MMS API prototype to get the feature off the ground and then integrate later. I'm not aware that this stategy has changed.

> This will affect where should I place new files, dom/sms or dom/mms or dom/message.

As I pointed out at the work week, dom/mms seems like an good place. It's likely only going to be temporary anyway.

> I had a merging proposal on webapi
> mailing list. Could you help reviewing it and give some feedback/opinions?

Thanks, I'll do that. But please focus on getting your work through the review pipeline. The only way we will be able to manage a big feature like MMS is in chunks and iteratively. There's no way we can get everything right just by planning and discussing on mailinglists. Thanks!
Comment 19 Vicamo Yang [:vicamo][:vyang] 2012-05-17 18:58:43 PDT
(In reply to Philipp von Weitershausen [:philikon] from comment #18)
> (In reply to Vicamo Yang [:vicamo] from comment #17)
> > (In reply to Philipp von Weitershausen [:philikon] from comment #16)
> > > Hey Vicamo, are these patches ready for review yet?
> > 
> > No. Since work week had ended last week, maybe we should go directly toward
> > integrating SMS/MMS together?
> 
> I'm not sure if there's a causality here. I would also like to recall our
> discussion at the work week where we said we could first go ahead with a
> separate MMS API prototype to get the feature off the ground and then
> integrate later. I'm not aware that this stategy has changed.
> 
> > This will affect where should I place new files, dom/sms or dom/mms or dom/message.
> 
> As I pointed out at the work week, dom/mms seems like an good place. It's
> likely only going to be temporary anyway.

I thought it was a temporary target and was only valid during the work week. And, like what we agreed here, dom/mms might last only for a few weeks. I don't know whether should I create such a short-lived folder at all. But anyway, since the target is re-clarified, I'll update current works and get it through the review process asap.


> > I had a merging proposal on webapi
> > mailing list. Could you help reviewing it and give some feedback/opinions?
> 
> Thanks, I'll do that. But please focus on getting your work through the
> review pipeline. The only way we will be able to manage a big feature like
> MMS is in chunks and iteratively. There's no way we can get everything right
> just by planning and discussing on mailinglists. Thanks!
Comment 20 Vicamo Yang [:vicamo][:vyang] 2012-05-18 12:35:37 PDT
Created attachment 625198 [details] [diff] [review]
Part 1: IDL changes

I'd like to replace WapBinaryInputStream with BinaryInputStream, but had problem in seeking it. There seems no easy way to create a BIS, so I must create an StorageStream first. However, StorageStream doesn't seem to support nsISeekableStream(?) I mark this as feedback for now.
Comment 21 Vicamo Yang [:vicamo][:vyang] 2012-05-18 12:36:35 PDT
Created attachment 625199 [details] [diff] [review]
Part 2: initial WapPushManager
Comment 22 Vicamo Yang [:vicamo][:vyang] 2012-05-18 12:37:24 PDT
Created attachment 625200 [details] [diff] [review]
Part 3: parse WSP Push PDU
Comment 23 Vicamo Yang [:vicamo][:vyang] 2012-05-18 12:49:49 PDT
Comment on attachment 625199 [details] [diff] [review]
Part 2: initial WapPushManager

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

::: b2g/installer/package-manifest.in
@@ +172,5 @@
>  @BINPATH@/components/dom_range.xpt
>  @BINPATH@/components/dom_settings.xpt
>  @BINPATH@/components/dom_sidebar.xpt
>  @BINPATH@/components/dom_sms.xpt
> +#ifdef MOZ_B2G_RIL

Shall I create fallback implementation and remove conditional build here?

::: dom/mms/src/ril/WapBinaryInputStream.js
@@ +14,5 @@
> +
> +/**
> + * WapBinaryInputStream
> + */
> +function WapBinaryInputStream() {

I only need a random accessible array-liked object for WSP/MMS PDU parsing. Shall I add this interface at all?

::: dom/system/gonk/RadioInterfaceLayer.js
@@ +198,5 @@
>    Services.obs.addObserver(this, kMozSettingsChangedObserverTopic, false);
>  
>    this._sentSmsEnvelopes = {};
>    this.portAddressedSmsApps = {};
> +  this.portAddressedSmsApps[Ci.nsIWdpServiceAccessPoint.PORT_PUSH] = (function (msg) {

maybe I should add an interface to nsIRadioInterfaceLayer instead.
Comment 24 Vicamo Yang [:vicamo][:vyang] 2012-05-18 13:05:00 PDT
Comment on attachment 625200 [details] [diff] [review]
Part 3: parse WSP Push PDU

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

::: dom/mms/src/ril/WspPduHelper.jsm
@@ +1408,5 @@
> +        headers["content-type"] = contentType;
> +        headers["content-length"] = contentLen;
> +
> +	//let blob = new Blob(null, {type: contentType.media});
> +	//blob.append(stream.readByteArray(contentLen));

Blob creation here fails with "Blob not defined" error, don't know why.
Comment 25 Philipp von Weitershausen [:philikon] 2012-05-23 17:17:30 PDT
Comment on attachment 625198 [details] [diff] [review]
Part 1: IDL changes

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

I may have missed this in the previous patches, but I'm not quite sure why this is needed. Why is a Uint8Array not sufficient?
Comment 26 Philipp von Weitershausen [:philikon] 2012-05-23 17:30:14 PDT
Comment on attachment 625199 [details] [diff] [review]
Part 2: initial WapPushManager

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

This looks good apart from the already discussed WapBinaryInputStream.

::: dom/system/gonk/RadioInterfaceLayer.js
@@ +98,5 @@
>                                     "nsIFrameMessageManager");
>  
> +XPCOMUtils.defineLazyServiceGetter(this, "gWapPushManager",
> +                                   "@mozilla.org/wap/rilwappushmanager;1",
> +                                   "nsIWapPushManager");

Is there a reason why this is an XPCOM service? It's much more efficient to just have this available a JS object from a JSM.
Comment 27 Philipp von Weitershausen [:philikon] 2012-05-23 18:03:07 PDT
(In reply to Vicamo Yang [:vicamo] from comment #23)
> ::: b2g/installer/package-manifest.in
> @@ +172,5 @@
> >  @BINPATH@/components/dom_range.xpt
> >  @BINPATH@/components/dom_settings.xpt
> >  @BINPATH@/components/dom_sidebar.xpt
> >  @BINPATH@/components/dom_sms.xpt
> > +#ifdef MOZ_B2G_RIL
> 
> Shall I create fallback implementation and remove conditional build here?

No, this is fine since it's just an initial prototype.

> > +/**
> > + * WapBinaryInputStream
> > + */
> > +function WapBinaryInputStream() {
> 
> I only need a random accessible array-liked object for WSP/MMS PDU parsing.
> Shall I add this interface at all?

If you indeed only need random array-like access, a Uint8Array should be perfectly fine. So my answer would be: let's not add this.

> ::: dom/system/gonk/RadioInterfaceLayer.js
> @@ +198,5 @@
> >    Services.obs.addObserver(this, kMozSettingsChangedObserverTopic, false);
> >  
> >    this._sentSmsEnvelopes = {};
> >    this.portAddressedSmsApps = {};
> > +  this.portAddressedSmsApps[Ci.nsIWdpServiceAccessPoint.PORT_PUSH] = (function (msg) {
> 
> maybe I should add an interface to nsIRadioInterfaceLayer instead.

Not sure what you mean. Can you elaborate?
Comment 28 Philipp von Weitershausen [:philikon] 2012-05-23 18:57:41 PDT
Comment on attachment 625200 [details] [diff] [review]
Part 3: parse WSP Push PDU

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

Excellent work! With the question of how to handle the binary data (Uint8Array vs the WapBinaryInputStream) and the blob (see below) still up in the air, I'm not going to r+ this just yet.

::: dom/mms/src/ril/WapPushManager.js
@@ +64,1 @@
>    },

Where and how is this used?

::: dom/mms/src/ril/WspPduHelper.jsm
@@ +1408,5 @@
> +        headers["content-type"] = contentType;
> +        headers["content-length"] = contentLen;
> +
> +	//let blob = new Blob(null, {type: contentType.media});
> +	//blob.append(stream.readByteArray(contentLen));

We don't expose `Blob` or `File` to XPCOM JS or JSMs. Only to `window` contexts, AFAIK. In any case, we would have to transmit this data to the content process and we don't want to serialize and deserialize it in memory.

I think the best course of action here would be to write the blob data to disk (sufficiently random filename in a directory below the profile, e.g. <ProfD>/mms/<uuid>) and then pass just the filename to the content process. Here's an example of how to do that:

    let file = FileUtils.getFile("ProfD", ["mms", uuid], true);
    let outputStream = FileUtils.openSafeFileOutputStream(file);
    NetUtil.asyncCopy(inputStream, outputStream, function (result) {
      if (!Components.isSuccessCode(result)) {
        // error handling
        return;
      }
      // ...
    });

The only disappointment here is that asyncCopy needs an inputStream, so you will have to either have convert your Uint8Array to an nsIInpitStream via StorageStream, or just work with a StorageStream from the beginning.

In the content process, we can then convert this to a File object (File inherits from Blob):

  let file = FileUtils.getFile("ProfD", ["mms", uuid], true);
  let blob = window.File(file);
Comment 29 Vicamo Yang [:vicamo][:vyang] 2012-05-24 11:00:06 PDT
(In reply to Philipp von Weitershausen [:philikon] from comment #26)
> > +XPCOMUtils.defineLazyServiceGetter(this, "gWapPushManager",
> > +                                   "@mozilla.org/wap/rilwappushmanager;1",
> > +                                   "nsIWapPushManager");
> 
> Is there a reason why this is an XPCOM service? It's much more efficient to
> just have this available a JS object from a JSM.

I think we may have multiple functions depending on WAP Push in the future, so I made it a independent service. Currently, MmsService implements nsIWapPushApplication and registers itself to WapPushManager in constructor. The WapPushManager delivers WAP Push messages by looking up application-id carried by the message in its own registered applications hash map. So MmsService must be instantiated before WapPushManager can ever deliver any message to it. That's why I had a MmsService lazy getter in RadioInterfaceLayer. WapPushManager doesn't have this design and is directly referenced in RadioInterfaceLayer. I wanted to create an interface between WapPushManager and RadioInterfaceLayer, although I want this to be done in WebIntent or similar broadcast techniques more.

Refactor all these ugly workarounds to a more simpler way and refine it when we get better way is a good idea. I'll do that.
Comment 30 Vicamo Yang [:vicamo][:vyang] 2012-05-24 11:03:37 PDT
(In reply to Philipp von Weitershausen [:philikon] from comment #27)
> (In reply to Vicamo Yang [:vicamo] from comment #23)
> > Shall I create fallback implementation and remove conditional build here?
> 
> No, this is fine since it's just an initial prototype.
> 

Ok. Thanks.

> > I only need a random accessible array-liked object for WSP/MMS PDU parsing.
> > Shall I add this interface at all?
> 
> If you indeed only need random array-like access, a Uint8Array should be
> perfectly fine. So my answer would be: let's not add this.
> 

Removed. I also finished test scripts for all elements in WspPduHelper today.

> > maybe I should add an interface to nsIRadioInterfaceLayer instead.
> 
> Not sure what you mean. Can you elaborate?

See my comment #29.
Comment 31 Vicamo Yang [:vicamo][:vyang] 2012-05-24 11:11:37 PDT
(In reply to Philipp von Weitershausen [:philikon] from comment #28)
> Where and how is this used?

See my comment #29.
 
> ::: dom/mms/src/ril/WspPduHelper.jsm
> @@ +1408,5 @@
> > +        headers["content-type"] = contentType;
> > +        headers["content-length"] = contentLen;
> > +
> > +	//let blob = new Blob(null, {type: contentType.media});
> > +	//blob.append(stream.readByteArray(contentLen));
> 
> We don't expose `Blob` or `File` to XPCOM JS or JSMs. Only to `window`
> contexts, AFAIK. In any case, we would have to transmit this data to the
> content process and we don't want to serialize and deserialize it in memory.
> 
> I think the best course of action here would be to write the blob data to
> disk (sufficiently random filename in a directory below the profile, e.g.
> <ProfD>/mms/<uuid>) and then pass just the filename to the content process.
> Here's an example of how to do that:
> 
>     let file = FileUtils.getFile("ProfD", ["mms", uuid], true);
>     let outputStream = FileUtils.openSafeFileOutputStream(file);
>     NetUtil.asyncCopy(inputStream, outputStream, function (result) {
>       if (!Components.isSuccessCode(result)) {
>         // error handling
>         return;
>       }
>       // ...
>     });
> 
> The only disappointment here is that asyncCopy needs an inputStream, so you
> will have to either have convert your Uint8Array to an nsIInpitStream via
> StorageStream, or just work with a StorageStream from the beginning.
> 
> In the content process, we can then convert this to a File object (File
> inherits from Blob):
> 
>   let file = FileUtils.getFile("ProfD", ["mms", uuid], true);
>   let blob = window.File(file);

I had asked Thinker a few days ago. I'm not pretty sure whether this is a policy or simply an can-do enhancement. Expose Blob/File into XPCOM js/jsm can be very convenient. For now, I'll take your suggestion, but maybe slightly modify it by replacing uuid with per message transaction-id, which also acts as an unique identity of each MMS message.
Comment 32 Philipp von Weitershausen [:philikon] 2012-05-24 11:37:03 PDT
(In reply to Vicamo Yang [:vicamo] from comment #29)
> I wanted to create an interface between
> WapPushManager and RadioInterfaceLayer, although I want this to be done in
> WebIntent or similar broadcast techniques more.

I think you're overthinking this a bit. ;) We could use the nsIObserverService, an XPCOM service lookup, or whatever, but realistically, a simple JS method call will do for now and is also the most efficient. XPCOM does argument marshalling and demarshalling.

(In reply to Vicamo Yang [:vicamo] from comment #31)
> I had asked Thinker a few days ago. I'm not pretty sure whether this is a
> policy or simply an can-do enhancement. Expose Blob/File into XPCOM js/jsm
> can be very convenient. For now, I'll take your suggestion, but maybe
> slightly modify it by replacing uuid with per message transaction-id, which
> also acts as an unique identity of each MMS message.

Yes, that sounds good. And I agree, it would be useful to expose Blob and File to XPCOM JS and JSM contexts. But in our case, it would not help us much because we can't pass a blob or file from the chrome to the content process.
Comment 33 Vicamo Yang [:vicamo][:vyang] 2012-05-28 14:52:28 PDT
Created attachment 627795 [details] [diff] [review]
Part 1: add WspPduHelper & test scripts

1) moved to first patch
2) remove customized stream type, use NetUtil.asyncCopy(). Thanks Philipp & Yoshi.
3) add test cases for all decoder elements
Comment 34 Vicamo Yang [:vicamo][:vyang] 2012-05-28 14:56:39 PDT
Created attachment 627796 [details] [diff] [review]
Part 2: add WapPushManager

1) moved to second patch to keep all manager code in the same patch.
2) obsolete all XPCOM interfaces, let it be a simple jsm.
3) export constan variables from wap_consts.js as well
Comment 35 Vicamo Yang [:vicamo][:vyang] 2012-05-30 02:51:51 PDT
Created attachment 628284 [details] [diff] [review]
Part 1: add WspPduHelper & test scripts : V2

1) rename `decoder` to `coder` for future encoder support
2) add string utility function for test script
Comment 36 Philipp von Weitershausen [:philikon] 2012-05-30 13:32:47 PDT
Comment on attachment 628284 [details] [diff] [review]
Part 1: add WspPduHelper & test scripts : V2

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

Awesome. One nit below.

::: b2g/installer/package-manifest.in
@@ +179,5 @@
>  @BINPATH@/components/dom_sidebar.xpt
>  @BINPATH@/components/dom_sms.xpt
> +#ifdef MOZ_B2G_RIL
> +@BINPATH@/components/dom_mms.xpt
> +#endif

This patch adds no IDL files, so this isn't needed. At least not in this patch.

::: browser/installer/package-manifest.in
@@ +184,5 @@
>  @BINPATH@/components/dom_sidebar.xpt
>  @BINPATH@/components/dom_sms.xpt
> +#ifdef MOZ_B2G_RIL
> +@BINPATH@/components/dom_mms.xpt
> +#endif

Ditto.
Comment 37 Philipp von Weitershausen [:philikon] 2012-05-30 13:34:42 PDT
Comment on attachment 627796 [details] [diff] [review]
Part 2: add WapPushManager

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

Nicely done as usual. Just a few suggestions below to make the code simpler and more efficient. r=me with those

::: dom/mms/src/ril/WapPushManager.js
@@ +13,5 @@
> +
> +// Copy constant symbols.
> +WSP.ALL_CONST_SYMBOLS.forEach(function (name) {
> +  if (name != "EXPORTED_SYMBOLS") {
> +    Object.defineProperty(GLOBAL_SCOPE, name, {value: WSP[name]});

Why not

  GLOBAL_SCOPE[name] = WSP[name];

though what harm would it be if we just imported all of WspPduHelper.jsm into the global scope?

::: dom/system/gonk/RadioInterfaceLayer.js
@@ +12,5 @@
>  var RIL = {};
>  Cu.import("resource://gre/modules/ril_consts.js", RIL);
>  
> +var WAP = {};
> +Cu.import("resource://gre/modules/WapPushManager.js", WAP);

I think this code is only needed when we receive an MMS, right? So I propose we load it lazily:

  XPCOMUtils.defineLazyGetter(this, "WAP", function () {
    let WAP = {};
    Cu.import("resource://gre/modules/WapPushManager.js", WAP);
    return WAP;
  });

Saves us a bit of start up time and memory footprint until the MMS feature is used (which may never happen for some users).

@@ +178,2 @@
>    this.portAddressedSmsApps = {};
> +  this.portAddressedSmsApps[WAP.WDP_PORT_PUSH] = (function (message) {

Can we make this a named method in the prototype and then just do

  this.portAddressedSmsApps[WAP.WDP_PORT_PUSH] = this.handleWdpPortPush.bind(this);

(or whatever you choose to name the method.)

@@ +585,5 @@
> +      let u8array = new Uint8Array(message.fullData.length);
> +      for (let i = 0; i < message.fullData.length; i++) {
> +        u8array[i] = message.fullData[i];
> +      }
> +      message.fullData = u8array;

I'm pretty sure you can just do

  message.fullData = Uint8Array(message.fullData);
Comment 38 Vicamo Yang [:vicamo][:vyang] 2012-05-30 22:34:29 PDT
(In reply to Philipp von Weitershausen [:philikon] from comment #36)
> Comment on attachment 628284 [details] [diff] [review]
> Part 1: add WspPduHelper & test scripts : V2
> ::: b2g/installer/package-manifest.in
> @@ +179,5 @@
> >  @BINPATH@/components/dom_sidebar.xpt
> >  @BINPATH@/components/dom_sms.xpt
> > +#ifdef MOZ_B2G_RIL
> > +@BINPATH@/components/dom_mms.xpt
> > +#endif
> 
> This patch adds no IDL files, so this isn't needed. At least not in this
> patch.

Yes, I'll move them to 3rd patch of bug 749856. Also removes dom-config.mk changes/references for we don't have native code at all.
Comment 39 Vicamo Yang [:vicamo][:vyang] 2012-05-31 15:56:50 PDT
Created attachment 628934 [details] [diff] [review]
Part 1: add WspPduHelper & test scripts : V3

1) address comments #36
2) rewrite test scripts for more easy debug
3) check all javadoc of each function again
Comment 40 Vicamo Yang [:vicamo][:vyang] 2012-05-31 15:59:11 PDT
Created attachment 628936 [details] [diff] [review]
Part 2: add WapPushManager : V2

address comment #37
Comment 41 Philipp von Weitershausen [:philikon] 2012-05-31 16:13:11 PDT
Comment on attachment 628936 [details] [diff] [review]
Part 2: add WapPushManager : V2

Thanks! I already r+ed the previous iteration... No need to ask for review again.
Comment 43 Marco Bonardo [::mak] 2012-06-01 03:00:51 PDT
Bakced out cause this caused a failure in xpcshell

TEST-UNEXPECTED-FAIL | /home/cltbld/talos-slave/test/build/xpcshell/tests/dom/mms/tests/test_wsp_pdu_helper.js | test failed (with xpcshell return code: 3), see following log:
>>>>>>>
uncaught exception: Error opening input stream (invalid filename?)
<<<<<<<
Comment 45 Vicamo Yang [:vicamo][:vyang] 2012-06-03 19:49:22 PDT
Created attachment 629681 [details] [diff] [review]
Part 1: add WspPduHelper & test scripts : V4

fix build break in https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=795990989746
Comment 46 Vicamo Yang [:vicamo][:vyang] 2012-06-03 19:51:11 PDT
Created attachment 629682 [details] [diff] [review]
Part 2: add WapPushManager : V3

add "r=philikon".
Comment 47 Vicamo Yang [:vicamo][:vyang] 2012-06-03 19:55:48 PDT
Newer try results are availabe in https://tbpl.mozilla.org/?tree=Try&rev=2707fc628fdd

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