Implement support for STK Events

Yoshi, Beatriz: do you consider events are also a blocking-basecamp issue?

(In reply to Fernando R. Sela from comment #1) > Yoshi, Beatriz: do you consider events are also a blocking-basecamp issue? Sorry I didn't notice you asked this, yes, we need event handling.

(In reply to Fernando R. Sela from comment #2) > Created attachment 671825 [details] [diff] [review] > Patch to support location event download Seems you attached to wrong bug, this bug is for call event, but your patch is for location event.

(In reply to Yoshi Huang[:yoshi][:allstars.chh] from comment #4) > (In reply to Fernando R. Sela from comment #2) > > Created attachment 671825 [details] [diff] [review] > > Patch to support location event download > > Seems you attached to wrong bug, this bug is for call event, but your patch > is for location event. That's true, anyway my idea is to include here all event downloads instead of creating more bugs ... WDYT? If you agree, we can rename the bug. to st like: "B2G STKUI: Implement event download: Call, Location, ..."

(In reply to Fernando R. Sela from comment #5) > > That's true, anyway my idea is to include here all event downloads instead > of creating more bugs ... WDYT? > > If you agree, we can rename the bug. to st like: "B2G STKUI: Implement event > download: Call, Location, ..." I don't have any opinion here, but my suggestion is if you want to do it in one bug, you can split those events into several patches.

(In reply to Yoshi Huang[:yoshi][:allstars.chh] from comment #6) > (In reply to Fernando R. Sela from comment #5) > > > > That's true, anyway my idea is to include here all event downloads instead > > of creating more bugs ... WDYT? > > > > If you agree, we can rename the bug. to st like: "B2G STKUI: Implement event > > download: Call, Location, ..." > > I don't have any opinion here, but my suggestion is if you want to do it in > one bug, you can split those events into several patches. I agree, it's better to split in several patches ;) - I'll do it !

Remember: To merge is needed Bug #799851 before NOTE: If blocking-basecamp+ is set, just land it for now. [Approval Request Comment] Bug caused by (feature/regressing bug #): User impact if declined: Testing completed: Risk to taking this patch (and alternatives if risky):

Comment on attachment 672198 [details] [diff] [review] Patch to support location event download Review of attachment 672198 [details] [diff] [review]: ----------------------------------------------------------------- ::: apps/settings/js/icc.js @@ +267,5 @@ > + * Handle Events > + */ > + function handleLocationStatusEvent(evt) { > + if (evt.type != 'voicechange') { > + return; Self-review: NIT: Should be added datachange too :p if (evt.type != 'voicechange' && evt.type != 'datachange') { return; } This will be added in next version after vingtetun review ;)

Comment on attachment 672198 [details] [diff] [review] Patch to support location event download Review of attachment 672198 [details] [diff] [review]: ----------------------------------------------------------------- I would like to see the final version of the patch reviewed by kaze before giving approval.

Comment on attachment 672277 [details] [diff] [review] Patch to support call event download Yoshi, I added you as feedback in order to assure I'm using the STK Event Download API in the good way :)

@allstars: In the SimToolkit.idl (http://dxr.mozilla.org/mozilla-central/dom/icc/interfaces/SimToolKit.idl.html) I only see two dictionaries related to Event download: MozStkLocationEvent and MozStkCallEvent When do you plan to support the rest of events? or I can use a generic dictionary to download them ...

Not for landing, only for testing purposes ....

Per b2g-drivers, no feature work will be marked as blocking, and require driver approval to land. Marking as blocking-, please get approval from Vivien before landing.

Comment on attachment 672277 [details] [diff] [review] Patch to support call event download Review of attachment 672277 [details] [diff] [review]: ----------------------------------------------------------------- ::: apps/settings/js/icc.js @@ +238,5 @@ > case icc.STK_EVENT_TYPE_CALL_CONNECTED: > case icc.STK_EVENT_TYPE_CALL_DISCONNECTED: > + debug(' STK: Registering to communications changes event'); > + var comm = navigator.mozTelephony; > + comm.addEventListener('callschanged', handleCallsChangeEvent); maybe handleCallsChangedEvent? (Changed with d) @@ +262,4 @@ > } > > /** > + * Handle Events Handle Call Events. @@ +273,5 @@ > + debug( ' STK:CALLS State change: ' + call.state); > + var outgoing = null; > + switch (call.state) { > + case 'incoming': > + outgoing = false; STK_EVENT_TYPE_MT_CALL should be notified when receiving incoming call (MT means Mobile Terminated). @@ +278,5 @@ > + break; > + case 'dialing': > + outgoing = true; > + break; > + } Do you think if we can use outgoing = call.state == 'incoming'; @@ +282,5 @@ > + } > + call.addEventListener('error',function callError(err) { > + // MozStkCallEvent > + debug(' STK:CALL Error: ', err); > + icc.sendStkEventDownload({ No need to notify MT_CALL when error, but you need to provide this error message when disconnected, see below. @@ +305,5 @@ > + // MozStkCallEvent > + icc.sendStkEventDownload({ > + eventType: icc.STK_EVENT_TYPE_CALL_DISCONNECTED, > + number: call.number, > + isIssuedByRemote: outgoing for CALL_DISCONNECTED, you also need to provide 'error', as you did in line 289.

Thank you Yoshi. Inline (In reply to Yoshi Huang[:yoshi][:allstars.chh] from comment #16) > Comment on attachment 672277 [details] [diff] [review] > Patch to support call event download > > Review of attachment 672277 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: apps/settings/js/icc.js > @@ +238,5 @@ > > case icc.STK_EVENT_TYPE_CALL_CONNECTED: > > case icc.STK_EVENT_TYPE_CALL_DISCONNECTED: > > + debug(' STK: Registering to communications changes event'); > > + var comm = navigator.mozTelephony; > > + comm.addEventListener('callschanged', handleCallsChangeEvent); > > maybe handleCallsChangedEvent? (Changed with d) > Agree ;) > @@ +262,4 @@ > > } > > > > /** > > + * Handle Events > > Handle Call Events. > Well, under this scope I would like to add all events (location, calls, ...) so a more generic description is desirable, isn't it? > @@ +273,5 @@ > > + debug( ' STK:CALLS State change: ' + call.state); > > + var outgoing = null; > > + switch (call.state) { > > + case 'incoming': > > + outgoing = false; > > STK_EVENT_TYPE_MT_CALL should be notified when receiving incoming call > (MT means Mobile Terminated). > Yes, I know means Mobile Terminated so I assumed this message should be downloaded on errors and disconnected ... I'll add it here. > @@ +278,5 @@ > > + break; > > + case 'dialing': > > + outgoing = true; > > + break; > > + } > > Do you think if we can use outgoing = call.state == 'incoming'; > Sure! this is faster than the switch :) > @@ +282,5 @@ > > + } > > + call.addEventListener('error',function callError(err) { > > + // MozStkCallEvent > > + debug(' STK:CALL Error: ', err); > > + icc.sendStkEventDownload({ > > No need to notify MT_CALL when error, > but you need to provide this error message when disconnected, > see below. So to clarify: I should remove the onerror event and MT_CALL should be downloaded onincoming event? > > @@ +305,5 @@ > > + // MozStkCallEvent > > + icc.sendStkEventDownload({ > > + eventType: icc.STK_EVENT_TYPE_CALL_DISCONNECTED, > > + number: call.number, > > + isIssuedByRemote: outgoing > > for CALL_DISCONNECTED, you also need to provide 'error', as you did in line > 289. Oh I understood that error was only for MT_CALL ... I'll fix this.

(In reply to Fernando R. Sela from comment #17) > > > + * Handle Events > > > > Handle Call Events. > > > > Well, under this scope I would like to add all events (location, calls, ...) > so a more generic description is desirable, isn't it? > But this function is handleCallsChangedEvent, right? > > @@ +282,5 @@ > > > + } > > > + call.addEventListener('error',function callError(err) { > > > + // MozStkCallEvent > > > + debug(' STK:CALL Error: ', err); > > > + icc.sendStkEventDownload({ > > > > No need to notify MT_CALL when error, > > but you need to provide this error message when disconnected, > > see below. > > So to clarify: I should remove the onerror event and MT_CALL should be > downloaded onincoming event? > Yes

(In reply to Yoshi Huang[:yoshi][:allstars.chh] from comment #18) > (In reply to Fernando R. Sela from comment #17) > > > > + * Handle Events > > > > > > Handle Call Events. > > > > > > > Well, under this scope I would like to add all events (location, calls, ...) > > so a more generic description is desirable, isn't it? > > > But this function is handleCallsChangedEvent, right? > > Yes, you're right. > > > @@ +282,5 @@ > > > > + } > > > > + call.addEventListener('error',function callError(err) { > > > > + // MozStkCallEvent > > > > + debug(' STK:CALL Error: ', err); > > > > + icc.sendStkEventDownload({ > > > > > > No need to notify MT_CALL when error, > > > but you need to provide this error message when disconnected, > > > see below. > > > > So to clarify: I should remove the onerror event and MT_CALL should be > > downloaded onincoming event? > > > Yes Great, thanks

Please, check if I understood all your comments :p Finally I maintained the onerror event in order to get errors but in that case I download the DISCONNECT event instead MT_CALL

Comment on attachment 672738 [details] [diff] [review] Patch to support call event download V1.1 Review of attachment 672738 [details] [diff] [review]: ----------------------------------------------------------------- ::: apps/settings/js/icc.js @@ +249,5 @@ > + var outgoing = call.state == 'incoming'; > + if (call.state == 'incoming') { > + // MozStkCallEvent > + icc.sendStkEventDownload({ > + eventType: icc.STK_EVENT_TYPE_CALL_DISCONNECTED, MT_CALL? @@ +262,5 @@ > + eventType: icc.STK_EVENT_TYPE_MT_CALL, > + number: call.number, > + error: err > + }); > + }); Don't understand here, what are you trying to do? @@ +281,5 @@ > + icc.sendStkEventDownload({ > + eventType: icc.STK_EVENT_TYPE_CALL_DISCONNECTED, > + number: call.number, > + isIssuedByRemote: outgoing, > + error: null Again, please provide error @@ +283,5 @@ > + number: call.number, > + isIssuedByRemote: outgoing, > + error: null > + }); > + // TODO: Notify to the ICC TODO?

(In reply to Yoshi Huang[:yoshi][:allstars.chh] from comment #21) > Comment on attachment 672738 [details] [diff] [review] > Patch to support call event download V1.1 > > Review of attachment 672738 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: apps/settings/js/icc.js > @@ +249,5 @@ > > + var outgoing = call.state == 'incoming'; > > + if (call.state == 'incoming') { > > + // MozStkCallEvent > > + icc.sendStkEventDownload({ > > + eventType: icc.STK_EVENT_TYPE_CALL_DISCONNECTED, > > MT_CALL? > Oh shit, thats true ... copy-paste bad friend ... and the speed changing things also is a bad friend ... > @@ +262,5 @@ > > + eventType: icc.STK_EVENT_TYPE_MT_CALL, > > + number: call.number, > > + error: err > > + }); > > + }); > > Don't understand here, > what are you trying to do? On error event, the call will be disconected ... so error message will be delivered, but I'm thinking now that the evetType in this case is STK_EVENT_TYPE_CALL_DISCONNECTED, isn't it? > > @@ +281,5 @@ > > + icc.sendStkEventDownload({ > > + eventType: icc.STK_EVENT_TYPE_CALL_DISCONNECTED, > > + number: call.number, > > + isIssuedByRemote: outgoing, > > + error: null > > Again, please provide error > On disconnect event I don't have any error message, AFAIK is only on error event ... > @@ +283,5 @@ > > + number: call.number, > > + isIssuedByRemote: outgoing, > > + error: null > > + }); > > + // TODO: Notify to the ICC > > TODO? I forgot to remove this old comment... I'll review my next patch better before upload it, sorry.

Comment on attachment 672793 [details] [diff] [review] Patch to support call event download V1.2 Review of attachment 672793 [details] [diff] [review]: ----------------------------------------------------------------- Great, I don't have any further question. But I am wondering do you think can we reuse the object in sendStkEventDownload? So we don't have to create a new object each time when we call sendStkEventDownload. ::: apps/settings/js/icc.js @@ +242,5 @@ > + function handleCallsChangedEvent(evt) { > + if (evt.type != 'callschanged') { > + return; > + } > + debug(' STK Communication changed - ' + evt.type); nit, there is an extra space, right? @@ +244,5 @@ > + return; > + } > + debug(' STK Communication changed - ' + evt.type); > + navigator.mozTelephony.calls.forEach(function callIterator(call) { > + debug( ' STK:CALLS State change: ' + call.state); nit, here also extra ws. @@ +245,5 @@ > + } > + debug(' STK Communication changed - ' + evt.type); > + navigator.mozTelephony.calls.forEach(function callIterator(call) { > + debug( ' STK:CALLS State change: ' + call.state); > + var outgoing = call.state == 'incoming'; move this line to where we need it. @@ +252,5 @@ > + icc.sendStkEventDownload({ > + eventType: icc.STK_EVENT_TYPE_MT_CALL, > + number: call.number, > + isIssuedByRemote: outgoing, > + error: null For MT_CALL you don't have to provide error.

(In reply to Fernando R. Sela from comment #13 > @allstars: In the SimToolkit.idl > (http://dxr.mozilla.org/mozilla-central/dom/icc/interfaces/SimToolKit.idl. > html) > > I only see two dictionaries related to Event download: MozStkLocationEvent > and MozStkCallEvent > > When do you plan to support the rest of events? or I can use a generic > dictionary to download them ... Currently we only need these two events to run the SIM apps, (also Call events has already cover three events) so currently I don't have plan to support other events so far.

Comment on attachment 672793 [details] [diff] [review] Patch to support call event download V1.2 NOTE: If blocking-basecamp+ is set, just land it for now. [Approval Request Comment] Bug caused by (feature/regressing bug #): User impact if declined: Testing completed: Risk to taking this patch (and alternatives if risky):

Comment on attachment 672198 [details] [diff] [review] Patch to support location event download NOTE: If blocking-basecamp+ is set, just land it for now. [Approval Request Comment] Bug caused by (feature/regressing bug #): User impact if declined: Testing completed: Risk to taking this patch (and alternatives if risky):

(In reply to Yoshi Huang[:yoshi][:allstars.chh] from comment #25) > (In reply to Fernando R. Sela from comment #13 > > @allstars: In the SimToolkit.idl > > (http://dxr.mozilla.org/mozilla-central/dom/icc/interfaces/SimToolKit.idl. > > html) > > > > I only see two dictionaries related to Event download: MozStkLocationEvent > > and MozStkCallEvent > > > > When do you plan to support the rest of events? or I can use a generic > > dictionary to download them ... > > Currently we only need these two events to run the SIM apps, > (also Call events has already cover three events) > so currently I don't have plan to support other events so far. Ok, so rest of events for V2. Thank you Yoshi !

Comment on attachment 672793 [details] [diff] [review] Patch to support call event download V1.2 Review of attachment 672793 [details] [diff] [review]: ----------------------------------------------------------------- Re-Request approval when you will have a review.

Comment on attachment 672198 [details] [diff] [review] Patch to support location event download Review of attachment 672198 [details] [diff] [review]: ----------------------------------------------------------------- I would like to see the final version of the patch reviewed by kaze before giving approval.

I think this should be blocking-basecamp as per new agreement even if it is new feature work

Blocking+ -> feature required for certification. For better visibility, we're now making these bugs blockers.

(In reply to Vivien Nicolas (:vingtetun) from comment #30) > Comment on attachment 672198 [details] [diff] [review] > Patch to support location event download > > Review of attachment 672198 [details] [diff] [review]: > ----------------------------------------------------------------- > > I would like to see the final version of the patch reviewed by kaze before > giving approval. Kaze is assigned as reviewer of both patches, I'll re-ask you for approval as soon as kaze review it :)

Are these patches ready to land now?

Landed: https://github.com/mozilla-b2g/gaia/pull/6352 https://github.com/mozilla-b2g/gaia/pull/6355

I think we can close this bug. :yoshi, do you agree?