Closed
Bug 755943
Opened 13 years ago
Closed 13 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•13 years ago
|
blocking-basecamp: --- → +
Comment 1•13 years ago
|
||
How are we doing with this?
Updated•13 years ago
|
Priority: -- → P1
Reporter | ||
Comment 2•13 years ago
|
||
No movement, nor will there be until after sockets are done unless we get more resources.
Reporter | ||
Updated•13 years ago
|
Whiteboard: [LOE:M]
Assignee | ||
Comment 4•13 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•13 years ago
|
||
Removed some extra files that aren't supposed to be here.
Attachment #657992 -
Attachment is obsolete: true
Updated•13 years ago
|
Whiteboard: [LOE:M] → [LOE:M] [WebAPI:P2]
Assignee | ||
Comment 7•13 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•13 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•13 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•13 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•13 years ago
|
||
Comment 17•13 years ago
|
||
Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/d8e0ceb1fb06 since it didn't quite actually build.
Assignee | ||
Comment 18•13 years ago
|
||
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•