827831 - B2G STK: Support variable timeout for GET INKEY

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

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

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 Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

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]

Attachment: Interface updated to decode 'duration' TLV tag for GetInkey

Comment 3

[psiddh]

Attachment: Part 2: Decode comprehension TLV tag - Duration for GET INKEY variable time out GCF T.C

Comment 4

[psiddh]

Attachment: Part 1 : Interface updated to decode 'duration' TLV tag for GetInkey

Comment 5

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

Attachment: Part 3: Marionette test case for GET_INKEY with timeout

Help Siddartha to upload the patch.

Comment 6

[psiddh]

Thanks Yoshi for uploading

Comment 7

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

I suspect this one should block since this is the platform side of a Gaia blocker.

Comment 8

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

(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]]

Is this a certification requirement, Yoshi?

Comment 10

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

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]]

(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]]

(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]]

(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 Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

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]

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 Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

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]

Attachment: Part 1 : Interface updated to decode 'duration' TLV tag for GetInkey

Comment 18

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

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]

Attachment: Part 1 : Interface updated to decode 'duration' TLV tag for GetInkey

Comment 20

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

Great, thanks !
I'll push your patches.

Comment 21

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

Attachment: Part 3: Marionette test case for GET_INKEY with timeout. v2

Rebase to avoid conflict.

Comment 22

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

Oh, seems mozilla-inbound is closed now, I'll push later.

Comment 23

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/8fc6d7d32482
https://hg.mozilla.org/integration/mozilla-inbound/rev/3d2b3f7eac93
https://hg.mozilla.org/integration/mozilla-inbound/rev/ed1ef43ce9af

Comment 24

[Ryan VanderMeulen [:RyanVM]]

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 Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

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]

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]

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 <[e-mail]>
># 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]

Attachment: Part 3: Marionette test case for GET_INKEY with timeout. v2

Comment 29

[psiddh]

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 Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Hi, Siddartha
I have made it obsolete,
can you run the try server first this time?

Comment 31

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

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 Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

I've also tried to run the marionette case by myself, and it's pass now.

Comment 33

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/781a5c27a1e5
https://hg.mozilla.org/integration/mozilla-inbound/rev/2d9481837500
https://hg.mozilla.org/integration/mozilla-inbound/rev/8370a0b60561

Comment 34

[psiddh]

Yoshi, so do I still have to try on the 'try server' ?

Comment 35

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/781a5c27a1e5
https://hg.mozilla.org/mozilla-central/rev/2d9481837500
https://hg.mozilla.org/mozilla-central/rev/8370a0b60561

Comment 36

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

(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