Last Comment Bug 714973 - Emergency calls on B2G
: Emergency calls on B2G
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM: Device Interfaces (show other bugs)
: Trunk
: ARM Gonk (Firefox OS)
: -- normal (vote)
: mozilla15
Assigned To: José Antonio Olivera Ortega [:jaoo]
:
Mentors:
Depends on:
Blocks: b2g-telephony
  Show dependency treegraph
 
Reported: 2012-01-03 14:54 PST by Kyle Machulis [:kmachulis] [:qdot]
Modified: 2012-05-16 02:51 PDT (History)
12 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Part 1 (WIP v1) - Add emergency calls to RIL's state machine. (8.42 KB, patch)
2012-03-12 08:58 PDT, José Antonio Olivera Ortega [:jaoo]
no flags Details | Diff | Review
Part 2 (WIP v1) - Add emergency call flag to WebMobileConnection. (4.29 KB, patch)
2012-03-12 09:00 PDT, José Antonio Olivera Ortega [:jaoo]
no flags Details | Diff | Review
(WIP v2) - Add emergency calls to RIL's state machine. (18.23 KB, patch)
2012-03-28 04:42 PDT, José Antonio Olivera Ortega [:jaoo]
no flags Details | Diff | Review
(WIP v3) - Add emergency calls to RIL's state machine. (11.95 KB, patch)
2012-03-31 18:22 PDT, José Antonio Olivera Ortega [:jaoo]
no flags Details | Diff | Review
(WIP v4) - Add emergency calls to RIL's state machine. (8.13 KB, patch)
2012-04-10 06:37 PDT, José Antonio Olivera Ortega [:jaoo]
no flags Details | Diff | Review
(WIP v5) - Add emergency calls to RIL's state machine. (9.27 KB, patch)
2012-04-17 09:23 PDT, José Antonio Olivera Ortega [:jaoo]
no flags Details | Diff | Review
(WIP v6) - Add emergency calls to RIL's state machine. (9.77 KB, patch)
2012-04-26 07:42 PDT, José Antonio Olivera Ortega [:jaoo]
philipp: feedback+
Details | Diff | Review
(WIP v7) - Add emergency calls to RIL's state machine. (10.66 KB, patch)
2012-04-30 12:02 PDT, José Antonio Olivera Ortega [:jaoo]
philipp: review-
Details | Diff | Review
v8 (10.63 KB, patch)
2012-04-30 19:43 PDT, Philipp von Weitershausen [:philikon]
philipp: review+
Details | Diff | Review

Description Kyle Machulis [:kmachulis] [:qdot] 2012-01-03 14:54:47 PST
We need to be able to make emergency calls without a SIM card on B2G. I believe this is a legal regulation in quite a few nations for cell phones.
Comment 1 José Antonio Olivera Ortega [:jaoo] 2012-02-17 05:46:30 PST
I would like to take this bug and start working on it. Is anyone already working on it?
Comment 2 Philipp von Weitershausen [:philikon] 2012-02-17 05:51:43 PST
Go for it!
Comment 3 José Antonio Olivera Ortega [:jaoo] 2012-03-12 08:58:00 PDT
Created attachment 604972 [details] [diff] [review]
Part 1 (WIP v1) - Add emergency calls to RIL's state machine.

This patch adds the possibility of making emergency calls within
RIL's state machine implemented between RadioLayerInterface and
ril_worker workers.

Emergency calls are enabled (without SIM) when the registration
state is equal to 10, 11, 12, or 14. The emergency call could be
established by using the RIL_REQUEST_DIAL_EMERGENCY_CALL
parcel. It seems there is a set of hidden parcels that do not
appears in the RIL interface definition (ril.h). I discovered
that parcel by looking into the Android logs during an emergency
call in a S2 running a stock ROM.

This funtionality could be tested through the Gaia/OWD dialer by
dialing an emergency number (It could be different in every
countries).
Comment 4 José Antonio Olivera Ortega [:jaoo] 2012-03-12 09:00:41 PDT
Created attachment 604974 [details] [diff] [review]
Part 2 (WIP v1) - Add emergency call flag to WebMobileConnection.

This patch add the 'emergencyCallsPossible' information to
existing exposed information through the WebMobileConnection
interface.
Comment 5 José Antonio Olivera Ortega [:jaoo] 2012-03-12 09:04:52 PDT
Previous attachments are work-in-progress patches. Further work is
needed to test it on Akami (especially the 'hidden'
RIL_REQUEST_DIAL_EMERGENCY_CALL parcel).

My idea is to deal with emergency calls through current Telephony API
without any addition or modification. Emergency calls will be allowed
at any time (with SIM present and ready) and handled as an usual
call. Emergency calls are possible when the device is not connected
(SIM absent, pin puk required, etc.) and registration state is equal
to 10, 11, 12 or 14. This information will be exposed through the
WebMobileConnection interface (emergencyCallsPossible). In this
situation any call will be handled as an emergency call but always by
using the dial function of WebTelephony.

Thoughts?

Android saves a list of emergency numbers in a system
property. Typical names for this property are:

- ro.ril.oem.nosim.ecclist
- ril.ecclist
- ro.ril.ecclist
- ro.ril.domestic_ecclist
- persist.ril.ecclist
- ro.ril.oem.ecclist

So the dial number used in any emergency call has to be any of those
included in this list.

Note these patches apply on top of cgjones/m-c git fork. Waiting to
someone willing to upstream everything that was landed to cgjones/m-c
during the work week. Once done I will be happy to change the pacthes
to hg-mq ones.
Comment 6 Philipp von Weitershausen [:philikon] 2012-03-12 10:30:54 PDT
Thanks for the patches, José! I will take a look. Formally we will incorporate the emergencyCallsPossible flag into the final WebMobileConnection implementation, so Part 2 will not be done here, nor does this bug block WebMobileConnection in any way.
Comment 7 Philipp von Weitershausen [:philikon] 2012-03-14 02:20:30 PDT
Comment on attachment 604972 [details] [diff] [review]
Part 1 (WIP v1) - Add emergency calls to RIL's state machine.

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

::: dom/system/b2g/RadioInterfaceLayer.js
@@ +205,5 @@
> +        }
> +        this.radioState.emergencyCallsPossible =
> +          (state.regState >= RIL.NETWORK_CREG_STATE_NOT_SEARCHING_EMERGENCY_CALLS &&
> +           state.regState <= RIL.NETWORK_CREG_STATE_UNKNOWN_EMERGENCY_CALLS) ?
> +           true : false;

This might be easier to read with by initializing `state.regState` to a value and then setting it to the other based on the condition in an `if` statement.

I don't think the logic you're implementing here is quite correct. Unless the `emergencyCallsPossible` flag should only ever be looked at when `connected` is false? This is something we should discuss on dev-webapi because it has implications for WebMobileConnection.

::: dom/system/b2g/ril_consts.js
@@ +141,4 @@
>  const REQUEST_SET_SMSC_ADDRESS = 101;
>  const REQUEST_REPORT_SMS_MEMORY_STATUS = 102;
>  const REQUEST_REPORT_STK_SERVICE_IS_RUNNING = 103;
> +const REQUEST_DIAL_EMERGENCY_CALL = 10016;

Please retain the separating line before RESPONSE_TYPE_SOLICITED.

Also, it seems like this is a non-standard addition to the RIL... which devices support it?

::: dom/system/b2g/ril_worker.js
@@ +67,4 @@
>  // We leave this as 'undefined' instead of setting it to 'false'. That
>  // way an outer scope can define it to 'true' (e.g. for testing purposes)
>  // without us overriding that here.
> +let DEBUG = true;

Please revert this. :)

@@ +1317,5 @@
>  RIL[REQUEST_REPORT_SMS_MEMORY_STATUS] = null;
>  RIL[REQUEST_REPORT_STK_SERVICE_IS_RUNNING] = null;
> +RIL[REQUEST_DIAL_EMERGENCY_CALL] = function REQUEST_DIAL_EMERGENCY_CALL(length) {
> +  Phone.onDialEmergencyCall();
> +};

Let's not add empty methods anymore. This was kind of anti-pattern that we developed. I am working on cleaning these up.

@@ +2053,5 @@
>      RIL.getDataCallList();
>    },
>  
> +  onDialEmergencyCall: function onDialEmergencyCall() {
> +  },

See above. Please remove this.

@@ +2298,5 @@
> +   */
> +  _isEmergencyNumber: function _isEmergencyNumber(number) {
> +    // TODO.
> +    return true;
> +  },

Obviously that still needs to be done, e.g. in a second part to this patch, or in a follow-up... Do you know yet how we can check? Also, if we have to ask rild, how can this be a synchronous call? -- Would like to know more about the details here!
Comment 8 José Antonio Olivera Ortega [:jaoo] 2012-03-28 04:39:29 PDT
(In reply to Philipp von Weitershausen [:philikon] from comment #7)
> Comment on attachment 604972 [details] [diff] [review]
> Part 1 (WIP v1) - Add emergency calls to RIL's state machine.
> 
> Review of attachment 604972 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/system/b2g/RadioInterfaceLayer.js
> @@ +205,5 @@
> > +        }
> > +        this.radioState.emergencyCallsPossible =
> > +          (state.regState >= RIL.NETWORK_CREG_STATE_NOT_SEARCHING_EMERGENCY_CALLS &&
> > +           state.regState <= RIL.NETWORK_CREG_STATE_UNKNOWN_EMERGENCY_CALLS) ?
> > +           true : false;
> 
> This might be easier to read with by initializing `state.regState` to a
> value and then setting it to the other based on the condition in an `if`
> statement.

Yes, you are right. I have changed that to:

+	this.radioState.emergencyCallsOnly =
+          (state.regState != RIL.NETWORK_CREG_STATE_REGISTERED_HOME);

This is because to be able to dial emergency numbers -without SIM- the phone has to reach a registration state 10, 12, 13, or 14. That is the case for S2 (it reaches inmediately state 13 without SIM) but not for Akami. On akami device we never reach any of them. Our friends at Qualcomm have told me that they really don't differentiate between states like REGISTRATION_STATE 2 and 12. The modem also defaults to emergency mode until the SIM is validated at boot. Thoughts?

> 
> I don't think the logic you're implementing here is quite correct. Unless
> the `emergencyCallsPossible` flag should only ever be looked at when
> `connected` is false? This is something we should discuss on dev-webapi
> because it has implications for WebMobileConnection.

Discussed at https://bugzilla.mozilla.org/show_bug.cgi?id=729173#c20.

> 
> ::: dom/system/b2g/ril_consts.js
> @@ +141,4 @@
> >  const REQUEST_SET_SMSC_ADDRESS = 101;
> >  const REQUEST_REPORT_SMS_MEMORY_STATUS = 102;
> >  const REQUEST_REPORT_STK_SERVICE_IS_RUNNING = 103;
> > +const REQUEST_DIAL_EMERGENCY_CALL = 10016;
> 
> Please retain the separating line before RESPONSE_TYPE_SOLICITED.

Done!

> 
> Also, it seems like this is a non-standard addition to the RIL... which
> devices support it?

AFAIK only on Samsung Galaxy S2. This is a problem for us. There are different behaviours in every devices tested. On  S2 there is an special RIL request type for emergency calls. This is a non-standard addition to the RIL (see [1]). On akami we can deal with emergency calls by using the REQUEST_DIAL RIL request type. IMHO the best choice for implementing this is to deal with it as a quirk.

> 
> ::: dom/system/b2g/ril_worker.js
> @@ +67,4 @@
> >  // We leave this as 'undefined' instead of setting it to 'false'. That
> >  // way an outer scope can define it to 'true' (e.g. for testing purposes)
> >  // without us overriding that here.
> > +let DEBUG = true;
> 
> Please revert this. :)

Done!

> 
> @@ +1317,5 @@
> >  RIL[REQUEST_REPORT_SMS_MEMORY_STATUS] = null;
> >  RIL[REQUEST_REPORT_STK_SERVICE_IS_RUNNING] = null;
> > +RIL[REQUEST_DIAL_EMERGENCY_CALL] = function REQUEST_DIAL_EMERGENCY_CALL(length) {
> > +  Phone.onDialEmergencyCall();
> > +};
> 
> Let's not add empty methods anymore. This was kind of anti-pattern that we
> developed. I am working on cleaning these up.
> 
> @@ +2053,5 @@
> >      RIL.getDataCallList();
> >    },
> >  
> > +  onDialEmergencyCall: function onDialEmergencyCall() {
> > +  },
> 
> See above. Please remove this.

Ok, done!

> 
> @@ +2298,5 @@
> > +   */
> > +  _isEmergencyNumber: function _isEmergencyNumber(number) {
> > +    // TODO.
> > +    return true;
> > +  },
> 
> Obviously that still needs to be done, e.g. in a second part to this patch,
> or in a follow-up... Do you know yet how we can check? Also, if we have to
> ask rild, how can this be a synchronous call? -- Would like to know more
> about the details here!

The ril.ecclist property is updated by rild with the current list of emergency numbers. The idea is to retrieve that list and check if the given number matches with any of those contained within the list. 


[1] http://groups.google.com/group/android-porting/browse_thread/thread/d426ccefa561c419
Comment 9 José Antonio Olivera Ortega [:jaoo] 2012-03-28 04:42:18 PDT
Created attachment 610071 [details] [diff] [review]
(WIP v2) - Add emergency calls to RIL's state machine.
Comment 10 Philipp von Weitershausen [:philikon] 2012-03-28 18:41:42 PDT
(In reply to José Antonio Olivera Ortega [:jaoo] from comment #8)
> +	this.radioState.emergencyCallsOnly =
> +          (state.regState != RIL.NETWORK_CREG_STATE_REGISTERED_HOME);
> 
> This is because to be able to dial emergency numbers -without SIM- the phone
> has to reach a registration state 10, 12, 13, or 14. That is the case for S2
> (it reaches inmediately state 13 without SIM) but not for Akami. On akami
> device we never reach any of them. Our friends at Qualcomm have told me that
> they really don't differentiate between states like REGISTRATION_STATE 2 and
> 12. The modem also defaults to emergency mode until the SIM is validated at
> boot. Thoughts?

That means the Akami does its own thing. We should cover this in a RILQUIRK constant and explicitly call it out. Otherwise what's the point of the emergencyCallsOnly at all? We should look at the 1x states on all other spec-compliant devices.

> > Also, it seems like this is a non-standard addition to the RIL... which
> > devices support it?
> 
> AFAIK only on Samsung Galaxy S2. This is a problem for us. There are
> different behaviours in every devices tested. On  S2 there is an special RIL
> request type for emergency calls. This is a non-standard addition to the RIL
> (see [1]). On akami we can deal with emergency calls by using the
> REQUEST_DIAL RIL request type. IMHO the best choice for implementing this is
> to deal with it as a quirk.

I agree. So please do that and add a RILQUIRK constant.
Comment 11 Philipp von Weitershausen [:philikon] 2012-03-28 18:47:22 PDT
Comment on attachment 610071 [details] [diff] [review]
(WIP v2) - Add emergency calls to RIL's state machine.

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

This patch has a lot of unrelated whitespace changes. Please remove them.

::: dom/system/gonk/RadioInterfaceLayer.js
@@ +203,5 @@
> +          return;
> +        }
> +
> +	this.radioState.emergencyCallsOnly =
> +          (state.regState != RIL.NETWORK_CREG_STATE_REGISTERED_HOME);

See comment 10 about making this dependent on a RILQUIRK. For that reason, we probably want to compute emergencyCallsOnly in ril_worker.js. Perhaps even all of radioState.

@@ +264,5 @@
>          this.radioState.cardState = message.cardState;
>          if (!message.cardState || message.cardState == "absent") {
>            this.resetRadioState();
>          }
> +        this.radioState.emergencyCallsOnly = emergencyCallsOnly;

Those two lines can move into the "if" block, no?

@@ +505,5 @@
> +    // Note: If 'this.radioState.emergencyCallsOnly' is true it is because
> +    // 'connected' is false and emergency calls are possible.
> +    this.worker.postMessage({type: "dial",
> +                             number: number,
> +                             emergencyCallsOnly: this.radioState.emergencyCallsOnly});

If radioState, in particular emergencyCallsOnly, is computed in ril_worker.js, then we don't need to channel it back and forth.

::: dom/system/gonk/ril_worker.js
@@ +76,5 @@
>  const PARCEL_SIZE_SIZE = UINT32_SIZE;
>  
>  let RILQUIRKS_CALLSTATE_EXTRA_UINT32 = false;
>  let RILQUIRKS_DATACALLSTATE_DOWN_IS_UP = false;
> +let RILQUIRKS_REQUEST_DIAL_IS_DIAL_EMERGENCY_CALL = false;

Better: RILQUIRKS_REQUEST_USE_DIAL_EMERGENCY_CALL
Comment 12 José Antonio Olivera Ortega [:jaoo] 2012-03-31 18:22:52 PDT
Created attachment 611239 [details] [diff] [review]
(WIP v3) - Add emergency calls to RIL's state machine.

It adds two RIL quirks (RILQUIRKS_REQUEST_USE_DIAL_EMERGENCY_CALL and
RILQUIRKS_MODEM_DEFAULTS_TO_EMERGENCY_MODE) and computes the emergency
call only flag in ril_worker.js.

BTW, on ICS emergency calls use the REQUEST_DIAL RIL request type and
the device reaches inmediately state 13 without SIM so current
implementation should work on it as well.
Comment 13 Philipp von Weitershausen [:philikon] 2012-04-04 22:03:28 PDT
Comment on attachment 611239 [details] [diff] [review]
(WIP v3) - Add emergency calls to RIL's state machine.

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

::: dom/system/gonk/RadioInterfaceLayer.js
@@ +150,5 @@
> +    bars:               null,
> +    operator:           null,
> +    type:               null,
> +    msisdn:             null,
> +    emergencyCallsOnly: null,

So, in bug 729173 I'm revamping this code a bit. I will include your handling of emergencyCallsOnly, so I will not review any changes to RadioInterfaceLayer.js in this patch. Maybe you can remove them from the patch so that we don't get conflicts?

::: dom/system/gonk/ril_worker.js
@@ +665,5 @@
>        }
>        RILQUIRKS_DATACALLSTATE_DOWN_IS_UP = true;
>      }
> +    let ril_impl = libcutils.property_get("gsm.version.ril-impl");
> +    if (ril_impl == "Qualcomm RIL 1.0") {

Interesting. They don't use ril.model_id?

@@ +930,5 @@
> +      if (!this._isEmergencyNumber(options.number)) {
> +        if (DEBUG) {
> +          debug(options.number + " is not a valid emergency number.");
> +        }
> +        return;

We should probably notify an error here so that the DOM will see an error event. Except we don't have errors for telephony yet, so we can't do that. But please add a // TODO comment for that so we don't forget.

@@ +1205,5 @@
> +   * @param number
> +   *        The number to look up.
> +   */
> +   _isEmergencyNumber: function _isEmergencyNumber(number) {
> +     //Check read-write ecclist property first

Nit: space after // and period at the end of the sentence (also in other comments below)

@@ +1207,5 @@
> +   */
> +   _isEmergencyNumber: function _isEmergencyNumber(number) {
> +     //Check read-write ecclist property first
> +     let numbers = libcutils.property_get("ril.ecclist");
> +     if (numbers === "") {

if (!numbers)

@@ +1219,5 @@
> +           return true;
> +         }
> +       }
> +       return false;
> +     }

Much simpler and faster:

  if (numbers) {
    numbers = numbers.split(",");
  } else {
    numbers = DEFAULT_EMERGENCY_NUMBERS;
  }
  return numbers.indexOf(number) != -1;

@@ +1222,5 @@
> +       return false;
> +     }
> +
> +     //No ecclist system property, so use our own list.
> +     return ((number === "112") || (number === "911"));

Please const these values, e.g. using the name from the example above:

  const DEFAULT_EMERGENCY_NUMBERS = ["112", "911"];

@@ +1323,5 @@
>    },
>  
>    _processVoiceRegistrationState: function _processVoiceRegistrationState(state) {
> +    this.initRILQuirks();
> +

This is an interesting case. We need to be very careful about not initializing the RIL quirks too soon. Because the RIL needs to be "warm" for the SGS2 quirks to be detectable, which in this case means there needs to be an active voice or data call.

We might need to split the "call quirks" from the other quirks. I would suggest doing that in order not to jeopardize the SGS2 quirk detection.
Comment 14 José Antonio Olivera Ortega [:jaoo] 2012-04-10 06:37:52 PDT
Created attachment 613575 [details] [diff] [review]
(WIP v4) - Add emergency calls to RIL's state machine.

This new patch addresses all the comments above except what Philikon suggests about splitting the emergency call quirk detection from the other S2 RIL quirks. This is reason because I do not request review right now at anyone until I figure out the best solution for that problem.
Comment 15 José Antonio Olivera Ortega [:jaoo] 2012-04-17 09:23:15 PDT
Created attachment 615737 [details] [diff] [review]
(WIP v5) - Add emergency calls to RIL's state machine.

New WIP patch. This one updates remaining issues still open regarding
how/when to detect the RIL quirks. The 'gsm.version.ril-impl' (see [1])
and 'ril.model_id' system properties are set by rild through
libsecril-client.so at booting time. I have tested that on both
Samsung Galasy S2 and akami devices. I am pretty sure the system
property value 'ril.model_id' will be never empty if we read it when
processing the voice registration state as we commentend on IRC
discussion last week. I touched a bit the initRILQuirks() function in
order to read first the 'gsm.version.ril-impl' property because it
also defined in SGS2 devices and is useful to diferenciate between
both devices.

This pacth is fully tested on both SGS2 and akami devices. Emergency
calls work on decices with SIM as well.

[1] https://github.com/mozilla-b2g/android-hardware-ril/blob/b2g-gingerbread/libril/ril.cpp#L2240
Comment 16 José Antonio Olivera Ortega [:jaoo] 2012-04-25 21:13:09 PDT
Comment on attachment 615737 [details] [diff] [review]
(WIP v5) - Add emergency calls to RIL's state machine.

Need to rebase this pacth. I'll request review at philikon again.
Comment 17 José Antonio Olivera Ortega [:jaoo] 2012-04-26 07:42:57 PDT
Created attachment 618654 [details] [diff] [review]
(WIP v6) - Add emergency calls to RIL's state machine.

Patch rebased. Need feedback about the initRILQuirks function.
Comment 18 Philipp von Weitershausen [:philikon] 2012-04-27 18:14:05 PDT
Comment on attachment 618654 [details] [diff] [review]
(WIP v6) - Add emergency calls to RIL's state machine.

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

Looks good! I can verify what you said in comment 15, so this approach is definitely a good one. The only piece that's missing is updating the info in the mainthread/content process. This would be just a line or two in RadioInterfaceLayer.js and should perhaps just be part of this patch.

::: dom/system/gonk/ril_worker.js
@@ +82,4 @@
>  let RILQUIRKS_CALLSTATE_EXTRA_UINT32 = false;
>  let RILQUIRKS_DATACALLSTATE_DOWN_IS_UP = false;
>  // This flag defaults to true since on RIL v6 and later, we get the
> +// version number via the UNSOLICITED_RIL_CONNECTED Parcel.

Unrelated change, please remove.

@@ +1149,5 @@
> +    let dial_request_type = REQUEST_DIAL;
> +    if (this.voiceRegistrationState.emergencyCallsOnly) {
> +      if (!this._isEmergencyNumber(options.number)) {
> +        if (DEBUG) {
> +          // TODO: Notify an error here so that the DOM will see an error event.

Please file a follow-up bug for this so that we can track that and work on it once bug 712944 has landed, and refer to the bug in this comment.
Comment 19 José Antonio Olivera Ortega [:jaoo] 2012-04-30 12:02:05 PDT
Created attachment 619633 [details] [diff] [review]
(WIP v7) - Add emergency calls to RIL's state machine.

Updated emergencyCallsOnly info in the main/content process. Please, take a look at it. Is that what you mean?
Comment 20 Philipp von Weitershausen [:philikon] 2012-04-30 14:31:18 PDT
Comment on attachment 619633 [details] [diff] [review]
(WIP v7) - Add emergency calls to RIL's state machine.

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

Perfecto
Comment 21 Philipp von Weitershausen [:philikon] 2012-04-30 15:14:49 PDT
(In reply to Philipp von Weitershausen [:philikon] from comment #18)
> @@ +1149,5 @@
> > +    let dial_request_type = REQUEST_DIAL;
> > +    if (this.voiceRegistrationState.emergencyCallsOnly) {
> > +      if (!this._isEmergencyNumber(options.number)) {
> > +        if (DEBUG) {
> > +          // TODO: Notify an error here so that the DOM will see an error event.
> 
> Please file a follow-up bug for this so that we can track that and work on
> it once bug 712944 has landed, and refer to the bug in this comment.

Please don't forget to file this bug!
Comment 22 Philipp von Weitershausen [:philikon] 2012-04-30 17:18:35 PDT
Comment on attachment 619633 [details] [diff] [review]
(WIP v7) - Add emergency calls to RIL's state machine.

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

::: dom/system/gonk/ril_worker.js
@@ +698,5 @@
> +        // The Samsung Galaxy S2 I-9100 radio sends an extra Uint32 in the
> +        // call state.
> +        let model_id = libcutils.property_get("ril.model_id");
> +        if (DEBUG) debug("Detected RIL model " + model_id);
> +          if (model_id == "I9100") {

Bad indention

@@ +709,5 @@
> +            RILQUIRKS_CALLSTATE_EXTRA_UINT32 = true;
> +            RILQUIRKS_DATACALLSTATE_DOWN_IS_UP = true;
> +            RILQUIRKS_REQUEST_USE_DIAL_EMERGENCY_CALL = true;
> +          }
> +        }

Extra } (due to bad indention above).

This manifests as Syntax Error and completely breaks the RIL. Please don't upload patches you haven't tested. It wastes everybody's time.
Comment 23 Philipp von Weitershausen [:philikon] 2012-04-30 17:20:31 PDT
(PS: I highly recommend an editor that understands JS syntax and tells you when you create invalid syntax. Also helps with indentation.)
Comment 24 Philipp von Weitershausen [:philikon] 2012-04-30 19:43:20 PDT
Created attachment 619816 [details] [diff] [review]
v8

fixed version (only because I'm nice... you might not get so lucky next time ;))
Comment 25 José Antonio Olivera Ortega [:jaoo] 2012-05-01 02:03:08 PDT
(In reply to Philipp von Weitershausen [:philikon] from comment #22)
> Comment on attachment 619633 [details] [diff] [review]
> (WIP v7) - Add emergency calls to RIL's state machine.
> 
> Review of attachment 619633 [details] [diff] [review]:
> -----------------------------------------------------------------
> 

> Extra } (due to bad indention above).
> 
> This manifests as Syntax Error and completely breaks the RIL. Please don't
> upload patches you haven't tested. It wastes everybody's time.

Oh, crap! My fault, I made a mistake when requesting review in the last two versions of the patch. I wanted to request feedback about the two last changes before building and testing again but I selected 'review' in the form. You went ahead too fast and I have not had time to let you know about that. Anyway I admit it is my fault. Sorry, sorry, sorry. Gracias.
Comment 26 José Antonio Olivera Ortega [:jaoo] 2012-05-01 03:44:08 PDT
(In reply to Philipp von Weitershausen [:philikon] from comment #24)
> Created attachment 619816 [details] [diff] [review]
> v8
> 
> fixed version (only because I'm nice... you might not get so lucky next time
> ;))

Tested on Samsung Galaxy S2. Need to test it on Akami, I'll do it on Thursday because I have the device in the office. Please, let me know if you tested it on Akami.
Comment 27 Philipp von Weitershausen [:philikon] 2012-05-01 17:49:56 PDT
(In reply to José Antonio Olivera Ortega [:jaoo] from comment #26)
> Tested on Samsung Galaxy S2. Need to test it on Akami, I'll do it on
> Thursday because I have the device in the office. Please, let me know if you
> tested it on Akami.

Did a rudimentary test on the Akami, seems to work fine!
Comment 28 Philipp von Weitershausen [:philikon] 2012-05-02 16:35:43 PDT
https://hg.mozilla.org/mozilla-central/rev/dbafb657a59f

Note You need to log in before you can comment on or make changes to this bug.