The default bug view has changed. See this FAQ.

implement backend for dispatching OMNA WAP Push notification

RESOLVED FIXED in mozilla15

Status

()

Core
DOM: Device Interfaces
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: gal, Assigned: vicamo)

Tracking

(Blocks: 1 bug)

Trunk
mozilla15
ARM
Gonk (Firefox OS)
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(8 attachments, 11 obsolete attachments)

704 bytes, patch
philikon
: review+
Details | Diff | Splinter Review
49.23 KB, patch
philikon
: review+
Details | Diff | Splinter Review
5.73 KB, patch
philikon
: review+
Details | Diff | Splinter Review
5.18 KB, patch
philikon
: review+
Details | Diff | Splinter Review
30.50 KB, patch
philikon
: review+
Details | Diff | Splinter Review
6.96 KB, patch
philikon
: review+
Details | Diff | Splinter Review
5.58 KB, patch
philikon
: review+
Details | Diff | Splinter Review
17.31 KB, patch
vicamo
: review+
Details | Diff | Splinter Review
(Reporter)

Description

5 years ago
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.
(Reporter)

Updated

5 years ago
Blocks: 710489
The work we're doing in bug 744360 for MMS seems related to this and probably be harnessed.
(Reporter)

Comment 2

5 years ago
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.
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!
Assignee: nobody → vyang
(Assignee)

Comment 4

5 years ago
(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.
(Assignee)

Comment 5

5 years ago
(In reply to Vicamo Yang [:vicamo] from comment #4)
> bug 744369.

Sorry, should be bug 744360.
(Assignee)

Updated

5 years ago
Depends on: 744360
(Assignee)

Comment 6

5 years ago
Created attachment 621385 [details] [diff] [review]
Part 1: add MmsService
(Assignee)

Comment 7

5 years ago
Created attachment 621386 [details] [diff] [review]
Part 2: parse MMS notification PDU
(Assignee)

Comment 8

5 years ago
Created attachment 625201 [details] [diff] [review]
Part 0: IDL changes
Attachment #625201 - Flags: superreview?(philipp)
(Assignee)

Comment 9

5 years ago
Created attachment 625203 [details] [diff] [review]
Part 1: add MmsService
Attachment #621385 - Attachment is obsolete: true
Attachment #625203 - Flags: review?(philipp)
(Assignee)

Comment 10

5 years ago
Created attachment 625205 [details] [diff] [review]
Part 2: add MMS PDU Helper
Attachment #621386 - Attachment is obsolete: true
Attachment #625205 - Flags: review?(philipp)
(Assignee)

Comment 11

5 years ago
Created attachment 625206 [details] [diff] [review]
Part 3: handle WAP Push notification
Attachment #625206 - Flags: review?(philipp)
(Assignee)

Comment 12

5 years ago
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
(Assignee)

Comment 13

5 years ago
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 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
Attachment #625201 - Flags: superreview?(philipp) → review+
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.
Attachment #625203 - Flags: review?(philipp) → review+
(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 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?
Attachment #625205 - Flags: review?(philipp) → review+
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 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.
Attachment #625206 - Flags: review?(philipp) → feedback+
(Assignee)

Comment 20

5 years ago
(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?
(Assignee)

Comment 21

5 years ago
(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.
(Assignee)

Comment 22

5 years ago
(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.
(Assignee)

Comment 23

5 years ago
(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
(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.
(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.
(Assignee)

Comment 26

5 years ago
Created attachment 628287 [details] [diff] [review]
Part 1: IDL changes : V2

s/Part 0/Part 1/ in commit summary
Attachment #625201 - Attachment is obsolete: true
(Assignee)

Comment 27

5 years ago
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
Attachment #625205 - Attachment is obsolete: true
Attachment #628289 - Flags: review?(philipp)
(Assignee)

Comment 28

5 years ago
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.
Attachment #625203 - Attachment is obsolete: true
Attachment #625206 - Attachment is obsolete: true
Attachment #628292 - Flags: review?(philipp)
(Assignee)

Comment 29

5 years ago
Created attachment 628293 [details] [diff] [review]
Part 4: add WSP/MMS encoding support
Attachment #628293 - Flags: review?(philipp)
(Assignee)

Comment 30

5 years ago
Created attachment 628294 [details] [diff] [review]
Part 5: send M-NotifyResp.ind response back
Attachment #628294 - Flags: review?(philipp)
(Assignee)

Updated

5 years ago
Attachment #628287 - Flags: review?(philipp)
(Assignee)

Comment 31

5 years ago
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.
(Assignee)

Comment 32

5 years ago
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.
(Assignee)

Comment 33

5 years ago
(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.
Attachment #628287 - Flags: review?(philipp) → review+
(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?
Attachment #628289 - Flags: review?(philipp) → review+
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.
Attachment #628292 - Flags: review?(philipp) → review+
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?
Attachment #628293 - Flags: review?(philipp) → review+
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 :)
Attachment #628294 - Flags: review?(philipp) → review+
(Assignee)

Comment 38

5 years ago
(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.
(Assignee)

Comment 39

5 years ago
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
Attachment #628289 - Attachment is obsolete: true
Attachment #628937 - Flags: review?(philipp)
(Assignee)

Comment 40

5 years ago
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.
Attachment #628292 - Attachment is obsolete: true
Attachment #628939 - Flags: review?(philipp)
(Assignee)

Comment 41

5 years ago
Created attachment 628940 [details] [diff] [review]
Part 4: handle M-Retrieve.conf PDU
Attachment #628940 - Flags: review?(philipp)
(Assignee)

Comment 42

5 years ago
Created attachment 628941 [details] [diff] [review]
Part 5: save message attachments
Attachment #628941 - Flags: review?(philipp)
(Assignee)

Comment 43

5 years ago
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
Attachment #628293 - Attachment is obsolete: true
Attachment #628942 - Flags: review?(philipp)
(Assignee)

Comment 44

5 years ago
Created attachment 628950 [details] [diff] [review]
Part 7: send M-NotifyResp.ind response back : V2

1) address comment #37
Attachment #628294 - Attachment is obsolete: true
Attachment #628950 - Flags: review?(philipp)
(Assignee)

Comment 45

5 years ago
Created attachment 628951 [details] [diff] [review]
Part 8: Respect X-Mms-Delivery-Report field
Attachment #628951 - Flags: review?(philipp)
Attachment #628937 - Flags: review?(philipp) → review+
Attachment #628939 - Flags: review?(philipp) → review+
Attachment #628940 - Flags: review?(philipp) → review+
Attachment #628941 - Flags: review?(philipp) → review+
Attachment #628942 - Flags: review?(philipp) → review+
Attachment #628950 - Flags: review?(philipp) → review+
Attachment #628951 - Flags: review?(philipp) → review+
Blocks: 744684
No longer blocks: 710489
(Assignee)

Comment 46

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/1deabba3bef5
https://hg.mozilla.org/integration/mozilla-inbound/rev/b7c14c8eec98
https://hg.mozilla.org/integration/mozilla-inbound/rev/743ebe50e914
https://hg.mozilla.org/integration/mozilla-inbound/rev/90465173c7e3
https://hg.mozilla.org/integration/mozilla-inbound/rev/472671930994
https://hg.mozilla.org/integration/mozilla-inbound/rev/429f20288742
https://hg.mozilla.org/integration/mozilla-inbound/rev/ab1f122c4b1a
https://hg.mozilla.org/integration/mozilla-inbound/rev/795990989746
Flags: in-testsuite+
Target Milestone: --- → mozilla15
HAd to backout this bug 744360 caused an xpcshell failure, and this was touching the same code.
Target Milestone: mozilla15 → ---
(Assignee)

Comment 48

5 years ago
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
Attachment #628939 - Attachment is obsolete: true
Attachment #629683 - Flags: review+
(Assignee)

Comment 49

5 years ago
Newer try results are availabe in https://tbpl.mozilla.org/?tree=Try&rev=2707fc628fdd
(Assignee)

Comment 50

5 years ago
rebased try: https://tbpl.mozilla.org/?tree=Try&rev=2707fc628fdd
(Assignee)

Comment 51

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/71140763abea
https://hg.mozilla.org/integration/mozilla-inbound/rev/8d6c0457e630
https://hg.mozilla.org/integration/mozilla-inbound/rev/abb9036641e2
https://hg.mozilla.org/integration/mozilla-inbound/rev/728c759ed654
https://hg.mozilla.org/integration/mozilla-inbound/rev/abcc96067a0f
https://hg.mozilla.org/integration/mozilla-inbound/rev/a0785f8d3a75
https://hg.mozilla.org/integration/mozilla-inbound/rev/936df980303f
https://hg.mozilla.org/integration/mozilla-inbound/rev/c6aa41df6d3e
Target Milestone: --- → mozilla15
(Assignee)

Comment 52

5 years ago
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
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.