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

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

[Philipp von Weitershausen [:philikon]]

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.

Comment 1

[Philipp von Weitershausen [:philikon]]

Attachment: v1

Comment 2

[Vicamo Yang [:vicamo][:vyang]]

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

Comment 3

[Philipp von Weitershausen [:philikon]]

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

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

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

Comment 5

[Philipp von Weitershausen [:philikon]]

Attachment: v1.1 r=hsinyi

Nit addressed. Thanks for the review, Hsinyi!

Comment 6

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0c18c15c4985

Comment 7

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/0c18c15c4985

Comment 8

[Philipp von Weitershausen [:philikon]]

Attachment: follow-up fix r=fabrice

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

Comment 9

[Philipp von Weitershausen [:philikon]]

Attachment: patch for aurora/beta

[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

Comment 10

[Alex Keybl [:akeybl]]

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

Comment 11

[Andrew Overholt [:overholt]]

Philipp told me on IRC that he'll investigate the perf benefits and re-nom if it's significant enough for uplift.

Comment 12

[Gene Lian (I already quit Mozilla)]

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+!

Comment 13

[Gene Lian (I already quit Mozilla)]

Please see bug 830425, comment #25.

Comment 14

[Gene Lian (I already quit Mozilla)]

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.

Comment 15

[Alex Keybl [:akeybl]]

Blocks a cert blocker. Please only land after verifying that this is not the cause of bug 818623, or after having resolved that regression.

Comment 16

[Gene Lian (I already quit Mozilla)]

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 17

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

Attachment: patch rebased on latest hg-b2g18

Comment 18

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

Comment on attachment 705714 [details] [diff] [review]
patch rebased on latest hg-b2g18

Oops, typo in the description :P

Comment 19

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

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

Comment 20

[Gene Lian (I already quit Mozilla)]

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

Comment 21

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

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

Comment 22

[Gene Lian (I already quit Mozilla)]

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

Comment 23

[Gene Lian (I already quit Mozilla)]

Comment on attachment 705714 [details] [diff] [review]
patch rebased on latest hg-b2g18

https://hg.mozilla.org/releases/mozilla-b2g18/rev/49c25ca40020

Comment 24

[Lukas Blakk [:lsblakk] use ?needinfo]

Landed on mozilla-b2g18 prior to the 1/25 branching to mozilla-b2g18_v1_0_0, updating status-b2g-v1.0.0