Since we can't use gtk, we'll need to start and maintain our own dbus event loop thread for receiving signals.

By "signals", you mean dbus notifications right? (A la Qt "signals".) This should be pretty easy. Let's chat about it next week. The RIL IO thread is a good model for what I think we need here.

Yup, just need a thread to run the pump. Main question is getting information from the signal callbacks back to the main thread. I honestly haven't researched it much yet, so it may be more trivial than I think (i.e. if it just gets a DBusMessage object). Most of this is implemented in Android in glue/gonk/frameworks/base/core/jni/android_server_BluetoothEventLoop.cpp

You just need to poll a socket, right? Anything else required?

Yup. Though unlike RIL, this isn't just a simple back and forth, we're going to have to distribute signals/information to different objects. Whether that'll actually effect this, I dunno, still haven't gotten to that part of the research portion yet (I've mainly been working in either gtk, where it's handled for me, or in sync raw dbus calls so far).

This now exists on github in https://github.com/qdot/mozilla-central/tree/bluetooth-thread It's a mess, but it works. Will need LOTS of cleaning and libevent-izing before it can land though.

Looking for general feedback on this. Things that I see could be done still, not sure if they're needed: - Fix function names in DBusThread - Cleaning up signal setup/teardown (possibly making a const char*[] of the signals and looping thru) - Make socket containers in DBusThread class a list instead of a raw array (works as is for the moment)

Comment on attachment 613488 [details] [diff] [review] WIP: DBus Thread Feels a little short of ready for review. High level approach looks good. Some things to fix up - naming: make sure to |Follow::TheStupid(Style aGuide)| - comments: missing some juicy ones on interfaces, especially DBusEventHandler. Would be a good place to stick a brief overview of how stuff fits together. - make sure to use the new license header for new files - it's not 100% clear to me what code is coming from assp vs. what code we've written internally. We can't relicense assp code under MPL, so the distinction is important legally in addition to review-wise. - some of the code is a more C-y than we seem to need here, but without knowing what's ours and what's not it's hard for me to judge. - global nit: nothing in this code is gonk-specific, per se. I would recommend renaming to something like "dbuslite", or "dbike". Name not all that important. >+ dbus_bus_add_match(mConnection, >+ "type='signal',interface='org.freedesktop.DBus'", This code desparately wants to be a loop over a static data structure. >+ dbus_bus_remove_match(mConnection, >+ "type='signal',interface='org.bluez.AudioSink'", And here. (Only bringing these up because they really jumped out at me.) Some issues with the rest of the stuff but would rather start with the higher-level things.

Fixes license header

Comment on attachment 613911 [details] [diff] [review] DBus Thread Implementation for B2G Bluetooth >diff --git a/ipc/dbus/DBusThread.cpp b/ipc/dbus/DBusThread.cpp >+#define EVENT_LOOP_EXIT 1 >+#define EVENT_LOOP_ADD 2 >+#define EVENT_LOOP_REMOVE 3 >+ Make this a C++ enum with a descriptive typename, plz. >+static const PollFdComparator gPollFdComparator = PollFdComparator(); >+ No need to store a global here: just use |PollFdComparator()| when you need one. C++ compilers know how to boil that pattern away entirely. >+static RefPtr<DBusThread> sDBusThread; >+ You can use nsAutoPtr<> for this. Since DBusThread references don't escape this file-static scope, refcounting isn't needed. (Change propagates to DBusThread decl too.) >+void* >+DBusThread::EventLoop(void *aPtr) >+{ >+ while (1) { >+ poll(dbt->mPollData.Elements(), dbt->mPollData.Length(), -1); OCD nit: Put this at beginning of loop, please. >+bool >+DBusThread::IsEventLoopRunning() >+{ >+ MutexAutoLock lock(mMutex); >+ bool result = false; >+ if (mIsRunning) { >+ result = true; >+ } >+ >+ return result; Erm how about |return mIsRunning;| ? >diff --git a/ipc/dbus/DBusThread.h b/ipc/dbus/DBusThread.h >+bool StartDBus(); >+bool StopDBus(); Need some brief comments here: on which threads can Start/Stop() be called? Is it safe to call them at any time and in any order? Etc. Nit: newline here. >+} >+} >+#endif >diff --git a/ipc/dbus/RawDBusConnection.h b/ipc/dbus/RawDBusConnection.h >+// LOGE and free a D-Bus error >+// Using #define so that __FUNCTION__ resolves usefully >+#define LOG_AND_FREE_DBUS_ERROR_WITH_MSG(err, msg) \ >+ { LOG("%s: D-Bus error in %s: %s (%s)", __FUNCTION__, \ >+ dbus_message_get_member((msg)), (err)->name, (err)->message); \ >+ dbus_error_free((err)); } >+#define LOG_AND_FREE_DBUS_ERROR(err) \ >+ { LOG("%s: D-Bus error: %s (%s)", __FUNCTION__, \ >+ (err)->name, (err)->message); \ >+ dbus_error_free((err)); } >+ Let's make a function helper here that does the heavy lifting, to which these macros route with a resolved __FUNCTION__. That tends to be very helpful when trying to break on "any dbus error": break in your error helper. >+class RawDBusConnection >+{ >+ bool createDBusConnection(); Nit: |Create...()|. >+protected: >+ DBusConnection* mConnection; Bare pointers make me nervous ... I assume this is a magic handle that dbus gives us, that we're not really supposed to manage as a pointer? (Kind of like xlib magical objects.) If so, please doc as such. >diff --git a/toolkit/library/Makefile.in b/toolkit/library/Makefile.in >@@ -398,6 +402,7 @@ OS_LIBS += \ > -lcamera_client \ > -lbinder \ > -lsensorservice \ >+ -ldbus \ Nit: \t here, use \x20 to match code above. I like this a lot, overall. Would like to see the next version.

I broke the error macros out into DBusUtils.h, as that's where they'd be going anyways, and we're planning to put a lot more functions in there soon. Apache licensed that file, since we'll be lifting a ton more stuff out of android for it most likely. Other than that, patch is updated with prior review comments implemented.

Comment on attachment 614688 [details] [diff] [review] DBus Thread Implementation for B2G Bluetooth Review of attachment 614688 [details] [diff] [review]: ----------------------------------------------------------------- Forgot to fix DBusConnection* ptr. Cancelling review.

https://hg.mozilla.org/mozilla-central/rev/db5d4c1aece7 https://hg.mozilla.org/mozilla-central/rev/4469d30b6943

I just want to point out that function: dbus_threads_init_default(), it's called twice, once in: RawDbusConnection::Create(), and again in: DBusThread::SetUpEventLoop(), just after returning from Create(). As D-Bus documentation [1] says, all other calls to this function after the first one, actually does nothing, so it's not a real bug. [1] http://dbus.freedesktop.org/doc/api/html/group__DBusThreads.html#ga33b6cf3b8f1e41bad5508f84758818a7

This was already reviewed once, but: - Unsubmittable due to collision with gb emulator - Had major bug where mControlFdW wasn't actually initialized - Bitrotted in a very short period due to .mFd->.get() change in ScopedClose So, getting a quick re-review just in case. Can't land until gonk-gb on B2G server is updated, too.

Adding dependency to toolchain update bug, since I rolled dbus inclusion into the cpufeatures update.

Removed some unneeded ifdefs. Last update before this was fixing a botched merge that brought back some old jstypedarray stuff.

Comment on attachment 618549 [details] [diff] [review] DBus Thread Implementation for B2G Bluetooth Skimmed but looks OK to me.

Backed out due to bustage: https://hg.mozilla.org/integration/mozilla-inbound/rev/2a0b7a536677 https://tbpl.mozilla.org/php/getParsedLog.php?id=11541953&tree=Mozilla-Inbound configure: loading cache /builds/slave/m-in-b2g/build/obj-b2g/tools/profiler/libunwind/src/config.cache configure: warning: ignoring whitespace changes in `LIBS' since the previous run: configure: former value: ` -lstlport ' configure: current value: ` -lstlport' configure: error: `CPPFLAGS' has changed since the previous run: configure: former value: `-DANDROID -isystem /builds/slave/m-in-b2g/build/gonk-toolchain/bionic/libc/include/ -isystem /builds/slave/m-in-b2g/build/gonk-toolchain/bionic/libc/kernel/common -isystem /builds/slave/m-in-b2g/build/gonk-toolchain/bionic/libc/arch-arm/include -isystem /builds/slave/m-in-b2g/build/gonk-toolchain/bionic/libc/kernel/arch-arm -isystem /builds/slave/m-in-b2g/build/gonk-toolchain/bionic/libm/include -I/builds/slave/m-in-b2g/build/gonk-toolchain/frameworks/base/opengl/include -I/builds/slave/m-in-b2g/build/gonk-toolchain/frameworks/base/native/include -I/builds/slave/m-in-b2g/build/gonk-toolchain/hardware/libhardware/include -I/builds/slave/m-in-b2g/build/gonk-toolchain/hardware/libhardware_legacy/include -I/builds/slave/m-in-b2g/build/gonk-toolchain/system/core/include -isystem /builds/slave/m-in-b2g/build/gonk-toolchain/bionic -I/builds/slave/m-in-b2g/build/gonk-toolchain/frameworks/base/include -I/builds/slave/m-in-b2g/build/gonk-toolchain/frameworks/base/services/sensorservice' configure: current value: `-DANDROID -isystem /builds/slave/m-in-b2g/build/gonk-toolchain/bionic/libc/include/ -isystem /builds/slave/m-in-b2g/build/gonk-toolchain/bionic/libc/kernel/common -isystem /builds/slave/m-in-b2g/build/gonk-toolchain/bionic/libc/arch-arm/include -isystem /builds/slave/m-in-b2g/build/gonk-toolchain/bionic/libc/kernel/arch-arm -isystem /builds/slave/m-in-b2g/build/gonk-toolchain/bionic/libm/include -I/builds/slave/m-in-b2g/build/gonk-toolchain/frameworks/base/opengl/include -I/builds/slave/m-in-b2g/build/gonk-toolchain/frameworks/base/native/include -I/builds/slave/m-in-b2g/build/gonk-toolchain/hardware/libhardware/include -I/builds/slave/m-in-b2g/build/gonk-toolchain/hardware/libhardware_legacy/include -I/builds/slave/m-in-b2g/build/gonk-toolchain/system/core/include -isystem /builds/slave/m-in-b2g/build/gonk-toolchain/bionic -I/builds/slave/m-in-b2g/build/gonk-toolchain/frameworks/base/include -I/builds/slave/m-in-b2g/build/gonk-toolchain/external/dbus -I/builds/slave/m-in-b2g/build/gonk-toolchain/frameworks/base/services/sensorservice' configure: error: in `/builds/slave/m-in-b2g/build/obj-b2g/tools/profiler/libunwind/src': configure: error: changes in the environment can compromise the build configure: error: run `make distclean' and/or `rm /builds/slave/m-in-b2g/build/obj-b2g/tools/profiler/libunwind/src/config.cache' and start over configure: error: /builds/slave/m-in-b2g/build/tools/profiler/libunwind/src/configure failed for tools/profiler/libunwind/src And re-landed after clobbering all the b2g build slaves. Sorry for the churn. https://hg.mozilla.org/integration/mozilla-inbound/rev/6f6ba1e4eae2