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

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Bluetooth, defect)

Description

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

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".

Comment 1

[[e-mail], [e-mail]]

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.

Comment 2

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

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.

Comment 3

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

Attachment: Patch 1(v1): Add event handlers 'onpairedstatuschanged' and 'onhfpstatuschanged'

Add two event handlers in nsIDOMBluetoothAdapter:
- onpairedstatuschanged
- onhfpstatuschanged

Comment 4

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

Attachment: 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.

Comment 5

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

Attachment: 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.

Comment 6

[Eric Chou [:ericchou] [: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

Comment 7

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

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.

Comment 8

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

Attachment: Patch 1(v2): Implement DispatchStatusChangedEvent()

Comment 9

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

Attachment: Patch 2(v2): Distribute signal "PairedStatusChanged" to BluetoothAdatper

Comment 10

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

Attachment: Patch 3(v2): Distribute signal "HfpStatusChanged" and "ScoStatusChanged" to BluetoothAdatper

Comment 11

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

Attachment: Patch 4(v2): Distribute signal "A2dpStatusChanged" to BluetoothAdatper

Comment 12

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

Attachment: Patch 5(v2): Add event handlers for each Bluetooth profile

Comment 13

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

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. :)

Comment 14

[Blake Kaplan (:mrbkap) (inactive)]

(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.

Comment 15

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

Thanks, mrbkap. Let me check with Evelyn/Arthur.

Comment 16

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

Attachment: Patch1(v3): Distribute connection/pair status to BluetoothAdapter

Comment 17

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

Attachment: Patch 2(v3): Distribute events from BluetoothAdapter

Comment 18

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

(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.

Comment 19

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

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.

Comment 20

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 21

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

(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 22

[Blake Kaplan (:mrbkap) (inactive)]

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

I'll look at the updated patch.

Comment 23

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

Attachment: Patch 1(v4): Distribute connection/pair status to BluetoothAdapter

Eric, please help to review this patch. Thanks.

Comment 24

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

Attachment: Patch 2(v4): Distribute events from BluetoothAdapter

Patch is updated. Please help to review it. Thanks.

Comment 25

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

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.

Comment 26

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

Attachment: [Final] Patch 1: Distribute connection/pair status to BluetoothAdapter, r=echou

Comment 27

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

Attachment: [Final] Patch 2: Distribute events from BluetoothAdapter, r=smaug, sr=mrbkap

Comment 28

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

Attachment: [Final] Patch 1: Distribute connection/pair status to BluetoothAdapter, r=echou

Comment 29

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

try: 
https://tbpl.mozilla.org/?tree=Try&rev=99a6330bd1fd
https://tbpl.mozilla.org/?tree=Try&rev=e15c42d981ed

Comment 30

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

Attachment: [Final] Patch 2: Distribute events from BluetoothAdapter, r=smaug, sr=mrbkap

Comment 31

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

https://hg.mozilla.org/integration/b2g-inbound/rev/989eeb2e71ff
https://hg.mozilla.org/integration/b2g-inbound/rev/cf744991c7e0

Comment 32

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/989eeb2e71ff
https://hg.mozilla.org/mozilla-central/rev/cf744991c7e0