1015833 - [PTCRB][STK]27.22.4.1.1/2 DISPLAY TEXT, normal priority, Unpacked 8 bit data for Text String, screen busy case failed

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::Settings, defect)

Description

[xiaokang.chen]

Attachment: log_SH.txt

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:13.0) Gecko/20100101 Firefox/13.0.1 (Beta/Release)
Build ID: 20120615040358

Steps to reproduce:

27.22.4.1.1/2 DISPLAY TEXT, normal priority, Unpacked 8 bit data for Text String, screen busy case failed


Actual results:

TR incorrect. Display wrong.

Comment 1

[xiaokang.chen]

Dear Fernando,
There is a related bug 873908. We need fix this for PTCRB. Thanks a lot!

Comment 2

[Fernando R. Sela (no CC, needinfo please) [:frsela]]

Checking logs. I'll inform ASAP

Comment 3

[Fernando R. Sela (no CC, needinfo please) [:frsela]]

Attachment: Show ICC views over attention screens

Now, on a call the ICC Alert is showed over the attention screen

Comment 4

[xiaokang.chen]

Dear Fernando,

This is the Step:
1
USER ME
Set the ME screen to a display mode other than the normal stand‑by display 
The ME will be set to a mode so that normal priority text commands shall be rejected.
2
SIM ME
PROACTIVE COMMAND PENDING: DISPLAY TEXT 1.2.1

3
ME SIM
FETCH

4
SIM ME
PROACTIVE COMMAND: DISPLAY TEXT 1.2.1
[Normal priority]
5
ME USER
No change of the currently being used display.

6
ME SIM
TERMINAL RESPONSE: DISPLAY TEXT 1.2.1
[ME currently unable to process command - screen busy]
7
SIM ME
PROACTIVE SIM SESSION ENDED

As I know, the patch you provided can't solve this problem. What do you think?

Comment 5

[xiaokang.chen]

Comment on attachment 8428598 [details] [diff] [review]
Show ICC views over attention screens

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

Sorry, I don't think this patch can solve the problem.

Comment 6

[Fernando R. Sela (no CC, needinfo please) [:frsela]]

Hi,

If I understood correctly, the issue is that with priority screens (like in an established call, for example) the STK dialogs are not showed.

Considering previous paragraph correct, the problem is that attention screen (used by these priority screens) are showed over any other one (using the z-index attribute)

So this patch sets the ICC screens with a higher z-index value so them are showed over these priority screens.

Later I'll record a video to show you it working.

If my assumptions are not correct, please tell me ;)

Comment 7

[xiaokang.chen]

Hi Fernando,
IMHO, the scenario of this case is:

The "display text" command should be rejected with an TR "ME currently unable to process command - screen busy(additional info)" while a priority screen is showing. Maybe we should define two display mode
to deal with this case.

And the normal priority and high priority "display text" command should be treated differently.

Comment 8

[Fernando R. Sela (no CC, needinfo please) [:frsela]]

Oh, OK!

So, if "isHighPriority" is true (http://mxr.mozilla.org/mozilla-central/source/dom/webidl/MozStkCommandEvent.webidl#33) the STK dialog shall be showed over other screens (current patch)

But in any other case, if screen is busy the app shall responde the STK command with STK_RESULT_TERMINAL_CRNTLY_UNABLE_TO_PROCESS (http://mxr.mozilla.org/mozilla-central/source/dom/webidl/MozIccManager.webidl#89)

Is this correct?

Thank you for your clarification :)

Comment 9

[xiaokang.chen]

(In reply to Fernando R. Sela (no CC, needinfo please) [:frsela] from comment #8)
> Oh, OK!
> 
> So, if "isHighPriority" is true
> (http://mxr.mozilla.org/mozilla-central/source/dom/webidl/MozStkCommandEvent.
> webidl#33) the STK dialog shall be showed over other screens (current patch)
> 
> But in any other case, if screen is busy the app shall responde the STK
> command with STK_RESULT_TERMINAL_CRNTLY_UNABLE_TO_PROCESS
> (http://mxr.mozilla.org/mozilla-central/source/dom/webidl/MozIccManager.
> webidl#89)
> 
> Is this correct?
> 
> Thank you for your clarification :)

Yes, that's right. 
And the expected TR is "81 03 01 21 80 82 02 82 81 83 02 20 01" (see TS 51.010-4 27.22.4.1.1 SEQ 1.2) so we need add a "additional info" for this case.

Comment 10

[xiaokang.chen]

Dear Carol,
Shall we add some code in vendor to support "additional infomation"? Thanks a lot! 
Shall I file a case in qualcomm platform?

Comment 11

[Jason Smith [:jsmith]]

Vance - Is this a cert blocker for TCL?

Comment 12

[Preeti Raghunath(:Preeti)]

Vance

Comment on cert blocker or not?

Comment 13

[Carol Yang [:cyang]]

(In reply to xiaokang.chen from comment #10)
> Dear Carol,
> Shall we add some code in vendor to support "additional infomation"? Thanks
> a lot! 
> Shall I file a case in qualcomm platform?

Hi xiaokang, since this is a new feature, it would be risky for 1.3. Looking to see the response to Comment 11.

Comment 14

[xiaokang.chen]

(In reply to Carol Yang [:cyang] from comment #13)
> (In reply to xiaokang.chen from comment #10)
> > Dear Carol,
> > Shall we add some code in vendor to support "additional infomation"? Thanks
> > a lot! 
> > Shall I file a case in qualcomm platform?
> 
> Hi xiaokang, since this is a new feature, it would be risky for 1.3. Looking
> to see the response to Comment 11.

Dear Carol,
We can't always waive for this, this pr blocked PTCRB.

Comment 15

[xiaokang.chen]

(In reply to Fernando R. Sela (no CC, needinfo please) [:frsela] from comment #8)
> Oh, OK!
> 
> So, if "isHighPriority" is true
> (http://mxr.mozilla.org/mozilla-central/source/dom/webidl/MozStkCommandEvent.
> webidl#33) the STK dialog shall be showed over other screens (current patch)
> 
> But in any other case, if screen is busy the app shall responde the STK
> command with STK_RESULT_TERMINAL_CRNTLY_UNABLE_TO_PROCESS
> (http://mxr.mozilla.org/mozilla-central/source/dom/webidl/MozIccManager.
> webidl#89)
> 
> Is this correct?
> 
> Thank you for your clarification :)

Dear Fernando,
Can you provide a patch in gaia for this:

If the isHighPriority is false, send TR like this
------------------code start------------------
            iccManager.responseSTKCommand({
              resultCode: iccManager._iccManager.STK_RESULT_TERMINAL_CRNTLY_UNABLE_TO_PROCESS,
              additionalInformation: 0x01
            }); 
------------------code end--------------------
If the screen is not in idle mode(the mode after we press home button).

I will continue working on vendor problems.

Thanks a lot!

Comment 16

[Vance Chen [:vchen][skype:vance.lucida][[e-mail]]]

Per Comment#14, this is a cert blocker

Comment 17

[Preeti Raghunath(:Preeti)]

Making it a 1.3+ blocker given it is a cert blocker

Comment 18

[Fernando R. Sela (no CC, needinfo please) [:frsela]]

Attachment: Proposed patch

Comment 19

[Carol Yang [:cyang]]

(In reply to xiaokang.chen from comment #14)
> (In reply to Carol Yang [:cyang] from comment #13)
> > (In reply to xiaokang.chen from comment #10)
> > > Dear Carol,
> > > Shall we add some code in vendor to support "additional infomation"? Thanks
> > > a lot! 
> > > Shall I file a case in qualcomm platform?
> > 
> > Hi xiaokang, since this is a new feature, it would be risky for 1.3. Looking
> > to see the response to Comment 11.
> 
> Dear Carol,
> We can't always waive for this, this pr blocked PTCRB.

Hi xiaokang, please open an SR for this.

Comment 20

[xiaokang.chen]

(In reply to Carol Yang [:cyang] from comment #19)
> (In reply to xiaokang.chen from comment #14)
> > (In reply to Carol Yang [:cyang] from comment #13)
> > > (In reply to xiaokang.chen from comment #10)
> > > > Dear Carol,
> > > > Shall we add some code in vendor to support "additional infomation"? Thanks
> > > > a lot! 
> > > > Shall I file a case in qualcomm platform?
> > > 
> > > Hi xiaokang, since this is a new feature, it would be risky for 1.3. Looking
> > > to see the response to Comment 11.
> > 
> > Dear Carol,
> > We can't always waive for this, this pr blocked PTCRB.
> 
> Hi xiaokang, please open an SR for this.

SR 01572527. Thanks a lot!

Comment 21

[xiaokang.chen]

Comment on attachment 8430678 [details] [review]
Proposed patch

Dear Fernando,
Thanks a lot for your strong support.

I have communicate with the worker of TMC LAB(who did the gcf/ptcrb test), they told me we just need "only display the normal priority display text when the screen is idle. If not in idle mode, don't display the text and reject with ME currently unable to process command - screen busy"

So I suggest this:
----------------code start-----------------------
    var currentApp = WindowManager.getDisplayedApp();
    var idle = (currentApp === HomescreenLauncher.origin) ? true: false;
    DUMP('currentApp = '+currentApp+' and idle = '+idle);
    if (!options.isHighPriority && !idle) {
      DUMP('Do not display the text because normal priority.');
      iccManager.responseSTKCommand({
        resultCode: iccManager._iccManager.STK_RESULT_TERMINAL_CRNTLY_UNABLE_TO_PROCESS,
        additionalInformation: 0x01
      });
      return;
    }
-----------------code end---------------

And we don't have to change the z-index of icc-view.

What do you think?

Comment 22

[Fernando R. Sela (no CC, needinfo please) [:frsela]]

(In reply to xiaokang.chen from comment #21)
> Comment on attachment 8430678 [details] [review]
> Proposed patch
> 
> Dear Fernando,
> Thanks a lot for your strong support.
> 
> I have communicate with the worker of TMC LAB(who did the gcf/ptcrb test),
> they told me we just need "only display the normal priority display text
> when the screen is idle. If not in idle mode, don't display the text and
> reject with ME currently unable to process command - screen busy"
> 

Cool, I'll create a new proposal asap.

Thank you !

Comment 23

[Nivi]

Comment on attachment 8430678 [details] [review]
Proposed patch

Hi Fernando, This patch doesn't work for me. But the proposed patch on comment #21 works.

Comment 24

[Fernando R. Sela (no CC, needinfo please) [:frsela]]

Comment on attachment 8430678 [details] [review]
Proposed patch

Updated, can you verify it's working now?
Thanks in advance

Comment 25

[xiaokang.chen]

Comment on attachment 8430678 [details] [review]
Proposed patch

Dear Fernando,
Sorry to disturb agian, the QA of tcl suggest we should display normal text in settings too.

And as I know, if don't change the z-index, it also works well. why not just change the code of icc_worker.js ?

Feel free to correct me. Thanks a lot!

Comment 26

[Fernando R. Sela (no CC, needinfo please) [:frsela]]

(In reply to xiaokang.chen from comment #25)
> Comment on attachment 8430678 [details] [review]
> Proposed patch
> 
> Dear Fernando,
> Sorry to disturb agian, the QA of tcl suggest we should display normal text
> in settings too.
> 
> And as I know, if don't change the z-index, it also works well. why not just
> change the code of icc_worker.js ?
> 
> Feel free to correct me. Thanks a lot!

Agree, I forgot remove the z-index ... just updated ;)

Comment 27

[Fernando R. Sela (no CC, needinfo please) [:frsela]]

Sorry, my mistake

Comment 28

[Nivi]

Hi Fernando,

Is there a final patch to test?

Please let me know and I would be happy to test it for you.

Thanks,
Nivi.

Comment 29

[Nivi]

Comment on attachment 8430678 [details] [review]
Proposed patch

This breaks normal DISPLAY_TEXT tests on v1.4 and master. I am seeing test timeout while running my old DISPLAY_TEXT tests. Please make sure this doesn't cause any regressions.

Thanks,
Nivi.

Comment 30

[Fernando R. Sela (no CC, needinfo please) [:frsela]]

(In reply to Nivi from comment #29)
> Comment on attachment 8430678 [details] [review]
> Proposed patch
> 
> This breaks normal DISPLAY_TEXT tests on v1.4 and master. I am seeing test
> timeout while running my old DISPLAY_TEXT tests. Please make sure this
> doesn't cause any regressions.
> 
> Thanks,
> Nivi.

I copied the patch at comment 21 since you told in comment 23 that it works ... :p

I'll check it again. Thanks

Comment 31

[Fernando R. Sela (no CC, needinfo please) [:frsela]]

Updated, WindowManager object disappeared. Changed to the new AppWindowManager.

Thank you

Comment 32

[Nivi]

Fernando, yes it did work on v1.3 but broke v1.4 and master. I originally tested on v1.3 so I told you it works.
Sorry for that :).

Will try your new patch and let you know.

Thanks,
Nivi.

Comment 33

[Nivi]

Comment on attachment 8430678 [details] [review]
Proposed patch

Bug in code. Please see comments in github.

How did Travis pass? Did you run it on master or another branch?

Thanks,
Nivi.

Comment 34

[Fernando R. Sela (no CC, needinfo please) [:frsela]]

Comment on attachment 8430678 [details] [review]
Proposed patch

Fixed again ;)
Thank you

Comment 35

[Nivi]

Comment on attachment 8430678 [details] [review]
Proposed patch

This patch works on master for me.

But why is Travis failing?

Thanks,
Nivi.

Comment 36

[Fernando R. Sela (no CC, needinfo please) [:frsela]]

(In reply to Nivi from comment #35)
> Comment on attachment 8430678 [details] [review]
> Proposed patch
> 
> This patch works on master for me.
> 
> But why is Travis failing?
> 
> Thanks,
> Nivi.

Thank you Nivi,

Travis fail is not related to this patch, is a timeout in a contacts tests. I'll re-run it.

  1) Contacts shortcuts > touch touch on shortcuts press/release on scrollbar should show/hide shortcut:

  Error: timeout exceeded!

Comment 37

[Fernando R. Sela (no CC, needinfo please) [:frsela]]

(In reply to Fernando R. Sela (no CC, needinfo please) [:frsela] from comment #36)
> 
> Travis fail is not related to this patch, is a timeout in a contacts tests.
> I'll re-run it.
> 
>   1) Contacts shortcuts > touch touch on shortcuts press/release on
> scrollbar should show/hide shortcut:
> 
>   Error: timeout exceeded!

Travis passed now \o/

Comment 38

[Nivi]

Great!! :)

Comment 39

[Nivi]

Are we close on landing this?

Thanks,
Nivi.

Comment 40

[Fernando R. Sela (no CC, needinfo please) [:frsela]]

(In reply to Nivi from comment #39)
> Are we close on landing this?
> 
> Thanks,
> Nivi.

Yes. We need to be reviewed and after the r+ it will be landed.

Comment 41

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

Comment on attachment 8430678 [details] [review]
Proposed patch

I made a comment on github as there is a better way to check if the current activeApp is the homescreen or not.

Please fix it before landing.

Comment 42

[Fernando R. Sela (no CC, needinfo please) [:frsela]]

(In reply to Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please) from comment #41)
> Comment on attachment 8430678 [details] [review]
> Proposed patch
> 
> I made a comment on github as there is a better way to check if the current
> activeApp is the homescreen or not.
> 
> Please fix it before landing.

Thank you Vivien.

Patch updated. Waiting for Travis to land.

Comment 43

[Fernando R. Sela (no CC, needinfo please) [:frsela]]

https://tbpl.mozilla.org/?tree=Gaia-Try&rev=b4c21d578c2ef8c43016bd8f52435e009dcd3740

Comment 44

[Fernando R. Sela (no CC, needinfo please) [:frsela]]

(In reply to Fernando R. Sela (no CC, needinfo please) [:frsela] from comment #43)
> https://tbpl.mozilla.org/?tree=Gaia-
> Try&rev=b4c21d578c2ef8c43016bd8f52435e009dcd3740

TBPL fail not related to this patch.

Comment 45

[Fernando R. Sela (no CC, needinfo please) [:frsela]]

Landed: https://github.com/mozilla-b2g/gaia/commit/5fecb773534e958b7ec77015fcf7722e972bc726

Comment 46

[Ryan VanderMeulen [:RyanVM]]

v2.0: https://github.com/mozilla-b2g/gaia/commit/9397bf9756264f9780a9afb77364f2676b1b0ff9
v1.4: https://github.com/mozilla-b2g/gaia/commit/27beb91e75aedf14b1cdd8cae86da0ebcca62212

Please request approval-gaia-v1.3 on this patch when you feel it is sufficiently baked for uplift.

Comment 47

[Fernando R. Sela (no CC, needinfo please) [:frsela]]

Hi Ryan,

I don't understood correctly.

What do you mean? Only ask for the approval or create a specific patch for 1.3 & 1.3T branches?

Comment 48

[Ryan VanderMeulen [:RyanVM]]

v1.3 branch rules require that all patches require approval to be uplifted whether they have blocking status or not. You need to request approval-gaia-v1.3 on the patch and fill out the questions for it to be considered by Release Management.

Comment 49

[Fernando R. Sela (no CC, needinfo please) [:frsela]]

Comment on attachment 8430678 [details] [review]
Proposed patch

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

This patch avoids to show STK messages over any app if they are not High Priority
[Bug caused by] (feature/regressing bug #): - 
[User impact] if declined: The user will be bothered by non important messages
[Testing completed]:
[Risk to taking this patch] (and alternatives if risky): Low
[String changes made]: None

Comment 50

[[:fabrice] Fabrice Desré]

Comment on attachment 8430678 [details] [review]
Proposed patch

1.3 approval, really? I'll defer to release management.

Comment 51

[bhavana bajaj [:bajaj]]

(In reply to Fabrice Desré [:fabrice] from comment #50)
> Comment on attachment 8430678 [details] [review]
> Proposed patch
> 
> 1.3 approval, really? I'll defer to release management.

:-/ quite unfortunate, cert blocker :'(

Comment 52

[Ryan VanderMeulen [:RyanVM]]

Sorry, but this needs rebasing for v1.3 uplift.

Comment 53

[Fernando R. Sela (no CC, needinfo please) [:frsela]]

Attachment: Same patch for v1.3

Comment 54

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8447171 [details] [review]
Same patch for v1.3

Straight-up rebases typically don't require re-review unless there's major changes needed.

v1.3: https://github.com/mozilla-b2g/gaia/commit/b2af19b6982c2b469ad37c93e626c18ce276e8cb

Comment 55

[Lina.Guo]

Hi  Ryan VanderMeulen:

    var activeApp = AppWindowManager.getActiveApp();
    if (!options.isHighPriority && !activeApp.isHomescreen) {
    DUMP('Do not display the text because normal priority.');
    iccManager.responseSTKCommand({
    resultCode:
    iccManager._iccManager.STK_RESULT_TERMINAL_CRNTLY_UNABLE_TO_PROCESS,
    additionalInformation: 0x01
    });
    return;
 }

  As you merged those code to V1.3t, the STK menu has affected.We click each of the STK main menu can not enter the next page now.

  Please help check it ,thanks a lot .

Comment 56

[Ryan VanderMeulen [:RyanVM]]

Whatever you're asking is almost certainly better directly at the patch author, not the person who landed it for them.

Comment 57

[Lina.Guo]

 OK,thanks for Ryan VanderMeulen's advice.

 Hi  Fernando R. Sela:

   Issue: As you merged those code to V1.3t, the STK menu has affected.We click each of the STK main menu can not enter the next page now.
    
   In apps/system/js/icc_worker.js:
   
      var activeApp = AppWindowManager.getActiveApp();//code from the patch

   While in v1.3t there is not AppWindowManager,it should be WindowManager.

   Please be kindly help to confirm it ,thanks a lot.

Comment 58

[helloarvin]

Hi lina,

Could you please try to modify the patch and ask Fernando help to review?

Thanks.

Comment 59

[Lina.Guo]

   I have modified the AppWindowManager,but it does not work.

   So please Fernando R. Sela help check it ,thanks.

Comment 60

[Lina.Guo]

Attachment: v1.3t-stk-text.patch

Hi Fernando:

   I have modified the patch about the obtain of the judgement conditions. 
   Could you please help review the patch and feel free to contact me if there's something wrong.
   Thanks a lot.

Comment 61

[Fernando R. Sela (no CC, needinfo please) [:frsela]]

(In reply to Lina.Guo from comment #57)
>  OK,thanks for Ryan VanderMeulen's advice.
> 
>  Hi  Fernando R. Sela:
> 
>    Issue: As you merged those code to V1.3t, the STK menu has affected.We
> click each of the STK main menu can not enter the next page now.
>     
>    In apps/system/js/icc_worker.js:
>    
>       var activeApp = AppWindowManager.getActiveApp();//code from the patch
> 
>    While in v1.3t there is not AppWindowManager,it should be WindowManager.
> 
>    Please be kindly help to confirm it ,thanks a lot.

Hi Lina,

Sorry for the delay, I was in PTO.

Yes, I agree, the obect to use is WindowManager, but calling getDisplayedApp instead getActiveApp which doesn't exists in 1.3

Comment 62

[Fernando R. Sela (no CC, needinfo please) [:frsela]]

Comment on attachment 8465999 [details] [diff] [review]
v1.3t-stk-text.patch

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

Thank you Lina.

It looks good. Did you test it?
I'll test it today.

Comment 63

[Lina.Guo]

(In reply to Fernando R. Sela (no CC, needinfo please) [:frsela] from comment #62)
> Comment on attachment 8465999 [details] [diff] [review]
> v1.3t-stk-text.patch
> 
> Review of attachment 8465999 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thank you Lina.
> 
> It looks good. Did you test it?
> I'll test it today.



  Yes, I hava tested it .
  
  Please help test it again,thanks.

Comment 64

[Fernando R. Sela (no CC, needinfo please) [:frsela]]

(In reply to Lina.Guo from comment #63)
>   Please help test it again,thanks.

Tested with positive result :) - it works as expected

Comment 65

[helloarvin]

Hi Alive & Fernando,

Due to STK feature in v1.3t is broken currently, could you please help merge the patch attachment 8465999 [details] [diff] [review] into the branch ASAP to fix it?

Thanks a lot.

Comment 66

[Fernando R. Sela (no CC, needinfo please) [:frsela]]

1.3t landed - https://github.com/mozilla-b2g/gaia/commit/5174bdb74975caefb05719ababfe8d973db69674

Comment 67

[helloarvin]

Thank you very much.

As for the blocker released, I've already provided the PR for both master and v1.3t in bug1046564.

STK Done:)

Comment 68

[jinghua.xing]

In v2.1 branch, this case has been reported failed again. The response for this TR is "810301218002028281830120"

We have added an 'additionalInformation' property in the response for this case in gaia. But in the function sendStkTerminalResponse() in ril_worker.js, I can't find where to write the value of this property into the parcel which will be sended to the modem.

Can you help us check this please? Thank you.

Comment 69

[jinghua.xing]

Attachment: display.patch

And I have made a patch for this issue, please help to review it. 

Thank you.

Comment 70

[jinghua.xing]

I made a new bug for this issue: bug1117609