971543 - [System2] Instantiable RocketBar

Status: RESOLVED FIXED

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

Description

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

We need rocketbar.js to be instantiable and let bootstrap or its parent module to instantiate it.
Also jsdoc + unit test improvement wanted.

Comment 1

[Kevin Grandon :kgrandon]

Needed for bug 1003960, so taking.

Comment 2

[Ben Francis [:benfrancis]]

I'm really uncomfortable about refactoring Rocketbar again to be instantiable. What's wrong with it being a singleton? It seems to be over-engineering and adding a lot of complexity for no benefit as we will only ever have one Rocketbar. Can someone explain the benefits?

I understand that Kevin you are thinking about doing this in order to extend Rocketbar to build a Rocketbar with less features for 2.0 (which seems like a bit of backwards use of inheritance) but I think it might be cleaner to try to keep "baby Rocketbar" completely separate. Maybe even just part of the search app.

Comment 3

[Kevin Grandon :kgrandon]

I think we can ni? alive for a more in-depth response, but I guess all system modules are being refactored to be instantiable so we can move to a nice module pattern in the future. I think we need to do this regardless if baby rocketbar extends adult rocketbar, but you're right that it might be a bit of a backwards inheritance pattern. (Though it may be called for in our case if we want something standalone, that doesn't duplicate all of the code, and is easily disableable)

The original dev-gaia thread is here with some of the reasoning: https://groups.google.com/forum/#!topic/mozilla.dev.gaia/2Td_9GNgLXo

Comment 4

[Sam Foster [:sfoster] (he/him)]

The main benefits are an explicit lifecycle, with the ability to initialize at will, not as a side-effect of loading the module. This gets you much better testability, as well as the option to extend a thing - for the baby rocketbar, or whatever. It also improves debugging as you can easily inspect and serialize state at any point. 

Personally I seem to get bit sooner or later pretty much *every* time I use a singleton, so while I agree its some busy work, for my money its work that needs to be done, sooner or later.

Comment 5

[Kevin Grandon :kgrandon]

Attachment: Github pull request

Comment 6

[Kevin Grandon :kgrandon]

Comment on attachment 8415583 [details]
Github pull request

Would be great to get some feedback from both of you guys on this one if you have time.

A few notes:

- Follows standard system2 instantiable pattern /w jsdoc.
- Changes unit tests to test a subject (also a common gaia pattern), and instantiates and assigns the Rocketbar to subject. E.g., subject = new Rocketbar()

Comment 7

[Ben Francis [:benfrancis]]

Comment on attachment 8415583 [details]
Github pull request

I'm yet to see a current practical example of why this is necessary, only theoretical future ones. We still initialise at the same time and test all the same things. But if I'm the only one who doesn't think it's necessary then I'm not going to stand in the way of consistency :)

With regards to Baby Rocketbar extending this, my main concern is that moving fast on full Rocketbar features is more likely to regress Baby Rocketbar features as the baby might inherit bugs and the interface needs to be backwards compatible. But let's leave that discussion for that patch.

Comment 8

[Kevin Grandon :kgrandon]

Comment on attachment 8415583 [details]
Github pull request

I think we can go with Bens review here. Alive - if you have any comments feel free to leave them and I will address in a follow-up.

Comment 9

[Kevin Grandon :kgrandon]

Landed: https://github.com/mozilla-b2g/gaia/commit/b53cbf445a17de2028abd20303ec23a8cce3b0e0

Comment 10

[Kevin Grandon :kgrandon]

Mass modify - set status-b2g-v2.0 fixed for fixed bugs under vertical homescreen dependency tree.