Closed Bug 853221 Opened 7 years ago Closed 7 years ago

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

Categories

(Firefox OS Graveyard :: Bluetooth, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: gyeh, Assigned: gyeh)

References

Details

Attachments

(2 files, 14 obsolete files)

13.76 KB, patch
Details | Diff | Splinter Review
9.26 KB, patch
Details | Diff | Splinter Review
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.
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.
Component: DOM: Device Interfaces → Bluetooth
Product: Core → Boot2Gecko
Add two event handlers in nsIDOMBluetoothAdapter:
- onpairedstatuschanged
- onhfpstatuschanged
Assignee: nobody → gyeh
Attachment #734534 - Flags: review?(echou)
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)
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.
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)
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. :)
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)
Thanks, mrbkap. Let me check with Evelyn/Arthur.
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)
Attachment #764662 - Attachment is obsolete: true
Attachment #764662 - Flags: review?(echou)
Attachment #783053 - Flags: review?(echou)
(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 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-
(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)
Eric, please help to review this patch. Thanks.
Attachment #783052 - Attachment is obsolete: true
Attachment #783052 - Flags: review?(echou)
Attachment #783505 - Flags: review?(echou)
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: review?(bugs) → review+
Attachment #783508 - Flags: superreview?(mrbkap) → superreview+
Blocks: 900870
Blocks: 900874
Blocks: 900880
No longer blocks: 900880
You need to log in before you can comment on or make changes to this bug.