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

Status: RESOLVED FIXED

(Firefox OS Graveyard :: RIL, defect)

Description

[Hsin-Yi Tsai (she/her) [:hsinyi]]

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.

Comment 1

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Ben, do you want to try this? :P

Comment 2

[Ben Hsu [:HoPang]]

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.

Comment 3

[Hsin-Yi Tsai (she/her) [:hsinyi]]

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

Comment 4

[Szu-Yu Chen [:aknow] (inactive)]

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

Comment 5

[Ben Hsu [:HoPang]]

Attachment: Part 1 Remove some callschanged event firing

Comment 6

[Ben Hsu [:HoPang]]

Attachment: Part 2 Modify related test cases

Comment 7

[Szu-Yu Chen [:aknow] (inactive)]

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.

Comment 8

[Doug Sherk (:drs) (inactive)]

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

Comment 9

[Ben Hsu [:HoPang]]

Attachment: Remove unnecessary parameters

Comment 10

[Ben Hsu [:HoPang]]

Attachment: Part 3 Remove unnecassry parameters

Comment 11

[Ben Hsu [:HoPang]]

Attachment: Part 3 Remove unnecessary parameters

Comment 12

[Ben Hsu [:HoPang]]

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

[Szu-Yu Chen [:aknow] (inactive)]

Comment on attachment 8507606 [details] [diff] [review]
Part 3 Remove unnecessary parameters

Looks good. Suggest you to merge part 3 patch into part 1.

Comment 14

[Ben Hsu [:HoPang]]

Attachment: Bug 1081811 - Part 1 Remove some callschanged event firing and associated parameters

Comment 15

[Ben Hsu [:HoPang]]

Attachment: Part 1 Remove some callschanged event firing and associated parameters

Comment 16

[Ben Hsu [:HoPang]]

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

Comment 17

[Ben Hsu [:HoPang]]

Attachment: Part 2 Modify related test cases.V2

Comment 18

[Szu-Yu Chen [:aknow] (inactive)]

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

Comment 19

[Ben Hsu [:HoPang]]

Attachment: [final] Part 1 Remove some callschanged event firing and associated parameters. r=aknow

Comment 20

[Ben Hsu [:HoPang]]

Attachment: [final] Part 2 Modify related test cases. r=aknow

Comment 21

[Ben Hsu [:HoPang]]

https://tbpl.mozilla.org/?tree=Try&rev=6fd1f01324f5

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/e0388be11c15
https://hg.mozilla.org/integration/b2g-inbound/rev/3be2979fc273

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/e0388be11c15
https://hg.mozilla.org/mozilla-central/rev/3be2979fc273

Comment 24

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Congrats for the 1st patch, Ben :D

Comment 25

[Asa Dotzler [:asa]]

Please mark a review on the patch(es) that landed. Thanks.