Closed Bug 929376 Opened 6 years ago Closed 5 years ago

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

Categories

(Firefox OS Graveyard :: Bluetooth, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(blocking-b2g:2.1+, b2g-v2.1 fixed)

RESOLVED FIXED
2.1 S3 (29aug)
blocking-b2g 2.1+
Tracking Status
b2g-v2.1 --- fixed

People

(Reporter: gyeh, Assigned: jaliu)

References

Details

(Keywords: dev-doc-needed, Whiteboard: [p=1])

Attachments

(2 files, 5 obsolete files)

1.70 KB, patch
Details | Diff | Splinter Review
18.95 KB, patch
Details | Diff | Splinter Review
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.
Blocks: 900801
It's a blocker of bug 900801 (koi+).
blocking-b2g: --- → koi?
Target Milestone: --- → 1.2 C4(Nov8)
koi+ as it blocks a koi+
blocking-b2g: koi? → koi+
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?
Flags: needinfo?(mrbkap)
Flags: needinfo?(echou)
One more question, shall we move this API from BluetoothManager to BluetoothAdapter?

For consistency, we should put isConnected and isScoConnected in the same webidl.
(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.
Flags: needinfo?(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.
Assignee: nobody → gyeh
(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.
Flags: needinfo?(mrbkap)
bug 900801 is no longer koi+
remove koi+
blocking-b2g: koi+ → -
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
We're refactoring Bluetooth API and we'll take it into account. Thanks, Rex.
Ah, I will take it from now on ;)
Assignee: gyeh → shuang
ni btian for tracking this for bluetooth2 new API 2nd refinement.
Flags: needinfo?(btian)
Current WebBluetooth refinement doesn't cover connection API. I'll take this function into consideration when we refactor connection API.
Flags: needinfo?(btian)
Blocks: 1038162
[Blocking Requested - why for this release]:
blocking-b2g: - → 2.1?
We need to fix Bug 1038162, and it's CS 2.1 blocker. Bug 1038162 needs fix this API.
blocking-b2g: 2.1? → 2.1+
Target Milestone: 1.2 C4(Nov8) → 2.1 S2 (15aug)
Assignee: shuang → jaliu
Any update on this?
Status: NEW → ASSIGNED
Whiteboard: [ETA:8/21], [p=2]
Target Milestone: 2.1 S2 (15aug) → 2.1 S3 (29aug)
Whiteboard: [ETA:8/21], [p=2] → [ETA:8/21], [p=1]
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.
Attachment #8475717 - Attachment description: Move 'IsConnected' from BluetoothManager to BluetoothAdapter and allow it to be use on content process. (v1) → Move 'IsConnected' from BluetoothManager to BluetoothAdapter and allow it to be used on content process. (v1)
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 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].
Attachment #8475729 - Flags: superreview?(mrbkap)
Attachment #8475730 - Flags: review?(shuang)
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.
Attachment #8475730 - Flags: review?(shuang)
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.
> > ::: 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 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.
Attachment #8476565 - Flags: review?(shuang) → review+
(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.
Attachment #8475729 - Flags: superreview?(mrbkap) → superreview+
* Add reviewer's ID to commit message

Thank Blake for reviewing the patch.
Attachment #8475729 - Attachment is obsolete: true
* Add reviewer's ID to commit message

Thank Shawn for reviewing the patch.
Attachment #8476565 - Attachment is obsolete: true
(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.
Keywords: checkin-needed
* remove redundant "bool IsConnected()" implementation in Bluedroid service which causes build break.
Attachment #8478185 - Attachment is obsolete: true
Keywords: checkin-needed
(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
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)
Keywords: dev-doc-needed
This patch was accompanied with any automated tests, was there? Or is it already covered by existing tests?
Flags: in-qa-testsuite?
Unable to verify as it is a back-end issue.
QA Whiteboard: [QAnalyst-Triage?][QAnalyst-verify-]
Flags: needinfo?(ktucker)
QA Whiteboard: [QAnalyst-Triage?][QAnalyst-verify-] → [QAnalyst-Triage+][QAnalyst-verify-]
Flags: needinfo?(ktucker)
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).
Flags: in-qa-testsuite? → in-qa-testsuite-
You need to log in before you can comment on or make changes to this bug.