1062819 - [System2] Implement MobileConnectionsSubSystem

Status: RESOLVED FIXED

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

Description

[(inactive after 6/18) Alive Kuo [:alive]]

Bug 940164 will introduce a baseModule/coreLoader into system app.

Our goal is dividing system app into several API oriented subsystems,
and we need a module to manage all mobileConnection related modules.

Comment 1

[(inactive after 6/18) Alive Kuo [:alive]]

Bug 1072757 is landing soon; now I am on this one.

Comment 2

[(inactive after 6/18) Alive Kuo [:alive]]

I think it's better if we keep breaking down my wip in bug 940164.

Here is my list:
* Implement HierarchyManager to deal with focus issue.
* Implement SimLockDialogManager to manage SimLockDialogs.
* AirplaneMode change.
* OperatorVariant change.
* Statusbar change.
* All the remaining + The mobileConnectionCore.

Comment 3

[(inactive after 6/18) Alive Kuo [:alive]]

Attachment: Implement/Extract MobileConnection Subsystem

Extract MobileConnections from System app
* Implement MobileConnectionCore
* Add mobileConnection slot into core.js
* Introduce System.query to state check purpose
* AirplaneMode change
** Extract AirplaneModeServiceHelper from AirplaneMode (The future plan is deprecating the AirplaneModeServiceHelper and make all API handler to turn on/off on their own.)
** radio is now registering to airplaneMode actively; that is, airplaneMode has little knowledge about that.
* OperatorVariant change;
** Renaming and add test for operator_variant_manager
** Fetch device os info in manager before instantiating any operatorVariantHandler instances.

What will not happen in this patch:
* Statusbar is still actively holding radio infomation; we should extract it from current Statusbar into StatusbarMobileConnectionIcon.
* Icc/Cellbroadcast/FtuPing is still holding mozMobileConnections; didn't have a clear plan to extract them now.
* Update manager is holding mozMobileConnections; need to make sure why it blocks updating.

Let's start the first round :)

Comment 4

[Etienne Segonzac (:etienne)]

Comment on attachment 8507779 [details]
Implement/Extract MobileConnection Subsystem

It's really cool to see the migration to BaseModule being done with small diffs, kudos!

I'm nit picking a lot on the new test files since we're setting up the foundations for future tests. I want us to make the "right thing" easy (with pertinent mocks and helpers). Won't give you too much trouble on existing tests files though :)

On a more general note: once the patch is almost done (I'm already mostly focusing on tests, let's wait for the 2 other reviews) we should start working with QA to ensure a safe landing. Those are critical modules, all dependent on the radio so not well covered by marionette tests, I think we'll need a formal manual QA round.

Comment 5

[Arthur Chen [:arthurcc][inactive after 6/30]]

Comment on attachment 8507779 [details]
Implement/Extract MobileConnection Subsystem

Please check my comments in github, thanks.

Comment 6

[(inactive after 6/18) Alive Kuo [:alive]]

Comment on attachment 8507779 [details]
Implement/Extract MobileConnection Subsystem

Sorry for taking a while.
I am not changing everything based on your comments or may misunderstand something,
lemme know what you think.

Comment 7

[Etienne Segonzac (:etienne)]

Comment on attachment 8507779 [details]
Implement/Extract MobileConnection Subsystem

Happy about the tests :) So comfortable setting r+, but there's a few simple comments on github that need to be addressed.

Worth repeating: I believe we should get a QA to signoff on this patch before landing it.

Comment 8

[(inactive after 6/18) Alive Kuo [:alive]]

(In reply to Etienne Segonzac (:etienne) from comment #7)
> Comment on attachment 8507779 [details]
> Implement/Extract MobileConnection Subsystem
> 
> Happy about the tests :) So comfortable setting r+, but there's a few simple
> comments on github that need to be addressed.
> 
> Worth repeating: I believe we should get a QA to signoff on this patch
> before landing it.

I guess I could use this flag.

Hi QA,

we need a specific test run for the patch:
* SIM PIN dialog behavior
* Airplane mode behavior
* Operator variant behavior
* Emergency callback
* Call forwarding

to make sure it does not regress these features.

Comment 9

[(inactive after 6/18) Alive Kuo [:alive]]

Comment on attachment 8507779 [details]
Implement/Extract MobileConnection Subsystem

Arthur, I need your review again.

Comment 10

[(inactive after 6/18) Alive Kuo [:alive]]

Comment on attachment 8507779 [details]
Implement/Extract MobileConnection Subsystem

Hi Eric, please see comment 8.
I am not sure how you will run the tests; if there is a "basic" set please go with the basic test cases. If there's no basic run and the complete run takes too much time please let me know. Thank you.

Comment 11

[Arthur Chen [:arthurcc][inactive after 6/30]]

Comment on attachment 8507779 [details]
Implement/Extract MobileConnection Subsystem

r=me for the parts of radio, airplane mode, call forwarding, telephony settings, and eu roaming manager.

Comment 12

[Eric Chang [:ericcc] [:echang]]

Tests for these area all spreading across several test suites, let me try to create something and run it.

* SIM PIN dialog behavior
* Airplane mode behavior
* Operator variant behavior
* Emergency callback
* Call forwarding

Out (In reply to Alive Kuo [:alive][NEEDINFO!] from comment #10)
> Comment on attachment 8507779 [details]
> Implement/Extract MobileConnection Subsystem
> 
> Hi Eric, please see comment 8.
> I am not sure how you will run the tests; if there is a "basic" set please
> go with the basic test cases. If there's no basic run and the complete run
> takes too much time please let me know. Thank you.

Comment 13

[Eric Chang [:ericcc] [:echang]]

Hi Alive, There is issue when trying to make call in airplane, no warning to tell user the device is in airplane mode, could you help to check that.

https://moztrap.mozilla.org/runtests/run/5729/env/347/
* SIM PIN dialog behavior - okay 
* Airplane mode behavior - no popup warning said the device is in airplane mode
* Operator variant behavior - not done yet
* Emergency callback - okay 
* Call forwarding - okay

Comment 14

[(inactive after 6/18) Alive Kuo [:alive]]

(In reply to Eric Chang [:ericcc] [:echang] from comment #13)
> Hi Alive, There is issue when trying to make call in airplane, no warning to
> tell user the device is in airplane mode, could you help to check that.
> 
> https://moztrap.mozilla.org/runtests/run/5729/env/347/

Thanks, I am checking!

Comment 15

[(inactive after 6/18) Alive Kuo [:alive]]

(In reply to Alive Kuo [:alive][NEEDINFO!] from comment #14)
> (In reply to Eric Chang [:ericcc] [:echang] from comment #13)
> > Hi Alive, There is issue when trying to make call in airplane, no warning to
> > tell user the device is in airplane mode, could you help to check that.
> > 
> > https://moztrap.mozilla.org/runtests/run/5729/env/347/
> 
> Thanks, I am checking!

:/ I think this is implemented in dialer side, do you have clue about what's wrong?

Comment 16

[(inactive after 6/18) Alive Kuo [:alive]]

(In reply to Eric Chang [:ericcc] [:echang] from comment #13)
> Hi Alive, There is issue when trying to make call in airplane, no warning to
> tell user the device is in airplane mode, could you help to check that.
> 
> https://moztrap.mozilla.org/runtests/run/5729/env/347/
> * SIM PIN dialog behavior - okay 
> * Airplane mode behavior - no popup warning said the device is in airplane
> mode
> * Operator variant behavior - not done yet
> * Emergency callback - okay 
> * Call forwarding - okay

Eric - The function is even broken in master hence it's not a regression from my patch.
Could you confirm?

Comment 17

[Etienne Segonzac (:etienne)]

(In reply to Alive Kuo [:alive][NEEDINFO!] from comment #15)
> (In reply to Alive Kuo [:alive][NEEDINFO!] from comment #14)
> > (In reply to Eric Chang [:ericcc] [:echang] from comment #13)
> > > Hi Alive, There is issue when trying to make call in airplane, no warning to
> > > tell user the device is in airplane mode, could you help to check that.
> > > 
> > > https://moztrap.mozilla.org/runtests/run/5729/env/347/
> > 
> > Thanks, I am checking!
> 
> :/ I think this is implemented in dialer side, do you have clue about what's
> wrong?

Definitely implemented in the Dialer [1] (+Gecko giving back error codes) and I can also reproduce it on master.

[1] https://github.com/mozilla-b2g/gaia/blob/master/apps/communications/dialer/js/telephony_helper.js

Comment 18

[Eric Chang [:ericcc] [:echang]]

Bug 1093434 for the missing airplane mode warning, it works when calling from Contacts.

Gaia-Rev        73eeaa23172c26e732972c318ea6aab20e0e1cae
Gecko-Rev       https://hg.mozilla.org/mozilla-central/rev/a458860efad7
Build-ID        20141103160202
Version         36.0a1
Device-Name     flame
FW-Release      4.4.2
FW-Incremental  eng.cltbld.20141103.192327
FW-Date         Mon Nov  3 19:23:36 EST 2014
Bootloader      L1TC00011880

Comment 19

[(inactive after 6/18) Alive Kuo [:alive]]

Travis is green again. If we couldn't land it this week, I'd like to move the System.query part out of the patch hence we could unblock serveral incoming work.

Comment 20

[José Antonio Olivera Ortega [:jaoo]]

I'll review this today. Sorry for the lag but I've been really busy with Loop these days.

Comment 21

[(inactive after 6/18) Alive Kuo [:alive]]

(In reply to José Antonio Olivera Ortega [:jaoo] from comment #20)
> I'll review this today. Sorry for the lag but I've been really busy with
> Loop these days.

That's a really good news!

Comment 22

[José Antonio Olivera Ortega [:jaoo]]

Comment on attachment 8507779 [details]
Implement/Extract MobileConnection Subsystem

Left a few comments for the operator variant part in the pull request. Please have a look and address them. Request review at me when you finish please. If you need some help, please ping me on IRC. Thanks and sorry for the lag reviewing this.

Comment 23

[(inactive after 6/18) Alive Kuo [:alive]]

Comment on attachment 8507779 [details]
Implement/Extract MobileConnection Subsystem

Thanks for your review! Let me know what you think.

Comment 24

[José Antonio Olivera Ortega [:jaoo]]

Comment on attachment 8507779 [details]
Implement/Extract MobileConnection Subsystem

LGTM. r=me for the operator variant part.

Sorry for the lag reviewing this Alive.

Comment 25

[(inactive after 6/18) Alive Kuo [:alive]]

Rebased to master and all green now, merging, thanks all.
https://treeherder.mozilla.org/ui/#/jobs?repo=gaia-try&revision=960f2874dc02

Comment 26

[(inactive after 6/18) Alive Kuo [:alive]]

https://github.com/mozilla-b2g/gaia/commit/83eaaae5fc609aa414d27a13a0237acb2d647a3a