777665 - [b2g-bluetooth] hook up to settings API

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

[Eric Chou [:ericchou] [:echou]]

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

Comment 1

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

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.

Comment 2

[Eric Chou [:ericchou] [:echou]]

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.

Comment 3

[Eric Chou [:ericchou] [:echou]]

Attachment: WIP: implemented mozSettings observer in BluetoothManager

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 4

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Comment on attachment 654898 [details] [diff] [review]
WIP: implemented mozSettings observer in BluetoothManager

Review of attachment 654898 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good.

Comment 5

[Eric Chou [:ericchou] [:echou]]

Attachment: v1: patch 1: Hook up settings to Bluetooth

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

Comment 6

[Blake Kaplan (:mrbkap) (inactive)]

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 7

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

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.

Comment 8

[Eric Chou [:ericchou] [:echou]]

> 
> ::: 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?

Comment 9

[Eric Chou [:ericchou] [:echou]]

Attachment: v2: patch 1: Hook up settings to Bluetooth

Modified according to mrbkap's comment

Comment 10

[Blake Kaplan (:mrbkap) (inactive)]

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.

Comment 11

[Eric Chou [:ericchou] [:echou]]

Attachment: Final: Hook up settings to Bluetooth, r=qdot, sr=mrbkap

Added comments in HandleMozSettingsChanged.

Comment 12

[Eric Chou [:ericchou] [:echou]]

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

Comment 13

[Eric Chou [:ericchou] [:echou]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9659ba8dfa67

Comment 14

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/9659ba8dfa67