[b2g-bluetooth] Replace some system messages with event handlers

RESOLVED FIXED

Status

RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: gyeh, Assigned: gyeh)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 14 obsolete attachments)

13.76 KB, patch
Details | Diff | Splinter Review
9.26 KB, patch
Details | Diff | Splinter Review
(Assignee)

Description

6 years ago
Discussed with evelyn, echou, and shuang, we'd like to replace the following system messages with event handlers in nsIDOMBluetoothAdapter.idl

* bluetooth-pairedstatuschanged
* bluetooth-hfp-connection-status-changed

For Bluetooth Transfer app part, need to discuss with Ian. Maybe we can do so for "bluetooth-opp-transfer-start", "bluetooth-opp-transfer-complete", "bluetooth-opp-update-progress".
Add a system message "bluetooth-opp-receiving-file-confirmation" which is missed in above list. 

I also think using all bluetooth event via system message is not the best way now.
May I know what is the benefit for the change? Thanks.
(Assignee)

Comment 2

6 years ago
I'm afraid that we may still keep "bluetooth-opp-receiving-file-confirmation" to be sent via system message since it should be handled even if Bluetooth Transfer App hasn't been launched yet.

This is very similar to pairing scenario, and that's why we still keep pairing-related messages by system message.
(Assignee)

Updated

6 years ago
Component: DOM: Device Interfaces → Bluetooth
Product: Core → Boot2Gecko
(Assignee)

Comment 3

6 years ago
Created attachment 734534 [details] [diff] [review]
Patch 1(v1): Add event handlers 'onpairedstatuschanged' and 'onhfpstatuschanged'

Add two event handlers in nsIDOMBluetoothAdapter:
- onpairedstatuschanged
- onhfpstatuschanged
Assignee: nobody → gyeh
Attachment #734534 - Flags: review?(echou)
(Assignee)

Comment 4

6 years ago
Created attachment 734536 [details] [diff] [review]
Patch 2(v1): Distribute signal "PairedStatusChanged" to BluetoothAdatper

Let's keep the original implementation (system message) for now, and remove them when Gaia is ready for this change.
Attachment #734536 - Flags: review?(echou)
(Assignee)

Comment 5

6 years ago
Created attachment 734537 [details] [diff] [review]
Patch 3(v1): Distribute signal "HfpStatusChanged" to BluetoothAdatper

Still keep the original approach (system message) for now. Will remove them when Gaia land corresponding patches.
Attachment #734537 - Flags: review?(echou)
Comment on attachment 734534 [details] [diff] [review]
Patch 1(v1): Add event handlers 'onpairedstatuschanged' and 'onhfpstatuschanged'

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

::: dom/bluetooth/nsIDOMBluetoothStatusChangedEvent.idl
@@ +20,5 @@
> +dictionary BluetoothStatusChangedEventInit : EventInit
> +{
> +  DOMString address;
> +  bool status;
> +}; 

nit: trailing whitespace
Attachment #734534 - Flags: review?(echou) → review+
Hi Gina,

Sorry for late reply. For the other two patches, we need to rebase them onto current codebase. Please let me know after revision. Thanks.
(Assignee)

Comment 8

5 years ago
Created attachment 764658 [details] [diff] [review]
Patch 1(v2): Implement DispatchStatusChangedEvent()
Attachment #734534 - Attachment is obsolete: true
Attachment #734536 - Attachment is obsolete: true
Attachment #734537 - Attachment is obsolete: true
Attachment #734536 - Flags: review?(echou)
Attachment #734537 - Flags: review?(echou)
Attachment #764658 - Flags: review?(echou)
(Assignee)

Comment 9

5 years ago
Created attachment 764659 [details] [diff] [review]
Patch 2(v2): Distribute signal "PairedStatusChanged" to BluetoothAdatper
Attachment #764659 - Flags: review?(echou)
(Assignee)

Comment 10

5 years ago
Created attachment 764660 [details] [diff] [review]
Patch 3(v2): Distribute signal "HfpStatusChanged" and "ScoStatusChanged" to BluetoothAdatper
Attachment #764660 - Flags: review?(echou)
(Assignee)

Comment 11

5 years ago
Created attachment 764661 [details] [diff] [review]
Patch 4(v2): Distribute signal "A2dpStatusChanged" to BluetoothAdatper
Attachment #764661 - Flags: review?(echou)
(Assignee)

Comment 12

5 years ago
Created attachment 764662 [details] [diff] [review]
Patch 5(v2): Add event handlers for each Bluetooth profile
Attachment #764662 - Flags: review?(echou)
(Assignee)

Comment 13

5 years ago
Hi mrbkap, 

For current implementation, we define a system message topic/type for each profile, and broadcast system message whenever the connection status of profile changes.

The major change of these patches are that we add new event handlers and these events will be dispatched whenever the connection status changes.

With event handlers, one of advantages is that these information are clearly defined in our API and App/Web developers can find these information easily. Right?

Please let me know if you have any concern about this issue. Thanks. :)
(Assignee)

Updated

5 years ago
Flags: needinfo?(mrbkap)
Attachment #764658 - Flags: review?(echou) → review+
(In reply to Gina Yeh [:gyeh] [:ginayeh] from comment #13)
> With event handlers, one of advantages is that these information are clearly
> defined in our API and App/Web developers can find these information easily.
> Right?

Hey, sorry for the delay here. This is one advantage. The only disadvantage is if the event is important even if the settings application (or whatever other application cares about the event) isn't open the event will be lost. So as long as that's OK, I don't see a problem here.
Flags: needinfo?(mrbkap)
(Assignee)

Comment 15

5 years ago
Thanks, mrbkap. Let me check with Evelyn/Arthur.
(Assignee)

Comment 16

5 years ago
Created attachment 783052 [details] [diff] [review]
Patch1(v3): Distribute connection/pair status to BluetoothAdapter
Attachment #764658 - Attachment is obsolete: true
Attachment #764659 - Attachment is obsolete: true
Attachment #764660 - Attachment is obsolete: true
Attachment #764661 - Attachment is obsolete: true
Attachment #764659 - Flags: review?(echou)
Attachment #764660 - Flags: review?(echou)
Attachment #764661 - Flags: review?(echou)
Attachment #783052 - Flags: review?(echou)
(Assignee)

Comment 17

5 years ago
Created attachment 783053 [details] [diff] [review]
Patch 2(v3): Distribute events from BluetoothAdapter
Attachment #764662 - Attachment is obsolete: true
Attachment #764662 - Flags: review?(echou)
Attachment #783053 - Flags: review?(echou)
(Assignee)

Comment 18

5 years ago
(In reply to Blake Kaplan (:mrbkap) from comment #14)
> Hey, sorry for the delay here. This is one advantage. The only disadvantage
> is if the event is important even if the settings application (or whatever
> other application cares about the event) isn't open the event will be lost.
> So as long as that's OK, I don't see a problem here.

Per Arthur, it should be okay for these cases. So, let's keep it going.

I'll file follow-ups for Settings app and Dialer app. After migrating to event handlers, I'll file another follow-up to remove system messages.
(Assignee)

Comment 19

5 years ago
Comment on attachment 783053 [details] [diff] [review]
Patch 2(v3): Distribute events from BluetoothAdapter

smaug, please help to review this patch. A new event is created. Event generator is used, and I added both IDL and WebIDL.

mrbkap, please help to review this patch. Thanks.
Attachment #783053 - Flags: superreview?(mrbkap)
Attachment #783053 - Flags: review?(echou)
Attachment #783053 - Flags: review?(bugs)
Comment on attachment 783053 [details] [diff] [review]
Patch 2(v3): Distribute events from BluetoothAdapter

nsIDOMBluetoothStatusChangedEvent.idl is missing. r- because of that.

looks like in the patch 1 you use CamelCase for certain strings, but
events use lowercase. I think using lowercase everywhere would make more sense.
Attachment #783053 - Flags: review?(bugs) → review-
(Assignee)

Comment 21

5 years ago
(In reply to Olli Pettay [:smaug] from comment #20)
> Comment on attachment 783053 [details] [diff] [review]
> Patch 2(v3): Distribute events from BluetoothAdapter
> 
> nsIDOMBluetoothStatusChangedEvent.idl is missing. r- because of that.

Oops! Let me update the patch tmw morning. Thanks.

> 
> looks like in the patch 1 you use CamelCase for certain strings, but
> events use lowercase. I think using lowercase everywhere would make more
> sense.

Okay. I will also update patch 1.
Comment on attachment 783053 [details] [diff] [review]
Patch 2(v3): Distribute events from BluetoothAdapter

I'll look at the updated patch.
Attachment #783053 - Flags: superreview?(mrbkap)
(Assignee)

Comment 23

5 years ago
Created attachment 783505 [details] [diff] [review]
Patch 1(v4): Distribute connection/pair status to BluetoothAdapter

Eric, please help to review this patch. Thanks.
Attachment #783052 - Attachment is obsolete: true
Attachment #783052 - Flags: review?(echou)
Attachment #783505 - Flags: review?(echou)
(Assignee)

Comment 24

5 years ago
Created attachment 783508 [details] [diff] [review]
Patch 2(v4): Distribute events from BluetoothAdapter

Patch is updated. Please help to review it. Thanks.
Attachment #783053 - Attachment is obsolete: true
Attachment #783508 - Flags: superreview?(mrbkap)
Attachment #783508 - Flags: review?(bugs)
Comment on attachment 783505 [details] [diff] [review]
Patch 1(v4): Distribute connection/pair status to BluetoothAdapter

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

Looks good to me. Please file a followup for removing the implementation of broadcasting 'bluetooth*statuschanged' system messages after Gaia work is done. Thank you.

::: dom/bluetooth/linux/BluetoothDBusService.cpp
@@ +1481,3 @@
>      BluetoothNamedValue& property = v.get_ArrayOfBluetoothNamedValue()[0];
>      if (property.name().EqualsLiteral("Paired")) {
> +      // Original approach: Broadcast sending system message of

nit: 'sending' seems unnecessary.
Attachment #783505 - Flags: review?(echou) → review+
Attachment #783508 - Flags: superreview?(mrbkap) → superreview+
(Assignee)

Comment 26

5 years ago
Created attachment 784723 [details] [diff] [review]
[Final] Patch 1: Distribute connection/pair status to BluetoothAdapter, r=echou
Attachment #783505 - Attachment is obsolete: true
(Assignee)

Comment 27

5 years ago
Created attachment 784725 [details] [diff] [review]
[Final] Patch 2: Distribute events from BluetoothAdapter, r=smaug, sr=mrbkap
Attachment #783508 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Blocks: 900870
(Assignee)

Updated

5 years ago
Blocks: 900874
(Assignee)

Updated

5 years ago
Blocks: 900880
(Assignee)

Updated

5 years ago
No longer blocks: 900880
(Assignee)

Comment 28

5 years ago
Created attachment 784912 [details] [diff] [review]
[Final] Patch 1: Distribute connection/pair status to BluetoothAdapter, r=echou
Attachment #784723 - Attachment is obsolete: true
(Assignee)

Comment 30

5 years ago
Created attachment 784914 [details] [diff] [review]
[Final] Patch 2: Distribute events from BluetoothAdapter, r=smaug, sr=mrbkap
Attachment #784725 - Attachment is obsolete: true
You need to log in before you can comment on or make changes to this bug.