847866 - [Dialer] Avoid race conditions in overlays

Status: RESOLVED FIXED

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

Description

[Germán Toro del Valle (:gtorodelvalle)]

The current libraries used to show overlays in the Communications app may suffer for race conditions (if the overlay is hidden before the animation to show it has completed, for example) and do not support re-showing an already shown overlay (for example to change the message shown).

Comment 1

[Germán Toro del Valle (:gtorodelvalle)]

The affected files are:
   - https://github.com/mozilla-b2g/gaia/blob/master/apps/communications/contacts/js/confirm_dialog.js
   - https://github.com/mozilla-b2g/gaia/blob/master/apps/communications/contacts/js/utilities/overlay.js

Comment 2

[Germán Toro del Valle (:gtorodelvalle)]

Attachment: Associated PR.

Comment 3

[Etienne Segonzac (:etienne)]

Comment on attachment 721641 [details]
Associated PR.

Comments on github, can't wait to play with the updated, rebased and linted version ;)

Comment 4

[Germán Toro del Valle (:gtorodelvalle)]

Comments included in the PR ;-) Would you be so kind to review them? Thanks!

Comment 5

[Germán Toro del Valle (:gtorodelvalle)]

Merged! ;-)

Comment 6

[Julien Wajsberg [:julienw]]

Should this be tracking-b2g18 so that it could land in v1.1 ?

Comment 7

[Germán Toro del Valle (:gtorodelvalle)]

It should ;-) In fact, I was about to ask for the approval-gaia-v1 :)

Comment 8

[Germán Toro del Valle (:gtorodelvalle)]

Comment on attachment 721641 [details]
Associated PR.

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Uncomplete management of overlay messages in the Comms app
User impact if declined: Comms app may become unusable
Testing completed: Tested in the device
Risk to taking this patch (and alternatives if risky): Low risk as a consequence of heavy testing in the device
String or UUID changes made by this patch: None
Additional info: 2 changed files with 196 additions and 104 deletions.

Comment 9

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 721641 [details]
Associated PR.

Looks like a low risk win for email performance, approving for uplift.

Comment 10

[John Ford [:jhford] CET/CEST Berlin Time]

I was not able to uplift this bug to v1-train.  If this bug has dependencies which are not marked in this bug, please comment on this bug.  If this bug depends on patches that aren't approved for v1-train, we need to re-evaluate the approval.  Otherwise, if this is just a merge conflict, you might be able to resolve it with:

  git checkout v1-train
  git cherry-pick -x -m1 3444dd7ff1467e7b09e4c0bc3185a46f0a20b188
  <RESOLVE MERGE CONFLICTS>
  git commit

Comment 11

[Germán Toro del Valle (:gtorodelvalle)]

(In reply to John Ford [:jhford] from comment #10)
> I was not able to uplift this bug to v1-train.  If this bug has dependencies
> which are not marked in this bug, please comment on this bug.  If this bug
> depends on patches that aren't approved for v1-train, we need to re-evaluate
> the approval.  Otherwise, if this is just a merge conflict, you might be
> able to resolve it with:
> 
>   git checkout v1-train
>   git cherry-pick -x -m1 3444dd7ff1467e7b09e4c0bc3185a46f0a20b188
>   <RESOLVE MERGE CONFLICTS>
>   git commit

Hi John ;-) This bug and patch solves cases not previously considered in bug 816941 (this one inclues 2 patches).

Comment 12

[Germán Toro del Valle (:gtorodelvalle)]

BTW, I am solving the conflicts, John ;-) I have started with bug 816941

Comment 13

[Germán Toro del Valle (:gtorodelvalle)]

Hi John, again ;-) Conflicts should be solved and successfully landed in v1-train :-) Would you be so kind to confirm it? Thanks!

Comment 14

[Julien Wajsberg [:julienw]]

Could you please add the commit hash here ? thanks

Comment 15

[Germán Toro del Valle (:gtorodelvalle)]

After cherry-picking: https://github.com/mozilla-b2g/gaia/commit/2da00a8972b1155df722f6914f3de6eb06d719d0

Comment 16

[John Ford [:jhford] CET/CEST Berlin Time]

That looks reasonable, but I'm not the expert on what these changes do and how they fit in with the rest of the v1-train branch.

Comment 17

[Julien Wajsberg [:julienw]]

gtorodelvalle> the easiest to check if this is ok is you pushing the v1-train version on your device and testing this specific bug :)

Comment 18

[Germán Toro del Valle (:gtorodelvalle)]

Yeah, yeah, I mean I did that (the testing in the device :-D) Just wanted to let John have the last word about my cherry-picking ;-) I am 100% confident about it, and of course it works in the device ;-)