929376 - [B2G] [Bluetooth] Content process should be able to use API: BluetoothManager.IsConnected()

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Bluetooth, defect)

Description

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

This API should be available for both chrome process and content process. However, only chrome process can use it in current code base, and I think we should fix it.

Comment 1

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

It's a blocker of bug 900801 (koi+).

Comment 2

[Joe Cheng [:jcheng] (please needinfo)]

koi+ as it blocks a koi+

Comment 3

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

I have some concerns about this API.

As shown in the following, isConnected is a synchronous function and returns a boolean value after getting connection status from the specified profile manager.

interface BluetoothManager : EventTarget {
  ...

  [Throws]
  boolean     isConnected(unsigned short aProfile);

  ...
};

It should be fine for a chrome process. However, if we want to use this API in content process, ipc must be involved. Shall we make it to an asynchronous call?

Another solution is that we cache the connection status of each profile in BluetoothManager. With cache mechanism, the API can still be synchronous. Or, we can just remove the API and export profile connection status as readonly attributes.

Eric and Blake, any comment?

Comment 4

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

One more question, shall we move this API from BluetoothManager to BluetoothAdapter?

For consistency, we should put isConnected and isScoConnected in the same webidl.

Comment 5

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

(In reply to Gina Yeh [:gyeh] [:ginayeh] from comment #4)
> One more question, shall we move this API from BluetoothManager to
> BluetoothAdapter?
> 
> For consistency, we should put isConnected and isScoConnected in the same
> webidl.

Yes, I think we should move this API to BluetoothAdapter. It's more reasonable to make Connect/Disconnect/IsConnected belong to the same object.

Comment 6

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

(In reply to Gina Yeh [:gyeh] [:ginayeh] from comment #3)
> I have some concerns about this API.
> 
> As shown in the following, isConnected is a synchronous function and returns
> a boolean value after getting connection status from the specified profile
> manager.
> 
> interface BluetoothManager : EventTarget {
>   ...
> 
>   [Throws]
>   boolean     isConnected(unsigned short aProfile);
> 
>   ...
> };
> 
> It should be fine for a chrome process. However, if we want to use this API
> in content process, ipc must be involved. Shall we make it to an
> asynchronous call?
> 
> Another solution is that we cache the connection status of each profile in
> BluetoothManager. With cache mechanism, the API can still be synchronous.
> Or, we can just remove the API and export profile connection status as
> readonly attributes.
> 
> Eric and Blake, any comment?

Since other similar APIs (Connect/Disconnect/ConnectSco/DisconnectSco/IsScoConnected) have been designed with returning DOM Requests, it would be better if "IsConnected" could be implemented with the same manner.

Comment 7

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to Gina Yeh [:gyeh] [:ginayeh] from comment #3)
> It should be fine for a chrome process. However, if we want to use this API
> in content process, ipc must be involved. Shall we make it to an
> asynchronous call?

Is this for the DOM API or simply an internal API? For an internal API, I'd say that we should make this async.

For a DOM API, since we probably have to notify everybody when we connect or disconnect, it's only a little more work on our end to cache the value, which makes life a little easier for our users, so I tend to err on the side of making some of these smaller APIs sync. bent was worrying a little about adding more synchronous IPC calls and we discussed some solutions to mitigate that, but I still think having a couple of these APIs being sync is OK.

Comment 8

[Joe Cheng [:jcheng] (please needinfo)]

bug 900801 is no longer koi+
remove koi+

Comment 9

[KM Lee [:rexboy] (inactivate after Sep. 22)]

Bug 900801 is 1.3+ed again now.
It's not necessary to make it work with this patch, but using isConnected should be better and more efficient.
We can re-nominate if needed.

see:
https://github.com/rexboy7/gaia/compare/fix-bug900801?expand=1#diff-d92153efec08c1bd2eb8540c4a6a8c31R75
https://github.com/rexboy7/gaia/compare/fix-bug900801?expand=1#diff-d92153efec08c1bd2eb8540c4a6a8c31R708

Comment 10

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

We're refactoring Bluetooth API and we'll take it into account. Thanks, Rex.

Comment 11

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Ah, I will take it from now on ;)

Comment 12

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

ni btian for tracking this for bluetooth2 new API 2nd refinement.

Comment 13

[Ben Tian (inactive)]

Current WebBluetooth refinement doesn't cover connection API. I'll take this function into consideration when we refactor connection API.

Comment 14

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

[Blocking Requested - why for this release]:

Comment 15

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

We need to fix Bug 1038162, and it's CS 2.1 blocker. Bug 1038162 needs fix this API.

Comment 16

[Hema Koka [:hema]]

Any update on this?

Comment 17

[Jamin Liu [:jaliu][:Jamin]]

Sorry for the late reply.
I'll upload a patch for reviewing tomorrow and discuss with Dominic to see if it fit the need of Bug 1038162.

Comment 18

[Jamin Liu [:jaliu][:Jamin]]

Attachment: Move 'IsConnected' from BluetoothManager to BluetoothAdapter and allow it to be used on content process. (v1)

Comment 19

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Hey Jamin,
For webidl, you have to go through superview! So please divide these webidl patchset into another review request and change to dom peer (:mrbkap).

Comment 20

[Jamin Liu [:jaliu][:Jamin]]

Attachment: Part 1: Move 'IsConnected' from BluetoothManager.webidl to BluetoothAdapter.webidl and make it asynchronous. (v1)

Comment 21

[Jamin Liu [:jaliu][:Jamin]]

Attachment: Part 2: Move 'IsConnected' from BluetoothManager to BluetoothAdapter and allow it to be used on content process. (v1)

Comment 22

[Jamin Liu [:jaliu][:Jamin]]

Comment on attachment 8475729 [details] [diff] [review]
Part 1: Move 'IsConnected' from BluetoothManager.webidl to BluetoothAdapter.webidl and make it asynchronous. (v1)

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

Hi Blake,
I modified BluetoothManager.webidl and BluetoothAdapter.webidl to allow one of our Bluetooth APIs to be used by content process.
Would you mind to review the changes of webidl?

#Attachment 8475729 [details] [diff] Bug 929376 - Part 1: Move 'IsConnected' from ...
* Moved 'isConnected()' from BluetoothManager to BluetoothAdapter.
* Made 'isConnected()' become an asynchronous call.

Thank you.

P.S. The implementation of this patch is put at #Attachment 8475730 [details] [diff].

Comment 23

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Comment on attachment 8475730 [details] [diff] [review]
Part 2: Move 'IsConnected' from BluetoothManager to BluetoothAdapter and allow it to be used on content process. (v1)

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

::: dom/bluetooth/BluetoothAdapter.cpp
@@ +746,5 @@
>    return request.forget();
>  }
>  
>  already_AddRefed<DOMRequest>
> +BluetoothAdapter::IsConnected(const short unsigned int aServiceUuid, ErrorResult& aRv)

hmm I saw "const short unsigned int" and uint16_t. Maybe we shall use the same datatype in dom/bluetooth2.

::: dom/bluetooth/bluez/BluetoothDBusService.cpp
@@ +3466,5 @@
>  
>    BluetoothProfileManagerBase* profile =
>      BluetoothUuidHelper::GetBluetoothProfileManager(aServiceUuid);
>    if (!profile) {
> +    DispatchBluetoothReply(aRunnable, false, EmptyString());

NS_NAMED_LITERAL_STRING(replyError, "Calling IsConnected() failed, unknown profile");
DispatchBluetoothReply(aRunnable, BluetoothValue(), replyError);

@@ +3466,5 @@
>  
>    BluetoothProfileManagerBase* profile =
>      BluetoothUuidHelper::GetBluetoothProfileManager(aServiceUuid);
>    if (!profile) {
> +    DispatchBluetoothReply(aRunnable, false, EmptyString());

Please add BT_WARNING.

Comment 24

[Jamin Liu [:jaliu][:Jamin]]

Attachment: Part 2: Move 'IsConnected' from BluetoothManager to BluetoothAdapter and allow it to be used on content process. (v2)

Comment 25

[Jamin Liu [:jaliu][:Jamin]]

Thank you for your comments.

(In reply to Shawn Huang [:shuang] [:shawnjohnjr] from comment #23)
> Comment on attachment 8475730 [details] [diff] [review]
> Part 2: Move 'IsConnected' from BluetoothManager to BluetoothAdapter and
> allow it to be used on content process. (v1)
> 
> Review of attachment 8475730 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/bluetooth/BluetoothAdapter.cpp
> @@ +746,5 @@
> >    return request.forget();
> >  }
> >  
> >  already_AddRefed<DOMRequest>
> > +BluetoothAdapter::IsConnected(const short unsigned int aServiceUuid, ErrorResult& aRv)
> 
> hmm I saw "const short unsigned int" and uint16_t. Maybe we shall use the
> same datatype in dom/bluetooth2.

Yes, I think we should review every files in dom/bluetooth2 later.
I revised the datatypes for IsConnected(), Connect() and Disconnect() at #Attachment 8476565 [details] [diff].

> ::: dom/bluetooth/bluez/BluetoothDBusService.cpp
> @@ +3466,5 @@
> >  
> >    BluetoothProfileManagerBase* profile =
> >      BluetoothUuidHelper::GetBluetoothProfileManager(aServiceUuid);
> >    if (!profile) {
> > +    DispatchBluetoothReply(aRunnable, false, EmptyString());
> 
> NS_NAMED_LITERAL_STRING(replyError, "Calling IsConnected() failed, unknown
> profile");
> DispatchBluetoothReply(aRunnable, BluetoothValue(), replyError);

I think we don't have to report error when the profile manager isn't found, since the user might want to check the connection status of the profile which Firefox OS don't support. In that case, it's reasonable to reply 'false'.

Consider to another case, if BluetoothUuidHelper can't find the profile manager which Firefox OS support, we can treat it as an internal error or just reply it as 'unconnected'.

// --- Method 1, Report an internal error if BluetoothUuidHelper can't find the profile ---
  if (profile) {
    DispatchBluetoothReply(aRunnable, profile->IsConnected(), EmptyString());
  } else {
    BluetoothServiceClass serviceClass =
      BluetoothUuidHelper::GetBluetoothServiceClass(aServiceUuid);
    if (serviceClass == BluetoothServiceClass::HANDSFREE ||
        serviceClass == BluetoothServiceClass::HEADSET ||
        serviceClass == BluetoothServiceClass::HID ||
        serviceClass == BluetoothServiceClass::A2DP ||
        serviceClass == BluetoothServiceClass::OBJECT_PUSH) {
      BT_WARNING("Calling IsConnected() failed, can't find supported profile");
      NS_NAMED_LITERAL_STRING(replyError, "Calling IsConnected() failed, can't find supported profile");
      DispatchBluetoothReply(aRunnable, BluetoothValue(), replyError);
    } else {
      DispatchBluetoothReply(aRunnable, false, EmptyString());
    }
  }


// --- Method 2, Just reply it as 'unconnected'. ---
  BluetoothProfileManagerBase* profile =
    BluetoothUuidHelper::GetBluetoothProfileManager(aServiceUuid);
  if (profile) {
    DispatchBluetoothReply(aRunnable, profile->IsConnected(), EmptyString());
  } else {
    BT_WARNING("Can't find profile manager with uuid: %x", aServiceUuid);
    DispatchBluetoothReply(aRunnable, false, EmptyString());
  }

Personally, I prefer to reply 'false' directly (i.e. method 2) since the profile can't be 'connected' when the profile manager is not ready.

Furthermore, if BluetoothUuidHelper can't get the pointer of profile manager which Firefox OS support, the corresponding function BluetoothXxxManager::Get() would throw a NS_WARNING anyway.

> @@ +3466,5 @@
> >  
> >    BluetoothProfileManagerBase* profile =
> >      BluetoothUuidHelper::GetBluetoothProfileManager(aServiceUuid);
> >    if (!profile) {
> > +    DispatchBluetoothReply(aRunnable, false, EmptyString());
> 
> Please add BT_WARNING.
Got it.
Thank you.

Comment 26

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

> > ::: dom/bluetooth/bluez/BluetoothDBusService.cpp
> > @@ +3466,5 @@
> > >  
> > >    BluetoothProfileManagerBase* profile =
> > >      BluetoothUuidHelper::GetBluetoothProfileManager(aServiceUuid);
> > >    if (!profile) {
> > > +    DispatchBluetoothReply(aRunnable, false, EmptyString());
> > 
Well, in the beginning I thought we only need to take care of Service that we supported (ex:HFP/A2DP/OPP).
Yep, you're right, JS side only wants to query that specific service is get connected or not. JS doesn't really care the profile is one of our supported features. But IsConnected(uuid) function might extend to user defined profiles (although that depends on btsocket api and we are not there yet).

Comment 27

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Comment on attachment 8476565 [details] [diff] [review]
Part 2: Move 'IsConnected' from BluetoothManager to BluetoothAdapter and allow it to be used on content process. (v2)

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

Please try to test both IsConnected/IsScoConnected on bluedroid/bluez backend. And submit to try server.

Comment 28

[Jamin Liu [:jaliu][:Jamin]]

(In reply to Shawn Huang [:shuang] [:shawnjohnjr] from comment #27)
> Please try to test both IsConnected/IsScoConnected on bluedroid/bluez
> backend. And submit to try server.

Got it.
Thank you for reviewing the patch.

Comment 29

[Jamin Liu [:jaliu][:Jamin]]

Attachment: Part 1: Move 'IsConnected' from BluetoothManager.webidl to BluetoothAdapter.webidl and make it asynchronous. (v1), sr=mrbkap

* Add reviewer's ID to commit message

Thank Blake for reviewing the patch.

Comment 30

[Jamin Liu [:jaliu][:Jamin]]

Attachment: Part 2: Move 'IsConnected' from BluetoothManager to BluetoothAdapter and allow it to be used on content process. (v2), r=shawnjohnjr

* Add reviewer's ID to commit message

Thank Shawn for reviewing the patch.

Comment 31

[Jamin Liu [:jaliu][:Jamin]]

(In reply to Shawn Huang [:shuang] [:shawnjohnjr] from comment #27)
> Comment on attachment 8476565 [details] [diff] [review]
> Part 2: Move 'IsConnected' from BluetoothManager to BluetoothAdapter and
> allow it to be used on content process. (v2)
> 
> Review of attachment 8476565 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Please try to test both IsConnected/IsScoConnected on bluedroid/bluez
> backend. And submit to try server.

It looks find on try server.
https://tbpl.mozilla.org/?tree=Try&rev=5fa3617a2c9a

I've tested both IsConnected/IsScoConnected on flame(bluez) / flame-kk(bluedroid).

I tested these cases on flame(bluez).
1) IsConnected:false,  IsScoConnected: false
2) IsConnected:false,  IsScoConnected: true
3) IsConnected:true,  IsScoConnected: true

However, I didn't verify case 3) on flame-kk since RIL module on KK is not ready.

Comment 32

[(PTO June 19-July 7) Ryan VanderMeulen]

https://hg.mozilla.org/integration/b2g-inbound/rev/a38ad201299f
https://hg.mozilla.org/integration/b2g-inbound/rev/d9cbdfbed1f5

Comment 33

[(PTO June 19-July 7) Ryan VanderMeulen]

Backed out for bustage.
https://hg.mozilla.org/integration/b2g-inbound/rev/ae82bbf09403

https://tbpl.mozilla.org/php/getParsedLog.php?id=46687885&tree=B2g-Inbound

Comment 34

[Jamin Liu [:jaliu][:Jamin]]

Attachment: Part 2: Move 'IsConnected' from BluetoothManager to BluetoothAdapter and allow it to be used on content process. (v3), r=shawnjohnjr

* remove redundant "bool IsConnected()" implementation in Bluedroid service which causes build break.

Comment 35

[Jamin Liu [:jaliu][:Jamin]]

(In reply to Jamin Liu [:jaliu] from comment #34)
> Created attachment 8478743 [details] [diff] [review]
> Part 2: Move 'IsConnected' from BluetoothManager to BluetoothAdapter and
> allow it to be used on content process. (v3), r=shawnjohnjr
> 
> * remove redundant "bool IsConnected()" implementation in Bluedroid service
> which causes build break.

It looks fine on emulator-kk which uses Bluedroid as Bluetooth backend.
https://tbpl.mozilla.org/?tree=Try&rev=a4cc47c04644

Comment 36

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

Re-landed.

https://hg.mozilla.org/integration/b2g-inbound/rev/26df8a72a550
https://hg.mozilla.org/integration/b2g-inbound/rev/1e660f9005f0

Comment 37

[(PTO June 19-July 7) Ryan VanderMeulen]

https://hg.mozilla.org/mozilla-central/rev/26df8a72a550
https://hg.mozilla.org/mozilla-central/rev/1e660f9005f0

Comment 38

[Jamin Liu [:jaliu][:Jamin]]

Add keyword "dev-doc-needed". 

MDN is required for the changes in this bug.
Please refer to webidl patch Attachment #8478184 [details] [diff].
* The method IsConnected() is moved from BluetoothManager to BluetoothAdapter.
* The return value is changed from 'DOMRequest' to "boolean".

https://developer.mozilla.org/en-US/docs/Web/API/BluetoothManager (should be changed)
https://developer.mozilla.org/en-US/docs/Web/API/BluetoothAdapter (should be changed)
https://developer.mozilla.org/en-US/docs/Web/API/BluetoothManager.isConnected (should be deleted)
https://developer.mozilla.org/en-US/docs/Web/API/BluetoothAdapter.isConnected (should be created)

Comment 39

[Martijn Wargers (dead)]

This patch was accompanied with any automated tests, was there? Or is it already covered by existing tests?

Comment 40

[Yeojin Chung [:YeojinC]]

Unable to verify as it is a back-end issue.

Comment 41

[Geo Mealer [:geo] -- This account is inactive after 2015-07-07]

Don't think we're going to be able to do this one with the QA tests. Might be a good candidate for in-testsuite so it gets handled by back end unit tests (not sure if we use the flag in FxOS).