1068093 - Remove legacy transition code from the callscreen

Status: RESOLVED FIXED

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

Description

[Doug Sherk (:drs) (inactive)]

We have some legacy transition code in the callscreen from after bug 927862 landed, such as this:
https://github.com/mozilla-b2g/gaia/blob/master/apps/callscreen/js/calls_handler.js#L301

We should remove all references to this as the transitions are now handled in the AttentionWindow code in the System app.

Comment 1

[Tamara Hills [:thills]]

Attachment: 1068093.diff

Hi Doug,

Looking for feedback.  I removed what Etienne suggested, plus there was some more that became useless after those.

Right now the tests are just "working", but I realize I will need to add some more to replace the ones I removed to account for this change.

Comment 2

[Doug Sherk (:drs) (inactive)]

Comment on attachment 8503342 [details] [diff] [review]
1068093.diff

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

Looks reasonable. This can go into review whenever you're ready. I didn't check if you missed anything, but the general approach is good.

Comment 3

[Doug Sherk (:drs) (inactive)]

Let's track the in-sprint flag using the meta bug instead to avoid polluting the whiteboard of every sub-bug.

Comment 4

[Tamara Hills [:thills]]

Attachment: Patch for review

Comment 5

[Doug Sherk (:drs) (inactive)]

Attachment: Remove legacy transition code from the callscreen (author: thills)

Posted as a patch since I have no way to comment on this without either a PR or patch posted.

Comment 6

[Doug Sherk (:drs) (inactive)]

Comment on attachment 8505081 [details] [diff] [review]
Remove legacy transition code from the callscreen (author: thills)

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

In the future, please post changes as either a PR or patch(es).

::: apps/callscreen/js/call_screen.js
@@ -68,5 @@
> -    self.statusMessage.addEventListener('transitionend', function tend(evt) {
> -      evt.stopPropagation();
> -      self.statusMessage.removeEventListener('transitionend', tend);
> -      setTimeout(function hide() {
> -        self.statusMessage.classList.remove('visible');

I don't think we can safely remove this. Otherwise, we'll be stuck with status messages constantly visible. For example, when a member of a conference call hangs up on the call, we'll see "X has left the call" until a new status message is shown.

@@ +231,4 @@
>    _wallpaperImageHandler: function cs_wallpaperImageHandler(image) {
>      this.mainContainer.style.backgroundImage = 'url(' +
>        (typeof image === 'string' ? image : URL.createObjectURL(image)) + ')';
> +    setTimeout(this.setCallerContactImage.bind(this));

This is small enough that we should consider refactoring it so that it's part of `setCallerContactImage()`.

@@ +238,4 @@
>      var activeCallForContactImage = CallsHandler.activeCallForContactImage;
>      var blob = activeCallForContactImage && activeCallForContactImage.photo;
>  
>      this.contactBackground.classList.remove('ready');

I don't think we need this `ready` class anymore. I think the intention here was to flush the style and get the background image to fade to the new one, but this is not accomplishing that.

It's possible, though doubtful, that without this we will get a black screen or something. Please check what happens without it. This is also referred to in oncall.css:869.

::: apps/callscreen/style/oncall.css
@@ -183,5 @@
>      background: black;
>      z-index: 100;
>    }
>  
> -  /* This transition is needed to trigger a transitionend event in */

This change should be made in 'oncall_status_bar.css' as well.

::: apps/callscreen/test/unit/call_screen_test.js
@@ +411,1 @@
>      suite('once the transition is over', function() {

We don't need this suite anymore. We should keep the underlying tests, though.

@@ -776,5 @@
> -          this.sinon.clock.tick(2000);
> -          done();
> -        });
> -        test('should hide the banner', function() {
> -          assert.isFalse(bannerClass.contains('visible'));

We should keep this test and suite. See my comment in call_screen.js about this.

Comment 7

[Tamara Hills [:thills]]

Attachment: Changes after review

Hi Doug,

I put this in PR this time.  Sorry about that.  I also left the changes unsquashed for you... so there are two commits.  

Also, I tried removing the 'ready' class and the picture doesn't show up (we do get the wallpaper though).  So, I believe we need to keep that.

Thanks,
-tamara

Comment 8

[Doug Sherk (:drs) (inactive)]

Comment on attachment 8506142 [details] [review]
Changes after review

There are some issues to iron out here still. See the PR for my comments.

Comment 9

[Tamara Hills [:thills]]

Hi Doug, I commented on your comment on the PR:  https://github.com/mozilla-b2g/gaia/pull/25224.

If you can have a look, that will be great as need to discuss before we move this forward.

Thanks,
-tamara

Comment 10

[Doug Sherk (:drs) (inactive)]

Answered on the PR.

Comment 12

[Tamara Hills [:thills]]

Attachment: Updated PR after last review

Changes after the last review to fix the spacing and put back the wallpaper function.

Comment 13

[Doug Sherk (:drs) (inactive)]

Comment on attachment 8509771 [details] [review]
Updated PR after last review

I'm getting a black screen when receiving an incoming call from a contact with an image. Please investigate this. I left a couple more comments on the PR.

Comment 14

[Doug Sherk (:drs) (inactive)]

(In reply to Doug Sherk (:drs) (use needinfo?) from comment #13)
> I'm getting a black screen when receiving an incoming call from a contact
> with an image. Please investigate this.

I originally reproduced this on a Flame-JB build. I tried this again on a Flame-KK v188 build and I was no longer able to repro it. So I think it's safe to say that this isn't a problem.

Comment 15

[Tamara Hills [:thills]]

Hi Doug,

I fixed the one spacing comment on the latest PR.

Regarding the one comment about _wallpaperImageHandler needing a test, I checked and we already have two in this area:

https://github.com/tamarahills/gaia/blob/master/apps/callscreen/test/unit/call_screen_test.js#L358
https://github.com/tamarahills/gaia/blob/master/apps/callscreen/test/unit/call_screen_test.js#L376

Let me know if there was something else that needed addressing.

Thanks,
-tamara

Comment 16

[Doug Sherk (:drs) (inactive)]

Comment on attachment 8509771 [details] [review]
Updated PR after last review

Yep, looks good.

Comment 17

[Tamara Hills [:thills]]

Attachment: Final PR after merge

New PR as I had merge conflicts.

Comment 18

[Tamara Hills [:thills]]

https://github.com/tamarahills/gaia/commit/2bb22dda265b1aaf8c5c969b6fb52188d8cb7578

Comment 19

[Tamara Hills [:thills]]

correct commit from master
https://github.com/mozilla-b2g/gaia/commit/9af4a0040f8bb1cc23c7de7fe0a96373067541a8