Closed Bug 1227980 Opened 9 years ago Closed 8 years ago

[Dialer] Merge the callscreen app back into the dialer and remove the associated system app hacks

Categories

(Firefox OS Graveyard :: Gaia::Dialer, defect)

Product:
Firefox OS Graveyard
Component:
Gaia::Dialer
Platform:
ARM
Gonk (Firefox OS)
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: gsvelto, Assigned: gsvelto)

References

Details

(Keywords: perf)

Attachments

(2 files, 1 obsolete file)

[gaia] gabrielesvelto:bug-1227980-merge-callscreen-into-dialer > mozilla-b2g:master
46 bytes, text/x-github-pull-request
rickychien
: review+
etienne
: review+
thills
: review+
alwu
: review+
etienne
: feedback+
thills
: feedback+
Details | Review
[PATCH v2] Do not try to hold/resume calls in the wrong state when handling audio channels
2.23 KB, patch
gsvelto
: review+
Details | Diff | Splinter Review 
+++ This bug was initially created as a clone of Bug #1226195 +++

This is the bug I'll use to track the implementation of the user story in bug 1226195.
First WIP patch, still lots of work to do.
Assignee: nobody → gsvelto
Status: NEW → ASSIGNED
I've just pushed the first version of this patch that passes linting & builds correctly. The tests are still broken and I didn't even try to run it yet...
I've just pushed the first version that kinda works. The |callschanged| handler is still racy so the first call might not show up correctly, but the second always does.
Fixed the races when opening the callscreen, closing it is glitchy though. We can briefly see the empty call screen after having hung up the last call and it doesn't properly slide out when it's closed.
As part of this bug I'm planning to change the attention screen behaviour to always slide-to-top when closed. I think this used to be the case before Alive heavily refactored this code but I can't be sure. Marcus, it seems to me that only the clock app would be affected by this when showing alarms. Would it be fine to slide the alarm panel app when closed or do you prefer the current behaviour (it just disappears)? If the current behaviour is desirable then I'll make adjustments so that only the dialer attention window gets the slide-to-top animation.
Flags: needinfo?(m)
A slide sounds fine to me. I'd rather have system-wide consistency than emulating the old behavior. Thanks for asking!
Flags: needinfo?(m)
Thanks for the feedback Marcus! I've pushed a new version with slide-in/out of all attention screens and further fixes (mostly to the keypad which was broken in on-call mode and to the SIM lock screen). I still have to check if the telephony channel is handled correctly and after that I'll proceed to fixing the unit-tests.
I'm almost done checking the last bits of this patch (mostly audio-channel related) and there's only one thing that I think I won't be touching unless there's a good reason to do so. Etienne, in your original patch the way the attention screen visibility was handled changed so that we could have open attention screens which aren't shown (so that the callscreen could stay "hidden" after it was started). This logic was heavily refactored by Alive in bug 983126 and since it works perfectly I don't see an upside to touching it while it's quite likely that I might regress stuff if I mess with it. What do you think?
Flags: needinfo?(etienne)
Keywords: perf
(In reply to Gabriele Svelto [:gsvelto] from comment #9)
> I'm almost done checking the last bits of this patch (mostly audio-channel
> related) and there's only one thing that I think I won't be touching unless
> there's a good reason to do so. Etienne, in your original patch the way the
> attention screen visibility was handled changed so that we could have open
> attention screens which aren't shown (so that the callscreen could stay
> "hidden" after it was started). This logic was heavily refactored by Alive
> in bug 983126 and since it works perfectly I don't see an upside to touching
> it while it's quite likely that I might regress stuff if I mess with it.
> What do you think?

Looks like there's a typo in the bug reference...
We might want to remove it if it goes totally unused but I'll know for sure once I see the refactoring in question :)
Flags: needinfo?(etienne)
Yeah, totally wrong bug. I meant bug 927862.
(In reply to Gabriele Svelto [:gsvelto] from comment #11)
> Yeah, totally wrong bug. I meant bug 927862.

Yes I think it's fine to keep the `displayed` stuff in place for the scope of this bug.
Depends on: 990003
I've got the first working patch ready, it's passing all the unit-tests, a few smoke tests (didn't have time to run all of them yet), is fully lint-clean and does not regress raptor significantly (~15ms or so). This is almost ready for review.
Comment on attachment 8692512 [details] [review]
[gaia] gabrielesvelto:bug-1227980-merge-callscreen-into-dialer > mozilla-b2g:master

Here's the big patch ready for review, but let me make something clear first: this is no Christmas present, in fact I'd love if you'd review this *after* having relaxed, spent some time with family and friends and not thought about how messy our codebase is.

Back to the PR, I've split it in five patches for ease of review. The first two patches are for Tamara, they cover the callscreen code that was moved into the dialer and the related unit-tests, more on this later. The third and fourth patches are for Etienne, they remove all the ugly hacks we had in the system app and adjusts the unit-tests. The last patch is for Ricky and it removes the hacks that we had in the build system to accommodate for the callscreen.

Regarding the first patch almost all files there are simply moved from one dir to another, removed if they're obsolete or received trivial changes (formatting, comments, paths, etc...). The only exceptions are the following:

- communications/dialer/js/call_screen.js
  Only paths to the icons were adjusted were

- communications/dialer/js/calls_handler.js
  Two big changes here, first of all event-handlers were re-instated to receive headset commands from the main dailer window (via postMessage) instead of directly via message handlers. The second change is that we're calling the 'callschanged' event-handler directly when initializing. This is done to prevent races: sometimes the 'callschanged' event would not trigger as when the event-handler is bound it's already too late to detect the first call. Forcing it to run at least once makes this race-proof. It also has the added advantage that we initialize everything *before* telling the dialer we're ready. Before this the callscreen could be shown partially populated (though it was rare).

- communications/dialer/js/dialer.js
  Even though it looks like I've turned this file upside down what really happened is more or less a plain revert of the changes in bug 990003. This boils down to three things: handling (bluetooth) headset events by forwarding the appropriate ones to the callscreen window with postMessage() and opening the callscreen attention screen on demand. The reason why it's not a straightforward revert is that we changed this code in the meantime (for example adding multi-SIM support) so that came along from the callscreen.

- communications/dialer/js/oncall.html
  Adjusted all paths to accommodate for the new positions

The system app changes are mostly removals of dialer-specific code with the exception of the changes to apps/system/js/attention_window.js which changes the way attentions screens show up (slide-in vs pop-up) and apps/system/js/browser_frame.js which gives the dialer its right priority.

Finally the build system patch removes two horribly ugly hacks we had: the first that caused the build system to wait for the dialer to be built before building the callscreen due to shared locale strings (ugh). The other one forced b2g to restart when installing the callscreen (so that the system app would pick it up). Both are not needed anymore.
Attachment #8692512 - Flags: review?(thills)
Attachment #8692512 - Flags: review?(rchien)
Attachment #8692512 - Flags: review?(etienne)
Just one extra note. There's two minor issues I've found and which I don't know how to fix. When placing a second call from the dialer the callscreen doesn't pop up again, it stays minimized. I don't know how to workaround this, it probably needs some changes in the window management code. The second issue is that the callscreen icon in the statusbar is the generic communications icon, this will be fixed once we split the dialer and contacts apps.
Comment on attachment 8692512 [details] [review]
[gaia] gabrielesvelto:bug-1227980-merge-callscreen-into-dialer > mozilla-b2g:master

I'm waiting this for a long time.

Thanks a lot for doing this! We don't have to workaround it any more by arranging order of call-screen and communications every time.

Build system patch looks good to me!
Attachment #8692512 - Flags: review?(rchien) → review+
Comment on attachment 8692512 [details] [review]
[gaia] gabrielesvelto:bug-1227980-merge-callscreen-into-dialer > mozilla-b2g:master

I've been focusing on the system part for now. It looks good, made some comments on github.

A few notes from my testing:
* the attention screen notification has the communication icon instead of the dialer icon (entry point issue?)
* the lockscreen version of the callscreen looks pretty broken

Also, I had to make reset-gaia for the attention permission added to the communication app to get picked up. Hope this won't cause any migration issues.

What's the plan QA-wise for this patch? :)
Attachment #8692512 - Flags: review?(etienne) → feedback+
Comment on attachment 8692512 [details] [review]
[gaia] gabrielesvelto:bug-1227980-merge-callscreen-into-dialer > mozilla-b2g:master

I've pushed a patch that addresses the review comments (and fixed the lockscreen issue in the main patch, it was a wrong path in the CSS file).

Besides this also fixes the issue with the callscreen not being shown when making a second outgoing call and I've added some extra unit-tests to cover the bits I've changed. I had to re-introduce some callscreen-specific bits which made me sad but we're still culling 400+ lines from the system app which is good overall.

(In reply to Etienne Segonzac (:etienne) from comment #18)
> A few notes from my testing:
> * the attention screen notification has the communication icon instead of
> the dialer icon (entry point issue?)

Yep, it's an issue we can live with IMHO since we'll get rid of the entry points next month when we split the dialer from the contacts app. Then fixing it won't require ugly hacks and whatnot.

> * the lockscreen version of the callscreen looks pretty broken

My bad, fixed that too.

> Also, I had to make reset-gaia for the attention permission added to the
> communication app to get picked up. Hope this won't cause any migration
> issues.

That's something I need to test using a fake update on my phone. What worries me most is the removal of the callscreen app rather than the manifest changes.

> What's the plan QA-wise for this patch? :)

Lots of testing on my part (on my actual phone, so if I miss calls I'm the only one to blame) and as much testing as QA can afford. I still have to talk to Isabel about it but I want to be the patch to be in good shape before wasting their times on obvious bugs.
Attachment #8692512 - Flags: review?(etienne)
Comment on attachment 8692512 [details] [review]
[gaia] gabrielesvelto:bug-1227980-merge-callscreen-into-dialer > mozilla-b2g:master

> > A few notes from my testing:
> > * the attention screen notification has the communication icon instead of
> > the dialer icon (entry point issue?)
> 
> Yep, it's an issue we can live with IMHO since we'll get rid of the entry
> points next month when we split the dialer from the contacts app. Then
> fixing it won't require ugly hacks and whatnot.

Agreed :)

> 
> > * the lockscreen version of the callscreen looks pretty broken
> 
> My bad, fixed that too.
> 
> > Also, I had to make reset-gaia for the attention permission added to the
> > communication app to get picked up. Hope this won't cause any migration
> > issues.
> 
> That's something I need to test using a fake update on my phone. What
> worries me most is the removal of the callscreen app rather than the
> manifest changes.
> 
> > What's the plan QA-wise for this patch? :)
> 
> Lots of testing on my part (on my actual phone, so if I miss calls I'm the
> only one to blame) and as much testing as QA can afford. I still have to
> talk to Isabel about it but I want to be the patch to be in good shape
> before wasting their times on obvious bugs.

No worries, just hope we can get qa-approval? on this one.
Attachment #8692512 - Flags: review?(etienne) → review+
I just gave Etienne's suggestion to see if window.open()ing again the same URL brings the attention screen back and it does! I'll modify the system-app patch one more time to use this instead of the window manager hacks to bring up the callscreen when making or receiving a second call.
Quick update: calling window.open() again to re-open the callscreen works but there's some quirks. First of all a second pseudo-notification is added to the utility tray and then there's some issue related to the audio channel that crops up and I haven't figured it out yet. Long story short, if we don't want callscreen-specific hacks in the system app this needs more work.
Hi Gabriel, please feel free to ask me to help testing your patch. If you think it is already in a good shape, I could start testing it.
Thanks!
Comment on attachment 8692512 [details] [review]
[gaia] gabrielesvelto:bug-1227980-merge-callscreen-into-dialer > mozilla-b2g:master

Hi Gabriele,

I saw your last comment after I had gone through all the code and tested :)  

I left a feedback+ and then you can flag me again when you are ready.

Thanks,

-tamara
Attachment #8692512 - Flags: review?(thills) → feedback+
(In reply to Tamara Hills [:thills] from comment #24)
> I saw your last comment after I had gone through all the code and tested :)  
> 
> I left a feedback+ and then you can flag me again when you are ready.

Thanks Tamara! I haven't modified the callscreen part of the patch and won't be modifying the system-app part either because I found out it's not viable right now.

To summarize my last experiments I tried using window.open() again to bring the callscreen to the front after it had already been opened. I made an experiment which seemed to work only to find out I still had my workaround installed on the phone so it was a fluke. Afterwards I tried going down the "proper" way of doing this by calling the focus() method on the callscreen window. This is the standard way of bringing up a page/tab on the desktop when it's hidden/minimized but it doesn't seem to work on FxOS so I'll leave the patch as-is and file a followup to properly support window.focus() for attention screens and remove the last system app hacks with that.
Comment on attachment 8692512 [details] [review]
[gaia] gabrielesvelto:bug-1227980-merge-callscreen-into-dialer > mozilla-b2g:master

We should be good now. I haven't modified any of the existing patches so if you've already gone over them you shouldn't need to do it again. I have pushed another patch on top though that moves the TonePlayer initialization out of the KeypadManager and made it the first operation when launching either the regular dialer or the callscreen window. This was done to solve issues with cases where we would use the TonePlayer from outside of the KeypadManager and thus before having initialized it (e.g. for playing a busy tone). This also solves a long-standing issue we had with audio channel management; now the dialer keypad will always use the system channel (which is redirected appropriately between the external speaker and headset) and the in-call keypad uses the telephony one (which switches between the internal speaker and headset). This solves for example the end-of-call-tone issue where the tone would be played from the external speaker instead of the internal one.
Attachment #8692512 - Flags: review?(thills)
Isabel, I think we're good to go with the testing too. I've tested a lot of scenarios but there's still a few things to look out for. I've tested making and receiving calls (including multiple calls & conferences) and they all seem to work fine. I've tried Bluetooth headset commands (both in and outside of a call) and checked the audio output which also looks good. What I didn't test is commands audio output with a regular non-Bluetooth headset. Another thing worth looking for is if upgrades work: since we're deleting an app I'm not sure what's supposed to happen when we apply an update; I'll give it a shot later today and report back here.
Flags: needinfo?(irios.mozilla)
Blocks: 1023744
Hi Gabriele, great I will start with that. Do you mind if we have a quick chat so I can ask you a few questions? Thanks!
Flags: needinfo?(irios.mozilla) → needinfo?(gsvelto)
Flags: needinfo?(gsvelto)
Comment on attachment 8692512 [details] [review]
[gaia] gabrielesvelto:bug-1227980-merge-callscreen-into-dialer > mozilla-b2g:master

Isabel spotted an issue with the ringtone when a second incoming calls comes in  so I've pushed yet another patch on top of the series and this one is for Alastor to review.

Alastor, previously when we received a second call the callscreen would play a beeping tone and the system app would play the ringtone at the same time. The audio channel manager would then prioritize the callscreen and make the ringtone inaudible so one could only hear the beep-beep sound from the callscreen. With my patch applied this doesn't seem to be the case anymore and since I don't think we should rely on this behavior to play the sound I've patched the system app to do the following:

- When only one call is present play the ringtone as usual

- When more than one call is present, play a silent sound (as we already do now when the ringtone is silenced) but do so by setting the audio element volume to 0

This seems to be working fine. BTW the patch to review is the last one of the series. Tamara this doesn't affect the callscreen/dialer code so no need to look at it again.
Attachment #8692512 - Flags: review?(alwu)
Already tested that issue in comment #29 with the second incoming call ringtone and now that is working fine.
That was the only thing to comment about this patch that I saw during the testing.

Thank you Gabriele, you did a great work with this patch!
Hi, Gabriele,
Thanks for your great work!
This patch is LGTM, but I leave some questions in the PR.
I've left some comments and I'll be doing another check regarding audio channels just in case, thanks for the feedback!
OK, I did another check and we still need to use ownAudioChannel() otherwise things like background music will keep playing even during a call. I have to figure out why Gecko is throwing that error though...
Maybe you would feel little strange that keep music playing in the background, but it's the correct behavior defined in the UX spec. [1]

Here is an easy way to verify whether the telephony involves audio competing successfully.

STR.
1. Connect a call
2. Go to Setting app > Sound
3. Play notification or alarm (drag the volume bar)

Expected.
4. The phone should vibrate and no any sound can be playback (same, defined in the UX spec)

[1] Come-in "content", during "telephony" : https://bug1068219.bmoattachments.org/attachment.cgi?id=8579177
Comment on attachment 8692512 [details] [review]
[gaia] gabrielesvelto:bug-1227980-merge-callscreen-into-dialer > mozilla-b2g:master

LGTM, if we still have the ownAudioChannel().
Thanks :)
Attachment #8692512 - Flags: review?(alwu) → review+
After much digging and with Alastor's help I've finally figured out what's wrong. When calling ownAudioChannel() we try to resume or hold the current active call according whether it's muted or not. This works when the call is in the appropriate state (e.g. resuming an held call) but will trigger an exception if it's not (e.g. holding a call which is dialing or ringing). My gaia patch tripped over this but the issue is not limited to this case so I'll post a gecko patch to fix the issue as well as update the gaia patch to put back the onwAudioChannel() invocation. I've also verified that the scenario described in comment 34 works correctly.
This gecko patch prevents Telephony::WindowVolumeChanged() from calling the hold/resume methods on calls or group calls which are not in the appropriate state or which are not switchable. This not only fixes the issue here but also other potential scenarios we haven't hit yet when a call state changes.
Attachment #8709400 - Flags: review?(htsai)
Comment on attachment 8692512 [details] [review]
[gaia] gabrielesvelto:bug-1227980-merge-callscreen-into-dialer > mozilla-b2g:master

Hi Gabriele,

Looks good to me.  I did some testing as well. Really happy to see this!  I think it will simplify things.

Thanks,

-tamara
Attachment #8692512 - Flags: review?(thills) → review+
Comment on attachment 8709400 [details] [diff] [review]
[PATCH] Do not try to hold/resume calls in the wrong state when handling audio channels

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

Thanks for the logic enhancement!
Attachment #8709400 - Flags: review?(htsai) → review+
OK, we need to land the gecko patch first, it's attachment 8709400 [details] [diff] [review] and this is the try run:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=dd8b3d538ad6

Once that's done and the patch hit mozilla-central I'll merge the gaia PR. This is required otherwise we'd risk breaking regular call functionality. I'll close the bug myself once both bits have landed.
Keywords: checkin-needed, leave-open
https://hg.mozilla.org/integration/mozilla-inbound/rev/c93664e828088b19a60c36886f2b2a6cd411f1b3
Bug 1227980 - Do not try to hold/resume calls in the wrong state when handling audio channels r=hsinyi
I forgot to attach the updated patch the try run refers to. Carrying over the r+ flag. Sorry for the inconvenience.
Attachment #8709400 - Attachment is obsolete: true
Attachment #8712092 - Flags: review+
I'd really love to merge the gaia part but I cannot get a green try run and even though it doesn't seem like my changes are causing the orange I don't feel confident in merging such a large patch on a non-green tree.
So after gathering some info on IRC it seems that Gij11 and Gij18 have been broken by something else so I'll go ahead and merge, my change doesn't seem to introduce any other issue.
Merged to gaia/master d7f4b8e01a31d997428b90f3e95587e5cfcdd8a9

https://github.com/mozilla-b2g/gaia/commit/d7f4b8e01a31d997428b90f3e95587e5cfcdd8a9

I hope this sticks, thanks to everybody who reviewed, tested and worked on this!
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Removing leave-open keyword from resolved bugs, per :sylvestre.
Keywords: leave-open
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: