Create BluetoothManager object for managing multiple adapters and firmware loading

RESOLVED FIXED in mozilla15

Status

()

Core
DOM: Device Interfaces
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: qdot, Assigned: qdot)

Tracking

Trunk
mozilla15
All
Gonk (Firefox OS)
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 8 obsolete attachments)

28.12 KB, patch
Details | Diff | Splinter Review
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.
Assignee: nobody → kyle
Blocks: 727618
Summary: Create BluetoothManager object for managing dbus thread and multiple adapters → Create BluetoothManager object for managing multiple adapters and firmware loading
Created attachment 623907 [details] [diff] [review]
v1: Adding Bluetooth Manager object
Attachment #623907 - Flags: superreview?(bent.mozilla)
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.
Attachment #623907 - Flags: superreview?(bent.mozilla)
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.
Attachment #623907 - Attachment is obsolete: true
Attachment #623911 - Flags: superreview?(bent.mozilla)
Created attachment 623921 [details] [diff] [review]
v3: Adding Bluetooth Manager object

Forgot to rename setBluetoothEnabled to setEnabled
Attachment #623911 - Attachment is obsolete: true
Attachment #623911 - Flags: superreview?(bent.mozilla)
Attachment #623921 - Flags: superreview?(bent.mozilla)
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. :|
Attachment #623921 - Flags: superreview?(bent.mozilla)
Created attachment 624155 [details] [diff] [review]
v4: Adding Bluetooth Manager object
Attachment #623921 - Attachment is obsolete: true
Attachment #624155 - Flags: superreview?(bent.mozilla)
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.
Attachment #624155 - Flags: superreview?(bent.mozilla) → review?(bent.mozilla)
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.
Created attachment 626542 [details] [diff] [review]
v5: Adding Bluetooth Manager object

Nits picked, added new security function.
Attachment #624155 - Attachment is obsolete: true
Attachment #624155 - Flags: review?(bent.mozilla)
Attachment #626542 - Flags: review?(bent.mozilla)
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.
Created attachment 626606 [details] [diff] [review]
v6: Adding Bluetooth Manager object
Attachment #626542 - Attachment is obsolete: true
Attachment #626542 - Flags: review?(bent.mozilla)
Attachment #626606 - Flags: review?(bent.mozilla)
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.
Created attachment 626637 [details] [diff] [review]
v7: Adding Bluetooth Manager object

Fixed allowed issue.
Attachment #626606 - Attachment is obsolete: true
Attachment #626606 - Flags: review?(bent.mozilla)
Attachment #626637 - Flags: review?(bent.mozilla)
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'.
Attachment #626637 - Flags: review?(bent.mozilla) → review+
Created attachment 626658 [details] [diff] [review]
v8: Adding Bluetooth Manager object (final)
Attachment #626637 - Attachment is obsolete: true
http://hg.mozilla.org/integration/mozilla-inbound/rev/00238745331b
Version: unspecified → Trunk
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.
Created attachment 626675 [details] [diff] [review]
v9: Adding Bluetooth Manager object

Fixed mismerge of firmware loading functions.
Attachment #626658 - Attachment is obsolete: true
https://hg.mozilla.org/integration/mozilla-inbound/rev/e860dbf33c3e
Please set the target milestone when landing on inbound :-)

https://hg.mozilla.org/mozilla-central/rev/e860dbf33c3e
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla15
You need to log in before you can comment on or make changes to this bug.