Closed Bug 812368 Opened 7 years ago Closed 7 years ago

B2G RIL: dispatch 'telephony-new-call' system message directly after receiving UNSOLICITED_CALL_RING from rild

Categories

(Core :: DOM: Device Interfaces, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla20
blocking-b2g tef+
blocking-basecamp -
Tracking Status
firefox19 --- wontfix
firefox20 --- wontfix
firefox21 --- fixed
b2g18 --- fixed
b2g18-v1.0.0 --- fixed

People

(Reporter: philikon, Assigned: philikon)

References

Details

Attachments

(4 files, 1 obsolete file)

We currently wait for a call state change to occur and then analyse the call state before dispatching the 'telephony-new-call' system message (which launches the Phone app). As shown in bug 812059, we could do this sooner as the modem alerts us of an incoming call via a UNSOLICITED_CALL_RING parcel several hundred milliseconds earlier.
Assignee: nobody → philipp
Attached patch v1 (obsolete) — Splinter Review
Attachment #682482 - Flags: review?(htsai)
(In reply to Philipp von Weitershausen [:philikon] from comment #1)
> Created attachment 682482 [details] [diff] [review]
> v1

Actually, I think this might have two reasons: 1) Unagi/Otoro is so slow that default 1sec wake lock for unsolicited is not long enough to complete everything 2) We don't hold an additional wake lock during querying call states. The first one can be and should be fixed in rild as Gene's detailed comments, and the second one is something we should really do to solve similar problems.
(In reply to Vicamo Yang [:vicamo][:vyang] from comment #2)
> Actually, I think this might have two reasons: 1) Unagi/Otoro is so slow
> that default 1sec wake lock for unsolicited is not long enough to complete
> everything 2) We don't hold an additional wake lock during querying call
> states. The first one can be and should be fixed in rild as Gene's detailed
> comments, and the second one is something we should really do to solve
> similar problems.

Yes, all good points. But even so, we can send the system message at UNSOLICITED_CALL_RING. I don't see a reason not to. Even if we figure out the wakelock situation. Then we're just saving less, but we're still saving.
Comment on attachment 682482 [details] [diff] [review]
v1

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

Looks great, thanks!

::: dom/system/gonk/RadioInterfaceLayer.js
@@ +1126,5 @@
>    },
>  
>    /**
> +   * Handle an incoming call.
> +   * 

nit: No whitespace
Attachment #682482 - Flags: review?(htsai) → review+
Attached patch v1.1 r=hsinyiSplinter Review
Nit addressed. Thanks for the review, Hsinyi!
Attachment #682482 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/0c18c15c4985
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
Gah, I trusted the Marionette tests, but I forgot that they don't actually exercise the system messages. And sure enough, incoming calls were broken. Sorry about that!

Pushed follow-up fix that was IRC-reviewed by Fabrice:

https://hg.mozilla.org/mozilla-central/rev/58a4e9501f7e
Attachment #686202 - Flags: checkin+
[Approval Request Comment]
Bug caused by (feature/regressing bug #): 812059 
User impact if declined: potentially slower response to incoming phone calls
Testing completed (on m-c, etc.): landed on m-c today
Risk to taking this patch (and alternatives if risky): incoming phone calls and phone calls programmatically placed by the STK might be affected. Alternative is not to commit the patch.
String or UUID changes made by this patch: none
Attachment #686211 - Flags: approval-mozilla-beta?
Attachment #686211 - Flags: approval-mozilla-aurora?
(In reply to Philipp von Weitershausen [:philikon] from comment #9)
> Risk to taking this patch (and alternatives if risky): incoming phone calls
> and phone calls programmatically placed by the STK might be affected.
> Alternative is not to commit the patch.

Given the possible risk of regression here, and the fact that this hasn't been a major complaint from testers/partners, we'll only uplift if this ends up being b-b+.
blocking-basecamp: --- → ?
Attachment #686211 - Flags: approval-mozilla-beta?
Attachment #686211 - Flags: approval-mozilla-beta-
Attachment #686211 - Flags: approval-mozilla-aurora?
Attachment #686211 - Flags: approval-mozilla-aurora-
Philipp told me on IRC that he'll investigate the perf benefits and re-nom if it's significant enough for uplift.
blocking-basecamp: ? → -
Please see bug 830425, comment #c25. Highly suggesting we should check in this into b2g18! With the patches both at bug 830425 and here, we can have 1 doo-doo sound left in the long run. Re-nominating tef+!
blocking-b2g: --- → tef?
Blocks: 830425
No longer depends on: 830425
Note that we have to be very careful to check this in into b2g18, because we have a critical bug happening only on m-c but not on b2g18 that we cannot make a second call. *Not sure* if it's due to this patch. Please see bug 818623, comment #18 and bug 818623, comment #19.
Blocks a cert blocker. Please only land after verifying that this is not the cause of bug 818623, or after having resolved that regression.
blocking-b2g: tef? → tef+
Hi Philikon or Hsin-Yi,

Do you guys have time to uplift the patch for b2g-18? I think it's safer to let you do it by yourselves since you are the designer and reviewer. Thanks!
Comment on attachment 705714 [details] [diff] [review]
patch rebased on latest hg-b2g18

Oops, typo in the description :P
Attachment #705714 - Attachment description: patch rebased on latest hg-b2g19 → patch rebased on latest hg-b2g18
(In reply to Gene Lian [:gene] [:clian] from comment #16)
> Hi Philikon or Hsin-Yi,
> 
> Do you guys have time to uplift the patch for b2g-18? I think it's safer to
> let you do it by yourselves since you are the designer and reviewer. Thanks!

I've attached the after-rebasing patch, attachment 705714 [details] [diff] [review]. Unfortunately, after applying this to git-gecko-18 branch, bug 818623 happens... 

Agree with comment 15, we shouldn't land it right away.
(In reply to Hsin-Yi Tsai [:hsinyi] from comment #19)
> I've attached the after-rebasing patch, attachment 705714 [details] [diff] [review]
> [review]. Unfortunately, after applying this to git-gecko-18 branch, bug
> 818623 happens...

Oops... bad to hear that. It seems my first guess is right, unfortunately.

Philikon, do you have time to re-investigate your patch? We cannot make a second call by the m-c codes. Need to fix that before landing this to b2g18. If the patch is reasonably right, then we should ask for a Gaia-solution at bug 818623.
(In reply to Gene Lian [:gene] [:clian] from comment #20)
> (In reply to Hsin-Yi Tsai [:hsinyi] from comment #19)
> > I've attached the after-rebasing patch, attachment 705714 [details] [diff] [review]
> > [review]. Unfortunately, after applying this to git-gecko-18 branch, bug
> > 818623 happens...
> 
> Oops... bad to hear that. It seems my first guess is right, unfortunately.
> 
> Philikon, do you have time to re-investigate your patch? We cannot make a
> second call by the m-c codes. Need to fix that before landing this to b2g18.
> If the patch is reasonably right, then we should ask for a Gaia-solution at
> bug 818623.

Gene,
I think Philikon's path here is right. The problem of bug 818623 is that dialer will close callscreen at the wrong time. Since callscreen is the one who displays the call info and takes care of incoming ring tone, we have no idea to answer the call with callscreen closed. And why this path leads to that problem? Probably because this patch launches the dialer earlier, corner timing.
(In reply to Hsin-Yi Tsai [:hsinyi] from comment #21)
> Gene,
> And why this path leads to
> that problem? Probably because this patch launches the dialer earlier,
> corner timing.

Thanks for investigating! Well done! Launching Dialer app earlier is definitely good! That's *exactly* what we want to solve for this issue from the point of view of platform! No reason Gaia cannot handle that. Indeed, it should be a Gaia issue.
Landed on mozilla-b2g18 prior to the 1/25 branching to mozilla-b2g18_v1_0_0, updating status-b2g-v1.0.0
You need to log in before you can comment on or make changes to this bug.