B2G STKUI: Implement event download (several events)

RESOLVED FIXED

Status

Firefox OS
Gaia::Settings
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: frsela, Assigned: frsela)

Tracking

({feature})

unspecified
x86
Gonk (Firefox OS)
feature
Dependency tree / graph

Firefox Tracking Flags

(blocking-basecamp:+)

Details

(URL)

Attachments

(3 attachments, 3 obsolete attachments)

Implement support for STK Events
Depends on: 791935
Yoshi, Beatriz: do you consider events are also a blocking-basecamp issue?
blocking-basecamp: --- → ?
Assignee: nobody → frsela
Created attachment 671825 [details] [diff] [review]
Patch to support location event download
(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 !
Summary: B2G STKUI: Implement event download: MT Call, Call Connected and Call disconnected → B2G STKUI: Implement event download (several events)
Created attachment 672198 [details] [diff] [review]
Patch to support location event download

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):
Attachment #671825 - Attachment is obsolete: true
Attachment #672198 - Flags: review?(21)
Attachment #672198 - Flags: approval-gaia-master?(21)
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.
Attachment #672198 - Flags: review?(kaze)
Attachment #672198 - Flags: review?(21)
Attachment #672198 - Flags: approval-gaia-master?(21)
Created attachment 672277 [details] [diff] [review]
Patch to support call event download
Attachment #672277 - Flags: review?
Attachment #672277 - Flags: review? → review?(kaze)
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 :)
Attachment #672277 - Flags: feedback?(allstars.chh)
@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 ...
Created attachment 672299 [details] [diff] [review]
Patch to register events on handsets which don't support them. (Fake events)

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.
blocking-basecamp: ? → -
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.
Attachment #672277 - Flags: feedback?(allstars.chh)
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
Created attachment 672738 [details] [diff] [review]
Patch to support call event download V1.1

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
Attachment #672277 - Attachment is obsolete: true
Attachment #672277 - Flags: review?(kaze)
Attachment #672738 - Flags: review?(allstars.chh)
Attachment #672738 - Flags: review?(allstars.chh) → review?(allstars.chh)
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?
Attachment #672738 - Flags: review?(allstars.chh)
(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.
Created attachment 672793 [details] [diff] [review]
Patch to support call event download V1.2
Attachment #672738 - Attachment is obsolete: true
Attachment #672793 - Flags: feedback?(allstars.chh)
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.
Attachment #672793 - Flags: feedback?(allstars.chh) → feedback+
(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.
Attachment #672793 - Attachment description: Patch to supoprt Events registration V1.2 → Patch to support call event download V1.2
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):
Attachment #672793 - Flags: review?(kaze)
Attachment #672793 - Flags: approval-gaia-master?(21)
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):
Attachment #672198 - Flags: approval-gaia-master?(21)
(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.
Attachment #672793 - Flags: approval-gaia-master?(21) → approval-gaia-master-
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.
Attachment #672198 - Flags: approval-gaia-master?(21)
I think this should be blocking-basecamp as per new agreement even if it is new feature work
blocking-basecamp: - → ?
Blocking+ -> feature required for certification. For better visibility, we're now making these bugs blockers.
blocking-basecamp: ? → +
Keywords: feature
(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 :)
Flags: needinfo?(kaze)
Attachment #672198 - Flags: review?(kaze) → review+
Attachment #672793 - Flags: review?(kaze) → review+
Are these patches ready to land now?

Updated

6 years ago
Component: Gaia → Gaia::Settings
Flags: approval-gaia-master-
I think we can close this bug. :yoshi, do you agree?
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Flags: needinfo?(kaze) needinfo?(kaze) → needinfo+
Blocks: 819528
You need to log in before you can comment on or make changes to this bug.