bluedroid library, used for bluetooth firmware loading, doesn't exist on qemu. We need to expose 3 functions out of the library, so we should just dlopen it and load as needed.

Comment on attachment 603080 [details] [diff] [review] Bug 729470 - Make bluedroid calls load via dlopen Review of attachment 603080 [details] [diff] [review]: ----------------------------------------------------------------- Removing review request, there's some extra build stuff I forgot to do. :/

Comment on attachment 603080 [details] [diff] [review] Bug 729470 - Make bluedroid calls load via dlopen Actually, gonna line up build stuff in a different bug for backoutability. Game, er, review on!

Comment on attachment 603080 [details] [diff] [review] Bug 729470 - Make bluedroid calls load via dlopen >diff --git a/dom/bluetooth/BluetoothAdapter.cpp b/dom/bluetooth/BluetoothAdapter.cpp >+BEGIN_BLUETOOTH_NAMESPACE >+ This macro means namespace fail. Big r---- from me. But please sort this out with DOM peeps. >+static struct BluedroidFunctions { >+ bool initialized; >+ >+ int (* bt_enable)(); >+ int (* bt_disable)(); >+ int (* bt_is_enabled)(); >+} gBluedroidFunctions; Nit: sBluedroidFunctions. This is file-static. >+ >+bool init_bluedroid() { Nit: static bool EnsureBluedroidInit() { >+ if (gBluedroidFunctions.initialized) >+ return true; >+ >+ void* handle = dlopen("libbluedroid.so", RTLD_LAZY); > >+ if(!handle) { This approach is going to attempt to open the bluetooth lib for each invocation, even if a previous attempt failed. That's not great but I think I'm OK with that, since we should only run into this case in the emulator. If we port to non-bluetooth-enabled devices, we'll need a different approach, because dlopen() may need to do significant disk IO. Adding an |attemptedInit| flag to BluedroidFunctions, and doing if (sBluedroidFunctions.attemptedInit) return sBluedroidFunctions.initialized; would be a good long-term solution to this problem. >+ gBluedroidFunctions.bt_enable = (int (*)())dlsym(handle, "bt_enable"); >+ if(gBluedroidFunctions.bt_enable == NULL) { >+ NS_ERROR("Failed to attach bt_enable function"); >+ return false; One more of these and I would request a helper function, but 3 is OK. > class ToggleBtResultTask : public nsRunnable > { >@@ -47,14 +82,14 @@ class ToggleBtResultTask : public nsRunnable > } > > private: >- nsRefPtr<BluetoothAdapter> mAdapterPtr; > bool mResult; >+ nsRefPtr<BluetoothAdapter> mAdapterPtr; Reverse this reordering for better struct alignment. (Doesn't matter so much here, but it's a good habit to be in.) >@@ -93,7 +130,6 @@ class ToggleBtTask : public nsRunnable > bool mOnOff; > }; > >-DOMCI_DATA(BluetoothAdapter, BluetoothAdapter) > > NS_IMPL_CYCLE_COLLECTION_CLASS(BluetoothAdapter) > >@@ -123,18 +159,33 @@ BluetoothAdapter::BluetoothAdapter() > NS_IMETHODIMP > BluetoothAdapter::GetPower(bool* aPower) > { >+#ifdef MOZ_WIDGET_GONK I don't understand why everything above is platform-neutral but this is Gonk-specific. Platform-specific code should go in a platform-specific file, generally. Same question for other methods here.

(In reply to Chris Jones [:cjones] [:warhammer] from comment #5) > Comment on attachment 603080 [details] [diff] [review] > Bug 729470 - Make bluedroid calls load via dlopen > > >diff --git a/dom/bluetooth/BluetoothAdapter.cpp b/dom/bluetooth/BluetoothAdapter.cpp > > >+BEGIN_BLUETOOTH_NAMESPACE > >+ > > This macro means namespace fail. Big r---- from me. But please sort > this out with DOM peeps. Yeah, this was added due to my confusion about how we're handling DOMCI_DATA(BluetoothAdapter, BluetoothAdapter), since that fails to compile for me. The plan is to move to the MozBluetoothAdapter name in 731021, which I should possibly finish out before this to take care of this. I'll check with bent on this and see what I'm doing wrong too, I may just be misunderstanding something. > >@@ -93,7 +130,6 @@ class ToggleBtTask : public nsRunnable > > bool mOnOff; > > }; > > > >-DOMCI_DATA(BluetoothAdapter, BluetoothAdapter) > > > > NS_IMPL_CYCLE_COLLECTION_CLASS(BluetoothAdapter) > > > >@@ -123,18 +159,33 @@ BluetoothAdapter::BluetoothAdapter() > > NS_IMETHODIMP > > BluetoothAdapter::GetPower(bool* aPower) > > { > >+#ifdef MOZ_WIDGET_GONK > > I don't understand why everything above is platform-neutral but this > is Gonk-specific. Platform-specific code should go in a > platform-specific file, generally. > > Same question for other methods here. I wrapped anything that would call the dlopen stuff in the GONK checks, because we don't care about loading bluetooth firmware on linux, but the code will otherwise still work on there. I was planning on breaking things out into a gonk/linux/gtk set anyways in 730963, I guess I can just start doing that here now.

(In reply to Kyle Machulis [:kmachulis] [:qdot] from comment #6) > (In reply to Chris Jones [:cjones] [:warhammer] from comment #5) > > Comment on attachment 603080 [details] [diff] [review] > > Bug 729470 - Make bluedroid calls load via dlopen > > > I wrapped anything that would call the dlopen stuff in the GONK checks, > because we don't care about loading bluetooth firmware on linux, but the > code will otherwise still work on there. I was planning on breaking things > out into a gonk/linux/gtk set anyways in 730963, I guess I can just start > doing that here now. Sounds good. Don't worry about accelerating that schedule.

Made fixes recommended above, added comments to explain platform def's for the moment until we divide out into platform files.

Adding final version of patch, same as last patch but removes bluedroid linking option that's no longer needed.

Comment on attachment 604220 [details] [diff] [review] Bug 729470 - Make bluedroid calls load via dlopen Review of attachment 604220 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/bluetooth/BluetoothAdapter.cpp @@ +37,5 @@ > + > + sBluedroidFunctions.initialized = false; > + sBluedroidFunctions.tried_initialization = true; > + > + void* handle = dlopen("libbluedroid.so", RTLD_LAZY); Shouldn't this have been abstracted down in Hal so that other platforms could connect up these dom events? (Hal seems to be the place we do this type of work now.)

I'm not aware of any other platforms that require loading radio firmware programmatically, not to mention most of the bluetooth functionality at the moment doesn't necessarily need to be exposed to other platforms (we're basically reimplementing what built in OS bluetooth config/dialogs already do for you for B2G). That said, this could probably be moved down to the HAL at some point, yes.