Closed Bug 897869 Opened 7 years ago Closed 6 years ago

[b2g-ril] Need to keep CLIR settings across reboots

Categories

(Core :: DOM: Device Interfaces, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla25

People

(Reporter: jaoo, Assigned: jaoo)

References

Details

(Keywords: feature, Whiteboard: [TEF] u=commsapps-user c=dialer p=0 [fixed-in-birch])

Attachments

(1 file, 4 obsolete files)

Need to keep CLIR settings across reboots. The CLIR settings the user sets through the setting app or even MMI codes are not permanent and the phone reset them to network default on boot. Need to make them persist and set them once the phone boots.
blocking-b2g: --- → koi?
See Also: → 888904
Attached patch Gecko part - v1. (obsolete) — Splinter Review
Vicamo, we need to keep CLIR settings across reboots. Could you take a look a it please? Thanks!
Attachment #780914 - Flags: review?(vyang)
Attachment #780914 - Attachment description: 897869.patch → Gecko part - v1.
Attached patch Gaia part - v1. (obsolete) — Splinter Review
Arthur, we need to keep CLIR settings across reboots. Could you take a look a it please? Thanks!
Attachment #780918 - Flags: review?(arthur.chen)
Attached patch Gaia part - v1. (obsolete) — Splinter Review
This is the right one, sorry for the noise.
Attachment #780918 - Attachment is obsolete: true
Attachment #780918 - Flags: review?(arthur.chen)
Attachment #780944 - Flags: review?(arthur.chen)
Jose, I was thinking if it is necessary to keep the value in mozSettings. I checked my android phone, it gets the current value from the carrier every time I enter the page (so does call forwarding).
Flags: needinfo?(josea.olivera)
(In reply to Arthur Chen [:arthurcc] from comment #4)
> Jose, I was thinking if it is necessary to keep the value in mozSettings. I
> checked my android phone, it gets the current value from the carrier every
> time I enter the page (so does call forwarding).

Yes, it's necessary. The issue is at boot time. The CLIR settings reset to default on boot so we need to make sure the network knows the preferred CLIR setting. Android does that on boot as well.
Flags: needinfo?(josea.olivera)
Comment on attachment 780944 [details] [diff] [review]
Gaia part - v1.

Thanks for the clarification. r=me for the gaia part.
Attachment #780944 - Flags: review?(arthur.chen) → review+
Comment on attachment 780914 [details] [diff] [review]
Gecko part - v1.

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

Hi,

Just had a discussion with HsinYi, we think we don't really need an extra Settings entry for this function.  Instead, we have a new preference "ril.clirMode" that holds an integer mode and update its value every time a setCallingLineIdRestriction request succeeded.  We also fetch this pref inside RadioInterfaceLayer() and pass it into each RadioInterface instance and its ril_worker as "ril.cellbroadcast.disabled" does.  Then we just need an additional line in UNSOLICITED_RESPONSE_RADIO_STATE_CHANGED handler[1].  Note that we may have neither |options.rilMessageType| nor |options.requestId| set in this way, so a few fix to REQUEST_SET_CLIR handler[2] is required.

[1]: http://mxr.mozilla.org/mozilla-central/source/dom/system/gonk/ril_worker.js#5827
[2]: http://mxr.mozilla.org/mozilla-central/source/dom/system/gonk/ril_worker.js#5166
Attachment #780914 - Flags: review?(vyang)
Comment on attachment 780944 [details] [diff] [review]
Gaia part - v1.

Set to r? per comment 7.
Attachment #780944 - Flags: review+ → review?(arthur.chen)
(In reply to Vicamo Yang [:vicamo][:vyang] from comment #7)

> Just had a discussion with HsinYi, we think we don't really need an extra
> Settings entry for this function.  Instead, we have a new preference
> "ril.clirMode" that holds an integer mode and update its value every time a
> setCallingLineIdRestriction request succeeded.  We also fetch this pref
> inside RadioInterfaceLayer() and pass it into each RadioInterface instance
> and its ril_worker as "ril.cellbroadcast.disabled" does.  Then we just need
> an additional line in UNSOLICITED_RESPONSE_RADIO_STATE_CHANGED handler[1]. 
> Note that we may have neither |options.rilMessageType| nor
> |options.requestId| set in this way, so a few fix to REQUEST_SET_CLIR
> handler[2] is required.

Sounds good to me too, working on a new patch. Thanks for the feedback!

(In reply to Arthur Chen [:arthurcc] from comment #8)
> Set to r? per comment 7.

Good idea, once I finish the gecko part I'll see what is needed in Gaia.
Attached patch Gecko part - v2 (obsolete) — Splinter Review
Vicamo, review comments made in comment #7 addressed. Still need to test it but could you take a look to see whether that's the idea? Thanks!
Attachment #780914 - Attachment is obsolete: true
Attachment #780944 - Attachment is obsolete: true
Attachment #780944 - Flags: review?(arthur.chen)
Attachment #783030 - Flags: feedback?(vyang)
Comment on attachment 783030 [details] [diff] [review]
Gecko part - v2

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

::: dom/system/gonk/RadioInterfaceLayer.js
@@ +563,5 @@
>  
>    let options = {
>      debug: debugPref,
> +    cellBroadcastDisabled: false,
> +    clirMode: CLIR_DEFAULT

RIL.CLIR_DEFAULT

::: dom/system/gonk/ril_worker.js
@@ +1509,5 @@
> +    if (options) {
> +      this.clirMode = options.clirMode;
> +    }
> +    if (!this.clirMode) {
> +      return;

Why? User may just want to remove manual setting.

@@ +5179,5 @@
>  };
>  RIL[REQUEST_SET_CLIR] = function REQUEST_SET_CLIR(length, options) {
> +  if (!options.clirMode) {
> +    // The request was made by ril_worker itself automatically. Don't report.
> +    return;

Just check |options.rilMessageType == null|.
Attachment #783030 - Flags: feedback?(vyang)
Attached patch Gecko part - v3Splinter Review
Feedback comments from comment #11 addressed, other minor fixes and patch tested on device.

There is no need of gaia side development for this bug. The approach pointed out by Vicamo and HsinYi in comment #7 works fine. Thanks guys!
Attachment #783030 - Attachment is obsolete: true
Attachment #783183 - Flags: review?(vyang)
Comment on attachment 783183 [details] [diff] [review]
Gecko part - v3

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

Thank you :)
Attachment #783183 - Flags: review?(vyang) → review+
https://hg.mozilla.org/projects/birch/rev/6adb8b4b733b
Whiteboard: [TEF] u=commsapps-user c=dialer p=0 → [TEF] u=commsapps-user c=dialer p=0 [fixed-in-birch]
Target Milestone: --- → mozilla25
https://hg.mozilla.org/mozilla-central/rev/6adb8b4b733b
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Antonio, I know this issue has been closed but wondering why did we decide to not have Gaia request for CLIR just like it does for Call waiting and call forwarding instead of setting the preference in RIL. That would have been much cleaner.
blocking-b2g: koi? → ---
Removing koi? since this is in gecko 25 which is < 26 which will be the basis for Firefox OS 1.2.
You need to log in before you can comment on or make changes to this bug.