Closed Bug 970812 Opened 6 years ago Closed 5 years ago

B2G RIL: Combine GECKO_RADIOSTATE_* and GECKO_DETAILED_RADIOSTATE_*

Categories

(Firefox OS Graveyard :: RIL, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(tracking-b2g:backlog)

RESOLVED DUPLICATE of bug 843452
tracking-b2g backlog

People

(Reporter: edgar, Assigned: edgar)

References

Details

Attachments

(1 file)

Now we have two different const set for radio state. [1]

this.GECKO_RADIOSTATE_UNAVAILABLE   = null;
this.GECKO_RADIOSTATE_OFF           = "off";
this.GECKO_RADIOSTATE_READY         = "ready";

and

this.GECKO_DETAILED_RADIOSTATE_UNKNOWN    = null;
this.GECKO_DETAILED_RADIOSTATE_ENABLING   = "enabling";
this.GECKO_DETAILED_RADIOSTATE_ENABLED    = "enabled";
this.GECKO_DETAILED_RADIOSTATE_DISABLING  = "disabling";
this.GECKO_DETAILED_RADIOSTATE_DISABLED   = "disabled";

I am thinking of combining GECKO_RADIOSTATE_* and GECKO_DETAILED_RADIOSTATE_*, given that
1). We only expose the value of "GECKO_DETAILED_RADIOSTATE_*" to the web. [2][3]
2). GECKO_RADIOSTATE_* and GECKO_DETAILED_RADIOSTATE_* are 1-1 mapping [4], so it seems we could replace GECKO_RADIOSTATE_* by using GECKO_DETAILED_RADIOSTATE_*.

Thank you.

[1] https://github.com/mozilla/gecko-dev/blob/d2ef16927622a687998c32093cc80d5357620a14/dom/system/gonk/ril_consts.js#L2425-L2433
[2] https://github.com/mozilla/gecko-dev/blob/d2ef16927622a687998c32093cc80d5357620a14/dom/mobileconnection/interfaces/nsIDOMMobileConnection.idl#L86-L92
[3] https://github.com/mozilla/gecko-dev/blob/d2ef16927622a687998c32093cc80d5357620a14/dom/system/gonk/RadioInterfaceLayer.js#L2670-L2677
[4] https://github.com/mozilla/gecko-dev/blob/d2ef16927622a687998c32093cc80d5357620a14/dom/system/gonk/RadioInterfaceLayer.js#L2648-L2657
Blocks: 843452
Sounds good.
Attached patch Patch, v1Splinter Review
Thanks for this. Then we might be able to set bug 953023 as a duplicate.
Duplicate of this bug: 953023
Comment on attachment 8373926 [details] [diff] [review]
Patch, v1

I had quick tested unagi/fugu by power on/off radio and also run marionette tests in my local environment.
Attachment #8373926 - Flags: review?(htsai)
Comment on attachment 8373926 [details] [diff] [review]
Patch, v1

After offline discuss with Hsinyi, the |rilContext.radioState| is reflect the underlying state and that is why we have a separated "detailedRadioState" for 'transient' state in current implementation [1]. But in this patch, we combine detailedRadioState and radioState together, |rilContext.radioState| may report 'transient' state, like "enabling" or "disabling", this breaks the definition of |rilContext.radioState|, though no one reference it now. 
In bug 843452, we are going to deprecate |rilContext.radioState|, so let do this change together with bug 843452.
Thank you, Hsinyi.

[1] Please see bug 951942 comment #4.
Attachment #8373926 - Flags: review?(htsai)
No longer blocks: 843452
See Also: → 843452
Leave this open for tracking, I will set this as a duplicated bug, once the patch of bug 843452 is ready.
Put this bug into backlog.
blocking-b2g: --- → backlog
(In reply to Edgar Chen [:edgar][:echen] from comment #6)
> Comment on attachment 8373926 [details] [diff] [review]
> Patch, v1
> 
> After offline discuss with Hsinyi, the |rilContext.radioState| is reflect
> the underlying state and that is why we have a separated
> "detailedRadioState" for 'transient' state in current implementation [1].
> But in this patch, we combine detailedRadioState and radioState together,
> |rilContext.radioState| may report 'transient' state, like "enabling" or
> "disabling", this breaks the definition of |rilContext.radioState|, though
> no one reference it now. 
> In bug 843452, we are going to deprecate |rilContext.radioState|, so let do
> this change together with bug 843452.
> Thank you, Hsinyi.
> 
> [1] Please see bug 951942 comment #4.

Patch part5-5* in bug 843452 includes the changes of this bug.
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 843452
blocking-b2g: backlog → ---
You need to log in before you can comment on or make changes to this bug.