1045820 - Port SimPicker implementation in MSAB and comms apps to GaiaSimPicker

Status: VERIFIED FIXED

(Firefox OS Graveyard :: Gaia, defect)

Description

[Doug Sherk (:drs) (inactive)]

In bug 1037868, we're moving SimPicker into its own web component. Once this is ready, we should port the dialer's implementation over as well.

Comment 1

[Doug Sherk (:drs) (inactive)]

Attachment: Port SimPicker implementation in MSAB and comms apps to GaiaSimPicker.

This is a big change, so please review only the changes to your app, and, if you'd like, the changes to the shared section.

PR: https://github.com/mozilla-b2g/gaia/pull/22386

Comment 2

[Doug Sherk (:drs) (inactive)]

Comment on attachment 8465966 [details] [diff] [review]
Port SimPicker implementation in MSAB and comms apps to GaiaSimPicker.

Actually, I don't think the API has been stabilized enough yet. Sorry about the spam.

Comment 3

[Doug Sherk (:drs) (inactive)]

Attachment: Port SimPicker implementation in MSAB and comms apps to GaiaSimPicker.

Updated PR.

Will ask for review once bug 1037868 is landed. This should be very stable now, though.

Comment 4

[Doug Sherk (:drs) (inactive)]

Attachment: Port SimPicker implementation in MSAB and comms apps to GaiaSimPicker.

PR: https://github.com/mozilla-b2g/gaia/pull/22386

Each of you, please review only your components. Someone (or maybe multiple of you) should review the shared code changes as well, such as multi_sim_action_button.js and its tests.

Comment 5

[Doug Sherk (:drs) (inactive)]

Attachment: Port SimPicker implementation in MSAB and comms apps to GaiaSimPicker.

With 8 lines of context.

Comment 6

[Julien Wajsberg [:julienw]]

Comment on attachment 8494957 [details] [diff] [review]
Port SimPicker implementation in MSAB and comms apps to GaiaSimPicker.

left some comments on github for the common code, but the SMS code is ok (it was merely removing code, I'm always ok with removing code :) ).
r=me for the SMS code.

Please answer questions and fix the missing "use strict" in setup.js.

Thanks !

Comment 7

[Doug Sherk (:drs) (inactive)]

Comment on attachment 8494957 [details] [diff] [review]
Port SimPicker implementation in MSAB and comms apps to GaiaSimPicker.

Redirecting to Ricky as Yuren is leaving.

Comment 8

[Ricky Chien [:rickychien] (inactive)]

Comment on attachment 8494957 [details] [diff] [review]
Port SimPicker implementation in MSAB and comms apps to GaiaSimPicker.

r=me for build.test.js

Don't forget to modify your PR title to r=rickychien.

Thanks!

Comment 9

[Francisco Jordano [:arcturus] [:francisco]]

Comment on attachment 8494957 [details] [diff] [review]
Port SimPicker implementation in MSAB and comms apps to GaiaSimPicker.

Hei Jose,

It's been a while that I cannot check this, do you have some cycles to add this to your review queue?

Thanks!

Comment 10

[Jose Manuel Cantera]

Comment on attachment 8494957 [details] [diff] [review]
Port SimPicker implementation in MSAB and comms apps to GaiaSimPicker.

I cannot apply the patch over master. Could it be possible to operate over a PR? It can be easier to follow the changes on the code ... 

thanks

Comment 11

[Doug Sherk (:drs) (inactive)]

Attachment: Port SimPicker implementation in MSAB and comms apps to GaiaSimPicker.

Rebased PR: https://github.com/mozilla-b2g/gaia/pull/25671/files

Carrying r+ from Ricky and Julien.

Comment 12

[Germán Toro del Valle (:gtorodelvalle)]

Comment on attachment 8514596 [details] [diff] [review]
Port SimPicker implementation in MSAB and comms apps to GaiaSimPicker.

Hi Doug, I left some minor comments in the pull request ;)

Apart from this, I saw that when using the Emergency Call app and having the "always ask" settings as the selected one to force the selection menu to be shown, if an ICE contact is selected the SIM picker menu is not shown. The reason is the dependency between gaia-sim-picker and gaia-menu. Including |<link rel="stylesheet" type="text/css" href="/shared/elements/gaia_menu/style.css">| and |<script defer src="/shared/elements/gaia_menu/script.js"></script>| in the lazy loading area (https://github.com/DouglasSherk/gaia/blob/1045820-sim-picker-dialer/apps/emergency-call/index.html#L33) would do the trick and solve the issue :)

I also noticed that communications-contacts/test/unit/views/form_test.js but I didn't check if it related to your patch, sorry.

Comment 13

[Jose Manuel Cantera]

Comment on attachment 8514596 [details] [diff] [review]
Port SimPicker implementation in MSAB and comms apps to GaiaSimPicker.

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

As per the comments on the Github and subsequent discussion on the IRC, we need to lazy load the SIM Picker in order not to affect the startup time of contacts.

Comment 14

[Doug Sherk (:drs) (inactive)]

Attachment: Port SimPicker implementation in MSAB and comms apps to GaiaSimPicker.

Updated PR: https://github.com/mozilla-b2g/gaia/pull/25671/files

Addressed code review comments. The PR has the changes as a new commit. Carrying r+ from Julien and Ricky.

Comment 15

[Germán Toro del Valle (:gtorodelvalle)]

Comment on attachment 8516444 [details] [diff] [review]
Port SimPicker implementation in MSAB and comms apps to GaiaSimPicker.

Hi Doug, sorry for the delay :( Working nicely now :) Thanks!

r+ for the Dialer / Emergency Call part ;)

Comment 16

[Jose Manuel Cantera]

Comment on attachment 8516444 [details] [diff] [review]
Port SimPicker implementation in MSAB and comms apps to GaiaSimPicker.

thanks. tested and working

Comment 17

[Doug Sherk (:drs) (inactive)]

https://github.com/mozilla-b2g/gaia/commit/7e5242f20282fd32cf989b3676162d2901ccbb40

Comment 18

[Johan Lorenzo [:jlorenzo]]

Verified on:
Gaia-Rev        7004ccfd16e2ad20739bd04b72fa1672ee58686f
Gecko-Rev       afea13fdb3de3abd9ece29d3da5b700abe450988
Build-ID        20141107145850
Version         36.0a1
Device-Name     flame
FW-Release      4.4.2
FW-Incremental  eng.jlorenzo.20141002.104456
FW-Date         Thu Oct  2 10:45:09 CEST 2014
Bootloader      L1TC00011880

Testing done:
With SIM1 as default, then with "Always ask":
* Call a regular number on the dialer tab with both SIM
* Call voicemail using hotkey
* Set up a conf call with contact tab with both SIM 
* Call a contact with contact tab on the dialer.
* Call a contact on the call info page.
* Send a text message.

Remaining issues:
* After a conference call made with the non-default SIM, the SIM picker displays this SIM instead of the default one (see bug 1095537)
* The tabs are still visible when you long press an MSAB in the contact tab (same issue as bug 1090066).