We need to do the same thing in Bluetooth as well. For more info, please check bug 729877.

This seems like a gaia logic order issue? Basically, we need to solidify the setting as the very first thing we do, instead of turning off bluetooth (which can take a bit) then changing the setting? That all happens as part of gaia in system.js.

Discussed with Gene, who's in charge of the same part of implementation of Wifi, he said that we need to implement "mozsettings-changed" listener in BluetoothManager. The flow would be: user enable bluetooth by pressing "Enable" button in Settings app --> modify bluetooth.enabled in mozSettings, which means not to call Bluetooth.setEnabled() --> all mozsettings-changed listeners will get notification, including other apps like the quick menu on the top of the status bar and BluetoothManager in Gecko. --> then, BluetoothManager enables Bluetooth. I'm o.k with this change. Waiting for Kyle's suggestion.

Changes: 1. Followed implementation in geolocation (dom/src/geolocation), let BluetoothManager extends nsIObserver. So that we can observe mozSettings changed via function Observe(). 2. Base class of ToggleBtResultTask has been replaced with nsRunnable because we don't send event via DOMRequest anymore. Instead, onenabled & ondisabled event handler were added to nsIDOMBluetoothManager.idl

Comment on attachment 654898 [details] [diff] [review] WIP: implemented mozSettings observer in BluetoothManager Review of attachment 654898 [details] [diff] [review]: ----------------------------------------------------------------- Looks good.

Almost the same as previous feedback'd version, just let HandleMozsettingsChanged function return nsresult 'cause bs->Start & bs->Stop can fail.

Comment on attachment 655913 [details] [diff] [review] v1: patch 1: Hook up settings to Bluetooth Review of attachment 655913 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/bluetooth/BluetoothManager.cpp @@ +193,3 @@ > } > > + JSContext *cx = stack->GetSafeJSContext(); This should probably use aManager->GetContextForEventHandlers. @@ +198,5 @@ > } > > + nsDependentString dataStr(aData); > + JS::Value val; > + if (!JS_ParseJSON(cx, dataStr.get(), dataStr.Length(), &val) || !val.isObject()) { This style is odd -- even though the return value of Observe is unused, it would be cleaner to propagate an exception if the JS_ function fails. This will expand the code a little bit (since you'll have two returns instead of one), but more closely follows JS style and is more correct. @@ +209,5 @@ > + return NS_OK; > + } > + > + JSBool match; > + if (!JS_StringEqualsAscii(cx, key.toString(), "bluetooth.enabled", &match) || (match != JS_TRUE)) { Don't compare directly to JS_TRUE.

Comment on attachment 655913 [details] [diff] [review] v1: patch 1: Hook up settings to Bluetooth Review of attachment 655913 [details] [diff] [review]: ----------------------------------------------------------------- Looks decent from the review side, cancelling review until sr fixes are made though.

> > ::: dom/bluetooth/BluetoothManager.cpp > @@ +193,3 @@ > > } > > > > + JSContext *cx = stack->GetSafeJSContext(); > > This should probably use aManager->GetContextForEventHandlers. > > @@ +198,5 @@ > > } > > > > + nsDependentString dataStr(aData); > > + JS::Value val; > > + if (!JS_ParseJSON(cx, dataStr.get(), dataStr.Length(), &val) || !val.isObject()) { > > This style is odd -- even though the return value of Observe is unused, it > would be cleaner to propagate an exception if the JS_ function fails. This > will expand the code a little bit (since you'll have two returns instead of > one), but more closely follows JS style and is more correct. > The content of function HandleMozsettingChanged() was mostly copied from nsGeolocationService::HandleMozsettingChanged() & AutoMounterSetting::Observe(), I'm not sure if it's a good idea to change the style. In addition, since HandleMozsettingChanged() will get called for all settings changes, some JS_ functions returning false just means we're not interested in this JSON message. Do we really need to return NS_ERROR_FAILURE for each failures of those JS_ functions?

Modified according to mrbkap's comment

Comment on attachment 656358 [details] [diff] [review] v2: patch 1: Hook up settings to Bluetooth The problem with having code that mixes JS_* function returning false cases with other types of errors is that in general it's wrong. Here, you're OK because you can assume a bunch of stuff thanks to the fact that you're in an observer, but it's a bad habit to get into (and IMO worth commenting that because you *know* that you're not called from JS, returning true, even when JS_ functions fail, won't leave exceptions sitting around on the context.

Added comments in HandleMozSettingsChanged.

try: https://tbpl.mozilla.org/?tree=Try&rev=80f13007c219