755943 - [b2g-bluetooth] Electrolysize b2g-bluetooth

Status: RESOLVED FIXED

Description

[Kyle Machulis [:qdot] [:kmachulis] (if a patch has no decent commit message, automatic r-)]

Bluetooth is currently not e10s friendly. Fix it.

Comment 1

[Andreas Gal :gal]

How are we doing with this?

Comment 2

[Kyle Machulis [:qdot] [:kmachulis] (if a patch has no decent commit message, automatic r-)]

No movement, nor will there be until after sockets are done unless we get more resources.

Comment 3

[Johnny Stenback (:jst)]

bent is on this now.

Comment 4

[Ben Turner (not reading bugmail, use the needinfo flag!)]

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.

Comment 5

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Created attachment 657995 [details] [diff] [review]
WIP

Removed some extra files that aren't supposed to be here.

Comment 7

[Ben Turner (not reading bugmail, use the needinfo flag!)]

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.

Comment 8

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

This patch is only 99KB, you're slipping.

Comment 9

[Kyle Machulis [:qdot] [:kmachulis] (if a patch has no decent commit message, automatic r-)]

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

Comment 10

[Ben Turner (not reading bugmail, use the needinfo flag!)]

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

Comment 11

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(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 12

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

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.

Comment 13

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

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

Comment 14

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

bent: r=me on IRC interdiff.  Looks good!

Comment 15

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Created attachment 660871 [details] [diff] [review]
Final patch

Ok, comments addressed, nits picked, merged with recent changes, ready to land.

Comment 16

[Ben Turner (not reading bugmail, use the needinfo flag!)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/aee383cd56dc

Comment 17

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/d8e0ceb1fb06 since it didn't quite actually build.

Comment 18

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Fixed up: https://hg.mozilla.org/mozilla-central/rev/8604438c67ad