[DSDS][STK] Display multiple cards in settings

VERIFIED FIXED in 1.4 S3 (14mar)

Status

VERIFIED FIXED
5 years ago
5 years ago

People

(Reporter: contacto, Assigned: frsela)

Tracking

unspecified
1.4 S3 (14mar)
x86_64
Linux
Dependency tree / graph
Bug Flags:
in-testsuite +
in-moztrap +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

(Reporter)

Description

5 years ago
Depending metabug 934404 (STK menu and events management) and 951586 (Suport multiple cards in system STK code).

Display info about multiple cards if there are diferents active cards.
(Reporter)

Updated

5 years ago
Depends on: 934404, 951586

Updated

5 years ago
No longer depends on: 934404

Updated

5 years ago
Status: UNCONFIRMED → NEW
Ever confirmed: true

Updated

5 years ago
Assignee: nobody → contacto
Status: NEW → ASSIGNED
Created attachment 8362514 [details] [review]
This patch implements STK DSDS in settings app

Pending to be tested in a real DSDS phone.
Assignee: contacto → frsela
Attachment #8362514 - Flags: feedback?(cyang)

Comment 2

5 years ago
Comment on attachment 8362514 [details] [review]
This patch implements STK DSDS in settings app

Nivi, please help verify this patch.
Attachment #8362514 - Flags: feedback?(cyang) → feedback?(nsarkar)

Comment 3

5 years ago
Sure. Will verify and send an update.

Comment 4

5 years ago
+1 on this change. I tested it with 2 STK sims and I can see two stk apps under settings. Also, I can run the test successfully.

Comment 5

5 years ago
Comment on attachment 8362514 [details] [review]
This patch implements STK DSDS in settings app

Verified. patch works.
Attachment #8362514 - Flags: feedback?(nsarkar) → feedback+
Attachment #8362514 - Flags: review?(kaze)
(In reply to Nivi from comment #5)
> Comment on attachment 8362514 [details] [review]
> This patch implements STK DSDS in settings app
> 
> Verified. patch works.

\o/ Thank you !

Updated

5 years ago
Target Milestone: --- → 1.4 S1 (14feb)
Hi Kaze, Could you please help us with the review here?. Many Thanks!
Flags: needinfo?(kaze)

Updated

5 years ago
Target Milestone: 1.4 S1 (14feb) → 1.4 S2 (28feb)
Duplicate of this bug: 947220
Duplicate of this bug: 960424
Comment on attachment 8362514 [details] [review]
This patch implements STK DSDS in settings app

I’m afraid I still don’t have any DSDS device to test. :-(

Arthur, would you have a look?
Attachment #8362514 - Flags: review?(kaze) → review?(arthur.chen)
Flags: needinfo?(kaze)
Comment on attachment 8362514 [details] [review]
This patch implements STK DSDS in settings app

LGTM. I left a few nitpicks on your PR.

Sorry it took me so long to review this patch, I was really hoping to get a DSDS device this week.
Attachment #8362514 - Flags: feedback+
(In reply to Fabien Cazenave [:kaze] from comment #11)
> Comment on attachment 8362514 [details] [review]
> This patch implements STK DSDS in settings app
> 
> LGTM. I left a few nitpicks on your PR.
> 
> Sorry it took me so long to review this patch, I was really hoping to get a
> DSDS device this week.

Thanks; rebased and nits fixed.

Waiting for Arthur's review to land this.
Target Milestone: 1.4 S2 (28feb) → ---
It works well on single sim devices but not fugu. It does not receive the stk message (0x25) so there is no STK items in settings app. I'll check this issue with the RIL team first.
Flags: in-moztrap+
QA Contact: echang
Comment on attachment 8362514 [details] [review]
This patch implements STK DSDS in settings app

The patch works well on my dual sim device. (Actually there are chances that we are not able to receive the stk command, but it might be a gecko problem, see also bug 975950) Please check my nits in github, thanks!
Attachment #8362514 - Flags: review?(arthur.chen)
(In reply to Arthur Chen [:arthurcc] from comment #15)
> Comment on attachment 8362514 [details] [review]
> This patch implements STK DSDS in settings app
> 
> The patch works well on my dual sim device. (Actually there are chances that
> we are not able to receive the stk command, but it might be a gecko problem,
> see also bug 975950) Please check my nits in github, thanks!

Nits fixed. Can you review it? Thanks

Updated

5 years ago
Flags: needinfo?(arthur.chen)
Edgar found the root cause of the lost of system messages. Fernando, as you suggested in https://bugzilla.mozilla.org/show_bug.cgi?id=935802#c21, if the system message is sent before we register the handler, the messages will lost. It is easy to reproduce with fugu (15%), I was thinking can we register the handler as soon as the init function is executed? If we receive the message before everything gets initialized, we save the message for later use, but we will never lost it. What do you think?
Flags: needinfo?(arthur.chen)
(In reply to Arthur Chen [:arthurcc] from comment #17)
> Edgar found the root cause of the lost of system messages. Fernando, as you
> suggested in https://bugzilla.mozilla.org/show_bug.cgi?id=935802#c21, if the
> system message is sent before we register the handler, the messages will
> lost. It is easy to reproduce with fugu (15%), I was thinking can we
> register the handler as soon as the init function is executed? If we receive
> the message before everything gets initialized, we save the message for
> later use, but we will never lost it. What do you think?

Registering first will reduce the number of lost messages but doesn't solve this race condition since icc.js code is loaded with to many other JSs in system so can be delayed :(

Is it possible to notify Gecko when system is loaded and Gecko init SIM card after that...

WDYT?
Flags: needinfo?(arthur.chen)
Actually in bootstrap.js system app does notify gecko with the event 'system-message-listener-ready', but there may still be potential race conditions as your comment.

Edgar had a discussion with guys working on system messages, and they agreed that the system messages should be queued in this case. As the fix on gecko may not be ready soon, do you think if we can use the workaround to reduce the chance of failure?
Flags: needinfo?(arthur.chen)
(In reply to Arthur Chen [:arthurcc] from comment #19)
> Actually in bootstrap.js system app does notify gecko with the event
> 'system-message-listener-ready', but there may still be potential race
> conditions as your comment.
> 
> Edgar had a discussion with guys working on system messages, and they agreed
> that the system messages should be queued in this case. As the fix on gecko
> may not be ready soon, do you think if we can use the workaround to reduce
> the chance of failure?

Done: https://github.com/frsela/gaia/commit/85ece8e6da2a888baca8d951f3a0e676f6e6bef8
Attachment #8362514 - Flags: review?(arthur.chen)
Comment on attachment 8362514 [details] [review]
This patch implements STK DSDS in settings app

r=me, thanks!
Attachment #8362514 - Flags: review?(arthur.chen) → review+
Landing in master since this is a DSDS key feature for v1.4.
https://github.com/mozilla-b2g/gaia/commit/f3502367ed427569f0e4c2dd09b8432bca39e7e3
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Can someone please back out this patch?

1) The commits should be squashed.
2) The commit message doesn't include the bug number.
3) It is lacking tests.

This is nowhere near the standard of quality we have for master.
Per comment 23, I've backed out the commits.

Fernando, is this a committed feature of v1.4? If no, we are not able to land this to master.
Status: RESOLVED → REOPENED
Flags: needinfo?(frsela)
Resolution: FIXED → ---
(In reply to Anthony Ricaud (:rik) from comment #23)
> Can someone please back out this patch?
> 
> 1) The commits should be squashed.
> 2) The commit message doesn't include the bug number.
> 3) It is lacking tests.
> 
> This is nowhere near the standard of quality we have for master.

Oups, I forgot change the commit name and squash ... oh my head. I'll fix just now.
Flags: needinfo?(frsela)
(In reply to Arthur Chen [:arthurcc] from comment #24)
> Per comment 23, I've backed out the commits.
> 
> Fernando, is this a committed feature of v1.4? If no, we are not able to
> land this to master.

Hi,

This bug blocks bug 934404 and bug 947220 that are listed as key features for v1.4 (https://wiki.mozilla.org/B2G/Roadmap), taking this into account, can we directly land it in master or should we ask for v1.4 approval?. Thanks!
Flags: needinfo?(arthur.chen)
(In reply to Fernando R. Sela (no CC, needinfo please) [:frsela] from comment #25)
> Oups, I forgot change the commit name and squash ... oh my head. I'll fix
> just now.
Don't forget new tests too.
(In reply to Anthony Ricaud (:rik) from comment #27)
> (In reply to Fernando R. Sela (no CC, needinfo please) [:frsela] from
> comment #25)
> > Oups, I forgot change the commit name and squash ... oh my head. I'll fix
> > just now.
> Don't forget new tests too.

Yeap; I'll work on a new PR (https://github.com/frsela/gaia/tree/STK_DSDS/Bug951599_v2)
In this case it's squashed :p

I hope finish it ASAP

Sorry for the mistake, I pressed the merge button too fast :'(
(In reply to Noemí Freire (:noemi) from comment #26)
> (In reply to Arthur Chen [:arthurcc] from comment #24)
> > Per comment 23, I've backed out the commits.
> > 
> > Fernando, is this a committed feature of v1.4? If no, we are not able to
> > land this to master.
> 
> Hi,
> 
> This bug blocks bug 934404 and bug 947220 that are listed as key features
> for v1.4 (https://wiki.mozilla.org/B2G/Roadmap), taking this into account,
> can we directly land it in master or should we ask for v1.4 approval?.
> Thanks!

We can land the patch in master without additional approvals if it is a v1.4 DSDS feature. Note that the patch should meet as criteria suggested in comment 23.
Flags: needinfo?(arthur.chen)

Updated

5 years ago
Target Milestone: --- → 1.4 S3 (14mar)
Created attachment 8385192 [details] [review]
STK DSDS Support in settings app V2

The previously r+ patch with unit tests.
Can you review the new tests added?
Attachment #8385192 - Flags: review?(arthur.chen)
Comment on attachment 8385192 [details] [review]
STK DSDS Support in settings app V2

Overall it looks good to me. Please address the comments in github and request a review when ready, thanks!
Attachment #8385192 - Flags: review?(arthur.chen)
Comment on attachment 8385192 [details] [review]
STK DSDS Support in settings app V2

Nits addressed
Attachment #8385192 - Flags: review?(arthur.chen)
Comment on attachment 8385192 [details] [review]
STK DSDS Support in settings app V2

r=me. Please squash the commits before merging, thanks!
Attachment #8385192 - Flags: review?(arthur.chen) → review+
(In reply to Arthur Chen [:arthurcc] from comment #33)
> Comment on attachment 8385192 [details] [review]
> STK DSDS Support in settings app V2
> 
> r=me. Please squash the commits before merging, thanks!

Thanks Arthur.
Squashed and merged: https://github.com/mozilla-b2g/gaia/commit/83197dc6f9ad88780f1fcbc3b01ee55756f566d6
Status: REOPENED → RESOLVED
Last Resolved: 5 years ago5 years ago
Resolution: --- → FIXED

Updated

5 years ago
Flags: in-testsuite+
Duplicate of this bug: 937082
verifyme
Flags: needinfo?(atsai)
Keywords: verifyme

Comment 37

5 years ago
Verified on Fugu. No gating issue.
Gaia      b5d0f9b0cd5dce38b79287a9363226b912929270
Gecko     7312341e00e2785419fc4746e291646299ba3018
BuildID   20140313120331
Version   30.0a1
Status: RESOLVED → VERIFIED

Updated

5 years ago
Flags: needinfo?(atsai)
Keywords: verifyme

Updated

5 years ago
Blocks: 999370
You need to log in before you can comment on or make changes to this bug.