Closed Bug 1081811 Opened 5 years ago Closed 5 years ago

[B2G][Telephony] correct the usage of 'callschanged' event

Categories

(Firefox OS Graveyard :: RIL, defect)

x86_64
Linux
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
2.1 S7 (24Oct)

People

(Reporter: hsinyi, Assigned: bhsu)

References

Details

Attachments

(2 files, 8 obsolete files)

With Bug 1036851 and given the fact that there's no reference to callschanged for the 'ready' notification, it's time to remove dummy 'callschanged' event firing.
Ben, do you want to try this? :P
Flags: needinfo?(bhsu)
Assignee: nobody → bhsu
Flags: needinfo?(bhsu)
Oringinal, there are only 'callschanged' events, and a 'callschanged' event is fired when one of the three belowing conditions occurs. With Bug 1036851, the former two conditions also fire 'ready' event.

(1). When the telephony object is ready.
(2). When adding a new listener to 'callschanged' events.
(3). When the number of calls changes.

Currently, I seperate 'callschanged' event and 'ready' event, and  make 'callschanged' be fired only when condition(3), while the other two conditions are notified only by 'ready' event. However, when tracing some related Gaia Apps, I found that 'ready' events haven't been adopted in current design. After discussing with my mentor, we think applying this modification will make some Gaia codes misbehave, since they possibly rely on 'callschanged' event to do something (e.g., initializing) when the former two conditions happen. Thus, if applying this modification, we suggest those codes to listen to 'ready' events as well, and modifcations of current 'callschanged' event handler may be needed.
Flags: needinfo?(htsai)
Flags: needinfo?(drs.bugzilla)
Thanks Ben! My comment 0 "there's no reference to callschanged for the 'ready' notification" is coming from the understanding of comments of bug 1048680 but not sure if something changes during the period. I'll defer the NI to Doug and see if we need to do something on gaia first, thank you :)
Flags: needinfo?(htsai)
(In reply to Hsin-Yi Tsai [:hsinyi] from comment #3)
> Thanks Ben! My comment 0 "there's no reference to callschanged for the
> 'ready' notification" is coming from the understanding of comments of bug
> 1048680 but not sure if something changes during the period. I'll defer the
> NI to Doug and see if we need to do something on gaia first, thank you :)

For example:

https://github.com/mozilla-b2g/gaia/blob/bc8be5081676ffed4cf9c2c9c53fc48caa0f1a49/apps/callscreen/js/calls_handler.js#L21

I guess this part might have some problem if we only fire "callschange" on condition (3). The CallsHandler will lose the chance to init and handle the existing calls.
Attachment #8506795 - Flags: review?(szchen)
Attached patch Part 2 Modify related test cases (obsolete) — Splinter Review
Attachment #8506796 - Flags: review?(szchen)
Comment on attachment 8506795 [details] [diff] [review]
Part 1 Remove some callschanged event firing

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

Thank you for the first patch.
I found some existing code that could be removed. Please see my comment.

::: dom/telephony/Telephony.cpp
@@ +473,1 @@
>      EnqueueEnumerationAck(NS_LITERAL_STRING("ready"));

Since this is the only place we use EnqueueEnumerationAck(), the parameter for the function is  unnecessary. I mean the value will always be "ready" so we can simply remove it and directly write it in the function body of EnqueueEnumerationAck().

With the same idea, you can also remove some parameter and variable in EnumerationAck.
Attachment #8506795 - Flags: review?(szchen)
(In reply to Ben Hsu [:benhsu] from comment #2)
> Currently, I seperate 'callschanged' event and 'ready' event, and  make
> 'callschanged' be fired only when condition(3), while the other two
> conditions are notified only by 'ready' event. However, when tracing some
> related Gaia Apps, I found that 'ready' events haven't been adopted in
> current design. After discussing with my mentor, we think applying this
> modification will make some Gaia codes misbehave, since they possibly rely
> on 'callschanged' event to do something (e.g., initializing) when the former
> two conditions happen. Thus, if applying this modification, we suggest those
> codes to listen to 'ready' events as well, and modifcations of current
> 'callschanged' event handler may be needed.

Thanks, I filed bug 1085007 to deal with this. I currently don't expect there to be any problems as I don't think we're abusing the 'callschanged' event for initialization. When this patch is further along, I'll give it a try and make sure it doesn't obviously break anything.
Flags: needinfo?(drs.bugzilla)
Attached patch Remove unnecessary parameters (obsolete) — Splinter Review
Attachment #8507597 - Flags: review?(szchen)
Attachment #8507597 - Attachment is obsolete: true
Attachment #8507597 - Flags: review?(szchen)
Attachment #8507602 - Flags: review?(szchen)
Attachment #8507602 - Attachment is obsolete: true
Attachment #8507602 - Flags: review?(szchen)
Attachment #8507606 - Flags: review?(szchen)
(In reply to Szu-Yu Chen [:aknow] from comment #7)
> I found some existing code that could be removed. Please see my comment.

Thanks for your advice. I've removed the unnecessary parameters, and the new patch is currently being tested. Can you review the new patch?
Comment on attachment 8507606 [details] [diff] [review]
Part 3 Remove unnecessary parameters

Looks good. Suggest you to merge part 3 patch into part 1.
Attachment #8507606 - Flags: review?(szchen) → review+
Attachment #8506795 - Flags: review?(szchen)
Attachment #8506795 - Attachment is obsolete: true
Attachment #8507606 - Attachment is obsolete: true
Attachment #8506795 - Flags: review?(szchen)
Attachment #8507761 - Flags: review?(szchen)
Attachment #8507761 - Attachment is obsolete: true
Attachment #8507761 - Flags: review?(szchen)
Attachment #8507766 - Flags: review?(szchen)
(In reply to Szu-Yu Chen [:aknow] from comment #13)
> Looks good. Suggest you to merge part 3 patch into part 1.

Thanks again. I've uploaded the merged patch.
Attachment #8507766 - Flags: review?(szchen) → review+
Attachment #8506796 - Attachment is obsolete: true
Attachment #8506796 - Flags: review?(szchen)
Attachment #8509153 - Flags: review?(szchen)
Comment on attachment 8509153 [details] [diff] [review]
Part 2 Modify related test cases.V2

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

Thank you.

r=me with comment addressed

::: dom/telephony/test/marionette/head.js
@@ +266,5 @@
>    function check_oncallschanged(container, containerName, expectedCalls,
>                                  callback) {
>      container.oncallschanged = function(event) {
>        log("Received 'callschanged' event for the " + containerName);
> +    

nit: remove the trailing whitespace
Attachment #8509153 - Flags: review?(szchen) → review+
Attachment #8509153 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/e0388be11c15
https://hg.mozilla.org/mozilla-central/rev/3be2979fc273
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → 2.1 S7 (24Oct)
Congrats for the 1st patch, Ben :D
Please mark a review on the patch(es) that landed. Thanks.
Duplicate of this bug: 1086189
You need to log in before you can comment on or make changes to this bug.