850680 - B2G MMS: broadcast "sms-received" and "sms-sent" system messages

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

[Gene Lian (I already quit Mozilla)]

After receiving or sending an MMS message in MmsService.js, we need a way to broadcast an "mms-received" or "mms-sent" system message to the content side to launch the Notification and/or the Message App.

Not very sure if this is blocking any existing MMS user stories. Let PMs decide it.

Comment 1

[Gene Lian (I already quit Mozilla)]

Need to decide the permission name for MMS first because the system message is only allowed to be broadcasted to the specific permission(s).

Comment 3

[Gene Lian (I already quit Mozilla)]

Attachment: Part 1, code clean-up!

Comment 4

[Gene Lian (I already quit Mozilla)]

Attachment: Part 2, broadcast system messages

Comment 5

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

Let's unite SMS and MMS in system messages as well.

Comment 6

[Gene Lian (I already quit Mozilla)]

Need to fix bug 853725 first because we fail to read |nsIDOMMozMmsMessage.receivers| when preparing to send the system message.

Comment 7

[Gene Lian (I already quit Mozilla)]

Attachment: Part 1, code clean-up! V2

Comment 8

[Gene Lian (I already quit Mozilla)]

Attachment: Part 2, broadcast system messages, V2

Comment 9

[Gene Lian (I already quit Mozilla)]

Hi Alex,

I understand most of the MMS bugs are marked as leo- recently. However, I believe this one is the last one for leo-. Without this, we cannot launch the Notification, Costcontrol, Message app whenever an MMS is received or sent.

Nominate for leo+ again. Please feel free to let me know if you don't agree with me. Thanks! :)

Comment 10

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

Comment on attachment 728112 [details] [diff] [review]
Part 1, code clean-up! V2

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

::: dom/mobilemessage/src/ril/MobileMessageDatabaseService.js
@@ +1129,5 @@
>          aMessage.sender == undefined ||
>          (aMessage.type == "sms" && aMessage.messageClass == undefined) ||
>          (aMessage.type == "mms" && aMessage.delivery == undefined) ||
> +        (aMessage.type == "mms" && !Array.isArray(aMessage.deliveryStatus)) ||
> +        (aMessage.type == "mms" && !Array.isArray(aMessage.receivers)) ||

((aMessage.type == "sms") &&
 ((aMessage.delivery == undefined) ||
  !Array.isArray(aMessage.deliveryStatus) ||
  !Array.isArray(aMessage.receivers))) ||

Comment 11

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

Comment on attachment 728113 [details] [diff] [review]
Part 2, broadcast system messages, V2

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

::: dom/system/gonk/RadioInterfaceLayer.js
@@ +1635,5 @@
>                                                       DOM_MOBILE_MESSAGE_DELIVERY_SENT,
>                                                       options.sms.deliveryStatus,
>                                                       function notifyResult(rv, domMessage) {
>        // TODO bug 832140 handle !Components.isSuccessCode(rv)
> +      this.broadcastSmsSystemMessage("sms-sent", domMessage);      

nit: trailing ws

Comment 12

[Gene Lian (I already quit Mozilla)]

Attachment: Part 1, code clean-up! V2.1 (checked-in)

Thanks for the review! If you don't mind, I'd prefer writing them as:

if ((aMessage.type != "sms" && aMessage.type != "mms") ||
    (aMessage.type == "sms" && aMessage.messageClass == undefined) ||
    (aMessage.type == "mms" && (aMessage.delivery == undefined ||
                                !Array.isArray(aMessage.deliveryStatus) ||
                                !Array.isArray(aMessage.receivers))) ||
    aMessage.sender == undefined ||
    aMessage.timestamp == undefined) {
  if (aCallback) {
    aCallback.notify(Cr.NS_ERROR_FAILURE, null);
  }
  return;
}

Comment 13

[Gene Lian (I already quit Mozilla)]

Attachment: Part 2, broadcast system messages, V2.1 (checked-in)

Comment 14

[Gene Lian (I already quit Mozilla)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7ba21d4b0337
https://hg.mozilla.org/integration/mozilla-inbound/rev/89f25483717a

Comment 15

[Joe Drew (not getting mail)]

https://hg.mozilla.org/mozilla-central/rev/7ba21d4b0337
https://hg.mozilla.org/mozilla-central/rev/89f25483717a

Comment 16

[Lukas Blakk [:lsblakk] use ?needinfo]

Without a clear idea of whether this blocks user stories I think we should leave this as tracked (non-blocking) but please go ahead with an uplift nomination and risk/reward assessment.  If any user stories do depend on this, please include those too.

Comment 17

[Gene Lian (I already quit Mozilla)]

Comment on attachment 728146 [details] [diff] [review]
Part 1, code clean-up! V2.1 (checked-in)

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): no
User impact if declined: no
Testing completed: no
Risk to taking this patch (and alternatives if risky): no
String or UUID changes made by this patch: no

Note: the part-1 patch is just for code clean-up. No risk to land.

Comment 18

[Gene Lian (I already quit Mozilla)]

(In reply to Gene Lian [:gene] from comment #17)
> Testing completed: no
                     ^^ Sorry for the typo. This is "yes".

Comment 19

[Gene Lian (I already quit Mozilla)]

Comment on attachment 728162 [details] [diff] [review]
Part 2, broadcast system messages, V2.1 (checked-in)

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): no
User impact if declined: this bug/patch directly blocks two MMS User Stories: bug 840087 and bug 840090. Please see comment #9 for why it should be a leo+ in nature. When receiving or sending an MMS, we need to broadcast system messages to launch some related apps like:

  - Message App
  - Costcontrol App
  - Notification

Without the system message, we have no way to wake them up when they have been killed or doesn't run in the background.

Testing completed: yes
Risk to taking this patch (and alternatives if risky): low
String or UUID changes made by this patch: no

Comment 20

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 728146 [details] [diff] [review]
Part 1, code clean-up! V2.1 (checked-in)

The two user stories are non-blocking, since this is low risk we can take the uplift now and get some bake time.

Comment 21

[Gene Lian (I already quit Mozilla)]

Attachment: [b2g18] Patch Part 1

Comment 22

[Gene Lian (I already quit Mozilla)]

Attachment: [b2g18] Patch Part 2

Comment 23

[Gene Lian (I already quit Mozilla)]

Attachment: [b2g18] Patch Part 2

Comment 24

[Gene Lian (I already quit Mozilla)]

Attachment: [b2g18] Patch Part 2

Comment 25

[Gene Lian (I already quit Mozilla)]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/f805ffce21cf
https://hg.mozilla.org/releases/mozilla-b2g18/rev/37f172605376

Comment 26

[Gene Lian (I already quit Mozilla)]

Added [NO_UPLIFT] per recent commercial RIL compatibility issue. Waiting on further decision to keep the patch in b2g18 or to back it out.

------------------------------
If we really want to back them out, backing the following MMS bugs should be enough to make the commercial RIL compatible:

Bug 854422 - B2G MMS: should call .NotifyResponseTransaction() with MMS_PDU_STATUS_RETRIEVED after an MMS is retrieved under the RETRIEVAL_MODE_AUTOMATIC mode (a follow-up for bug 845643)
Bug 850680 - B2G MMS: broadcast "sms-received" and "sms-sent" system messages
Bug 850530 - B2G MMS: Use the same attribute name for delivery (s/state/delivery) like SMS
Bug 852911 - B2G MMS: fail to expose correct nsIDOMMozMmsMessage.attachments.
Bug 853725 - B2G MMS: fail to read nsIDOMMozMmsMessage.receivers for a received MMS (a follow-up of bug 849741).
Bug 853329 - B2G MMS: other Android phones cannot read attachments sent from FFOS
Bug 852471 - B2G MMS: provide nsIDOMMobileMessageManager interface (with sendMMS() first) (follow-up fix)
Bug 852460 - B2G MMS: provide nsIDOMMobileMessageManager.onreceived event (follow-up fix)
Bug 849741 - B2G MMS: provide nsIDOMMobileMessageManager.onreceived event
Bug 847756 - B2G MMS: provide nsIDOMMobileMessageManager.markMessageRead().
Bug 847736 - B2G MMS: provide nsIDOMMobileMessageManager.delete().
Bug 847738 - B2G MMS: provide nsIDOMMobileMessageManager.getMessage(). 
Bug 844431 - B2G MMS: provide nsIDOMMobileMessageManager interface (with sendMMS() first)
Bug 845643 - B2G MMS: Save retrieved MMS into database.

Comment 27

[Gene Lian (I already quit Mozilla)]

Following the previous comment, some more needs to back out:

Bug 792321 - Check max values of MMS parameters in sendRequest.
Bug 833291 - B2G SMS & MMS: getMessages it's not working with PhoneNumberJS
Bug 844429 - B2G SMS & MMS: move SMS codes into dom/mobilemessage to make it generic for MMS
Bug 839436 - B2G MMS: make DB be able to save MMS messages.

Comment 28

[Gene Lian (I already quit Mozilla)]

Confirming with Michael to see we should back out to Bug 839436 or just Bug 844431 (if we eventually decide to back out). Please see Bug 857632, comment #17.

Comment 29

[Gene Lian (I already quit Mozilla)]

Per off-line discussion with Michael, we decided not to back out the MMS bugs that have already been in mozilla-b2g18. Removing [NO_UPLIFT] to make the check-in status sync'ed.