Closed Bug 755943 Opened 10 years ago Closed 10 years ago

[b2g-bluetooth] Electrolysize b2g-bluetooth


(Core :: DOM: Device Interfaces, defect, P1)

Gonk (Firefox OS)



blocking-basecamp +


(Reporter: qdot, Assigned: bent.mozilla)



(Keywords: feature, Whiteboard: [LOE:M] [WebAPI:P2])


(1 file, 4 obsolete files)

Bluetooth is currently not e10s friendly. Fix it.
Assignee: nobody → kyle
blocking-basecamp: --- → +
How are we doing with this?
Priority: -- → P1
No movement, nor will there be until after sockets are done unless we get more resources.
Whiteboard: [LOE:M]
bent is on this now.
Assignee: kyle → bent.mozilla
Attached patch WIP (obsolete) — Splinter Review
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.
Attached patch WIP (obsolete) — Splinter Review
Removed some extra files that aren't supposed to be here.
Attachment #657992 - Attachment is obsolete: true
Duplicate of this bug: 776667
Whiteboard: [LOE:M] → [LOE:M] [WebAPI:P2]
Attached patch Patch, v1 (obsolete) — Splinter Review
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.
Keywords: feature
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+
Attached patch Patch, v1.1 (obsolete) — Splinter Review
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"

Please pseuo-indent the #includes one character for readability

# include "BluetoothServiceGonk.h"

>+nsRefPtr<BluetoothService> gBluetoothService;

You want a StaticRefPtr here.

>+inline bool
>+  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
>+#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

>+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


Style would be |inline void|, but please drop the |inline| since
compilers will just laugh at you and gleefully ignore it.

>+BluetoothParent::ActorDestroy(ActorDestroyReason aWhy)

>+#ifdef DEBUG
>+    mService = nullptr;


>diff --git a/dom/bluetooth/ipc/BluetoothServiceChildProcess.cpp b/dom/bluetooth/ipc/BluetoothServiceChildProcess.cpp

>+SendRequest(BluetoothReplyRunnable* aRunnable, const Request& aRequest)

Same nit about |inline|.

>diff --git a/dom/bluetooth/ipc/ b/dom/bluetooth/ipc/

Ted, I, and our build times would be happier if you just VPATH this
into the main bluetooth Makefile instead of building the dirs

>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.

>+    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!
Attached patch Final patchSplinter Review
Ok, comments addressed, nits picked, merged with recent changes, ready to land.
Attachment #659808 - Attachment is obsolete: true
Attachment #660871 - Flags: review+
Backed out in since it didn't quite actually build.
Fixed up:
Closed: 10 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.