899014 - [Bluetooth][HID] HID dom bluetooth implementation

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Bluetooth, defect)

Description

[Ben Tian (inactive)]

This bug implements HID manager under dom/bluetooth and is part of bug 880759.

Comment 1

[Ben Tian (inactive)]

Attachment: Patch 1: v1: HID dom bluetooth implementation

The patch implements HID under dom/bluetooth.

Comment 2

[Eric Chou [:ericchou] [:echou]]

Comment on attachment 784160 [details] [diff] [review]
Patch 1: v1: HID dom bluetooth implementation

Review of attachment 784160 [details] [diff] [review]:
-----------------------------------------------------------------

Hi Ben,

Besides these comments, you should also complete HID related implmentation in GetConnectedDevicePropertiesInternal().

Please ask me to review again when the patch is ready. Thanks.

::: dom/bluetooth/BluetoothHidManager.cpp
@@ +15,5 @@
> +#include "mozilla/dom/bluetooth/BluetoothTypes.h"
> +#include "mozilla/Services.h"
> +#include "mozilla/StaticPtr.h"
> +#include "nsIObserverService.h"
> +

super-nit: extra line

@@ +110,5 @@
> +{
> +  MOZ_ASSERT(NS_IsMainThread());
> +  MOZ_ASSERT(!aDeviceAddress.IsEmpty());
> +
> +  if (sInShutdown) {

nit: you may want to use NS_ENSURE_FALSE(sInShutdown, false). Here and below.

@@ +159,5 @@
> +  const BluetoothValue& value = arr[0].value();
> +
> +  if (name.EqualsLiteral("Connected")) {
> +    MOZ_ASSERT(value.type() == BluetoothValue::Tbool);
> +    mConnected = value.get_bool();

I suggest that we can do "MOZ_ASSERT(mConnected != value.get_bool())" before assigning the new value to mConnected.

@@ +195,5 @@
> +void
> +BluetoothHidManager::OnGetServiceChannel(const nsAString& aDeviceAddress,
> +                                         const nsAString& aServiceUuid,
> +                                         int aChannel)
> +{

Whether we will implement this function or not, please add comment here to explain why this function is empty.

@@ +200,5 @@
> +}
> +
> +void
> +BluetoothHidManager::OnUpdateSdpRecords(const nsAString& aDeviceAddress)
> +{

Whether we will implement this function or not, please add comment here to explain why this function is empty.

::: dom/bluetooth/BluetoothHidManager.h
@@ +3,5 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> + * You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +#ifndef mozilla_dom_bluetooth_BluetoothHidmanager_h__

Usually write in lower cases.

@@ +12,5 @@
> +
> +BEGIN_BLUETOOTH_NAMESPACE
> +
> +class BluetoothHidManagerObserver;
> +class BluetoothValue;

Seems not used.

@@ +30,5 @@
> +  virtual void OnUpdateSdpRecords(const nsAString& aDeviceAddress) MOZ_OVERRIDE;
> +  virtual void GetAddress(nsAString& aDeviceAddress) MOZ_OVERRIDE;
> +  virtual bool IsConnected() MOZ_OVERRIDE;
> +
> +  bool Connect(const nsAString& aDeviceAddress);

Connect() should also take runnable as a parameter. See the other comment.

::: dom/bluetooth/BluetoothService.h
@@ +297,5 @@
> +  virtual nsresult
> +  SendInputMessage(const nsAString& aDeviceAddresses,
> +                   const nsAString& aMessage) = 0;
> +
> +

super-nit: extra line

::: dom/bluetooth/linux/BluetoothDBusService.cpp
@@ +20,5 @@
>  #include "BluetoothDBusService.h"
>  #include "BluetoothA2dpManager.h"
>  #include "BluetoothHfpManager.h"
>  #include "BluetoothOppManager.h"
> +#include "BluetoothHidManager.h"

nit: alphabetic order, please.

@@ +2552,5 @@
>      BluetoothOppManager* opp = BluetoothOppManager::Get();
>      opp->Connect(aDeviceAddress, aRunnable);
> +  } else if (aProfileId == BluetoothServiceClass::HID) {
> +    BluetoothHidManager* hid = BluetoothHidManager::Get();
> +    hid->Connect(aDeviceAddress);

We can't leave aRunnable without dealing with it.

Comment 3

[Ben Tian (inactive)]

Attachment: Patch 1: v2: HID dom bluetooth implementation

Changes:
- handle runnable in Connect()
- add comments for empty functions
- revise nits

Comment 4

[Eric Chou [:ericchou] [:echou]]

Comment on attachment 786204 [details] [diff] [review]
Patch 1: v2: HID dom bluetooth implementation

Review of attachment 786204 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with nits addressed.

::: dom/bluetooth/linux/BluetoothDBusService.cpp
@@ +1930,5 @@
> +  DBusCallback callback;
> +  if (aMessage.EqualsLiteral("Connect")) {
> +    callback = GetVoidCallback;
> +  } else if (aMessage.EqualsLiteral("Disconnect")) {
> +    callback = InputDisconnectCallback;

This won't work since aRunnable is still not handled in the callback funciton InputDisconnectCallback().

Based on current implementation, I think GetVoidCallback could be the callback for Disconnect case as well.

@@ +1932,5 @@
> +    callback = GetVoidCallback;
> +  } else if (aMessage.EqualsLiteral("Disconnect")) {
> +    callback = InputDisconnectCallback;
> +  } else {
> +    BT_WARNING("Unknown input message");

aRunnable still needs to be handled. (DispatchBluetoothReply)

Comment 5

[Ben Tian (inactive)]

(In reply to Eric Chou [:ericchou] [:echou] from comment #4)
> Comment on attachment 786204 [details] [diff] [review]
> Patch 1: v2: HID dom bluetooth implementation

> ::: dom/bluetooth/linux/BluetoothDBusService.cpp
> @@ +1930,5 @@
> > +  DBusCallback callback;
> > +  if (aMessage.EqualsLiteral("Connect")) {
> > +    callback = GetVoidCallback;
> > +  } else if (aMessage.EqualsLiteral("Disconnect")) {
> > +    callback = InputDisconnectCallback;
> 
> This won't work since aRunnable is still not handled in the callback
> funciton InputDisconnectCallback().
>
> Based on current implementation, I think GetVoidCallback could be the
> callback for Disconnect case as well.
For Disconnect case, SendInputMessage receives aRunnable=nullptr so InputDisconnectCallback doesn't need to handle the runnable. BluetoothDBusService doesn't pass the runnable into Disconnect() for all profile managers.
 
> @@ +1932,5 @@
> > +    callback = GetVoidCallback;
> > +  } else if (aMessage.EqualsLiteral("Disconnect")) {
> > +    callback = InputDisconnectCallback;
> > +  } else {
> > +    BT_WARNING("Unknown input message");
> 
> aRunnable still needs to be handled. (DispatchBluetoothReply)
I'll re-write here with moz assertion to avoid the 'else' case, since the parameters are internally assigned in BluetoothHidManager.

Comment 6

[Eric Chou [:ericchou] [:echou]]

(In reply to Ben Tian [:btian] from comment #5)
> (In reply to Eric Chou [:ericchou] [:echou] from comment #4)
> > Comment on attachment 786204 [details] [diff] [review]
> > Patch 1: v2: HID dom bluetooth implementation
> 
> > ::: dom/bluetooth/linux/BluetoothDBusService.cpp
> > @@ +1930,5 @@
> > > +  DBusCallback callback;
> > > +  if (aMessage.EqualsLiteral("Connect")) {
> > > +    callback = GetVoidCallback;
> > > +  } else if (aMessage.EqualsLiteral("Disconnect")) {
> > > +    callback = InputDisconnectCallback;
> > 
> > This won't work since aRunnable is still not handled in the callback
> > funciton InputDisconnectCallback().
> >
> > Based on current implementation, I think GetVoidCallback could be the
> > callback for Disconnect case as well.
> For Disconnect case, SendInputMessage receives aRunnable=nullptr so
> InputDisconnectCallback doesn't need to handle the runnable.
> BluetoothDBusService doesn't pass the runnable into Disconnect() for all
> profile managers.
>  

Ah, I missed that. Thanks for clarifying that!

Comment 7

[Ben Tian (inactive)]

Attachment: [final] Patch 1: v3: HID dom bluetooth implementation, r=echou

Comment 8

[Ben Tian (inactive)]

try server (build all): https://tbpl.mozilla.org/?tree=Try&rev=52e8da94ae3a

try server (unit tests): https://tbpl.mozilla.org/?tree=Try&rev=2e0bb6608f81

Comment 9

[Ben Tian (inactive)]

Attachment: [final] Patch 1: v4: HID dom bluetooth implementation, r=echou

Update patch to solve conflict on the latest m-c.

Comment 10

[Ben Tian (inactive)]

try server link (build all):
https://tbpl.mozilla.org/?tree=Try&rev=c574ae8c9edf

try server link (unit test):
https://tbpl.mozilla.org/?tree=Try&rev=6bbceb70abc3

Comment 11

[Eric Chou [:ericchou] [:echou]]

https://hg.mozilla.org/integration/b2g-inbound/rev/95da71532621

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/95da71532621