906305 - [B2G] [Bluetooth] Connect multiple files automatically based on the class of device (CoD)

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Bluetooth, defect)

Description

[Gina Yeh [:gyeh] [:ginayeh]]

We'd like to make our API more friendly by automatically connecting to multiple profiles with the class of device as basis.

Comment 1

[Gina Yeh [:gyeh] [:ginayeh]]

Attachment: Patch 1(v1): Implementation of BluetoothProfileController

Comment 2

[Gina Yeh [:gyeh] [:ginayeh]]

Attachment: Patch 2(v1): Make parameter service UUID of Connect() optional

Comment 3

[Gina Yeh [:gyeh] [:ginayeh]]

Attachment: Patch 3(v1): Implement SendAsyncDBusMessage and CheckDBusReply, and remove BluetoothReplyRunnable for SendInputMessage

Comment 4

[Gina Yeh [:gyeh] [:ginayeh]]

Attachment: Patch 4(v1): Fix BluetoothHfpManager

Comment 5

[Gina Yeh [:gyeh] [:ginayeh]]

Attachment: Patch 5(v1): Fix BluetoothOppManager

Comment 6

[Gina Yeh [:gyeh] [:ginayeh]]

Attachment: Patch 6(v1): Fix BluetoothA2dpManager

Comment 7

[Gina Yeh [:gyeh] [:ginayeh]]

Attachment: Patch 7(v1): Fix BluetoothHidManager

Comment 8

[Gina Yeh [:gyeh] [:ginayeh]]

I'm going to drawing an architecture diagram for these patches. Will attach it once I finish. Please hold the review requests and wait for the diagram. It should be helpful to understand what I'm doing in these patches.

Comment 9

[Gina Yeh [:gyeh] [:ginayeh]]

Attachment: Current Architecture

This is a simple diagram to describe the current architecture and call flow.

Comment 10

[Gina Yeh [:gyeh] [:ginayeh]]

Attachment: Proposed Architecture

Compared with the other attachment, "Current Architecture", this diagram describes the general idea of the proposed architecture and new call flow.

Comment 11

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

Comment on attachment 792052 [details] [diff] [review]
Patch 1(v1): Implementation of BluetoothProfileController

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

Gina, please see my comments below. I'll review the rest of the patch once modifications are complete. Thanks.

::: dom/bluetooth/BluetoothProfileController.cpp
@@ +39,5 @@
> +      profile = BluetoothHidManager::Get();
> +      break;
> +  }
> +
> +  NS_ENSURE_TRUE_VOID(profile);

We have to deal with aRunnable once 'profile' is invalid, otherwise it won't be handled.

@@ +57,5 @@
> +  bool hasRendering = HAS_RENDERING(aCod);
> +  bool isPeripheral = IS_PERIPHERAL(aCod);
> +
> +  NS_ENSURE_FALSE_VOID(!hasAudio && !hasObjectTransfer &&
> +                       !hasRendering && !isPeripheral);

Just curious, why not NS_ENSURE_TRUE_VOID(hasAudio || hasObjectTransfer || hasRendering || isPeripheral); ?

@@ +68,5 @@
> +   * It's almost impossible to send file to a remote device which is an Audio
> +   * device or a Rendering device, so we won't connect OPP in that case.
> +   */
> +  if (hasAudio) {
> +    mProfiles.AppendElement(BluetoothHfpManager::Get());

You should check if return value of BluetoothHfpManager::Get() is nullptr, just like how you did in the ctor above. Otherwise it may cause a crash in ConnectNext(). Here and below.

@@ +163,5 @@
> +    BT_WARNING(NS_ConvertUTF16toUTF8(aErrorStr).get());
> +  }
> +
> +  mProfilesIndex++;
> +  ConnectNext();

Since the function name is Connect"Next"(), it seems to be more reasonable to increase the value of mProfilesIndex in that function.

::: dom/bluetooth/BluetoothProfileController.h
@@ +6,5 @@
> +
> +#ifndef mozilla_dom_bluetooth_bluetoothprofilecontroller_h__
> +#define mozilla_dom_bluetooth_bluetoothprofilecontroller_h__
> +
> +#include "BluetoothReplyRunnable.h"

Forward declaration should be fine.

@@ +24,5 @@
> + * https://www.bluetooth.org/en-us/specification/assigned-numbers/baseband
> + */
> +
> +// Bit 23 ~ Bit 13: Major service class
> +#define GetMajorServiceClass(cod) ((cod & 0xffe000) >> 13);

Please do not add a semicolon at the end of a macro. Here and below.

In addition, we usually make macros all caps.

@@ +33,5 @@
> +// Bit 7 ~ Bit 2: Minor device class
> +#define GetMinorDeviceClass(cod)  ((cod & 0xfc) >> 2);
> +
> +// Bit 21: Major service class = 0x100, Audio
> +#define HAS_AUDIO(cod)            ((cod & 0x200000) >> 21);

According to the macro name, I guess we will use this macro when we need to know if a device has Audio service. If so, the only thing we care about would be the value of bit 21. So how about:

  #define HAS_AUDIO(cod)   ((cod & 0x200000) > 0) 

or even

  #define HAS_AUDIO(cod)   (cod & 0x200000)


Here and below.

@@ +39,5 @@
> +// Bit 20: Major service class = 0x80, Object Transfer
> +#define HAS_OBJECT_TRANSFER(cod)  ((cod & 0x100000) >> 20);
> +
> +// Bit 18: Major service class = 0x20, Rendering
> +#define HAS_RENDERING(cod)          ((cod & 0x40000) >> 18);

super-nit: please align with other macros.

@@ +45,5 @@
> +// Bit 11 and Bit 9: Major device class = 0xA, Peripheral
> +#define IS_PERIPHERAL(cod)        (((cod & 0x200) >> 9) && ((cod & 0x800) >> 11));
> +
> +// Bit 10: Major device class = 0x4, Audio/Video
> +// Bit 2: Minor device class = 0x1, Wearable Headset Device 

super-nit: trailing whitespace

@@ +98,5 @@
> +  nsTArray<BluetoothProfileManagerBase*> mProfiles;
> +
> +  uint32_t mCod;
> +  nsString mDeviceAddress;
> +  BluetoothReplyRunnable* mRunnable;

Please use nsRefPtr<BluetoothReplyRunnable> to hold a reference of mRunnable.

Comment 12

[Gina Yeh [:gyeh] [:ginayeh]]

Thanks for reviewing. All suggestions are taken and I'll update them in the next patch. A few questions are answered below.

(In reply to Eric Chou [:ericchou] [:echou] from comment #11)
> @@ +57,5 @@
> > +  bool hasRendering = HAS_RENDERING(aCod);
> > +  bool isPeripheral = IS_PERIPHERAL(aCod);
> > +
> > +  NS_ENSURE_FALSE_VOID(!hasAudio && !hasObjectTransfer &&
> > +                       !hasRendering && !isPeripheral);
> 
> Just curious, why not NS_ENSURE_TRUE_VOID(hasAudio || hasObjectTransfer ||
> hasRendering || isPeripheral); ?

They are exactly the same. Either way should be fine with me.

> @@ +33,5 @@
> > +// Bit 7 ~ Bit 2: Minor device class
> > +#define GetMinorDeviceClass(cod)  ((cod & 0xfc) >> 2);
> > +
> > +// Bit 21: Major service class = 0x100, Audio
> > +#define HAS_AUDIO(cod)            ((cod & 0x200000) >> 21);
> 
> According to the macro name, I guess we will use this macro when we need to
> know if a device has Audio service. If so, the only thing we care about
> would be the value of bit 21. So how about:
> 
>   #define HAS_AUDIO(cod)   ((cod & 0x200000) > 0) 
> 
> or even
> 
>   #define HAS_AUDIO(cod)   (cod & 0x200000)

Yes, we only care about the value of specific bit, and I prefer the latter. Will update soon.

Comment 13

[Gina Yeh [:gyeh] [:ginayeh]]

Attachment: Patch 1(v2): Implementation of BluetoothProfileController

Eric, please take a look at the updated patch. Thanks

Comment 14

[Gina Yeh [:gyeh] [:ginayeh]]

Attachment: Patch 3(v1): Remove BluetoothReplyRunnable in SendInputMessage. Implement SendAsyncDBusMessage, CheckDBusReply, and ReplyErrorToProfileManager

Comment 15

[Gina Yeh [:gyeh] [:ginayeh]]

Attachment: Patch 4(v1): Fix BluetoothHfpManager

Comment 16

[Gina Yeh [:gyeh] [:ginayeh]]

Attachment: Patch 5(v1): Fix BluetoothOppManager

Comment 17

[Gina Yeh [:gyeh] [:ginayeh]]

Attachment: Patch 6(v1): Fix BluetoothA2dpManager

Comment 18

[Gina Yeh [:gyeh] [:ginayeh]]

Attachment: Patch 7(v1): Fix BluetoothHidManager

Comment 19

[Gina Yeh [:gyeh] [:ginayeh]]

Attachment: Patch 3(v1): Remove BluetoothReplyRunnable in SendInputMessage. Implement SendAsyncDBusMessage, CheckDBusReply, and ReplyErrorToProfileManager

Comment 20

[Gina Yeh [:gyeh] [:ginayeh]]

Attachment: Patch 4(v1): Fix BluetoothHfpManager

Comment 21

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

Comment on attachment 795340 [details] [diff] [review]
Patch 1(v2): Implementation of BluetoothProfileController

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

Hi Gina,

I have several suggestions about the design of BluetoothProfileController.

First, let's make constructors simpler. Currently you did different things in three ctors in order to initialize an environment for further use, which are actually Connnect() and Disconnect(). In my opinion, we should only take essential parameters in the ctor. In this case, they are [const nsAString& aDeviceAddress], [BluetoothReplyRunnable* aRunnable] and [BluetoothProfileControllerCallback aCallback].

Next, we need to extend the functionality of Connect() and Disconnect() since mProfiles hasn't been set yet. We can define interface like:

  Connect(BluetoothServiceClass aClass) / Connect(uint32_t aCod) / Disconnect(BluetoothServiceClass aClass = 0)

In other words, I think overriding Connect() / Disconnect() would be better than overriding constructor. Make sense?

::: dom/bluetooth/BluetoothProfileController.cpp
@@ +139,5 @@
> +
> +void
> +BluetoothProfileController::Init(const nsAString& aDeviceAddress,
> +                                 BluetoothReplyRunnable* aRunnable,
> +                                 BluetoothProfileControllerCallback aCallback)

I think we could simplify the interface by using setter in ctor, then these parameters are no longer needed.

@@ +153,5 @@
> +  mProfiles.Clear();
> +}
> +
> +void
> +BluetoothProfileController::Connect()

We should directly call ConnectNext() since Connect() seems to be redundant.

@@ +188,5 @@
> +
> +void
> +BluetoothProfileController::Disconnect()
> +{
> +  DisconnectNext();

We should directly call DisconnectNext() since Disconnect() seems to be redundant.

Comment 22

[Gina Yeh [:gyeh] [:ginayeh]]

(In reply to Eric Chou [:ericchou] [:echou] from comment #21)
> Next, we need to extend the functionality of Connect() and Disconnect()
> since mProfiles hasn't been set yet. We can define interface like:
> 
>   Connect(BluetoothServiceClass aClass) / Connect(uint32_t aCod) /
> Disconnect(BluetoothServiceClass aClass = 0)
> 
> In other words, I think overriding Connect() / Disconnect() would be better
> than overriding constructor. Make sense?

Sounds good to me. Let me try and update patch later. Thanks!

Comment 23

[Gina Yeh [:gyeh] [:ginayeh]]

Attachment: Patch 1(v3): Implementation of BluetoothProfileController

Hi Eric,

The patch has been updated. Please review it again. Thanks.

Comment 24

[Gina Yeh [:gyeh] [:ginayeh]]

Attachment: Patch 2(v1): Make parameter service UUID of Connect() optional

Comment 25

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

Comment on attachment 797129 [details] [diff] [review]
Patch 1(v3): Implementation of BluetoothProfileController

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

Almost there, please see my comments below.

::: dom/bluetooth/BluetoothProfileController.cpp
@@ +34,5 @@
> +}
> +
> +BluetoothProfileController::~BluetoothProfileController()
> +{
> +  mProfilesIndex = -1;

nit: this seems to be unnecessary.

@@ +70,5 @@
> +}
> +
> +void
> +BluetoothProfileController::Connect(BluetoothServiceClass aClass)
> +{

Please add assertion to check if it's on the right thread. Here and below (Connect / Disconnect / OnConnect / OnDisconnect).

@@ +89,5 @@
> +  bool hasAudio = HAS_AUDIO(aCod);
> +  bool hasObjectTransfer = HAS_OBJECT_TRANSFER(aCod);
> +  bool hasRendering = HAS_RENDERING(aCod);
> +  bool isPeripheral = IS_PERIPHERAL(aCod);
> +

There are several NS_ENSURE* function calls in this function, but mRunnable would not be handled once returned early.

@@ +185,5 @@
> +  NS_ENSURE_TRUE_VOID(profile);
> +  if (profile->IsConnected()) {
> +    mProfiles.AppendElement(profile);
> +  }
> +  profile = BluetoothA2dpManager::Get();

I would like to suggest that moving A2DP to the top of disconnecting queue. This is because it's very common to drop A2DP connection before SLC (HFP connection) for most devices which support both HFP and A2DP. Maybe we should reverse the connecting sequence as disconnecting sequence.

::: dom/bluetooth/BluetoothProfileController.h
@@ +73,5 @@
> +   * corresponding profile. Otherwise, disconnect all profiles connected to the
> +   * remote device.
> +   */
> +  void Disconnect(BluetoothServiceClass aClass = BluetoothServiceClass::UNKNOWN);
> +  

nit: extra blanks

Comment 26

[Gina Yeh [:gyeh] [:ginayeh]]

Attachment: Patch 1(v4): Implementation of BluetoothProfileController

Comment 27

[Gina Yeh [:gyeh] [:ginayeh]]

Attachment: Patch 1(v4): Implementation of BluetoothProfileController

Comment 28

[Gina Yeh [:gyeh] [:ginayeh]]

Comment on attachment 798408 [details] [diff] [review]
Patch 1(v4): Implementation of BluetoothProfileController

Changeset:

* Add thread assertion in public functions.
* Change the order of disconnect profiles.
* Add function |AddProfile| and |AddProfileWithServiceClass|. In these functions, |mRunnable| is replied when we failed to get profile manager.

Comment 29

[Gina Yeh [:gyeh] [:ginayeh]]

Attachment: Patch 6(v1): Fix BluetoothA2dpManager

Comment 30

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

Comment on attachment 798408 [details] [diff] [review]
Patch 1(v4): Implementation of BluetoothProfileController

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

Overall looks good. Thanks for your effort. :)

::: dom/bluetooth/BluetoothProfileController.cpp
@@ +79,5 @@
> +    mCallback();
> +    return false;
> +  }
> +
> +  NS_ENSURE_TRUE(!aCheckConnected || aProfile->IsConnected(), false);

NS_WARNING would be called inside NS_ENSURE_TRUE() once the condition checking failed, so we should use NS_ENSURE_TRUE when warning on failure cases are necessary. In this case, when Disconnect() is called, the warning won't show up only if connections for 4 profiles all exist. Thus, I prefer using common 'if' here.

Comment 31

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

Comment on attachment 797130 [details] [diff] [review]
Patch 2(v1): Make parameter service UUID of Connect() optional

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

Hi Gina, please see my comments below.

::: dom/bluetooth/BluetoothSocketObserver.h
@@ +31,1 @@
>   

Please assist with removing the extra blank.

@@ +37,1 @@
>   

Please assist with removing the extra blank.

::: dom/bluetooth/linux/BluetoothDBusService.cpp
@@ +2522,5 @@
> +    BluetoothUuidHelper::GetBluetoothServiceClass(aServiceUuid);
> +
> +  sController =
> +    new BluetoothProfileController(aDeviceAddress, aRunnable,
> +                                   DestroyBluetoothProfileController);

I don't think this will work. Assume that Connect() is called while sController has value, which means that the previous Connect() is still ongoing, the memory block which is held by sController will become invalid right after sController is assigned to a new BluetoothProfileController object. That may cause crash or other unexpected errors.

Since we don't want to forbid users to share files and connect to a Bluetooth headset at the same time, we should solve this.

::: dom/webidl/BluetoothAdapter.webidl
@@ +48,5 @@
>    [GetterThrows]
>    readonly attribute any            uuids;
>  
>    [SetterThrows]
> +  attribute EventHandler   ondevicefound;

Per comment in bug 910974, please modify the format.

@@ +99,5 @@
>     *
> +   * Note that service UUID is optional. If it isn't passed when calling
> +   * Connect, multiple profiles are tried sequencely based on the class of
> +   * device (CoD). If it isn't passed when calling Disconnect, all connected
> +   * profiles are going to closed.

nit: to /be/ closed

Comment 32

[Gina Yeh [:gyeh] [:ginayeh]]

(In reply to Eric Chou [:ericchou] [:echou] from comment #31)
> ::: dom/bluetooth/linux/BluetoothDBusService.cpp
> @@ +2522,5 @@
> > +    BluetoothUuidHelper::GetBluetoothServiceClass(aServiceUuid);
> > +
> > +  sController =
> > +    new BluetoothProfileController(aDeviceAddress, aRunnable,
> > +                                   DestroyBluetoothProfileController);
> 
> I don't think this will work. Assume that Connect() is called while
> sController has value, which means that the previous Connect() is still
> ongoing, the memory block which is held by sController will become invalid
> right after sController is assigned to a new BluetoothProfileController
> object. That may cause crash or other unexpected errors.
> 
> Since we don't want to forbid users to share files and connect to a
> Bluetooth headset at the same time, we should solve this.

I think we can return the second DOM request directly with an error while we're processing the first request. At the same time, I'll file a follow-up to queue all requests and process them 1-by-1. What do you think?

Comment 33

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

(In reply to Gina Yeh [:gyeh] [:ginayeh] from comment #32)
> (In reply to Eric Chou [:ericchou] [:echou] from comment #31)
> > ::: dom/bluetooth/linux/BluetoothDBusService.cpp
> > @@ +2522,5 @@
> > > +    BluetoothUuidHelper::GetBluetoothServiceClass(aServiceUuid);
> > > +
> > > +  sController =
> > > +    new BluetoothProfileController(aDeviceAddress, aRunnable,
> > > +                                   DestroyBluetoothProfileController);
> > 
> > I don't think this will work. Assume that Connect() is called while
> > sController has value, which means that the previous Connect() is still
> > ongoing, the memory block which is held by sController will become invalid
> > right after sController is assigned to a new BluetoothProfileController
> > object. That may cause crash or other unexpected errors.
> > 
> > Since we don't want to forbid users to share files and connect to a
> > Bluetooth headset at the same time, we should solve this.
> 
> I think we can return the second DOM request directly with an error while
> we're processing the first request. At the same time, I'll file a follow-up
> to queue all requests and process them 1-by-1. What do you think?

I'm fine with this. Go ahead. :)

Comment 34

[Gina Yeh [:gyeh] [:ginayeh]]

Attachment: Patch 2(v2): Make parameter service UUID of Connect() optional

Hi Eric,

Patch has been updated based on your comment. Will file a follow-up for creating a request queue. I'll send a superreview request to Blake after your review. Thanks.

Comment 35

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

Comment on attachment 798809 [details] [diff] [review]
Patch 2(v2): Make parameter service UUID of Connect() optional

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

Looks good, and please remember to file a followup for the potential 'connecting at the same time' issue. Thanks.

Comment 36

[Gina Yeh [:gyeh] [:ginayeh]]

Comment on attachment 798809 [details] [diff] [review]
Patch 2(v2): Make parameter service UUID of Connect() optional

Hi Blake,

Please help to review this patch. Thanks.

Comment 37

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

Comment on attachment 795363 [details] [diff] [review]
Patch 3(v1): Remove BluetoothReplyRunnable in SendInputMessage. Implement SendAsyncDBusMessage, CheckDBusReply, and ReplyErrorToProfileManager

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

ok, it's almost there. Please ask for review again once the patch is updated. Thanks.

::: dom/bluetooth/linux/BluetoothDBusService.cpp
@@ +504,5 @@
>    nsAutoString replyError;
>    UnpackVoidMessage(aMsg, nullptr, v, replyError);
> +
> +  BluetoothServiceClass* serviceClass =
> +    static_cast<BluetoothServiceClass*>(aServiceClass);

Please use nsAutoPtr<BluetoothServiceClass> to avoid manipulating memory ourselves.

@@ +1930,4 @@
>    } else if (aMessage.EqualsLiteral("Disconnect")) {
>      callback = InputDisconnectCallback;
> +  } else {
> +    BT_WARNING("Unknown sink message");

We can use MOZ_ASSERT(false) instead warnings since it's totally unreasonable to reach this point. Here and below.

@@ +1950,5 @@
> +  MOZ_ASSERT(aCallback);
> +  MOZ_ASSERT(!aObjectPath.IsEmpty());
> +  MOZ_ASSERT(aInterface);
> +
> +  BluetoothServiceClass* serviceClass = new BluetoothServiceClass();

Please use nsAutoPtr<BluetoothServiceClass> to avoid manipulating memory ourselves.

@@ +3158,5 @@
>  
>  static void
>  ControlCallback(DBusMessage* aMsg, void* aParam)
>  {
> +  if (!aMsg) {

nit: NS_ENSURE_TRUE_VOID(aMsg)

@@ +3166,5 @@
> +
> +  BluetoothValue v;
> +  nsAutoString replyError;
> +  UnpackVoidMessage(aMsg, nullptr, v, replyError);
> +  if(!v.get_bool()) {

nit: need a blank between if and '('

Comment 38

[Gina Yeh [:gyeh] [:ginayeh]]

Attachment: Patch 3(v2): Remove BluetoothReplyRunnable in SendInputMessage. Implement SendAsyncDBusMessage, CheckDBusReply, and ReplyErrorToProfileManager

nit picked.

Comment 39

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

Comment on attachment 799259 [details] [diff] [review]
Patch 3(v2): Remove BluetoothReplyRunnable in SendInputMessage. Implement SendAsyncDBusMessage, CheckDBusReply, and ReplyErrorToProfileManager

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

r=me with nits addressed.

::: dom/bluetooth/linux/BluetoothDBusService.cpp
@@ +509,5 @@
> +      profile = BluetoothHidManager::Get();
> +    } else if (mServiceClass == BluetoothServiceClass::A2DP) {
> +      profile = BluetoothA2dpManager::Get();
> +    } else {
> +      BT_WARNING("Invalid profile");

Use MOZ_ASSERT(false) for consistency.

@@ +549,3 @@
>    }
> +
> +  delete serviceClass;

Please don't call delete here. serviceClass will delete the memory it holds after leaving this function..

@@ +1982,4 @@
>    bool ret = dbus_func_args_async(mConnection,
>                                    -1,
> +                                  aCallback,
> +                                  static_cast<void*>(serviceClass),

You have to use serviceClass.forget() here, or the memory would become invalid after leaving this function.

Comment 40

[Gina Yeh [:gyeh] [:ginayeh]]

(In reply to Eric Chou [:ericchou] [:echou] from comment #39)
> > +  delete serviceClass;
> 
> Please don't call delete here. serviceClass will delete the memory it holds
> after leaving this function..
> 
> @@ +1982,4 @@
> >    bool ret = dbus_func_args_async(mConnection,
> >                                    -1,
> > +                                  aCallback,
> > +                                  static_cast<void*>(serviceClass),
> 
> You have to use serviceClass.forget() here, or the memory would become
> invalid after leaving this function.

My bad... I'll cover them in the final patch.

Comment 41

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 798809 [details] [diff] [review]
Patch 2(v2): Make parameter service UUID of Connect() optional

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

::: dom/webidl/BluetoothAdapter.webidl
@@ +97,5 @@
>     * To check the value of service UUIDs, please check "Bluetooth Assigned
>     * Numbers" / "Service Discovery Protocol" for more information.
>     *
> +   * Note that service UUID is optional. If it isn't passed when calling
> +   * Connect, multiple profiles are tried sequencely based on the class of

"sequentially"

Comment 42

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

Comment on attachment 795364 [details] [diff] [review]
Patch 4(v1): Fix BluetoothHfpManager

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

Hi Gina, the main reason of r- is I think we should move OnConnect/OnDisconnect from interface BluetoothProfileManagerBase to each Bluetooth*Manager. Please see my comments below.

::: dom/bluetooth/BluetoothHfpManager.cpp
@@ +1098,2 @@
>  {
> +

Please add an assertion here to check current thread.

::: dom/bluetooth/BluetoothHfpManager.h
@@ +7,5 @@
>  #ifndef mozilla_dom_bluetooth_bluetoothhfpmanager_h__
>  #define mozilla_dom_bluetooth_bluetoothhfpmanager_h__
>  
>  #include "BluetoothCommon.h"
> +#include "BluetoothProfileController.h"

Forward declaration of BluetoothProfileController should be fine.

@@ +152,5 @@
>  
>    nsTArray<Call> mCurrentCallArray;
>    nsAutoPtr<BluetoothTelephonyListener> mListener;
>    nsRefPtr<BluetoothReplyRunnable> mRunnable;
> +  BluetoothProfileController* mController;

Please use nsAutoPtr<BluetoothProfileController>

::: dom/bluetooth/BluetoothProfileManagerBase.h
@@ +59,5 @@
> +   * If it establishes/releases a connection successfully, the error string
> +   * will be empty. Otherwise, the error string shows the failure reason.
> +   */
> +  virtual void OnConnect(const nsAString& aErrorStr) = 0;
> +  virtual void OnDisconnect(const nsAString& aErrorStr) = 0;

I may want to remove these two functions from interface BluetoothProfileManagerBase. It's weird to define a virtual function which will not be called from outside, but in another pure virtual function which is also implemented in the same class. Since OnConnect/OnDisconnect have already been implemented in BluetoothProfileController, I think we can change these two into local functions.

Comment 43

[Gina Yeh [:gyeh] [:ginayeh]]

(In reply to Blake Kaplan (:mrbkap) [PTO Sep 5-Sep 19] from comment #41)
> Comment on attachment 798809 [details] [diff] [review]
> Patch 2(v2): Make parameter service UUID of Connect() optional
> 
> Review of attachment 798809 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/webidl/BluetoothAdapter.webidl
> @@ +97,5 @@
> >     * To check the value of service UUIDs, please check "Bluetooth Assigned
> >     * Numbers" / "Service Discovery Protocol" for more information.
> >     *
> > +   * Note that service UUID is optional. If it isn't passed when calling
> > +   * Connect, multiple profiles are tried sequencely based on the class of
> 
> "sequentially"

Thanks for your review, Blake. :)

Comment 44

[Gina Yeh [:gyeh] [:ginayeh]]

(In reply to Eric Chou [:ericchou] [:echou] from comment #42)
> ::: dom/bluetooth/BluetoothProfileManagerBase.h
> @@ +59,5 @@
> > +   * If it establishes/releases a connection successfully, the error string
> > +   * will be empty. Otherwise, the error string shows the failure reason.
> > +   */
> > +  virtual void OnConnect(const nsAString& aErrorStr) = 0;
> > +  virtual void OnDisconnect(const nsAString& aErrorStr) = 0;
> 
> I may want to remove these two functions from interface
> BluetoothProfileManagerBase. It's weird to define a virtual function which
> will not be called from outside, but in another pure virtual function which
> is also implemented in the same class. Since OnConnect/OnDisconnect have
> already been implemented in BluetoothProfileController, I think we can
> change these two into local functions.

These functions are used in Patch 3 for error handling.

For BluetoothA2dpManager and BluetoothHidManager, we might fail to establish/release connection with an error string returned, and OnConnect/OnDisconnect is invoked in this case.

Comment 45

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

Comment on attachment 795364 [details] [diff] [review]
Patch 4(v1): Fix BluetoothHfpManager

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

ok, r+ per comment 44 and discussion with Gina, Shawn and Ben. Please still pick other nits mentioned in comment 42. Thank you.

Comment 46

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

Comment on attachment 795360 [details] [diff] [review]
Patch 5(v1): Fix BluetoothOppManager

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

::: dom/bluetooth/BluetoothOppManager.h
@@ +214,5 @@
>    nsCOMPtr<nsIOutputStream> mOutputStream;
>    nsCOMPtr<nsIInputStream> mInputStream;
>    nsCOMPtr<nsIVolumeMountLock> mMountLock;
>    nsRefPtr<BluetoothReplyRunnable> mRunnable;
> +  BluetoothProfileController* mController;

Please use nsAutoPtr<BluetoothProfileController>. In addition, BluetoothProfileController should be declared somewhere in this header file or it will cause build break, however I don't see related code.

Comment 47

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

(In reply to Eric Chou [:ericchou] [:echou] from comment #46)
> Comment on attachment 795360 [details] [diff] [review]
> Patch 5(v1): Fix BluetoothOppManager
> 
> Review of attachment 795360 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/bluetooth/BluetoothOppManager.h
> @@ +214,5 @@
> >    nsCOMPtr<nsIOutputStream> mOutputStream;
> >    nsCOMPtr<nsIInputStream> mInputStream;
> >    nsCOMPtr<nsIVolumeMountLock> mMountLock;
> >    nsRefPtr<BluetoothReplyRunnable> mRunnable;
> > +  BluetoothProfileController* mController;
> 
> Please use nsAutoPtr<BluetoothProfileController>. In addition,
> BluetoothProfileController should be declared somewhere in this header file
> or it will cause build break, however I don't see related code.

Wait, I found that BluetoothProfileController can't be held by nsAutoPtr in each Bleutooth*Manager because the BluetoothProfileController instance will be killed after mController set to nullptr. Therefore please ignore my comment in comment 42 and comment 46.

It's ok to keep it raw pointer for now, however I think it would be better if we could make BluetoothProfileController reference counted.

Comment 48

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

Comment on attachment 798437 [details] [diff] [review]
Patch 6(v1): Fix BluetoothA2dpManager

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

r=me with questions answered.

::: dom/bluetooth/BluetoothA2dpManager.cpp
@@ +255,5 @@
> +    aSignal.value().get_ArrayOfBluetoothNamedValue();
> +  MOZ_ASSERT(arr.Length() == 1);
> +
> +  const nsString& name = arr[0].name();
> +  NS_ENSURE_TRUE_VOID(name.EqualsLiteral("State"));

Question: This means that we will only process the PropertyChanged event of attribute "State" and ignore "Connected" and "Playing". Is that what we want? If so, please leave a comment here. Thanks.

@@ +278,5 @@
> +    // case 5: Audio stream suspended
> +  } else if (mSinkState == SinkState::SINK_DISCONNECTED &&
> +             prevState == SinkState::SINK_CONNECTING) {
> +    // case 2: Connection attempt failed
> +    OnConnect(EmptyString());

Question: Does it make sense to call OnConnect() without error string on connecting failed case?

@@ +305,5 @@
> +    // case 7: Disconnected from local
> +    if (prevState == SinkState::SINK_DISCONNECTING) {
> +      OnDisconnect(EmptyString());
> +    }
> +  }

Suggestion: Since there are only 5 states, I would probably check mSinkState first, then all unreasonable cases could be handled in each if/else-if blocks.

Comment 49

[Gina Yeh [:gyeh] [:ginayeh]]

Thanks, Eric :)

Comment 50

[Gina Yeh [:gyeh] [:ginayeh]]

(In reply to Eric Chou [:ericchou] [:echou] from comment #48)
> ::: dom/bluetooth/BluetoothA2dpManager.cpp
> @@ +255,5 @@
> > +    aSignal.value().get_ArrayOfBluetoothNamedValue();
> > +  MOZ_ASSERT(arr.Length() == 1);
> > +
> > +  const nsString& name = arr[0].name();
> > +  NS_ENSURE_TRUE_VOID(name.EqualsLiteral("State"));
> 
> Question: This means that we will only process the PropertyChanged event of
> attribute "State" and ignore "Connected" and "Playing". Is that what we
> want? If so, please leave a comment here. Thanks.

Yep! I will add a comment here.

> @@ +278,5 @@
> > +    // case 5: Audio stream suspended
> > +  } else if (mSinkState == SinkState::SINK_DISCONNECTED &&
> > +             prevState == SinkState::SINK_CONNECTING) {
> > +    // case 2: Connection attempt failed
> > +    OnConnect(EmptyString());
> 
> Question: Does it make sense to call OnConnect() without error string on
> connecting failed case?

Agree. We should return an error string in this case.

Comment 51

[Gina Yeh [:gyeh] [:ginayeh]]

Attachment: [Final] Patch 1: Implementation of BluetoothProfileController, r=echou

Comment 52

[Gina Yeh [:gyeh] [:ginayeh]]

Attachment: [Final] Patch 2: Make parameter service UUID of Connect() optional, r=echou, sr=mrbkap

Comment 53

[Gina Yeh [:gyeh] [:ginayeh]]

Attachment: [Final] Patch 3: Implement SendAsyncDBusMessage and CheckDBusReply, and remove BluetoothReplyRunnable for SendInputMessage, r=echou

Comment 54

[Gina Yeh [:gyeh] [:ginayeh]]

Attachment: [Final] Patch 4: Fix BluetoothHfpManager, r=echou

Comment 55

[Gina Yeh [:gyeh] [:ginayeh]]

Attachment: [Final] Patch 5: Fix BluetoothOppManager, r=echou

Comment 56

[Gina Yeh [:gyeh] [:ginayeh]]

Attachment: [Final] Patch 6: Fix BluetoothA2dpManager, r=echou

Comment 57

[Gina Yeh [:gyeh] [:ginayeh]]

Attachment: [Final] Patch 7(v1): Fix BluetoothHidManager, r=echou

Comment 58

[Gina Yeh [:gyeh] [:ginayeh]]

try:

https://tbpl.mozilla.org/?tree=Try&rev=faf8c3653943
https://tbpl.mozilla.org/?tree=Try&rev=f639cd256c53

Comment 59

[Gina Yeh [:gyeh] [:ginayeh]]

https://hg.mozilla.org/integration/b2g-inbound/rev/ffd9e8e157dd
https://hg.mozilla.org/integration/b2g-inbound/rev/ef9b729c38a1
https://hg.mozilla.org/integration/b2g-inbound/rev/585da14f78e2
https://hg.mozilla.org/integration/b2g-inbound/rev/a79e09ef9e0b
https://hg.mozilla.org/integration/b2g-inbound/rev/066ddde89edf
https://hg.mozilla.org/integration/b2g-inbound/rev/141a21a230d2
https://hg.mozilla.org/integration/b2g-inbound/rev/abaf0fa5fd0d

Comment 60

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/ffd9e8e157dd
https://hg.mozilla.org/mozilla-central/rev/ef9b729c38a1
https://hg.mozilla.org/mozilla-central/rev/585da14f78e2
https://hg.mozilla.org/mozilla-central/rev/a79e09ef9e0b
https://hg.mozilla.org/mozilla-central/rev/066ddde89edf
https://hg.mozilla.org/mozilla-central/rev/141a21a230d2
https://hg.mozilla.org/mozilla-central/rev/abaf0fa5fd0d