Last Comment Bug 761057 - B2G MMS: support sending M-Send.req PDUs
: B2G MMS: support sending M-Send.req PDUs
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM: Device Interfaces (show other bugs)
: unspecified
: ARM Gonk (Firefox OS)
: -- normal (vote)
: mozilla17
Assigned To: Vicamo Yang [:vicamo][:vyang]
:
Mentors:
Depends on: 775038
Blocks: b2g-mms
  Show dependency treegraph
 
Reported: 2012-06-03 22:20 PDT by Vicamo Yang [:vicamo][:vyang]
Modified: 2012-07-26 05:07 PDT (History)
7 users (show)
vicamo: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
-


Attachments
Part 1: Add required encoders for M-Send.req/M-Send.conf PDU : WIP (84.18 KB, patch)
2012-07-18 05:15 PDT, Vicamo Yang [:vicamo][:vyang]
no flags Details | Diff | Splinter Review
Part 2: send M-Send.req : WIP (9.66 KB, patch)
2012-07-18 05:16 PDT, Vicamo Yang [:vicamo][:vyang]
no flags Details | Diff | Splinter Review
[Debug] DO NOT SUBMIT (5.46 KB, patch)
2012-07-18 05:16 PDT, Vicamo Yang [:vicamo][:vyang]
no flags Details | Diff | Splinter Review
Part 1: disable some v1.4 parameters for compatibility (9.95 KB, patch)
2012-07-25 04:39 PDT, Vicamo Yang [:vicamo][:vyang]
philipp: review+
Details | Diff | Splinter Review
Part 2: Add required encoders for M-Send.req/M-Send.conf PDU (84.42 KB, patch)
2012-07-25 04:52 PDT, Vicamo Yang [:vicamo][:vyang]
philipp: review+
Details | Diff | Splinter Review
Part 3: send M-Send.req (9.45 KB, patch)
2012-07-25 04:56 PDT, Vicamo Yang [:vicamo][:vyang]
philipp: review+
Details | Diff | Splinter Review
[Debug] DO NOT SUBMIT : V2 (2.24 KB, patch)
2012-07-25 04:57 PDT, Vicamo Yang [:vicamo][:vyang]
no flags Details | Diff | Splinter Review
Part 1: disable some v1.4 parameters for compatibility : V2 (9.96 KB, patch)
2012-07-25 21:04 PDT, Vicamo Yang [:vicamo][:vyang]
no flags Details | Diff | Splinter Review
Part 2: Add required encoders for M-Send.req/M-Send.conf PDU : V2 (87.49 KB, patch)
2012-07-25 21:06 PDT, Vicamo Yang [:vicamo][:vyang]
no flags Details | Diff | Splinter Review
Part 3: send M-Send.req : V2 (10.09 KB, patch)
2012-07-25 21:07 PDT, Vicamo Yang [:vicamo][:vyang]
no flags Details | Diff | Splinter Review
[Debug] DO NOT SUBMIT : V3 (2.85 KB, patch)
2012-07-25 21:07 PDT, Vicamo Yang [:vicamo][:vyang]
no flags Details | Diff | Splinter Review

Description Vicamo Yang [:vicamo][:vyang] 2012-06-03 22:20:27 PDT
This is the protocol part of MMS outgoing messages support. It should open an API that accepts a pre-contructed message object, encode it into a M-Send.req PDU, send it out and process returning M-Send.conf message as well.
Comment 1 JP Rosevear [:jpr] 2012-07-10 13:10:54 PDT
MMS is desired to be ready for basecamp, but does not actually block.
Comment 2 Vicamo Yang [:vicamo][:vyang] 2012-07-18 05:15:40 PDT
Created attachment 643324 [details] [diff] [review]
Part 1: Add required encoders for M-Send.req/M-Send.conf PDU : WIP
Comment 3 Vicamo Yang [:vicamo][:vyang] 2012-07-18 05:16:04 PDT
Created attachment 643325 [details] [diff] [review]
Part 2: send M-Send.req : WIP
Comment 4 Vicamo Yang [:vicamo][:vyang] 2012-07-18 05:16:40 PDT
Created attachment 643326 [details] [diff] [review]
[Debug] DO NOT SUBMIT
Comment 5 Vicamo Yang [:vicamo][:vyang] 2012-07-25 04:39:02 PDT
Created attachment 645708 [details] [diff] [review]
Part 1: disable some v1.4 parameters for compatibility
Comment 6 Vicamo Yang [:vicamo][:vyang] 2012-07-25 04:52:46 PDT
Created attachment 645715 [details] [diff] [review]
Part 2: Add required encoders for M-Send.req/M-Send.conf PDU

1. support email addressing model, see bug 761069
2. refactor MmsPduHelper.encodeHeader* and add test cases
3. decoded msg.content is an array, so refactor encoder to accept the same type
4. do not use magic pdu type numbers in test script
Comment 7 Vicamo Yang [:vicamo][:vyang] 2012-07-25 04:56:04 PDT
Created attachment 645716 [details] [diff] [review]
Part 3: send M-Send.req

use msg.content rather than msg.contentStream
Comment 8 Vicamo Yang [:vicamo][:vyang] 2012-07-25 04:57:08 PDT
Created attachment 645718 [details] [diff] [review]
[Debug] DO NOT SUBMIT : V2

updates for changes made in patch 2, 3
Comment 9 Philipp von Weitershausen [:philikon] 2012-07-25 12:52:45 PDT
Comment on attachment 645715 [details] [diff] [review]
Part 2: Add required encoders for M-Send.req/M-Send.conf PDU

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

::: dom/mms/src/ril/MmsPduHelper.jsm
@@ +115,5 @@
> +      throw new WSP.CodeError("Address: invalid value");
> +    }
> +
> +
> +    let [type, addr] = [value.type, value.address];

Sadly I think this creates at least one array object which seems a bit excessive just to save one line of code.

@@ +117,5 @@
> +
> +
> +    let [type, addr] = [value.type, value.address];
> +    let str;
> +    if (type == "email") {

It seems like a `switch` statement would fit better here.

@@ +122,5 @@
> +      if (addr.indexOf("@") > 0) {
> +        str = addr;
> +      }
> +    } else if (type == "num") {
> +      if (addr.match(/^[\+*#]\d+$/)) {

Please define these RegExps globally. You can `const` them or make them lazy getters on the global scope, e.g.:

  function defineLazyRegExp(obj, name, pattern) {
    obj.__defineGetter__(name, function() {
      delete obj[name];
      return obj[name] = new RegExp(pattern);
    });
  }

  defineLazyRegExp(this, "MMS_ADDR_TYPE_NUM", "^[\+*#]\d+$");
  defineLazyRegExp(this, "MMS_ADDR_TYPE_ALPHANUM", "^\w+$");
  etc.
Comment 10 Philipp von Weitershausen [:philikon] 2012-07-25 13:27:13 PDT
Comment on attachment 645716 [details] [diff] [review]
Part 3: send M-Send.req

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

::: dom/mms/src/ril/MmsService.js
@@ +234,5 @@
> +    msg.headers["x-mms-message-type"] = MMS.MMS_PDU_TYPE_SEND_REQ;
> +    if (!msg.headers["x-mms-transaction-id"]) {
> +      // Create an unique transaction id
> +      let uuidgen = Cc["@mozilla.org/uuid-generator;1"]
> +                    .getService(Ci.nsIUUIDGenerator);

Please use a lazy getter for this in the top scope:

  XPCOMUtils.defineLazyServiceGetter(this, gUUIDGenerator, "@mozilla.org/uuid-generator;1", "nsIUUIDGenerator");
Comment 11 Vicamo Yang [:vicamo][:vyang] 2012-07-25 21:04:12 PDT
Created attachment 646009 [details] [diff] [review]
Part 1: disable some v1.4 parameters for compatibility : V2

Update r=philikon only. Since these patches were previously reviewed+, won't set review flag here.
Comment 12 Vicamo Yang [:vicamo][:vyang] 2012-07-25 21:06:04 PDT
Created attachment 646010 [details] [diff] [review]
Part 2: Add required encoders for M-Send.req/M-Send.conf PDU : V2

Accommodate to comment #9. Thanks Philipp!
Comment 13 Vicamo Yang [:vicamo][:vyang] 2012-07-25 21:07:02 PDT
Created attachment 646011 [details] [diff] [review]
Part 3: send M-Send.req : V2

Accommodate to comment #10. Thanks Philipp!
Comment 14 Vicamo Yang [:vicamo][:vyang] 2012-07-25 21:07:55 PDT
Created attachment 646012 [details] [diff] [review]
[Debug] DO NOT SUBMIT : V3

add text/plain for easier debug

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