1206641 - Merge BaseModule to TV System App

Status: RESOLVED FIXED

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

Description

[Evelyn Hung]

Since we are going to backport some components from phone's system app (2.5) to TV system (2.1), with BaseModule back-ported first, our life will be easier. However, we won't migrate all components in smart-system in BaseModule shape, we just want back-ported new features using BaseModule.

Comment 1

[Evelyn Hung]

System owners, it seems we only need bug 1072757. Could you suggest any follow-up or related bugs we need to pick as well?

Comment 2

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

I would say bug 1072757 turn out to be the fundamentals of bug 1094759 (More than just SettingsCore) and I am not aware of other bugs.

Is the scope of this bug is only about allowing new TV code to inherit BaseModule? If so I guess bug 1094759 or some kind of smart copy-paste of the current System app should do it.

I would not recommend TV system try to re-implement bug 1094759 in it's own folder -- that would be a departure from trying to merge the two app instead of a way to get there -- unless we want to experiment first.

Comment 3

[Etienne Segonzac (:etienne)]

(In reply to Evelyn Hung [:evelyn] from comment #1)
> System owners, it seems we only need bug 1072757. Could you suggest any
> follow-up or related bugs we need to pick as well?

If the goal is to be able to migrate modules with little to no change I think you'll also need
* bug 1062819 for the STATES support
* bug 1080905 (simple rename, but it should help preventing conflicts)
* part of bug 1094759 for the |loadWhenIdle| support (you might want to take just the base_module.js/base_module_test.js part of the patch...)
* bug 1156596 which I think is for the TV :)
* bug 1166214

Comment 4

[Etienne Segonzac (:etienne)]

Racing a bit but looks like we agree :)

Comment 5

[Evelyn Hung]

It's not possible to redo the whole work of bug 1094759 nor back porting it to TV. 

What we want is when an existing phone's module uses |BaseModule.create| to create itself, the code could be run on TV as well without too much modification (e.g. bug 1206621). Therefore, my guess was just bug 1072757 needed. Less modification at this moment helps two System merge task in the future.

Comment 6

[Evelyn Hung]

(In reply to Etienne Segonzac (:etienne) from comment #3)
> (In reply to Evelyn Hung [:evelyn] from comment #1)
> > System owners, it seems we only need bug 1072757. Could you suggest any
> > follow-up or related bugs we need to pick as well?
> 
> If the goal is to be able to migrate modules with little to no change

Yep, you know me. :)

> I think you'll also need
> * bug 1062819 for the STATES support
> * bug 1080905 (simple rename, but it should help preventing conflicts)
> * part of bug 1094759 for the |loadWhenIdle| support (you might want to take
> just the base_module.js/base_module_test.js part of the patch...)
> * bug 1156596 which I think is for the TV :)
> * bug 1166214

Great! Thank you. I will take a look of these bugs.

Comment 7

[Evelyn Hung]

Attachment: backport BaseModule to TV

In this patch, I try to just pick a common and minimum set of functions provided by BaseModule, therefore only backported Bug 1072757 and Bug 1062819. Now we have SERVICES, STATES, IMPORTS, SUB_MODULES, EVENTS, and SETTINGS of BaseModule. I tested SERVICES, STATES and SUB_MODULES by copying FeatureDetector to TV, and it works well. Not sure if there is a simple way to test EVENTS and SETTINGS, I will keep investigating.

I didn't take bug 1094759 for the |loadWhenIdle| support because it seems not so critical now, and we could always backport this part if we find a case using it. Bug 1156596 and bug 1166214 are made for the preparation of system app merge, so we won't need it now.

I'd like to get your feedback first. Thank you. :)

Comment 8

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

Comment on attachment 8664129 [details] [review]
backport BaseModule to TV

It is preferable if we could backport the tests too to prove/maintain the claims with machines :)

Comment 9

[Etienne Segonzac (:etienne)]

Comment on attachment 8664129 [details] [review]
backport BaseModule to TV

(In reply to Tim Guan-tin Chien [:timdream] (slow response; please ni? to queue) from comment #8)
> Comment on attachment 8664129 [details] [review]
> backport BaseModule to TV
> 
> It is preferable if we could backport the tests too to prove/maintain the
> claims with machines :)

Yeah we definitely want to backport base_module_test.js too :)

But it might me easier to just snapshot the current version of base_module.js and base_module_test.js. They are pretty stable and I don't think there's much value in keeping references to older system bugs since we're only partially backporting them (and not cherry picking commits).

The changes to core.js and service.js should still be good.

Comment 10

[Evelyn Hung]

Comment on attachment 8664129 [details] [review]
backport BaseModule to TV

Updated according to comment 9 that I just copied the current base_module.js and its test to smart-system, and it works! Let me know if I missed anything else. Thanks!

@yifan, since your work might depend on this module when you backporting Data Sync logic to TV, please checkout this patch as well, and let me know if it doesn't fit your case. Thanks!

Comment 11

[Etienne Segonzac (:etienne)]

Comment on attachment 8664129 [details] [review]
backport BaseModule to TV

Looking good, we should probably squash the PR too.
Trick question: are the tv_apps/* unit tests running on try?

Comment 12

[yifan [:yifan][:yliao]]

Yes, according to Bug 1162887 - (gu-tv) Support tv_apps unit test on Gu job . For example we could find smart-home and smart-system unit tests in GU20.

Comment 13

[yifan [:yifan][:yliao]]

Comment on attachment 8664129 [details] [review]
backport BaseModule to TV

Ran GijTV locally and all passes.

Notice some difference in the core.js. While trying to backport sync to smart-system with this patch, I found that the _start method in core.js is quite different, one major difference is that the method in system/js/core.js returns a Promise. Not sure if this will break any code after we add lazily loaded modules in smart-system/js/core.js. Probably need some check to make core.js work as apps/system does.

Comment 14

[KM Lee [:rexboy] (inactivate after Sep. 22)]

Comment on attachment 8664129 [details] [review]
backport BaseModule to TV

I'm just not sure which service.js you used, but looks good to me.

Comment 15

[Evelyn Hung]

There is a service.js in smart-system which was added by John, probably a snapshot when we forked system app. Thing I did in this patch is adding just necessary code for base module.

Comment 16

[Evelyn Hung]

Merge into gaia-master
https://github.com/mozilla-b2g/gaia/commit/106f1806a6889fedd6708aebd3a602cf71ca8ee0