913372 - [B2G] [Bluetooth] Make a queue for active connection requests

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Bluetooth, defect)

Description

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

Per https://bugzilla.mozilla.org/show_bug.cgi?id=906305#c32

Comment 1

[Preeti Raghunath(:Preeti)]

Gina,

Has there been any progress with this bug?

Comment 2

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

I'm working on it this week. Will update soon here.

Comment 3

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

Attachment: Patch 1(v1): Refine the architecture of BluetoothProfileController

The public interface of class BluetoothProfileController is refined.
- In c-tor, aConnect is set to true for a connect request. Otherwise, set to false for a disconnect request.
- Start(): Set up |mProfiles| and start connecting/disconnecting.
- No return value for AddProfile() and AddProfileWithServiceClass().
- Make a union for CoD and BluetoothServiceClass.

Comment 4

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

Attachment: Patch 2(v1): Set mController to null before invoking callback function

Comment 5

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

Attachment: Patch 3(v1): Make a queue for active connection requests

Comment 6

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

Comment on attachment 818934 [details] [diff] [review]
Patch 1(v1): Refine the architecture of BluetoothProfileController

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

Gina, please see my comments below.

::: dom/bluetooth/BluetoothProfileController.cpp
@@ +48,5 @@
> +  /**
> +   * If the service uuid is not specified, either connect multiple profiles
> +   * based on Cod, or disconnect all connected profiles.
> +   */
> +  bool assignServiceClass = false;

This could be saved if we call SetupProfiles(true) / SetupProfiles(false) inside the if-else block.

@@ +175,5 @@
> +BluetoothProfileController::Start()
> +{
> +  MOZ_ASSERT(NS_IsMainThread());
> +  MOZ_ASSERT(!mDeviceAddress.IsEmpty());
> +  MOZ_ASSERT(++mProfilesIndex == 0);

Please use MOZ_ASSERT(mProfilesIndex == -1), otherwise the value of mProfilesIndex won't be the same after this line in release and debug build.

@@ +195,3 @@
>  
>    if (++mProfilesIndex < mProfiles.Length()) {
>      MOZ_ASSERT(!mDeviceAddress.IsEmpty());

nit: extra assertion.

::: dom/bluetooth/BluetoothProfileController.h
@@ +121,5 @@
> +  // Either CoD or BluetoothServiceClass is assigned.
> +  union {
> +    uint32_t mCod;
> +    BluetoothServiceClass mClass;
> +  } u;

'u' doesn't seem to be clear enough to indicate what this field means. In addition, the union name should start with 'm' since it's a member variable of BluetoothProfileController. For instance,

 union {
   uint32_t cod;

   // I avoid naming it "class" because 1) it's a keyword 2) 'cod' is actually 'class of device',
   // which is quite similar to 'class'.
   BluetoothServiceClass service; 
 } mTarget;  // There should be a better name than 'target' :)

Comment 7

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

Thanks for your comments, Eric. I'll update the patch soon.

> @@ +175,5 @@
> > +BluetoothProfileController::Start()
> > +{
> > +  MOZ_ASSERT(NS_IsMainThread());
> > +  MOZ_ASSERT(!mDeviceAddress.IsEmpty());
> > +  MOZ_ASSERT(++mProfilesIndex == 0);
> 
> Please use MOZ_ASSERT(mProfilesIndex == -1), otherwise the value of
> mProfilesIndex won't be the same after this line in release and debug build.

Nice catch ;) I didn't notice that until you pointed out.

> ::: dom/bluetooth/BluetoothProfileController.h
> @@ +121,5 @@
> > +  // Either CoD or BluetoothServiceClass is assigned.
> > +  union {
> > +    uint32_t mCod;
> > +    BluetoothServiceClass mClass;
> > +  } u;
> 
> 'u' doesn't seem to be clear enough to indicate what this field means. In
> addition, the union name should start with 'm' since it's a member variable
> of BluetoothProfileController. For instance,
> 
>  union {
>    uint32_t cod;
> 
>    // I avoid naming it "class" because 1) it's a keyword 2) 'cod' is
> actually 'class of device',
>    // which is quite similar to 'class'.
>    BluetoothServiceClass service; 
>  } mTarget;  // There should be a better name than 'target' :)

Agree. It's much clearer.

Comment 8

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

Attachment: Patch 1(v2): Refine the architecture of BluetoothProfileController

Patch is updated. Please review again. Thanks.

Comment 9

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

Comment on attachment 821441 [details] [diff] [review]
Patch 1(v2): Refine the architecture of BluetoothProfileController

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

r=me with nits addressed.

::: dom/bluetooth/BluetoothProfileController.cpp
@@ +174,5 @@
> +{
> +  MOZ_ASSERT(NS_IsMainThread());
> +  MOZ_ASSERT(!mDeviceAddress.IsEmpty());
> +  MOZ_ASSERT(mProfilesIndex == -1);
> +  BT_LOGR_PROFILE(mProfiles[++mProfilesIndex], "");

I still have concerns about putting operations which would affect the value of variables inside macros. Can we put "++mProfilesIndex" before BT_LOGR_PROFILE()?

::: dom/bluetooth/BluetoothProfileController.h
@@ +108,5 @@
>  
> +  // Connect/Disconnect next profile in the array
> +  void Next();
> +
> +  bool mConnect;

suggestion: mConnect is a very important variable and shouldn't be modified outside ctor. You could make it a const variable.

Comment 10

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

Both are good suggestions. I'll make sure that these changes are included in the final patch.

Comment 11

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

Comment on attachment 818935 [details] [diff] [review]
Patch 2(v1): Set mController to null before invoking callback function

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

r=me with nits addressed.

::: dom/bluetooth/BluetoothOppManager.cpp
@@ +1481,4 @@
>     * connections. On the other hand, we do nothing for inbound connections.
>     */
>    NS_ENSURE_TRUE_VOID(mController);
>  

Per offline discussion, please leave explanation of why you made this change in commit message.

::: dom/bluetooth/BluetoothOppManager.h
@@ -217,5 @@
>    nsCOMPtr<nsIThread> mReadFileThread;
>    nsCOMPtr<nsIOutputStream> mOutputStream;
>    nsCOMPtr<nsIInputStream> mInputStream;
>    nsCOMPtr<nsIVolumeMountLock> mMountLock;
> -  nsRefPtr<BluetoothReplyRunnable> mRunnable;

Thanks for cleaning up! Then the forward declaration of BluetoothReplyRunnable can be removed as well.

Comment 12

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

Comment on attachment 818936 [details] [diff] [review]
Patch 3(v1): Make a queue for active connection requests

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

Hi Gina, please see my comments below.

::: dom/bluetooth/linux/BluetoothDBusService.cpp
@@ +176,5 @@
>  static nsString sAdapterPath;
>  static Atomic<int32_t> sIsPairing(0);
>  static int sConnectedDeviceCount = 0;
>  static StaticAutoPtr<Monitor> sStopBluetoothMonitor;
> +static nsTArray<BluetoothProfileController*> sControllerArray;

Please use nsTArray<nsRefPtr<BluetoothProfileController> > sControllerArray.

@@ +2555,5 @@
>    return result;
>  }
>  
>  static void
> +NextBluetoothProfileController()

Suggestion: the implementation of this function looks good, however I have an idea: what if we always use the first element as the next one? Then we won't need to iterate the whole array. It looks like:

sControllerArray[0] = nullptr;
sControllerArray.RemoveElementAt(0);

if (!sControllerArray.IsEmpty()) {
  sControllerArray[0]->Start();
}

Make sense?

@@ +2588,2 @@
>  
> +  if (sControllerArray.Length() == 1) {

Please add comments to explain why checking if "sControllerArray.Length() == 1" here.

Comment 13

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

It makes sense to me. Let me refine the patch and send the review request again. Thanks.

Comment 14

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

Attachment: [Final] Patch 1: Refine the architecture of BluetoothProfileController, r=echou

Comment 15

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

Attachment: [Final] Patch 2: Set mController to null before invoking callback function, r=echou

Comment 16

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

Attachment: Patch 3(v2): Make a queue for active connection requests

Eric, please help to check the patch. Thanks.

Comment 17

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

Comment on attachment 821635 [details] [diff] [review]
Patch 3(v2): Make a queue for active connection requests

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

Looks good. Ready to go. :)

Comment 18

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

try server:

https://tbpl.mozilla.org/?tree=Try&rev=d274dc382370
https://tbpl.mozilla.org/?tree=Try&rev=dc5e2b00eb5b

Comment 19

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

Attachment: [Final] Patch 2: Set mController to null before invoking callback function, r=echou

Comment 20

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

Attachment: [Final] Patch 3: Make a queue for active connection requests, r=echou

Comment 21

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

https://hg.mozilla.org/integration/b2g-inbound/rev/704e9cae5ed6
https://hg.mozilla.org/integration/b2g-inbound/rev/cfd0b69ef12b
https://hg.mozilla.org/integration/b2g-inbound/rev/10ad5c75e102

Comment 22

[Ed Morley [:emorley]]

Unfortunately I had to back this out, since it caused conflicts with the backout of bug 920551.

This can reland after rebase :-)

Comment 23

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

Patches have been rebased but b2g-inbound is closed. Will push them after re-opening.

Comment 24

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

https://hg.mozilla.org/integration/b2g-inbound/rev/a11f3d159ce7
https://hg.mozilla.org/integration/b2g-inbound/rev/af4f7b0cebe4
https://hg.mozilla.org/integration/b2g-inbound/rev/00f66158905b

Comment 25

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/a11f3d159ce7
https://hg.mozilla.org/mozilla-central/rev/af4f7b0cebe4
https://hg.mozilla.org/mozilla-central/rev/00f66158905b

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/1c03d1fe7e83
https://hg.mozilla.org/releases/mozilla-aurora/rev/e24d53ad1bad
https://hg.mozilla.org/releases/mozilla-aurora/rev/87bfb72548fa