793137 - B2G STK: Support Proactive Command 'Play Tone', 'Refresh' and 'Poll Interval'

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

We also need to support some proactive commands 
- Play Tone
- Refresh
- Poll Interval

As these commands will be used in the SIM apps from our partners.

Comment 1

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: WIP - Part 1: IDL for PLAY_TONE, POLL_INTERVAL and REFRESH

Comment 2

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: WIP - Part 2: Support PLAY_TONE, POLL_INTERVAL and REFRESH in RIL

Comment 3

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Part 1: IDL for PLAY_TONE, POLL_INTERVAL and REFRESH. v2

Comment 4

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Part 2: Support PLAY_TONE, POLL_INTERVAL and REFRESH in RIL

Comment 5

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Part 3: xpcshell tests for Play Tone, Refresh and Poll Interval

Comment 6

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Comment on attachment 665381 [details] [diff] [review]
Part 1: IDL for PLAY_TONE, POLL_INTERVAL and REFRESH. v2

Hi, philikon
This patch is for supporting the proactive commands needed by our partner.

Also in MozDuration will be reused in other places, so I didn't put its members in MozStkPlayTone to make it a flat object.

Would you review this for me ?

Thank you.

Comment 7

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Comment on attachment 665382 [details] [diff] [review]
Part 2: Support PLAY_TONE, POLL_INTERVAL and REFRESH in RIL

This is about the implementations.

Comment 8

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Comment on attachment 665383 [details] [diff] [review]
Part 3: xpcshell tests for Play Tone, Refresh and Poll Interval

This patch is for test cases~

Comment 9

[Philipp von Weitershausen [:philikon]]

Comment on attachment 665382 [details] [diff] [review]
Part 2: Support PLAY_TONE, POLL_INTERVAL and REFRESH in RIL

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

::: dom/system/gonk/ril_worker.js
@@ +5745,5 @@
> +      case STK_CMD_REFRESH:
> +        param = this.processRefresh(cmdDetails, ctlvs);
> +        break;
> +      case STK_CMD_POLL_INTERVAL:
> +        param = this.processPollInterval(cmdDetails, ctlvs);

This `switch` block is feeling very C-like. I think at some point we should refactor StkCommandParamsFactory to be more like the RIL object, so that we can just do:

  let method = StkCommandParamsFactory[cmdDetails.typeCommand];
  method.call(this, cmdDetails, ctlvs);

and have

  StkCommandParamsFactory[STK_CMD_REFRESH] = function processRefresh(...) {
    ...
  };

etc. Maybe file a follow-up bug for this that we can tackle after v1?

@@ +6141,5 @@
>          return this.retrieveDeviceId(length);
>        case COMPREHENSIONTLV_TAG_ALPHA_ID:
>          return this.retrieveAlphaId(length);
> +      case COMPREHENSIONTLV_TAG_DURATION:
> +        return this.retrieveDuration(length);

Same thing what I said above.

@@ +6234,5 @@
> +   * |  4   | Time interval         |   1    |
> +   */
> +  retrieveDuration: function retrieveDuration(length) {
> +    let duration = {
> +      timeUnit :GsmPDUHelper.readHexOctet(),

Nit: whitespace

Comment 10

[Philipp von Weitershausen [:philikon]]

Comment on attachment 665383 [details] [diff] [review]
Part 3: xpcshell tests for Play Tone, Refresh and Poll Interval

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

I like tests.

Comment 11

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Comment on attachment 665381 [details] [diff] [review]
Part 1: IDL for PLAY_TONE, POLL_INTERVAL and REFRESH. v2

Hi, Jonas
This patch is added to support more proactive commands sent from ICC.
Could you review this for me ?

Thanks

Comment 12

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

(In reply to Philipp von Weitershausen [:philikon] from comment #9)
> ::: dom/system/gonk/ril_worker.js
> @@ +5745,5 @@
> > +      case STK_CMD_REFRESH:
> > +        param = this.processRefresh(cmdDetails, ctlvs);
> > +        break;
> > +      case STK_CMD_POLL_INTERVAL:
> > +        param = this.processPollInterval(cmdDetails, ctlvs);
> 
> This `switch` block is feeling very C-like. I think at some point we should
> refactor StkCommandParamsFactory to be more like the RIL object, so that we
> can just do:
> 
>   let method = StkCommandParamsFactory[cmdDetails.typeCommand];
>   method.call(this, cmdDetails, ctlvs);
> 
> and have
> 
>   StkCommandParamsFactory[STK_CMD_REFRESH] = function processRefresh(...) {
>     ...
>   };
> 
> etc. Maybe file a follow-up bug for this that we can tackle after v1?
> 

Hi, philikon :
Thanks for your comments,
filed a bug 795252 for this. 

Thank you.

Comment 13

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Part 2: Support PLAY_TONE, POLL_INTERVAL and REFRESH in RIL. v2

remove whitespace addressed by philikon.

Comment 14

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

Sorry guys, I really wish I could be more helpful here but I haven't been able to keep enough of the STK in my head to have very strong opinions on things here. If you guys want I'm more than happy to have a more overarching look at the various APIs involved at some point, in the meantime this stuff looks totally sensible to me.

Comment 15

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

(In reply to Jonas Sicking (:sicking) from comment #14)
> Sorry guys, I really wish I could be more helpful here but I haven't been
> able to keep enough of the STK in my head to have very strong opinions on
> things here. If you guys want I'm more than happy to have a more overarching
> look at the various APIs involved at some point, in the meantime this stuff
> looks totally sensible to me.

Thanks, Jonas :)
Your comments will always be appreciated~

Thank you

Comment 16

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Part 1: IDL for PLAY_TONE, POLL_INTERVAL and REFRESH. v3

added r=philikon and sr=sicking in bug title.

Comment 17

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Part 2: Support PLAY_TONE, POLL_INTERVAL and REFRESH in RIL. v3

rebase

Comment 18

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Part 3: xpcshell tests for Play Tone, Refresh and Poll Interval

added r=philikon.

Comment 19

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Part 3: xpcshell tests for Play Tone, Refresh and Poll Interval. v2

upload again.

Comment 20

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b4d33b66bfba
https://hg.mozilla.org/integration/mozilla-inbound/rev/e60e324d1fa3
https://hg.mozilla.org/integration/mozilla-inbound/rev/bf8b676ae8d2

Comment 21

[Mozilla RelEng Bot]

Try run for e6fbeb30ad0c is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=e6fbeb30ad0c
Results (out of 290 total builds):
    exception: 1
    success: 272
    warnings: 16
    failure: 1
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-e6fbeb30ad0c

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/b4d33b66bfba
https://hg.mozilla.org/mozilla-central/rev/e60e324d1fa3
https://hg.mozilla.org/mozilla-central/rev/bf8b676ae8d2

Comment 23

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Hi, Anshul
Sorry to ask you for this old bug,
I wonder if the modem already handles the 'POLL_INTERVAL' and 'POLL_OFF' proactive commands, so actually gecko nor gaia need to handle this?

Thanks

Comment 24

[Anshul]

Yoshi, yes this would be part of QC deliverable.