Closed Bug 1062819 Opened 6 years ago Closed 5 years ago

[System2] Implement MobileConnectionsSubSystem

Categories

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

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(tracking-b2g:+)

RESOLVED FIXED
tracking-b2g +

People

(Reporter: alive, Assigned: alive)

References

Details

Attachments

(1 file)

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.
Summary: [System2] Implement MobileConnectionsSystem → [System2] Implement MobileConnectionsSubSystem
Depends on: 1072757
No longer depends on: gaia-bootstrap
Bug 1072757 is landing soon; now I am on this one.
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.
Depends on: 1079748
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 :)
Attachment #8507779 - Flags: review?(josea.olivera)
Attachment #8507779 - Flags: review?(etienne)
Attachment #8507779 - Flags: review?(arthur.chen)
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.
Attachment #8507779 - Flags: review?(etienne) → feedback+
Comment on attachment 8507779 [details]
Implement/Extract MobileConnection Subsystem

Please check my comments in github, thanks.
Attachment #8507779 - Flags: review?(arthur.chen) → feedback+
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.
Attachment #8507779 - Flags: review?(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.
Attachment #8507779 - Flags: review?(etienne) → review+
(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.
Flags: qe-verify?
Comment on attachment 8507779 [details]
Implement/Extract MobileConnection Subsystem

Arthur, I need your review again.
Attachment #8507779 - Flags: review?(arthur.chen)
Depends on: 1089010
Flags: qe-verify?
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.
Attachment #8507779 - Flags: qa-approval?(echang)
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.
Attachment #8507779 - Flags: review?(arthur.chen) → review+
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.
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
Flags: needinfo?(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!
Flags: needinfo?(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?
Flags: needinfo?(etienne)
(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?
Flags: needinfo?(echang)
(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
Flags: needinfo?(etienne)
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
Flags: needinfo?(echang)
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.
I'll review this today. Sorry for the lag but I've been really busy with Loop these days.
(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!
Attachment #8507779 - Flags: qa-approval?(echang) → qa-approval+
Assignee: nobody → alive
blocking-b2g: --- → backlog
Blocks: 1089511
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.
Attachment #8507779 - Flags: review?(josea.olivera)
Comment on attachment 8507779 [details]
Implement/Extract MobileConnection Subsystem

Thanks for your review! Let me know what you think.
Attachment #8507779 - Flags: review?(josea.olivera)
Comment on attachment 8507779 [details]
Implement/Extract MobileConnection Subsystem

LGTM. r=me for the operator variant part.

Sorry for the lag reviewing this Alive.
Attachment #8507779 - Flags: review?(josea.olivera) → review+
Rebased to master and all green now, merging, thanks all.
https://treeherder.mozilla.org/ui/#/jobs?repo=gaia-try&revision=960f2874dc02
https://github.com/mozilla-b2g/gaia/commit/83eaaae5fc609aa414d27a13a0237acb2d647a3a
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Depends on: 1098334
Depends on: 1100406
No longer blocks: 1090859
Depends on: 1100608
blocking-b2g: backlog → ---
You need to log in before you can comment on or make changes to this bug.