810067 - B2G MMS: support automatic/manual/never retrieval modes

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

[Vicamo Yang [:vicamo][:vyang]]

MMS supports delayed retrieval. On receiving a M-Notification.ind, application should be able to determine what to do next according to either user settings or X-Mms-Recommended-Retrieval-Mode and X-Mms-Recommended-Retrieval-Mode-Text.

Comment 1

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

We might need below task to do...
1. Hook settings to decide the retrieval mode(auto/auto-home/deferred/never)
2. We might need a transaction service to 
  1. Check MMS network availability
  2. According to retrieval mode to decide when to download MMS entity. 
  3. Control the transaction state. 
  4. Keep pending/processing transaction in somewhere.
3. Need M-acknowledge.ind pdu for deferred mode. 
4. Need Delivery Report related PDU

Comment 2

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

We might need to save the pending/processing transaction into database. And remove from database when finished.

Comment 3

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

After read more document, I think we don't need Delivery Report related PDU in MMS client. Because below reasons.

"A recipient MMS client can deny the generation of delivery reports (parameter X-Mms-Report-Allowed for M-acknowledge.ind and M-notifyresp.ind PDUs). If allowed by the recipient MMS client, the recipient MMSC generates the delivery report and forwards it to the originator MMSC over the MM4 interface. Upon receipt of the delivery report, the originator MMSC delivers it to the originator MMS client over the MM1 interface with the M-delivery.ind PDU"

Comment 4

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

But we might need read report and message forward related PDUs.

Comment 5

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Attachment: WIP-v1

Comment 6

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Attachment: WIP-v1

Comment 7

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Attachment: WIP-v1

Comment 8

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Attachment: Retrieval mode support

Comment 9

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Attachment: Retrieval mode support

Comment 10

[Vicamo Yang [:vicamo][:vyang]]

Comment on attachment 717704 [details] [diff] [review]
Retrieval mode support

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

::: dom/mms/src/ril/MmsService.js
@@ +35,5 @@
>  const TIME_TO_RELEASE_MMS_CONNECTION = 30000;
>  
> +const MANUAL_RETRIEVAL_MODE = "manual";
> +const AUTOMATIC_RETRIEVAL_MODE = "automatic";
> +const NEVER_RETRIEVAL_MODE = "never";

|const RETRIEVAL_MODE_MANUAL = "manual";| instead.

@@ +745,5 @@
>     * and M-Acknowledge.ind PDU.
>     */
>    confSendDeliveryReport: CONFIG_SEND_REPORT_DEFAULT_YES,
>  
> +  retrievalModePerferenceName: 'dom.mms.retrieval_mode',

|const PERF_RETRIEVAL_MODE = "dom.mms.retrieval_mode";| before RETRIEVAL_MODE_FOO instead.

@@ +836,5 @@
> +          new NotifyResponseTransaction(transactionId, mmsStatus, reportAllowed);
> +        transaction.run();
> +      }).bind(this));
> +      return;
> +    } else if (NEVER_RETRIEVAL_MODE === retrievalMode) {

You don't need |else| after a return statement.

@@ +843,5 @@
> +      let transactionId = notification.headers["x-mms-transaction-id"];
> +
> +      let transaction = new NotifyResponseTransaction(transactionId,
> +                                                      MMS.MMS_PDU_STATUS_REJECTED,
> +                                                      false);

Should calculate reportAllowed here based only on notification.headers["x-mms-delivery-report"].

@@ +846,5 @@
> +                                                      MMS.MMS_PDU_STATUS_REJECTED,
> +                                                      false);
> +      transaction.run();
> +      return;
> +    } else { //MANUAL_RETRIEVAL_MODE and wrong retrievalMode value

|else|

Comment 11

[[:fabrice] Fabrice Desré]

Comment on attachment 717704 [details] [diff] [review]
Retrieval mode support

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

::: dom/mms/src/ril/MmsService.js
@@ +707,5 @@
> +  // Optional fields
> +  headers["x-mms-report-allowed"] = reportAllowed;
> +
> +  this.istream = MMS.PduHelper.compose(null, {headers: headers});
> +}

Nit: add a blank line

@@ +745,5 @@
>     * and M-Acknowledge.ind PDU.
>     */
>    confSendDeliveryReport: CONFIG_SEND_REPORT_DEFAULT_YES,
>  
> +  retrievalModePerferenceName: 'dom.mms.retrieval_mode',

Nit: s/retrievalModePerferenceName/retrievalModePreferenceName

@@ +809,5 @@
>      //                     notification
> +
> +    let retrievalMode = MANUAL_RETRIEVAL_MODE;
> +    try {
> +      retrievalMode = Services.prefs.getCharPref(this.retrievalModePerferenceName);

Nit: s/retrievalModePerferenceName/retrievalModePreferenceName

@@ +813,5 @@
> +      retrievalMode = Services.prefs.getCharPref(this.retrievalModePerferenceName);
> +    } catch (e) {}
> +
> +    if (AUTOMATIC_RETRIEVAL_MODE === retrievalMode) {
> +      this.retrieveMessage(url, (function (mmsStatus, retrievedMsg) {

Please name the anonymous function

Comment 12

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Attachment: Retrieval mode supported

Comment 13

[Vicamo Yang [:vicamo][:vyang]]

Comment on attachment 717712 [details] [diff] [review]
Retrieval mode supported

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

Thank you :)

::: dom/mms/src/ril/MmsService.js
@@ +848,2 @@
>  
>        let transactionId = notification.headers["x-mms-transaction-id"];

Please move this outside if-block and share with manual mode. Actually you you can share everything by:

  let mmsStatus = RETRIEVAL_MODE_NEVER === retrievalMode
                ? MMS.MMS_PDU_STATUS_REJECTED : MMS.MMS_PDU_STATUS_DEFERRED;

Comment 14

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Attachment: Retrieval mode supported

Comment 15

[[:fabrice] Fabrice Desré]

Comment on attachment 717734 [details] [diff] [review]
Retrieval mode supported

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

::: dom/mms/src/ril/MmsService.js
@@ +34,5 @@
>  const TIME_TO_BUFFER_MMS_REQUESTS    = 30000;
>  const TIME_TO_RELEASE_MMS_CONNECTION = 30000;
>  
> +
> +const PERF_RETRIEVAL_MODE = 'dom.mms.retrieval_mode';

Nit: PREF_RETRIEVAL_MODE

@@ +810,4 @@
>  
> +    let retrievalMode = RETRIEVAL_MODE_MANUAL;
> +    try {
> +      retrievalMode = Services.prefs.getCharPref(PERF_RETRIEVAL_MODE);

Nit: PREF_RETRIEVAL_MODE

Comment 16

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Attachment: Retrieval mode supported

Comment 17

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Attachment: Retrieval mode supported

Comment 18

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Fixed.
Thanks for your comment.

(In reply to Fabrice Desré [:fabrice] from comment #15)
> Comment on attachment 717734 [details] [diff] [review]
> Retrieval mode supported
> 
> Review of attachment 717734 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/mms/src/ril/MmsService.js
> @@ +34,5 @@
> >  const TIME_TO_BUFFER_MMS_REQUESTS    = 30000;
> >  const TIME_TO_RELEASE_MMS_CONNECTION = 30000;
> >  
> > +
> > +const PERF_RETRIEVAL_MODE = 'dom.mms.retrieval_mode';
> 
> Nit: PREF_RETRIEVAL_MODE
> 
> @@ +810,4 @@
> >  
> > +    let retrievalMode = RETRIEVAL_MODE_MANUAL;
> > +    try {
> > +      retrievalMode = Services.prefs.getCharPref(PERF_RETRIEVAL_MODE);
> 
> Nit: PREF_RETRIEVAL_MODE

Comment 19

[Vicamo Yang [:vicamo][:vyang]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5e226e40ed7c

Comment 20

[Joe Cheng [:jcheng] (please needinfo)]

leo+, required to support MMS in v1.1

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/5e226e40ed7c

Comment 22

[Ryan VanderMeulen [:RyanVM]]

This doesn't apply cleanly to b2g18. Please either post a branch-specific patch suitable for landing or request approval for any bugs that this depends on.

Comment 23

[Vicamo Yang [:vicamo][:vyang]]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #22)
> This doesn't apply cleanly to b2g18. Please either post a
> branch-specific patch suitable for landing or request approval
> for any bugs that this depends on.

leo? for bug 833697 first, thank you :)

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/60c8c92a90f7