Last Comment Bug 749856 - implement backend for dispatching OMNA WAP Push notification
: implement backend for dispatching OMNA WAP Push notification
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: 744360
Blocks: b2g-mms
  Show dependency treegraph
 
Reported: 2012-04-27 17:53 PDT by Andreas Gal :gal
Modified: 2012-06-04 22:32 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 MmsService (5.32 KB, patch)
2012-05-05 20:56 PDT, Vicamo Yang [:vicamo][:vyang]
no flags Details | Diff | Splinter Review
Part 2: parse MMS notification PDU (23.61 KB, patch)
2012-05-05 20:57 PDT, Vicamo Yang [:vicamo][:vyang]
no flags Details | Diff | Splinter Review
Part 0: IDL changes (704 bytes, patch)
2012-05-18 12:38 PDT, Vicamo Yang [:vicamo][:vyang]
philipp: review+
Details | Diff | Splinter Review
Part 1: add MmsService (6.59 KB, patch)
2012-05-18 12:38 PDT, Vicamo Yang [:vicamo][:vyang]
philipp: review+
Details | Diff | Splinter Review
Part 2: add MMS PDU Helper (28.29 KB, patch)
2012-05-18 12:39 PDT, Vicamo Yang [:vicamo][:vyang]
philipp: review+
Details | Diff | Splinter Review
Part 3: handle WAP Push notification (7.77 KB, patch)
2012-05-18 12:40 PDT, Vicamo Yang [:vicamo][:vyang]
philipp: feedback+
Details | Diff | Splinter Review
Part 1: IDL changes : V2 (704 bytes, patch)
2012-05-30 02:57 PDT, Vicamo Yang [:vicamo][:vyang]
philipp: review+
Details | Diff | Splinter Review
Part 2: add MMS PDU Helper : V2 (38.17 KB, patch)
2012-05-30 03:05 PDT, Vicamo Yang [:vicamo][:vyang]
philipp: review+
Details | Diff | Splinter Review
Part 3: handle WAP Push notification : V2 (20.45 KB, patch)
2012-05-30 03:16 PDT, Vicamo Yang [:vicamo][:vyang]
philipp: review+
Details | Diff | Splinter Review
Part 4: add WSP/MMS encoding support (13.10 KB, patch)
2012-05-30 03:17 PDT, Vicamo Yang [:vicamo][:vyang]
philipp: review+
Details | Diff | Splinter Review
Part 5: send M-NotifyResp.ind response back (6.72 KB, patch)
2012-05-30 03:18 PDT, Vicamo Yang [:vicamo][:vyang]
philipp: review+
Details | Diff | Splinter Review
Part 2: add MMS PDU Helper : V3 (49.23 KB, patch)
2012-05-31 16:04 PDT, Vicamo Yang [:vicamo][:vyang]
philipp: review+
Details | Diff | Splinter Review
Part 3: handle WAP Push notification : V3 (17.27 KB, patch)
2012-05-31 16:08 PDT, Vicamo Yang [:vicamo][:vyang]
philipp: review+
Details | Diff | Splinter Review
Part 4: handle M-Retrieve.conf PDU (5.73 KB, patch)
2012-05-31 16:09 PDT, Vicamo Yang [:vicamo][:vyang]
philipp: review+
Details | Diff | Splinter Review
Part 5: save message attachments (5.18 KB, patch)
2012-05-31 16:10 PDT, Vicamo Yang [:vicamo][:vyang]
philipp: review+
Details | Diff | Splinter Review
Part 6: add WSP/MMS encoding support : V2 (30.50 KB, patch)
2012-05-31 16:13 PDT, Vicamo Yang [:vicamo][:vyang]
philipp: review+
Details | Diff | Splinter Review
Part 7: send M-NotifyResp.ind response back : V2 (6.96 KB, patch)
2012-05-31 16:17 PDT, Vicamo Yang [:vicamo][:vyang]
philipp: review+
Details | Diff | Splinter Review
Part 8: Respect X-Mms-Delivery-Report field (5.58 KB, patch)
2012-05-31 16:18 PDT, Vicamo Yang [:vicamo][:vyang]
philipp: review+
Details | Diff | Splinter Review
Part 3: handle WAP Push notification : V4 (17.31 KB, patch)
2012-06-03 20:14 PDT, Vicamo Yang [:vicamo][:vyang]
vicamo: review+
Details | Diff | Splinter Review

Description Andreas Gal :gal 2012-04-27 17:53:42 PDT
Implement a service in Gecko that receives OMNA WAP Push notifications and allows receivers to register to be notified when certain MIME types are received. In that case the text content and the uri are passed to the receiver.
Comment 1 Philipp von Weitershausen [:philikon] 2012-04-27 17:58:49 PDT
The work we're doing in bug 744360 for MMS seems related to this and probably be harnessed.
Comment 2 Andreas Gal :gal 2012-04-27 18:39:20 PDT
Yes, this is basically a mild extension of the MMS protocol. Philipp, who in Taipei should own this? Its pretty high priority to get this off the ground at least to the level where the service dumps messages to logcat so we can stand up the server side in parallel.
Comment 3 Philipp von Weitershausen [:philikon] 2012-04-27 18:49:27 PDT
Vicamo is familiar with the problem space and actively working on this as part of MMS. I'm tentatively assigning this to him.

Vicamo, feel free to delegate this or some of the MMS work. There's seems to be a lot of stuff that we can share, so you probably know best how to cut up the work. Thanks!
Comment 4 Vicamo Yang [:vicamo][:vyang] 2012-04-28 23:33:39 PDT
(In reply to Philipp von Weitershausen [:philikon] from comment #3)
> Vicamo is familiar with the problem space and actively working on this as
> part of MMS. I'm tentatively assigning this to him.
> 
> Vicamo, feel free to delegate this or some of the MMS work. There's seems to
> be a lot of stuff that we can share, so you probably know best how to cut up
> the work. Thanks!

We've already have a parser for MMS notification receiving with bug 744369. I'm working on creating internal interfaces for Push application registration to complete its function. I think this issue will be covered in bug 744369.
Comment 5 Vicamo Yang [:vicamo][:vyang] 2012-04-28 23:34:50 PDT
(In reply to Vicamo Yang [:vicamo] from comment #4)
> bug 744369.

Sorry, should be bug 744360.
Comment 6 Vicamo Yang [:vicamo][:vyang] 2012-05-05 20:56:49 PDT
Created attachment 621385 [details] [diff] [review]
Part 1: add MmsService
Comment 7 Vicamo Yang [:vicamo][:vyang] 2012-05-05 20:57:29 PDT
Created attachment 621386 [details] [diff] [review]
Part 2: parse MMS notification PDU
Comment 8 Vicamo Yang [:vicamo][:vyang] 2012-05-18 12:38:12 PDT
Created attachment 625201 [details] [diff] [review]
Part 0: IDL changes
Comment 9 Vicamo Yang [:vicamo][:vyang] 2012-05-18 12:38:51 PDT
Created attachment 625203 [details] [diff] [review]
Part 1: add MmsService
Comment 10 Vicamo Yang [:vicamo][:vyang] 2012-05-18 12:39:53 PDT
Created attachment 625205 [details] [diff] [review]
Part 2: add MMS PDU Helper
Comment 11 Vicamo Yang [:vicamo][:vyang] 2012-05-18 12:40:58 PDT
Created attachment 625206 [details] [diff] [review]
Part 3: handle WAP Push notification
Comment 12 Vicamo Yang [:vicamo][:vyang] 2012-05-18 12:53:02 PDT
Comment on attachment 625203 [details] [diff] [review]
Part 1: add MmsService

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

::: dom/system/gonk/RadioInterfaceLayer.js
@@ +201,5 @@
>    Services.obs.addObserver(this, "xpcom-shutdown", false);
>    Services.obs.addObserver(this, kMozSettingsChangedObserverTopic, false);
>  
>    this._sentSmsEnvelopes = {};
> +  debug("HAVE_MMS = " + gMmsService.hasSupport());

Should be instanciated somewhere else automatically
Comment 13 Vicamo Yang [:vicamo][:vyang] 2012-05-18 13:02:02 PDT
Comment on attachment 625206 [details] [diff] [review]
Part 3: handle WAP Push notification

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

::: b2g/chrome/content/shell.js
@@ +657,5 @@
> +
> +  ['ril.data.mmsport'].forEach(function(key) {
> +    SettingsListener.observe(key, false, function(value) {
> +      Services.prefs.setIntPref(key, value);
> +    });

corresponding Gaia pull request is https://github.com/andreasgal/gaia/pull/1441

::: dom/mms/src/ril/MmsService.js
@@ +37,5 @@
> +
> +  Services.obs.addObserver(this, kXpcomShutdownObserverTopic, false);
> +  Services.obs.addObserver(this, kMozSettingsChangedObserverTopic, false);
> +
> +  this.updateProxyInfo(true);

Need 3G network auto config, mechanism to switch MMS apn etc in the future.
Comment 14 Philipp von Weitershausen [:philikon] 2012-05-23 16:14:07 PDT
Comment on attachment 625201 [details] [diff] [review]
Part 0: IDL changes

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

No need to copy the outdated and overly complicated directory structure (src, interfaces, etc.) from other folders. Let's just make a flat directory in dom/mms and do dom/mms/nsIMmsService.idl
Comment 15 Philipp von Weitershausen [:philikon] 2012-05-23 16:17:19 PDT
Comment on attachment 625203 [details] [diff] [review]
Part 1: add MmsService

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

Same comment re: flat directory structure. It's fine to just do dom/mms/MmsService.js for now.

::: dom/system/gonk/RadioInterfaceLayer.js
@@ +201,5 @@
>    Services.obs.addObserver(this, "xpcom-shutdown", false);
>    Services.obs.addObserver(this, kMozSettingsChangedObserverTopic, false);
>  
>    this._sentSmsEnvelopes = {};
> +  debug("HAVE_MMS = " + gMmsService.hasSupport());

The lazy getter will instantiate the service at first use. There's no reason to do this there and potentially cause a startup slowdown.
Comment 16 Mounir Lamouri (:mounir) 2012-05-23 16:26:16 PDT
(In reply to Philipp von Weitershausen [:philikon] from comment #14)
> Comment on attachment 625201 [details] [diff] [review]
> Part 0: IDL changes
> 
> Review of attachment 625201 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> No need to copy the outdated and overly complicated directory structure
> (src, interfaces, etc.) from other folders. Let's just make a flat directory
> in dom/mms and do dom/mms/nsIMmsService.idl

Please, keep the same structure as dom/sms/ for consistency with other folders. In addition, this is a good habit to have to prevent chaos happening when files get numerous. Now that you have the makefiles to handle this, there is no point in reverting this, really.
Comment 17 Philipp von Weitershausen [:philikon] 2012-05-23 16:27:04 PDT
Comment on attachment 625205 [details] [diff] [review]
Part 2: add MMS PDU Helper

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

Beautiful. Though I wonder if his should perhaps not live in dom/system/gonk?

::: dom/mms/src/ril/MmsService.js
@@ +10,5 @@
>  Cu.import("resource://gre/modules/Services.jsm");
>  
> +Cu.import("resource://gre/modules/mms_consts.js");
> +let MMS = {};
> +Cu.import("resource://gre/modules/MmsPduHelper.jsm", MMS);

Is there a reason for the separate files here? Can't MmsPduHelper.js include the constants?
Comment 18 [:fabrice] Fabrice Desré 2012-05-23 16:30:15 PDT
Comment on attachment 625203 [details] [diff] [review]
Part 1: add MmsService

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

::: dom/mms/src/ril/MmsService.js
@@ +39,5 @@
> +  // nsIWapPushApplication
> +
> +  receiveWapPush: function receiveWapPush(stream, options) {
> +    // Do something.
> +  },

does strict mode accept this comma?

@@ +51,5 @@
> +    dump("-@- MmsService: " + s + "\n");
> +  };
> +} else {
> +  debug = function (s) {};
> +}

nit: all this debug stuff is unused.
Comment 19 Philipp von Weitershausen [:philikon] 2012-05-23 16:43:40 PDT
Comment on attachment 625206 [details] [diff] [review]
Part 3: handle WAP Push notification

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

Nice work! A few questions below.

::: dom/mms/src/ril/MmsService.js
@@ +50,5 @@
> +
> +  proxyInfo: null,
> +  MMSC: null,
> +
> +  getPdu: function getPdu(httpMethod, uri, callback) {

Does the spec ever specify anything other than "GET" for httpMethod?

@@ +79,5 @@
> +          }
> +
> +          stream = Cc["@mozilla.org/wap/rilwapbinaryinputstream;1"]
> +                   .createInstance(Ci.nsIWapBinaryInputStream);
> +          stream.initFromByteArray(u8array, u8array.length);

What's the purpose of this? I don't quite understand why the Uint8Array isn't good enough. This is essentially duplicating the memory consumption. Can you explain? Thanks!

@@ +84,5 @@
> +          break;
> +        }
> +        default:
> +          debug("xhr done, but status = " + xhr.status);
> +          break;

No error handling? I guess we're just passing stream = null to the callback at this point. Might be nicer to tell the callback why it failed. A common callback convention is callback(error, result), so

  callback(Components.Exception(...), null);

in case of an error, and

  callback(null, result);

when things went fine.
Comment 20 Vicamo Yang [:vicamo][:vyang] 2012-05-24 11:19:51 PDT
(In reply to Philipp von Weitershausen [:philikon] from comment #15)
> > +  debug("HAVE_MMS = " + gMmsService.hasSupport());
> 
> The lazy getter will instantiate the service at first use. There's no reason
> to do this there and potentially cause a startup slowdown.

Please see my comment in bug 744360 comment #29. I think eventually we'll have to start MmsService on startup just like the SmsService does. I should find a better solution like profile-after-change category, not hard-code in a debug statement. Right?
Comment 21 Vicamo Yang [:vicamo][:vyang] 2012-05-24 11:23:09 PDT
(In reply to Philipp von Weitershausen [:philikon] from comment #17)
> Beautiful. Though I wonder if his should perhaps not live in dom/system/gonk?

Sorry, but I can't fully understand what you mean?

> > +Cu.import("resource://gre/modules/mms_consts.js");
> > +let MMS = {};
> > +Cu.import("resource://gre/modules/MmsPduHelper.jsm", MMS);
> 
> Is there a reason for the separate files here? Can't MmsPduHelper.js include
> the constants?

I'll fix it. BTW, test script for MmsPduHelper is almost done.
Comment 22 Vicamo Yang [:vicamo][:vyang] 2012-05-24 11:24:53 PDT
(In reply to Fabrice Desré [:fabrice] from comment #18)
> > +  receiveWapPush: function receiveWapPush(stream, options) {
> > +    // Do something.
> > +  },
> 
> does strict mode accept this comma?
> 

Yes, at least last time I run it ;)

> @@ +51,5 @@
> > +    dump("-@- MmsService: " + s + "\n");
> > +  };
> > +} else {
> > +  debug = function (s) {};
> > +}
> 
> nit: all this debug stuff is unused.

I'll move them to later patch.
Comment 23 Vicamo Yang [:vicamo][:vyang] 2012-05-24 11:43:06 PDT
(In reply to Philipp von Weitershausen [:philikon] from comment #19)
> Nice work! A few questions below.
> 

Thanks. Working with you always make me feel much pleased!

> > +  getPdu: function getPdu(httpMethod, uri, callback) {
> 
> Does the spec ever specify anything other than "GET" for httpMethod?
>

Yes, there are. According to OMA-TS-MMS_ENC-V1_3-20110913-A[1] section 7:

  The MMS PDU is stored to the Data field of the Post, Reply and Push PDUs [WAPWSP]
  when using the WSP based stack, and to the Message Body of the POST or Response
  HTTP message when using the HTTP based stack.

It follows all MMS PDU must be sent by POST when using HTTP, and all MMS operations involve sending some requests, which are encoded as MMS PDU for certain. There is only one exception: the message fetching from MMS Proxy-Relay. Section 4 in the same document explicitly says message fetching should be done under WSP/HTTP Get.req without a MMS PDU attached. So, in fact, GET method is used by only one case, the very beginning case here. ;)

> @@ +79,5 @@
> > +          }
> > +
> > +          stream = Cc["@mozilla.org/wap/rilwapbinaryinputstream;1"]
> > +                   .createInstance(Ci.nsIWapBinaryInputStream);
> > +          stream.initFromByteArray(u8array, u8array.length);
> 
> What's the purpose of this? I don't quite understand why the Uint8Array
> isn't good enough. This is essentially duplicating the memory consumption.
> Can you explain? Thanks!
> 

I've removed them. I found HttpChannel utilizes streams in many case and thought it's the only way to use it. I was wrong.

> @@ +84,5 @@
> > +          break;
> > +        }
> > +        default:
> > +          debug("xhr done, but status = " + xhr.status);
> > +          break;
> 
> No error handling? I guess we're just passing stream = null to the callback
> at this point. Might be nicer to tell the callback why it failed. A common
> callback convention is callback(error, result), so
> 
>   callback(Components.Exception(...), null);
> 
> in case of an error, and
> 
>   callback(null, result);
> 
> when things went fine.

Also fixed.

[1]: http://www.openmobilealliance.org/technical/release_program/docs/CopyrightClick.aspx?pck=MMS&file=V1_3-20110913-A/OMA-TS-MMS_ENC-V1_3-20110913-A.pdf
Comment 24 Philipp von Weitershausen [:philikon] 2012-05-24 11:45:16 PDT
(In reply to Vicamo Yang [:vicamo] from comment #20)
> (In reply to Philipp von Weitershausen [:philikon] from comment #15)
> > > +  debug("HAVE_MMS = " + gMmsService.hasSupport());
> > 
> > The lazy getter will instantiate the service at first use. There's no reason
> > to do this there and potentially cause a startup slowdown.
> 
> Please see my comment in bug 744360 comment #29. I think eventually we'll
> have to start MmsService on startup just like the SmsService does. I should
> find a better solution like profile-after-change category, not hard-code in
> a debug statement. Right?

Yes, ok. In that case please do that. That would be much cleaner.

(In reply to Vicamo Yang [:vicamo] from comment #21)
> (In reply to Philipp von Weitershausen [:philikon] from comment #17)
> > Beautiful. Though I wonder if his should perhaps not live in dom/system/gonk?
> 
> Sorry, but I can't fully understand what you mean?

I was thinking that MmsPduHelper.jsm should perhaps live in dom/system/gonk.
Comment 25 Philipp von Weitershausen [:philikon] 2012-05-24 11:49:30 PDT
(In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #16)
> Please, keep the same structure as dom/sms/ for consistency with other
> folders. In addition, this is a good habit to have to prevent chaos
> happening when files get numerous. Now that you have the makefiles to handle
> this, there is no point in reverting this, really.

Seems silly to me to create a deep directory structure (that's also no longer used AFAIK) for a prototype implementation that will disappear, but ok.
Comment 26 Vicamo Yang [:vicamo][:vyang] 2012-05-30 02:57:59 PDT
Created attachment 628287 [details] [diff] [review]
Part 1: IDL changes : V2

s/Part 0/Part 1/ in commit summary
Comment 27 Vicamo Yang [:vicamo][:vyang] 2012-05-30 03:05:12 PDT
Created attachment 628289 [details] [diff] [review]
Part 2: add MMS PDU Helper : V2

1) add test scripts for decoder functions
2) use octet array instead of customized stream
3) improve exception error messages
4) more strict Address parsing
5) add Parameter.decodeMultiple()
6) check mandatory header fields
Comment 28 Vicamo Yang [:vicamo][:vyang] 2012-05-30 03:16:29 PDT
Created attachment 628292 [details] [diff] [review]
Part 3: handle WAP Push notification : V2

1) Merge attachment $625203 and attachment 625206 [details] [diff] [review]
2) WapPushManager is no longer an service.
3) Watch kNetworkInterfaceStateChangedTopic and update MMS config automatically
4) Only register proxy filter when needed and unregister it when no longer referenced.
5) Add TRANSPARENT_PROXY_RESOLVES_HOST flag to proxy filter constructor.
6) Use octet array instead of customized stream
7) Enhance error handling in message processing via callbacks
8) Save multipart data as files
9) Include MmsPduHelper.jsm only. Don't have to include mms_consts.js again.
Comment 29 Vicamo Yang [:vicamo][:vyang] 2012-05-30 03:17:31 PDT
Created attachment 628293 [details] [diff] [review]
Part 4: add WSP/MMS encoding support
Comment 30 Vicamo Yang [:vicamo][:vyang] 2012-05-30 03:18:27 PDT
Created attachment 628294 [details] [diff] [review]
Part 5: send M-NotifyResp.ind response back
Comment 31 Vicamo Yang [:vicamo][:vyang] 2012-05-30 10:56:54 PDT
Comment on attachment 628289 [details] [diff] [review]
Part 2: add MMS PDU Helper : V2

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

::: dom/mms/src/ril/MmsPduHelper.jsm
@@ +834,5 @@
> +  add("x-mms-status",                            0x15, StatusValue);
> +  add("subject",                                 0x16, EncodedStringValue);
> +  add("to",                                      0x17, Address);
> +  add("x-mms-transaction-id",                    0x18, WSP.TextString);
> +  //add("x-mms-retrieve-status", 0x19);

Optional field in M-Retrieve.conf PDU. Since we'll retrieve the MMS message in M-Notificaion.ind handling process, we might need this field for more completed support and better error handling as well.

@@ +835,5 @@
> +  add("subject",                                 0x16, EncodedStringValue);
> +  add("to",                                      0x17, Address);
> +  add("x-mms-transaction-id",                    0x18, WSP.TextString);
> +  //add("x-mms-retrieve-status", 0x19);
> +  //add("x-mms-retrieve-text", 0x1A);

ditto.

@@ +841,5 @@
> +  add("x-mms-reply-charging",                    0x1C, WSP.ReplyChargingValue);
> +  add("x-mms-reply-charging-deadline",           0x1D, ExpiryValue);
> +  add("x-mms-reply-charging-id",                 0x1E, WSP.TextString);
> +  add("x-mms-reply-charging-size",               0x1F, WSP.LongInteger);
> +  //add("x-mms-previously-sent-by", 0x20);

ditto.

@@ +842,5 @@
> +  add("x-mms-reply-charging-deadline",           0x1D, ExpiryValue);
> +  add("x-mms-reply-charging-id",                 0x1E, WSP.TextString);
> +  add("x-mms-reply-charging-size",               0x1F, WSP.LongInteger);
> +  //add("x-mms-previously-sent-by", 0x20);
> +  //add("x-mms-previously-sent-date", 0x21);

ditto.

@@ +844,5 @@
> +  add("x-mms-reply-charging-size",               0x1F, WSP.LongInteger);
> +  //add("x-mms-previously-sent-by", 0x20);
> +  //add("x-mms-previously-sent-date", 0x21);
> +  add("x-mms-store",                             0x22, BooleanValue);
> +  //add("x-mms-mm-state", 0x23);

ditto.

@@ +845,5 @@
> +  //add("x-mms-previously-sent-by", 0x20);
> +  //add("x-mms-previously-sent-date", 0x21);
> +  add("x-mms-store",                             0x22, BooleanValue);
> +  //add("x-mms-mm-state", 0x23);
> +  //add("x-mms-mm-flags", 0x24);

ditto.
Comment 32 Vicamo Yang [:vicamo][:vyang] 2012-05-30 11:03:08 PDT
Comment on attachment 628293 [details] [diff] [review]
Part 4: add WSP/MMS encoding support

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

::: dom/mms/src/ril/MmsPduHelper.jsm
@@ +573,5 @@
>    },
> +
> +  encode: function encode(data, value) {
> +    if ((value < 128) || (value > 151)) {
> +      throw new WSP.CodeError("Message-type-value: invalid type " + type);

`type` is not defined here.
Comment 33 Vicamo Yang [:vicamo][:vyang] 2012-05-30 11:23:24 PDT
(In reply to Vicamo Yang [:vicamo] from comment #31)
> Comment on attachment 628289 [details] [diff] [review]
> > +  //add("x-mms-retrieve-status", 0x19);
> > +  //add("x-mms-retrieve-text", 0x1A);
> > +  //add("x-mms-previously-sent-by", 0x20);
> > +  //add("x-mms-previously-sent-date", 0x21);
> > +  //add("x-mms-mm-state", 0x23);
> > +  //add("x-mms-mm-flags", 0x24);
> 
> Optional field in M-Retrieve.conf PDU. Since we'll retrieve the MMS message
> in M-Notificaion.ind handling process, we might need this field for more
> completed support and better error handling as well.

X-Mms-Recommended-Retrieval-Mode-Text is optional to M-Notification.ind PDU.
Comment 34 Philipp von Weitershausen [:philikon] 2012-05-30 13:53:12 PDT
(In reply to Vicamo Yang [:vicamo] from comment #31)
> ::: dom/mms/src/ril/MmsPduHelper.jsm
> @@ +834,5 @@
> > +  add("x-mms-status",                            0x15, StatusValue);
> > +  add("subject",                                 0x16, EncodedStringValue);
> > +  add("to",                                      0x17, Address);
> > +  add("x-mms-transaction-id",                    0x18, WSP.TextString);
> > +  //add("x-mms-retrieve-status", 0x19);
> 
> Optional field in M-Retrieve.conf PDU. Since we'll retrieve the MMS message
> in M-Notificaion.ind handling process, we might need this field for more
> completed support and better error handling as well.

Follow-up?
Comment 35 Philipp von Weitershausen [:philikon] 2012-05-30 14:19:40 PDT
Comment on attachment 628292 [details] [diff] [review]
Part 3: handle WAP Push notification : V2

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

Stellar work! r=me with comments below addressed.

::: dom/mms/src/ril/MmsService.js
@@ +12,5 @@
> +Cu.import("resource://gre/modules/FileUtils.jsm");
> +Cu.import("resource://gre/modules/NetUtil.jsm");
> +
> +let MMS = {};
> +Cu.import("resource://gre/modules/MmsPduHelper.jsm", MMS);

Similar to what I said in bug 744360 comment 37: let's load this lazily:

  XPCOMUtils.defineLazyGetter(this, "MMS", function () {
    let MMS = {};
    Cu.import("resource://gre/modules/MmsPduHelper.jsm", MMS);
    return MMS;
  });

@@ +160,5 @@
> +  saveContentToFile: function saveContentToFile(file, data, callback) {
> +    // Write to a StorageStream for NetUtil.asyncCopy()
> +    let sstream = Cc["@mozilla.org/storagestream;1"]
> +                  .createInstance(Ci.nsIStorageStream);
> +    sstream.init(4096, data.length, null);

const the 4096 at the top of the file, please.

@@ +215,5 @@
> +   */
> +  handleNotificationIndication: function handleNotificationIndication(msg, callback) {
> +    let url = msg.headers["x-mms-content-location"].uri;
> +    this.sendMmsRequest("GET", url, (function (status, data) {
> +        if (!data) {

Nit: indent by 2, not 4 spaces.
Comment 36 Philipp von Weitershausen [:philikon] 2012-05-30 14:25:00 PDT
Comment on attachment 628293 [details] [diff] [review]
Part 4: add WSP/MMS encoding support

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

r=me with your `type` reference fixed. Also, tests?
Comment 37 Philipp von Weitershausen [:philikon] 2012-05-30 14:32:07 PDT
Comment on attachment 628294 [details] [diff] [review]
Part 5: send M-NotifyResp.ind response back

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

Looks good!

::: dom/mms/src/ril/MmsPduHelper.jsm
@@ +917,5 @@
>                                           "content-type"]);
> +  add(MMS_PDU_TYPE_NOTIFYRESP_IND, false, ["x-mms-message-type",
> +                                           "x-mms-transaction-id",
> +                                           "x-mms-mms-version",
> +                                           "x-mms-status"]);

Is this what you were talking about in comment 31?

::: dom/mms/src/ril/MmsService.js
@@ +86,5 @@
>     *        "GET" or "POST".
>     * @param url
>     *        Target url string.
> +   * @param istream
> +   *        An nsIInputStream instance as data source to be sent.

Mark this

  @param istream [optional]

since the parameter seems to be optional :)
Comment 38 Vicamo Yang [:vicamo][:vyang] 2012-05-30 22:59:16 PDT
(In reply to Philipp von Weitershausen [:philikon] from comment #37)
> Comment on attachment 628294 [details] [diff] [review]
> Part 5: send M-NotifyResp.ind response back
> ::: dom/mms/src/ril/MmsPduHelper.jsm
> @@ +917,5 @@
> >                                           "content-type"]);
> > +  add(MMS_PDU_TYPE_NOTIFYRESP_IND, false, ["x-mms-message-type",
> > +                                           "x-mms-transaction-id",
> > +                                           "x-mms-mms-version",
> > +                                           "x-mms-status"]);
> 
> Is this what you were talking about in comment 31?

No. They are mandatory fields of M-NotifyResp.ind type PDU. I was talking about unsupported optional fields of M-Retrieve.conf. Anyway, I've added them in 2nd patch with test cases.
Comment 39 Vicamo Yang [:vicamo][:vyang] 2012-05-31 16:04:43 PDT
Created attachment 628937 [details] [diff] [review]
Part 2: add MMS PDU Helper : V3

1) address comment #31, comment #33
2) check all function doc again
3) fix redundent \0 in CharsetEncodedString
4) rewrite test script for more easier debug
Comment 40 Vicamo Yang [:vicamo][:vyang] 2012-05-31 16:08:50 PDT
Created attachment 628939 [details] [diff] [review]
Part 3: handle WAP Push notification : V3

1) address comment #35
2) divided into smaller patches. This one contains only basic notification handling. M-Retrieve.conf PDU fetched, but not processed yet.
Comment 41 Vicamo Yang [:vicamo][:vyang] 2012-05-31 16:09:54 PDT
Created attachment 628940 [details] [diff] [review]
Part 4: handle M-Retrieve.conf PDU
Comment 42 Vicamo Yang [:vicamo][:vyang] 2012-05-31 16:10:41 PDT
Created attachment 628941 [details] [diff] [review]
Part 5: save message attachments
Comment 43 Vicamo Yang [:vicamo][:vyang] 2012-05-31 16:13:04 PDT
Created attachment 628942 [details] [diff] [review]
Part 6: add WSP/MMS encoding support : V2

1) address comment #36
2) add test cases for encoding methods
Comment 44 Vicamo Yang [:vicamo][:vyang] 2012-05-31 16:17:39 PDT
Created attachment 628950 [details] [diff] [review]
Part 7: send M-NotifyResp.ind response back : V2

1) address comment #37
Comment 45 Vicamo Yang [:vicamo][:vyang] 2012-05-31 16:18:23 PDT
Created attachment 628951 [details] [diff] [review]
Part 8: Respect X-Mms-Delivery-Report field
Comment 47 Marco Bonardo [::mak] 2012-06-01 03:00:19 PDT
HAd to backout this bug 744360 caused an xpcshell failure, and this was touching the same code.
Comment 48 Vicamo Yang [:vicamo][:vyang] 2012-06-03 20:14:47 PDT
Created attachment 629683 [details] [diff] [review]
Part 3: handle WAP Push notification : V4

fix build break in https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=795990989746
Comment 49 Vicamo Yang [:vicamo][:vyang] 2012-06-03 20:23:45 PDT
Newer try results are availabe in https://tbpl.mozilla.org/?tree=Try&rev=2707fc628fdd
Comment 50 Vicamo Yang [:vicamo][:vyang] 2012-06-03 22:02:10 PDT
rebased try: 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.