1123066 - B2G RIL: Do some JavaScript cleaning up for 'ril_worker.js' (tracking bug)

Status: RESOLVED WONTFIX

(Firefox OS Graveyard :: RIL, defect)

Description

[Greg Weng [:snowmantw][:gweng][:λ]]

The 'ril_worker.js' now is a 16K+ LOCS JavaScript file since Vicamo first fired the Bug 811754 to try to clean it up, at that moment it's "just" a 7K+ JavaScript file (see Bug 811754 Description). It would be fine if it's just huge but with one rigorous and uniform style and even architecture. Unfortunately, the components in the file comes with very different styles, and the dependencies is somewhat unclear.

From my colleagues I've heard that there are several attempts to split them and re-write the components in C++. However, the plan is still plain, meanwhile Gecko do continuously show the world how a most webby platform dogfoods it's own webby technologies in a way that most of real web developers wouldn't do. So I think it may be worth to slightly re-organize code in the file, at least uniform the styles before we could done the C++ migrating work. I understand RIL is very important and I'm a new to do any work for that, but there are still some things absolutely could be done without harness:

1. We have another huge JS file, the 'ril_consts.js', which contains almost all consts. However, there are sill some consts in 'ril_worker.js'. If the 'ril_consts' really intend to be the place to put all consts in it, we could move these consts into the file, unless we have some implicit concerns about that.

2. There are at least two styles to define a constructor with its prototype in the file. From our experience of Gaia development, to have one style is definitely better to get people confused and switching between different styles, especially in the same file. I prefer to use 'anonymous function wrapper + component.prototype.method' style to refactor this part, as we used in Gaia. And since this is just a style change, it should harm no functionalities.

3. Rather than using literal object to create 'singleton', in Gaia we uniformed our way to create instance with constructor-prototype style. The strong reason is the literal object 'singleton' made us suffer from testing to clarifying the timing of components bootstraping. So we should remove literal object style if it's possible.

4. Some functions & objects look like lack of context. For example, at the end of the file, there is a 'onmessage' method that tells us no context about where it belongs to and what functional group it should be with. All members of this file should be organized as under the proper 'namespaces', namely 'components' with constructors and prototypes. We of course would left some bootstraping code to be evaluated while we load the file since JavaScript lacks the unique entry of a file of the program, but such entry should be the only one exception.

5. From Bug 811754 it tried to split the file into different files. If there is no critical performance issue of loading those files, I tend to do the same work. And even with such issue, we could still split it and then merge it back to a monolithic file in the build time. Such a ultimate file should be read by machine only, and it's also could be the first to re-write these parts in C++.

I would consult some RIL forks, and update the status at this bug. For real changes I would fire other bugs according to the subjects.

Comment 1

[Greg Weng [:snowmantw][:gweng][:λ]]

(And I just found in Gaia we're forced to use single quotes, while in ril_worker.js people use double quotes. Should we have some principles for JavaScript code, no matter where they are?)

Comment 2

[[:fabrice] Fabrice Desré]

(In reply to Greg Weng [:snowmantw][:gweng][:λ] from comment #1)
> (And I just found in Gaia we're forced to use single quotes, while in
> ril_worker.js people use double quotes. Should we have some principles for
> JavaScript code, no matter where they are?)

Historically chrome JS code used double quotes, and I still prefer that. I'm not even sure what the argument for single quotes is when you have no escaping issues as in attribute values. So please don't do a mass replacement of double to single quotes, that would mess up the blame for no real value.

I'm quite surprised we want to rewrite the RIL worker in c++. What is the expected gain?
Also iirc, this file was intentionally written in a "js like C" style to minimize gc work. Beware of that if you make it use all the shinny js features - even though our gc improved greatly since we started b2g.

Comment 3

[Greg Weng [:snowmantw][:gweng][:λ]]

The quote is just a sign that we have different thoughs on how to write JS.I don't want to argue that actually, and in fact I support double quotes for strings, and single quotes for 'enum' or 'char' that types JS should but haven't had. So it's actually an inversed question that why Gaia couldn't adopt this, or other things that Gecko did them well. Anyways, I think the issue is both too minor and too too big for this bug, since the real question may be how to share the benefits and concerns of JS conventions between Gaia and Gecko, so I wouldn't handle it.

For the C++ migration what I have heard is  it can help us to save the memory and maybe speed it up. That is, if we could implement the manual GC code right. However, I don't know the details, so it's a question need some NI.

I understand the intentions you mentioned, so I wouldn't do too much changes. However, even pure C works deserve a clear and uniformed style in each single file. For example, I hardly to believe C or C++ guys would be glad to see both K&R and C99 styles mixed in the same file, not to mention the file has its own size issue, that I think even in C works, except you want to write some special things like the linker, you usually split it according to the function groups and then using including mechanism to weave them together, although I don't know if for chrome JS like this there are some concerns to prevent us to do the similar thing so it didn't happen before (does 'importScript' really cost lots of memory?). So, no, I neither would use any too edging techs to do the work, nor be too orthogonal to insist the 'right' looka & feels and waste lots of time to argue about that. What I want to do is try to eliminate the unnecessary complex in the one single file, and maybe split it as Bug 811754 did, if to import different scripts does no harm.

Comment 4

[[:fabrice] Fabrice Desré]

Sounds good to me Greg. I'm not opposed to refactoring if that helps us get the code base in a better shape!

Note that you can split up the file without using importScript with #include, like in http://mxr.mozilla.org/mozilla-central/source/browser/base/content/newtab/newTab.js#57

Comment 5

[Greg Weng [:snowmantw][:gweng][:λ]]

Great! Than I think I could do the splitting job without the overhead concern. I would fire other bugs later, and submit WIP patch to discuss if the proposal is acceptable.

Comment 6

[BMO Automation]

Firefox OS is not being worked on