1068109 - Restore 2s delay before callscreen is closed

Status: VERIFIED FIXED

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

Description

[Doug Sherk (:drs) (inactive)]

We appear to have broken the 2s delay before the callscreen is hidden in bug 927862. This delay is needed so that the user can see the duration of the call that just ended.

The delay time is defined here:
https://github.com/mozilla-b2g/gaia/blob/master/apps/callscreen/js/call_screen.js#L11

It's supposed to be used here:
https://github.com/mozilla-b2g/gaia/blob/master/apps/callscreen/js/handled_call.js#L325

But this is never called because the app window is closed before the setTimeout can fire. We don't know the exact problem here, but it appears to be more than just the transitionend handler which is set here:
https://github.com/mozilla-b2g/gaia/blob/master/apps/callscreen/js/calls_handler.js#L301

Comment 1

[Tamara Hills [:thills]]

Attachment: Prototype for fixing the 2s timeout

Putting this up for feedback.  It's a working prototype that fixes the 2s delay problem.  Note that this also deals with turning on the screen, but I'll deal with that separately in the other bug, so you can ignore the screenmanager change.  

I've tested the following cases so far:
Single call scenarios
=======================
incoming - remote hup
incoming - local reject
incoming - local hup
incoming - remote hup while alerting

outgoing - remote hup
outgoing - local hup
outgoing - local hup before connect achieved

Multiple calls
==============
1 active 1 held - remote hups
1 active 1 held - local hups

conference call
===============
1 active conf call - local hup all
1 active conf call - remote hup one at a time
1 active conf call + 1 held separate call - remote hup
1 active conf call + 1 held separate call - local hup

Comment 2

[Doug Sherk (:drs) (inactive)]

Comment on attachment 8491114 [details] [diff] [review]
Prototype for fixing the 2s timeout

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

Nice! I think we got too stuck on the old logic at first, and didn't do a good job of stepping back and rethinking it. This seems like a good method, and I'm happy you came up with it.

I didn't try it, but your tests seem reasonable. One thing I'm curious about is what happens if a call gets hung up while the callscreen is backgrounded.

::: apps/callscreen/js/calls_handler.js
@@ -303,5 @@
> -      // now closing the window.
> -      if (!displayed) {
> -        closeWindow();
> -      }
> -    });

I'm not sure that this does anything anymore. The only useful code I can see is a use of `displayed` on line 644.

@@ +306,4 @@
>      }
>    }
>  
>    function closeWindow() {

This name is no longer accurate and should be changed to reflect what it does.

@@ +308,5 @@
>  
>    function closeWindow() {
> +    if(handledCalls.length === 0) {
> +      window.close();
> +    }

I think it makes more sense for the window closing code to live in CallScreen, but all the logic for when to call it to live in CallsHandler.

::: apps/callscreen/js/handled_call.js
@@ +321,5 @@
>    this.node.classList.add('ended');
>    setTimeout(function(evt) {
>      CallScreen.removeCall(self.node);
>      self.node = null;
> +    CallsHandler.closeWindow();

The SoC here is not very good, but I can't think of a better way to structure it. I think this is just an unfortunate consequence of the poorly defined line between HandledCall and CallsHandler, especially regarding state change handling.

::: apps/system/js/screen_manager.js
@@ +276,5 @@
>            }
>  
> +          if (evt.call.state == "disconnected") {
> +            this.turnScreenOn();
> +          }

Ignoring as per comment 1.

Comment 3

[Tamara Hills [:thills]]

Attachment: 1068109.diff

Hi Doug,

Let me know if you think the SoC is better on this version.  I tried to group more into CallScreen, then have CallsHandler have the logic.  

Thanks,

-tamara

Comment 4

[Doug Sherk (:drs) (inactive)]

Comment on attachment 8492352 [details] [diff] [review]
1068109.diff

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

I think I understand your thought process here. The SoC in the surrounding code is really bad though, and I'd like to take some steps to improve it. Here's the code layout/flow that I was thinking we could have:

CallScreen: Responsible for UI only. When we call into it, there should be next to no logic in it.

One thing that would be nice to do here is clearly document which public functions are intended to be associated with a single call, and which ones are associated with the entire CallScreen. Having this problem is indicative of needing another class (HandledCallUI or something), but we'll ignore that for now.

HandledCall: Handles any UI changes and logic for a single call. So it owns its visual elements on the CallScreen (yuck, kind of grey line and difficult to enforce) but not the app or anything on CallScreen other than that.

CallsHandler: This is where most of the logic should go.

I don't think we need the `displayed` variable, but I haven't tested this assumption. If we don't, then there's a lot of cleanup we can do here.

One consequence of my suggestions above is that we'll now have 2 calls to `setTimeout()` every time a call is removed. I don't like this, but I think it's necessary for good SoC. As long as they're both getting their delays from CallScreen, there shouldn't be any problems.

::: apps/callscreen/js/call_screen.js
@@ +622,5 @@
> +      cb();
> +      if(CallsHandler.isOKToCloseCallScreen()) {
> +        window.close();
> +      }
> +    }, CallScreen.callEndPromptTime);

This is too logic-y for CallScreen. As far as I can see, the only thing that should live on CallScreen is the wrapper for `window.close()`.

::: apps/callscreen/js/calls_handler.js
@@ +297,5 @@
>    /* === Call Screen === */
>    function toggleScreen() {
>      displayed = !displayed;
> +    // We did animate the call screen off the viewport
> +    CallScreen.toggle(function transitionend() {});

This is the only call-site for this function, so we're free to do what we want with it. I'm not even sure that we need this function at all anymore as it appears to be legacy code for the initial displaying of the callscreen, bundled with the logic for closing it.

We have this block above in `onCallsChanged()`:
```
    if (handledCalls.length === 0) {
      exitCallScreen(false);
    } else if (!displayed) {
      toggleScreen();
    }
```

It makes more sense to write our code into there. We could have a separate function called something like `exitCallScreenIfNoCalls()`. All it would do is the following:

1) `setTimeout()` and store the handle for it.
2) If another `exitCallScreenIfNoCalls()` call comes in before the timeout finishes, we cancel the previous one and set another.
3) When the setTimeout fires, we check if |handledCalls === 0| and if so, signal the CallScreen to close.

@@ +308,2 @@
>      }
>    }

I don't think we need this function at all anymore. But we should look further and test this.

@@ +317,1 @@
>    }

I don't think this needs to be a separate function as we can embed it in the name of the function that handles the closing logic.

::: apps/callscreen/js/handled_call.js
@@ +323,2 @@
>      self.node = null;
> +  });

This shouldn't be calling into anything other than CallScreen, and only to remove itself from the DOM. We can restore the old `setTimeout()` call.

Comment 5

[Tamara Hills [:thills]]

Attachment: 1068109v2.diff

Per your suggestions, I kept the Handled_call as is and added the additional timer in CallsHandler.  Also, this seems to work fine without the displayed.  I still need to test with BT though.

If you can comment on the use of the timer variable, I'm not sure setting the handle to null is the preferred method, but I could not find any documentation on what invalid timer is (e.g. 0, -1. etc).  

Thanks
-tamara

Comment 6

[Doug Sherk (:drs) (inactive)]

Comment on attachment 8493813 [details] [diff] [review]
1068109v2.diff

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

This is much cleaner and makes more sense to me.

(In reply to Tamara Hills [:thills] from comment #5)
> If you can comment on the use of the timer variable, I'm not sure setting
> the handle to null is the preferred method, but I could not find any
> documentation on what invalid timer is (e.g. 0, -1. etc).

That's fine.

::: apps/callscreen/js/calls_handler.js
@@ +123,5 @@
>        } else if (handledCalls.length !== 0) {
>          CallScreen.showPlaceNewCallButton();
>        }
>      }
>      if (handledCalls.length === 0) {

This check is part of `exitCallScreenIfNoCalls()`.

Comment 7

[Tamara Hills [:thills]]

Attachment: 1068109v3.diff

Comment 8

[Doug Sherk (:drs) (inactive)]

Comment on attachment 8495230 [details] [diff] [review]
1068109v3.diff

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

The whole calls_handler_test.js file needs to be brought into this era. It's missing the following patterns:
1. `this.sinon.spy(obj, 'foo');` instead of `var spy = this.sinon.spy(obj, 'foo');`
2. `sinon.assert.***called***(obj.foo)` instead of `assert.***(obj.foo.called***)`

Don't fix this everywhere, but please apply these patterns anywhere you're working.

There should be an additional test in the `> extra call ended` suite checking that we don't close the call screen when the extra call ends.

There are some spacing/indentation issues that I'm ignoring for now. Please fix them for the next iteration.

::: apps/callscreen/js/calls_handler.js
@@ +13,5 @@
>    var CALLS_LIMIT = 2;
>    var CDMA_CALLS_LIMIT = 2;
>  
>    var handledCalls = [];
> +  var timerId = null;

s/timerId/exitCallScreenTimeout/

@@ +123,5 @@
>        } else if (handledCalls.length !== 0) {
>          CallScreen.showPlaceNewCallButton();
>        }
>      }
> +    exitCallScreenIfNoCalls();

Extra line break above here.

@@ +289,5 @@
>      playWaitingTone(call);
>    }
>  
>    /* === Call Screen === */
> +function exitCallScreenIfNoCalls() {

This function could use a comment explaining what it does.

@@ +290,5 @@
>    }
>  
>    /* === Call Screen === */
> +function exitCallScreenIfNoCalls() {
> +    //Start (or restart) a timer if there are no calls left

I don't think this comment is necessary.

@@ +291,5 @@
>  
>    /* === Call Screen === */
> +function exitCallScreenIfNoCalls() {
> +    //Start (or restart) a timer if there are no calls left
> +    if (handledCalls.length === 0) {

Have you verified that this is a valid check in all situations? I tend to think that it is based on reading the code that calls this function, but I'm not sure if we may run into a situation where `handledCalls.length !== 0` here, but then `handledCalls.length === 0` 2 seconds later.

@@ +292,5 @@
>    /* === Call Screen === */
> +function exitCallScreenIfNoCalls() {
> +    //Start (or restart) a timer if there are no calls left
> +    if (handledCalls.length === 0) {
> +      //If there is a timer in progress, then cancel existing one.

I don't think this comment is necessary.

@@ +295,5 @@
> +    if (handledCalls.length === 0) {
> +      //If there is a timer in progress, then cancel existing one.
> +      if (timerId != null) {
> +        clearTimeout(timerId);
> +        timerId = null;

I think we should test this. We may run into weird functionality and quicker closes than 2s if we accidentally break this.

@@ +625,5 @@
>    function switchToDefaultOut(doNotConnect) {
>      if (telephony.speakerEnabled) {
>        telephony.speakerEnabled = false;
>      }
> +    if (!doNotConnect && telephony.active) {

When we talked with Etienne, he mentioned that this should also check `!document.hidden`.

Also, this needs a test in the '> CallsHandler.switchToDefaultOut when visible' suite. The changes to the tests that you made are insufficient as they don't test `mozTelephony.active` in relative isolation.

::: apps/callscreen/test/unit/calls_handler_test.js
@@ +204,5 @@
>          MockNavigatorMozTelephony.mTriggerCallsChanged();
>          assert.isTrue(setChannelSpy.notCalled);
>        });
> +
> +      test('should close the callscreen app', function() {

'should close the callscreen app after a delay'

@@ +208,5 @@
> +      test('should close the callscreen app', function() {
> +        this.sinon.spy(MockCallScreen, 'exitCallScreen');
> +        MockNavigatorMozTelephony.mTriggerCallsChanged();
> +        sinon.assert.notCalled(MockCallScreen.exitCallScreen);
> +        this.sinon.clock.tick(2000);

This should use `MockCallScreen.callEndPromptTime` instead.

@@ +209,5 @@
> +        this.sinon.spy(MockCallScreen, 'exitCallScreen');
> +        MockNavigatorMozTelephony.mTriggerCallsChanged();
> +        sinon.assert.notCalled(MockCallScreen.exitCallScreen);
> +        this.sinon.clock.tick(2000);
> +        sinon.assert.called(MockCallScreen.exitCallScreen);

`sinon.assert.calledOnce()`

@@ +612,5 @@
> +          MockNavigatorMozTelephony.conferenceGroup.calls = [secondConfCall];
> +          MockNavigatorMozTelephony.mTriggerGroupCallsChanged();
> +          MockNavigatorMozTelephony.mTriggerCallsChanged();
> +          sinon.assert.notCalled(MockCallScreen.exitCallScreen);
> +          this.sinon.clock.tick(2000);

This should use `MockCallScreen.callEndPromptTime` instead.

@@ +613,5 @@
> +          MockNavigatorMozTelephony.mTriggerGroupCallsChanged();
> +          MockNavigatorMozTelephony.mTriggerCallsChanged();
> +          sinon.assert.notCalled(MockCallScreen.exitCallScreen);
> +          this.sinon.clock.tick(2000);
> +          sinon.assert.notCalled(MockCallScreen.exitCallScreen);

This should be broken up into two tests starting here.

@@ +619,5 @@
> +          MockNavigatorMozTelephony.calls = [];
> +          MockNavigatorMozTelephony.conferenceGroup.calls = [];
> +          MockNavigatorMozTelephony.mTriggerCallsChanged();
> +          sinon.assert.notCalled(MockCallScreen.exitCallScreen);
> +          this.sinon.clock.tick(2000);

This should use `MockCallScreen.callEndPromptTime` instead.

@@ +620,5 @@
> +          MockNavigatorMozTelephony.conferenceGroup.calls = [];
> +          MockNavigatorMozTelephony.mTriggerCallsChanged();
> +          sinon.assert.notCalled(MockCallScreen.exitCallScreen);
> +          this.sinon.clock.tick(2000);
> +          sinon.assert.called(MockCallScreen.exitCallScreen);

`sinon.assert.calledOnce()`

@@ +639,5 @@
>        });
>  
>        test('should resume the first call', function() {
>          this.sinon.spy(firstCall, 'resume');
> +        this.sinon.spy(MockCallScreen, 'exitCallScreen');

I don't think the changes to this test are necessary.

@@ +681,5 @@
>        });
>  
>        test('should resume the conference call', function() {
>          this.sinon.spy(MockNavigatorMozTelephony.conferenceGroup, 'resume');
> +        this.sinon.spy(MockCallScreen, 'exitCallScreen');

I don't think the changes to this test are necessary.

@@ +1532,5 @@
>  
>        test('should connect bluetooth SCO', function() {
>          var connectScoSpy = this.sinon.spy(
>            MockBluetoothHelperInstance, 'connectSco');
> +        MockNavigatorMozTelephony.active = true;

mozTelephony.active is null, or a TelephonyCall object. See https://developer.mozilla.org/en-US/docs/Web/API/Telephony.active

::: apps/callscreen/test/unit/mock_call_screen.js
@@ +98,5 @@
>      this.mGetScenarioCalled = true;
>      return this.mScenario;
>    },
>  
> +  exitCallScreen: function() {

This can be on one line since it has no contents.

Comment 9

[Tamara Hills [:thills]]

Attachment: 1068109v4.diff

Comment 10

[Doug Sherk (:drs) (inactive)]

Comment on attachment 8496323 [details] [diff] [review]
1068109v4.diff

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

This is pretty good now. I think we only need one more iteration. Please post a PR when you're ready with the next version so that I can test it.

::: apps/callscreen/js/call_screen.js
@@ +616,5 @@
>      return scenario;
> +  },
> +
> +  exitCallScreen: function cs_exitCallScreen() {
> +      window.close();

This is indented in 2 spaces too far.

::: apps/callscreen/js/calls_handler.js
@@ +292,5 @@
>  
> +  /* This function has the logic to determine when we should exit the
> +   * CallScreen app based on the number of handledCalls and the required UX
> +   * timeouts.
> +   */

This is really wordy and doesn't tell me much. Let's keep it concise:
/**
 * Checks now and also in CallScreen.callEndPromptTime seconds if there
 * are no currently handled calls, and if not, exits the app. Resets
 * this timer on each successive invocation.
 */

@@ +627,5 @@
>    function switchToDefaultOut(doNotConnect) {
>      if (telephony.speakerEnabled) {
>        telephony.speakerEnabled = false;
>      }
> +    if (!doNotConnect && telephony.active && !document.hidden) {

Let's add back the line break here.

::: apps/callscreen/test/unit/calls_handler_test.js
@@ +216,5 @@
> +      test('should only set 1 timer for app close delay', function() {
> +        this.sinon.spy(MockCallScreen, 'exitCallScreen');
> +        MockNavigatorMozTelephony.mTriggerCallsChanged();
> +        sinon.assert.notCalled(MockCallScreen.exitCallScreen);
> +        this.sinon.clock.tick(MockCallScreen.callEndPromptTime/2);

Reverse the two lines above.

@@ +454,4 @@
>          MockNavigatorMozTelephony.mTriggerCallsChanged();
> +        sinon.assert.calledOnce(overflowCall.hangUp);
> +        this.sinon.clock.tick(MockCallScreen.callEndPromptTime);
> +        sinon.assert.notCalled(MockCallScreen.exitCallScreen);

This extra testing is not necessary.

@@ +458,3 @@
>        });
>  
>        test('should still instanciate a handled call', function() {

Not your fault but if you feel like fixing this,
s/instanciate/instantiate/

@@ +626,4 @@
>          MockNavigatorMozTelephony.mTriggerGroupCallsChanged();
>          MockNavigatorMozTelephony.mTriggerCallsChanged();
>  
> +        sinon.assert.notCalled(MockCallScreen.exitCallScreen);

This check is not necessary. It is inclusive to the second one.

@@ +640,3 @@
>          MockNavigatorMozTelephony.mTriggerCallsChanged();
> +
> +        sinon.assert.notCalled(MockCallScreen.exitCallScreen);

This check is not necessary. We test that the callscreen only gets closed after a delay elsewhere.

Comment 11

[Tamara Hills [:thills]]

Attachment: Pull request for review

Comment 12

[Tamara Hills [:thills]]

Hi Etienne,

We were wondering if you had any pointers on the purpose of the background image waiting code in the CallScreen:

https://github.com/mozilla-b2g/gaia/blob/master/apps/callscreen/js/call_screen.js#L278

I tried removing this _contactBackgroundWaiting variable and didn't see any noticeable difference, but there must be some good reason for it.

Also, Doug had a thought to separate the background image logic into a separate class and wanted to get your thoughts on that as well.

Comment 13

[Tony Chung [:tchung]]

[Blocking Requested - why for this release]:

We need this bug fixed for 2.1, as it blocks a blocker in 1070066.

Comment 14

[Tamara Hills [:thills]]

Attachment: Pull Request Updated

Comment 15

[Doug Sherk (:drs) (inactive)]

Comment on attachment 8498396 [details] [review]
Pull Request Updated

Nice! I left a couple of nits. Please address them and then we can land this.

Comment 16

[Wesley Huang [:wesley_huang] (EPM) (NI me)]

triage: per comment 13, to inherit the blocking

Comment 17

[Etienne Segonzac (:etienne)]

(In reply to Tamara Hills [:thills] from comment #12)
> Hi Etienne,
> 
> We were wondering if you had any pointers on the purpose of the background
> image waiting code in the CallScreen:
> 
> https://github.com/mozilla-b2g/gaia/blob/master/apps/callscreen/js/
> call_screen.js#L278
> 
> I tried removing this _contactBackgroundWaiting variable and didn't see any
> noticeable difference, but there must be some good reason for it.


We used to set the contact picture while the call screen was transitioning, causing the transition to skip frames (the transition was done inside the callscreen app). Now that the transition is handled by the system app we trigger _onTransitionDone right away, rendering the _contactBackgroundWaiting useless. Thankfully we're note skipping frames during the transition either since it's done by the system app :)

I definitely recommend removing all _contactBackgroundWaiting refererences and the _onTransitionDone method.

> 
> Also, Doug had a thought to separate the background image logic into a
> separate class and wanted to get your thoughts on that as well.

Once all the "waiting" pieces are removed the background image logic should be pretty lightweight, maybe a bit too light to be a separate singleton :)
But I don't have a super strong opinion on this.

Comment 18

[Tamara Hills [:thills]]

Attachment: New PR

This is the PR with the nits from the last one fixed.  I had to open a new one because the old one was closed.

Comment 19

[Doug Sherk (:drs) (inactive)]

Comment on attachment 8498888 [details] [review]
New PR

It doesn't feel like 2s is long enough to me, especially once we wake up the screen when the call ends. We could punt changes here to bug 1068104 or bug 1068106.

Thoughts, Carrie?

Comment 20

[Carrie Wang [New account: [e-mail]]]

Comment on attachment 8498888 [details] [review]
New PR

I think 2 secs work fine to me. In addition, I just surprisingly found that we have proximity sensor on the device. So the backlight of the screen will automatically turns on when you move the phone from the ear. However, for those cases that don't have proximity sensor built in, can we start calculating 2 secs after users manually wake up the screen? Thanks!

Comment 21

[Tamara Hills [:thills]]

Hi Carrie, 

I don't believe we need to worry about the cases when they don't have proximity sensor because bug 1068104 https://bugzilla.mozilla.org/show_bug.cgi?id=1024341 will take care of this when they hang up.  Basically, it will light up the screen when there is a hangup.  The two seconds kicks in at that time.  So essentially, they should never have to manually wake the device after the hangup.

Let us know if this is ok.

Thanks,
-tamara

Comment 22

[Carrie Wang [New account: [e-mail]]]

Hi Tamara, 

Hmmm...I think I misunderstood something here. I think 2 secs is fine to me now. :) Thanks!

Comment 23

[Tamara Hills [:thills]]

Merged PR:  https://github.com/mozilla-b2g/gaia/pull/24707

Comment 24

[Doug Sherk (:drs) (inactive)]

Merge commit: https://github.com/mozilla-b2g/gaia/commit/cfe5165889a5e07779fe73aaa7d9d1b08b2845fe

Comment 26

[Tamara Hills [:thills]]

Attachment: Pull request for 2.1 approval

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): unknown
[User impact] if declined: user will not be able to see the duration of a call upon hangup as there is no delay after the hangup.  
[Testing completed]:yes
[Risk to taking this patch] (and alternatives if risky):low
[String changes made]:no

Comment 27

[Anthony Ricaud (:rik)]

(In reply to Tamara Hills [:thills] from comment #26)
> [Bug caused by] (feature/regressing bug #): unknown
Actually, we know it is bug 927862 that introduced this regression.

Comment 28

[Ryan VanderMeulen [:RyanVM]]

v2.1: https://github.com/mozilla-b2g/gaia/commit/e92ae85175701a79ccbc01a849055ccd0a960791

Comment 29

[Corinne Innes [:CorinneI]]

Verified fixed on 2.1 and 2.2

Flame 2.1 

Device: Flame 2.1 (319mb)(Kitkat Base)(Shallow Flash)
BuildID: 20141125001201
Gaia: 1bdd49770e2cb7a7321e6202c9bf036ab5d8f200
Gecko: db893274d9a6
Version: 34.0 (2.1)
Firmware: V188-1
User Agent: Mozilla/5.0 (Mobile; rv:34.0) Gecko/34.0 Firefox/34.0

Flame 2.2
Environmental Variables:
Device: Flame 2.2  (319mb)(Kitkat Base)(Shallow Flash)
Build ID: 20141125040209
Gaia: 824a61cccec4c69be9a86ad5cb629a1f61fa142f
Gecko: acde07cb4e4d
Version: 36.0a1 (2.2)
Firmware Version: v188-1
User Agent: Mozilla/5.0 (Mobile; rv:36.0) Gecko/36.0 Firefox/36.0

Actual: 
Minimum 2s delay after call ends and before callscreen is closed.