I support to deprecate it because of the issues you mentioned. However, when people tried to wrap an API again and again, it obviously something failed to make people happy to use the API without any wrappers, although some debts are from the time we don't have any better way to manage async flow like Promise promised.
So, IMO we may better to find an usable (hopefully compact and nice to use) alternative before we wipe out the code use the listener.Maybe Tim could elaborate how to use his solution to solve the common issues in Gaia, with a gist or readme under the app, and we could replace the listener with that stepwise. Or, if it's not general enough, we could evaluate different solutions with clear descriptions. At least, we should have some clear practices about how to replace it with the semantically equal snippets manually manipulating the API, if we don't expect an one-man army to submit a 3K+ lines patch across different components to do that. Without such best practices, people involve in may submit and land different patches with very different way from callback to Promise to handle the similar issues in different components, which would definitely add the entropy of our codebase, not eradicate that. 2015-07-25 17:32 GMT+08:00 Tim Guan-tin Chien <timdr...@mozilla.com>: > 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-gaia mailing list > dev-g...@lists.mozilla.org > https://lists.mozilla.org/listinfo/dev-gaia > -- <http://about.me/snowmantw>Greg Weng http://about.me/snowmantw *Understand y f = f [ y f ] ; lose last remaining non-major friend* * -- Anonymous*
_______________________________________________ dev-b2g mailing list dev-b2g@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-b2g
