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.

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

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

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

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.

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 on attachment 8506142 [details] Changes after review There are some issues to iron out here still. See the PR for my comments.

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

Answered on the PR.

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

Comment on attachment 8509771 [details] 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.

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

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 on attachment 8509771 [details] Updated PR after last review Yep, looks good.

New PR as I had merge conflicts.

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