970812 - B2G RIL: Combine GECKO_RADIOSTATE_* and GECKO_DETAILED_RADIOSTATE_*

Status: RESOLVED DUPLICATE of bug 843452

(Firefox OS Graveyard :: RIL, defect)

Description

[Edgar Chen [:edgar]]

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

Comment 1

[Szu-Yu Chen [:aknow] (inactive)]

Sounds good.

Comment 2

[Edgar Chen [:edgar]]

Attachment: Patch, v1

Comment 3

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

Thanks for this. Then we might be able to set bug 953023 as a duplicate.

Comment 5

[Edgar Chen [:edgar]]

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.

Comment 6

[Edgar Chen [:edgar]]

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.

Comment 7

[Edgar Chen [:edgar]]

Leave this open for tracking, I will set this as a duplicated bug, once the patch of bug 843452 is ready.

Comment 8

[Ken Chang[:kenkai]]

Put this bug into backlog.

Comment 9

[Edgar Chen [:edgar]]

(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.