816101 - System message when a call ends

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect, P2)

Description

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

This is motivated by the need of keeping accurate statistics about call time for the telephony module of Cost Control application in order to avoid any sort of background service to preserve as much battery as possible.

So I suggest to trigger a system message every time a call ends with the information about the duration, a flag with 'outgoing'/'incoming' (I mean, the call was received or sent) and may be other relevant information.

Without system messages we need to keep a script listening on DOM APIs. If this daemon is en system we can not communicate with CostControl (despite some ugly hacks). If it is in Cost Control, it could be killed missing the calls made while not awake.

Comment 1

[Marcelino Veiga Tuimil [:sonmarce]]

With this bug together with bug #816110 it can be kept all the current functionality of Cost Control Application, but using less memory resources, as it is not necessary to have a background service running all the time.

Comment 2

[Andrew Overholt [:overholt]]

Blocking due to memory saving.

Can we also get a bug filed to stop running that Cost Control daemon?

Comment 3

[Dale Harvey (:daleharvey)]

Attachment: Send a system message when a call is complete

Not sure if you dont mind whitespace changes in patches, I prefer to fix them as we see them, but I can take them out.

The aim is send system messages only when a connected call has ended (dialing then hanging up doesnt count).

Comment 4

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

Comment on attachment 686613 [details] [diff] [review]
Send a system message when a call is complete

HsinYi is the better reviewer in telephony calls :)

Comment 5

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

Comment on attachment 686613 [details] [diff] [review]
Send a system message when a call is complete

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

I am cancelling the review until my concerns are resolved.

Why do we need to maintain existing calls in RadioInterfaceLayer.js? We already have 'currentCalls' in ril_worker.js. And we should be able to count the call duration in ril_worker.js, no?

Also, thank you for taking care of the whitespaces, though I'd prefer taking them out of the patch, to make the commit change clear. :)

Comment 6

[Dale Harvey (:daleharvey)]

You are right, this work would be cleaner done within ril_worker, are you comfortable of the approach of categorising and timing the calls based on the transition from (INCOMING|DIALLING) to (CONNECTED|HELD)? 

Will do my patches without whitespaces changes from now, but I would really appreciate no more trailing whitespace getting through review (and to fix them when in surrounding code), Its a pretty universal rule to dissallow trailing whitespace and makes patches on these files quite annoying

Will redo the patch to track the call duration in ril_worker and pass that information for RadioInterfaceLayer to trigger the system message.

Comment 7

[Dale Harvey (:daleharvey)]

Attachment: Send a system message when a call is complete

This isnt ready for the final patch, but wanted to check that you were happy with the approach, we may want to calculate duration on the stateChange, but mostly that you were happy with the statetransitions (DIALING|INCOMING) -> (ALERTING|ACTIVE|HOLDING)

Comment 8

[Andrew Overholt [:overholt]]

Setting priority based on triage discussions.  Feel free to decrease priority if you disagree.

Comment 9

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

Comment on attachment 687108 [details] [diff] [review]
Send a system message when a call is complete

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

The patch looks good. Please see some my comments, thanks you.

::: dom/system/gonk/RadioInterfaceLayer.js
@@ +1183,5 @@
> +        number: call.number,
> +        duration: new Date().getTime() - call.started,
> +        direction: call.direction
> +      };
> +      gSystemMessenger.broadcastMessage("telephony-call-ended", data);

Since this message names 'telephony-call-ended', I would prefer sending it anytime when a call ends, no matter how long the duration is. How do you think?

::: dom/system/gonk/ril_worker.js
@@ +3465,5 @@
> +              (newCall.state == CALL_STATE_ACTIVE ||
> +               newCall.state == CALL_STATE_ALERTING ||
> +               newCall.state == CALL_STATE_HOLDING)) {
> +            currentCall.started = new Date().getTime();
> +          }

I am fine with the condition here, but simply wondering your definition of 'call duration.' Does that imply when the charges start? If yes, then per my understanding, charges for a call starts at the call being answered. Do we really want to count from CALL_STATE_ALERTING? There might be different billing model per operator, please correct me if I am wrong. :)

@@ +3488,5 @@
>              newCall.number[0] != "+") {
>            newCall.number = "+" + newCall.number;
>          }
> +        // This makes the assumption that all calls will be initialised here
> +        // and that they will be in an INCOMING / DIALLING state

nit: place a period at the end of a line, please.

@@ +3494,5 @@
> +          newCall.direction = 'incoming';
> +        } else if (newCall.state == CALL_STATE_DIALING) {
> +          newCall.direction = 'outgoing';
> +        }
> +        // We dont want to deal with possible nulls later

ditto.

Comment 10

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

(In reply to Dale Harvey (:daleharvey) from comment #6)
> You are right, this work would be cleaner done within ril_worker, are you
> comfortable of the approach of categorising and timing the calls based on
> the transition from (INCOMING|DIALLING) to (CONNECTED|HELD)? 
> 
> Will do my patches without whitespaces changes from now, but I would really
> appreciate no more trailing whitespace getting through review (and to fix
> them when in surrounding code), Its a pretty universal rule to dissallow
> trailing whitespace and makes patches on these files quite annoying

I agree that we shouldn't let trailing whitespaces be there; I do appreciate you were dealing with them, thank you :) However, simply as I explained, I also think it's nice to have change commit clear. And we could file a separate bug to address those trailing whitespaces and empty lines. :)

> 
> Will redo the patch to track the call duration in ril_worker and pass that
> information for RadioInterfaceLayer to trigger the system message.

Comment 11

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

Comment on attachment 687108 [details] [diff] [review]
Send a system message when a call is complete

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

::: dom/system/gonk/RadioInterfaceLayer.js
@@ +1183,5 @@
> +        number: call.number,
> +        duration: new Date().getTime() - call.started,
> +        direction: call.direction
> +      };
> +      gSystemMessenger.broadcastMessage("telephony-call-ended", data);

Since this message names 'telephony-call-ended', I would prefer sending it anytime when a call ends, no matter how long the duration is.

Comment 12

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

If you want, instead of computing the duration, send the duration of every state change. With this, I could count the time taking in count the operator polish about billing. It is a more general and versatile way to do things. x)

Comment 13

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

*polish -> policy

Comment 14

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

(In reply to Salvador de la Puente González [:salva] from comment #12)
> If you want, instead of computing the duration, send the duration of every
> state change. 

You meant sending system message whenever call state changes?

> With this, I could count the time taking in count the operator
> polish about billing. It is a more general and versatile way to do things. x)

Comment 15

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

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #14)
> You meant sending system message whenever call state changes?

No, I was suggesting only one system message including a summary of states and durations. Nothing more.

Comment 16

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

Comment on attachment 687108 [details] [diff] [review]
Send a system message when a call is complete

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

::: dom/system/gonk/ril_worker.js
@@ +3465,5 @@
> +              (newCall.state == CALL_STATE_ACTIVE ||
> +               newCall.state == CALL_STATE_ALERTING ||
> +               newCall.state == CALL_STATE_HOLDING)) {
> +            currentCall.started = new Date().getTime();
> +          }

I asked for other colleagues' opinions. Per their experiences in call charging, the call duration starts from 'CALL_STATE_ACTIVE.'

Comment 17

[Dale Harvey (:daleharvey)]

Agreed that it should send even if duration is 0, will fix

Now should I edit the patch to only start counting from active, or to maintain a list of state changes with their timestamps and the duration and the client can figure out how long it took?

My worry with the latter is leaking the constants from RIL, which may be extra burden on the API, my worry about the former is its a very single use case API. 

Hsin, I am happy to do either? I expect counting from ACTIVE fulfills what we need and doesnt add much burden so I would guess that way, will wait for what you think before doing a follow up patch

Comment 18

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

(In reply to Dale Harvey (:daleharvey) from comment #17)
> Agreed that it should send even if duration is 0, will fix
> 
> Now should I edit the patch to only start counting from active, or to
> maintain a list of state changes with their timestamps and the duration and
> the client can figure out how long it took?
> 
> My worry with the latter is leaking the constants from RIL, which may be
> extra burden on the API, my worry about the former is its a very single use
> case API. 
>
> Hsin, I am happy to do either? I expect counting from ACTIVE fulfills what
> we need and doesnt add much burden so I would guess that way, will wait for
> what you think before doing a follow up patch
 
Dale, thanks for pointing the details that I also have in mind. :) Same as you, I prefer counting from ACTIVE if we do need the duration information coming with this system message. I don't want to expose that much and add that cost in RIL, especially the list of state change can be easily obtained by APP through WebTelephony API.

Comment 19

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

I agree with the solution more battery-friendly but it could be great to introduce a non-user-customizable FLAG to allow further details of time duration. Maybe for V2 we could enable these details for advanced devices.

Comment 20

[Dale Harvey (:daleharvey)]

Attachment: Bug 816101 - Send a system message when a call is complete

Comment 21

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

Comment on attachment 690238 [details] [diff] [review]
Bug 816101 - Send a system message when a call is complete

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

The patch looks great! Thank you for this :)
Before pushing it to inbound, please make sure that it won't break telephony marionette tests. I believe it won't but we never know ;-)

Comment 22

[[:fabrice] Fabrice Desré]

Attachment: updated patch

Updated patch to add the permission check in SystemMessagePermissionsChecker.jsm

Is there anything preventing this to land?

Comment 23

[Dale Harvey (:daleharvey)]

Nope, tested the updated patch and is working here, tagged it checkin-needed (I cant check stuff in)

Comment 24

[Fernando Jiménez Moreno [:ferjm]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2755715976f0

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/2755715976f0

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/443cf9f1bf3b
https://hg.mozilla.org/releases/mozilla-b2g18/rev/cc39d1ace23d

Comment 27

[Philipp von Weitershausen [:philikon]]

Comment on attachment 692853 [details] [diff] [review]
updated patch

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

::: dom/system/gonk/RadioInterfaceLayer.js
@@ +1242,5 @@
>    handleCallDisconnected: function handleCallDisconnected(call) {
>      debug("handleCallDisconnected: " + JSON.stringify(call));
>      call.state = nsIRadioInterfaceLayer.CALL_STATE_DISCONNECTED;
> +    let duration = ("started" in call && typeof call.started == "number") ?
> +      new Date().getTime() - call.started : 0;

Why not Date.now()? Should be quicker and doesn't allocate a new object.

(Sorry for the late drive-by.)