1036851 - Add 'ready' event to telephony to signal the completion of initialization

Status: RESOLVED FIXED

(Firefox OS Graveyard :: RIL, defect)

Description

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

Originally, we overload the meaning of 'callschanged' event to indicate that the telephony object is fully initialized. However, I think that it might be misleading and cause some side effects. For example, sometimes the event means the real 'callschanged', but sometimes it is used for the above special meaning. Thus, it is possible for an user to receive 'callscahgned' event without any calls changes. It could confuse the api user.

For the reason, I am going to add a new event 'ready' to directly convey the information. So in order to check whether the telephony is ready, we should listen to 'ready' event.

Use case:

1. If the telephony is not ready, the listener will receive 'ready' event as soon as the telephony finishes the initialization.

2. If the telephony is ready, the listener will receive the event immediately.

Note that in order to achieve 2, it is possible for one listener to receive the event more than once. Be aware of that and we suggest the users to remove the listener once they could make sure the telephony is ready.

Comment 1

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

Attachment: Part 1: Add ready event (webidl)

Comment 2

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

Attachment: Part 2: Add ready event (dom)

Comment 3

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

Attachment: Part 3: Add test case for telephony.onready

Comment 4

[Anthony Ricaud (:rik)]

Can I suggest using a promise rather than an event? When I see bug 1033943, I think we might want to disable call buttons until we can place a call. So at startup, Dialer would ask if the telephony is ready. But if the telephony was ready before Dialer was launched, we'd never receive the event. With a promise, it is possible to query the readiness at any time.

Comment 5

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

(In reply to Anthony Ricaud (:rik) from comment #4)
> Can I suggest using a promise rather than an event? When I see bug 1033943,
> I think we might want to disable call buttons until we can place a call. 

Hey :rik,

I think this is another issue.
The issue reported on bug 1033943 is that call can't be made because network registration isn't connected, not because Telephony service isn't ready. That said, at that moment, emergency call could be dialed out without problems.
 
> So
> at startup, Dialer would ask if the telephony is ready. But if the telephony
> was ready before Dialer was launched, we'd never receive the event. 

Aknow's patch guarantees that every time gaia registers an 'onready' listener gaia is guaranteed to get a callback at some point, just as what we currently guarantee |oncallschanged| listener. Is your concern still valid?

> With a
> promise, it is possible to query the readiness at any time.

Comment 6

[Anthony Ricaud (:rik)]

Nope, that solves my concern, thanks.

Comment 7

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

Comment on attachment 8453648 [details] [diff] [review]
Part 1: Add ready event (webidl)

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

It is nice. Once we have agreement on bug 986333, we won't need this anymore.

Comment 8

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

Comment on attachment 8453649 [details] [diff] [review]
Part 2: Add ready event (dom)

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

::: dom/telephony/Telephony.cpp
@@ +695,5 @@
>  Telephony::DispatchCallEvent(const nsAString& aType,
>                               TelephonyCall* aCall)
>  {
> +  // For incoming event, the call should not be null.
> +  MOZ_ASSERT(!aType.EqualsLiteral("incoming") || aType);

The code isn't align with your comment. And the assertion looks wrong...

Comment 9

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

Comment on attachment 8453650 [details] [diff] [review]
Part 3: Add test case for telephony.onready

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

Can you extend the case a bit -- create another iframe after the 1st telephony object is ready, then see if the iframe receives onready or not?

Comment 10

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

Attachment: Part 2#2: Add ready event (dom)

Comment 11

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

Attachment: Part 3#2: Add test case for telephony.onready

Test telephony in another iframe.

Comment 12

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

Comment on attachment 8459456 [details] [diff] [review]
Part 3#2: Add test case for telephony.onready

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

Thank you :)

Comment 13

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

Attachment: [final] Part 1: Add ready event (webidl). r=hsinyi

Comment 14

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

Attachment: [final] Part 2: Add ready event (dom). r=hsinyi

Comment 15

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

Attachment: [final] Part 3: Add test case for telephony.onready. r=hsinyi

Comment 16

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

https://tbpl.mozilla.org/?tree=Try&rev=8a8361cff1a1

Comment 17

[Carsten Book [:Tomcat]]

Hi,

seems part 1 need a dom peer review, since the checkin failed with:

remote: WebIDL file dom/webidl/Telephony.webidl altered in changeset 1abe20dd51fa without DOM peer review
remote: Changes to WebIDL files in this repo require review from a DOM peer in the form of r=...

Comment 18

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

Comment on attachment 8459961 [details] [diff] [review]
[final] Part 1: Add ready event (webidl). r=hsinyi

Hi Kyle,
It seems that we need a dom peer's review on webidl. Could you help it?

Comment 19

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Sorry for the delay.  In the future if you have really easy reviews like this and I'm taking a while a quick email will get these done faster.

Comment 20

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

Attachment: [final] Part 3#2: Add test case for telephony.onready. r=hsinyi, khuey

Comment 21

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/b2g-inbound/rev/1af0c88cf7be
https://hg.mozilla.org/integration/b2g-inbound/rev/d0318ff81a3b
https://hg.mozilla.org/integration/b2g-inbound/rev/f745810d7bac

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/1af0c88cf7be
https://hg.mozilla.org/mozilla-central/rev/d0318ff81a3b
https://hg.mozilla.org/mozilla-central/rev/f745810d7bac