Closed Bug 1036851 Opened 5 years ago Closed 5 years ago

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

Categories

(Firefox OS Graveyard :: RIL, defect)

x86_64
Linux
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
2.1 S1 (1aug)

People

(Reporter: aknow, Assigned: aknow)

References

Details

(Keywords: dev-doc-needed)

Attachments

(3 files, 6 obsolete files)

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.
Attached patch Part 1: Add ready event (webidl) (obsolete) — Splinter Review
Attachment #8453648 - Flags: review?(htsai)
Attached patch Part 2: Add ready event (dom) (obsolete) — Splinter Review
Attachment #8453649 - Flags: review?(htsai)
Attachment #8453650 - Flags: review?(htsai)
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.
(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.
Nope, that solves my concern, thanks.
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.
Attachment #8453648 - Flags: review?(htsai) → review+
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...
Attachment #8453649 - Flags: review?(htsai)
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?
Attachment #8453650 - Flags: review?(htsai) → review+
Attached patch Part 2#2: Add ready event (dom) (obsolete) — Splinter Review
Attachment #8453649 - Attachment is obsolete: true
Attachment #8459425 - Flags: review?(htsai)
Test telephony in another iframe.
Attachment #8453650 - Attachment is obsolete: true
Attachment #8459456 - Flags: review?(htsai)
Attachment #8459425 - Flags: review?(htsai) → review+
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 :)
Attachment #8459456 - Flags: review?(htsai) → review+
Attachment #8459425 - Attachment is obsolete: true
Attachment #8459963 - Flags: review+
Attachment #8459456 - Attachment is obsolete: true
Attachment #8459964 - Flags: review+
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=...
Keywords: checkin-needed
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?
Attachment #8459961 - Flags: review+ → review?(khuey)
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.
Keywords: checkin-needed
Target Milestone: --- → 2.1 S1 (1aug)
Blocks: 1048680
Blocks: 1100895
Duplicate of this bug: 1008104
You need to log in before you can comment on or make changes to this bug.