Closed
Bug 853221
Opened 12 years ago
Closed 12 years ago
[b2g-bluetooth] Replace some system messages with event handlers
Categories
(Firefox OS Graveyard :: Bluetooth, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: gyeh, Assigned: gyeh)
References
Details
Attachments
(2 files, 14 obsolete files)
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•12 years ago
|
||
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•12 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•12 years ago
|
Component: DOM: Device Interfaces → Bluetooth
Product: Core → Boot2Gecko
Assignee | ||
Comment 3•12 years ago
|
||
Add two event handlers in nsIDOMBluetoothAdapter:
- onpairedstatuschanged
- onhfpstatuschanged
Assignee: nobody → gyeh
Attachment #734534 -
Flags: review?(echou)
Assignee | ||
Comment 4•12 years ago
|
||
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•12 years ago
|
||
Still keep the original approach (system message) for now. Will remove them when Gaia land corresponding patches.
Attachment #734537 -
Flags: review?(echou)
Comment 6•12 years ago
|
||
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+
Comment 7•12 years ago
|
||
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•12 years ago
|
||
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•12 years ago
|
||
Attachment #764659 -
Flags: review?(echou)
Assignee | ||
Comment 10•12 years ago
|
||
Attachment #764660 -
Flags: review?(echou)
Assignee | ||
Comment 11•12 years ago
|
||
Attachment #764661 -
Flags: review?(echou)
Assignee | ||
Comment 12•12 years ago
|
||
Attachment #764662 -
Flags: review?(echou)
Assignee | ||
Comment 13•12 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•12 years ago
|
Flags: needinfo?(mrbkap)
Updated•12 years ago
|
Attachment #764658 -
Flags: review?(echou) → review+
Comment 14•12 years ago
|
||
(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•12 years ago
|
||
Thanks, mrbkap. Let me check with Evelyn/Arthur.
Assignee | ||
Comment 16•12 years ago
|
||
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•12 years ago
|
||
Attachment #764662 -
Attachment is obsolete: true
Attachment #764662 -
Flags: review?(echou)
Attachment #783053 -
Flags: review?(echou)
Assignee | ||
Comment 18•12 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•12 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 20•12 years ago
|
||
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•12 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 22•12 years ago
|
||
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•12 years ago
|
||
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•12 years ago
|
||
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 25•12 years ago
|
||
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+
Updated•12 years ago
|
Attachment #783508 -
Flags: review?(bugs) → review+
Updated•12 years ago
|
Attachment #783508 -
Flags: superreview?(mrbkap) → superreview+
Assignee | ||
Comment 26•12 years ago
|
||
Attachment #783505 -
Attachment is obsolete: true
Assignee | ||
Comment 27•12 years ago
|
||
Attachment #783508 -
Attachment is obsolete: true
Assignee | ||
Comment 28•12 years ago
|
||
Attachment #784723 -
Attachment is obsolete: true
Assignee | ||
Comment 29•12 years ago
|
||
Assignee | ||
Comment 30•12 years ago
|
||
Attachment #784725 -
Attachment is obsolete: true
Assignee | ||
Comment 31•12 years ago
|
||
Comment 32•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/989eeb2e71ff
https://hg.mozilla.org/mozilla-central/rev/cf744991c7e0
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•