B2G CDMA Tell if we are on CDMA network

RESOLVED FIXED in mozilla21

Status

()

Core
DOM: Device Interfaces
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: kk1fff, Assigned: kk1fff)

Tracking

(Blocks: 1 bug)

unspecified
mozilla21
ARM
Gonk (Firefox OS)
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

We need to tell if we are on CDMA network. We can get current radio technology from UNSOLICITED_RESPONSE_RADIO_STATE_CHANGED handler, or use REQUEST_VOICE_RADIO_TECH that is introduced in RIL v7.
Created attachment 705245 [details] [diff] [review]
Patch: Tell if we are on CDMA network
Blocks: 830264
(Assignee)

Updated

5 years ago
Attachment #705245 - Flags: review?(vyang)
Comment on attachment 705245 [details] [diff] [review]
Patch: Tell if we are on CDMA network

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

r=me with some nits fixed :)

::: dom/system/gonk/ril_consts.js
@@ +2309,5 @@
>    "evdob",
>    "ehrpd",
>    "lte",
>    "hspa+",
> +  "gsm"

You'll also need a NETWORK_CREG_TECH_GSM.

::: dom/system/gonk/ril_worker.js
@@ +3268,5 @@
> +        radioTechString == 'hsupa' ||
> +        radioTechString == 'hspa' ||
> +        radioTechString == 'lte' ||
> +        radioTechString == 'hspa+' ||
> +        radioTechString == 'gsm') {

Could you compare radioTech with NETWORK_CREG_TECH_* instead?

@@ +4951,5 @@
>    this.acknowledgeIncomingGsmSmsWithPDU(success, responsePduLen, options);
>  };
> +RIL[REQUEST_VOICE_RADIO_TECH] = function REQUEST_VOICE_RADIO_TECH(length, options) {
> +  let radioTech = Buf.readUint32List();
> +  if (radioTech && radioTech.length > 0) {

Please check rilRequestError first.

@@ +4954,5 @@
> +  let radioTech = Buf.readUint32List();
> +  if (radioTech && radioTech.length > 0) {
> +    this._processRadioTech(radioTech[0]);
> +  } else {
> +    debug("Error: REQUEST_VOICE_RADIO_TECH has no tech!"); // Should not happen.

It seems it's unnecessary. Just remove it.
Created attachment 705805 [details] [diff] [review]
Patch: Tell if we are on CDMA network v2

Fix according to previous review and use switch/case to identify GSM family.
Attachment #705245 - Attachment is obsolete: true
Attachment #705245 - Flags: review?(vyang)
Attachment #705805 - Flags: review?(vyang)
Attachment #705805 - Flags: review?(vyang) → review+
Comment on attachment 705805 [details] [diff] [review]
Patch: Tell if we are on CDMA network v2

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

::: dom/system/gonk/ril_worker.js
@@ +5012,5 @@
>    this.acknowledgeIncomingGsmSmsWithPDU(success, responsePduLen, options);
>  };
> +RIL[REQUEST_VOICE_RADIO_TECH] = function REQUEST_VOICE_RADIO_TECH(length, options) {
> +  if (options.rilRequestError) {
> +    return;

Sorry, I should have commented in previous review, but could you add a debug message here? If somehow we have to fetch voice radio tech and it fails, then the following steps to fetch necessary info will never proceed. Having an debug message here gives a handy hint for development phase.
Created attachment 706225 [details] [diff] [review]
Patch for checkin: Tell if we are on CDMA network

Adding comment according to comment 4.
Attachment #705805 - Attachment is obsolete: true
(In reply to Patrick Wang [:kk1fff] from comment #5)
> Adding comment according to comment 4.
         ^ sorry... adding debug message for failure to get voice radio tech.
https://hg.mozilla.org/integration/mozilla-inbound/rev/418c2f404d74
https://hg.mozilla.org/mozilla-central/rev/418c2f404d74
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
You need to log in before you can comment on or make changes to this bug.