[b2g-bluetooth] Electrolysize b2g-bluetooth

RESOLVED FIXED

Status

()

Core
DOM: Device Interfaces
P1
normal
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: qdot, Assigned: Ben Turner (not reading bugmail, use the needinfo flag!))

Tracking

({feature})

Trunk
x86_64
Gonk (Firefox OS)
feature
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking-basecamp:+)

Details

(Whiteboard: [LOE:M] [WebAPI:P2])

Attachments

(1 attachment, 4 obsolete attachments)

Bluetooth is currently not e10s friendly. Fix it.

Updated

6 years ago
blocking-basecamp: --- → +

Comment 1

6 years ago
How are we doing with this?

Updated

6 years ago
Priority: -- → P1
No movement, nor will there be until after sockets are done unless we get more resources.
bent is on this now.
Assignee: kyle → bent.mozilla
Created attachment 657992 [details] [diff] [review]
WIP

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.
Created attachment 657995 [details] [diff] [review]
WIP

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]
Created attachment 659139 [details] [diff] [review]
Patch, v1

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+
Created attachment 659808 [details] [diff] [review]
Patch, v1.1

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!
Created attachment 660871 [details] [diff] [review]
Final patch

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 https://hg.mozilla.org/integration/mozilla-inbound/rev/d8e0ceb1fb06 since it didn't quite actually build.
Fixed up: https://hg.mozilla.org/mozilla-central/rev/8604438c67ad
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.