996863 - [B2G][Tarako][Dialer][Airplane Mode] Large lag occurs when making an emergency call with airplane mode activated

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Vendcom, defect, P1)

Description

[Rachel Pribble [:rpribble]]

Attachment: Video.ogg

Description:
A large lag (~10 seconds) is experienced when the user tries to dial 112 (state patrol) while airplane mode is activated. The dialer screen appears to freeze up, even if the user tries to tap the dial button repeatedly.

Repro Steps:
1) Update a Tarako to BuildID: 20140415004002
2) Pull down notifications bar and activate airplane mode
3) Tap home and enter dialer 
4) Dial 112 and tap the call button
5) Observe the phone sit on the dialer screen for an extended amount of time (~10 seconds) before finally connecting the emergency phone call

Actual:
Large lag is experienced between making an emergency call in airplane mode and the call actually connecting.

Expected:
Call goes through in a timely fashion (especially in state of emergency).

v1.4 Environmental Variables:
Device: Tarako v1.4 MOZ ril
BuildID: 20140415004002
Gaia: 44ff6248c28ff83b9ad1161847a176399f93d3bb
Gecko: 28aea220e338
Version: 28.1
Firmware Version: sp8810

Repro frequency: 2/2

Attachments: Video

Comment 1

[Rachel Pribble [:rpribble]]

Attachment: emergency_lag_logcat.txt

Comment 2

[Rachel Pribble [:rpribble]]

Attachment: firewatch.txt

Comment 3

[Rachel Pribble [:rpribble]]

The variables in the initial post incorrectly cite 'Tarako v1.4 MOZ ril' when they should read 'Tarako v1.3T'.

This issue does not occur on the Buri v1.3 MOZ ril.

Environmental Variables:
Device: Buri v1.3 MOZ ril
BuildID: 20140414004002
Gaia: 8b92c56267fe50772095f1f25d6cc1f9c9966eb4
Gecko: 3e26908fca71
Version: 28.0
Firmware Version: v1.2-device.cfg

An emergency call made while the device is in airplane mode goes through immediately as normal (~2 seconds).

Comment 4

[Jason Smith [:jsmith]]

Historically, emergency calling has been deemed as a critical requirement that must work in a timely manner in past releases. Bugs that have been associated with this feature typically are certification blockers. On that regard, I'm nominating this block, as I think 10 seconds to start an emergency call is too long.

Comment 5

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

HsinYi, is this MozRIL issue?

Comment 6

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Attachment: log: adb logcat -b radio -b main -v time

log snippet:

=== Dialing 112 when airplane mode ===

#2187 04-16 09:30:05.890 I/Gecko   (   83): TelephonyProvider: Dialing emergency 112
#2188 04-16 09:30:05.890 I/Gecko   (   83): RIL Worker: [0] Received chrome message {"number":"112","isDialEmergency":true,"rilMessageClientId":0,"rilMessageToken":20,"rilMessageType":     "dial"}

=== Gecko turns radio on immediately ===

#2189 04-16 09:30:05.890 I/Gecko   (   83): RIL Worker: [0] Automatically enable radio for an emergency call.
#2190 04-16 09:30:05.890 I/Gecko   (   83): RIL Worker: [0] New outgoing parcel of type 23
#2193 04-16 09:30:05.890 D/RILC    (   98): PCB request code 23 token 101
#2194 04-16 09:30:05.890 D/RILC    (   98): [0101]> RADIO_POWER (1)
#2195 04-16 09:30:05.890 D/RIL     (   98): onRequest: RADIO_POWER sState=0

=== After 9 seconds, modem responds ===

#2265 04-16 09:30:14.130 D/AT      (   98): Channel1: AT< OK
#2266 04-16 09:30:14.130 D/RILC    (   98): [0101]< RADIO_POWER
#2267 04-16 09:30:14.130 D/RIL     (   98): put Channel ID '1'
#2268 04-16 09:30:14.130 I/RILC    (   98): -->CommandThread [350] free one comma

=== In 1 second, gecko makes a DIAL request ===

#2452 04-16 09:30:14.240 I/Gecko   (   83): RIL Worker: [0] New outgoing parcel of type 10
#2715 04-16 09:30:14.510 I/Gecko   (   83): RIL Worker: [0] Solicited response for request type 10, token 109, error 0
#2716 04-16 09:30:14.510 I/Gecko   (   83): RIL Worker: [0] Handling parcel as REQUEST_DIAL

Comment 7

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Per analysis on comment 6, modem/rilc spent 9 seconds on turning radio on. It seems not obviously a RIL issue, more like a modem performance issue instead.

Comment 8

[Jason Smith [:jsmith]]

(In reply to Hsin-Yi Tsai  [:hsinyi] from comment #7)
> Per analysis on comment 6, modem/rilc spent 9 seconds on turning radio on.
> It seems not obviously a RIL issue, more like a modem performance issue
> instead.

So is this a vendor bug?

Comment 9

[Hsin-Yi Tsai (she/her) [:hsinyi]]

(In reply to Jason Smith [:jsmith] from comment #8)
> (In reply to Hsin-Yi Tsai  [:hsinyi] from comment #7)
> > Per analysis on comment 6, modem/rilc spent 9 seconds on turning radio on.
> > It seems not obviously a RIL issue, more like a modem performance issue
> > instead.
> 
> So is this a vendor bug?

I think so, but let's ask for their opinions first.

Hi Sam,

Any ideas of the issue? I don't think there's much gecko could do, but please let me know if you see some. Thanks.

Comment 10

[sam.hua]

yes, it is modem issue.we need more than 10s to complete the register of voice network after turn off airplane mode.

maybe gaia could add a waiting icon ?

Comment 11

[thomas tsai]

Need gaia help

Comment 12

[Evelyn Hung]

(In reply to sam.hua from comment #10)
> yes, it is modem issue.we need more than 10s to complete the register of
> voice network after turn off airplane mode.
> 
> maybe gaia could add a waiting icon ?

I don't think adding a waiting icon helps. Per comment 4, it's not a user experience issue but a performance issue or a certification blocker. Is it possible to improve from modem?

Comment 13

[sam.hua]

yes, if we add a waiting icon, it is a performance issue
but if we don't add it , it will be a performance and user experience issue

Comment 14

[James Zhang (Spreadtrum)]

spreadtrum known issue, need gaia workaround.

Comment 15

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

Etienne, could you take a look to see if you can work your magic here please?  :)

Comment 16

[Etienne Segonzac (:etienne)]

(In reply to Naoki Hirata :nhirata (please use needinfo instead of cc) from comment #15)
> Etienne, could you take a look to see if you can work your magic here
> please?  :)

Sadly I can't :/
Going out of the airplane mode from the notification tray also takes ~20sec on the tarako, so I guess this is platform. The call is only dialed once we're out of airplane mode, and the callscreen only shows once the call is dialed.

Comment 17

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

Ouch.  Thanks for the info.

I think what was asked was some UI around the performance hit.  I should have asked UX first.

Comment 18

[James Zhang (Spreadtrum)]

It's modem known issue, we need gaia workaround.
Arvin, can you take it?

Comment 19

[Stephany Wilkes]

Carrie is on PTO so flagging Victoria and Omega to advise on our system pattern for "waiting but something is happening," which I believe is spinner - still, defer to others to be sure.

Comment 20

[Omega Feng [:Omega] [:馮於懋]]

I think it's Spinner too, although it's not that good.
We can put it on the Call button, or on a toast with text like "Connecting..." or "Dialing...".

Comment 21

[Victoria Gerchinhoren [:vicky]]

Hi All, 

We've defined the use of activities indicators to follow a pattern depending on the scenario, find the description here:

https://wiki.mozilla.org/Gaia/Design/BuildingBlocks#Progress_.26_Activity_Indicators

Note that all this cases are building blocks, please use them.

Thanks!

Comment 22

[helloarvin]

The same issue exists on Qcom V1.3 version.

please wei help to fix it referred to the comment20 and comment21.

Thanks.

Comment 23

[James Zhang (Spreadtrum)]

(In reply to helloarvin from comment #22)
> The same issue exists on Qcom V1.3 version.
> 
> please wei help to fix it referred to the comment20 and comment21.
> 
> Thanks.

Remove POVB.

Comment 24

[Wei Gao (Spreadtrum)]

I suggest to refresh the information of radioState at a higher frequencies during dialing emergency call. When dialing emergency number at the close state of radio, show the notice and after detecting the radio is turned on, close the notice.

Thanks.

Comment 25

[Wei Gao (Spreadtrum)]

(In reply to Victoria Gerchinhoren [:vicky] from comment #21)
> Hi All, 
> 
> We've defined the use of activities indicators to follow a pattern depending
> on the scenario, find the description here:
> 
> https://wiki.mozilla.org/Gaia/Design/BuildingBlocks#Progress_.
> 26_Activity_Indicators
> 
> Note that all this cases are building blocks, please use them.
> 
> Thanks!

The design on the website is pretty beautiful, but I'm sorry to say all of them can't be download at any download link. 

Thanks.

Comment 26

[Wei Gao (Spreadtrum)]

Attachment: bug996863_wait_radio.patch

The bug996863_wait_radio.patch is a patch for this bug. 

At the beginning, As telephony.oncallschanged in calls_handler.js of callscreen app is executed when the '112' is really connected, like that, I registered window.navigator.mozTelephony.oncallschanged = onCallsChanged in telephony_helper.js of communication app. I hope this will do like in calls_handler.js, so I can close the radio notice in onCallsChanged. but I found that onCallsChanged in telephony_helper.js is executed once dialEmergency() is done. That's my wonder.

So I change strategy and decide to refresh the information of radioState at a higher frequencies during dialing emergency call. When dialing emergency number at close state of radio, show the notice, after detecting the radio is turned on, close the notice.

But the question is the emergency parameter of return call is not correct after executing dialEmergency() with a sim card inside, and there is no interface of emergency number judgement, so I have to list all the emergency numbers.

So I summarize my comment:
1) Can anyone help me explain why onCallsChanged in telephony_helper.js is executed once dialEmergency() is done.
2) Can FireFox OS provide a interface of emergency number judgement?
3) Is the emergency parameter of return call is not correct after executing dialEmergency() with a sim card inside a bug? 

Please help to review.
Thanks.

Comment 27

[Evelyn Hung]

Comment on attachment 8429046 [details] [diff] [review]
bug996863_wait_radio.patch

I can't review communication, redirect to etienne.

Comment 28

[Etienne Segonzac (:etienne)]

Comment on attachment 8429046 [details] [diff] [review]
bug996863_wait_radio.patch

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

I don't think this approach is going to work.

What we could do is:
- when we're calling dialEmergency (and only then)
- check the mozSetting |ril.radio.disabled|
- if it's on, show the message until it's off
- do nothing otherwise

::: apps/communications/dialer/js/telephony_helper.js
@@ +107,5 @@
> +
> +        // As the return call.emergency from dialEmergency() is not correct with a sim inside,
> +        // and there is no moz interface of emergency number judgement,
> +        // I have to list all the emergency numbers as follow.w
> +        if(sanitizedNumber == '112' || sanitizedNumber == '911') {

we can't do that.
the list of emergency calls depends on the country/sim/etc...

it is maintained by gecko we don't have access to it here...

@@ +111,5 @@
> +        if(sanitizedNumber == '112' || sanitizedNumber == '911') {
> +          var _ = navigator.mozL10n.get;
> +          ConfirmDialog.show(
> +            _('RadioNotAvailable'),
> +            _('connecting') + ' ...',

I understand we can't land new l10n strings, but we need to file a follow up bug right away to create a real l10n string for this instead of composing from various existing ones.

Comment 29

[Wei Gao (Spreadtrum)]

(In reply to Etienne Segonzac (:etienne) from comment #28)
> Comment on attachment 8429046 [details] [diff] [review]
> bug996863_wait_radio.patch
> 
> Review of attachment 8429046 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I don't think this approach is going to work.
> 
> What we could do is:
> - when we're calling dialEmergency (and only then)
> - check the mozSetting |ril.radio.disabled|
> - if it's on, show the message until it's off
> - do nothing otherwise
> 
> ::: apps/communications/dialer/js/telephony_helper.js
> @@ +107,5 @@
> > +
> > +        // As the return call.emergency from dialEmergency() is not correct with a sim inside,
> > +        // and there is no moz interface of emergency number judgement,
> > +        // I have to list all the emergency numbers as follow.w
> > +        if(sanitizedNumber == '112' || sanitizedNumber == '911') {
> 
> we can't do that.
> the list of emergency calls depends on the country/sim/etc...
> 
> it is maintained by gecko we don't have access to it here...
> 
> @@ +111,5 @@
> > +        if(sanitizedNumber == '112' || sanitizedNumber == '911') {
> > +          var _ = navigator.mozL10n.get;
> > +          ConfirmDialog.show(
> > +            _('RadioNotAvailable'),
> > +            _('connecting') + ' ...',
> 
> I understand we can't land new l10n strings, but we need to file a follow up
> bug right away to create a real l10n string for this instead of composing
> from various existing ones.

At the beginning I cycle checked the mozSetting |ril.radio.disabled| to decide when to hide the message. But I found the "ril.radio.disabled" setting was delay a few seconds, that's to say even the emergency call is called, the "ril.radio.disabled" was still "true". So I check navigator.mozMobileConnections[i].radioState instead of "ril.radio.disabled" setting.

As there is no available APIs to obtain the list of emergency calls in Gaia, I don't know how to get the list of emergency calls. I think only if know the emergency calls list, Gaia can judge whether to show the message. So can you provide some suggesttion about how Gaia can get the list?

About the new l10n strings, are "Radio is now disabled" and "Turning on the radio" OK?
Can you give some suggesttion about this?

Thank Etienne very much.

Comment 30

[Etienne Segonzac (:etienne)]

(In reply to wei.gao from comment #29)
> 
> At the beginning I cycle checked the mozSetting |ril.radio.disabled| to
> decide when to hide the message. But I found the "ril.radio.disabled"
> setting was delay a few seconds, that's to say even the emergency call is
> called, the "ril.radio.disabled" was still "true". So I check
> navigator.mozMobileConnections[i].radioState instead of "ril.radio.disabled"
> setting.

Interesting, new proposition :) It's not ideal but it might work.
- we show a "Connecting..." message when calling dialEmergency
- We add an visibilitychange event listener
  - if the visibility changes, the call screen is now displayed, we can remove the message
- we also keep a safety timeout to remove the message (and the visibility change event listener) in case the callscreen never appears.

> 
> About the new l10n strings, are "Radio is now disabled" and "Turning on the
> radio" OK?
> Can you give some suggesttion about this?

That's actually a UX question, my point was just that we should never concatenate multiple l10n strings witch each other but make it 1 string because the order might be different in some languages.

Comment 31

[Wei Gao (Spreadtrum)]

(In reply to Etienne Segonzac (:etienne) from comment #30)
> 
> Interesting, new proposition :) It's not ideal but it might work.
> - we show a "Connecting..." message when calling dialEmergency
> - We add an visibilitychange event listener
>   - if the visibility changes, the call screen is now displayed, we can
> remove the message
> - we also keep a safety timeout to remove the message (and the visibility
> change event listener) in case the callscreen never appears.

I know "telephony.oncallschanged" in calls_handler.js of callscreen app is executed when a call is really connected, like that, I registered "window.navigator.mozTelephony.oncallschanged = onCallsChanged" in telephony_helper.js of communication app. I hope this will do like in calls_handler.js, so I can close the message in onCallsChanged when a call is connected. but I found that onCallsChanged in telephony_helper.js is executed once press call button instead of this call is really connected. I don't know the reason of this performance.

Hi Etienne
Can you give some suggestion of my operation about this?
Is my operation has some question?
Can you help me?

Thanks.

Comment 32

[Etienne Segonzac (:etienne)]

(In reply to wei.gao from comment #31)
> Can you give some suggestion of my operation about this?

oncallschanged will always be triggered at least once when the RIL is ready, so it's possible you're getting a first oncallschanged where telephony.calls is an empty array.

Comment 33

[Wei Gao (Spreadtrum)]

Attachment: Bug996863_visibilitychange.patch

HI Etienne

I tried to test visibilitychange to remove the message when this event happen. I found that this event delay about two seconds after the message is hiden by callscreen.

The trouble is if we hang up as soon as the callscreen display, visibilitychange event will not execute forever.

So this is a wonder for me. Do you have any other idea for this?
Thanks very much.

Comment 34

[Wei Gao (Spreadtrum)]

Attachment: Bug996863_v1.4_oncallschanged.patch

HI Etienne

This patch work normal on v1.4.

I found it feasible using oncallschanged event on v1.4 to judge when to remove message. The oncallschanged event will do when a call is really connected.

But this method is not ok on v1.3t as oncallschanged event execute once press call button instead of this call is really connected. 

I don't know why.

Comment 35

[Etienne Segonzac (:etienne)]

Comment on attachment 8436608 [details] [diff] [review]
Bug996863_visibilitychange.patch

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

Yes this should work and worst case scenario the user will be able to dismiss the dialog.

Some comments below, and we will also need to cover this with the test.
You can find inspiration on visibility change related tests here [1].

[1] https://github.com/mozilla-b2g/gaia/blob/master/apps/communications/dialer/test/unit/dialer_test.js#L362-366

::: apps/communications/dialer/js/telephony_helper.js
@@ +81,5 @@
>          error();
>          return;
>        } else if (emergencyOnly) {
> +
> +        window.addEventListener("visibilitychange", function hideDialog() {

we should only add the listener after the ConfirmDialog.show and remove the listener the first time it gets triggered.

@@ +88,5 @@
> +        );
> +
> +        var _ = navigator.mozL10n.get;
> +        ConfirmDialog.show(
> +          _('connecting') + ' ...',

we still need to open a bug to create a new l10n key for this and reference it here

Comment 36

[Wei Gao (Spreadtrum)]

Attachment: Bug996863_visibilitychange_2.patch

> Yes this should work and worst case scenario the user will be able to dismiss the dialog.
> 
> Some comments below, and we will also need to cover this with the test.
> You can find inspiration on visibility change related tests here [1].
> 
> [1] https://github.com/mozilla-b2g/gaia/blob/master/apps/communications/dialer/test/unit/dialer_test.js#L362-> 366

HI Etienne
 I modify the code refer to your recommendation. The issue that "visibilitychange" event delay about two seconds after the message is hiden by callscreen is still exist. If we hang up as soon as the callscreen display, the message can only be closed manually. So can this patch be granted?

 The "Connecting" l10n key is existent in "dialer.properties", is this string can't meet our demand?

 Thanks.

Comment 37

[Etienne Segonzac (:etienne)]

(In reply to wei.gao from comment #36)
> Created attachment 8440560 [details] [diff] [review]
> Bug996863_visibilitychange_2.patch
> 
> > Yes this should work and worst case scenario the user will be able to dismiss the dialog.
> > 
> > Some comments below, and we will also need to cover this with the test.
> > You can find inspiration on visibility change related tests here [1].
> > 
> > [1] https://github.com/mozilla-b2g/gaia/blob/master/apps/communications/dialer/test/unit/dialer_test.js#L362-> 366
> 
> HI Etienne
>  I modify the code refer to your recommendation. The issue that
> "visibilitychange" event delay about two seconds after the message is hiden
> by callscreen is still exist. If we hang up as soon as the callscreen
> display, the message can only be closed manually. So can this patch be
> granted?

As long as we display the message only when calling dialEmergency it's ok.

> 
>  The "Connecting" l10n key is existent in "dialer.properties", is this
> string can't meet our demand?

If you have to manually add the "..." at the end it means we need a new string eventually.

Comment 38

[Wei Gao (Spreadtrum)]

(In reply to Etienne Segonzac (:etienne) from comment #37)
>
> If you have to manually add the "..." at the end it means we need a new
> string eventually.

Hi Etienne

I have filed a bug to do this, 
As i don't know assign to who, so please help to check.

https://bugzilla.mozilla.org/show_bug.cgi?id=1026318

Thanks very much.

Comment 39

[Etienne Segonzac (:etienne)]

(In reply to wei.gao from comment #38)
> (In reply to Etienne Segonzac (:etienne) from comment #37)
> >
> > If you have to manually add the "..." at the end it means we need a new
> > string eventually.
> 
> Hi Etienne
> 
> I have filed a bug to do this, 
> As i don't know assign to who, so please help to check.
> 
> https://bugzilla.mozilla.org/show_bug.cgi?id=1026318

All good, can you reference a link to this bug in the final version of this patch so we don't forget to add the l10n string on master later? Thanks!

Comment 40

[Wei Gao (Spreadtrum)]

Attachment: Bug996863_visibilitychange.patch

Hi Etienne

I am glad to do that.
Does it mean I add a new comment and post this link in it when the final version of this patch be granted?

I modified the "_('connecting') + ' ...'" to "_('connecting...)'" so as to match a new l10n string "Connecting ...", can this patch be granted or I should change some what?

Thanks very much.

Comment 41

[Etienne Segonzac (:etienne)]

(In reply to wei.gao from comment #40)
> Created attachment 8441801 [details] [diff] [review]
> Bug996863_visibilitychange.patch
> 
> Hi Etienne
> 
> I am glad to do that.
> Does it mean I add a new comment and post this link in it when the final
> version of this patch be granted?
> 
> I modified the "_('connecting') + ' ...'" to "_('connecting...)'" so as to
> match a new l10n string "Connecting ...", can this patch be granted or I
> should change some what?
> 

But the key 'connecting...' does not exist yet :/
Sorry I wasn't clear. For this bug, you can use the 'connecting' like you did since we can't land new l10n keys, you just need to put a comment a top mentioning that this should be fixed in https://bugzilla.mozilla.org/show_bug.cgi?id=1026318.

Comment 42

[Wei Gao (Spreadtrum)]

(In reply to Etienne Segonzac (:etienne) from comment #41)

> But the key 'connecting...' does not exist yet :/
> Sorry I wasn't clear. For this bug, you can use the 'connecting' like you
> did since we can't land new l10n keys, you just need to put a comment a top
> mentioning that this should be fixed in
> https://bugzilla.mozilla.org/show_bug.cgi?id=1026318.

Ok, Thanks.
Do you mean I should first use 'connecting' first and land it on, then the bug1026318 will be fixed.
After that, I change the 'connecting' to "_('connecting...)'" to completely repair this bug?
Does my understanding right?

But should I use "_('connecting') + ' ...'  or "_('connecting')" first?
Thanks.

Comment 43

[Etienne Segonzac (:etienne)]

(In reply to wei.gao from comment #42)
> (In reply to Etienne Segonzac (:etienne) from comment #41)
> 
> > But the key 'connecting...' does not exist yet :/
> > Sorry I wasn't clear. For this bug, you can use the 'connecting' like you
> > did since we can't land new l10n keys, you just need to put a comment a top
> > mentioning that this should be fixed in
> > https://bugzilla.mozilla.org/show_bug.cgi?id=1026318.
> 
> Ok, Thanks.
> Do you mean I should first use 'connecting' first and land it on, then the
> bug1026318 will be fixed.
> After that, I change the 'connecting' to "_('connecting...)'" to completely
> repair this bug?
> Does my understanding right?
> 
> But should I use "_('connecting') + ' ...'  or "_('connecting')" first?
> Thanks.

You should use _('connecting') + ' ...' for this patch, once it lands we'll make another patch for bug1026318

Comment 44

[Wei Gao (Spreadtrum)]

Attachment: Bug996863_visibilitychange.patch

(In reply to Etienne Segonzac (:etienne) from comment #43)
> You should use _('connecting') + ' ...' for this patch, once it lands we'll
> make another patch for bug1026318

Hi Etienne
So can this patch be granted to be landed on?
Thanks.

Comment 45

[Etienne Segonzac (:etienne)]

(In reply to wei.gao from comment #44)
> Created attachment 8441976 [details] [diff] [review]
> Bug996863_visibilitychange.patch
> 
> (In reply to Etienne Segonzac (:etienne) from comment #43)
> > You should use _('connecting') + ' ...' for this patch, once it lands we'll
> > make another patch for bug1026318
> 
> Hi Etienne
> So can this patch be granted to be landed on?
> Thanks.

See Comment 35, we still need to cover this with a test (I gave some pointers during the earlier review).
Please use the review flag on github when it's done it'll get my attention faster :)

Comment 46

[Wei Gao (Spreadtrum)]

Attachment: Bug996863_visibilitychange.patch

(In reply to Etienne Segonzac (:etienne) from comment #45)
> See Comment 35, we still need to cover this with a test (I gave some
> pointers during the earlier review).
> Please use the review flag on github when it's done it'll get my attention
> faster :)

Hi Etienne

I have added a test in patch, can you help to check it?
As I know little about test file, there may be some issue in it.
Please help to correct me,
Thanks a great.

Comment 47

[Etienne Segonzac (:etienne)]

Comment on attachment 8443875 [details] [diff] [review]
Bug996863_visibilitychange.patch

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

- Your test should go in telephony_helper_test.js not dialer_test.js
- It should be part of the emergency dialing suite [1]
- The assertion making sure the message is displayed should look similar to this one [2]
- The assertion making sure that we hide the message should look similar to this one [3]

Hope this helps!

[1] https://github.com/mozilla-b2g/gaia/blob/master/apps/communications/dialer/test/unit/telephony_helper_test.js#L104
[2] https://github.com/mozilla-b2g/gaia/blob/master/apps/communications/dialer/test/unit/telephony_helper_test.js#L378
[3] https://github.com/mozilla-b2g/gaia/blob/master/apps/communications/dialer/test/unit/dialer_test.js#L362

Comment 48

[Wei Gao (Spreadtrum)]

Attachment: Bug996863_show_message_when_emergencyOnly.patch

Hi Etienne

I am sorry to trouble you again. I learned the test file you recommended me kindly, and then I modified this patch.
Can you help to review this patch. Maybe there still have some question, I will sorry for that.

Thanks a great.

Comment 49

[Etienne Segonzac (:etienne)]

Comment on attachment 8446925 [details] [diff] [review]
Bug996863_show_message_when_emergencyOnly.patch

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

::: apps/communications/dialer/test/unit/telephony_helper_test.js
@@ +122,5 @@
> +                                                'emergencyDialogBtnOk');
> +      }).then(done, done);
> +    });
> +
> +    test('should hide the connecting message', function() {

we should first do a |subject.call('112', '0)| like the first test to display the message, this test isn't actually going through your code.

@@ +125,5 @@
> +
> +    test('should hide the connecting message', function() {
> +      document.dispatchEvent(new CustomEvent('visibilitychange'));
> +      message = document.getElementById('confirmation-message');
> +      assert.isTrue(message.classList.contains('displayed'));

I  think we should spy |ConfirmDialog.hide()| instead and make sure it gets called when the visibilitychange is dispatched.

Comment 50

[Wei Gao (Spreadtrum)]

Attachment: Bug996863_show_message_when_emergencyOnly.patch

Hi Etienne

Thank you for your patience to answer.
I modified this patch again.
Please help to check it, if there still have some mistakes, please let me know without hesitation.

Thanks a lot.

Comment 51

[James Zhang (Spreadtrum)]

Wei, does v1.4 dolphin have this issue.

Comment 52

[Wei Gao (Spreadtrum)]

(In reply to James Zhang (Spreadtrum) from comment #51)
> Wei, does v1.4 dolphin have this issue.

Hi James

This also happen on v1.4 dolphin.
After I test it, the only difference is v1.4 dolphin costs 8~9 seconds a little less than v1.3 tarako when making an emergency call with airplane mode activated.

Comment 53

[Etienne Segonzac (:etienne)]

Comment on attachment 8447618 [details] [diff] [review]
Bug996863_show_message_when_emergencyOnly.patch

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

Hey, the test are not working but it should be pretty easy to fix once you get set up to run them locally (it'll be worth it I promise :)).
You can go to https://developer.mozilla.org/en-US/Firefox_OS/Platform/Automated_testing/Gaia_unit_tests, running |./bin/gaia-test| and saving telephony_helper.js should get you most of the way there.

Please ask me for review as soon as the tests are green locally on your computer.

::: apps/communications/dialer/test/unit/telephony_helper_test.js
@@ +114,4 @@
>        MockIccHelper.mCardState = initialState;
>      });
>  
> +    test('should display the connecting message', function() {

you need to get the |done| function as a parameter here, |function(done) {...|

@@ +114,5 @@
>        MockIccHelper.mCardState = initialState;
>      });
>  
> +    test('should display the connecting message', function() {
> +      mockPromise = Promise.reject('EmergencyOnly');

you probably don't need the pomise

@@ +118,5 @@
> +      mockPromise = Promise.reject('EmergencyOnly');
> +      subject.call('112', 0);
> +      mockPromise.catch(function() {
> +        sinon.assert.calledWith(spyConfirmShow, 'connecting',
> +                                                'emergencyDialogBtnOk');

those are not the exacts args your passing to spyConfirmShow (your also passing a callback and call)

@@ +122,5 @@
> +                                                'emergencyDialogBtnOk');
> +      }).then(done, done);
> +    });
> +
> +    test('should hide the connecting message', function() {

you need to add the spy first
this.sinon.spy(ConfirmDialog, 'hide')

@@ +126,5 @@
> +    test('should hide the connecting message', function() {
> +      subject.call('112', 0);
> +      document.dispatchEvent(new CustomEvent('visibilitychange'));
> +      sinon.assert.called(ConfirmDialog, 'hide');
> +    });

probably |calledOnce|

Comment 54

[Wei Gao (Spreadtrum)]

Attachment: Bug996863_show_message_when_emergencyOnly.patch

Hi Etienne

I think it can work fine now.
But "sinon.assert.called(spyConfirmHide);" can't be made calledOnce. I don't know why.
Please help to review it.
Thanks very much.

Comment 55

[Etienne Segonzac (:etienne)]

Comment on attachment 8451202 [details] [diff] [review]
Bug996863_show_message_when_emergencyOnly.patch

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

Almost there!
The last comments should be easy to address.
You can already take a screenshot of the new dialog and attach it for ui-review.

::: apps/communications/dialer/js/telephony_helper.js
@@ +90,3 @@
>          onerror();
>          return;
>        } else if (emergencyOnly) {

you need to but this inside a loadConfirm(function() {}); block

@@ +100,5 @@
> +              ConfirmDialog.hide();
> +            }
> +          }
> +        );
> +        document.addEventListener("visibilitychange", function hideDialog() {

please use single quotes

::: apps/communications/dialer/test/unit/telephony_helper_test.js
@@ +114,1 @@
>        MockIccHelper.mCardState = initialState;

since we're adding the visibility change listener every time, please add a
|      document.dispatchEvent(new CustomEvent('visibilitychange'));| in the teardown() function to make sure we remove it.

@@ +122,5 @@
> +    test('should hide the connecting message', function() {
> +      subject.call('112', 0);
> +      var spyConfirmHide = this.sinon.spy(ConfirmDialog, 'hide');
> +      document.dispatchEvent(new CustomEvent('visibilitychange'));
> +      sinon.assert.called(spyConfirmHide);

then you'll be able to switch to |calledOnce|

Comment 56

[Wei Gao (Spreadtrum)]

Attachment: Bug996863_show_message_when_emergencyOnly.patch

That is cool.
I am glad to see your suggestion. 
By the way, does mozilla have some learning documents about how to write unit test? I think that's too hard for me as I don't know its rules.
Thank you very much for your patience.

Comment 57

[Etienne Segonzac (:etienne)]

Comment on attachment 8451561 [details] [diff] [review]
Bug996863_show_message_when_emergencyOnly.patch

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

r=me with a last change (see below).

Thanks for making it work, can you open a pull request with this patch?

::: apps/communications/dialer/js/telephony_helper.js
@@ +107,5 @@
> +            }
> +          );
> +        });
> +
> +        document.addEventListener('visibilitychange', function hideDialog() {

this part should also be in the loadConfirm block

Comment 58

[Wei Gao (Spreadtrum)]

Attachment: [master] pull request

Hi Etienne

This is a pull request for master.
Please help to review it.
Thanks a great.

Comment 59

[Etienne Segonzac (:etienne)]

Comment on attachment 8452257 [details] [review]
[master] pull request

r=me with the last comment (on github) addressed.

Comment 60

[Wei Gao (Spreadtrum)]

(In reply to Etienne Segonzac (:etienne) from comment #59)
> Comment on attachment 8452257 [details] [review]
> [master] pull request
> 
> r=me with the last comment (on github) addressed.

I am sorry to trouble you so many times.
I have modified it.
I can't help to say your help is very valuable to me, Thank your for your kindly help.
May you have a beautiful life.

Comment 61

[Wei Gao (Spreadtrum)]

Comment on attachment 8452257 [details] [review]
[master] pull request

Hi Etienne

I have modified this patch.
Can it be granted now?
Thanks a great.

Comment 62

[Etienne Segonzac (:etienne)]

Comment on attachment 8452257 [details] [review]
[master] pull request

All good, can you squash the 2 commits of the pull request into one, then we'll be able to merge the pull request!

Comment 63

[Wei Gao (Spreadtrum)]

(In reply to Etienne Segonzac (:etienne) from comment #62)
> Comment on attachment 8452257 [details] [review]
> [master] pull request
> 
> All good, can you squash the 2 commits of the pull request into one, then
> we'll be able to merge the pull request!

Ok, I will do that now.
You are so kind.
Thanks so much.

Comment 64

[Wei Gao (Spreadtrum)]

Comment on attachment 8452257 [details] [review]
[master] pull request

Dear Etienne

Although there were some small problems when I squashed the 2 commits of the pull request into one, I made it ok at last.
Could you help to check it, can it be merged now? or do I have to recommit a PR to instead?

Thanks a lot.

Comment 65

[Etienne Segonzac (:etienne)]

Comment on attachment 8452257 [details] [review]
[master] pull request

all good, I'll make sure it lands once the tests are green.

Comment 66

[Etienne Segonzac (:etienne)]

https://github.com/mozilla-b2g/gaia/commit/9bc0f7a9a0c1dd74ad01d6e045b0c9d715deefa8

Congrats!

Comment 67

[Wei Gao (Spreadtrum)]

(In reply to Etienne Segonzac (:etienne) from comment #66)
> https://github.com/mozilla-b2g/gaia/commit/
> 9bc0f7a9a0c1dd74ad01d6e045b0c9d715deefa8
> 
> Congrats!

Oh, yes, How exciting it is. I can't help to appreciate your kindly help.

By the way, could it be landed on v1.3t and v1.4 also?
Thanks a great.

Comment 68

[Etienne Segonzac (:etienne)]

(In reply to Wei Gao (Spreadtrum) from comment #67)
> By the way, could it be landed on v1.3t and v1.4 also?
> Thanks a great.

The tracking flags are set properly so it should happen soon.