Closed Bug 793137 Opened 12 years ago Closed 12 years ago

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

Categories

(Core :: DOM: Device Interfaces, defect)

Product:
Core
Component:
DOM: Device Interfaces
Platform:
ARM
Gonk (Firefox OS)
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla18
Project Flags:
blocking-basecamp +

People

(Reporter: allstars.chh, Assigned: allstars.chh)

References

Details

Attachments

(3 files, 7 obsolete files)

Part 1: IDL for PLAY_TONE, POLL_INTERVAL and REFRESH. v3
5.92 KB, patch
Details | Diff | Splinter Review
Part 2: Support PLAY_TONE, POLL_INTERVAL and REFRESH in RIL. v3
12.90 KB, patch
Details | Diff | Splinter Review
Part 3: xpcshell tests for Play Tone, Refresh and Poll Interval. v2
3.95 KB, patch
Details | Diff | Splinter Review 
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.
Assignee: nobody → allstars.chh
Blocks: b2g-stk
Status: NEW → ASSIGNED
blocking-basecamp: --- → ?
blocking-basecamp: ? → +
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.
Attachment #665381 - Flags: review?(philipp)
Comment on attachment 665382 [details] [diff] [review]
Part 2: Support PLAY_TONE, POLL_INTERVAL and REFRESH in RIL

This is about the implementations.
Attachment #665382 - Flags: review?(philipp)
Comment on attachment 665383 [details] [diff] [review]
Part 3: xpcshell tests for Play Tone, Refresh and Poll Interval

This patch is for test cases~
Attachment #665383 - Flags: review?(philipp)
Attachment #665381 - Flags: review?(philipp) → review+
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
Attachment #665382 - Flags: review?(philipp) → review+
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.
Attachment #665383 - Flags: review?(philipp) → review+
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
Attachment #665381 - Flags: superreview?(jonas)
(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.
remove whitespace addressed by philikon.
Attachment #665382 - Attachment is obsolete: true
Attachment #665381 - Flags: superreview?(jonas) → superreview+
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.
(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
added r=philikon and sr=sicking in bug title.
Attachment #665381 - Attachment is obsolete: true
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/yhuang@mozilla.com-e6fbeb30ad0c
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
Flags: needinfo?(anshulj)
Yoshi, yes this would be part of QC deliverable.
Flags: needinfo?(anshulj)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: