Hi, SettingsListener was one of the oldest piece of Gaia, and it even reaches into Gecko b2g/. However for years it's problem has not been widely communicated.
== Problems == 1) The "default value" on the 2nd parameter It asks for a "default value" on the 2nd parameter, and it will use it when there is no mozSettings available and when mozSettings returns undefined. Both behavior does not make sense nowadays because: 1.1) It makes no sense to return a fail safe value when mozSettings breaks. We also don't expect Gaia apps to run on environments w/o mozSettings. 1.2) It creates two or more places in the code base in which people would need update the "default value". The only place the default value should live is the common-settings.json, which is the preload values of mozSettings. We should use it and bump the db version in Gecko to make the new value follow into the profile, instead of duplicate that value in the code base. 2) Races, start-up races everywhere The callback will be invoked when the value is first returned (via a get() call), and when the installed observer invokes due to the change in value. Before the first value returns, the value remain unknown in the JS world but users of SettingsListener.observe() usually assume user will not reach their code before getting that value. That's a false assumption. The code ended up behaves incorrectly with an undefined value (or, with an default value which assumed safe but may not be). 3) It hides lock management from the caller The current implementation hides the lock inside the library, and reuse if in the same function loop, but in fact lock exists to facilitate ... lock on the database changes. There will be some follow-up on the problem with mozSettings itself but in the mean time we should stop using an helper library that hide locks and introduce footguns around possible setting change races. == Proposal == To my surprises, people have not moving away from it already on other apps. Either all the uses have carefully workaround the problems stated, or the bugs are still there. My proposal here is simply deprecate SettingsListener and have each of the users interact with mozSettings directly, or have each of the apps develop libraries they feel comfortable. Async flow in JS is way better than what we have at the beginning of the project, thank to mostly the introduction of Promise. We have also learned a lot through bugs and regressions. Given the fact DOMRequest is already a then-able, it's shouldn't be hard to manage the code and make it only do things (or response to things) only until the setting value is available. Even in just one System app, there are already multiple library-level implementations that attempt to wrap mozSettings. Without saying which one is the best one, since the problem with SettingsListener should be understood, I would assume these implementations should not be suffering the problem that SettingsListener have. I personally did a implementation in Keyboard app too [1], but it might not be general enough. If this is a generally agreed path to move forward, we should start doing it and new patches should stop using SettingsListener. We would have to deal with the same thing in Gecko too. I don't know who will be responsible of that though. Thoughts? Tim PS A related topic would be a thread named "mozSettings API refinement proposal" started by Alive (hat tip!). Proposal was generally perceived well but left unimplemented beyond DOMRequest#then. We should get better at tracking these things. [1] https://github.com/mozilla-b2g/gaia/blob/master/apps/keyboard/js/keyboard/settings.js _______________________________________________ dev-b2g mailing list [email protected] https://lists.mozilla.org/listinfo/dev-b2g
