Closed
Bug 755943
Opened 13 years ago
Closed 12 years ago
[b2g-bluetooth] Electrolysize b2g-bluetooth
Categories
(Core :: DOM: Device Interfaces, defect, P1)
Tracking
()
RESOLVED
FIXED
blocking-basecamp | + |
People
(Reporter: qdot, Assigned: bent.mozilla)
References
Details
(Keywords: feature, Whiteboard: [LOE:M] [WebAPI:P2])
Attachments
(1 file, 4 obsolete files)
103.45 KB,
patch
|
bent.mozilla
:
review+
|
Details | Diff | Splinter Review |
Bluetooth is currently not e10s friendly. Fix it.
Reporter | ||
Updated•13 years ago
|
Assignee: nobody → kyle
Updated•12 years ago
|
blocking-basecamp: --- → +
Comment 1•12 years ago
|
||
How are we doing with this?
Updated•12 years ago
|
Priority: -- → P1
Reporter | ||
Comment 2•12 years ago
|
||
No movement, nor will there be until after sockets are done unless we get more resources.
Reporter | ||
Updated•12 years ago
|
Whiteboard: [LOE:M]
Assignee | ||
Comment 4•12 years ago
|
||
Attached is a WIP patch that gets us basic enable/disable/discoverable/discovering support. If I tweak gaia a little then the settings app works just fine out of process (apparently pairing has not yet been implemented in gaia?). Gecko has evolved a bit since I started so it won't apply cleanly to m-c (for reference I'm using rev d1a5d4cda6ac). When I tried building against tip gaia was totally broken due to some recent change (bug 777665 I think). Tomorrow I'll try to sort it all out and get this applying to tip.
Assignee | ||
Comment 5•12 years ago
|
||
Removed some extra files that aren't supposed to be here.
Attachment #657992 -
Attachment is obsolete: true
Updated•12 years ago
|
Whiteboard: [LOE:M] → [LOE:M] [WebAPI:P2]
Assignee | ||
Comment 7•12 years ago
|
||
Ok, this is merged to m-c as of today and seems to work for everything that the Settings app currently does. There is more work to do here surrounding pairing but I think qDot can run with that once this lands and we get Settings OOP. r? to cjones for the IPDL bits, r? to qDot for the rest.
Attachment #657995 -
Attachment is obsolete: true
Attachment #659139 -
Flags: review?(kyle)
Attachment #659139 -
Flags: review?(jones.chris.g)
This patch is only 99KB, you're slipping.
Reporter | ||
Comment 9•12 years ago
|
||
Comment on attachment 659139 [details] [diff] [review] Patch, v1 Review of attachment 659139 [details] [diff] [review]: ----------------------------------------------------------------- And I have to maintain this? :( ::: dom/bluetooth/BluetoothAdapter.cpp @@ +152,5 @@ > // We can be null on shutdown, where this might happen > if (bs) { > + // XXXbent I don't see anything about LOCAL_AGENT_PATH or REMOTE_AGENT_PATH > + // here. Probably a bug? Maybe use UnregisterAll. > + bs->UnregisterBluetoothSignalHandler(mPath, this); Note: This will be getting fixed in bug 789014, since we're moving agent stuff back to the service. ::: dom/bluetooth/ipc/BluetoothServiceChildProcess.cpp @@ +170,5 @@ > + return NS_OK; > +} > + > +int > +BluetoothServiceChildProcess::GetDeviceServiceChannelInternal( This got taken out in 756299 @@ +182,5 @@ > + > +nsTArray<uint32_t> > +BluetoothServiceChildProcess::AddReservedServicesInternal( > + const nsAString& aAdapterPath, > + const nsTArray<uint32_t>& aServices) Nit: space at end of line, and signature changed in bug 788254
Attachment #659139 -
Flags: review?(kyle) → review+
Assignee | ||
Comment 10•12 years ago
|
||
Fixed qDot's comments. cjones, the sooner you review this the sooner it'll stop getting bitrotted...
Attachment #659139 -
Attachment is obsolete: true
Attachment #659139 -
Flags: review?(jones.chris.g)
Attachment #659808 -
Flags: review?(jones.chris.g)
(In reply to Kyle Machulis [:kmachulis] [:qdot] from comment #9) > Comment on attachment 659139 [details] [diff] [review] > Patch, v1 > > Review of attachment 659139 [details] [diff] [review]: > ----------------------------------------------------------------- > > And I have to maintain this? :( > Ben wrote all this code for you, and this is your gratitude? Wow. Anyway, thanks Ben!
Comment on attachment 659808 [details] [diff] [review] Patch, v1.1 >diff --git a/dom/bluetooth/BluetoothService.cpp b/dom/bluetooth/BluetoothService.cpp >+#if defined(MOZ_BLUETOOTH_GONK) >+#include "BluetoothServiceGonk.h" >+#elif defined(MOZ_BLUETOOTH_DBUS) >+#include "BluetoothDBusService.h" >+#endif > Please pseuo-indent the #includes one character for readability #if defined(MOZ_BLUETOOTH_GONK) # include "BluetoothServiceGonk.h" >+nsRefPtr<BluetoothService> gBluetoothService; You want a StaticRefPtr here. >+inline bool >+IsMainProcess() >+{ >+ return XRE_GetProcessType() == GeckoProcessType_Default; >+} Please drop the |inline| since compilers will just laugh at you and gleefully ignore it. The only reason to use |inline| is to suppress warnings about unused static functions defined in headers. >+// static >+BluetoothService* >+BluetoothService::Create() >+{ >+#if defined(MOZ_BLUETOOTH_GONK) >+ if (!IsMainProcess()) { >+ return BluetoothServiceChildProcess::Create(); >+ } >+ return new BluetoothGonkService(); >+#elif defined(MOZ_BLUETOOTH_DBUS) >+ if (!IsMainProcess()) { >+ return BluetoothServiceChildProcess::Create(); >+ } I don't like the duplicated code here, and it gives the illusion that we'd have platform-specific subprocess services, which is an explicit non-goal. Is there some kind of MOZ_HAVE_BLUETOOTH macro we can use, and then factor out the child-process service creation under that check? >+void >+BluetoothService::RegisterBluetoothActor(BluetoothParent* aActor) >+{ >+ MOZ_ASSERT(NS_IsMainThread()); >+ MOZ_ASSERT(aActor); >+ MOZ_ASSERT(!mBluetoothActors.Contains(aActor)); >+ >+ mBluetoothActors.AppendElement(aActor); >+ Why are we duplicating the actor list that IPDL already provides? I confess to not understanding this part of the code. >diff --git a/dom/bluetooth/ipc/BluetoothParent.cpp b/dom/bluetooth/ipc/BluetoothParent.cpp >+inline >+void >+BluetoothParent::UnregisterAllSignalHandlers() Style would be |inline void|, but please drop the |inline| since compilers will just laugh at you and gleefully ignore it. >+void >+BluetoothParent::ActorDestroy(ActorDestroyReason aWhy) >+#ifdef DEBUG >+ mService = nullptr; >+#endif Aroo? >diff --git a/dom/bluetooth/ipc/BluetoothServiceChildProcess.cpp b/dom/bluetooth/ipc/BluetoothServiceChildProcess.cpp >+inline >+void >+SendRequest(BluetoothReplyRunnable* aRunnable, const Request& aRequest) Same nit about |inline|. >diff --git a/dom/bluetooth/ipc/Makefile.in b/dom/bluetooth/ipc/Makefile.in Ted, I, and our build times would be happier if you just VPATH this into the main bluetooth Makefile instead of building the dirs separately. >diff --git a/dom/bluetooth/ipc/PBluetooth.ipdl b/dom/bluetooth/ipc/PBluetooth.ipdl >+protocol PBluetooth >+{ Can you add the state machine we worked out, that the type checker isn't powerful enough to accept yet? Please leave it commented out with a FIXME/bug 547703 header. >diff --git a/dom/ipc/ContentChild.cpp b/dom/ipc/ContentChild.cpp >+using mozilla::dom::bluetooth::PBluetoothChild; |using mozilla::dom::bluetooth;| and please insert it in alphabetical order in the list above. >+PBluetoothChild* >+ContentChild::AllocPBluetooth() >+{ >+ NS_NOTREACHED("No one should be allocating PBluetoothChild actors"); Use MOZ_NOT_REACHED(), which actually has teeth. Do you have followup bugs filed for the XXXbent comments? I wouldn't say that I did a particularly in-depth review because I don't understand most of this code. But your IPC stuff looks good. If there's something in particular you/kyle want me to look particularly closely at, please let me know. Overall looks very good --- would like to take a quick peek at v2 with build-system fixes.
Attachment #659808 -
Flags: review?(jones.chris.g)
Comment on attachment 659808 [details] [diff] [review] Patch, v1.1 bent and I followed up on IRC. We addressed the comments and discussed what's needed for r+. r=me with those
Attachment #659808 -
Flags: review+
bent: r=me on IRC interdiff. Looks good!
Assignee | ||
Comment 15•12 years ago
|
||
Ok, comments addressed, nits picked, merged with recent changes, ready to land.
Attachment #659808 -
Attachment is obsolete: true
Attachment #660871 -
Flags: review+
Assignee | ||
Comment 16•12 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/aee383cd56dc
Comment 17•12 years ago
|
||
Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/d8e0ceb1fb06 since it didn't quite actually build.
Assignee | ||
Comment 18•12 years ago
|
||
Fixed up: https://hg.mozilla.org/mozilla-central/rev/8604438c67ad
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•