Last Comment Bug 742044 - Create BluetoothManager object for managing multiple adapters and firmware loading
: Create BluetoothManager object for managing multiple adapters and firmware lo...
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM: Device Interfaces (show other bugs)
: Trunk
: All Gonk (Firefox OS)
: -- normal (vote)
: mozilla15
Assigned To: Kyle Machulis [:kmachulis] [:qdot]
:
Mentors:
Depends on:
Blocks: b2g-bluetooth
  Show dependency treegraph
 
Reported: 2012-04-03 13:45 PDT by Kyle Machulis [:kmachulis] [:qdot]
Modified: 2012-05-24 09:21 PDT (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
v1: Adding Bluetooth Manager object (23.41 KB, patch)
2012-05-14 19:21 PDT, Kyle Machulis [:kmachulis] [:qdot]
no flags Details | Diff | Review
v2: Adding Bluetooth Manager object (23.27 KB, patch)
2012-05-14 19:29 PDT, Kyle Machulis [:kmachulis] [:qdot]
no flags Details | Diff | Review
v3: Adding Bluetooth Manager object (23.26 KB, patch)
2012-05-14 20:09 PDT, Kyle Machulis [:kmachulis] [:qdot]
no flags Details | Diff | Review
v4: Adding Bluetooth Manager object (25.01 KB, patch)
2012-05-15 12:56 PDT, Kyle Machulis [:kmachulis] [:qdot]
no flags Details | Diff | Review
v5: Adding Bluetooth Manager object (28.31 KB, patch)
2012-05-23 12:09 PDT, Kyle Machulis [:kmachulis] [:qdot]
no flags Details | Diff | Review
v6: Adding Bluetooth Manager object (28.18 KB, patch)
2012-05-23 15:38 PDT, Kyle Machulis [:kmachulis] [:qdot]
no flags Details | Diff | Review
v7: Adding Bluetooth Manager object (28.16 KB, patch)
2012-05-23 17:09 PDT, Kyle Machulis [:kmachulis] [:qdot]
bent.mozilla: review+
Details | Diff | Review
v8: Adding Bluetooth Manager object (final) (28.17 KB, patch)
2012-05-23 18:04 PDT, Kyle Machulis [:kmachulis] [:qdot]
no flags Details | Diff | Review
v9: Adding Bluetooth Manager object (28.12 KB, patch)
2012-05-23 19:28 PDT, Kyle Machulis [:kmachulis] [:qdot]
no flags Details | Diff | Review

Description Kyle Machulis [:kmachulis] [:qdot] 2012-04-03 13:45:52 PDT
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 1 Kyle Machulis [:kmachulis] [:qdot] 2012-05-14 19:21:18 PDT
Created attachment 623907 [details] [diff] [review]
v1: Adding Bluetooth Manager object
Comment 2 Kyle Machulis [:kmachulis] [:qdot] 2012-05-14 19:22:42 PDT
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.
Comment 3 Kyle Machulis [:kmachulis] [:qdot] 2012-05-14 19:29:14 PDT
Created attachment 623911 [details] [diff] [review]
v2: Adding Bluetooth Manager object

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.
Comment 4 Kyle Machulis [:kmachulis] [:qdot] 2012-05-14 20:09:40 PDT
Created attachment 623921 [details] [diff] [review]
v3: Adding Bluetooth Manager object

Forgot to rename setBluetoothEnabled to setEnabled
Comment 5 Kyle Machulis [:kmachulis] [:qdot] 2012-05-14 20:12:36 PDT
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 6 Kyle Machulis [:kmachulis] [:qdot] 2012-05-15 12:56:22 PDT
Created attachment 624155 [details] [diff] [review]
v4: Adding Bluetooth Manager object
Comment 7 Ben Turner (not reading bugmail, use the needinfo flag!) 2012-05-15 13:16:53 PDT
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.
Comment 8 Kyle Machulis [:kmachulis] [:qdot] 2012-05-15 14:10:49 PDT
Ah, ok. Misunderstood, thanks for the update.
Comment 9 Ben Turner (not reading bugmail, use the needinfo flag!) 2012-05-23 00:00:34 PDT
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.
Comment 10 Kyle Machulis [:kmachulis] [:qdot] 2012-05-23 12:09:36 PDT
Created attachment 626542 [details] [diff] [review]
v5: Adding Bluetooth Manager object

Nits picked, added new security function.
Comment 11 Ben Turner (not reading bugmail, use the needinfo flag!) 2012-05-23 14:54:03 PDT
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 12 Kyle Machulis [:kmachulis] [:qdot] 2012-05-23 15:38:31 PDT
Created attachment 626606 [details] [diff] [review]
v6: Adding Bluetooth Manager object
Comment 13 Ben Turner (not reading bugmail, use the needinfo flag!) 2012-05-23 16:25:30 PDT
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.
Comment 14 Kyle Machulis [:kmachulis] [:qdot] 2012-05-23 17:09:39 PDT
Created attachment 626637 [details] [diff] [review]
v7: Adding Bluetooth Manager object

Fixed allowed issue.
Comment 15 Ben Turner (not reading bugmail, use the needinfo flag!) 2012-05-23 17:21:56 PDT
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'.
Comment 16 Kyle Machulis [:kmachulis] [:qdot] 2012-05-23 18:04:52 PDT
Created attachment 626658 [details] [diff] [review]
v8: Adding Bluetooth Manager object (final)
Comment 17 Kyle Machulis [:kmachulis] [:qdot] 2012-05-23 18:06:56 PDT
http://hg.mozilla.org/integration/mozilla-inbound/rev/00238745331b
Comment 18 Kyle Machulis [:kmachulis] [:qdot] 2012-05-23 18:30:11 PDT
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.
Comment 19 Kyle Machulis [:kmachulis] [:qdot] 2012-05-23 19:28:01 PDT
Created attachment 626675 [details] [diff] [review]
v9: Adding Bluetooth Manager object

Fixed mismerge of firmware loading functions.
Comment 20 Kyle Machulis [:kmachulis] [:qdot] 2012-05-23 21:03:12 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/e860dbf33c3e
Comment 21 Ed Morley [:emorley] 2012-05-24 09:21:07 PDT
Please set the target milestone when landing on inbound :-)

https://hg.mozilla.org/mozilla-central/rev/e860dbf33c3e

Note You need to log in before you can comment on or make changes to this bug.