816416 - B2G SMS: Use system messages to notify sent sms events

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

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

See bug 816110, emit system messages for SMS onsent in order to remove CostControl background service. See also bug 782489.

Comment 1

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

BB? for blocking bug 816110, a bb+ bug.

Comment 2

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Vicamo: How is this bug different from bug 816110?

Comment 3

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

This bug is for Gecko, and bug 816110 is for Gaia.

Comment 4

[Salvador de la Puente González [:salva]]

(In reply to Jonas Sicking (:sicking) from comment #2)
> Vicamo: How is this bug different from bug 816110?

It is not different, this only does not appear on possible duplicated. You can close the other one or add this as blocking the other one. ;)

Comment 5

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

You guys are sending mixed messages :)

If both this one and bug 816110 covers the gecko side of things, is there another bug filed on the gaia changes that are needed?

Comment 6

[Dale Harvey (:daleharvey)]

Attachment: Send system message on sms being sent

This sends more information than the cost control application necessarily needs, but I thought it would be a good idea to keep reasonably symmetrical with the sms-received system messages, the only difference is the additional segmentsMaxSeq property

Comment 7

[Salvador de la Puente González [:salva]]

Ok, I understand. So this fixes the problem in Gecko and now, the bug #816110 says that Cost Control should use it. Is this what you mean? If so, as Vicamo says, then they are two different bugs.

Comment 8

[Lawrence Mandel [:lmandel] (use needinfo)]

Marking as a soft blocker. We may need to revisit this decision based on the decision for bug 816110.

Comment 9

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Comment on attachment 687063 [details] [diff] [review]
Send system message on sms being sent

Refer the review to Vicamo because he knows much more sms impl. than I :)

Comment 10

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

Comment on attachment 687063 [details] [diff] [review]
Send system message on sms being sent

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

Thank you for the patch, but please don't make any differences to SystemMessage and SmsManager events.

::: dom/system/gonk/RadioInterfaceLayer.js
@@ +1370,5 @@
> +    gSystemMessenger.broadcastMessage("sms-sent",
> +                                      {id: id,
> +                                       delivery: DOM_SMS_DELIVERY_SENT,
> +                                       deliveryStatus: RIL.GECKO_SMS_DELIVERY_STATUS_PENDING,
> +                                       receiver: options.number,

sender: message.sender || null,

@@ +1375,5 @@
> +                                       body: options.fullBody,
> +                                       messageClass: messageClass,
> +                                       timestamp: timestamp,
> +                                       read: true,
> +                                       segmentMaxSeq: options.segmentMaxSeq});

You don't need this. There is already a getNumberOfMessagesForText() API. I know it's stupid to invoke it again, but I don't really want to have any differences between messages got from SmsEvent and SystemMessage. If you feel like it's something necessary, please fire a new bug so that we can include this attribute in every received/sent messages and benefits every application despite of the event target registered.

@@ +2306,5 @@
>      options.envelopeId = this.createSmsEnvelope({request: request,
>                                                   number: options.number,
>                                                   fullBody: options.fullBody,
> +                                                 requestStatusReport: options.requestStatusReport,
> +                                                 segmentMaxSeq: options.segmentMaxSeq});

Ditto

Comment 11

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to Lawrence Mandel [:lmandel] from comment #8)
> Marking as a soft blocker. We may need to revisit this decision based on the
> decision for bug 816110.

It has been marked as blocking. Renominating.

Comment 12

[Andrew Overholt [:overholt]]

Okay, based on the decision here and in the dependent bugs, blocking on this.

Comment 13

[Jason Smith [:jsmith]]

(In reply to Andrew Overholt [:overholt] from comment #12)
> Okay, based on the decision here and in the dependent bugs, blocking on this.

What priority? P1 or P2 or P3?

Comment 14

[Dale Harvey (:daleharvey)]

Nice thanks Vicamo, I should have realised that function was exposed to web content, patch is getting simpler :)

Comment 15

[Dale Harvey (:daleharvey)]

Attachment: Send system message on sms being sent

Comment 16

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

Comment on attachment 688746 [details] [diff] [review]
Send system message on sms being sent

Thank you so much :)

Comment 17

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/09a07d2fbd23

Comment 18

[Alex Keybl [:akeybl]]

Mass Modify: All un-milestoned, unresolved blocking-basecamp+ bugs are being moved into the C3 milestone. Note that the target milestone does not mean that these bugs can't be resolved prior to 12/10, rather C2 bugs should be prioritized ahead of C3 bugs.

Comment 19

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/09a07d2fbd23

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/1ea6bf56eb87
https://hg.mozilla.org/releases/mozilla-beta/rev/343b4240a4a0