940164 - (gaia-bootstrap) [System2] Refine Gaia Bootstrap Process

Status: RESOLVED FIXED

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

Description

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

The role of bootstrap and appWindowManager isn't clear. We should move some code from bootstrap to AWM or init AWM in bootstrap.

Comment 1

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

On it.

Comment 2

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

WIP
https://github.com/alivedise/gaia/compare/system2?expand=1

Comment 3

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

Attachment: WIP bootstrap

It's still in progress. The naming of the new modules might change, but basically:
* App.js will launch Non-API sensitive modules at first
* Then launch API-sensitive modules
* ApplicationsMediator will launch all window management modules.
* If a module A is heavily relying on another module B, B should launch A.

Next:
* Change all init() to start()
* Testing

Comment 4

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8470767 [details] [review]
WIP bootstrap

Look great, thank you very much for figuring out the approach toward a cleaner System app.

I am not entirely sure the risk of this patch, so we should target landing the patch in this week to avoid regressions right before FL (though regressions caused by start-up sequence changes can be easily addressed).

Comment 5

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

Attachment: System 2 Bootstrap ver2

Hey, this is not working now(because I remove the initlogo handler), I am fixing it soon, but would like you to take a look.

* Implement BaseModule to behave all module parent in system app.
* Implement core.js to lazy load singletons as sub modules and do API detection to lazy load all API handler.
* Facade pattern: hide the detail as most as global object under |System|
* Remove as most as javascripts from the system app header.
* Work out the dependency tree between all the singletons in system app.

On going:
* Make it works.
* Implement TelephonyHandler & MobileConnectionsHandler.
* Lazy load all DOM(View) in BaseModule.
* Change every module in system to this form.
* Think about a way to merge or split this patch.
* Try loading stuff in an iframe sandbox as Vivien's favorites.

I know some of you don't like AMD but prefer native module solution,
but as I said many time, what loader we choose is not the problem, but how we design is. In the past two years until now I feel I am the only person care about these.

I am opened to change all of these modules into AMD/harmony/whatever styles in another bug if necessary. But no cleanup, no switch is possible.

System app is now growing more complex. I hope we could fix the dependency issues before things go out of control or before we need to rewrite but not refactor.

Thanks to people working on instantiable stuff this change is not that painful now.

Comment 6

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8474509 [details]
System 2 Bootstrap ver2

Feedback:

I agree the System app need a deterministic start-up path and a centralized messaging system, but I am not entirely sure that justify requiring having ALL modules prototype-inherit a BaseModule. It divergences from plain JS function/object too much and requiring people to understand a lot of things before writing their first module. You are going to meet with a few resistance on that and we will have trouble bringing new people into System app. Can we simply use something as simple as |this.app.addListener()| pattern? If it's a concern that people will ignore them, we can remove the native API and force people to use our wrapped methods.

Also, I don't think the circumstances of this project allow you to land a patch of this size in any phase of development. Please work on a better landing strategy before we could continue. We have done this before in removing window_manager.js and keyboard.js, we can certainly do that again.

Anyway, thank you for taking the initiative. I do care about this very much; it's not true that no one cares about it, but I am overwhelmed by other stuff.

Comment 7

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

(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) (please ni?) from comment #6)
> Comment on attachment 8474509 [details]
> System 2 Bootstrap ver2
> 
> Feedback:
> 
> I agree the System app need a deterministic start-up path and a centralized
> messaging system, but I am not entirely sure that justify requiring having
> ALL modules prototype-inherit a BaseModule. It divergences from plain JS
> function/object too much and requiring people to understand a lot of things
> before writing their first module. You are going to meet with a few
> resistance on that and we will have trouble bringing new people into System
> app. Can we simply use something as simple as |this.app.addListener()|
> pattern? If it's a concern that people will ignore them, we can remove the
> native API and force people to use our wrapped methods.

For new comer and new module after this landed,
we need to consider it case by case.

Yes, a new module could just ignore baseModule if it don't need it.

What I am concern is:
## If it is a singleton, like mediator/manager/handler ##
* Who depends this new module?
* Who is responsibility to load and launch this new module?
## If it is not a singleton, like appWindow ##
* Who is responsibile to load this class?

We should always answer these questions when we invent new modules afterwards.
The centralized code base is something better for maintenance. I will think about this again.

I agree that people should have his/her freedom to create a new module.
But freedom means out of control at some time.

If a new module like FxA or mobileID is living in its own iframe context I will say yes, do whatever it wants.

> 
> Also, I don't think the circumstances of this project allow you to land a
> patch of this size in any phase of development. Please work on a better
> landing strategy before we could continue. We have done this before in
> removing window_manager.js and keyboard.js, we can certainly do that again.

If BaseModule is not *highly* resisted/rejected, I will start from telephony handler after bug 927862 lands.

> 
> Anyway, thank you for taking the initiative. I do care about this very much;
> it's not true that no one cares about it, but I am overwhelmed by other
> stuff.

Comment 8

[Etienne Segonzac (:etienne)]

Comment on attachment 8474509 [details]
System 2 Bootstrap ver2

I took a look at core/bootstrap/system/base_module + a few small modules that I know well.

I like the approach, I think the content of the base module is very pertinent. But I wish we could separate the listeners helpers (events, sysmsg, settings) from the imports/submodule stuff.

I agree with Tim that the main issue is how to roll this out. We can do it by phase (windowmanager, hardware related modules, radio related modules...) but definitely not in 1 bug. Which means we need a stable way to make this cohabit with the current bootstrap (non-)architecture... :/

One last note: since one of the goals is to ease system app development we should think about a base_module_test helper too :)

Comment 9

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

(In reply to Etienne Segonzac (:etienne) from comment #8)
> Comment on attachment 8474509 [details]
> System 2 Bootstrap ver2
> 
> I took a look at core/bootstrap/system/base_module + a few small modules
> that I know well.
> 
> I like the approach, I think the content of the base module is very
> pertinent. But I wish we could separate the listeners helpers (events,
> sysmsg, settings) from the imports/submodule stuff.

Do you mean you dislike we centralize event/message/settings handler in base module?
What's you concern here?

* Install an eventHelper subModule for baseModule and any module needs.
* Don't like centralized event handler anyway.

BTW this is already working in window management.
I expect all the event publish/subscribe could be managed in the same place so it could be easily changed to event emitter or something else.

For system message Tim has different opinion.

> 
> I agree with Tim that the main issue is how to roll this out. We can do it
> by phase (windowmanager, hardware related modules, radio related modules...)
> but definitely not in 1 bug. Which means we need a stable way to make this
> cohabit with the current bootstrap (non-)architecture... :/

Yeah, as I said, what I am doing is:
* Prove it work
* Get approval from system app owners/peers
* Split it into lots of patches/bugs.

> 
> One last note: since one of the goals is to ease system app development we
> should think about a base_module_test helper too :)

Not sure what is base_module_test helper, is it unit test of base_module or something different?

Comment 10

[Etienne Segonzac (:etienne)]

(In reply to Alive Kuo [:alive][NEEDINFO!] from comment #9)
> (In reply to Etienne Segonzac (:etienne) from comment #8)
> > Comment on attachment 8474509 [details]
> > System 2 Bootstrap ver2
> > 
> > I took a look at core/bootstrap/system/base_module + a few small modules
> > that I know well.
> > 
> > I like the approach, I think the content of the base module is very
> > pertinent. But I wish we could separate the listeners helpers (events,
> > sysmsg, settings) from the imports/submodule stuff.
> 
> Do you mean you dislike we centralize event/message/settings handler in base
> module?
> What's you concern here?

Oh no I like it!
I'm just wondering if separating those concerns would ease the adoption by modules.

> 
> * Install an eventHelper subModule for baseModule and any module needs.

More like a standalone event helper. But that's no big deal.
I'm just looking for ways to split the work into manageable chunks.

> BTW this is already working in window management.
> I expect all the event publish/subscribe could be managed in the same place
> so it could be easily changed to event emitter or something else.
> 
> For system message Tim has different opinion.

Is the issue having this code for all submodules or just having an helper for something rather simple?

> > One last note: since one of the goals is to ease system app development we
> > should think about a base_module_test helper too :)
> 
> Not sure what is base_module_test helper, is it unit test of base_module or
> something different?

Let me rephrase.
I think the way you extracted the essence of a system module as
- listening to settings
- listening to/publishing events
- listening to system messages
- managing submodules
is really pragmatic.

And it means that all the test suites for those modules will need:
- to mock the settings api
- to make sure events are dispatched properly
- to mock setMessageHandler
- to load certain submodules (or parent modules)

We should make this easy, and fix the inconsistencies we currently have along the way.

Comment 11

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

Vivien said he agree this in general on IRC.

If we all think this is the correct direction, I am going to split this patch into amount of bugs/patches and ask for review. The first playground should be mobileConnection stuff.

Thanks everyone.

Comment 12

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

WIP: Implement Core+BaseModule+MobileConnectionCore
https://github.com/mozilla-b2g/gaia/pull/23853

Comment 13

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

(In reply to Alive Kuo [:alive][NEEDINFO!] from comment #12)
> WIP: Implement Core+BaseModule+MobileConnectionCore
> https://github.com/mozilla-b2g/gaia/pull/23853

Next:
* Remove all SIMSlotManager reference
* Make all sub modules under mobileConnectionCore inherit baseModule.
* Split statusbar if necessary

Comment 14

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

Attachment: Implement BaseModule + Core + MobileConnectionCore

The patch:
* Introduce BaseModule
* Introduce SystemCore
* Introduce MobileConnection Subsystem Core
* Enhance System interface
* Resolve the simpin* modules messy to certain degree
* Resolve the operator_variant* modules messy to certain degree
* Boot-able, no any test yet.

Comment 15

[Etienne Segonzac (:etienne)]

Comment on attachment 8487168 [details] [review]
Implement BaseModule + Core + MobileConnectionCore

Definite f+ on the shape, the base_module and on starting with this subset of the system app!

I don't know those modules very well, so we should probably request early feedback from the "top of the blame" for these files too.

I wonder if "MobileConnectionCore" we'll evolve into a "TelephonyCore" down the road but I much prefer keeping the scope of this patch as small as possible for now. I also wonder if we have modules with multiple API dependencies but again, not for this patch :)

Not so sure about |System.getAPI| it might make grep-ing harder and doesn't add much imo, but I could be convinced otherwise.

To sump up: shaping up nicely :)

Comment 16

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

(In reply to Etienne Segonzac (:etienne) from comment #15)
> Comment on attachment 8487168 [details] [review]
> Implement BaseModule + Core + MobileConnectionCore
> 
> Definite f+ on the shape, the base_module and on starting with this subset
> of the system app!
> 
> I don't know those modules very well, so we should probably request early
> feedback from the "top of the blame" for these files too.
> 
> I wonder if "MobileConnectionCore" we'll evolve into a "TelephonyCore" down
> the road but I much prefer keeping the scope of this patch as small as
> possible for now. I also wonder if we have modules with multiple API
> dependencies but again, not for this patch :)

Multiple API dependency is something we need to study case by case!
1. IccManager and MobileConnection are always coupled but I am not going to fix it in this bug
2. MobileConnection and Telephony doesn't have strong link in backend.

For IccManager/MobileConnection stuff, one idea is having a more high level Module to load both of them and make them reference each other.
For MobileConnection/Telephony I think we should just keep them seperated until we have strict use case. A third party module to resolve the dependency is necessary if we see some use case. At least we could use System as the sandbox to let modules belong these two subsystems to play well with each other.

> 
> Not so sure about |System.getAPI| it might make grep-ing harder and doesn't
> add much imo, but I could be convinced otherwise.

I am not sure, too. My initial purpose is to remove all navigator.XXXX reference from the code base and inject the mock stuff in System.getAPI if necessary. If this doesn't fit and nobody uses it in the long run, we could easily switch back I think.

> 
> To sump up: shaping up nicely :)

:)

Comment 17

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

Comment on attachment 8487168 [details] [review]
Implement BaseModule + Core + MobileConnectionCore

Hi Jose,
I want to acquire your help on the operatorVariant* incoming refactor since it's a little hard for me to test it locally. This is still in progress, so unit test is not reworked.

The goal of this bug is lazy loading all mobileConnection-oriented modules in system app and build these modules on a tiny framework + library.

I hope you could take a look for now and maybe I will need your helper later on real device test. Thanks!

Comment 18

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

Today progress:
* Move AirplaneModeServiceHelper out of AirplaneMode. (done)
* Rework AirplaneMode unit test. (progressing)
Todo:
* Develop a mocksHelper-like helper to inject all baseModule related stuff for testing purpose.

Comment 19

[José Antonio Olivera Ortega [:jaoo]]

Comment on attachment 8487168 [details] [review]
Implement BaseModule + Core + MobileConnectionCore

(In reply to Alive Kuo [:alive][NEEDINFO!] from comment #17)
> Comment on attachment 8487168 [details] [review]
> Implement BaseModule + Core + MobileConnectionCore

> I hope you could take a look for now and maybe I will need your helper later
> on real device test. Thanks!

Looking good. Sure, I'll be happy to help!

Comment 20

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

Status update:

* I designed the process the register/request services to enable cross subsystem communications in System.
* With that we will have
System.register('isMultiSIM', SIMSlotManageer);
System.register('addObserver', SettingsCore);

System.request('addObserver').then(...);
System.request('readSettings').then(...);
System.request('UtilityTray:open');

So the problem the previous event dispatching is if the event subscriber is not started before the event is dispatched, we will lose the event. With this mechanism, System will queue the request until the desired service provider is started and call System.register(SERVICE_NAME, this); in its start() function ends.

Also if we want to query some information from other module we need to know the object before accessing it and this causes loading sequences issues. Now system will return a promise to let the queryer to know when it's ready to get the result.

System.query('isMultiSIM').then(function() {}) will be mapped to MobileConnectionCore.isMultiSIM
System.query('locked').then(function() {}) will be mapped to LockscreenWindowManager.locked

Comment 21

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

(In reply to Alive Kuo [:alive][NEEDINFO!] from comment #20)
> Status update:
> 
> * I designed the process the register/request services to enable cross
> subsystem communications in System.
> * With that we will have
> System.register('isMultiSIM', SIMSlotManageer);
> System.register('addObserver', SettingsCore);
> 
> System.request('addObserver').then(...);
> System.request('readSettings').then(...);
> System.request('UtilityTray:open');
> 
> So the problem the previous event dispatching is if the event subscriber is
> not started before the event is dispatched, we will lose the event. With
> this mechanism, System will queue the request until the desired service
> provider is started and call System.register(SERVICE_NAME, this); in its
> start() function ends.
> 
> Also if we want to query some information from other module we need to know
> the object before accessing it and this causes loading sequences issues. Now
> system will return a promise to let the queryer to know when it's ready to
> get the result.
> 
> System.query('isMultiSIM').then(function() {}) will be mapped to
> MobileConnectionCore.isMultiSIM
> System.query('locked').then(function() {}) will be mapped to
> LockscreenWindowManager.locked

Now I am stuck in the test because there are too many things to rework.
I could keep working and rebase every day, but it's really time-consuming...wanna to split the patch *again* and open followups for the remaining part, what do you think?

Proposed 1st part:
* System
* BaseModule
* Core
* SettingsCore

Comment 22

[Etienne Segonzac (:etienne)]

(In reply to Alive Kuo [:alive][NEEDINFO!] from comment #21)
> (In reply to Alive Kuo [:alive][NEEDINFO!] from comment #20)
> > Status update:
> > 
> > * I designed the process the register/request services to enable cross
> > subsystem communications in System.
> > * With that we will have
> > System.register('isMultiSIM', SIMSlotManageer);
> > System.register('addObserver', SettingsCore);
> > 
> > System.request('addObserver').then(...);
> > System.request('readSettings').then(...);
> > System.request('UtilityTray:open');
> > 
> > So the problem the previous event dispatching is if the event subscriber is
> > not started before the event is dispatched, we will lose the event. With
> > this mechanism, System will queue the request until the desired service
> > provider is started and call System.register(SERVICE_NAME, this); in its
> > start() function ends.
> > 
> > Also if we want to query some information from other module we need to know
> > the object before accessing it and this causes loading sequences issues. Now
> > system will return a promise to let the queryer to know when it's ready to
> > get the result.
> > 
> > System.query('isMultiSIM').then(function() {}) will be mapped to
> > MobileConnectionCore.isMultiSIM
> > System.query('locked').then(function() {}) will be mapped to
> > LockscreenWindowManager.locked
> 
> Now I am stuck in the test because there are too many things to rework.
> I could keep working and rebase every day, but it's really
> time-consuming...wanna to split the patch *again* and open followups for the
> remaining part, what do you think?
> 
> Proposed 1st part:
> * System
> * BaseModule
> * Core
> * SettingsCore

Yes we can do a "plumbing" patch first if it's well tested / documented (which I'm sure it will!).
And we should make sure the mocks have an easy way to buy in the register/request system.

Also writing this down as a reminder: but we should do a quick perf/memory test during the review of every of the steps in this project :)

Comment 23

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

(In reply to Etienne Segonzac (:etienne) from comment #22)
> 
> Yes we can do a "plumbing" patch first if it's well tested / documented
> (which I'm sure it will!).
> And we should make sure the mocks have an easy way to buy in the
> register/request system.

https://bugzilla.mozilla.org/show_bug.cgi?id=1072757 is filed.

> 
> Also writing this down as a reminder: but we should do a quick perf/memory
> test during the review of every of the steps in this project :)

For this part what kind of exact ations do you suggest to do? Using developer HUD? Or utilizing other tools (sorry, unfamiliar with tools we have now.)

Comment 24

[[e-mail]]

Is it an outdate issue that we can close it?

Comment 25

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Look like everything is landed in bug 1094759.