Right now we use a single BluetoothAdapter as the root object for B2G's bluetooth dbus implementation. However, this won't scale to multiple adapters, and puts too much context on the creation of the adapter. There needs to be a singleton object one level higher that deals with the root dbus event handling, creation/deletion/management of adapters, so on and so forth.

Comment on attachment 623907 [details] [diff] [review] v1: Adding Bluetooth Manager object Nevermind on review. Forgot to cleanup events that aren't even there anymore in BluetoothManager.

Fixed the unused event blocks. Note that BluetoothManager still needs to be an event handler, since it will handle things like new adapters being plugged into the system. This will just be implemented in a later bug.

Forgot to rename setBluetoothEnabled to setEnabled

Comment on attachment 623921 [details] [diff] [review] v3: Adding Bluetooth Manager object Ok I give up. Compile error in the patch. Going to deal with this tomorrow. :|

Comment on attachment 624155 [details] [diff] [review] v4: Adding Bluetooth Manager object I'm not an sr, you just need a dom peer to do a regular review.

Ah, ok. Misunderstood, thanks for the update.

Comment on attachment 624155 [details] [diff] [review] v4: Adding Bluetooth Manager object Review of attachment 624155 [details] [diff] [review]: ----------------------------------------------------------------- Sorry for the delay. Not stamping because I think that we should make a common helper function for the security check stuff, but found some other small things too. ::: dom/base/Navigator.h @@ +69,4 @@ > #include "nsIDOMNavigatorBluetooth.h" > #endif > > +class nsIDOMManager; Nit: Looks like this should get removed since it's clearly not forward declaring anything real. ::: dom/bluetooth/BluetoothAdapter.h @@ +28,4 @@ > NS_DECL_CYCLE_COLLECTION_CLASS_INHERITED(BluetoothAdapter, > nsDOMEventTargetHelper) > > + BluetoothAdapter(); Nit: Empty constructors can either be inlined in the header or simply removed (let compiler generate it for you). ::: dom/bluetooth/BluetoothManager.cpp @@ +5,5 @@ > + * You can obtain one at http://mozilla.org/MPL/2.0/. */ > + > +#include "BluetoothCommon.h" > +#include "BluetoothFirmware.h" > +#include "BluetoothManager.h" Nit: I always recommend putting your own header before all others so that you can be sure your header compiles on its own. @@ +49,5 @@ > + > + NS_WARN_IF_FALSE(NS_SUCCEEDED(rv), "Bluetooth firmware loading failed"); > +} > + > +USING_BLUETOOTH_NAMESPACE Any reason this isn't above this other function? @@ +218,5 @@ > +NS_IMETHODIMP > +BluetoothManager::GetDefaultAdapter(nsIDOMBluetoothAdapter** aAdapter) > +{ > + // TODO: Hook up proper dbus fetching to the adapter object > + aAdapter = NULL; Setting 'aAdapter' does nothing (only modifies your function's local copy). You mean '*aAdapter'. However, there's no reason to set anything if you're returning an error code, xpconnect won't ever look at it. Nit: Everything else here uses 'nsnull'. @@ +223,5 @@ > + return NS_ERROR_FAILURE; > + > + // nsRefPtr<BluetoothAdapter> adapter = new BluetoothAdapter(); > + // adapter.forget(aAdapter); > + // return NS_OK Nit: Let's remove this before checkin. @@ +227,5 @@ > + // return NS_OK > +} > + > +nsresult > +NS_NewBluetoothManager(nsPIDOMWindow* aWindow, nsIDOMBluetoothManager** aBluetoothManager) All this security check code should be factored out into a helper function on nsContentUtils, I think, and then you can avoid duplicating the Telephony code. @@ +299,5 @@ > + } > + } > + > + nsRefPtr<BluetoothManager> bluetoothManager = new BluetoothManager(aWindow); > + NS_ENSURE_TRUE(bluetoothManager, NS_ERROR_UNEXPECTED); Nit: This can't fail. ::: dom/bluetooth/BluetoothManager.h @@ +9,5 @@ > + > +#include "BluetoothCommon.h" > +#include "nsDOMEventTargetHelper.h" > +#include "nsIDOMBluetoothManager.h" > +#include "nsIDOMDOMRequest.h" Nit: This looks unused. @@ +19,5 @@ > +class BluetoothManager : public nsDOMEventTargetHelper > + , public nsIDOMBluetoothManager > +{ > +public: > + NS_DECL_ISUPPORTS NS_DECL_ISUPPORTS_INHERITED @@ +31,5 @@ > + BluetoothManager(nsPIDOMWindow*); > + inline void SetEnabledInternal(bool aEnabled) {mEnabled = aEnabled;} > + > +protected: > + bool mEnabled; Nit: Why is this not private? ::: dom/bluetooth/Makefile.in @@ +24,5 @@ > $(NULL) > > XPIDLSRCS = \ > nsIDOMNavigatorBluetooth.idl \ > + nsIDOMBluetoothManager.idl \ Nit: You added a tab here. ::: dom/bluetooth/nsIDOMBluetoothManager.idl @@ +6,5 @@ > + > +#include "nsIDOMEventTarget.idl" > + > +interface nsIDOMDOMRequest; > +interface nsIDOMEventListener; Nit: This looks unused. @@ +15,5 @@ > +{ > + readonly attribute bool enabled; > + > + nsIDOMDOMRequest setEnabled(in boolean enabled); > + nsIDOMBluetoothAdapter getDefaultAdapter(); Nit: I'd make this a readonly attribute. Nicer from JS.

Nits picked, added new security function.

Comment on attachment 626542 [details] [diff] [review] v5: Adding Bluetooth Manager object Review of attachment 626542 [details] [diff] [review]: ----------------------------------------------------------------- There's a problem with allowing chrome access in your function, plus a couple small things. ::: content/base/public/nsContentUtils.h @@ +2015,5 @@ > + * to access pref > + * > + * @return NS_OK on successful preference lookup, error code otherwise > + */ > + static nsresult IsOnPrefWhitelist(nsPIDOMWindow* aWindow, const char* aPrefURL, bool *aAllowed); Nit: Make sure this stays under 80 chars, or wrap. ::: content/base/src/nsContentUtils.cpp @@ +6668,5 @@ > } > + > +// static > +nsresult > +nsContentUtils::IsOnPrefWhitelist(nsPIDOMWindow* aWindow, const char* aPrefURL, bool* aAllowed) Nit: Check 80 chars here too. @@ +6670,5 @@ > +// static > +nsresult > +nsContentUtils::IsOnPrefWhitelist(nsPIDOMWindow* aWindow, const char* aPrefURL, bool* aAllowed) > +{ > + Nit: remove extra newline @@ +6671,5 @@ > +nsresult > +nsContentUtils::IsOnPrefWhitelist(nsPIDOMWindow* aWindow, const char* aPrefURL, bool* aAllowed) > +{ > + > + *aAllowed = false; Nit: No need to set this until you return a success code. @@ +6690,5 @@ > + do_QueryInterface(innerWindow->GetExtantDocument()); > + NS_ENSURE_TRUE(document, NS_NOINTERFACE); > + > + // Do security checks. We assume that chrome is always allowed and we also > + // allow a single page specified by preferences. Nit: comment isn't true, you're checking against a comma-separated whitelist. You should probably mention that in the header comment too. @@ +6691,5 @@ > + NS_ENSURE_TRUE(document, NS_NOINTERFACE); > + > + // Do security checks. We assume that chrome is always allowed and we also > + // allow a single page specified by preferences. > + if (!nsContentUtils::IsSystemPrincipal(document->NodePrincipal())) { It looks like you're not allowing for chrome here (you never set '*aAllowed = true' if we use the system principal). I'd suggest early-returning if we have the system principal. ::: dom/bluetooth/BluetoothManager.h @@ +34,5 @@ > + bool mEnabled; > + > + NS_DECL_EVENT_HANDLER(enabled) > + > +private: Nit: duplicate private: can be removed. ::: dom/bluetooth/nsIDOMBluetoothAdapter.idl @@ +9,2 @@ > interface nsIDOMDOMRequest; > interface nsIDOMEventListener; Nit: These can go away now. ::: dom/telephony/Telephony.cpp @@ +497,5 @@ > { > NS_ASSERTION(aWindow, "Null pointer!"); > > + bool allowed; > + nsresult rv = nsContentUtils::IsOnPrefWhitelist(aWindow, DOM_TELEPHONY_APP_PHONE_URL_PREF, &allowed); Nit: Wrap at 80 chars here. @@ +500,5 @@ > + bool allowed; > + nsresult rv = nsContentUtils::IsOnPrefWhitelist(aWindow, DOM_TELEPHONY_APP_PHONE_URL_PREF, &allowed); > + NS_ENSURE_SUCCESS(rv, rv); > + > + if(!allowed) { Nit: space between 'if (' @@ +511,5 @@ > NS_ENSURE_TRUE(ril, NS_ERROR_UNEXPECTED); > > + nsPIDOMWindow* innerWindow = aWindow->IsInnerWindow() ? > + aWindow : > + aWindow->GetCurrentInnerWindow(); Nit: Let's move this above the call to IsOnPrefWhitelist, then pass 'innerWindow' to it.

Comment on attachment 626606 [details] [diff] [review] v6: Adding Bluetooth Manager object Review of attachment 626606 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/base/src/nsContentUtils.cpp @@ +6738,5 @@ > + } > + } > + } > + > + return NS_OK; You're not setting *aAllowed here before returning a success code if the pref doesn't exist. How about you change it like so: ... if (allowed) { break; } ... *aAllowed = allowed; return NS_OK; That way you always set correctly and in the same spot.

Fixed allowed issue.

Comment on attachment 626637 [details] [diff] [review] v7: Adding Bluetooth Manager object Review of attachment 626637 [details] [diff] [review]: ----------------------------------------------------------------- r=me with this fixed: ::: dom/bluetooth/BluetoothManager.cpp @@ +233,5 @@ > + nsresult rv = nsContentUtils::IsOnPrefWhitelist(aWindow, DOM_BLUETOOTH_URL_PREF, &allowed); > + NS_ENSURE_SUCCESS(rv, rv); > + > + if (allowed) { > + *aBluetoothManager = NULL; Hm. This is reversed. You want '!allowed'.

Backed out on m-i twice now. First time because it was munged with bug fix for Bug 756389, second time because of bustage against the try server/emulator toolchain.

Fixed mismerge of firmware loading functions.

Please set the target milestone when landing on inbound :-) https://hg.mozilla.org/mozilla-central/rev/e860dbf33c3e