Last Comment Bug 827831 - B2G STK: Support variable timeout for GET INKEY
: B2G STK: Support variable timeout for GET INKEY
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM: Device Interfaces (show other bugs)
: unspecified
: ARM Gonk (Firefox OS)
: -- normal (vote)
: mozilla21
Assigned To: psiddh
:
Mentors:
Depends on:
Blocks: b2g-stk
  Show dependency treegraph
 
Reported: 2013-01-08 07:29 PST by Yoshi Huang[:allstars.chh]
Modified: 2013-01-24 17:41 PST (History)
8 users (show)
ryanvm: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
-


Attachments
Interface updated to decode 'duration' TLV tag for GetInkey (11.87 KB, text/plain)
2013-01-09 05:54 PST, psiddh
psiddh: review+
Details
Part 2: Decode comprehension TLV tag - Duration for GET INKEY variable time out GCF T.C (879 bytes, patch)
2013-01-09 05:56 PST, psiddh
allstars.chh: review+
Details | Diff | Splinter Review
Part 1 : Interface updated to decode 'duration' TLV tag for GetInkey (910 bytes, patch)
2013-01-09 05:58 PST, psiddh
allstars.chh: review+
jonas: superreview+
Details | Diff | Splinter Review
Part 3: Marionette test case for GET_INKEY with timeout (1.94 KB, patch)
2013-01-09 08:23 PST, Yoshi Huang[:allstars.chh]
allstars.chh: review+
Details | Diff | Splinter Review
Part 1 : Interface updated to decode 'duration' TLV tag for GetInkey (1014 bytes, patch)
2013-01-22 12:00 PST, psiddh
no flags Details | Diff | Splinter Review
Part 1 : Interface updated to decode 'duration' TLV tag for GetInkey (1007 bytes, patch)
2013-01-22 18:24 PST, psiddh
no flags Details | Diff | Splinter Review
Part 3: Marionette test case for GET_INKEY with timeout. v2 (1.99 KB, patch)
2013-01-22 18:44 PST, Yoshi Huang[:allstars.chh]
no flags Details | Diff | Splinter Review
Part 3: Marionette test case for GET_INKEY with timeout. v2 (1.88 KB, patch)
2013-01-23 12:13 PST, psiddh
no flags Details | Diff | Splinter Review

Description Yoshi Huang[:allstars.chh] 2013-01-08 07:29:09 PST
See TS 101.223 clause 6.4.2
if the UICC requests a variable timeout, the terminal shall wait until either the user enters a single character or
the timeout expires.
Comment 1 Yoshi Huang[:allstars.chh] 2013-01-09 00:58:02 PST
Hi, Siddarth
I think this bug needs at least 3 patches,
The 1st part is for modifying IDL (dom/icc/interfaces/SimToolKit.idl
The 2nd is for ril_worker
and the last is the marionette test case.

Let me know if you need any help.
Comment 2 psiddh 2013-01-09 05:54:43 PST
Created attachment 699741 [details]
Interface updated to decode 'duration' TLV tag for GetInkey
Comment 3 psiddh 2013-01-09 05:56:14 PST
Created attachment 699743 [details] [diff] [review]
Part 2: Decode comprehension TLV tag - Duration for GET INKEY variable time out GCF T.C
Comment 4 psiddh 2013-01-09 05:58:19 PST
Created attachment 699745 [details] [diff] [review]
Part 1 :  Interface updated to decode 'duration' TLV tag for GetInkey
Comment 5 Yoshi Huang[:allstars.chh] 2013-01-09 08:23:50 PST
Created attachment 699845 [details] [diff] [review]
Part 3: Marionette test case for GET_INKEY with timeout

Help Siddartha to upload the patch.
Comment 6 psiddh 2013-01-09 08:31:50 PST
Thanks Yoshi for uploading
Comment 7 Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please) 2013-01-09 13:31:05 PST
I suspect this one should block since this is the platform side of a Gaia blocker.
Comment 8 Yoshi Huang[:allstars.chh] 2013-01-09 14:33:07 PST
(In reply to Vivien Nicolas (:vingtetun) from comment #7)
> I suspect this one should block since this is the platform side of a Gaia
> blocker.

No, 
Bug 827831 is for GET_INKEY, timeout value could be provided by SIM.
So this bug is filed to parse the timeout value from SIM and pass it to gaia.

On the other hand,
Bug 827655 is for gaia part of GET_INPUT, which SIM doesn't provide the timeout value, so we need to configurable timeout value in gaia.
Comment 9 Andrew Overholt [:overholt] 2013-01-10 01:30:34 PST
Is this a certification requirement, Yoshi?
Comment 10 Yoshi Huang[:allstars.chh] 2013-01-10 01:32:18 PST
Hi, Andrew 
No, this is just an optional feature we think we could do during work week.
It doesn't related to any requirement or certification.
Comment 11 Andrew Overholt [:overholt] 2013-01-10 02:15:51 PST
(In reply to Yoshi Huang[:yoshi][:allstars.chh] from comment #10)
> No, this is just an optional feature we think we could do during work week.
> It doesn't related to any requirement or certification.

Okay, thanks for the info.

Vivien, what Gaia bug does this block?
Comment 12 Alexandre Poirot [:ochameau] PTO, back on 1st 2013-01-10 02:25:43 PST
(In reply to Andrew Overholt [:overholt] from comment #11)
> (In reply to Yoshi Huang[:yoshi][:allstars.chh] from comment #10)
> > No, this is just an optional feature we think we could do during work week.
> > It doesn't related to any requirement or certification.
> 
> Okay, thanks for the info.
> 
> Vivien, what Gaia bug does this block?

Vivien thought that this platform bug was necessary to fix gaia bug 827655 which is bb+, but that's not the case.
Instead, we added support for the new property added in this platform bug (duration for INKEY).
So gaia bug 827655 allows to use feature implemented in this gecko bug (827831) even if it isn't the key to fix the original gaia issue. I'd say there is a weak link between both bugs that doesn't forward blocking status to the platform bug.
Comment 13 Andrew Overholt [:overholt] 2013-01-10 02:37:05 PST
(In reply to Alexandre Poirot (:ochameau) from comment #12)
> (In reply to Andrew Overholt [:overholt] from comment #11)
> > (In reply to Yoshi Huang[:yoshi][:allstars.chh] from comment #10)
> > > No, this is just an optional feature we think we could do during work week.
> > > It doesn't related to any requirement or certification.
> > 
> > Okay, thanks for the info.
> > 
> > Vivien, what Gaia bug does this block?
> 
> Vivien thought that this platform bug was necessary to fix gaia bug 827655
> which is bb+, but that's not the case.
> Instead, we added support for the new property added in this platform bug
> (duration for INKEY).
> So gaia bug 827655 allows to use feature implemented in this gecko bug
> (827831) even if it isn't the key to fix the original gaia issue. I'd say
> there is a weak link between both bugs that doesn't forward blocking status
> to the platform bug.

Okay, thanks for the clarification.  I've marked this as a soft blocker but please re-nom if that isn't an appropriate status.
Comment 14 Yoshi Huang[:allstars.chh] 2013-01-16 18:07:20 PST
Comment on attachment 699745 [details] [diff] [review]
Part 1 :  Interface updated to decode 'duration' TLV tag for GetInkey

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

Hi, Siddarth
Can you revise the comments as I addressed below, also add sr=sicking in your bug comment ?

Thanks

::: dom/icc/interfaces/SimToolKit.idl
@@ +118,4 @@
>     */
>    DOMString text;
>  
> +  /** The length of time for which the ME shall display the dialog.

Hi Siddartha, 
Sorry seems I didn't notice during our work week, this line should be moved to below.

/**
 * The length ...

@@ +118,5 @@
>     */
>    DOMString text;
>  
> +  /** The length of time for which the ME shall display the dialog.
> +   *

Also can you add another comment for this property is only available for GET_INKEY?
Comment 15 psiddh 2013-01-17 07:02:39 PST
Sure Yoshi, not sure why this 'DOMString' is there, but yes I did mention in the comments  the exact GCF T.C that needs this
- "@see TS 11.14, clause 11.8, duration, GET INKEY T.C 27.22.4.2.8.4.2"
Comment 16 Yoshi Huang[:allstars.chh] 2013-01-17 07:28:44 PST
Comment on attachment 699745 [details] [diff] [review]
Part 1 :  Interface updated to decode 'duration' TLV tag for GetInkey

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

::: dom/icc/interfaces/SimToolKit.idl
@@ +123,5 @@
> +   * @see TS 11.14, clause 11.8, duration, GET INKEY T.C 27.22.4.2.8.4.2
> +   *
> +   */
> +  jsval duration;
> +

Hi, Siddartha
I mean

/**
 * The length of time for which the ME shall display the dialog.
 *  
 *  .. And mention this property is only for GET_INKEY 
 *  @see TS 11.14, clause 11.8, duration, GET INKEY T.C 27.22.4.2.8.4.2
 */
Comment 17 psiddh 2013-01-22 12:00:21 PST
Created attachment 705040 [details] [diff] [review]
Part 1 : Interface updated to decode 'duration' TLV tag for GetInkey
Comment 18 Yoshi Huang[:allstars.chh] 2013-01-22 17:56:39 PST
Comment on attachment 705040 [details] [diff] [review]
Part 1 : Interface updated to decode 'duration' TLV tag for GetInkey

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

Hi, Siddartha 
You already got r+ and sr+ so you don't have to request for review again.

::: dom/icc/interfaces/SimToolKit.idl
@@ +122,5 @@
> +   * The length of time for which the ME shall display the dialog. This field
> +   * is used only for GET INKEY.
> +   *
> +   * @see TS 11.14, clause 11.8, duration, GET INKEY T.C 27.22.4.2.8.4.2
> +   *

extra line here.
Comment 19 psiddh 2013-01-22 18:24:25 PST
Created attachment 705190 [details] [diff] [review]
Part 1 :  Interface updated to decode 'duration' TLV tag for GetInkey
Comment 20 Yoshi Huang[:allstars.chh] 2013-01-22 18:35:40 PST
Great, thanks !
I'll push your patches.
Comment 21 Yoshi Huang[:allstars.chh] 2013-01-22 18:44:32 PST
Created attachment 705199 [details] [diff] [review]
Part 3: Marionette test case for GET_INKEY with timeout. v2

Rebase to avoid conflict.
Comment 22 Yoshi Huang[:allstars.chh] 2013-01-22 18:46:12 PST
Oh, seems mozilla-inbound is closed now, I'll push later.
Comment 24 Ryan VanderMeulen [:RyanVM] 2013-01-23 04:48:58 PST
Backed out for Marionette orange.
https://hg.mozilla.org/integration/mozilla-inbound/rev/ec09cdff4e48

https://tbpl.mozilla.org/php/getParsedLog.php?id=19040992&tree=Mozilla-Inbound

23:29:55     INFO -  TEST-START test_stk_proactive_command.js
23:29:56     INFO -  /home/cltbld/talos-slave/test/build/tests/marionette/tests/dom/icc/tests/marionette/test_stk_proactive_command.js, runTest (marionette_test.MarionetteJSTestCase) ... ERROR
23:29:56     INFO -  ======================================================================
23:29:56     INFO -  ERROR: /home/cltbld/talos-slave/test/build/tests/marionette/tests/dom/icc/tests/marionette/test_stk_proactive_command.js, runTest (marionette_test.MarionetteJSTestCase)
23:29:56     INFO -  ----------------------------------------------------------------------
23:29:56    ERROR -  Traceback (most recent call last):
23:29:56     INFO -    File "/home/cltbld/talos-slave/test/build/tests/marionette/marionette/marionette_test.py", line 213, in runTest
23:29:56     INFO -      results = self.marionette.execute_js_script(js, args, special_powers=True)
23:29:56     INFO -    File "/home/cltbld/talos-slave/test/build/tests/marionette/marionette/marionette.py", line 471, in execute_js_script
23:29:56     INFO -      specialPowers=special_powers)
23:29:56     INFO -    File "/home/cltbld/talos-slave/test/build/tests/marionette/marionette/marionette.py", line 243, in _send_message
23:29:56     INFO -      self._handle_error(response)
23:29:56     INFO -    File "/home/cltbld/talos-slave/test/build/tests/marionette/marionette/marionette.py", line 278, in _handle_error
23:29:56    ERROR -      raise JavascriptException(message=message, status=status, stacktrace=stacktrace)
23:29:56    ERROR -  TEST-UNEXPECTED-FAIL | test_stk_proactive_command.js | JavascriptException: TypeError: cmd.duration is undefined
23:29:56     INFO -  START LOG:
23:29:56     INFO -  INFO TEST-START: /home/cltbld/talos-slave/test/build/tests/marionette/tests/dom/icc/tests/marionette/test_stk_proactive_command.js Tue Jan 22 2013 20:29:44 GMT-1100 (SST)
23:29:56     INFO -  INFO STK CMD {"commandNumber":1,"typeOfCommand":34,"commandQualifier":0,"rilMessageType":"stkcommand","options":{"text":"Enter \"+\"","duration":{"timeUnit":1,"timeInterval":10},"minLength":1,"maxLength":1}} Tue Jan 22 2013 20:29:44 GMT-1100 (SST)
23:29:56     INFO -  END LOG:
23:29:56     INFO -  ----------------------------------------------------------------------
23:29:56     INFO -  Ran 1 test in 0.923s
23:29:56    ERROR -  FAILED (errors=1)
Comment 25 Yoshi Huang[:allstars.chh] 2013-01-23 05:45:21 PST
Sorry for causing trouble, Ryan.

Hi, Siddartha
Can you run the test again ?
Also, you need to apply an account for Mozilla Try server,
See Garner's example in Bug 810157.
Please also file a bug and CC me, I'll vouch for you.
Comment 26 psiddh 2013-01-23 11:48:12 PST
Ryan , Yoshi  I am sorry for the trouble caused by this patch .

Yoshi, This marionette test case , I was hoping that you would help me in testing and landing as I couldn't test it on the setup I had during the work week. ( Also I couldn't sync to the latest code base as well)

I do have access to try server and I have been vouched already.

I have now managed to test the marionette test case by tweaking my emulator setup and I was able to see the error. I have now fixed the ' Part 3: Marionette test case for GET_INKEY with timeout. v2' with the fix. Please review and do the needful
Comment 27 psiddh 2013-01-23 12:09:30 PST
Comment on attachment 705199 [details] [diff] [review]
Part 3: Marionette test case for GET_INKEY with timeout. v2

># HG changeset patch
># Parent 49b02c973756d7c954c4d4c1193cb5b02b35acda
># User Siddartha P <psiddh@gmail.com>
># Date 1357747996 -3600
>
>Bug 827831 - Part 3: [B2G STK]: Marionette test case. r=allstars.chh
>
>diff --git a/dom/icc/tests/marionette/test_stk_proactive_command.js b/dom/icc/tests/marionette/test_stk_proactive_command.js
>--- a/dom/icc/tests/marionette/test_stk_proactive_command.js
>+++ b/dom/icc/tests/marionette/test_stk_proactive_command.js
>@@ -125,16 +125,25 @@ function testTimerManagementGetCurrentVa
>   is(cmd.commandNumber, 0x01);
>   is(cmd.commandQualifier, icc.STK_TIMER_GET_CURRENT_VALUE);
>   is(cmd.options.timerAction, icc.STK_TIMER_GET_CURRENT_VALUE);
>   is(cmd.options.timerId, 0x08);
> 
>   runNextTest();
> }
> 
>+function testGetInKeyVariableTimeout(cmd) {
>+  log("STK CMD " + JSON.stringify(cmd));
>+  is(cmd.typeOfCommand, icc.STK_CMD_GET_INKEY);
>+  is(cmd.options.duration.timeUnit, 0x01);
>+  is(cmd.options.duration.timeInterval, 0x0A);
>+
>+  runNextTest();
>+}
>+
> function testSetupCall(cmd) {
>   log("STK CMD " + JSON.stringify(cmd));
>   is(cmd.typeOfCommand, icc.STK_CMD_SET_UP_CALL);
>   is(cmd.commandNumber, 0x01);
>   is(cmd.commandQualifier, 0x04);
>   is(cmd.options.address, "012340123456,1,2");
>   is(cmd.options.confirmMessage, "Disconnect");
>   is(cmd.options.callMessage, "Message");
>@@ -164,16 +173,18 @@ let tests = [
>   {command: "d011810301270082028182a40101a503102030",
>    func: testTimerManagementStart},
>   {command: "d00c810301270182028182a40104",
>    func: testTimerManagementDeactivate},
>   {command: "d00c810301270282028182a40108",
>    func: testTimerManagementGetCurrentValue},
>   {command: "d029810301100482028182050a446973636f6e6e6563748609811032042143651c2c05074d657373616765",
>    func: testSetupCall},
>+  {command: "D0198103012200820281828D0A04456E74657220222B228402010A",
>+   func: testGetInKeyVariableTimeout},
> ];
> 
> let pendingEmulatorCmdCount = 0;
> function sendStkPduToEmulator(cmd, func) {
>   ++pendingEmulatorCmdCount;
> 
>   runEmulatorCmd(cmd, function (result) {
>     --pendingEmulatorCmdCount;
Comment 28 psiddh 2013-01-23 12:13:49 PST
Created attachment 705487 [details] [diff] [review]
Part 3: Marionette test case for GET_INKEY with timeout. v2
Comment 29 psiddh 2013-01-23 12:18:47 PST
Yoshi, can you please mark your uploaded patch ' Part 3: Marionette test case for GET_INKEY with timeout. v2' as obsolete and take my uploaded patch with the same name.

Also please ignore comment#27. 

Pls let me know if this works!
Comment 30 Yoshi Huang[:allstars.chh] 2013-01-23 18:00:59 PST
Hi, Siddartha
I have made it obsolete,
can you run the try server first this time?
Comment 31 Yoshi Huang[:allstars.chh] 2013-01-23 18:53:16 PST
I already submitted a try 
https://tbpl.mozilla.org/?tree=Try&rev=a291d25ef234

Siddartha, currently under the B2G folder, the upstream for gecko is git.mozilla.org, which has only b2g18 branch update-to-date,
I think you could also fork the latest gecko from https://github.com/mozilla/releases-mozilla-central or https://github.com/mozilla/mozilla-central

And you could refer to the README.md in B2G folder, 
you can specify the location of gecko by using GECKO_PATH and GECKO_OBJDIR environment variables.
Comment 32 Yoshi Huang[:allstars.chh] 2013-01-24 03:12:40 PST
I've also tried to run the marionette case by myself, and it's pass now.
Comment 34 psiddh 2013-01-24 08:33:30 PST
Yoshi, so do I still have to try on the 'try server' ?
Comment 36 Yoshi Huang[:allstars.chh] 2013-01-24 17:41:20 PST
(In reply to Siddartha P from comment #34)
> Yoshi, so do I still have to try on the 'try server' ?

I've already done that for you :)
and the patches work fine now.

Thanks for your work

Note You need to log in before you can comment on or make changes to this bug.