Vicamo Yang [:vicamo][:vyang] Assignee
	Description • 13 years ago

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

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 4 • 13 years ago

failed to parse X-Wap-Initiator-URI for now

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 5 • 13 years ago

WSP sessionless PDU parser done. Since we'll only need sessionless PDU for now, let's find a way to parse contained MMS message.

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 6 • 13 years ago

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.

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 7 • 13 years ago

This patch parses WSP sessionless Push PDU for MMS initiation process. The message body is left to another MMS message parser.

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 8 • 13 years ago

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.

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 9 • 13 years ago

(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

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 10 • 13 years ago

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"

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 11 • 13 years ago

Address comment 8, comment 10

	Philipp von Weitershausen [:philikon]
	Comment 12 • 13 years ago

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

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 13 • 13 years ago

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

	Philipp von Weitershausen [:philikon]
	Comment 16 • 13 years ago

Hey Vicamo, are these patches ready for review yet?

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 17 • 13 years ago

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

	Philipp von Weitershausen [:philikon]
	Comment 18 • 13 years ago

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

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 19 • 13 years ago

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

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 20 • 13 years ago

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.

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 23 • 13 years ago

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.

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 24 • 13 years ago

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.

	Philipp von Weitershausen [:philikon]
	Comment 25 • 13 years ago

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?

	Philipp von Weitershausen [:philikon]
	Comment 26 • 13 years ago

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.

	Philipp von Weitershausen [:philikon]
	Comment 27 • 13 years ago

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

	Philipp von Weitershausen [:philikon]
	Comment 28 • 13 years ago

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

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 29 • 13 years ago

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

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 30 • 13 years ago

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

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 31 • 13 years ago

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

	Philipp von Weitershausen [:philikon]
	Comment 32 • 13 years ago

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

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 33 • 13 years ago

1) moved to first patch
2) remove customized stream type, use NetUtil.asyncCopy(). Thanks Philipp & Yoshi.
3) add test cases for all decoder elements

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 34 • 13 years ago

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

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 35 • 13 years ago

1) rename `decoder` to `coder` for future encoder support
2) add string utility function for test script

	Philipp von Weitershausen [:philikon]
	Comment 36 • 13 years ago

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.

	Philipp von Weitershausen [:philikon]
	Comment 37 • 13 years ago

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

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 38 • 13 years ago

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

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 39 • 13 years ago

1) address comments #36
2) rewrite test scripts for more easy debug
3) check all javadoc of each function again

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 40 • 13 years ago

address comment #37

	Philipp von Weitershausen [:philikon]
	Comment 41 • 13 years ago

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.

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 42 • 13 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/3dbfcccc583c
https://hg.mozilla.org/integration/mozilla-inbound/rev/a704675ff365

	Marco Bonardo [:mak]
	Comment 43 • 13 years ago

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

	(no longer active)
	Comment 44 • 13 years ago

https://hg.mozilla.org/mozilla-central/rev/3dbfcccc583c
https://hg.mozilla.org/mozilla-central/rev/a704675ff365
https://hg.mozilla.org/mozilla-central/rev/1deabba3bef5
https://hg.mozilla.org/mozilla-central/rev/b7c14c8eec98
https://hg.mozilla.org/mozilla-central/rev/743ebe50e914
https://hg.mozilla.org/mozilla-central/rev/90465173c7e3
https://hg.mozilla.org/mozilla-central/rev/472671930994
https://hg.mozilla.org/mozilla-central/rev/429f20288742
https://hg.mozilla.org/mozilla-central/rev/ab1f122c4b1a
https://hg.mozilla.org/mozilla-central/rev/795990989746

... and backed out:
https://hg.mozilla.org/mozilla-central/rev/677105bb23bf

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 45 • 13 years ago

fix build break in https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=795990989746

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 46 • 13 years ago

add "r=philikon".

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 47 • 13 years ago

Newer try results are availabe in https://tbpl.mozilla.org/?tree=Try&rev=2707fc628fdd

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 48 • 13 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/c65989339787
https://hg.mozilla.org/integration/mozilla-inbound/rev/026af54fa929

	(no longer active)
	Comment 49 • 13 years ago

https://hg.mozilla.org/mozilla-central/rev/c65989339787
https://hg.mozilla.org/mozilla-central/rev/026af54fa929
https://hg.mozilla.org/mozilla-central/rev/71140763abea
https://hg.mozilla.org/mozilla-central/rev/8d6c0457e630
https://hg.mozilla.org/mozilla-central/rev/abb9036641e2
https://hg.mozilla.org/mozilla-central/rev/728c759ed654
https://hg.mozilla.org/mozilla-central/rev/abcc96067a0f
https://hg.mozilla.org/mozilla-central/rev/a0785f8d3a75
https://hg.mozilla.org/mozilla-central/rev/936df980303f
https://hg.mozilla.org/mozilla-central/rev/c6aa41df6d3e